Skip to content

fix: keep a slow disk from killing the gateway on Windows (anchors off-loop, wider liveness probe) - #9172

Merged
bolichen97 merged 1 commit into
mainfrom
fix/windows-loop-stall-liveness
Sep 7, 2026
Merged

fix: keep a slow disk from killing the gateway on Windows (anchors off-loop, wider liveness probe)#9172
bolichen97 merged 1 commit into
mainfrom
fix/windows-loop-stall-liveness

Conversation

@bolichen97

@bolichen97 bolichen97 commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

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.

  1. The 04:56 loop-stall crash dump had the main thread in _home_dir_targets_uncached -> ntpath.realpath for the full 25 s watchdog budget. That is the sensitive-path gate resolving its own TARGET anchors ($HOME and the keystone leaves) inline, on the event loop, inside on_tool_call.
  2. The 13:16 restart came from the desktop app's liveness monitor: 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 realpath onto the bounded mc-pathres pool. The anchors still ran inline: _resolved_root_key resolved $HOME and five override roots on every is_sensitive_path call, and _home_dir_targets_uncached resolved $HOME plus about 40 keystone leaves every time its 0.1 s cache expired. realpath on 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 raises PathResolutionStalled, and every gate refuses — the _home_dir_targets call sites sit inside the same except arms as their candidate half. Each anchor path is one pool job, not one per path: _resolve_root_anchors resolves all six roots in one hop, and _rebuild_targets_bounded runs 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 symlinked KIROCREW_HOME on 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 own realpath raises keeps its verbatim form, via _lexical_root rather than abspath because abspath strips 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_anchors and 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 retryable AcpError naming the slow disk — the enforced adapter is refused, never started with its mask missing.

The desktop liveness monitor polled /api/status with the boot poll's checkBackend(), 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 (in gateway-liveness.js, where the monitor lives and is unit-tested) is now the one request implementation: the liveness monitor builds its probe from it with LIVENESS_PROBE_TIMEOUT_MS = 8000, and the boot poll's checkBackend() delegates to the same factory with its own BOOT_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.py
    • test_a_stalled_root_anchor_refuses_within_the_budget: with realpath wedged, _resolved_root_key raises within the budget even though a canonical resolution was made a moment earlier, is_sensitive_path / path_contains_sensitive refuse, 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_HOME symlinked at A, anchors warmed, link repointed at B under a stall — the canonical B security_policy.json is 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 but sandbox_credential_targets still returns the canonical mask.
  • test/test_acp_client_more_coverage.py
    • test_sandbox_preflight_is_bounded_on_a_stalled_disk: a preflight that never returns raises AcpError at the deadline instead of holding the spawn open; test_sandbox_preflight_within_budget_returns_the_mask pins the fast path.
  • website/electron/test/gateway-liveness.test.js
    • the probe budget is wider than the 2 s boot poll and no longer than the 10 s poll interval, and reaches the request as { timeout };
    • the probe resolves on any non-5xx (including 401) and drains the body; rejects on 5xx, connection error, and timeout, destroying the timed-out request;
    • a source-shape contract that startLivenessMonitor polls with createBackendProbe, not checkBackend().

Existing coverage that pins the surrounding contract still passes: TestHomeDirTargetsCache (cache equivalence, immediate re-key on a repointed $HOME symlink, 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.0s interleaved with backend probe failed (1/3)..(3/3)). Behaviour under a real stall is covered by the wedged-realpath tests 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

  • 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

@bolichen97
bolichen97 requested a review from a team September 7, 2026 01:10
@bolichen97
bolichen97 requested a review from a team as a code owner September 7, 2026 01:10
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of c7c43b1663cf572318c981542e9b59ea215820c3 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • _run_preflight_bounded's AcpError leads with mechanism ("computing its sandbox credential mask needs the home and credential roots resolved on disk"); keep the clear diagnosis and action but say it in task terms, e.g. "Could not start the {backend} adapter: the disk did not respond within 60 s while its security paths were being checked. Nothing was started; retry once the disk responds."

[UX-REVIEWED] c7c43b1

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of c7c43b1663cf572318c981542e9b59ea215820c3 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • _rebuild_targets_bounded runs ~40 realpath calls as ONE pool job under the same _PATH_RESOLVE_TIMEOUT_SECS (2 s) sized for one candidate path. On a disk that is slow but alive (the PR's own 13:16 evidence: 1–5 s lag, every request answered), ~50–60 ms per leaf blows the budget while individual candidate resolutions still pass → PathResolutionStalled under the $HOME prefix → every gate refuses every path (not just home-relative ones, since _resolved_root_key raises before any comparison) for a cooldown that doubles to 30 min. The fix converts "gateway dies" into "all file tools denied", which is the right direction, but the threshold for entering that state is now set by batch size × per-leaf latency, not by an actual wedge. Consider a budget proportionate to the job (e.g. rebuild gets N× the single-path budget), or measure the rebuild's wall time under the AV-pressure profile before shipping.

Suggestions

  • Surface the anchor-stall refusal distinctly to the operator (log line or status flag naming the cooldown expiry); a 30-minute window in which every path check refuses will otherwise present as inexplicable blanket denials.

[DESIGN-REVIEWED] c7c43b1

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of c7c43b1663cf572318c981542e9b59ea215820c3 — 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.

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 ships

Intent: stop a slow (not wedged) disk from killing the gateway on Windows — a FIX.

  1. A disk stall in the gate's anchor half no longer crashes the gateway; anchors resolve on the existing mc-pathres pool — justified (crash dump names the frame; extends fix(security): bound sensitive-path symlink resolution off the event loop #8022's mechanism instead of a second one)
  2. During a stall or cooldown every sensitive-path gate refuses instead of matching stale/lexical anchors — justified (governance-ceiling boundary; three weaker fallbacks shown open, each pinned by a test)
  3. Roots and rebuild are one pool job each, so gate cost stays flat — justified (measured: ~200 rebuilds per bash command)
  4. Adapter spawn gives up after 60 s on a stalled disk instead of hanging to the startup watchdog — declared; fixes the one in-scope sibling of the same unbounded-disk-wait cause (grep os.path.realpath|.resolve() in security.py: no other inline gate-path site remains)
  5. An override root whose realpath raises keeps its verbatim spelling on Windows — declared, pinned by test
  6. Liveness probe waits 8 s (was 2 s) before a miss, so a slow-but-answering loop is not force-killed — justified (launcher log quoted; true wedge still trips inside the 25 s watchdog)
  7. Boot poll and liveness probe share one request implementation, boot keeps 2 s — justified consolidation (was two copies; the only behavior delta, Error-carrying rejections, has 0 reason-reading consumers across the 5 checkBackend call sites)
  8. security.md updated in the same commit — mandated by AGENTS.md

Consumer counts ran clean: _run_resolution_bounded 3, _resolve_root_anchors 2, createBackendProbe 2; no zero-consumer surface.

[FIRST-PRINCIPLES-REVIEWED] c7c43b1

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of c7c43b1663cf572318c981542e9b59ea215820c3 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c7c43b1

False positive or not applicable? A repository writer can comment:
/ai-review override gpt c7c43b1663cf572318c981542e9b59ea215820c3: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed c7c43b1663cf572318c981542e9b59ea215820c3 — this comment is updated in place on each push.

Review details

I've independently verified the load-bearing invariants of this change:

  • All three gate functions (_path_in_home_dirs, _is_keystone_publish_artifact, path_contains_sensitive) place the now-raising _home_dir_targets(...) call inside a try that returns True (fail-closed) on PathResolutionStalled; is_sensitive_path/is_sensitive_write_path delegate to those and never let the exception escape.
  • The only callers of _home_dir_targets/_resolved_root_key are those three gates plus the cached path — no unwrapped event-loop caller now propagates PathResolutionStalled.
  • No nested pool submission: _rebuild_targets_bounded's worker calls _home_dir_targets_uncached(home_dirs, roots) with roots supplied, so line 10094 skips _resolved_root_key() and never re-enters the pool → no deadlock on the 2-worker pool.
  • sandbox_credential_targets deliberately calls _resolve_root_anchors inline (never raises the stall the sandbox ladder can't decode); its blocking wait is the same as the pre-PR _resolved_root_key() inline realpath, now additionally bounded by _run_preflight_bounded (60 s) off the loop → strict improvement, no regression.
  • Electron createBackendProbe reproduces the old checkBackend request shape (res.resume(), resolve <500), boot poll keeps 2 s, liveness widens to 8 s; the changed rejection value is inert to the boot poll's retry loop.

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 c7c43b1663cf572318c981542e9b59ea215820c3.

False positive or not applicable? A repository writer can comment:
/ai-review override fable c7c43b1663cf572318c981542e9b59ea215820c3: <one-sentence reason>

@bolichen97
bolichen97 force-pushed the fix/windows-loop-stall-liveness branch from 9b1690e to 3646af8 Compare September 7, 2026 01:42
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 7, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Batched timeout drops canonical sensitive-path anchors span=3c5b15a2d41e — fixed in 3646af8

Transient stall resolving a symlinked KIROCREW_HOME -> lexical fallback omits its canonical target -> a file-tool request naming canonical security_policy.json passes the gate.

Legitimate: the lexical fallback was charged to $HOME's prefix, so a symlinked override root on a different mount lost its canonical target while its own prefix stayed healthy. Fixed by never degrading an anchor to lexical spellings. A stalled root resolution now serves the last canonical resolution for the same inputs (_last_resolved_roots, keyed on Path.home() plus every override variable verbatim), a stalled rebuild serves the canonical set its cache slot held before it expired, and with nothing to serve the stall propagates and every gate refuses — the five _home_dir_targets call sites are inside the same except PathResolutionStalled arms as their candidate half. The lexical set remains only for a resolution that was never attempted (UNC home on Windows) or a pool fault, which is what the pre-existing OSError arms did. Regression test for this exact scenario: test_symlinked_override_root_keeps_its_canonical_target_through_a_stall (symlinked KIROCREW_HOME, anchors stalled, candidate resolving on its own healthy prefix — canonical security_policy.json is denied), plus test_a_stalled_root_anchor_with_nothing_to_serve_fails_closed and test_a_stalled_rebuild_with_nothing_to_serve_fails_closed.

@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 7, 2026
@bolichen97
bolichen97 force-pushed the fix/windows-loop-stall-liveness branch from 3646af8 to a6e32b9 Compare September 7, 2026 02:01
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 7, 2026
@bolichen97
bolichen97 force-pushed the fix/windows-loop-stall-liveness branch from a6e32b9 to a58eaad Compare September 7, 2026 02:23
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 7, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • UNC homes skip resolution of override anchors span=3c5b15a2d41e — fixed in a58eaad

UNC home + junction-backed KIROCREW_HOME + canonical file-tool path -> lexical targets miss the governance file -> sensitive access is permitted.

Legitimate: the UNC shortcut sat in the shared bounded core, so a UNC home returned None for the anchors and the root key / rebuild took their lexical arm. Fixed exactly as suggested: the shortcut now lives only in _resolved_forms_bounded (candidate tokens), the anchors are always probed on the bounded pool, and the lexical arm is gone entirely — a pool fault is handled like a stall (serve the last canonical resolution for the same inputs / the previous canonical target set, else refuse). _no_resolve and _lexical_env_root are removed. Regression test: test_a_unc_home_still_has_its_anchors_resolved (UNC Path.home() under _ON_WINDOWS, the home is probed and its canonical spelling anchors the roots, while a UNC candidate token still takes the lexical shortcut).

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 7, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

/ai-review override gpt 4ba35fb: same change as 2a88265 minus two review-requested subtractions (drop the unused resolve parameter; checkBackend now delegates to createBackendProbe), no semantic change to the gate; pre-existing residual, strictly narrower than base — the sub-timeout accumulation is main's own inline-realpath cost (47·t unbounded per call) which this PR caps at ≤4 s per call and otherwise leaves equal, so no environment that survives main crashes here; the per-tool-call anchor sharing it asks for is a further improvement on main's 0.1 s TTL design, out of this fix's scope.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • New resolve parameter on _home_dir_targets_uncached — undeclared, zero consumersfixed in 4ba35fb

0 callers pass it ... every new test monkeypatches security._realpath_or_none at module level — which a def-time default never sees.

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 _realpath_or_none directly (module lookup at call time), which is what the tests already relied on.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • createBackendProbe repeats checkBackend's body line-for-linefixed in 4ba35fb

The only capability difference is the budget, which a timeoutMs parameter on checkBackend delivers without a new exported function and constant.

Agreed there must be one implementation; it is kept in gateway-liveness.js rather than the supervisor because that is where the monitor lives and is unit-tested without Electron (the probe's resolve/reject/timeout paths and the budget are pinned there), and checkBackend(healthUrl, timeoutMs = BOOT_PROBE_TIMEOUT_MS) now delegates to that same factory. Net: one request body, two named budgets, no second spelling to maintain.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • An anchor stall now closes the whole gate — every path refused, with cooldowns escalating to 30 minutesneeds-a-decision (maintainers)

Deliberate and safer than the pre-PR crash, but a human should confirm the home-prefix cooldown ceiling is the intended availability trade.

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.

@bolichen97

Copy link
Copy Markdown
Collaborator Author

Give the stall refusal its own user-facing reason so an operator staring at 30 minutes of denials learns it's a cooldown, not policy.

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 deferred-finding, assigned, due 2026-10-06). Pre-existing shape for candidate stalls since #8022; this PR makes it more visible, which is why it is filed now.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

AI-review override not recorded: keep the reason to 500 characters or fewer.

@bolichen97
bolichen97 enabled auto-merge (squash) September 7, 2026 04:12
@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 7, 2026
…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.
@bolichen97
bolichen97 force-pushed the fix/windows-loop-stall-liveness branch from 4ba35fb to c7c43b1 Compare September 7, 2026 04:38
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Sep 7, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

/ai-review override gpt c7c43b1: same change as 4ba35fb plus two review-requested follow-ups (spawn-side 60 s deadline on the sandbox preflight; checkBackend loses its never-passed timeoutMs), gate semantics unchanged; pre-existing residual, strictly narrower than base — the sub-timeout accumulation is main's own inline-realpath cost (47·t unbounded per call) which this PR caps at ≤4 s per call and otherwise leaves equal, so no environment that survives main crashes here; the per-tool-call anchor sharing it asks for is a further improvement on main's 0.1 s TTL design, out of this fix's scope.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • sandbox_credential_targets "simply waits" unbounded inside _sandbox_preflight's to_thread; confirm the transport ladder times that spawn outfixed in c7c43b1

confirm the transport ladder it defers to actually times that spawn out, since this PR is the first thing to send it there.

Checked, and the ladder does not: ensure_ready bounds the ACP handshake (_INIT_TIMEOUT), which comes after _spawn, and _spawn awaited the preflight bare, so the only backstop was the subagent startup watchdog (120 s) or the turn deadline. The claim in the code comment was wrong and is corrected. The preflight now runs under _run_preflight_bounded (60 s: a cold sandbox probe already has a 20 s subprocess budget, plus root resolution on a slow disk, with headroom). Expiry raises the retryable AcpError with a message naming the slow disk; the enforced adapter is refused, never started with its mask missing. Pinned by test_sandbox_preflight_is_bounded_on_a_stalled_disk (a preflight that never returns raises at the deadline) and test_sandbox_preflight_within_budget_returns_the_mask; the H13 ratchet in test_acp_tool_gate still sees exactly one _sandbox_preflight call site per enforced harness. Spec updated.

@bolichen97

Copy link
Copy Markdown
Collaborator Author

make the stall-cooldown refusal distinguishable in user-facing failure text (and consider a lower cooldown ceiling for the anchor side)

Same finding as on the previous head; the dispositions stand: the user-facing "stalled, cooling down" reason is tracked in #9187 (deferred-finding, assigned, dated), and the ceiling is the open question to maintainers — the cooldown machinery is #8022's unchanged, anchors now route through it, and a shorter anchor-side cap is a one-constant follow-up if you want it. Author's recommendation is to merge as-is and decide the cap in #9187's design pass, since the alternative on the motivating machine was a dead gateway.

@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • Drop timeoutMs from checkBackend's signature — 5 call sites, none passes itfixed in c7c43b1

pass BOOT_PROBE_TIMEOUT_MS straight to createBackendProbe inside.

Done exactly so. The "undeclared probe error messages" in the headline refers to createBackendProbe rejecting with an Error where checkBackend rejected with undefined; every caller of both treats rejection as a boolean miss and none reads the message, so no behaviour changes — the message exists for the liveness log line only.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

AI-review override not recorded: keep the reason to 500 characters or fewer.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 7, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author
  • The batched rebuild (~40 realpaths as one job) inherits the single-path 2 s budget, so a slow-but-alive disk trips the refusal ~40× more easily than a candidate doesneeds-a-decision (maintainers)

Consider a budget proportionate to the job (e.g. rebuild gets N× the single-path budget), or measure the rebuild's wall time under the AV-pressure profile before shipping.

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 _HOME_TARGETS_TTL_SECS is 0.1 s), and a per-leaf latency above ~50 ms sustained makes the 41-leaf job miss 2 s while single candidates still pass, so in the 50–600 ms/leaf band the PR refuses where base merely ran a 2–25 s loop block and survived. Measured on healthy NTFS and ext4 the rebuild is ~10 ms (per-leaf realpath 0.1–0.3 ms), ~200× inside the budget; in the incident profile (1–5 s per single realpath) base and PR both fail, base by crashing. The lever is one constant: a rebuild-only budget (say _ANCHOR_REBUILD_TIMEOUT_SECS = 6.0, passed to _run_resolution_bounded for the rebuild job) widens the tolerated band ~3× at the cost of a per-call worst-case loop block of ~8 s instead of ~4 s — still well inside the 25 s watchdog, but it is exactly the accumulation argument the GPT lane pressed on this span, and the author has already overridden that lane once with the 4 s figure. Author's recommendation: keep 2 s in this PR (the bound that is proven against the crash dump) and decide the rebuild budget together with the cooldown ceiling in #9187's design pass; if maintainers prefer 6 s now it is a one-line change and the override reason is re-stated for it.

@bolichen97

Copy link
Copy Markdown
Collaborator Author

a 30-minute window in which every path check refuses will otherwise present as inexplicable blanket denials.

Same item as the previous head's; #9187 (deferred-finding, assigned, dated) tracks the user-facing "stalled, cooling down until T" reason. Note the gate already logs the stall at WARNING with the budget it missed and the cooldown length when it enters cooldown (#8022's _mark_stalled); what is missing is the operator-facing surface, which is what the issue is for.

@bolichen97

Copy link
Copy Markdown
Collaborator Author

"Could not start the {backend} adapter: the disk did not respond within 60 s while its security paths were being checked. Nothing was started; retry once the disk responds."

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.

@bolichen97
bolichen97 merged commit 1bdd8aa into main Sep 7, 2026
71 of 73 checks passed
@bolichen97
bolichen97 deleted the fix/windows-loop-stall-liveness branch September 7, 2026 06:08
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants