Skip to content

fix(server): opt the compact route out of the request idle timeout - #3744

Closed
mashfromband wants to merge 1 commit into
lidge-jun:devfrom
mashfromband:fix/compact-route-request-timeout
Closed

fix(server): opt the compact route out of the request idle timeout#3744
mashfromband wants to merge 1 commit into
lidge-jun:devfrom
mashfromband:fix/compact-route-request-timeout

Conversation

@mashfromband

@mashfromband mashfromband commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Delivered — superseded by merged work

Verified in dev at 5759d9ea2f1e7281cdc01eb9628f2e0a123fb59c: #3792 (823ffeb771).

Original contribution: fix(server): opt the compact route out of the request idle timeout, by @mashfromband.

Accepted compact-request lifetime, with complete-body admission and bounded response-body inactivity.

The original PR is closed as superseded; its contribution remains credited in the landed history.

Attribution strengthened by #3811, merged as cf9f662190c4c6770697c45c870941509cc98f9c. See CREDITS.md for the source-to-landing attribution record.

Summary

  • /v1/responses/compact collects the whole upstream compaction before it sends the first byte, so nothing keeps the client connection alive while a long remote compact is in flight. The server-level idleTimeout (255 s) then closes the socket and the client receives a truncated body (Codex reports stream disconnected before completion: ... error decoding response body from Error running remote compact task).
  • The other buffered data-plane routes (/v1/images/*, /v1/alpha/search, /v1/messages, /v1/chat/completions, /v1/live) already opt out with disableResponsesRequestTimeout(req, requestServer). This PR makes the compact route do the same, right after admission and the origin gate.
  • Adds a source contract test next to the existing timeout-helper tests that pins the call inside the compact branch (fails on dev without the fix, passes with it).

Reproduced locally with a fake upstream that stays silent for >255 s on the compact request: before the change the client saw a cut body at ~255 s, after it the compaction completed. The GoDD-AO proxy in front of this gateway has carried the same one-line change as a local patch since 2.43.0; this upstreams it.

Verification

  • bun test tests/server/server-auth.test.ts -t "compact route opts out" — 1 pass with the fix; the same test fails on dev with the fix stashed.
  • bun test tests/server/server-auth.test.ts — 107 pass, 0 fail.
  • bun x tsc --noEmit — clean.
  • bun scripts/privacy-scan.ts — passed.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (none needed: no user-facing option changed)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. (the opt-out runs after admission and the origin gate, matching the other routes)

https://claude.ai/code/session_01VcTuv1wGXdywDr8xeQftdP

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 reliability for response compaction requests by preventing long-running operations from being interrupted by the server’s idle timeout.
  • Tests

    • Added coverage to verify that response compaction requests receive the appropriate timeout handling.

`/v1/responses/compact` buffers the whole upstream turn before it answers,
so the server-level `idleTimeout` (255 s) closes the client connection under
a long remote compact even though the upstream is still working; the client
then sees a truncated body ("error decoding response body" in Codex). The
other buffered data-plane routes already call
`disableResponsesRequestTimeout(req, requestServer)`; the compact route now
does the same, and a source contract test pins the call inside that branch.

Claude-Session: https://claude.ai/code/session_01VcTuv1wGXdywDr8xeQftdP
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

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

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

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

What to do

  • Tick all four boxes in the PR description once you're done (currently 3/4).
  • The PR is more than 10 commits behind dev; the latest dev box has been unticked.
  • The checklist has been reset: re-test against the latest code and tick the boxes again.

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.

3/4 boxes ticked.

The PR is more than 10 commits behind dev; the latest dev box has been unticked.
The checklist has been reset: re-test against the latest code and tick the boxes again.
Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked.

@github-actions
github-actions Bot marked this pull request as draft September 6, 2026 04:09
@coderabbitai

coderabbitai Bot commented Sep 6, 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: Team

Run ID: a698a9d0-4463-4606-a5d8-ccb5fd645c53

📥 Commits

Reviewing files that changed from the base of the PR and between 25c8d2b and baba447.

📒 Files selected for processing (2)
  • src/server/index.ts
  • tests/server/server-auth.test.ts

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


📝 Walkthrough

Walkthrough

The /v1/responses/compact route now disables the server-level idle timeout before buffering upstream compaction. A test verifies that the timeout opt-out occurs before handleResponsesCompact.

Changes

Compact response timeout handling

Layer / File(s) Summary
Disable timeout before compaction
src/server/index.ts, tests/server/server-auth.test.ts
At src/server/index.ts:1755-1759, the compact route calls disableResponsesRequestTimeout(req, requestServer) before handleResponsesCompact. At tests/server/server-auth.test.ts:605-617, a test verifies this call order.

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

Merge Risk: ⚪ Minimal · up to baba4

Compact responses can now complete long buffered upstream work without the server idle timeout truncating the response. No current merge-blocking risk remains.

Suggested reviewers: invalid-email-address

🚥 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 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 Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change in src/server/index.ts: excluding the /v1/responses/compact route from the server request idle timeout.
  • 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.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 72 / 80

이 PR은 Codex가 부르는 원격 컴팩트 경로 /v1/responses/compact서버 쪽 HTTP idleTimeout(255초) 에 걸려서, 업스트림 컴팩션이 아직 도는 중에 클라이언트 연결이 잘리는 문제를 고칩니다. 지금 dev(HEAD 25c8d2b4e, 패키지 2.44.0, #3739 maintainer-dev-integration 직후)의 src/server/index.ts를 보면 Bun serve 옵션에 idleTimeout: 255가 걸려 있고, disableResponsesRequestTimeout(req, requestServer)로 요청마다 그 타이머를 끄는 헬퍼는 이미 src/server/responses/fetch-helpers.ts에 있습니다. 같은 파일에서 /v1/images/*, /v1/alpha/search, /v1/messages, /v1/chat/completions, /v1/live·/v1/realtime/calls 같은 버퍼형 데이터플레인은 이미 이 헬퍼를 호출합니다. 스트리밍 /v1/responses는 본문을 다 읽은 뒤 onRequestBodyRead에서 끕니다. 그런데 컴팩트 분기(약 1746행)만은 admission·origin 게이트 다음에 바로 handleResponsesCompact로 들어가고, 타임아웃 옵트아웃이 없습니다.

왜 컴팩트만 특히 위험한가. src/server/responses/compact.tsbufferCompactResponse는 업스트림 턴 전체를 모은 뒤에야 첫 바이트를 클라이언트에 보냅니다. 스트리밍처럼 중간에 바이트가 흘러 idle을 리셋해 주지 않습니다. 그래서 원격 컴팩트가 255초보다 길면 Bun이 소켓을 닫고, Codex 쪽은 stream disconnected before completion / error decoding response body 같은 잘린 본문만 봅니다. PR 본문이 말한 재현(가짜 업스트림이 255초 넘게 침묵)과도 맞습니다. GoDD-AO 프록시가 2.43.0부터 같은 한 줄을 로컬 패치로 들고 있었다는 점도, 이미 현장에서 검증된 구멍입니다.

고치는 내용은 아주 작습니다. 컴팩트 분기에서 origin 게이트를 통과한 직후 disableResponsesRequestTimeout(req, requestServer) 한 줄을 넣고, tests/server/server-auth.test.ts에 소스 계약 테스트를 추가해 "/v1/responses/compact" 분기와 await handleResponsesCompact(...) 사이에 그 호출이 있는지 고정합니다. 기존 타임아웃 헬퍼 테스트 옆에 두었고, 본문 기준으로 focused 1통과·server-auth 107통과·tsc·privacy:scan까지 적혀 있습니다. types.ts/config.ts 대분할과는 무관해서 close-don't-rebase 대상이 아닙니다.

우선순위 72인 이유다. 원격 컴팩트는 Codex 실제 사용 경로이고, HEAD에 이미 있는 형제 라우트 패턴과 한 줄로 정렬하는 독립 버그픽스라 머지 비용이 거의 없습니다. release-244 로드맵(다음 task-input 020 등)과도 충돌하지 않습니다. 참고로 열려 있는 #3736(컴팩션 진행 heartbeat + stallTimeoutSec 기본 600)은 업스트림 스톨 감시 층이고, 이 PR은 Bun 요청 idleTimeout 층이라 서로 대체하지 않습니다. 둘 다 컴팩트 장시간 실패를 줄이지만, 이 PR만으로도 255초 절단은 막힙니다. #3736을 기다릴 이유가 없습니다.

라인 1755 근처 (PR diff src/server/index.ts 컴팩트 분기) - origin 게이트 직후 disableResponsesRequestTimeout(req, requestServer) 추가. 형제 버퍼 라우트와 같은 헬퍼
경로/심볼 - src/server/index.ts idleTimeout: 255 - 컴팩트가 첫 바이트 없이 버퍼하는 동안 이 값이 소켓을 끊던 원인
경로/심볼 - src/server/responses/compact.ts bufferCompactResponse - 업스트림 EOF까지 모은 뒤 응답. idle 리셋 바이트가 없음
경로 - tests/server/server-auth.test.ts "compact route opts out…" - 분기 문자열 사이 소스 계약. 리팩터에 깨질 수 있으나 의도 고정용으로는 충분
경로 - 형제 라우트(/v1/images/* 등) - 그쪽은 drain/auth 에서 옵트아웃. 이 PR은 admission·origin . 거절된 요청은 255초 타이머를 유지해 더 보수적

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

  • 옵트아웃 위치를 형제처럼 분기 최상단(drain 전)으로 맞출지, 지금처럼 admission 뒤로 둘지 (기능 차이는 거의 없고 스타일 통일 문제)
  • #3736과 같은 컴팩트 장시간 묶음으로 볼지, 이 PR을 먼저 단독 머지할지 (권장은 단독 선머지)
  • 소스 계약 테스트만으로 충분한지, 255초+ 가짜 업스트림 통합 테스트를 CI에 넣을지 (느리고 비싸면 계약 테스트로 충분)

너의 추천
지금 dev에 바로 머지하세요. 한 줄 + 계약 테스트로 형제 라우트와 맞추는 명확한 픽스이고, #3736·task-input 열차와 겹치지 않습니다. 위치는 admission 뒤여도 안전하니 스타일 통일은 선택 사항입니다. types/config 대분할 close-don't-rebase 아님. 미리보기 배포 없음.

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

@github-actions
github-actions Bot marked this pull request as ready for review September 6, 2026 04:43
@lidge-jun lidge-jun closed this Sep 6, 2026
zigzag-007 pushed a commit to zigzag-007/opencodex that referenced this pull request Sep 6, 2026
Carry the progress portion of lidge-jun#3736 and adapt lidge-jun#3744 to release listener idle protection only after a complete valid request body. Preserve the 300-second default and existing encrypted output.

Local validation deferred to the final hosted CI by maintainer instruction.

Co-authored-by: mashfromband <matsumoto.yukuhashi@gmail.com>
Co-authored-by: Hylouis233 <88263959+Hylouis233@users.noreply.github.com>
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