fix: use detached context for CreateMember rollback - #169
Merged
Conversation
When an HTTP client times out mid-request (e.g. the v1-sync-helper's 30 s http.Client.Timeout under onboarding load), the committee-service request context is cancelled. The deferred rollback in CreateMember was calling deleteMemberKeys with that cancelled context, causing every NATS KV cleanup call to fail immediately. The uniqueness key written by UniqueMember (step 7) was left orphaned, blocking future create attempts for the same email+committee pair with a spurious 409 Conflict. Fix: use context.WithTimeout(context.Background(), 10s) for the rollback, matching the pattern already used by UpdateMember for its stale-key cleanup goroutine. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-2984 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
WalkthroughCreateMember rollback cleanup now uses a fresh background context with a 10-second timeout. Tests verify that member-key deletion continues after request cancellation. ChangesMember creation cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/service/committee_member_writer.go (1)
104-106: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a regression test for canceled request contexts.
The test at
internal/service/committee_member_writer_test.goLines [711]-[741] callsdeleteMemberKeysdirectly withcontext.Background(). It does not prove thatCreateMembercompletes rollback after the caller context is canceled.Force a post-
UniqueMemberfailure, cancel the caller context, and verify that all tracked keys are deleted.🤖 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.go` around lines 104 - 106, Add a new regression test in committee_member_writer_test.go that verifies CreateMember completes rollback cleanup even when the caller's context is canceled. The test should set up a scenario that triggers a failure after the UniqueMember check (to populate tracked keys), then cancel the caller context passed to CreateMember, and finally assert that all tracked keys are deleted despite the context cancellation. This ensures that the timeout-based cleanup context created within CreateMember (as shown in the deleteMemberKeys call) operates independently and successfully cleans up resources even when the request context becomes canceled.
🤖 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.
Nitpick comments:
In `@internal/service/committee_member_writer.go`:
- Around line 104-106: Add a new regression test in
committee_member_writer_test.go that verifies CreateMember completes rollback
cleanup even when the caller's context is canceled. The test should set up a
scenario that triggers a failure after the UniqueMember check (to populate
tracked keys), then cancel the caller context passed to CreateMember, and
finally assert that all tracked keys are deleted despite the context
cancellation. This ensures that the timeout-based cleanup context created within
CreateMember (as shown in the deleteMemberKeys call) operates independently and
successfully cleans up resources even when the request context becomes canceled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e65da023-1924-4d9f-8906-aa792040fd81
📒 Files selected for processing (1)
internal/service/committee_member_writer.go
Add TestCreateMember_RollbackSurvivesCancelledContext to prove the LFXV2-2984 fix: rollback cleanup uses a detached context and succeeds even when the request context is already cancelled. - Add rejectCancelledCtx field to TestMockCommitteeMemberWriter so DeleteMember/GetMemberRevision return ctx.Err() when the context is done. Without this flag the existing mock ignores context state and the test would pass with either the old or new code. - Pre-seed the uniqueness key in memberWriter.members so GetMemberRevision can resolve it during rollback (UniqueMember stores to w.keys but GetMemberRevision reads w.members). - Cancel the caller context before calling CreateMember to simulate a timed-out HTTP client, force orgIndexErr to trigger rollback, and assert the uniqueness key appears in deletedKeys. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-2984 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
Contributor
Author
Review Feedback AddressedCommit: e4d8727 — test(member): regression for cancelled-ctx rollback Changes Made
Threads Resolved1 of 1 unresolved threads addressed in this iteration. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/service/committee_member_writer_test.go`:
- Around line 971-1033: Convert
TestCreateMember_RollbackSurvivesCancelledContext into a table-driven test using
a test-case slice and subtests, while retaining the existing internal
infrastructure mock fake setup and regression assertions. Keep the
cancelled-context scenario, injected org-index failure, rollback configuration,
and uniqueness-key deletion verification unchanged within the table-driven
structure.
🪄 Autofix
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: 85d34be8-938d-463d-a740-a7df79a515f6
📒 Files selected for processing (1)
internal/service/committee_member_writer_test.go
Contributor
Author
Review Feedback Addressed (iteration 2)No new code changes — one thread responded to. No Change Needed
Threads Resolved0 of 1 thread resolved in this iteration — leaving open for reviewer confirmation. |
andrest50
approved these changes
Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CreateMemberrollback deferred func was callingdeleteMemberKeyswith the HTTP request context, which is cancelled when the client times out (e.g. v1-sync-helper's 30-secondhttp.Client.Timeoutunder onboarding load)lookup/member/<sha256-hash>) orphaned in thecommittee-membersbucketUniqueMember→ErrKeyExists→ 409 Conflict, blocking the member from ever being createdcontext.WithTimeout(context.Background(), 10s)in the rollback, matching the pattern already used byUpdateMemberfor its stale-key cleanup goroutineTest plan
make test)make lintcleanmake build && make build-clicleanCloses LFXV2-2984
🤖 Generated with Claude Code