Skip to content

fix: serve crash with --publisher-http-enabled (TS-456)#1418

Merged
gfyrag merged 4 commits into
release/v2.4from
hotfix/v2.4/publisher-http-jwt-conflict
Jun 19, 2026
Merged

fix: serve crash with --publisher-http-enabled (TS-456)#1418
gfyrag merged 4 commits into
release/v2.4from
hotfix/v2.4/publisher-http-jwt-conflict

Conversation

@gfyrag

@gfyrag gfyrag commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • ledger serve (>= v2.3.3, including all v2.4.x) crashes at startup whenever --publisher-http-enabled (or PUBLISHER_HTTP_ENABLED=true) is set, with the error `cannot provide *http.Client from [0]: already provided by "reflect".makeFuncStub`.
  • In v2.4, the bug fires whether or not auth is enabled because cmd.NewServeCommand loads authnfx.JWTModuleFromFlags unconditionally (which always supplies a *http.Client). messagingfx.HTTPModule supplies another one as soon as the HTTP publisher is enabled, and uber/fx rejects the duplicate.
  • Fix is in go-libs (go-libs#651 on main). This PR pulls it in via a replace directive on a v5.3.2-based cherry-pick of the fix commit, so the only diff vs v5.3.2 is the scoping change itself.
  • Adds an e2e regression test (test/e2e/app_publisher_http_jwt_test.go) that boots ledger with --publisher-http-enabled and asserts startup succeeds.

Follow-up (before / after merge)

  1. Merge go-libs#651.
  2. Tag v5.3.3 (or v5.4.1) on go-libs.
  3. Replace the temporary replace directive in go.mod with a regular bump to the released version.

The // TS-456 comment in go.mod is there to make this hand-off obvious.

Customer impact

Reported by Mews on TS-456. The exact repro from the ticket:

```sh
docker run --rm -e DEBUG=true \
-e POSTGRES_URI='postgresql://x:x@127.0.0.1/x?sslmode=disable' \
-e PUBLISHER_HTTP_ENABLED=true \
-e PUBLISHER_TOPIC_MAPPING='*:http://example:8080' \
ghcr.io/formancehq/ledger:v2.4.10 serve
```

Test plan

  • test/e2e/app_publisher_http_jwt_test.go (-tags=it) fails on release/v2.4 head with the exact provider-conflict error.
  • Same test passes with the replace directive applied.
  • go build ./... clean.
  • CI green on this branch.
  • Swap replace for require v5.3.3 once go-libs releases.

Related PRs

go-libs v5.3.2's authnfx.JWTModule (loaded unconditionally by serve) and
messagingfx.HTTPModule both supply an unnamed *http.Client at the parent
fx scope. uber/fx rejects the duplicate provider, so ledger serve
crashes at startup with:

    cannot provide *http.Client from [0]: already provided by "reflect".makeFuncStub

Pull in the go-libs fix (formancehq/go-libs#651) via a `replace`
directive on a v5.3.2-based commit of the fix. Drop the replace and
bump to v5.3.3 (or v5.4.x) once tagged.

Adds an e2e regression test that boots ledger serve with
--publisher-http-enabled and asserts startup succeeds. Fails on
release/v2.4 head with the exact error above; passes with this fix.

Refs: TS-456
@gfyrag gfyrag requested a review from a team as a code owner June 18, 2026 14:01
@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4416c0e2-2f47-4ca4-b115-bf8750b8c800

📥 Commits

Reviewing files that changed from the base of the PR and between 3a300e7 and 73c293b.

⛔ Files ignored due to path filters (6)
  • go.mod is excluded by !**/*.mod
  • go.sum is excluded by !**/*.sum, !**/*.sum
  • tools/generator/go.mod is excluded by !**/*.mod
  • tools/generator/go.sum is excluded by !**/*.sum, !**/*.sum
  • tools/provisioner/go.mod is excluded by !**/*.mod
  • tools/provisioner/go.sum is excluded by !**/*.sum, !**/*.sum
📒 Files selected for processing (1)
  • test/e2e/app_publisher_http_jwt_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/e2e/app_publisher_http_jwt_test.go

Walkthrough

A new end-to-end Ginkgo regression test is added in test/e2e/app_publisher_http_jwt_test.go. It starts a ledger serve instance with --publisher-http-enabled, a templated Postgres database (MaxOpenConns: 100), and a permissive topic mapping, then asserts the server starts without error (guarding against TS-456).

Changes

Publisher HTTP startup regression test

Layer / File(s) Summary
Regression test: publisher HTTP server startup
test/e2e/app_publisher_http_jwt_test.go
New Ginkgo Context/It block that provisions a templated Postgres DB, starts a deferred test server with --publisher-http-enabled and a permissive topic mapping, and asserts testServer.Wait(...) returns no error.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐇 A rabbit once feared the fx graph would break,
With duplicate clients — oh, what a mistake!
So I wrote a test, neat and precise,
To boot up the server not once, but twice!
No error returned — all systems are green. 🌿

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main fix: resolving a crash in the serve command when --publisher-http-enabled is enabled, directly tied to the TS-456 issue.
Description check ✅ Passed The description thoroughly explains the bug, root cause, the fix, regression test, and follow-up steps, all directly related to the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch hotfix/v2.4/publisher-http-jwt-conflict

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@NumaryBot NumaryBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approve — automated review

No discrete correctness issues were identified in the dependency replacement or the added regression test based on the diff.

No findings.

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.19%. Comparing base (a164691) to head (73c293b).
⚠️ Report is 2 commits behind head on release/v2.4.

Additional details and impacted files
@@               Coverage Diff                @@
##           release/v2.4    #1418      +/-   ##
================================================
- Coverage         81.21%   81.19%   -0.03%     
================================================
  Files               206      206              
  Lines             11250    11259       +9     
================================================
+ Hits               9137     9142       +5     
- Misses             1561     1566       +5     
+ Partials            552      551       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

go-libs#651 is merged and v5.5.0 is tagged. Drop the temporary
`replace` directive and bump to the released version.
@gfyrag

gfyrag commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

go-libs#651 merged + tagged as v5.5.0. replace directive removed, now requires go-libs/v5 v5.5.0 directly. Ready for human review/merge.

@NumaryBot NumaryBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approve — automated review

No concrete correctness issues were identified in the dependency bump or the added regression test based on the checked-out diff.

No findings.

@NumaryBot NumaryBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approve — automated review

The dependency bump and regression test align with the reported fx provider conflict, and no discrete introduced correctness issue is evident from the diff.

No findings.

@NumaryBot NumaryBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approve — automated review

The dependency bump brings in the fx scoping fix and the added e2e regression test exercises the reported startup path. I did not find a discrete regression introduced by these changes.

No findings.

@gfyrag gfyrag merged commit ee1aa8c into release/v2.4 Jun 19, 2026
6 checks passed
@gfyrag gfyrag deleted the hotfix/v2.4/publisher-http-jwt-conflict branch June 19, 2026 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants