fix: forward idempotency key on balance creation and document create race (EN-1205)#110
fix: forward idempotency key on balance creation and document create race (EN-1205)#110flemzord wants to merge 3 commits into
Conversation
…race
CreateBalance ignored the Idempotency-Key (passed "" to the ledger), so a
retried POST /wallets/{id}/balances could clobber the balance metadata, and
the check-then-act on account metadata was an undocumented race.
- Forward the Idempotency-Key to AddMetadataToAccount so retries are
deduplicated by the ledger
- Document the residual concurrent-first-create behaviour (last-write-wins on
priority/expiresAt for the single account); a fully atomic create would
need a conditional metadata write the ledger API does not expose
Adds a test asserting the key is forwarded.
WalkthroughThe PR adds idempotency support to balance creation. The manager method now accepts an idempotency key, hashes it, and stores it in account metadata to detect and replay responses for retried requests. The handler extracts the key from the request header and forwards it. Tests validate key forwarding, replay behavior, and rejection of duplicate operations with different keys. ChangesIdempotency Support for Balance Creation
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Manager
participant Metadata
Note over Client,Metadata: Request 1: Create balance with key ABC
Client->>Handler: POST /balances + Idempotency-Key: ABC
Handler->>Manager: CreateBalance(ctx, "ABC", data)
Manager->>Manager: Hash "ABC" → hash1
Manager->>Manager: Look up account "my-balance"
Manager->>Metadata: Account does not exist
Manager->>Metadata: Create account + store hash1
Manager->>Handler: Return new Balance
Handler->>Client: 201 Created + Balance
Note over Client,Metadata: Request 2: Same operation with same key ABC
Client->>Handler: POST /balances + Idempotency-Key: ABC
Handler->>Manager: CreateBalance(ctx, "ABC", data)
Manager->>Manager: Hash "ABC" → hash1
Manager->>Manager: Look up account "my-balance"
Manager->>Metadata: Account exists + stored hash == hash1
Manager->>Handler: Return existing Balance
Handler->>Client: 201 Created + same Balance (replayed)
Note over Client,Metadata: Request 3: Same operation with different key XYZ
Client->>Handler: POST /balances + Idempotency-Key: XYZ
Handler->>Manager: CreateBalance(ctx, "XYZ", data)
Manager->>Manager: Hash "XYZ" → hash2
Manager->>Manager: Look up account "my-balance"
Manager->>Metadata: Account exists + stored hash (hash1) != hash2
Manager->>Handler: Return error
Handler->>Client: 400 Bad Request
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
flemzord
changed the title
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Revue inline: le forwarding de l’Idempotency-Key ne suffit pas encore à rendre CreateBalance idempotent après un premier succès.
Address review feedback: after a successful create, a retry with the same Idempotency-Key hit the existence check and returned ErrBalanceAlreadyExists (400) rather than replaying the original 201. When an Idempotency-Key is present, treat an already-existing balance as an idempotent replay and return it; without a key, keep the explicit ALREADY_EXISTS conflict. Adds TestBalancesCreateIdempotentReplay (two same-key/same-body calls both return 201 and the metadata write happens only once).
💬 Comments — multi-model reviewThe PR correctly threads the HTTP Idempotency-Key through to AddMetadataToAccount, introduces an app-level replay short-circuit on retries, and documents the residual concurrent-create race. The core intent is sound and the primary changes are well-structured. However, several issues remain. Most importantly, the newly added idempotency hash stored in metadata is susceptible to the same last-write-wins race the PR documents for priority/expiresAt: a concurrent first-time create with a different key will overwrite the hash, silently breaking replay for the earlier caller without any documentation of this behavior. The relationship between the app-level hash check and ledger-level deduplication is also undocumented, making future maintenance error-prone. On the testing side, the replay test only validates the app-level short-circuit path and does not cover the ledger-dedup path that is the PR's primary fix, and replay correctness for Priority/ExpiresAt fields is not asserted. 🟡 [minor] Concurrent creates can clobber the idempotency replay marker
The new idempotency hash is stored in a single metadata field. In the documented concurrent-create race where two first-time creates of the same balance pass the existence check with different idempotency keys, the later metadata write overwrites not only priority/expiresAt but also this idempotency hash. If the earlier caller retries after a timeout, CreateBalance sees the balance exists but the stored hash no longer matches, so it falls through to ErrBalanceAlreadyExists instead of replaying success. The PR's comment only documents priority/expiresAt as last-write-wins, leaving the idempotency marker's susceptibility undocumented and weakening the retry guarantee in exactly the race being described. Suggestion: Either document that the idempotency marker is also last-write-wins in the concurrent first-create race, or store replay markers in non-overlapping metadata keys (e.g. keyed by hash/prefix) so concurrent writes preserve replayability for both callers while priority/expiresAt remain last-write-wins. 🟡 [minor] App-level idempotency replay is best-effort on top of ledger dedup, but this is not documented
The short-circuit at the hash-match check requires that a prior create successfully persisted the idempotency hash into metadata. The ledger's own deduplication of AddMetadataToAccount operates independently and may have a different retention window or ordering guarantee. These two mechanisms can disagree: the ledger may dedup a write the app never recorded, or the stored hash may have been overwritten by a concurrent create. In either case a retry with the original key hits ErrBalanceAlreadyExists (400) instead of replaying, with no code comment to explain this is intentional and expected. Suggestion: Add a brief comment near the hash-match check noting that this is a best-effort app-level replay complementing ledger-level idempotency, and that a key mismatch falls through to ErrBalanceAlreadyExists by design. 🟡 [minor] Replay path reconstructs balance from metadata rather than from request data — divergence undetected by tests
On replay, CreateBalance returns BalanceFromAccount(*ret), reconstructing the balance from existing account metadata. The create path returns the freshly-built balance struct. If the stored metadata encodes priority or expiresAt differently from what BalanceFromAccount expects, the replayed response can diverge from the original. The test only asserts Name equality, so any divergence in Priority or ExpiresAt between the original and replayed responses would go undetected. Suggestion: Add test assertions comparing Priority and ExpiresAt between the first create response and the replayed (idempotent) response, to lock in that replay returns an equivalent balance. ⚪ [nit] Idempotent-replay test does not exercise the ledger-dedup path
TestBalancesCreateIdempotentReplay asserts addCalls==1, proving the second create did not re-write, but it relies entirely on the app-level GetAccount short-circuit (the existing account has a matching hash). It does not test the scenario where GetAccount keeps returning not-found and the ledger itself deduplicates the AddMetadataToAccount call. Since the PR's primary fix is forwarding the idempotency key to the ledger, this path is not covered. Suggestion: Consider adding a test variant where GetAccount always returns not-found and the AddMetadataToAccount mock simulates ledger dedup (no-op on duplicate key), to document expected end-to-end behaviour and verify the forwarded key is actually used. ⚪ [nit] Verify no duplicate ptr/Ptr helper across package
A ptr generic helper is defined at the top of the test file. If a wallet.Ptr or similar helper already exists elsewhere in the package, there is potential for reader confusion about which to use. Suggestion: Confirm there is no redundant helper in the package and, if one exists, consolidate to a single shared utility. Reviewed in parallel by claude (anthropic/claude-opus-4-8) and gpt (openai/gpt-5.5), then consolidated. This comment is updated on each push. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
pkg/manager.go (1)
577-593:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftConcurrent first-create requests can invalidate each other's replay key.
The residual race is wider than the new comment suggests. If two first-time creates for the same balance arrive with different idempotency keys, both can pass Line 573, and the later write at Line 592 overwrites the earlier caller's stored hash. After that, a retry of the earlier successful request no longer matches Line 579 and returns
ErrBalanceAlreadyExistsinstead of replaying the original201.Please either make that limitation explicit and cover it with a regression test, or move replay identity out of this single last-write-wins metadata slot.
🤖 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 `@pkg/manager.go` around lines 577 - 593, Concurrent creates can race when assigning MetadataKeyBalanceIdempotencyKey: change the write to store the hashed idempotency key only if the metadata slot is currently empty (atomic set-if-not-exists) instead of always overwriting balanceMetadata[MetadataKeyBalanceIdempotencyKey]; implement this using the storage layer's conditional write/transaction or a compare-and-set operation in the Create/ensure-balance path (where NewBalance, balanceMetadata and hashIdempotencyKey are used) so the first writer wins deterministically, and add a regression test that fires two concurrent first-time creates with different ik values to assert the original creator's hash is preserved and retries replay correctly (use BalanceFromAccount/MetadataKeyWalletBalance to verify behavior).
🤖 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 `@pkg/manager.go`:
- Around line 577-593: Concurrent creates can race when assigning
MetadataKeyBalanceIdempotencyKey: change the write to store the hashed
idempotency key only if the metadata slot is currently empty (atomic
set-if-not-exists) instead of always overwriting
balanceMetadata[MetadataKeyBalanceIdempotencyKey]; implement this using the
storage layer's conditional write/transaction or a compare-and-set operation in
the Create/ensure-balance path (where NewBalance, balanceMetadata and
hashIdempotencyKey are used) so the first writer wins deterministically, and add
a regression test that fires two concurrent first-time creates with different ik
values to assert the original creator's hash is preserved and retries replay
correctly (use BalanceFromAccount/MetadataKeyWalletBalance to verify behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fd763bd-cac2-4154-9164-4b59f3c47271
📒 Files selected for processing (4)
pkg/api/handler_balances_create.gopkg/api/handler_balances_create_test.gopkg/manager.gopkg/metadata.go
4 participants
Problem (Medium — M5, M7-balance)
CreateBalancepassed""as the Idempotency-Key toAddMetadataToAccount, so a retriedPOST /wallets/{id}/balanceswas not deduplicated by the ledger and could re-apply (clobber) the balance metadata.CreateBalanceis a check-then-act on account metadata (GetAccountexistence check, thenAddMetadataToAccount), an undocumented race.Fix
Idempotency-KeytoAddMetadataToAccountso retries of the same request are deduplicated by the ledger — this removes the common-case duplicate/clobber.priority/expiresAtfor that single account (no fund movement, no duplicate account).A fully atomic create would require a conditional metadata write on the ledger side, which the API does not currently expose — called out in the doc comment as the proper follow-up rather than faking a distributed lock here.
Tests
TestBalancesCreateForwardsIdempotencyKey: theIdempotency-Keyheader reaches the ledger call.Note
This is the balance half of M7 (the wallet half is in #109), grouped here because both changes touch
CreateBalance.From the in-depth repository review.