US-131: fence a lapsed-lease writer with a sandbox epoch

[Hazzng]

Closes #131
Closes #170

Stacks on #190 (fix/181-test-scaffolding-error-shape) — review/merge that first.

The bug

appendFile takes its base from the in-memory pathCache (existing.contentSha256, sql-fs.ts) before any lock is held, and getBlob is content-addressed — it reads by hash, so it keeps serving that base long after another replica replaced the dirent. A writer A whose Redis writer key expired therefore rebuilds stale-base + append and points the dirent at the result, erasing B's committed line.

pg_advisory_xact_lock cannot stop it: it serializes the transactions, but the read it would need to fence was already taken, in memory, before the lock existed. The heartbeat cannot stop it either — markLost() only fires on the renew timer, so any request that starts and finishes inside one interval (20 s by default) commits without ever consulting a lease it no longer holds.

The fix

• Migration 0007: sandboxes.version BIGINT NOT NULL DEFAULT 0.
• The epoch is read in the same call that loads the pathCache (epoch first, tree second — under READ COMMITTED that order can only leave the pin older than the tree, which fails closed) and pinned on SqlFs.
• Every composite write (writeFileComposite, mkdirComposite, rmComposite, mvComposite) carries that pin into
  UPDATE sandboxes SET version = version + 1 WHERE id = $s AND version = $expected
  inside its advisory-locked ctx CTE, and every mutating CTE in the statement reads from that fence — directly, or through new_inode / old_dirent, which is cross-joined with it. Gating only the INSERTs would still run deleted_old_inode and drop the live writer's inode, cascading its dirent away; there is an integration test that commits a fenced statement on purpose to prove it does not.
• The epoch advances in memory in lockstep, so later writes in the same transaction match the row the first one already bumped; a rollback re-reads it from Postgres. rm/mv gate the advance on the target existing, so an ENOENT cannot spend an epoch and desynchronise the pin.
• Committing was the other half of the hole. A fenced CTE matches zero rows; it does not raise. The transaction stays committable, and bash swallows a failed command in a script without set -e — so endScriptScope would have COMMITted and answered 200/exit 0 for a script whose writes were all dropped. The verdict is now sticky for the scope: every later operation fails and endScriptScope rolls back instead of committing.
• New code ESTALEEPOCH → 503, retryable: true (the statement wrote nothing and the transaction rolled back).

RLS, SET LOCAL and the defense-in-depth runTrustedDbAsync wrapping are unchanged — the fence rides inside the existing composite statements, which already set app.sandbox_id in ctx and already run under the trusted-async wrapper.

Verification

replica-steal.mjs (two replicas, one Postgres, one Redis) — before:

STEAL:
  stole writer key (DEL -> 1)
  B saw           : "base\n"
  final           : "base\nA-line\n"
  A http/exit     : 200/0   B http/exit: 200/0
  >>> LOST UPDATE: B reported success (exit 0) and its line is gone — #170 reproduced

after (3/3 runs):

STEAL:
  stole writer key (DEL -> 1)
  B saw           : "base\n"
  final           : "base\nB-line\n"
  A http/exit     : 503/undefined   B http/exit: 200/0
  >>> both writes survived, or the loser failed cleanly — the property holds

A's body is {"error":"ESTALEEPOCH: another writer committed to sandbox '…' after this scope pinned its epoch; nothing was applied, retry","code":"ESTALEEPOCH","retryable":true}. Confirmed against the blobs table (the live dirent's content_sha256 resolves to base\nB-line\n) and on both replicas — readFile and cat agree on 8101 and 8102.

concurrency.mjs: all 9 checks PASS, exit 0. pnpm typecheck / lint:fix clean; unit 1288 passed / 4 skipped (baseline 1271/4). Integration serial against a local non-superuser Postgres: 105 passed / 29 skipped, 0 failures (baseline 97/29). The fence + advisory-lock suites also pass against a Neon transaction-mode pooler.

Every test was checked by reverting its source change in isolation — including each individual CTE gate — and confirming the failure.

Cost

One extra row UPDATE per composite write, in the same statement (no extra round trip), plus one SELECT version per cache load (cold start and reload, not per exec). Measured on the load harness at concurrency 1 over a 40/25/20/15 read/write/exec/edit mix: ~577 → ~478 ops/s, about 17%, narrowing to under 10% by concurrency 8. The structural cost is that sandboxes now has one hot row per sandbox — every write rewrites it, so it accumulates dead tuples at write rate and leans on autovacuum — and a long-running script transaction now holds a row lock on sandboxes for its whole duration, so DDL against that table waits behind it where it previously did not.

Rolling deploys

The column is backward compatible in both directions: createSandbox inserts with an explicit column list and nothing selects *, so a replica that predates the migration runs unchanged against a migrated database — it simply never bumps the counter, and is neither fenced nor fencing. Protection is therefore complete only once every replica is on the new code; until then the mixed fleet behaves exactly as it does today.

Not covered

Only the four composite writes are fenced. bulkIngest, mkdir -p, rm -r, cp, link, symlink, chmod, utimes and truncate still mutate through the non-composite path — they neither advance the epoch nor are fenced by it. Sandbox delete-and-recreate reuses epoch 0, so a scope pinned at 0 against a deleted sandbox would pass its replacement's fence; that is the tombstone half of #136 and is deliberately out of scope. The lastSeenVersion divergence defect (session-manager.ts:1127) that #170 also describes is untouched here.

🤖 Generated with Claude Code

---

Summary by cubic

Fixes the lost-update bug where a writer whose Redis lease lapsed could silently erase another replica's committed write. Writers now pin a per-sandbox epoch that every composite write must advance conditionally; a stale pin makes the write fail with ESTALEEPOCH (503, retryable) instead of committing stale data.

• Adds sandboxes.version (migration 0007), read in the same call that loads the pathCache and stamped into writeFileComposite, mkdirComposite, rmComposite, and mvComposite.
• A fenced statement matches zero rows, writes nothing, and rolls back the transaction; rm/mv advance the epoch only when the target exists so an ENOENT cannot desynchronize the pin.
• A fenced script scope now rolls back instead of committing silently, since the fence is a zero-row UPDATE that bash would otherwise swallow and report as exit 0.
• ESTALEEPOCH maps to 503 with retryable: true; a retry reloads the cache and re-runs against the live epoch.
• Non-composite writes (bulkIngest, mkdir -p, rm -r, cp, link, symlink, chmod, utimes, truncate) and sandbox delete-and-recreate are not fenced.
• Costs about 17% write throughput at concurrency 1, narrowing to under 10% by concurrency 8; sandboxes now has one hot row per sandbox and long script transactions hold a row lock on it.

Migration

• The column is backward compatible in both directions: older replicas run unchanged against a migrated database and are neither fenced nor fencing, so protection is complete only once every replica runs the new code.

^{Written for commit c0bb1a5. Summary will update on new commits.}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. 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: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 28a274fc-c1b5-475e-8207-53292becdd8d

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

---

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.}

[cubic-dev-ai]

6 issues found across 15 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/sql-fs/tests/integration/epoch-fence.integration.test.ts">

<violation number="1" location="src/sql-fs/tests/integration/epoch-fence.integration.test.ts:55">
P3: When `open()` fails before `a`/`b` are assigned (e.g., `createSandbox` throws on a transient DB error), the `finally` block calls `.disconnect()` on undefined instances and raises a TypeError that masks the real failure. Guard the disconnect calls with optional chaining.</violation>
</file>

<file name="src/api/errors.ts">

<violation number="1" location="src/api/errors.ts:32">
P3: When an epoch fence rejects an exec, this new classification returns `retryable: true`, but the served OpenAPI 503 contract still omits `ESTALEEPOCH` and says six codes share this status. Update the API documentation and related count so clients generated from the spec recognize the new safe retry case.</violation>
</file>

<file name="src/sql-fs/tests/unit/sql-fs.epoch-fence.test.ts">

<violation number="1" location="src/sql-fs/tests/unit/sql-fs.epoch-fence.test.ts:92">
P2: The harness records every lock-taking transaction as a script transaction, so its rollback/commit assertions can include unrelated transactions. Track only the transaction opened by `#openScriptTx` instead of classifying every `setSandboxContextWithLock` call.</violation>

<violation number="2" location="src/sql-fs/tests/unit/sql-fs.epoch-fence.test.ts:164">
P2: This assertion does not prove the epoch and path tree load share a transaction. A regression that reads the epoch in a separate transaction would still pass, so record both calls and assert the version read precedes `loadAllPaths` on the same transaction.</violation>
</file>

<file name="src/sql-fs/sql-fs.ts">

<violation number="1" location="src/sql-fs/sql-fs.ts:332">
P2: After a composite sets `#scopeFenced`, `readdirWithFileTypes` still serves the cache without checking the sticky verdict, so a script can observe writes that will be rolled back. Add `#assertScriptTxAlive()` at the start of every cache-served read, including `readdirWithFileTypes`.

(Based on your team's feedback about cache-served reads after transaction loss.) [439ef221-3655-4eff-93a7-698ae087780f]</violation>

<violation number="2" location="src/sql-fs/sql-fs.ts:604">
P0: When a Redis version publish is pending, this accepts an old path snapshot while pinning the current SQL epoch. The next composite can then pass the fence with stale cache data and overwrite the committed write. Store the SQL epoch with each snapshot and validate it, or fall back to `loadAllPaths` when it cannot be validated.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P0: When a Redis version publish is pending, this accepts an old path snapshot while pinning the current SQL epoch. The next composite can then pass the fence with stale cache data and overwrite the committed write. Store the SQL epoch with each snapshot and validate it, or fall back to loadAllPaths when it cannot be validated.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/sql-fs/sql-fs.ts, line 604:

<comment>When a Redis version publish is pending, this accepts an old path snapshot while pinning the current SQL epoch. The next composite can then pass the fence with stale cache data and overwrite the committed write. Store the SQL epoch with each snapshot and validate it, or fall back to `loadAllPaths` when it cannot be validated.</comment>

<file context>
@@ -542,7 +601,7 @@ export class SqlFs<Tx = unknown> implements ICoherentFs, IReadOnlyScopeFs {
 						}),
 					);
-					return { entries: snap.entries, fromSnapshot: true };
+					return { entries: snap.entries, fromSnapshot: true, epoch: snapshotEpoch };
 				}
 			} catch (err) {
</file context>

[cubic-dev-ai]

P2: The harness records every lock-taking transaction as a script transaction, so its rollback/commit assertions can include unrelated transactions. Track only the transaction opened by #openScriptTx instead of classifying every setSandboxContextWithLock call.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/sql-fs/tests/unit/sql-fs.epoch-fence.test.ts, line 92:

<comment>The harness records every lock-taking transaction as a script transaction, so its rollback/commit assertions can include unrelated transactions. Track only the transaction opened by `#openScriptTx` instead of classifying every `setSandboxContextWithLock` call.</comment>

<file context>
@@ -0,0 +1,288 @@
+		}),
+		setSandboxContext: vi.fn(),
+		setSandboxContextWithLock: vi.fn(async (tx: unknown) => {
+			scriptTxs.add(tx as object);
+		}),
+		getSandboxVersion,
</file context>

[cubic-dev-ai]

P2: This assertion does not prove the epoch and path tree load share a transaction. A regression that reads the epoch in a separate transaction would still pass, so record both calls and assert the version read precedes loadAllPaths on the same transaction.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/sql-fs/tests/unit/sql-fs.epoch-fence.test.ts, line 164:

<comment>This assertion does not prove the epoch and path tree load share a transaction. A regression that reads the epoch in a separate transaction would still pass, so record both calls and assert the version read precedes `loadAllPaths` on the same transaction.</comment>

<file context>
@@ -0,0 +1,288 @@
+
+	it("reads the epoch in the same call that loads the pathCache", () => {
+		expect(h.getSandboxVersion).toHaveBeenCalledTimes(1);
+		expect(h.getSandboxVersion).toHaveBeenCalledWith(expect.anything(), "s1");
+	});
+
</file context>

[cubic-dev-ai]

P2: After a composite sets #scopeFenced, readdirWithFileTypes still serves the cache without checking the sticky verdict, so a script can observe writes that will be rolled back. Add #assertScriptTxAlive() at the start of every cache-served read, including readdirWithFileTypes.

(Based on your team's feedback about cache-served reads after transaction loss.) [439ef221-3655-4eff-93a7-698ae087780f]

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/sql-fs/sql-fs.ts, line 332:

<comment>After a composite sets `#scopeFenced`, `readdirWithFileTypes` still serves the cache without checking the sticky verdict, so a script can observe writes that will be rolled back. Add `#assertScriptTxAlive()` at the start of every cache-served read, including `readdirWithFileTypes`.

(Based on your team's feedback about cache-served reads after transaction loss.) [439ef221-3655-4eff-93a7-698ae087780f]</comment>

<file context>
@@ -306,6 +329,33 @@ export class SqlFs<Tx = unknown> implements ICoherentFs, IReadOnlyScopeFs {
 	 */
 	#assertScriptTxAlive(): void {
 		if (this.#scriptScope && this.#scriptTxLost !== undefined) throw this.#scriptTxLost;
+		if (this.#scriptScope && this.#scopeFenced !== undefined) throw this.#scopeFenced;
+	}
+
</file context>

[cubic-dev-ai]

P3: When open() fails before a/b are assigned (e.g., createSandbox throws on a transient DB error), the finally block calls .disconnect() on undefined instances and raises a TypeError that masks the real failure. Guard the disconnect calls with optional chaining.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/sql-fs/tests/integration/epoch-fence.integration.test.ts, line 55:

<comment>When `open()` fails before `a`/`b` are assigned (e.g., `createSandbox` throws on a transient DB error), the `finally` block calls `.disconnect()` on undefined instances and raises a TypeError that masks the real failure. Guard the disconnect calls with optional chaining.</comment>

<file context>
@@ -0,0 +1,264 @@
+		try {
+			await admin.transaction((tx) => admin.deleteSandbox(tx, sandboxId));
+		} finally {
+			await a.disconnect();
+			await b.disconnect();
+		}
</file context>

Suggested change

	await a.disconnect();
	await b.disconnect();
	await a?.disconnect();
	await b?.disconnect();

[cubic-dev-ai]

P3: When an epoch fence rejects an exec, this new classification returns retryable: true, but the served OpenAPI 503 contract still omits ESTALEEPOCH and says six codes share this status. Update the API documentation and related count so clients generated from the spec recognize the new safe retry case.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/api/errors.ts, line 32:

<comment>When an epoch fence rejects an exec, this new classification returns `retryable: true`, but the served OpenAPI 503 contract still omits `ESTALEEPOCH` and says six codes share this status. Update the API documentation and related count so clients generated from the spec recognize the new safe retry case.</comment>

<file context>
@@ -29,6 +29,7 @@ export const SAFE_FS_ERROR_CODES: ReadonlySet<string> = new Set([
 	"ELOCKLOST",
 	"ECOHERENCE",
 	"ECOHERENCE_UNAPPLIED",
+	"ESTALEEPOCH",
 	"ERUNTIME_BUSY",
 	"EREADONLY",
</file context>

[Hazzng]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: c0bb1a51f7

ℹ️ 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".

[Hazzng]

Closing as superseded by #161, which merged to main (600b550) while this was in review.

Both implement the #131 fence, and they collide directly: 25+ conflict markers across the core files, and two migrations numbered 0007 both adding sandboxes.version.

Having compared them, this PR is redundant and #161's is the better base:

• Same coverage. Both fence exactly the four composites (writeFileComposite, mkdirComposite, rmComposite, mvComposite) off the same sandboxes.version column. feat(sql-fs): fence stale sandbox writers with durable epochs #161's getSandboxEpoch and this PR's getSandboxVersion read the same thing.
• feat(sql-fs): fence stale sandbox writers with durable epochs #161 also ships the sandbox_epochs tombstone, which this PR deliberately dropped as F7: Destroy must reach warm replicas — tombstone + sandbox-gone reload guard #136's lifecycle problem. So feat(sql-fs): fence stale sandbox writers with durable epochs #161 is a strict superset on coverage.
• feat(sql-fs): fence stale sandbox writers with durable epochs #161's advancing pin is better placed. This PR advances #epoch in JS memory after the await resolves, and documents that a rolled-back scope "leaves the pin too high". feat(sql-fs): fence stale sandbox writers with durable epochs #161 keeps its pin in app.sandbox_epoch, set with is_local = true in the same CTE that bumps version — so it is transaction-scoped, pooler-safe, and rolls back atomically with the write it describes. State that rolls back with the transaction cannot desynchronize from it; state kept in sync by hand can.

The version > expected branch in #161 is not looser than this PR's strict equality, as it first appeared: it is gated on version = app.sandbox_epoch, and that GUC is only ever set from #161's own advanced CTE, so it can only admit the same transaction's own advances.

What does not carry over is #192 (PR #197): the fourteen non-composite mutation paths — bulkIngest, mkdir -p, rm -r, cp, cp -r, link, symlink, chmod, utimes and the non-composite fallbacks — which main neither fences nor advances the counter for. That second half matters: a live writer using only those paths leaves the counter unmoved, so a genuinely stale writer is not fenced afterwards. #197 will be reworked against #161's GUC-based shape rather than this one.

Comparison written up in thoughts/shared/plans/2026-09-19_epoch-fence-collision-161-vs-131.md.

The rest of the hardening stack has been rebased onto the new main with this PR dropped; every branch passes its own gate.