fix(dashboard): stop cautious boot slowing the recovery it exists for by GoZippy · Pull Request #11288 · kirodotdev/KiroCrew

GoZippy · 2026-09-16T10:47:29Z

Problem / Motivation

Cautious boot staggers the dashboard's startup battery when a recent loop-stall
dump says the previous instance wedged. It decides from the dump's age and the
host's current memory posture, but not from anything about the instance that
wrote the dump. So a gateway that served for nine hours and then stalled is
treated identically to one that wedged part-way through starting up.

The result is that the slowest start this code can produce is the one
immediately after a stall, on exactly the host that most needs to come back.
On a host already near the stall threshold the stagger is itself enough to trip
the watchdog again, which writes another dump, which justifies another stagger.

Observed directly while reproducing #6590: two-minute
instance lifetimes for as long as the cycle held, and 7.4h, 9h and 15.5h
lifetimes once the dumps were moved aside by hand.

Why it matters

The stagger exists to protect the startup battery. An instance that reached a
serving state already got the whole battery away before it stalled, so the
battery is demonstrably not what wedged it, and slowing the battery cannot
help. Worse, the failure is self-sustaining: the mitigation manufactures the
evidence that triggers the mitigation. An operator sees a gateway that will not
stay up and no indication that the recovery path is what is holding it down.

What changed

DashboardState.ready is this codebase's own definition of "this instance
finished starting up", so both sites that publish it now record the instance's
identity triple, the same (pid, domain, start_id) a dump header already
carries. On the next boot, cautious boot asks whether the dump it found was
written by a process that had reached that point, and staggers only when the
answer is no.

Every existing decision path is untouched: a dump from an instance that never
became ready staggers exactly as it did before.

Recording the marker is best-effort and never raises. The data home can be on
network storage and this runs on the gateway loop, so the write is off-loop,
and a data home that cannot take it must not fail a gateway that is otherwise
healthy. A missing marker reads as "did not reach healthy", which is the
conservative answer and preserves today's behaviour.

Tests

test/test_cautious_boot.py — 31 passed, run on Windows 10 Pro 19045 with
Python 3.12. The new cases cover a dump from a ready instance (no stagger), a
dump from an instance that never became ready (stagger, as today), a missing
marker (stagger), and an unwritable data home (no raise, stagger).

Manual verification

Reproduced on the reporting host: with the cycle in place, instance lifetimes
were ~2 minutes; with the dumps moved aside by hand, 7.4h, 9h and 15.5h. The
marker makes the second outcome the automatic one.

Pattern harvest

Not generalizable: the defect is that a mitigation read one property of a
failure (a dump exists, and how old it is) and not the property that decides
whether the mitigation applies (what the failing instance had already
accomplished). The general shape — a self-sustaining mitigation whose own cost
manufactures its trigger — is worth a reviewer's attention, but there is no
mechanical rule here. No lint can tell which property a mitigation should have
read, and a rule against "recovery paths that can cause the condition they
recover from" would have no decidable predicate.

Related Issues

Part of #6590. Does not close it: #6590 asks for a
supervisor as well as this, and the supervisor half is a separate pull request.

Checklist

Tests pass
Existing decision paths unchanged
Failure of the new write cannot fail the gateway
Spec updated in the same commit where behaviour is documented

github-actions · 2026-09-16T12:54:09Z

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

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

First-Principles-Verdict: CONCERNS

The separate marker file manufactures its own complexity: identity-triple matching, recycled-PID logic and symlink/FIFO hardening all exist only because the ready-record lives outside the dump.

Not justified as shipped

Item 3 — oversized: "both sites that publish it now record the instance's identity triple" — but the instance's own dump file is already open at ready (_active_dump_file, held for faulthandler's lifetime, crash_dump_store.py:596), so one ready line appended to that fd would travel, rotate and sweep with the dump itself. The cross-file matching in dump_owner_reached_healthy (PID, domain, start-id), the recycled-PID and other-host cases, and the O_NOFOLLOW/O_NONBLOCK/256-byte read hardening are all costs of choosing a second agent-writable file over the file the reader is already parsing.
Subtraction: delete HEALTHY_MARKER_NAME, _read_healthy_marker, the marker file and the triple-match; append a ready line to the instance's own open dump file and have dump_owner_reached_healthy scan the dump for it.
Clears when: the in-dump line replaces the marker file, or the author names the constraint that keeps the ready line out of the open dump fd.

What this change ships

Inventory (8 items) — 6 justified

Intent: stop the post-stall recovery boot from being the slowest boot when the stalled instance had already finished starting up (#6590) — a FIX.

A boot after a stall of a previously-serving instance no longer staggers on a calm host — justified
On a still-pressured host that boot staggers mildly instead of maximally — justified
A new last-healthy-boot file is written in the dumps folder at every ready — oversized (see above)
The marker write runs in the background after readiness is published — justified
A planted symlink, FIFO or oversized marker is refused on read — justified
Boot log reasons now say whether the prior instance had reached serving — justified
Three files fully reformatted and dropped from the black baseline — rides along (mandated by the format-what-you-touched gate)
New crash-dump-store helpers other code may call — justified

[FIRST-PRINCIPLES-REVIEWED] f630e71

github-actions · 2026-09-16T12:54:12Z

Design Review (Fable 5, fork) — ✅ PASS

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

Design-Verdict: PASS

Verify the repro host's cycle also breaks when posture stays tight/critical, where an exonerated dump still keeps the 2s mild stagger rather than zero.

Suggestions

dump_owner_reached_healthy compares m_start == start_id raw where every other reader routes through _start_ids_comparable; a representation drift only costs a slower boot here, but reusing the guard keeps one definition of "comparable identity".

[DESIGN-REVIEWED] f630e71

github-actions · 2026-09-16T12:55:28Z

Opus 5 Review (fork) — ✅ no blocking findings

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

Review details

One non-blocking regression: the new healthy-boot marker lands inside the crash-dumps directory and silently evicts a real stall dump from every diagnostics bundle.

FINDING — src/kiro_crew/dashboard/crash_dump_store.py:913 — HEALTHY_MARKER_NAME = "last-healthy-boot" is resolved under get_dumps_dir() and rewritten on every boot that reaches state.ready, so it is always the newest file in crash-dumps/, while diagnostics.py:1117 collects dumps_dir.glob("*") — not DUMP_PREFIX/DUMP_SUFFIX — mtime-sorts descending and slices [:_MAX_CRASH_DUMPS] (5), so on any host holding 5+ files there the oldest genuine stall dump is dropped from the bundle in favour of a 3-token marker (the marker is also never reaped, since _list_dumps filters by prefix and neither sweep_stale_dumps nor rotate_dumps can see it) → Fix: build the marker path from config_dir() / "logs" rather than get_dumps_dir(), so non-dump bookkeeping stops competing for the bundle's dump quota.

[OPUS-REVIEWED] f630e71

github-actions · 2026-09-16T12:56:10Z

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] f630e71

dwu96 · 2026-09-23T16:52:19Z

🤖 Kiro Crew [operator: dwu96#8c08bee0]: This PR has been inactive for 7+ days with failing CI. I've assessed the blockers and they appear resolvable — I'll push fixes directly to this branch as a co-author.

Assessment: nothing in this PR's own code is failing. All six review lanes are ✅ on the current head 7ba66de8 (GPT 5.6, Opus 4.8, Design, First Principles, UX, Security Scope), there are no review threads, and the branch is MERGEABLE. The four red shards are:

Backend Tests (Windows) (4) and (8) — base drift, not this PR. The merge base is 69aeb280 (2026-09-16T02:13Z), 886 commits behind main. Shard 4 fails test_ledger_edge_concurrency.py::test_many_producers_one_session_all_entries_land on flush timed out, which is the flake tracked in test flake: test_many_producers_one_session_all_entries_land times out on Windows shard 4 #11344 — fixed on main by PR feat(agent): queue, admit and recover work under overload #10521 (progress-aware _drained() replacing the fixed timeout=10.0), and the file has since been renamed to test/test_crew_log_edge_concurrency.py. Shard 8 fails test_workflows_run_identity.py::test_simultaneous_services_never_share_run_identity[alice] on a 10s workflow bound, the class tracked in test_workflows_service.py fails on Windows shards across unrelated PRs #10964 (closed COMPLETED 2026-09-18, after this base, via test(workflows): wait on the save handshake, not a clock, on Windows #11115 / fix(session): recover the provider group after the leader is reaped #11456). A pull_request rerun replays the frozen merge ref and can never pick those fixes up — the remedy is a rebase onto current main plus a force-push-with-lease, which is the whole fix plan.
macOS Tests / Backend Tests (macOS) (2) and (3) — unrelated and repo-wide, out of scope for this PR. Shard 2 fails test_pdf_extract.py (TestBound detail rss vs PdfminerException, plus a crashed gw0 worker) — open issues ci(macos): test_pdf_extract TestBound pins detail='PdfminerException', but on darwin the RSS watchdog answers 'rss' first #13095 (same failure on unrelated PR feat(notifications): route notifications to chat transports (bridge B1) #11794) and ci: test_pdf_extract TestChildCaps crashes the xdist worker on main (in-process RSS watchdog os._exit) #13030 (crashes the worker on main) — and test_memory_store_dashboard.py::test_dashboard_backup_uses_configured_retention. Shard 3 fails seven test_pr_readiness_evaluate.py cases on tls: failed to verify certificate and a gh: HTTP 403: API rate limit exceeded for installation, i.e. runner environment, not code. This PR touches none of those files, so I will not change them.

Fix plan: rebase onto current main, force-push with a SHA-pinned lease, re-run the suite, and report. If the macOS reds survive the rebase they are the repo-wide failures above and belong to #13030 / #13095, not here — I'll say so rather than touching unrelated tests.

If you'd prefer I don't touch this PR, add the pr-no-autofix label.

bolichen97 · 2026-09-24T19:18:24Z

@GoZippy Maintainer sweep (2026-09-24, on behalf of @bolichen97): this PR carries readiness: passed and its required PR Readiness check is green, so it is on the merge list — the only thing holding it is the branch state below. Once you rebase onto current main (git rebase origin/main, resolve, force-push to your branch), CI will re-run and a fresh approval will be given; note that dismiss stale reviews on push is on, so any existing approval is expected to drop and will be re-issued after the rebase.

State: GitHub still reports a clean merge, but the fork head is 1060 commits behind main, the (non-required) macOS Tests / Backend Tests (macOS) (2) and (3) shards are red on the latest rerun, and @dwu96 announced on 09-23 a co-author push that has not landed yet. Holding the approval until the branch is rebased onto current main and CI is green on the new head — @dwu96, if your push includes the rebase, just ping here and I will re-audit.

Cautious boot staggers the startup battery when a recent loop-stall dump says the last instance wedged. It reads the dump's age and the host's current memory posture, but not whether the instance that wrote the dump had ever finished starting up -- so a gateway that served for nine hours and then stalled got the same treatment as one that wedged mid-boot. The start immediately after a stall is therefore the slowest one, on exactly the host that most needs to come back, and on a host already near the stall threshold that stagger can trip the watchdog again and write another dump. Reported upstream as kirodotdev#6590, with the cycle observed directly: two-minute lifetimes while it held, and 7.4h, 9h and 15.5h once the dumps were moved aside by hand. The stagger exists to protect the startup BATTERY. An instance that reached a serving state got the whole battery away before it stalled, so the battery is not what wedged it. DashboardState.ready is the codebase's own definition of that point, so both sites that publish it now record the instance's identity triple -- the same (pid, domain, start_id) a dump header carries -- and the next boot asks whether the dump it found was written by a process that had reached it. A downgrade, not an off-switch: a host still reading tight or critical keeps a mild stagger, because current pressure is its own signal and is not answered by what the previous instance managed to do. The match is deliberately strict, because the two errors do not cost the same. A false "reached healthy" removes the stagger and is how a host that wedges during startup re-wedges; a false negative only costs the slower boot that happens today. So a missing marker, a start identity absent on either side, a recycled pid, a marker from another host and the legacy header that carries no identity at all all answer no. That also makes the change inert on any platform without a start identity, and leaves every existing decision path untouched. The marker is written off-loop: it is a small write, but the data home can be on network storage, and this runs on the gateway loop. Recording it is best-effort and never raises -- a data home that cannot take the write must not fail a gateway that is otherwise healthy. A missing marker reads as "did not reach healthy", which is the conservative answer.

Three ways the marker could cost the boot it exists to speed up. The write was awaited. `await asyncio.to_thread(...)` keeps the event loop responsive but not the coroutine, and this coroutine's RETURN is what publishes KIROCREW_READY -- so a data home on a stalled network share held readiness open indefinitely, and a supervisor waiting on that line respawns into the same hang. Nothing about the marker belongs to this boot, so it is dispatched as a tracked background task instead. The read followed and blocked. The marker sits in the dumps directory, which the agent can write to, and `read_text` follows a symlink planted at that name and blocks opening a FIFO. The enclosing except is fail-OPEN and cannot catch a hang. The read now uses O_NOFOLLOW and O_NONBLOCK where the platform has them, checks S_ISREG everywhere, and is bounded to one short line, so anything that is not a plain file reads as absent -- the same conservative answer a missing marker already gives. The write followed too, which is worse than reading wrong. Its temp name was derived from the PID, and PID 1 in a container is deterministic across restarts, so a symlink can be waiting at that name before the process that writes through it exists -- and a plain write follows it and truncates the target. It now publishes through atomic_write, whose temp file is unique and O_EXCL. Also drops an issue number from a test docstring, and prunes three entries that this change made black-clean from the formatting baseline.

iamwhatever · 2026-10-05T09:02:49Z

Thank you, @GoZippy, for this fix — it's a genuinely good one. Diagnosing that cautious boot was slowing exactly the recovery it exists to protect, and tying "did the previous instance actually reach a serving state?" to DashboardState.ready with the same identity triple a dump header carries, is a clean and well-reasoned approach. The off-loop marker write and the O_NOFOLLOW/O_NONBLOCK/S_ISREG hardening on the read path are exactly right.

To get it merged, your two commits (authorship unchanged) have been rebased onto current main and continue as a same-repo PR, #16954, where the full CI and review lanes can run and go green. This fork PR is being closed in favour of that one — the change lives on there, and you keep credit as the author of both commits.

Closing with thanks. Please follow #16954 for the rest of the way to merge.

GoZippy requested review from a team and Zedmor September 16, 2026 10:47

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 16, 2026

GoZippy force-pushed the fix/cautious-boot-readiness branch from c81a3a9 to bb14bd1 Compare September 16, 2026 13:04

GoZippy force-pushed the fix/cautious-boot-readiness branch from bb14bd1 to 4f14e97 Compare September 16, 2026 13:10

GoZippy force-pushed the fix/cautious-boot-readiness branch from 4f14e97 to 7ba66de Compare September 16, 2026 13:42

dwu96 added the needs-pr-triage PR scanner: awaiting automated triage label Sep 23, 2026

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 23, 2026

dwu96 added drive-to-green PR claimed by drive-to-green pipeline and removed needs-pr-triage PR scanner: awaiting automated triage labels Sep 23, 2026

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 23, 2026

bolichen97 mentioned this pull request Sep 25, 2026

After a loop-stall termination the gateway cannot recover unattended on Windows: no supervisor, and a recent dump slows every restart #6590

Closed

GoZippy added 2 commits September 26, 2026 20:29

GoZippy force-pushed the fix/cautious-boot-readiness branch from 7ba66de to f630e71 Compare September 27, 2026 01:36

iamwhatever mentioned this pull request Oct 5, 2026

fix(dashboard): stop cautious boot slowing the recovery it exists for #16954

Merged

4 tasks done

iamwhatever closed this Oct 5, 2026

github-actions Bot removed the readiness: action required A blocking check or review needs attention label Oct 5, 2026

cruisercohen removed the drive-to-green PR claimed by drive-to-green pipeline label Oct 5, 2026

Navigation Menu

Repository navigation

fix(dashboard): stop cautious boot slowing the recovery it exists for - #11288

fix(dashboard): stop cautious boot slowing the recovery it exists for#11288
GoZippy wants to merge 2 commits into
kirodotdev:mainfrom
GoZippy:fix/cautious-boot-readiness

GoZippy commented Sep 16, 2026

Uh oh!

github-actions Bot commented Sep 16, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 16, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 16, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 16, 2026 •

edited

Loading

Uh oh!

dwu96 commented Sep 23, 2026

Uh oh!

bolichen97 commented Sep 24, 2026

Uh oh!

iamwhatever commented Oct 5, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

GoZippy commented Sep 16, 2026

Problem / Motivation

Why it matters

What changed

Tests

Manual verification

Pattern harvest

Related Issues

Checklist

Uh oh!

github-actions Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Not justified as shipped

What this change ships

Uh oh!

github-actions Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design Review (Fable 5, fork) — ✅ PASS

Suggestions

Uh oh!

github-actions Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Opus 5 Review (fork) — ✅ no blocking findings

Uh oh!

github-actions Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GPT 5.6 Review (fork) — ✅ no blocking findings

Uh oh!

dwu96 commented Sep 23, 2026

Uh oh!

bolichen97 commented Sep 24, 2026

Uh oh!

iamwhatever commented Oct 5, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions Bot commented Sep 16, 2026 •

edited

Loading

github-actions Bot commented Sep 16, 2026 •

edited

Loading

github-actions Bot commented Sep 16, 2026 •

edited

Loading

github-actions Bot commented Sep 16, 2026 •

edited

Loading