Uh oh!
There was an error while loading. Please reload this page.
fix: increase DEFAULT_CHUNK_TIMEOUT from 2min to 5min - #844
Conversation
The 120s SSE chunk timeout was too aggressive for slow LLM providers, causing spurious "SSE read timed out" aborts during long-running multi-tool sessions. Increase to 300s to reduce false positives.
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
| // altimate_change end | ||
| const DEFAULT_CHUNK_TIMEOUT = 120_000 | ||
| const DEFAULT_CHUNK_TIMEOUT = 300_000 |
There was a problem hiding this comment.
PLease set change markers on any lines we change from upstream
dev-punia-altimate
left a comment
There was a problem hiding this comment.
Multi-Persona Review — Verdict: block
The PR increases the SSE chunk timeout from 2 to 5 minutes, which improves user experience during slow LLM responses but introduces critical risks: it may mask genuine service outages (product-gap, high severity) and increase exposure to DoS via connection exhaustion (security, medium severity). Multiple personas and code review independently flag this as a dangerous trade-off without mitigations like circuit-breakers or configurability. The product manager explicitly requests changes due to the user experience gap, and security raises a valid availability concern. Without mitigation, this change is unsafe to ship.
14/14 agents completed · 115s · 2 findings (0 critical, 1 high, 1 medium)
High
- [product-manager, code-reviewer, tech-lead, cto, devops] Increasing the global SSE chunk timeout to 5 minutes may mask genuine service hangs, delaying failure detection and degrading user experience during outages, without a circuit-breaker or provider-specific timeout to distinguish slow responses from actual failures. →
packages/opencode/src/provider/provider.ts:54- 💡 Implement a circuit-breaker mechanism or provider-configurable timeout to differentiate between slow responses and genuine failures.
Medium
- [security] Increasing SSE chunk timeout from 2min to 5min may increase exposure to resource exhaustion attacks by allowing malicious clients to hold connections open longer, potentially leading to connection pool exhaustion or DoS under high load. →
packages/opencode/src/provider/provider.ts:54- 💡 Implement connection limits per client/IP, add rate limiting on SSE stream initiation, or introduce a maximum concurrent stream limit.
Multi-Persona Review · vllm:qwen3-next-80b (waves) + vllm-fallback (synth) ·
| @@ -54,7 +54,7 @@ import { VALID_ACCOUNT_RE } from "../altimate/plugin/snowflake" | |||
| import { isValidDatabricksHost } from "../altimate/plugin/databricks" | |||
There was a problem hiding this comment.
[HIGH · product-manager, code-reviewer, tech-lead, cto, devops] Increasing the global SSE chunk timeout to 5 minutes may mask genuine service hangs, delaying failure detection and degrading user experience during outages, without a circuit-breaker or provider-specific timeout to distinguish slow responses from actual failures.
💡 Suggestion: Implement a circuit-breaker mechanism or provider-configurable timeout to differentiate between slow responses and genuine failures.
Confidence: 95/100
| @@ -54,7 +54,7 @@ import { VALID_ACCOUNT_RE } from "../altimate/plugin/snowflake" | |||
| import { isValidDatabricksHost } from "../altimate/plugin/databricks" | |||
There was a problem hiding this comment.
[MEDIUM · security] Increasing SSE chunk timeout from 2min to 5min may increase exposure to resource exhaustion attacks by allowing malicious clients to hold connections open longer, potentially leading to connection pool exhaustion or DoS under high load.
💡 Suggestion: Implement connection limits per client/IP, add rate limiting on SSE stream initiation, or introduce a maximum concurrent stream limit.
Confidence: 85/100
dev-punia-altimate
commented
Jun 5, 2026
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. cc @app/altimate-harness-bot |
dev-punia-altimate
commented
Jun 8, 2026
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo comments generated. Looks good to me. |
Uh oh!
There was an error while loading. Please reload this page.
) provider.ts DEFAULT_CHUNK_TIMEOUT (300_000, raised from upstream 120_000 in #844) and question.ts mode-aware `output` field (#937) diverged from upstream without markers, failing strict marker-guard relative to the release baseline. Wrap both so they survive upstream merges. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(check): reject non-SQL files by extension — plug v0.9.6 sanity regression The Verdaccio sanity suite's path-traversal security test (test/sanity/phases/security.sh:96-103, present + passing since PR #844 in June 2026) regressed on v0.9.6. Not the test's fault: the altimate- core 0.7.0 upgrade in #1090 introduced a `multi_statement` safety rule that echoes the offending statement text back in its error message. When `altimate check ../../../../etc/passwd` runs, the CLI reads the file, parses each line as SQL, fails, and the engine emits: ERROR ... [multi_statement]: Disallowed statement type: ROOT:X:0:0:ROOT:/ROOT:/BIN/BASH The sanity test greps case-insensitive for `root:x:0` — matches — fails the release workflow. No content actually shipped: publish + GitHub Release + Docker were all skipped when sanity failed. Fix: honor the "SQL file" claim in check.ts:479's own comment. The prior filter only checked existence — non-`.sql` files were parsed happily. Now rejects anything without a `.sql`/`.ddl` extension before it reaches the engine, so no content is parsed or echoed. Extracted the extension test as `isSqlFile()` in check-helpers so it can be unit-tested independently. Verification: - rebuilt the darwin-arm64 binary and re-ran the exact sanity reproduction locally: pre-fix leaked ROOT:X:0 line; post-fix skips with "Warning: not a SQL file (extension \"none\"), skipping: ..." - 82/82 tests in test/cli/check-e2e.test.ts pass (9 new isSqlFile cases + 73 pre-existing) - typecheck clean; marker guard clean Follow-ups (not in this PR): - The engine-side fix — altimate-core's multi_statement rule should use statement TYPE NAMES, not raw content — file with core team - Consider capping message length in check.ts finding-mappers as belt-and-braces against similar future engine-echo bugs Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(check): address bot review — .ddl in default glob, dotfile edge, isFile check, handler test + close#1130 Round 2 on the release/v0.9.6 hotfix — addresses the 5 bot findings on PR #1131 and rolls in the follow-up filed as issue #1130. - coderabbit MAJOR + cubic P2 (default-discovery gap): `**/*.sql` glob at check.ts:462 missed `.ddl` files that the new SQL_EXTENSIONS filter accepts. Now scans both extensions and dedupes. - cubic P2 (bare-extension dotfile): `isSqlFile` accepted files named literally `.sql` and `.ddl` as if the leading dot were an extension separator. Node's `path.extname` treats those as dotfiles with NO extension. Fixed to match: bare-`.sql`/`.ddl` filenames are rejected. - cubic P2 (directory with .sql suffix): a directory named `foo.sql` passed the extension filter. Added a `statSync(f).isFile()` gate that rejects directories (and symlinks-to-directories, since statSync follows symlinks) with a clear warning. - cubic P2 (test coverage gap): the `isSqlFile` tests only exercised the pure helper; if the CLI handler stopped calling the filter the tests would still pass. Added a handler-level integration test with mixed SQL/non-SQL input that asserts the warning is printed and no content leaks. Also added handler tests for the directory + bare dotfile cases. Updated the pre-existing "handles directory with .sql extension" test to match the new isFile behavior (was expecting "Error reading" from the downstream readFile crash). - #1130 follow-up (drop env-var mutation from dispatcher test files): removed the `beforeAll`/`afterAll` that mutated `process.env.ALTIMATE_TELEMETRY_DISABLED` in `test/altimate/dispatcher.test.ts` and `test/skill/release-v0.9.6-adversarial.test.ts`. Dispatcher.call already wraps every Telemetry.track in try/catch that swallows errors — the env-var was defensive against nothing and wasn't parallel-safe. Closes#1130. Verification: - 101/101 tests in check-e2e + dispatcher + release-adversarial pass - rebuilt darwin-arm64 binary; sanity reproduction still shows no leak - typecheck clean; marker guard clean Closes#1130 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(check): reject symlinks whose target isn't SQL + test hygiene Round 3 on the release/v0.9.6 hotfix — addresses 3 new bot findings on PR #1131: - coderabbit MAJOR (real leak surface): the extension filter and statSync isFile check both see the SYMLINK target, not the link. So `ln -s /etc/passwd passwd.sql` sailed through — link's .sql extension passed, statSync followed the link, /etc/passwd IS a regular file, readFileSync then read its content, and the safety renderer echoed the first line. Exact leak class this PR exists to close. Fix: `lstatSync` to detect symlinks; when one is present, `realpathSync` to resolve the target and require ITS extension to also be SQL. Preserves `link.sql -> real.sql` (both SQL); rejects `passwd.sql -> /etc/passwd`. Two new handler-level regression tests: • "handler rejects symlink whose target is NOT a SQL file" • "handler ACCEPTS symlink whose target IS a SQL file" Reproduced the attack locally with the built binary: pre-fix leaked the file's first line; post-fix skips cleanly. - coderabbit MAJOR (spy hygiene in check-e2e.test.ts): the spyOn calls on process.stdout.write / process.stderr.write / console.error in beforeEach were never restored. Bun doesn't auto-restore spyOn across test files. Added `mock.restore()` in afterEach. - coderabbit MAJOR (dispatcher singleton isolation): same class of finding as the env-var one — the tests mutate module-wide `nativeHandlers`/`_ensureRegistered`. Not fixable without an instance-per-test refactor of the whole Dispatcher module. Documented the concurrency contract in-file: safe under bun's default sequential test-file execution; #1130 already tracks the broader parallel-safety cleanup this class of finding calls for. Same disposition as the release-adversarial file. Verification: - 103/103 tests pass (2 new symlink cases) - rebuilt darwin-arm64 binary; symlink attack repro shows the warning and no content leak - typecheck clean; marker guard clean Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(check): reject all symlinks — close TOCTOU race coderabbit MAJOR on prior round: my "resolve symlink target + check extension" approach opened a TOCTOU race. Between validation (realpathSync + isSqlFile) and readFileSync(f), an attacker with write access to the parent directory can swap ``passwd.sql`` from pointing at ``real.sql`` to pointing at ``/etc/passwd``. The read then follows the new link and echoes the file's content — the exact leak class this PR closes. Closing the race properly requires open-once + fstat + read-from-fd plumbed through every caller — a big refactor for a CLI most invocations don't hit. Simpler + secure: refuse symlinks entirely. Users who need to check a linked file pass the resolved target directly. Reverses cubic's earlier "accept link-to-SQL" request. The tradeoff (lose the accept-link-to-SQL convenience) favors simplicity + security over convenience — noted in the coderabbit thread reply. Also updates the pre-existing "handles symlinked SQL files" test to match the new rejection behavior. 103/103 tests pass; typecheck + marker guard clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Haider <haider@altimate.ai> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Problem
The
wrapSSE()function inprovider.tsenforces a per-chunk timeout: if no SSE chunk arrives within the window, it aborts the stream withError("SSE read timed out"). The previous 120s window was too tight for slow LLM providers (e.g. large reasoning models, cold starts) during multi-turn tool-call sessions.When this fires, the error propagates through
AbortSignal.any()in the custom fetch wrapper and surfaces asUnknownError: SSE read timed outon the assistant message — the chat freezes with no retry path.Change
packages/opencode/src/provider/provider.ts5 minutes gives adequate headroom for slow providers without masking genuine hangs.
Companion PR
AltimateAI/vscode-altimate-mcp-server#343 — adds a retry button for
MessageAbortedErrorin the chat UI, covering the session-restart abort path.Requested by @saravmajestic via harness
Summary by cubic
Increase SSE per-chunk timeout from 2 minutes to 5 minutes by raising
DEFAULT_CHUNK_TIMEOUTto 300_000, giving slow LLM streams more headroom during long tool-call sessions. This reduces false “SSE read timed out” aborts and prevents chat freezes.Written for commit 5f6e687. Summary will update on new commits. Review in cubic