fix(cli): keep a live preview's ownership record and stop past a bad one

[miguel-heygen]

A missed liveness probe is not proof the preview is gone — a server blocked on a Puppeteer capture answers nothing for a second or two — but any miss retired the session record, and that record carries the only PID-reuse guard --stop has.

Reproduced

SIGSTOP a managed preview, then run preview --status. Before this change the record is deleted and never comes back, leaving every later stop to fall through to an unauthenticated port scan with no ownership proof at all. After it, the record survives and the preview is reachable again once resumed.

Only a wrapper process that is provably gone now retires a record. That record gains a process-birth token so a recycled PID reads as a different process, and it is written through a temp file and renamed — every reader deletes it when it fails to parse, so a torn read would otherwise destroy a live server's proof of ownership.

Two failure-propagation bugs in the stop path

• --kill-all collected the first unprovable record's exception and abandoned every server after it, so they were left running and unreported. Per-record failures are now collected and reported.
• A replacement refused to launch when the server it was replacing had already exited on its own — the goal state, treated as fatal, leaving the user to delete the session record by hand.

--list now shows managed sessions ahead of whatever else answers the scan, so the same server is not listed twice from its own self-report.

Stack

Based on u4a-process-kill-trust, whose OS-verified listener PID this relies on. Retarget to main before merging.

[terencecho]

LGTM.

Verified at 9c7fd170:

• Missed-probe fix: wrapperProcessIsAlive keeps the record when a scan misses but the wrapper's birth token still matches (previewLifecycle.ts:281-297). Test keeps a live preview's record when a single probe misses it pins it.
• PID-reuse guard: processIdentity returns a birth token (linux tick 22, mac lstart, Windows CIM CreationDate FileTime). ownedStopTargetPid requires identity(saved.pid) === saved.wrapperIdentity before treating saved.pid as a kill target and, when the live server pid ≠ saved.pid, additionally requires isProcessDescendant (bounded depth 64, visited-set, fail-closed on null parent). Tests pin both the descendant-happy path and the recycled-PID divergence.
• Fail-closed on server.pid=null: stopBackgroundPreview now throws preview ownership could not be proven instead of the prior saved.pid trust-degradation fallthrough. Test refuses to stop when the live server cannot prove its own PID pins it.
• Atomic session write: temp-file + renameSync in writePreviewSession — readers only see the pre-existing .json while the .pid.tmp is being filled, so a torn read can't destroy live ownership proof. listBackgroundPreviewStatuses filters .json only, ignoring temp orphans. Test never leaves a partial session record for a concurrent reader pins it.
• --kill-all per-record propagation: handlePreviewKillAll collects failures[] per session and continues, then sweeps via killActiveServers. Test keeps stopping after a record whose ownership cannot be proven pins it.
• Replacement-when-owned-exited: stopOwnedPreviewBeforeReplacement treats "nothing left to stop" as goal state, not fatal. Test launches the replacement when the owned server died on its own pins it.
• --list dedup: managed sessions prefix the scan, and the scan is filtered by ${resolve(projectDir)}\0${port}. Test prefers the managed record over the same server's own self-report pins it.
• Dead helpers retired: processIdentity + isProcessDescendant now have production callers.
• CI: all required green. Only marketplace WIP remains IN_PROGRESS (not required).

Prior comment on #3307 (COMMENT 4956803130) is closed by this PR: server.pid → saved.pid trust-degradation loop → replaced by fail-closed throw + birth-token gate. The Windows non-English netstat fail-open I flagged (portUtils.ts:223) is not directly patched here, but its harmful consequence in --kill-all is now fenced by pidSource !== "os" skip-and-warn in killActiveServers, and its consequence in --stop is fenced by the wrapperIdentity guard.

[jrusso1020]

Reviewed at 9c7fd170. All 8 required contexts are green, including both windows jobs — UNSTABLE is failing optional checks only, so CI is not the constraint here.

Audited end-to-end: previewLifecycle.ts (+249/-28) and its test file (+414/-16), preview.ts (+99/-29), preview.test.ts, plus utils/orphanCleanup.ts at this head (not in the diff, but it supplies the two helpers this PR makes load-bearing). Not executed: no suites run locally.

Strengths

• The two helpers that were dead at #3307's head are now genuinely load-bearing — processIdentity and isProcessDescendant feed readyPreviewSession, wrapperProcessIsAlive and ownedStopTargetPid. That was tai's finding on #3307; this closes it rather than deleting the helpers, which was the better of the two options since the birth token is what makes PID reuse detectable at all.
• The atomic write earns its comment. writePreviewSession goes through ${path}.${pid}.tmp + renameSync (:82-84), and the comment states the actual hazard: every reader deletes this record when it fails to parse, so a concurrent reader catching a half-written file would destroy a live server's only ownership proof. Same-directory rename, so it is genuinely atomic, and readdirSync filters on .json so the temp file is invisible to the lister.
• stopOwnedPreviewBeforeReplacement treats "nothing left to stop" as the goal state, not a failure, and says why — the previous behaviour refused to start any preview until the user hand-deleted the record. That is the right direction for a cleanup path.
• Test-to-source ratio is 486:348, and the added cases name real states rather than mechanics: "keeps a live preview's record when a single probe misses it", "retires the record once the wrapper PID has been recycled", "never leaves a partial session record for a concurrent reader".

blocker — a failed identity lookup retires a live preview's record, which is the one thing this PR exists to prevent

The invariant is stated on the line immediately above the call (:274-275):

wrapperIdentity … is the only PID-reuse guard --stop has, and it never comes back. Only a wrapper process that is provably gone retires a record.

But the check cannot distinguish gone from unanswered (:290-296):

if (!saved.wrapperIdentity) return false;
const identity = dependencies.identity ?? processIdentity;
return identity(saved.pid) === saved.wrapperIdentity;

processIdentity returns null on failure, not only on absence — and on two of three platforms the failure is a subprocess timeout on a live process: win32 shells out to powershell.exe for a Win32_Process CIM query with timeout: 2000, and the non-Linux POSIX branch runs ps -o lstart= with the same 2s budget. Either throws on a slow spawn and is caught into return null. Only Linux reads /proc/<pid>/stat directly and is reliable. A null then compares unequal to a real token, wrapperProcessIsAlive returns false, and :277 deletes the record.

Failure scenario: a Windows or macOS preview is alive but momentarily blocked (the PR's own comment cites a Puppeteer thumbnail capture), so the HTTP probe misses; PowerShell is slow to start under that same load and the identity call times out; the record is deleted. wrapperIdentity is now gone permanently, so the next --stop has no PID-reuse guard — the exact loss the comment says must not happen, reached through a different door than the one this PR closed.

Three things make me call it blocker rather than important:

1. It hits the PR's stated purpose. The title is keep a live preview's ownership record; on 2 of 3 platforms, under the load that causes the missed probe in the first place, it still does not.
2. It is untested in the direction that matters. The added cases pin identity returning a matching token (() => "posix:birth" → kept) and a different one (() => "posix:someone-else" → retired). Nothing makes it return null in the status path, so the suite proves the comparison discriminates same-from-different and never same-from-unknown — which is the distinction wrapperProcessIsAlive's own docstring claims to make.
3. The same ambiguity is already handled the safe way 60 lines down, which shows the two cases were not meant to behave alike. ownedStopTargetPid:356 also compares identity(saved.pid) !== saved.wrapperIdentity, and a null there falls back to liveServerPid — it kills less than it might, and nothing is destroyed. At :296 the same null destroys state that never regenerates.

To be fair on direction: this is an incomplete fix, not a regression. Before this PR the record was deleted on any missed probe, so every case this now keeps is a strict improvement. The fix looks like two lines — have the caller distinguish "no answer" from "different answer" (keep the record when the lookup yields null but the process is still signalable, e.g. kill(pid, 0)), or let processIdentity report failure distinguishably from absence.

note — processIdentity is synchronous, so --list pays for it serially

listBackgroundPreviewStatuses maps records through Promise.all, but processIdentity uses execFileSync. On Windows and macOS that is one blocking subprocess spawn per record with a 2s ceiling each, so preview --list with several stale records blocks the CLI for up to N × 2s — and every one of those spawns is an independent chance to hit the timeout above, so the two findings compound: the more records, the likelier at least one live record is wrongly retired. On Linux this is a cheap /proc read and does not apply.

nit — a failed rename leaves the temp file behind

writePreviewSession:82-84 has no cleanup if renameSync throws — the .tmp is orphaned in the session directory. Harmless to correctness (the .json filter hides it from the lister) but it accumulates, and a try/finally with rmSync(temporary, { force: true }) is one line.

Verdict: REQUEST CHANGES
Reasoning: The ownership model is a solid rework and strictly better than what it replaces, but the keep-alive path it adds still retires a live record when the identity lookup times out on Windows or macOS — the PR's own stated invariant, unpinned by any test. I recognise this blocks a PR that already has an approval at this head; I think a two-line distinction between "no answer" and "different answer" is worth that, and I will convert as soon as it lands.

— Rames Jusso

[miguel-heygen]

Blocker confirmed and fixed at cc93fee03.

I verified it in source before touching anything, because the claim is subtle: processIdentity has a single blanket catch { return null } wrapped around execFileSync calls budgeted at timeout: 2000 — the win32 Win32_Process CIM query and the POSIX ps -o lstart=. So null genuinely means either "no such process" or "the lookup didn't finish", and only the Linux /proc read is exempt. wrapperProcessIsAlive compared that null against a real token, got false, and deleted the record. Your framing is right: reached through a different door than the one this PR closed, and under precisely the load that makes the HTTP probe miss.

The distinction is now explicit, and I took your kill(pid, 0) suggestion:

if (!signalable(saved.pid)) return false;        // gone — no token needed to say so
const identity = processIdentity(saved.pid);
if (identity === null) return true;              // no answer is not a different answer
return identity === saved.wrapperIdentity;       // only a differing token retires it

EPERM counts as alive — the process is there, just not ours to signal.

Two things fell out of putting the cheap check first, which I hadn't planned:

• It answers the --list note. The identity subprocess no longer runs for records whose PID is already gone, which is exactly the stale-record case that made --list pay N × 2 s. Fewer spawns also means fewer chances to hit the timeout that caused the bug, so the compounding you flagged goes away rather than being mitigated.
• The retire path spawns nothing at all now, which the second test asserts directly (expect(identity).not.toHaveBeenCalled()).

On your point 2 — that the suite proved same-vs-different but never same-vs-unknown — both directions are pinned now: identity null + signalable → record kept; identity null + not signalable → retired. Verified by mutation: restoring "no answer means gone" reds the first one.

The temp-file nit is fixed too — try/finally with rmSync, so a failed rename can't orphan it.

Thanks for holding this one despite the approval at that head. The two-line distinction was the right call, and the reordering it forced made --list better as a side effect.

[jrusso1020]

Re-reviewed at cc93fee0. Blocker resolved. All 8 required contexts green at this head, including both windows jobs.

wrapperProcessIsAlive now separates the three states instead of two, and does it in the right order:

if (!signalable(saved.pid)) return false;          // gone — no token needed to say so
const identity = (dependencies.identity ?? processIdentity)(saved.pid);
if (identity === null) return true;                // no answer ≠ different answer
return identity === saved.wrapperIdentity;         // recycled PID

Putting processIsSignalable (kill(pid, 0)) first is better than what I asked for. It makes the cheap, decisive check authoritative for the case that actually matters, and it keeps the subprocess off the path entirely for stale records — which also retires the --list cost note I raised, since the records that made it slow are exactly the ones that now short-circuit before any spawn.

Test coverage closes the gap that made this a blocker rather than a nit. All three states are pinned now, not two: identity: () => null, isSignalable: () => true → "keeps a live preview's record when the identity lookup gives no answer"; isSignalable: () => false → "retires the record when the PID cannot be signalled at all"; and the recycled-token case still retires. A null return can no longer be mistaken for a mismatch without a test going red.

note — the underlying 2s budget is real, and main is currently proving it

Not this PR's to fix, but worth having on the record while it is fresh. main is red on Tests on windows-latest right now, and the failing assertion is orphanCleanup.test.ts:22, the test #3307 added:

const first = processIdentity(process.pid);
expect(first).toMatch(/^(?:linux|posix|windows):/);

It fails with .toMatch() expects to receive a string, but got object — which is null reaching the matcher, since typeof null === "object". So processIdentity returned null for the current, provably alive process, and the case took 2181 ms against that function's 2000 ms PowerShell budget. Main's windows job was green at 3e4b08cd (pre-#3307) and red at b31dde35 (post-#3307), and this PR's own run passed on the same code — so it is a flaky timeout, not a deterministic break.

That is the premise of the finding this PR just fixed, reproducing in CI rather than in argument: the win32 identity lookup does time out on live processes under load. This PR now handles that safely, which is the important half. The remaining half is that the lookup itself is unreliable at 2 s and it is currently breaking a required context on main — raising the budget, or making that test tolerate a null, looks like the next thing rather than anything here.

Verdict: APPROVE
Reasoning: The blocker is fixed in the shape that matters and better than proposed — liveness first, null treated as unknown rather than recycled — with all three states pinned by tests, and every required context is green.

— Rames Jusso