From cfb5e05fb93cfdc1e4917aa06a4561bbcaac691b Mon Sep 17 00:00:00 2001 From: Volodymyr Vreshch Date: Wed, 8 Jul 2026 00:03:18 +0200 Subject: [PATCH] fix(net): offline hangs, stderr discipline, loopback callback bind --- src/cli.ts | 2 +- src/commands/status.ts | 2 +- src/lib/api.test.ts | 14 +++++++ src/lib/api.ts | 11 ++++- src/lib/callback-server.test.ts | 12 ++++++ src/lib/callback-server.ts | 3 +- src/lib/http.test.ts | 13 ++++++ src/lib/http.ts | 39 ++++++++++++++++++ src/lib/oauth.test.ts | 14 +++++++ src/lib/oauth.ts | 9 +++- src/lib/status-info.test.ts | 73 ++++++++++++++++++--------------- src/lib/status-info.ts | 11 +++-- src/lib/update-cache.ts | 41 +++++------------- src/lib/update-check.test.ts | 42 ++++++++----------- src/lib/update-check.ts | 24 +++++------ 15 files changed, 195 insertions(+), 115 deletions(-) create mode 100644 src/lib/http.test.ts create mode 100644 src/lib/http.ts diff --git a/src/cli.ts b/src/cli.ts index 00d9cbb..bed8bb5 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -34,7 +34,7 @@ program.hook('postAction', (_thisCommand, actionCommand) => { if (name === 'status' || name === 'update') return; if (!actionCommand.opts()['json']) { const hint = updateHint(); - if (hint) console.log(chalk.dim(hint)); + if (hint) console.error(chalk.dim(hint)); // stderr: never corrupt piped stdout } void refreshUpdateCache(); }); diff --git a/src/commands/status.ts b/src/commands/status.ts index 1e8f542..1f1246d 100644 --- a/src/commands/status.ts +++ b/src/commands/status.ts @@ -52,7 +52,7 @@ const warnDaemonMismatch = async (): Promise => { const h = await health(resolvePort()); if (!h) return; const notice = mismatchNotice(h.version); - if (notice) console.log(chalk.yellow(notice)); + if (notice) console.error(chalk.yellow(notice)); // diagnostics go to stderr }; export const runStatus = async (opts: { json?: boolean } = {}): Promise => { diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts index 3d74246..7d67294 100644 --- a/src/lib/api.test.ts +++ b/src/lib/api.test.ts @@ -38,6 +38,7 @@ describe('authedGet', () => { expect(result.ok).toBe(true); expect(fetchMock).toHaveBeenCalledWith('https://x.example/me', { headers: { authorization: 'Bearer old-token' }, + redirect: 'manual', }); }); @@ -79,6 +80,18 @@ describe('authedGet', () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue(jsonResponse(500, {}))); await expect(authedGet(makeAuth(), target, 'https://x.example/x')).rejects.toThrow('500'); }); + + it('refuses to follow a redirect and reports its status', async () => { + const fetchMock = vi.fn().mockResolvedValue(jsonResponse(302, {})); + vi.stubGlobal('fetch', fetchMock); + await expect(authedGet(makeAuth(), target, 'https://x.example/me')).rejects.toThrow( + 'refused redirect (302)' + ); + expect(fetchMock).toHaveBeenCalledWith('https://x.example/me', { + headers: { authorization: 'Bearer old-token' }, + redirect: 'manual', + }); + }); }); describe('authedPost', () => { @@ -107,6 +120,7 @@ describe('authedPost', () => { method: 'POST', headers: { authorization: 'Bearer old-token', 'content-type': 'application/json' }, body: JSON.stringify({ name: 'acct', channel: 'couch' }), + redirect: 'manual', }); }); diff --git a/src/lib/api.ts b/src/lib/api.ts index 3d0c1db..2bdb3ac 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -46,12 +46,20 @@ export const currentBearer = async ( return auth.tokens.accessToken; }; +// redirect: 'manual' so the bearer is never replayed to a redirect target; any 3xx is an error. +const isRedirect = (res: Response): boolean => + res.type === 'opaqueredirect' || (res.status >= 300 && res.status < 400); + export const authedGet = async (auth: AuthState, links: Links, url: string): Promise => { const call = (): Promise => - fetch(url, { headers: { authorization: `Bearer ${auth.tokens.accessToken}` } }); + fetch(url, { + headers: { authorization: `Bearer ${auth.tokens.accessToken}` }, + redirect: 'manual', + }); let res = await call(); if (res.status === 401 && (await tryRefresh(auth, links))) res = await call(); if (res.status === 401) throw new AuthRequiredError('session expired'); + if (isRedirect(res)) throw new Error(`GET ${url} refused redirect (${res.status})`); if (!res.ok) throw new Error(`GET ${url} failed (${res.status})`); return (await res.json()) as T; }; @@ -72,6 +80,7 @@ export const authedPost = async ( 'content-type': 'application/json', }, body: JSON.stringify(body), + redirect: 'manual', }); let res = await call(); if (res.status === 401 && (await tryRefresh(auth, links))) res = await call(); diff --git a/src/lib/callback-server.test.ts b/src/lib/callback-server.test.ts index 1d826e3..9b8376a 100644 --- a/src/lib/callback-server.test.ts +++ b/src/lib/callback-server.test.ts @@ -63,6 +63,18 @@ describe('startCallbackServer', () => { } }); + it('binds loopback so the callback is reachable on 127.0.0.1', async () => { + const server = await startCallbackServer('state-1'); + try { + const port = new URL(server.redirectUri).port; + const res = await fetch(`http://127.0.0.1:${port}/callback?code=code-4&state=state-1`); + expect(res.status).toBe(200); + await expect(server.waitForCode(1000)).resolves.toBe('code-4'); + } finally { + server.close(); + } + }); + it('times out when no callback arrives', async () => { const server = await startCallbackServer('s'); try { diff --git a/src/lib/callback-server.ts b/src/lib/callback-server.ts index c96ebe9..febf2fc 100644 --- a/src/lib/callback-server.ts +++ b/src/lib/callback-server.ts @@ -54,7 +54,8 @@ export const startCallbackServer = (expectedState: string): Promise { + // Bind loopback only so the one-shot sign-in callback is never LAN-reachable. + server.listen(0, '127.0.0.1', () => { const address = server.address(); const port = typeof address === 'object' && address ? address.port : 0; resolveServer({ diff --git a/src/lib/http.test.ts b/src/lib/http.test.ts new file mode 100644 index 0000000..26cbd3c --- /dev/null +++ b/src/lib/http.test.ts @@ -0,0 +1,13 @@ +import { describe, expect, it } from 'vitest'; +import { fetchJsonUnref } from './http.js'; + +describe('fetchJsonUnref', () => { + it('resolves null within the timeout bound on an unreachable host', async () => { + const started = Date.now(); + // 192.0.2.1 (TEST-NET-1) either blackholes (the destroy timer fires) or refuses fast. + const result = await fetchJsonUnref('https://192.0.2.1/x', 500); + expect(result).toBeNull(); + // Generous CI bound (timers stretch under load); still well under undici's ~10s floor. + expect(Date.now() - started).toBeLessThan(8000); + }); +}); diff --git a/src/lib/http.ts b/src/lib/http.ts new file mode 100644 index 0000000..d87be83 --- /dev/null +++ b/src/lib/http.ts @@ -0,0 +1,39 @@ +import { get } from 'node:https'; + +export interface UnrefResponse { + ok: boolean; + status: number; + json: unknown; +} + +// node:https instead of global fetch: undici keeps a ref'd ~10s connect timer alive even after +// its AbortSignal fires, stalling process exit on an unreachable network. req.destroy() from an +// unref'd timer caps the whole attempt and frees the event loop the moment it settles. Never +// throws: an unreachable host or malformed body resolves null (unreachable) or json:null. +export const fetchJsonUnref = (url: string, timeoutMs: number): Promise => + new Promise((resolve) => { + const req = get(url, { headers: { Accept: 'application/json' } }, (res) => { + let body = ''; + res.setEncoding('utf8'); + res.on('data', (chunk: string) => { + body += chunk; + }); + res.on('end', () => { + const status = res.statusCode ?? 0; + let json: unknown = null; + try { + json = body ? JSON.parse(body) : null; + } catch { + json = null; + } + resolve({ ok: status >= 200 && status < 300, status, json }); + }); + }); + const timer = setTimeout(() => req.destroy(), timeoutMs); + timer.unref(); + req.once('error', () => resolve(null)); + req.once('close', () => { + clearTimeout(timer); + resolve(null); // no-op when already resolved + }); + }); diff --git a/src/lib/oauth.test.ts b/src/lib/oauth.test.ts index c239aff..8f79693 100644 --- a/src/lib/oauth.test.ts +++ b/src/lib/oauth.test.ts @@ -144,4 +144,18 @@ describe('revokeToken', () => { await expect(revokeToken(baseUrl, 'at')).resolves.toBeUndefined(); await expect(revokeToken('http://127.0.0.1:1', 'at')).resolves.toBeUndefined(); }); + + it('aborts a stalled revoke within the timeout instead of hanging', async () => { + const hang = createServer(() => { + // never responds: the AbortSignal.timeout must cap the request + }); + await new Promise((resolve) => hang.listen(0, '127.0.0.1', resolve)); + const addr = hang.address(); + const url = `http://127.0.0.1:${typeof addr === 'object' && addr ? addr.port : 0}`; + const started = Date.now(); + await expect(revokeToken(url, 'at', 50)).resolves.toBeUndefined(); + expect(Date.now() - started).toBeLessThan(2000); + hang.closeAllConnections(); + hang.close(); + }); }); diff --git a/src/lib/oauth.ts b/src/lib/oauth.ts index ff72321..a870944 100644 --- a/src/lib/oauth.ts +++ b/src/lib/oauth.ts @@ -100,12 +100,19 @@ export const refreshTokens = ( client_id: clientId, }); -export const revokeToken = async (authUrl: string, token: string): Promise => { +// AbortSignal.timeout caps a stalled revoke on a packet-drop network; residual undici keepalive is +// acceptable here since --disconnect exits right after and errors are swallowed anyway. +export const revokeToken = async ( + authUrl: string, + token: string, + timeoutMs = 3000 +): Promise => { try { await fetch(`${authUrl}${REVOKE_PATH}`, { method: 'POST', headers: { 'content-type': 'application/x-www-form-urlencoded' }, body: new URLSearchParams({ token }).toString(), + signal: AbortSignal.timeout(timeoutMs), }); } catch { // best-effort: local credentials are removed regardless diff --git a/src/lib/status-info.test.ts b/src/lib/status-info.test.ts index bffbca5..a7a2e81 100644 --- a/src/lib/status-info.test.ts +++ b/src/lib/status-info.test.ts @@ -1,7 +1,11 @@ -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { type AuthState } from './config.js'; +import { fetchJsonUnref } from './http.js'; import { gatherStatus } from './status-info.js'; +vi.mock('./http.js', () => ({ fetchJsonUnref: vi.fn() })); +const httpMock = vi.mocked(fetchJsonUnref); + const auth: AuthState = { siteFqdn: 'dev.agentage.io', clientId: 'c1', @@ -11,25 +15,27 @@ const auth: AuthState = { const jsonResponse = (status: number, body: unknown): Response => new Response(JSON.stringify(body), { status, headers: { 'content-type': 'application/json' } }); -const stubFetch = (routes: Record Response>): void => { - // The update check now reads the npm registry; default it to "current" so it never adds noise. - const all = { 'cli/latest': () => jsonResponse(200, { version: '0.0.0' }), ...routes }; - vi.stubGlobal( - 'fetch', - vi.fn((url: string) => { - for (const [suffix, response] of Object.entries(all)) { - if (url.endsWith(suffix)) return Promise.resolve(response()); - } - return Promise.reject(new Error(`unmatched url: ${url}`)); - }) - ); +// health + the npm-registry update check go through fetchJsonUnref (node:https); introspection +// (get-session) still uses global fetch via authedGet. +const stubHttp = (health: 'reachable' | 'unreachable'): void => { + httpMock.mockImplementation((url: string) => { + if (url.endsWith('/latest')) + return Promise.resolve({ ok: true, status: 200, json: { version: '0.0.0' } }); + if (url.endsWith('/health')) + return Promise.resolve(health === 'reachable' ? { ok: true, status: 200, json: {} } : null); + return Promise.resolve(null); + }); }; -afterEach(() => vi.unstubAllGlobals()); +beforeEach(() => stubHttp('reachable')); + +afterEach(() => { + httpMock.mockReset(); + vi.unstubAllGlobals(); +}); describe('gatherStatus', () => { it('reports a degraded status when not signed in', async () => { - stubFetch({ '/health': () => jsonResponse(200, { ok: true }) }); const report = await gatherStatus(null, 'dev.agentage.io'); expect(report.env).toBe('development'); expect(report.auth.signedIn).toBe(false); @@ -39,26 +45,27 @@ describe('gatherStatus', () => { }); it('marks the endpoint unreachable on network failure', async () => { - vi.stubGlobal('fetch', vi.fn().mockRejectedValue(new Error('refused'))); + stubHttp('unreachable'); const report = await gatherStatus(null, 'agentage.io'); expect(report.endpoint.reachable).toBe(false); }); it('reports signed-in with token expiry when introspection succeeds', async () => { - stubFetch({ - '/health': () => jsonResponse(200, {}), - '/get-session': () => - jsonResponse(200, { userId: 'u1', accessTokenExpiresAt: '2026-06-12T20:00:00Z' }), - }); + vi.stubGlobal( + 'fetch', + vi.fn(async () => + jsonResponse(200, { userId: 'u1', accessTokenExpiresAt: '2026-06-12T20:00:00Z' }) + ) + ); const report = await gatherStatus(auth, 'dev.agentage.io'); expect(report.auth).toEqual({ signedIn: true, tokenExpiresAt: '2026-06-12T20:00:00Z' }); }); it('treats a 200 + null session as signed-out instead of crashing', async () => { - stubFetch({ - '/health': () => jsonResponse(200, {}), - '/get-session': () => jsonResponse(200, null), - }); + vi.stubGlobal( + 'fetch', + vi.fn(async () => jsonResponse(200, null)) + ); const report = await gatherStatus(auth, 'dev.agentage.io'); expect(report.auth.signedIn).toBe(false); expect(report.auth.note).toContain('agentage setup'); @@ -66,20 +73,20 @@ describe('gatherStatus', () => { }); it('downgrades to signed-out with a hint when the token is rejected', async () => { - stubFetch({ - '/health': () => jsonResponse(200, {}), - '/get-session': () => jsonResponse(401, {}), - }); + vi.stubGlobal( + 'fetch', + vi.fn(async () => jsonResponse(401, {})) + ); const report = await gatherStatus(auth, 'dev.agentage.io'); expect(report.auth.signedIn).toBe(false); expect(report.auth.note).toContain('session expired'); }); it('reports verification failures without claiming signed-in', async () => { - stubFetch({ - '/health': () => jsonResponse(200, {}), - '/get-session': () => jsonResponse(500, {}), - }); + vi.stubGlobal( + 'fetch', + vi.fn(async () => jsonResponse(500, {})) + ); const report = await gatherStatus(auth, 'dev.agentage.io'); expect(report.auth.signedIn).toBe(false); expect(report.auth.note).toContain('could not verify session'); diff --git a/src/lib/status-info.ts b/src/lib/status-info.ts index 22f3e0d..b87d197 100644 --- a/src/lib/status-info.ts +++ b/src/lib/status-info.ts @@ -1,5 +1,6 @@ import { AuthRequiredError, introspectToken } from './api.js'; import { type AuthState } from './config.js'; +import { fetchJsonUnref } from './http.js'; import { environment, links, type Env } from './origins.js'; import { checkForUpdate, type UpdateInfo } from './update-check.js'; import { VERSION } from '../utils/version.js'; @@ -13,13 +14,11 @@ export interface StatusReport { update: UpdateInfo; } +// node:https via fetchJsonUnref, not global fetch: undici's ref'd connect timer keeps the process +// alive ~10s after an aborted request on a packet-drop network, stalling `status` exit. const checkEndpoint = async (apiUrl: string): Promise => { - try { - const res = await fetch(`${apiUrl}/health`, { signal: AbortSignal.timeout(3000) }); - return res.ok; - } catch { - return false; - } + const res = await fetchJsonUnref(`${apiUrl}/health`, 3000); + return res?.ok ?? false; }; export const gatherStatus = async (auth: AuthState | null, fqdn: string): Promise => { diff --git a/src/lib/update-cache.ts b/src/lib/update-cache.ts index 8be5ba1..0a3fb43 100644 --- a/src/lib/update-cache.ts +++ b/src/lib/update-cache.ts @@ -1,7 +1,7 @@ import { readFileSync, writeFileSync } from 'node:fs'; -import { get } from 'node:https'; import { join } from 'node:path'; import { ensureConfigDir, getConfigDir } from './config.js'; +import { fetchJsonUnref } from './http.js'; import { compareVersions, INSTALL_HINT, REGISTRY_URL } from './update-check.js'; import { VERSION } from '../utils/version.js'; @@ -40,38 +40,17 @@ export const updateHint = (): string | null => { : null; }; -// node:https instead of global fetch: undici keeps a ref'd ~10s connect timer alive even after -// its AbortSignal fires, stalling process exit on an unreachable network. req.destroy() from an -// unref'd timer caps the whole attempt and frees the event loop the moment it settles. -export const fetchLatestVersion = ( +// Shares fetchJsonUnref (node:https, unref'd timer) so an offline background check never stalls +// process exit the way an aborted global fetch does. +export const fetchLatestVersion = async ( timeoutMs: number, url: string = REGISTRY_URL -): Promise => - new Promise((resolve) => { - const req = get(url, { headers: { Accept: 'application/json' } }, (res) => { - let body = ''; - res.setEncoding('utf8'); - res.on('data', (chunk: string) => { - body += chunk; - }); - res.on('end', () => { - if (res.statusCode !== 200) return resolve(null); - try { - const v = (JSON.parse(body) as { version?: unknown }).version; - resolve(typeof v === 'string' ? v : null); - } catch { - resolve(null); - } - }); - }); - const timer = setTimeout(() => req.destroy(), timeoutMs); - timer.unref(); - req.once('error', () => resolve(null)); - req.once('close', () => { - clearTimeout(timer); - resolve(null); // no-op when already resolved - }); - }); +): Promise => { + const res = await fetchJsonUnref(url, timeoutMs); + if (!res?.ok) return null; + const v = (res.json as { version?: unknown } | null)?.version; + return typeof v === 'string' ? v : null; +}; export interface RefreshDeps { now?: () => number; diff --git a/src/lib/update-check.test.ts b/src/lib/update-check.test.ts index 1b3397a..7a32060 100644 --- a/src/lib/update-check.test.ts +++ b/src/lib/update-check.test.ts @@ -1,10 +1,14 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; +import { fetchJsonUnref } from './http.js'; import { compareVersions, evaluateUpdate, fetchCliLatest, type CliLatest } from './update-check.js'; -const jsonResponse = (status: number, body: unknown): Response => - new Response(JSON.stringify(body), { status, headers: { 'content-type': 'application/json' } }); +vi.mock('./http.js', () => ({ fetchJsonUnref: vi.fn() })); +const httpMock = vi.mocked(fetchJsonUnref); -afterEach(() => vi.unstubAllGlobals()); +afterEach(() => { + httpMock.mockReset(); + vi.unstubAllGlobals(); +}); describe('compareVersions', () => { it('orders by major, minor, then patch', () => { @@ -23,10 +27,7 @@ describe('compareVersions', () => { describe('fetchCliLatest', () => { it('reads .version from the npm registry (no server floor or notice)', async () => { - vi.stubGlobal( - 'fetch', - vi.fn(async () => jsonResponse(200, { name: '@agentage/cli', version: '0.2.0' })) - ); + httpMock.mockResolvedValue({ ok: true, status: 200, json: { version: '0.2.0' } }); expect(await fetchCliLatest()).toEqual({ version: '0.2.0', minSupported: '0.0.0', @@ -34,31 +35,20 @@ describe('fetchCliLatest', () => { }); }); - it('hits the public npm registry with a JSON Accept header', async () => { - const spy = vi.fn(async (_url: string, _init?: RequestInit) => - jsonResponse(200, { version: '0.2.0' }) - ); - vi.stubGlobal('fetch', spy); - await fetchCliLatest(); - const [url, init] = spy.mock.calls[0]!; - expect(url).toBe('https://registry.npmjs.org/@agentage/cli/latest'); - expect(init?.headers).toMatchObject({ Accept: 'application/json' }); + it('hits the public npm registry through the unref-timer helper', async () => { + httpMock.mockResolvedValue({ ok: true, status: 200, json: { version: '0.2.0' } }); + await fetchCliLatest(1234); + expect(httpMock).toHaveBeenCalledWith('https://registry.npmjs.org/@agentage/cli/latest', 1234); }); - it('returns null on a non-2xx, a throw, or a body without a version', async () => { - vi.stubGlobal( - 'fetch', - vi.fn(async () => jsonResponse(503, {})) - ); + it('returns null on a non-2xx, an unreachable host, or a body without a version', async () => { + httpMock.mockResolvedValue({ ok: false, status: 503, json: {} }); expect(await fetchCliLatest()).toBeNull(); - vi.stubGlobal('fetch', vi.fn().mockRejectedValue(new Error('net'))); + httpMock.mockResolvedValue(null); // unreachable expect(await fetchCliLatest()).toBeNull(); - vi.stubGlobal( - 'fetch', - vi.fn(async () => jsonResponse(200, { nope: true })) - ); + httpMock.mockResolvedValue({ ok: true, status: 200, json: { nope: true } }); expect(await fetchCliLatest()).toBeNull(); }); }); diff --git a/src/lib/update-check.ts b/src/lib/update-check.ts index f8a14e6..3033afb 100644 --- a/src/lib/update-check.ts +++ b/src/lib/update-check.ts @@ -2,6 +2,8 @@ // (the canonical source of truth for a published package). Never throws: an unreachable registry // or any malformed payload yields 'unknown'. +import { fetchJsonUnref } from './http.js'; + export const INSTALL_HINT = 'npm i -g @agentage/cli@latest'; export const REGISTRY_URL = 'https://registry.npmjs.org/@agentage/cli/latest'; @@ -41,20 +43,14 @@ export const compareVersions = (a: string, b: string): number => { }; export const fetchCliLatest = async (timeoutMs = 5000): Promise => { - try { - const res = await fetch(REGISTRY_URL, { - headers: { Accept: 'application/json' }, - signal: AbortSignal.timeout(timeoutMs), - }); - if (!res.ok) return null; - const body = (await res.json().catch(() => null)) as { version?: unknown } | null; - const version = typeof body?.version === 'string' ? body.version : null; - if (!version) return null; - // The registry carries no support floor or notice, so neither gates an update hint. - return { version, minSupported: '0.0.0', message: null }; - } catch { - return null; - } + // node:https via fetchJsonUnref, not global fetch: undici's ref'd connect timer stalls exit. + const res = await fetchJsonUnref(REGISTRY_URL, timeoutMs); + if (!res?.ok) return null; + const body = res.json as { version?: unknown } | null; + const version = typeof body?.version === 'string' ? body.version : null; + if (!version) return null; + // The registry carries no support floor or notice, so neither gates an update hint. + return { version, minSupported: '0.0.0', message: null }; }; export const evaluateUpdate = (installed: string, latest: CliLatest | null): UpdateInfo => {