mirror of
https://github.com/stablyai/orca.git
synced 2026-10-24 16:03:45 +00:00
fix(daemon): stop killing a daemon we merely failed to observe
Ten review rounds each found a different band where this branch replaced a daemon that origin/main would have kept, and every fix bought a new one. The reason is arithmetic, not carelessness: matching the old tolerance for a single probe costs about 25s, the classification clock has 15s to give, and funding the difference puts startup past the 60s fail-open once the kill ladder and the fork are paid. No assignment of those numbers is safe. So the residual stops being lethal. daemon-occupancy.ts always said it — "'unknown' is the residual, and it is not permission" — while daemon-init.ts fell through from unknown to killStaleDaemon. That fall-through is what made every millisecond of budget a correctness parameter. Now an unclassifiable daemon is held in degraded mode: existing terminals keep working, fresh ones run locally, and being wrong costs a degraded session instead of somebody's agent. Two exclusions, both about never holding something unrecoverable. A proven-dead endpoint is a cold start or a corpse, and holding one would hand every first launch a provider pointed at no daemon. 'rejected' answered and refused, so it can never be adopted and its sessions can never be reattached. The cost is real and deliberate: a wedged-but-empty daemon is no longer replaced at launch, so #8689 degrades to "restart it from Manage Sessions". Which only works if the user knows — and degraded mode was computed, plumbed through preload, and read by nothing. It now renders where the Restart button already lives, and says what it actually costs: new terminals close when you quit, and restarting ends whatever the host is still holding. Rejected on the way here: a background reclassifier (a permanently wedged daemon never answers, and recovery is already handled by degraded-daemon-fresh-spawn- routing.ts), letting accumulated process-table reads license a kill (its errors are systematic, so more samples agree rather than converge), and sidelining the daemon onto a renamed socket (verified working at the syscall level, then abandoned: the daemon's own endpoint-ownership watch reads the moved entry as lost and retires itself, precisely when it recovers).
This commit is contained in:
Neil
@@ -26,6 +26,25 @@ hands → probe once more → `rename` in one syscall → verify we kept it.
|
||||
- **Never collapse "can't tell" into "dead."** Only `connected` means occupied; only
|
||||
`refused`/`missing` prove death. A timeout or `EPERM` proves nothing and must decline — treating
|
||||
it as death deletes an endpoint still serving every terminal on the host.
|
||||
- **"Can't tell" does not license a kill at launch either.** When the launcher cannot establish
|
||||
what a health-check-failing daemon is hosting, it holds it in degraded mode rather than
|
||||
replacing it. Only the daemon itself can prove it is empty, over IPC; the process table may
|
||||
only ever *raise* a verdict toward "occupied", never lower one toward a kill.
|
||||
|
||||
Two exclusions, both about never holding something unrecoverable: an endpoint that is proven
|
||||
dead (a cold start has nothing to hold), and `rejected` (it answered and refused, so it can
|
||||
never be adopted and its sessions can never be reattached).
|
||||
|
||||
The cost is deliberate and known: a wedged-but-empty daemon is no longer replaced at launch,
|
||||
so #8689 degrades to "restart it from Manage Sessions" instead of being handled automatically.
|
||||
That was chosen over the alternative, which was killing daemons whose live agents we had
|
||||
merely failed to observe — unrecoverable, versus one click.
|
||||
|
||||
**Do not try to fix this by tuning the classification budget.** Ten review rounds each found a
|
||||
different timing band where a bounded classification kills a session an unbounded one keeps.
|
||||
Matching the old tolerance for a single probe costs more clock than the 60s startup fail-open
|
||||
leaves once the kill ladder and the fork are paid for. The budget is a latency bound, not a
|
||||
correctness parameter, and it must stay that way.
|
||||
- **`link` first, never an unconditional `rename`.** `rename` replaces whatever it finds, so it
|
||||
would let a starting daemon destroy a healthy one. `link` fails loudly and forces the liveness
|
||||
question.
|
||||
|
||||
@@ -3134,8 +3134,12 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => {
|
||||
expect(forkMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('replaces a permanently wedged daemon after the grace window is exhausted (#8689)', async () => {
|
||||
// Why: a socket that accepts connections but never answers hello was preserved forever (#8689); after grace it must be replaced.
|
||||
it('holds a permanently wedged daemon rather than killing what it might be hosting', async () => {
|
||||
// The trade this makes, deliberately: a socket that accepts connections but never answers
|
||||
// hello can no longer be replaced at launch, so a wedged-but-empty daemon stays until the
|
||||
// user restarts it (#8689 regresses to degraded mode). The alternative was killing a daemon
|
||||
// whose live agents we had simply failed to observe, and that loss is unrecoverable while
|
||||
// this one is one click from repaired.
|
||||
const mod = await importFresh()
|
||||
await mod.initDaemonPtyProvider()
|
||||
|
||||
@@ -3191,25 +3195,20 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => {
|
||||
try {
|
||||
await launcher('/fake/socket', '/fake/token')
|
||||
|
||||
expect(killStaleDaemonMock).toHaveBeenCalledWith(
|
||||
FAKE_RUNTIME_DIR,
|
||||
'/fake/socket',
|
||||
'/fake/token'
|
||||
)
|
||||
expect(forkMock).toHaveBeenCalled()
|
||||
// 1 adoption + 1 patient probe + the cheap retries + 1 post-fork adoption. The patient
|
||||
// probe comes first and is the point: retrying a four-second handshake never reaches a
|
||||
// daemon that consistently needs longer, and spending the clock on the cheap asks first
|
||||
// left the tolerant one unable to fund an answer.
|
||||
expect(daemonClientMock).toHaveBeenCalledTimes(3 + WEDGED_DAEMON_GRACE_RETRIES)
|
||||
// Why: this replace path used to kill the daemon with no log, so a post-hoc
|
||||
// reader could not tell it apart from an adoption; the verdict must be recorded.
|
||||
expect(killStaleDaemonMock).not.toHaveBeenCalled()
|
||||
expect(forkMock).not.toHaveBeenCalled()
|
||||
// 1 adoption + 1 patient probe + the cheap retries. No post-fork adoption, because
|
||||
// nothing was forked. The patient probe comes first and is the point: retrying a
|
||||
// four-second handshake never reaches a daemon that consistently needs longer, and
|
||||
// spending the clock on the cheap asks first left the tolerant one unable to fund
|
||||
// an answer.
|
||||
expect(daemonClientMock).toHaveBeenCalledTimes(2 + WEDGED_DAEMON_GRACE_RETRIES)
|
||||
// The verdict must still be recorded: holding without a log is indistinguishable from
|
||||
// a successful adoption to anyone reading the log afterwards.
|
||||
expect(warnSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Replacing daemon that failed the health check')
|
||||
)
|
||||
expect(warnSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining(`graceRetries=${WEDGED_DAEMON_GRACE_RETRIES}`)
|
||||
expect.stringContaining('holding a daemon that failed the health check')
|
||||
)
|
||||
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('Manage Sessions'))
|
||||
} finally {
|
||||
warnSpy.mockRestore()
|
||||
// Restore the answering default: clearAllMocks clears calls not impls, so the throwing impl would leak into later tests.
|
||||
@@ -3480,9 +3479,6 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => {
|
||||
checkDaemonHealthMock.mockResolvedValueOnce('unreachable')
|
||||
probeSocketExistsMock.mockReturnValue(true)
|
||||
netConnectMock.mockImplementation(stubAliveSocketConnect)
|
||||
forkMock.mockImplementationOnce(() => {
|
||||
throw new Error('stop after replacement decision')
|
||||
})
|
||||
|
||||
// Stand in for the hello timeout each probe really waits out: mocked probes return instantly,
|
||||
// so only a moving clock can exercise the budget without a slow (or flaky) real wait.
|
||||
@@ -3496,9 +3492,7 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => {
|
||||
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
|
||||
try {
|
||||
await expect(launcher('/fake/socket', '/fake/token')).rejects.toThrow(
|
||||
'stop after replacement decision'
|
||||
)
|
||||
await launcher('/fake/socket', '/fake/token')
|
||||
|
||||
const verdict = warnSpy.mock.calls
|
||||
.map((call) => String(call[0]))
|
||||
@@ -3513,6 +3507,94 @@ describe('daemon-init: runRestartDaemon (7-step sequence)', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('still replaces when the endpoint is proven dead, so a cold start is not held', async () => {
|
||||
// The regression holding most risks: 'unknown' is also what a cold start looks like, since
|
||||
// nothing answers when nothing is there. Holding then would hand every first launch a
|
||||
// provider pointed at no daemon. A missing socket is the discriminator.
|
||||
const mod = await importFresh()
|
||||
await mod.initDaemonPtyProvider()
|
||||
|
||||
const answeringDefault = function MockDaemonClient() {
|
||||
return {
|
||||
ensureConnected: vi.fn(async () => {}),
|
||||
request: vi.fn(async () => ({ sessions: [] })),
|
||||
disconnect: vi.fn()
|
||||
}
|
||||
}
|
||||
daemonClientMock.mockImplementation(function MockWedgedDaemonClient() {
|
||||
return {
|
||||
ensureConnected: vi.fn(async () => {
|
||||
throw new Error('Hello response timed out')
|
||||
}),
|
||||
request: vi.fn(),
|
||||
disconnect: vi.fn()
|
||||
}
|
||||
})
|
||||
|
||||
const launcher = spawnerInstances[0].launcher as (
|
||||
socketPath: string,
|
||||
tokenPath: string
|
||||
) => Promise<{ shutdown(): Promise<void> }>
|
||||
checkDaemonHealthMock.mockResolvedValueOnce('unreachable')
|
||||
probeSocketExistsMock.mockReturnValue(false)
|
||||
// Reaching the fork IS the assertion; throwing there stops before the spawn plumbing.
|
||||
forkMock.mockImplementationOnce(() => {
|
||||
throw new Error('reached the replacement fork')
|
||||
})
|
||||
|
||||
try {
|
||||
await expect(launcher('/fake/socket', '/fake/token')).rejects.toThrow(
|
||||
'reached the replacement fork'
|
||||
)
|
||||
} finally {
|
||||
daemonClientMock.mockImplementation(answeringDefault)
|
||||
}
|
||||
})
|
||||
|
||||
it('still replaces a daemon that refused the handshake', async () => {
|
||||
// 'rejected' answered and refused — bad token or foreign protocol — so it can never be
|
||||
// adopted and its sessions can never be reattached. Holding one would be permanent
|
||||
// degradation buying nothing, which is the opposite of the trade holding exists to make.
|
||||
const mod = await importFresh()
|
||||
await mod.initDaemonPtyProvider()
|
||||
|
||||
const answeringDefault = function MockDaemonClient() {
|
||||
return {
|
||||
ensureConnected: vi.fn(async () => {}),
|
||||
request: vi.fn(async () => ({ sessions: [] })),
|
||||
disconnect: vi.fn()
|
||||
}
|
||||
}
|
||||
daemonClientMock.mockImplementation(function MockRejectingDaemonClient() {
|
||||
return {
|
||||
ensureConnected: vi.fn(async () => {
|
||||
throw new Error('Hello response timed out')
|
||||
}),
|
||||
request: vi.fn(),
|
||||
disconnect: vi.fn()
|
||||
}
|
||||
})
|
||||
|
||||
const launcher = spawnerInstances[0].launcher as (
|
||||
socketPath: string,
|
||||
tokenPath: string
|
||||
) => Promise<{ shutdown(): Promise<void> }>
|
||||
checkDaemonHealthMock.mockResolvedValueOnce('rejected')
|
||||
probeSocketExistsMock.mockReturnValue(true)
|
||||
netConnectMock.mockImplementation(stubAliveSocketConnect)
|
||||
forkMock.mockImplementationOnce(() => {
|
||||
throw new Error('reached the replacement fork')
|
||||
})
|
||||
|
||||
try {
|
||||
await expect(launcher('/fake/socket', '/fake/token')).rejects.toThrow(
|
||||
'reached the replacement fork'
|
||||
)
|
||||
} finally {
|
||||
daemonClientMock.mockImplementation(answeringDefault)
|
||||
}
|
||||
})
|
||||
|
||||
it('preserves a daemon that drains on the last probe the budget allows', async () => {
|
||||
// Why the clock is frozen: the probes here are instantaneous, so without pinning time the
|
||||
// wall-clock ceiling would never bind and this would assert a retry depth the real code
|
||||
|
||||
@@ -712,6 +712,28 @@ function createOutOfProcessLauncher(
|
||||
)
|
||||
return preserveDaemon()
|
||||
}
|
||||
// 'unknown' is not permission to kill. Everything above has failed to establish what
|
||||
// this daemon is hosting, and killing it on that basis is what destroyed live agents.
|
||||
// Bounded classification cannot be made safe by budgeting — matching main's tolerance
|
||||
// for one ask costs more clock than the 60s fail-open leaves — so the residual stops
|
||||
// being lethal instead. Being wrong now costs a degraded session, not an agent.
|
||||
//
|
||||
// Two exclusions, both about not holding something that can never be recovered:
|
||||
// - a proven-dead endpoint is a cold start or a corpse; there is nothing to hold, and
|
||||
// holding would hand every first launch a provider pointed at no daemon. Read fresh
|
||||
// here rather than reused, because the daemon can die during the grace window.
|
||||
// - 'rejected' answered and refused the handshake, so it can never be adopted and its
|
||||
// sessions can never be reattached. Holding one is permanently degraded for nothing.
|
||||
if (
|
||||
occupancy.state === 'unknown' &&
|
||||
health !== 'rejected' &&
|
||||
!endpointIsProvenDead(await probeSocketConnect(socketPath))
|
||||
) {
|
||||
console.warn(
|
||||
`[daemon] DEGRADED MODE: holding a daemon that failed the health check (health=${health}, graceRetries=${graceRetry}) because we could not establish whether it still owns live terminals. Replacing it would end them if it does. Fresh terminals run on the local provider WITHOUT daemon persistence until it recovers or you restart it (Manage Sessions → Restart).`
|
||||
)
|
||||
return holdIncumbentDaemon()
|
||||
}
|
||||
// Why: a cold start reaches this same fall-through with nothing to replace, so stay
|
||||
// quiet unless something actually answered — a verified count, a socket that survived
|
||||
// a grace retry, or a refused hello.
|
||||
|
||||
@@ -14,9 +14,11 @@ import {
|
||||
*
|
||||
* 'unknown' is not "empty" — it means the process table could not be read, or did not
|
||||
* contain the daemon at all. It is not evidence of absence, and it is deliberately not
|
||||
* evidence of presence either: the only caller preserves on 'owns-live-ptys' alone and
|
||||
* treats 'unknown' as the pre-existing behavior, so that a permanently wedged daemon with
|
||||
* nothing to lose stays replaceable (#8689). Widening that is a separate decision.
|
||||
* evidence of presence either: the only caller raises to preserve on 'owns-live-ptys' alone.
|
||||
*
|
||||
* What 'unknown' costs changed with the launcher: it no longer falls through to a kill, it
|
||||
* holds the daemon in degraded mode. So this module's job is now to spare the user that
|
||||
* degradation where it safely can, not to stand between them and a dead agent.
|
||||
*/
|
||||
export type DaemonPtyOwnership = 'owns-live-ptys' | 'no-live-ptys' | 'unknown'
|
||||
|
||||
@@ -192,9 +194,11 @@ export async function inspectDaemonPtyOwnership(
|
||||
// has — forkpty makes it a session leader — and Windows has no equivalent, so the branch
|
||||
// that lived here could only count descendants. That reads a wedged daemon's orphaned
|
||||
// conpty hosts as live work, since ClosePseudoConsole runs on the daemon's own JS thread
|
||||
// and a daemon too wedged to answer is too wedged to reap them. Holding on those would make
|
||||
// a wedged, empty daemon unreplaceable forever (#8689), so Windows keeps the behaviour it
|
||||
// had before this change rather than trading one failure mode for a worse one.
|
||||
// and a daemon too wedged to answer is too wedged to reap them.
|
||||
//
|
||||
// Abstaining costs Windows nothing it had: this evidence can only ever raise 'unknown' to
|
||||
// 'occupied', and both already hold the daemon. A verdict here would only let Windows print
|
||||
// the more accurate of two identical outcomes, which is not worth guessing for.
|
||||
if (platform === 'win32') {
|
||||
return 'unknown'
|
||||
}
|
||||
|
||||
@@ -0,0 +1,46 @@
|
||||
import React from 'react'
|
||||
import { renderToStaticMarkup } from 'react-dom/server'
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { DaemonDegradedNotice } from './DaemonDegradedNotice'
|
||||
|
||||
function render(props: Partial<React.ComponentProps<typeof DaemonDegradedNotice>> = {}): string {
|
||||
return renderToStaticMarkup(
|
||||
React.createElement(DaemonDegradedNotice, {
|
||||
degraded: true,
|
||||
isBusy: false,
|
||||
onRestartDaemon: vi.fn(),
|
||||
...props
|
||||
})
|
||||
)
|
||||
}
|
||||
|
||||
describe('DaemonDegradedNotice', () => {
|
||||
it('renders nothing when the daemon is healthy', () => {
|
||||
// The common case by far; a notice that shows up here would train the user to ignore it.
|
||||
expect(render({ degraded: false })).toBe('')
|
||||
})
|
||||
|
||||
it('warns that new terminals will not survive quitting', () => {
|
||||
// The consequence the user actually needs, not the mechanism. Degraded mode's real cost is
|
||||
// that a terminal opened now disappears on quit, and nothing else in the app says so.
|
||||
const html = render()
|
||||
expect(html).toContain('role="alert"')
|
||||
expect(html).toMatch(/aren’t being saved/)
|
||||
expect(html).toMatch(/close when you quit/)
|
||||
})
|
||||
|
||||
it('says the remedy also ends whatever the host is still holding', () => {
|
||||
// Restarting is not free: it kills the sessions this whole change exists to protect. A
|
||||
// remedy offered without its cost is how a user loses agents by clicking the helpful button.
|
||||
expect(render()).toMatch(/ends any session the host is still holding/)
|
||||
})
|
||||
|
||||
it('offers the restart action, disabled while another daemon action runs', () => {
|
||||
// Matched as an attribute, not a substring: the button's utility classes contain
|
||||
// `disabled:opacity-50`, so a contains-check passes whether or not it is really disabled.
|
||||
const disabledAttribute = /<button[^>]*\sdisabled[=>]/
|
||||
expect(render()).toContain('Restart host')
|
||||
expect(render({ isBusy: true })).toMatch(disabledAttribute)
|
||||
expect(render({ isBusy: false })).not.toMatch(disabledAttribute)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,53 @@
|
||||
import { TriangleAlert } from 'lucide-react'
|
||||
import { Button } from '../ui/button'
|
||||
import { translate } from '@/i18n/i18n'
|
||||
|
||||
/**
|
||||
* Degraded mode used to be rare and transient, so a console warning was enough. It is now the
|
||||
* settled outcome for a daemon the launcher could not classify — it holds one rather than
|
||||
* killing terminals it might still be hosting — which makes it permanent until the user acts.
|
||||
* A permanent state nothing renders is one the user cannot act on.
|
||||
*/
|
||||
export function DaemonDegradedNotice(props: {
|
||||
degraded: boolean
|
||||
isBusy: boolean
|
||||
onRestartDaemon: () => void
|
||||
}): React.JSX.Element | null {
|
||||
if (!props.degraded) {
|
||||
return null
|
||||
}
|
||||
|
||||
return (
|
||||
<div
|
||||
role="alert"
|
||||
className="flex items-start justify-between gap-4 rounded-lg border border-amber-500/40 bg-amber-500/10 px-4 py-3 text-amber-700 dark:text-amber-300"
|
||||
>
|
||||
<div className="flex min-w-0 items-start gap-2.5">
|
||||
<TriangleAlert className="mt-0.5 size-4 shrink-0" />
|
||||
<div className="min-w-0 space-y-1">
|
||||
<p className="text-sm font-medium">
|
||||
{translate(
|
||||
'components.settings.DaemonDegradedNotice.title',
|
||||
'New terminals aren’t being saved'
|
||||
)}
|
||||
</p>
|
||||
<p className="text-sm opacity-90">
|
||||
{translate(
|
||||
'components.settings.DaemonDegradedNotice.body',
|
||||
'The terminal host stopped responding, so Orca kept it rather than ending anything it may still be running. Terminals already open keep working. New ones run outside it and will close when you quit Orca. Restarting the host fixes this — it also ends any session the host is still holding.'
|
||||
)}
|
||||
</p>
|
||||
</div>
|
||||
</div>
|
||||
<Button
|
||||
variant="outline"
|
||||
size="sm"
|
||||
className="shrink-0"
|
||||
disabled={props.isBusy}
|
||||
onClick={props.onRestartDaemon}
|
||||
>
|
||||
{translate('components.settings.DaemonDegradedNotice.action', 'Restart host')}
|
||||
</Button>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
@@ -8,6 +8,7 @@ import { activateAndRevealWorktree } from '@/lib/worktree-activation'
|
||||
import { activateTabAndFocusPane } from '@/lib/activate-tab-and-focus-pane'
|
||||
import { useDaemonActions, DaemonActionDialog } from '../shared/useDaemonActions'
|
||||
import { ManageSessionKillDialog } from './ManageSessionKillDialog'
|
||||
import { DaemonDegradedNotice } from './DaemonDegradedNotice'
|
||||
import { ManageSessionsTable } from './ManageSessionsTable'
|
||||
import { notifyDaemonSessionInventoryInvalidated } from '../status-bar/daemon-session-inventory-invalidation'
|
||||
import {
|
||||
@@ -20,6 +21,7 @@ type ConfirmKind = 'killOne'
|
||||
|
||||
export function ManageSessionsSection(): React.JSX.Element {
|
||||
const [sessions, setSessions] = useState<PtyManagementSession[]>([])
|
||||
const [isDaemonDegraded, setIsDaemonDegraded] = useState(false)
|
||||
const [isRefreshing, setIsRefreshing] = useState(true)
|
||||
const [hasLoadedOnce, setHasLoadedOnce] = useState(false)
|
||||
const [pendingKillSession, setPendingKillSession] = useState<PtyManagementSession | null>(null)
|
||||
@@ -81,6 +83,7 @@ export function ManageSessionsSection(): React.JSX.Element {
|
||||
if (!isMounted.current || mutationInFlight.current) {
|
||||
return result.sessions
|
||||
}
|
||||
setIsDaemonDegraded(result.degraded === true)
|
||||
setSessions(result.sessions)
|
||||
return result.sessions
|
||||
} catch (err) {
|
||||
@@ -218,6 +221,11 @@ export function ManageSessionsSection(): React.JSX.Element {
|
||||
showManageSessionsButton={false}
|
||||
refreshRevision={attributionRefreshRevision}
|
||||
/>
|
||||
<DaemonDegradedNotice
|
||||
degraded={isDaemonDegraded}
|
||||
isBusy={isBusy}
|
||||
onRestartDaemon={() => daemonActions.setPending('restart')}
|
||||
/>
|
||||
<ManageSessionsTable
|
||||
sessions={sessions}
|
||||
hasLoadedOnce={hasLoadedOnce}
|
||||
|
||||
Reference in New Issue
Block a user