fix(lock): treat sync compromise-check I/O as a lost lock

[SebTardif]

What Problem This Solves

Fixes an issue where consumers using acquireFileLockSync() with onCompromised would never be told the sidecar became unreadable. The periodic check threw from setInterval on I/O such as EIO or EACCES, so the holder kept working as if it still owned the lock.

Why This Change Was Made

#134 already maps asynchronous compromise-check I/O to a lost lock via .catch(() => false). The synchronous timer introduced in #59 / #64 still called verifyStillHeld() unguarded. This PR applies the same fail-closed detector on the sync path and leaves explicit verifyStillHeld() throwing so callers that inspect on demand still see the I/O error.

User Impact

A sync lock with compromiseCheckIntervalMs now fires onCompromised once when a verification read fails, then stops the timer. Direct verifyStillHeld() calls are unchanged.

Evidence

terminal output from the patched package at /tmp/fs-safe-sync-compromise. After acquire, lstatSync on the sidecar was forced to EIO:

$ node /tmp/proof-fs-safe-sync-compromise.mjs
elapsed_ms=17
onCompromised_count=1
lockPath=.../fs-safe-sync-proof-Bk8Ycz/state.json.lock
explicit_verify_throws=true
result=PASS

pnpm check passed: 1045 tests, 61 skipped, docs examples match, pack check clean.

• Tests added or updated when behavior changed
• Security and compatibility impact considered
• CHANGELOG.md updated when release-relevant
• No credentials, private paths, private hosts, or sensitive contents included

Real behavior proof

• Behavior or issue addressed: Sync compromise monitoring threw from the interval on snapshot I/O and never invoked onCompromised.
• Real environment tested: macOS 15, Node v22, patched worktree at /tmp/fs-safe-sync-compromise built with pnpm build.
• Exact steps or command run after this patch: Acquired acquireFileLockSync with a 15ms compromise interval, then forced lstatSync on the sidecar to throw EIO.
• Evidence after fix: terminal output above. onCompromised fired once at 17ms. A later explicit verifyStillHeld() still threw EIO.
• Observed result after fix: The sync interval matches the async lost-lock contract from fix(lock): treat compromise-check I/O failure as a lost lock #134. Holders are notified once. On-demand verification still surfaces the I/O error.
• What was not tested: A live disk EIO from a failing volume (the injected EIO is the same Node error object the runtime uses).

Related

• Follows openclaw/fs-safe#134 (async interval).
• Sync timer introduced in openclaw/fs-safe#59 and openclaw/fs-safe#64 (2026-07-26 / 2026-07-27).

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed August 15, 2026, 3:54 PM ET / 19:54 UTC.

ClawSweeper review

What this changes

The PR catches synchronous periodic sidecar-lock verification failures, reports the lock compromised once, and preserves throwing behavior for direct verification calls.

Regression provenance

Possible regression — suspected (reviewed change). No predecessor PR is attributed.

Merge readiness

✅ Ready for maintainer review

Keep open for merge: current v0.5.6 still lets synchronous periodic verification throw from its timer, while this focused patch matches the existing asynchronous fail-closed behavior. The regression test and terminal proof support the change; no actionable patch defect was found.

Priority: P2
Reviewed head: d65a8e2411baf4dadbd0240be3d7b27c4567f721

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused, documented repair with direct runtime proof, regression coverage, and green cross-platform checks.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (terminal): The PR body includes after-fix terminal proof from a built package with injected sidecar EIO, showing one compromise callback and preserved direct-verification failure.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The PR body includes after-fix terminal proof from a built package with injected sidecar EIO, showing one compromise callback and preserved direct-verification failure.
Evidence reviewed	6 items	Current released behavior: The v0.5.6 current-main implementation invokes synchronous verification directly inside the compromise timer, so a sidecar-read error can escape the timer before the callback runs. Patch preserves fail-closed handling: The branch maps a thrown synchronous verification to not-held, clears the timer, and invokes the existing callback once; direct handle verification remains outside that catch. Existing async contract: The current asynchronous acquisition path already converts a rejected periodic verification to false before its once-only callback path, establishing the matching contract.
Findings	None	None.
Security	None	None.

How this fits together

Sidecar locks record ownership next to a target file and periodically re-check that ownership. The synchronous monitor must notify the holder when it cannot verify the sidecar, so downstream work does not continue trusting an unverifiable lock.

flowchart LR
A[Sync lock holder] --> B[Periodic sidecar check]
B --> C[Verify ownership snapshot]
C -->|held| B
C -->|changed or I/O failure| D[Stop timer]
D --> E[Call compromise callback]
E --> F[Holder handles lost lock]

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Focused lock repair	4 files changed: production +8/-2, tests +44, docs and release note +10	The change is confined to synchronous compromise monitoring and includes a direct regression case.

Technical review

Best possible solution:

Merge the narrow parity repair so both lock-monitoring modes fail closed and notify once when periodic verification cannot read the sidecar.

Do we have a high-confidence way to reproduce the issue?

Yes—current main directly calls synchronous sidecar verification from the timer, and the added focused test reproduces the relevant lstatSync EIO path from source.

Is this the best way to solve the issue?

Yes—the patch is the narrowest maintainable fix because it mirrors the already-shipped asynchronous lost-lock policy without changing explicit verification semantics.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 01ef06f04e49.

Labels

Label justifications:

• P2: This is a bounded reliability and safety correction for synchronous lock holders, with no evidence of a broad active outage.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix terminal proof from a built package with injected sidecar EIO, showing one compromise callback and preserved direct-verification failure.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal proof from a built package with injected sidecar EIO, showing one compromise callback and preserved direct-verification failure.

Evidence

What I checked:

• Current released behavior: The v0.5.6 current-main implementation invokes synchronous verification directly inside the compromise timer, so a sidecar-read error can escape the timer before the callback runs. (src/file-lock-sync.ts:268, 01ef06f04e49)
• Patch preserves fail-closed handling: The branch maps a thrown synchronous verification to not-held, clears the timer, and invokes the existing callback once; direct handle verification remains outside that catch. (src/file-lock-sync.ts:268, d65a8e2411ba)
• Existing async contract: The current asynchronous acquisition path already converts a rejected periodic verification to false before its once-only callback path, establishing the matching contract. (src/sidecar-lock-acquire.ts:186, 01ef06f04e49)
• Focused regression coverage: The added test forces lstatSync on the owned sidecar to throw EIO, verifies one callback and no uncaught exception, and confirms a later direct verification still throws. (test/file-lock-sync-failure.test.ts:159, d65a8e2411ba)
• Feature provenance: Blame attributes the unguarded timer lines on current main to the existing sync-lock implementation, while the branch attributes the new catch and consolidated once-only guard to this PR. (src/file-lock-sync.ts:268, d65a8e2411ba)
• Release check: Current main is tagged v0.5.6 and does not contain the branch catch; this remains an unmerged repair rather than work already shipped. (CHANGELOG.md:5, 01ef06f04e49)

Likely related people:

• steipete: Merged the initial sync-lock surface and the owner-scoped sync-lock refactor visible in the file history. (role: sync-lock feature introducer and recent area contributor; confidence: high; commits: b1be2a119986, 7706b966edf3; files: src/file-lock-sync.ts, test/file-lock-sync-failure.test.ts, docs/sidecar-lock.md)
• SebTardif: Authored the prior merged asynchronous compromise-check repair and this synchronous counterpart. (role: related behavior contributor; confidence: high; commits: 4cde8b1eaa50, d65a8e2411ba; files: src/sidecar-lock-acquire.ts, src/file-lock-sync.ts, test/file-lock-sync-failure.test.ts)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)

• reviewed 2026-08-15T09:55:56.320Z sha d65a8e2 :: needs maintainer review before merge. :: none

[clawsweeper]

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(lock): treat sync compromise-check I/O as a lost lock This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.