Skip to content

Crash-recovery loop at the composition root: offer restart instead of dying (#166) - #179

Merged
hellices merged 4 commits into
mainfrom
feat/crash-recovery-loop
Aug 3, 2026
Merged

Crash-recovery loop at the composition root: offer restart instead of dying (#166)#179
hellices merged 4 commits into
mainfrom
feat/crash-recovery-loop

Conversation

@hellices

@hellices hellices commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Closes #166

A fatal exception escaping Textual (see #165 for a concrete instance) terminated korvid immediately, losing the whole session. Textual has no supported in-app catch-and-resume hook, so the containment layer lives at the process level, in the composition root.

What changed

main() now wraps each attempt in _run_with_recovery:

  1. Each attempt is a fresh asyncio.run(_run(...)) — new event loop, new wiring, new KubeClient / provider / MCP controller / proxies. Nothing is reused from a crashed run (the single-loop aiohttp invariant holds per attempt), and _run's finally: _teardown(...) runs for the crashed instance before any rebuild.
  2. On an unexpected exception: full traceback via logging.exception, one-line summary on stderr, then korvid crashed — restart? [Y/n] (empty answer = restart).
  3. Cap: 3 crashes within a 60-second monotonic window stop the loop with korvid crashed N times within 60s — not restarting and re-raise — a deterministic startup crash cannot spin. Old crash timestamps age out, so long healthy runs reset the window naturally.
  4. Non-interactive (stdin/stderr not a TTY) or --no-restart: exactly today's behavior — re-raise, exit non-zero, traceback logged, never prompt.
  5. KeyboardInterrupt, SystemExit, and clean exits propagate untouched.

No approval state, pending proposals, or client objects survive a restart implicitly; the append-only audit file is unaffected.

Tests

tests/test_main_recovery.py (10): restart offered and second run executes, empty answer defaults to restart, declined prompt re-raises, disabled restart never prompts, crash cap stops the loop, window aging, clean exit / KeyboardInterrupt / SystemExit never prompt, traceback logged with exc_info. Runner, prompt, and clock are injected — no TTY, no real app.

Docs: docs/ops.md gains a "Crash recovery" section.

Full gate green: ruff, mypy --strict, 3083 passed, tach.

A fatal exception escaping the TUI no longer just kills the session:
main() wraps each attempt in _run_with_recovery, which logs the full
traceback, prints a one-line summary, and asks
'korvid crashed — restart? [Y/n]'. Every attempt is a fresh
asyncio.run(_run(...)) — new event loop, new wiring, new clients —
so nothing from the crashed run is reused, and _run's finally-teardown
runs for the crashed instance before the rebuild.

- Cap: 3 crashes within a 60s monotonic window stop the loop with a
  clear message (deterministic startup crashes cannot spin); old
  crashes age out so long healthy runs reset naturally.
- Non-interactive stdin/stderr or --no-restart keep today's behavior
  exactly: exit non-zero, traceback logged, no prompt.
- KeyboardInterrupt / SystemExit / clean exits propagate untouched.

Closes #166

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 3, 2026 16:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Introduces process-level crash recovery for fatal TUI failures.

Changes:

  • Adds interactive restart handling with crash-rate limiting.
  • Adds --no-restart and non-interactive behavior.
  • Documents and tests recovery behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/korvid/__main__.py Implements recovery and restart CLI handling.
tests/test_main_recovery.py Tests recovery-loop behavior.
docs/ops.md Documents crash recovery.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/korvid/__main__.py
Comment thread src/korvid/__main__.py Outdated
Comment thread src/korvid/__main__.py Outdated

@my-reviewer-agent my-reviewer-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 요약 (korvid #179 @ dc939ee)

issue #166 크래시 복구 루프 — 컴포지션 루트(main())에서 각 시도를 독립 asyncio.run(_run(...))으로 감싸는 설계가 깔끔합니다. 시도마다 새 이벤트 루프/KubeClient/provider/MCP 배선이라 single-loop aiohttp 불변식이 시도 단위로 유지되고, 크래시한 인스턴스의 finally: _teardown 이 재빌드 전에 실행됩니다. KeyboardInterrupt/SystemExit/정상 종료는 그대로 전파, 비대화식·--no-restart는 기존 동작(즉시 re-raise) 보존 — 모두 테스트로 고정되어 있고 runner/prompt/clock 주입으로 TTY 없이 결정적입니다. 크래시 캡의 윈도우 aging 테스트, exc_info 로깅 테스트 포함 10건 모두 실질적(비공허)입니다.

Suggestion (문서/메시지 불일치): 코드상 캡은 len(crash_times) > RESTART_CAP라서 윈도우 내 3회까지는 재시작을 제안하고 4번째 크래시에서 멈춥니다(테스트도 calls == RESTART_CAP + 1 == 4). 그런데 docs/ops.md와 PR 본문은 "3 crashes within 60 seconds stop the loop"라고 서술 — 실제로는 stderr 메시지도 crashed 4 times로 찍힙니다. 문서를 "허용된 재시작 3회를 넘기면 중단"으로 맞추거나 조건을 >=로 바꿔 서술과 일치시키는 것을 권합니다.

Suggestion (input() EOFError): allow_restart 판정은 시작 시점 stdin.isatty()이지만, 세션 중 터미널이 사라지면 _restart_prompt()input()EOFError를 던져 원래 크래시 대신 EOFError가 전파됩니다. except EOFError: return "n" 식으로 거절 처리하면 원인 예외가 보존됩니다. 같은 맥락에서 interactive 판정은 stdin/stderr TTY인데 프롬프트는 stdout으로 나가므로, stdout만 리다이렉트된 환경에선 프롬프트가 보이지 않은 채 입력 대기할 수 있습니다.

둘 다 advisory 수준이라 승인합니다.

APPROVE

…ompt on stderr

- _run now places everything after kube.connect() under a teardown
  guard: _wire_and_run fills a _RunState (mcp controller, live provider
  box, discovery box) as wiring progresses, and the guard releases
  exactly what was built — a probe/wiring failure can no longer leak
  the connected client (or provider/controller) into a recovery
  restart. _shutdown/_teardown accept a None discovery task.
- The crash cap stops at the RESTART_CAP-th crash within the window,
  matching the documented '3 crashes within 60 seconds'.
- The restart question is emitted on stderr (interactivity keys off
  stdin/stderr; a redirected stdout must not swallow or absorb it).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/korvid/main.py:911

  • state.provider_box is updated only after _build_agent_wiring() returns, but that function creates the provider before constructing ToolExecutor, AgentRuntime, and the configurator. If any of that later wiring raises, the teardown guard still sees [None]; for GitHub Copilot this leaks the eagerly created credential HTTP client into the recovery path. Hand ownership to _RunState immediately when the provider is created so partial agent wiring is also cleaned up.
    state.provider_box = provider_box

src/korvid/main.py:1064

  • The recovery tests pass allow_restart directly, so they do not cover this production TTY/CLI gate. Add main()-level tests with mocked stdin.isatty(), stderr.isatty(), and --no-restart to verify a crashing run is prompt-enabled only when both streams are TTYs and the flag is absent; otherwise a regression here would still leave all helper tests green.
    interactive = sys.stdin.isatty() and sys.stderr.isatty()

…, main() TTY gate tests

- _build_agent_wiring accepts the teardown guard's provider box and
  fills it the moment create_provider returns: a failure in the rest
  of the agent wiring (executor, runtime, configurator) no longer
  leaks an eagerly created provider (e.g. GitHub Copilot's credential
  HTTP client) into a recovery restart.
- main()-level tests pin the production restart gate: prompt-enabled
  only when stdin AND stderr are TTYs and --no-restart is absent.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

tests/test_main_recovery.py:117

  • Add match= here to follow the mandatory pytest.raises convention in AGENTS.md:68 and verify that the propagated SystemExit retains code 2.
    with pytest.raises(SystemExit):

tests/test_main_recovery.py:106

  • Add match= here; AGENTS.md:68 requires every pytest.raises assertion to verify the exception text. Because this runner raises a bare KeyboardInterrupt, matching the empty message preserves the test's current intent.

This issue also appears on line 117 of the same file.

    with pytest.raises(KeyboardInterrupt):

@my-reviewer-agent my-reviewer-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

재리뷰 (신규 커밋 2건) — 이전 라운드 지적사항이 정확히 반영되었습니다.

확인한 변경점

  • 크래시 캡 off-by-one 수정: len(crash_times) >= RESTART_CAP로 변경되어 문서("60초 내 3회 크래시 시 중단")와 조건이 일치합니다. 테스트도 runner.calls == RESTART_CAP으로 갱신되어 실제 동작을 고정합니다.
  • 재시작 질문 stderr 출력: _restart_prompt가 질문을 stderr로 내보내 리다이렉트된 stdout이 질문을 삼키거나 오염되지 않습니다. main()의 TTY 게이트(stdin AND stderr 둘 다 TTY + --no-restart 부재)도 4-케이스 테스트로 고정.
  • _RunState teardown guard: _runkube.connect() 이후 전 과정을 guard 아래 두고, _wire_and_run이 진행에 따라 state를 채우므로 wiring/probe 실패 시 "만들어진 것만 정확히" 해제됩니다 — 연결된 클라이언트가 recovery 재시작으로 새는 경로 차단. _shutdown/_teardowndiscovery_task: None 허용도 대칭적.
  • provider 소유권 즉시 이전: _build_agent_wiring(provider_box=state.provider_box)로 provider 생성 직후 guard의 box에 기록 — executor/runtime/configurator 단계 실패에도 provider(예: Copilot credential HTTP client)가 닫힙니다. provider_seen == [fake] 테스트가 teardown에 정확히 그 객체가 전달됨을 증명 (비공허).
  • discovery_box는 _run guard와 _wire_and_run이 같은 리스트를 공유하므로 :ctx 스위치로 교체된 live task를 teardown이 읽는 기존 불변식 유지 확인.

잔여 Suggestion (advisory): _restart_promptinput()이 여전히 bare — 크래시 후 터미널이 사라지면 EOFError가 원래 크래시 예외를 대체합니다. except EOFError: return "n"으로 거절 처리 권장 (이전 라운드 지적, 미반영).

APPROVE

KeyboardInterrupt matches the empty message it propagates with;
SystemExit verifies the preserved exit code 2.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@hellices
hellices merged commit ab4181d into main Aug 3, 2026
9 checks passed
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.

Crash-recovery loop at the composition root: offer restart instead of dying on a fatal TUI exception

2 participants