Skip to content

Commit e7e0525

Browse files
Hardening: atomic quota reservation + idempotent deposit credit (#619)
* fix(quota): atomic reserve to close the check-and-commit race saveQuota was made atomic (temp + rename) to stop torn reads (#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. * fix(pay): idempotent deposit credit to close a double-credit window 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. * fix(pay): persist the utxo cache when an outpoint is appended 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. --------- Co-authored-by: Melvin Carvalho <melvincarvalho@gmail.com>
1 parent f9f7a4d commit e7e0525

7 files changed

Lines changed: 357 additions & 20 deletions

File tree

‎src/handlers/container.js‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import * as storage from '../storage/filesystem.js';
2-
import { initializeQuota, checkQuota, updateQuotaUsage } from '../storage/quota.js';
2+
import { initializeQuota, reserveQuota, updateQuotaUsage } from '../storage/quota.js';
33
import { getAllHeaders } from '../ldp/headers.js';
44
import { isContainer, getEffectiveUrlPath, getPodName } from '../utils/url.js';
55
import { generateProfile, generatePreferences, generateTypeIndex, serialize } from '../webid/profile.js';
@@ -159,18 +159,20 @@ export async function handlePost(request, reply) {
159159

160160
// Check storage quota before writing (skip in public mode - no pod structure)
161161
const podName = request.config?.public ? null : getPodName(request);
162+
// Atomically reserve before writing so concurrent creates can't both pass
163+
// the check and overshoot the limit; release the reservation on failure.
162164
if (podName) {
163-
const { allowed, error } = await checkQuota(podName, content.length, request.defaultQuota || 0);
165+
const { allowed, error } = await reserveQuota(podName, content.length, request.defaultQuota || 0);
164166
if (!allowed) {
165167
return reply.code(507).send({ error: 'Insufficient Storage', message: error });
166168
}
167169
}
168170

169171
success = await storage.write(newStoragePath, content);
170172

171-
// Update quota usage after successful write
172-
if (success && podName) {
173-
await updateQuotaUsage(podName, content.length);
173+
// reserveQuota already recorded the usage; release it if the write failed.
174+
if (!success && podName) {
175+
await updateQuotaUsage(podName, -content.length);
174176
}
175177
}
176178

‎src/handlers/pay.js‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727

2828
import crypto from 'crypto';
2929
import { getNostrPubkey, pubkeyToDidNostr } from '../auth/nostr.js';
30-
import { readLedger, writeLedger, getBalance, credit, debit } from '../webledger.js';
30+
import { readLedger, writeLedger, getBalance, credit, creditOnce, debit } from '../webledger.js';
3131
import { verifyMrc20Deposit, verifyMrc20Anchor, jcs, btAddress } from '../mrc20.js';
3232
import { loadTrail, transferToken, buildTransaction, broadcastTx, p2trScript, btDeriveChainedPrivkey } from '../token.js';
3333
import { secp256k1 } from '@noble/curves/secp256k1';
@@ -319,6 +319,7 @@ export function createPayHandler(options = {}) {
319319
const utxos = await loadUtxos();
320320
const ledger = await readLedger();
321321
let credited = 0;
322+
let appended = false;
322323

323324
for (const chainId of payChains) {
324325
const chain = CHAIN_REGISTRY[chainId];
@@ -341,14 +342,29 @@ export function createPayHandler(options = {}) {
341342
}
342343
} catch { /* best effort */ }
343344
const currency = chain.unit;
344-
credit(ledger, didUri, u.value, currency);
345+
// Idempotent credit keyed on the outpoint: the balance and the
346+
// "already counted" marker commit together in the ledger, so a
347+
// crash before saveUtxos below can't double-credit on the next
348+
// balance poll (the scanner re-runs on every GET /pay/.balance).
349+
const depositKey = `${chainId}:${u.txid}:${u.vout}`;
350+
const { credited: didCredit } = creditOnce(ledger, depositKey, didUri, u.value, currency);
345351
utxos.push({ txid: u.txid, vout: u.vout, amount: u.value, scriptpubkey, chain: chainId, tweak: didUri, spent: false });
346-
credited += u.value;
352+
appended = true;
353+
if (didCredit) credited += u.value;
347354
}
348355
}
349356

357+
// Ledger first: a crash before saveUtxos leaves the credit recorded
358+
// together with its idempotency key, so the rescan is a no-op rather
359+
// than a double-credit. The reverse order would under-credit.
350360
if (credited > 0) {
351361
await writeLedger(ledger);
362+
}
363+
// Keyed on `appended`, not `credited`: a replayed outpoint credits
364+
// nothing (didCredit === false) but must still land in the cache.
365+
// Otherwise the crash window never heals — the scanner re-queries the
366+
// explorer for that outpoint on every single balance poll.
367+
if (appended) {
352368
await saveUtxos(utxos);
353369
}
354370
} catch { /* scan failure is non-fatal */ }

‎src/handlers/resource.js‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import * as storage from '../storage/filesystem.js';
2-
import { checkQuota, updateQuotaUsage } from '../storage/quota.js';
2+
import { reserveQuota, updateQuotaUsage } from '../storage/quota.js';
33
import { getAllHeaders, getNotFoundHeaders } from '../ldp/headers.js';
44
import { generateContainerJsonLd, serializeJsonLd } from '../ldp/container.js';
55
import { isContainer, getContentType, isRdfContentType, getEffectiveUrlPath, safeJsonParse, getPodName } from '../utils/url.js';
@@ -1045,20 +1045,26 @@ export async function handlePut(request, reply) {
10451045
const oldSize = stats?.size || 0;
10461046
const sizeDelta = content.length - oldSize;
10471047

1048+
// Atomically reserve the growth before writing so concurrent writers can't
1049+
// both pass the check and overshoot the limit. reserveQuota commits the
1050+
// reservation, so we must release it if the write then fails.
10481051
if (podName && sizeDelta > 0) {
1049-
const { allowed, error } = await checkQuota(podName, sizeDelta, request.defaultQuota || 0);
1052+
const { allowed, error } = await reserveQuota(podName, sizeDelta, request.defaultQuota || 0);
10501053
if (!allowed) {
10511054
return reply.code(507).send({ error: 'Insufficient Storage', message: error });
10521055
}
10531056
}
10541057

10551058
const success = await storage.write(storagePath, content);
10561059
if (!success) {
1060+
if (podName && sizeDelta > 0) {
1061+
await updateQuotaUsage(podName, -sizeDelta); // release the reservation
1062+
}
10571063
return reply.code(500).send({ error: 'Write failed' });
10581064
}
10591065

1060-
// Update quota usage after successful write
1061-
if (podName && sizeDelta !== 0) {
1066+
// Growth was already recorded by reserveQuota; only a shrink needs recording.
1067+
if (podName && sizeDelta < 0) {
10621068
await updateQuotaUsage(podName, sizeDelta);
10631069
}
10641070

‎src/storage/quota.js‎

Lines changed: 92 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,35 @@ const QUOTA_FILE = '.quota.json';
1313
// between writeFile and rename) doesn't inflate pod usage.
1414
const QUOTA_TMP_PREFIX = `${QUOTA_FILE}.tmp.`;
1515

16+
/**
17+
* Per-pod serialization of read-modify-write on .quota.json.
18+
*
19+
* saveQuota is already atomic (temp file + rename), which stops a concurrent
20+
* reader from seeing a truncated file (#309). But atomicity of a single write
21+
* does not stop a lost update: two writers that both `loadQuota` the same
22+
* `used` value, each add their bytes and `saveQuota`, race to clobber one
23+
* another — and, worse, both can pass a `checkQuota` before either records its
24+
* usage, letting concurrent writes overshoot the limit. Serializing the whole
25+
* load→mutate→save critical section per pod closes both windows. This mirrors
26+
* QuotaPolicy::reserve in the solid-pod-rs parity port, which holds a per-pod
27+
* lock for atomic check-and-commit.
28+
*/
29+
const podLocks = new Map();
30+
31+
function withPodLock(podName, fn) {
32+
const prev = podLocks.get(podName) || Promise.resolve();
33+
// Chain onto the previous holder regardless of how it settled.
34+
const run = prev.then(fn, fn);
35+
// Store a tail that never rejects so the chain can't wedge on an error.
36+
const tail = run.then(() => {}, () => {});
37+
podLocks.set(podName, tail);
38+
// Opportunistic cleanup so the map doesn't grow unbounded across many pods.
39+
tail.then(() => {
40+
if (podLocks.get(podName) === tail) podLocks.delete(podName);
41+
});
42+
return run;
43+
}
44+
1645
/**
1746
* Get quota file path for a pod
1847
*/
@@ -168,19 +197,75 @@ export async function checkQuota(podName, additionalBytes, defaultQuota) {
168197
}
169198

170199
/**
171-
* Update quota usage after a write
200+
* Atomically reserve quota for a pending write: check-and-commit under a
201+
* per-pod lock so concurrent writers cannot both pass the check and then
202+
* overshoot the limit. On success the reserved bytes are already recorded in
203+
* `used`, so callers must NOT also call updateQuotaUsage for the same bytes —
204+
* instead, release the reservation with `updateQuotaUsage(pod, -bytes)` if the
205+
* write subsequently fails.
206+
*
207+
* This is the authoritative enforcement point and the only one the write path
208+
* uses. checkQuota remains exported as a lock-free read-only probe for callers
209+
* that want to inspect headroom without reserving it, but it performs no
210+
* pre-check for PUT/POST any more — do not go looking for one. Mirrors
211+
* QuotaPolicy::reserve in the solid-pod-rs parity port.
212+
*
213+
* @param {string} podName - The pod name
214+
* @param {number} additionalBytes - Bytes to reserve (expected >= 0)
215+
* @param {number} defaultQuota - Default quota limit
216+
* @returns {Promise<{allowed: boolean, quota: object, error?: string}>}
217+
*/
218+
export async function reserveQuota(podName, additionalBytes, defaultQuota) {
219+
return withPodLock(podName, async () => {
220+
let quota = await loadQuota(podName);
221+
222+
// Initialize limit from the default on first use, preserving any usage
223+
// reconciled from a recovered corrupt/empty file (see checkQuota).
224+
if (quota.limit === 0 && defaultQuota > 0) {
225+
quota = { limit: defaultQuota, used: quota.used };
226+
}
227+
228+
// No enforcement (and no tracking) when no limit is in effect — matches
229+
// updateQuotaUsage, which skips uninitialized quotas.
230+
if (quota.limit === 0) {
231+
return { allowed: true, quota };
232+
}
233+
234+
const projectedUsage = quota.used + additionalBytes;
235+
236+
if (additionalBytes > 0 && projectedUsage > quota.limit) {
237+
const usedMB = (quota.used / (1024 * 1024)).toFixed(2);
238+
const limitMB = (quota.limit / (1024 * 1024)).toFixed(2);
239+
return {
240+
allowed: false,
241+
quota,
242+
error: `Storage quota exceeded. Used: ${usedMB}MB / ${limitMB}MB`
243+
};
244+
}
245+
246+
// Commit the reservation as part of the same locked critical section.
247+
quota.used = Math.max(0, projectedUsage);
248+
await saveQuota(podName, quota);
249+
return { allowed: true, quota };
250+
});
251+
}
252+
253+
/**
254+
* Update quota usage after a write (or to release a reservation).
172255
* @param {string} podName - The pod name
173256
* @param {number} bytesChange - Bytes added (positive) or removed (negative)
174257
*/
175258
export async function updateQuotaUsage(podName, bytesChange) {
176-
const quota = await loadQuota(podName);
259+
return withPodLock(podName, async () => {
260+
const quota = await loadQuota(podName);
177261

178-
// Skip if no quota initialized
179-
if (quota.limit === 0) return quota;
262+
// Skip if no quota initialized
263+
if (quota.limit === 0) return quota;
180264

181-
quota.used = Math.max(0, quota.used + bytesChange);
182-
await saveQuota(podName, quota);
183-
return quota;
265+
quota.used = Math.max(0, quota.used + bytesChange);
266+
await saveQuota(podName, quota);
267+
return quota;
268+
});
184269
}
185270

186271
/**

‎src/webledger.js‎

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,11 @@ export function createLedger(options = {}) {
3434
defaultCurrency: options.defaultCurrency ?? 'satoshi',
3535
created: now,
3636
updated: now,
37-
entries: []
37+
entries: [],
38+
// Idempotency keys for deposits already absorbed into balances. Seeded
39+
// here so a freshly created ledger has the same shape readLedger's
40+
// migration produces for a legacy one.
41+
credited: []
3842
};
3943
}
4044

@@ -56,6 +60,11 @@ export async function readLedger(ledgerPath = DEFAULT_PATH) {
5660
if (!ledger.defaultCurrency) ledger.defaultCurrency = 'satoshi';
5761
if (!ledger.created) ledger.created = Math.floor(Date.now() / 1000);
5862
if (!ledger.entries) ledger.entries = [];
63+
// Idempotency keys for deposits already absorbed into balances. Kept in
64+
// the ledger itself so crediting is crash-safe: the balance and the "this
65+
// deposit was counted" marker commit together in one writeLedger, instead
66+
// of the balance landing in one file and the spent-marker in another.
67+
if (!ledger.credited) ledger.credited = [];
5968
return ledger;
6069
} catch {
6170
return createLedger();
@@ -164,6 +173,34 @@ export function debit(ledger, uri, amount, currency) {
164173
return { success: true, balance: newBalance };
165174
}
166175

176+
/**
177+
* Idempotently credit a keyed deposit exactly once.
178+
*
179+
* The dedup key (e.g. `${chain}:${txid}:${vout}` for an on-chain UTXO) is
180+
* recorded in the ledger's own `credited` list, so the balance increment and
181+
* the "already counted" marker are committed together by a single writeLedger.
182+
* A crash after that single write can never re-credit the same deposit — which
183+
* is exactly what happened when the credit and the UTXO-spent record lived in
184+
* two separate files written one after another.
185+
*
186+
* @param {object} ledger - WebLedger object (must be persisted afterwards)
187+
* @param {string} key - Stable idempotency key for this deposit
188+
* @param {string} uri - Agent URI
189+
* @param {number} amount - Amount to add
190+
* @param {string} [currency] - Currency code
191+
* @returns {{credited: boolean, balance: number}} `credited` is false when the
192+
* key was already absorbed (no change made)
193+
*/
194+
export function creditOnce(ledger, key, uri, amount, currency) {
195+
if (!ledger.credited) ledger.credited = [];
196+
if (ledger.credited.includes(key)) {
197+
return { credited: false, balance: getBalance(ledger, uri, currency) };
198+
}
199+
const balance = credit(ledger, uri, amount, currency);
200+
ledger.credited.push(key);
201+
return { credited: true, balance };
202+
}
203+
167204
/**
168205
* List all entries with non-zero balances
169206
* @param {object} ledger - WebLedger object

‎test/quota-reserve.test.js‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
/**
2+
* Regression tests for the quota check-and-commit race.
3+
*
4+
* saveQuota is atomic (temp + rename, #309), but check→write→record used to be
5+
* three separate async steps: two concurrent writers could both pass the check
6+
* and then overshoot the limit, and the read-modify-write in updateQuotaUsage
7+
* could lose updates. reserveQuota + a per-pod lock make check-and-commit
8+
* atomic — mirroring QuotaPolicy::reserve in the solid-pod-rs parity port.
9+
*/
10+
11+
import { describe, it, before, after, beforeEach } from 'node:test';
12+
import assert from 'node:assert';
13+
import fs from 'fs-extra';
14+
import os from 'os';
15+
import path from 'path';
16+
import {
17+
initializeQuota,
18+
updateQuotaUsage,
19+
reserveQuota,
20+
loadQuota
21+
} from '../src/storage/quota.js';
22+
23+
const POD = 'testpod';
24+
let TEST_ROOT;
25+
let originalDataRoot;
26+
27+
describe('quota — atomic reservation (check-and-commit race)', () => {
28+
before(async () => {
29+
originalDataRoot = process.env.DATA_ROOT;
30+
TEST_ROOT = await fs.mkdtemp(path.join(os.tmpdir(), 'jss-quota-reserve-'));
31+
process.env.DATA_ROOT = TEST_ROOT;
32+
await fs.ensureDir(path.join(TEST_ROOT, POD));
33+
});
34+
35+
after(async () => {
36+
if (originalDataRoot === undefined) delete process.env.DATA_ROOT;
37+
else process.env.DATA_ROOT = originalDataRoot;
38+
await fs.remove(TEST_ROOT);
39+
});
40+
41+
beforeEach(async () => {
42+
await initializeQuota(POD, 1000);
43+
});
44+
45+
it('concurrent reservations never overshoot the limit', async () => {
46+
// 40 writers each try to reserve 100 bytes against a 1000-byte limit.
47+
// Only 10 can be admitted; a non-atomic check would let many more through.
48+
const N = 40;
49+
const results = await Promise.all(
50+
Array.from({ length: N }, () => reserveQuota(POD, 100, 0))
51+
);
52+
53+
const admitted = results.filter((r) => r.allowed).length;
54+
assert.strictEqual(admitted, 10, 'exactly limit/size reservations may be admitted');
55+
56+
const final = await loadQuota(POD);
57+
assert.ok(final.used <= final.limit, `used ${final.used} must not exceed limit ${final.limit}`);
58+
assert.strictEqual(final.used, 1000, 'committed usage must equal the admitted reservations');
59+
});
60+
61+
it('concurrent updateQuotaUsage does not lose updates', async () => {
62+
await initializeQuota(POD, 10 * 1024 * 1024);
63+
const N = 50;
64+
const each = 100;
65+
await Promise.all(Array.from({ length: N }, () => updateQuotaUsage(POD, each)));
66+
67+
const final = await loadQuota(POD);
68+
assert.strictEqual(final.used, N * each, 'every increment must be recorded (no lost update)');
69+
});
70+
71+
it('a released reservation frees the space for a later writer', async () => {
72+
// Fill the quota, release one slot, then a new reservation must succeed.
73+
const filled = await Promise.all(
74+
Array.from({ length: 10 }, () => reserveQuota(POD, 100, 0))
75+
);
76+
assert.strictEqual(filled.filter((r) => r.allowed).length, 10);
77+
78+
const overflow = await reserveQuota(POD, 100, 0);
79+
assert.strictEqual(overflow.allowed, false, 'quota is full');
80+
81+
await updateQuotaUsage(POD, -100); // simulate a failed write releasing its reservation
82+
const retry = await reserveQuota(POD, 100, 0);
83+
assert.strictEqual(retry.allowed, true, 'freed space is reusable');
84+
});
85+
});

0 commit comments

Comments
 (0)