feat(microvm): host-written per-session pidfiles with PID-reuse defense (RIG-2498) - #974
Merged
trunk-io[bot] merged 3 commits intoSep 8, 2026
Conversation
|
Compass engineering docs preview: https://compass-runner-2498-w1-pidfi.compass-eng-docs.pages.dev Deployed from |
TestLaunchFailClosedTeardown narrows PATH to its own fake bin so that cloud-hypervisor is absent, but both fakes ended in a bare `sleep 30`. There is no `sleep` in that dir and /bin/sh has no `sleep` builtin, so each fake died on `sleep: command not found` milliseconds after recording its pid. The daemons were not orphans being reaped; they exited on their own, and the no-orphan assertion held trivially. Once waitForSockets became liveness-aware, that latent fixture defect turned into a merge-queue failure: launch now fails at the socket wait with "virtiofsd exited before its socket was serving" instead of reaching the cloud-hypervisor lookup, the second fake never records a pid, and readPidFile could only ever exhaust its 2s budget. Every observed failure landed at ~2.02s, the deadline itself. It reproduced only in the merge result, never on either head alone. Resolve `sleep` on the real PATH before narrowing it and burn the absolute path into the stubs, so the fakes outlive launch as the test always claimed. Add the control that makes this class of vacuity self-reporting: assert the error names cloud-hypervisor, the only absence this test arranges. Anything earlier means the daemons never came up. On the unfixed merge result that control fires on 39/40 runs while the assertion it guards stays green. readPidFile then needs no poll at all: each fake records its pid before it touches its socket, and the caller has confirmed launch got past the socket wait, which orders the write before the read. A missing pidfile is now a real ordering defect that fails immediately rather than a wall-clock budget that can only expire. Measured on the merge result (git merge-tree of main and the V6 head), 40 runs 5-way concurrent: 4/40 then 3/40 failures before, 0/40 after. Mutating Shutdown to skip reaping passt fails 12/12 on the real invariant in ~0.03s, so the assertion is load-bearing rather than merely green.
…se (RIG-2498) V7 W1 per the frozen record §(a)/§W1: the host writes a pidfile for each of the three children, carrying enough identity that a later reaper can tell a live child from a recycled PID. Each record is written in two steps. An `intent <bootid>` line goes down before the spawn, so a crash between fork and the settled write still leaves a removable marker; the settled `<pid> <starttime> <bootid>` line replaces it once the child is up. Both go through one atomic temp-file-plus-rename helper that chmods 0600 explicitly, because CreateTemp's mode is umask-masked and the record's permissions should not be an environment dependency. `alive()` checks the boot id first, so a record from a previous boot reads as dead rather than reaching the PID comparison at all. Within the same boot an intent record returns `errPidUnknown` — the spawn outcome is genuinely unknown, which is a third verdict and not a synonym for dead. A settled record compares `/proc/<pid>` field 22 against the recorded starttime, so a recycled PID does not read as the original child. Field 22 is parsed from the last `)` in the stat line: a process can put parentheses in its own comm, and splitting the whole line on whitespace shifts every later field. A round-trip through live `/proc` cannot catch that, since the write and the read share the parse and misparse identically, so `parseProcStartTime` is tested against synthetic lines whose field 22 is a known sentinel with decoys in the other numeric fields. passt's `--pid` argument is retired: the host now owns all three records, so there is one writer and one format rather than a daemon-specific special case. `VM.pidfiles` is registered before the intent write, not after the settled one, so teardown removes a record left behind by a failed spawn. Refs: RIG-2498
…idfile errors (RIG-2498) Two HIGH findings, both in the new launch wiring rather than the pidfile primitives. A settled-write failure hung Shutdown on a nil channel. vm.vmmExited is hoisted onto the VM only after startRecordedChild returns, but startRecordedChild can fail after startChild has already spawned the VMM — leaving a live process handle beside a nil channel, which is exactly the state launch deferred Shutdown runs in. The bare receive there blocked forever, replacing the launch error it was cleaning up after with a hang carrying no stack. Guard the RECEIVE alone, not the arm: such a child is started, live and unrecorded, so skipping its Kill would orphan precisely what the deferred Shutdown exists to reap. Guarding the whole arm was measured to leave the VMM alive past Shutdown. A fast-exiting child failed the boot with a /proc path instead of its own diagnostic. writePidfile reads /proc/<pid>/stat after startChild has installed the reaper, so the read can lose to cmd.Wait reaping the pid. Reshape that one case — and only when the child has provably exited — into the shape launch already uses for a dead child: name the daemon, wrap waitResult, carry the log tail. Every other write failure stays fatal, and the on-disk record is left alone so a child that may have become live is never under-named. That failure has TWO kinds, roughly half each: ENOENT when /proc/<pid> is already gone at open time, and ESRCH when the open succeeds but the task is reaped before the read. ESRCH does not satisfy errors.Is(err, os.ErrNotExist), so the obvious predicate silently misses about half the occurrences. Add procReadMeansGone covering both and use it at the alive() site too, where the same predicate decided whether a dead process routes to reapable or surfaces as a fault. Also: scope writePidRecordLine durability doc to a process crash (host-crash durability is deliberately not bought, since a changed boot id short-circuits every surviving record dead); document alive() two-kinds-of-error contract so the natural-but-wrong W2 call is harder to write; bound readPidfile read at 256 bytes; wrap the bare error returns with the path; move the three-name wiring assertion into the hermetic tier so it no longer needs KVM; dot-prefix the temp pidfile pattern.
rigel-mintaka
force-pushed
the
compass-runner/2498-w1-pidfiles
branch
from
September 7, 2026 17:34
45874a6 to
f399d70
Compare
mattwilkinsonn
approved these changes
Sep 8, 2026
|
😎 Stack merged successfully - 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
2 participants
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.
This PR is part of a stack containing 2 PRs:
mainV7 W1 per the frozen record §(a)/§W1: the host writes a pidfile for each of
the three children, carrying enough identity that a later reaper can tell a
live child from a recycled PID.
Each record is written in two steps. An
intent <bootid>line goes downbefore the spawn, so a crash between fork and the settled write still leaves
a removable marker; the settled
<pid> <starttime> <bootid>line replaces itonce the child is up. Both go through one atomic temp-file-plus-rename helper
that chmods 0600 explicitly, because CreateTemp's mode is umask-masked and the
record's permissions should not be an environment dependency.
alive()checks the boot id first, so a record from a previous boot reads asdead rather than reaching the PID comparison at all. Within the same boot an
intent record returns
errPidUnknown— the spawn outcome is genuinely unknown,which is a third verdict and not a synonym for dead. A settled record compares
/proc/<pid>field 22 against the recorded starttime, so a recycled PID doesnot read as the original child.
Field 22 is parsed from the last
)in the stat line: a process can putparentheses in its own comm, and splitting the whole line on whitespace shifts
every later field. A round-trip through live
/proccannot catch that, sincethe write and the read share the parse and misparse identically, so
parseProcStartTimeis tested against synthetic lines whose field 22 is a knownsentinel with decoys in the other numeric fields.
passt's
--pidargument is retired: the host now owns all three records, sothere is one writer and one format rather than a daemon-specific special case.
VM.pidfilesis registered before the intent write, not after the settled one,so teardown removes a record left behind by a failed spawn.
Refs: RIG-2498