[r266-tech]

Description

A stale-lock claimant can pause before creating its generation claim, then resume after another taker has stamped a fresh owner and entered the critical section. The delayed claimant currently overwrites that live owner and enters too. A release after claim creation can instead escape as ENOENT.

Revalidate the observed owner, inode and timestamp under the existing generation claim, require it still to be stale, and treat release races as a lost claim. Always remove the claim in finally. Keep the existing owner/heartbeat/release protocol and state format.

Human Involvement

• A human participated in the implementation or review loop
• This PR was generated entirely by AI agents without human participation in the loop

Related Issue

Follow-up to #4591 and its unresolved generation-race finding:
#4591 (comment)

Type of Change

• Bug fix
• Test update

Changes Made

• Refresh onto main at 9ac008f, preserving current marker handling.
• Keep claimStaleLock private. Replace its old direct-helper tests with deterministic public withSessionLock tests using real filesystem operations and a bounded fs scheduling hook.
• Cover a live successor, heartbeat refresh, same-token/same-mtime inode replacement, and release after claim creation.
• Advance the Codex plugin manifest from 0.11.20 to 0.11.21 as required by the current distribution gate.

Testing

• node --test examples/codex-memory-plugin/scripts/session-state.test.mjs: 10 passed. On unmodified main, the same contract tests give 4 failed, 6 passed: three invalid entrants and one ENOENT. The successor callback remains active while the delayed claimant resumes.
• node --test examples/codex-memory-plugin/scripts/*.test.mjs: 204 passed, using local dummy credentials/configuration and stub servers.
• node --check on both changed modules: passed.
• git diff --check: passed.
• bash .github/scripts/check-plugin-version-bumps.sh refs/remotes/origin/main: checked against the actual commit before publishing.

macOS arm64, Node 26.13.0. No real Codex/model call, live OpenViking deployment, cross-platform or full repository suite was run. Local plugin passes do not establish that the historical hosted API failure has been repaired; checks for the refreshed head are reported separately.

Checklist

• Complete diff reviewed
• Existing state format and public lock signature preserved
• Validation scope and limits reported

[now-ing]

Review: fix(codex): revalidate stale lock generation before takeover

Verified at head 11dfadfda8 (local checkout, Node 26). The race analysis is correct and the fix is the right shape — details below, plus one coverage gap worth a fourth test.

The fix holds up

The base code's claimStaleLock did writeFile(ownerFile, token) unconditionally once it won the per-generation claim directory, so a claimant that paused between observing the stale lock and mkdir(claim-…) really can unseat a live successor. The revalidation chain now run under the claim gives each race its own tripwire, and the tripwires are genuinely independent:

• current !== seen → catches the next generation's stamp (token changed);
• sameNode(currentStamp, observed) → catches same-content replacement (inode changed);
• currentStamp.mtimeMs !== observed.mtimeMs (stamped case) → catches a heartbeat refresh;
• Date.now() - applicableMtimeMs <= staleMs → re-checks age at stamp time, not observation time;
• flag: "wx" for the unstamped case → catches a stamp that lands between the re-read and the write.

Cross-generation soundness also checks out: claimants of the same generation serialize on the shared claim-<hash> dir, and claimants of a different generation can only pass current !== seen if the owner changed underneath them — which it can't while the first claimant holds the claim. At most one taker passes validation at a time.

Two semantic changes I want to explicitly endorse:

1. finally { rmdir(claimDir) } — the base only cleaned the claim on the success path; now every exit cleans up (the release-race test asserts this via readdir).
2. Throwing on non-release-race fs errors instead of the base's catch {} + write-anyway. A broken fs state (EACCES/EIO) used to be silently converted into "claim failed" and then overwritten anyway; now it surfaces to the caller of withSessionLock, while isReleaseRace keeps the benign release-during-takeover window (ENOENT/ENOTDIR, including the missing-parent ENOTDIR the third test exercises) as a clean false.

Verification

• node --test examples/codex-memory-plugin/scripts/session-state.test.mjs → 9/9 at head (6 pre-existing + 3 new).
• CI double-green at head (PR Checks + API & CLI Integration), and the test file is registered in the plugin-tests lane (pr.yml:46), so the new race regressions actually run on every PR.

One coverage gap (non-blocking): sameNode is load-bearing but untested

I probed the one ABA shape no test pins: same token, same mtime, replaced inode — delete the owner file, rewrite identical content, restore the exact stale mtime. Token and mtime checks both pass; the only thing standing between the delayed claimant and the live owner is sameNode:

// observed = stat(ownerFile) taken before the pause, ownerFile then replaced with
// identical content and restored mtime:
const claimed = await claimStaleLock(dir, ownerFile, "delayed-owner", seen, 300_000, observed);
// head: claimed === false, owner preserved — rejected solely by sameNode ✓

The existing tests cover token inequality ("delayed generation") and mtime inequality ("heartbeat"), but nothing covers inode identity. Suggested fourth test, verbatim recipe from my probe: after taking seen/observed, rm(ownerFile) → writeFile(ownerFile, seen) → utimes(ownerFile, old, old) → expect claimStaleLock(...) === false and the owner preserved. That converts the inode check from "probably right" to "can't regress".

Minor style nit (feel free to ignore): applicableMtimeMs is computed before the seen !== null && currentStamp.mtimeMs !== observed.mtimeMs guard that makes its stamped branch meaningful — guarding first, then computing, would read in dependency order.

From my side this is approve-with-that-one-suggestion.

[r266-tech]

Added the inode-only regression in 58e8f12. It creates the replacement before renaming it over the old owner, avoiding allocator-dependent inode reuse, and asserts identical device/mtime but a different inode. The stale claimant is rejected, the owner content survives, and its claim directory is removed.

The new test fails with true !== false when only the sameNode guard is removed. With the guard restored, node --test session-state.test.mjs passes all 10 tests. Production code is unchanged in this follow-up.