fix(build): predictable error handling + transient-read retry for build logs#104
Open
kristof-siket wants to merge 1 commit into
Open
fix(build): predictable error handling + transient-read retry for build logs#104kristof-siket wants to merge 1 commit into
kristof-siket wants to merge 1 commit into
Conversation
…ld logs
`build logs` now maps a 401 from the streaming endpoint to the shared
authRequiredError ("run prisma-cli auth login") instead of a generic
"Failed to read build logs" — the SDK returns the 401 as a response, so
the command-runner's SDKAuthError mapping never fired for this path.
The read (open + NDJSON consumption) now runs inside a bounded retry
loop: at most 3 attempts, backing off ~500ms then ~1500ms, abort-aware.
It retries a transient open status (408/429/5xx), a non-Abort network
error, or the Management API's existing retryable terminal error record,
resuming each reconnect from the last cursor so output isn't reprinted.
401, 404, other non-transient statuses, and non-retryable terminals are
surfaced immediately. --follow reconnects through the same bounded loop.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
Summary by CodeRabbit
Walkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/controllers/build.ts`:
- Around line 251-274: The build flow in build() currently lets terminal errors
with retryable false fall through to writeBuildLogRecord() and then return done,
which can incorrectly succeed. Update the terminal-record handling branch in the
record-processing loop to detect non-retryable terminal errors (using
record.type, record.kind, and record.retryable) and immediately fail via a
distinct fatal-terminal outcome or by setting process.exitCode = 1, while
keeping retryable terminals in the existing retryable-terminal path.
In `@packages/cli/tests/build-logs-controller.test.ts`:
- Around line 87-212: The build logs controller tests are missing coverage for
two retry paths in the controller logic: a rejected GET that is not an
AbortError, and an AbortError raised during the retry backoff. Add one focused
test for each branch in build-logs-controller.test.ts, using runWithClient and
the existing get stub to drive the specific failure mode and assert the expected
retry/exit behavior so regressions in the controller’s retry handling are
caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b386456-f543-43a0-96fd-4b8d48a6b376
📒 Files selected for processing (2)
packages/cli/src/controllers/build.tspackages/cli/tests/build-logs-controller.test.ts
Comment on lines
+251
to
+274
| if ( | ||
| record.type === "terminal" && | ||
| record.kind === "error" && | ||
| record.retryable | ||
| ) { | ||
| retryableTerminal = record; | ||
| return; | ||
| } | ||
| writeBuildLogRecord(context, record); | ||
| }); | ||
| } catch (error) { | ||
| if (isAbortError(error) || context.runtime.signal.aborted) { | ||
| throw error; | ||
| } | ||
| writeBuildLogRecord(context, record); | ||
| }); | ||
| return { outcome: { kind: "retryable-network" }, cursor: latestCursor }; | ||
| } | ||
|
|
||
| if (sawError) { | ||
| process.exitCode = 1; | ||
| if (retryableTerminal) { | ||
| return { | ||
| outcome: { kind: "retryable-terminal", record: retryableTerminal }, | ||
| cursor: latestCursor, | ||
| }; | ||
| } | ||
| return { outcome: { kind: "done" }, cursor: latestCursor }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make non-retryable terminal errors fail the command.
A terminal { kind: "error", retryable: false } falls through to writeBuildLogRecord() and then returns done, so the command can exit 0 after an error terminal. Return a distinct fatal-terminal outcome or set process.exitCode = 1 for non-retryable terminal errors. As per PR objectives, “Non-transient cases like 404 and non-retryable terminal errors remain immediate failures.” <pr_objectives>
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/src/controllers/build.ts` around lines 251 - 274, The build flow
in build() currently lets terminal errors with retryable false fall through to
writeBuildLogRecord() and then return done, which can incorrectly succeed.
Update the terminal-record handling branch in the record-processing loop to
detect non-retryable terminal errors (using record.type, record.kind, and
record.retryable) and immediately fail via a distinct fatal-terminal outcome or
by setting process.exitCode = 1, while keeping retryable terminals in the
existing retryable-terminal path.
Comment on lines
+87
to
+212
| describe("build logs controller", () => { | ||
| it("maps a 401 to the shared auth-required error", async () => { | ||
| const get = vi.fn().mockResolvedValue(errorResult(401)); | ||
| const { run } = await runWithClient(get); | ||
|
|
||
| await expect(run).rejects.toMatchObject({ | ||
| code: "AUTH_REQUIRED", | ||
| domain: "auth", | ||
| }); | ||
| expect(get).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("retries a retryable terminal error and resumes from the cursor", async () => { | ||
| const get = vi | ||
| .fn() | ||
| .mockResolvedValueOnce( | ||
| streamResult([logLine("first", "c1"), retryableTerminal("c1")]), | ||
| ) | ||
| .mockResolvedValueOnce( | ||
| streamResult([ | ||
| logLine("second", "c2"), | ||
| { | ||
| type: "terminal", | ||
| kind: "end", | ||
| code: "end", | ||
| retryable: false, | ||
| cursor: "c2", | ||
| message: "", | ||
| }, | ||
| ]), | ||
| ); | ||
| const { run, stdout } = await runWithClient(get); | ||
|
|
||
| await run; | ||
|
|
||
| expect(process.exitCode).toBeUndefined(); | ||
| expect(stdout.buffer).toContain("first"); | ||
| expect(stdout.buffer).toContain("second"); | ||
| expect(get).toHaveBeenCalledTimes(2); | ||
| expect(queryOf(get.mock.calls[0])).not.toHaveProperty("cursor"); | ||
| expect(queryOf(get.mock.calls[1])).toMatchObject({ cursor: "c1" }); | ||
| }); | ||
|
|
||
| it("exits non-zero and surfaces the message once when every attempt fails", async () => { | ||
| const get = vi | ||
| .fn() | ||
| .mockImplementation(async () => streamResult([retryableTerminal("c1")])); | ||
| const { run, stderr } = await runWithClient(get); | ||
|
|
||
| await run; | ||
|
|
||
| expect(process.exitCode).toBe(1); | ||
| expect(get).toHaveBeenCalledTimes(3); | ||
| const occurrences = | ||
| stderr.buffer.split("Failed to read build logs.").length - 1; | ||
| expect(occurrences).toBe(1); | ||
| }); | ||
|
|
||
| it("reconnects a dropped --follow stream from the cursor", async () => { | ||
| const get = vi | ||
| .fn() | ||
| .mockResolvedValueOnce( | ||
| streamResult([logLine("a", "c1"), retryableTerminal("c1")]), | ||
| ) | ||
| .mockResolvedValueOnce(streamResult([logLine("b", "c2")])); | ||
| const { run, stdout } = await runWithClient(get, { follow: true }); | ||
|
|
||
| await run; | ||
|
|
||
| expect(process.exitCode).toBeUndefined(); | ||
| expect(get).toHaveBeenCalledTimes(2); | ||
| expect(queryOf(get.mock.calls[0])).toMatchObject({ follow: "true" }); | ||
| expect(queryOf(get.mock.calls[1])).toMatchObject({ | ||
| follow: "true", | ||
| cursor: "c1", | ||
| }); | ||
| expect(stdout.buffer).toContain("a"); | ||
| expect(stdout.buffer).toContain("b"); | ||
| }); | ||
|
|
||
| it("retries a transient open status and then succeeds", async () => { | ||
| const get = vi | ||
| .fn() | ||
| .mockResolvedValueOnce(errorResult(503)) | ||
| .mockResolvedValueOnce(streamResult([logLine("ok", "c1")])); | ||
| const { run, stdout } = await runWithClient(get); | ||
|
|
||
| await run; | ||
|
|
||
| expect(process.exitCode).toBeUndefined(); | ||
| expect(get).toHaveBeenCalledTimes(2); | ||
| expect(stdout.buffer).toContain("ok"); | ||
| }); | ||
|
|
||
| it("does not retry a 404 and surfaces BUILD_NOT_FOUND", async () => { | ||
| const get = vi.fn().mockResolvedValue(errorResult(404)); | ||
| const { run } = await runWithClient(get); | ||
|
|
||
| await expect(run).rejects.toMatchObject({ code: "BUILD_NOT_FOUND" }); | ||
| expect(get).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it("does not retry a non-retryable terminal end", async () => { | ||
| const get = vi.fn().mockResolvedValue( | ||
| streamResult([ | ||
| logLine("only", "c1"), | ||
| { | ||
| type: "terminal", | ||
| kind: "end", | ||
| code: "no_logs", | ||
| retryable: false, | ||
| cursor: "c1", | ||
| message: "No logs were produced.", | ||
| }, | ||
| ]), | ||
| ); | ||
| const { run, stdout, stderr } = await runWithClient(get); | ||
|
|
||
| await run; | ||
|
|
||
| expect(process.exitCode).toBeUndefined(); | ||
| expect(get).toHaveBeenCalledTimes(1); | ||
| expect(stdout.buffer).toContain("only"); | ||
| expect(stderr.buffer).toContain("No logs were produced."); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the two untested retry branches.
This suite still never drives a rejected GET (non-AbortError) or an abort during the retry backoff, even though the controller now has dedicated logic for both. A regression in either path would currently pass unnoticed; please add one test for each branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/cli/tests/build-logs-controller.test.ts` around lines 87 - 212, The
build logs controller tests are missing coverage for two retry paths in the
controller logic: a rejected GET that is not an AbortError, and an AbortError
raised during the retry backoff. Add one focused test for each branch in
build-logs-controller.test.ts, using runWithClient and the existing get stub to
drive the specific failure mode and assert the expected retry/exit behavior so
regressions in the controller’s retry handling are caught.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two fixes to the
build logs <build_id>command's failure behavior, stacked on top of #102 (now merged tomain).1. Predictable error handling — 401 maps to the shared auth error
The command streams
GET /v1/builds/{buildId}/logsthrough openapi-fetch, which does not throw on non-2xx — a 401 arrives asresponse.status === 401. The command-runner'sSDKAuthError → authRequiredErrormapping therefore never fired for this path, so an expired/invalid CLI session surfaced as a genericBUILD_LOGS_FAILED("Failed to read build logs… / Retry").A 401 now maps to the shared
authRequiredError(["prisma-cli auth login"]), so the output is consistent with the rest of the CLI and tells the user what to actually do.404 → BUILD_NOT_FOUNDand the generic fallback are unchanged; 403 is intentionally not special-cased (the API uses 404-collapse for unauthorized access).2. Bounded retry honoring the API's
retryablesignal, resuming from cursorThe read (stream open + NDJSON consumption) now runs inside a bounded retry loop:
408, 429, 500, 502, 503, 504), a non-Abort network-style error from the GET/stream read, or the Management API's existing terminalerrorrecord withretryable: true.401 → authRequiredError,404 → BUILD_NOT_FOUND, any other non-transient status→ BUILD_LOGS_FAILED, or a terminalerrorwithretryable: false.process.exitCode = 1(terminal-record path), or the genericCliErroris thrown (transient-open / network path).--followkeeps working: a retryable drop reconnects from cursor through the same bounded loop (the attempt count stays bounded — follow does not retry unbounded).This fixes the intermittent 408-from-Durable-Streams "Failed to read build logs." seen right after a build completes.
Trace evidence
A real failure showed the two-failure pattern this PR targets:
authRequiredErrorinstead of a generic read failure.)GET …/v1/stream/build-logs) returned HTTP 408 after ~5.2s. (Now → a retryable transient that the loop reconnects from the last cursor.)Tests
New
packages/cli/tests/build-logs-controller.test.ts(deterministic, backoff injected as zeros — no real timers):authRequiredError(assertsAUTH_REQUIRED, notBUILD_LOGS_FAILED).cursor.process.exitCode = 1, message surfaced exactly once.--followdrop → reconnects from cursor withfollow=truepreserved.end/no_logs→ not retried (unchanged).pnpm --filter @prisma/cli test(562 tests) green;tsc --noEmit,tsdownbuild, andbiome checkall clean.Notes
feat/build-logs-command(feat(build): addbuild logs <build_id>command #102). Since feat(build): addbuild logs <build_id>command #102 merged tomainfirst, the branch is rebased ontomainand targetsmain— the diff is exactly these two files (no feat(build): addbuild logs <build_id>command #102 diff). It can still be folded into the feat(build): addbuild logs <build_id>command #102 line of work if preferred.sleep, and the small phase helpers (openBuildLogStream/consumeBuildLogStream) are local tobuild.ts.🤖 Generated with Claude Code