Conversation
proper-lockfile defaults onCompromised to `(err) => { throw err; }` and calls
it from inside the lock's mtime-refresh filesystem callback, so the throw is
an uncaught exception rather than something the caller can handle. None of the
daemon's four lock sites passed the option; core/auth-storage.ts already does,
which is what the correct handling looks like.
The socket-path lease is the dangerous one. The supervisor acquires it at
startup and holds it until it exits, while the refresh runs on a timer that
cannot fire when the event loop is blocked - and the supervisor blocks it
routinely (execFileSync("ps") for process identity, Atomics.wait in the
session-lease guard, readFileSync journal loads). A stall past the 5s stale
window lets a second supervisor declare the lock stale, rmdir it, and take
over; the first supervisor's next refresh then throws. daemon-supervisor.ts
installs no uncaughtException handler, so the process dies and every session's
control plane goes with it.
Record the loss on the lease instead of throwing, and act on it where it
matters: prepareDaemonSocketPath fails startup loudly, and
cleanupDaemonSocketPath skips the unlink, because the socket at that path may
already belong to the successor that stole the lease.
The other three sites - the supervisor ownership registry guard, the
update-restart coordinator guard, and the session-lease guard - get a
non-throwing handler for the same reason.
Without this change the new tests report unhandled errors from the refresh
callback.
|
Hi @Jiaaqiliu, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/PrimeIntellect-ai/prime-agent/blob/main/CONTRIBUTING.md for more details. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
proper-lockfiledefaultsonCompromisedto(err) => { throw err; }and calls it fromsetLockAsCompromised, which runs inside the lock's mtime-refresh filesystem callback. A throw there is an uncaught exception, not something the caller can handle.None of the daemon's four lock sites pass the option:
modes/daemon/daemon-socket.ts(socket-path lease, and the sync cleanup lock)modes/daemon/daemon-supervisor-ownership.ts(registry guard)cli/daemon-update-restart.ts(coordinator guard)core/session-lease.ts(lease guard)core/auth-storage.tsalready passesonCompromised, which is what the correct handling looks like.Impact
The socket-path lease is the dangerous one. The supervisor acquires it at startup and holds it until it exits, while the refresh runs on a
setTimeoutthat cannot fire while the event loop is blocked — and the supervisor blocks it routinely:execFileSync("ps")for process identity,Atomics.waitin the session-lease guard,readFileSyncjournal loads.A stall past the 5s
stalewindow lets a second supervisor declare the lock stale,rmdirit, and take over. The first supervisor's next refresh then throwsECOMPROMISED.daemon-supervisor.tsinstalls nouncaughtExceptionhandler (a repo-wide grep finds them only indaemon-mode.ts), so the process dies outright and every session's control plane goes with it.Fix
Record the loss on the lease instead of throwing, and act on it where it actually matters:
prepareDaemonSocketPathfails startup loudly with a clear messagecleanupDaemonSocketPathskips the unlink, because the socket at that path may already belong to the successor that stole the leaseThe other three sites get a non-throwing handler for the same reason: nothing may throw from that callback.
Tests
Adds
packages/coding-agent/test/daemon-socket-lease-compromise.test.ts, which acquires a lease and then removes the lockfile to simulate a steal. Onmainthe run reports unhandled errors from the refresh callback and both assertions fail.All 33 existing daemon/socket/lease/supervisor suites (690 tests) still pass.
Note
Fix compromised lockfile events to prevent daemon supervisor crashes
onCompromised: () => {}toproper-lockfilecalls across daemon socket, session lease, coordinator registry, and supervisor registry guards so that lockfile compromise events no longer throw from filesystem callbacks.compromisedstate onDaemonSocketPathLeaseviamarkCompromised(error)andcompromisedErrorgetter, allowing callers to detect and react to stolen leases without crashing.prepareDaemonSocketPathnow throws a descriptive error when passed a compromised lease;cleanupDaemonSocketPathskips unlinking in the same case.Macroscope summarized 4d310e8.