refactor(fga): make committee member FGA publishing asynchronous-only - #164
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change replaces generic FGA access publishing with dedicated asynchronous ChangesAsynchronous FGA membership
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AcceptInvite
participant committee_member_writer
participant messagePublisher
participant CoreNATS
AcceptInvite->>committee_member_writer: CreateMember with sync=false
committee_member_writer->>messagePublisher: MemberPut
messagePublisher->>CoreNATS: Publish member_put asynchronously
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
prabodhcs
added
the
do-not-merge
There was a problem hiding this comment.
Pull request overview
Refactors committee-member FGA publishing to use asynchronous core NATS exclusively.
Changes:
- Replaces generic
Accesspublishing with dedicatedMemberPutandMemberRemovemethods. - Reverts
AcceptInviteto asynchronous member creation publishing. - Updates tests, FGA documentation, Goa design, and generated API artifacts.
Reviewed changes
Copilot reviewed 17 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
internal/service/message_handler_test.go |
Updates publisher spy methods. |
internal/service/document_writer_test.go |
Updates error publisher stub. |
internal/service/committee_writer_test.go |
Updates mocks and routing assertions. |
internal/service/committee_member_writer.go |
Routes membership FGA messages through dedicated methods. |
internal/service/committee_member_writer_test.go |
Tests publishing failures, ordering, and sync behavior. |
internal/infrastructure/nats/messaging_publish.go |
Adds shared asynchronous FGA publisher. |
internal/infrastructure/nats/messaging_publish_test.go |
Verifies core publishing and error paths. |
internal/infrastructure/mock/committee.go |
Adds membership publisher captures. |
internal/domain/port/committee_publisher.go |
Replaces the generic access interface. |
gen/http/openapi3.yaml |
Regenerates OpenAPI 3 documentation. |
gen/http/openapi.yaml |
Regenerates OpenAPI documentation. |
gen/committee_service/service.go |
Regenerates Goa payload documentation. |
docs/fga-contract.md |
Documents asynchronous membership publishing. |
cmd/committee-cli/commands/sync/reindex_invites_test.go |
Updates CLI publisher mock. |
cmd/committee-api/service/group_weekly_brief_test.go |
Updates publisher stub. |
cmd/committee-api/service/committee_service.go |
Makes AcceptInvite use the asynchronous path. |
cmd/committee-api/service/committee_service_test.go |
Updates the service mock and acceptance tests. |
cmd/committee-api/design/type.go |
Updates the X-Sync API description. |
Files not reviewed (1)
- gen/committee_service/service.go: Generated file
Remove the generic, sync-capable Access(subject, msg, sync) publisher method and replace it with dedicated MemberPut/MemberRemove methods that always core-publish, mirroring UpdateAccess/DeleteAccess. JetStream (LFXV2-2831) cannot preserve request/reply semantics for these subjects, so removing them now avoids a silent degradation to storage-ack instead of fga-sync completion-ack later. BREAKING: AcceptInvite no longer waits for FGA processing before returning, reverting PR #163's sync=true workaround (which itself addressed LFXV2-2645). The eventual-consistency window this reopens is mitigated by client-side polling in Self Serve (LFXV2-2890), which must deploy before this change. BREAKING: X-Sync no longer has any effect on FGA membership publishing for any caller (none currently send X-Sync: true on a member endpoint). Jira: LFXV2-2856 Link: https://linuxfoundation.atlassian.net/browse/LFXV2-2856 Signed-off-by: Prabodh Chaudhari <pchaudhari@linuxfoundation.org> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 20 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- gen/committee_service/service.go: Generated file
Suppressed comments (1)
cmd/committee-api/service/committee_service_test.go:120
- Keep recording the
syncargument and assert that successfulAcceptInvitecalls passfalse. If the call regresses totrue, membership FGA remains asynchronous, butCreateMembercan still block on its synchronous indexer request for up to 10 seconds. Removing the previous argument assertion leaves this endpoint-level behavior untested; the new orchestrator test only verifies how a supplied flag is routed.
createMemberCalls []*model.CommitteeMember
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/committee-api/service/committee_service_test.go (1)
112-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore test coverage for the
syncargument passed toCreateMemberinAcceptInvite.
createMemberSyncArgstracking was removed instead of updated.TestAcceptInviteno longer verifies thatCreateMemberreceivessync=false. This PR's stated purpose is to guaranteeAcceptInvitealways passessync=falsetoCreateMember. Without this assertion, a future regression that reintroducessync=trueinAcceptInvitewould not be caught by this test.Track the
syncargument again and assert it inTestAcceptInvite.♻️ Proposed fix to restore sync-argument coverage
type mockCommitteeWriterOrchestrator struct { deleteError error deleteCalls []deleteCall updateMember *model.CommitteeMember updateMemberErr error updateMemberCalls []updateMemberCall createMember *model.CommitteeMember createMemberErr error createMemberCalls []*model.CommitteeMember + createMemberSyncArgs []bool } func (m *mockCommitteeWriterOrchestrator) CreateMember(ctx context.Context, member *model.CommitteeMember, sync bool, skipEnrichment bool) (*model.CommitteeMember, error) { m.createMemberCalls = append(m.createMemberCalls, member) + m.createMemberSyncArgs = append(m.createMemberSyncArgs, sync) if m.createMemberErr != nil { return nil, m.createMemberErr } if m.createMember != nil { return m.createMember, nil } return nil, errs.NewUnexpected("not implemented for test") }Then in
TestAcceptInvite, after a successful accept:require.NotNil(t, result) assert.Equal(t, "Active", result.Status) + require.NotEmpty(t, mockOrch.createMemberSyncArgs) + assert.False(t, mockOrch.createMemberSyncArgs[len(mockOrch.createMemberSyncArgs)-1], "AcceptInvite must call CreateMember with sync=false")Also applies to: 149-158, 1416-1430
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/committee-api/service/committee_service_test.go` around lines 112 - 121, The mockCommitteeWriterOrchestrator no longer records the sync argument passed to CreateMember. Restore sync tracking in the mock’s CreateMember call data, then update TestAcceptInvite’s successful-accept case to assert that CreateMember received sync=false, preserving coverage against regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/infrastructure/mock/committee.go`:
- Around line 1312-1325: Replace the new slog.InfoContext calls in
MockCommitteePublisher.MemberPut and MemberRemove with the committee service
pkg/log helper, preserving the existing context and message fields. Ensure these
internal mock logging calls follow the pkg/log convention.
---
Nitpick comments:
In `@cmd/committee-api/service/committee_service_test.go`:
- Around line 112-121: The mockCommitteeWriterOrchestrator no longer records the
sync argument passed to CreateMember. Restore sync tracking in the mock’s
CreateMember call data, then update TestAcceptInvite’s successful-accept case to
assert that CreateMember received sync=false, preserving coverage against
regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43187683-0b0a-4126-967d-099a6c522a37
⛔ Files ignored due to path filters (5)
gen/committee_service/service.gois excluded by!**/gen/**gen/http/openapi.jsonis excluded by!**/gen/**gen/http/openapi.yamlis excluded by!**/gen/**gen/http/openapi3.jsonis excluded by!**/gen/**gen/http/openapi3.yamlis excluded by!**/gen/**
📒 Files selected for processing (15)
cmd/committee-api/design/type.gocmd/committee-api/service/committee_service.gocmd/committee-api/service/committee_service_test.gocmd/committee-api/service/group_weekly_brief_test.gocmd/committee-cli/commands/sync/reindex_invites_test.godocs/fga-contract.mdinternal/domain/port/committee_publisher.gointernal/infrastructure/mock/committee.gointernal/infrastructure/nats/messaging_publish.gointernal/infrastructure/nats/messaging_publish_test.gointernal/service/committee_member_writer.gointernal/service/committee_member_writer_test.gointernal/service/committee_writer_test.gointernal/service/document_writer_test.gointernal/service/message_handler_test.go
TestAcceptInvite no longer verified that CreateMember always receives sync=false, since createMemberSyncArgs tracking was dropped when the generic sync-argument assertion was removed. Restore it: a regression to sync=true would reintroduce the request/reply indexer timeout this change removed, and nothing else in the suite catches that at the endpoint level. Addresses PR #164 review feedback (Copilot, CodeRabbit). Jira: LFXV2-2856 Link: https://linuxfoundation.atlassian.net/browse/LFXV2-2856 Signed-off-by: Prabodh Chaudhari <pchaudhari@linuxfoundation.org> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/committee-api/service/committee_service_test.go (1)
118-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse the shared infrastructure fake for this test state.
createMemberSyncArgsextendsmockCommitteeWriterOrchestrator, a fake declared in this test file. Move this behavior to a compatible fake ininternal/infrastructure/mock, or add the orchestrator fake there before using it here.As per coding guidelines,
*_test.gofiles must use table-driven tests withinternal/infrastructure/mockfakes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/committee-api/service/committee_service_test.go` around lines 118 - 121, Move the mockCommitteeWriterOrchestrator fake from the committee_service_test.go file to internal/infrastructure/mock so it can be reused as a shared test infrastructure. Add the createMemberSyncArgs field to the moved orchestrator fake definition. Update the test to import and use the orchestrator fake from the shared mock package instead of declaring it locally, ensuring the test follows the table-driven test pattern with infrastructure fakes as required by the coding guidelines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/committee-api/service/committee_service_test.go`:
- Around line 1434-1442: Update the AcceptInvite test table and assertions
around mockOrch.createMemberSyncArgs to include the expected CreateMember call
count per case. Assert exactly one call with sync=false for pending and declined
invites, and zero calls for accepted and revoked invites; remove the conditional
last-value-only check.
---
Nitpick comments:
In `@cmd/committee-api/service/committee_service_test.go`:
- Around line 118-121: Move the mockCommitteeWriterOrchestrator fake from the
committee_service_test.go file to internal/infrastructure/mock so it can be
reused as a shared test infrastructure. Add the createMemberSyncArgs field to
the moved orchestrator fake definition. Update the test to import and use the
orchestrator fake from the shared mock package instead of declaring it locally,
ensuring the test follows the table-driven test pattern with infrastructure
fakes as required by the coding guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7e48c0f-c6c7-4e96-b548-18330cc0be5b
📒 Files selected for processing (1)
cmd/committee-api/service/committee_service_test.go
Postflight summary
All threads resolved. CI green (Build and Test, Analyze, CodeQL, DCO, License checks, MegaLinter). No stale human approvals to dismiss. |
The prior sync=false assertion only inspected the last recorded arg, so it would silently pass even if CreateMember were called an unexpected number of times (or not at all) for a given case. Add an explicit expectCreateMemberCalled flag per test case and assert exactly one sync=false call for pending/declined accepts, and zero calls for the idempotent accepted/revoked paths. Addresses PR #164 review feedback (CodeRabbit). Jira: LFXV2-2856 Link: https://linuxfoundation.atlassian.net/browse/LFXV2-2856 Signed-off-by: Prabodh Chaudhari <pchaudhari@linuxfoundation.org> Co-authored-by: Cursor <cursoragent@cursor.com>
Postflight summary (round 2)
All 3 threads on this PR are now resolved. CI green (Build and Test, Analyze, CodeQL, DCO, License checks, MegaLinter, CodeRabbit re-review). |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/service/committee_member_writer_test.go (3)
2285-2351: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAssert the publication order.
The test checks
MemberRemoveandMemberPutin separate slices. It does not prove thatmember_removeoccurs beforemember_put. A regression that grantsnew_usernamefirst would still pass. Record one ordered publication sequence and assert the required order. (raw.githubusercontent.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/committee_member_writer_test.go` around lines 2285 - 2351, The test TestPublishMemberMessages_ChangedUsername_RevokesOldBeforeGrantingNew currently validates message contents but not publication order. Add an ordered publication sequence to spyCommitteePublisher, record both member_remove and member_put events as they are published, and assert in the successful revoke subtest that the remove event precedes the put event while preserving the existing identity assertions.Source: MCP tools
2365-2393: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCover
member_removein the sync-independence test.The test invokes only
ActionCreatedand assertsMemberPutCallCount. It never exercisesActionDeletedorMemberRemove, although the test claims both membership publications ignoresync. Add a table-driven created/deleted matrix and assert the corresponding FGA call for each action. As per coding guidelines, files matching**/*_test.gomust use table-driven tests withinternal/infrastructure/mockfakes. (raw.githubusercontent.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/committee_member_writer_test.go` around lines 2365 - 2393, The TestPublishMemberMessages_SyncControlsOnlyIndexer test currently covers only ActionCreated and MemberPutCallCount. Convert it to a table-driven created/deleted matrix using the existing internal/infrastructure/mock fakes, invoke publishMemberMessages for each action with sync=true, and assert indexer synchronization plus the corresponding MemberPutCallCount for ActionCreated and MemberRemoveCallCount for ActionDeleted.Sources: Coding guidelines, MCP tools
1011-1030: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winVerify the uniqueness reservation is removed.
UniqueMemberstoresuniqueKeyinmemberWriter.keys, but the fakeDeleteMemberonly deletesmemberWriter.members[uid].wasDeleted(uniqueKey)can pass while the uniqueness reservation remains. Add key-map cleanup to the fake or callUniqueMemberagain after rollback and require no conflict. (raw.githubusercontent.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/committee_member_writer_test.go` around lines 1011 - 1030, Update the fake writer cleanup used by the rollback test around memberWriter.wasDeleted so DeleteMember also removes the corresponding entry from memberWriter.keys, or verify after rollback by calling UniqueMember again and requiring no conflict. Ensure the assertion confirms the uniqueness reservation itself was removed, not only memberWriter.members[uniqueKey].Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/service/committee_member_writer_test.go`:
- Around line 2285-2351: The test
TestPublishMemberMessages_ChangedUsername_RevokesOldBeforeGrantingNew currently
validates message contents but not publication order. Add an ordered publication
sequence to spyCommitteePublisher, record both member_remove and member_put
events as they are published, and assert in the successful revoke subtest that
the remove event precedes the put event while preserving the existing identity
assertions.
- Around line 2365-2393: The TestPublishMemberMessages_SyncControlsOnlyIndexer
test currently covers only ActionCreated and MemberPutCallCount. Convert it to a
table-driven created/deleted matrix using the existing
internal/infrastructure/mock fakes, invoke publishMemberMessages for each action
with sync=true, and assert indexer synchronization plus the corresponding
MemberPutCallCount for ActionCreated and MemberRemoveCallCount for
ActionDeleted.
- Around line 1011-1030: Update the fake writer cleanup used by the rollback
test around memberWriter.wasDeleted so DeleteMember also removes the
corresponding entry from memberWriter.keys, or verify after rollback by calling
UniqueMember again and requiring no conflict. Ensure the assertion confirms the
uniqueness reservation itself was removed, not only
memberWriter.members[uniqueKey].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 921d4a04-da2c-44e8-b841-37be513bbbe7
📒 Files selected for processing (2)
internal/service/committee_member_writer.gointernal/service/committee_member_writer_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/committee_member_writer.go
prabodhcs
removed
the
do-not-merge
andrest50
left a comment
andrest50
left a comment
There was a problem hiding this comment.
Review: refactor(fga): make committee member FGA publishing asynchronous-only
Approving. The refactor is clean and the breaking change is correctly characterized. Some minor test-coverage gaps worth noting.
What this does and why it's correct
The Access(subject, msg, sync bool) method was a footgun: a caller could accidentally pass sync=true on member_put/member_remove subjects even after they're captured by a JetStream stream, at which point the reply comes from the broker acknowledging storage, not from fga-sync completing the operation — a silent false-positive that looks like convergence. Replacing it with dedicated MemberPut/MemberRemove methods that are unconditionally async removes the option entirely. No current caller sends X-Sync: true on a member endpoint so this is a safe removal.
The AcceptInvite revert (sync=true → sync=false) is correctly characterized as a deliberate reversal of PR #163's workaround. The live evidence in the PR description confirms the guarantee — member_put goes out with an empty Reply header even when sync=true is passed to CreateMember (which now affects only the indexer).
Acceptance criteria
| Requirement | Status |
|---|---|
member_put / member_remove always use core-NATS publish, never request/reply |
✅ publishAccessAsync is called unconditionally from MemberPut/MemberRemove; the old Access dispatch is gone |
| No callsite can select request/reply for FGA membership | ✅ MemberPut/MemberRemove take no sync parameter; the interface enforces it |
AcceptInvite does not block on FGA processing |
✅ CreateMember(ctx, member, false, false) — confirmed by TestAcceptInvite asserting exactly one sync=false call |
sync continues to gate only the indexer |
✅ TestPublishMemberMessages_SyncControlsOnlyIndexer verifies IndexerSyncValues[0] is true while MemberPutCallCount is 1 regardless |
| Changed-username revoke fires before the new grant | ✅ Structurally enforced — MemberRemove error returns before MemberPut is reached; "revoke failure prevents the put" subtest verifies this |
| Existing message subjects and payloads unchanged | ✅ Subjects come from fgaconstants.GenericMemberPutSubject/GenericMemberRemoveSubject (unchanged); only the dispatch path changed |
| Tests cover membership add, removal, and publish failures | ✅ TestCommitteeWriterOrchestrator_DeleteMember_MessagePublishingFailure, TestPublishMemberMessages_ChangedUsername_RevokesOldBeforeGrantingNew, TestPublishMemberMessages_SyncControlsOnlyIndexer |
Minor test gaps (CodeRabbit findings, out-of-diff)
1. Changed-username ordering not asserted in the success path
TestPublishMemberMessages_ChangedUsername_RevokesOldBeforeGrantingNew stores capturedMemberRemoveMsgs and capturedMemberPutMsgs in separate slices and checks their contents, but not their order. If someone swapped the MemberRemove/MemberPut call order in the production code, both slices would still contain one message each with the right content. The "revoke failure prevents the put" subtest protects the error path but not the success ordering. Adding a combined ordered call log to spyCommitteePublisher (e.g. callOrder []string) and asserting ["member_remove", "member_put"] would close this.
In practice the structural guarantee is there (the production code gates MemberPut on MemberRemove returning nil), so this is a test-quality gap, not a correctness gap.
2. Sync-independence test covers only ActionCreated
TestPublishMemberMessages_SyncControlsOnlyIndexer tests ActionCreated and asserts MemberPutCallCount. It never exercises ActionDeleted or asserts MemberRemoveCallCount. The test's own name says "sync controls ONLY the indexer" — a table-driven created/deleted matrix would verify both membership paths ignore sync.
Deployment gate
This PR's commit message and PR description both state: "[PR #1323's self-serve polling fix] must ship before this change." That ordering isn't CI-enforced — it's a rollout-process constraint. Since PR #1323 (LFXV2-2890) is open and has had its own rounds of review, please ensure it is deployed first. Landing this without the frontend mitigation reopens the post-accept 403 window with no user-visible fallback.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 20 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- gen/committee_service/service.go: Generated file
Suppressed comments (1)
docs/fga-contract.md:41
- This update leaves the contract internally inconsistent. The changed member path calls
MemberRemoveonActionUpdatedwhen a username is cleared or changed (committee_member_writer.go:1251-1269), but themember_removesection at lines 111-113 and the trigger table at line 189 still describe it as delete-only. Update those sections to document both update-time revocation cases so consumers have the complete emission contract.
`member_put` and `member_remove` publication follows the same asymmetry as committee writes: create and update publish errors are logged and best-effort, while `DeleteMember` still returns an immediate publish error after the member record is deleted. `AcceptInvite` no longer uses request/reply for `member_put`; access checks issued immediately after acceptance may not yet see the membership.
Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # gen/http/openapi.json # gen/http/openapi3.json # gen/http/openapi3.yaml
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 20 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- gen/committee_service/service.go: Generated file
Suppressed comments (1)
docs/fga-contract.md:41
- The contract still describes
member_removeas delete-only in the dedicated section and trigger table (docs/fga-contract.md:111-113,189), but the changed path publishes it when an update clears or changes a username (committee_member_writer.go:1253-1269). Since this PR is updating the FGA delivery contract, please also document those update triggers so downstream consumers have an internally consistent contract.
`member_put` and `member_remove` publication follows the same asymmetry as committee writes: create and update publish errors are logged and best-effort, while `DeleteMember` still returns an immediate publish error after the member record is deleted. `AcceptInvite` no longer uses request/reply for `member_put`; access checks issued immediately after acceptance may not yet see the membership.
Summary
sync-capableCommitteePublisher.Access(subject, msg, sync)method and replaces it with dedicatedMemberPut/MemberRemovemethods that always core-publish (fire-and-forget), mirroring the existingUpdateAccess/DeleteAccessmethods.AcceptInvite'sCreateMembercall tosync=false. JetStream (LFXV2-2831) cannot preserve request/reply semantics for FGA subjects, so keeping a synchronous FGA path would either time out or silently degrade to a storage ack instead of an fga-sync completion ack. Removing it now, before the JetStream migration, avoids that failure mode entirely.Why
PR #163 made
AcceptInvite'smember_putsynchronous (request/reply) to close a post-accept 403 window. That directly conflicts with this ticket's requirement that FGA membership publishing be asynchronous-only, and it would break outright once NATS core publishes migrate to JetStream, since JetStream stream-backed subjects ack on durable storage rather than waiting for thefga-syncconsumer to actually process the message — so the synchronous wait would either time out (ifNoAckis required) or silently stop guaranteeing what it appears to guarantee.Breaking changes
AcceptInviteno longer waits for FGA processing before returning. This reverts PR [LFXV2-2645] fix(invites): sync FGA member_put on AcceptInvite to prevent post-accept 403 #163'ssync=trueworkaround. The eventual-consistency window it reopens is mitigated by client-side polling in Self Serve, which should ship before/alongside this change.X-Sync: trueno longer has any effect on FGA membership publishing for any caller. It continues to gate only the indexer publish (unchanged). No current caller sendsX-Sync: trueon a member endpoint.Changes
internal/domain/port/committee_publisher.go: interface now exposesMemberPut/MemberRemoveinstead of the genericAccessmethod.internal/infrastructure/nats/messaging_publish.go: implementsMemberPut/MemberRemove; extracts a sharedpublishAccessAsynchelper used byUpdateAccess/DeleteAccess/MemberPut/MemberRemove.internal/service/committee_member_writer.go: member write orchestration calls the new dedicated methods instead ofAccess(subject, msg, sync).cmd/committee-api/service/committee_service.go:AcceptInvitereverted toCreateMember(ctx, member, false, false), with a comment explaining the deliberate revert and its dependency on the Self Serve polling fix.cmd/committee-api/design/type.go,docs/fga-contract.md: documentation updated to statemember_put/member_remove(along withupdate_access/delete_access) are always asynchronous, regardless ofX-Sync.internal/infrastructure/mock,internal/infrastructure/nats,internal/service, andcmd/committee-api/service/cmd/committee-clito reflect the new interface shape.gen/...) regenerated for the design doc change.Live evidence (real NATS traffic, IDs/usernames redacted below)
Verified against a real local
nats-server(JetStream enabled), using the production publisher and orchestrator with only the repository layer mocked. Full write-up lives in the change's evidence doc (not part of this diff; local-only OpenSpec artifact).Scenario 1 —
CreateMember(sync=false), i.e.AcceptInvite's reverted path:Returns in microseconds — no blocking on FGA at all.
Scenario 2 —
DeleteMember, happy path:Scenario 3 —
CreateMember(sync=true)— provessyncreaches only the indexer, never FGA:The call blocked for the full 10s NATS request timeout only on the indexer subject (no subscriber replied). The
member_putfor this same call still went out as a core publish with no reply subject.Captured wire evidence across all three scenarios:
Replyempty (core publish)?lfx.fga-sync.member_putlfx.index.committee_membersync=false)lfx.index.committee_membersync=false)lfx.fga-sync.member_removelfx.fga-sync.member_putsync=truelfx.index.committee_membersync=true⇒ request/reply)Every
lfx.fga-sync.member_put/member_removemessage across all three scenarios had an emptyReply, including Scenario 3 where the indexer message for the sameCreateMembercall had a non-emptyReplyand blocked for 10s. This is the change's core guarantee, observed on real NATS wire traffic: FGA membership publication is unconditionally asynchronous, andsyncaffects only the indexer.Raw captured payloads (IDs/usernames redacted)
Not covered by this harness (already covered by
go test -race ./...):AcceptInviteendpoint, Heimdall/JWT,X-Syncheader parsing) — requires infra not stood up here.DeleteMemberimmediate-publish-failure-is-returned, and changed-username revoke-then-put ordering — covered byTestCommitteeWriterOrchestrator_DeleteMember_MessagePublishingFailureandTestPublishMemberMessages_ChangedUsername_RevokesOldBeforeGrantingNew.Test plan
go build ./...go test -race ./...nats-server(JetStream enabled) — see aboveAcceptInviteeventual-consistency windowJira: LFXV2-2856
Made with Cursor