-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(devin): stop the header deadline from killing turns that are still generating #4450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,8 +46,33 @@ import { resolveDevinApiBaseUrl } from '../../../oauth/devin/api-base.js'; | |
| * we only trigger when the server has genuinely stopped responding. | ||
| */ | ||
| const CLOUD_STREAM_IDLE_MS = 120_000; | ||
| /** Time-to-first-byte timeout. */ | ||
| const CLOUD_STREAM_TTFB_MS = 60_000; | ||
| /** | ||
| * Budget for the response HEADERS, which is not the same thing as a connect | ||
| * timeout. Cognition holds the headers until the model produces its first | ||
| * token, so on a high-effort reasoning model this bounds generation. A 60s | ||
| * value killed live swe-2 high turns at exactly 60000ms with no output while | ||
| * a sibling call on the same account was still alive at 76s, which is the | ||
| * defect this constant exists to record. | ||
| * | ||
| * It has to be at least as generous as the body idle budget above. The cost of | ||
| * the larger value is bounded and understood: a peer that goes silent at the | ||
| * TCP level without sending RST/FIN now hangs for this long instead of 60s. A | ||
| * peer that actually dies still rejects immediately. This timer is the only | ||
| * bound on that case once `timeout: 0` is set on the fetch, so it must not be | ||
| * removed. Override with OPENCODEX_DEVIN_TTFB_MS. | ||
| */ | ||
| const CLOUD_STREAM_HEADERS_DEFAULT_MS = 300_000; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This changes the Devin adapter's streaming deadline and error-mapping contract, but none of the AGENTS.md reference: src/AGENTS.md:L11-L11 Useful? React with 👍 / 👎. |
||
| /** Upper bound for the override, so a stray value cannot wedge a turn forever. */ | ||
| const CLOUD_STREAM_HEADERS_MAX_MS = 1_800_000; | ||
| function cloudStreamHeadersMs(): number { | ||
| const raw = process.env.OPENCODEX_DEVIN_TTFB_MS?.trim(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Operators can now change request behavior through AGENTS.md reference: src/AGENTS.md:L29-L29 Useful? React with 👍 / 👎. |
||
| if (!raw) return CLOUD_STREAM_HEADERS_DEFAULT_MS; | ||
| const parsed = Number(raw); | ||
| if (!Number.isFinite(parsed) || parsed <= 0) return CLOUD_STREAM_HEADERS_DEFAULT_MS; | ||
| return Math.min(parsed, CLOUD_STREAM_HEADERS_MAX_MS); | ||
| } | ||
| /** Test seam for the headers budget; the resolver itself stays private. */ | ||
| export const cloudStreamHeadersMsForTests = cloudStreamHeadersMs; | ||
| /** Maximum acceptable Connect-RPC frame length (16 MB). */ | ||
| const MAX_FRAME_LEN = 16 * 1024 * 1024; | ||
|
|
||
|
|
@@ -1115,12 +1140,24 @@ export async function* streamChatEvents(req: CloudChatRequest): AsyncGenerator<C | |
| const framed = frameConnectStream(proto, false); | ||
| const body = new Blob([new Uint8Array(framed)], { type: "application/connect+proto" }); | ||
|
|
||
| // Compose caller signal with a TTFB timeout. If the cloud takes longer | ||
| // than CLOUD_STREAM_TTFB_MS to start the response, abort. Once any byte | ||
| // arrives we cancel the TTFB timer and start the per-chunk idle timer | ||
| // inside the read loop instead. | ||
| // Compose the caller signal with a deadline on the response HEADERS. The | ||
| // timer is cleared in the finally below, which runs when `await fetch` | ||
| // resolves — and fetch resolves on headers, not on the first body byte. An | ||
| // earlier comment here claimed "once any byte arrives", which was wrong and | ||
| // hid the defect: Cognition withholds headers until the first token, so this | ||
| // budget is a generation deadline. Body silence after headers is a separate | ||
| // budget, the per-chunk idle timer in the read loop below. | ||
| const ttfbController = new AbortController(); | ||
| const ttfbTimer = setTimeout(() => ttfbController.abort(new Error(`cloud-direct: time-to-first-byte timeout (${CLOUD_STREAM_TTFB_MS}ms)`)), CLOUD_STREAM_TTFB_MS); | ||
| const headersMs = cloudStreamHeadersMs(); | ||
| // Abort with no reason and remember that we are the one who fired. Bun rejects | ||
| // the fetch with its own AbortError rather than handing back `signal.reason`, | ||
| // so attaching a typed error to abort() would be discarded; the catch below is | ||
| // what actually produces a classifiable failure. | ||
| let headersDeadlineFired = false; | ||
| const ttfbTimer = setTimeout(() => { | ||
| headersDeadlineFired = true; | ||
| ttfbController.abort(); | ||
| }, headersMs); | ||
| const ttfbSignal = ttfbController.signal; | ||
| // Compose req.signal + ttfbSignal. AbortSignal.any was added in Node | ||
| // 20.3 / Bun 1.0; our `engines` allows Node ≥18, so on Node 18-20.2 the | ||
|
|
@@ -1148,7 +1185,28 @@ export async function* streamChatEvents(req: CloudChatRequest): AsyncGenerator<C | |
| body, | ||
| redirect: 'error', | ||
| signal: initialSignal, | ||
| }); | ||
| // Bun applies its own fetch idle timeout (~5 minutes) on top of ours. | ||
| // Two independent deadlines on the same hop means the shorter one wins | ||
| // silently and this function can no longer explain its own failure, so | ||
| // the deadline above is made the single authority. Same reason as | ||
| // src/server/responses/fetch-helpers.ts. | ||
| timeout: 0, | ||
| } as RequestInit); | ||
| } catch (err) { | ||
| if (headersDeadlineFired) { | ||
| // Ours, not the upstream failing. Raised as a typed error with an explicit | ||
| // status because devinErrorClassification reads CloudChatError.status and | ||
| // would otherwise return {} for a bare Error, leaving src/lib/errors.ts to | ||
| // guess from the message text. The message deliberately no longer says | ||
| // "timeout", so the status is the only thing carrying the classification. | ||
| throw new CloudChatError( | ||
| `cloud-direct: no response headers within ${headersMs}ms`, | ||
| undefined, | ||
| undefined, | ||
| 504, | ||
| ); | ||
| } | ||
| throw err; | ||
| } finally { | ||
| clearTimeout(ttfbTimer); | ||
| // The composed signal only guards the headers hop; the body is cancelled | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { readFileSync } from "node:fs"; | ||
| import { cloudStreamHeadersMsForTests, CloudChatError } from "../../src/adapters/devin/cloud-direct/chat"; | ||
| import { devinErrorClassification } from "../../src/adapters/devin"; | ||
| import { repoPath } from "../helpers/repo-root"; | ||
|
|
||
| const CHAT_SRC = readFileSync(repoPath("src/adapters/devin/cloud-direct/chat.ts"), "utf8"); | ||
|
|
||
| function withEnv(value: string | undefined, run: () => void): void { | ||
| const key = "OPENCODEX_DEVIN_TTFB_MS"; | ||
| const before = process.env[key]; | ||
| if (value === undefined) delete process.env[key]; | ||
| else process.env[key] = value; | ||
| try { | ||
| run(); | ||
| } finally { | ||
| if (before === undefined) delete process.env[key]; | ||
| else process.env[key] = before; | ||
| } | ||
| } | ||
|
|
||
| describe("cloud-direct response-headers deadline", () => { | ||
| // A 60s budget killed live swe-2 high turns at exactly 60000ms with no output | ||
| // while a sibling call on the same account was still alive at 76s. Cognition | ||
| // withholds headers until the first token, so this budget bounds generation. | ||
| test("the default outlives a reasoning model that thinks past a minute", () => { | ||
| withEnv(undefined, () => { | ||
| expect(cloudStreamHeadersMsForTests()).toBe(300_000); | ||
| }); | ||
| }); | ||
|
|
||
| test("the headers budget is not shorter than the body idle budget", () => { | ||
| // Inverting these is what made the pre-header window the tightest part of a | ||
| // long turn, which is backwards. | ||
| const idle = Number(/CLOUD_STREAM_IDLE_MS = ([0-9_]+)/.exec(CHAT_SRC)?.[1]?.replace(/_/g, "")); | ||
| expect(idle).toBeGreaterThan(0); | ||
| withEnv(undefined, () => { | ||
| expect(cloudStreamHeadersMsForTests()).toBeGreaterThanOrEqual(idle); | ||
| }); | ||
| }); | ||
|
|
||
| test("an operator override is honoured", () => { | ||
| withEnv("1000", () => { | ||
| expect(cloudStreamHeadersMsForTests()).toBe(1000); | ||
| }); | ||
| }); | ||
|
|
||
| test("an override is clamped so a stray value cannot wedge a turn forever", () => { | ||
| withEnv("999999999", () => { | ||
| expect(cloudStreamHeadersMsForTests()).toBe(1_800_000); | ||
| }); | ||
| }); | ||
|
|
||
| test.each(["", " ", "0", "-5", "not-a-number"])("a useless override %p falls back to the default", (raw) => { | ||
| withEnv(raw, () => { | ||
| expect(cloudStreamHeadersMsForTests()).toBe(300_000); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe("cloud-direct headers-deadline failure is ours, not the upstream", () => { | ||
| // The old abort raised a bare Error, so devinErrorClassification returned {} | ||
| // and the failure was inferred from message text as an upstream 502/504. | ||
| test("the deadline error classifies as a gateway timeout the caller may retry", () => { | ||
| const err = new CloudChatError("cloud-direct: no response headers within 300000ms", undefined, undefined, 504); | ||
| expect(devinErrorClassification(err)).toEqual({ status: 504, retryable: true }); | ||
| }); | ||
|
|
||
| test("a bare Error still classifies as nothing, which is why the status is set explicitly", () => { | ||
| expect(devinErrorClassification(new Error("cloud-direct: no response headers within 300000ms"))).toEqual({}); | ||
| }); | ||
|
|
||
| test("the message no longer claims a first-byte measurement it cannot make", () => { | ||
| // Nothing is on the wire before headers, so "time-to-first-byte" described a | ||
| // measurement that does not exist. Dropping the word "timeout" from it is why | ||
| // the explicit 504 above is mandatory rather than cosmetic. | ||
| expect(CHAT_SRC).not.toContain("time-to-first-byte"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("cloud-direct headers deadline guards", () => { | ||
| test("the abort callback records that we fired before aborting", () => { | ||
| // Bun rejects the fetch with its own AbortError instead of handing back | ||
| // signal.reason, so a typed error passed to abort() would be discarded and | ||
| // the catch could not tell our deadline from a caller cancel. | ||
| expect(CHAT_SRC).toMatch(/headersDeadlineFired = true;\s*\n\s*ttfbController\.abort\(\);/); | ||
| expect(CHAT_SRC).not.toMatch(/ttfbController\.abort\(new /); | ||
| }); | ||
|
|
||
| test("the GetChatMessage fetch disables the competing runtime timeout", () => { | ||
| // Two independent deadlines on one hop means the shorter wins silently and | ||
| // this function can no longer explain its own failure. | ||
| const call = CHAT_SRC.slice(CHAT_SRC.indexOf("ApiServerService/GetChatMessage")); | ||
| expect(call.slice(0, call.indexOf("} as RequestInit"))).toContain("timeout: 0"); | ||
| }); | ||
|
|
||
| test("a caller cancel is re-thrown unchanged", () => { | ||
| expect(CHAT_SRC).toMatch(/if \(headersDeadlineFired\) \{[\s\S]*?\}\s*\n\s*throw err;/); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Run the required validation before merge.
scripts/test-layout/layout.json:644is configuration consumed by thescripts/test-layouttooling. Runbun scripts/test-layout/verify.ts --domain providers,bun run typecheck, andbun run privacy:scan. Report any platform-specific validation that was not executed.🤖 Prompt for AI Agents