-
-
Notifications
You must be signed in to change notification settings - Fork 315
fix(daemon): probe a live daemon again before replacing it as unreachable #3050
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
okwasniewski
wants to merge
5
commits into
callstack:main
Choose a base branch
from
okwasniewski:oskar/daemon-probe-survives-client-stall
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
ed3b8e6
fix(daemon): probe a live daemon again before replacing it as unreach…
okwasniewski 7b49dd3
test(daemon): cover the probe retry with a deterministic missed probe
okwasniewski 058b06a
fix(daemon): gate the probe retry on liveness, not the ps identity read
okwasniewski 57ed4a8
fix(daemon): ask client-transport reachability only when it decides t…
okwasniewski 1aeb7b9
test(daemon): pin the dead-pid replacement on a port the fresh daemon…
okwasniewski File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
280 changes: 280 additions & 0 deletions
280
src/daemon-client/__tests__/daemon-client-stalled-probe.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,280 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import fs from 'node:fs'; | ||
| import net from 'node:net'; | ||
| import path from 'node:path'; | ||
| import { test, vi } from 'vitest'; | ||
| import { runCmdBackground } from '@agent-device/host-kit/command'; | ||
| import { computeDaemonCodeSignature } from '@agent-device/host-kit/code-signature'; | ||
| import { | ||
| isProcessAlive, | ||
| readProcessCommand, | ||
| readProcessStartTime, | ||
| waitForProcessExit, | ||
| } from '@agent-device/host-kit/process'; | ||
| import { findProjectRoot, readVersion } from '@agent-device/host-kit/version'; | ||
| import { resolveDaemonPaths } from '../../daemon-resolution.ts'; | ||
| import { sendToDaemon } from '../daemon-client.ts'; | ||
| import { | ||
| closeLoopbackServer, | ||
| listenOnLoopback, | ||
| supportsLoopbackBind, | ||
| } from '../../__tests__/test-utils/loopback.ts'; | ||
| import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; | ||
|
|
||
| // The spawned stand-in's identity is read once and pinned: a second real `ps` can miss its | ||
| // deadline under suite load and misclassify the live process as gone. | ||
| const { mockReadProcessStartTime, mockReadProcessCommand } = vi.hoisted(() => ({ | ||
| mockReadProcessStartTime: vi.fn<(pid: number) => string | null | undefined>(), | ||
| mockReadProcessCommand: vi.fn<(pid: number) => string | null | undefined>(), | ||
| })); | ||
|
|
||
| // Every probe's answer, in order, with the port it asked; a test can also make the next one miss. | ||
| const { probeAnswers, probedPorts, mockMissNextProbe, mockEmitDiagnostic, mockSpawnDaemon } = | ||
| vi.hoisted(() => ({ | ||
| probeAnswers: [] as boolean[], | ||
| probedPorts: [] as (number | undefined)[], | ||
| mockMissNextProbe: { value: false }, | ||
| mockEmitDiagnostic: vi.fn(), | ||
| mockSpawnDaemon: vi.fn(), | ||
| })); | ||
|
|
||
| vi.mock('@agent-device/host-kit/diagnostics', async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import('@agent-device/host-kit/diagnostics')>(); | ||
| return { | ||
| ...actual, | ||
| emitDiagnostic: (...args: Parameters<typeof actual.emitDiagnostic>) => { | ||
| mockEmitDiagnostic(...args); | ||
| actual.emitDiagnostic(...args); | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| vi.mock('@agent-device/host-kit/command', async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import('@agent-device/host-kit/command')>(); | ||
| return { ...actual, runCmdDetachedMonitored: mockSpawnDaemon }; | ||
| }); | ||
|
|
||
| vi.mock('../daemon-client-transport.ts', async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import('../daemon-client-transport.ts')>(); | ||
| return { | ||
| ...actual, | ||
| canConnect: async (...args: Parameters<typeof actual.canConnect>) => { | ||
| const reachable = mockMissNextProbe.value ? false : await actual.canConnect(...args); | ||
| mockMissNextProbe.value = false; | ||
| probeAnswers.push(reachable); | ||
| probedPorts.push(args[0].port); | ||
| return reachable; | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| vi.mock('@agent-device/host-kit/process', async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import('@agent-device/host-kit/process')>(); | ||
| return { | ||
| ...actual, | ||
| readProcessStartTime: (pid: number) => | ||
| mockReadProcessStartTime(pid) ?? actual.readProcessStartTime(pid), | ||
| readProcessCommand: (pid: number) => | ||
| mockReadProcessCommand(pid) ?? actual.readProcessCommand(pid), | ||
| }; | ||
| }); | ||
|
|
||
| function resolveCurrentDaemonCodeSignature(): string { | ||
| const root = findProjectRoot(); | ||
| const distPath = path.join(root, 'dist', 'src', 'internal', 'daemon.js'); | ||
| const sourcePath = path.join(root, 'src', 'daemon.ts'); | ||
| const entryPath = | ||
| process.execArgv.includes('--experimental-strip-types') || !fs.existsSync(distPath) | ||
| ? sourcePath | ||
| : distPath; | ||
| return computeDaemonCodeSignature(entryPath, root); | ||
| } | ||
|
|
||
| type LiveStandIn = { stateDir: string; pid: number }; | ||
|
|
||
| /** | ||
| * Runs `body` against a daemon.json naming a live process that reads as an agent-device daemon | ||
| * and a loopback socket that answers every request, as a running daemon does. | ||
| */ | ||
| async function withLiveStandIn( | ||
| t: { skip: (reason: string) => void }, | ||
| body: (standIn: LiveStandIn) => Promise<void>, | ||
| ): Promise<void> { | ||
| if (!(await supportsLoopbackBind())) { | ||
| t.skip('loopback listeners are not permitted in this environment'); | ||
| return; | ||
| } | ||
| const stateDir = mkdtempForTestSync('agent-device-stalled-probe-'); | ||
| const root = mkdtempForTestSync('agent-device-stalled-probe-daemon-'); | ||
| const daemonDir = path.join(root, 'agent-device', 'dist', 'src', 'internal'); | ||
| const daemonScriptPath = path.join(daemonDir, 'daemon.js'); | ||
| fs.mkdirSync(daemonDir, { recursive: true }); | ||
| fs.writeFileSync(daemonScriptPath, 'setInterval(() => {}, 1000);\n', 'utf8'); | ||
| const daemonProcess = runCmdBackground(process.execPath, [daemonScriptPath], { | ||
| stdio: 'ignore', | ||
| allowFailure: true, | ||
| captureOutput: false, | ||
| }); | ||
| void daemonProcess.wait.catch(() => {}); | ||
| const pid = daemonProcess.child.pid; | ||
| assert.ok(pid, 'spawned child should have a pid'); | ||
| const server = net.createServer((socket) => { | ||
| let requestBody = ''; | ||
| socket.setEncoding('utf8'); | ||
| socket.on('data', (chunk) => { | ||
| requestBody += chunk; | ||
| if (!requestBody.includes('\n')) return; | ||
| socket.end(`${JSON.stringify({ ok: true, data: { via: 'live-daemon' } })}\n`); | ||
| }); | ||
| }); | ||
|
|
||
| try { | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
| const processStartTime = readProcessStartTime(pid) ?? undefined; | ||
| const command = readProcessCommand(pid); | ||
| if (command === null || processStartTime === undefined) { | ||
| t.skip('process command/start inspection is unavailable in this environment'); | ||
| return; | ||
| } | ||
| mockReadProcessStartTime.mockImplementation((queriedPid: number) => | ||
| queriedPid === pid ? processStartTime : undefined, | ||
| ); | ||
| mockReadProcessCommand.mockImplementation((queriedPid: number) => | ||
| queriedPid === pid ? command : undefined, | ||
| ); | ||
| const port = await listenOnLoopback(server); | ||
| const paths = resolveDaemonPaths(stateDir); | ||
| fs.mkdirSync(paths.baseDir, { recursive: true }); | ||
| fs.writeFileSync( | ||
| paths.infoPath, | ||
| `${JSON.stringify({ | ||
| port, | ||
| transport: 'socket', | ||
| token: 'local-secret', | ||
| pid, | ||
| version: readVersion(), | ||
| codeSignature: resolveCurrentDaemonCodeSignature(), | ||
| processStartTime, | ||
| })}\n`, | ||
| 'utf8', | ||
| ); | ||
| probeAnswers.length = 0; | ||
| probedPorts.length = 0; | ||
| mockEmitDiagnostic.mockClear(); | ||
| await body({ stateDir, pid }); | ||
| } finally { | ||
| mockMissNextProbe.value = false; | ||
| mockReadProcessStartTime.mockReset(); | ||
| mockReadProcessCommand.mockReset(); | ||
| await closeLoopbackServer(server); | ||
| if (isProcessAlive(pid)) { | ||
| process.kill(pid, 'SIGKILL'); | ||
| await waitForProcessExit(pid, 1_500); | ||
| } | ||
| fs.rmSync(stateDir, { recursive: true, force: true }); | ||
| fs.rmSync(root, { recursive: true, force: true }); | ||
| } | ||
| } | ||
|
|
||
| function sendSmoke(stateDir: string) { | ||
| return sendToDaemon({ | ||
| session: 'default', | ||
| command: 'stalled-probe-smoke', | ||
| positionals: [], | ||
| flags: { stateDir, daemonTransport: 'socket' }, | ||
| meta: { requestId: 'req-stalled-probe' }, | ||
| }); | ||
| } | ||
|
|
||
| test('sendToDaemon keeps a live daemon whose first probe missed', async (t) => { | ||
| await withLiveStandIn(t, async ({ stateDir, pid }) => { | ||
| mockMissNextProbe.value = true; | ||
|
|
||
| const response = await sendSmoke(stateDir); | ||
|
|
||
| assert.deepEqual(probeAnswers.slice(0, 2), [false, true]); | ||
| assert.deepEqual(response, { ok: true, data: { via: 'live-daemon' } }); | ||
| assert.equal(isProcessAlive(pid), true); | ||
| assert.ok( | ||
| mockEmitDiagnostic.mock.calls.some( | ||
| ([event]) => event.phase === 'daemon_probe_recovered' && event.data?.pid === pid, | ||
| ), | ||
| 'a recovered probe names the daemon it kept', | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| test('sendToDaemon replaces a daemon whose process is gone after a single probe', async (t) => { | ||
| if (!(await supportsLoopbackBind())) { | ||
| t.skip('loopback listeners are not permitted in this environment'); | ||
| return; | ||
| } | ||
| const stateDir = mkdtempForTestSync('agent-device-dead-probe-'); | ||
| const gone = runCmdBackground(process.execPath, ['-e', ''], { | ||
| stdio: 'ignore', | ||
| allowFailure: true, | ||
| captureOutput: false, | ||
| }); | ||
| await gone.wait.catch(() => {}); | ||
| const deadPid = gone.child.pid; | ||
| assert.ok(deadPid, 'spawned child should have a pid'); | ||
| const fresh = net.createServer((socket) => { | ||
| socket.setEncoding('utf8'); | ||
| socket.on('data', () => { | ||
| socket.end(`${JSON.stringify({ ok: true, data: { via: 'fresh-daemon' } })}\n`); | ||
| }); | ||
| }); | ||
| const writeInfo = (port: number, pid: number) => { | ||
| const paths = resolveDaemonPaths(stateDir); | ||
| fs.mkdirSync(paths.baseDir, { recursive: true }); | ||
| fs.writeFileSync( | ||
| paths.infoPath, | ||
| `${JSON.stringify({ | ||
| port, | ||
| transport: 'socket', | ||
| token: 'local-secret', | ||
| pid, | ||
| version: readVersion(), | ||
| codeSignature: resolveCurrentDaemonCodeSignature(), | ||
| processStartTime: readProcessStartTime(process.pid) ?? undefined, | ||
| })}\n`, | ||
| 'utf8', | ||
| ); | ||
| }; | ||
|
|
||
| try { | ||
| // Bound before the dead port is picked, so the port the dead daemon recorded cannot be | ||
| // handed back to the fresh one. | ||
| const freshPort = await listenOnLoopback(fresh); | ||
| const unused = net.createServer(); | ||
| const deadPort = await listenOnLoopback(unused); | ||
| await closeLoopbackServer(unused); | ||
| writeInfo(deadPort, deadPid); | ||
| mockSpawnDaemon.mockImplementation(() => { | ||
| writeInfo(freshPort, process.pid); | ||
| return { pid: process.pid, exited: new Promise(() => {}) }; | ||
| }); | ||
| probeAnswers.length = 0; | ||
| probedPorts.length = 0; | ||
| mockEmitDiagnostic.mockClear(); | ||
| if (isProcessAlive(deadPid)) { | ||
| t.skip('the host recycled the exited stand-in pid before the probe'); | ||
| return; | ||
| } | ||
|
|
||
| const response = await sendSmoke(stateDir); | ||
|
|
||
| assert.deepEqual(response, { ok: true, data: { via: 'fresh-daemon' } }); | ||
| assert.equal(mockSpawnDaemon.mock.calls.length, 1, 'the dead daemon was replaced'); | ||
| const deadProbes = probeAnswers.filter((_, index) => probedPorts[index] === deadPort); | ||
| assert.deepEqual(deadProbes, [false], 'a daemon whose pid is gone gets no patient retry'); | ||
| assert.equal( | ||
| mockEmitDiagnostic.mock.calls.some(([event]) => event.phase === 'daemon_probe_recovered'), | ||
|
okwasniewski marked this conversation as resolved.
|
||
| false, | ||
| ); | ||
| } finally { | ||
| mockSpawnDaemon.mockReset(); | ||
| await closeLoopbackServer(fresh); | ||
| fs.rmSync(stateDir, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.