fix(cron): remove duplicate main-session relay for isolated jobs with announce delivery#15737
Conversation
| // Note: We do NOT enqueue a system event here for isolated jobs with | ||
| // delivery.requested=true. The runIsolatedAgentJob function already | ||
| // handles announce delivery via runSubagentAnnounceFlow, which posts | ||
| // the response to the main session. Enqueuing here would cause duplicate | ||
| // messages. See: https://github.com/openclaw/openclaw/issues/15692 |
There was a problem hiding this comment.
Isolated jobs no longer announce
executeJobCore() no longer calls enqueueSystemEvent() for any isolated job result, but the removed code only posted to main when resolveCronDeliveryPlan(job).requested and res.summary?.trim() were truthy. runIsolatedAgentJob is just an injected dep (CronServiceDeps), and in the unit tests it’s stubbed to return {status, summary} without performing the announce flow—so with this change, isolated jobs that previously relied on the CronService-level relay (i.e., any delivery.requested=true job when the injected runner doesn’t itself call runSubagentAnnounceFlow) will silently stop posting the summary to main.
If the intent is “only skip the relay when the isolated runner is the cron isolated-agent implementation that already announces”, this needs to be gated accordingly (e.g., only skip for delivery.mode=announce when the runner is known to do announce delivery), otherwise behavior changes for other runIsolatedAgentJob implementations.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cron/service/timer.ts
Line: 485:489
Comment:
**Isolated jobs no longer announce**
`executeJobCore()` no longer calls `enqueueSystemEvent()` for *any* isolated job result, but the removed code only posted to main when `resolveCronDeliveryPlan(job).requested` and `res.summary?.trim()` were truthy. `runIsolatedAgentJob` is just an injected dep (`CronServiceDeps`), and in the unit tests it’s stubbed to return `{status, summary}` without performing the announce flow—so with this change, isolated jobs that previously relied on the CronService-level relay (i.e., any `delivery.requested=true` job when the injected runner doesn’t itself call `runSubagentAnnounceFlow`) will silently stop posting the summary to main.
If the intent is “only skip the relay when the isolated runner is the cron isolated-agent implementation that already announces”, this needs to be gated accordingly (e.g., only skip for `delivery.mode=announce` when the runner is known to do announce delivery), otherwise behavior changes for other `runIsolatedAgentJob` implementations.
How can I resolve this? If you propose a fix, please make it concise.|
Thanks for the thorough review! To clarify: this is an intentional fix for the duplicate message bug (#15692). The issue was:
Why the tests pass:
Re: other implementations: Happy to add a comment documenting this expectation if that would help! 🦞 |
9a6263c to
b885581
Compare
|
Great catch! You're right that removing the relay entirely would break other Updated approach:
This preserves backwards compatibility while fixing the duplicate message issue for explicit delivery configs. |
…ayload Fixes openclaw#15692 The previous fix was too broad — it removed the relay for ALL isolated jobs. This broke backwards compatibility for jobs without explicit delivery config. The correct behavior is: - If job.delivery exists → isolated runner handles it via runSubagentAnnounceFlow - If only legacy payload.deliver fields → relay to main if requested (original behavior) This addresses Greptile's review feedback about runIsolatedAgentJob being an injected dependency that might not call runSubagentAnnounceFlow. Uses resolveCronDeliveryPlan().source to distinguish between explicit delivery config and legacy payload-only jobs.
b885581 to
cb7bc43
Compare
|
Landed via temp rebase onto main.
Thanks @brandonwise! |
|
Correction:
|
…law#15737) (thanks @brandonwise) (cherry picked from commit b870354) # Conflicts: # CHANGELOG.md
…law#15737) (thanks @brandonwise) (cherry picked from commit b870354) # Conflicts: # CHANGELOG.md
Summary
Fixes #15692
Previously, isolated cron jobs with
delivery.mode=announcecaused duplicate messages:runIsolatedAgentJob()callsrunSubagentAnnounceFlow()which sends a message to the main sessionexecuteJobCore()ALSO calledenqueueSystemEvent()+requestHeartbeatNow()which triggered another main session runThe second step was redundant since
runSubagentAnnounceFlowalready handles the announce delivery.Changes
enqueueSystemEvent+requestHeartbeatNowblock fromexecuteJobCore()for isolated jobsTesting
Greptile Overview
Greptile Summary
This PR removes the CronService-level “post summary back to main session” relay from
executeJobCore()for isolated jobs, based on the rationale thatrunIsolatedAgentJob()’s announce flow already delivers the message (fixing duplicate relays fordelivery.mode=announce). Tests were updated to assertenqueueSystemEvent()/requestHeartbeatNow()are not called for isolated jobs.One behavior risk:
runIsolatedAgentJobis an injected dependency. If it does not itself run the announce flow, isolated jobs withdelivery.requested=truewill no longer be relayed to the main session at all (previously they were, gated byresolveCronDeliveryPlan(job).requestedand a non-emptyres.summary). Consider scoping the removal to only the cases where the injected runner is known to already announce, or reintroducing the relay for other runners.Confidence Score: 3/5
runIsolatedAgentJobis a dependency, this can break consumers that relied on CronService to perform the relay rather than the runner.Last reviewed commit: 9a6263c