Skip to content

Commit 6ce1a53

Browse files
jamesbhobbsclaude
jamesbhobbs
and
claude
committed
fix(cloud): roll the request deadline by hand instead of AbortSignal.any
`AbortSignal.any([caller, AbortSignal.timeout(ms)])` has a history of dropping the timeout: the composed signal can be collected while the request is in flight, so the deadline never fires and the request hangs — the exact failure the deadline exists to prevent (nodejs/node#57736, fixed by nodejs/node#57867). Raising the package's Node floor would fix it, but that is a breaking constraint on every consumer to work around an implementation detail. Wiring the controller by hand behaves identically on every supported version, and lets the timer be cleared once the request settles rather than leaving one pending per request for its full timeout. The body read moved inside the deadline too — headers can arrive promptly while the body stalls. Also from review: - Docs: `--session` is cloud-only, so the flag table says so. - Docs: the isolation wording implied `storageMode: 'readonly'` removed project-file interaction. It stops *writes*; the session still reads the same shared files. Split into what is and is not isolated. - Docs: the sessions example called `getNotebook` without importing it, and passed the optional `sessionNotebookId` where a string is required — neither would have compiled if copied. - Tests: the pending-fetch helper now rejects immediately for an already-aborted signal, as real fetch does. Added cases for timer cleanup on both the success and failure paths, and for an already-aborted caller signal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 30325f0 commit 6ce1a53

4 files changed

Lines changed: 157 additions & 48 deletions

File tree

‎packages/cloud/README.md‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,21 +62,39 @@ is in preview and its exact shape may drift. Failures throw `ApiError`
6262
**Note on interactive sessions.** Two properties, useful for different reasons.
6363

6464
**It runs its own copy**, so the source notebook is never modified — the basis for executing a
65-
notebook without writing to the original. The guarantee is narrow, and worth reading literally: it
66-
covers the notebook _document_. The session still runs real code against the project's files (unless
67-
`storageMode: 'readonly'`), its databases and integrations, and anything the notebook calls out to,
68-
and each run persists as a run snapshot.
65+
notebook without writing to the original. Read the guarantee literally, because it is narrow: the
66+
notebook _document_ is isolated, and nothing else is. Everything the code reaches is still the real
67+
thing, shared with everyone else:
68+
69+
- **Project files** are shared. `storageMode: 'readonly'` stops the session _writing_ to project
70+
storage, but it still reads the same files; it does not give the session files of its own.
71+
- **Databases, integrations and external APIs** are untouched by any of this. A notebook that writes
72+
to a warehouse writes to it from a session too.
73+
- **The run is recorded** — each run persists as a run snapshot. A session is not a private run.
6974

7075
**The kernel stays warm across submissions**, which is what makes a session worth holding open
7176
rather than creating per run. That is the interactive-tool case:
7277

7378
```ts
79+
import {
80+
createSession,
81+
getNotebook,
82+
getSessionStatus,
83+
interruptSession,
84+
stopSession,
85+
submitSessionRun,
86+
} from "@deepnote/cloud";
87+
7488
const { session } = await createSession(baseUrl, token, {
7589
notebookId,
76-
storageMode: "readonly", // the copy is all a session isolates; project files are shared
90+
storageMode: "readonly", // stops writes to project storage; files are still shared
7791
});
7892
try {
7993
// Block ids must come from the session's *copy* — source-notebook ids are rejected.
94+
// `sessionNotebookId` is optional on the response, so it is checked rather than assumed.
95+
if (!session.sessionNotebookId) {
96+
throw new Error("Session did not report the notebook copy it runs.");
97+
}
8098
const copy = await getNotebook(baseUrl, token, session.sessionNotebookId);
8199

82100
await submitSessionRun(baseUrl, token, session.id, {

‎packages/cloud/src/http.test.ts‎

Lines changed: 51 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,20 @@ function response(body: unknown, init: { ok?: boolean; status?: number; statusTe
2323
* Required to test the deadline at all: a mock that resolves immediately passes whether or not the
2424
* timeout is wired up, so it proves nothing about a request that hangs.
2525
*/
26-
function pendingFetch(): void {
27-
vi.spyOn(global, 'fetch').mockImplementation(
26+
function pendingFetch(): ReturnType<typeof vi.spyOn> {
27+
return vi.spyOn(global, 'fetch').mockImplementation(
2828
(_url, init) =>
2929
new Promise((_resolve, reject) => {
30-
init?.signal?.addEventListener('abort', () => reject((init.signal as AbortSignal).reason))
30+
const signal = init?.signal
31+
// Real `fetch` rejects at once for a signal that is already aborted rather than waiting for
32+
// an event that will never fire again.
33+
if (signal?.aborted) {
34+
reject(signal.reason)
35+
return
36+
}
37+
signal?.addEventListener('abort', () => reject(signal.reason))
3138
})
32-
)
39+
) as ReturnType<typeof vi.spyOn>
3340
}
3441

3542
function call(options: Partial<Parameters<typeof request>[2]> = {}) {
@@ -174,6 +181,46 @@ describe('request — deadline', () => {
174181
expect(controller.signal.aborted).toBe(false)
175182
})
176183

184+
it('clears the deadline timer once the request settles', async () => {
185+
// Otherwise every request leaves a pending timer behind for its full timeout — the leak that
186+
// comes free with rolling the deadline by hand instead of composing signals.
187+
vi.useFakeTimers()
188+
try {
189+
vi.spyOn(global, 'fetch').mockResolvedValue(response({ ok: true }))
190+
191+
await call({ timeoutMs: 30_000 })
192+
193+
expect(vi.getTimerCount()).toBe(0)
194+
} finally {
195+
vi.useRealTimers()
196+
}
197+
})
198+
199+
it('clears the deadline timer even when the request fails', async () => {
200+
vi.useFakeTimers()
201+
try {
202+
vi.spyOn(global, 'fetch').mockResolvedValue(
203+
response({ message: 'nope' }, { ok: false, status: 500, statusText: 'Server Error' })
204+
)
205+
206+
await expect(call({ timeoutMs: 30_000 })).rejects.toMatchObject({ statusCode: 500 })
207+
208+
expect(vi.getTimerCount()).toBe(0)
209+
} finally {
210+
vi.useRealTimers()
211+
}
212+
})
213+
214+
it('aborts before sending when the caller’s signal is already aborted', async () => {
215+
const fetchSpy = pendingFetch()
216+
const controller = new AbortController()
217+
controller.abort(new Error('already gone'))
218+
219+
await expect(call({ signal: controller.signal })).rejects.toThrow(/already gone/)
220+
// The signal handed to fetch is aborted from the outset rather than aborting later.
221+
expect((fetchSpy.mock.calls[0]?.[1] as RequestInit).signal?.aborted).toBe(true)
222+
})
223+
177224
it('still aborts when the caller aborts, well before the deadline', async () => {
178225
pendingFetch()
179226
const controller = new AbortController()

‎packages/cloud/src/http.ts‎

Lines changed: 61 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,50 @@ export interface ApiRequestOptions<T> {
5555
}
5656

5757
/**
58-
* A signal that aborts on the caller's signal *or* the request deadline, whichever comes first.
58+
* A signal that aborts on the caller's signal *or* the request deadline, whichever comes first,
59+
* plus a `release` to call once the request has settled.
5960
*
6061
* The deadline is not optional: a request that hangs forever is the failure mode this whole module
6162
* exists to prevent, and a caller passing a signal is asking to cancel early, not to opt out of it.
63+
*
64+
* Deliberately not `AbortSignal.any([callerSignal, AbortSignal.timeout(ms)])`, which is the obvious
65+
* way to write this. That composition has a history of dropping the timeout — the composed signal
66+
* can be collected while the request is still in flight, so the deadline never fires and the request
67+
* hangs anyway (nodejs/node#57736, fixed by nodejs/node#57867). Rather than pin a Node floor for
68+
* every consumer of this package on an implementation detail, this wires the two together by hand,
69+
* which behaves the same on every supported version and lets us clear the timer afterwards instead
70+
* of leaving one pending per request.
6271
*/
63-
function combineSignals(callerSignal: AbortSignal | undefined, timeoutMs: number): AbortSignal {
64-
const timeout = AbortSignal.timeout(timeoutMs)
65-
return callerSignal ? AbortSignal.any([callerSignal, timeout]) : timeout
72+
function withDeadline(
73+
callerSignal: AbortSignal | undefined,
74+
timeoutMs: number
75+
): { signal: AbortSignal; release: () => void } {
76+
const controller = new AbortController()
77+
78+
const timer = setTimeout(() => {
79+
// Same shape as `AbortSignal.timeout` so callers can keep distinguishing a deadline from a
80+
// caller-initiated abort by `name`.
81+
controller.abort(new DOMException(`Request timed out after ${timeoutMs}ms`, 'TimeoutError'))
82+
}, timeoutMs)
83+
// A pending deadline must never be the reason a process stays alive.
84+
timer.unref?.()
85+
86+
const onCallerAbort = () => controller.abort(callerSignal?.reason)
87+
if (callerSignal) {
88+
if (callerSignal.aborted) {
89+
controller.abort(callerSignal.reason)
90+
} else {
91+
callerSignal.addEventListener('abort', onCallerAbort, { once: true })
92+
}
93+
}
94+
95+
return {
96+
signal: controller.signal,
97+
release: () => {
98+
clearTimeout(timer)
99+
callerSignal?.removeEventListener('abort', onCallerAbort)
100+
},
101+
}
66102
}
67103

68104
/** Raise the right {@link ApiError} for a failed response. Never returns for a non-ok response. */
@@ -90,21 +126,29 @@ export async function throwForResponse(
90126
/** Issue an authenticated JSON request and validate the response against `schema`. */
91127
export async function request<T>(baseUrl: string, token: string, options: ApiRequestOptions<T>): Promise<T> {
92128
const { method, path, schema, body, fallback, forbiddenMessage } = options
93-
const response = await fetch(`${trimTrailingSlash(baseUrl)}${path}`, {
94-
method,
95-
headers: authHeaders(token),
96-
body: body === undefined ? undefined : JSON.stringify(body),
97-
// Both, not either: `signal ?? timeout` silently drops the deadline for every caller that
98-
// passes a signal, so those requests could hang forever. This package targets Node 22.14+,
99-
// where `AbortSignal.any` is available.
100-
signal: combineSignals(options.signal, options.timeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS),
101-
})
129+
// Both, not either: `signal ?? timeout` silently drops the deadline for every caller that passes
130+
// a signal, so those requests could hang forever.
131+
const deadline = withDeadline(options.signal, options.timeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS)
102132

103-
if (!response.ok) {
104-
await throwForResponse(response, fallback, forbiddenMessage)
105-
}
133+
let response: Response
134+
let text: string
135+
try {
136+
response = await fetch(`${trimTrailingSlash(baseUrl)}${path}`, {
137+
method,
138+
headers: authHeaders(token),
139+
body: body === undefined ? undefined : JSON.stringify(body),
140+
signal: deadline.signal,
141+
})
106142

107-
const text = await response.text()
143+
if (!response.ok) {
144+
await throwForResponse(response, fallback, forbiddenMessage)
145+
}
146+
147+
// Read inside the deadline too: the headers can arrive promptly and the body still stall.
148+
text = await response.text()
149+
} finally {
150+
deadline.release()
151+
}
108152
let json: unknown
109153
try {
110154
// A 204 (or any empty body) is valid for endpoints that return nothing; parse it as `{}` so a

‎skills/deepnote/references/cli-run.md‎

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,28 +6,28 @@ Install: `npm install -g @deepnote/cli`
66

77
Execute notebooks (.deepnote, .ipynb, .py, .qmd).
88

9-
| Option | Description |
10-
| ------------------------- | --------------------------------------------------------------------------------------------- |
11-
| `--python <path>` | Path to Python (executable, bin directory, or venv root) |
12-
| `--cwd <path>` | Working directory for execution |
13-
| `--notebook <name>` | Run only the specified notebook |
14-
| `--block <id>` | Run only the specified block |
15-
| `-i, --input <key=value>` | Set input variable value (repeatable) |
16-
| `--list-inputs` | List all input variables without running |
17-
| `--prompt <text>` | Run an agent block with the given prompt |
18-
| `-o, --output <format>` | Output format: `json`, `toon`, `llm` |
19-
| `--dry-run` | Show what would be executed without running |
20-
| `--top` | Display resource usage (CPU, memory) during execution |
21-
| `--profile` | Show per-block timing and memory usage |
22-
| `--open` | Open the project in Deepnote Cloud after execution |
23-
| `--context` | Include analysis context in output |
24-
| `--cloud` | Run in Deepnote Cloud, then download the snapshot locally |
25-
| `--notebook-id <uuid>` | Cloud notebook id to run (with `--cloud`) |
26-
| `--out <path>` | Write the downloaded cloud snapshot to this exact path |
27-
| `--timeout <seconds>` | Max seconds to wait for a cloud run (with `--cloud`, default 600) |
28-
| `--session` | Run in an interactive session on its own copy of the notebook (not combinable with `--block`) |
29-
| `--url <url>` | API base URL (default `https://api.deepnote.com`) |
30-
| `--token <token>` | Bearer token (or `DEEPNOTE_TOKEN` env var) |
9+
| Option | Description |
10+
| ------------------------- | ------------------------------------------------------------------------------------------------------------- |
11+
| `--python <path>` | Path to Python (executable, bin directory, or venv root) |
12+
| `--cwd <path>` | Working directory for execution |
13+
| `--notebook <name>` | Run only the specified notebook |
14+
| `--block <id>` | Run only the specified block |
15+
| `-i, --input <key=value>` | Set input variable value (repeatable) |
16+
| `--list-inputs` | List all input variables without running |
17+
| `--prompt <text>` | Run an agent block with the given prompt |
18+
| `-o, --output <format>` | Output format: `json`, `toon`, `llm` |
19+
| `--dry-run` | Show what would be executed without running |
20+
| `--top` | Display resource usage (CPU, memory) during execution |
21+
| `--profile` | Show per-block timing and memory usage |
22+
| `--open` | Open the project in Deepnote Cloud after execution |
23+
| `--context` | Include analysis context in output |
24+
| `--cloud` | Run in Deepnote Cloud, then download the snapshot locally |
25+
| `--notebook-id <uuid>` | Cloud notebook id to run (with `--cloud`) |
26+
| `--out <path>` | Write the downloaded cloud snapshot to this exact path |
27+
| `--timeout <seconds>` | Max seconds to wait for a cloud run (with `--cloud`, default 600) |
28+
| `--session` | Run in an interactive session on its own copy of the notebook (with `--cloud`; not combinable with `--block`) |
29+
| `--url <url>` | API base URL (default `https://api.deepnote.com`) |
30+
| `--token <token>` | Bearer token (or `DEEPNOTE_TOKEN` env var) |
3131

3232
**Examples:**
3333

0 commit comments

Comments
 (0)