Skip to content

fix(tasks): preserve both cron-run session key shapes during maintenance#96352

Merged
vincentkoc merged 4 commits into
openclaw:mainfrom
ly-wang19:fix/cron-running-jobid-both-shapes
Jun 24, 2026
Merged

fix(tasks): preserve both cron-run session key shapes during maintenance#96352
vincentkoc merged 4 commits into
openclaw:mainfrom
ly-wang19:fix/cron-running-jobid-both-shapes

Conversation

@ly-wang19

Copy link
Copy Markdown
Contributor

What Problem This Solves

openclaw tasks maintenance prunes stale session-registry rows but preserves rows belonging to running cron jobs. readRunningCronJobIds (src/commands/tasks.ts) built that preserve-set with job.id.toLowerCase() only. But cron-run session keys carry two job-segment shapes:

  • Main-session runs: slugified via normalizeCronLaneSegment(job.id)agent:main:cron:daily-report:run:… (resolveMainSessionCronRunSessionKey).
  • Default-isolated runs (no explicit sessionKey): raw lowercased — built from cron:${job.id} (src/cron/service/task-runs.ts) → resolveCronAgentSessionKeytoAgentStoreSessionKey, which lowercases but does not slugify → agent:main:cron:daily report:run:….

So for any non-slug job id (e.g. "Daily Report"), the lowercase-only matcher ("daily report") preserved the isolated run but pruned the live main-session run ("daily-report") as stale.

Why This Change Was Made

The preserve-set now includes both shapes — job.id.toLowerCase() (raw isolated) and normalizeCronLaneSegment(job.id, "job") (slug main-session). Because it only adds to a preserve-set, it is strictly more-preserving: no live running cron session can be dropped. This supersedes #95319, which switched to slug-only and would have traded the main-session false-prune for an isolated-session false-prune.

User Impact

tasks maintenance --apply no longer deletes the live session row of a running cron job whose id is not already a slug (contains spaces/uppercase/punctuation), for either the main-session or default-isolated run shape. No config or output-shape change.

Evidence

oxfmt, oxlint, tsgo:core all pass. New colocated regression test exercises the real tasksMaintenanceCommand({apply:true}) against a real session store + cron store.

Real behavior proof

Behavior addressed: tasks maintenance --apply pruned the live main-session cron-run session row of a running non-slug job id (e.g. "Daily Report") because the running-job preserve-set used only the raw lowercased id, not the slugified session-key segment.

Real environment tested: Ran the exported tasksMaintenanceCommand({ json: true, apply: true }) via the colocated Vitest harness (node scripts/run-vitest.mjs) on Node v22.22.1 against a real temp state dir — a real sessions.json store and a real cron jobs.json store written to disk — not mocks.

Exact steps or command run after this patch: seed a running cron job id "Daily Report"; write three stale (8-day-old) cron-run session rows — slug agent:main:cron:daily-report:run:old-run, raw agent:main:cron:daily report:run:old-run, and non-running agent:main:cron:retired-job:run:old-run; run tasksMaintenanceCommand({apply:true}); then node scripts/run-vitest.mjs src/commands/tasks.test.ts -t "preserves both slug and raw", plus node scripts/run-oxlint.mjs and pnpm tsgo:core.

Evidence after fix:

after-fix test  -> exit 0 (pass): both slug + raw rows survive, retired-job row pruned
fail-before     -> exit 1 (fail) when src/commands/tasks.ts is reverted to main's job.id.toLowerCase():
                   the slug main-session row is wrongly pruned, so the assertion fails
oxlint          -> exit 0
tsgo:core       -> exit 0

Observed result after fix: the running job's slug (daily-report) and raw (daily report) cron-run session rows are both retained; the non-running job's stale row is still pruned. Reverting the one-line matcher change reproduces the prune (fail-before), confirming the test pins the fix.

What was not tested: a full live gateway run that actually generates the isolated vs main-session keys end-to-end (no live gateway in this environment); the segment shapes are taken from the production key builders (task-runs.ts, toAgentStoreSessionKey) and asserted at the maintenance-command level with on-disk stores.

@clawsweeper

clawsweeper Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed June 24, 2026, 9:52 AM ET / 13:52 UTC.

Summary
The PR changes task session-registry maintenance to preserve multiple running cron-run session key segments and adds colocated regression coverage.

PR surface: Source +20, Tests +119. Total +139 across 2 files.

Reproducibility: yes. Source inspection shows current main lowercases running cron ids while runtime producers can write slugged or custom cron-run session segments; I did not execute tests because this review is read-only.

Review metrics: 1 noteworthy metric.

  • Cron-run segment coverage: 2 covered, 1 missing. The matcher now covers job-id and job.sessionKey cron segments, but not the sessionTarget cron segment that the runtime can also produce.

Stored data model
Persistent data-model change detected: serialized state: src/commands/tasks.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: canonical
Canonical: #96352
Summary: This PR is the current canonical both-shapes repair for the cron session-registry running-id mismatch, with one remaining target-alias gap to fix before merge.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

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

Rank-up moves:

  • [P2] Add sessionTarget cron-alias preservation and a focused regression test.

Risk before merge

  • [P1] Merging as-is would leave running cron jobs targeted with session:cron:* pruneable when their job id does not match that target segment.

Maintainer options:

  1. Cover sessionTarget cron aliases before merge (recommended)
    Add the sessionTarget-derived cron segment to the preserve set and prove it with the same task-maintenance harness.
  2. Accept the partial repair knowingly
    Maintainers could merge only the raw/slug/job.sessionKey coverage and track session:cron:* target preservation separately, but that leaves a known live-prune gap.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Add preservation of cron segments derived from `resolveCronSessionTargetSessionKey(job.sessionTarget)` when it resolves to `cron:<segment>` or `agent:<id>:cron:<segment>`, keep `runningCronJobs` based on the filtered running-job count, and add a regression test for a running job with `sessionTarget: "session:cron:daily-monitor"`, no `sessionKey`, and a stale `agent:main:cron:daily-monitor:run:*` row.

Next step before merge

  • [P2] A narrow mechanical repair can add the missing sessionTarget cron segment and cover it in the existing maintenance test file.

Security
Cleared: The diff only changes task/session maintenance logic and tests; it does not alter dependencies, workflows, secrets, auth, network calls, package metadata, or code execution surfaces.

Review findings

  • [P2] Preserve cron sessionTarget aliases too — src/commands/tasks.ts:151-152
Review details

Best possible solution:

Derive the preserve set from the job id, job.sessionKey, and any cron-shaped sessionTarget key, while keeping runningCronJobs tied to the number of running jobs.

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

Yes. Source inspection shows current main lowercases running cron ids while runtime producers can write slugged or custom cron-run session segments; I did not execute tests because this review is read-only.

Is this the best way to solve the issue?

No, not yet. Preserving raw, slugged, and job.sessionKey aliases is the right direction, but the best fix also covers cron-shaped sessionTarget aliases before merge.

Full review comments:

  • [P2] Preserve cron sessionTarget aliases too — src/commands/tasks.ts:151-152
    This only adds aliases from job.sessionKey. A custom target such as session:cron:daily-monitor is accepted by resolveCronSessionTargetSessionKey, and the gateway passes that resolved key into the isolated cron run, which stores agent:main:cron:daily-monitor:run:<id>. With id: "job-uuid" and no job.sessionKey, the preserve set still misses daily-monitor, so maintenance can prune the live row. Include the sessionTarget-derived cron segment and add a regression test.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against f1e38f2ed637.

Label changes

Label changes:

  • add merge-risk: 🚨 session-state: The PR changes stale cron-run session pruning and still leaves one runtime-produced cron-run key shape vulnerable to live row pruning.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body includes copied after-fix and fail-before terminal output from tasksMaintenanceCommand using real temporary cron and session stores, which is sufficient for the raw/slug maintenance path it demonstrates.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.

Label justifications:

  • P2: This is a bounded task/session maintenance bug fix with limited blast radius but real session-state impact.
  • merge-risk: 🚨 session-state: The PR changes stale cron-run session pruning and still leaves one runtime-produced cron-run key shape vulnerable to live row pruning.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body includes copied after-fix and fail-before terminal output from tasksMaintenanceCommand using real temporary cron and session stores, which is sufficient for the raw/slug maintenance path it demonstrates.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes copied after-fix and fail-before terminal output from tasksMaintenanceCommand using real temporary cron and session stores, which is sufficient for the raw/slug maintenance path it demonstrates.
Evidence reviewed

PR surface:

Source +20, Tests +119. Total +139 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 30 10 +20
Tests 1 119 0 +119
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 149 10 +139

Acceptance criteria:

  • [P1] node scripts/run-vitest.mjs src/commands/tasks.test.ts -t "preserves".
  • [P1] node scripts/run-oxlint.mjs src/commands/tasks.ts src/commands/tasks.test.ts.
  • [P1] pnpm tsgo:core.

What I checked:

  • Repository policy read: Root AGENTS.md was read fully; the session-state, whole-surface PR review, and best-fix guidance affected this review. (AGENTS.md:1, f1e38f2ed637)
  • Current main only preserves raw lowercased job ids: readRunningCronJobIds on current main builds the preserve set from job.id.toLowerCase(), which misses slugified cron-run segments for non-slug job ids. (src/commands/tasks.ts:131, f1e38f2ed637)
  • Maintenance matches cron-run row segments: Session-registry maintenance extracts the cron:<segment>:run:<id> segment and only preserves stale cron-run rows when that segment appears in the running set. (src/config/sessions/session-registry-maintenance.ts:25, f1e38f2ed637)
  • Main-session cron runs slugify the job id: resolveMainSessionCronRunSessionKey uses normalizeCronLaneSegment(job.id, "job"), producing a slug segment such as daily-report. (src/cron/service/task-runs.ts:30, f1e38f2ed637)
  • Default isolated cron runs can keep raw cron bases: The gateway isolated-agent path falls back to cron:${job.id}, and the isolated runner appends :run:<sessionId> when the base starts with cron:. (src/gateway/server-cron.ts:395, f1e38f2ed637)
  • PR head preserves raw, slug, and job.sessionKey segments: The PR head returns a running-job count separately from the expanded preserve ids and adds raw, slugified, and job.sessionKey-derived cron segments. (src/commands/tasks.ts:147, 95a0539ee186)

Likely related people:

  • brokemac79: Authored merged PR [AI-assisted] fix(tasks): prune stale cron session registry entries #74023, which added stale cron session-registry pruning while preserving running cron rows. (role: introduced behavior; confidence: high; commits: c1993601c111, 46a3f6be1fca, 1539efff5866; files: src/commands/tasks.ts, src/commands/tasks.test.ts)
  • jalehman: Authored merged PR refactor: add task session registry maintenance seam #93734, which created the session-registry maintenance helper and current preserve-set contract. (role: recent area contributor; confidence: high; commits: 1153f94ef45d, 784518241040; files: src/commands/tasks.ts, src/config/sessions/session-registry-maintenance.ts, src/config/sessions/session-registry-maintenance.test.ts)
  • Vincent Koc: Current-main blame in this checkout points the affected task/session maintenance lines to commit 2ab3b223ed697bc62734f811f04265cdda647677; the shallow/grafted history makes this a routing hint rather than full feature provenance. (role: current line author; confidence: medium; commits: 2ab3b223ed69; files: src/commands/tasks.ts, src/config/sessions/session-registry-maintenance.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 proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 24, 2026
ly-wang19 and others added 4 commits June 24, 2026 22:00
Session-registry maintenance keeps running cron jobs' session rows, but
readRunningCronJobIds built the preserve-set with job.id.toLowerCase() only.
Cron-run session keys carry two job-segment shapes: main-session runs use the
slugified segment (normalizeCronLaneSegment, e.g. "daily-report") while
default-isolated runs use the raw lowercased id ("daily report", built from
cron:${job.id} via toAgentStoreSessionKey, which lowercases but does not
slugify). The lowercase-only matcher preserved isolated runs but pruned
main-session runs of any non-slug job id (e.g. "Daily Report") as stale.

Preserve both shapes (raw lowercased id and slugified segment). This is
strictly more-preserving, so no live running cron session is dropped. Adds a
regression test seeding both a slug main-session run and a raw isolated run for
a non-slug job id, asserting both survive while a non-running job's run is still
pruned.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@vincentkoc vincentkoc force-pushed the fix/cron-running-jobid-both-shapes branch from 95a0539 to f5b8a9d Compare June 24, 2026 14:00
@vincentkoc vincentkoc merged commit cb13be3 into openclaw:main Jun 24, 2026
92 checks passed
@vincentkoc

Copy link
Copy Markdown
Member

Merged via squash.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jun 25, 2026
…nce (openclaw#96352)

* fix(tasks): preserve both cron-run session key shapes during maintenance

Session-registry maintenance keeps running cron jobs' session rows, but
readRunningCronJobIds built the preserve-set with job.id.toLowerCase() only.
Cron-run session keys carry two job-segment shapes: main-session runs use the
slugified segment (normalizeCronLaneSegment, e.g. "daily-report") while
default-isolated runs use the raw lowercased id ("daily report", built from
cron:${job.id} via toAgentStoreSessionKey, which lowercases but does not
slugify). The lowercase-only matcher preserved isolated runs but pruned
main-session runs of any non-slug job id (e.g. "Daily Report") as stale.

Preserve both shapes (raw lowercased id and slugified segment). This is
strictly more-preserving, so no live running cron session is dropped. Adds a
regression test seeding both a slug main-session run and a raw isolated run for
a non-slug job id, asserting both survive while a non-running job's run is still
pruned.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(tasks): match cron session keys to target shape

* fix(tasks): preserve active cron aliases across retargeting

* fix(tasks): retain explicit cron session aliases

---------

Co-authored-by: ly-wang19 <ly-wang19@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Vincent Koc <vincentkoc@ieee.org>
QiuYuang pushed a commit to QiuYuang/openclaw that referenced this pull request Jul 1, 2026
…nce (openclaw#96352)

* fix(tasks): preserve both cron-run session key shapes during maintenance

Session-registry maintenance keeps running cron jobs' session rows, but
readRunningCronJobIds built the preserve-set with job.id.toLowerCase() only.
Cron-run session keys carry two job-segment shapes: main-session runs use the
slugified segment (normalizeCronLaneSegment, e.g. "daily-report") while
default-isolated runs use the raw lowercased id ("daily report", built from
cron:${job.id} via toAgentStoreSessionKey, which lowercases but does not
slugify). The lowercase-only matcher preserved isolated runs but pruned
main-session runs of any non-slug job id (e.g. "Daily Report") as stale.

Preserve both shapes (raw lowercased id and slugified segment). This is
strictly more-preserving, so no live running cron session is dropped. Adds a
regression test seeding both a slug main-session run and a raw isolated run for
a non-slug job id, asserting both survive while a non-running job's run is still
pruned.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(tasks): match cron session keys to target shape

* fix(tasks): preserve active cron aliases across retargeting

* fix(tasks): retain explicit cron session aliases

---------

Co-authored-by: ly-wang19 <ly-wang19@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Vincent Koc <vincentkoc@ieee.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands Command implementations merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. size: S status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants