fix(autocompact): retry circuit breaker after cooldown - #1375
Conversation
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The cooldown breaker change looks focused, but I think the PR should avoid closing the whole linked issue as written.
Findings
- [P2] Keep #1373 open until the remaining OOM paths are tracked or fixed
src/utils/swarm/inProcessRunner.ts:1004
#1373 reports the auto-compact breaker as the primary cause, but it also asks for an absolutestate.messagescap and calls out the in-process teammateallMessagesbuffer as another unbounded OOM path. This PR fixes the main auto-compact breaker path, but it explicitly leaves the cap and teammate-retention work out of scope. WithFixes #1373, merging this would close the issue while those reported memory-growth protections are still unresolved. Please change the closing keyword toRefs #1373/Partially addresses #1373and link a follow-up for the remaining OOM work, or include those pieces before closing the issue.
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed issue-linking concern and took another pass through the auto-compact cooldown paths. I found one small issue worth tightening before this lands.
Findings
- [P3] Either use or remove the cooldown duration in breaker resolution
src/services/compact/autoCompact.ts:97
resolveAutoCompactCircuitBreakerStateacceptscooldownMs, and the resolver tests pass different cooldown values, but the helper never reads that parameter. The current runtime path still works whennextRetryAtMswas set by the trip path, but the helper API and tests make it look like the cooldown duration participates in the decision when it does not. Please either removecooldownMsfrom this helper/tests, or have the resolver derive the active cooldown fromlastFailureAtMs + cooldownMswhen appropriate so the duration is actually covered by the unit tests.
Derive the circuit breaker retry time from lastFailureAtMs plus the configured cooldown when nextRetryAtMs is missing or invalid. Also clear SDK auto-compact tracking after manual compact boundaries and cover both paths with regressions.
|
Pushed follow-up commit a017b52 addressing the cooldown resolver finding. Changes:
Local validation:
|
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed cooldown resolver path and ran through the focused auto-compact changes again. I found one portability issue in the new regression coverage.
Findings
- [P2] Use a cross-platform cwd in the SDK cooldown fixture
src/test/fixtures/queryEngineManualCompactCooldown.fixture.ts:116
The newQueryEngine.autoCompactCooldown.test.tsfixture constructsQueryEnginewithcwd: '/tmp'. On Windows that path does not normally exist, andQueryEngine.submitMessage()callssetCwd(cwd), which rejects the fixture before the manual/compactregression can run. This makesbun test src/QueryEngine.autoCompactCooldown.test.tsfail locally on Windows withPath "/tmp" does not exist. Please use a real cross-platform temporary directory, such astmpdir()fromnode:os, for both the fixturecwdand the mocked init message.
|
Pushed follow-up commit Changes:
Review and validation:
|
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the earlier issue-linking, cooldown resolver, and Windows portability follow-ups and took another pass through the final auto-compact changes. I found one remaining issue before this is ready.
Findings
- [P2] Give the SDK cooldown fixture test more than Bun's default 5s timeout
src/QueryEngine.autoCompactCooldown.test.ts:8
This regression test now spawns a second Bun process to loadQueryEnginefrom a cold TypeScript graph. On a fresh Windows checkout,bun test src/QueryEngine.autoCompactCooldown.test.tstimed out at Bun's default 5 second per-test limit before the fixture finished, even though rerunning with a higher timeout passes. That makes the new SDK coverage flaky exactly on the platform this PR just fixed. Please set an explicit longer timeout for this test or avoid the extra-process path so the regression stays reliable on cold runs.
|
Pushed follow-up commit Changes:
Validation:
GitHub Actions on this commit are green: |
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed issue-linking, cooldown resolver, Windows fixture, and fixture-timeout paths, then took another pass through the current auto-compact cooldown changes. I do not see any remaining actionable issues from my side.
* fix(autocompact): retry circuit breaker after cooldown * fix: honor auto-compact cooldown fallback Derive the circuit breaker retry time from lastFailureAtMs plus the configured cooldown when nextRetryAtMs is missing or invalid. Also clear SDK auto-compact tracking after manual compact boundaries and cover both paths with regressions. * test: make auto-compact cooldown fixture portable * test: extend auto-compact cooldown fixture timeout
- fix(autocompact): retry circuit breaker after cooldown (Twigpine#1375) - fix(provider): require API key input when adding OpenGateway (Twigpine#1384) - fix(provider): allow remote Ollama without OPENAI_API_KEY (Twigpine#952) - fix(codex-stream): recover tool args delivered only via done events (Twigpine#1262) - fix: route MiniMax compacting through Anthropic-compatible API (Twigpine#1154) - fix(thinking): disable thinking for unsupported Ollama models (Twigpine#1376) - feat(agents): set active session agent from agents menu (Twigpine#1349) - fix(repl): show permission prompts while draft input is present (Twigpine#1393) - fix(model): include profile models in descriptor picker (Twigpine#1361) - Improve warning notice formatting (Twigpine#1415) - fix(codex): allow credential storage fallback (Twigpine#1347) - fix(attribution): make git attribution opt-in by default (Twigpine#1335) - fix(agent): allow custom model overrides (Twigpine#1337) - feat(query): robust multi-lingual and structural continuation nudge (Twigpine#1280) - fix(watchers): debounce skills and settings reload bursts (Twigpine#1370) - feat: configure API retry backoff (Twigpine#370) (Twigpine#1095) - chore(main): release 0.15.0 (Twigpine#1325) - ci: retrigger CodeQL after action download outage (Twigpine#1374) - Fix launcher heap setup for long sessions (Twigpine#1242)
Apply from upstream commit 11d59ec (8 files, 1443 lines): Core fix: when MAX_CONSECUTIVE_AUTOCOMPACT_FAILURES is hit, the cooldown circuit breaker now properly retries after the cooldown window instead of failing the whole session. - src/services/compact/autoCompact.ts (184 lines): new state nextRetryAtMs/lastFailureAtMs on AutoCompactTrackingState; new resolveAutoCompactCircuitBreakerState() helper; new getAutoCompactFailureCooldownMs() with OPENCLAUDE_AUTOCOMPACT_FAILURE_COOLDOWN_MS env override; MAX_CONSECUTIVE_AUTOCOMPACT_FAILURES now exported. - src/services/compact/autoCompact.test.ts: full rewrite with 22 new tests covering circuit-breaker state machine (allow / skip / half-open retry / user abort / stale state cleanup). - src/QueryEngine.ts: wire autoCompactTracking through query() params. - src/query.ts: integrate cooldown fallback derivation; clear SDK tracking after manual compact boundaries; expose nextRetryAtMs/lastFailureAtMs/circuitBreaker* from autocompact return value. - src/QueryEngine.autoCompactCooldown.test.ts (NEW, 41 lines): regression for SDK tracking after manual compact. - src/query/autoCompactCooldown.test.ts (NEW, 339 lines): cooldown derivation tests. - src/test/fixtures/queryEngineManualCompactCooldown.fixture.ts (NEW, 156 lines): portable manual-compact cooldown fixture. - src/screens/REPL.tsx: forward autoCompactTracking from REPL state through to query(). Type shims: // @ts-nocheck added to 3 test files where upstream test fixtures use SDKMessage variants that don't yet exist in OpenCC's main-openccv2 Message type. 27/27 tests pass; build + typecheck clean. Refs upstream PR Twigpine#1375.
…unch to bin/opencc
Cherry-pick (6522ca1d) carried upstream's pre-rename `OPENCLAUDE_*` env var
names and the heap relaunch was only applied to `bin/openclaude`. OpenCC's
real entrypoint is `bin/opencc` and the fork convention is `OPENCC_*` env
vars (per opencc-env-var-disable-naming-convention memory).
- `OPENCLAUDE_MAX_MEMORY_MB` → `OPENCC_MAX_MEMORY_MB`
(memoryPressure.ts, concurrentSessions.ts, bin/openclaude)
- `OPENCLAUDE_MAX_ACTIVE_MESSAGES` → `OPENCC_MAX_ACTIVE_MESSAGES` (query.ts)
- `OPENCLAUDE_HEAP_RELAUNCHED` / `OPENCLAUDE_DISABLE_HEAP_RELAUNCH` /
`OPENCLAUDE_NODE_MAX_OLD_SPACE_SIZE_MB` → `OPENCC_*` (bin/openclaude)
- Port `relaunchWithLongSessionHeapIfNeeded()` from bin/openclaude to
bin/opencc so the real entrypoint gets --expose-gc / 8GB heap. This is
the launcher the actual `opencc` binary (and the npm bin field) runs.
Leaves `bin/openclaude` untouched in shape (env-var renames only) so
provider-launch.ts:runProcess('node', ['bin/openclaude', ...]) still
works. `OPENCLAUDE_AUTOCOMPACT_FAILURE_COOLDOWN_MS` (autoCompact.ts:90)
is a pre-existing inconsistency from Twigpine#1375 sync — out of scope.
Summary
Refs #1373; partially addresses #1373 by fixing the auto-compact cooldown breaker. Remaining long-session OOM guards are tracked in #1379.
Why
The existing breaker stops auto-compact after 3 consecutive failures to avoid retry storms. However, once tripped, it can suppress future auto-compaction for the rest of a long-running query loop. If the conversation continues accumulating large tool results/history, memory pressure can grow until Node/V8 crashes with heap OOM.
This PR keeps the retry-storm protection but makes it recoverable.
Behavior
Out of scope
state.messagescap because arbitrary message slicing can corrupttool_use/tool_resultpairing and compact-boundary semantics; follow-up work is tracked in Track remaining long-session OOM guards after auto-compact cooldown #1379.allMessagesretention is tracked in Track remaining long-session OOM guards after auto-compact cooldown #1379.Testing
bun test src/services/compact/autoCompact.test.tsbun test src/query/autoCompactCooldown.test.tsbun run smokebun run --cwd web typecheck && bun run --cwd web buildpython -m pytest -q python/testsbun run security:pr-scan -- --base upstream/mainbun run test:providernpm run test:provider-recommendationbun run buildgit diff --checkFork CI also passed on this branch before opening the upstream PR:
smoke-and-testsandweb.Local full-suite note:
bun test --max-concurrency=1was run in a sanitized environment and had one unrelated baseline failure insrc/utils/conversationRecovery.hooks.test.ts(deserializeMessagesWithInterruptDetection strips thinking blocks only for OpenAI-compatible providers). The focused and CI-relevant suites above pass.Notes
The breaker remains intentionally conservative. Cooldown retry is half-open rather than a full reset, so an unrecoverable compaction failure causes one retry per cooldown window rather than another burst of repeated attempts.