fix(hub,cli): four hub-restart-cascade cleanup bugs (#913 #914 #916 #919)

[heavygee]

Summary

Four contained bugs uncovered by the 2026-06-15 hub-restart cascade incident
where systemctl restart hapi-hub.service SIGTERMed 23 cursor ACP sessions.
Each fix is small and lands independently of the architectural #915
(cascade-archive root cause) and the hypothesis-pending #917. They produce
audit-trail correctness and idempotency wins that stand on their own.

Refs #913, #914, #916, #919.

#913 - cursor ACP fresh-session cursorSessionId persistence race

Fresh ACP sessions could be SIGTERMed mid-flight in the async
update-metadata ACK round-trip, stranding the on-disk ACP store with no DB
handle. Distinct from #834 (which fixed the resume path via
pre-registration before session/load).

Fix: add ApiSessionClient.flushMetadata() (drains the metadata lock,
5s timeout) and await it after onSessionFoundWithProtocol on the
fresh-session branch in cursorAcpRemoteLauncher.ts. The resume-path
pre-registration shape stays intact. A 5s no-ACK timeout logs a warning so
the operator has a diagnostic if hub backpressure ever drops the write.

#914 - default archiveReason polluted audit trail with "User terminated"

runnerLifecycle.ts defaulted archiveReason = 'User terminated'. SIGTERM
from a hub-restart cascade hit the same cleanup path that the web-UI
Archive click did, both recording "User terminated" — but only one was
intentional.

Fix:

• Default in runnerLifecycle.ts flipped to 'Hub restart'.
• registerKillSessionHandler (the hub-driven kill-RPC, sent only on
  web-UI Archive) now calls setArchiveReason('User terminated') before
  cleanupAndExit.
• SIGINT (local-terminal Ctrl-C) keeps 'User terminated'.
• All call sites (runClaude, runCodex, runCursor, runGemini,
  runKimi, runOpencode) updated to pass the lifecycle object instead of
  the bare closure. Legacy closure shape still accepted for
  back-compat (runAgentSession.ts).

#916 - POST /api/sessions/:id/archive returned 500 when CLI is gone

Archive route called engine.archiveSession → rpcGateway.killSession →
sessionRpc, which threw a generic Error when no target socket was
registered. The unhandled throw became 500. Operator clicking Archive on
split-brain zombies (after a cascade) saw nothing but a toast.

Fix:

• New typed RpcTargetMissingError (code: handler-not-registered or
  socket-disconnected) thrown from rpcGateway.rpcCall.
• syncEngine.archiveSession narrows on it: still flips lifecycleState
  to archived via a new sessionCache.markSessionArchivedFromHub helper
  (writes archivedBy='hub', archiveReason='Archived from hub (CLI unreachable)'),
  then handleSessionEnd marks inactive in cache.
• Route drops requireActive: true and short-circuits 2xx with
  { ok: true, alreadyArchived: true } for already-archived rows.
• Non-RPC errors (DB write failure, etc.) still propagate as 5xx.

#919 - sessionCache: three metadata writers threw on version-mismatch without refresh

renameSession, clearSessionArchiveMetadata, and
restoreSessionArchiveMetadata followed a one-shot-throw pattern on
version-mismatch. The cache stayed stale, so the caller's retry hit the
same mismatch → forever-409 on PATCH /sessions/:id,
POST /sessions/:id/reopen, and the reopen rollback path. The codebase
already had the good pattern: sessionCache.mergeSessions retries with a
fresh snapshot.

Fix: rewrite all three helpers as for (attempt = 0; attempt < 5; …)
retry-with-refresh loops. After 5 attempts of genuine concurrent contention
they still throw the original 409 message, so the existing operator-facing
contract is preserved.

§6 gates

• bun typecheck — passes on the branch against upstream/main@93d00414.
• bun run test — 982/983 tests pass. The single failure is the
  pre-existing runner.integration.test.ts > should detect version mismatch and kill old runner flake, confirmed to fail on upstream/main before the
  branch too (expected '0.0.0-integration-test-...' to be '0.20.2' —
  version-substitution issue unrelated to this diff).
• New unit tests:
  • cli/src/agent/runnerLifecycle.test.ts — default reason, override,
    crash override.
  • cli/src/claude/registerKillSessionHandler.test.ts — RPC stamps
    'User terminated' before cleanup; back-compat with bare closure.
  • cli/src/cursor/cursorAcpRemoteLauncher.test.ts — fresh-path
    flushMetadata ordering; resume-path fix(cursor): pre-register cursorSessionId on resume before ACP session/load completes #834 shape preserved.
  • hub/src/sync/rpcGateway.test.ts — RpcTargetMissingError typed
    thrown for both no-target cases.
  • hub/src/sync/sessionModel.test.ts — markSessionArchivedFromHub
    idempotency + self-heal; three sessionCache: renameSession / clearSessionArchiveMetadata / restoreSessionArchiveMetadata throw on version-mismatch without refresh; forever-409 until out-of-band cache refresh #919 self-heal tests for the metadata
    writers.
  • hub/src/web/routes/sessions.test.ts — /archive route: active 2xx,
    already-archived 2xx idempotent, missing-CLI 2xx (via engine mock),
    DB-failure 500, unknown id 404.
• Operator-side soup verification: branch merged into local
  driver/integration; 529/529 hub tests + 409/409 relevant cli tests
  pass on the integrated soup build.

Coordination notes

• web: text-only send to inactive session silently restores composer text with no error UI; operator perceives 'message disappeared' #918 (web silent message drop) is a separate parallel peer. No overlap.
• hub: mergeSessionData transfers a fixed field list then cascade-deletes the old session row, silently dropping any FK-tied data not enumerated (blocks scratchlist v2 #893 / migrate-on-delete #894 data-preservation guarantees) #920 (mergeSessionData cascade-delete) is handed off to PR feat(web,hub): scratchlist v2 - hub sync via typed table + session-updated piggyback #896 peer.
  This branch touches sessionCache.ts but in different methods
  (renameSession, clearSessionArchiveMetadata,
  restoreSessionArchiveMetadata, new markSessionArchivedFromHub) — the
  feat(web,hub): scratchlist v2 - hub sync via typed table + session-updated piggyback #896 peer's mergeSessionData work is untouched.
• architecture: hub restart cascade-archives every idle agent session; sessions should be resilient to hub bouncing #915 (architectural cascade-archive) and POST /reopen on archived cursor ACP session: spawned cursor-agent dies ~90s after successful ACP load, merges away the original (no recovery) #917 (reopen creates dead
  session) intentionally NOT addressed in this PR; if architecture: hub restart cascade-archives every idle agent session; sessions should be resilient to hub bouncing #915 lands later and
  reshapes the cascade behavior, the four fixes here still stand
  independently because their wins are audit-trail correctness and
  idempotency, not preventing the cascade.

AI disclosure

Per CONTRIBUTING.md: implementation by Claude Sonnet 4.5 (Cursor IDE
agent, auto-model) acting as a feature peer in a multi-agent HAPI fork
workflow. Issue triage and codepath verification were done by a sibling
discovery agent (c8ac5e86). Operator supervised; tests + cold review +
soup verification gates passed before raising the PR. The code follows the
operator's friction-mode disposition: smallest defensible change per bug.

Made with Cursor

[github-actions]

Findings

• [Major] Runner-driven stop paths now get mislabeled as hub restarts — the new default makes every SIGTERM archive as Hub restart unless a caller stamps a different reason first. Web Archive now does that through registerKillSessionHandler, but hapi runner stop-session, runner webhook timeout cleanup, and orphan cleanup terminate child sessions with SIGTERM directly (cli/src/runner/run.ts:267, cli/src/runner/run.ts:587). Those are operator/runner actions, not hub restarts, yet the changed SIGTERM handler keeps the new default at cli/src/agent/runnerLifecycle.ts:122, so archived metadata becomes misleading and the audit-trail fix regresses another supported termination path.
  Suggested fix:

  // propagate an explicit reason from runner stop/cleanup paths, e.g. via env on spawn
  const archiveReason = process.env.HAPI_ARCHIVE_REASON
  if (archiveReason) {
      lifecycle.setArchiveReason(archiveReason)
  }

Summary

• Review mode: initial
• Found one audit-trail regression introduced by the SIGTERM default change. Remaining risk: I did not run the full test suite in this automation pass.

Testing

• Not run (automation)

HAPI Bot

[heavygee]

Bot review (Codex PR Review run 27567766623) caught a real Major: with the #914 default flipped to 'Hub restart', three runner-driven SIGTERM paths get mislabeled.

Verified active paths (hub never calls RPC_METHODS.StopSession, that surface is dead):

• hapi runner stop-session <id> (controlServer.ts -> run.ts stopSession -> killProcessByChildProcess)
• webhook-timeout cleanup at cli/src/runner/run.ts:587
• orphan-cleanup at cli/src/runner/run.ts:267

Pushed 1c8972a3 with the smallest defensible fix matching the bot's suggested shape: parameterise the lifecycle default via HAPI_DEFAULT_ARCHIVE_REASON env, runner sets 'Stopped by runner' on spawn. Terminal-launched sessions (no runner parent) keep 'Hub restart' as the default since #915 cascade is the most plausible SIGTERM source there. Explicit overrides (KillSession RPC, SIGINT Ctrl-C, markCrash) still win. Two new unit tests cover env-default and override precedence.

Out of scope for this PR (flagging for maintainer): hapi runner stop-session is currently a bare SIGTERM with no pre-RPC, so it lands as 'Stopped by runner' instead of 'User terminated'. Sending a kill-RPC before SIGTERM would be a follow-up; not changing it here to keep the diff small.

[github-actions]

Findings

• [Major] Missing-CLI archive can report success even when the hub archive metadata write fails — markSessionArchivedFromHub returns silently on updateSessionMetadata errors or exhausted version retries, and archiveSession then still marks the in-memory session ended and lets /archive return { ok: true }. That regresses the stated #916 contract that non-RPC failures still surface as 5xx; after a DB/write failure the operator sees success while the persisted row can remain unarchived/running. Evidence hub/src/sync/sessionCache.ts:532, caller continues at hub/src/sync/syncEngine.ts:445 and hub/src/sync/syncEngine.ts:450.
  Suggested fix:

  markSessionArchivedFromHub(sessionId: string, reason: string): void {
      // ...
      if (result.result === 'error') {
          throw new Error('Failed to archive session metadata from hub')
      }
      // ...
      throw new Error('Session was modified concurrently while archiving from hub')
  }

Summary

• Review mode: follow-up after new commits
• Found one archive idempotency/data-persistence issue. Remaining risk: I did not run the full test suite in this automation pass.

Testing

• Not run (automation)

HAPI Bot

[heavygee]

Bot follow-up review on 1c8972a3 caught a second Major: markSessionArchivedFromHub silently returned on updateSessionMetadata errors and on exhausted version-retry attempts, which would let /archive claim 200 OK while the row stayed unarchived. That regresses the #916 contract that non-RPC failures still surface as 5xx.

Pushed 123c625e:

• Both fall-through paths in markSessionArchivedFromHub now throw, matching the contract of the sibling writers (renameSession, mergeSessions).
• Two new unit tests in sessionModel.test.ts spy on store.sessions.updateSessionMetadata to force result: 'error' and persistent result: 'version-mismatch' and assert the helper throws.
• No new route test needed: the existing still surfaces a 5xx for non-RPC errors (e.g. DB write failure) test at sessions.test.ts:1015 already covers route-level 500 propagation for any error from archiveSession.
• bun typecheck clean, 984/985 tests pass (same pre-existing runner.integration.test.ts version-mismatch flake that fails on upstream/main too).

[github-actions]

Findings

• [Major] Runner-spawned sessions are mislabeled on hub restart — cli/src/runner/run.ts:468 now injects HAPI_DEFAULT_ARCHIVE_REASON='Stopped by runner' into every runner-spawned child. Because runnerLifecycle uses that env var as the child’s default archive reason, a systemd/hub restart that SIGTERMs the runner and its children directly still archives those child sessions as “Stopped by runner”, not “Hub restart”. That reintroduces audit-trail pollution for the same restart-cascade path this PR is trying to fix; the runner’s own SIGTERM handler only shuts the runner down and does not selectively restamp already-spawned child env before the external signal reaches them.
  Suggested fix:

  // Keep the lifecycle default as Hub restart for externally delivered SIGTERM.
  // Stamp runner-owned stops at the kill sites instead of via static spawn env.
  const stopSession = (sessionId: string): boolean => {
      // before killProcessByChildProcess(...)
      void session.childProcess?.send?.({
          type: 'set-archive-reason',
          reason: 'Stopped by runner'
      })
      void killProcessByChildProcess(session.childProcess)
      return true
  }

Questions

• None.

Summary

• Review mode: follow-up after new commits
• The previous markSessionArchivedFromHub persistence failure finding is addressed, but the follow-up runner env override creates a new audit-attribution regression for hub-restart SIGTERM cascades.

Testing

• Not run (automation). Suggested coverage: runner-spawned child receiving plain SIGTERM without a stop-session request archives as Hub restart; stop-session/webhook-timeout paths archive as Stopped by runner.

HAPI Bot

[heavygee]

Bot review round 3 on `123c625e` flagged that `HAPI_DEFAULT_ARCHIVE_REASON='Stopped by runner'` on spawn mislabels hub-restart-cascade SIGTERMs against runner-spawned children: systemd cgroup-kill on `hapi-runner.service` stop hits runner-children directly with SIGTERM, and those now archive as 'Stopped by runner' instead of the 'Hub restart' that the cascade narrative wants.

This is the mirror image of the bot's round-1 finding. The two suggestions are mutually incompatible without adding an IPC channel (`stdio: 'ipc'` on spawn + `childProcess.send()`) so the runner can stamp `setArchiveReason` via IPC before SIGTERMing - which is a refactor, not a smallest-defensible change.

Pushed `a97b9dca` to revert the env-var override. The SIGTERM default is now plain 'Hub restart' for everyone, including runner-internal stop paths (`hapi runner stop-session`, webhook-timeout cleanup, orphan cleanup). The original #914 acceptance criterion - "Hub-restart cascade archives recorded with a reason that is NOT 'User terminated'" - is still met. Finer attribution between cascade vs runner-stop is deferred as a follow-up that needs the IPC channel.

`bun typecheck` clean. `bun run test` 982/983 (same pre-existing `runner.integration.test.ts` flake on upstream/main).

@tiann happy to add the IPC channel as a separate PR if you'd rather have the finer attribution in-flight; just wanted to keep this bundle scoped to the four issues it's titled for.

[github-actions]

Findings

• [Major] Clean completions are archived as hub restarts — changing the lifecycle default to Hub restart means every normal agent-loop completion that only calls setSessionEndReason('completed') now writes archiveReason: 'Hub restart' during archiveAndClose. For example, Codex/Claude/Cursor/Gemini/Kimi/OpenCode all set the end reason to completed before cleanupAndExit, but none set an archive reason on that path, so successful exits pollute the audit trail as restart cascades. Evidence: cli/src/agent/runnerLifecycle.ts:46 (changed default), with the write at cli/src/agent/runnerLifecycle.ts:59 and completion call sites such as cli/src/codex/runCodex.ts:377.
  Suggested fix:

  const setSessionEndReason = (reason: SessionEndReason) => {
      sessionEndReason = reason
      if (reason === 'completed' && archiveReason === 'Hub restart') {
          archiveReason = 'Session completed'
      }
  }

Questions

• None.

Summary

• Review mode: follow-up after new commits
• The previous runner env override finding is addressed, but the new lifecycle default still leaks into clean completion paths and misattributes successful exits as hub restarts.

Testing

• Not run (automation). Suggested coverage: a lifecycle cleanup after setSessionEndReason('completed') writes a non-restart archive reason while SIGTERM/default cleanup still writes Hub restart.

HAPI Bot

[heavygee]

Round 4 bot finding is legitimate and was pre-existing — even before this PR, clean completions wrote `archiveReason: 'User terminated'` (the OLD default), which was also wrong. My default-flip just changed the noise from one wrong label to another. Worth fixing while we're here.

Verified all six agent runners call `setSessionEndReason('completed')` before `cleanupAndExit()` on the natural exit path without setting an archive reason: `runClaude.ts:461`, `runCodex.ts:377`, `runCursor.ts:203`, `runGemini.ts:195`, `runKimi.ts:185`, `runOpencode.ts:308`.

Pushed `98b10319`. Applied the bot's suggested shape: the setter now flips `archiveReason` to 'Session completed' when transitioning to 'completed' AND no caller has overridden first. One change covers all six runners. Two new tests cover the natural-completion default and the override-precedence guard (explicit `setArchiveReason('User terminated')` before `setSessionEndReason('completed')` still wins).

`bun typecheck` clean, 984/985 (same pre-existing `runner.integration.test.ts` flake).

[github-actions]

Findings

• No issues found.

Questions

• None.

Summary

• Review mode: follow-up after new commits
• No high-confidence correctness, security, data-loss, performance, or maintainability issues found in the current diff. The previous bot finding about clean completions being archived as hub restarts is addressed in this head.

Testing

• Not run (automation).

HAPI Bot

[heavygee]

Reviewer context (2026-06-16): relationship to #915 / PR #928

This PR (#923) is defense-in-depth for four concrete bugs filed from the 2026-06-15 hub-restart cascade incident (#913, #914, #916, #919). It does not attempt to solve the architectural root cause described in #915.

Since filing #915, post-incident tracing narrowed the primary cascade mechanism:

• Runner already spawns children with detached: true and does not kill them on its own cleanup (cli/src/runner/run.ts).
• The actual SIGTERM cascade on hub/runner restart comes from systemd cgroup membership with default KillMode=control-group, which kills every PID in the unit cgroup regardless of setsid().
• PR docs(installation): add KillMode=process to runner systemd unit (closes #915) #928 (docs(installation): add KillMode=process to runner systemd unit) addresses that root cause directly.

How to read this PR alongside #928:

If #928 lands	What #923 still buys
Children survive runner bounce	#913 - cursorSessionId still needs to persist before ACK if a child is killed by other means (crash, explicit kill, non-systemd deploys)
Archives stop being cascade-induced	#914 - archiveReason correctness still matters for genuine user-archive vs crash vs runner-stop paths
Operator cleanup after rare deaths	#916 - POST /archive should still be idempotent when CLI is already gone
Cache writes after out-of-band refresh	#919 - version-mismatch retry-with-refresh still prevents forever-409 on rename/reopen rollback

Recommend reviewing #923 on its own merits as belt-and-braces correctness, not as a substitute for #928. If both merge, the operator-facing incident class from 2026-06-15 should be largely closed; #917 (reopen post-load death) remains a separate open track.

[heavygee]

Rebased onto upstream/main (c311afdd, +2 commits: #904 Codex fast mode, #886 imported Codex machine binding). Resolves CONFLICTING / behind-2 — head is now 1495971d.

sessionCache.ts conflict was against #886 machine-binding changes; rebased cleanly with #919 retry logic + #916 markSessionArchivedFromHub intact.

Bot Major status (orchestrator triage):

Round	Finding	Status at 1495971d
1	Runner stop-session/webhook/orphan SIGTERM → mislabeled	Deferred — env-on-spawn fix (round 1 suggestion) contradicted round 3 (cascade mislabel). IPC channel follow-up; #914 acceptance ("not User terminated") still met
2	markSessionArchivedFromHub silent DB failure → false 200	Fixed 123c625e
3	Env override mislabels cascade children	Fixed — reverted env approach a97b9dca
4	Clean completions archived as Hub restart	Fixed 98b10319 / 1495971d

Round-5 bot review on prior head: "No issues found." CI re-running on rebased head.

Local: bun typecheck clean, 1001/1003 tests pass (2 pre-existing/flaky runner.integration.test.ts failures unrelated to this diff).

[github-actions]

Findings

• [Major] Preserve the inactive-session guard in /archive — the route now removes requireActive: true and only short-circuits rows already marked lifecycleState="archived". Any inactive, non-archived row (for example a completed/stub session or a normal inactive row that the UI would offer Delete/Reopen for) now falls through to engine.archiveSession; if no CLI RPC target exists, the new fallback writes archivedBy="hub" / archiveReason="Archived from hub (CLI unreachable), changing audit metadata for a session that was not active. Evidence hub/src/web/routes/sessions.ts:304.
  Suggested fix:

  const lifecycleState = sessionResult.session.metadata?.lifecycleState
  if (lifecycleState === "archived") {
      return c.json({ ok: true, alreadyArchived: true })
  }
  if (!sessionResult.session.active && lifecycleState !== "running") {
      return c.json({ error: "Session is inactive" }, 409)
  }

Questions

• None.

Summary

• Review mode: follow-up after new commits
• The archive cleanup path still needs to preserve the old inactive-session contract while allowing the intended stale lifecycleState="running" cleanup case.

Testing

• Not run (automation): bun is unavailable in this runner (bun: command not found).

HAPI Bot

[github-actions]

Findings
No issues found.

Questions
None.

Summary

• Review mode: follow-up after new commits
• Prior /archive inactive-session concern is addressed in the current head. Static review found no high-confidence correctness or security regressions in the changed lifecycle cleanup, archive fallback, metadata retry, and Cursor ACP flush paths.
• Residual risk: tests were not executed here; validation was static because PR content is untrusted in this review workflow.

Testing

• Not run (automation): static review only; did not execute PR code.

HAPI Bot

[github-actions]

Findings

• [Major] Pi archive RPC still uses the legacy cleanup callback — runnerLifecycle now defaults archives to Hub restart, and registerKillSessionHandler only stamps User terminated when callers pass an object with setArchiveReason. The changed call sites were updated for Claude/Codex/Cursor/Gemini/Kimi/OpenCode, but the live Pi runner still registers registerKillSessionHandler(apiSession.rpcHandlerManager, lifecycle.cleanupAndExit) at cli/src/pi/runPi.ts:91. That means clicking Archive from the web for a Pi session will go through KillSession but persist archiveReason: 'Hub restart', reintroducing the audit-trail bug this PR is fixing. Evidence: cli/src/claude/registerKillSessionHandler.ts:43, related context cli/src/agent/runnerLifecycle.ts:47 and cli/src/pi/runPi.ts:91.
  Suggested fix:

  // cli/src/pi/runPi.ts
  registerKillSessionHandler(apiSession.rpcHandlerManager, lifecycle);

Questions
None.

Summary

• Review mode: follow-up after new commits
• One audit-trail regression remains for Pi sessions because the new lifecycle default is not paired with the new KillSession registration shape on that runner.
• Residual risk: validation was static only; PR code was not executed in this review workflow.

Testing

• Not run (automation): static review only; did not execute PR code.
• Suggested: add a Pi-side KillSession registration test or extend an existing Pi runner mock test to assert the lifecycle object is passed.

HAPI Bot

[github-actions]

Findings
None.

Questions
None.

Summary

• Review mode: follow-up after new commits
• No issues found in the latest PR diff. The prior Pi KillSession lifecycle finding is addressed at cli/src/pi/runPi.ts:91.
• Residual risk: static review only; lifecycle timing and hub/CLI disconnect behavior still rely on existing integration coverage.

Testing

• Not run (automation): static review only; did not execute PR code.

HAPI Bot