fix(api): stop contended PaymentIntent claims leaking their map entry

[sanity]

Fixes the intermittent payment_claim::tests::entry_survives_while_a_waiter_is_queued failure (seen in run 36506939413: payment_claim.rs:218 "entry outlived the last guard for this key"). The test was right; the code had a real race. This should unblock CI on #192.

Root cause

ClaimGuard::drop removed a PaymentIntent's entry from CLAIM_LOCKS only when Arc::strong_count(lock) == 2. That inference had two leak paths.

1. Handoff race (the CI failure). tokio's OwnedMutexGuard::drop releases the semaphore before its Arc field is dropped (checked in tokio 1.38.1, the locked version). So with a holder and one queued waiter:

1. The holder's ClaimGuard::drop runs first, sees count 3 (map + holder + waiter) and keeps the entry.
2. The _guard field drops and releases the mutex; the holder's Arc is still alive.
3. In that gap the waiter acquires on another worker, finishes, and runs its own ClaimGuard::drop. It also sees count 3 and keeps the entry.
4. The holder's Arc drops. Nothing removes the entry.

2. Woken-then-dropped waiter (deterministic). If the holder releases while a waiter is queued, the holder sees count 3 and keeps the entry. If the waiter's future is then dropped before it is polled again, it never builds a ClaimGuard, so nothing ever runs the removal check and the entry leaks every time. axum drops a handler's future when the client disconnects, so this can happen in production.

Either way a contended PaymentIntent could leak its entry permanently, which breaks the bound the module documents against unauthenticated garbage ids. Mutual exclusion was not affected.

Fix

Track claimants explicitly instead of inferring them from strong_count. This closes both leak paths:

• Each entry holds its mutex plus a claimants count, only touched under the map lock.
• claim() increments the count under the map lock before awaiting and gets back a Registration. Dropping it decrements the count and removes the entry at zero.
• The registration is a plain local until the lock is acquired, so a claim() future dropped while waiting (queued, or woken but not yet polled) withdraws as well.
• ClaimGuard declares _guard before _registration, so the mutex is released before the registration is withdrawn. The other order would not break exclusion, since the holder's critical section is already over by then. It would only let a newcomer get a fresh mutex while the old holder is still running its destructor, so this order is the tidy one.
• If Registration::drop finds its entry missing (an invariant break), it logs an error as well as hitting a debug_assert, so a release build does not ignore it silently.

The module docs now describe the count and why strong_count wasn't reliable.

entry_survives_while_a_waiter_is_queued no longer sleeps 50ms and hopes. It waits (with yield_now, bounded by a 10s timeout that fails rather than hangs) until the waiter is actually registered, so it proves what its name says.

Also: only well-formed PaymentIntent ids are accepted, and the retrieved PaymentIntent must be the one requested.

Tests

• waiter_cancelled_after_holder_released_does_not_leak: a waiter is polled once, the holder releases, then the waiter is dropped without being polled again. It fails deterministically on the old code (leak path 2): 5 of 5 runs against the origin/main implementation. It passes with the fix.
• cancelled_waiter_does_not_leak_its_key: a waiter is dropped (by timeout) while still queued, before the holder releases. It also passes on the old code, because the dropped waiter's Arc goes away before the holder checks the count. It does not demonstrate the old bug. It guards the new design's cancellation contract: the registration must be withdrawn when the future is dropped.
• To check that both cancellation tests can fail against the new design, I made the registration non-RAII in a scratch edit (mem::forget before the await, recreated after). Both tests failed. Then I reverted the edit.
• Unit tests for the id format check. Hermetic tests show that /sign-certificate, /update-donation and /check-payment-status return 400 for malformed ids without Stripe configured; with the checks disabled in a scratch edit, those tests failed. Source-order pins check that the format check comes before the claim and the Stripe retrieval, and that the id match comes before the mark.

Evidence (handoff race)

Stress runs of the payment_claim tests binary (--test-threads=8), many processes in parallel on a 16-core box so threads get preempted inside the gap:

code	parallel	runs	failures
before (origin/main 436c663)	48	4000	134
before, second run	48	4000	115, every one payment_claim.rs:218 "entry outlived the last guard for this key"
after (95cbf49)	48	4000	0
after (95cbf49)	64	10000	0
after (8341348)	48	2000	0

A scratch in-process test that runs 2000 holder/waiter handoffs per run (not committed: slow and probabilistic) leaked in 2 of 5 runs before the fix and 0 of 10 after. The failure reproduced on unmodified code without any sleep or yield injected.

At 780e022 (current head), cargo test -p ghostkey-api passes (62 passed, 1 ignored as before), and cargo fmt -p ghostkey-api -- --check is clean.

https://claude.ai/code/session_01C7ph5zmqbaZdYnnuuNjGnv

[AI-assisted - Claude]

[sanity]

Comprehensive PR Review: #194

• Review tier: Full (concurrency in the payment path)
• Reviewers run:
  • Round 1 (95cbf49f): skeptical; testing plus code-first; Codex; Gemini 3.1 Pro via AI Gateway ($0.25).
  • Round 2 (83413488): skeptical; Codex.
  • Final commit 780e022a: diff-of-diff check.
• HEAD SHA reviewed: 780e022a. CI green there (build, test, CLA).

Findings

The claimant-count design is correct on every lens.

• Mutual exclusion holds. An entry exists while any registration for its key is alive, and the mutex guard drops before the registration does.
• Cancellation cleans up. A claim is withdrawn whether it's dropped queued, woken but not yet polled, or after acquiring.
• Stress results. A scratch stress harness (8 workers, cancellations and aborts) found 0 exclusion violations and 0 leaks in 40 runs. An off-by-one mutant violated exclusion on every run, so the detector works. The PR's own stress loop had 0 failures in 16,000+ runs after the fix, against 115–134 per 4,000 before.

Review items, all addressed in this PR:

• Tests:
  • cancelled_waiter_does_not_leak_its_key also passes on the old code. The PR body now says so, and that waiter_cancelled_after_holder_released_does_not_leak fails every time on the old code (a second leak path, also fixed).
  • entry_survives_while_a_waiter_is_queued now waits deterministically for the waiter to register, instead of sleeping 50ms.
• Robustness: Registration::drop logs, as well as debug_asserts, if its invariant ever breaks. The field-order comment was corrected: that order is tidy, not safety-critical.
• Input hardening:
  • Only well-formed PaymentIntent ids are accepted: pi_ plus ASCII alphanumerics, at most 255 bytes, checked before any claim or Stripe call.
  • The retrieved PaymentIntent must be the one requested before it is marked.
  • Malformed ids on /update-donation and /check-payment-status now get 400 "Invalid payment reference" instead of "Invalid currency".

Dismissed

• The retrieved-id check is pinned by source order only, since testing it by behaviour would need a Stripe mock. The source pin is non-vacuous (production source only).

Verdict

State: Ready to Merge
HEAD SHA reviewed: 780e022a

[AI-assisted - Claude]