GeezerVison is showing github.com — Decide: keep .socket/apply.lock transient, or give the lock a file that never has to be deleted · Issue #808 · SocketDev/socket-patch
Decide: keep .socket/apply.lock transient, or give the lock a file that never has to be deleted #808Description[agent] Filed by the scheduled architecture audit routine (CLI and core). Register: discussion #560 register.
Question: should .socket/apply.lock keep its v5.0 "never outlives the command" lifecycle, which costs about 200 of the 553 production lines in patch/apply_lock.rs, or should the lock live somewhere it never has to be deleted?
Options:
- A. Keep a persistent lock file in the project.
acquire becomes create-if-missing → try_lock_exclusive → backoff; release is closing the handle. The identity check, the vanished and delete-pending retry streaks, the unlink-under-lock, the .socket/ prune and their Windows grace period all go. Cost: a run that persists nothing still leaves .socket/apply.lock (and a .socket/), so users need a .gitignore line or socket-patch writes .socket/.gitignore. This reverses the "nothing to .gitignore" promise in the contract's Lock lifecycle (v5.0) paragraph.
- B. Lock a file outside the project. Lock
<user cache dir>/socket-patch/locks/<sha256 of the canonical .socket path>.lock, which is never deleted. The project stays residue-free and the deletion machinery goes, as in A. Cost: two containers sharing one project volume but not one cache directory would no longer serialize, and canonicalization has to be right on case-insensitive and symlinked paths.
- C. No change. Keep the transient file and its machinery; only the layering refactor below proceeds.
Whichever option is chosen, the lock primitive should stop doing vendored crash recovery (see "Layering" below); that part changes no behavior.
Kind: decision. Source: review Part 7.4 (apply.lock); register row C26.
Problem
patch/apply_lock.rs is 553 production lines (1,157 with tests) for an advisory lock. Much of it exists only because the guard deletes the lock file on exit:
- Drop unlinks
apply.lock while still holding it, gated on an inode-identity probe, then prunes an empty .socket/;
- because a releaser can unlink between a waiter's open and lock, every attempt re-checks
same_file::Handle identity, and the acquire loop keeps two extra retry streaks (Vanished, bounded by VANISHED_LIMIT = 256, and Windows DeletePending, with a 2 s grace floor);
open_failure classifies macOS EINVAL and a missing parent as "a releaser's cleanup racing us".
The contract makes the lifecycle a v5.0 guarantee (introduced by #247), so changing it is a decision.
Layering (independent of the decision)
Taking the lock also does vendored-mode work:
acquire replays an interrupted vendored group commit (recover_group_commit) for every lock-taking command, agent-mode apply included;
- the guard's Drop runs the vendored artifact durability barrier.
So patch::apply_lock (agent-mode infrastructure) depends on utils::group_commit and utils::durability, and the commands that read vendored state without the lock (list, vex, vendor --check) never get the replay. A ProjectSession::open(socket_dir) (lock + recover, with the barrier on close) would make the pairing explicit and leave the lock a plain primitive.
Symptoms: none filed. Impact: about 200 production lines (comments included) and their race tests (waiter_does_not_lock_orphaned_inode_after_holder_release, orphaned_inode_holder_does_not_block_the_path, orphan_drop_leaves_the_live_holders_replacement_file_alone, the delete-pending classifier) exist only for the transient file.
Proposed change
- A or B: delete the identity check,
Attempt::Vanished/DeletePending, VANISHED_LIMIT, DELETE_PENDING_GRACE, open_failure's cleanup-race arms, the unlink in Drop and prune_empty_socket_dir, plus the race tests above; rewrite the contract's Lock lifecycle paragraph and the --dry-run row.
- C: none here.
- All options: move
recover_group_commit and the durability barrier out of apply_lock into one project-session type the CLI's lock_cli wrappers call.
Size and scope
patch/apply_lock.rs (about −200 production lines for A/B), commands/lock_cli.rs (docs only), CLI_CONTRACT.md. The layering move is ~80 lines, mechanical. Out of scope: which commands take the lock.
Acceptance criteria
- A maintainer picks A, B or C on this issue.
- For A/B:
apply_lock.rs has no inode-identity check and no vanished/delete-pending retry; concurrent_acquire_release_never_double_holds_and_leaves_no_residue is rewritten to assert no double hold.
- For A:
.socket/apply.lock is ignored by git in a fresh project (a test runs git status --porcelain after apply --dry-run).
- For B: two processes locking the same project through different spellings of its path (symlink, case on macOS/Windows) contend.
-
vendor_group_commit_e2e stays green.
Dependencies
Blocked by nothing. Related to the RunCtx tracking issue #793, which would own the session type. Activitymikolalysenko commented on Oct 4, 2026 CollaboratorAuthor More actions [agent] Triaged as priority:p3 (CLI core lock lifecycle refactor). This is a maintainer decision (options A/B/C), so it keeps agent:needs-human and is not eligible for agent work until someone picks an option. Related to #809, which also needs journal replay to work outside the lock. Not a duplicate, and no open PR addresses it.
Generated by Claude Code mikolalysenko commented on Oct 7, 2026 CollaboratorAuthor More actions Keeping a persistent lock is really annoying and honestly not even a problem most users are likely to care about. If we need a persistent lock we could put it in the home directory or somewhere unobtrusive, but it is not acceptable for this tool to leave junk all over a project directory. If we do need a persistent lock, putting it in the home directory is an option or adding some configurable path could work. mikolalysenko commented on Oct 7, 2026 CollaboratorAuthor More actions [agent] Decision, following the maintainer's comment: Option C, keep .socket/apply.lock transient. socket-patch must never leave a lock file in the project directory. A persistent lock in the project (A) is rejected, and so is a lock under the user cache dir (B), because nothing needs a persistent lock. If one is ever needed, it goes outside the project: the home/cache dir or a configurable path, never .socket/.
Is the transient design broken? No. The issue lists no symptoms, and the extra code (identity check, vanished/delete-pending retries, unlink while held) exists to make deleting the file safe, with race tests behind it. Those ~200 lines are what it costs to leave nothing behind.
Does anything leave apply.lock behind today? One path does: an interrupted run.
- Normal exits, error exits and panics all drop
LockGuard, which deletes the file and an empty .socket/ (apply_lock.rs:169-207). main calls process::exit only after the command has returned (main.rs:93-111), and release builds unwind on panic.
- Ctrl-C, SIGTERM or SIGHUP while
apply, vendor, scan --mode vendored, rollback, repair or remove holds the lock ends the process without running the guard's drop. The only signal handler today is the prompt's cursor guard (ui/prompt.rs:218-308), and it re-raises into the default action. On Windows, Ctrl-C/Ctrl-Break likewise exits without running destructors. Either way .socket/apply.lock stays (and so does a .socket/ the run created) until some later lock-taking command reclaims it.
Plan (PR to follow):
- While a lock is held, an interrupt (SIGINT/SIGTERM/SIGHUP on Unix; Ctrl-C/Ctrl-Break/console close on Windows) deletes
apply.lock before the process dies. It only deletes the file if it is still the one this process locked, the same check Drop uses. On Unix it also removes .socket/ if that leaves it empty. Then the signal goes on to its normal action, so exit codes and the prompt's cursor restore don't change.
- Only SIGKILL or power loss can still leave the file. The next lock-taking command already reclaims and deletes it.
- Docs: the contract's Lock lifecycle (v5.0) paragraph and the
--dry-run row gain the interrupt case and the rule "socket-patch never keeps a persistent lock in the project; a persistent lock, if ever needed, lives outside it". The module docs in apply_lock.rs and lock_cli.rs say the same. No migration note is needed, because this only makes the v5.0 promise hold in one more case.
- Tests: a Unix e2e test sends SIGINT to a run paused while it holds the lock and checks that
apply.lock and the empty .socket/ are gone. The pause is a debug-only failpoint.
Moving the journal replay and durability barrier out of the lock (the "Layering" half of this issue) is not part of this decision and stays with #809 / #793.
Generated by Claude Code mikolalysenko commented on Oct 7, 2026 CollaboratorAuthor More actions
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|