Repository navigation
fix(start-cli): verify live core on restart + split graceful/force-restart - #2418
Conversation
Incident 2026-07-30: an app-menu "Restart Core CLI" killed the core but no successor came up — the host was dark ~6.3h while Sutando.app reported a successful restart. The heartbeat daemon kept beating so nothing flagged it. Root cause in start-cli.sh --restart: it killed the core, polled only ~1s for exit, and when the core was still shutting down (Claude Code flushes telemetry/state on exit and routinely takes longer) fell into the "orphan claude already running -> reuse it -> exit 0" guard: the script returned SUCCESS without creating a fresh core. The zombie then died, leaving the session with no core, and the exit-0 made Sutando.app report "Core restarted". Fix (three parts): 1. --restart escalates SIGTERM -> SIGKILL and, if the core still won't die, aborts non-zero instead of stacking a second core / exiting a torn-down state. 2. the orphan-reuse guard is restart-gated: --restart never adopts a lingering or racing claude — it always tears down and creates fresh. 3. the no-TTY create path verifies tmux_core_session_running (session + a live `claude --name`) before exit 0, and exits non-zero otherwise, so a failed bring-up surfaces as a real failure via the caller (Sutando.app restartCore already surfaces stderr + non-zero exit) instead of a silent dead core. Regression test tests/start-cli-restart-verify.test.py drives a stubbed tmux/pgrep/ps/claude harness: it fails on the unpatched script (fresh-start and --restart both exit 0 with no live core) and passes on the fix; the legitimate non-restart orphan-reuse path still exits 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tempts Per review: don't mix force and graceful restart in one path. - `--restart` = graceful: SIGTERM + ~3s wait; if the core won't stop it ABORTS with a "use --force-restart" pointer (never SIGKILLs — it may be mid-task). - `--force-restart` = SIGTERM → SIGKILL escalation for a wedged/unresponsive core; hard-aborts if it still can't die (no stacking a second core). Both fall through to the create path's liveness verify (exit non-zero on a failed bring-up). Every attempt is appended, timestamped, to logs/restart-attempts.log so a failed restart is diagnosable even when the caller routes stdout to /dev/null — the logging gap that made the 2026-07-30 outage invisible. Test adds two cases (restart-wedged-aborts-not-kills, force-restart-wedged-escalates-to-sigkill); 6/6 green, fails on unpatched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pairs with the start-cli restart/force split. "Restart Core CLI" stays graceful (--restart: aborts rather than SIGKILL a possibly-mid-task core); the new "Force Restart Core CLI" runs --force-restart (SIGTERM→SIGKILL) for a wedged core. forceRestartCore() reuses the runCoreAction runner (detached bash, stderr→notify on non-zero). Also fixes the src/agent/start-cli.sh shim's runtime-switch guard to treat --force-restart as restart-class so it isn't clobbered with a prepended --restart. Menu-item choice (a) per sonichi's review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 68%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head c8b2209c3c624907ca324c9d0e73425ca4eebb5c.
The restart false-success class looks closed from this lane: graceful restart now aborts when the old core will not stop, force restart is explicit, detached launch verifies a live core before returning success, and restart does not silently adopt an orphan process.
Focused checks passed locally:
tests/start-cli-restart-verify.test.py- shell syntax check for
src/agent/claude/cli/start-cli.shandsrc/agent/start-cli.sh git diff --check origin/main...HEADscripts/review-checks.shon the PR diff
GitHub required checks are green. Approved.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact head c8b2209c3c624907ca324c9d0e73425ca4eebb5c.
The code-level direction is sound: graceful restart no longer silently adopts a dying orphan, force restart is explicit, detached launches verify a live core before success, and restart attempts are durably logged. Focused verification passed: restart harness 6/6, shell syntax, full Cocoa app compilation, diff hygiene, and the repository path gate.
Blocking merge readiness: the PR body explicitly marks live evidence as owed. This is a destructive startup/restart path, so the repository requires a real current-head post-restart round trip on the disposable Mini host. Please demonstrate graceful restart/abort and force-restart as applicable, a live successor core/heartbeat plus a completed task round trip, and the expected failure notification/log when startup does not recover. Worst case is the menu killing the only core and leaving the host dark again; harness-only proof does not close that operational risk. Not ready to merge until the live path is shown.
Reviewed by Qingyun's Personal Codex.
|
Closing at owner's request. The restart-race hardening + tests remain on branch fix/start-cli-restart-race and can be reopened or folded into the auth/boot work (#2413) if wanted — the tonight outage root cause is likely auth (logged-out boot), not this race. |
|
Reopening — I misread an unrelated 'close it' as this PR. Apologies for the churn. |
|
Cold review from Mini (REVIEW.md criteria run against head Verdict: LGTM, non-blocking — pending only the live round trip the body already owes.
|
|
Live evidence window complete (Chis-Mac-mini, 2026-07-30 17:08–17:11Z) — graceful restart + successor round trip, per qingyun asks #3/#4. Window authorized by Chi ("Y" 16:47Z, "why not now" 17:02Z). Branch bits ( 1. On-host harness: 6/6 greenThe full suite ( 2. Live graceful restart (real core, real tmux session)
Pre/post service evidence (pre-state snapshotted at 17:10:10Z, before the restart):
3. Successor round trip (this comment is written by the successor)The continuation task 4. #2398 outage capture: deferredThe outage-aware-bridge capture folds in only while a core is down between kill and successor. That gap (17:10:30–31Z, ~1s) closed before any staging was possible, and re-creating it would require deliberately downing the live core — not authorized. Deferred to a dedicated window. 5. RestorePhase 3 restore (checkout cc @sonichi |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed same head c8b2209c3c624907ca324c9d0e73425ca4eebb5c after the new live-evidence comment.
The remaining blocker from my prior review was operational proof of the restart path. The Mini evidence now shows the harness green on-host, a real graceful restart producing a live successor core, heartbeat re-arm, and a completed task round trip across the restart window. That closes the merge-readiness concern I raised on this head.
GitHub checks are green. Approved.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed head 07c07164b83bf5af827d8052902e87f1df935605.
No blocking findings. This is a merge-forward of the previously approved restart fix; the PR diff still keeps graceful restart non-destructive, gives force-restart a separate explicit path, and verifies a live tmux/core before claiming success. The focused restart harness still passes against this head.
Focused checks run:
python3 tests/start-cli-restart-verify.test.pypassed.bash -n src/agent/claude/cli/start-cli.sh src/agent/start-cli.shpassed.PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2418-pycache-1901 python3 -m py_compile tests/start-cli-restart-verify.test.pypassed.git diff --check origin/main...HEADpassed.
GitHub checks were green except the Python diff-coverage job was still pending when I checked; I did not find a code blocker in the PR diff.
Reviewed by Qingyun's Personal Codex.
|
@cla-assistant check |
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed exact refreshed head 870945f37810c7653386506eb7cd0f4be830315f and the complete current diff. No blockers found. Graceful restart aborts without SIGKILL when the old core will not stop, force restart is explicit, detached launch verifies a live core before claiming success, and attempts are durably logged. The Mini evidence demonstrates a real graceful restart, successor PID and heartbeat, watcher re-arm, and a task round trip spanning the restart window; that satisfies the live-path gate for this change.\n\nReviewer-run verification in an isolated exact-head worktree passed: restart harness 6/6; shell syntax for both launchers; Python compilation; diff hygiene; and the repository hardcoded-path scan. Required clean-install and coverage jobs are still running, so merge remains conditional on those finishing green.\n\nApproved.\n\nReviewed by John's Personal Codex.
|
Post-merge finding: the "~5s come-up" verify added here ( |
…2437) * fix(tests): start-cli config-dir stubs must model the core lifecycle This test has been failing on main since #2418 (920c472) — my own PR — and nothing caught it because **no CI job runs tests/*.test.sh at all**. #2418 added a post-launch liveness verify: after `tmux new-session`, start-cli polls `tmux_core_session_running` (session exists AND a live `claude --name $SESSION` under it) and exits non-zero when the core never comes up, so a failed launch can no longer report success. The stubs here report "nothing is running" unconditionally. That satisfies the PRE-launch short-circuit but can never satisfy the POST-launch verify, so start-cli correctly exited 1 and cases 1-2 failed on the exit code before reaching their real assertions. The config-dir behavior under test was fine throughout — case 2 was printing `CLAUDE_CONFIG_DIR=<expected>` while failing. Fix: make the stubs model the lifecycle instead of a fixed state. `new-session` writes a marker; `has-session`, `pgrep` and a new `ps` stub honor it, so the core reads as down before launch and up after. `ps` is needed because `core_claude_pids` cross-checks each pgrep pid against `ps -p <pid> -o args=` for `--name $SESSION` — faking pgrep alone is not enough. Before (clean origin/main, and identically on this branch's parent): 1. helper missing -> silent fallback FAIL: start-cli exit 1 2. helper + valid config -> env exported FAIL: start-cli exit 1 PASSED: 2 FAILED: 2 After: PASSED: 4 FAILED: 0 Verified the test still catches a genuinely dead core rather than being neutered — removed the marker write (core launches, never lives) and re-ran: PASSED: 2 FAILED: 2 <- same failure as before the fix Related finding, deliberately NOT fixed here (separate concern): `npm test` runs `tests/**/*.test.ts` and `tests/*.test.py`, so all 41 `tests/*.test.sh` are unrun by CI. On clean main 38 pass and 3 fail — this one, plus sutando-config-tsx-resolution and sync-workspace, which are environment- sensitive (nvm presence, local vault state) and need hermeticity work before any CI wiring would be stable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DkcAxdb5YmqrhNb7sLor7H * fix(tests): burn start-cli-claude-config-dir off the CI known-failures allowlist Correction to this PR's own description: CI DOES run tests/*.test.sh — ci.yml's "Run shell standalone tests" step, added for #1934 — with tests/shell-ci-known-failures.txt listing suites that run but do not gate CI. This suite is on that list, so the real deliverable is removing it. I claimed "no CI job runs them" from a grep for `tests/.*\.test\.sh`, which never matches `find tests -name '*.test.sh'`. A grep that finds nothing is evidence about the pattern, not the repo. The removal is earned, verified in the runner log rather than assumed: on this branch's CI run the suite appears as `→ tests/start-cli-claude-config-dir.test.sh` with NO `✖ shell test failed` line, while it still fails on a sibling branch that lacks the stub fix. So the lifecycle-modelling fix clears ubuntu-latest, not just this laptop, and a future regression should gate CI again. #1934 asks for this list to reach zero; this is one entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DkcAxdb5YmqrhNb7sLor7H --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
Problem
On 2026-07-30 an app-menu "Restart Core CLI" killed the core and no successor came up — the host was dark ~6.3h while Sutando.app had reported a successful restart. The heartbeat daemon kept beating, so nothing flagged it.
Honest root-cause status: the 00:01 attempt left no log — Sutando.app runs
start-cliwith stdout→/dev/nulland only a transient stderr→notify, so the exact mechanism isn't provable after the fact. What's confirmed (owner observation): no core at all existed overnight — not a logged-out core, not a wedged one; the restart killed the old core and created nothing. That's consistent withstart-cli.sh --restart's kill→create path returning success without a live core. This PR (a) makes that class of failure impossible to hit silently, and (b) makes the next occurrence diagnosable in one look.What's here (3 commits)
60535c5verify-live-before-success + robust kill.--restartwaits for the core to actually exit (Claude Code flushes state on shutdown, routinely >1s — the old ~1s poll let a still-dying core slip into the "orphan reuse → exit 0" guard), and the no-TTY create path now verifiestmux_core_session_runningbefore exit 0, exiting non-zero otherwise. No more silent false-success.1c8b1b8split graceful--restartfrom--force-restart(per review — don't mix force + graceful).--restart= SIGTERM + wait; if the core won't stop it aborts with a "use--force-restart" pointer (never SIGKILLs a possibly-mid-task core).--force-restart= SIGTERM→SIGKILL escalation for a wedged core. Every attempt is appended timestamped tologs/restart-attempts.log— closing the→/dev/nullblind spot that made the outage invisible.c8b2209"Force Restart Core CLI" menu item (Swift) + shim guard fix so--force-restartisn't clobbered with a prepended--restarton a runtime switch.Evidence
Regression test
tests/start-cli-restart-verify.test.py(stubbed tmux/pgrep/ps/claude harness — no real core touched). It fails on unpatchedmainand passes on this branch:Swift: full
swiftc -O … -framework Cocoa …compile is clean (binary produced).Live evidence — OWED. This is a startup/restart path, so a real post-restart round-trip is required, not just the harness. Per review guidance ("test on Mini only") I have not live-tested against a production core. The live
--force-restart+ graceful-abort round-trip will be captured on Mini and added here before merge.Relation to other PRs
exec start-cli.sh --restartonrestart-ready; this hardens exactly that call. Prep-before-kill (theirs) + reliable kill+verify (this) compose.