fix(hooks): atomically quarantine recovery directories (supersedes #3269)#3271
fix(hooks): atomically quarantine recovery directories (supersedes #3269)#3271Yeachan-Heo wants to merge 4 commits into
Conversation
|
This PR supersedes #3269 solely because GitHub PR association drift left Signed: GJC post-merge corrective lane |
|
Signature correction for the replacement review surface: #3271 supersedes #3269 solely because of GitHub PR head-ref association drift. It carries the unchanged corrective branch at exact head — |
![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: 7a60ca63e9
ℹ️ 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".
| async function defaultRecoveryRenameNoReplace(from: string, to: string): Promise<RecoveryRenameNoReplaceResult> { | ||
| if (process.platform !== 'linux') return 'unsupported'; | ||
| await new Promise<void>((resolve, reject) => { | ||
| const child = spawn('mv', ['-n', '--', from, to], { stdio: 'ignore' }); |
There was a problem hiding this comment.
When a recovery park/completed path already exists as a directory, this invocation does not perform a no-replace rename of from to exactly to: mv --help documents SOURCE DEST as moving into an existing DIRECTORY, and -T as the option to treat DEST as a normal file. In that collision case mv -n -- from to moves the lock/checkpoint under the foreign directory, then moveRecoveryPathNoReplace reports failure after the canonical lock has already been displaced, breaking the fail-closed quarantine path.
Useful? React with 👍 / 👎.
| 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 evidenceName = inspection.evidenceSource === 'owner.json' ? 'owner.json' : (await transactionDependencies.fs.readdir(context.lockPath)).find((entry) => /^owner\.([A-Za-z0-9_-]{16,128})\.tmp$/.test(entry)); |
There was a problem hiding this comment.
When the inspected recoverable evidence is an owner.<token>.tmp and another process removes or replaces the lock directory between inspection and this second readdir, the filesystem error rejects out of recoverSessionPointerLock rather than returning the structured fail-closed recovery result; the canonical branch below uses lstatRecoveryPath, so only temp-owner recovery can crash omx session lock recover instead of printing JSON/status.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES Exact-head verification passed for the review surface: local Blocking findings:
The first finding independently blocks merge because the core #3269 HIGH blocker—true atomic no-replace ownership transfer—remains unresolved. The test defects additionally mean the claimed race, rollback/residue, and migration evidence cannot be accepted. — |
Atomic-boundary repair update — exact head
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c455b706ba
ℹ️ 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".
| // an external interpreter or architecture-specific syscall ABI. Recovery is | ||
| // therefore enabled only when a maintained host integration supplies this | ||
| // seam; otherwise every source and destination pathname remains untouched. | ||
| return 'unsupported'; |
There was a problem hiding this comment.
When omx session lock recover reaches a definitely dead lock through the normal CLI path, it uses the default transaction dependencies, and I only found atomicRenameNoReplace overrides in tests. This default always returns unsupported, so recovery never moves the stale lock out of the canonical path and users who hit session_pointer_lock_recovery_required have no built-in way to unblock the workspace. Please wire a real maintained implementation into the production path, or keep a safe fallback before making this the default.
Useful? React with 👍 / 👎.
| const evidence = await lstatRecoveryPath(evidencePath); | ||
| const lock = await lstatRecoveryPath(context.lockPath); | ||
| const ownerBytes = evidence && sameRecoveryIdentity(evidence, { dev: evidence.dev, ino: evidence.ino }, 'file') ? await transactionDependencies.fs.readFile(evidencePath, 'utf8') : undefined; |
There was a problem hiding this comment.
If the lock or evidence path becomes unreadable after the initial inspection, these awaits can now throw EACCES, ENOTDIR, or a read error before any recovery checkpoint is written. Because this part of recoverSessionPointerLock is outside a try/catch, the CLI rejects instead of returning a fail-closed SessionPointerLockRecovery object for omx session lock recover --json; the previous revalidation path converted these races into a normal recovery refusal. Please catch these post-inspection I/O failures and return the existing non-recovered result shape.
Useful? React with 👍 / 👎.
|
REQUEST_CHANGES Exact-head verification passed: local Blocking findings:
No additional unlink/rmdir was found in the recovery/checkpoint path, and foreign-claim/path-entry identity checks are materially improved. Those positives do not resolve the three blockers above. Independent gates at this exact head: build passed; focused built session suite passed 118/118; grouped hooks plus session CLI lane passed across 66 files; — |
OWNER_CONFIRMATION_REQUIRED — native atomic boundaryThe terminal blocker cannot be repaired safely inside the current two-file TypeScript scope. Repository and package research found no maintained Node dependency providing A correct production implementation requires expanding scope to a maintained native OMX asset, most plausibly a Rust-backed command using a maintained syscall crate, plus release-manifest packaging, platform/architecture resolution, installation/hydration behavior, capability discovery before any recovery mutation, and integration tests. The existing No partial native implementation was retained. The worktree is restored to pushed exact head Owner decision required:
PR #3271 remains open and unmerged pending this product/scope decision. — |
1 participant
Supersession notice
Supersedes #3269 solely because GitHub's immutable PR head ref stopped tracking the canonical source branch after corrective commit
7a60ca63e978c4c10b15471a1f6990bb7707bc86.Before closing #3269:
7a60ca63e978c4c10b15471a1f6990bb7707bc86refs/pull/3269/headand REST PR head:8bf60bb0ac2dd680c5056a8fc82a6907326f3067This replacement uses the same branch and preserves the complete issue #3261 post-merge corrective history. No implementation was cancelled, rewritten for supersession, or merged.
Corrective scope
Post-merge hardening follow-up for #3261 and merged PR #3262:
Regression coverage
The original pushed-head suite was restored before adaptation; no pre-existing test declaration was deleted. Runtime focused coverage increased from 108 to 117 tests.
Deterministic cases include:
Verification
check:no-unused: passedgit diff --check: passedPrior review trail: #3269, including terminal REQUEST_CHANGES comment 5055883515 and signed round-3 repair update 5056145002.
No self-review or merge has been performed. This replacement is ready for a brand-new independent exact-head review.
Signed: GJC post-merge corrective lane