fix: resolve the provider CLI off the event loop - #4195
Conversation
The sidebar PR check-status refresh resolved gh/glab synchronously on the event loop thread. Resolution stats every candidate install dir plus the full parent chain of each hit, and a miss re-walks PATH, so once those syscalls turned slow the loop stopped serving every other task -- the liveness heartbeat included -- until the loop watchdog killed the gateway and the supervisor respawned into the same condition. One host took three such kills in fourteen hours, each one ending every live chat session. Offload the walk with asyncio.to_thread, the way this same function already offloads its SEL audit write. A slow filesystem now costs latency on one sidebar chip refresh instead of the whole gateway. Add the first test that asserts a handler does not block the loop: it compares the resolver's thread identity against the loop thread's rather than measuring elapsed time, so it cannot flake on a loaded host. Correct one existing test alongside it. Its fake to_thread replaced the real one wholesale, assuming the audit write was the only offload on this path, and dropped the wrapped call's return value; it now dispatches on the function it received and returns the real result, so it tests the audit ordering it names.
github-actions
Bot
added
the
readiness: checking
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS A one-line off-loop fix aimed precisely at the traced crash cause, with a thread-identity test that can't flake — proportionate and sound. [DESIGN-REVIEWED] 97e3018 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe diff is a minimal, correct change: No findings. [OPUS-REVIEWED] 97e3018 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSAdvisory premise-level review of Contract read and followed. I verified the diff against the repository: the fix is one production line in Key checks I ran:
First-Principles-Verdict: PASS A reported crash-loop fixed at the blocking call itself, using the file's existing offload mechanism, with siblings and the deferred general fix explicitly counted. What this change shipsIntent: stop a slow filesystem from freezing the whole gateway when the PR sidebar refreshes — a FIX.
The fix sits at mechanism level; the description itself names the cause (no mechanical [FIRST-PRINCIPLES-REVIEWED] 97e3018 |
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: |
github-actions
Bot
added
readiness: passed
github-actions
Bot
removed
the
readiness: passed
The sidebar PR check-status refresh resolved gh/glab synchronously on the event loop thread. Resolution stats every candidate install dir plus the full parent chain of each hit, and a miss re-walks PATH, so once those syscalls turned slow the loop stopped serving every other task -- the liveness heartbeat included -- until the loop watchdog killed the gateway and the supervisor respawned into the same condition. One host took three such kills in fourteen hours, each one ending every live chat session. Offload the walk with asyncio.to_thread, the way this same function already offloads its SEL audit write. A slow filesystem now costs latency on one sidebar chip refresh instead of the whole gateway. Add the first test that asserts a handler does not block the loop: it compares the resolver's thread identity against the loop thread's rather than measuring elapsed time, so it cannot flake on a loaded host. Correct one existing test alongside it. Its fake to_thread replaced the real one wholesale, assuming the audit write was the only offload on this path, and dropped the wrapped call's return value; it now dispatches on the function it received and returns the real result, so it tests the audit ordering it names. Co-authored-by: t <t@t>
2 participants
What broke
On 2026-08-17 a single gateway was killed by
LoopStallWatchdogthree times infourteen hours (05:02, 05:19, 18:47 local). Every chat session died with it. The
crash dump's main-thread stack is unambiguous:
_run_jsonresolved thegh/glabexecutable synchronously on the eventloop thread. Resolution is stat-heavy: it walks every well-known install dir,
and for each hit validates the file plus every parent directory
(
resolve(strict=True),Path.stat(),os.access()per component ingithub_runner.validate_provider_executable). A miss additionally re-walks allof
PATH. When those syscalls got slow, the loop stopped serving every othertask — including the liveness heartbeat — the watchdog fired at its 25s
exit_after, and the supervisor respawned into the same condition.Why it took this long to bite
The defect is compositional; no single diff contains it.
PATH, relaxed ownership checks#84's synchronous call was nearly harmless on its own. #290 is what turned it
into something that can kill an idle gateway, and #630 multiplied the cost per
call. Each PR was individually defensible.
Worth noting:
_run_jsonalready offloads its SEL audit write withasyncio.to_thread, and the same module offloads config reads and Jira auth thesame way. The offload habit was present; this one call site was simply missed.
The fix
One line — the resolution now runs on a worker thread:
A slow filesystem now costs latency on one sidebar chip refresh instead of
freezing the gateway.
Deliberately not in this change
No resolution cache.
github_runner.resolve_ghkeeps a_RESOLVE_CACHEkeyed on the override env values, and mirroring it here would cut the repeat
cost — but that key does not cover
KIROCREW_PROVIDER_BIN_STRICT,PATH, orPROVIDER_EXECUTABLE_CANDIDATES, all three of which change what resolutionreturns. A cache is a performance optimization with a real staleness surface; it
is not what keeps the loop alive, so it does not belong in a fix for a crash
loop.
Sibling call sites are untouched. The same shape exists elsewhere — at
minimum
apps/builtins/dev_fleet/server.py:1102(_trusted_bin, structurallyidentical),
apps/routes.py:1085(an unboundedshutil.rmtreedirectly insidean
async defuninstall handler), andknowledge/watcher.py:302(anos.statin a periodic scan whose seven sibling I/O calls all go through
to_thread).Those deserve their own change rather than being folded into an incident fix.
Tests
test_run_json_resolves_the_provider_cli_off_the_event_loopasserts theresolver ran on a thread whose id differs from the loop thread's. It compares
thread identity rather than elapsed time, so it cannot flake on a loaded host.
Verified load-bearing: reverting the production line to the synchronous call
makes it fail, restoring the line makes it pass.
This is the first test in the repo that asserts a handler does not block the
event loop.
test/test_loop_watchdog.py's 13 tests all cover the watchdog's owndecision logic — whether it dumps, debounces, and re-arms — never whether any
handler is off-loop.
One existing test needed a correction.
fake_to_threadintest_run_json_awaits_critical_audit_off_loop_before_spawnreplacedasyncio.to_threadwholesale, assuming the audit write was the only offload onthis path, and discarded the wrapped function's return value. It now dispatches
on the function it received and returns the real result, so it tests the audit
ordering it names instead of pinning "the audit is the only thing offloaded
here".
Review-gap note
AUTOSDE.yamlalready carries ablocking: truerule namedno-blocking-call-on-event-loop, added by #82 five days before #84 introducedthis call. Its text covers both this defect's shapes — "large synchronous file
IO or filesystem walks" and "a sync helper called transitively from an async
handler" — and it predicts the watchdog-plus-supervisor crash loop.
The rule has no deterministic counterpart. The backend grep pre-gate in
code-review.ymlchecks four unrelated things and its comment routes this classof rule to the semantic AI layer, and the repo pins bare
flake8with noflake8-async/ASYNCrule set. So the rule was enforceable only by LLMjudgment, and on #84 that judgment read it as "does this use blocking
subprocess.run?", answered no, and passed — in the same review that describedthe parent-directory stat walk as a security property.
Making the rule mechanical (the
flake8-asyncASYNC22x family, or promotinghistory.py'sOnLoopPersistErrordiscipline into a general off-loopassertion) is the durable follow-up. It is out of scope here.
Verification
pytest test/test_source_providers.py— 438 passed, 2 skippedremain, all in
test_artifact_source.py/test_artifacts_handlers.py, andall pre-existing on this host: stashing this diff and re-running the same
files reproduces the identical seven. They assert paths are outside
$HOME,which misreads a host where the real home path and
$HOMEdiffer by a symlink.isort --check-only src/kiro_crew test— cleanflake8 src/kiro_crew test— cleanmypy src/kiro_crew/(1.14.1, no faiss, matching CI) — no issues in 982 filestsc/vitestare unaffected