Skip to content

Commit 0e84e92

Browse files
authored
fix(cli): bound, trace, and explain the requests the CLI makes (#6798)
* fix(cli): bound, trace, and explain the requests the CLI makes Four transport gaps, all of which failed silently. A request had no timeout, so a connection that was accepted and then never answered hung the terminal indefinitely. `SIM_TIMEOUT_SECONDS` now bounds one, defaulting to 3600s — deliberately above every timeout the server itself applies, since a synchronous workflow run is allowed 3000s on a paid plan and a tighter default would abort real work and report it as a transport failure. `0` removes the bound, for a self-hosted deployment that runs executions without one of its own. The caller's abort signal is composed with the timeout rather than replaced, so neither masks the other. Node ignores HTTP(S)_PROXY unless NODE_USE_ENV_PROXY opts in, and only from v22.21 and v24.5, so on a network that reaches the API only through a proxy every command failed to connect while the variable that would have fixed it was already set. The CLI cannot enable that from inside the process — Node reads it at startup — so it says what to do rather than bundling an HTTP stack for a setting the platform now owns. An API key was sent to any http:// endpoint with no signal. Now a warning, not a refusal: http is the documented way to reach a local dev server, and a deployment terminating TLS at a gateway is real. Loopback stays silent. `SIM_DEBUG=1` traces method, URL, status and duration. Bodies and headers are deliberately absent — the request carries the API key, and `secrets set` carries the secret itself. All four write to stderr, so a piped stdout stays parseable. * fix(cli): make the request bound safe on every runtime it supports Two ways the new timeout could fail before the request was made. `AbortSignal.any` arrived in Node 20.3 and this package supports Node 20, so composing a caller's abort signal with the timeout threw a bare TypeError on the earliest 20.x releases. It is now used when present and composed through an AbortController when not. `AbortSignal.timeout` rejects a fractional millisecond outright, and past 2^31-1 ms it does not fail at all — it clamps to 1ms, so the longest timeout anyone asked for became the shortest. The value is now rounded and refused above what Node can actually wait, pointing at 0 for an unbounded wait. Also unstubs env vars between tests: `stubEnv` is not undone by `unstubAllGlobals`, so a SIM_TIMEOUT_SECONDS set for one test configured every test after it. * fix(cli): correct the proxy version table, and classify a timeout mid-body `runtimeCanProxy` treated any release between 22 and 24 as capable, so on Node 23 — which reached end of life before the backport — a configured proxy was ignored and the CLI stayed silent about it, which is the exact failure the warning exists to report. The table is now the two lines that shipped the support, and anything after them. `AbortSignal.timeout` keeps firing after `fetch` resolves, so a bound that elapsed while the body was still being read — a large `files get` — escaped the client's own handling and printed a raw TimeoutError stack. The top-level handler now names it, which covers the streaming path as well as the JSON one. A user's own Ctrl-C raises AbortError and is deliberately left alone. * fix(cli): report a timed-out download as a timeout `files get --output-file` streams the body to disk, and `streamToFile` converted anything the stream threw into a write failure. So a request bound elapsing mid-download read as `Could not write <path>: ...`, sending the reader to check permissions and free space for a timeout they can raise, and hiding the one instruction that resolves it. The predicate and that instruction now live beside the timeout that raises them, so the client, the top-level handler and the download path all say the same thing. The wrapping stays where it is: the staged-download cleanup runs off that failure, and rethrowing past it would leak the temporary directory. * fix(cli): keep a sub-millisecond timeout bounded Zero is how this function says "no bound", so rounding a positive SIM_TIMEOUT_SECONDS down to zero inverted the request: anything under 0.0005s asked for the shortest possible timeout and got none at all, leaving a stalled request to hang. Introduced by the rounding that fixed the fractional-millisecond rejection. Floored at 1ms for every positive value; only a literal 0 still disables.
1 parent 0b4d341 commit 0e84e92

9 files changed

Lines changed: 568 additions & 5 deletions

File tree

apps/docs/content/docs/en/cli/configuration.mdx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,12 @@ credentials. The `default` profile is `[default]` in both.
101101
| `SIM_CONFIG_DIR` | Relocate both files away from `~/.sim` |
102102
| `SIM_CONFIG_FILE` | Relocate only the config file |
103103
| `SIM_CREDENTIALS_FILE` | Relocate only the credentials file |
104+
| `SIM_TIMEOUT_SECONDS` | Per-request timeout; `0` waits indefinitely. Defaults to `3600`, above every timeout the server itself applies |
105+
| `SIM_DEBUG` | Trace each request's method, URL, status and duration to stderr |
106+
107+
Node ignores `HTTPS_PROXY` unless you also set `NODE_USE_ENV_PROXY=1`, and only
108+
from Node 22.21 and 24.5. The CLI warns when a proxy is configured but will not
109+
be used.
104110

105111
For CI, set `SIM_API_KEY` and `SIM_WORKSPACE` and nothing needs to touch the
106112
filesystem at all.

packages/sim-cli/README.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,11 @@ Each setting resolves independently, first match wins:
5757
| --- | --- |
5858
| 1 | Command-line flag (`--endpoint`, `--workspace`, `--output`) |
5959
| 2 | Environment (`SIM_ENDPOINT`, `SIM_API_KEY`, `SIM_WORKSPACE`, `SIM_OUTPUT`) |
60+
61+
`SIM_TIMEOUT_SECONDS` bounds each request (default `3600`, `0` waits
62+
indefinitely) and `SIM_DEBUG=1` traces requests to stderr. Node ignores
63+
`HTTPS_PROXY` unless `NODE_USE_ENV_PROXY=1` is also set, on Node 22.21+ or
64+
24.5+; the CLI warns when a proxy is configured but will not be used.
6065
| 3 | `~/.sim/config` / `~/.sim/credentials` for the selected profile |
6166
| 4 | Built-in default (`https://www.sim.ai`, `table`) |
6267

packages/sim-cli/src/commands/protocol/files-get.test.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,16 @@ function failingBody(): ReadableStream<Uint8Array> {
6767
})
6868
}
6969

70+
/** What `fetch` does to a body when the request's own timeout elapses. */
71+
function timedOutBody(): ReadableStream<Uint8Array> {
72+
return new ReadableStream({
73+
start(controller) {
74+
controller.enqueue(new TextEncoder().encode('partial'))
75+
controller.error(new DOMException('The operation was aborted due to timeout', 'TimeoutError'))
76+
},
77+
})
78+
}
79+
7080
function program(): Command {
7181
const root = new Command('sim').exitOverride()
7282
for (const group of buildGeneratedCommands()) root.addCommand(group)
@@ -81,6 +91,23 @@ describe('streamToFile', () => {
8191
expect(existsSync(target)).toBe(true)
8292
})
8393

94+
it('reports an elapsed request bound as a timeout, not as a failed write', async () => {
95+
// The stream is torn down by the request's own timeout, which is not a
96+
// disk problem: calling it "could not write" sent the reader to check
97+
// permissions and free space for a bound they can raise.
98+
const target = join(dir, 'out.txt')
99+
await expect(
100+
streamToFile(timedOutBody(), createWriteStream(target, { flags: 'wx' }))
101+
).rejects.toThrow(/SIM_TIMEOUT_SECONDS/)
102+
})
103+
104+
it('still reports a genuine write failure as one', async () => {
105+
const target = join(dir, 'out.txt')
106+
await expect(
107+
streamToFile(failingBody(), createWriteStream(target, { flags: 'wx' }))
108+
).rejects.toThrow(/Could not write/)
109+
})
110+
84111
it('refuses to clobber an existing file, naming --force', async () => {
85112
const target = join(dir, 'out.txt')
86113
writeFileSync(target, 'precious')

packages/sim-cli/src/commands/protocol/files-get.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,17 @@ import { pipeline } from 'node:stream/promises'
77
import type { Command } from 'commander'
88
import { clientFrom } from '../../context'
99
import { V2_OPERATIONS } from '../../generated/v2-api'
10-
import { resolvePath, SimApiError } from '../../http/client'
10+
import { isRequestTimeout, RAISE_TIMEOUT_HINT, resolvePath, SimApiError } from '../../http/client'
1111
import { printProtocolResult } from './result'
1212

1313
function writeFailure(path: WriteStream['path'], error: unknown): SimApiError {
14+
// A body torn down by the request's own bound is not a disk problem. Calling
15+
// it "could not write" sent the reader to check permissions and free space
16+
// for a timeout they can raise, and hid the one instruction that resolves it.
17+
if (isRequestTimeout(error)) {
18+
return new SimApiError(`Downloading ${path} timed out. ${RAISE_TIMEOUT_HINT}`, 0)
19+
}
20+
1421
const code = (error as NodeJS.ErrnoException).code
1522
if (code === 'EEXIST') {
1623
return new SimApiError(

packages/sim-cli/src/http/client.test.ts

Lines changed: 174 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { afterEach, describe, expect, it, vi } from 'vitest'
22
import { CLI_CONTRACT } from '../contract/commands'
33
import { V2_OPERATIONS, type V2OperationName } from '../generated/v2-api'
4+
import { sleep } from '../helpers'
45
import { USER_AGENT } from '../version'
56
import {
67
formatApiErrorDetails,
@@ -13,6 +14,9 @@ import {
1314

1415
afterEach(() => {
1516
vi.unstubAllGlobals()
17+
// `stubEnv` is not undone by `unstubAllGlobals`, so a SIM_TIMEOUT_SECONDS or
18+
// SIM_DEBUG set for one test would otherwise configure every test after it.
19+
vi.unstubAllEnvs()
1620
})
1721

1822
function client(options: { apiKey?: string } = { apiKey: 'key' }): SimClient {
@@ -314,6 +318,168 @@ describe('non-JSON responses', () => {
314318
})
315319
})
316320

321+
describe('a request that never answers', () => {
322+
it('bounds a request by default, above every timeout the server itself applies', async () => {
323+
// A synchronous workflow run is allowed 3000s on a paid plan, so a tighter
324+
// default would abort real work and report it as a transport failure. What
325+
// this catches is a connection that is accepted and then never answers.
326+
const fetchMock = vi.fn().mockResolvedValue(
327+
new Response(JSON.stringify({ data: [] }), {
328+
status: 200,
329+
headers: { 'content-type': 'application/json' },
330+
})
331+
)
332+
vi.stubGlobal('fetch', fetchMock)
333+
334+
await client().request('/api/v2/workflows')
335+
336+
const signal = fetchMock.mock.calls[0][1].signal as AbortSignal
337+
expect(signal).toBeInstanceOf(AbortSignal)
338+
expect(signal.aborted).toBe(false)
339+
})
340+
341+
it('sends no signal at all when the bound is switched off', async () => {
342+
// A self-hosted deployment can run executions without a timeout of its own,
343+
// and there the client must not invent one.
344+
vi.stubEnv('SIM_TIMEOUT_SECONDS', '0')
345+
const fetchMock = vi.fn().mockResolvedValue(
346+
new Response(JSON.stringify({ data: [] }), {
347+
status: 200,
348+
headers: { 'content-type': 'application/json' },
349+
})
350+
)
351+
vi.stubGlobal('fetch', fetchMock)
352+
353+
await client().request('/api/v2/workflows')
354+
expect(fetchMock.mock.calls[0][1].signal).toBeUndefined()
355+
})
356+
357+
it('refuses a timeout that is not a number, naming the variable', async () => {
358+
vi.stubEnv('SIM_TIMEOUT_SECONDS', 'soon')
359+
vi.stubGlobal('fetch', vi.fn())
360+
361+
await expect(client().request('/api/v2/workflows')).rejects.toThrow(
362+
/Invalid SIM_TIMEOUT_SECONDS "soon"/
363+
)
364+
})
365+
366+
it('rounds a fractional millisecond rather than letting the timer reject it', async () => {
367+
// `AbortSignal.timeout` rejects a non-integer delay outright, so an
368+
// unrounded 0.0005s threw ERR_OUT_OF_RANGE before the request was made.
369+
vi.stubEnv('SIM_TIMEOUT_SECONDS', '0.0005')
370+
const fetchMock = vi.fn().mockResolvedValue(
371+
new Response(JSON.stringify({ data: [] }), {
372+
status: 200,
373+
headers: { 'content-type': 'application/json' },
374+
})
375+
)
376+
vi.stubGlobal('fetch', fetchMock)
377+
378+
await expect(client().request('/api/v2/workflows')).resolves.toBeDefined()
379+
})
380+
381+
it('keeps a bound below half a millisecond bounded, rather than disabling it', async () => {
382+
// Zero means "no bound", so rounding a positive value down to zero inverted
383+
// the request: the shortest timeout anyone could ask for became none.
384+
vi.stubEnv('SIM_TIMEOUT_SECONDS', '0.0004')
385+
const fetchMock = vi.fn().mockResolvedValue(
386+
new Response(JSON.stringify({ data: [] }), {
387+
status: 200,
388+
headers: { 'content-type': 'application/json' },
389+
})
390+
)
391+
vi.stubGlobal('fetch', fetchMock)
392+
393+
await client().request('/api/v2/workflows')
394+
expect(fetchMock.mock.calls[0][1].signal).toBeInstanceOf(AbortSignal)
395+
})
396+
397+
it('refuses a delay longer than Node can wait, which would silently become 1ms', async () => {
398+
// Past 2^31-1 ms Node does not fail — it clamps to 1ms, so the request the
399+
// caller asked to wait longest for would be the first one aborted.
400+
vi.stubEnv('SIM_TIMEOUT_SECONDS', String(2 ** 31))
401+
vi.stubGlobal('fetch', vi.fn())
402+
403+
await expect(client().request('/api/v2/workflows')).rejects.toThrow(/longer than Node can wait/)
404+
})
405+
406+
it('composes the caller signal with the timeout without AbortSignal.any', async () => {
407+
// `AbortSignal.any` arrived in Node 20.3 and this package supports Node 20,
408+
// so the earliest 20.x releases would have thrown a bare TypeError here.
409+
const original = AbortSignal.any
410+
// biome-ignore lint/performance/noDelete: restoring the property is the point
411+
delete (AbortSignal as { any?: unknown }).any
412+
const controller = new AbortController()
413+
const fetchMock = vi.fn().mockResolvedValue(
414+
new Response(JSON.stringify({ data: [] }), {
415+
status: 200,
416+
headers: { 'content-type': 'application/json' },
417+
})
418+
)
419+
vi.stubGlobal('fetch', fetchMock)
420+
421+
try {
422+
await client().request('/api/v2/workflows', { signal: controller.signal })
423+
const sent = fetchMock.mock.calls[0][1].signal as AbortSignal
424+
expect(sent.aborted).toBe(false)
425+
controller.abort()
426+
expect(sent.aborted).toBe(true)
427+
} finally {
428+
;(AbortSignal as { any?: unknown }).any = original
429+
}
430+
})
431+
432+
it('explains a timeout as a timeout, not as an unreachable endpoint', async () => {
433+
vi.stubEnv('SIM_TIMEOUT_SECONDS', '0.001')
434+
vi.stubGlobal(
435+
'fetch',
436+
vi.fn().mockImplementation(async (_url: string, init: RequestInit) => {
437+
await sleep(20)
438+
init.signal?.throwIfAborted()
439+
return new Response('{}')
440+
})
441+
)
442+
443+
await expect(client().request('/api/v2/workflows')).rejects.toThrow(/did not answer within/)
444+
})
445+
})
446+
447+
describe('tracing a request', () => {
448+
it('traces method, url, status and duration when asked, and nothing otherwise', async () => {
449+
const response = () =>
450+
new Response(JSON.stringify({ data: [] }), {
451+
status: 200,
452+
headers: { 'content-type': 'application/json' },
453+
})
454+
455+
const quiet = stubStderr(false)
456+
try {
457+
vi.stubGlobal('fetch', vi.fn().mockResolvedValue(response()))
458+
await client().request('/api/v2/workflows')
459+
} finally {
460+
quiet.restore()
461+
}
462+
expect(quiet.writes).toEqual([])
463+
464+
vi.stubEnv('SIM_DEBUG', '1')
465+
const traced = stubStderr(false)
466+
try {
467+
vi.stubGlobal('fetch', vi.fn().mockResolvedValue(response()))
468+
await client().request('/api/v2/workflows')
469+
} finally {
470+
traced.restore()
471+
}
472+
473+
const line = traced.writes.join('')
474+
expect(line).toContain('GET https://sim.example/api/v2/workflows')
475+
expect(line).toContain('200')
476+
expect(line).toMatch(/\d+ms/)
477+
// The request carries the API key, and `secrets set` carries the secret
478+
// itself, so a trace must never include headers or bodies.
479+
expect(line).not.toContain('key')
480+
})
481+
})
482+
317483
describe('request identity', () => {
318484
it('identifies the CLI, its version and its runtime to the API', async () => {
319485
// Without a User-Agent a CLI request is indistinguishable from any other
@@ -597,14 +763,21 @@ describe('raw requests', () => {
597763
'https://sim.example/api/v2/chat',
598764
expect.objectContaining({
599765
method: 'POST',
600-
signal: controller.signal,
601766
headers: expect.objectContaining({
602767
accept: 'text/event-stream',
603768
'content-type': 'application/json',
604769
'x-api-key': 'key',
605770
}),
606771
})
607772
)
773+
774+
// The signal is composed with the request timeout, so it is no longer the
775+
// caller's object. What has to hold is the behaviour: aborting the
776+
// caller's controller still aborts the request.
777+
const sent = fetch.mock.calls[0][1].signal as AbortSignal
778+
expect(sent.aborted).toBe(false)
779+
controller.abort()
780+
expect(sent.aborted).toBe(true)
608781
})
609782

610783
it('turns an aborted fetch into a clean CLI error', async () => {

0 commit comments

Comments
 (0)