Fix intermittent 500 on concurrent PUTs via atomic quota writes (#309) by melvincarvalho · Pull Request #310 · JavaScriptSolidServer/JavaScriptSolidServer

melvincarvalho · 2026-04-23T02:04:36Z

Summary

Concurrent PUTs to the same pod race on .quota.json. fs.writeFile truncates before writing, so a second updateQuotaUsage can read an empty file — JSON.parse("") throws "Unexpected end of JSON input", which Fastify returns as a 500.
Fixes by writing to a temp file and atomically renaming (POSIX guarantee).
loadQuota now also tolerates empty/corrupt files (ENOENT or SyntaxError → treat as uninitialized) for defense-in-depth against already-corrupted files left behind by past races.

Why 91 bytes of body content?

The observed 500 response body on solid.social is exactly 91 bytes — Fastify's default error format with "Unexpected end of JSON input":

{"statusCode":500,"error":"Internal Server Error","message":"Unexpected end of JSON input"}

That is 91 bytes precisely, which matches the production log size.

Test plan

New test/quota-race.test.js reproduces the race: 200 interleaved save/read ops would fail JSON.parse on unpatched code (confirmed by reverting the saveQuota change — test fails with SyntaxError: Expected ',' or '}' after property value in JSON); passes with the fix.
Includes explicit tolerance tests for empty and partial-JSON quota files.
Full suite still passes (430/430).

Fixes #309

Concurrent PUTs to the same pod race on .quota.json. fs.writeFile truncates before writing, so a second updateQuotaUsage can read an empty file and JSON.parse throws "Unexpected end of JSON input", which Fastify returns as a 500. Fix: write to a temp file and rename (POSIX-atomic). Also tolerate empty/corrupt quota files on read as a self-healing fallback.

Copilot

Pull request overview

This PR addresses an intermittent 500 error during concurrent PUT requests by making .quota.json writes atomic and hardening quota loading against empty/corrupt files, with regression tests to reproduce the original race.

Changes:

Update saveQuota() to write to a temp file and rename() it into place to prevent torn reads.
Update loadQuota() to treat empty files and JSON parse failures as “uninitialized” quota state.
Add test/quota-race.test.js to reproduce the concurrent read/write failure mode and validate the fix.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`src/storage/quota.js`	Implements atomic quota writes and defensive loading behavior.
`test/quota-race.test.js`	Adds concurrency/race regression tests for quota file handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-23T02:09:28Z

+  before(async () => {
+    process.env.DATA_ROOT = TEST_ROOT;
+    await fs.emptyDir(TEST_ROOT);
+    await fs.ensureDir(path.join(TEST_ROOT, POD));
+    await initializeQuota(POD, 50 * 1024 * 1024);
+  });
+
+  after(async () => {
+    await fs.remove(TEST_ROOT);
+  });


This test mutates global process.env.DATA_ROOT but never restores it. Since the test runner executes test/*.test.js in one Node process, leaving DATA_ROOT pointed at data-quota-race-test can break subsequent tests (e.g., startTestServer() expects ./data). Capture the previous value in before() and restore (or delete) it in after().

Copilot · 2026-04-23T02:09:28Z

+    // one read would land on a truncated file and JSON.parse would throw.
+    const ops = [];
+    for (let i = 0; i < 200; i++) {
+      ops.push(saveQuota(POD, { limit: 1000, used: i }));
+      ops.push(fs.readFile(quotaPath, 'utf-8').then((data) => {
+        if (data.length === 0) return;


This test claims a concurrent read during saveQuota should "never see an empty file", but it currently treats data.length === 0 as success and skips the assertion/parse. That means a regression back to truncating writes could still pass if a read lands on an empty file. Consider failing the test on empty reads (or always JSON.parse the content) so it actually enforces the intended guarantee.

Suggested change

// one read would land on a truncated file and JSON.parse would throw.

const ops = [];

for (let i = 0; i < 200; i++) {

ops.push(saveQuota(POD, { limit: 1000, used: i }));

ops.push(fs.readFile(quotaPath, 'utf-8').then((data) => {

if (data.length === 0) return;

// one read would land on an empty/truncated file and this assertion or

// JSON.parse would fail.

const ops = [];

for (let i = 0; i < 200; i++) {

ops.push(saveQuota(POD, { limit: 1000, used: i }));

ops.push(fs.readFile(quotaPath, 'utf-8').then((data) => {

assert.notStrictEqual(

data.length,

0,

'concurrent read saw an empty quota file'

);

Copilot · 2026-04-23T02:09:28Z

+  const finalPath = getQuotaPath(podName);
+  const tmpPath = `${finalPath}.tmp.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}`;
+  await fs.writeFile(tmpPath, JSON.stringify(quota, null, 2));
+  await fs.rename(tmpPath, finalPath);


saveQuota writes a uniquely-named temp file and then renames it. If writeFile succeeds but rename fails (permissions, disk full, etc.), the temp file will be left behind and could accumulate over time. Wrap the rename in a try/finally (or catch) that cleans up tmpPath on failure.

Suggested change

await fs.rename(tmpPath, finalPath);

try {

await fs.rename(tmpPath, finalPath);

} catch (err) {

try {

await fs.unlink(tmpPath);

} catch (cleanupErr) {

if (cleanupErr.code !== 'ENOENT') {

// Ignore cleanup failure and preserve the original rename error.

}

}

throw err;

}

- saveQuota: clean up the temp file if rename fails, so failed atomic writes don't leave orphan files behind. - test: save/restore DATA_ROOT in before/after so the test doesn't leak global state into other tests in the same process. - test: assert reads are non-empty instead of skipping the check, so a regression back to truncating writes actually fails the test.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-23T02:16:04Z

    const data = await fs.readFile(getQuotaPath(podName), 'utf-8');
+    // Empty/partial file can appear mid-write from a racing saveQuota on older
+    // writes; treat as uninitialized and let the next save repair it.
+    if (!data.trim()) return { limit: 0, used: 0 };
    return JSON.parse(data);
  } catch (err) {
-    if (err.code === 'ENOENT') {
-      // No quota file - return defaults (will be initialized on first write)
+    if (err.code === 'ENOENT' || err instanceof SyntaxError) {
      return { limit: 0, used: 0 };


loadQuota treats empty/partial/corrupt quota files as { limit: 0, used: 0 }. With the current checkQuota logic, this can silently reset used to 0 (via initializeQuota) while keeping quota enforcement enabled (defaultQuota > 0), which undercounts actual disk usage and can allow writes beyond the configured quota until a later reconciliation. Consider repairing by recalculating used (e.g., calculatePodSize) on parse/empty-file paths and initializing quota with that used value, or otherwise ensuring quota doesn’t get reset to 0 usage after corruption.

Copilot · 2026-04-23T02:16:05Z

+const TEST_ROOT = path.resolve('./data-quota-race-test');
+const POD = 'testpod';
+let originalDataRoot;
+
+describe('quota — concurrent updates (#309)', () => {
+  before(async () => {
+    originalDataRoot = process.env.DATA_ROOT;
+    process.env.DATA_ROOT = TEST_ROOT;
+    await fs.emptyDir(TEST_ROOT);
+    await fs.ensureDir(path.join(TEST_ROOT, POD));
+    await initializeQuota(POD, 50 * 1024 * 1024);


This test uses a fixed on-disk directory (./data-quota-race-test). Other tests in this repo tend to use fs.mkdtemp* under os.tmpdir() to avoid collisions between local runs/CI shards and to ensure cleanup even if a prior run left artifacts. Consider switching to a unique temp dir per run (mkdtemp) and storing it in TEST_ROOT.

Previously, loadQuota returned {limit:0, used:0} on empty or malformed quota files. That caused checkQuota's initialize branch to reset the pod's tracked usage to 0, silently under-counting real disk use and allowing writes past the configured quota until the next reconciliation. Now loadQuota calls calculatePodSize on the empty/SyntaxError paths and returns the real usage, and checkQuota's initialize branch preserves the reconciled used value instead of calling initializeQuota (which is still used by pod-creation callsites that legitimately start at 0). Test tidying: use fs.mkdtemp under os.tmpdir() instead of a fixed ./data-quota-race-test directory, and make repair tests self-contained so they don't depend on ordering.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-23T02:22:05Z

+  const finalPath = getQuotaPath(podName);
+  const tmpPath = `${finalPath}.tmp.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}`;
+  await fs.writeFile(tmpPath, JSON.stringify(quota, null, 2));
+  try {
+    await fs.rename(tmpPath, finalPath);
+  } catch (err) {
+    // Clean up the temp file so failures don't accumulate orphans.
+    await fs.unlink(tmpPath).catch(() => {});
+    throw err;


saveQuota only cleans up the temp file if the rename step fails. If fs.writeFile(tmpPath, ...) throws after creating/truncating the temp file (e.g., disk full / permission error), the partially-written temp file will be left behind. Consider wrapping the write+rename in a single try/finally (or catch) that unlinks tmpPath on any failure to avoid accumulating orphan temp files.

Copilot · 2026-04-23T02:22:05Z

+  // Initialize if no quota set. Preserve `used` so reconciled usage from a
+  // recovered corrupt/empty file is not reset to 0.
  if (quota.limit === 0 && defaultQuota > 0) {
-    quota = await initializeQuota(podName, defaultQuota);
+    quota = { limit: defaultQuota, used: quota.used };
+    await saveQuota(podName, quota);
  }


checkQuota preserves quota.used when initializing the default limit. If a malformed-but-parseable .quota.json is missing used (or has a non-number), this will propagate undefined/NaN into quota calculations. Consider normalizing used here (e.g., defaulting to 0 or recalculating) before saving and using it in projectedUsage.

- saveQuota: wrap both writeFile and rename in the cleanup try/catch so a failed writeFile mid-truncate also unlinks the temp file. - checkQuota: normalize quota.used to 0 when it's missing, negative, or non-finite, to avoid propagating undefined/NaN into projected usage calculations from a parseable-but-malformed .quota.json.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-23T02:27:40Z

  let quota = await loadQuota(podName);

-  // Initialize if no quota set
+  // Initialize if no quota set. Preserve `used` so reconciled usage from a
+  // recovered corrupt/empty file is not reset to 0, but guard against a
+  // parseable-but-malformed file where `used` is missing, negative, or NaN.
  if (quota.limit === 0 && defaultQuota > 0) {
-    quota = await initializeQuota(podName, defaultQuota);
+    const used = Number.isFinite(quota.used) && quota.used >= 0 ? quota.used : 0;
+    quota = { limit: defaultQuota, used };
+    await saveQuota(podName, quota);
  }


checkQuota only normalizes quota.used when it is initializing limit. If the quota file is parseable-but-malformed (e.g., { "limit": 123 } or { "limit": 123, "used": null }), later arithmetic in checkQuota/updateQuotaUsage can produce NaN and effectively bypass enforcement or persist invalid values. Consider validating/coercing limit and used to finite non-negative numbers immediately after loadQuota (or inside loadQuota) so all callers get a sane shape.

Copilot · 2026-04-23T02:27:40Z

+  // don't accumulate orphans.
+  const finalPath = getQuotaPath(podName);
+  const tmpPath = `${finalPath}.tmp.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}`;


saveQuota now writes a temp file alongside .quota.json (e.g., .quota.json.tmp...). calculateDirSize only skips the exact .quota.json filename, so any orphaned temp files (e.g., process crash between write and rename) will be counted toward pod usage and can inflate used. Consider either (a) placing temp files outside the pod tree, or (b) updating the size calculator to also ignore .quota.json.tmp.* files (or a dedicated prefix).

Suggested change

// don't accumulate orphans.

const finalPath = getQuotaPath(podName);

const tmpPath = `${finalPath}.tmp.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}`;

// don't accumulate orphans. Keep the temp file outside the pod directory so

// orphaned temp files cannot be counted toward pod usage during reconciliation.

const finalPath = getQuotaPath(podName);

const tmpPath = join(

getDataRoot(),

`.quota.tmp.${encodeURIComponent(podName)}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}`

);

- loadQuota: normalize parsed {limit, used} to finite non-negative numbers via sanitizeQuota so all downstream callers (checkQuota, updateQuotaUsage) get a sane shape regardless of malformed fields (missing/null/negative/string). - calculateDirSize: skip .quota.json.tmp.* orphans so a crash between writeFile and rename in saveQuota cannot inflate reconciled usage. - checkQuota: drop now-redundant used normalization.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-23T02:33:22Z

+ * arithmetic on it without worrying about undefined/NaN/negative values.
+ */
+function sanitizeQuota(q) {
+  const toNum = (v) => (Number.isFinite(v) && v >= 0 ? v : 0);


sanitizeQuota currently treats numeric strings (e.g. "100") as invalid because Number.isFinite('100') is false, coercing limit/used to 0. If you want to be more tolerant of already-on-disk values (manual edits, legacy data), consider coercing via Number(v) first and then validating finite and >= 0, so numeric strings are preserved while still sanitizing NaN/undefined/negative values.

Suggested change

const toNum = (v) => (Number.isFinite(v) && v >= 0 ? v : 0);

const toNum = (v) => {

const n = Number(v);

return Number.isFinite(n) && n >= 0 ? n : 0;

};

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-23T02:39:10Z

+      // Skip the quota file and any orphaned temp files from saveQuota.
      if (entry.name === QUOTA_FILE) continue;
+      if (entry.name.startsWith(QUOTA_TMP_PREFIX)) continue;


calculateDirSize() skips any file whose name starts with .quota.json.tmp. at every directory level. saveQuota() only creates temp files next to the real quota file (pod root), so skipping this pattern recursively can under-count real pod usage if a similarly-named file exists elsewhere (e.g., created out-of-band), weakening quota enforcement. Consider restricting the temp-file exclusion to the quota file’s directory (pod root), or matching the exact temp-file location rather than just the basename prefix during recursive traversal.

melvincarvalho requested a review from Copilot April 23, 2026 02:06

Copilot started reviewing on behalf of melvincarvalho April 23, 2026 02:06 View session