Skip to content

a recorded identity that is never checked against the process it names: pidfile stops and the Windows detached handle both signal a pid on the strength of a written-down value #909

Description

@davidfarah2003

The defect is not a missing check. It is a stored value that reads as verified because it was written down.

ProcessOwner records a startedAt (packages/workspace/src/maintenance.ts:46), written the moment a process is spawned: new Date().toISOString() beside the pid (maintenance.ts:728, and identically at implementations/cli/src/commands/up.ts:928, up.ts:2025, implementations/cli/src/lib/restore.ts:604, implementations/cli/src/lib/isolated-broker.ts:405). It survives in journals, listener proofs and claim records. It is schema-validated on read (maintenance.ts:955) and compared field-by-field (maintenance.ts:1109, up.ts:1174). Everything about its treatment says evidence. And nowhere in the repo is it ever compared against the live process it describes: localProcessOwnerStatus (maintenance.ts:731) checks the hostname and sends kill(pid, 0) (maintenance.ts:734), and the only comparisons startedAt ever enters are record against record. A declaration is being consumed as a fact.

The shaped Windows handle in detached-spawn.ts is the same move one layer down. To outlive its parent, the launcher gives up the one identifier the OS guarantees not to reuse (the process handle, closed inside the native script at detached-spawn.ts:92) and hands back a pid-only ChildProcess (detached-spawn.ts:34) whose kill() forwards straight to process.kill(pid, signal) (detached-spawn.ts:41). The number is the recorded identity token, and it is never checked against the thing it claims to identify.

Both are declarations treated as facts. That is what makes them one class rather than two coincidentally related bugs, and why this is one issue: the remedy is not "add a check" to each site, it is to make the recorded value load-bearing, and to refuse the destructive action when it cannot be compared.

The mechanism, walked

A daemon dies without cleanup (SIGKILL, OOM, reboot, power loss); its pidfile survives, because preserving records on unconfirmed death is exactly what these paths correctly do. The OS later reuses the number for an unrelated process owned by the same user: an editor, a build, another service. A later cotal down reads the pidfile, parses the number, probes it, hears alive, sends SIGTERM, and escalates to SIGKILL when the stranger does not exit. Every weaker doubt is already handled fail-closed (unparsable content, unknown probes, refused signals, unconfirmed deaths); "alive, but is it the same process?" is the one question the stop paths never ask, and the one question their own recorded startedAt was positioned to answer and never does. Worse than absent, the value supplies the appearance of the answer: a record carrying a timestamp reads as a verified identity to every reader, human or code, that does not go looking for the comparison that does not exist.

What is already right, which is what makes the gap easy to miss

This is not a repo missing a safety culture; it is a repo whose stops stop one question short.

  • processStartToken in packages/workspace/src/advisory-lock.ts:68 IS the real pid-reuse check on Linux and macOS: /proc/<pid>/stat field 22 and ps -o lstart=, recorded at acquire and compared at inspect (advisory-lock.ts:156). And it is not sitting unused in one file: the extension mutation markers (packages/workspace/src/extension-mutation.ts:135, :148, :208) and the seed reconcile lock (implementations/cli/src/seed/lock.ts:113, :162) record and compare start tokens the same way. Every consumer is a lock or mutation coordination, and no stop path has one. That is the sharpest form of the gap: the repo reaches for this check whenever it needs to know whether a recorded owner is still real, and never reaches for it before sending a signal.
  • The liveness probe is a deliberate tri-state (packages/workspace/src/pid.ts:62): only ESRCH proves death, EPERM means alive under another user, everything else is unknown and never acted on as death.
  • stopManager reads argv before signalling on POSIX (implementations/cli/src/lib/manager-proc.ts:281) and refuses when the live pid is provably not a manager: "A LIVE pid that is provably NOT a manager is never signalled."

So the permissive sites below sit in the middle of a codebase that refuses on weaker doubts everywhere else. The gap is easy to miss precisely because the surrounding code is good: the hard parts (torn writes, errno semantics, mutual exclusion between concurrent stoppers) are done, and startedAt's paper trail looks like the last one. What remains is the one fact that requires cooperation from the OS rather than from our own bookkeeping.

The sites

Two rows below cite origin/feat/up-daemonize-persist rather than main, because those files do
not exist at HEAD yet: they arrive with an unmerged branch, so those citations will move with it and
lapse entirely if it never lands. The pidfile rows stand on main on their own, and the argument
does not depend on the Windows rows.

Site What it does today What it cannot prove
packages/workspace/src/pid.ts:62 probeLiveness process.kill(pid, 0) plus the errno mapping Which process the number names. It answers existence, never identity.
packages/workspace/src/pid.ts:111 readProcessCommand argv via /proc or ps on POSIX; hardcoded unreadable on win32 (line 112) Any identity fact on win32, by explicit decision (see the win32 section)
packages/workspace/src/maintenance.ts:731 localProcessOwnerStatus kill(pid, 0) and a hostname check; startedAt sits in the same record (maintenance.ts:46, written at :728) That the live pid is the recorded process. The recorded start time is never compared against the OS's own start time for that pid; startedAt is only ever compared record-against-record (maintenance.ts:1109), never against the process
implementations/cli/src/commands/down.ts:237 stopLocalProcess The generic stop for every registered component (manager, delivery, auth, broker, web, extension processes): parse pid, coordinate the .stopping marker, SIGTERM (line 314), SIGKILL (line 332), await ESRCH Attribution of any kind. No readProcessCommand, no start token, no owner comparison anywhere in its body
implementations/cli/src/lib/auth-proc.ts:224 stopAuthService parse, probe, SIGTERM, await confirmed death The pid's identity. No argv check even on POSIX, though argv reading is imported one file over in manager-proc
implementations/cli/src/lib/delivery-proc.ts:243 stopDelivery (on origin/feat/up-daemonize-persist) same shape as stopAuthService same
implementations/cli/src/lib/detached-spawn.ts:34 windowsDetachedChild (on origin/feat/up-daemonize-persist) returns a pid-only ChildProcess whose kill() forwards to process.kill(pid, signal) (line 41) after the native launcher closed its process handle The identity half of the shape it already names. assertDetachedChildExitObservable (line 52) handles the observability half ("A detached child may be signalable without being observable"); identity has no seam at all
implementations/cli/src/lib/isolated-broker.ts:362 stopByPid signals pids read from the durable handshake file after an alive() check Identity; and its ProcessOwner.startedAt (isolated-broker.ts:405) is stamped with the time the coordinator READ the handshake, not the process's start, so it could not satisfy a real start check even if one existed
implementations/cli/src/commands/down.ts:368 preserveStateDown finds this root's mesh with mesh.root === root, a raw string equality on filesystem paths, on the --preserve-state path That the two spellings name the same directory. meshesForRoot (packages/workspace/src/mesh-registry.ts:276) exists for exactly this and warns that "a raw === silently misses" (symlinked roots, macOS /var versus /private/var, Windows 8.3 short names); the plain down path already routes through it (down.ts:190, via removeMeshesByRoot), target resolution applies the same canonicalized predicate (packages/workspace/src/mesh-target.ts:364), and only this call bypasses it. Consequence is misidentification (a recorded mesh reads as absent, so the recovery verb refuses where found 0), not a signal

Callers worth naming: the Ctrl-C teardown of a foreground up stops the delivery daemon, manager and auth service by pidfile (implementations/cli/src/commands/up.ts:961), under its own comment "All kill by pidfile, symmetric" (up.ts:955), and only the manager leg has the argv check. cotal down manager does not go through stopManager at all; it goes through the generic stopLocalProcess, so even on POSIX the argv check does not cover the documented stop verb. The presence direction carries the same unproven claim without the destruction: implementations/web/src/web.ts:216 treats a live prior pid as "web dashboard is already running", which pid reuse turns into a false-up.

Why this is one class, not two issues

The pidfile paths and the Windows handle are the same defect on both sides of one spawn. The daemon side writes a record carrying only a number; the stop side signals a number. The Windows launcher in detached-spawn.ts is the newest and purest instance of the class: to survive the parent it gives up the process handle, the one identifier the OS guarantees not to reuse, keeps the number, and hands back an object whose kill is process.kill(pid, signal). The file already contains the shape of the problem half-recognized: the seam's doc says the handle is returned "after the native Windows launcher closes its process handle", and assertDetachedChildExitObservable refuses to treat absent event methods as evidence of exit. The observability half of "a pid without a handle" got a contract; the identity half got nothing.

Filing these as two issues would be the wrong cut for a concrete reason: a reader with two issues closes the cheap one and feels finished. The pidfile half looks like plumbing (add a token beside the pid on POSIX), and closing it alone leaves every Windows stop, including the new launcher path, signalling pids it cannot attribute while the class reads as handled. Closing the Windows half alone leaves cotal down, the highest-frequency destructive verb in the CLI, signalling unproven pids on every platform. One class, one issue: a signal sent from a record requires the record's identity claim to be checked against the live process, everywhere the repo sends one.

A fourth surface belongs in the same class, kept distinct from the signalling paths because its consequence is misidentification rather than a signal: preserveStateDown compares a recorded root by raw === (down.ts:368) where the registry's own doc says that comparison "silently misses" and ships the canonicalized one this call bypasses (mesh-registry.ts:274, mesh-target.ts:364). A recorded root compared by an equality that cannot detect the case it exists to catch, a start time treated as verified because it was written: the same habit wearing a different record.

The rule this repo already follows

Nothing here needs a new doctrine. The repo's existing rule, applied uniformly, covers every site: when identity cannot be established, refuse the destructive action.

  • down.ts:135: "Fail CLOSED: an unselected dependant that MIGHT be running (including one behind an unattributable/unknown pidfile) blocks a stop-last shutdown".
  • stopLocalProcess itself refuses unattributable content (down.ts:309), a refused signal (down.ts:325), and an unconfirmed death (down.ts:336), preserving the record each time.
  • assertManagerRecordReplaceable (manager-proc.ts:174) refuses to start over an unknown or unattributable record.
  • advisory-lock.ts treats an unobtainable token as active/unknown: "NEVER reclaimed on the missing token alone; only a PID-dead owner is reclaimable" (advisory-lock.ts:28). This is the repo making a recorded identity load-bearing: a token that no longer matches means recycled, and matching is checked against the live process, not against another record.
  • materializeSecretToFile (packages/workspace/src/auth-paths.ts:388) fails loud on an absent key rather than materializing an empty credential.
  • maintenance.ts:1141 throws listener-owner-ambiguous when ownership cannot be proven locally.

The contrast that sharpens the rule: divergentCwdAnchor (auth-paths.ts:429) is deliberately a REPORTER that never throws and never chooses, and that is correct, because it is a diagnostic with no destructive action behind it ("a detector that can abort its caller is that same defect once more, wearing the cwd's failure instead of its answer"). The distinction is not "silence bad, throwing good"; it is that a destructive action requires proof its target is what the record says. Every stop path in the table has a destructive action behind it, and none of them (except stopManager on POSIX) has the proof.

Suggested fix shape: make the recorded value load-bearing

The remedy is not "add a check" at N sites; it is to make the value that is already recorded answer for itself, and to refuse the destructive action when it cannot be compared. The shape is the advisory-lock mechanism generalized: at write time, record the process start token next to the pid in every pidfile the stop paths read; at stop time, compare the recorded token against the live process and refuse to signal on mismatch, which is verbatim the rule at advisory-lock.ts:156 ("A recorded start token that no longer matches means the PID was recycled: the original owner is gone (stale)."). One shared identity contract beside pid.ts, consumed by stopLocalProcess, stopAuthService, stopDelivery, stopManager, and the launcher's windowsDetachedChild, rather than five site-local patches. The migration question (what a reader does with a legacy bare pid, where no token was ever recorded) is part of the design, not something to settle in this issue. On POSIX, argv attribution can remain the manager's extra check. No patch is proposed here on purpose; the win32 half has an open question first.

The win32 question is open, and the existing position is argued

The gap on win32 is not an oversight being caught; it is a reasoned position being revisited. The rationale is written out at advisory-lock.ts:69:

Windows has no cheap, STABLE start token here: /proc is absent and a ps on PATH (MSYS/Git) returns inconsistent output between calls, which would make the SAME live lock read differently from the acquirer vs a reader and be misjudged stale. Per the no-token rule, return undefined so a live PID keeps the lock active/unknown (bounded-wait is the backstop) rather than a false reclaim.

And readProcessCommand's doc carries the same argument for argv (pid.ts:106): a platform that cannot look "behaves exactly as every platform did before this existed", because attribution may only downgrade a record on affirmative evidence that the live process is something else.

Two things change when the same question is asked in front of a signal rather than a lock. First, the lock's safety argument does not transfer: an absent token is safe there because a bounded wait is a real backstop, while a stop that proceeds on an unproven pid has nothing behind it; the destructive act itself is the fallback, so the thing being degraded is someone else's process. Second, the requirement is different: the lock polls, so its token must be cheap; a stop runs once, so its token only needs to be stable. And a stable win32 start time is not unobtainable, it is merely not free: the launcher itself held the process handle a moment before closing it (detached-spawn.ts:92 closes pi.hProcess in the same script that returns the pid), and the PowerShell Add-Type seam it already uses can read a process creation time without introducing new machinery of a different kind.

Does the no-fallbacks doctrine ("Throw if something is not supported in the current environment or config, rather than silently degrading", AGENTS.md:161) already decide this? Mostly yes, with one honest counterargument. Proceeding when identity cannot be established is a silent degradation: the stop reports a clean teardown over a signal sent to an unverified number, which is the shape the doctrine exists to prevent. The counterargument is the repo's own precedent that "cannot be represented on this platform" is sometimes not a refusal: manager-proc.ts:127 treats a chmod a Windows volume cannot represent as not a reason to refuse starting the manager. But that precedent covers hygiene around an action (a defence-in-depth file mode), not the identity of the target of the action; a kill whose target is unproven is a different category from a log whose mode is loose. So the reading holds on the stop side, and it would make the stop refuse on win32 until a token mechanism exists, exactly as it refuses on every weaker doubt today. What the doctrine does not settle is whether that refusal ships as the interim behaviour or arrives together with a win32 token, and that tradeoff (a platform whose stops currently work, versus the correctness of a destructive verb) is the open design decision.

Why this is worth fixing rather than noting

A stop that signals a pid it cannot attribute is indistinguishable, in every line it prints, from one that can, right up until the number belongs to something else. The failure is destructive outside the repo's blast radius: the process at the other end belongs to the operator and did nothing wrong. The paths involved are not rare corners; cotal down is the documented heal verb, and the Ctrl-C teardown of a foreground up runs the pidfile stops on every dev session. The reuse window is narrow, but this codebase already pays the cost of refusing on every other narrow window (torn pidfiles, seccomp errnos, refused signals), and on Linux the check is already written, tested, and sitting in advisory-lock.ts. Windows is where the real decision is, and the new detached launcher is about to make the class larger: every Windows stack it starts will be stopped through a handle-shaped object whose only identity is a number that was never asked to prove itself.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:releasePrimary affected area: release.bugSomething isn't workingseverity:criticalConfirmed critical-impact defect or security issue.triage:confirmedReported defect reproduces, or requested non-bug gap is independently verified.triage:owned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions