close
Skip to content

fix: surface the wedged thread's stack in doctor and boot replay - #2490

Merged
iamwhatever merged 1 commit into
kirodotdev:mainfrom
rubencu:fix/crash-dump-main-thread-stack
Aug 10, 2026
Merged

fix: surface the wedged thread's stack in doctor and boot replay#2490
iamwhatever merged 1 commit into
kirodotdev:mainfrom
rubencu:fix/crash-dump-main-thread-stack

Conversation

@rubencu

@rubencu rubencu commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Problem

A loop-stall crash dump exists to answer one question: where did the event loop wedge? Both consumers of the dump currently answer it wrong.

faulthandler.dump_traceback_later (armed by the loop watchdog) writes threads newest-first, so the MAIN thread — where the gateway's asyncio loop runs — lands last in the dump, behind dozens of idle thread-pool workers parked in Queue.get. Both readers scan top-down:

  1. kirocrew doctor prints the first thread in the file under the label MainThread stuck at: — on every real dump that is an idle worker (concurrent/futures/thread.py:90 in _worker), actively misleading whoever is diagnosing the stall.
  2. The boot-time journal replay (Replaying prior crash dump stacks, capped at 120 lines / 8KB) truncates before ever reaching the wedged stack. A gateway with a saturated default executor produces 200+ lines of idle workers first.

Evidence (from a real stall, sanitized)

A gateway stalled ≥25s and hard-exited. Doctor reported:

  last dump:   ⚠️  loopstall-….txt (12.0h ago)
  MainThread stuck at:
    Timeout (0:00:25)!
    Thread 0x… (most recent call first):
      File ".../concurrent/futures/thread.py", line 90 in _worker      ← idle pool worker
      File ".../threading.py", line 1012 in run
      File ".../threading.py", line 1075 in _bootstrap_inner

The journal replay of the same dump ended in [truncated — full dump at above path] while still printing idle workers. The actual wedged stack — sitting unread at the bottom of the dump — was:

Thread 0x… (most recent call first):
  File ".../concurrent/futures/thread.py", line 166 in submit
  File ".../asyncio/base_events.py", line 867 in run_in_executor
  File ".../asyncio/threads.py", line 25 in to_thread
  File ".../kiro_crew/dashboard/server.py", line 246 in _should_prevent_sleep
  File ".../kiro_crew/dashboard/server.py", line 1654 in _prevent_sleep_poll

Fix

In crash_dump_store.py, parse the dump into per-thread blocks and pick the wedged thread: a block explicitly marked Current thread when present (synchronous Python-initiated dumps mark the dumping thread), otherwise the last block (CPython dumps threads newest-first; the main thread is created first, so it is written last — and dump_traceback_later fires from an internal C thread, so no marker exists in watchdog dumps).

  • dump_first_stack_lines (doctor) now returns the preamble (Timeout (…)!) + the wedged thread's top frames. Doctor shows 8 lines so the first project frame past the asyncio plumbing is visible.
  • dump_replay_lines (boot journal replay) now replays the wedged thread first, then the remaining threads until the caps — the one stack that explains the stall always survives truncation.
  • Unrecognizable content (no thread headers) falls back to the previous top-down behavior.

Tests

  • 4 new tests: wedged-thread selection on a realistic multi-thread dump, Current thread marker preference, fallback on unrecognizable content, and replay-truncation survival (40 idle workers, 12-line cap — wedged frames still present).
  • Full test_crash_dump_store.py: 46 passed. test_loop_watchdog.py: 13 passed. Doctor tests: 3 passed.
  • Verified against the two real stall dumps that motivated this: doctor now shows the wedged thread's project frames, and the replay retains them under the 8KB cap (previously cut).
  • isort / flake8 / mypy clean on touched files.

@rubencu
rubencu requested a review from a team as a code owner August 10, 2026 06:24
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 10, 2026
… idle workers

faulthandler.dump_traceback_later writes threads newest-first, putting the
main thread (where the gateway's asyncio loop wedged) LAST in a loop-stall
dump, behind dozens of idle thread-pool workers parked in Queue.get.

Both consumers read the dump top-down:
- kirocrew doctor printed the FIRST thread under 'MainThread stuck at:' --
  always an idle worker, actively misleading whoever diagnoses the stall.
- The boot-time journal replay (120-line/8KB caps) truncated before ever
  reaching the wedged stack on real dumps.

Fix: parse per-thread blocks, pick the wedged thread (explicit 'Current
thread' marker if present, else the last block), surface it first in both
paths. Falls back to old top-down behavior for unrecognizable content.
@rubencu
rubencu force-pushed the fix/crash-dump-main-thread-stack branch from 9cf4da5 to 5c4c72d Compare August 10, 2026 15:33
@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 Aug 10, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Advisory design-level review of 5c4c72de58735f576c54163ff1e2afb3daf1b0d6 via the fork AI-review pipeline — updated in place on each push; does not block merge.

The patch is small and self-contained; I've verified the base file context (parsing lives in crash_dump_store.py, both consumers route through it, re already imported). Assessment below.

Design-Verdict: PASS

Real diagnostic harm, fixed at the right layer: one shared parser in crash_dump_store.py serves both consumers, with a documented fallback for unrecognizable dumps.

Suggestions

  • The "last block = wedged main thread" rule leans on CPython's undocumented newest-first dump order; matching the block containing the loop frame (run_forever / a kiro_crew frame) would survive an interpreter ordering change and any future non-main-thread loop — cheap to add as a tiebreaker ahead of the positional fallback.

[DESIGN-REVIEWED] 5c4c72d

@github-actions

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

This is a diagnostic-display change: reorders/selects which thread's stack kirocrew doctor and the journal replay show from a faulthandler dump. Let me verify the parsing logic against the header contract.

The header skip (_HEADER_LINES = 4) correctly lands on the Timeout preamble; _split_stack_content correctly separates preamble from thread blocks; _wedged_thread_block prefers an explicit Current thread marker and otherwise takes the last block; both consumers preserve all content (just reordered) and reapply the caps. The except OSError relocation still only guards the read. No security-, path-, or data-integrity-sensitive surface; output is human-facing diagnostics only.

No findings.

[OPUS-REVIEWED] 5c4c72d

@github-actions

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] 5c4c72d

@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 Aug 10, 2026
@iamwhatever
iamwhatever merged commit 745bb43 into kirodotdev:main Aug 10, 2026
53 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 10, 2026
@rubencu
rubencu deleted the fix/crash-dump-main-thread-stack branch August 10, 2026 18:26
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
… idle workers (kirodotdev#2490)

faulthandler.dump_traceback_later writes threads newest-first, putting the
main thread (where the gateway's asyncio loop wedged) LAST in a loop-stall
dump, behind dozens of idle thread-pool workers parked in Queue.get.

Both consumers read the dump top-down:
- kirocrew doctor printed the FIRST thread under 'MainThread stuck at:' --
  always an idle worker, actively misleading whoever diagnoses the stall.
- The boot-time journal replay (120-line/8KB caps) truncated before ever
  reaching the wedged stack on real dumps.

Fix: parse per-thread blocks, pick the wedged thread (explicit 'Current
thread' marker if present, else the last block), surface it first in both
paths. Falls back to old top-down behavior for unrecognizable content.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants