[#239] Atomic durable writes + ownership-safe stale-lock reclaim

[realproject7]

Fixes #239

Implements #239's reviewed storage contract (Batch 39, code batch) — atomic durable writes + ownership-safe stale-lock reclaim + serialized token/joined-room RMW. Baseline fresh origin/main@6ee4951; head 5895e45.

EPIC Alignment

• Epic: [Audit][P1] Full-codebase quality, privacy, and release-hygiene sweep #238 — full-codebase quality/privacy/release-hygiene sweep. This ticket's role: repair confirmed storage durability + lock-ownership defects without widening token or persistence surfaces.
• No merged Batch-39 siblings to reconcile ([P1][Storage] Make durable state writes atomic and stale-lock reclamation ownership-safe #239 is the batch's only/first item); [P2][CLI] Handle broker, platform, and room bind failures as command errors #240–[P2][Release] Make npm package assembly reproducible and correct stale release documentation #243 remain out of scope and untouched (this diff is storage + cli/state.ts only).
• Contracts respected: atomic replacement preserves prior valid JSON until rename; 0600/0700 modes preserved; stale-lock reclaim cannot delete a newly acquired lock; token + joined-room RMW serialize under the appropriate lock. The reviewed flag:"wx" no-clobber amendment is honored by splitting replacement and create into separate operations.

What changed

• src/storage/secure-fs.ts — writeSecureFile is now atomic replacement: write payload to a sibling .tmp-<pid>-<n>, fsync, then rename over the target. Readers observe the whole old or whole new file, never a partial one; a crash/ENOSPC before the rename leaves the prior file intact. Added a separate createSecureFile for atomic no-clobber create (exclusive "wx" open) — the create path is never a rename-over-existing. Removed the flag option.
• Create-only callers moved to createSecureFile (identical no-clobber semantics): room-store brief/messages/state/participants (writeNewJson), forum-store new-post (writeJsonNew).
• src/storage/lock.ts — ownership-safe stale-lock reclaim, hardened across the three reviewer findings:
  1. Serialized, identity-checked primary reclaim. Reclamation runs behind an exclusive <lock>.reclaim lock (open "wx"); under it the primary is re-read and removed only if still byte-for-byte the record judged stale. TOCTOU-free — while the primary still holds that record it cannot be acquired (acquire needs the file absent, and only the serialized reclaimer removes it), so a live successor is never removed and check-then-remove cannot race a fresh acquire. (Replaces the earlier move-aside + link-restore approach entirely.)
  2. Liveness-based reclaim-lock recovery. The reclaim lock is stamped with the owner's identity ({pid, createdAt}) and recovered by liveness — a live owner keeps exclusivity regardless of age; only a dead owner (crash mid-reclaim), or an unparseable/legacy lock older than the stale window, is a removal candidate. A merely-slow reclaimer never loses the reclaim lock.
  3. Atomic takeover for abandoned-lock removal. Recovery renames the abandoned lock to a unique name (exactly one cleaner wins), then deletes only that moved inode, and only if it is still the record judged abandoned; a fresh successor moved in the gap is restored without clobbering rather than destroyed. (takeOverAbandonedFile, exported for direct regression coverage.)
• src/cli/state.ts (writeToken) and src/storage/joined-rooms-store.ts (recordJoinedRoom / setJoinedRoomArchived / deleteJoinedRoom) — the whole read-modify-write now runs under withWriterLock (new tokens.lock / joined-rooms.lock). Lock records hold only pid/createdAt — never a token.

Self-Verification

• Acceptance criteria 1:1:
  1. Failure before rename leaves prior valid JSON readable → atomic temp+rename by construction; test writeSecureFile replacement is atomic — concurrent reads never see a partial file (would fail on the old in-place writeFile).
  2. Concurrent writers can't both enter the critical section / lose token/joined-room entries → tests concurrent writers reclaim one stale lock without ever overlapping, racing reclaimers never double-hold, move a fresh lock aside, or wedge the lock (60 rounds × 4 reclaimers; verified it fails on the prior move-aside code and passes on the serialized reclaim), concurrent writeToken … keeps every token, concurrent recordJoinedRoom … keeps every entry.
  3. Tests cover atomic replacement, stale-lock contention, concurrent token/joined-room mutations → yes (above), plus lock-recovery regressions: an active-but-old reclaim lock is not cleared, preserving reclaim exclusion (liveness, not mtime — deterministic, fails on the prior mtime-only recovery) and takeover deletes only the record it judged; a fresh successor survives (deterministic two-cleaner model).
  4. Existing storage/CLI/token tests stay green → full suite 431/431 pass.
  5. No token to output/URLs/logs/browser-local metadata → lock records carry only pid/createdAt; token still flows only into tokens.json as before.
  • Amendment: flag:"wx" no-clobber retained, incl. concurrent-create contention; replacement vs create distinguished → tests createSecureFile refuses to clobber an existing target …, writeSecureFile replaces where createSecureFile refuses, concurrent createSecureFile — exactly one wins, every loser gets the already-exists failure.
• Kill-list scan: no token/URL in lock files or errors; no new runtime dependencies (Node built-ins open/rename/link/rm only; package.json unchanged); no token-format/public-API/message-semantics change; 0600/0700 preserved (tests assert mode & 0o777 === 0o600); directory enumerations safe (forum-store filters to dirs, registry filters .endsWith(".json") so transient .tmp- are excluded); no #240–#243 files touched.
• Evidence: pnpm typecheck ✓ · pnpm lint ✓ · pnpm kit-guard ✓ (4 panes clean) · pnpm no-stub ✓ · pnpm build ✓ · node --test dist/test/**/*.js → 431 pass / 0 fail. CI Release gates pass on 5895e45 (run 29380793335).

Deviations

• Reclaim architecture (evolved under review): stale-lock reclaim is serialized behind an exclusive, identity-stamped <lock>.reclaim lock with revalidate-before-remove, liveness-based recovery, and atomic-takeover cleanup — rather than the initial move-aside + link-restore. This closes the reviewer-identified races (moving a fresh successor aside; mtime-only clearing an active-but-slow reclaimer; a second cleaner force-rming a successor). Mutual exclusion ultimately rests on the primary's exclusive open("wx") + identity-checked removal.
• takeOverAbandonedFile is exported solely so the two-cleaner / fresh-reclaimer race has a deterministic, non-vacuous regression (a mismatched expectedRaw models a second cleaner acting after a successor replaced the judged record; a plain force-rm would delete it).
• Added fsync on the temp handle before rename (standard atomic-write durability) — not explicitly required by the contract, but it is what makes "a crash leaves the prior/complete file" hold across a real power loss. No runtime-dependency or API impact.
• Non-blocking residual (per @re2): a deep, crash-recovery-only edge remains inherent to any file-based lock's stale-recovery path (unobservable in single-process tests; cannot cause a primary double-hold, since the primary's exclusive open + identity-checked removal are the real mutex). Reviewer recommendation is not a further [P1][Storage] Make durable state writes atomic and stale-lock reclamation ownership-safe #239 patch round; a stronger primitive (OS advisory flock/lock-server) would be a separate follow-up, out of [P1][Storage] Make durable state writes atomic and stale-lock reclamation ownership-safe #239's scope.
• No UI surface → no Design Fidelity table.

[project7-interns]

Verdict: BLOCK

Epic Alignment: PASS

The live diff stays within #239: secure replacement/create paths, stale-lock reclaim, and token/joined-room RMW locking.

Checked (evidence)

• Structural gate: live REST PR body contains filled EPIC Alignment, Self-Verification, and Deviations sections.
• Contract/diff: src/storage/secure-fs.ts:22-55 separates atomic replace from exclusive create; src/storage/lock.ts:61-135 moves stale locks aside and restores without overwrite; src/cli/state.ts:42-52 and src/storage/joined-rooms-store.ts:48-125 lock complete RMW operations.
• Tests: test/storage-secure-fs.test.ts, test/storage-lock.test.ts, and test/storage-concurrent-rmw.test.ts cover the reviewed contract paths.
• Riskiest part: stale-lock reclaim; the move-aside plus link-only restore preserves an intervening lock rather than clobbering it.
• Kill-list: scanned all items — clean.
• CI: gh pr checks 244 → Release gates pending (live; checked twice).

Findings

• [blocking] Required CI has not completed.
  • File: N/A
  • Why it fails: mandatory live checks evidence is unavailable while Release gates is pending.
  • Do instead: wait for the required check to complete successfully, then request re-review; no code change is identified.

Decision

The reviewed diff aligns with #239, but approval is blocked until live CI completes successfully.

[realproject7]

RE2 review — REQUEST CHANGES (authoritative verdict delivered to @dev in project chat).

Atomic-write/create split (secure-fs) and the token/joined-room RMW locking are correct and well-tested; boundaries and safety invariants hold.

[REQUIRED] blocker — stale-lock reclaim mutual-exclusion TOCTOU. reclaimIfUnchanged (src/storage/lock.ts:98-123) renames the lock aside unconditionally (:101) without re-confirming the on-disk record still equals expectedRaw before the move. A lagging reclaimer can move aside a fresh, live lock a peer just acquired; the move→re-read→restore path (:101→:111→:130) is not atomic, so lockPath is empty across two awaits.

Reachable with 2 contending processes + a pre-existing dead-pid stale lock:

• P1 reclaims R_stale and acquires R_P1 (live).
• P2 (preempted between its read :64 and rename :101) renames R_P1 aside → lockPath empty while P1 is in its critical section → double-hold; and if P1 releases during P2's restore window, P2 links R_P1 back with pid P1 still alive → lock wedged until P1 exits.

This is the exact defect #239's acceptance forbids ("stale reclaim cannot delete a newly acquired lock"); the expectedRaw compare runs after the destructive rename, too late to prevent the empty window.

Fix: serialize reclamation under a dedicated meta-lock (<lockPath>.reclaim via exclusive wx) and re-confirm expectedRaw immediately before removal — or otherwise never rename aside a record not just re-validated as present, and never leave lockPath empty for a lock you don't own.

Test gap: test/storage-lock.test.ts contention runs in one process (one pid); it can't reproduce two independent reclaimers with distinct live records. Add a two-reclaimer (distinct pid records / two child processes) regression asserting no double-hold and no wedged lock.

[project7-interns]

Verdict: REQUEST CHANGES

Epic Alignment: FAIL

The stale-lock implementation does not yet meet #239's contract that reclaim cannot delete or break a newly acquired lock.

Checked (evidence)

• Structural gate: live REST PR body contains filled EPIC Alignment, Self-Verification, and Deviations sections.
• Atomic replace/create: src/storage/secure-fs.ts:22-55 and migrated create callers preserve replacement versus wx no-clobber semantics.
• RMW: src/cli/state.ts:42-52 and src/storage/joined-rooms-store.ts:48-125 lock the complete token/joined-room read-modify-write paths.
• Riskiest part: stale-lock reclaim. It is not acceptable as written; see finding.
• Kill-list: scanned all items — hit listed below.
• CI: gh pr checks 244 → Release gates pending (live), not verified.

Findings

• [required] Delayed stale reclaim can remove a live successor lock before checking identity.
  • File: src/storage/lock.ts:98-134
  • Why it fails: reclaimIfUnchanged performs rename(lockPath, moved) at :101 before comparing movedRaw to expectedRaw at :116. If P1 reclaims the stale record and acquires a fresh lock, then delayed P2 moves P1's live lock aside, leaving lockPath empty during P1's critical section; a third writer can enter concurrently. The later link-based restore can also resurrect a released lock and wedge future writers.
  • Do instead: serialize stale reclamation (for example, an exclusive reclaim meta-lock), re-read/revalidate immediately before removal, and ensure a reclaimer never moves a record it has not just verified as the stale record. Add a regression with two independently scheduled reclaimers/distinct lock records proving no double hold and no resurrected/wedged lock.

Decision

Request changes. The write/create split and RMW locking are sound, but the stale-lock race violates a mandatory #239 acceptance criterion.

[project7-interns]

Verdict: REQUEST CHANGES

Epic Alignment: FAIL

The new primary-lock serialization addresses the prior race, but its reclaim meta-lock is not ownership-safe and can re-open the same class of race.

Checked (evidence)

• Re-review range: ce2a230..596dea3; only src/storage/lock.ts and test/storage-lock.test.ts changed.
• Prior finding: resolved at src/storage/lock.ts:100-126 for a normally held reclaim lock; primary record is re-read before removal.
• Riskiest part: reclaim meta-lock recovery. It is not acceptable as written; see finding.
• Kill-list: scanned new ranges — hit listed below.
• CI: gh pr checks 244 → Release gates pass (live).

Findings

• [required] The active reclaim meta-lock is deleted solely by age.
  • File: src/storage/lock.ts:100-133
  • Why it fails: the meta-lock contains no owner identity, and a competing reclaimer executes rm(reclaimPath, { force: true }) whenever mtime exceeds staleAfterMs (:130-133). An active reclaimer paused or delayed past that interval loses its exclusion lock; another reclaimer can enter and both can independently revalidate/remove the primary, recreating the concurrent-reclaimer race. This also contradicts the comment that age alone proves a crash.
  • Do instead: store an owner record in the reclaim lock and reclaim it only after identity/liveness-safe verification, or otherwise make the reclaim lock recovery itself ownership-safe. Add a deterministic test with an active reclaim operation older than staleAfterMs showing a second reclaimer cannot remove its meta-lock or overlap primary reclamation.

Decision

Request changes. CI is green and the original primary-lock move-aside bug is fixed, but the required serialized-reclaim guard must itself be safe before #239 can be approved.

[realproject7]

RE2 — APPROVE (authoritative verdict delivered to @dev in project chat; re-review of my earlier REQUEST CHANGES).

My [required] stale-lock reclaim TOCTOU (msg 830) is resolved by 596dea3:

• Move-aside path removed (rename/link gone from lock.ts); reclamation now serialized under an exclusive <lockPath>.reclaim lock, then re-reads the primary and rms it only if current === expectedRaw — TOCTOU-free in normal operation (a live successor has a different record and is never removed; while the stale record is present the lock can't be acquired).
• Non-vacuous regression (racing reclaimers …, 60×4): verifies the on-disk lock stays byte-identical for the whole critical section, maxActive===1, clean release, no wedged .reclaim. Confirmed fail-on-old / pass-on-new.
• Release gates CI green on 596dea3 (run 29379662089); delta confined to lock.ts + its test.

Non-blocking follow-up (not required for this PR): clearAbandonedReclaimLock uses if (isOlderThan) rm(reclaimPath, {force:true}) — the same "unconditional rm after a time-based stale check" pattern, reachable only in crash recovery (an abandoned .reclaim >staleAfterMs old, then a multi-clearer race straddling a fresh acquire). Suggest an atomic takeover (rename the abandoned lock to a unique scratch name — exactly one clearer wins — then rm the scratch). Recommend a small follow-up ticket.

[project7-interns]

Verdict: REQUEST CHANGES

Epic Alignment: FAIL

The live-owner meta-lock case is fixed, but dead-owner meta-lock cleanup is still not identity-safe under concurrent cleaners.

Checked (evidence)

• Re-review range: 596dea3..6977288; only src/storage/lock.ts and test/storage-lock.test.ts changed.
• Prior active-but-old finding: resolved by owner record plus live-pid retention at src/storage/lock.ts:100-108,136-157.
• Riskiest part: concurrent dead-owner cleanup. It is not acceptable as written; see finding.
• Kill-list: scanned new ranges — hit listed below.
• CI: gh pr checks 244 → Release gates pass (live).

Findings

• [required] Two reclaim-lock cleaners can delete a fresh successor reclaim lock.
  • File: src/storage/lock.ts:136-157
  • Why it fails: P1 and P2 can both read the same dead reclaim record at :139-151. P1 removes it at :153, a new reclaimer creates a fresh reclaim lock, then P2's unconditional rm(..., force) at :153 deletes that successor. The same race exists for two cleaners of the old malformed path at :155-157. This breaks the serialized-reclaim exclusion the primary fix relies on.
  • Do instead: atomically take ownership of the stale reclaim record (for example rename it to a unique scratch path, with exactly one cleaner winning) and remove only that moved inode; a loser must not remove whatever is now at reclaimPath. Add a two-cleaner regression that races dead reclaim recovery with a fresh reclaimer and asserts the fresh reclaim lock survives.

Decision

Request changes. The liveness fix is correct, but #239 cannot approve until dead reclaim-lock takeover is also atomic/ownership-safe.

[realproject7]

RE2 — APPROVE (re-affirmed at 6977288) — authoritative verdict in project chat.

Both @re2 review items are now resolved: my [required] reclaim TOCTOU (fixed in 596dea3) and @re1's reclaim-lock recovery finding (fixed here in 6977288).

6977288 makes reclaim-lock recovery liveness-safe: the .reclaim lock is identity-stamped {pid, createdAt}, and clearAbandonedReclaimLock keeps a live owner's lock regardless of age (only a dead owner, or an unparseable/legacy lock past the stale window, is cleared). Non-vacuous deterministic regression: a live-owned reclaim lock backdated 10 min is NOT cleared → the waiter times out rather than re-entering reclamation (fails on the prior mtime-only recovery, passes now). Release gates CI green on 6977288 (run 29380022370); delta confined to lock.ts + its test.

Non-blocking fast-follow (unchanged): the dead-owner clearAbandonedReclaimLock double-clear remnant — crash-recovery-only, contained by the primary current === expectedRaw revalidation. Atomic-takeover (rename-to-unique-scratch, one winner) would close it; suggest a small follow-up ticket, not required here.

[project7-interns]

Verdict: APPROVE

Epic Alignment: PASS

The full PR remains within #239: durable replacement/create separation, ownership-safe stale-lock recovery, and serialized token/joined-room RMW; #240-#243 remain untouched.

Checked (evidence)

• Re-review range: 6977288..5895e45; only src/storage/lock.ts and test/storage-lock.test.ts changed.
• Prior finding resolved: src/storage/lock.ts:153-203 atomically renames an abandoned reclaim record to a unique takeover path and deletes only that moved inode when bytes still match; mismatch restores without overwriting a fresh successor.
• Regression: test/storage-lock.test.ts adds matching-record removal, successor preservation, already-gone no-op, and no leftover takeover scratch coverage.
• Riskiest part: concurrent reclaim-lock cleanup; atomic takeover makes one cleaner win and preserves a reappearing successor, so it cannot delete the current reclaim lock by stale path.
• Kill-list: scanned new ranges — clean.
• CI: gh pr checks 244 → Release gates pass (live, run 29380793335).

Findings

• None.

Decision

APPROVE. All of my stale-reclaim findings are resolved on 5895e45, with green live CI and focused regression coverage.

[realproject7]

RE2 — APPROVE (re-affirmed at 5895e45) — authoritative verdict in project chat.

@re1's takeover finding is fixed via the atomic-takeover primitive: takeOverAbandonedFile renames the abandoned lock to a unique name (exactly one cleaner wins; loser gets ENOENT and no-ops), deletes only that moved inode if it still matches the judged record, and restores a fresh successor via non-clobber link rather than destroying it. Deterministic regression proves a successor survives (a plain force-rm would delete it), matching records are removed, and no .takeover- scratch lingers. Release gates CI green on 5895e45 (run 29380793335); delta confined to lock.ts + its test.

Assessment: normal-operation primary mutual exclusion is sound. The remaining reclaim-lock crash-recovery edge is a deep, single-process-unobservable cross-process window — non-blocking, with diminishing returns on further file-lock hardening; if provably race-free crash recovery is ever wanted, that's a stronger-primitive (flock/lock-server) follow-up, not a #239 blocker. #239's contract (atomic durable writes + ownership-safe stale-lock reclaim) is met.

All @re2 review items resolved. Both reviewers approve on the current head.