Skip to content

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

Merged
gfyrag merged 4 commits into
mainfrom
fix/publisher-http-jwt-conflict
Jun 19, 2026
Merged

fix: serve crash with --publisher-http-enabled (TS-456)#1419
gfyrag merged 4 commits into
mainfrom
fix/publisher-http-jwt-conflict

Conversation

@gfyrag

@gfyrag gfyrag commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Same fix as #1418 (release/v2.4) and #1417 (release/v2.3), now on main.

  • ledger serve 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`.
  • 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; uber/fx rejects the duplicate.
  • Fix is in go-libs (go-libs#651). This PR pulls it in via a replace directive on a v5.3.2-based cherry-pick of the fix commit (minimal diff vs v5.3.2).
  • 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.x) 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 flags it.

Customer impact

Reported by Mews on TS-456.

Test plan

  • test/e2e/app_publisher_http_jwt_test.go (-tags=it) fails on main 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 cherry-pick 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.

Refs: TS-456
@gfyrag gfyrag requested a review from a team as a code owner June 18, 2026 14:04
@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: 28e561f8-5a7e-4918-ad80-c8aec55ebf9f

📥 Commits

Reviewing files that changed from the base of the PR and between 78e7138 and 7f61c6c.

⛔ 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 integration test file is added under test/e2e with a //go:build it tag. It registers a regression test (TS-456) that starts a ledger serve instance with --publisher-http-enabled and a wildcard topic mapping, then asserts the server boots without error.

Changes

HTTP Publisher Boot Regression Test

Layer / File(s) Summary
HTTP publisher e2e regression test
test/e2e/app_publisher_http_jwt_test.go
Introduces a build-tagged integration test that provisions a templated Postgres database, boots a deferred test server with --publisher-http-enabled and --publisher-topic-mapping flags, and asserts testServer.Wait returns nil to confirm no duplicate *http.Client fx provider error occurs.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Poem

🐇 A bug once lurked in the fx provider tree,
Duplicate clients, oh the horror to see!
A test was crafted, swift and precise,
To catch the mischief and make the boot nice.
Now servers start clean, as they ought to be! 🌿

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main fix: resolving a startup crash when the --publisher-http-enabled flag is used, referencing the specific issue TS-456.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, explaining the root cause, fix implementation, testing, and follow-up actions.
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 fix/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

The dependency replacement and regression test are consistent with the described startup conflict fix, and I did not find any introduced issues that would break existing behavior.

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 80.66%. Comparing base (6a84e4c) to head (7f61c6c).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1419      +/-   ##
==========================================
+ Coverage   80.55%   80.66%   +0.11%     
==========================================
  Files         206      206              
  Lines       11293    11257      -36     
==========================================
- Hits         9097     9081      -16     
+ Misses       1645     1622      -23     
- Partials      551      554       +3     

☔ 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

The dependency bump includes the upstream fx scoping fix and the added e2e test exercises startup with the HTTP publisher enabled. I did not find a discrete regression introduced by these changes.

No findings.

ascandone
ascandone previously approved these changes Jun 19, 2026
paul-nicolas
paul-nicolas previously approved these changes Jun 19, 2026
@gfyrag gfyrag dismissed stale reviews from paul-nicolas and ascandone via b68e7d7 June 19, 2026 10:36

@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 change bumps go-libs to a version containing the fx scoping fix and adds a targeted integration regression test. I did not identify any discrete issue introduced by the diff that would break existing behavior.

No findings.

paul-nicolas
paul-nicolas previously approved these changes Jun 19, 2026

@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 I did not identify a discrete regression introduced by these changes.

No findings.

@gfyrag gfyrag enabled auto-merge June 19, 2026 11:28
@gfyrag gfyrag added this pull request to the merge queue Jun 19, 2026
Merged via the queue into main with commit 97b1d20 Jun 19, 2026
13 of 14 checks passed
@gfyrag gfyrag deleted the fix/publisher-http-jwt-conflict branch June 19, 2026 11:40
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.

4 participants