Skip to content

fix(auto-reply): add memory flush failure tracking with retry exhaustion#88676

Merged
steipete merged 2 commits into
openclaw:mainfrom
Jerry-Xin:fix/memory-flush-failure-escalation
May 31, 2026
Merged

fix(auto-reply): add memory flush failure tracking with retry exhaustion#88676
steipete merged 2 commits into
openclaw:mainfrom
Jerry-Xin:fix/memory-flush-failure-escalation

Conversation

@Jerry-Xin

@Jerry-Xin Jerry-Xin commented May 31, 2026 •
edited by steipete
Loading

Copy link
Copy Markdown
Contributor

Summary

Fix memory flush failures permanently wedging sessions by adding per-session failure tracking, bounded retry, and a non-destructive retry-exhaustion path.

When a session exceeds the memoryFlush threshold, OpenClaw starts an embedded agent turn to create a handoff summary before compacting history. If that provider call fails, current main only logs/surfaces the error and persists no failure state. The transcript stays over threshold, so each later user message can retry the same failing flush and never reach the normal reply path.

This PR records consecutive non-abort memory flush failures and, after three failures in the same compaction cycle, marks that cycle as exhausted so the next normal reply can proceed. No transcript content is truncated or deleted by this PR.

Partially addresses #85645.

Changes

  • Add memoryFlushFailureCount, memoryFlushLastFailedAt, and memoryFlushLastFailureError to SessionEntry.
  • Reserve the new session fields from plugin session-entry slots.
  • On non-abort flush failure, persist failure metadata and emit memory_flush_failed lifecycle events.
  • On successful flush, clear failure metadata.
  • After 3 consecutive failures, persist memoryFlushCompactionCount for the current cycle, emit memory_flush_exhausted, and show a visible warning that memory flush is being skipped for this cycle.
  • Keep abort errors retryable without recording failure metadata.
  • Add regression coverage for failure tracking, abort exclusion, success cleanup, retry-before-exhaustion, and exhaustion marking.

Real behavior proof

  • Behavior addressed: A memoryFlush provider failure can leave the session over threshold with no durable failure state, causing repeated flush failure before normal replies can proceed.
  • Real environment tested: Local OpenClaw checkout on macOS with the repository Node/Vitest harness; PR author also reported Docker node:24-bookworm coverage before maintainer fixup.
  • Exact steps or command run after this patch:
node scripts/run-vitest.mjs src/auto-reply/reply/agent-runner-memory.test.ts
pnpm check:test-types
.agents/skills/autoreview/scripts/autoreview --mode local
git diff --check
  • Evidence after fix:
src/auto-reply/reply/agent-runner-memory.test.ts: 40 tests passed
pnpm check:test-types: no longer fails on agent-runner-memory.test.ts; local checkout then hit unrelated current-main doctor migration errors in src/commands/doctor/shared/legacy-config-migrations.channels.ts
autoreview: clean, no accepted/actionable findings
git diff --check: clean
  • Observed result after fix: Non-abort flush failures increment durable session failure metadata and emit lifecycle events. Before the exhaustion threshold, the next message retries memory flush. At the threshold, the current compaction cycle is marked exhausted so the normal reply path can proceed without deleting transcript history; a later compaction cycle can retry memory flush.
  • What was not tested: Live end-to-end provider crash with a real OpenClaw Gateway instance. The failure path is covered with runWithModelFallback rejection against real session-store JSON on disk.

Test plan

  • node scripts/run-vitest.mjs src/auto-reply/reply/agent-runner-memory.test.ts
  • pnpm check:test-types to confirm the PR-owned type error is gone; unrelated current-main doctor migration type errors remain locally
  • .agents/skills/autoreview/scripts/autoreview --mode local
  • git diff --check

@openclaw-barnacle openclaw-barnacle Bot added size: S proof: supplied External PR includes structured after-fix real behavior proof. labels May 31, 2026
@clawsweeper

clawsweeper Bot commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed May 31, 2026, 12:44 PM ET / 16:44 UTC.

Summary
The branch adds per-session memoryFlush failure metadata, lifecycle events, a three-failure exhaustion marker for the current compaction cycle, reserved session slot keys, and regression tests.

PR surface: Source +103, Tests +235. Total +338 across 4 files.

Reproducibility: yes. from source inspection: current main catches memoryFlush failures without advancing durable state, and the caller returns visible error payloads before the normal reply path. I did not run a live provider-crash reproduction in this read-only review.

Review metrics: 1 noteworthy metric.

  • Persistent session metadata: 3 fields added, 3 reserved keys added. SessionEntry persistence and plugin slot reservations are compatibility-sensitive surfaces that maintainers should review before merge.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • [P1] Add redacted Gateway or close integration proof showing repeated memoryFlush failure reaches exhaustion and later messages can proceed.
  • Get explicit maintainer acceptance for the fail-open retry-exhaustion policy.
  • Keep the PR body proof aligned with the current non-destructive implementation.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body supplies focused tests and local check output only, while explicitly saying live Gateway/provider-crash behavior was not tested; the contributor should add redacted terminal/log/live output or a close integration run, then update the PR body to trigger re-review or ask a maintainer for @clawsweeper re-review.

Risk before merge

  • [P1] The PR intentionally changes persistent session behavior: after three failed memoryFlush attempts, OpenClaw marks the current compaction cycle as flushed and lets later messages skip memoryFlush until a later compaction cycle.
  • [P1] The external PR still lacks after-fix real Gateway or close integration proof; the supplied evidence is focused unit/typecheck/autoreview output with a mocked provider failure path.
  • [P1] Adding three persisted SessionEntry fields and reserving three plugin slot keys is compatibility-sensitive for existing session files and plugin-owned slot collision behavior.

Maintainer options:

  1. Require proof and policy sign-off (recommended)
    Ask for current after-fix Gateway or close integration output showing the exhaustion path, and have a maintainer explicitly accept the fail-open-after-three behavior before merge.
  2. Accept the session-state tradeoff
    Maintainers can intentionally land the non-destructive skip policy with the focused tests, while leaving configurable escalation as follow-up work.
  3. Pause for configurable escalation
    If maintainers want model-independent reset or configurable escalation first, pause this PR and keep the related issue as the canonical product-decision track.

Next step before merge

  • [P1] Human review should decide the fail-open session-state policy and require current real behavior proof; I did not find a narrow automated repair to queue.

Security
Cleared: No concrete security or supply-chain regression was found; the diff does not touch dependencies, CI, permissions, secrets, or downloaded code, and persisted error text goes through the existing redacting formatter.

Review details

Best possible solution:

Land the non-destructive exhaustion path only after redacted real Gateway or close integration proof and explicit maintainer acceptance of the fail-open policy; keep broader configurable escalation or force-truncate behavior with #85645 or #30452.

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

Yes, from source inspection: current main catches memoryFlush failures without advancing durable state, and the caller returns visible error payloads before the normal reply path. I did not run a live provider-crash reproduction in this read-only review.

Is this the best way to solve the issue?

Mostly yes: the non-destructive exhaustion marker is a narrow safer shape than transcript truncation. The remaining question is whether maintainers accept fail-open after three failures and whether the contributor can provide current real behavior proof.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against c7b190beec73.

Label changes

Label justifications:

  • P1: The PR targets a real user-facing session wedge where repeated memoryFlush provider failures can prevent normal replies.
  • merge-risk: 🚨 compatibility: The diff adds persisted SessionEntry fields and reserves new plugin slot keys, which can affect existing session state and plugin extension slots.
  • merge-risk: 🚨 session-state: The patch deliberately advances memoryFlushCompactionCount after repeated failures, changing how session state gates future memory flush attempts.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body supplies focused tests and local check output only, while explicitly saying live Gateway/provider-crash behavior was not tested; the contributor should add redacted terminal/log/live output or a close integration run, then update the PR body to trigger re-review or ask a maintainer for @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +103, Tests +235. Total +338 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 3 105 2 +103
Tests 1 235 0 +235
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 340 2 +338

What I checked:

  • Repository policy applied: Root AGENTS.md was read fully, and src/plugins/AGENTS.md was read for the touched plugin slot-reservation file; both make session state, fallback behavior, and plugin slot contracts compatibility-sensitive review surfaces. (AGENTS.md:5, c7b190beec73)
  • Current main failure path: Current main catches memory-flush failures by logging and forwarding a visible error payload, with no persisted failure count, timestamp, or exhaustion marker. (src/auto-reply/reply/agent-runner-memory.ts:1344, c7b190beec73)
  • Current main reply blockage: The caller treats visible memory-flush error payloads as a failed run and returns before creating the normal follow-up runner, which supports the reported wedge when every later message re-enters the same failing flush path. (src/auto-reply/reply/agent-runner.ts:1526, c7b190beec73)
  • PR failure tracking implementation: At PR head, non-abort flush failures increment durable metadata and emit memory_flush_failed; after three failures the code writes memoryFlushCompactionCount for the current cycle and emits memory_flush_exhausted. (src/auto-reply/reply/agent-runner-memory.ts:1358, ead22a929f0c)
  • PR reserves new session fields: The PR head reserves memoryFlushFailureCount, memoryFlushLastFailedAt, and memoryFlushLastFailureError so plugin session-entry slots cannot claim those names. (src/plugins/session-entry-slot-keys.ts:104, ead22a929f0c)
  • PR regression tests: The PR head adds tests for non-abort failure counting, abort exclusion, success cleanup, exhaustion marking, and retry-before-exhaustion behavior. (src/auto-reply/reply/agent-runner-memory.test.ts:497, ead22a929f0c)

Likely related people:

  • steipete: Peter Steinberger authored the original pre-compaction memoryFlush work and has the dominant shortlog/blame signal around the current memoryFlush and session metadata paths. (role: introduced behavior and recent area contributor; confidence: high; commits: 7dbb21be8e57, d5ac97652a53; files: src/auto-reply/reply/agent-runner-memory.ts, src/auto-reply/reply/memory-flush.ts, src/config/sessions/types.ts)
  • hcl: Authored the transcript-size forced memoryFlush repeated-run guard that touched the same compaction-cycle gating boundary. (role: adjacent bugfix author; confidence: medium; commits: 503d39578066; files: src/auto-reply/reply/agent-runner-memory.ts, src/auto-reply/reply/memory-flush.ts, src/auto-reply/reply/reply-state.test.ts)
  • Menglin Li: Authored a recent memoryFlush dedup/content-hash fix in the same agent-runner-memory area. (role: adjacent memoryFlush contributor; confidence: medium; commits: ef7a5c354611; files: src/auto-reply/reply/agent-runner-memory.ts, src/auto-reply/reply/memory-flush.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 rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels May 31, 2026
@Jerry-Xin
Jerry-Xin force-pushed the fix/memory-flush-failure-escalation branch from ea725dc to 74385e7 Compare May 31, 2026 15:55
@Jerry-Xin Jerry-Xin changed the title fix(auto-reply): persist memory flush failure marker to break infinite retry loop fix(auto-reply): add memory flush failure tracking with escalation to force-truncate May 31, 2026
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. label May 31, 2026
@Jerry-Xin

Jerry-Xin commented May 31, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

忻役
fix(auto-reply): replace destructive force-truncate with non-destruct…
b3737db 
…ive flush skip

After MAX_FLUSH_FAILURES consecutive memory flush failures, mark the
flush as attempted for the current compaction cycle instead of truncating
the session transcript. The next compaction cycle will retry. This avoids
data loss while still breaking the retry loop.

Reserve the three new failure-tracking session fields in the slot key
list so plugins cannot collide with them.
@Jerry-Xin
Jerry-Xin force-pushed the fix/memory-flush-failure-escalation branch from 74385e7 to b3737db Compare May 31, 2026 16:16
@Jerry-Xin

Jerry-Xin commented May 31, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both P1 findings in b3737db:

[P1] Reserve the new session fields — Fixed

Added memoryFlushFailureCount, memoryFlushLastFailedAt, and memoryFlushLastFailureError to SESSION_ENTRY_RESERVED_SLOT_KEY_LIST in src/plugins/session-entry-slot-keys.ts, right after the existing memoryFlushContextHash entry.

[P1] Replace destructive force-truncation with non-destructive loop break — Fixed

Removed the entire forceTruncateSession mechanism and all related helpers (parseTranscriptLine, isTranscriptMessageRecord, buildForceTruncateMarker, estimateForceTruncatedTotalTokens, isTranscriptMessageRole). No session context is destroyed.

New behavior after MAX_FLUSH_FAILURES (3) consecutive failures:

  1. Sets memoryFlushCompactionCount to mark the flush as "attempted for this cycle" — this breaks the retry loop
  2. Emits a memory_flush_exhausted lifecycle event
  3. Shows a visible warning: "Memory flush failed after 3 attempts; skipping for this cycle"
  4. The next compaction cycle (after new messages exceed the threshold) will retry the flush
  5. No transcript data is lost or truncated

Failure tracking (kept):

  • memoryFlushFailureCount — incremented on each non-abort failure, reset to 0 on success
  • memoryFlushLastFailedAt — timestamp of last failure
  • memoryFlushLastFailureError — truncated error message for diagnostics
  • memory_flush_failed lifecycle event on each failure
  • memory_flush_exhausted lifecycle event when retries are exhausted

All 40 agent-runner-memory tests pass.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@steipete steipete self-assigned this May 31, 2026
@steipete steipete changed the title fix(auto-reply): add memory flush failure tracking with escalation to force-truncate fix(auto-reply): add memory flush failure tracking with retry exhaustion May 31, 2026
@steipete

steipete commented May 31, 2026

Copy link
Copy Markdown
Contributor

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@steipete

steipete commented May 31, 2026

Copy link
Copy Markdown
Contributor

Maintainer land-ready note for ead22a929f0c8ebf336708f68e8ec37c91196098.

Policy decision: accepting the non-destructive fail-open tradeoff for this PR. After three consecutive non-abort memoryFlush failures, OpenClaw marks only the current compaction cycle as exhausted so later messages can proceed; it does not force-truncate or delete transcript history. Configurable escalation / reset policy remains follow-up scope.

Local proof run:

node scripts/run-vitest.mjs src/auto-reply/reply/agent-runner-memory.test.ts
pnpm check:test-types
.agents/skills/autoreview/scripts/autoreview --mode local
git diff --check

Results:

agent-runner-memory.test.ts: 40 tests passed
pnpm check:test-types: PR-owned agent-runner-memory.test.ts type failure fixed; local checkout then hit unrelated current-main doctor migration type errors in src/commands/doctor/shared/legacy-config-migrations.channels.ts
autoreview: clean, no accepted/actionable findings
git diff --check: clean

CI proof:

CI: https://github.com/openclaw/openclaw/actions/runs/26718309601 passed on ead22a929f0c8ebf336708f68e8ec37c91196098
OpenGrep PR Diff: https://github.com/openclaw/openclaw/actions/runs/26718309594 passed after rerun; first attempt failed installing opengrep from GitHub
Real behavior proof check: passed on ead22a929f0c8ebf336708f68e8ec37c91196098

Known proof gap: no live Gateway/provider-crash repro was run. The failure path is covered with focused regression tests using a rejected runWithModelFallback against real session-store JSON on disk.

@steipete
steipete merged commit 4e84d0e into openclaw:main May 31, 2026
153 of 154 checks passed
@Jerry-Xin
Jerry-Xin deleted the fix/memory-flush-failure-escalation branch May 31, 2026 18:05
@clawsweeper clawsweeper Bot mentioned this pull request Jun 2, 2026
Closed
@clawsweeper clawsweeper Bot mentioned this pull request Jun 15, 2026
Closed
@clawsweeper clawsweeper Bot mentioned this pull request Jun 27, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. 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: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Jerry-Xin @steipete