fix: preserve compaction progress and use a 600s stall budget - #3736
fix: preserve compaction progress and use a 600s stall budget#3736Hylouis233 wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe Responses bridge now emits heartbeats for non-empty output and reasoning deltas during compaction. The default upstream stall timeout changes from 300 to 600 seconds. Tests, timeout calculations, structure documents, localized references, and Windows test-lock propagation reflect the changes. ChangesResponses stall handling
Windows test lock propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Responses compaction now records non-empty text and reasoning deltas as progress without exposing buffered output, and the default stall timeout is extended to 600 seconds. No concrete current-head merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant ResponsesStream
participant ResponsesPassthroughAdapter
participant StallWatchdog
ResponsesStream->>ResponsesPassthroughAdapter: send non-empty output or reasoning delta
ResponsesPassthroughAdapter->>StallWatchdog: emit heartbeat
StallWatchdog-->>ResponsesPassthroughAdapter: keep compaction active
ResponsesStream->>ResponsesPassthroughAdapter: send empty or malformed delta
StallWatchdog-->>ResponsesPassthroughAdapter: emit upstream_stall_timeout after deadline
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
1/4 boxes ticked. This PR stays in draft until every box above is ticked. |
리뷰 · 우선순위 74 / 80이 PR은 Responses 버퍼 컴팩션이 업스트림에서 글자·추론 델타를 계속 받는데도, 브릿지 스톨 감시가 “침묵”으로 보고 응답을 끊는 문제를 고칩니다. 지금 고치는 방법은 두 갈래입니다. 첫째, 우선순위 74인 이유다. 컴팩션·장시간 추론은 실제 Codex 사용 경로이고, HEAD의 Responses 패스스루/브릿지 불변식과 정면으로 맞닿아 있습니다. 코드 변경은 어댑터 하트비트 한 덩어리 + 기본값/문서/테스트라 머지 비용이 낮습니다. 다만 기본 스톨을 두 배로 늘리는 정책 변경이라 운영 영향(진짜 죽은 스트림이 더 오래 남는 것)은 메인테이너가 한 번 짚고 가야 합니다. 본문에 affected-module 309통과·typecheck·privacy:scan·docs 빌드는 적혀 있으나, 라인 2550 근처 (PR diff 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
The new test harness leaks a TranslatorBudget and the Japanese server configuration doc table row is malformed (missing a trailing |).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves reliability of the Responses bridge during buffered compaction by treating certain upstream deltas as progress (without leaking buffered content) and by increasing the default upstream stall watchdog budget to 600 seconds, with documentation and regression tests updated accordingly.
Changes:
- Treat non-empty
response.*.delta(text + reasoning) frames as upstream liveness during buffered compaction by yielding adapterheartbeatevents, while still keeping the completed snapshot authoritative. - Increase the default stall timeout from 300s → 600s and update unit tests accordingly.
- Add compaction progress regression tests, register them in test-layout maps, and synchronize docs defaults across locales.
File summaries
| File | Description |
|---|---|
| tests/responses/compaction-progress.test.ts | Adds regressions ensuring buffered compaction stays alive on real deltas and still hides buffered content. |
| tests/lib/stall-timeout.test.ts | Updates assertions for the new 600s default and non-finite fallback behavior. |
| tests/fixtures/test-layout-expected.json | Registers the new compaction progress test under the responses domain. |
| scripts/test-layout/layout.json | Registers the new compaction progress test under the responses domain. |
| src/stall-timeout.ts | Raises DEFAULT_STALL_TIMEOUT_SEC to 600 and updates explanatory comments. |
| src/adapters/openai-responses.ts | Emits adapter heartbeats for non-empty text/reasoning deltas so the stall watchdog sees progress during buffered compaction. |
| docs-site/src/content/docs/reference/configuration/server.md | Updates documented default stallTimeoutSec to 600. |
| docs-site/src/content/docs/reference/architecture.md | Documents buffered-compaction progress semantics and the 600s default. |
| docs-site/src/content/docs/fr/reference/configuration/server.md | Updates documented default stallTimeoutSec to 600. |
| docs-site/src/content/docs/fr/reference/architecture.md | Updates documented default stall deadline to 600. |
| docs-site/src/content/docs/ja/reference/configuration/server.md | Updates documented default stallTimeoutSec to 600. |
| docs-site/src/content/docs/ja/reference/architecture.md | Updates documented default stall deadline to 600. |
| docs-site/src/content/docs/ko/reference/configuration/server.md | Updates documented default stallTimeoutSec to 600. |
| docs-site/src/content/docs/ko/reference/architecture.md | Updates documented default stall deadline to 600. |
| docs-site/src/content/docs/ru/reference/configuration/server.md | Updates documented default stallTimeoutSec to 600. |
| docs-site/src/content/docs/ru/reference/architecture.md | Updates documented default stall deadline to 600. |
| docs-site/src/content/docs/tr/reference/configuration/server.md | Updates documented default stallTimeoutSec to 600. |
| docs-site/src/content/docs/tr/reference/architecture.md | Updates documented default stall deadline to 600. |
| docs-site/src/content/docs/zh-cn/reference/configuration/server.md | Updates documented default stallTimeoutSec to 600. |
| docs-site/src/content/docs/zh-cn/reference/architecture.md | Updates documented default stall deadline to 600. |
| docs-site/src/content/docs/zh-tw/reference/configuration/server.md | Updates documented default stallTimeoutSec to 600. |
| docs-site/src/content/docs/zh-tw/reference/architecture.md | Updates documented default stall deadline to 600. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 723348c33d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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>
|
Thank you for the compaction progress fix. Closing this PR as superseded by #3792 ( The proposed global 600-second default was not adopted. If a default change is still needed, please open a narrowly scoped proposal/PR with:
Do not recarry the already shipped compaction-progress implementation. A new default requires its own review; this closure does not approve 600 seconds. |
Primary work delivered — superseded
Content-free compaction progress shipped through #3792. The global 600-second default is an unadopted separate proposal; the released default remains 300 seconds. Both the main delivery and its retained scope are documented in the closure comment. Released in v2.46.0.
Summary
Buffered Responses compaction accumulates text until EOF without reporting upstream progress. The stall watchdog can therefore cancel a response even while text or reasoning deltas continue to arrive.
Emit content-free progress events for non-empty text and reasoning deltas while preserving buffered output and the authoritative completed snapshot. Gateway comments, empty deltas, and malformed deltas do not reset the watchdog.
Increase the default stall budget from 300 to 600 seconds while preserving explicit finite overrides.
Add compaction regressions, register them in both test-layout maps, and synchronize documented defaults across all eight locales.
Verification
Regression-first check: the original adapter failed all three active-progress cases; the corrected adapter passed all eight focused tests, including the timeout-default checks.
Affected-module run: 309 passed, 0 failed across compaction, Responses passthrough, bridge lifecycle, cancellation, timeout resolution, and test-layout guards:
bun scripts/test.ts --parallel=2 tests/lib/stall-timeout.test.ts tests/responses/compaction-progress.test.ts tests/responses/openai-responses-passthrough.test.ts tests/responses/responses-compaction.test.ts tests/responses/responses-compaction-routing.test.ts tests/adapters/bridge-lifecycle.test.ts tests/responses/passthrough-abort.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts
bun run typecheck— passed.bun run privacy:scan— passed.cd docs-site && bun run build— passed; 425 pages built.Offline installed-source probes on Windows and macOS passed for active-progress completion and comment-only expiry. Both installations were restarted with an explicit 600-second setting.
Independent behavior and privacy reviews passed. The behavior review found missing test-layout entries; both entries were added and the finding was rechecked.
Incomplete broad run:
bun scripts/test.ts --changed=origin/devexceeded the wrapper's 900-second suite limit (exit 124). No complete per-test result was produced, so this run is not claimed as passing. The affected-module run above completed successfully afterward.Follow-up review fixes: use the shared test-budget cleanup helper, update derived web-search defaults to 630 seconds while retaining explicit overrides, correct stale architecture statements, and normalize the Japanese table delimiter. 78 tests passed, 0 failed across the updated compaction, web-search, Cursor stream-health, and timeout checks; typecheck, privacy scan, and the 425-page documentation build also passed.
The follow-up diff passed independent behavior and privacy audits. All three actionable inline discussions from the first review round have been addressed in code and resolved.
The standard local full-suite retry reached the 900-second parallel-lane limit (exit 124) after recording test failures; local CI readiness is not confirmed. Required upstream workflows are awaiting repository-administrator approval. Supplemental fork workflow 34009533322 finished with 23 successful jobs. Windows shard 1/6 failed in the existing Desktop client-lifecycle test fixture (ClientStateProbeError, child_failed), causing the aggregate CI failure. That bare-test startup failure is addressed by the follow-up below; fork results do not replace the required upstream checks.
Checklist
Scope stays focused and avoids unrelated cleanup.
Docs or release notes were updated when needed.
Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.
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
New Features
Bug Fixes
Documentation