Add time-based session maintenance modes#56575
Conversation
openclaw-barnacle
Bot
added
gateway
There was a problem hiding this comment.
Pull request overview
Adds configurable, time-based session maintenance workflows (idle compaction “cache warming” and idle reset) driven by per-model TTL settings, and wires a periodic maintenance sweep into the gateway so long-lived sessions can be compacted or reset based on inactivity.
Changes:
- Introduces
sessionCacheTtl+timeBasedContextCompact(none/compact/reset) and resolves them from model extra params. - Records new session timing fields (
lastUserMessageAt,lastAssistantMessageAt,lastCacheTouchAt,lastIdleCompactionForCacheTouchAt) and updates them during runs. - Adds a non-overlapping gateway maintenance sweep that applies the selected reset/compact policy and includes new unit tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/gateway/session-cache-maintenance.ts | Implements policy resolution + sweep logic for idle compaction/reset. |
| src/gateway/session-cache-maintenance.test.ts | Unit tests for idle compaction/reset decision helpers. |
| src/gateway/server-maintenance.ts | Schedules maintenance sweep with an in-flight guard to prevent overlap. |
| src/config/sessions/types.ts | Adds persisted session timing fields used by the new maintenance logic. |
| src/config/defaults.ts | Defaults model params for cacheRetention and sessionCacheTtl. |
| src/config/config.pruning-defaults.test.ts | Verifies new pruning defaults behavior and non-enablement of mode by default. |
| src/auto-reply/reply/session.ts | Initializes lastUserMessageAt and clears timing fields for new sessions. |
| src/auto-reply/reply/session-usage.ts | Updates assistant/cache-touch timestamps when assistant activity is recorded. |
| src/auto-reply/reply/followup-runner.ts | Marks followup runs as assistant activity for timestamping. |
| src/auto-reply/reply/agent-runner.ts | Avoids recording assistant activity for heartbeat runs. |
| src/agents/pi-embedded-runner/cache-ttl.ts | Adds TTL/mode resolution (resolveCacheTtlMs, resolveTimeBasedContextCompactMode) + mismatch warning. |
| src/agents/pi-embedded-runner/cache-ttl.test.ts | Adds tests for TTL resolution and mismatch warning behavior. |
| src/agents/command/session-store.ts | Records assistant/cache-touch timestamps after agent runs. |
| src/agents/agent-command.ts | Records user-turn timestamp when persisting command overrides. |
| const patch: Partial<SessionEntry> = { | ||
| lastIdleCompactionForCacheTouchAt: cacheTouchAt, | ||
| }; | ||
| if (result.ok && result.compacted) { | ||
| patch.compactionCount = (params.entry.compactionCount ?? 0) + 1; | ||
| patch.lastCacheTouchAt = params.now; | ||
| if (typeof result.result?.tokensAfter === "number" && result.result.tokensAfter > 0) { | ||
| patch.totalTokens = result.result.tokensAfter; | ||
| patch.totalTokensFresh = true; | ||
| } | ||
| log.info("idle cache-warming compaction completed", { | ||
| sessionKey: params.sessionKey, | ||
| provider, | ||
| model, | ||
| tokensBefore: params.totalTokens, | ||
| tokensAfter: result.result?.tokensAfter, | ||
| }); | ||
| } else { | ||
| log.info("idle cache-warming compaction skipped", { | ||
| sessionKey: params.sessionKey, | ||
| provider, | ||
| model, | ||
| reason: result.reason ?? "not_compacted", | ||
| tokens: params.totalTokens, | ||
| }); | ||
| } |
There was a problem hiding this comment.
lastIdleCompactionForCacheTouchAt is persisted even when compactEmbeddedPiSession returns ok: false (error case). That marks the cache-touch as “serviced” and prevents any retry on subsequent sweeps, so transient compaction failures would permanently disable cache-warming for that idle window. Consider only setting lastIdleCompactionForCacheTouchAt when the compaction attempt completed successfully (result.ok === true), or otherwise recording a separate failure timestamp/backoff while still allowing retries.
| const totalTokens = estimateSessionTotalTokens({ entry, target }); | ||
| if ( | ||
| shouldRunIdleCacheCompaction({ | ||
| entry, | ||
| policy, | ||
| now, | ||
| totalTokens, | ||
| }) | ||
| ) { | ||
| await maybeCompactIdleSession({ | ||
| cfg, | ||
| entry, | ||
| sessionKey, | ||
| target, | ||
| now, | ||
| totalTokens: totalTokens ?? 0, | ||
| }); |
There was a problem hiding this comment.
estimateSessionTotalTokens() is called for every session that reaches this point, even when the policy is reset (where tokens are never used). Because the estimator can read and tokenize the session transcript from disk, this can add significant per-minute overhead. Move token estimation behind a policy.mode === "compact" check (and ideally after the cheap time/window checks) so transcripts are only read when an idle compaction is actually possible.
| const totalTokens = estimateSessionTotalTokens({ entry, target }); | |
| if ( | |
| shouldRunIdleCacheCompaction({ | |
| entry, | |
| policy, | |
| now, | |
| totalTokens, | |
| }) | |
| ) { | |
| await maybeCompactIdleSession({ | |
| cfg, | |
| entry, | |
| sessionKey, | |
| target, | |
| now, | |
| totalTokens: totalTokens ?? 0, | |
| }); | |
| if (policy.mode === "compact") { | |
| const totalTokens = estimateSessionTotalTokens({ entry, target }); | |
| if ( | |
| shouldRunIdleCacheCompaction({ | |
| entry, | |
| policy, | |
| now, | |
| totalTokens, | |
| }) | |
| ) { | |
| await maybeCompactIdleSession({ | |
| cfg, | |
| entry, | |
| sessionKey, | |
| target, | |
| now, | |
| totalTokens: totalTokens ?? 0, | |
| }); | |
| } |
| return false; | ||
| } | ||
| const lastUser = resolvePositiveTimestamp(entry.lastUserMessageAt); | ||
| return lastUser == null || lastAssistant > lastUser; |
There was a problem hiding this comment.
isAwaitingUserReply uses lastAssistant > lastUser to determine whether the session is idle. Several call sites write these timestamps with independent Date.now() calls; if both values land in the same millisecond, the equality case will be treated as “not awaiting user reply” and will prevent reset/compaction from ever running for that session. Consider using >= here (or otherwise ensuring monotonic timestamps when recording user/assistant turns).
| return lastUser == null || lastAssistant > lastUser; | |
| return lastUser == null || lastAssistant >= lastUser; |
| const warnedSessionCacheTtlMismatchKeys = new Set<string>(); | ||
|
|
There was a problem hiding this comment.
warnedSessionCacheTtlMismatchKeys is a process-global Set that grows for every unique mismatch tuple (provider/model/sessionCacheTtl/explicitTtl) and never evicts entries. In long-lived gateway processes with many models/agents/config changes, this can become an unbounded memory leak. Consider bounding this structure (e.g., size cap + clear, TTL-based eviction, or keying only on provider/model to reduce cardinality).
| const warnedSessionCacheTtlMismatchKeys = new Set<string>(); | |
| const MAX_WARNED_SESSION_CACHE_TTL_MISMATCH_KEYS = 1000; | |
| class BoundedStringSet extends Set<string> { | |
| private readonly maxSize: number; | |
| constructor(maxSize: number) { | |
| super(); | |
| this.maxSize = maxSize; | |
| } | |
| override add(value: string): this { | |
| if (this.maxSize > 0 && this.size >= this.maxSize) { | |
| // Prevent unbounded growth in long-lived processes by clearing | |
| // previously recorded mismatch keys once the cap is reached. | |
| this.clear(); | |
| } | |
| return super.add(value); | |
| } | |
| } | |
| const warnedSessionCacheTtlMismatchKeys = new BoundedStringSet( | |
| MAX_WARNED_SESSION_CACHE_TTL_MISMATCH_KEYS, | |
| ); |
| ...entry, | ||
| sessionId, | ||
| updatedAt: Date.now(), | ||
| lastAssistantMessageAt: Date.now(), | ||
| contextTokens, | ||
| }; | ||
| const cacheTtlMs = resolveCacheTtlMs({ | ||
| config: cfg, | ||
| provider: providerUsed, | ||
| modelId: modelUsed, | ||
| agentId: resolveAgentIdFromSessionKey(sessionKey), | ||
| }); | ||
| if (cacheTtlMs != null) { | ||
| next.lastCacheTouchAt = next.updatedAt; | ||
| next.lastIdleCompactionForCacheTouchAt = undefined; | ||
| } |
There was a problem hiding this comment.
updateSessionStoreAfterAgentRun calls Date.now() multiple times while constructing next (for updatedAt and lastAssistantMessageAt) and then reuses next.updatedAt for lastCacheTouchAt. Using a single now value for all related timestamps avoids same-ms ordering edge cases and keeps the new time-based maintenance logic consistent.
| const next: SessionEntry = { | ||
| ...entry, | ||
| sessionId, | ||
| updatedAt: Date.now(), | ||
| lastUserMessageAt: Date.now(), | ||
| }; |
There was a problem hiding this comment.
This session-store patch uses multiple Date.now() calls (updatedAt and lastUserMessageAt). For the new idle-detection logic (lastAssistantMessageAt vs lastUserMessageAt), keeping turn timestamps derived from a single now value per command reduces equality/ordering edge cases.
| skillsSnapshot: params.entry.skillsSnapshot, | ||
| provider, | ||
| model, | ||
| trigger: "manual", |
There was a problem hiding this comment.
maybeCompactIdleSession calls compactEmbeddedPiSession with trigger: "manual". In compactEmbeddedPiSession this implies force: true and uses the threshold compaction target, which will over-compact/force-compaction during the automated idle maintenance sweep. Use a non-manual trigger (e.g. "budget") or introduce an explicit "idle" trigger so maintenance compaction remains non-forced and uses the intended target.
| trigger: "manual", | |
| trigger: "budget", |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2953e42e1c
ℹ️ 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".
| ...baseEntry, | ||
| sessionId, | ||
| updatedAt: Date.now(), | ||
| lastUserMessageAt: Date.now(), |
There was a problem hiding this comment.
lastUserMessageAt is now updated unconditionally during session initialization, but initSessionState is also used for heartbeat-triggered runs. In heartbeat flows, recordAssistantActivity is intentionally disabled, so this write can make lastUserMessageAt >= lastAssistantMessageAt, causing isAwaitingUserReply() to return false and preventing time-based reset/compact maintenance from running for otherwise idle sessions.
Useful? React with 👍 / 👎.
| const patch: Partial<SessionEntry> = { | ||
| lastIdleCompactionForCacheTouchAt: cacheTouchAt, | ||
| }; |
There was a problem hiding this comment.
maybeCompactIdleSession sets lastIdleCompactionForCacheTouchAt before checking whether compaction succeeded. If compaction fails or is skipped for a transient reason, this still marks the current cache-touch window as handled, and shouldRunIdleCacheCompaction() will suppress all retries for that window, so cache-warming can be permanently skipped until another assistant turn happens.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds two time-based session maintenance modes ( Key changes:
Issue found: The mock for Confidence Score: 4/5Not safe to merge until the test mock is fixed — the test suite will have failing tests as-is. The implementation logic for session maintenance is solid and well-tested in session-cache-maintenance.test.ts. However, the isCacheTtlEligibleProvider mock in cache-ttl.test.ts was not updated to match the changed moonshot expectations, meaning two test cases are broken and the stated validation claim cannot hold. This is a concrete, verifiable test failure that blocks merge. src/agents/pi-embedded-runner/cache-ttl.test.ts — the vi.mock at the top of the file needs to be updated so that moonshot no longer returns true, aligning the mock with the new expectations on lines 31 and 36.
|
| Filename | Overview |
|---|---|
| src/agents/pi-embedded-runner/cache-ttl.test.ts | Test expectations for moonshot eligibility changed to false, but the mock still returns true for moonshot — two tests will fail. |
| src/gateway/session-cache-maintenance.ts | New file implementing idle-compaction and expired-session-reset sweep logic; guard against overlapping runs and active sessions is sound. |
| src/agents/pi-embedded-runner/cache-ttl.ts | Adds resolveCacheTtlMs, resolveTimeBasedContextCompactMode, and supporting helpers; logic is correct and well-guarded. |
| src/config/defaults.ts | Correctly extends pruning defaults to also set sessionCacheTtl: '1h' alongside cacheRetention: 'short', with independent mutation guards. |
| src/auto-reply/reply/session-usage.ts | Correctly gates lastAssistantMessageAt and lastCacheTouchAt tracking behind recordAssistantActivity, using a single Date.now() snapshot. |
| src/gateway/server-maintenance.ts | Adds session-cache maintenance sweep inside the dedupe-cleanup interval with a correct in-flight guard to prevent overlapping sweeps. |
| src/agents/agent-command.ts | Sets lastUserMessageAt via a second Date.now() call instead of reusing the same timestamp as updatedAt. |
| src/auto-reply/reply/session.ts | Sets lastUserMessageAt via a second Date.now() call at line 474, same minor timestamp-drift issue as agent-command.ts. |
| src/config/sessions/types.ts | Adds four new optional timestamp fields with clear JSDoc comments. |
Comments Outside Diff (1)
-
src/agents/pi-embedded-runner/cache-ttl.test.ts, line 10-37 (link)Mock not updated to match new expectations — tests will fail
The mock for
resolveProviderCacheTtlEligibilitystill returnstrueformoonshot:if (params.context.provider === "moonshot" || params.context.provider === "zai") { return true; }
But the two test cases now expect
isCacheTtlEligibleProvider("moonshot", "kimi-k2.5")andisCacheTtlEligibleProvider("Moonshot", "Kimi-K2.5")to returnfalse. BecauseisCacheTtlEligibleProviderdelegates directly toresolveProviderCacheTtlEligibilityand returns its result when it is notundefined, the mock value oftrueis what is returned — notfalse. Both affected tests will fail at runtime:"uses provider-specific native eligibility rules"(line 31)"normalizes native eligibility rules case-insensitively"(line 36)
If the intent is that
moonshotis no longer a directly eligible provider, the mock needs to be updated to return a falsy/undefinedvalue for moonshot (while still returningtrueforzai). For example:vi.mock("../../plugins/provider-runtime.js", () => ({ resolveProviderCacheTtlEligibility: (params: { context: { provider: string; modelId: string }; }) => { if (params.context.provider === "anthropic") { return true; } if (params.context.provider === "zai") { return true; } if (params.context.provider === "openrouter") { return ["anthropic/", "moonshot/", "moonshotai/", "zai/"].some((prefix) => params.context.modelId.startsWith(prefix), ); } return undefined; }, }));
Also worth checking whether the underlying
resolveProviderCacheTtlEligibilityplugin implementation (inplugins/provider-runtime.js, not in this PR) already reflects the new moonshot exclusion, or whether that change is still pending.Prompt To Fix With AI
This is a comment left during a code review. Path: src/agents/pi-embedded-runner/cache-ttl.test.ts Line: 10-37 Comment: **Mock not updated to match new expectations — tests will fail** The mock for `resolveProviderCacheTtlEligibility` still returns `true` for `moonshot`: ```typescript if (params.context.provider === "moonshot" || params.context.provider === "zai") { return true; } ``` But the two test cases now expect `isCacheTtlEligibleProvider("moonshot", "kimi-k2.5")` and `isCacheTtlEligibleProvider("Moonshot", "Kimi-K2.5")` to return `false`. Because `isCacheTtlEligibleProvider` delegates directly to `resolveProviderCacheTtlEligibility` and returns its result when it is not `undefined`, the mock value of `true` is what is returned — not `false`. Both affected tests will fail at runtime: - `"uses provider-specific native eligibility rules"` (line 31) - `"normalizes native eligibility rules case-insensitively"` (line 36) If the intent is that `moonshot` is no longer a directly eligible provider, the mock needs to be updated to return a falsy/`undefined` value for moonshot (while still returning `true` for `zai`). For example: ```typescript vi.mock("../../plugins/provider-runtime.js", () => ({ resolveProviderCacheTtlEligibility: (params: { context: { provider: string; modelId: string }; }) => { if (params.context.provider === "anthropic") { return true; } if (params.context.provider === "zai") { return true; } if (params.context.provider === "openrouter") { return ["anthropic/", "moonshot/", "moonshotai/", "zai/"].some((prefix) => params.context.modelId.startsWith(prefix), ); } return undefined; }, })); ``` Also worth checking whether the underlying `resolveProviderCacheTtlEligibility` plugin implementation (in `plugins/provider-runtime.js`, not in this PR) already reflects the new moonshot exclusion, or whether that change is still pending. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/cache-ttl.test.ts
Line: 10-37
Comment:
**Mock not updated to match new expectations — tests will fail**
The mock for `resolveProviderCacheTtlEligibility` still returns `true` for `moonshot`:
```typescript
if (params.context.provider === "moonshot" || params.context.provider === "zai") {
return true;
}
```
But the two test cases now expect `isCacheTtlEligibleProvider("moonshot", "kimi-k2.5")` and `isCacheTtlEligibleProvider("Moonshot", "Kimi-K2.5")` to return `false`. Because `isCacheTtlEligibleProvider` delegates directly to `resolveProviderCacheTtlEligibility` and returns its result when it is not `undefined`, the mock value of `true` is what is returned — not `false`. Both affected tests will fail at runtime:
- `"uses provider-specific native eligibility rules"` (line 31)
- `"normalizes native eligibility rules case-insensitively"` (line 36)
If the intent is that `moonshot` is no longer a directly eligible provider, the mock needs to be updated to return a falsy/`undefined` value for moonshot (while still returning `true` for `zai`). For example:
```typescript
vi.mock("../../plugins/provider-runtime.js", () => ({
resolveProviderCacheTtlEligibility: (params: {
context: { provider: string; modelId: string };
}) => {
if (params.context.provider === "anthropic") {
return true;
}
if (params.context.provider === "zai") {
return true;
}
if (params.context.provider === "openrouter") {
return ["anthropic/", "moonshot/", "moonshotai/", "zai/"].some((prefix) =>
params.context.modelId.startsWith(prefix),
);
}
return undefined;
},
}));
```
Also worth checking whether the underlying `resolveProviderCacheTtlEligibility` plugin implementation (in `plugins/provider-runtime.js`, not in this PR) already reflects the new moonshot exclusion, or whether that change is still pending.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/agent-command.ts
Line: 538-539
Comment:
**Two separate `Date.now()` calls can drift apart**
`updatedAt` and `lastUserMessageAt` are set with two independent `Date.now()` calls. On a heavily loaded system these could differ by a millisecond or two, meaning `lastUserMessageAt > updatedAt` is possible in theory.
`session-usage.ts` (updated in this same PR) already uses the correct pattern of capturing `const now = Date.now()` once and reusing it. The same pattern is worth applying here and in `src/auto-reply/reply/session.ts` (line 473–474) where the same issue exists.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(session): add time-based session ma..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| updatedAt: Date.now(), | ||
| lastUserMessageAt: Date.now(), |
There was a problem hiding this comment.
Date.now() calls can drift apart
updatedAt and lastUserMessageAt are set with two independent Date.now() calls. On a heavily loaded system these could differ by a millisecond or two, meaning lastUserMessageAt > updatedAt is possible in theory.
session-usage.ts (updated in this same PR) already uses the correct pattern of capturing const now = Date.now() once and reusing it. The same pattern is worth applying here and in src/auto-reply/reply/session.ts (line 473–474) where the same issue exists.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/agent-command.ts
Line: 538-539
Comment:
**Two separate `Date.now()` calls can drift apart**
`updatedAt` and `lastUserMessageAt` are set with two independent `Date.now()` calls. On a heavily loaded system these could differ by a millisecond or two, meaning `lastUserMessageAt > updatedAt` is possible in theory.
`session-usage.ts` (updated in this same PR) already uses the correct pattern of capturing `const now = Date.now()` once and reusing it. The same pattern is worth applying here and in `src/auto-reply/reply/session.ts` (line 473–474) where the same issue exists.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcbebed44e
ℹ️ 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".
| resolvePositiveTimestamp(entry.lastCacheTouchAt) ?? | ||
| resolvePositiveTimestamp(entry.lastAssistantMessageAt) |
There was a problem hiding this comment.
resolveCacheTouchAt always prefers lastCacheTouchAt over lastAssistantMessageAt, but lastCacheTouchAt is only refreshed when resolveCacheTtlMs(...) is enabled in session write paths. If timeBasedContextCompact is disabled for a while and later re-enabled, an old lastCacheTouchAt can survive while assistant turns continue, so the sweep treats the session as already expired and may reset/compact immediately. Using the newer of the two timestamps avoids false expiry decisions.
Useful? React with 👍 / 👎.
| sessionId, | ||
| updatedAt: Date.now(), | ||
| updatedAt, | ||
| lastUserMessageAt: params.isHeartbeat === true ? baseEntry?.lastUserMessageAt : updatedAt, |
There was a problem hiding this comment.
This updates lastUserMessageAt for every non-heartbeat inbound turn, including directive-only commands that can return before any model run updates lastAssistantMessageAt (for example, immediate /status-style replies). In that case the session is left with lastUserMessageAt > lastAssistantMessageAt, so isAwaitingUserReply() remains false and time-based reset/compact maintenance is skipped until a later assistant model turn happens.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8458b6d28
ℹ️ 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".
| if (!isCacheTtlEligibleProvider(params.provider, params.modelId)) { | ||
| return null; |
There was a problem hiding this comment.
resolveCacheTtlMs bails out before reading timeBasedContextCompact whenever isCacheTtlEligibleProvider(...) is false, and runSessionCacheMaintenanceSweep skips maintenance entirely when cacheTtlMs is null. That means a configured timeBasedContextCompact: "reset" policy silently never runs for non-cache-eligible providers (for example, providers without prompt-cache support), even though reset logic is provider-agnostic and only needs idle timestamps. This makes the new reset mode a no-op for those models.
Useful? React with 👍 / 👎.
|
|
||
| for (const target of targets) { | ||
| const store = loadSessionStore(target.storePath); | ||
| for (const [sessionKey, entry] of Object.entries(store)) { |
There was a problem hiding this comment.
The sweep iterates raw Object.entries(store) keys and performs reset/compaction directly on each one. If a store still contains both canonical and legacy aliases for the same session key, the first alias can mutate/reset the canonical entry, but the later alias in this stale snapshot can still satisfy the old predicates and trigger another maintenance action in the same pass. Deduping by canonical key (or reloading before acting) avoids duplicate resets/compactions.
Useful? React with 👍 / 👎.
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep open for maintainer review: the branch is a plausible feature, but current main does not already provide these explicit reset/compact modes, and PR head still has session-state blockers, missing real behavior proof, and a config/API boundary conflict with existing cache-TTL pruning plus #62475. Canonical path: Close this PR as superseded by #62179. So I’m closing this here and keeping the remaining discussion on #62179. Review detailsBest possible solution: Close this PR as superseded by #62179. Do we have a high-confidence way to reproduce the issue? Yes for the review blockers: PR-head source inspection shows the reset provider gate, stale cache-touch baseline, raw alias iteration, command-only timestamp risk, and failing Moonshot test expectation. I did not run live compact/reset behavior in this read-only review. Is this the best way to solve the issue? No. The direction is plausible, but this implementation is not the safest merge path until maintainers choose the config/API boundary and the session-state defects are fixed. Security review: Security review cleared: No concrete security or supply-chain concern was found; the diff touches runtime TypeScript and tests without dependency, workflow, install, release, package, or secret-handling changes. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against c4525104e9e0. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
ClawSweeper applied the proposed close for this PR.
|
Summary
sessionCacheTtlplustimeBasedContextCompactcompactandresetmaintenance modes instead of implicitly coupling both behaviors togetherWhy
This change adds two time-based session maintenance workflows for long-lived sessions:
compactresetThe new behavior is controlled explicitly per model:
sessionCacheTtldefines the timeout windowtimeBasedContextCompactchooses the maintenance algorithm:none,compact, orresetcacheRetentionremains a model prompt-cache setting and is no longer used to enable or disable these new maintenance features.Implementation notes
cacheRetention: "short"sessionCacheTtl: "1h"as a defaulted timeout value without auto-enabling maintenance modesessionCacheTtlexceeds an explicitly configured model cache TTLlastUserMessageAt,lastAssistantMessageAt,lastCacheTouchAt, andlastIdleCompactionForCacheTouchAtin session stateValidation
pnpm exec vitest run src/gateway/session-cache-maintenance.test.ts src/gateway/server-maintenance.test.ts src/config/config.pruning-defaults.test.ts src/agents/pi-embedded-runner/cache-ttl.test.ts src/agents/pi-embedded-runner/run/attempt.spawn-workspace.cache-ttl.test.ts src/agents/pi-embedded-runner/extra-params.cache-retention-default.test.ts src/commands/agent/session-store.test.tsNODE_OPTIONS=--max-old-space-size=8192 pnpm exec tsc -p tsconfig.json --noEmit