fix(hooks): recover positively dead canonical session-pointer locks#3262
Conversation
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
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cc3b292e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // status, and owner token before quarantining. Live, reused, | ||
| // identity-indeterminate, ambiguous, symlink, malformed, unexpected, | ||
| // and io-error states stay fail-closed. | ||
| safeToRecover: status === 'dead', |
There was a problem hiding this comment.
When two recoveries overlap and one removes the stale canonical lock before a new launcher recreates session.json.lock, this now treats the new owner.json as recoverable by path alone. recoverSessionPointerLock() then renames that successor owner.json to an .recovery file, fails revalidation, and returns without restoring it; subsequent launches hit acquirePointerLock() with no live canonical owner and keep aborting until manual cleanup. Canonical-owner recovery needs to claim/validate the lock directory identity (or remain limited to owner-temp evidence) before enabling owner.json as safe to recover.
Useful? React with 👍 / 👎.
Independent exact-head adversarial review — REQUEST_CHANGESReviewed commit: What survives scrutiny
P1-1 — Canonical claim can strand a displaced successor (src/hooks/session.ts:1031)The canonical claim renames the shared pathname P1-2 — No lock-directory/evidence type revalidation before mutation (src/hooks/session.ts:1002-1031)Inspection rejects symlinked lock dirs and symlinked/non-regular evidence (src/hooks/session.ts:934-966), but that is a snapshot. After Regression coverage gapsNo tests for: live canonical successor remaining releasable/acquirable after a refused claim; lock-directory symlink replacement after inspection; evidence-file symlink/non-file replacement after inspection; quarantine-path appearing between preflight Required before re-review
Signed verdict: REQUEST_CHANGES. — |
…-clobber reversible Independent exact-head review of 4cc3b29 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
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 308628559e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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); |
There was a problem hiding this comment.
When the selected state root is on a filesystem that rejects hard links (for example exFAT/FAT, some SMB/network mounts, or Windows paths returning EPERM), this link() throws; the catch rolls the claim back and returns recovered: false, so launches continue to abort on the same positively-dead owner and omx session lock recover cannot clear it. Use an exclusive copy/no-clobber fallback for hard-link-unavailable errors before rolling back.
Useful? React with 👍 / 👎.
Fresh independent exact-head adversarial review — REQUEST_CHANGESReviewed commit: Survives scrutiny
Blockers (all at exact head 3086285)
Required before re-review
Signed verdict: REQUEST_CHANGES. — |
…ine, and removal Second independent exact-head review of 3086285 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
Repair update — new head
|
Fresh independent exact-head adversarial review (round 3) — REQUEST_CHANGESReviewed commit: Round-2 finding status
SurvivesPID reuse and identity-indeterminate fail-closed (908-990); no-clobber claim/rollback/quarantine destinations; the repaired live-successor chain. Blockers at this exact head
Accepted direction (from the review's own tradeoffs)Pure Node has no atomic unlink-if-same-inode / rmdir-if-same-directory. The review accepts the conservative contract: never delete a pathname after a stale observation. Planned implementation ("park-and-verify"): every removal becomes an atomic Signed verdict: REQUEST_CHANGES. — |
GJC exact-head adversarial review verdict\n\nVerdict: BLOCKED — independent review unavailable.\n\nReviewed target:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 643e7e687a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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.` : ''}` }; |
There was a problem hiding this comment.
When the hard-link claim succeeds but parking/removing the original evidence fails (for example EPERM/EBUSY from Windows file sharing, antivirus, or a transient permission error), this returns without deleting the just-created .recovery link. The lock directory is then left with both owner.json and the recovery entry, so later inspection reports an ambiguous lock and omx session lock recover refuses to proceed until manual cleanup; this failure path should remove or roll back the claim before returning.
Useful? React with 👍 / 👎.
Fresh independent exact-head adversarial review (round 4) — REQUEST_CHANGESReviewed immutable commit: Prior REQUEST_CHANGES disposition
Blockers at this exact head
Verification
Passing tests do not cover either unsupported intermediate state above. Signed verdict: REQUEST_CHANGES. — |
Repair update — new head
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a271673726
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await transactionDependencies.fs.unlink(checkpointPath); | ||
| await transactionDependencies.fs.rmdir(context.lockPath); |
There was a problem hiding this comment.
When a checkpoint resume reaches this point but the final rmdir fails, for example because Windows reports a transient EBUSY or an unexpected extra entry makes the directory ENOTEMPTY, the checkpoint has already been deleted and the parked evidence has already been moved to quarantine with the claim removed. The catch then returns failure, but later attempts see a canonical lock directory with no owner and no checkpoint to resume, so launches keep aborting with a missing-owner recovery requirement until manual cleanup.
Useful? React with 👍 / 👎.
| 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'); |
There was a problem hiding this comment.
This resume validation only accepts checkpoints whose sourcePath is owner.json, but the recovery path still handles pre-rename owner.<token>.tmp evidence and writes that temp path into the checkpoint. If recovery is interrupted after checkpointing such a temp-owner lock, every subsequent recoverSessionPointerLock() rejects the checkpoint as malformed before it can finish or retry the still-dead temp evidence, leaving that lock stuck until the files are removed manually.
Useful? React with 👍 / 👎.
Red-team review — REQUEST_CHANGESIndependent exact-head review of What's solid
Blocking findingTransient checkpoint-write / evidence-park failure strands the claim hardlink and re-creates the permanent deadlock this PR exists to fix. In
In both cases the lock dir then contains Fix is small and matches the function's own idiom: Non-blocking notes
Signed: red-team review at immutable head |
Repair update — new head
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f3362c0dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await transactionDependencies.fs.writeFile(evidenceCheckpointPath, JSON.stringify({ | ||
| version: 1, | ||
| sourcePath: evidencePath, | ||
| parkPath: `${context.lockPath}.parked-${claimToken}`, |
There was a problem hiding this comment.
When using the default token() implementation, this checkpoint records ...parked-${claimToken}, but removeIfIdentityMatches() mints a fresh token for the actual park name, so the recorded path is not the file that gets renamed out of owner.json. If the process dies after the evidence is parked but before the checkpoint is removed, the next recoverSessionPointerLock() looks for the wrong parkPath, rejects the checkpoint as malformed, and leaves the lock directory with only the .recovery hard link until manual cleanup. Pass the intended park path into the helper or have the checkpoint use the helper's chosen path.
Useful? React with 👍 / 👎.
Independent exact-head adversarial review (round 6) — REQUEST_CHANGESReviewed immutable head Round-5 blocker disposition — FIXEDThe narrow cleanup repair at New P1 — checkpoint
|
Repair update — new head
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06162998fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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); |
There was a problem hiding this comment.
In the checkpoint-resume path, if the recovery process is killed after this rename succeeds but before the claim, lock directory, and checkpoint are cleaned up, the checkpoint still records only parkPath even though the owner bytes now live at quarantinePath. The next recoverSessionPointerLock() rejects the checkpoint when lstat(checkpoint.parkPath) fails, while the lock directory can still contain the recovery claim or be empty, leaving future launches stuck behind a recovery that now refuses to resume. Record/update the checkpoint phase before moving the parked evidence, or accept an already-existing quarantine path and continue cleanup.
Useful? React with 👍 / 👎.
Fresh independent exact-head adversarial review (round 7) — REQUEST_CHANGESReviewed immutable head Round-6 finding disposition
Standing automated Codex finding on this head — concur
What survives scrutinyNo-clobber claiming and rollback (link-only, EEXIST refusal), dev/ino revalidation pre/post claim and on quarantine, park-before-delete with EBUSY re-verification, residue-preserving failure diagnostics, symlink/foreign-inode fail-closed paths, checkpoint schema/phase/path binding with out-of-root rejection, multiple-checkpoint refusal, and PID-reuse start-ticks gating all hold; no foreign-byte deletion, no-clobber regression, or path-traversal found. Local verification at this head: Verdict: REQUEST_CHANGES — the round-6 rmdir-retryability P2 is reproduced as unresolved (checkpoint retained but unresumable; lock permanently bricked), and the directory-incarnation claim race remains open. — |
Independent exact-head adversarial review (round 7 / terminal) — APPROVEReviewed immutable head Round-6 P1 — distinct park-token recording: FIXED
Round-6 P2a — temp-owner checkpoint resumption: FIXEDResume validation now accepts both the canonical Round-6 P2b — final-rmdir/checkpoint ordering: FIXEDResume now removes the lock directory before unlinking the checkpoint (1150-1151), so a transient Residual non-blocking observations (follow-ups, not merge blockers)
Hosted CIAll lanes on Regression sweep (non-constant tokens)No regressions found: claim/evidence/park/quarantine naming composes correctly with per-call Verdict: APPROVE — all round-6 findings are fixed and regression-covered; residuals are hardening follow-ups. No mutation, no merge performed from this lane. — |
1 participant
Summary
Fixes #3261.
After SIGKILL/OOM/power loss while holding the session pointer lock, launch aborts with
session_pointer_lock_recovery_requiredwhileomx session lock recoverrefuses a positively dead canonicalowner.json(safeToRecoverwas gated toowner-tempevidence only) — a permanent deadlock requiring manualrm -rfof the lock dir.Change
One gate line in
inspectSessionPointerLockAtContext(src/hooks/session.ts): broadensafeToRecoverfromstatus === 'dead' && evidenceSource === 'owner-temp'tostatus === 'dead'. Positively dead canonicalowner.jsonnow flows through the existing atomic claim/quarantine protocol, which revalidates the exactly-one entry, still-dead status, and owner token before quarantining — the identical code path already race-tested for temp evidence.deadrequires positive ESRCH viaprobePid; EPERM/EACCES/unknown staysidentity-indeterminate→ fail-closed.session.json.tmp-<token>cleanup added: pointer temps are inert by design (token-unique, writtenwx, never scanned by inspect/acquire; documented by the existing successor-commit test).Tests
owner.json→ claim → quarantine rename sequence, idempotent second recover (absent/none).session_pointer_lock_recovery_required) → explicit recover →writeSessionStartsucceeds.Verification
dist/hooks/__tests__/session.test.js— 76/76 pass.dist/hooks/__tests__files +dist/cli/__tests__/session-search-help.test.js— pass.npm run build,check:no-unused,lint(775 files) — clean.npm testwas started; the harness aborted the run mid-suite with zero test failures up to that point, and the one file-level crash artifact (doctor-warning-copy) passes 70/70 in isolated rerun. Full-suite green is left to CI on this PR.—
[repo owner's gaebal-gajae (clawdbot) 🦞]