Hardening: atomic quota reservation + idempotent deposit credit - #619
Hardening: atomic quota reservation + idempotent deposit credit#619jjohare wants to merge 3 commits into
Conversation
saveQuota was made atomic (temp + rename) to stop torn reads (JavaScriptSolidServer#309), but enforcement was still three separate async steps: checkQuota, then storage.write, then updateQuotaUsage. Two concurrent writers could both pass the check before either recorded its usage, overshooting the pod limit; and the read-modify-write in updateQuotaUsage could lose updates outright. Add reserveQuota — check-and-commit under a per-pod async lock — and route the PUT/POST write paths through it, releasing the reservation if the write fails. updateQuotaUsage now takes the same lock, fixing the lost-update on the delete/shrink path. checkQuota is kept as a cheap lock-free pre-check. Found while hardening the Rust parity port solid-pod-rs (v0.5.0-alpha.8), which added QuotaPolicy::reserve for the same reason.
The GET /pay/.balance auto-scanner credited the ledger (writeLedger) and then, in a separate write, recorded the funding UTXO as seen (saveUtxos). A crash between the two writes lost the seen-record; because the scanner re-runs on every balance poll, the same on-chain UTXO was then credited again on the next request — minting balance from nothing. Record each deposit's idempotency key (chain:txid:vout) inside the ledger via creditOnce, so the balance increment and the "already counted" marker commit together in the single authoritative writeLedger. saveUtxos becomes an advisory cache: losing it can no longer double-credit. Legacy ledgers gain the `credited` array on read. Found while hardening the Rust parity port solid-pod-rs (v0.5.0-alpha.8), which consolidated ledger + replay-set + order-book + exchange into one atomic state.json commit for the same reason.
|
|
There was a problem hiding this comment.
Pull request overview
This PR hardens two independent concurrency/crash-safety weaknesses inherited from the codebase, discovered via the solid-pod-rs parity port. First, it makes per-pod storage-quota enforcement atomic by serializing the load→check→commit sequence under a per-pod async lock (reserveQuota), so concurrent writers can no longer both pass a stale check and overshoot the limit, and lost updates in updateQuotaUsage are eliminated. Second, it makes deposit crediting idempotent by recording each deposit's outpoint key inside the ledger itself (creditOnce), so a crash between the ledger write and the UTXO cache write can no longer re-credit the same on-chain deposit on the next balance poll. Both changes are additive (no API/schema breaks) and ship with new regression tests.
Changes:
- Add
reserveQuota+ a per-pod lock instorage/quota.js; routePUT/POSTwrite paths through it with release-on-failure, and take the same lock insideupdateQuotaUsage. - Add
creditOnceinwebledger.jswith a back-compatiblecreditedarray migration inreadLedger; use it in thepay.jsbalance auto-scanner. - Add regression tests for atomic quota reservation and idempotent deposit crediting.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/storage/quota.js | Adds per-pod lock + reserveQuota (atomic check-and-commit); wraps updateQuotaUsage in the lock. |
| src/handlers/resource.js | handlePut reserves growth atomically and releases the reservation on write failure. |
| src/handlers/container.js | handlePost reserves before write and releases on failure. |
| src/webledger.js | Adds creditOnce idempotency helper and a credited array migration in readLedger. |
| src/handlers/pay.js | Balance auto-scanner uses creditOnce keyed on chain:txid:vout; only counts credited deposits. |
| test/quota-reserve.test.js | New tests: no overshoot, no lost updates, released reservations reusable. |
| test/webledger-credit-once.test.js | New tests for creditOnce idempotency and replay no-op. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| utxos.push({ txid: u.txid, vout: u.vout, amount: u.value, scriptpubkey, chain: chainId, tweak: didUri, spent: false }); | ||
| credited += u.value; | ||
| if (didCredit) credited += u.value; |
| it('the credited marker survives a ledger read migration', async () => { | ||
| // A legacy ledger without a `credited` array must gain one on read so the | ||
| // idempotency guard works on pre-existing deposits. | ||
| const legacy = { entries: [], name: 'x' }; | ||
| const parsed = JSON.parse(JSON.stringify(legacy)); | ||
| // readLedger performs the migration for on-disk ledgers; emulate its shape | ||
| // guard here without hitting storage. | ||
| if (!parsed.credited) parsed.credited = []; | ||
| const r1 = creditOnce(parsed, KEY, DID, 100, 'tbtc4'); | ||
| const r2 = creditOnce(parsed, KEY, DID, 100, 'tbtc4'); | ||
| assert.strictEqual(r1.credited, true); | ||
| assert.strictEqual(r2.credited, false); | ||
| // Keep readLedger referenced so the import is meaningful to linters. | ||
| assert.strictEqual(typeof readLedger, 'function'); |
| * checkQuota is retained as a cheap, lock-free cooperative pre-check; this is | ||
| * the authoritative enforcement point. Mirrors QuotaPolicy::reserve in the |
Review follow-ups on the hardening branch. 1. fix(pay) — the saveUtxos call was gated on `credited > 0`, but in the crash-then-rescan case this branch exists to fix, creditOnce returns didCredit=false, so nothing was credited and the cache write was skipped. The appended outpoint was then never persisted and the next GET /pay/.balance re-queried the explorer for it — repeating on every poll until an unrelated deposit happened to trigger a save. Gate saveUtxos on a new `appended` flag instead, so the cache self-repairs. Ledger-first ordering is kept: a crash between the two writes must leave the credit recorded (no double-credit), never the reverse (under-credit). 2. test(webledger) — 'the credited marker survives a ledger read migration' never called readLedger; it re-implemented the guard inline and asserted typeof readLedger === 'function'. Since creditOnce also self-heals the field, the test passed regardless of the migration. Replaced with two tests that go through real storage: a legacy ledger gains credited: [] on read (asserted before any creditOnce call), and a persisted marker survives a write/read round trip. Verified: deleting the migration line from readLedger now fails the first test. 3. webledger — createLedger seeds credited: [] so a new ledger has the same shape the migration produces for a legacy one. 4. quota — the reserveQuota docblock described checkQuota as a retained pre-check, but no production caller invokes it after this change. Reworded so nobody hunts for a pre-check that is not there. Full suite: 1065/1065 pass.
melvincarvalho
left a comment
There was a problem hiding this comment.
Nice work — both defects are real, I reproduced the reasoning in each case, and the provenance table (including the three candidates that don't apply) is exactly the right way to send a cross-port fix. Taking this.
I've pushed four follow-ups straight to the branch (3160082) since you'd offered to adjust to our conventions — shout if you'd rather I'd left them as comments. Everything below is either already pushed or flagged as your call.
Pushed
1. saveUtxos was gated on the wrong thing (this one's a real regression, and Copilot caught it too)
In the exact crash-then-rescan case this branch fixes, creditOnce returns didCredit: false — so credited stays 0, the if (credited > 0) guard skips both writes, and the outpoint just pushed onto utxos is never persisted. The next GET /pay/.balance re-queries the explorer for it and repeats, on every poll, until an unrelated deposit happens to trigger a save. Before this branch credit was unconditional, so the cache always absorbed it.
The balance stays correct throughout — the double-credit fix genuinely works — so this is a self-healing/efficiency defect rather than a money bug. But it's a polled payments endpoint, so it compounds. Now gated on a separate appended flag. I kept your ledger-first ordering deliberately: a crash between the two writes must leave the credit recorded (no double-credit), never the reverse (under-credit) — same asymmetry you flagged for the one-shot paths.
Worth noting writeLedger is still correctly skipped when nothing was credited: creditOnce doesn't mutate the ledger on the replay path.
2. The migration test was vacuous
'the credited marker survives a ledger read migration' never called readLedger — it re-implemented the guard inline and closed with assert.strictEqual(typeof readLedger, 'function'). It's slightly worse than it looks: creditOnce itself does if (!ledger.credited) ledger.credited = [], so even the inline line was redundant and the test would have passed with the migration deleted outright.
Replaced with two tests that go through real storage (temp DATA_ROOT): a legacy ledger gains credited: [] on read — asserted before any creditOnce call, since that would self-heal it — and a persisted marker survives a write/read round trip, which is the actual cross-restart replay case. Verified by deleting the migration line from readLedger: the first test now fails with readLedger must add a credited array to a legacy ledger.
3. createLedger now seeds credited: [] — every other spec field is initialized there, and this is why the old test could pass without the migration.
4. Reworded the reserveQuota docblock. It described checkQuota as "retained as a cheap, lock-free cooperative pre-check", but after this change no production caller invokes it — handlePut and handlePost both call only reserveQuota. Didn't want anyone hunting for a pre-check that isn't there.
Your call — not pushed
reserveQuota largely duplicates checkQuota. Both do loadQuota → init-limit-from-default → limit === 0 bail → projectedUsage → the same MB-formatted error string, verbatim. The delta is the lock wrapper and the commit. That duplication is really the root of #4 above: checkQuota became dead production code.
Factoring the evaluation once — say evaluateQuota(quota, bytes) → {allowed, error}, used lock-free by checkQuota and inside withPodLock by reserveQuota — would drop ~20 lines and one duplicated string. I didn't do it because there's a genuine semantic difference to resolve first, and it's your design: reserveQuota guards the limit check with additionalBytes > 0 while checkQuota doesn't, so an already-over-limit pod is denied by one and allowed by the other at bytes === 0. quota-race.test.js calls checkQuota(POD, 0, …), so that isn't purely academic. checkQuota is internal (nothing in docs/, README, or src/index.js), so deleting it outright is also on the table.
ledger.credited is unbounded. Nothing prunes it — compact() only filters zero-balance entries. Every deposit key (~76 bytes) persists forever in a file parsed and rewritten on every balance poll, and includes() is an O(n) scan over it per new UTXO. Low urgency (10k deposits ≈ 760KB) but it only grows.
There's a tidy bound available, and #1 above unlocks it: the scanner short-circuits on the cache (if (utxos.find(…)) continue), so a credited key is only load-bearing until its outpoint is durably in the utxos cache. Once the cache reliably absorbs appended outpoints, keys present in it are dead weight and could be pruned. The tradeoff is that credited is then the only protection if the cache file is ever lost, so it's a real design decision rather than a free win — worth its own issue rather than scope creep here.
Notes
- Full suite is green on my run: 1065/1065. Your pre-existing
idp-login-errorfailure didn't reproduce here, so it may be environment-dependent rather than a hard #514 break. - The
UNSTABLEstatus is justlicense/clapending, not the code. - Yes please to the follow-up PR for the one-shot credit paths (
pay.js~426/463/528) — samecreditOncetreatment, and the under-credit direction is the one worth getting right.
Hardening: atomic quota reservation + idempotent deposit credit
Two concurrency/crash-safety fixes, one commit each, with regression tests.
Provenance
These were found while hardening
solid-pod-rs— a Rust parity port that tracks JSS (currently 0.0.220). Its v0.5.0-alpha.8 release hardened several behaviours it had inherited from JSS; two of those defects still exist in the JavaScript today, so I'm sending the fixes back upstream. The port tracks parity in both directions, so fixes flow either way.I checked five hardening candidates from that release against
main. Three do not apply to JSS — reported here for completeness so you can see the search was scoped, not cherry-picked:checkQuota→write→updateQuotaUsageare unsynchronised; see belowJSS_STORAGE_TYPEto fall back fromauthorize()derives the WebID only from verified tokens (getWebIdFromRequestAsync); no request header is trusted as identityverifyDpopProofalready enforces ajtireplay cache (isJtiUsed/recordJti) with periodic cleanup1. Atomic quota reservation (
fix(quota))Failure scenario.
saveQuotais already atomic (temp-file + rename, #309), but enforcement is three separateawaits:checkQuota, thenstorage.write, thenupdateQuotaUsage. Two concurrentPUTs to the same pod both callcheckQuota, both see the pre-writeused, both pass, both write — the pod ends up over its limit. Independently, the read-modify-write insideupdateQuotaUsagecan lose increments: two concurrent updates read the sameused, and the later save clobbers the earlier one.The existing
quota-race.test.jsonly asserts concurrent updates don't throw and thatused > 0— it does not assert the limit is respected or that increments aren't lost, so this slipped through.Fix. Add
reserveQuota(pod, bytes, defaultQuota)— check-and-commit under a per-pod async lock — and route thePUT/POSTwrite paths through it, releasing the reservation (updateQuotaUsage(pod, -bytes)) if the write then fails.updateQuotaUsagenow takes the same per-pod lock, fixing the lost update on the shrink/delete path.checkQuotais retained as a cheap, lock-free cooperative pre-check. This mirrorsQuotaPolicy::reservein the Rust port.New test
quota-reserve.test.js: 40 concurrent 100-byte reservations against a 1000-byte limit admit exactly 10; 50 concurrent updates lose zero increments; a released reservation frees space for a later writer.2. Idempotent deposit credit (
fix(pay))Failure scenario.
GET /pay/.balanceauto-scans the user's tweaked address for new deposits. For each new UTXO it doescredit(ledger, …)thenwriteLedger(ledger), and separatelysaveUtxos(utxos)to record the UTXO as seen. If the process dies betweenwriteLedgerandsaveUtxos, the balance is credited but the seen-record is lost. Because the scanner re-runs on every balance poll, the nextGET /pay/.balancere-discovers the same on-chain UTXO and credits it again — balance minted from nothing, repeatable on each poll.Fix. Record each deposit's idempotency key (
chain:txid:vout) inside the ledger itself, via a newcreditOnce(ledger, key, uri, amount, currency). The balance increment and the "already counted" marker now commit together in the single authoritativewriteLedger;saveUtxosbecomes an advisory cache whose loss can no longer double-credit. Legacy ledgers gain thecreditedarray on read (back-compatible migration). This mirrors the Rust port's consolidation of ledger + replay-set + order-book + exchange into one atomicstate.jsoncommit.New test
webledger-credit-once.test.js: a replayed deposit key (the exact crash-then-rescan effect) is a no-op; distinct outpoints each credit once; the marker survives the read migration.Both changes are small and self-contained (no API or schema breaks;
creditedis additive).npm test: 1063/1064 pass — the single failure (idp-login-error.test.js, #514 form rendering) is pre-existing onmainand untouched by this branch; the 7 new tests all pass.Follow-up noted, deliberately not in this PR to keep it reviewable: the one-shot credit paths (
pay.js~426/463/528) writesaveUtxosbefore the ledger — the inverse ordering, whose crash window under-credits rather than double-credits. SamecreditOncetreatment applies; happy to send it as a second PR if this shape suits you.Happy to adjust naming/placement to match your conventions.
🤖 Generated by Claude Code
https://claude.ai/code/session_01MbuLJ4d2J7HaiYhzQfyGjy