fix: scope timeoutOf by deadline code so nesting composes (codex round 2)
Round 2 P2: timeoutOf() accepted ANY TimeoutReason, so under nesting — when the upstream handed to deadline() is itself a deadline (the RFC's named tools/execute middleware follow-up) and its outer timer fires first — AbortSignal.any preserves the outer reason and the inner bash/web would report the outer timeout as their own (timedOut / WEB_FETCH_TIMEOUT) though their local timer never expired. Add an optional code to timeoutOf; bash and web pass their own code, so a foreign timeout falls through to the upstream-cancel path.
This commit is contained in:
@@ -53,11 +53,11 @@ export function deadline(
|
||||
code: string,
|
||||
): { signal: AbortSignal; [Symbol.dispose](): void }
|
||||
|
||||
/** Recover the TimeoutReason from an aborted signal (or error), else undefined. */
|
||||
export function timeoutOf(x: AbortSignal | { reason?: unknown }): TimeoutReason | undefined
|
||||
/** Recover the TimeoutReason from an aborted signal (or error); `code` scopes the match to this deadline's timer. */
|
||||
export function timeoutOf(x: AbortSignal | { reason?: unknown }, code?: string): TimeoutReason | undefined
|
||||
```
|
||||
|
||||
`deadline` is `AbortSignal.any([upstream, <timeout controller>])` with three things the standard library does not give: a typed, identifiable `TimeoutReason` on the timeout abort (native `AbortSignal.timeout()` yields a fixed `TimeoutError`, indistinguishable across timeout kinds), an internal `timeoutMs <= 0` "no timeout" sentinel for backend-owned background work, and a `Symbol.dispose` cleanup that works with both `using` and manual disposal. `AbortSignal.any` is a Node ≥ 20 primitive; it is the single mechanism that fuses two abort sources into one, adopting the reason of whichever fires first. External request hints validate as positive finite numbers via `clampTimeout` before they reach `deadline`; `0` is not a model-/plugin-facing "disable timeout" value. When `timeoutMs <= 0` and no upstream signal is present, `deadline()` returns a never-aborting signal plus a no-op disposer so callers keep one call shape. `TimeoutReason` is an internal classification reason: providers translate it into seam-specific public errors or result fields before returning to callers.
|
||||
`deadline` is `AbortSignal.any([upstream, <timeout controller>])` with three things the standard library does not give: a typed, identifiable `TimeoutReason` on the timeout abort (native `AbortSignal.timeout()` yields a fixed `TimeoutError`, indistinguishable across timeout kinds), an internal `timeoutMs <= 0` "no timeout" sentinel for backend-owned background work, and a `Symbol.dispose` cleanup that works with both `using` and manual disposal. `AbortSignal.any` is a Node ≥ 20 primitive; it is the single mechanism that fuses two abort sources into one, adopting the reason of whichever fires first. External request hints validate as positive finite numbers via `clampTimeout` before they reach `deadline`; `0` is not a model-/plugin-facing "disable timeout" value. When `timeoutMs <= 0` and no upstream signal is present, `deadline()` returns a never-aborting signal plus a no-op disposer so callers keep one call shape. `TimeoutReason` is an internal classification reason: providers translate it into seam-specific public errors or result fields before returning to callers. `timeoutOf`'s optional `code` scopes classification to the caller's own deadline: when the `upstream` is itself a deadline (a future `tools/execute` middleware arming a per-call deadline), `AbortSignal.any` preserves the outer `TimeoutReason` if it fires first, and an unscoped match would misreport the outer timeout as the inner capability's own; scoping to `code` reads a foreign timeout as an ordinary upstream cancel.
|
||||
|
||||
### The division of labor
|
||||
|
||||
@@ -76,7 +76,7 @@ The signal only *notifies*; termination is always the listener's job, and the li
|
||||
### How each capability consumes it
|
||||
|
||||
- **web_fetch** — the tool stays validate-and-forward; the provider's hand-rolled controller + `setTimeout` + manual listener + `finally` + `signal.reason` recovery is replaced by provider-owned `deadline`/`timeoutOf`. A pre-aborted upstream signal still throws `WEB_ABORTED` up front; otherwise `fetch` runs against the fused `d.signal`, and `translateAbortOrNetwork` classifies a thrown error by the signal (`timeoutOf` → `WEB_FETCH_TIMEOUT`, else aborted → `WEB_ABORTED`, else network → `WEB_PROVIDER_ERROR`). The public error-code contract is unchanged, and `TimeoutReason` never crosses the web seam as the public error.
|
||||
- **bash** — `resolve()` stays a pure request-to-spec step: it clamps with `clampTimeout(request.timeoutMs, config.timeoutMs, config.maxTimeoutMs, 'bash-local: request.timeoutMs')` and carries `request.signal` through unchanged. Foreground `run()` owns the timeout: `using d = deadline(spec.signal, spec.timeoutMs, 'BASH_TIMEOUT')`, then `runBash` receives only `d.signal`. `runBash` no longer owns any timer — it listens for abort and runs its existing SIGTERM→grace→SIGKILL process-group kill, and its `SpawnSpec`/`SpawnOutcome` no longer carry `timeoutMs`/`timedOut`/`aborted` (the executor classifies from the deadline signal instead). `run()` computes `timedOut = timeoutOf(d.signal) !== undefined` and `aborted = d.signal.aborted && !timedOut`, so the public seam booleans (`BashRunResult.timedOut`/`aborted`) are mutually exclusive — the shared deadline reports the cause that first cut the command short. Background `start()` creates no deadline and forwards only the upstream signal, so background tasks stay timeout-free; a task's killed-vs-completed status reads its own `spec.signal.aborted`.
|
||||
- **bash** — `resolve()` stays a pure request-to-spec step: it clamps with `clampTimeout(request.timeoutMs, config.timeoutMs, config.maxTimeoutMs, 'bash-local: request.timeoutMs')` and carries `request.signal` through unchanged. Foreground `run()` owns the timeout: `using d = deadline(spec.signal, spec.timeoutMs, 'BASH_TIMEOUT')`, then `runBash` receives only `d.signal`. `runBash` no longer owns any timer — it listens for abort and runs its existing SIGTERM→grace→SIGKILL process-group kill, and its `SpawnSpec`/`SpawnOutcome` no longer carry `timeoutMs`/`timedOut`/`aborted` (the executor classifies from the deadline signal instead). `run()` computes `timedOut = timeoutOf(d.signal, 'BASH_TIMEOUT') !== undefined` and `aborted = d.signal.aborted && !timedOut`, so the public seam booleans (`BashRunResult.timedOut`/`aborted`) are mutually exclusive — the shared deadline reports the cause that first cut the command short, and the `code` scope keeps a nested outer deadline from being misread as bash's own timeout. Background `start()` creates no deadline and forwards only the upstream signal, so background tasks stay timeout-free; a task's killed-vs-completed status reads its own `spec.signal.aborted`.
|
||||
|
||||
## Consequences
|
||||
|
||||
|
||||
@@ -150,11 +150,12 @@ export class LocalBashExecutor extends BashExecutor {
|
||||
stdin: spec.stdin,
|
||||
env: spec.env,
|
||||
}, this.internals).done
|
||||
// Classify the FIRST abort reason: a TimeoutReason means the timeout cut the
|
||||
// command short; any other abort is upstream cancellation. Mutually
|
||||
// exclusive by construction — the fused signal reports one cause, not two
|
||||
// independently-latched facts.
|
||||
const timedOut = timeoutOf(d.signal) !== undefined
|
||||
// Classify the FIRST abort reason: a BASH_TIMEOUT TimeoutReason means our
|
||||
// timeout cut the command short; any other abort — an upstream cancel, or a
|
||||
// foreign (outer) deadline's timeout under nesting — is aborted. Scoping to
|
||||
// our own code keeps a nested outer deadline from reading as our timeout.
|
||||
// Mutually exclusive by construction — the fused signal reports one cause.
|
||||
const timedOut = timeoutOf(d.signal, 'BASH_TIMEOUT') !== undefined
|
||||
const aborted = d.signal.aborted && !timedOut
|
||||
return { ...outcome, timedOut, aborted, timeoutMs: spec.timeoutMs }
|
||||
}
|
||||
|
||||
@@ -16,7 +16,7 @@ import { clampTimeout, deadline, timeoutOf, TimeoutReason } from '@deepseek-ai/d
|
||||
|---|---|
|
||||
| `clampTimeout(requested, def, max, name?)` | Validate the caller's optional positive-finite hint, fill from `def`, cap at `max`. Throws (with `name`) on a non-positive/non-finite hint. |
|
||||
| `deadline(upstream, timeoutMs, code)` | Fuse `upstream` cancellation with a timeout into one `AbortSignal` (`AbortSignal.any`); the timeout carries a `TimeoutReason`. `[Symbol.dispose]` clears the timer. |
|
||||
| `timeoutOf(signal \| { reason })` | Recover the `TimeoutReason` from an aborted signal/error, else `undefined` — the timeout-vs-cancel classifier. |
|
||||
| `timeoutOf(signal \| { reason }, code?)` | Recover the `TimeoutReason` from an aborted signal/error, else `undefined` — the timeout-vs-cancel classifier. Pass `code` to match only THIS deadline's timer (see nesting below). |
|
||||
| `TimeoutReason` | The internal reason (`code` + `timeoutMs`) stamped on a timeout abort. Not a public error — providers translate it into their own error/field. |
|
||||
|
||||
## The `timeoutMs <= 0` sentinel
|
||||
@@ -28,13 +28,15 @@ import { clampTimeout, deadline, timeoutOf, TimeoutReason } from '@deepseek-ai/d
|
||||
```ts ignore-check
|
||||
// Scope-lifetime consumer (foreground bash, one fetch): `using` disposes the timer.
|
||||
using d = deadline(upstream, timeoutMs, 'BASH_TIMEOUT')
|
||||
const outcome = await runWork({ signal: d.signal }) // work listens on d.signal and terminates itself
|
||||
const timedOut = timeoutOf(d.signal) !== undefined // classify the first abort
|
||||
const aborted = d.signal.aborted && !timedOut // mutually exclusive: timeout won, or cancel did
|
||||
const outcome = await runWork({ signal: d.signal }) // work listens on d.signal and terminates itself
|
||||
const timedOut = timeoutOf(d.signal, 'BASH_TIMEOUT') !== undefined // classify the first abort, scoped to OUR code
|
||||
const aborted = d.signal.aborted && !timedOut // mutually exclusive: timeout won, or cancel did
|
||||
```
|
||||
|
||||
The signal only *notifies* — the caller MUST attach its own termination (`d.signal.addEventListener('abort', kill)`, or hand `d.signal` to `fetch`). Racing a promise against a timer would resolve the tool-call while the child process or socket leaks on; handing out a signal forces a real termination path to exist.
|
||||
|
||||
Pass your own `code` to `timeoutOf` so classification composes under nesting: when the `upstream` you were handed is *itself* a deadline signal (a future `tools/execute` middleware arming a per-call deadline), `AbortSignal.any` preserves the outer `TimeoutReason` if the outer timer fires first. Scoping to your `code` makes a foreign timeout read as an ordinary upstream cancel — the correct classification from your capability's view — instead of your own timeout firing when your local timer never expired.
|
||||
|
||||
## What does NOT get a timeout
|
||||
|
||||
Local file `read`/`write`/`edit` take no `timeoutMs`: a syscall is best-effort-abortable at most, a timeout could not force `fsync`/`rename` to stop, and adding one would be an implicit default that violates explicit-over-implicit. See [`fs/`](../../fs/README.md).
|
||||
|
||||
@@ -138,12 +138,25 @@ export function deadline(
|
||||
* was its timeout (translate to the capability's timeout error/field) or an
|
||||
* ordinary upstream cancellation (`undefined` → the cancel path).
|
||||
*
|
||||
* Pass `code` to scope the match to THIS deadline's timer. It matters under
|
||||
* nesting: when the `upstream` handed to {@link deadline} is itself a deadline
|
||||
* signal (e.g. a future `tools/execute` middleware arming a per-call deadline),
|
||||
* `AbortSignal.any` preserves the OUTER `TimeoutReason` if the outer timer fires
|
||||
* first. Without `code`, the inner capability would misclassify that outer
|
||||
* timeout as its own (`timedOut:true` / `WEB_FETCH_TIMEOUT`) though its local
|
||||
* timer never expired; with `code`, a foreign timeout reads as `undefined` and
|
||||
* falls through to the upstream-cancel path, which is the correct classification
|
||||
* from the inner capability's view. Omit `code` only to ask "was this ANY
|
||||
* timeout" (a generic middleware that owns no single code).
|
||||
*
|
||||
* @param x An {@link AbortSignal} or any `{ reason }` carrier (e.g. a caught abort error).
|
||||
* @returns The {@link TimeoutReason} when the abort was a timeout, else `undefined`.
|
||||
* @param code When provided, only a {@link TimeoutReason} with this exact `code` matches.
|
||||
* @returns The matching {@link TimeoutReason}, else `undefined`.
|
||||
*/
|
||||
export function timeoutOf(x: AbortSignal | { reason?: unknown }): TimeoutReason | undefined {
|
||||
export function timeoutOf(x: AbortSignal | { reason?: unknown }, code?: string): TimeoutReason | undefined {
|
||||
// AbortSignal.reason is typed `any`; pin it to `unknown` so no `any` leaks and
|
||||
// the instanceof narrows cleanly for both a signal and a bare reason carrier.
|
||||
const reason: unknown = x.reason
|
||||
return reason instanceof TimeoutReason ? reason : undefined
|
||||
if (!(reason instanceof TimeoutReason)) return undefined
|
||||
return code === undefined || reason.code === code ? reason : undefined
|
||||
}
|
||||
|
||||
@@ -157,4 +157,25 @@ describe('timeoutOf', () => {
|
||||
expect(timeoutOf({ reason: 'user cancelled' })).toBeUndefined()
|
||||
expect(timeoutOf({})).toBeUndefined()
|
||||
})
|
||||
|
||||
it('matches only the requested code when one is given', () => {
|
||||
const reason = new TimeoutReason('BASH_TIMEOUT', 100)
|
||||
expect(timeoutOf({ reason }, 'BASH_TIMEOUT')).toBe(reason)
|
||||
expect(timeoutOf({ reason }, 'WEB_FETCH_TIMEOUT')).toBeUndefined()
|
||||
})
|
||||
})
|
||||
|
||||
describe('deadline — nested deadlines', () => {
|
||||
it("does not misclassify an outer deadline's timeout as the inner code", () => {
|
||||
// The upstream handed to the inner deadline is ITSELF a deadline that has
|
||||
// already timed out (outer). AbortSignal.any preserves the outer reason;
|
||||
// scoping timeoutOf to the inner code keeps the inner capability from
|
||||
// reporting the outer timeout as its own — it reads as an upstream cancel.
|
||||
const outer = new AbortController()
|
||||
outer.abort(new TimeoutReason('OUTER_TIMEOUT', 30))
|
||||
using inner = deadline(outer.signal, 60_000, 'BASH_TIMEOUT')
|
||||
expect(inner.signal.aborted).toBe(true)
|
||||
expect(timeoutOf(inner.signal, 'BASH_TIMEOUT')).toBeUndefined() // not ours → upstream-cancel path
|
||||
expect(timeoutOf(inner.signal)?.code).toBe('OUTER_TIMEOUT') // but IS a timeout, unscoped
|
||||
})
|
||||
})
|
||||
|
||||
@@ -249,13 +249,14 @@ function resolveRedirect(location: string, base: URL): URL {
|
||||
* Translate a thrown fetch/stream error into a `WebError`, classified by the
|
||||
* deadline signal rather than the error's shape (which differs by phase: the
|
||||
* request-phase `fetch` rejects with the abort reason, while the read-phase
|
||||
* reader surfaces a bare `AbortError`). `timeoutOf(signal)` recovering a
|
||||
* `TimeoutReason` means our timeout fired (`WEB_FETCH_TIMEOUT`); any other abort
|
||||
* is upstream cancellation (`WEB_ABORTED`); a throw with the signal NOT aborted
|
||||
* is a transport/network failure (`WEB_PROVIDER_ERROR`).
|
||||
* reader surfaces a bare `AbortError`). `timeoutOf(signal, 'WEB_FETCH_TIMEOUT')`
|
||||
* recovering OUR reason means our timeout fired (`WEB_FETCH_TIMEOUT`); any other
|
||||
* abort — an upstream cancel, or a foreign/outer deadline's timeout under
|
||||
* nesting — is `WEB_ABORTED`; a throw with the signal NOT aborted is a
|
||||
* transport/network failure (`WEB_PROVIDER_ERROR`).
|
||||
*/
|
||||
function translateAbortOrNetwork(error: unknown, signal: AbortSignal): WebError {
|
||||
const timeout = timeoutOf(signal)
|
||||
const timeout = timeoutOf(signal, 'WEB_FETCH_TIMEOUT')
|
||||
if (timeout !== undefined) return new WebError('web fetch timed out', 'WEB_FETCH_TIMEOUT', { cause: timeout })
|
||||
if (signal.aborted) return new WebError('web fetch aborted', 'WEB_ABORTED', { cause: error })
|
||||
return new WebError(`web fetch failed: ${String(error)}`, 'WEB_PROVIDER_ERROR', { cause: error })
|
||||
|
||||
Reference in New Issue
Block a user