feat(file-lock): add retainOnExit opt-out for process-exit cleanup - #227
Conversation
Locks acquired with retainOnExit: true keep their sidecar on natural process exit, for callers whose ownership records are deliberately fail-closed (PID death alone must not release them). Default exit-cleanup behavior is unchanged.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: blocked before merge. Reviewed September 4, 2026, 5:49 PM ET / 21:49 UTC. ClawSweeper reviewWhat this changesThis PR adds a Merge readiness⛔ Blocked before merge - 3 items remain Keep open for maintainer approval: the final head resolves the prior lifecycle findings and provides direct built-package proof, but it adds a security-sensitive public opt-out whose caller-owned stale-lock policy needs an explicit contract decision. Priority: P2 Review scores
Verification
How this fits togetherFile locks create sidecar ownership records next to protected files to coordinate cross-process access. Exit handlers normally remove those records; this option leaves selected records for a caller-defined stale-lock policy to govern. flowchart LR
A[Caller acquires file lock] --> B[Sidecar ownership record]
B --> C{Natural process exit}
C -->|Default| D[Guarded cleanup removes sidecar]
C -->|Retain on exit| E[Sidecar remains]
E --> F[Caller stale-lock policy]
D --> G[Later lock acquisition]
F --> G
Decision needed
Why: The implementation is contained and fail-closed across mixed package copies, but accepting a caller-controlled exemption from shutdown cleanup is a public filesystem-safety boundary decision. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Adopt the documented opt-out only if maintainers endorse retained sidecars as an explicit fail-closed public policy; otherwise keep the v0.8.0 cleanup-only contract. Do we have a high-confidence way to reproduce the issue? Not applicable as a bug report: the submitted built-package child-process evidence directly demonstrates the new default and retained exit behaviors. Is this the best way to solve the issue? Unclear pending maintainer direction: the implementation is narrow and preserves defaults, but the public retained-lock policy itself requires approval. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 3afa58251b49. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles)
|
…he option Review feedback: manager reset() is explicit teardown, not process exit, so it must still remove retained raw locks; only the exit handlers honor the flag. List retainOnExit in the published acquire-options docs and cover the reset path.
|
Addressing the review on head 54f875c (pushed). P2 — reset() independence: correct; P3 — documented option shape: Real-behavior proof: direct child-process runs against built {
"default": { "lockFileSurvivesExit": false },
"retainOnExit": { "lockFileSurvivesExit": true }
}The default mode removed the sidecar at exit; the retained mode preserved it. The same proof passes on the child-process suite in CI. Full serial suite: 7,513 passed / 80 skipped; the two @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
merge-risk: 🚨 security-boundary
retainOnExit is only honored when this package copy registered the process-exit handlers. When an older copy registered them first, retained acquisition now rejects with helper-unavailable instead of silently losing the guarantee at shutdown.
|
Addressing the re-review on head 70ea15a (pushed). P1 — mixed-version exit handlers: agree, and fixed fail-closed. Exit-handler registration now stamps a retain-aware marker ( The same reasoning covers the Root-backed Verification: the exit suite passes 15/15 (retained raw + Root survival, reset independence, legacy refusal, plus the existing default-release and changed-sidecar cases). Full gate: 7,514 passed / 80 skipped; the two @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
A reentrant same-owner acquisition reuses the held entry; a retainOnExit request now upgrades it so the sidecar survives exit. A later default acquisition never downgrades. Covers the default-then-retain sequence in-process and across natural child exit.
|
Addressing the re-review on head 0b761f6 (pushed). P2 — reentrant upgrade: agree. Retention is now monotonic: a same-owner acquisition with Re-verified against built {
"default": { "lockFileSurvivesExit": false },
"retainOnExit": { "lockFileSurvivesExit": true }
}Serial suite: 7,516 passed / 80 skipped; the two @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦞 diamond lobster
|
Maintainer disposition for the remaining review item: Rationale: the package's documented model is that natural-exit cleanup is best-effort and callers with fail-closed ownership records already exist (OpenClaw's dist-artifact build lock, where PID death cannot prove a detached writer stopped). The option is opt-in, defaults are unchanged, mixed-version processes fail closed rather than silently losing the guarantee, and stale recovery remains the caller's explicit policy. Documented in The current-head review (2026-09-04T21:49:07Z, head All exact-head CI, coverage, benchmarks, and CodeQL gates pass. Serial local suite: 7,516 passed / 80 skipped; the two |
…ct lock on exit fs-safe 0.8.0 made process-exit sidecar release the default, which broke the deliberately fail-closed dist-artifact owner record. 0.8.1 adds the retainOnExit opt-out (openclaw/fs-safe#227); adopt it so only the explicit release after a joined child removes the record. Also update the whatsapp session assertion for the conditional parent-directory chmod.
* chore(deps): update @openclaw/fs-safe to 0.8.0 fs-safe 0.8.0 stops repairing existing secret-directory permissions and fails closed instead. Wrap OpenClaw secret writes to tighten OpenClaw-owned directory modes before delegating, preserving the documented tighten-then-write behavior. Move the cooldown exclusions to the 0.8.0 family. * fix(infra): adapt to fs-safe 0.8.0 fail-closed boundaries - privateFileStore: tighten OpenClaw-owned store roots to 0700 at creation, preserving tighten-then-write behavior for drifted dirs - doctor transcripts: mask dirMode to permission bits; raw stat.mode includes file-type bits that the strict final directory-mode check rejects - file-transfer: resolve the symlink canonical target locally when the fs-safe error no longer carries it - archive test: 0.8.0 preserves the destination when a raced hardlink is rejected (PR197); update assertions * chore(deps): update @openclaw/fs-safe to 0.8.1 and retain dist-artifact lock on exit fs-safe 0.8.0 made process-exit sidecar release the default, which broke the deliberately fail-closed dist-artifact owner record. 0.8.1 adds the retainOnExit opt-out (openclaw/fs-safe#227); adopt it so only the explicit release after a joined child removes the record. Also update the whatsapp session assertion for the conditional parent-directory chmod. * fix(infra): never chmod through a symlinked secret root The 0.8.x tightening wrapper resolved rootDir with realpath before its lstat walk, so a symlinked root had its destination chmodded before fs-safe could reject the write. Walk the lexical chain instead; any symlinked component, including the root, is left for fs-safe to reject. Adds a regression test asserting the destination's mode is untouched. * chore(deps): narrow the lockfile refresh to the fs-safe bump The previous lockfile was rebuilt from scratch, drifting unrelated resolutions. Regenerate from main's lockfile so only the fs-safe family moves 0.7.2 -> 0.8.1 (temporarily allowing the old entries for the supply-chain policy check during regeneration). * fix(infra): pin directory identity through permission tightening Both 0.8.x permission adapters checked a directory with lstat and then chmodded it by pathname, leaving a check/use window where a swapped component redirected the chmod. Open each component with O_NOFOLLOW|O_DIRECTORY and fchmod the pinned descriptor instead; swapped or symlinked directories are never mutated and remain fs-safe's to reject. Adds private-file-store coverage for the tighten and symlinked-root paths. * refactor(infra): share descriptor-pinned directory tightening Both permission adapters now use one module (private-dir-mode): an async chain walk for secret writes and a sync single-root tighten for store factories, eliminating the duplicated safety logic. The module documents the residual same-principal bound: Node has no dirfd-relative open, so inter-component traversal is by absolute pathname; an actor able to replace an OpenClaw-owned directory mid-walk already holds write access to that tree, and fs-safe still validates the final write. * ci: retrigger checks-ui-e2e on the control-ui runner class Reruns route to GitHub-hosted runners (run_attempt > 1), where the dashboard-gallery Playwright test exceeds its 30s locator timeout. The full shard passes locally (228/228) on this head and the identical UI code passed CI at 9cbd8c6; a fresh run restores attempt-1 placement.
1 participant
Summary
0.8.0 started releasing Root-backed sidecar locks on natural event-loop shutdown. That is the right default, but it broke a legitimate consumer pattern: OpenClaw's dist-artifact ownership deliberately retains its lock file across exit when a build child leaves unjoined work, because PID death alone cannot prove the writer stopped.
This adds
retainOnExit?: booleantoacquireFileLock/withFileLockoptions. When set, the lock's sidecar is exempt from both thebeforeExitRoot cleanup and the synchronousexitfallback, and remains governed only by the caller's stale policy. Default behavior is unchanged.Validation
retainOnExit: truesurvives natural exit in both Root-backed and raw modes; existing tests continue to prove default release, changed-sidecar preservation, and single-attempt failure cleanup.consumer-pnpm-lifecyclefailures reproduce identically on unmodified main in this environment (the suite requires a pnpm lifecycle context this shell does not provide); they are unrelated to this change.docs/sidecar-lock.md) and changelog updated.Boundaries
Explicit
release()still works normally; the option only exempts the process-exit handlers. This is an opt-out for fail-closed ownership records, not a way to skip cleanup silently.