diff --git a/src/daemon-client/__tests__/daemon-client-transport.test.ts b/src/daemon-client/__tests__/daemon-client-transport.test.ts index 83eb72b4bd..117ef21681 100644 --- a/src/daemon-client/__tests__/daemon-client-transport.test.ts +++ b/src/daemon-client/__tests__/daemon-client-transport.test.ts @@ -1,6 +1,6 @@ import assert from 'node:assert/strict'; import http from 'node:http'; -import { test } from 'vitest'; +import { test, vi } from 'vitest'; import { AppError } from '@agent-device/kernel/errors'; import { DAEMON_HTTP_INSTANCE_HEADER, @@ -222,6 +222,40 @@ test('a delayed restart health probe stops at the RPC deadline without retrying' } }); +test('a restart health probe cut short by the RPC deadline reports the deadline on a lagging clock', async (t) => { + if (await skipWhenLoopbackUnavailable(t)) return; + let rpcCount = 0; + let healthProbes = 0; + const server = http.createServer((req, res) => { + if (req.url === '/health') { + healthProbes += 1; + const delayedResponse = setTimeout(() => res.end('{}'), 1000); + res.on('close', () => clearTimeout(delayedResponse)); + return; + } + rpcCount += 1; + res.statusCode = 409; + res.setHeader(DAEMON_HTTP_INSTANCE_MISMATCH_HEADER, 'true'); + res.end(); + }); + // Timers start from the event loop's cached clock, so the probe's timer can fire while + // performance.now() is still short of the deadline. A frozen clock makes that gap certain. + const now = vi.spyOn(performance, 'now').mockReturnValue(performance.now()); + try { + const port = await listenOnLoopback(server); + await assert.rejects( + sendWithStaleInstance(port, 150), + (error: unknown) => + error instanceof AppError && error.details?.reason === 'daemon_transport_timeout', + ); + assert.equal(rpcCount, 1); + assert.equal(healthProbes, 1); + } finally { + now.mockRestore(); + await closeLoopbackServer(server); + } +}); + test('proxy forwards cached upstream identity and rejects a restarted upstream before dispatch', async (t) => { if (await skipWhenLoopbackUnavailable(t)) return; let upstreamInstance = 'upstream-one'; diff --git a/src/daemon-client/daemon-client-transport.ts b/src/daemon-client/daemon-client-transport.ts index d7ac9d2f43..03d0d71b98 100644 --- a/src/daemon-client/daemon-client-transport.ts +++ b/src/daemon-client/daemon-client-transport.ts @@ -42,6 +42,8 @@ export type RemoteDaemonHealth = { instanceId?: string; /** The daemon behind a proxy, as the proxy's health reported it. */ upstream?: RemoteDaemonHealthLink; + /** The probe ran out of its time budget before an answer, rather than failing outright. */ + timedOut?: true; }; type RemoteDaemonHealthLink = Pick< @@ -137,9 +139,12 @@ async function readDaemonHttpHealth( info.baseUrl ? REMOTE_DAEMON_HEALTHCHECK_TIMEOUT_MS : LOCAL_DAEMON_HEALTHCHECK_TIMEOUT_MS, probeTimeoutMs ?? Number.POSITIVE_INFINITY, ); - if (timeoutMs <= 0) return { reachable: false }; + if (timeoutMs <= 0) return { reachable: false, timedOut: true }; + const signal = AbortSignal.timeout(Math.ceil(timeoutMs)); return await new Promise((resolve) => { const headers = info.baseUrl ? buildDaemonHttpAuthHeaders(info.token) : {}; + const unreachable = (): RemoteDaemonHealth => + signal.aborted ? { reachable: false, timedOut: true } : { reachable: false }; const req = transport.request( { protocol: url.protocol, @@ -148,7 +153,7 @@ async function readDaemonHttpHealth( path: url.pathname + url.search, method: 'GET', timeout: timeoutMs, - signal: AbortSignal.timeout(Math.ceil(timeoutMs)), + signal, headers, }, (res) => { @@ -165,16 +170,16 @@ async function readDaemonHttpHealth( ...readHealthPayload(body), }); }); - res.on('error', () => resolve({ reachable: false })); - res.on('aborted', () => resolve({ reachable: false })); + res.on('error', () => resolve(unreachable())); + res.on('aborted', () => resolve(unreachable())); }, ); req.on('timeout', () => { req.destroy(); - resolve({ reachable: false }); + resolve({ reachable: false, timedOut: true }); }); req.on('error', () => { - resolve({ reachable: false }); + resolve(unreachable()); }); req.end(); }); @@ -255,6 +260,21 @@ async function retryAfterRemoteInstanceMismatch( deadline, ); const health = await readRemoteDaemonHealth(info, probeTimeoutMs); + // The probe's timer starts from the event loop's cached clock, so it can expire while + // performance.now() is still short of the deadline: a probe the RPC deadline capped that ran out + // of time is the RPC timing out. + if ( + health.timedOut && + timeoutMs !== undefined && + probeTimeoutMs !== undefined && + probeTimeoutMs <= REMOTE_DAEMON_HEALTHCHECK_TIMEOUT_MS + ) { + throw handleRequestTimeout({ + info, + statePaths, + ...timeoutRequestContext(req, true, timeoutMs), + }); + } const remainingMs = remainingRemoteRequestTimeoutMs(info, req, statePaths, timeoutMs, deadline); if (!health.reachable) { throw new AppError('COMMAND_FAILED', 'Remote daemon is unavailable', { diff --git a/test/wire-compat/ledger.json b/test/wire-compat/ledger.json index 351e440b00..a2ba44360a 100644 --- a/test/wire-compat/ledger.json +++ b/test/wire-compat/ledger.json @@ -63,9 +63,9 @@ "src/daemon-client/daemon-client-rpc.ts#rejectDaemonHttpRpcError": "sha256:83b0312fe88bc3b3497de799e23d8d14455f665b7cf880f4617cd62b181351cd", "src/daemon-client/daemon-client-rpc.ts#resolveDaemonHttpResult": "sha256:296f9d376ce67c8cb20209bdf1c59c2423ffcfa35b5565a99e70271263149048", "src/daemon-client/daemon-client-rpc.ts#toDaemonHttpRpcError": "sha256:888246763c48670e7da893054d025744654f8715c3b4906312617a2b5028316b", - "src/daemon-client/daemon-client-transport.ts#RemoteDaemonHealth": "sha256:df6b92e343a451a03b194e1e4aacac0c89b48a632723af24e087b377e73e05e3", + "src/daemon-client/daemon-client-transport.ts#RemoteDaemonHealth": "sha256:a3380ef1b8d85808111eb0a00533430cf64ea76d9d0c35596080077795380475", "src/daemon-client/daemon-client-transport.ts#RemoteDaemonHealthLink": "sha256:464809c9bb14b44d098781722e9e9e2ff044c45464e3c7c4cb5690886e857fdf", - "src/daemon-client/daemon-client-transport.ts#readDaemonHttpHealth": "sha256:6b64ae9b8e461a4b7dc1afb88465fcbaef908c31cc0be24499b2f440616e1cb5", + "src/daemon-client/daemon-client-transport.ts#readDaemonHttpHealth": "sha256:eef0e153eb6f0bc02175e464dd6d98559b500b65ea8c74ba2203cfef09ec09a0", "src/daemon-client/daemon-client-transport.ts#readHealthLink": "sha256:d7a84687d1c9b089460151a0532870e92a932d45f9567d248cea7accf8908593", "src/daemon-client/daemon-client-transport.ts#readHealthPayload": "sha256:4e85ffc3e35e02379c393e9312344757e003cf1f0ad9eb8d1f77d90c81c861f1", "src/daemon-client/daemon-client-transport.ts#readRemoteDaemonHealth": "sha256:bcefa89fbb7fcbd6fee1b5ecb217955b9eee8d6fd2ad175fb653011edbd199d9", @@ -267,11 +267,6 @@ "digest": "sha256:4e85ffc3e35e02379c393e9312344757e003cf1f0ad9eb8d1f77d90c81c861f1", "rationale": "#2198 slice B teaches the client to read the `upstream` link a proxy's /health already nests. The field is optional and additive: a daemon or proxy that does not send it parses exactly as before, and no request shape changes. readHealthPayload reads the same top-level fields through readHealthLink and additionally the nested `upstream` object when present; a payload without it yields the previous shape." }, - { - "declaration": "src/daemon-client/daemon-client-transport.ts#readDaemonHttpHealth", - "digest": "sha256:6b64ae9b8e461a4b7dc1afb88465fcbaef908c31cc0be24499b2f440616e1cb5", - "rationale": "#2650 gives every HTTP health probe a total-time cap (3s remote or 500ms local), including response-body reads. A restart retry also uses the RPC's remaining deadline. The health request and accepted payload fields are unchanged; stalled probes now end sooner." - }, { "declaration": "src/daemon-client/daemon-client-transport.ts#readRemoteDaemonHealth", "digest": "sha256:bcefa89fbb7fcbd6fee1b5ecb217955b9eee8d6fd2ad175fb653011edbd199d9", @@ -357,11 +352,6 @@ "digest": "sha256:4398710f0faac4dbe76ca448c1dd87ff6d062016c1f4e52f610f7a8729453688", "rationale": "#2650 adds an optional daemon instance ID to health metadata or reads it when present. Older peers ignore the added field, and new clients keep probing legacy peers without an instance ID; existing fields and RPC envelopes are unchanged." }, - { - "declaration": "src/daemon-client/daemon-client-transport.ts#RemoteDaemonHealth", - "digest": "sha256:df6b92e343a451a03b194e1e4aacac0c89b48a632723af24e087b377e73e05e3", - "rationale": "#2650 adds an optional daemon instance ID to health metadata or reads it when present. Older peers ignore the added field, and new clients keep probing legacy peers without an instance ID; existing fields and RPC envelopes are unchanged." - }, { "declaration": "src/daemon-client/daemon-client-transport.ts#RemoteDaemonHealthLink", "digest": "sha256:464809c9bb14b44d098781722e9e9e2ff044c45464e3c7c4cb5690886e857fdf", @@ -426,6 +416,16 @@ "declaration": "src/daemon-client/daemon-client-transport.ts#isRemoteInstanceMismatch", "digest": "sha256:6d4cfdd36bd44686d2a48990491faf63ba9117fb760f982fd72922c99be8daee", "rationale": "#2650 adds conditional RPC refusal and retry only for peers that advertise complete instance IDs. Released peers advertise none, so new clients keep per-command health probes and send their unchanged RPC; older clients omit the new request precondition and parse existing responses as before." + }, + { + "declaration": "src/daemon-client/daemon-client-transport.ts#RemoteDaemonHealth", + "digest": "sha256:a3380ef1b8d85808111eb0a00533430cf64ea76d9d0c35596080077795380475", + "rationale": "A restart retry must tell a probe that ran out of time from one that failed, so the client can report the RPC deadline instead of 'Remote daemon is unavailable'. `timedOut` is client-local: the prober sets it on its own timeout and abort paths and never reads it from a /health payload. The health request and the accepted payload fields are unchanged, so a released daemon or proxy is probed and parsed exactly as before." + }, + { + "declaration": "src/daemon-client/daemon-client-transport.ts#readDaemonHttpHealth", + "digest": "sha256:eef0e153eb6f0bc02175e464dd6d98559b500b65ea8c74ba2203cfef09ec09a0", + "rationale": "A restart retry must tell a probe that ran out of time from one that failed, so the client can report the RPC deadline instead of 'Remote daemon is unavailable'. `timedOut` is client-local: the prober sets it on its own timeout and abort paths and never reads it from a /health payload. The health request and the accepted payload fields are unchanged, so a released daemon or proxy is probed and parsed exactly as before." } ] }