fix(clients): report a request that ran out of time as a timeout - #1418
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughThe CLI adds timeout detection and duration formatting. Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CallPublicApi
participant TrpcClient
participant SendWireRequest
participant Fetch
CallPublicApi->>TrpcClient: Send request with timeout options
TrpcClient->>SendWireRequest: Resolve and pass timeoutMs
SendWireRequest->>Fetch: Abort-controlled fetch
Fetch-->>SendWireRequest: Response or TimeoutError
SendWireRequest-->>TrpcClient: Return WireResult
TrpcClient-->>CallPublicApi: Return API result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Every platform call shared one fifteen-second deadline, and reaching it was reported as an unreachable host -- so the caller was told to check their network and QAWOLF_HOST_URL when the truth was that the CLI stopped waiting. A reached deadline is now its own wire-error kind, named as the wait it was. A call can also carry its own deadline now, for endpoints whose work outlasts a database read. Nothing asks for one yet; every call keeps the same default.
61f7368 to
5d43046
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/shell/platform/getIdentity.ts`:
- Around line 34-39: Separate response-body parsing from the initial fetch in
getIdentity.ts (34-39), fetchSignedUrl.ts (23-25), and sendWireRequest.ts
(43-48). Classify isTimeoutError failures from response.json() or
response.arrayBuffer() as timeout, while preserving parse handling for other
body errors and network classification for writeFile filesystem failures. Add
delayed-body timeout tests covering all three request paths so requestWithRetry
receives timeout errors.
In `@src/shell/platform/sendWireRequest.ts`:
- Around line 43-48: Update the response body parsing catch in sendWireRequest
to check the caught error with isTimeoutError before classifying it. Return kind
"timeout" for deadline errors raised by response.json(), while preserving kind
"parse" for other body-read failures.
- Around line 15-60: Extract the shared fetch, timeout, network, HTTP-status,
and response-body handling from sendWireRequest, getIdentity, and fetchSignedUrl
into a reusable helper, and update all three callers to use it. Ensure
TimeoutError from response body reads is classified as timeout with the
configured timeoutMs, while preserving network, HTTP, and JSON/schema parse
classifications for their respective failures.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85210c3d-a00d-48d7-a33f-a73002d1bdee
📒 Files selected for processing (18)
.changeset/patient-requests-explain.mdAGENTS.mdsrc/core/errors.test.tssrc/core/errors.tssrc/core/formatSeconds.tssrc/core/messages/auth.tssrc/shell/platform/callPublicApi.test.tssrc/shell/platform/callPublicApi.tssrc/shell/platform/createPlatformClient.testUtils.tssrc/shell/platform/createPlatformClient.timeout.test.tssrc/shell/platform/createTrpcClient.timeout.test.tssrc/shell/platform/createTrpcClient.tssrc/shell/platform/describeErrors.tssrc/shell/platform/fetchSignedUrl.tssrc/shell/platform/getIdentity.tssrc/shell/platform/requestWithRetry.tssrc/shell/platform/sendWireRequest.tssrc/shell/platform/slowFetch.testUtils.ts
Review feedback: drop comments that restate their code, and name the type-guard parameters for what they hold.
The deadline outlives the headers, so a response that arrives and then stops part-way through its body reaches it during the read. That was reported as a body we could not parse -- or, for a download, as a network fault -- which cost the caller both the retry and the true reason. A failed write is still local.
Relates to NOVA-1403
Overview of Changes
Every call the CLI makes to the platform shares one fifteen-second deadline, and reaching it was indistinguishable from a dead host — so a request the CLI simply stopped waiting for printed "Check your network connection and QAWOLF_HOST_URL", sending the caller to look in the wrong place. A reached deadline is now its own wire-error kind carrying how long it waited, described as a timeout everywhere
WireErroris turned into a message, and applied at all three fetch sites (tRPC calls, identity, signed-URL downloads).A call can also carry its own deadline now:
RequestOptions.timeoutMsthreads fromcallPublicApithrough the tRPC client to the request. Nothing asks for one yet, so every call keeps today's fifteen seconds; the point is that an endpoint whose work outlasts a database read — starting a container, say — can ask for the time it needs, with the number declared in the domain that knows why. Retry behavior is unchanged: timeouts stay retryable exactly where network errors were, and writes still never retry.The transport moved into
sendWireRequest.tsto stay under the file-length limit, and the timeout tests live in their own*.timeout.test.tsfiles for the same reason.Testing
bun run typecheck bun run lint bun run format:check bun run knip bun run test bun run buildChecklist