fix: make wallet creation and pending debits idempotent (EN-1204)#109
fix: make wallet creation and pending debits idempotent (EN-1204)#109flemzord wants to merge 12 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 56 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughWallet creation now accepts request idempotency keys, replays matching creates, and returns conflicts for reused keys with different payloads. Debit now rejects non-deterministic idempotent requests, assigns deterministic pending-hold IDs, and adds tests for replay and rejection cases. ChangesIdempotency for Wallet Create and Debit
Sequence Diagram(s)Wallet create replay flow sequenceDiagram
participant Client
participant createWalletHandler
participant Manager
participant AddMetadataToAccount
Client->>createWalletHandler: POST /wallets with Idempotency-Key
createWalletHandler->>Manager: CreateWallet(ctx, ik, data)
Manager->>Manager: deterministicID("wallet", ik)
Manager->>AddMetadataToAccount: store create-request fingerprint
alt matching fingerprint
Manager-->>createWalletHandler: replayed wallet
createWalletHandler-->>Client: 201 Created
else different request body
Manager-->>createWalletHandler: ErrIdempotencyConflict
createWalletHandler-->>Client: 409 Conflict
end
Debit replay validation flow sequenceDiagram
participant Client
participant debitWalletHandler
participant Manager
participant "wallet.PostTransaction"
Client->>debitWalletHandler: POST /wallets/{id}/debit with Idempotency-Key
debitWalletHandler->>Manager: Debit(ctx, ik, request)
Manager->>Manager: deterministicID("hold", ik)
Manager->>Manager: reject wildcard or expiring sources
alt replayable pending debit
Manager->>"wallet.PostTransaction": submit pending debit
"wallet.PostTransaction"-->>Manager: hold created
Manager-->>debitWalletHandler: hold
debitWalletHandler-->>Client: 200 OK
else non-idempotent sources
Manager-->>debitWalletHandler: ErrNonIdempotentDebit
debitWalletHandler-->>Client: 400 Bad Request
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
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: l’idempotence est améliorée, mais deux cas restent fragiles dans les tests et dans les sources expirables.
| } | ||
|
|
||
| hold = Ptr(debit.newHold()) | ||
| // Derive the hold ID from the Idempotency-Key so a retry produces an |
There was a problem hiding this comment.
Ça stabilise le hold ID, mais pas forcément toute la requête ledger. Plus bas, les sources sont recalculées puis filtrées avec time.Now(); pour un pending debit avec balances:["*"] ou une balance nommée qui expire entre deux essais, le retry avec la même Idempotency-Key peut produire un script/body différent et être rejeté par le ledger. Il faut rendre la résolution des sources déterministe pour une clé/requête donnée, ou au minimum couvrir ce cas par un test d’expiration.
There was a problem hiding this comment.
You're right — the deterministic hold ID stabilises the ID but not the whole body. I documented this explicitly at the call site and strengthened TestWalletsDebitPendingIdempotency to assert a byte-identical PostTransaction across retries for the stable case (explicit, non-expiring source set). The wildcard (balances:["*"]) and expiry-crossing cases remain non-deterministic because sources are resolved against live ledger state with time.Now(); fully fixing those needs deterministic source resolution (e.g. persisting the resolved request per key), which I called out in the comment as separate follow-up rather than solve in this PR.
🛑 Changes requested — multi-model reviewThe PR stabilizes wallet and hold IDs by deriving them deterministically from the Idempotency-Key via UUIDv5, and forwards the key to the ledger for wallet creation. However, pending-debit idempotency is only partially achieved: the hold ID is now stable, but the ledger request body is still rebuilt from live balance state on every retry. A retry crossing an expiry boundary or involving wildcard source resolution will produce a different Numscript body and be rejected by the ledger as a conflict rather than replayed. The new test 🟠 [major] Pending debit retries can still yield a different ledger body, breaking idempotency
Only the hold ID is made deterministic; the full ledger request body is still reconstructed from live balance state (source balances fetched and filtered against Suggestion: Make the entire pending-debit ledger body deterministic for a given idempotency key. For example, persist or cache the resolved transaction body (or at minimum the resolved source set) keyed by the idempotency key before submitting to the ledger, and reuse it on retry. As a short-term alternative, explicitly reject idempotent pending-debit retries that depend on expiring or wildcard source resolution with a clear error until deterministic body construction is implemented. 🟡 [minor] Wallet ID and hold ID derived from the same namespace without a type discriminator
Suggestion: Prefix the key with a resource-type discriminator before hashing, e.g. 🟡 [minor] Documented idempotency limitation for expiring/wildcard balances needs follow-up tracking
The NOTE comment and the Suggestion: Ensure the limitation is surfaced clearly in user-facing API documentation and that a follow-up issue (e.g. for deterministic source resolution) is filed and linked from the NOTE comment, so the production gap is not forgotten. ⚪ [nit] testEnv captured by closure before assignment is a fragile pattern
Suggestion: Optionally extract 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. |
Creation paths generated a fresh UUID on every request, so retrying with the same Idempotency-Key created duplicates (and for pending debits the ledger rejected the retry, since the hold ID baked into the request body changed each time). - Derive the wallet ID and the pending-debit hold ID from the Idempotency-Key (UUIDv5) when one is provided; keep random IDs otherwise - Forward the Idempotency-Key to the ledger when creating a wallet Adds tests asserting stable IDs and key forwarding across retries.
Address review feedback: - Wallet-create idempotency test now replays a fixed payload (a regenerated name would be a different body for the same key, i.e. a conflict, not a replay) and asserts the targeted account is stable across retries. - Strengthen the pending-debit idempotency test to assert a byte-identical ledger body across retries for a stable source set. - Document that pending-debit idempotency only holds for an explicit, non-expiring source set: the wildcard / expiry-crossing case is resolved against live state with time.Now() and may still differ on retry.
A pending debit derives a stable hold ID from the Idempotency-Key, but the
ledger enforces idempotency by hashing the whole request body. The source set
is resolved from live ledger state: a wildcard ("*") set can change between
attempts, and a balance with a future expiry can cross time.Now() on retry.
Either case yields a different body, which the ledger rejects as a conflict
rather than replaying — a false idempotency guarantee.
Reject such debits up front with ErrNonIdempotentDebit (400 VALIDATION) when an
Idempotency-Key is present, instead of submitting a request that cannot be
safely replayed. Explicit, non-expiring source sets remain idempotent.
Constraint: Ledger idempotency is keyed on a hash of the full request body
Rejected: Persist/cache the resolved body per key | larger change, deferred
Confidence: high
Scope-risk: narrow
Directive: The ik!="" guard is what makes wildcard/expiring debits fail; debits without a key keep their previous behaviour
Wallet IDs and hold IDs were both derived from deterministicID(ik) using the
same namespace, so reusing one Idempotency-Key across a wallet creation and a
pending debit would derive the same UUID. The accounts live in different chart
namespaces, but the separation was implicit.
Prefix the hashed input with a resource-kind discriminator ("wallet:", "hold:")
so the derived IDs are disjoint by construction and the intent is explicit.
Confidence: high
Scope-risk: narrow
TestWalletsDebit assigned testEnv and captured it inside the newTestEnv callbacks in the same statement. The closures only run during ServeHTTP, so it worked, but the assign-and-capture pattern is subtle. Extract chart and ledgerName locals after newTestEnv returns and reference those inside the closures, making the data-flow explicit. No behaviour change. Confidence: high Scope-risk: narrow
fguery
force-pushed
the
fix/idempotency-create
branch
from
ccf7dca to
e512dc0
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
Wallet creation retries still produce different ledger request bodies because the created-at metadata remains time-dependent, so the main idempotency fix is incomplete.
| wallet := NewWallet(data.Name, m.ledgerName, data.Metadata) | ||
| // Derive the wallet ID from the Idempotency-Key so a retry targets the | ||
| // same account instead of creating a duplicate wallet. | ||
| if ik != "" { |
There was a problem hiding this comment.
🔴 [blocker] Stabilize CreatedAt for idempotent wallet creation
When an Idempotency-Key is present, this only stabilizes wallet.ID; NewWallet still sets CreatedAt from time.Now(), and wallet.LedgerMetadata() includes that timestamp in the AddMetadataToAccount body. Retrying the same POST /wallets with the same key a moment later therefore sends a different ledger request body, so the ledger's hash-based idempotency will see a conflict instead of a replay.
![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
🤖 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 `@pkg/manager.go`:
- Around line 472-485: The CreateWallet retry path still builds a
non-deterministic payload because NewWallet() stamps CreatedAt with time.Now()
and wallet.LedgerMetadata() includes it, so repeated requests with the same
idempotency key can send different bodies. Update CreateWallet to make the
metadata stable whenever ik is set, either by reusing the originally persisted
wallet metadata/created timestamp or by deriving a deterministic CreatedAt
before calling m.client.AddMetadataToAccount. Keep the wallet ID logic and make
sure the returned Wallet body is also consistent for retries.
🪄 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: 5950e791-7e87-46e1-8b52-6a18f82f81ac
📒 Files selected for processing (6)
pkg/api/handler_wallets_create.gopkg/api/handler_wallets_create_test.gopkg/api/handler_wallets_debit.gopkg/api/handler_wallets_debit_test.gopkg/error.gopkg/manager.go
Deriving wallet.ID from the Idempotency-Key stabilised the ID but not the whole ledger body: NewWallet stamps CreatedAt with time.Now(), and LedgerMetadata() serialises it into the AddMetadataToAccount body. The ledger hashes that body to enforce idempotency (ComputeIdempotencyHash, unique idempotency_hash), so a retry under the same key a moment later sent a different body and was rejected as an idempotency conflict rather than replayed. The returned wallet drifted for the same reason. When a key is present, GetAccount the derived account first: if it already exists, replay the persisted wallet instead of re-sending a body with a fresh CreatedAt. Mirrors the CreateBalance replay path. Constraint: Ledger idempotency hashes the full request body (same key + different body = conflict) Rejected: Derive CreatedAt deterministically from the key | timestamp would be meaningless Confidence: high Scope-risk: narrow Not-tested: two truly-concurrent first creates with the same key still send divergent bodies; the second gets ErrIdempotencyKeyConflict (follow-up: catch and reload)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
Wallet creation idempotency is still incomplete: it can bypass conflicts for reused keys with different payloads and can still conflict for overlapping identical retries because CreatedAt remains non-deterministic.
| switch { | ||
| case errors.Is(err, ErrAccountNotFound): | ||
| // First attempt for this key: fall through to create below. | ||
| case err == nil: |
There was a problem hiding this comment.
🟠 [major] Do not bypass conflict checks on key reuse
When the same Idempotency-Key is reused with a different wallet creation body after the first request has committed, this branch returns the existing wallet before sending the new body to the ledger. That bypasses the ledger's body-hash conflict detection and turns a non-replay into a successful response with the original wallet/name/metadata, which can silently give clients the wrong result.
| m.ledgerName, | ||
| m.chart.GetMainBalanceAccount(wallet.ID), | ||
| "", | ||
| ik, |
There was a problem hiding this comment.
🟠 [major] Keep create-wallet bodies stable for overlapping retries
When two identical POST /wallets requests with the same Idempotency-Key overlap before the account is visible, both can pass the GetAccount not-found check and reach this ledger call. Since wallet.LedgerMetadata() still includes CreatedAt from NewWallet's time.Now(), the two submitted bodies differ and the ledger will reject the second as an idempotency conflict instead of replaying the first result.
The replay short-circuit fixed CreatedAt drift for sequential retries but left two gaps: it returned the existing wallet even when the same Idempotency-Key was reused with a different body (masking a real conflict), and two concurrent first creates could both pass the existence check and send bodies differing only by CreatedAt, so the ledger rejected the second as a conflict. Resolve idempotency against the persisted wallet and let the ledger arbitrate the race: - existing + matching request (name+metadata) -> replay the persisted wallet; - existing + different request -> ErrIdempotencyConflict (409 CONFLICT) instead of silently replaying the original; - not yet visible + ledger conflict on write (a concurrent writer committed first) -> reload and replay-or-conflict. CreatedAt is excluded from the match: it legitimately differs between attempts. DefaultLedger.AddMetadataToAccount now translates the ledger's CONFLICT (V2ErrorsEnumConflict) into ErrIdempotencyConflict so the manager can act on it. Constraint: Ledger idempotency hashes the full body; same key + different body = CONFLICT Constraint: A meaningful CreatedAt can only be the first-write time, read back (no deterministic value) Rejected: Derive CreatedAt from the key | would be a meaningless timestamp Rejected: Distributed lock around create | infra for a benign, ledger-detectable race Confidence: high Scope-risk: moderate Directive: replayOrConflict deliberately ignores CreatedAt; do not add it to the comparison
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The patch improves immediate retries, but wallet create idempotency is checked against mutable wallet state, which can misclassify retries or conflicts after subsequent updates.
| // from the comparison: it legitimately differs between attempts and must not be | ||
| // treated as a conflicting change. | ||
| func replayOrConflict(existing, requested *Wallet) (*Wallet, error) { | ||
| if existing.Name != requested.Name || !maps.Equal(existing.Metadata, requested.Metadata) { |
There was a problem hiding this comment.
🟠 [major] Avoid validating replays against mutable wallet state
When a wallet created with an Idempotency-Key is patched before the key is retried or reused, this compares the incoming create body against the wallet's current metadata instead of the original create body. In that scenario the original retry can incorrectly become a 409 after metadata changes, or a different create body matching the patched metadata can be accepted as a replay, so the idempotency conflict semantics depend on later wallet updates.
Drop the strict "same key + different body -> 409 conflict" contract. For an internal service whose callers control their own Idempotency-Keys, distinguishing a replay from a conflict adds significant surface (a stored request fingerprint, SDK error translation, a 409 path) to police a low-probability client misuse — and it was the source of a recurring review spiral (mask-conflict -> compare body -> compare against mutable state). Use the simpler, equally valid idempotency semantic: same key -> same result. If a wallet already exists for the derived ID, replay it regardless of the retried body. On the rare concurrent first-create race, the ledger rejects the losing writer's body (different CreatedAt) with a conflict, which we catch to reload and replay the winner's wallet. This also removes the mutable-state comparison entirely, so idempotency no longer depends on later UpdateWallet calls. The ledger CONFLICT is still translated to ErrIdempotencyConflict, but only used internally to trigger the reload. Rejected: Strict Stripe-style key-reuse conflict (409) | gold-plating for a trusted-caller internal service Confidence: high Scope-risk: narrow Directive: replay-on-existence is intentional; a key reused with a different body returns the original wallet by design
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The intended concurrent idempotent wallet-create replay path does not handle the ledger error code used for body-hash mismatches, so a real race can still fail instead of returning the original wallet.
| // The ledger rejects an Idempotency-Key reused with a different body | ||
| // hash as a CONFLICT. Translate it so callers can replay or report it. | ||
| var v2 *sdkerrors.V2ErrorResponse | ||
| if errors.As(err, &v2) && v2.ErrorCode == shared.V2ErrorsEnumConflict { |
There was a problem hiding this comment.
🟠 [major] Map validation idempotency mismatches to replay
When two create-wallet calls race with the same Idempotency-Key, both can miss existingWallet, and the second ledger request differs because CreatedAt was regenerated. The ledger reports that body-hash mismatch as a V2 VALIDATION error, not CONFLICT, so this branch never wraps ErrIdempotencyConflict and Manager.CreateWallet falls through to internalError instead of reloading the wallet.
There was a problem hiding this comment.
Good catch — confirmed against the ledger: ErrInvalidIdempotencyInput (internal/controller/ledger/log_process.go) maps to VALIDATION (400) when the prior log is found and the recomputed body hash differs; CONFLICT (409) only fires on a simultaneous insert race. The old translation matched CONFLICT only, so the common concurrent case fell through to a 500.
Fixed in 01bbe93: CreateWallet no longer classifies the write error. On any AddMetadataToAccount failure after the existence check missed, it re-checks whether the wallet now exists — if a concurrent attempt created it, it replays the persisted wallet; otherwise it surfaces the original error. This is robust to whichever code the ledger returns (VALIDATION or CONFLICT), and the CONFLICT-only ErrIdempotencyConflict translation has been removed. Covered by TestWalletsCreateConcurrentReplaysOnWriteRejection.
The concurrent-create replay path only triggered on a ledger CONFLICT, but the ledger reports a reused Idempotency-Key with a different body hash as a VALIDATION error when it finds the prior log (ErrInvalidIdempotencyInput in internal/controller/ledger/log_process.go); CONFLICT only occurs on a simultaneous insert race. So the common concurrent case fell through to a 500. Stop classifying the write error. On any AddMetadataToAccount failure after the existence check missed, re-check whether the wallet now exists: if a concurrent attempt created it, replay the persisted wallet; otherwise surface the original error. This is robust to whichever code the ledger returns (VALIDATION or CONFLICT) and removes the now-unneeded ErrIdempotencyConflict translation. Constraint: Ledger reports same-key/different-body as VALIDATION (prior log found) or CONFLICT (insert race) Rejected: Match the VALIDATION error message/code | brittle, ledger-internal wording Confidence: high Scope-risk: narrow Directive: do not classify the write error; existence after the write is the source of truth for replay-vs-fail
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The wallet-create idempotency path can incorrectly treat a different request body with the same idempotency key as a successful replay, bypassing the ledger's mismatch detection.
Callers cannot be assumed to be trusted internal services, so silently replaying the original wallet when an Idempotency-Key is reused with a different body would hide a divergent request and return the wrong resource. Adopt the strict contract: same key + different body -> 409 CONFLICT. Match the retry against an immutable fingerprint of the original create request (name + custom metadata), stamped into the account metadata at creation under MetadataKeyWalletCreateRequestHash. The comparison is never against the wallet's live metadata — UpdateWallet can mutate that — so the replay/conflict outcome is stable across later patches. The concurrent path stays error-code-agnostic: on any write failure after the existence check missed, reload and replay-or-conflict against the persisted fingerprint, since the ledger reports a body-hash mismatch as VALIDATION or CONFLICT depending on timing. Constraint: callers are not guaranteed trusted; key reuse with a different body must not be masked Constraint: idempotency comparison must use immutable data (UpdateWallet mutates live metadata) Rejected: replay-only (same key -> same result) | unsafe when callers aren't trusted Rejected: compare against live wallet metadata | depends on mutable state (flagged earlier) Confidence: high Scope-risk: moderate Directive: replayOrConflict compares the stored create fingerprint, never live metadata; do not fold CreatedAt into the fingerprint
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The idempotency fingerprint can collide for distinct valid request strings, so the patch can silently replay a different wallet creation request instead of reporting a conflict.
| sort.Strings(keys) | ||
|
|
||
| h := sha256.New() | ||
| _, _ = h.Write([]byte(name)) |
There was a problem hiding this comment.
🟠 [major] Serialize fingerprint fields unambiguously
When wallet names or metadata values contain \u0000, different create bodies can produce the same byte stream here (for example metadata {"a":"b\u0000c\u0000d"} collides with {"a":"b","c":"d"}). In that case reusing an Idempotency-Key with a different request can be incorrectly treated as a replay instead of returning the intended conflict; consider hashing a canonical JSON object or length-prefixing each field.
There was a problem hiding this comment.
Fixed in 68de18f — real bug, thanks. The NUL-separated concatenation could collide ({"a":"b\x00c\x00d"} vs {"a":"b","c":"d"} hashed identically), so a reused key with a different body could be replayed instead of returning 409.
The fingerprint now hashes canonical JSON of {name, metadata} (encoding/json sorts map keys); quoting and escaping make the encoding self-delimiting, so no embedded byte can forge a field boundary. Added an in-package regression test (TestWalletCreateRequestFingerprint) covering exactly that collision example plus metadata order-independence.
The fingerprint concatenated name and metadata with a NUL separator, so a
metadata value containing NUL could collide with a differently-structured
request — e.g. {"a":"b\x00c\x00d"} produced the same byte stream as
{"a":"b","c":"d"}. A reused Idempotency-Key with such a different body could
then be treated as a replay instead of a 409 conflict.
Hash canonical JSON of {name, metadata} instead (encoding/json sorts map keys);
quoting and escaping make the encoding self-delimiting, so no embedded byte can
forge a field boundary. Add an in-package regression test covering the
collision example and order-independence.
Constraint: fingerprint must be collision-free for distinct valid requests
Confidence: high
Scope-risk: narrow
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The main sequential retry paths are improved, but wallet creation still sends non-deterministic ledger bodies for the same idempotency key under a realistic concurrent retry race. That can still produce failures instead of an idempotent replay.
| // immutable fingerprint of the original create request (stored at creation), | ||
| // never against the wallet's live metadata which UpdateWallet can mutate. | ||
| fingerprint := walletCreateRequestFingerprint(data.Name, data.Metadata) | ||
| body := wallet.LedgerMetadata() |
There was a problem hiding this comment.
🟠 [major] Stabilize the wallet create body before ledger write
When two same-key POST /wallets calls run concurrently, both can miss the preflight GetAccount and reach this write with different CreatedAt values in body; the ledger then rejects the second request as a different body for the same idempotency key. If the first write is not yet visible when the error-path recheck runs, this still returns a 500 instead of replaying the wallet, so the idempotent create path remains racy.
There was a problem hiding this comment.
Intentional / accepted residual, with the ledger's actual semantics in mind.
The fully-deterministic-body fix would require a deterministic CreatedAt, which means either a synthetic (non-wall-clock) timestamp for idempotent creates or surfacing the ledger's insertion timestamp through the SDK (not currently exposed in the mapped account type). Both trade a real downside for closing a very narrow window — so weighing it against how the ledger actually behaves:
- The common concurrent case returns VALIDATION, and it does so because the ledger found the winner's already-committed log (
ErrInvalidIdempotencyInput, internal/controller/ledger/log_process.go). On a single-Postgres ledger that write is therefore visible to our immediate error-pathGetAccountrecheck (read-committed), so it replays — no 500. - The only path that can transiently 500 is a true simultaneous insert deadlock (CONFLICT) where the winner isn't committed at that instant. It self-heals on the next retry (by then the winner is visible → replay). No duplicate wallet, no data corruption — just a rare transient that a normal retry resolves.
Given the downsides of forcing a deterministic body (synthetic createdAt) versus a rare self-healing transient, we're accepting this residual rather than changing wallet creation-time semantics. Happy to revisit if a deployment uses read replicas (where the visibility window widens) or if a deterministic/ledger-sourced createdAt becomes acceptable.
3 participants
Problem (Medium — M1, M7-wallet)
uuid.NewString()for the hold per request and baked it into the Numscript and metadata. A retry with the sameIdempotency-Keytherefore produced a different ledger request body, so the ledger (which hashes the body to enforce idempotency) rejected the retry instead of returning the original result.POST /walletsignored theIdempotency-Keyand minted a fresh wallet UUID on every call, so a retry created a duplicate wallet.Fix
Idempotency-Key(UUIDv5 over a fixed namespace) when one is provided; keep random IDs when it is absent.Idempotency-Keyto the ledger when creating a wallet.Tests
TestWalletsCreateIdempotency: twoPOST /walletswith the same key forward the key to the ledger and resolve to the same wallet ID.TestWalletsDebitPendingIdempotency: two pending debits with the same key yield the same hold ID.Note
The balance-creation idempotency (M7-balance) is handled together with the
CreateBalanceconcurrency fix (M5) in a sibling PR, since both touch the same function.From the in-depth repository review.