Skip to content

fix(setup): ask the kernel whether a pid is the state lock's owner - #1072

Merged
Juliusolsson05 merged 3 commits into
mainfrom
fix/state-lock-live-pid-steal
Sep 20, 2026
Merged

Juliusolsson05 merged 3 commits into
mainfrom
fix/state-lock-live-pid-steal

Conversation

@Juliusolsson05

@Juliusolsson05 Juliusolsson05 commented Sep 20, 2026 •

Copy link
Copy Markdown
Owner

Refs #993.

The evidence

On 2026-09-18 a dev build and the installed app ran against ~/.config/agent-code simultaneously, while agent-code.process-lock.json did not exist at all — with the dev process actively writing control history and layout mutations.

The mechanism

defaultIsLockOwnerActive answered 'inactive' — delete the file and take over — in two very different situations: the owner pid is gone, and the owner pid is alive but ps printed a command line that does not contain the recorded argv0.

The second is a guess, and there are ordinary reasons for it to fail while the owner is very much alive and writing: the bundle replaced in place by an update, the app renamed or moved, a dev binary at a path that has since changed, or ps rendering the argv differently than process.argv[0] spelled it.

Acting on it puts two mains on one state directory, and it leaves the lock file unable to describe either. Once the incumbent's lock is overwritten the file carries the successor's token: the incumbent's release no longer matches and is a silent no-op, while the successor's release deletes the file although the incumbent is still running.

The fix: ask the kernel, don't guess

A first version routed the mismatch into the existing five-minute 'inconclusive' grace window. Review showed that does almost nothing — the window is measured from the lock's startedAt, which is when the incumbent acquired the lock, i.e. roughly its uptime. Every reason for a mismatch is permanent for the life of that process and never resolves by waiting, so anything older than five minutes was stolen from exactly as before. The app packages macOS only and /bin/ps is always on the Finder-launched PATH, so 'inconclusive' had become almost exclusively "argv0 mismatch" — the one input for which waiting is guaranteed not to help.

There is no need to guess at all. The question the heuristic was groping for is "is this pid the process that wrote the lock, or a recycled number wearing it?", and the kernel answers it exactly: a process cannot have started after the lock its own acquisition wrote.

classifyLockOwner now reads ps -o lstart= (under LC_ALL=C, as mitmproxyReaper already does for the same field) and compares against the lock's startedAt:

  • started later → a reused pid, and nothing else → take over immediately
  • started at or before → the genuine owner, however its command line now reads → refuse, at any age, no window involved
  • neither question answerable → the bounded window, which is now only "ps said nothing at all"

A two-second tolerance covers lstart's one-second resolution.

Also fixed

  • Impossible timestamps unbounded the windows. A clock corrected backwards leaves a recorded time in the future; a negative age is below every threshold forever, so the window never opened and the user had to delete a JSON file by hand. Clamping to zero does not fix it either — the age stays pinned at zero until the real clock catches up. An impossible timestamp is not evidence of freshness, so it now makes the lock immediately eligible for cleanup, with a one-minute jitter margin because filesystem mtimes carry sub-millisecond precision that Date.now() does not.
  • Two clauses that could never decide anything are gone: any command line containing the full argv0 necessarily contains its basename, and an empty basename is accepted by includes from every string. Mutation testing found both unreachable.

What was ruled out

Recorded in the code so the next reader does not redo it:

  • ps truncation — ps -p <pid> -o command= does not truncate when piped on macOS 25.6 (measured; the reviewer independently confirmed with ~3,650 characters of argv).
  • The shutdown gate — onQuitAllowed runs only on the re-entered will-quit after teardown settled. A rejected drain keeps the quit prevented and never releases.
  • The crash hooks — every uncaughtException arm releases and then process.exit(1).
  • The startup-throw path — keeps the journal and lock and calls app.quit(); verified not a hole.

Tests

This file had no tests, which is how a concurrency guard this load-bearing drifted. classifyLockOwner is exported and reads the world through an injectable probe, so every branch is reachable — including the 'active' half, which nothing could previously reach, and which a mutation could previously delete outright with the suite green. Acquisition and release are driven against a real directory.

mutation result
live-pid mismatch steals again (the bug) 3 failed
drop the executable match 3 failed
a dead pid treated as alive 1 failed
reused pid kept as the owner 1 failed
no tolerance on lstart rounding 1 failed
unanswerable probes steal 1 failed
grace window shrunk to nothing 1 failed
grace window never expires 1 failed
future timestamps blow the window open 2 failed
no jitter tolerance 1 failed
release ignores the token 2 failed
acquire writes a constant token 1 failed
fresh unparseable lock treated stale 1 failed
restored 65 passed

npx tsc -b clean.

Why Refs and not Fixes

The mechanism does not reproduce the reported snapshot exactly: under this story the successor holds a valid lock for as long as both mains run, so the reported moment — both live and the file already absent — needs the successor to have exited. It is one way to reach that state, not a proven account of the incident.

Two of the issue's suspects remain untouched: no dev-plus-installed reproduction was run, and install-time migration was not investigated. requestSingleInstanceLock being per-bundle-id is unchanged and remains a residual.

🤖 Generated with Claude Code

Juliusolsson05 and others added 3 commits September 20, 2026 03:40
On 2026-09-18 a dev build and the installed app ran against
`~/.config/agent-code` at the same time, while the lock file that is supposed
to prevent exactly that DID NOT EXIST — no live owner, no stale lock, just
absent, with the dev process actively writing control history and layout.

THE MECHANISM. `defaultIsLockOwnerActive` answered 'inactive' in two very
different situations: the owner pid is GONE, and the owner pid is ALIVE but
`ps` printed a command line that does not contain the recorded `argv0`. Both
went down the stale-lock path — delete the file and take over.

The second one is a guess, and there are ordinary reasons for that string
match to fail while the owner is very much alive and writing: the bundle was
replaced in place by an update, the app was renamed or moved, a dev binary
lives at a path that has since changed, or `ps` rendered the argv differently
than `process.argv[0]` spelled it.

Acting on that guess produces #993's evidence exactly. The newcomer overwrites
a live incumbent's lock, so both run. And from that moment the incumbent's own
release no longer matches the token in the file, so whichever process exits
first deletes the lock and leaves the other one running unprotected and
invisible — which is the "lock file absent while a live process writes
STATE_DIR" that was observed.

THE FIX is the asymmetry. Refusing to steal costs a bounded wait: the existing
five-minute grace window expires and the lock is cleaned up anyway, so PID
reuse after a reboot still cannot lock anyone out. Stealing costs two main
processes writing one state directory. So 'inactive' is now reserved for the
one thing actually known — the pid is gone — and a live-but-unconfirmable
owner joins the case where `ps` itself failed, which already had the grace
window and the reasoning for it.

WHAT WAS RULED OUT along the way, so the next reader does not redo it:
- `ps -p <pid> -o command=` does NOT truncate when its output is a pipe on
  macOS 25.6 (measured: 477 characters of argv came through intact), so a
  truncated command line is not the cause.
- The shutdown gate does not leak the lock. `onQuitAllowed` runs only on the
  re-entered `will-quit` after teardown settled, or when there is no
  SessionManager to tear down; a rejected drain keeps the quit prevented and
  never releases.
- The crash hooks do not leak it either: every `uncaughtException` arm
  releases and then `process.exit(1)`.

This file had NO TESTS, which is how a concurrency guard this load-bearing
drifted. It has them now, driving the real function against a real directory.
The live-pid case uses the REAL classifier and a REAL live pid — this test
process — because the defect is in the classifier, not in how acquisition
treats its answer; `ps` genuinely runs, and the command line it prints for
vitest cannot contain the recorded argv0, which is the same shape as an app
whose bundle moved.

Mutation-checked, 52 tests:

  restore the live-pid steal (the bug)   1 failed
  a dead pid also waits                  1 failed
  grace never expires                    1 failed
  release ignores the token              1 failed
  async release ignores the token        1 failed
  fresh unparseable lock treated stale   1 failed
  an active owner is stolen from         1 failed
  restored                              52 passed

Fixes #993

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review findings on #1072. The first version's title was false: it still stole
from a live pid, just five minutes later.

FINDING 1 (high). The grace window is measured from the lock's `startedAt`,
which is when the incumbent ACQUIRED the lock — roughly its uptime — not how
long we have been unable to confirm it. Every reason for an argv0 mismatch
(bundle replaced in place, app renamed or moved, dev binary at a path that has
since changed, `ps` rendering argv differently) is permanent for the life of
that process and never resolves by waiting. So routing the mismatch into that
window protected only incumbents younger than five minutes, and five minutes
of uptime is nothing. The app packages macOS only and `/bin/ps` is always on
the Finder-launched PATH, so after that change 'inconclusive' meant almost
exclusively "argv0 mismatch" — the one input for which waiting is guaranteed
not to help.

There is no need to guess. The question the heuristic was groping for is "is
this pid the process that wrote the lock, or a recycled number wearing it?",
and the kernel answers it exactly: a process cannot have started AFTER the
lock its own acquisition wrote. `classifyLockOwner` now reads `ps -o lstart=`
(LC_ALL=C, as mitmproxyReaper already does for the same field) and compares.
Later than `startedAt` is a reused pid and nothing else; at or before it is the
genuine owner, however its command line now reads — with no window involved,
so it holds at any age.

FINDING 5b. `ownerAgeMs`/`invalidAgeMs` went negative when a recorded time sat
in the future — a clock corrected backwards, a VM snapshot restore, a
dual-boot local-time RTC — and a negative age is below every threshold
forever, so the window never opened and the user had to delete a JSON file by
hand. Clamping to zero does not fix it either: the age stays pinned at zero
until the real clock catches up. An impossible timestamp is not evidence of
freshness, so it now makes the lock immediately eligible for cleanup, with a
one-minute jitter margin because filesystem mtimes carry sub-millisecond
precision that `Date.now()` does not and the freshest possible lock can read a
hair ahead of the clock.

FINDING 4. A false WHY comment: "whichever process exits first deletes the
file". It is not whichever — after a steal the file carries the SUCCESSOR's
token, so the incumbent's release is a no-op and the successor's delete is
what leaves a live writer with no lock.

FINDING 3. Ten of nineteen mutations survived, including deleting the entire
command-line match. `classifyLockOwner` is now exported and reads the world
through an injectable probe, so every branch is reachable — including the
'active' half, which no test could previously reach. Mutation testing also
found two clauses that could never decide anything: any command line
containing the full `argv0` necessarily contains its basename, and an empty
basename is accepted by `includes` from every string. Both are gone.

FINDING 2. Nothing pinned the window from BELOW — shrinking it to a second
shipped green. It is pinned now, in both directions.

FINDING 6. The test drove the real classifier against the vitest pid and would
have flipped had this repository lived under a directory named "Agent Code".
The probe removes the coupling.

FINDING 7. `Fixes #993` is now `Refs #993`. The mechanism does not reproduce
the reported snapshot exactly — under this story the successor holds a valid
lock while both mains run, so the reported moment (both live AND the file
already absent) needs the successor to have exited. Two of the issue's
suspects are untouched: no dev-plus-installed reproduction was run, and
install-time migration was not investigated. The third (startup-throw path) I
verified independently is NOT a hole, and that is recorded.

Mutation-checked, 65 tests:

  live-pid mismatch steals again (the bug)    3 failed
  drop the executable match                   3 failed
  a dead pid treated as alive                 1 failed
  reused pid kept as the owner                1 failed
  no tolerance on lstart rounding             1 failed
  unanswerable probes steal                   1 failed
  grace window shrunk to nothing              1 failed
  grace window never expires                  1 failed
  future timestamps blow the window open      2 failed
  no jitter tolerance                         1 failed
  release ignores the token                   2 failed
  acquire writes a constant token             1 failed
  fresh unparseable lock treated stale        1 failed
  restored                                   65 passed

Refs #993

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review finding 4 on #1072. The earlier text said "whichever process exits
first deletes the file", which is not what happens: after a steal the file
carries the successor's token, so the incumbent's release is a silent no-op
and the successor's delete is what leaves a live writer with no lock. It also
now says plainly that this reaches the reported state without being a proven
account of the reported moment.

Refs #993

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Juliusolsson05 Juliusolsson05 changed the title fix(setup): never take the state lock from a pid that is still alive fix(setup): ask the kernel whether a pid is the state lock's owner Sep 20, 2026
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Review round addressed. Finding 1 was the one that mattered and it was right: the grace window is anchored to the incumbent's uptime, and every reason for an argv0 mismatch is permanent, so five minutes bought essentially nothing. My own test asserting the self-heal was documenting the surviving steal.

Your discriminator is the fix. ps -o lstart= against the lock's startedAt — a process cannot have started after the lock its own acquisition wrote. Later → reused pid, take over immediately. At or before → the genuine owner at any age, no window involved. Neither answerable → the bounded window, which now means only "ps said nothing at all". Two-second tolerance for lstart's one-second resolution. The heuristic is gone from the decision entirely.

5b — clamping to zero would not have fixed it. I tried that first: the age stays pinned at zero until the real clock catches up, which may be years. An impossible timestamp is not evidence of freshness, so it now makes the lock immediately eligible for cleanup — with a one-minute jitter margin, because without one the freshest possible lock (the create/write race, mtime a hair ahead of Date.now()) becomes instantly stale. That margin is its own test; removing it fails.

3 — the injectable probe closes all ten. And chasing M4/M6 turned up something better than a test: both clauses are unreachable. Any command line containing the full argv0 necessarily contains its basename, and an empty basename is accepted by includes from every string. They are deleted rather than covered.

4 — you are right, it is not "whichever". The comment now says the successor's release is what deletes the file while the incumbent runs on.

2 — pinned in both directions now; shrinking the window to a second fails.

6 — the probe removes the checkout-path coupling entirely. Good catch; that would have been a miserable failure to debug.

7 — I have retitled to Refs #993 and said so in the body. You are right that the mechanism does not reproduce the reported snapshot: under this story the successor holds a valid lock while both mains run, so the reported moment needs the successor to have exited already. The body now lists which suspects remain untouched (no dev-plus-installed repro, install-time migration uninvestigated) and records that the startup-throw path is verified not a hole, which is worth keeping whoever closes the issue.

65 tests, 13 mutations caught, npx tsc -b clean.

@Juliusolsson05
Juliusolsson05 merged commit 3e2e1f9 into main Sep 20, 2026
2 checks passed
@Juliusolsson05
Juliusolsson05 deleted the fix/state-lock-live-pid-steal branch September 20, 2026 11:21
Juliusolsson05 added a commit that referenced this pull request Sep 20, 2026
Four PRs merged (#1068, #1069, #1071, #1072), closing #1025, #952 and #930.
#1073 and #1074 are open with their review findings fixed. #993 stays open
deliberately: the mechanism does not reproduce the reported snapshot.

Records the two reviewer instructions that produced every blocking finding —
mutate the implementation and report the survivors, and count the real corpus
rather than reasoning about it — because three of the four blockers were
regressions this loop had just introduced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant