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 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR adds idempotency-aware balance creation by forwarding a request key into the manager, storing a hashed marker in account metadata, and replaying matching creates. It also updates wallet tests to use the shared pointer helper for timestamp fields. ChangesIdempotency Support for Balance Creation
Wallet Test Pointer Helper Updates
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 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.
💬 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
…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.
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).
The replay marker was a single shared metadata field (idempotencyKeyHash). In the documented concurrent first-create race, the later write overwrites that field, so the earlier caller's retry no longer matches and falls through to ErrBalanceAlreadyExists (400) instead of replaying — the marker is clobbered in exactly the race being described. Stamp the marker under a per-key metadata key (prefix + hash of the Idempotency-Key) instead. The ledger merges account metadata additively, so concurrent creates with different keys write different keys and every caller's marker survives — preserving replay for all of them. priority/expiresAt remain last-write-wins for the single account (no duplicate account, no fund movement); making those deterministic too needs a conditional create-if-absent write the ledger does not yet expose. Also document that the app-level replay is best-effort on top of the ledger's own AddMetadataToAccount dedup, and that a missing marker intentionally falls through to ErrBalanceAlreadyExists. Constraint: Ledger AddMetadataToAccount is an additive merge, not a replace Constraint: Ledger exposes no conditional (create-if-absent) metadata write Rejected: Distributed/advisory lock around create | infra + latency for a benign race Rejected: Require an Idempotency-Key on create | diverges from wallet/debit which allow keyless best-effort Confidence: high Scope-risk: narrow Directive: per-key markers rely on additive metadata merge; pinned by TestBalancesCreateConcurrentCreatePreservesReplayForAllKeys
Strengthen the idempotency tests around CreateBalance: - TestBalancesCreateIdempotentReplay now creates with a non-default Priority and an ExpiresAt and asserts both match between the original and the replayed response. The replay reconstructs the balance from stored metadata, so this catches any divergence from the freshly-built create response (ExpiresAt is compared via RFC3339Nano to avoid monotonic-clock flakiness). - TestBalancesCreateForwardsKeyForLedgerDedupWhenNotShortCircuited exercises the path where GetAccount never reports the account, so the app-level replay marker is never seen and idempotency falls entirely to the ledger: it asserts the same Idempotency-Key is forwarded to AddMetadataToAccount on every attempt. The per-key marker approach relies on the ledger merging account metadata additively (metadata = accounts.metadata || excluded.metadata in ledger internal/storage/ledger/accounts.go); the concurrent-create test's merge mock models that behaviour. Confidence: high Scope-risk: narrow
The api test package had a local generic ptr helper alongside the exported wallet.Ptr and go-libs pointer.For, which is the dominant idiom across these tests. Remove the local helper and standardise its call sites on pointer.For so there is a single, unambiguous pointer constructor in the tests. Confidence: high Scope-risk: narrow
fguery
force-pushed
the
fix/balance-create-idempotency
branch
from
1ddfa22 to
70ac346
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
✅ Approve — automated review
The changes correctly forward the idempotency key and add replay handling without introducing an evident regression in the modified code paths.
No findings.
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.