fix(hub): verify runner before trusting archiveSession's dead-CLI fallback - #1706
fix(hub): verify runner before trusting archiveSession's dead-CLI fallback#1706heavygee wants to merge 5 commits into
Conversation
…lback tiann#916 treats any RpcTargetMissingError from the session-scoped killSession RPC as proof the CLI is gone and archives the row. That RPC is keyed on whatever session id the CLI last registered under, which goes stale independently of the process itself (SessionCache.mergeSessions rotates the canonical id on resume; the session socket can also just be reconnecting), so a missing target does not prove the process is dead. Fall back to the machine-level stopRunnerSession RPC first, which resolves the child by PID and checks both the requested and confirmed session ids, before trusting archiveSession's fallback path. Fixes tiann#1705
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- [Major] Distinguish an unknown runner lookup from confirmed liveness — the new branch treats every
still_aliveresult as proof that a process is running, but the runner also returnsstill_alivewhen the requested ID is not tracked and has no verified-exit tombstone (cli/src/runner/run.ts:1045-1050; explicitly asserted incli/src/runner/runner.integration.test.ts:222). After a runner restart or tracking loss, an actually dead stale session with a reachable machine now fails archive indefinitely athub/src/sync/syncEngine.ts:1702.
Suggested fix:type StopSessionStatus = 'stopped' | 'already_gone' | 'still_alive' | 'unknown' // cli/src/runner/run.ts: no matching PID and no verified exit return 'unknown' // hub: only reject when a matched process survived the stop attempt if (status === 'still_alive') { throw new Error('Session process is still running and could not be stopped') } // Treat 'unknown' like the existing best-effort fallback.
Questions
- None.
Summary
- Review mode: initial
- One Major finding: the runner RPC contract currently conflates “confirmed process survived termination” with “session ID unknown,” so this change can make stale sessions unarchivable.
Testing
- Not run (automation; PR content was not executed).
- Add a regression test using the real runner status semantics for an unknown session ID; the added unit tests only mock
stopRunnerSession. - GitHub checks at review time:
integrationfailed;teststill in progress.
HAPI Bot
| // to the prior best-effort behavior. | ||
| status = 'already_gone' | ||
| } | ||
| if (status === 'still_alive') { |
There was a problem hiding this comment.
[MAJOR] Distinguish unknown lookup from confirmed liveness
stopRunnerSession does not currently mean “a process was confirmed alive” whenever it returns still_alive. Its no-match path returns the same value when the ID is absent from all runner maps and has no verified-exit tombstone (cli/src/runner/run.ts:1045-1050), and that behavior is asserted for an arbitrary unknown ID at cli/src/runner/runner.integration.test.ts:222. Therefore, after runner restart/tracking loss, an actually dead stale row with a reachable machine will hit this throw and cannot be archived.
Suggested fix:
type StopSessionStatus = 'stopped' | 'already_gone' | 'still_alive' | 'unknown'
// Runner no-match path:
return 'unknown'
// Hub only rejects a matched process that survived termination:
if (status === 'still_alive') {
throw new Error('Session process is still running and could not be stopped')
}
// 'unknown' keeps the prior best-effort archive behavior.Please also add a regression test that exercises the real unknown-ID contract rather than only mocking the status.
Code review caught a regression of this PR's own bug one RPC layer down: any failure of stopRunnerSession (ack timeout, protocol error — not just a genuinely offline machine) was being coerced to 'already_gone' and archiving the session, exactly the false-positive this fix exists to prevent. Distinguish "no runner connected to ask" (machineCache reports the machine offline — nothing stronger to check than the original RpcTargetMissingError, so fall back to archiving as before) from "the machine is online but the RPC itself failed" (treat as still_alive, the same conservative default terminateInPlacePiResume / terminateUnexpectedPiTemp already use for this exact RPC elsewhere in this file).
|
Follow-up commit: a self-review ( |
There was a problem hiding this comment.
Findings
- [Major] Distinguish an unknown runner lookup from confirmed liveness — the follow-up now separates offline machines from RPC failures, but it still rejects every
still_aliveresponse athub/src/sync/syncEngine.ts:1717. The runner uses that same response when the requested session ID is not tracked and has no verified-exit tombstone (cli/src/runner/run.ts:1045-1050), with the behavior explicitly asserted atcli/src/runner/runner.integration.test.ts:222. After runner restart or tracking loss, an actually dead stale session on an online machine therefore remains unarchivable.
Suggested fix:type StopSessionStatus = 'stopped' | 'already_gone' | 'still_alive' | 'unknown' // cli/src/runner/run.ts: no matching PID and no verified exit return 'unknown' // hub: only reject a matched process that survived termination if (status === 'still_alive') { throw new Error('Session process is still running and could not be stopped') } // Preserve the existing best-effort archive behavior for 'unknown'.
Questions
- None.
Summary
- Review mode: follow-up after new commits
- One Major finding remains unresolved: machine connectivity does not resolve the runner RPC contract's conflation of confirmed liveness with an unknown session ID.
Testing
- Not run (automation; PR content was not executed).
- Add a regression test covering the real runner response for an unknown session ID; the added hub tests only mock
stopRunnerSessionstatuses. - GitHub checks at review time:
integrationfailed;testwas pending.
HAPI Bot
| status = 'still_alive' | ||
| } | ||
| } | ||
| if (status === 'still_alive') { |
There was a problem hiding this comment.
[MAJOR] Distinguish unknown lookup from confirmed liveness
This still treats every still_alive result as proof that a process is running. The runner also returns still_alive when the requested ID is absent from all tracking maps and has no verified-exit tombstone (cli/src/runner/run.ts:1045-1050), and that behavior is asserted at cli/src/runner/runner.integration.test.ts:222. Thus, after runner restart or tracking loss, an actually dead stale session on an online machine cannot be archived.
Suggested fix:
type StopSessionStatus = 'stopped' | 'already_gone' | 'still_alive' | 'unknown'
// Runner no-match path:
return 'unknown'
// Hub only rejects a matched process that survived termination:
if (status === 'still_alive') {
throw new Error('Session process is still running and could not be stopped')
}
// 'unknown' keeps the prior best-effort archive behavior.Please also add a regression test using the real unknown-ID contract rather than only mocking the status.
Codex review caught a real gap: cli/src/runner/run.ts's stopSession
returns 'still_alive' both when it finds a PID that's confirmed still
running AND when no PID matches the id anywhere and there's no
verified-exit tombstone (a fail-closed default for a genuinely unknown
id). archiveSession's new fallback treated both cases identically,
so a stale row whose original runner generation lost track of it
(the exact 235-row backlog this investigation started from) would
permanently fail to archive on an otherwise-healthy machine.
Add a fourth 'unknown' status distinct from 'still_alive' for the
no-match case, threaded through the runner<->hub RPC contract
(cli/src/runner/{run,controlServer,controlClient}.ts,
cli/src/api/apiMachine.ts, hub/src/sync/rpcGateway.ts). archiveSession
now treats 'unknown' like the offline-machine case (best-effort
archive); every other existing stopRunnerSession call site
(fork-child cleanup, OpenCode reserved-clear recovery, PTY/Pi resume
termination and reconciliation) keeps its prior conservative behavior
by treating 'unknown' the same as 'still_alive', since those all deal
with sessions the hub has fresh, concrete knowledge of.
Updated the existing runner.integration.test.ts assertion for an
unknown id (was 'still_alive', now correctly 'unknown') and added a
hub-side regression test for the archive path.
|
Addressed the Major finding from both review rounds: added a fourth Updated the pre-existing
|
There was a problem hiding this comment.
Findings
- [Major] Do not equate an expired machine heartbeat with an unreachable runner —
MachineCache.activebecomes false after 45 seconds withoutmachine-alive(hub/src/sync/machineCache.ts:227), while the machine RPC registration is removed only when its socket disconnects (hub/src/socket/rpcRegistry.ts:37). A delayed/dropped heartbeat can therefore leavestop-sessionreachable, but the new branch athub/src/sync/syncEngine.ts:1700skips it, assumesalready_gone, and archives a live child—the same silent-orphan failure this PR is intended to prevent.
Suggested fix:try { status = await this.rpcGateway.stopRunnerSession(machineId, sessionId) } catch (runnerError) { status = runnerError instanceof RpcTargetMissingError ? 'already_gone' : 'still_alive' }
Questions
- None.
Summary
- Review mode: follow-up after new commits
- One Major finding: the heartbeat-derived
activeflag is not a safe proxy for machine-RPC reachability, so the fallback can still report successful archival while leaving the process running.
Testing
- Not run (automation; PR content was not executed).
- Add a regression test with a cached machine marked inactive while
stopRunnerSessionremains registered and returnsstill_alive; archival must fail and the session must remain active. - GitHub checks at review time:
integrationfailed;testwas still pending.
HAPI Bot
| const machineId = this.sessionCache.getSession(sessionId)?.metadata?.machineId | ||
| if (machineId) { | ||
| let status: 'stopped' | 'already_gone' | 'still_alive' | 'unknown' | ||
| if (this.machineCache.getMachine(machineId)?.active !== true) { |
There was a problem hiding this comment.
[MAJOR] Use RPC reachability, not heartbeat state
machine.active === false does not establish that the runner RPC is unreachable. MachineCache.expireInactive flips this flag after 45 seconds without a heartbeat (hub/src/sync/machineCache.ts:227-238), but RpcRegistry retains the machine handler until the socket actually disconnects (hub/src/socket/rpcRegistry.ts:37-48). If heartbeats are delayed or dropped while the socket/runner is still usable, this branch skips stopRunnerSession, coerces the state to already_gone, and archives a live child.
Suggested fix:
let status: StopSessionStatus
try {
status = await this.rpcGateway.stopRunnerSession(machineId, sessionId)
} catch (runnerError) {
status = runnerError instanceof RpcTargetMissingError
? 'already_gone' // no machine RPC target: preserve best-effort fallback
: 'still_alive' // timeout/protocol failure: fail closed
}Please add a test where the cached machine is inactive but the machine RPC still returns still_alive; the session must not be archived.
Codex review caught a third variant of this PR's own bug: MachineCache active flips false 45s after the last machine-alive heartbeat (machineCache.ts expireInactive), which is a different signal from whether the machine's RPC target is actually registered (that only clears on socket disconnect, rpcRegistry.unregisterAll). A delayed or dropped heartbeat could leave stopRunnerSession fully reachable while this fallback's `active !== true` pre-check skipped it and archived a live session anyway. Drop the machineCache pre-check entirely. Just attempt stopRunnerSession and let its own failure mode decide: a thrown RpcTargetMissingError means there is genuinely no RPC target to check (fall back to archiving, same as before); any other failure means the machine answered the transport layer but this call didn't resolve, which is not evidence the process is gone (treat as still_alive). Rewrote the regression tests to match: no longer registers a machine in machineCache at all (proving the decision doesn't depend on it), and asserts on the RpcTargetMissingError-vs-other-error distinction directly.
|
Addressed the third Major finding: Dropped the machineCache pre-check entirely. Now it just attempts |
There was a problem hiding this comment.
Findings
- [Major] Treat
unknownas unconfirmed, not gone — the new runner result explicitly means no matching PID or verified-exit tombstone was found, not that the process exited (cli/src/runner/run.ts:1049). Terminal-started sessions are registered only in the in-memory tracking map (cli/src/runner/run.ts:456), unlike runner-spawned sessions whose PID mapping is persisted. After a runner restart, a still-live terminal session can therefore returnunknown; if its session RPC is also temporarily missing during reconnect,hub/src/sync/syncEngine.ts:1734archives the row and leaves the process running.
Suggested fix:if (status === "still_alive" || status === "unknown") { throw new Error("Session process termination was not confirmed") }
Questions
- None.
Summary
Review mode: follow-up after new commits
- One Major finding: the fresh head removes the heartbeat pre-check, but still treats an explicitly ambiguous runner result as proof that archival is safe.
Testing
- Not run (automation; PR content was not executed).
- Replace the current
unknownarchival expectation with a regression test asserting that the session remains active and archival rejects. - GitHub checks at review time:
integrationfailed;testwas in progress. Failure logs were not yet available because the workflow was still running.
HAPI Bot
| // fallback exists to unblock — so treat it like the | ||
| // machine-offline branch above, not like a confirmed-alive | ||
| // `'still_alive'`. | ||
| if (status === 'still_alive') { |
There was a problem hiding this comment.
[MAJOR] Treat unknown as unconfirmed, not gone
unknown explicitly means the runner found neither a matching PID nor verified-exit evidence; it does not confirm process death. A live terminal-started session can reach this state after the runner restarts because the terminal webhook only populates pidToTrackedSession in memory (cli/src/runner/run.ts:456-486), while only runner-spawned PID mappings are persisted. If the live CLI session RPC is simultaneously missing during reconnect, this condition allows the row to be archived while the process continues running.
Suggested fix:
if (status === "still_alive" || status === "unknown") {
throw new Error("Session process termination was not confirmed")
}Please invert the current unknown test: archival must reject and the session must remain active.
Codex review caught that treating 'unknown' as archivable was itself unsafe: terminal-started sessions are tracked only in-memory (pidToTrackedSession), never persisted to the resume-process file unlike runner-spawned children. A still-alive terminal session can therefore return 'unknown' right after this exact runner process restarts and loses that in-memory tracking — indistinguishable, from this RPC alone, from a row this runner generation genuinely never knew about (the stale-row case 'unknown' was introduced for). Given that ambiguity can't be resolved from this RPC's response alone, treat 'unknown' conservatively — the same as 'still_alive' — matching every other stopRunnerSession call site in this file. The 'unknown' type distinction stays (it's still the runner's accurate contract, and fixes the pre-existing integration test's assertion), it just doesn't unblock archival on its own; that's now squarely tiann#842's reconciler territory, not this fallback's job.
|
Addressed the fourth finding: treating The |
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits
No ≥80%-confidence issues found in the fresh-head diff. Residual risk: the archive regression suite mocks RpcGateway; no end-to-end hub↔runner test exercises machine-level StopSession through Socket.IO on the stale-session-ID path.
Testing
- Not run (automation; PR content was not executed).
- Static diff check passed.
- GitHub checks at review time: integration failed; test and pr-review were pending. Failure logs were not yet available because the workflow run was still in progress.
HAPI Bot
1 participant
Summary
archiveSession's only kill path (rpcGateway.killSession, a session-scoped socket RPC) treated anyRpcTargetMissingErroras proof the CLI process was dead and unconditionally archived the row. That RPC target goes stale independently of the process — most notably whenSessionCache.mergeSessionsrotates the hub's canonical session id during a resume — so a missing target does not prove the process is gone.stopRunnerSessionRPC, which resolves the child by PID on the runner side and checks both the requested and confirmed session ids (already used this way by the PTY/Pi resume reconciliation paths). Only archive once that also reports the process gone; if it reportsstill_alive, surface an error instead of silently archiving a live process.machineId, or the machine RPC itself is unreachable, preserve the prior best-effort behavior (nothing stronger to check against).Fixes #1705
Test plan
hub/src/sync/syncEngineArchiveSession.test.tscover: still-alive (rejects, stays active), confirmed-gone (archives), no-known-machine (falls back to old behavior), machine-unreachable (best-effort archive as before)bun test src/sync/andbun test src/web/routes/sessions.test.tspass (pre-existing unrelated failures inloadServerSettings,TitleSuggestionService,resolveFcmConfigreproduce identically onmain)tsc --noEmitclean inhub/