fix(cron): clean up deleteAfterRun direct deliveries by MonkeyLeeT · Pull Request #67807 · openclaw/openclaw

MonkeyLeeT · 2026-04-16T20:24:22Z

Summary

Problem: direct cron deliveries only cleaned up deleteAfterRun sessions on the text-finalization path; structured payloads and threaded direct deliveries could leave isolated cron sessions behind.
Why it matters: deleteAfterRun jobs could accumulate transcript state and hit the context-overflow behavior reported in Cron sessions with deleteAfterRun: true not being deleted — causes context overflow #67718.
What changed: direct delivery now routes through a shared cleanup wrapper, and session deletion is deduplicated per dispatch so silent direct deliveries only issue one sessions.delete call.
What did NOT change (scope boundary): this does not change cron retention policy, the session reaper, or job-store deletion rules.

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes Cron sessions with deleteAfterRun: true not being deleted — causes context overflow #67718
Related #
This PR fixes a bug or regression

Root Cause (if applicable)

Root cause: cleanup for deleteAfterRun direct cron sessions was attached to the text finalization path, but the structured direct-delivery and threaded direct-delivery branches called deliverViaDirect without that cleanup.
Missing detection / guardrail: there was no regression test asserting cleanup for the raw useDirectDelivery branches, and no guard against duplicate deletion after refactoring cleanup into a shared wrapper.
Contributing context (if known): silent direct deliveries can return through the same direct path after payload normalization, which made duplicate cleanup easy to introduce.

Regression Test Plan (if applicable)

Coverage level that should have caught this:
- Unit test
- Seam / integration test
- End-to-end test
- Existing coverage already sufficient
Target test or file: src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts
Scenario the test should lock in: deleteAfterRun cleans up after threaded direct deliveries, structured direct deliveries, and structured silent replies without double-deleting the session.
Why this is the smallest reliable guardrail: the bug lives entirely inside dispatchCronDelivery branch selection and cleanup behavior, so the isolated dispatch test file exercises the exact control flow without extra cron runtime noise.
Existing test that already covers this (if any): existing text and silent cleanup assertions in the same file covered only the text-finalization path.
If no new test is added, why not: N/A

User-visible / Behavior Changes

Cron jobs using deleteAfterRun: true now reliably delete isolated direct-delivery sessions after structured payload sends and threaded direct sends. Silent direct deliveries still clean up the session, but only issue one delete call.

Diagram (if applicable)

Before:
[direct delivery with structured payload/thread target] -> [send succeeds] -> [session kept]

After:
[direct delivery with structured payload/thread target] -> [send succeeds] -> [shared cleanup wrapper] -> [session deleted once]

Security Impact (required)

New permissions/capabilities? (No)
Secrets/tokens handling changed? (No)
New/changed network calls? (No)
Command/tool execution surface changed? (No)
Data access scope changed? (No)
If any Yes, explain risk + mitigation:

Repro + Verification

Environment

OS: macOS
Runtime/container: local Node test lane
Model/provider: N/A
Integration/channel (if any): cron direct-delivery dispatch
Relevant config (redacted): deleteAfterRun: true, isolated cron session

Steps

Run a cron agent turn that resolves to direct delivery through the structured-payload path or threaded direct-delivery path.
Set deleteAfterRun: true on the job.
Observe session cleanup behavior after delivery completes.

Expected

The isolated cron session is deleted after direct delivery finishes.
Silent direct deliveries delete the session exactly once.

Actual

Before this change, the structured/threaded direct-delivery branches could skip cleanup entirely.

Evidence

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

Verified scenarios: targeted dispatch tests for threaded direct delivery cleanup, structured direct delivery cleanup, and structured silent direct delivery single-delete behavior.
Edge cases checked: silent direct replies after payload normalization.
What you did not verify: full pnpm test / repo-wide gate in this shell, because the local environment lacks a pnpm binary on PATH; verification used the underlying Vitest cron config directly.

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

Backward compatible? (Yes)
Config/env changes? (No)
Migration needed? (No)
If yes, exact upgrade steps:

Risks and Mitigations

Risk: cleanup now runs through a shared wrapper for direct deliveries, which could accidentally double-delete the session on silent direct replies.
- Mitigation: the dispatch now tracks whether deletion already succeeded, and the regression test asserts a single sessions.delete call for the structured silent path.

greptile-apps · 2026-04-16T20:33:17Z

Greptile Summary

This PR fixes a gap where deleteAfterRun cleanup for cron direct-delivery sessions only ran on the text-finalization path; structured-payload and threaded direct deliveries bypassed it entirely. The fix introduces deliverViaDirectAndCleanup (a finally-guarded wrapper over deliverViaDirect) and a directCronSessionDeleted dedup flag that prevents double-deletes when finishSilentReplyDelivery is reached from inside deliverViaDirect, which is itself wrapped by deliverViaDirectAndCleanup. Three new regression tests cover the previously unprotected paths and include an exactly-once callGateway assertion for the structured silent-reply case.

Confidence Score: 5/5

Safe to merge; the cleanup logic is correct and well-tested, with only a minor test mock path issue that does not affect current test outcomes.

All findings are P2. The core fix (finally-guarded cleanup wrapper + dedup flag) is logically sound, covers all direct-delivery branches, and the new tests assert both cleanup and single-delete behavior correctly. The only concern is a mock targeting the wrong sessions module path, which currently has no impact on test results.

src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts — the vi.mock for the sessions module path should be verified/corrected, though it does not affect current test correctness.

Comments Outside Diff (1)

src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts, line 22-25 (link)

Mock path targets wrong module

vi.mock("../../config/sessions.js") resolves to src/config/sessions.ts, but delivery-dispatch.ts imports those functions from ../../config/sessions/main-session.js (src/config/sessions/main-session.ts) — a different module that sessions.ts does not re-export. Vitest won't intercept the production import.

Tests pass today because the real resolveAgentMainSessionKey with an empty config and agentId: "main" happens to return "agent:main:main" (the default mainKey constant is "main"), matching the mock value. Any future test using a non-default mainKey or global scope would silently call the real implementation rather than the mock, making those assertions unreliable.

The path in the mock should be updated to match the actual import location used by the production code.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/cron/isolated-agent/delivery-dispatch.double-announce.test.ts
Line: 22-25

Comment:
**Mock path targets wrong module**

`vi.mock("../../config/sessions.js")` resolves to `src/config/sessions.ts`, but `delivery-dispatch.ts` imports those functions from `../../config/sessions/main-session.js` (`src/config/sessions/main-session.ts`) — a different module that `sessions.ts` does not re-export. Vitest won't intercept the production import.

Tests pass today because the real `resolveAgentMainSessionKey` with an empty config and `agentId: "main"` happens to return `"agent:main:main"` (the default `mainKey` constant is `"main"`), matching the mock value. Any future test using a non-default `mainKey` or global scope would silently call the real implementation rather than the mock, making those assertions unreliable.

The path in the mock should be updated to match the actual import location used by the production code.

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/cron/isolated-agent/delivery-dispatch.double-announce.test.ts
Line: 22-25

Comment:
**Mock path targets wrong module**

`vi.mock("../../config/sessions.js")` resolves to `src/config/sessions.ts`, but `delivery-dispatch.ts` imports those functions from `../../config/sessions/main-session.js` (`src/config/sessions/main-session.ts`) — a different module that `sessions.ts` does not re-export. Vitest won't intercept the production import.

Tests pass today because the real `resolveAgentMainSessionKey` with an empty config and `agentId: "main"` happens to return `"agent:main:main"` (the default `mainKey` constant is `"main"`), matching the mock value. Any future test using a non-default `mainKey` or global scope would silently call the real implementation rather than the mock, making those assertions unreliable.

The path in the mock should be updated to match the actual import location used by the production code.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "Cron: clean up deleteAfterRun direct del..." | Re-trigger Greptile}

MonkeyLeeT · 2026-04-17T15:29:10Z

@hxy91819 PTAL

hxy91819 · 2026-04-18T10:17:28Z

Merged via squash.

Prepared head SHA: d23711c2e97030ec5862f47927d3bf41ddaba94f
Merge commit: 9501656a8eee3a8e8ed6faa2d43ef4671a3a8329

Thanks @MonkeyLeeT!

hxy91819 · 2026-04-18T10:27:29Z

Thanks @MonkeyLeeT for the clean fix and thorough regression tests — the double-delete guard via directCronSessionDeleted is a nice touch. When you have bandwidth, I documented a couple of follow-up ideas at our internal case notes that could further harden this surface:

Lift cleanup to the outer dispatchCronDelivery try/finally — so no future exit path can accidentally skip it. The current deliverViaDirectAndCleanup wrapper handles the known paths correctly, but a top-level guarantee would make the invariant structural rather than manual.
Add a meta-level assertion that callGateway('sessions.delete') is called exactly once for all deleteAfterRun=true test scenarios — catches new gaps automatically.
Flatten the nested closure dispatch — 850+ LOC with 3 levels of closures and multiple early returns makes it easy to miss an exit. A strategy-then-execute-then-cleanup flow would be more robust.

No rush on any of these — the current PR is solid and ready to land as-is.

…ordering

* changelog: move #67807 entry to Fixes section * changelog: move #67807 entry to Fixes section with correct PR-number ordering

Two related gaps in the CHANGELOG write path both caused PRs to land in the wrong place: 1) `resolve_merge_changelog_section` only consulted the environment override and PR labels. If a PR was only tagged with something like `size: S` (no `bug`/`fix`/`feature` label), the default fell through to Changes even when the PR title made the category obvious (e.g. `fix(cron): clean up deleteAfterRun direct deliveries` on PR openclaw#67807). That Conventional-Commits prefix is the repo's actual classification signal for most PRs and was being ignored. Add a title-prefix fallback after the label check: a title starting with `fix`, `bugfix`, or `hotfix` (followed by `(`, `:`, `!`, or whitespace) routes to Fixes; `feat`, `feature`, `enhance` route to Changes. Labels still win when present. Deliberately strict so `fixup!`, `fixing ...`, `fixture updates`, `featured posts` are not misread as `fix:`/`feat:`. 2) `appendUnreleasedChangelogEntry` deduped only on full-text equivalence. When a PR had a detailed entry written during prepare and then the merge step called `ensure_pr_changelog_entry` again with the short PR-title form, the two text bodies differed and the same PR got a duplicate line — that is what happened to PR openclaw#67679, which ended up with lines under both `### Changes` and `### Fixes`. Add a stronger precheck: scan the `## Unreleased` block for any existing bullet whose first PR reference matches the new entry's PR number. If one exists anywhere in the block (any sub-section), skip insertion. Dedup is scoped to Unreleased so the same PR number in a released block does not suppress a new Unreleased entry, and uses integer-equality on the PR number so `openclaw#67` is not shadowed by `openclaw#6767`. Together these two changes cover both of the recently observed failure modes: - Missing section signal from labels (PR openclaw#67807) is now picked up from the title. - Second ensure at merge time (PR openclaw#67679) no longer plants a duplicate in a different sub-section. Covered by shell smoke (27 resolver scenarios) and vitest (12 cases incl. the openclaw#67679 cross-section regression, released-block false positive, and PR-number prefix collision).

@hxy91819