fix: keep a slow disk from killing the gateway on Windows (anchors off-loop, wider liveness probe) - #9172
Conversation
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS Backend/Electron reliability fix with no UI surface — nothing for a first-time user to see, learn, or misread; the one new user-facing string is a well-formed error. Suggestions
[UX-REVIEWED] c7c43b1 |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound completion of the #8022 bound to the anchor half, but the batched rebuild inherits the single-path 2 s budget, making slow-disk refusals ~40× easier to trip. Watch
Suggestions
[DESIGN-REVIEWED] c7c43b1 |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Both halves of the fix trace to a named crash dump or log line, reuse the existing bounded core, and every rider fixes a counted in-scope sibling of the same cause. What this change shipsIntent: stop a slow (not wedged) disk from killing the gateway on Windows — a FIX.
Consumer counts ran clean: [FIRST-PRINCIPLES-REVIEWED] c7c43b1 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've independently verified the load-bearing invariants of this change:
Nothing here yields a groundable defect at the 80+ bar on a changed line. No findings. [OPUS-REVIEWED] c7c43b1 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
9b1690e to
3646af8
Compare
Legitimate: the lexical fallback was charged to |
3646af8 to
a6e32b9
Compare
a6e32b9 to
a58eaad
Compare
Legitimate: the UNC shortcut sat in the shared bounded core, so a UNC home returned |
|
/ai-review override gpt 4ba35fb: same change as 2a88265 minus two review-requested subtractions (drop the unused |
Correct on both counts: no production caller passed it, and a def-time default is exactly the thing a module-level monkeypatch cannot reach. The parameter is removed; the four anchor sites call |
Agreed there must be one implementation; it is kept in |
Author's position, for the record: this is the trade the review itself drove — three narrower fallbacks (lexical anchors, a UNC skip, serving the previous canonical resolution) were each found open, and the reviewer-stated invariant "fresh canonical within budget, else refuse" is what closes them. The cooldown machinery (30 s base, doubling to a 30 min ceiling, reset on the first successful probe) is #8022's, unchanged; this PR routes the anchors through it. On the motivating Windows machine the alternative was a dead gateway and orphaned subagents, so refusing file tools for the cooldown is the intended trade. Question for maintainers: is the shared home-prefix ceiling acceptable as-is, or should anchor stalls use a shorter cap (e.g. 5 min) than a dead mount's 30 min? Either answer is a one-constant follow-up; the PR does not need to wait on it unless you want the shorter cap in this diff. |
Legitimate and worth doing, but it needs a small design pass: the gate's boolean contract is used at many call sites, so carrying the stall reason to the denial means a structured result or a thread-local "last refusal reason". Tracked in #9187 (label |
|
AI-review override not recorded: keep the reason to 500 characters or fewer. |
…f-loop, wider liveness probe) Two watchdogs restarted a gateway twice in one day while a long task ran on a Windows desktop, orphaning six subagents each time. Neither restart caught a wedged loop; both caught a loop that was merely slow under heavy disk I/O. 1. security: the sensitive-target ANCHORS still resolved inline on the event loop. #8022 bounded the agent-supplied candidate, but `_resolved_root_key` (`Path.home().resolve()` + five override roots, once per is_sensitive_path call) and the rebuild in `_home_dir_targets_uncached` (`realpath($HOME)` + ~40 keystone leaves, every 0.1s cache expiry) were untouched. The 04:56 loop-stall dump sat in `_home_dir_targets_uncached -> ntpath.realpath` for the full 25s budget. Both now run on the `mc-pathres` pool through the same bounded core as the candidate (`_run_resolution_bounded`), one job each so a command that drives ~200 rebuilds pays ~400 hops instead of ~11,000. A stall degrades the anchors to their lexical spelling (the fallback the OSError arms always took) instead of refusing every path check for the cooldown; the stall is still recorded so the rebuild does not re-probe the wedged mount every 0.1s. `_lexical_root` replaces `os.path.abspath` in the fallback because abspath strips a trailing space on Windows and the anchor must keep the verbatim spelling. 2. electron: the post-handoff liveness probe used the boot poll's 2s `checkBackend()`. Three 2s misses at the 10s cadence force-kill the gateway, and a loop lagging 1-9s from synchronous disk I/O still answers but not within 2s. `createBackendProbe` gives the monitor its own 8s budget (`LIVENESS_PROBE_TIMEOUT_MS`): a slow loop answers, a wedged loop still trips in ~28s, inside the in-process watchdog's own 25s exit. Tests: bounded-anchor stall/cooldown/one-hop/lexical-fallback cases in test_security_path_resolve_bounded.py; probe budget, resolve/reject paths and a source-shape contract for the supervisor wiring in gateway-liveness.test.js.
4ba35fb to
c7c43b1
Compare
|
/ai-review override gpt c7c43b1: same change as 4ba35fb plus two review-requested follow-ups (spawn-side 60 s deadline on the sandbox preflight; |
Checked, and the ladder does not: |
Same finding as on the previous head; the dispositions stand: the user-facing "stalled, cooling down" reason is tracked in #9187 ( |
Done exactly so. The "undeclared probe error messages" in the headline refers to |
|
AI-review override not recorded: keep the reason to 500 characters or fewer. |
Accurate, and worth a human's call because it is a straight trade between two failure modes. The facts: the rebuild runs on nearly every gate call (main's |
Same item as the previous head's; #9187 ( |
Better wording, agreed. Folded into #9187, which is the pass over every stall-refusal message the operator can see (this one and the gate denials); not re-spun here so the reviewed head stays the one that was verified end to end. |
Problem / Motivation
On a Windows desktop running a long task (a full test run plus several subagents, all under real-time antivirus scanning), the gateway was restarted twice in one day and every running subagent was orphaned each time. Neither restart found a wedged event loop. Both found a loop that was slow because of disk I/O.
_home_dir_targets_uncached -> ntpath.realpathfor the full 25 s watchdog budget. That is the sensitive-path gate resolving its own TARGET anchors ($HOMEand the keystone leaves) inline, on the event loop, insideon_tool_call.backend probe failed (1/3) ... (3/3) ... force-killing wedged gateway and restarting. The gateway log around it shows lag of 1–5 s and every request still being answered. The probe gave it 2 s.Why it matters
A long-running task on Windows cannot survive a busy disk. The gateway exits or is killed, every in-flight turn dies, and the subagents it spawned become orphans (
6 subagent(s) orphaned by a gateway restart). #8022 fixed the same crash for the agent-supplied candidate path, but the anchors it is matched against were still resolved inline, so the same stall killed the gateway from the other half of the check.What changed (motivation → approach → change)
The gate compares a candidate path against a set of target anchors. #8022 moved the candidate's
realpathonto the boundedmc-pathrespool. The anchors still ran inline:_resolved_root_keyresolved$HOMEand five override roots on everyis_sensitive_pathcall, and_home_dir_targets_uncachedresolved$HOMEplus about 40 keystone leaves every time its 0.1 s cache expired.realpathon Windows opens the directory, so under disk pressure each call can block for seconds.Both anchor paths now run through the same bounded core as the candidate (
_run_resolution_bounded, 2 s budget, per-prefix cooldown), and under the same rule: the gate only compares against anchors resolved fresh, canonically, within the budget. A stall, an open cooldown, or a pinned or faulted pool raisesPathResolutionStalled, and every gate refuses — the_home_dir_targetscall sites sit inside the sameexceptarms as their candidate half. Each anchor path is one pool job, not one per path:_resolve_root_anchorsresolves all six roots in one hop, and_rebuild_targets_boundedruns the whole rebuild in one hop, so the cost stays flat (one bash command can drive about 200 rebuilds; measured on Windows and Linux the gate is unchanged within noise). Review walked this PR through three weaker fallbacks before it settled on refusing, and each is now pinned shut by a test: lexical spellings lose the canonical target of a symlinkedKIROCREW_HOMEon another mount; the Windows UNC shortcut did the same through a junction inside a UNC home, so that shortcut now applies to candidate tokens only and a UNC home is probed; and serving the previous canonical resolution through a stall misses a symlink repointed during the stall. The one lexical spelling left is pre-existing — an override root whose ownrealpathraises keeps its verbatim form, via_lexical_rootrather thanabspathbecauseabspathstrips a trailing space on Windows (test_env_root_override_is_read_verbatim).sandbox_credential_targets, the OS sandbox deny mask, already runs off the loop in the spawn preflight, so it resolves the roots inline through_resolve_root_anchorsand never sees a stall; review then asked what bounds that wait, and the answer was nothing on the spawn path (the handshake timeout comes after the spawn), so the preflight now runs under its own 60 s deadline (_run_preflight_bounded, sized for a cold sandbox probe plus root resolution) and on expiry raises a retryableAcpErrornaming the slow disk — the enforced adapter is refused, never started with its mask missing.The desktop liveness monitor polled
/api/statuswith the boot poll'scheckBackend(), a 2 s timeout. Three misses at the 10 s cadence force-kill the gateway. A loop lagging 1–9 s answers, just not within 2 s.createBackendProbe(ingateway-liveness.js, where the monitor lives and is unit-tested) is now the one request implementation: the liveness monitor builds its probe from it withLIVENESS_PROBE_TIMEOUT_MS = 8000, and the boot poll'scheckBackend()delegates to the same factory with its ownBOOT_PROBE_TIMEOUT_MS = 2000, so the two cannot drift. A slow loop answers within 8 s; a wedged loop answers nothing and still trips in about 28 s, inside the in-process watchdog's own 25 s exit. The boot poll keeps 2 s because a slow answer there only means "poll again".Tests
test/test_security_path_resolve_bounded.pytest_a_stalled_root_anchor_refuses_within_the_budget: withrealpathwedged,_resolved_root_keyraises within the budget even though a canonical resolution was made a moment earlier,is_sensitive_path/path_contains_sensitiverefuse, and the stall is recorded under the home's prefix.test_a_stalled_anchor_is_not_reprobed_until_the_cooldown_lapses: inside the cooldown nothing is submitted; past it the home is probed again.test_root_anchors_resolve_in_one_pool_hop/test_the_rebuild_is_one_pool_job: roots and rebuild are one submission each.test_a_stalled_rebuild_refuses_even_with_a_warm_cache: an expired cache slot is never handed back through a stall.test_a_repointed_override_root_is_never_served_stale_through_a_stall: the round-three scenario —KIROCREW_HOMEsymlinked at A, anchors warmed, link repointed at B under a stall — the canonical Bsecurity_policy.jsonis refused, and anchored canonically once the disk answers (real symlink; skipped where the host cannot create one).test_a_unc_home_still_has_its_anchors_resolved: the round-two scenario — a UNC home on Windows is probed for its canonical anchors while a UNC candidate token keeps the lexical shortcut.test_sandbox_mask_resolves_inline_and_never_sees_a_stall: with the home's prefix in cooldown the gate refuses butsandbox_credential_targetsstill returns the canonical mask.test/test_acp_client_more_coverage.pytest_sandbox_preflight_is_bounded_on_a_stalled_disk: a preflight that never returns raisesAcpErrorat the deadline instead of holding the spawn open;test_sandbox_preflight_within_budget_returns_the_maskpins the fast path.website/electron/test/gateway-liveness.test.js{ timeout };startLivenessMonitorpolls withcreateBackendProbe, notcheckBackend().Existing coverage that pins the surrounding contract still passes:
TestHomeDirTargetsCache(cache equivalence, immediate re-key on a repointed$HOMEsymlink, one resolution per fill), the #8022 stall suite including the real-symlink cases on Linux,test_env_root_override_is_read_verbatim, and the Electron supervisor / stop / recovery / shell-contract suites.Manual verification
Root cause taken from the two restarts on the affected machine: the loop-stall crash dump's main-thread frame (
_home_dir_targets_uncached -> ntpath.realpath,Timeout (0:00:25)!) and the desktop launcher log (event-loop heartbeat: lag 1.2s ... 5.0sinterleaved withbackend probe failed (1/3)..(3/3)). Behaviour under a real stall is covered by the wedged-realpathtests above; there is no reproducible way to make a healthy disk stall on demand.Related Issues
Related: #6590 (Windows cannot recover unattended after a loop-stall exit), #8022 (bounded the candidate half of this gate).
no linked issue: the anchor-side stall and the 2 s liveness probe had no issue on file; this PR is the first report of both.
Pattern harvest
Rule candidate: review-prompt
Pattern: "a bounded/off-loop fix for one half of a check leaves the other half (the thing it is compared against) inline on the event loop"
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)