Skip to content

fix(agents): preserve active compaction retries#94421

Merged
steipete merged 2 commits into
mainfrom
fix/compaction-retry-active-94391
Jun 18, 2026
Merged

fix(agents): preserve active compaction retries#94421
steipete merged 2 commits into
mainfrom
fix/compaction-retry-active-94391

Conversation

@steipete

@steipete steipete commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Preserve the existing 60-second idle recovery guard for lost compaction retry resolution.
  • Treat the retry model request as active work while the agent session is streaming, so valid long-running retries are not abandoned at 60 seconds.
  • Add regression coverage for active retry work and the idle fallback.

This fixes long-session compaction retries without adding a config surface or coupling retry duration to the separate compaction-call timeout.

Linked context

Closes #94391

Related #40324

Real behavior proof

  • Behavior or issue addressed: A post-compaction retry model request lasting longer than 60 seconds was treated as idle because compaction itself had ended, so the aggregate guard discarded a still-valid result.
  • Real environment tested: macOS source checkout and an isolated Azure-backed Linux test box using the repository Node/pnpm toolchain.
  • Exact steps or command run after this patch: pnpm test src/agents/embedded-agent-runner/run/compaction-retry-aggregate-timeout.test.ts src/agents/embedded-agent-subscribe.subscribe-embedded-agent-session.waits-multiple-compaction-retries-before-resolving.test.ts src/agents/embedded-agent-runner/run/compaction-timeout.test.ts; then a source-level runtime probe with a 60 ms aggregate window and a 170 ms active retry.
  • Evidence after fix: 31 focused tests passed on Linux. The runtime probe printed PASS active retry completed without timeout after 171ms.
  • Observed result after fix: The aggregate guard extends while either compaction or the retry model run is active, then retains the existing timeout once both phases are idle.
  • What was not tested: A paid 200K-token Anthropic compaction session.
  • Proof limitations or environment constraints: The field report already contains the real 143-187 second provider trajectories. The deterministic probe compresses the same timing relationship to milliseconds and exercises the production helper and lifecycle predicate.
  • Before evidence: Current source used only the compaction-in-flight flag. During the post-compaction retry, that flag is false even though the agent session remains streaming.

Tests and validation

  • pnpm test src/agents/embedded-agent-runner/run/compaction-retry-aggregate-timeout.test.ts -- --reporter=verbose
  • pnpm test src/agents/embedded-agent-runner/run/compaction-retry-aggregate-timeout.test.ts src/agents/embedded-agent-subscribe.subscribe-embedded-agent-session.waits-multiple-compaction-retries-before-resolving.test.ts src/agents/embedded-agent-runner/run/compaction-timeout.test.ts
  • Same focused suite plus the active-retry runtime probe on remote Linux: 31 tests passed; probe passed.
  • autoreview --mode local --engine codex --model gpt-5.5 --thinking high: clean, no actionable findings.

Risk checklist

Did user-visible behavior change? (Yes)

Long-running valid compaction retries are allowed to finish instead of silently falling back at 60 seconds.

Did config, environment, or migration behavior change? (No)

Did security, auth, secrets, network, or tool execution behavior change? (No)

Highest risk: extending a genuinely stuck retry. Mitigation: the retry is extended only while the session runtime reports active streaming; once idle, the original 60-second lost-resolution fallback still applies, and the enclosing run abort deadline remains authoritative.

Current review state

Ready for CI and maintainer review. No unresolved autoreview findings.

@steipete steipete self-assigned this Jun 18, 2026
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S maintainer Maintainer-authored PR labels Jun 18, 2026
chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed Jun 18, 2026
View reviewed changes

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c61a6ed09a

ℹ️ 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".

Comment thread src/agents/embedded-agent-runner/run/attempt.ts
@clawsweeper clawsweeper Bot mentioned this pull request Jun 18, 2026
Closed
@github-actions github-actions Bot mentioned this pull request Jun 18, 2026
Open
@clawsweeper clawsweeper Bot mentioned this pull request Jun 18, 2026
Closed
@clawsweeper

clawsweeper Bot commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 18, 2026, 9:25 AM ET / 13:25 UTC.

Summary
The PR changes embedded-agent compaction retry waiting so the aggregate guard treats either active compaction or active session streaming as active retry work, with focused regression coverage and an updated test mock.

PR surface: Source +22, Tests +21. Total +43 across 4 files.

Reproducibility: yes. at source level: current main races the aggregate wait against only isCompactionInFlight, while #94391 supplies production trajectories where the retry model request completed after the 60-second guard. I did not run a paid 200K-token provider reproduction in this read-only review.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #94391
Summary: This PR is a candidate fix for the canonical long-session compaction retry aggregate-wait bug; several sibling PRs propose alternate timeout-based fixes for the same report.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Maintainer should choose this active-streaming fix or one overlapping timeout-budget PR as the canonical path.

Risk before merge

  • [P1] If activeSession.isStreaming remains true after retry work is genuinely stuck, recovery moves from the 60-second idle guard to the broader run abort or deadline path.
  • [P2] Several overlapping open PRs propose timeout-budget fixes for the same canonical report, so maintainers should choose one canonical implementation path before landing.

Maintainer options:

  1. Accept session streaming as the retry-active signal
    Merge this path if maintainers agree that an actively streaming session is the right boundary for preserving long compaction retry results.
  2. Prefer timeout-budget coupling instead
    If maintainers want the aggregate wait tied to compaction timeout configuration, choose one of the overlapping timeout-budget PRs as the canonical fix and close the rest after it lands.
  3. Add an explicit stale-streaming cap
    If maintainers do not want stuck streaming to defer recovery to the broader run deadline, add a bounded stale-streaming recovery rule and regression test before merge.

Next step before merge

  • [P2] Protected-label and session-state tradeoff review are the remaining actions; there is no narrow ClawSweeper repair to queue from the current diff.

Security
Cleared: The current diff touches only agent runtime TypeScript and focused tests, with no dependency, workflow, secret, package, or code-download surface.

Review details

Best possible solution:

Land one maintainer-approved canonical fix that preserves the idle deadlock fallback while not discarding valid long-running compaction retry results, then close the overlapping timeout-only candidates.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: current main races the aggregate wait against only isCompactionInFlight, while #94391 supplies production trajectories where the retry model request completed after the 60-second guard. I did not run a paid 200K-token provider reproduction in this read-only review.

Is this the best way to solve the issue?

Likely yes, with a maintainer tradeoff: using active session streaming closes the specific post-compaction retry gap without adding config or only raising a constant. Maintainers still need to accept the stale-streaming recovery shift versus the overlapping timeout-budget approaches.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against aad0559de0e2.

Label changes

Label changes:

  • add merge-risk: 🚨 availability: A stale streaming signal could keep the session lane waiting longer than the existing idle recovery guard before broader run timeout handling takes over.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body and follow-up provide after-fix terminal proof from focused tests plus a compressed runtime timing probe that exercises the production helper and active-retry predicate.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove merge-risk: 🚨 compatibility: Current PR review merge-risk labels are merge-risk: 🚨 session-state, merge-risk: 🚨 availability.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P1: The PR targets a severe reported long-session compaction failure that can discard valid provider results and leave sessions stuck at the overflow ceiling.
  • merge-risk: 🚨 session-state: The diff changes the active/idle decision for compaction retry waits, affecting when session compaction state is adopted or abandoned.
  • merge-risk: 🚨 availability: A stale streaming signal could keep the session lane waiting longer than the existing idle recovery guard before broader run timeout handling takes over.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body and follow-up provide after-fix terminal proof from focused tests plus a compressed runtime timing probe that exercises the production helper and active-retry predicate.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body and follow-up provide after-fix terminal proof from focused tests plus a compressed runtime timing probe that exercises the production helper and active-retry predicate.
Evidence reviewed

PR surface:

Source +22, Tests +21. Total +43 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 3 28 6 +22
Tests 1 30 9 +21
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 58 15 +43

Acceptance criteria:

  • [P2] pnpm test src/agents/embedded-agent-runner/run/compaction-retry-aggregate-timeout.test.ts src/agents/embedded-agent-subscribe.subscribe-embedded-agent-session.waits-multiple-compaction-retries-before-resolving.test.ts src/agents/embedded-agent-runner/run/compaction-timeout.test.ts.
  • [P1] Exact-head or merge-ref required checks before landing.

What I checked:

Likely related people:

  • Vincent Koc: Current blame maps the aggregate wait helper and call-site lines in the checked-out main tree to commit 95b6df8, which carried this area through a recent agent refactor. (role: recent current-main carrier; confidence: medium; commits: 95b6df834350; files: src/agents/embedded-agent-runner/run/attempt.ts, src/agents/embedded-agent-runner/run/compaction-retry-aggregate-timeout.ts)
  • cgdusek: The merged related PR 40324 added the aggregate compaction retry timeout family that this PR refines. (role: related feature contributor; confidence: medium; commits: 54be30ef89f5; files: src/agents/embedded-agent-runner/run/attempt.ts, src/agents/embedded-agent-runner/run/compaction-retry-aggregate-timeout.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P1 High-priority user-facing bug, regression, or broken workflow. labels Jun 18, 2026
@steipete

steipete commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Land-ready verification and landing for 70a27bda48:

  • Local and Azure-backed Linux: 94 focused attempt and compaction tests passed.
  • Runtime proof: a retry stayed active for 171ms under a 60ms deadlock guard while compaction was idle and the session remained streaming; it completed without timeout.
  • Exact-SHA GitHub release gate 27762291396: all 126 jobs passed; Workflow Sanity also passed.
  • Fresh Codex autoreview: no accepted/actionable findings.
  • Landed through fix(agents): preserve active compaction retries #94421 as 9a571399bb7c98ec06aa8d661bfb62929f225130.

Proof gap: no paid-model 200K-context compaction run; the exact retry lifecycle and timeout path were exercised directly.

@wings1029 wings1029 mentioned this pull request Jun 18, 2026
Closed
6 tasks
@clawsweeper clawsweeper Bot mentioned this pull request Jun 18, 2026
Closed
@steipete
steipete requested a review from a team as a code owner June 18, 2026 09:46
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation channel: discord Channel integration: discord channel: imessage Channel integration: imessage channel: msteams Channel integration: msteams channel: slack Channel integration: slack channel: telegram Channel integration: telegram channel: whatsapp-web Channel integration: whatsapp-web channel: zalo Channel integration: zalo app: web-ui App: web-ui gateway Gateway runtime extensions: memory-core Extension: memory-core cli CLI command changes scripts Repository scripts labels Jun 18, 2026
@openclaw-barnacle openclaw-barnacle Bot added size: S and removed app: web-ui App: web-ui gateway Gateway runtime extensions: memory-core Extension: memory-core cli CLI command changes scripts Repository scripts commands Command implementations docker Docker and sandbox tooling extensions: openai channel: qqbot extensions: qa-lab extensions: codex extensions: tokenjuice Changes to the bundled tokenjuice extension plugin: google-meet extensions: gmi size: XL labels Jun 18, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 18, 2026
@steipete

steipete commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

@steipete steipete mentioned this pull request Jun 18, 2026
Closed
@github-actions github-actions Bot mentioned this pull request Jun 18, 2026
Open
This was referenced Jun 18, 2026
Merged
Merged
Closed
Merged
Open
This was referenced Jun 26, 2026
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

Assignees

@steipete steipete

Labels

agents Agent runtime and tooling maintainer Maintainer-authored PR merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: S status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

1 participant

@steipete