Skip to content

fix(cursor): retry pre-header discovery eof - #3052

Draft
terrytan95 wants to merge 1 commit into
lidge-jun:devfrom
terrytan95:terry-fix/cursor-discovery-http2-retry
Draft

fix(cursor): retry pre-header discovery eof#3052
terrytan95 wants to merge 1 commit into
lidge-jun:devfrom
terrytan95:terry-fix/cursor-discovery-http2-retry

Conversation

@terrytan95

@terrytan95 terrytan95 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix Cursor live model discovery when an HTTP/2 stream ends before response headers. The adapter now classifies status-less EOF as a transient transport failure, so the existing bounded retry can recover without changing completed HTTP-status, authentication, decode, or empty-response handling.

Closes #3051

Verification

  • bun test tests/cursor-hardening.test.ts (42 pass)
  • PATH="/Users/terrytan/.nvm/versions/node/v24.3.0/bin:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin" bun run test:changed (9637 pass, 3 skip)
  • PATH="/Users/terrytan/.nvm/versions/node/v24.3.0/bin:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin" bun run typecheck
  • PATH="/Users/terrytan/.nvm/versions/node/v24.3.0/bin:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin" bun run privacy:scan
  • PATH="/Users/terrytan/.nvm/versions/node/v24.3.0/bin:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin" bun run test (exit 0; parallel phase 16345 pass, 12 skip)
  • git diff --check

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed (internal transport resilience; no user-facing configuration change).
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults; no credential or auth behavior changed.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes

    • Improved model discovery reliability when an HTTP/2 connection closes before response headers are received.
    • Automatically retries the discovery request once and successfully loads available models when the connection recovers.
  • Tests

    • Added coverage for bounded retry behavior during interrupted model discovery.

@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6d3ec6da-b351-46d3-ad32-4e9c3e9386dc

📥 Commits

Reviewing files that changed from the base of the PR and between 293a2b8 and 46125ea.

📒 Files selected for processing (2)
  • src/adapters/cursor/live-models.ts
  • tests/cursor-hardening.test.ts

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


📝 Walkthrough

Walkthrough

The Cursor HTTP/2 discovery handler now classifies pre-header stream closure as a transport failure. Tests verify one bounded retry and successful model discovery after the next request succeeds.

Changes

Cursor discovery resilience

Layer / File(s) Summary
Pre-header transport classification and retry coverage
src/adapters/cursor/live-models.ts, tests/cursor-hardening.test.ts
The handler returns a transport error when an HTTP/2 response ends before headers. The test verifies one retry, exactly two requests, and successful protobuf model discovery.

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

Merge Risk: 🟡 Moderate · up to 46125

The localized transport retry fix has focused verification and no identified architecture concerns, but the PR still marks its required readiness conditions as incomplete, so it should not be treated as merge-ready until those conditions are confirmed or explicitly accepted.

Suggested reviewers: lidge-j

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy issue #3051 by classifying pre-header HTTP/2 EOF as a transient transport failure and exercising the existing bounded retry with a deterministic test. The stated handling for compl…
Out of Scope Changes check ✅ Passed The changes are limited to the Cursor live model discovery classification and its retry test. Both files directly support issue #3051, with no unrelated code or behavior changes identified.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the Cursor discovery fix and the retry behavior for EOF before response headers. It matches the primary change in the pull request.
Full details: Linked Issues check

Explanation

The changes satisfy issue #3051 by classifying pre-header HTTP/2 EOF as a transient transport failure and exercising the existing bounded retry with a deterministic test. The stated handling for completed statuses, authentication failures, decode failures, and empty responses remains unchanged.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Aug 31, 2026
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (0/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ All CI tests are green on my local testing.
  • ⬜ I pushed my PR to the latest dev commit.
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@terrytan95
terrytan95 marked this pull request as ready for review August 31, 2026 06:07
@github-actions
github-actions Bot marked this pull request as draft August 31, 2026 06:07
@terrytan95
terrytan95 marked this pull request as ready for review August 31, 2026 06:28
@github-actions
github-actions Bot marked this pull request as draft August 31, 2026 06:28
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 71 / 80

설명

이 PR은 Cursor live model discovery가 HTTP/2에서 응답 헤더가 오기 전에 스트림이 끊길 때 생기는 간헐 실패를 고친다. 지금 dev HEAD는 293a2b887 (패키지 2.38.0)이다. 현재 src/adapters/cursor/live-models.tsfetchCursorUsableModelsHttp2Once는 스트림 end에서 status가 여전히 0이면 error: "http", detail: "HTTP unknown"으로 끝낸다. 바깥쪽 fetchCursorUsableModels의 재시도 게이트는 RETRYABLE_DISCOVERY_ERRORS = timeout|transport만 다시 시도하므로, 이 경우 재시도가 한 번도 안 나가고 카탈로그는 discovery 실패로 찍힌 뒤 오래된/정적 모델로 내려앉는다. 대시보드 Models 페이지에는 Discovery failed가 뜨는데, 같은 OAuth 계정으로 바로 다시 치면 204개 모델이 나오는 식이다.

고치는 방식은 한 줄이다다. req.on("end")에서 status === 0이면 error: "transport"와 detail HTTP/2 response ended before headers로 닫는다. 그러면 이미 있는 1회 바운드 재시도(250–500ms 지터, 재시도 timeout 최대 3초)가 그대로 살아난다. 401/403 auth, 완료된 non-2xx http, decode, empty, too_large 경로는 손대지 않는다. 테스트는 tests/cursor-hardening.test.ts의 Cursor discovery bounded retry 블록에 첫 스트림을 NGHTTP2_NO_ERROR로 닫고 두 번째에서 protobuf 200을 주는 픽스처를 추가해서 requests===2와 성공 모델을 잠근다. #3051을 Closes로 묶는다.

현재 dev 방향과도 잘 맞는다. Cursor 쪽은 이미 #2865 envelope 가드, #2868/#2876 call-id, #2900/#2903/#2910/#2936/#2940 tool-result/checkpoint 연속 작업이 HEAD에 있다. 이번 변경은 그 열차와 겹치지 않는 discovery 전송 계층의 작은 구멍이다. types.ts/config.ts 스플릿과도 무관하고, 설정 키나 새 플래그도 없다. 작성자 로컬 검증 주장(cursor-hardening 42 pass, test:changed 9637, full suite 16345)도 범위에 맞다. 다만 PR은 아직 draft이고 readiness checklist 네 칸이 비어 있다.

라인 265 (PR 추가분, live-models.ts req.on end) - status===0을 transport로 재분류하는 한 줄은 맞다. 기존 status!==200 분기가 status||"unknown"으로 HTTP unknown을 만들던 경로를 먼저 끊는다.
라인 268 (현재 HEAD) - status!==200 분기는 그대로 두고 auth(401/403)도 그대로라서 재시도 범위가 넓어지지 않는다. 좋다.
경로/심볼 RETRYABLE_DISCOVERY_ERRORS - timeout|transport만 재시도하는 계약이 이 한 줄로 실제로 먹히게 된다. 새 상수나 새 재시도 루프를 만들지 않은 점이 좋다.
경로/심볼 tests/cursor-hardening.test.ts 새 테스트 - 이슈에 적힌 NGHTTP2_NO_ERROR 픽스처를 그대로 옮겼다. 기존 timeout 재시도 / auth 비재시도 / non-2xx 비재시도 테스트 옆에 두어서 회귀 범위가 분명하다.
경로/심볼 isDraft=true - 머지 전 ready로 올리고 checklist를 채워야 한다. mergeable은 MERGEABLE이다.

메인테이너의 판단이 필요한 지점

  • draft를 ready로 올린 뒤 full CI(특히 Windows/dispatch)를 한 번 더 볼지, 로컬 suite 주장만으로 바로 받을지
  • HTTP/1.1 경로(fetchCursorUsableModelsHttp1Once)에도 같은 pre-header 공백이 있는지 — 이번 PR 범위 밖이지만 upstreamHttpVersion 핀 사용자에게도 증상 보고가 있는지
  • status===0이지만 body chunk가 이미 온 기형 케이스를 더 엄격히 막을지(지금은 end에서 status만 본다)

너의 추천

작성자에게 checklist를 채우고 draft를 ready로 올리라고 한 뒤, 그 상태로 머지해도 된다. 한 줄 수정 + 이슈 재현 픽스처 테스트라서 스코프가 깨끗하고 #3051을 닫는다. types/config 스플릿과 무관하니 close-don't-rebase 대상이 아니다. 머지 후 #3051은 Closes로 자동 종료되는지 확인하면 끝.

이 댓글은 grok-bot이 작성했습니다

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants