Skip to content

fix(shared): bound readiness response body reads - #10603

Open
yashranaway wants to merge 1 commit into
pingdotgg:mainfrom
yashranaway:fix/readiness-body-timeout
Open

fix(shared): bound readiness response body reads#10603
yashranaway wants to merge 1 commit into
pingdotgg:mainfrom
yashranaway:fix/readiness-body-timeout

Conversation

@yashranaway

@yashranaway yashranaway commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

What Changed

Put response-body consumption inside the existing per-probe HTTP readiness deadline so a stalled body is interrupted and retried.

Why

A backend or proxy can send successful headers and then stop sending its body. SSH tunnel and desktop backend startup currently spend the entire overall deadline on that one response, even if the next probe would succeed.

Checked open PR file lists and relevant patches before implementing. The two open branches touching this helper, #9376 and #10051, only change an export and do not address body deadlines.

Testing

  • Reproduced on upstream 02443335b: both stalled-body regressions fail before the fix.
  • 44 focused tests pass across shared HTTP readiness, SSH tunnel, and desktop backend manager suites.
  • Regression coverage checks recovery, request abortion, repeated body stalls, and the overall deadline using TestClock.
  • Targeted lint and shared-package typecheck pass.

Model: GPT-6 Astra
Harness: T3 code

Note

Bound response body reads under per-probe timeout in waitForHttpReady

  • Moves response-body consumption into the HttpClient pipeline before the per-probe timeout transform in httpReadiness.ts, so a stalled body now hits the probe deadline instead of hanging outside it.
  • Adds an Effect-based test suite in httpReadiness.test.ts using TestClock to verify probe interruption, retry on stalled headers/body, and overall-timeout reporting for never-completing bodies.

Macroscope summarized db0916b.

Summary by CodeRabbit

  • Bug Fixes

    • HTTP readiness checks now include response-body consumption within each probe’s timeout window.
    • Stalled readiness probes are retried correctly, with timed-out attempts canceled before the next attempt begins.
  • Tests

    • Added coverage for retries, probe cancellation, abort signals, per-probe timeouts, and overall timeout behavior.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Sep 7, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at db0916b

Macroscope's review found this PR approvable — This is a focused readiness-timeout bug fix: response-body reads now share the existing per-probe deadline, with regression tests covering retries and interruption. It introduces no schema, deployment, default, security, or static-analysis changes.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: c60046c4-d37d-4fc2-862b-bbf3b781f46b

📥 Commits

Reviewing files that changed from the base of the PR and between 09e8de9 and db0916b.

📒 Files selected for processing (2)
  • packages/shared/src/httpReadiness.test.ts
  • packages/shared/src/httpReadiness.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The readiness probe now reads the HTTP response body within the per-probe timeout. New tests cover stalled headers, stalled bodies, retries, interruption, abort signals, and timeout diagnostics.

Changes

HTTP readiness timeout handling

Layer / File(s) Summary
Include body reads in probe timeout
packages/shared/src/httpReadiness.ts
The probe reads the response body before transformation. The later body-consumption step is removed.
Validate stalled probe behavior
packages/shared/src/httpReadiness.test.ts
Tests verify retries after stalled headers or bodies, interruption of stalled probes, abort-signal state, and overall-timeout details.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to db091

HTTP readiness probes now include response-body consumption in each probe deadline, so stalled bodies can be aborted and retried rather than consuming the full readiness window. The supplied coverage indicates the intended recovery and timeout behavior is addressed, with no remaining merge-blocking risk identified.

Suggested reviewers: juliusmarminge

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description clearly explains what changed, why it changed, and how it was tested. The UI Changes section is not needed because this PR does not modify UI. The repository checklist is not included,…
Title check ✅ Passed The title clearly and concisely describes the main change: limiting readiness response-body reads to the per-probe timeout.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant