fix(crash-dumps): take the PID-reuse identity from the persisted-identity routine

[leonlaiyc]

Problem / Motivation

crash_dump_store._pid_start_id supplies the start identity that decides whether a crash dump's recorded owner is still the process holding its fd. Two readers act on that answer destructively:

• sweep_stale_dumps treats a mismatch as "owner is gone" and unlinks the dump — while the owning gateway is alive and faulthandler still holds that file's fd, so its future stall evidence goes to an unreachable inode. There is no recovery path.
• cron_inflight.RunningMarker.owner_alive reads the same identity to conclude a run was abandoned, and the breaker then parks the job.

#8282 (merged after this PR was opened) gave the function a fallback to platform_compat.process_start_time so it could answer off Linux. That routine has two legs, and only one of them is suitable here:

• Its Windows leg is the process creation FILETIME read through a query-only handle — a machine integer at 100-ns resolution, no locale or timezone in it. Sound.
• Its remaining POSIX leg is ps -o lstart=: 1-second, locale- and TZ-rendered. Its own docstring calls it safe precisely because "a format or resolution drift can only make the guard decline to act, never act on the wrong process."

That sentence states a kill-guard contract, where a mismatch means do nothing. Both readers above do the opposite — they act on a mismatch — so the same drift deletes a live gateway's dump rather than declining. Two gateways sharing one data home (an overlapping restart, an isolated pod — cases sweep_stale_dumps documents) need only differ in TZ or LC_TIME to render the same start instant as two different strings. The 1-second granularity is the milder half: it cannot separate two processes that started in the same second.

Why it matters

The dump this can delete is the only artifact of a loop-stall wedge, and the failure is silent and self-concealing: the file disappears while the gateway that will need it is still running, so the loss is only discovered when someone goes looking for evidence that no longer exists.

It also lands on #8282's own new feature. That PR built the cron circuit breaker to answer "which job was in flight when the gateway died" from exactly this identity — so a drifted render both destroys the dump the breaker joins against and can mark a live run abandoned.

What changed (motivation → approach → change)

Root cause is the direction of the fail-safe, not the probe. A drift-tolerant helper is safe only where a mismatch means decline to act; routed into a caller that unlinks, the same tolerance becomes a deletion.

1. The source. The identity now comes from platform_compat.get_process_start_id — the routine this repository already uses everywhere a start identity is written down and compared back later (mcp_gateway.claim, session_pid, metrics.sessions). It is in-process on every platform it covers, and on macOS it reads libproc's microsecond start instant, so macOS keeps the coverage #8282 gave it, at a resolution that also separates two processes started in the same second. Where that routine declines and the host is Windows, #8282's process_start_time fallback is kept exactly as it is, sanitiser included.

Both legs still go through platform_compat; neither the procfs read nor the ps invocation is re-implemented locally, so the shim-table row in AGENTS.md ("Process start time (PID-reuse guard)", whose NOT column names raw /proc/<pid>/stat and raw ps -o lstart=) is satisfied either way. Its stated concern — that both raw probes answer None on Windows — is exactly what the retained Windows leg covers.

Platform	On origin/main (post-#8282)	Here
Linux	procfs field 22	procfs field 22 — byte-identical
macOS	ps -o lstart=, 1 s, locale/TZ	libproc, microsecond, machine
Windows	creation FILETIME	creation FILETIME — unchanged
other POSIX	ps -o lstart=	None → plain PID liveness

Because Linux is byte-identical, Linux headers written before this change still compare equal. A None is "identity unknown", never a mismatch, so the last row degrades to plain PID liveness, which only ever protects a file.

2. The migration guard. Changing the representation is itself a hazard, and it is guarded rather than ignored. A macOS gateway that wrote its header under the previous build recorded a ps render; it can never equal what is read now, and comparing the two as though they were one kind of identity would unlink the dump of a gateway still running across the upgrade — the overlapping restart sweep_stale_dumps documents a live PID as protecting.

_start_ids_comparable therefore treats a value that is not in the current representation as unknown rather than different, and both destructive readers consult it — _owner_alive and pid_identity_alive. Patching only the sweep would leave the cron marker path open.

There is a third comparison site, RunningMarker.same_process, and it deliberately gets no guard: it joins a marker to a dump, so its mismatch answer is "not the same process" and the breaker then attributes nothing and pauses nothing. That is already the declining direction.

Two deliberate choices in that predicate:

• It is an allowlist of what this build writes (digits, optionally one . — a Linux jiffy count, a Windows FILETIME, or macOS's <seconds>.<microseconds>), not a denylist of the retired render. The ps text depends on the writer's locale and TZ, so no property of it — not even the HH:MM:SS colon — is reliable across locales.
• A legacy token is never converted into the new representation. The timezone and locale it was rendered under are not recoverable, and guessing them would re-introduce the misjudgement this exists to prevent.

Everything else #8282 added to this module is untouched: the bounded _read_dump_bytes / _read_dump_lines readers, and dump_owner_identity / current_process_identity / pid_identity_alive / dump_wedged_frames.

Residual, stated plainly. The guard fixes the direction this build controls: a new gateway reading an old header. The reverse — a gateway still running the previous build reading a header this build wrote — cannot be fixed from here, because that code is already deployed and compares with its own ps render. It is bounded to the macOS upgrade overlap window and is inherent to any identity-format migration; against it stands a permanent removal of the locale/TZ misjudgement on every subsequent restart.

Tests

test/test_crash_dump_store.py. The two platform routines are stubbed apart so each assertion names a source rather than merely observing a value, and the stub also pins IS_WINDOWS so every case asserts the same contract on every CI host. The stub patches the source module and any name the module bound directly, so the tests pin behaviour rather than import style — re-adding a from platform_compat import process_start_time and calling it stays visible.

Red on origin/main at this PR's base (control = main's crash_dump_store.py in-tree with these tests):

• test_start_id_comes_from_the_persisted_identity_routine — assert 'Wed_Sep_3_10:00:00_2026' == 'stable-id'
• test_posix_start_id_is_none_rather_than_the_ps_render — assert 'Wed_Sep_3_10:00:00_2026' is None
• test_header_records_start_id_on_a_non_procfs_platform — the header carries no token
• test_pid_reuse_detected_without_procfs — assert True is False
• test_rotation_reclaims_recycled_pid_dumps_without_procfs — assert 0 == 2

Red on the pre-guard head 3aa7f839a, for the migration mechanism itself (not merely because a helper is absent):

• test_legacy_ps_header_does_not_sweep_a_live_gateways_dump — assert False is True; the sweep unlinks a live gateway's dump.
• test_legacy_ps_header_is_not_a_rotation_victim — assert 2 == 0; rotation's never-a-victim rule stops protecting it, so two more live-owner dumps are sacrificed. This consequence is beyond the reported one.
• test_legacy_cron_marker_is_not_reported_abandoned — assert False is True; a still-executing run reports abandoned.
• test_legacy_cron_marker_falls_back_to_liveness_not_the_own_pid_shortcut — assert False is True; the same on another gateway's PID, so the green answer comes from the liveness fallback rather than pid_identity_alive's own-PID shortcut.

Two further tests (test_windows_filetime_is_a_comparable_current_identity, test_comparability_is_an_allowlist_of_the_current_representation) are direct unit tests of the new predicate and fail on that head with AttributeError — helper-absence, listed separately because that is not mechanism evidence.

Green on both sides, pinning what is preserved rather than changed:

• test_windows_keeps_the_creation_filetime_fallback and test_windows_fallback_value_stays_one_header_token — the Windows leg and its single-token sanitiser.
• test_current_format_mismatch_still_detects_pid_reuse and test_current_format_mismatch_still_detects_a_recycled_cron_marker — the guard does not blunt the check it protects; two current-format values that differ are still proof of reuse, on both readers.
• test_legacy_token_does_not_resurrect_a_dead_owner — the guard turns different into unknown, never confirmed-dead into alive; a dead PID's dump is still swept.

Three pre-existing tests used placeholder identity strings (fabricated-mismatch, recycled-token) that no real producer emits; their fixtures now carry values in the real representation, leaving each test's subject unchanged.

Local runs (Windows, repo-pinned toolchain): test_crash_dump_store.py 66 passed / 1 skipped; test_stall_attribution.py + test_subagent_stall_attribution.py 53 passed / 3 skipped. scripts/check_black_formatting.py and scripts/check_subprocess_encoding.py both pass in scope; flake8 clean; mypy reports nothing in this file.

Manual verification

N/A — unit coverage sufficient: the change is a source swap plus a comparability predicate inside one module, and every divergence it turns on is between two platform_compat routines that the tests stub apart deterministically. The real macOS libproc and Windows FILETIME legs are platform_compat's own, already covered there.

Related Issues

None — found by source review while confirming the PID-reuse guard off Linux.

Interacts with #8282 (merged): this PR is not a revert of it. #8282's Windows leg and all four of its new module APIs are preserved; only its POSIX ps leg is replaced, by a routine that gives macOS strictly better resolution than the one it drops.

Pattern harvest

Rule candidate: review-prompt
Pattern: a drift-tolerant identity/liveness helper is safe only where a mismatch means decline to act; routing one into a caller that deletes, kills, or otherwise acts on a mismatch inverts its documented fail-safe direction. When a helper's docstring justifies its coarseness with "drift can only make the guard decline", that sentence is a contract on the caller, and reusing the helper elsewhere must re-check it. Corollary, and the second half of this PR: changing the representation of a persisted identity is a migration — a recorded value in the retired representation must read as unknown, never as a mismatch, on every reader that acts destructively.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

🤖 Generated with Claude Code

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed f6172d616184c1374cde2ffb8334685605b872a5 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] f6172d6

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed f6172d616184c1374cde2ffb8334685605b872a5 via the fork AI-review pipeline; updated in place on each push.

Review details

I've analyzed the single candidate and the full diff. Let me verify the key falsification point about the pre-change vs. post-change robustness and the documented contract.

The candidate claims a regression: the diff replaces a byte-based /proc/<pid>/stat reader (which decoded only the all-digit field 22 as ASCII) with a call to platform_compat.get_process_start_id, which uses read_text() (locale decode). For a Linux process whose comm field carries non-UTF-8 bytes, read_text() raises, gets caught, and returns None, whereas the old reader would still have extracted the numeric start-id.

Assessing this against the bar:

• (a) input is plausible but exotic (recycled PID now owned by a process with an invalid-UTF-8 comm, under a UTF-8/ASCII locale).
• (b) call path is real: _pid_start_id → get_process_start_id → read_text() → raises → None → not IS_WINDOWS → returns None.
• (c) The claimed outcome is: reuse check skipped → _owner_alive returns True → stale header-only dump not swept.

That third leg is exactly where it dies. Returning None on an unreadable/undecodable identity is the documented, intended contract ("Returns None where the identity is unknown … callers fall back to plain PID liveness — conservative"). The result is fail-safe: no wrong deletion, no data loss, no crash, no security consequence; and rotation's cap still reclaims the file. The pre-change reader was marginally more robust at extracting the id despite a weird comm, but the new behavior lands squarely inside the fail-safe envelope the module defines as correct. That is a narrowing of a best-effort optimization for a rare input, not an observable wrong outcome. It does not clear 80. The candidate itself rates confidence "low."

No new grounded defect surfaced in Step 2: the _start_ids_comparable allowlist correctly requires both sides to be digits-only, legacy ps renders always carry month names/colons so are never miscompared, current-format values (Linux jiffies, macOS sec.usec, Windows FILETIME) all match and preserve reuse detection, and the guard only ever narrows toward the conservative "protect the file" direction.

No findings.

[OPUS-REVIEWED] f6172d6

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of f6172d616184c1374cde2ffb8334685605b872a5 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified against the base tree. Writing the review.

First-Principles-Verdict: CONCERNS

The fix is aimed at the real cause and every piece earns its place, but the same destroy-on-mismatch ps render survives uncorrected in warm.py.

What this change ships

Intent: stop the dump sweeper and cron breaker from destroying a live gateway's evidence when the macOS start-identity render drifts — a FIX.

1. macOS start identity is now libproc microseconds, not the locale/TZ ps render — justified
2. Other-POSIX hosts lose PID-reuse detection; plain liveness protects the file — declared, justified
3. Windows keeps the FILETIME fallback — justified
4. Linux identity byte-identical — justified
5. Legacy-format headers/markers read as "unknown", never swept or declared abandoned while the PID lives — justified, 2 consumers (_owner_alive, pid_identity_alive)
6. A legacy header naming a live-but-recycled PID is kept until that PID dies — inherent cost of item 5, declared
7. Tests pinning the source split and the migration guard — justified

Watch

• One counted unfixed sibling of the named root cause. Grepped every process_start_time persist-and-compare site: apps/backend.py, ssh_tunnel_manager.py, instances/registry.py are kill/reclaim guards (mismatch declines — safe), but connections/warm.py persists the ps render (_warm_generation_owner, warm.py:780,785) and _process_identity_live (warm.py:996) treats mismatch as dead, letting _scavenge_warm_generation_dirs delete a directory its own docstring calls "still in use". Same drift, same destructive direction, out of this PR's scope — accepted-and-deferred, but it should be named.
• The retained Windows chain is mechanism-level. The reachable cause is get_process_start_id (platform_compat.py:1194) lacking the FILETIME leg process_start_time already implements; folding it in would delete this fallback and the identical chain at run_marker.pid_start_token (run_marker.py:140-152) — 2 chaining sites counted.

[FIRST-PRINCIPLES-REVIEWED] f6172d6

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of f6172d616184c1374cde2ffb8334685605b872a5 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All the description's claims check out against the base tree: get_process_start_id exists with exactly the stated semantics (in-process, procfs field 22 on Linux so existing Linux headers stay byte-identical, libproc microseconds on macOS, None on Windows with an explicit "must not treat as mismatch" contract), process_start_time's docstring states the quoted kill-guard contract, the base _pid_start_id really does fall through to the ps render, and the third comparison site same_process (cron_inflight.py:153, joined at stall_attribution.py:170) is indeed the declining direction, so leaving it unguarded is right. The migration guard covers both destructive readers, keeps genuine current-format mismatches detected, and refuses to resurrect a confirmed-dead owner. This is a root-cause fix (fail-safe direction at the acting callers), and the one residual — an old-build reader during the macOS upgrade overlap — is honestly bounded and unfixable from this side.

The only design fragility I found: _CURRENT_START_ID_RE encodes the output shape of platform_compat.get_process_start_id in a distant module with no shared contract; a future platform leg emitting a new shape would silently classify fresh identities as legacy and degrade reuse-detection to plain liveness (conservative direction, so bounded harm). That's a suggestion, not a blocker.

Design-Verdict: PASS

Root-cause fix with the migration hazard it creates explicitly guarded on both destructive readers; every description claim is backed by verified code.

Suggestions

• The _CURRENT_START_ID_RE allowlist duplicates get_process_start_id's output shape without a shared contract; document/pin that shape in platform_compat (a constant or co-located test) so a future platform leg can't silently degrade this module's PID-reuse detection to plain liveness.

[DESIGN-REVIEWED] f6172d6