fix: prevent unbounded narrative session growth by stabilizing session key (#68354)#68364
fix: prevent unbounded narrative session growth by stabilizing session key (#68354)#68364jasonmakr wants to merge 1 commit into
Conversation
…n key (openclaw#68354) The buildNarrativeSessionKey function included a millisecond timestamp (nowMs) in the session key, creating a unique session for every dream run. When session deletion fails in the finally block, these sessions accumulate indefinitely (observed: 251 orphan sessions in 4 days). Changes: - Remove nowMs from session key → stable key per workspace+phase - Keep nowMs in idempotencyKey to prevent run deduplication - Add pre-run deleteSession call to clear stale sessions from previous failed runs, preventing conversation context pollution - Update tests to reflect new deleteSession call count (2 per run: pre-run cleanup + finally cleanup)
openclaw-barnacle
Bot
added
extensions: memory-core
Greptile SummaryThis PR fixes the root cause of unbounded narrative session accumulation (#68354): the session key previously embedded Confidence Score: 5/5Safe to merge — the fix correctly bounds session accumulation with no regressions introduced. All remaining findings are P2 style suggestions. The core logic (stable session key, idempotency key carrying nowMs, pre-run cleanup) is correct. Tests are comprehensive and accurately reflect the 2-deleteSession-per-run invariant. The acknowledged trade-off (stale context if both cleanup calls fail) is minor and well-reasoned. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/memory-core/src/dreaming-narrative.ts
Line: 860
Comment:
**Catch comment understates swallowed errors**
The comment says `// Ignore — session may not exist yet.` but the catch block also silently swallows real failures (network errors, auth errors, etc.). Since this is intentionally best-effort, the comment would be more accurate as:
```suggestion
// Ignore — session may not exist yet, or cleanup may fail transiently.
```
Minor, but future readers chasing unexpected state may be misled by the narrower framing.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: prevent unbounded narrative session..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| await params.subagent.deleteSession({ sessionKey }); | ||
| } catch { | ||
| // Ignore — session may not exist yet. |
There was a problem hiding this comment.
The comment says // Ignore — session may not exist yet. but the catch block also silently swallows real failures (network errors, auth errors, etc.). Since this is intentionally best-effort, the comment would be more accurate as:
| // Ignore — session may not exist yet. | |
| // Ignore — session may not exist yet, or cleanup may fail transiently. |
Minor, but future readers chasing unexpected state may be misled by the narrower framing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/dreaming-narrative.ts
Line: 860
Comment:
**Catch comment understates swallowed errors**
The comment says `// Ignore — session may not exist yet.` but the catch block also silently swallows real failures (network errors, auth errors, etc.). Since this is intentionally best-effort, the comment would be more accurate as:
```suggestion
// Ignore — session may not exist yet, or cleanup may fail transiently.
```
Minor, but future readers chasing unexpected state may be misled by the narrower framing.
How can I resolve this? If you propose a fix, please make it concise.
|
Related work from PRtags group Title: Open PR candidate: memory-core dreaming-narrative session leak cleanup
|
|
Closing this as duplicate or superseded after Codex automated review. PR #68364 is a narrow stable-session-key patch that remains absent from current main, but it is superseded by open PR #70464, which carries the same memory-core dreaming narrative key/idempotency/pre-cleanup fix and adds the missing scoped gateway permission needed for cleanup to work. Best possible solution: Close #68364 as superseded and keep review focused on #70464 or its successor, because that path combines the stable workspace+phase narrative session key, per-run idempotency key, pre-run cleanup, and the narrow gateway permission needed for memory-core to delete its own transient dreaming sessions. What I checked:
So I’m closing this here and keeping the remaining discussion on the canonical linked item. Codex Review notes: model gpt-5.5, reasoning high; reviewed against d54d2d6b9b8a. |
1 participant
Problem
buildNarrativeSessionKeyincludes a millisecond timestamp (nowMsfromDate.now()) in the session key, making it unique per dream run:When
deleteSessionfails in thefinallyblock, these uniquely-keyed sessions accumulate indefinitely. As reported in #68354: 251 orphan sessions in 4 days, consuming ~4.8M tokens.Fix
1. Stabilize session key (remove
nowMs)Now each workspace+phase combination reuses the same session key. If
deleteSessionfails, the next run reuses the existing session instead of creating a new orphan. Maximum accumulation: 2-3 sessions (one per phase) instead of unbounded.2. Keep
nowMsin idempotency keyPrevents deduplication across dream runs — each run is still treated as unique by the subagent runtime.
3. Pre-run defensive cleanup
Added a
deleteSessioncall beforesubagent.run()to clear any leftover session from a previous failed run. This prevents stale conversation context from polluting the new narrative.Trade-offs considered
deleteSessionfails infinallyAND the pre-run cleanup also fails, the next run could see stale messages. This is a minor cosmetic risk (dream diary entries slightly influenced by previous context) vs. the current unbounded resource leak.Tests updated
deleteSessioncall count expectations (now 2 per run: pre-run cleanup + finally)Closes #68354