test(memory): cron credit-gate 4-placed/3-removed flake is a test race, not a hold leak — drain settles deterministically; prove the stranded-hold TTL backstop

[2witstudios]

Verdict: TEST BUG, not an orphan-hold leak

credit-gate.integration.test.ts › "a funded user alongside an exhausted one…" failed once on CI (run 37394207810) with 4 holds placed, 3 removed (expectOneHoldPerCall, line 197). No hold is orphaned. The test read the hold audit while the evaluator's settle was still in flight.

Mechanism. The memory services settle fire-and-forget (discardUsageOutcome(AIMonitoring.trackUsage(...))), so POST /api/memory/cron can return before the evaluator's trackUsage has written its usage row. The old settled() polled ai_usage_logs and waited until "every usage row is applied". With only 3 rows written (all applied), it returned at once, and withHoldAudit read 4 placed / 3 removed while hold #4 was still live. When that settle runs, consumeCredits deletes the hold in the same transaction that marks its ledger row applied (packages/lib/src/billing/credit-consume.ts:187-192), so the hold is always removed. The test just looked too early.

Evidence

What	Result
Natural repro, old test, own PG17, 200 sequential + 4×60 parallel (separate DBs)	1 / 440 failed (seq run 35): same assertion, same line 197, 4 placed / 3 removed
Deterministic repro (RED, commit 06c2886): the evaluator's settle is held back until after settled() returns	5 / 5 failed with the exact CI signature
Fixed test, same 440-run loop (200 seq + 4×60 parallel)	440 / 440 passed (fseq 200/0, fpar1-4 60/0 each); afterwards 0 wallets, 0 holds, 0 usage rows and 0 drives left. The 2 remaining users are migration-seeded system users

Correction, in the interest of an honest record: my first GREEN commit (f9887fa) failed this loop 171/440. The cause was my own new test: it read the mid-settle state before the three discovery settles (also fire-and-forget) had finished. On failure it stranded the gated settle, and afterEach's user delete deadlocked (40P01) against it, leaving an in-debt user that polluted every later run. Fixed in 19e6b9b. The 440/440 above is from that head.

The fix (test-only, plus one behaviour-preserving extraction in lib for test isolation)

• apps/web/src/test/usage-settles.ts (new): captureUsageSettles() spies AIMonitoring.trackUsage and keeps every settle promise a run starts. drain() waits for all of them, including any started while waiting. An optional intercept lets a test hold one settle back.
• settled() now drains the captured settles, then checks the ledger once. No vi.waitFor and no polling. This is deterministic: every trackUsage is invoked before POST returns (each service calls it right after its awaited model call), so the drain sees all of them.
• afterEach drains settles before deleting rows, so cleanup can no longer race a settle on the wallet row (the 40P01 above).
• New test: "a settle that starts after the cron returns…". It holds the evaluator's settle back and asserts the exact CI state (3 usage rows, all applied, 1 hold live). Then it opens the gate and proves that hold is removed: 4/4, no live holds.
• Guard (would catch a real leak): "a call whose settle never takes its hold is reported…". It injects a real orphan (the evaluator's settle reports done without touching its hold). The usage rows alone look clean (3/3 applied); the hold audit reports exactly the one leaked hold, and that hold is still live in credit_holds.

No hold can leak for longer than its TTL, on any path

Path	Where the hold goes
Gate refuses	No hold is inserted. The hold is inserted only on allow, inside the gate transaction (credit-gate.ts:633, :818)
Gate throws	The transaction rolls back and no hold exists. reserveMemoryCall returns gate_error (apps/web/src/lib/memory/memory-credit.ts:76-82)
Model rejects	.catch → reservation.release() → releaseHold (memory-credit.ts:89-93). Covered by existing unit tests and by memory-credit-gate.integration.test.ts "a model call that throws releases its one hold"
Model resolves	trackUsage is invoked immediately. Every branch either settles (hold deleted in the settle transaction, credit-consume.ts:187-192; zero-charge :806; refused :710) or calls releaseHold (ai-monitoring.ts:1601,1614,1660,1682,1690)
Wallet-mismatch refusal (ai-monitoring.ts:1517-1528, returns without release)	Unreachable for memory: walletId comes from the same gate result as holdId. TTL backstop below
Process dies, is redeployed or freezes mid-call (the fire-and-forget window)	TTL backstop. See next section

Safety net for a stranded hold (orchestrator's question)

• Expiry: every hold gets expiresAt = now + CREDIT_HOLD_TTL_SECONDS (packages/lib/src/billing/credit-pricing.ts:394, default 900 s; credit-core.ts:282; set at credit-gate.ts:313/339, :541/633, :705/818).
• An expired hold stops reserving immediately, before any sweep. Every reader sums only expiresAt > now: the gate (credit-gate.ts:321, :582, :788), the balance (credit-balance.ts:213), concurrency (live-concurrency-query.ts:26). The worst case is credits tied up for ≤15 min.
• Sweeper: backfillCredits() deletes expiresAt < now (packages/lib/src/billing/credit-backfill.ts:78-82). It is called by GET /api/cron/reconcile-credits (apps/web/src/app/api/cron/reconcile-credits/route.ts:32), scheduled */10 * * * * (docker/cron/crontab:88).
• Money invariant: a hold never moves money. It is a reservation, and the wallet is debited only by the settle. Expiry and sweep leave wallets and credit_ledger untouched.
• Real-PG test (memory-credit-gate.integration.test.ts, new): reserve via the real reserveMemoryCall and never settle or release. Age the hold past its expiry (the row is aged rather than the clock moved). Then: reserved 0 and spendable = full wallet before any sweep; the cron's own sweep removes it; the wallet is unchanged and the ledger is empty.
• Isolated on the shared CI DB (Codex thread, ff2f937): the expiry sweep is extracted from backfillCredits into sweepExpiredHolds({ now?, userIds? }) (packages/lib/src/billing/credit-backfill.ts). The cron still calls it unscoped, so production behaviour is unchanged. The test calls the same function scoped to its user, so it touches neither other workers' holds nor any pending/orphan usage reconciliation. A bystander user's expired hold is asserted to survive the sweep. An empty scope sweeps nothing rather than widening to every hold.

Separate finding, not fixed here (outside this leaf): if the process dies after the provider call but before writeAiUsage, no usage row exists, so that call is unbilled. That is revenue PageSpace loses; the user is never over-charged, and their reservation clears within the TTL. Awaiting the memory settles inside each service would narrow that window (the cron runs on the long-lived web server via cron-curl, not serverless), but it changes the cron's control flow. I'm flagging it for the orchestrator to decide.

Mutation checks (clean committed tree, git restore after each, git status clean)

1. apps/web/src/lib/memory/integration-service.ts line 372: holdId: reservation.holdId, → holdId: undefined,. The cron file goes 3 failed | 3 passed (6), with "every hold placed is removed" red in the funded test, the late-settle test and the compaction test. Restored.
2. packages/lib/src/billing/credit-backfill.ts line 81 (on ff2f937, inside sweepExpiredHolds): const expired = lt(creditHolds.expiresAt, now) → lt(creditHolds.expiresAt, new Date(0)), lib rebuilt. The memory integration file goes 1 failed | 3 passed (4) ("expected +0 to be 1") and credit-backfill.test.ts goes 1 failed | 17 passed (18). Restored, git status clean, rebuilt, 4/4 green.

Tests (DATABASE_URL = own throwaway PG17)

bun run --filter web test -- src/app/api/memory/cron/__tests__/credit-gate.integration.test.ts    → Tests 6 passed (6)
bun run --filter web test -- src/lib/memory/__tests__/memory-credit-gate.integration.test.ts       → Tests 4 passed (4)
bun run --filter web test -- src/lib/memory/__tests__/memory-credit.test.ts                        → Tests 7 passed (7)
bun run --filter web test -- src/lib/memory/__tests__/discovery-service.test.ts                    → Tests 16 passed (16)
bun run --filter web test -- src/lib/memory/__tests__/integration-service.test.ts                  → Tests 11 passed (11)
bun run --filter web test -- src/lib/memory/__tests__/compaction-service.test.ts                   → Tests 13 passed (13)
bun run --filter @pagespace/lib test -- src/billing/__tests__/credit-backfill.test.ts             → Tests 18 passed (18)
bun run --filter @pagespace/lib test -- src/billing/__tests__/credits-flow.integration.test.ts    → Tests 58 passed (58)
bun run --filter @pagespace/lib test -- src/monitoring/__tests__/ai-monitoring.test.ts             → Tests 124 passed (124)
bun run --filter web test -- src/app/api/cron/reconcile-credits/__tests__/route.test.ts            → Tests 8 passed (8)
NODE_OPTIONS=--max-old-space-size=8192 bunx tsc --noEmit -p packages/lib; echo exit=$?            → exit=0
NODE_OPTIONS=--max-old-space-size=8192 bunx tsc --noEmit -p apps/web; echo exit=$?                → exit=0
bunx eslint <3 touched files>                                                                       → exit 0

Requirement IDs

Requirement IDs: none. This PR fixes a test race and proves the existing hold invariants; it implements no Spec requirement and leaves scripts/spec-coverage-allowlist.txt untouched.

Test	Guards
a settle that starts after the cron returns…	the CI flake: the audit waits for late settles, and a late hold is removed
guard: a call whose settle never takes its hold…	a real orphan is reported (gate self-test, no Spec ID)
a reservation whose settle never runs…	TTL expiry and the reconcile sweep free a stranded memory hold; money invariant intact; the scoped sweep leaves a bystander's hold
sweepExpiredHolds › the cron sweep is unscoped / scoped deletes only those users' / empty scope deletes nothing / billing off / failed delete	the extraction keeps the cron's sweep identical and the scope cannot widen

Callers of changed functions (rule 13)

Production functions changed: backfillCredits now delegates its expiry sweep to the new exported sweepExpiredHolds (same query, same emit, same never-throw). Its one production caller, apps/web/src/app/api/cron/reconcile-credits/route.ts:32, sees an identical BackfillResult; reconcile-credits/__tests__/route.test.ts (8/8) and credits-flow.integration.test.ts (58/58) are unchanged and green. sweepExpiredHolds has two callers: backfillCredits (unscoped) and the memory stranded-hold test (scoped). Test helpers: settled() is local to credit-gate.integration.test.ts (4 call sites, all in that file, all now drain-based). captureUsageSettles is new, used only in that file. withHoldAudit (apps/web/src/test/hold-audit.ts) is unchanged; its other caller, memory-credit-gate.integration.test.ts, waits on a known row count and is unaffected.

No user-visible change, so no changelog entry.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LGdDqDdYLahEfRrcBTb5T8

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 65cae6a0-c585-4690-9311-d42995955fb9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-06T02:24:19.134616Z	277801a	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 277801af7f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[2witstudios]

[independent-review] agent:irv-2843

Head reviewed: 277801a (base pu/org-wallets). P1: 0. P2: 0. Mergeable: yes. CI is green.

1. Verdict: TEST BUG. Confirmed.

• Memory services. Discovery (discovery-service.ts:355), integration (integration-service.ts:367) and compaction (compaction-service.ts:154) each call AIMonitoring.trackUsage synchronously, right after the awaited model call and before the service returns. So every settle is started before POST returns. A model rejection calls reservation.release() → releaseHold.
• Settle paths in trackUsage.
  • A successful settle deletes the hold in the consume transaction (credit-consume.ts:192).
  • The release branches (ai-monitoring.ts:1601/1614/1660/1682/1690) call releaseHold.
  • The wallet-mismatch refusal (:1517-1528) and a deferred consume (:1626-1654) leave the hold in place. Expiry and the sweep handle it.
  • A hold is a reservation, not a debit, so it can't be counted twice. A late settle after the sweep only deletes a missing row.
• Readers of credit_holds. I grepped every .from(creditHolds) and every raw credit_holds in non-test source. Every aggregate that reserves funds or counts in-flight work filters expiresAt > now:
  • credit-gate.ts:321/582/788
  • credit-balance.ts:213
  • live-concurrency-query.ts:26
  • consumer-caps.ts:100
  • seat-allowance.ts:186
  • spend-resolution.ts:130
  • wallet-funding-shell.ts:673
  • drive-wallet-service.ts:210/545
  • web and admin monitoring-queries (expiresAt > NOW())
  • holdsInScope (person-bound-scope.ts) is only used inside the expiry-filtered gate queries.
• Other references, none of which reserve funds:
  • by-id lookups during settle (credit-consume.ts:312/639)
  • diagnostic counts (packages/db wallet-backfill.ts)
  • the GDPR export table list
  • e2e helpers
• No unfiltered reader exists, so there is no P1.

2. The fix is deterministic. Confirmed.

• No timing. settled() now drain()s the captured trackUsage promises; drain loops until no new ones appear, then checks the ledger once. There are no sleeps and no vi.waitFor. afterEach drains before deleting rows.
• Loop. I ran the cron file 120 times in a row on my own throwaway PG17 (fresh initdb, all migrations): 120/120 green. Afterwards the DB had 0 holds, 0 wallets and 0 usage rows.
• RED reproduction. I ran the RED commit 06c2886 5 times: 5/5 red, at expectOneHoldPerCall line 199, with 4 placed and 3 removed. That is the CI signature.
• Own mutation on HEAD. Removing await settles?.drain() from settled() turns "a settle that starts after the cron returns…" red 3/3 times. The guard test also went red once.

3. The stranded-hold test proves expiry, sweep and the money invariant. Confirmed.

• Sweep mutation. I changed credit-backfill.ts:81 from lt(creditHolds.expiresAt, now) to new Date(0) and rebuilt lib. The test went red: "expected 0 to be greater than or equal to 1". After restoring and rebuilding: 4/4 green.
• Extra mutation (expiry filter). I changed credit-balance.ts:213 to ignore expiry. The test went red at "reserved is 0 before any sweep" (expected 2 to be 0). So it proves that expiry frees the hold before the sweep, not only that the sweep runs.
• Money invariant. The test asserts the wallet is unchanged, there are no credit_ledger rows for the user, and spendable/reserved are correct after the sweep.

4. Test-only. Confirmed.

The diff touches 3 files, all test code:

• credit-gate.integration.test.ts
• memory-credit-gate.integration.test.ts
• the new apps/web/src/test/usage-settles.ts

No production lines changed.

5. CI

All checks pass on 277801a:

• Lint & TypeScript
• Unit Tests
• E2E (agent-session user stories)
• CodeRabbit (skipped for this base)

No check failed, so nothing was re-run and no red check needed attributing.

Non-blocking nits (not P2)

• Global sweep in a test. The stranded-hold test calls the global backfillCredits(). It also sweeps or reconciles other suites' rows if they share the DB. The 5-minute grace window keeps that harmless for fresh rows, and the expired-hold sweep is a no-op for live holds. I'm noting it only for future suites that create back-dated rows.
• Reproducing locally. The scratchpad path is too long for a Unix socket. I ran postgres with a relative -k (socket inside the data dir) plus TCP localhost.

[2witstudios]

[independent-review] agent:irv-2843-r2

Delta review of 277801a..ff2f937 (1 commit, 3 files). This head changes production code: packages/lib/src/billing/credit-backfill.ts (+30/-15). The other two files are tests. New counts: P1 0, P2 0. Mergeable: yes. CI is 5/5 green on ff2f937 (Lint & TypeScript, Unit Tests, E2E, Spec ID coverage, CodeRabbit). No check ended with 0 steps, so nothing was re-run.

1. Production behaviour is unchanged. Confirmed.

The sweep is extracted, not changed. backfillCredits still:

1. returns zeros when billing is off;
2. computes now and cutoff = now - GRACE_MS;
3. sweeps first, via sweepExpiredHolds({ now }) with no userIds. That is the same DELETE … WHERE expiresAt < now, the same now, the same returning and the same per-user emitBalancesBestEffort (still void).

The rest also matches the old code:

• Errors. Same try/catch and warn. A failure still gives expiredHolds = 0 and the cron continues.
• Transactions. Before and after, the sweep is a standalone db.delete outside any transaction.
• Everything after the sweep. The pending and orphan reconciliation is byte-identical in the diff.
• Billing check. isBillingEnabled() is now checked twice, which is harmless.
• Callers. git grep finds one production caller of backfillCredits: apps/web/src/app/api/cron/reconcile-credits/route.ts:32. sweepExpiredHolds has no production caller other than backfillCredits.

2. Scoping is safe. Confirmed.

• userIds: [...] → and(lt(expiresAt, now), inArray(userId, userIds)). It can only delete those users' expired holds.
• userIds: [] → returns 0 before any query, so an empty array sweeps nothing.
• userIds undefined → unscoped lt(expiresAt, now), which is the cron's behaviour.

Mutations on my own PG17 (lib rebuilt each time, restored after each):

Mutation	Lib unit test	Real-PG test
(a) credit-backfill.ts:81 cutoff → new Date(0)	red	red (expected 0 to be 1)
(b) scope dropped (.where(expired) always)	red	red, bystander swept (expected 2 to be 1)
(c) empty-array guard removed	red ("scoped to no users, deletes nothing")	green (expected: its scope is non-empty)
(d) cron passes userIds: []	red, 2 tests (the cron sweep must stay unscoped)	green

3. The test is isolated and still proves the real sweep path. Confirmed.

• Isolation. The real-PG test now inserts an expired hold for a bystander user and sweeps scoped to its own user. It asserts exactly 1 deleted, its own hold gone and the bystander's hold still there. Mutation (b) proves the bystander assertion bites.
• Real path. It calls the same sweepExpiredHolds that backfillCredits uses, and the unit test pins that the cron calls it unscoped. Mutation (a) on line 81 still turns it red.
• Unchanged assertions. The expiry-before-sweep check (reserved 0) and the money invariant (wallet unchanged, no ledger rows) are as in r1.
• Stability. 20/20 green in a loop. Teardown removes the bystander's rows, and the DB had 0 holds and 0 wallets afterwards.

4. Local runs

• tsc --noEmit -p apps/web → exit 0; tsc --noEmit -p packages/lib → exit 0, both on a clean tree at ff2f937.
• memory-credit-gate integration 4/4, cron credit-gate integration 6/6, lib credit-backfill unit 18/18.

Non-blocking nit

• Unused now option. The now option on sweepExpiredHolds is only passed by backfillCredits and the unit tests. That's fine, but a scoped caller that passes a future now would sweep live holds. The option is not reachable from any route.