Cron: auto-cleanup isolated :run: session entries#12570
Cron: auto-cleanup isolated :run: session entries#12570gigi-trifle wants to merge 3 commits intoopenclaw:mainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/cron/service/timer.ts
Outdated
| /** True if the session key is a per-run cron key (contains `:run:`). */ | ||
| function isCronRunKey(key: string): boolean { | ||
| return /:run:/.test(key); | ||
| } |
There was a problem hiding this comment.
Overbroad :run: match
isCronRunKey currently treats any sessionKey containing ":run:" as a cron run key (/:run:/). That will incorrectly delete non-cron session entries if their key format happens to include :run: (e.g. other subsystems using run markers). Since cleanupCronRunSession does a hard delete store[sessionKey], this can remove unrelated sessions. Consider reusing the stricter isCronRunSessionKey logic from src/cron/service/sweeper.ts (or otherwise constrain the match to ...:cron:<jobId>:run:<uuid>).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/service/timer.ts
Line: 16:19
Comment:
**Overbroad `:run:` match**
`isCronRunKey` currently treats any `sessionKey` containing `":run:"` as a cron run key (`/:run:/`). That will incorrectly delete non-cron session entries if their key format happens to include `:run:` (e.g. other subsystems using run markers). Since `cleanupCronRunSession` does a hard `delete store[sessionKey]`, this can remove unrelated sessions. Consider reusing the stricter `isCronRunSessionKey` logic from `src/cron/service/sweeper.ts` (or otherwise constrain the match to `...:cron:<jobId>:run:<uuid>`).
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 33ef6bb — removed the local isCronRunKey and replaced both call sites with an import of isCronRunSessionKey from sweeper.ts, which uses the stricter /:cron:[^:]+:run:[^:]+$/ pattern.
| const { cfg, log } = params; | ||
| const agentId = resolveDefaultAgentId(cfg); | ||
| const sessStorePath = resolveStorePath(cfg.session?.store, { agentId }); | ||
| const now = Date.now(); | ||
| let removed = 0; |
There was a problem hiding this comment.
Sweeper ignores per-job agentId
sweepStaleCronRunSessions always resolves the session store path using resolveDefaultAgentId(cfg) (sessStorePath = resolveStorePath(cfg.session?.store, { agentId })). But isolated cron runs can target non-default agents (job.agentId), and their :run: entries will be written to that agent’s session store. As written, the sweeper will never clean up stale :run: sessions for non-default agents.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-cron.ts
Line: 147:151
Comment:
**Sweeper ignores per-job agentId**
`sweepStaleCronRunSessions` always resolves the session store path using `resolveDefaultAgentId(cfg)` (`sessStorePath = resolveStorePath(cfg.session?.store, { agentId })`). But isolated cron runs can target non-default agents (`job.agentId`), and their `:run:` entries will be written to that agent’s session store. As written, the sweeper will never clean up stale `:run:` sessions for non-default agents.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 33ef6bb — sweepStaleCronRunSessions now iterates over all configured agents via listAgentIds(cfg) and sweeps each agent's session store independently.
…ores - Replace overbroad isCronRunKey (/:run:/) in timer.ts with the stricter isCronRunSessionKey from sweeper.ts (/:cron:[^:]+:run:[^:]+$/) - Update sweepStaleCronRunSessions to iterate over all configured agent session stores via listAgentIds(), not just the default agent Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Fix landed on main via e19a235. Thanks! |
2 participants
Summary
:run:session entries from the session store after isolated cron job completion (default behavior via newcleanupfield, defaulting to"delete").:run:entries older than 10 minutes, catching any missed by crashes or race conditions.cleanup?: "delete" | "keep"field toCronJobtype, schemas, and job create/patch logic.Fixes #11260.
Test plan
cleanup("delete")cleanup: "keep"sessionKeyis not a:run:keyisCronRunSessionKeyregex matchingpnpm lintclean (4 pre-existing errors in unrelated file)pnpm buildpasses🤖 Generated with Claude Code
Greptile Overview
Greptile Summary
This PR adds a
cleanup?: "delete" | "keep"field to cron jobs (defaulting to delete) and invokes a best-effort cleanup hook after isolated job execution to remove per-run:run:session entries from the session store. It also introduces a background interval sweeper insrc/gateway/server-cron.tsthat periodically deletes stale:run:entries (older than 10 minutes) as a crash/race-condition backstop, and updates the gateway protocol cron schemas accordingly.Confidence Score: 3/5
/:run:/matching in the execution path can delete non-cron sessions, and (2) the sweeper currently only scans the default agent’s session store, missing stale:run:sessions for other agents targeted by cron jobs.(2/5) Greptile learns from your feedback when you react with thumbs up/down!
Context used:
dashboard- CLAUDE.md (source)dashboard- AGENTS.md (source)