From 4cc3b292e5f6f4f7e27fd62b563413d62bac902a Mon Sep 17 00:00:00 2001 From: Yeachan-Heo Date: Wed, 22 Jul 2026 12:40:21 +0000 Subject: [PATCH 1/7] fix(hooks): recover positively dead canonical session-pointer locks Issue #3261: after SIGKILL/OOM/power loss while holding the session pointer lock, launch aborts demanding recovery while 'omx session lock recover' refuses a positively dead canonical owner.json, a permanent deadlock. Broaden the recoverability gate in inspectSessionPointerLockAtContext from dead owner-temp evidence only to any positively dead (ESRCH) evidence, so canonical owner.json flows through the existing atomic claim/quarantine protocol, which revalidates the exact entry, still-dead status, and owner token before quarantining. Fail-closed behavior is unchanged for live, reused, identity-indeterminate, ambiguous, malformed, symlink, unexpected, and io-error states. No orphan pointer-temp cleanup is added; pointer temps remain inert by design. Tests: rewrite the codifying dead-canonical case to assert quarantine, idempotence, and the owner.json claim rename sequence; add launch-then-recover-then-launch integration; add canonical successor-displacement race coverage for the pre-claim and claim-revalidation paths. Fixes #3261 --- src/hooks/__tests__/session.test.ts | 132 ++++++++++++++++++++++++++-- src/hooks/session.ts | 8 +- 2 files changed, 134 insertions(+), 6 deletions(-) diff --git a/src/hooks/__tests__/session.test.ts b/src/hooks/__tests__/session.test.ts index b785b2247..53df6646a 100644 --- a/src/hooks/__tests__/session.test.ts +++ b/src/hooks/__tests__/session.test.ts @@ -912,20 +912,142 @@ describe('session pointer transaction', () => { } }); - it('does not recover a dead canonical owner because owner.json is not an exact acquisition claim', async () => { + it('atomically quarantines dead canonical owner evidence and stays idempotent', async () => { const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-dead-canonical-')); + try { + const context = resolveSessionPointerContext(cwd); + await writeLockOwner(cwd, validLockOwner()); + const renames: Array<[string, string]> = []; + await withPointerDependencies({ + token: () => SUCCESSOR_TOKEN, + probePid: () => 'dead', + fs: { + rename: async (from, to) => { + renames.push([from, to]); + await rename(from, to); + }, + }, + }, async () => { + const inspected = await inspectSessionPointerLock(cwd); + assert.equal(inspected.status, 'dead'); + assert.equal(inspected.evidenceSource, 'owner.json'); + assert.equal(inspected.safeToRecover, true); + const recovered = await recoverSessionPointerLock(cwd); + assert.equal(recovered.recovered, true); + assert.equal(recovered.action, 'quarantined'); + assert.equal(existsSync(context.lockPath), false); + assert.equal(existsSync(recovered.quarantinePath!), true); + const repeated = await recoverSessionPointerLock(cwd); + assert.equal(repeated.recovered, false); + assert.equal(repeated.action, 'none'); + assert.equal(repeated.status, 'absent'); + }); + assert.match(renames[0]![0], /owner\.json$/); + assert.match(renames[0]![1], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.match(renames[1]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.equal(renames[1]![1].includes('.quarantine.'), true); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + it('launches only after explicit recovery of a dead canonical owner lock', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recover-launch-')); try { const context = resolveSessionPointerContext(cwd); await writeLockOwner(cwd, validLockOwner()); await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead' }, async () => { + await assert.rejects( + writeSessionStart(cwd, 'sess-blocked', { platform: 'win32' }), + (error: unknown) => isSessionPointerLaunchAbort(error) + && error.code === 'session_pointer_lock_recovery_required' + && error.lockOwnerStatus === 'dead', + ); + assert.equal(existsSync(context.lockPath), true); + const recovered = await recoverSessionPointerLock(cwd); + assert.equal(recovered.recovered, true); + assert.equal(recovered.action, 'quarantined'); + await writeSessionStart(cwd, 'sess-after-recovery', { platform: 'win32' }); + assert.equal((await readSessionState(cwd))?.session_id, 'sess-after-recovery'); + assert.equal(existsSync(context.lockPath), false); + }); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + it('does not claim a successor lock when the inspected dead canonical owner is displaced before the exact rename', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-canonical-race-')); + try { + const context = resolveSessionPointerContext(cwd); + const staleOwner = join(context.lockPath, 'owner.json'); + const successorTemp = join(context.lockPath, `owner.${SUCCESSOR_TOKEN}.tmp`); + const displacedPath = `${context.lockPath}.displaced`; + await writeLockOwner(cwd, validLockOwner()); + let displaced = false; + await withPointerDependencies({ + token: () => SUCCESSOR_TOKEN, + probePid: () => 'dead', + fs: { + rename: async (from, to) => { + if (!displaced && from === staleOwner) { + displaced = true; + await rename(context.lockPath, displacedPath); + await mkdir(context.lockPath); + await writeFile(successorTemp, JSON.stringify(validLockOwner({ token: SUCCESSOR_TOKEN })), 'utf-8'); + } + await rename(from, to); + }, + }, + }, async () => { const recovered = await recoverSessionPointerLock(cwd); - assert.equal(recovered.status, 'dead'); - assert.equal(recovered.evidenceSource, 'owner.json'); - assert.equal(recovered.safeToRecover, false); assert.equal(recovered.recovered, false); assert.equal(recovered.action, 'none'); - assert.equal(existsSync(context.lockPath), true); + assert.match(recovered.reason, /changed before recovery claim/i); }); + assert.equal(displaced, true); + assert.deepEqual(await readdir(context.lockPath), [`owner.${SUCCESSOR_TOKEN}.tmp`]); + assert.equal(await readFile(successorTemp, 'utf-8'), JSON.stringify(validLockOwner({ token: SUCCESSOR_TOKEN }))); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + it('refuses to quarantine when the claim rename lands on a displaced successor canonical owner', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-canonical-claim-race-')); + try { + const context = resolveSessionPointerContext(cwd); + const staleOwner = join(context.lockPath, 'owner.json'); + const successorOwner = JSON.stringify(validLockOwner({ token: SUCCESSOR_TOKEN })); + const displacedPath = `${context.lockPath}.displaced`; + await writeLockOwner(cwd, validLockOwner()); + let displaced = false; + await withPointerDependencies({ + token: () => SUCCESSOR_TOKEN, + probePid: () => 'dead', + fs: { + rename: async (from, to) => { + if (!displaced && from === staleOwner) { + displaced = true; + await rename(context.lockPath, displacedPath); + await mkdir(context.lockPath); + await writeFile(staleOwner, successorOwner, 'utf-8'); + } + await rename(from, to); + }, + }, + }, async () => { + const recovered = await recoverSessionPointerLock(cwd); + assert.equal(recovered.recovered, false); + assert.equal(recovered.action, 'none'); + assert.match(recovered.reason, /revalidation failed/i); + assert.equal(recovered.quarantinePath, undefined); + }); + assert.equal(displaced, true); + const entries = await readdir(context.lockPath); + assert.equal(entries.length, 1); + assert.match(entries[0]!, new RegExp(`^owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.equal(await readFile(join(context.lockPath, entries[0]!), 'utf-8'), successorOwner); } finally { await rm(cwd, { recursive: true, force: true }); } diff --git a/src/hooks/session.ts b/src/hooks/session.ts index 83c4ac75f..ab5514069 100644 --- a/src/hooks/session.ts +++ b/src/hooks/session.ts @@ -974,7 +974,13 @@ async function inspectSessionPointerLockAtContext(context: SessionPointerContext status, lockPath: context.lockPath, evidenceSource, - safeToRecover: status === 'dead' && evidenceSource === 'owner-temp', + // Recoverable only on positively dead evidence (ESRCH), for canonical + // owner.json and pre-rename temp evidence alike; the atomic + // claim/quarantine protocol revalidates the exact entry, still-dead + // status, and owner token before quarantining. Live, reused, + // identity-indeterminate, ambiguous, symlink, malformed, unexpected, + // and io-error states stay fail-closed. + safeToRecover: status === 'dead', }; } From 308628559e8cce491a7b407c51fdc66fbe0b9944 Mon Sep 17 00:00:00 2001 From: Yeachan-Heo Date: Wed, 22 Jul 2026 15:01:29 +0000 Subject: [PATCH 2/7] fix(hooks): make dead-lock recovery claim identity-revalidated and no-clobber reversible Independent exact-head review of 4cc3b292 found two P1 races in the broadened dead-canonical recovery path: 1. The canonical claim renames the shared owner.json pathname; a lock directory replacement after revalidation but before the rename moved a successor's owner.json into the dead owner's claim name, and token revalidation refused quarantine without restoring the successor, stranding it under an unrecognizable .recovery name. 2. Neither the lock directory nor the evidence file was re-lstat revalidated between inspection-time checks and mutations, so a post-inspection symlink or non-file swap redirected the claim rename into foreign storage. Repair, keeping the one-gate broadening and the atomic claim/quarantine shape: - Re-lstat the lock directory (non-symlink directory) and evidence (non-symlink regular file) immediately before every mutation: at re-read, before the claim rename, before quarantine, and before rmdir; temp evidence also re-proves filename/content token equality. - Every refusal after the claim rename rolls the claim back to its original evidence name via atomic no-clobber link+unlink (rename fallback only when link is impossible, e.g. a swapped-in directory), never overwriting a reappeared name and never modifying foreign bytes. - Quarantine is created with no-clobber link()+unlink() instead of rename(), so a quarantine path appearing after its preflight check is never overwritten. Tests: replace the stranded-successor codification with rollback preservation proof; add live canonical successor survival/recognizable/releasable/acquirable, lock-dir symlink replacement, evidence symlink swap, non-file evidence replacement, and quarantine-appearance race regressions; update both claim sequence assertions for link-based quarantine. Focused session suite 81/81, broader hooks+cli lane, build, no-unused, and lint all green. Refs #3261 --- src/hooks/__tests__/session.test.ts | 138 ++++++++++++++++++++++++++-- src/hooks/session.ts | 122 +++++++++++++++++++++--- 2 files changed, 239 insertions(+), 21 deletions(-) diff --git a/src/hooks/__tests__/session.test.ts b/src/hooks/__tests__/session.test.ts index 53df6646a..6e9c9df5a 100644 --- a/src/hooks/__tests__/session.test.ts +++ b/src/hooks/__tests__/session.test.ts @@ -3,7 +3,7 @@ import assert from 'node:assert/strict'; import { spawn } from 'node:child_process'; import { once } from 'node:events'; import { existsSync } from 'node:fs'; -import { mkdir, mkdtemp, readFile, readdir, rename, rm, rmdir, symlink, writeFile } from 'node:fs/promises'; +import { link, mkdir, mkdtemp, readFile, readdir, readlink, rename, rm, rmdir, symlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { @@ -918,6 +918,7 @@ describe('session pointer transaction', () => { const context = resolveSessionPointerContext(cwd); await writeLockOwner(cwd, validLockOwner()); const renames: Array<[string, string]> = []; + const links: Array<[string, string]> = []; await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', @@ -926,6 +927,10 @@ describe('session pointer transaction', () => { renames.push([from, to]); await rename(from, to); }, + link: async (from, to) => { + links.push([from, to]); + await link(from, to); + }, }, }, async () => { const inspected = await inspectSessionPointerLock(cwd); @@ -944,8 +949,8 @@ describe('session pointer transaction', () => { }); assert.match(renames[0]![0], /owner\.json$/); assert.match(renames[0]![1], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); - assert.match(renames[1]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); - assert.equal(renames[1]![1].includes('.quarantine.'), true); + assert.match(links[0]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.equal(links[0]![1].includes('.quarantine.'), true); } finally { await rm(cwd, { recursive: true, force: true }); } @@ -1013,7 +1018,7 @@ describe('session pointer transaction', () => { } }); - it('refuses to quarantine when the claim rename lands on a displaced successor canonical owner', async () => { + it('rolls back a claim that lands on a displaced successor canonical owner', async () => { const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-canonical-claim-race-')); try { const context = resolveSessionPointerContext(cwd); @@ -1044,15 +1049,123 @@ describe('session pointer transaction', () => { assert.equal(recovered.quarantinePath, undefined); }); assert.equal(displaced, true); - const entries = await readdir(context.lockPath); - assert.equal(entries.length, 1); - assert.match(entries[0]!, new RegExp(`^owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); - assert.equal(await readFile(join(context.lockPath, entries[0]!), 'utf-8'), successorOwner); + assert.deepEqual(await readdir(context.lockPath), ['owner.json']); + assert.equal(await readFile(staleOwner, 'utf-8'), successorOwner); } finally { await rm(cwd, { recursive: true, force: true }); } }); + it('live canonical successor survives a refused recovery claim and stays recognizable, releasable, and acquirable', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-live-successor-')); + try { + const context = resolveSessionPointerContext(cwd); + const ownerPath = join(context.lockPath, 'owner.json'); + const successorPid = process.pid + 100_000; + const successor = JSON.stringify(validLockOwner({ token: SUCCESSOR_TOKEN, pid: successorPid })); + let displaced = false; + let live = true; + await writeLockOwner(cwd, validLockOwner()); + await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: (pid) => live && pid === successorPid ? 'alive' : 'dead', readProcessIdentity: () => matchingProcessIdentity(), fs: { + rename: async (from, to) => { + if (!displaced && from === ownerPath) { + displaced = true; + await rename(context.lockPath, `${context.lockPath}.displaced`); + await mkdir(context.lockPath); + await writeFile(ownerPath, successor, 'utf-8'); + } + await rename(from, to); + }, + } }, async () => { + assert.equal((await recoverSessionPointerLock(cwd)).recovered, false); + const inspected = await inspectSessionPointerLock(cwd); + assert.equal(inspected.status, 'live'); + assert.equal(inspected.safeToRecover, false); + assert.equal((await recoverSessionPointerLock(cwd)).status, 'live'); + live = false; + assert.equal((await recoverSessionPointerLock(cwd)).action, 'quarantined'); + await writeSessionStart(cwd, 'sess-after-survival', { platform: 'win32' }); + assert.equal((await readSessionState(cwd))?.session_id, 'sess-after-survival'); + }); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + + it('fails closed without mutating a foreign target when the lock directory is replaced by a symlink before the claim rename', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-lock-symlink-')); + const foreign = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-foreign-')); + try { + const context = resolveSessionPointerContext(cwd); + const ownerPath = join(context.lockPath, 'owner.json'); + const foreignOwner = join(foreign, 'owner.json'); + const marker = JSON.stringify(validLockOwner({ token: SUCCESSOR_TOKEN })); + let replaced = false; + await writeLockOwner(cwd, validLockOwner()); + await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { rename: async (from, to) => { + if (!replaced && from === ownerPath) { replaced = true; await rm(context.lockPath, { recursive: true }); await writeFile(foreignOwner, marker); await symlink(foreign, context.lockPath); } + await rename(from, to); + } } }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, false)); + assert.equal(await readFile(foreignOwner, 'utf-8'), marker); + assert.equal((await readdir(foreign)).some((entry) => entry.includes('.quarantine.')), false); + } finally { await rm(cwd, { recursive: true, force: true }); await rm(foreign, { recursive: true, force: true }); } + }); + + it('does not follow an evidence symlink swapped in before the claim rename', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-evidence-symlink-')); + try { + const context = resolveSessionPointerContext(cwd); + const ownerPath = join(context.lockPath, 'owner.json'); + const foreign = join(cwd, 'foreign-owner'); + const marker = 'foreign evidence marker'; + let replaced = false; + await writeLockOwner(cwd, validLockOwner()); + await writeFile(foreign, marker); + await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { rename: async (from, to) => { + if (!replaced && from === ownerPath) { replaced = true; await rm(ownerPath); await symlink(foreign, ownerPath); } + await rename(from, to); + } } }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, false)); + assert.equal(await readFile(foreign, 'utf-8'), marker); + assert.equal(await readlink(ownerPath), foreign); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + + it('restores a non-file evidence replacement instead of stranding it', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-evidence-directory-')); + try { + const context = resolveSessionPointerContext(cwd); + const ownerPath = join(context.lockPath, 'owner.json'); + const marker = join(ownerPath, 'marker'); + let replaced = false; + await writeLockOwner(cwd, validLockOwner()); + await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { rename: async (from, to) => { + if (!replaced && from === ownerPath) { replaced = true; await rm(ownerPath); await mkdir(ownerPath); await writeFile(marker, 'directory marker'); } + await rename(from, to); + } } }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, false)); + assert.equal(await readFile(marker, 'utf-8'), 'directory marker'); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + + it('refuses a quarantine path that appears after its preflight check without overwriting it', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-quarantine-race-')); + try { + const context = resolveSessionPointerContext(cwd); + const ownerPath = join(context.lockPath, 'owner.json'); + const owner = JSON.stringify(validLockOwner()); + const marker = 'foreign quarantine marker'; + await writeLockOwner(cwd, validLockOwner()); + await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { link: async (from, to) => { + if (to.includes('.quarantine.')) await writeFile(to, marker); + await link(from, to); + } } }, async () => { + const recovered = await recoverSessionPointerLock(cwd); + assert.equal(recovered.recovered, false); + assert.match(recovered.reason, /Unable to quarantine the exact session pointer recovery claim/); + }); + assert.equal(await readFile(ownerPath, 'utf-8'), owner); + assert.equal((await readdir(context.lockPath)).some((entry) => entry.includes('.recovery')), false); + assert.equal(await readFile(`${context.lockPath}.quarantine.${TEST_TOKEN}.${SUCCESSOR_TOKEN}`, 'utf-8'), marker); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + it('distinguishes a paused live pre-rename owner from the same owner after SIGKILL', async (t) => { if (process.platform !== 'linux') { t.skip('Linux process start identity is required for deterministic PID reuse protection.'); @@ -1130,6 +1243,7 @@ describe('session pointer transaction', () => { await mkdir(context.lockPath, { recursive: true }); await writeFile(join(context.lockPath, `owner.${TEST_TOKEN}.tmp`), JSON.stringify(validLockOwner()), 'utf-8'); const renames: Array<[string, string]> = []; + const links: Array<[string, string]> = []; let successorBlocked = false; await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, @@ -1143,6 +1257,10 @@ describe('session pointer transaction', () => { successorBlocked = true; } }, + link: async (from, to) => { + links.push([from, to]); + await link(from, to); + }, }, }, async () => { const recovered = await recoverSessionPointerLock(cwd); @@ -1151,8 +1269,8 @@ describe('session pointer transaction', () => { assert.equal(successorBlocked, true); assert.match(renames[0]![0], /owner\.transaction_token_123456\.tmp$/); assert.match(renames[0]![1], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); - assert.match(renames[1]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); - assert.equal(renames[1]![1].includes('.quarantine.'), true); + assert.match(links[0]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.equal(links[0]![1].includes('.quarantine.'), true); } finally { await rm(cwd, { recursive: true, force: true }); } diff --git a/src/hooks/session.ts b/src/hooks/session.ts index ab5514069..deb70f24e 100644 --- a/src/hooks/session.ts +++ b/src/hooks/session.ts @@ -7,6 +7,7 @@ */ import { appendFile, + link as nodeLink, lstat as nodeLstat, mkdir as nodeMkdir, open, @@ -408,6 +409,7 @@ export interface SessionPointerFsDependencies { regularFileSync?: SessionStartOptions['regularFileSync'], ): Promise; rename(from: string, to: string): Promise; + link(path: string, dest: string): Promise; unlink(path: string): Promise; rmdir(path: string): Promise; } @@ -465,6 +467,9 @@ const defaultFsDependencies: SessionPointerFsDependencies = { rename: async (from, to) => { await nodeRename(from, to); }, + link: async (path, dest) => { + await nodeLink(path, dest); + }, unlink: async (path) => { await nodeUnlink(path); }, @@ -989,6 +994,62 @@ export async function inspectSessionPointerLock(cwd: string): Promise { + try { + const lockStat = await transactionDependencies.fs.lstat(lockPath); + if (lockStat.isSymbolicLink() || !lockStat.isDirectory()) { + return 'Session pointer lock evidence changed before recovery claim.'; + } + const evidenceStat = await transactionDependencies.fs.lstat(join(lockPath, evidenceName)); + if (evidenceStat.isSymbolicLink() || !evidenceStat.isFile()) { + return 'Session pointer lock evidence changed before recovery claim.'; + } + } catch { + return 'Session pointer lock evidence changed before recovery claim.'; + } + return undefined; +} + +async function rollbackRecoveryClaim(lockPath: string, evidenceName: string, claimName: string): Promise<{ ok: boolean; reason?: string }> { + const evidencePath = join(lockPath, evidenceName); + const claimPath = join(lockPath, claimName); + try { + await transactionDependencies.fs.lstat(evidencePath); + return { ok: false, reason: 'the original evidence name reappeared' }; + } catch (error) { + if (!isNotFound(error)) return { ok: false, reason: 'unable to inspect the original evidence name' }; + } + try { + await transactionDependencies.fs.link(claimPath, evidencePath); + } catch (error) { + if (isAlreadyExists(error)) return { ok: false, reason: 'the original evidence name reappeared during rollback' }; + try { + await transactionDependencies.fs.lstat(evidencePath); + return { ok: false, reason: 'the original evidence name reappeared during rollback' }; + } catch { + try { + await transactionDependencies.fs.rename(claimPath, evidencePath); + return { ok: true }; + } catch { + return { ok: false, reason: 'unable to restore the exact claim' }; + } + } + } + try { + await transactionDependencies.fs.unlink(claimPath); + return { ok: true }; + } catch (error) { + if (isNotFound(error)) return { ok: true }; + return { ok: false, reason: 'unable to remove the rolled-back claim name' }; + } +} + +function recoveryRollbackReason(prefix: string, rollback: { ok: boolean; reason?: string }): string { + return rollback.ok + ? `${prefix}; the exact claim was rolled back to its original evidence name without modifying any other bytes.` + : `${prefix}; rollback was not possible: ${rollback.reason}. The exact claim was left in place under its recovery name.`; +} + /** Explicitly quarantine only a dead, atomically claimed pointer lock; never force recovery. */ export async function recoverSessionPointerLock(cwd: string): Promise { const context = resolveSessionPointerContext(cwd); @@ -1007,8 +1068,10 @@ export async function recoverSessionPointerLock(cwd: string): Promise> | undefined; + try { + claimedClaimLstat = await transactionDependencies.fs.lstat(claimPath); + } catch { + // Roll back below. + } + const claimOwner = claimedClaimLstat && !claimedClaimLstat.isSymbolicLink() && claimedClaimLstat.isFile() + ? await inspectLockOwnerFile(claimPath) + : { status: 'unexpected' as const }; + if (!claimedEntries || claimedEntries.length !== 1 || claimedEntries[0] !== claimName || claimOwner.status !== 'dead' || claimOwner.owner?.token !== owner.owner.token || claimed.status !== 'unexpected' || !claimedClaimLstat || claimedClaimLstat.isSymbolicLink() || !claimedClaimLstat.isFile()) { + const rollback = await rollbackRecoveryClaim(context.lockPath, evidenceName, claimName); + return { ...claimed, action: 'none', recovered: false, reason: recoveryRollbackReason('Recovery claim revalidation failed', rollback) }; } const quarantinePath = `${context.lockPath}.quarantine.${owner.owner.token}.${claimToken}`; try { - await transactionDependencies.fs.lstat(quarantinePath); - return { ...claimed, action: 'none', recovered: false, reason: 'Recovery quarantine path already exists.' }; + const lockStat = await transactionDependencies.fs.lstat(context.lockPath); + const claimStat = await transactionDependencies.fs.lstat(claimPath); + if (lockStat.isSymbolicLink() || !lockStat.isDirectory() || claimStat.isSymbolicLink() || !claimStat.isFile()) { + throw new Error('recovery evidence changed'); + } + try { + await transactionDependencies.fs.lstat(quarantinePath); + throw new Error('recovery quarantine path exists'); + } catch (error) { + if (!isNotFound(error)) throw error; + } + } catch { + const rollback = await rollbackRecoveryClaim(context.lockPath, evidenceName, claimName); + return { ...claimed, action: 'none', recovered: false, reason: recoveryRollbackReason('Unable to quarantine the exact session pointer recovery claim', rollback) }; + } + try { + await transactionDependencies.fs.link(claimPath, quarantinePath); + } catch { + const rollback = await rollbackRecoveryClaim(context.lockPath, evidenceName, claimName); + return { ...claimed, action: 'none', recovered: false, reason: recoveryRollbackReason('Unable to quarantine the exact session pointer recovery claim', rollback) }; + } + try { + await transactionDependencies.fs.unlink(claimPath); } catch (error) { - if (!isNotFound(error)) return { ...claimed, action: 'none', recovered: false, reason: 'Unable to inspect recovery quarantine path.' }; + if (!isNotFound(error)) { + return { ...claimed, action: 'none', recovered: false, reason: 'The exact recovery claim was quarantined, but its recovery name could not be removed.', quarantinePath }; + } } try { - await transactionDependencies.fs.rename(claimPath, quarantinePath); + const lockStat = await transactionDependencies.fs.lstat(context.lockPath); + if (lockStat.isSymbolicLink() || !lockStat.isDirectory()) { + return { ...claimed, action: 'none', recovered: false, reason: 'The exact recovery claim was quarantined, but the canonical lock directory was replaced before removal.', quarantinePath }; + } } catch { - return { ...claimed, action: 'none', recovered: false, reason: 'Unable to quarantine the exact session pointer recovery claim.' }; + return { ...claimed, action: 'none', recovered: false, reason: 'The exact recovery claim was quarantined, but the canonical lock directory was replaced before removal.', quarantinePath }; } try { await transactionDependencies.fs.rmdir(context.lockPath); From f5750ba4b9073c8d23f86f6c80117b4239b86b66 Mon Sep 17 00:00:00 2001 From: Yeachan-Heo Date: Wed, 22 Jul 2026 20:49:57 +0000 Subject: [PATCH 3/7] fix(hooks): identity-bind dead-lock recovery claim, rollback, quarantine, and removal Second independent exact-head review of 30862855 found the round-1 repair still emulated atomic no-replace moves with pathname checks: the rollback rename fallback could clobber, link+unlink moves were not identity-safe, the claim destination was only preflighted, rmdir was not bound to the validated directory, and distinct quarantine diagnostics were collapsed. Round-2 repair, mirroring the in-repo no-clobber restore convention: - Claim is a no-clobber link() with captured dev/ino evidence identity; EEXIST or any identity drift fails closed without mutating foreign entries, and the evidence name is unlinked only while its identity still matches the captured inode. - Rollback is link-only (the clobbering rename fallback is removed); where no no-replace move exists (e.g. a swapped-in directory claim), the exact claim is left in place under its recovery name with a precise diagnostic. Claim unlink proceeds only while both link names still identify the captured inode. - Quarantine keeps no-clobber link()+identity-verified unlink, restores the distinct 'Recovery quarantine path already exists.' / 'Unable to inspect recovery quarantine path.' diagnostics, and treats only ENOENT as absence everywhere. - Pre-rmdir revalidation compares the captured lock-directory dev/ino, so an empty replacement directory is never removed. - Post-claim content inspection is skipped entirely when the rebound lock path no longer inspects as an exact claim shape. Tests: claim/quarantine sequencing updated for link-based claims; live-successor coverage now asserts byte-identical owner.json and exercises the real token-checked release via a narrow @internal test seam before reacquisition; new adversarial regressions for claim destination appearance, rollback destination appearance, quarantine claim identity swap, non-ENOENT lstat at pre-claim and quarantine preflight, and empty lock-directory replacement before rmdir. Focused session suite 87/87; broader hooks+cli lane (66 files), build, no-unused, and lint all green. Refs #3261 --- src/hooks/__tests__/session.test.ts | 314 ++++++++++++++++++++++++---- src/hooks/session.ts | 212 +++++++------------ 2 files changed, 339 insertions(+), 187 deletions(-) diff --git a/src/hooks/__tests__/session.test.ts b/src/hooks/__tests__/session.test.ts index 6e9c9df5a..15f0df3f2 100644 --- a/src/hooks/__tests__/session.test.ts +++ b/src/hooks/__tests__/session.test.ts @@ -3,11 +3,12 @@ import assert from 'node:assert/strict'; import { spawn } from 'node:child_process'; import { once } from 'node:events'; import { existsSync } from 'node:fs'; -import { link, mkdir, mkdtemp, readFile, readdir, readlink, rename, rm, rmdir, symlink, writeFile } from 'node:fs/promises'; +import { link, lstat, mkdir, mkdtemp, readFile, readdir, readlink, rename, rm, rmdir, symlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { __createDefaultPidProbeForTests, + __releasePointerLockForTests, appendPromptSessionProvenanceRejection, closeLaunchSessionBindingOnce, establishLaunchSessionBinding, @@ -917,20 +918,20 @@ describe('session pointer transaction', () => { try { const context = resolveSessionPointerContext(cwd); await writeLockOwner(cwd, validLockOwner()); - const renames: Array<[string, string]> = []; const links: Array<[string, string]> = []; + const unlinks: string[] = []; await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { - rename: async (from, to) => { - renames.push([from, to]); - await rename(from, to); - }, link: async (from, to) => { links.push([from, to]); await link(from, to); }, + unlink: async (path) => { + unlinks.push(path); + await rm(path); + }, }, }, async () => { const inspected = await inspectSessionPointerLock(cwd); @@ -947,10 +948,12 @@ describe('session pointer transaction', () => { assert.equal(repeated.action, 'none'); assert.equal(repeated.status, 'absent'); }); - assert.match(renames[0]![0], /owner\.json$/); - assert.match(renames[0]![1], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); - assert.match(links[0]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); - assert.equal(links[0]![1].includes('.quarantine.'), true); + assert.match(links[0]![0], /owner\.json$/); + assert.match(links[0]![1], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.match(links[1]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.equal(links[1]![1].includes('.quarantine.'), true); + assert.match(unlinks[0]!, /owner\.json$/); + assert.match(unlinks[1]!, new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); } finally { await rm(cwd, { recursive: true, force: true }); } @@ -994,21 +997,21 @@ describe('session pointer transaction', () => { token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { - rename: async (from, to) => { + link: async (from, to) => { if (!displaced && from === staleOwner) { displaced = true; await rename(context.lockPath, displacedPath); await mkdir(context.lockPath); await writeFile(successorTemp, JSON.stringify(validLockOwner({ token: SUCCESSOR_TOKEN })), 'utf-8'); } - await rename(from, to); + await link(from, to); }, }, }, async () => { const recovered = await recoverSessionPointerLock(cwd); assert.equal(recovered.recovered, false); assert.equal(recovered.action, 'none'); - assert.match(recovered.reason, /changed before recovery claim/i); + assert.match(recovered.reason, /Unable to create exact recovery claim \(ENOENT\)/); }); assert.equal(displaced, true); assert.deepEqual(await readdir(context.lockPath), [`owner.${SUCCESSOR_TOKEN}.tmp`]); @@ -1031,21 +1034,21 @@ describe('session pointer transaction', () => { token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { - rename: async (from, to) => { + link: async (from, to) => { if (!displaced && from === staleOwner) { displaced = true; await rename(context.lockPath, displacedPath); await mkdir(context.lockPath); await writeFile(staleOwner, successorOwner, 'utf-8'); } - await rename(from, to); + await link(from, to); }, }, }, async () => { const recovered = await recoverSessionPointerLock(cwd); assert.equal(recovered.recovered, false); assert.equal(recovered.action, 'none'); - assert.match(recovered.reason, /revalidation failed/i); + assert.match(recovered.reason, /changed before recovery claim/i); assert.equal(recovered.quarantinePath, undefined); }); assert.equal(displaced, true); @@ -1064,17 +1067,16 @@ describe('session pointer transaction', () => { const successorPid = process.pid + 100_000; const successor = JSON.stringify(validLockOwner({ token: SUCCESSOR_TOKEN, pid: successorPid })); let displaced = false; - let live = true; await writeLockOwner(cwd, validLockOwner()); - await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: (pid) => live && pid === successorPid ? 'alive' : 'dead', readProcessIdentity: () => matchingProcessIdentity(), fs: { - rename: async (from, to) => { + await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: (pid) => pid === successorPid ? 'alive' : 'dead', readProcessIdentity: () => matchingProcessIdentity(), fs: { + link: async (from, to) => { if (!displaced && from === ownerPath) { displaced = true; await rename(context.lockPath, `${context.lockPath}.displaced`); await mkdir(context.lockPath); await writeFile(ownerPath, successor, 'utf-8'); } - await rename(from, to); + await link(from, to); }, } }, async () => { assert.equal((await recoverSessionPointerLock(cwd)).recovered, false); @@ -1082,8 +1084,9 @@ describe('session pointer transaction', () => { assert.equal(inspected.status, 'live'); assert.equal(inspected.safeToRecover, false); assert.equal((await recoverSessionPointerLock(cwd)).status, 'live'); - live = false; - assert.equal((await recoverSessionPointerLock(cwd)).action, 'quarantined'); + assert.equal(await readFile(ownerPath, 'utf-8'), successor); + assert.deepEqual(await __releasePointerLockForTests(cwd, SUCCESSOR_TOKEN), []); + assert.equal(existsSync(context.lockPath), false); await writeSessionStart(cwd, 'sess-after-survival', { platform: 'win32' }); assert.equal((await readSessionState(cwd))?.session_id, 'sess-after-survival'); }); @@ -1100,9 +1103,9 @@ describe('session pointer transaction', () => { const marker = JSON.stringify(validLockOwner({ token: SUCCESSOR_TOKEN })); let replaced = false; await writeLockOwner(cwd, validLockOwner()); - await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { rename: async (from, to) => { + await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { link: async (from, to) => { if (!replaced && from === ownerPath) { replaced = true; await rm(context.lockPath, { recursive: true }); await writeFile(foreignOwner, marker); await symlink(foreign, context.lockPath); } - await rename(from, to); + await link(from, to); } } }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, false)); assert.equal(await readFile(foreignOwner, 'utf-8'), marker); assert.equal((await readdir(foreign)).some((entry) => entry.includes('.quarantine.')), false); @@ -1119,16 +1122,16 @@ describe('session pointer transaction', () => { let replaced = false; await writeLockOwner(cwd, validLockOwner()); await writeFile(foreign, marker); - await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { rename: async (from, to) => { + await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { link: async (from, to) => { if (!replaced && from === ownerPath) { replaced = true; await rm(ownerPath); await symlink(foreign, ownerPath); } - await rename(from, to); + await link(from, to); } } }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, false)); assert.equal(await readFile(foreign, 'utf-8'), marker); assert.equal(await readlink(ownerPath), foreign); } finally { await rm(cwd, { recursive: true, force: true }); } }); - it('restores a non-file evidence replacement instead of stranding it', async () => { + it('preserves a non-file evidence replacement as exact recovery residue without clobbering', async () => { const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-evidence-directory-')); try { const context = resolveSessionPointerContext(cwd); @@ -1136,9 +1139,9 @@ describe('session pointer transaction', () => { const marker = join(ownerPath, 'marker'); let replaced = false; await writeLockOwner(cwd, validLockOwner()); - await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { rename: async (from, to) => { + await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { link: async (from, to) => { if (!replaced && from === ownerPath) { replaced = true; await rm(ownerPath); await mkdir(ownerPath); await writeFile(marker, 'directory marker'); } - await rename(from, to); + await link(from, to); } } }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, false)); assert.equal(await readFile(marker, 'utf-8'), 'directory marker'); } finally { await rm(cwd, { recursive: true, force: true }); } @@ -1158,7 +1161,7 @@ describe('session pointer transaction', () => { } } }, async () => { const recovered = await recoverSessionPointerLock(cwd); assert.equal(recovered.recovered, false); - assert.match(recovered.reason, /Unable to quarantine the exact session pointer recovery claim/); + assert.match(recovered.reason, /Recovery quarantine path already exists/); }); assert.equal(await readFile(ownerPath, 'utf-8'), owner); assert.equal((await readdir(context.lockPath)).some((entry) => entry.includes('.recovery')), false); @@ -1166,6 +1169,225 @@ describe('session pointer transaction', () => { } finally { await rm(cwd, { recursive: true, force: true }); } }); + it('fails closed without mutation when a recovery claim destination appears before the no-clobber claim link', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-claim-exist-')); + try { + const context = resolveSessionPointerContext(cwd); + const staleOwner = JSON.stringify(validLockOwner()); + await writeLockOwner(cwd, validLockOwner()); + const claimPath = join(context.lockPath, `owner.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.recovery`); + const foreignMarker = 'foreign claim destination'; + await withPointerDependencies({ + token: () => SUCCESSOR_TOKEN, + probePid: () => 'dead', + fs: { + link: async (from, to) => { + if (to === claimPath) await writeFile(to, foreignMarker, 'utf-8'); + await link(from, to); + }, + }, + }, async () => { + const recovered = await recoverSessionPointerLock(cwd); + assert.equal(recovered.recovered, false); + assert.equal(recovered.action, 'none'); + assert.match(recovered.reason, /changed before recovery claim/i); + }); + assert.equal(await readFile(join(context.lockPath, 'owner.json'), 'utf-8'), staleOwner); + assert.equal(await readFile(claimPath, 'utf-8'), foreignMarker); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + it('does not overwrite a rollback destination that appears before the no-clobber rollback link', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-rollback-exist-')); + try { + const context = resolveSessionPointerContext(cwd); + const staleOwner = JSON.stringify(validLockOwner()); + await writeLockOwner(cwd, validLockOwner()); + const ownerPath = join(context.lockPath, 'owner.json'); + const claimPath = join(context.lockPath, `owner.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.recovery`); + const foreignMarker = 'foreign rollback destination'; + let armed = false; + await withPointerDependencies({ + token: () => SUCCESSOR_TOKEN, + probePid: () => 'dead', + fs: { + unlink: async (path) => { + await rm(path, { force: false }); + if (path === ownerPath) armed = true; + }, + readdir: async (path) => { + const entries = await readdir(path); + if (armed && path === context.lockPath) entries.push('foreign-extra'); + return entries; + }, + link: async (from, to) => { + if (to === ownerPath) await writeFile(to, foreignMarker, 'utf-8'); + await link(from, to); + }, + }, + }, async () => { + const recovered = await recoverSessionPointerLock(cwd); + assert.equal(recovered.recovered, false); + assert.equal(recovered.action, 'none'); + assert.match(recovered.reason, /revalidation failed/i); + assert.match(recovered.reason, /reappeared during rollback/i); + }); + assert.equal(await readFile(ownerPath, 'utf-8'), foreignMarker); + assert.equal(await readFile(claimPath, 'utf-8'), staleOwner); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + it('never unlinks a quarantine claim whose identity changed after the no-clobber quarantine link', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-claim-swap-')); + try { + const context = resolveSessionPointerContext(cwd); + const staleOwner = JSON.stringify(validLockOwner()); + await writeLockOwner(cwd, validLockOwner()); + const claimPath = join(context.lockPath, `owner.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.recovery`); + const quarantinePath = `${context.lockPath}.quarantine.${TEST_TOKEN}.${SUCCESSOR_TOKEN}`; + const foreignMarker = 'foreign swapped claim'; + let armed = false; + await withPointerDependencies({ + token: () => SUCCESSOR_TOKEN, + probePid: () => 'dead', + fs: { + link: async (from, to) => { + await link(from, to); + if (to === quarantinePath) armed = true; + }, + lstat: async (path) => { + if (armed && path === claimPath) { + await rm(claimPath); + await writeFile(claimPath, foreignMarker, 'utf-8'); + } + return await lstat(path); + }, + }, + }, async () => { + const recovered = await recoverSessionPointerLock(cwd); + assert.equal(recovered.recovered, false); + assert.equal(recovered.action, 'none'); + assert.match(recovered.reason, /identity changed/i); + assert.equal(recovered.quarantinePath, quarantinePath); + }); + assert.equal(await readFile(claimPath, 'utf-8'), foreignMarker); + assert.equal(await readFile(quarantinePath, 'utf-8'), staleOwner); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + it('fails closed without mutation when evidence lstat fails non-ENOENT before the claim', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-lstat-preclaim-')); + try { + const context = resolveSessionPointerContext(cwd); + const staleOwner = JSON.stringify(validLockOwner()); + await writeLockOwner(cwd, validLockOwner()); + const ownerPath = join(context.lockPath, 'owner.json'); + const claimPath = join(context.lockPath, `owner.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.recovery`); + let armed = false; + await withPointerDependencies({ + token: () => SUCCESSOR_TOKEN, + probePid: () => 'dead', + fs: { + lstat: async (path) => { + if (path === claimPath) armed = true; + if (armed && path === ownerPath) throw codedError('EACCES'); + return await lstat(path); + }, + }, + }, async () => { + const recovered = await recoverSessionPointerLock(cwd); + assert.equal(recovered.recovered, false); + assert.equal(recovered.action, 'none'); + assert.match(recovered.reason, /changed before recovery claim/i); + }); + assert.deepEqual(await readdir(context.lockPath), ['owner.json']); + assert.equal(await readFile(ownerPath, 'utf-8'), staleOwner); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + it('restores evidence and keeps the exact quarantine inspect diagnostic when quarantine lstat fails non-ENOENT', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-lstat-quarantine-')); + try { + const context = resolveSessionPointerContext(cwd); + const staleOwner = JSON.stringify(validLockOwner()); + await writeLockOwner(cwd, validLockOwner()); + const ownerPath = join(context.lockPath, 'owner.json'); + const quarantinePath = `${context.lockPath}.quarantine.${TEST_TOKEN}.${SUCCESSOR_TOKEN}`; + let armed = false; + await withPointerDependencies({ + token: () => SUCCESSOR_TOKEN, + probePid: () => 'dead', + fs: { + unlink: async (path) => { + await rm(path, { force: false }); + if (path === ownerPath) armed = true; + }, + lstat: async (path) => { + if (armed && path === quarantinePath) throw codedError('EACCES'); + return await lstat(path); + }, + }, + }, async () => { + const recovered = await recoverSessionPointerLock(cwd); + assert.equal(recovered.recovered, false); + assert.equal(recovered.action, 'none'); + assert.match(recovered.reason, /Unable to inspect recovery quarantine path/); + assert.match(recovered.reason, /rolled back/i); + }); + assert.deepEqual(await readdir(context.lockPath), ['owner.json']); + assert.equal(await readFile(ownerPath, 'utf-8'), staleOwner); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + it('never removes an empty replacement lock directory whose identity differs from the validated one', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-empty-replace-')); + try { + const context = resolveSessionPointerContext(cwd); + const staleOwner = JSON.stringify(validLockOwner()); + await writeLockOwner(cwd, validLockOwner()); + const claimPath = join(context.lockPath, `owner.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.recovery`); + const quarantinePath = `${context.lockPath}.quarantine.${TEST_TOKEN}.${SUCCESSOR_TOKEN}`; + let armed = false; + await withPointerDependencies({ + token: () => SUCCESSOR_TOKEN, + probePid: () => 'dead', + fs: { + unlink: async (path) => { + await rm(path, { force: false }); + if (path === claimPath) armed = true; + }, + lstat: async (path) => { + if (armed && path === context.lockPath) { + const replacementPath = `${context.lockPath}.replacement`; + await mkdir(replacementPath); + await rename(replacementPath, context.lockPath); + } + return await lstat(path); + }, + }, + }, async () => { + const recovered = await recoverSessionPointerLock(cwd); + assert.equal(recovered.recovered, false); + assert.equal(recovered.action, 'none'); + assert.match(recovered.reason, /replaced before removal/i); + assert.equal(recovered.quarantinePath, quarantinePath); + }); + assert.deepEqual(await readdir(context.lockPath), []); + assert.equal(await readFile(quarantinePath, 'utf-8'), staleOwner); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); it('distinguishes a paused live pre-rename owner from the same owner after SIGKILL', async (t) => { if (process.platform !== 'linux') { t.skip('Linux process start identity is required for deterministic PID reuse protection.'); @@ -1242,24 +1464,24 @@ describe('session pointer transaction', () => { const context = resolveSessionPointerContext(cwd); await mkdir(context.lockPath, { recursive: true }); await writeFile(join(context.lockPath, `owner.${TEST_TOKEN}.tmp`), JSON.stringify(validLockOwner()), 'utf-8'); - const renames: Array<[string, string]> = []; const links: Array<[string, string]> = []; + const unlinks: string[] = []; let successorBlocked = false; await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { - rename: async (from, to) => { - renames.push([from, to]); - await rename(from, to); - if (from.endsWith(`owner.${TEST_TOKEN}.tmp`)) { + link: async (from, to) => { + links.push([from, to]); + await link(from, to); + if (from.endsWith(`owner.${TEST_TOKEN}.tmp`) && to.endsWith('.recovery')) { await assert.rejects(mkdir(context.lockPath), { code: 'EEXIST' }); successorBlocked = true; } }, - link: async (from, to) => { - links.push([from, to]); - await link(from, to); + unlink: async (path) => { + unlinks.push(path); + await rm(path); }, }, }, async () => { @@ -1267,10 +1489,12 @@ describe('session pointer transaction', () => { assert.equal(recovered.recovered, true); }); assert.equal(successorBlocked, true); - assert.match(renames[0]![0], /owner\.transaction_token_123456\.tmp$/); - assert.match(renames[0]![1], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); - assert.match(links[0]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); - assert.equal(links[0]![1].includes('.quarantine.'), true); + assert.match(links[0]![0], /owner\.transaction_token_123456\.tmp$/); + assert.match(links[0]![1], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.match(links[1]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.equal(links[1]![1].includes('.quarantine.'), true); + assert.match(unlinks[0]!, /owner\.transaction_token_123456\.tmp$/); + assert.match(unlinks[1]!, new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); } finally { await rm(cwd, { recursive: true, force: true }); } @@ -1290,21 +1514,21 @@ describe('session pointer transaction', () => { token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { - rename: async (from, to) => { + link: async (from, to) => { if (!displaced && from === staleTemp) { displaced = true; await rename(context.lockPath, displacedPath); await mkdir(context.lockPath); await writeFile(successorTemp, JSON.stringify(validLockOwner({ token: SUCCESSOR_TOKEN })), 'utf-8'); } - await rename(from, to); + await link(from, to); }, }, }, async () => { const recovered = await recoverSessionPointerLock(cwd); assert.equal(recovered.recovered, false); assert.equal(recovered.action, 'none'); - assert.match(recovered.reason, /changed before recovery claim/i); + assert.match(recovered.reason, /Unable to create exact recovery claim \(ENOENT\)/); }); assert.equal(displaced, true); assert.deepEqual(await readdir(context.lockPath), [`owner.${SUCCESSOR_TOKEN}.tmp`]); diff --git a/src/hooks/session.ts b/src/hooks/session.ts index deb70f24e..2350562cd 100644 --- a/src/hooks/session.ts +++ b/src/hooks/session.ts @@ -397,6 +397,8 @@ export interface SessionPointerFsDependencies { mkdir(path: string, options?: { recursive?: boolean }): Promise; readdir(path: string): Promise; lstat(path: string): Promise<{ + dev: number; + ino: number; isDirectory(): boolean; isFile(): boolean; isSymbolicLink(): boolean; @@ -994,182 +996,103 @@ export async function inspectSessionPointerLock(cwd: string): Promise { +async function revalidateRecoveryEvidence(lockPath: string, evidenceName: string): Promise< + | { ok: true; lockStat: { dev: number; ino: number }; evidenceStat: { dev: number; ino: number } } + | { ok: false; reason: string } +> { try { const lockStat = await transactionDependencies.fs.lstat(lockPath); - if (lockStat.isSymbolicLink() || !lockStat.isDirectory()) { - return 'Session pointer lock evidence changed before recovery claim.'; - } const evidenceStat = await transactionDependencies.fs.lstat(join(lockPath, evidenceName)); - if (evidenceStat.isSymbolicLink() || !evidenceStat.isFile()) { - return 'Session pointer lock evidence changed before recovery claim.'; - } + if (lockStat.isSymbolicLink() || !lockStat.isDirectory() || evidenceStat.isSymbolicLink() || !evidenceStat.isFile()) return { ok: false, reason: 'Session pointer lock evidence changed before recovery claim.' }; + return { ok: true, lockStat, evidenceStat }; } catch { - return 'Session pointer lock evidence changed before recovery claim.'; + return { ok: false, reason: 'Session pointer lock evidence changed before recovery claim.' }; } - return undefined; } -async function rollbackRecoveryClaim(lockPath: string, evidenceName: string, claimName: string): Promise<{ ok: boolean; reason?: string }> { +async function rollbackRecoveryClaim(lockPath: string, evidenceName: string, claimName: string, claimIdentity: { dev: number; ino: number }): Promise<{ ok: boolean; reason?: string }> { const evidencePath = join(lockPath, evidenceName); const claimPath = join(lockPath, claimName); - try { - await transactionDependencies.fs.lstat(evidencePath); - return { ok: false, reason: 'the original evidence name reappeared' }; - } catch (error) { - if (!isNotFound(error)) return { ok: false, reason: 'unable to inspect the original evidence name' }; - } - try { - await transactionDependencies.fs.link(claimPath, evidencePath); - } catch (error) { + try { await transactionDependencies.fs.lstat(evidencePath); return { ok: false, reason: 'the original evidence name reappeared' }; } catch (error) { if (!isNotFound(error)) return { ok: false, reason: 'unable to inspect the original evidence name' }; } + try { await transactionDependencies.fs.link(claimPath, evidencePath); } catch (error) { if (isAlreadyExists(error)) return { ok: false, reason: 'the original evidence name reappeared during rollback' }; - try { - await transactionDependencies.fs.lstat(evidencePath); - return { ok: false, reason: 'the original evidence name reappeared during rollback' }; - } catch { - try { - await transactionDependencies.fs.rename(claimPath, evidencePath); - return { ok: true }; - } catch { - return { ok: false, reason: 'unable to restore the exact claim' }; - } - } + return { ok: false, reason: `the exact claim cannot be restored on this filesystem (${errorCode(error) ?? 'unknown'}); it was left in place under its recovery name` }; } try { - await transactionDependencies.fs.unlink(claimPath); - return { ok: true }; - } catch (error) { - if (isNotFound(error)) return { ok: true }; - return { ok: false, reason: 'unable to remove the rolled-back claim name' }; - } + const [claimStat, evidenceStat] = await Promise.all([transactionDependencies.fs.lstat(claimPath), transactionDependencies.fs.lstat(evidencePath)]); + if (claimStat.dev !== claimIdentity.dev || claimStat.ino !== claimIdentity.ino || evidenceStat.dev !== claimIdentity.dev || evidenceStat.ino !== claimIdentity.ino) return { ok: false, reason: 'claim identity changed during rollback; the exact claim was left in place under its recovery name' }; + } catch { return { ok: false, reason: 'claim identity changed during rollback; the exact claim was left in place under its recovery name' }; } + try { await transactionDependencies.fs.unlink(claimPath); return { ok: true }; } catch (error) { return isNotFound(error) ? { ok: true } : { ok: false, reason: 'unable to remove the rolled-back claim name' }; } } function recoveryRollbackReason(prefix: string, rollback: { ok: boolean; reason?: string }): string { - return rollback.ok - ? `${prefix}; the exact claim was rolled back to its original evidence name without modifying any other bytes.` - : `${prefix}; rollback was not possible: ${rollback.reason}. The exact claim was left in place under its recovery name.`; + return rollback.ok ? `${prefix}; the exact claim was rolled back to its original evidence name without modifying any other bytes.` : `${prefix}; rollback was not possible: ${rollback.reason}. The exact claim was left in place under its recovery name.`; } /** Explicitly quarantine only a dead, atomically claimed pointer lock; never force recovery. */ export async function recoverSessionPointerLock(cwd: string): Promise { const context = resolveSessionPointerContext(cwd); const inspection = await inspectSessionPointerLockAtContext(context); - if (!inspection.safeToRecover) { - return { ...inspection, action: 'none', recovered: false, reason: inspection.status === 'absent' ? 'No session pointer lock exists.' : `Session pointer lock is not safe to recover (${inspection.status}).` }; - } - + if (!inspection.safeToRecover) return { ...inspection, action: 'none', recovered: false, reason: inspection.status === 'absent' ? 'No session pointer lock exists.' : `Session pointer lock is not safe to recover (${inspection.status}).` }; let entries: string[]; - try { - entries = await transactionDependencies.fs.readdir(context.lockPath); - } catch { - return { ...inspection, action: 'none', recovered: false, reason: 'Unable to re-read session pointer lock evidence.' }; - } + try { entries = await transactionDependencies.fs.readdir(context.lockPath); } catch { return { ...inspection, action: 'none', recovered: false, reason: 'Unable to re-read session pointer lock evidence.' }; } const evidenceName = inspection.evidenceSource === 'owner.json' ? 'owner.json' : entries.find((entry) => /^owner\.([A-Za-z0-9_-]{16,128})\.tmp$/.test(entry)); - if (!evidenceName || entries.length !== 1) { - return { ...inspection, action: 'none', recovered: false, reason: 'Session pointer lock evidence changed before recovery claim.' }; - } - const evidenceReason = await revalidateRecoveryEvidence(context.lockPath, evidenceName); - if (evidenceReason) return { ...inspection, action: 'none', recovered: false, reason: evidenceReason }; + if (!evidenceName || entries.length !== 1) return { ...inspection, action: 'none', recovered: false, reason: 'Session pointer lock evidence changed before recovery claim.' }; + const evidence = await revalidateRecoveryEvidence(context.lockPath, evidenceName); + if (!evidence.ok) return { ...inspection, action: 'none', recovered: false, reason: evidence.reason }; const owner = await inspectLockOwnerFile(join(context.lockPath, evidenceName)); - if (owner.status !== 'dead' || !owner.owner || (inspection.evidenceSource === 'owner-temp' && owner.owner.token !== /^owner\.([A-Za-z0-9_-]{16,128})\.tmp$/.exec(evidenceName)?.[1])) { - return { ...inspection, action: 'none', recovered: false, reason: 'Session pointer lock evidence changed before recovery claim.' }; - } - + if (owner.status !== 'dead' || !owner.owner || (inspection.evidenceSource === 'owner-temp' && owner.owner.token !== /^owner\.([A-Za-z0-9_-]{16,128})\.tmp$/.exec(evidenceName)?.[1])) return { ...inspection, action: 'none', recovered: false, reason: 'Session pointer lock evidence changed before recovery claim.' }; let claimToken: string; - try { - claimToken = transactionDependencies.token(); - } catch { - return { ...inspection, action: 'none', recovered: false, reason: 'Unable to create recovery claim token.' }; - } + try { claimToken = transactionDependencies.token(); } catch { return { ...inspection, action: 'none', recovered: false, reason: 'Unable to create recovery claim token.' }; } if (!isValidToken(claimToken)) return { ...inspection, action: 'none', recovered: false, reason: 'Recovery claim token is invalid.' }; const claimName = `owner.${owner.owner.token}.${claimToken}.recovery`; + const evidencePath = join(context.lockPath, evidenceName); const claimPath = join(context.lockPath, claimName); - try { - await transactionDependencies.fs.lstat(claimPath); - return { ...inspection, action: 'none', recovered: false, reason: 'Recovery claim path already exists.' }; - } catch (error) { - if (!isNotFound(error)) return { ...inspection, action: 'none', recovered: false, reason: 'Unable to inspect recovery claim path.' }; - } - const preClaimReason = await revalidateRecoveryEvidence(context.lockPath, evidenceName); - if (preClaimReason) return { ...inspection, action: 'none', recovered: false, reason: preClaimReason }; - try { - await transactionDependencies.fs.rename(join(context.lockPath, evidenceName), claimPath); - } catch { - return { ...inspection, action: 'none', recovered: false, reason: 'Session pointer lock evidence changed before recovery claim.' }; - } - + try { await transactionDependencies.fs.lstat(claimPath); return { ...inspection, action: 'none', recovered: false, reason: 'Recovery claim path already exists.' }; } catch (error) { if (!isNotFound(error)) return { ...inspection, action: 'none', recovered: false, reason: 'Unable to inspect recovery claim path.' }; } + const preClaim = await revalidateRecoveryEvidence(context.lockPath, evidenceName); + if (!preClaim.ok) return { ...inspection, action: 'none', recovered: false, reason: preClaim.reason }; + try { await transactionDependencies.fs.link(evidencePath, claimPath); } catch (error) { return { ...inspection, action: 'none', recovered: false, reason: isAlreadyExists(error) ? 'Session pointer lock evidence changed before recovery claim.' : `Unable to create exact recovery claim (${errorCode(error) ?? 'unknown'}).` }; } + let postLinkClaim: Awaited>; + try { + const [currentEvidence, currentClaim] = await Promise.all([transactionDependencies.fs.lstat(evidencePath), transactionDependencies.fs.lstat(claimPath)]); + postLinkClaim = currentClaim; + if (currentEvidence.dev !== preClaim.evidenceStat.dev || currentEvidence.ino !== preClaim.evidenceStat.ino || currentClaim.dev !== preClaim.evidenceStat.dev || currentClaim.ino !== preClaim.evidenceStat.ino) { + try { const currentClaimAgain = await transactionDependencies.fs.lstat(claimPath); if (currentClaimAgain.dev === currentClaim.dev && currentClaimAgain.ino === currentClaim.ino) await transactionDependencies.fs.unlink(claimPath); } catch { /* Leave uncertain residue fail-closed. */ } + return { ...inspection, action: 'none', recovered: false, reason: 'Session pointer lock evidence changed before recovery claim.' }; + } + } catch { return { ...inspection, action: 'none', recovered: false, reason: 'Session pointer lock evidence changed before recovery claim.' }; } + try { await transactionDependencies.fs.unlink(evidencePath); } catch (error) { if (!isNotFound(error)) return { ...inspection, action: 'none', recovered: false, reason: `The exact recovery claim was created, but its original evidence name could not be removed (${errorCode(error) ?? 'unknown'}).` }; } + const claimIdentity = { dev: postLinkClaim.dev, ino: postLinkClaim.ino }; const claimed = await inspectSessionPointerLockAtContext(context); - let claimedEntries: string[] | undefined; - try { - claimedEntries = await transactionDependencies.fs.readdir(context.lockPath); - } catch { - // Roll back below. + if (claimed.status !== 'unexpected') { + const rollback = await rollbackRecoveryClaim(context.lockPath, evidenceName, claimName, claimIdentity); + return { ...claimed, action: 'none', recovered: false, reason: recoveryRollbackReason('Recovery claim revalidation failed', rollback) }; } + let claimedEntries: string[] | undefined; let claimedClaimLstat: Awaited> | undefined; - try { - claimedClaimLstat = await transactionDependencies.fs.lstat(claimPath); - } catch { - // Roll back below. - } - const claimOwner = claimedClaimLstat && !claimedClaimLstat.isSymbolicLink() && claimedClaimLstat.isFile() - ? await inspectLockOwnerFile(claimPath) - : { status: 'unexpected' as const }; - if (!claimedEntries || claimedEntries.length !== 1 || claimedEntries[0] !== claimName || claimOwner.status !== 'dead' || claimOwner.owner?.token !== owner.owner.token || claimed.status !== 'unexpected' || !claimedClaimLstat || claimedClaimLstat.isSymbolicLink() || !claimedClaimLstat.isFile()) { - const rollback = await rollbackRecoveryClaim(context.lockPath, evidenceName, claimName); + try { claimedEntries = await transactionDependencies.fs.readdir(context.lockPath); } catch { /* Roll back below. */ } + try { claimedClaimLstat = await transactionDependencies.fs.lstat(claimPath); } catch { /* Roll back below. */ } + const claimOwner = claimedClaimLstat && !claimedClaimLstat.isSymbolicLink() && claimedClaimLstat.isFile() ? await inspectLockOwnerFile(claimPath) : { status: 'unexpected' as const }; + if (!claimedEntries || claimedEntries.length !== 1 || claimedEntries[0] !== claimName || claimOwner.status !== 'dead' || claimOwner.owner?.token !== owner.owner.token || !claimedClaimLstat || claimedClaimLstat.isSymbolicLink() || !claimedClaimLstat.isFile() || claimedClaimLstat.dev !== claimIdentity.dev || claimedClaimLstat.ino !== claimIdentity.ino) { + const rollback = await rollbackRecoveryClaim(context.lockPath, evidenceName, claimName, claimIdentity); return { ...claimed, action: 'none', recovered: false, reason: recoveryRollbackReason('Recovery claim revalidation failed', rollback) }; } - const quarantinePath = `${context.lockPath}.quarantine.${owner.owner.token}.${claimToken}`; - try { - const lockStat = await transactionDependencies.fs.lstat(context.lockPath); - const claimStat = await transactionDependencies.fs.lstat(claimPath); - if (lockStat.isSymbolicLink() || !lockStat.isDirectory() || claimStat.isSymbolicLink() || !claimStat.isFile()) { - throw new Error('recovery evidence changed'); - } - try { - await transactionDependencies.fs.lstat(quarantinePath); - throw new Error('recovery quarantine path exists'); - } catch (error) { - if (!isNotFound(error)) throw error; - } - } catch { - const rollback = await rollbackRecoveryClaim(context.lockPath, evidenceName, claimName); - return { ...claimed, action: 'none', recovered: false, reason: recoveryRollbackReason('Unable to quarantine the exact session pointer recovery claim', rollback) }; - } - try { - await transactionDependencies.fs.link(claimPath, quarantinePath); - } catch { - const rollback = await rollbackRecoveryClaim(context.lockPath, evidenceName, claimName); - return { ...claimed, action: 'none', recovered: false, reason: recoveryRollbackReason('Unable to quarantine the exact session pointer recovery claim', rollback) }; - } - try { - await transactionDependencies.fs.unlink(claimPath); - } catch (error) { - if (!isNotFound(error)) { - return { ...claimed, action: 'none', recovered: false, reason: 'The exact recovery claim was quarantined, but its recovery name could not be removed.', quarantinePath }; - } - } - try { - const lockStat = await transactionDependencies.fs.lstat(context.lockPath); - if (lockStat.isSymbolicLink() || !lockStat.isDirectory()) { - return { ...claimed, action: 'none', recovered: false, reason: 'The exact recovery claim was quarantined, but the canonical lock directory was replaced before removal.', quarantinePath }; - } - } catch { - return { ...claimed, action: 'none', recovered: false, reason: 'The exact recovery claim was quarantined, but the canonical lock directory was replaced before removal.', quarantinePath }; - } - try { - await transactionDependencies.fs.rmdir(context.lockPath); - } catch (error) { - if (!isNotFound(error)) { - return { - ...claimed, - action: 'none', - recovered: false, - reason: 'The exact recovery claim was quarantined, but the canonical lock directory was not empty.', - quarantinePath, - }; - } - } + let quarantineFailure: string | undefined; + try { + const [lockStat, claimStat] = await Promise.all([transactionDependencies.fs.lstat(context.lockPath), transactionDependencies.fs.lstat(claimPath)]); + if (lockStat.isSymbolicLink() || !lockStat.isDirectory() || lockStat.dev !== preClaim.lockStat.dev || lockStat.ino !== preClaim.lockStat.ino || claimStat.isSymbolicLink() || !claimStat.isFile() || claimStat.dev !== claimIdentity.dev || claimStat.ino !== claimIdentity.ino) quarantineFailure = 'Unable to quarantine the exact session pointer recovery claim'; + } catch { quarantineFailure = 'Unable to quarantine the exact session pointer recovery claim'; } + if (!quarantineFailure) try { await transactionDependencies.fs.lstat(quarantinePath); quarantineFailure = 'Recovery quarantine path already exists.'; } catch (error) { if (!isNotFound(error)) quarantineFailure = 'Unable to inspect recovery quarantine path.'; } + if (quarantineFailure) { const rollback = await rollbackRecoveryClaim(context.lockPath, evidenceName, claimName, claimIdentity); return { ...claimed, action: 'none', recovered: false, reason: recoveryRollbackReason(quarantineFailure, rollback) }; } + try { await transactionDependencies.fs.link(claimPath, quarantinePath); } catch (error) { const rollback = await rollbackRecoveryClaim(context.lockPath, evidenceName, claimName, claimIdentity); return { ...claimed, action: 'none', recovered: false, reason: recoveryRollbackReason(isAlreadyExists(error) ? 'Recovery quarantine path already exists.' : `Unable to quarantine the exact session pointer recovery claim (${errorCode(error) ?? 'unknown'})`, rollback) }; } + try { + const [quarantineStat, claimStat] = await Promise.all([transactionDependencies.fs.lstat(quarantinePath), transactionDependencies.fs.lstat(claimPath)]); + if (quarantineStat.dev !== claimIdentity.dev || quarantineStat.ino !== claimIdentity.ino || claimStat.dev !== claimIdentity.dev || claimStat.ino !== claimIdentity.ino) return { ...claimed, action: 'none', recovered: false, reason: 'The exact recovery claim was quarantined, but its recovery name could not be removed because its identity changed.', quarantinePath }; + } catch { return { ...claimed, action: 'none', recovered: false, reason: 'The exact recovery claim was quarantined, but its recovery name could not be removed because its identity changed.', quarantinePath }; } + try { await transactionDependencies.fs.unlink(claimPath); } catch (error) { if (!isNotFound(error)) return { ...claimed, action: 'none', recovered: false, reason: 'The exact recovery claim was quarantined, but its recovery name could not be removed.', quarantinePath }; } + try { const lockStat = await transactionDependencies.fs.lstat(context.lockPath); if (lockStat.isSymbolicLink() || !lockStat.isDirectory() || lockStat.dev !== preClaim.lockStat.dev || lockStat.ino !== preClaim.lockStat.ino) throw new Error('replaced'); } catch { return { ...claimed, action: 'none', recovered: false, reason: 'The exact recovery claim was quarantined, but the canonical lock directory was replaced before removal.', quarantinePath }; } + try { await transactionDependencies.fs.rmdir(context.lockPath); } catch (error) { if (!isNotFound(error)) return { ...claimed, action: 'none', recovered: false, reason: 'The exact recovery claim was quarantined, but the canonical lock directory was not empty.', quarantinePath }; } return { ...inspection, action: 'quarantined', recovered: true, reason: 'Dead session pointer lock was atomically claimed and quarantined.', quarantinePath }; } @@ -1469,6 +1392,11 @@ async function releasePointerLock(lock: HeldPointerLock): Promise { + return await releasePointerLock({ context: resolveSessionPointerContext(cwd), token }); +} + function recoveryAbort( context: SessionPointerContext, primary: ResolvedSessionPointerAbort, From 643e7e687a5a939913d7a2f00097257cf09f47a3 Mon Sep 17 00:00:00 2001 From: Yeachan-Heo Date: Thu, 23 Jul 2026 01:01:32 +0000 Subject: [PATCH 4/7] fix(hooks): preserve parked lock recovery races --- src/hooks/__tests__/session.test.ts | 115 +++++++++++++++------------- src/hooks/session.ts | 108 +++++++++++++++++++++----- 2 files changed, 152 insertions(+), 71 deletions(-) diff --git a/src/hooks/__tests__/session.test.ts b/src/hooks/__tests__/session.test.ts index 15f0df3f2..ae71f7080 100644 --- a/src/hooks/__tests__/session.test.ts +++ b/src/hooks/__tests__/session.test.ts @@ -919,7 +919,7 @@ describe('session pointer transaction', () => { const context = resolveSessionPointerContext(cwd); await writeLockOwner(cwd, validLockOwner()); const links: Array<[string, string]> = []; - const unlinks: string[] = []; + const renames: Array<[string, string]> = []; await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', @@ -928,9 +928,9 @@ describe('session pointer transaction', () => { links.push([from, to]); await link(from, to); }, - unlink: async (path) => { - unlinks.push(path); - await rm(path); + rename: async (from, to) => { + renames.push([from, to]); + await rename(from, to); }, }, }, async () => { @@ -952,8 +952,9 @@ describe('session pointer transaction', () => { assert.match(links[0]![1], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); assert.match(links[1]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); assert.equal(links[1]![1].includes('.quarantine.'), true); - assert.match(unlinks[0]!, /owner\.json$/); - assert.match(unlinks[1]!, new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.match(renames[0]![0], /owner\.json$/); + assert.match(renames[1]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.equal(renames[2]![0], context.lockPath); } finally { await rm(cwd, { recursive: true, force: true }); } @@ -1213,9 +1214,9 @@ describe('session pointer transaction', () => { token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { - unlink: async (path) => { - await rm(path, { force: false }); - if (path === ownerPath) armed = true; + rename: async (from, to) => { + await rename(from, to); + if (from === ownerPath) armed = true; }, readdir: async (path) => { const entries = await readdir(path); @@ -1241,7 +1242,7 @@ describe('session pointer transaction', () => { } }); - it('never unlinks a quarantine claim whose identity changed after the no-clobber quarantine link', async () => { + it('preserves a swapped parked quarantine claim rather than unlinking it', async () => { const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-claim-swap-')); try { const context = resolveSessionPointerContext(cwd); @@ -1249,32 +1250,36 @@ describe('session pointer transaction', () => { await writeLockOwner(cwd, validLockOwner()); const claimPath = join(context.lockPath, `owner.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.recovery`); const quarantinePath = `${context.lockPath}.quarantine.${TEST_TOKEN}.${SUCCESSOR_TOKEN}`; - const foreignMarker = 'foreign swapped claim'; - let armed = false; + const foreignMarker = 'foreign parked claim'; + let parkedPath: string | undefined; + let parkedIdentity: { dev: number; ino: number } | undefined; await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { - link: async (from, to) => { - await link(from, to); - if (to === quarantinePath) armed = true; - }, - lstat: async (path) => { - if (armed && path === claimPath) { - await rm(claimPath); - await writeFile(claimPath, foreignMarker, 'utf-8'); + rename: async (from, to) => { + await rename(from, to); + if (from === claimPath) { + parkedPath = to; + await rename(to, `${to}.original`); + await writeFile(to, foreignMarker, 'utf-8'); + const stat = await lstat(to); + parkedIdentity = { dev: stat.dev, ino: stat.ino }; } - return await lstat(path); }, }, }, async () => { const recovered = await recoverSessionPointerLock(cwd); assert.equal(recovered.recovered, false); assert.equal(recovered.action, 'none'); - assert.match(recovered.reason, /identity changed/i); + assert.match(recovered.reason, /parked pathname no longer holds the validated identity/i); assert.equal(recovered.quarantinePath, quarantinePath); }); - assert.equal(await readFile(claimPath, 'utf-8'), foreignMarker); + assert.ok(parkedPath); + assert.ok(parkedIdentity); + const parkedStat = await lstat(parkedPath); + assert.deepEqual({ dev: parkedStat.dev, ino: parkedStat.ino }, parkedIdentity); + assert.equal(await readFile(parkedPath, 'utf-8'), foreignMarker); assert.equal(await readFile(quarantinePath, 'utf-8'), staleOwner); } finally { await rm(cwd, { recursive: true, force: true }); @@ -1326,9 +1331,9 @@ describe('session pointer transaction', () => { token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { - unlink: async (path) => { - await rm(path, { force: false }); - if (path === ownerPath) armed = true; + rename: async (from, to) => { + await rename(from, to); + if (from === ownerPath) armed = true; }, lstat: async (path) => { if (armed && path === quarantinePath) throw codedError('EACCES'); @@ -1349,7 +1354,7 @@ describe('session pointer transaction', () => { } }); - it('never removes an empty replacement lock directory whose identity differs from the validated one', async () => { + it('preserves a swapped parked lock directory rather than removing it', async () => { const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-empty-replace-')); try { const context = resolveSessionPointerContext(cwd); @@ -1357,32 +1362,37 @@ describe('session pointer transaction', () => { await writeLockOwner(cwd, validLockOwner()); const claimPath = join(context.lockPath, `owner.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.recovery`); const quarantinePath = `${context.lockPath}.quarantine.${TEST_TOKEN}.${SUCCESSOR_TOKEN}`; - let armed = false; + const foreignMarker = 'foreign parked lock directory'; + let parkedPath: string | undefined; + let parkedIdentity: { dev: number; ino: number } | undefined; await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { - unlink: async (path) => { - await rm(path, { force: false }); - if (path === claimPath) armed = true; - }, - lstat: async (path) => { - if (armed && path === context.lockPath) { - const replacementPath = `${context.lockPath}.replacement`; - await mkdir(replacementPath); - await rename(replacementPath, context.lockPath); + rename: async (from, to) => { + await rename(from, to); + if (from === context.lockPath) { + parkedPath = to; + await rename(to, `${to}.original`); + await mkdir(to); + await writeFile(join(to, 'marker'), foreignMarker, 'utf-8'); + const stat = await lstat(to); + parkedIdentity = { dev: stat.dev, ino: stat.ino }; } - return await lstat(path); }, }, }, async () => { const recovered = await recoverSessionPointerLock(cwd); assert.equal(recovered.recovered, false); assert.equal(recovered.action, 'none'); - assert.match(recovered.reason, /replaced before removal/i); + assert.match(recovered.reason, /canonical lock directory was replaced before removal/i); assert.equal(recovered.quarantinePath, quarantinePath); }); - assert.deepEqual(await readdir(context.lockPath), []); + assert.ok(parkedPath); + assert.ok(parkedIdentity); + const parkedStat = await lstat(parkedPath); + assert.deepEqual({ dev: parkedStat.dev, ino: parkedStat.ino }, parkedIdentity); + assert.equal(await readFile(join(parkedPath, 'marker'), 'utf-8'), foreignMarker); assert.equal(await readFile(quarantinePath, 'utf-8'), staleOwner); } finally { await rm(cwd, { recursive: true, force: true }); @@ -1465,7 +1475,7 @@ describe('session pointer transaction', () => { await mkdir(context.lockPath, { recursive: true }); await writeFile(join(context.lockPath, `owner.${TEST_TOKEN}.tmp`), JSON.stringify(validLockOwner()), 'utf-8'); const links: Array<[string, string]> = []; - const unlinks: string[] = []; + const renames: Array<[string, string]> = []; let successorBlocked = false; await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, @@ -1479,9 +1489,9 @@ describe('session pointer transaction', () => { successorBlocked = true; } }, - unlink: async (path) => { - unlinks.push(path); - await rm(path); + rename: async (from, to) => { + renames.push([from, to]); + await rename(from, to); }, }, }, async () => { @@ -1493,8 +1503,9 @@ describe('session pointer transaction', () => { assert.match(links[0]![1], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); assert.match(links[1]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); assert.equal(links[1]![1].includes('.quarantine.'), true); - assert.match(unlinks[0]!, /owner\.transaction_token_123456\.tmp$/); - assert.match(unlinks[1]!, new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.match(renames[0]![0], /owner\.transaction_token_123456\.tmp$/); + assert.match(renames[1]![0], new RegExp(`owner\\.${TEST_TOKEN}\\.${SUCCESSOR_TOKEN}\\.recovery$`)); + assert.equal(renames[2]![0], context.lockPath); } finally { await rm(cwd, { recursive: true, force: true }); } @@ -1538,13 +1549,12 @@ describe('session pointer transaction', () => { } }); - it('does not quarantine a successor live lock displaced into the canonical path after claim revalidation', async () => { + it('does not report recovery after a successor live lock appears during parked-lock removal', async () => { const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-post-claim-race-')); try { const context = resolveSessionPointerContext(cwd); const staleTemp = join(context.lockPath, `owner.${TEST_TOKEN}.tmp`); const successorTemp = join(context.lockPath, `owner.${SUCCESSOR_TOKEN}.tmp`); - const displacedPath = `${context.lockPath}.claimed-displaced`; await mkdir(context.lockPath, { recursive: true }); await writeFile(staleTemp, JSON.stringify(validLockOwner()), 'utf-8'); let displaced = false; @@ -1552,21 +1562,20 @@ describe('session pointer transaction', () => { token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { - rmdir: async (path) => { - if (!displaced && path === context.lockPath) { + rename: async (from, to) => { + await rename(from, to); + if (!displaced && from === context.lockPath) { displaced = true; - await rename(context.lockPath, displacedPath); await mkdir(context.lockPath); await writeFile(successorTemp, JSON.stringify(validLockOwner({ token: SUCCESSOR_TOKEN })), 'utf-8'); } - await rmdir(path); }, }, }, async () => { const recovered = await recoverSessionPointerLock(cwd); assert.equal(recovered.recovered, false); assert.equal(recovered.action, 'none'); - assert.match(recovered.reason, /canonical lock directory was not empty/i); + assert.match(recovered.reason, /canonical lock directory was replaced before removal/i); assert.ok(recovered.quarantinePath); }); assert.equal(displaced, true); diff --git a/src/hooks/session.ts b/src/hooks/session.ts index 2350562cd..7df15f0ff 100644 --- a/src/hooks/session.ts +++ b/src/hooks/session.ts @@ -1010,7 +1010,59 @@ async function revalidateRecoveryEvidence(lockPath: string, evidenceName: string } } -async function rollbackRecoveryClaim(lockPath: string, evidenceName: string, claimName: string, claimIdentity: { dev: number; ino: number }): Promise<{ ok: boolean; reason?: string }> { +/** + * Atomically park a pathname at an unpredictable sibling of the lock directory + * and delete the parked object only when it still holds the captured dev/ino + * identity. The park rename cannot overwrite existing bytes (unpredictable, + * preflighted destination), so a replaced or foreign target is preserved as + * exact residue instead of being unlinked after a stale observation. + */ +async function removeIfIdentityMatches( + path: string, + identity: { dev: number; ino: number }, + kind: 'file' | 'directory', + parkPrefix: string, +): Promise<{ removed: boolean; reason?: string; code?: string; residuePath?: string }> { + const parkPath = `${parkPrefix}.parked-${randomUUID()}`; + try { + await transactionDependencies.fs.lstat(parkPath); + return { removed: false, reason: 'a park destination unexpectedly exists' }; + } catch (error) { + if (!isNotFound(error)) return { removed: false, reason: 'unable to inspect a park destination', code: errorCode(error) }; + } + try { + await transactionDependencies.fs.rename(path, parkPath); + } catch (error) { + if (isNotFound(error)) return { removed: true }; + return { removed: false, reason: `unable to park the exact pathname (${errorCode(error) ?? 'unknown'})`, code: errorCode(error) }; + } + let parkedStat: Awaited>; + try { + parkedStat = await transactionDependencies.fs.lstat(parkPath); + } catch (error) { + if (isNotFound(error)) return { removed: true }; + return { removed: false, reason: 'unable to verify the parked pathname', code: errorCode(error), residuePath: parkPath }; + } + const identityMatches = parkedStat.dev === identity.dev && parkedStat.ino === identity.ino + && (kind === 'directory' ? !parkedStat.isSymbolicLink() && parkedStat.isDirectory() : true); + if (!identityMatches) return { removed: false, reason: 'the parked pathname no longer holds the validated identity; it was preserved as residue', residuePath: parkPath }; + try { + if (kind === 'directory') await transactionDependencies.fs.rmdir(parkPath); + else await transactionDependencies.fs.unlink(parkPath); + } catch (error) { + if (isNotFound(error)) return { removed: true }; + return { removed: false, reason: `unable to remove the verified parked pathname (${errorCode(error) ?? 'unknown'})`, code: errorCode(error), residuePath: parkPath }; + } + try { + await transactionDependencies.fs.lstat(path); + return { removed: false, reason: 'the original pathname reappeared after its validated object was parked and removed', residuePath: path }; + } catch (error) { + if (isNotFound(error)) return { removed: true }; + return { removed: false, reason: 'unable to verify the original pathname after removing its parked object', code: errorCode(error), residuePath: path }; + } +} + +async function rollbackRecoveryClaim(lockPath: string, evidenceName: string, claimName: string, claimIdentity: { dev: number; ino: number }): Promise<{ ok: boolean; reason?: string; residuePath?: string }> { const evidencePath = join(lockPath, evidenceName); const claimPath = join(lockPath, claimName); try { await transactionDependencies.fs.lstat(evidencePath); return { ok: false, reason: 'the original evidence name reappeared' }; } catch (error) { if (!isNotFound(error)) return { ok: false, reason: 'unable to inspect the original evidence name' }; } @@ -1018,15 +1070,27 @@ async function rollbackRecoveryClaim(lockPath: string, evidenceName: string, cla if (isAlreadyExists(error)) return { ok: false, reason: 'the original evidence name reappeared during rollback' }; return { ok: false, reason: `the exact claim cannot be restored on this filesystem (${errorCode(error) ?? 'unknown'}); it was left in place under its recovery name` }; } + let restoredStat: Awaited>; try { - const [claimStat, evidenceStat] = await Promise.all([transactionDependencies.fs.lstat(claimPath), transactionDependencies.fs.lstat(evidencePath)]); - if (claimStat.dev !== claimIdentity.dev || claimStat.ino !== claimIdentity.ino || evidenceStat.dev !== claimIdentity.dev || evidenceStat.ino !== claimIdentity.ino) return { ok: false, reason: 'claim identity changed during rollback; the exact claim was left in place under its recovery name' }; - } catch { return { ok: false, reason: 'claim identity changed during rollback; the exact claim was left in place under its recovery name' }; } - try { await transactionDependencies.fs.unlink(claimPath); return { ok: true }; } catch (error) { return isNotFound(error) ? { ok: true } : { ok: false, reason: 'unable to remove the rolled-back claim name' }; } + restoredStat = await transactionDependencies.fs.lstat(evidencePath); + } catch { + return { ok: false, reason: 'unable to verify the restored evidence name' }; + } + if (restoredStat.dev !== claimIdentity.dev || restoredStat.ino !== claimIdentity.ino) { + const foreign = await removeIfIdentityMatches(evidencePath, claimIdentity, 'file', lockPath); + return { ok: false, reason: 'claim identity changed before rollback; the exact claim was left in place under its recovery name', residuePath: foreign.residuePath }; + } + const removal = await removeIfIdentityMatches(claimPath, claimIdentity, 'file', lockPath); + if (removal.removed) return { ok: true, residuePath: removal.residuePath }; + if (removal.residuePath) return { ok: true, residuePath: removal.residuePath }; + return { ok: false, reason: removal.reason }; } -function recoveryRollbackReason(prefix: string, rollback: { ok: boolean; reason?: string }): string { - return rollback.ok ? `${prefix}; the exact claim was rolled back to its original evidence name without modifying any other bytes.` : `${prefix}; rollback was not possible: ${rollback.reason}. The exact claim was left in place under its recovery name.`; +function recoveryRollbackReason(prefix: string, rollback: { ok: boolean; reason?: string; residuePath?: string }): string { + const residue = rollback.residuePath ? ` Residue preserved at ${rollback.residuePath}; inspect and remove it manually.` : ''; + return rollback.ok + ? `${prefix}; the exact claim was rolled back to its original evidence name without modifying any other bytes.${residue}` + : `${prefix}; rollback was not possible: ${rollback.reason}.${residue || ' The exact claim was left in place under its recovery name.'}`; } /** Explicitly quarantine only a dead, atomically claimed pointer lock; never force recovery. */ @@ -1053,15 +1117,23 @@ export async function recoverSessionPointerLock(cwd: string): Promise>; + const cleanupClaim = async (): Promise => { + const cleanup = await removeIfIdentityMatches(claimPath, preClaim.evidenceStat, 'file', context.lockPath); + return cleanup.removed + ? 'The recovery claim was removed.' + : `The recovery claim could not be removed (${cleanup.reason})${cleanup.residuePath ? `; residue preserved at ${cleanup.residuePath}` : '; it was left in place under its recovery name'}.`; + }; try { const [currentEvidence, currentClaim] = await Promise.all([transactionDependencies.fs.lstat(evidencePath), transactionDependencies.fs.lstat(claimPath)]); postLinkClaim = currentClaim; if (currentEvidence.dev !== preClaim.evidenceStat.dev || currentEvidence.ino !== preClaim.evidenceStat.ino || currentClaim.dev !== preClaim.evidenceStat.dev || currentClaim.ino !== preClaim.evidenceStat.ino) { - try { const currentClaimAgain = await transactionDependencies.fs.lstat(claimPath); if (currentClaimAgain.dev === currentClaim.dev && currentClaimAgain.ino === currentClaim.ino) await transactionDependencies.fs.unlink(claimPath); } catch { /* Leave uncertain residue fail-closed. */ } - return { ...inspection, action: 'none', recovered: false, reason: 'Session pointer lock evidence changed before recovery claim.' }; + return { ...inspection, action: 'none', recovered: false, reason: `Session pointer lock evidence changed before recovery claim. ${await cleanupClaim()}` }; } - } catch { return { ...inspection, action: 'none', recovered: false, reason: 'Session pointer lock evidence changed before recovery claim.' }; } - try { await transactionDependencies.fs.unlink(evidencePath); } catch (error) { if (!isNotFound(error)) return { ...inspection, action: 'none', recovered: false, reason: `The exact recovery claim was created, but its original evidence name could not be removed (${errorCode(error) ?? 'unknown'}).` }; } + } catch { + return { ...inspection, action: 'none', recovered: false, reason: `Session pointer lock evidence changed before recovery claim. ${await cleanupClaim()}` }; + } + const evidenceRemoval = await removeIfIdentityMatches(evidencePath, preClaim.evidenceStat, 'file', context.lockPath); + if (!evidenceRemoval.removed) return { ...inspection, action: 'none', recovered: false, reason: `The exact recovery claim was created, but its original evidence name could not be removed (${evidenceRemoval.reason}).${evidenceRemoval.residuePath ? ` Residue preserved at ${evidenceRemoval.residuePath}; inspect and remove it manually.` : ''}` }; const claimIdentity = { dev: postLinkClaim.dev, ino: postLinkClaim.ino }; const claimed = await inspectSessionPointerLockAtContext(context); if (claimed.status !== 'unexpected') { @@ -1086,13 +1158,13 @@ export async function recoverSessionPointerLock(cwd: string): Promise Date: Thu, 23 Jul 2026 03:51:37 +0000 Subject: [PATCH 5/7] fix(hooks): resume parked lock recovery checkpoints --- src/hooks/__tests__/session.test.ts | 158 ++++++++++++++++++++++++++++ src/hooks/session.ts | 86 +++++++++++++-- 2 files changed, 236 insertions(+), 8 deletions(-) diff --git a/src/hooks/__tests__/session.test.ts b/src/hooks/__tests__/session.test.ts index ae71f7080..54d2b6c1e 100644 --- a/src/hooks/__tests__/session.test.ts +++ b/src/hooks/__tests__/session.test.ts @@ -1148,6 +1148,164 @@ describe('session pointer transaction', () => { } finally { await rm(cwd, { recursive: true, force: true }); } }); + it('retries a transient token-bound park collision without allocating a second parked name', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-park-retry-')); + try { + await writeLockOwner(cwd, validLockOwner()); + const context = resolveSessionPointerContext(cwd); + const ownerPath = join(context.lockPath, 'owner.json'); + const parks: string[] = []; + let failOnce = true; + await withPointerDependencies({ + token: () => SUCCESSOR_TOKEN, + probePid: () => 'dead', + fs: { + rename: async (from, to) => { + if (from === ownerPath && failOnce) { + failOnce = false; + throw codedError('EBUSY'); + } + if (from === ownerPath) parks.push(to); + await rename(from, to); + }, + }, + }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, true)); + assert.deepEqual(parks, [`${context.lockPath}.parked-${SUCCESSOR_TOKEN}`]); + } finally { + await rm(cwd, { recursive: true, force: true }); + } + }); + + it('records a token-bound checkpoint when evidence parking is transiently busy', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-checkpoint-')); + try { + const context = resolveSessionPointerContext(cwd); + const ownerPath = join(context.lockPath, 'owner.json'); + await writeLockOwner(cwd, validLockOwner()); + await withPointerDependencies({ + token: () => SUCCESSOR_TOKEN, + probePid: () => 'dead', + fs: { rename: async (from, to) => { + if (from === ownerPath) throw codedError('EBUSY'); + await rename(from, to); + } }, + }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, false)); + const checkpointPath = `${context.lockPath}.recovery.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.json`; + const checkpoint = JSON.parse(await readFile(checkpointPath, 'utf-8')) as { sourcePath: string; parkPath: string; identity: { dev: number; ino: number }; phase: string }; + assert.equal(checkpoint.sourcePath, ownerPath); + assert.equal(checkpoint.parkPath, `${context.lockPath}.parked-${SUCCESSOR_TOKEN}`); + assert.equal(checkpoint.phase, 'evidence-pending'); + assert.equal(typeof checkpoint.identity.dev, 'number'); + assert.equal(typeof checkpoint.identity.ino, 'number'); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + + it('resumes an evidence-pending checkpoint without minting another recovery token', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-resume-')); + try { + const context = resolveSessionPointerContext(cwd); + const ownerPath = join(context.lockPath, 'owner.json'); + const claimPath = join(context.lockPath, `owner.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.recovery`); + const parkPath = `${context.lockPath}.parked-${SUCCESSOR_TOKEN}`; + const owner = JSON.stringify(validLockOwner()); + await writeLockOwner(cwd, validLockOwner()); + await link(ownerPath, claimPath); + await rename(ownerPath, parkPath); + const parked = await lstat(parkPath); + await writeFile(`${context.lockPath}.recovery.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.json`, JSON.stringify({ version: 1, sourcePath: ownerPath, parkPath, identity: { dev: parked.dev, ino: parked.ino }, phase: 'evidence-pending' })); + await withPointerDependencies({ token: () => { throw new Error('resume must reuse checkpoint token'); }, probePid: () => 'dead' }, async () => { + const resumed = await recoverSessionPointerLock(cwd); + assert.equal(resumed.recovered, true); + }); + assert.equal(await readFile(`${context.lockPath}.quarantine.${TEST_TOKEN}.${SUCCESSOR_TOKEN}`, 'utf-8'), owner); + assert.equal(existsSync(`${context.lockPath}.recovery.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.json`), false); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + + it('fails closed for malformed checkpoint JSON without minting a recovery token', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-malformed-checkpoint-')); + try { + const context = resolveSessionPointerContext(cwd); + await writeLockOwner(cwd, validLockOwner()); + const checkpointPath = `${context.lockPath}.recovery.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.json`; + await writeFile(checkpointPath, '{not json', 'utf-8'); + await withPointerDependencies({ token: () => { throw new Error('must not mint token'); }, probePid: () => 'dead' }, async () => { + const recovered = await recoverSessionPointerLock(cwd); + assert.equal(recovered.recovered, false); + assert.equal(recovered.safeToRecover, false); + }); + assert.equal(await readFile(checkpointPath, 'utf-8'), '{not json'); + assert.equal(await readFile(join(context.lockPath, 'owner.json'), 'utf-8'), JSON.stringify(validLockOwner())); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + + for (const [label, checkpoint] of [ + ['wrong version', { version: 2, phase: 'evidence-pending', sourcePath: 'x', parkPath: 'x', identity: { dev: 0, ino: 0 } }], + ['wrong phase', { version: 1, phase: 'claim-pending', sourcePath: 'x', parkPath: 'x', identity: { dev: 0, ino: 0 } }], + ['out-of-root path', { version: 1, phase: 'evidence-pending', sourcePath: '/foreign/owner.json', parkPath: '/foreign/parked', identity: { dev: 0, ino: 0 } }], + ]) it(`fails closed for ${label} checkpoint`, async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-invalid-checkpoint-')); + try { + const context = resolveSessionPointerContext(cwd); + await writeLockOwner(cwd, validLockOwner()); + const path = `${context.lockPath}.recovery.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.json`; + await writeFile(path, JSON.stringify(checkpoint), 'utf-8'); + await withPointerDependencies({ token: () => { throw new Error('must not mint token'); }, probePid: () => 'dead' }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, false)); + assert.equal(await readFile(path, 'utf-8'), JSON.stringify(checkpoint)); + assert.equal(await readFile(join(context.lockPath, 'owner.json'), 'utf-8'), JSON.stringify(validLockOwner())); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + + it('refuses multiple checkpoints without mutating either', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-multiple-checkpoints-')); + try { + const context = resolveSessionPointerContext(cwd); + await writeLockOwner(cwd, validLockOwner()); + for (const token of [SUCCESSOR_TOKEN, FOREIGN_TOKEN]) await writeFile(`${context.lockPath}.recovery.${TEST_TOKEN}.${token}.json`, '{}', 'utf-8'); + await withPointerDependencies({ token: () => { throw new Error('must not mint token'); }, probePid: () => 'dead' }, async () => assert.match((await recoverSessionPointerLock(cwd)).reason, /Multiple recovery checkpoints/)); + assert.equal(existsSync(join(context.lockPath, 'owner.json')), true); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + + it('preserves a checkpoint when its claim is missing', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-missing-claim-')); + try { + const context = resolveSessionPointerContext(cwd); await mkdir(context.lockPath, { recursive: true }); + const parkPath = `${context.lockPath}.parked-${SUCCESSOR_TOKEN}`; await writeFile(parkPath, 'stale', 'utf-8'); const stat = await lstat(parkPath); + const checkpointPath = `${context.lockPath}.recovery.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.json`; + await writeFile(checkpointPath, JSON.stringify({ version: 1, sourcePath: join(context.lockPath, 'owner.json'), parkPath, identity: { dev: stat.dev, ino: stat.ino }, phase: 'evidence-pending' })); + await withPointerDependencies({ token: () => { throw new Error('must not mint token'); }, probePid: () => 'dead' }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, false)); + assert.equal(await readFile(checkpointPath, 'utf-8').then(Boolean), true); assert.equal(await readFile(parkPath, 'utf-8'), 'stale'); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + + it('preserves a resumable checkpoint when checkpoint quarantine rename is busy', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-checkpoint-rename-busy-')); + try { + const context = resolveSessionPointerContext(cwd); const ownerPath = join(context.lockPath, 'owner.json'); const claimPath = join(context.lockPath, `owner.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.recovery`); const parkPath = `${context.lockPath}.parked-${SUCCESSOR_TOKEN}`; + await writeLockOwner(cwd, validLockOwner()); await link(ownerPath, claimPath); await rename(ownerPath, parkPath); const stat = await lstat(parkPath); + const checkpointPath = `${context.lockPath}.recovery.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.json`; await writeFile(checkpointPath, JSON.stringify({ version: 1, sourcePath: ownerPath, parkPath, identity: { dev: stat.dev, ino: stat.ino }, phase: 'evidence-pending' })); + await withPointerDependencies({ token: () => { throw new Error('must not mint token'); }, probePid: () => 'dead', fs: { rename: async (from, to) => { if (from === parkPath) throw codedError('EBUSY'); await rename(from, to); } } }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, false)); + assert.equal(await readFile(checkpointPath, 'utf-8').then(Boolean), true); assert.equal(await readFile(parkPath, 'utf-8'), JSON.stringify(validLockOwner())); assert.equal(await readFile(claimPath, 'utf-8'), JSON.stringify(validLockOwner())); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + + it('rolls a checkpoint quarantine move back when claim unlink is transiently busy', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-checkpoint-unlink-busy-')); + try { + const context = resolveSessionPointerContext(cwd); const ownerPath = join(context.lockPath, 'owner.json'); const claimPath = join(context.lockPath, `owner.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.recovery`); const parkPath = `${context.lockPath}.parked-${SUCCESSOR_TOKEN}`; const checkpointPath = `${context.lockPath}.recovery.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.json`; + await writeLockOwner(cwd, validLockOwner()); await link(ownerPath, claimPath); await rename(ownerPath, parkPath); const stat = await lstat(parkPath); + await writeFile(checkpointPath, JSON.stringify({ version: 1, sourcePath: ownerPath, parkPath, identity: { dev: stat.dev, ino: stat.ino }, phase: 'evidence-pending' })); + let fail = true; + await withPointerDependencies({ token: () => { throw new Error('must not mint token'); }, probePid: () => 'dead', fs: { unlink: async (path) => { if (path === claimPath && fail) { fail = false; throw codedError('EBUSY'); } await rm(path); } } }, async () => { + assert.equal((await recoverSessionPointerLock(cwd)).recovered, false); + assert.equal(await readFile(parkPath, 'utf-8'), JSON.stringify(validLockOwner())); + assert.equal((await recoverSessionPointerLock(cwd)).recovered, true); + }); + assert.equal(existsSync(checkpointPath), false); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + it('refuses a quarantine path that appears after its preflight check without overwriting it', async () => { const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-quarantine-race-')); try { diff --git a/src/hooks/session.ts b/src/hooks/session.ts index 7df15f0ff..ec5b480de 100644 --- a/src/hooks/session.ts +++ b/src/hooks/session.ts @@ -23,7 +23,7 @@ import { import { readFileSync } from 'fs'; import type { FileHandle } from 'fs/promises'; import { createHash, randomUUID } from 'crypto'; -import { dirname, join } from 'path'; +import { basename, dirname, join } from 'path'; import { omxRoot, omxLogsDir, sameFilePath } from '../utils/paths.js'; import { getBaseStateDirWithSource, @@ -1023,18 +1023,28 @@ async function removeIfIdentityMatches( kind: 'file' | 'directory', parkPrefix: string, ): Promise<{ removed: boolean; reason?: string; code?: string; residuePath?: string }> { - const parkPath = `${parkPrefix}.parked-${randomUUID()}`; + const parkPath = `${parkPrefix}.parked-${transactionDependencies.token()}`; try { await transactionDependencies.fs.lstat(parkPath); return { removed: false, reason: 'a park destination unexpectedly exists' }; } catch (error) { if (!isNotFound(error)) return { removed: false, reason: 'unable to inspect a park destination', code: errorCode(error) }; } + const park = async (): Promise => await transactionDependencies.fs.rename(path, parkPath); try { - await transactionDependencies.fs.rename(path, parkPath); + await park(); } catch (error) { - if (isNotFound(error)) return { removed: true }; - return { removed: false, reason: `unable to park the exact pathname (${errorCode(error) ?? 'unknown'})`, code: errorCode(error) }; + if (errorCode(error) === 'EBUSY') { + try { + await park(); + } catch (retryError) { + if (isNotFound(retryError)) return { removed: true }; + return { removed: false, reason: `unable to park the exact pathname (${errorCode(retryError) ?? 'unknown'})`, code: errorCode(retryError) }; + } + } else { + if (isNotFound(error)) return { removed: true }; + return { removed: false, reason: `unable to park the exact pathname (${errorCode(error) ?? 'unknown'})`, code: errorCode(error) }; + } } let parkedStat: Awaited>; try { @@ -1046,12 +1056,25 @@ async function removeIfIdentityMatches( const identityMatches = parkedStat.dev === identity.dev && parkedStat.ino === identity.ino && (kind === 'directory' ? !parkedStat.isSymbolicLink() && parkedStat.isDirectory() : true); if (!identityMatches) return { removed: false, reason: 'the parked pathname no longer holds the validated identity; it was preserved as residue', residuePath: parkPath }; - try { + const removeParked = async (): Promise => { if (kind === 'directory') await transactionDependencies.fs.rmdir(parkPath); else await transactionDependencies.fs.unlink(parkPath); + }; + try { + await removeParked(); } catch (error) { - if (isNotFound(error)) return { removed: true }; - return { removed: false, reason: `unable to remove the verified parked pathname (${errorCode(error) ?? 'unknown'})`, code: errorCode(error), residuePath: parkPath }; + if (errorCode(error) !== 'EBUSY') { + if (isNotFound(error)) return { removed: true }; + return { removed: false, reason: `unable to remove the verified parked pathname (${errorCode(error) ?? 'unknown'})`, code: errorCode(error), residuePath: parkPath }; + } + try { + const retryStat = await transactionDependencies.fs.lstat(parkPath); + if (retryStat.dev !== identity.dev || retryStat.ino !== identity.ino) return { removed: false, reason: 'the parked pathname no longer holds the validated identity; it was preserved as residue', residuePath: parkPath }; + await removeParked(); + } catch (retryError) { + if (isNotFound(retryError)) return { removed: true }; + return { removed: false, reason: `unable to remove the verified parked pathname (${errorCode(retryError) ?? 'unknown'})`, code: errorCode(retryError), residuePath: parkPath }; + } } try { await transactionDependencies.fs.lstat(path); @@ -1096,6 +1119,40 @@ function recoveryRollbackReason(prefix: string, rollback: { ok: boolean; reason? /** Explicitly quarantine only a dead, atomically claimed pointer lock; never force recovery. */ export async function recoverSessionPointerLock(cwd: string): Promise { const context = resolveSessionPointerContext(cwd); + const checkpointPrefix = `${basename(context.lockPath)}.recovery.`; + let checkpointNames: string[]; + try { + checkpointNames = (await transactionDependencies.fs.readdir(dirname(context.lockPath))).filter((name) => name.startsWith(checkpointPrefix) && name.endsWith('.json')); + } catch { + checkpointNames = []; + } + if (checkpointNames.length > 1) return { status: 'unexpected', lockPath: context.lockPath, evidenceSource: 'none', safeToRecover: false, action: 'none', recovered: false, reason: 'Multiple recovery checkpoints exist.' }; + if (checkpointNames.length === 1) { + const checkpointPath = join(dirname(context.lockPath), checkpointNames[0]!); + const match = new RegExp(`^${basename(context.lockPath).replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\.recovery\\.([A-Za-z0-9_-]{16,128})\\.([A-Za-z0-9_-]{16,128})\\.json$`).exec(checkpointNames[0]!); + try { + const checkpoint = JSON.parse(await transactionDependencies.fs.readFile(checkpointPath, 'utf8')) as { version: number; sourcePath: string; parkPath: string; identity: { dev: number; ino: number }; phase: string }; + if (!match || checkpoint.version !== 1 || checkpoint.phase !== 'evidence-pending' || checkpoint.sourcePath !== join(context.lockPath, 'owner.json') || checkpoint.parkPath !== `${context.lockPath}.parked-${match[2]}`) throw new Error('invalid checkpoint'); + const parked = await transactionDependencies.fs.lstat(checkpoint.parkPath); + if (parked.isSymbolicLink() || !parked.isFile() || parked.dev !== checkpoint.identity.dev || parked.ino !== checkpoint.identity.ino) throw new Error('checkpoint identity mismatch'); + const claimPath = join(context.lockPath, `owner.${match[1]}.${match[2]}.recovery`); + const claim = await transactionDependencies.fs.lstat(claimPath); + if (claim.isSymbolicLink() || !claim.isFile() || claim.dev !== parked.dev || claim.ino !== parked.ino) throw new Error('checkpoint claim mismatch'); + const quarantinePath = `${context.lockPath}.quarantine.${match[1]}.${match[2]}`; + await transactionDependencies.fs.rename(checkpoint.parkPath, quarantinePath); + try { + await transactionDependencies.fs.unlink(join(context.lockPath, `owner.${match[1]}.${match[2]}.recovery`)); + } catch (error) { + await transactionDependencies.fs.rename(quarantinePath, checkpoint.parkPath); + throw error; + } + await transactionDependencies.fs.unlink(checkpointPath); + await transactionDependencies.fs.rmdir(context.lockPath); + return { status: 'dead', lockPath: context.lockPath, evidenceSource: 'owner.json', safeToRecover: true, action: 'quarantined', recovered: true, reason: 'Dead session pointer lock recovery checkpoint resumed.', quarantinePath }; + } catch { + return { status: 'unexpected', lockPath: context.lockPath, evidenceSource: 'none', safeToRecover: false, action: 'none', recovered: false, reason: 'Recovery checkpoint is malformed or no longer identifies its recorded parked evidence.' }; + } + } const inspection = await inspectSessionPointerLockAtContext(context); if (!inspection.safeToRecover) return { ...inspection, action: 'none', recovered: false, reason: inspection.status === 'absent' ? 'No session pointer lock exists.' : `Session pointer lock is not safe to recover (${inspection.status}).` }; let entries: string[]; @@ -1132,8 +1189,21 @@ export async function recoverSessionPointerLock(cwd: string): Promise Date: Thu, 23 Jul 2026 04:18:32 +0000 Subject: [PATCH 6/7] fix(hooks): roll back failed recovery claims --- src/hooks/__tests__/session.test.ts | 40 +++++++++++++++++------------ src/hooks/session.ts | 9 +++++-- 2 files changed, 31 insertions(+), 18 deletions(-) diff --git a/src/hooks/__tests__/session.test.ts b/src/hooks/__tests__/session.test.ts index 54d2b6c1e..8e493f7f8 100644 --- a/src/hooks/__tests__/session.test.ts +++ b/src/hooks/__tests__/session.test.ts @@ -1176,27 +1176,35 @@ describe('session pointer transaction', () => { } }); - it('records a token-bound checkpoint when evidence parking is transiently busy', async () => { + it('rolls back a claim when checkpoint creation fails', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-checkpoint-write-fail-')); + try { + const context = resolveSessionPointerContext(cwd); + await writeLockOwner(cwd, validLockOwner()); + let fail = true; + await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { writeFile: async (path, data, options) => { if (fail && path.includes('.recovery.')) { fail = false; throw codedError('EBUSY'); } await writeFile(path, data, options); } } }, async () => { + assert.equal((await recoverSessionPointerLock(cwd)).recovered, false); + assert.deepEqual(await readdir(context.lockPath), ['owner.json']); + assert.equal((await recoverSessionPointerLock(cwd)).recovered, true); + }); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + + it('rolls back a double-EBUSY evidence park failure so a normal retry succeeds', async () => { const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-checkpoint-')); try { const context = resolveSessionPointerContext(cwd); const ownerPath = join(context.lockPath, 'owner.json'); await writeLockOwner(cwd, validLockOwner()); - await withPointerDependencies({ - token: () => SUCCESSOR_TOKEN, - probePid: () => 'dead', - fs: { rename: async (from, to) => { - if (from === ownerPath) throw codedError('EBUSY'); - await rename(from, to); - } }, - }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, false)); - const checkpointPath = `${context.lockPath}.recovery.${TEST_TOKEN}.${SUCCESSOR_TOKEN}.json`; - const checkpoint = JSON.parse(await readFile(checkpointPath, 'utf-8')) as { sourcePath: string; parkPath: string; identity: { dev: number; ino: number }; phase: string }; - assert.equal(checkpoint.sourcePath, ownerPath); - assert.equal(checkpoint.parkPath, `${context.lockPath}.parked-${SUCCESSOR_TOKEN}`); - assert.equal(checkpoint.phase, 'evidence-pending'); - assert.equal(typeof checkpoint.identity.dev, 'number'); - assert.equal(typeof checkpoint.identity.ino, 'number'); + let failures = 2; + await withPointerDependencies({ token: () => SUCCESSOR_TOKEN, probePid: () => 'dead', fs: { rename: async (from, to) => { + if (from === ownerPath && failures-- > 0) throw codedError('EBUSY'); + await rename(from, to); + } } }, async () => { + assert.equal((await recoverSessionPointerLock(cwd)).recovered, false); + assert.deepEqual(await readdir(context.lockPath), ['owner.json']); + assert.equal((await recoverSessionPointerLock(cwd)).recovered, true); + }); } finally { await rm(cwd, { recursive: true, force: true }); } }); diff --git a/src/hooks/session.ts b/src/hooks/session.ts index ec5b480de..976795f67 100644 --- a/src/hooks/session.ts +++ b/src/hooks/session.ts @@ -1199,10 +1199,15 @@ export async function recoverSessionPointerLock(cwd: string): Promise Date: Thu, 23 Jul 2026 05:29:42 +0000 Subject: [PATCH 7/7] fix(hooks): record exact recovery park paths --- src/hooks/__tests__/session.test.ts | 19 +++++++++++++++++++ src/hooks/session.ts | 12 +++++++----- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/hooks/__tests__/session.test.ts b/src/hooks/__tests__/session.test.ts index 8e493f7f8..69f892d3f 100644 --- a/src/hooks/__tests__/session.test.ts +++ b/src/hooks/__tests__/session.test.ts @@ -1230,6 +1230,25 @@ describe('session pointer transaction', () => { } finally { await rm(cwd, { recursive: true, force: true }); } }); + it('resumes the actual distinct-token evidence park path after an interruption', async () => { + const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-distinct-park-token-')); + try { + const context = resolveSessionPointerContext(cwd); + const ownerPath = join(context.lockPath, 'owner.json'); + const claimToken = 'claim_token_123456789'; + const parkToken = 'park_token_123456789'; + const claimPath = join(context.lockPath, `owner.${TEST_TOKEN}.${claimToken}.recovery`); + const parkPath = `${context.lockPath}.parked-${parkToken}`; + await writeLockOwner(cwd, validLockOwner()); + await link(ownerPath, claimPath); + await rename(ownerPath, parkPath); + const parked = await lstat(parkPath); + await writeFile(`${context.lockPath}.recovery.${TEST_TOKEN}.${claimToken}.json`, JSON.stringify({ version: 1, sourcePath: ownerPath, parkPath, identity: { dev: parked.dev, ino: parked.ino }, phase: 'evidence-pending' })); + await withPointerDependencies({ token: () => { throw new Error('resume must not mint a replacement token'); }, probePid: () => 'dead' }, async () => assert.equal((await recoverSessionPointerLock(cwd)).recovered, true)); + assert.equal(await readFile(`${context.lockPath}.quarantine.${TEST_TOKEN}.${claimToken}`, 'utf-8'), JSON.stringify(validLockOwner())); + } finally { await rm(cwd, { recursive: true, force: true }); } + }); + it('fails closed for malformed checkpoint JSON without minting a recovery token', async () => { const cwd = await mkdtemp(join(tmpdir(), 'omx-session-lock-recovery-malformed-checkpoint-')); try { diff --git a/src/hooks/session.ts b/src/hooks/session.ts index 976795f67..40c6d1a89 100644 --- a/src/hooks/session.ts +++ b/src/hooks/session.ts @@ -1022,8 +1022,9 @@ async function removeIfIdentityMatches( identity: { dev: number; ino: number }, kind: 'file' | 'directory', parkPrefix: string, + parkPathOverride?: string, ): Promise<{ removed: boolean; reason?: string; code?: string; residuePath?: string }> { - const parkPath = `${parkPrefix}.parked-${transactionDependencies.token()}`; + const parkPath = parkPathOverride ?? `${parkPrefix}.parked-${transactionDependencies.token()}`; try { await transactionDependencies.fs.lstat(parkPath); return { removed: false, reason: 'a park destination unexpectedly exists' }; @@ -1132,7 +1133,7 @@ export async function recoverSessionPointerLock(cwd: string): Promise