cron: clean up sessions and run logs on job removal (#46369)#46475
cron: clean up sessions and run logs on job removal (#46369)#46475xydt-610 wants to merge 2 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes an orphaned-data bug where Changes:
Issues found:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/cron/session-reaper.ts
Line: 17-18
Comment:
**Duplicate imports from same module**
`parseAgentSessionKey` and `isCronRunSessionKey` are imported from the same module in two separate statements. These should be combined into one.
```suggestion
import { parseAgentSessionKey, isCronRunSessionKey } from "../sessions/session-key-utils.js";
```
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/cron/session-reaper.ts
Line: 175-180
Comment:
**Missing deduplication guard differs from `sweepCronRunSessions`**
`removedSessions.set(entry.sessionId, entry.sessionFile)` unconditionally overwrites any previously stored value for a given `sessionId`. In `sweepCronRunSessions` (lines 97–99), the equivalent map is populated with a guard that preserves a non-`undefined` `sessionFile` over a `undefined` one:
```ts
if (!prunedSessions.has(entry.sessionId) || entry.sessionFile) {
prunedSessions.set(entry.sessionId, entry.sessionFile);
}
```
Without this guard, if multiple session keys share the same `sessionId` and the last-visited key carries `sessionFile: undefined`, the file-path reference used by `archiveRemovedSessionTranscripts` will be lost, causing that session's transcript to be skipped during cleanup. The fix is to apply the same guard here:
```suggestion
if (entry) {
if (!removedSessions.has(entry.sessionId) || entry.sessionFile) {
removedSessions.set(entry.sessionId, entry.sessionFile);
}
}
```
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/cron/service/ops.ts
Line: 349-361
Comment:
**Session cleanup silently skipped when only `sessionStorePath` is configured**
Session purge is gated on `state.deps.resolveSessionStorePath` being present. However, `CronServiceDeps` also exposes a simpler static `sessionStorePath?: string` that the existing `sweepCronRunSessions` reaper already relies on. If a deployment only wires up `sessionStorePath` (not `resolveSessionStorePath`), the new cleanup block is entirely skipped — the base session (`cron:<jobId>`) is never cleaned up by the sweeper either (the sweeper only prunes run sessions), so the ghost session would still linger indefinitely.
Consider falling back to `state.deps.sessionStorePath` when `resolveSessionStorePath` is absent:
```ts
const sessionStorePath =
state.deps.resolveSessionStorePath
? state.deps.resolveSessionStorePath(job.agentId)
: state.deps.sessionStorePath;
if (job && sessionStorePath) {
void purgeCronJobSessions({ jobId: id, sessionStorePath, log: state.deps.log })
.catch((err) => { ... });
}
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: edbd266 |
| import { parseAgentSessionKey } from "../sessions/session-key-utils.js"; | ||
| import { isCronRunSessionKey } from "../sessions/session-key-utils.js"; |
There was a problem hiding this comment.
Duplicate imports from same module
parseAgentSessionKey and isCronRunSessionKey are imported from the same module in two separate statements. These should be combined into one.
| import { parseAgentSessionKey } from "../sessions/session-key-utils.js"; | |
| import { isCronRunSessionKey } from "../sessions/session-key-utils.js"; | |
| import { parseAgentSessionKey, isCronRunSessionKey } from "../sessions/session-key-utils.js"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/session-reaper.ts
Line: 17-18
Comment:
**Duplicate imports from same module**
`parseAgentSessionKey` and `isCronRunSessionKey` are imported from the same module in two separate statements. These should be combined into one.
```suggestion
import { parseAgentSessionKey, isCronRunSessionKey } from "../sessions/session-key-utils.js";
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if (isBase || isRun) { | ||
| const entry = store[key]; | ||
| if (entry) { | ||
| removedSessions.set(entry.sessionId, entry.sessionFile); | ||
| } | ||
| delete store[key]; |
There was a problem hiding this comment.
Missing deduplication guard differs from sweepCronRunSessions
removedSessions.set(entry.sessionId, entry.sessionFile) unconditionally overwrites any previously stored value for a given sessionId. In sweepCronRunSessions (lines 97–99), the equivalent map is populated with a guard that preserves a non-undefined sessionFile over a undefined one:
if (!prunedSessions.has(entry.sessionId) || entry.sessionFile) {
prunedSessions.set(entry.sessionId, entry.sessionFile);
}Without this guard, if multiple session keys share the same sessionId and the last-visited key carries sessionFile: undefined, the file-path reference used by archiveRemovedSessionTranscripts will be lost, causing that session's transcript to be skipped during cleanup. The fix is to apply the same guard here:
| if (isBase || isRun) { | |
| const entry = store[key]; | |
| if (entry) { | |
| removedSessions.set(entry.sessionId, entry.sessionFile); | |
| } | |
| delete store[key]; | |
| if (entry) { | |
| if (!removedSessions.has(entry.sessionId) || entry.sessionFile) { | |
| removedSessions.set(entry.sessionId, entry.sessionFile); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/session-reaper.ts
Line: 175-180
Comment:
**Missing deduplication guard differs from `sweepCronRunSessions`**
`removedSessions.set(entry.sessionId, entry.sessionFile)` unconditionally overwrites any previously stored value for a given `sessionId`. In `sweepCronRunSessions` (lines 97–99), the equivalent map is populated with a guard that preserves a non-`undefined` `sessionFile` over a `undefined` one:
```ts
if (!prunedSessions.has(entry.sessionId) || entry.sessionFile) {
prunedSessions.set(entry.sessionId, entry.sessionFile);
}
```
Without this guard, if multiple session keys share the same `sessionId` and the last-visited key carries `sessionFile: undefined`, the file-path reference used by `archiveRemovedSessionTranscripts` will be lost, causing that session's transcript to be skipped during cleanup. The fix is to apply the same guard here:
```suggestion
if (entry) {
if (!removedSessions.has(entry.sessionId) || entry.sessionFile) {
removedSessions.set(entry.sessionId, entry.sessionFile);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| if (job && state.deps.resolveSessionStorePath) { | ||
| const sessionStorePath = state.deps.resolveSessionStorePath(job.agentId); | ||
| void purgeCronJobSessions({ | ||
| jobId: id, | ||
| sessionStorePath, | ||
| log: state.deps.log, | ||
| }).catch((err) => { | ||
| state.deps.log.warn( | ||
| { err: String(err), jobId: id }, | ||
| "cron: failed to purge job sessions on removal", | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Session cleanup silently skipped when only sessionStorePath is configured
Session purge is gated on state.deps.resolveSessionStorePath being present. However, CronServiceDeps also exposes a simpler static sessionStorePath?: string that the existing sweepCronRunSessions reaper already relies on. If a deployment only wires up sessionStorePath (not resolveSessionStorePath), the new cleanup block is entirely skipped — the base session (cron:<jobId>) is never cleaned up by the sweeper either (the sweeper only prunes run sessions), so the ghost session would still linger indefinitely.
Consider falling back to state.deps.sessionStorePath when resolveSessionStorePath is absent:
const sessionStorePath =
state.deps.resolveSessionStorePath
? state.deps.resolveSessionStorePath(job.agentId)
: state.deps.sessionStorePath;
if (job && sessionStorePath) {
void purgeCronJobSessions({ jobId: id, sessionStorePath, log: state.deps.log })
.catch((err) => { ... });
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/service/ops.ts
Line: 349-361
Comment:
**Session cleanup silently skipped when only `sessionStorePath` is configured**
Session purge is gated on `state.deps.resolveSessionStorePath` being present. However, `CronServiceDeps` also exposes a simpler static `sessionStorePath?: string` that the existing `sweepCronRunSessions` reaper already relies on. If a deployment only wires up `sessionStorePath` (not `resolveSessionStorePath`), the new cleanup block is entirely skipped — the base session (`cron:<jobId>`) is never cleaned up by the sweeper either (the sweeper only prunes run sessions), so the ghost session would still linger indefinitely.
Consider falling back to `state.deps.sessionStorePath` when `resolveSessionStorePath` is absent:
```ts
const sessionStorePath =
state.deps.resolveSessionStorePath
? state.deps.resolveSessionStorePath(job.agentId)
: state.deps.sessionStorePath;
if (job && sessionStorePath) {
void purgeCronJobSessions({ jobId: id, sessionStorePath, log: state.deps.log })
.catch((err) => { ... });
}
```
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: edbd266e77
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| void purgeCronJobSessions({ | ||
| jobId: id, | ||
| sessionStorePath, | ||
| log: state.deps.log, | ||
| }).catch((err) => { |
There was a problem hiding this comment.
Await session purge before returning from cron.remove
cron.remove fires purgeCronJobSessions in a detached promise (void ...) and returns immediately, so callers can observe stale cron sessions right after a successful remove response; this is especially visible when the UI deletes a job and immediately reloads session lists. Because cleanup is best-effort background work here, a process shutdown/restart between remove and purge also leaves orphaned session records behind, which is the same ghost-session symptom this change is trying to fix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9717195e4e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| // Clean up run log file | ||
| const logPath = resolveCronRunLogPath({ storePath: state.deps.storePath, jobId: id }); | ||
| void fs.unlink(logPath).catch(() => undefined); |
There was a problem hiding this comment.
Await run-log cleanup before returning from cron.remove
The new run-log cleanup is launched as void fs.unlink(...), so cron.remove can return success before the log file is actually deleted. In flows that delete a job and immediately refresh run-history data, the removed job’s log can still appear transiently, and if the process exits/crashes between return and the async unlink, the orphaned log file persists indefinitely. This undermines the cleanup guarantee this change is intended to provide.
Useful? React with 👍 / 👎.
|
@skyfallsin Please help review the code, thank you. |
|
Closing this as duplicate or superseded after Codex automated review. Close #46475 as superseded by the already-open canonical bug/fix thread for #46369, especially PR #46506. Current main still has the reported gap, so this is not implemented-on-main: Best possible solution: Close #46475 as the duplicate/superseded PR and consolidate the remaining #46369 work in #46506 or a maintainer-authored patch. The final fix should clean both canonical and run cron sessions, delete the per-job run log, select the correct per-agent session store with fallback behavior, avoid detached-cleanup races where feasible, and include regression tests for 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 8bbb143ab87e. |
Summary
cron.removeonly deleted the job fromcron.json, leaving orphaned run logs incron/runs/and session records insessions.json.purgeCronJobSessionstosession-reaper.tsto clean up base and run sessions from the session store.removeinops.tsto callpurgeCronJobSessionsand delete the run log file on job removal.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
None (internal cleanup).
Security Impact (required)
Repro + Verification
Environment
Steps
cron.remove.Expected
Actual
Evidence
pnpm checkpassed.Human Verification (required)
Compatibility / Migration