fix(heartbeat): deliver exec events with disabled heartbeat interval#67913
fix(heartbeat): deliver exec events with disabled heartbeat interval#67913ToniDonDoni wants to merge 5 commits into
Conversation
Greptile SummaryThis PR fixes a bug where background exec completions were silently dropped when Confidence Score: 5/5Safe to merge; the only finding is a minor logging inconsistency that doesn't affect runtime behavior. Fix is correct and well-scoped: the global heartbeat kill-switch is preserved, the synthetic one-shot agent never advances the schedule, and both the scheduler and delivery layers are tested. The single P2 comment (stale src/infra/heartbeat-runner.ts lines 795 and 800 (cosmetic log inconsistency only).
|
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Addressed the Greptile review comment about stale heartbeat target logging. Problem addressed:
CI note:
Verification run locally:
Push:
|
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Addressed blocker 1 in What changed:
Verification run locally:
Blocker 2 is intentionally deferred for separate investigation. I did not broaden this change to synthetic |
|
@codex review What changed:
Regression coverage added:
Blocker 2 is deferred for separate investigation: I did not change the synthetic Verification run locally:
Pushed commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f83a93f4a5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reason: "disabled", | ||
| } satisfies HeartbeatRunResult; | ||
| const reasonKind = resolveHeartbeatReasonKind(params?.reason); | ||
| if (reasonKind !== "exec-event" || (!params?.sessionKey && !params?.agentId)) { |
There was a problem hiding this comment.
When state.agents.size === 0, this guard only lets exec-event wakes run if they already include sessionKey or agentId, but node exec completions can intentionally enqueue unscoped wakes for synthetic node-* session fallbacks (via scopedHeartbeatWakeOptions). In that case the wake still returns skipped/disabled, so background node exec completions are not relayed under heartbeat.every: "0m", which leaves the original blocker unresolved for that execution path.
Useful? React with 👍 / 👎.
| heartbeatFileContent: preflight.heartbeatFileContent, | ||
| }); | ||
|
|
||
| if (preflight.isExecEventReason && !hasExecCompletion) { |
There was a problem hiding this comment.
This early return skips the run whenever the wake reason is exec-event and no exec completion is present, even if cron-tagged events are queued in the same session. Because wake coalescing gives exec-event higher priority than cron:* for the same wake target, a stale/duplicate exec wake can mask a cron wake, and this branch then exits without consuming or re-waking cron work; with disabled interval scheduling, those cron events can remain stuck until another unrelated wake arrives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b351829323
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (preflight.isExecEventReason && !hasExecCompletion) { | ||
| emitHeartbeatEvent({ | ||
| status: "skipped", | ||
| reason: "no-exec-completion", | ||
| durationMs: Date.now() - startedAt, |
There was a problem hiding this comment.
This new guard skips all exec-event wakes that don't match isExecCompletionEvent, but src/gateway/server-node-events.ts emits reason: "exec-event" for exec.denied and exec.started too. Those wakes now return no-exec-completion before any reply run or event consumption, so denied node exec outcomes can be left unsurfaced (and queued) unless a later true completion in the same session happens to drain them. Previously these wakes still executed a heartbeat turn and could relay the failure context.
Useful? React with 👍 / 👎.
|
@codex review The reviewed concern assumes synthetic
So I would not fix P1 blindly without a reproduction showing a different node exec completion path bypasses this canonicalization. The nearby comment about synthetic P2 still looks valid: the new |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b351829323
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| heartbeatFileContent: preflight.heartbeatFileContent, | ||
| }); | ||
|
|
||
| if (preflight.isExecEventReason && !hasExecCompletion) { |
There was a problem hiding this comment.
This guard skips the run whenever the wake reason is exec-event and no exec completion is present, but it does not account for hasCronEvents. In the current wake queue, reasons are coalesced per target and exec-event has higher priority than cron:* (src/infra/heartbeat-wake.ts), so a same-target cron wake can be collapsed into an exec-event run that reaches this branch with hasCronEvents === true. With heartbeat.every: "0m", that early return means the queued cron events are never processed unless another wake happens later.
Useful? React with 👍 / 👎.
|
Closing this PR. After re-checking the design, this is the wrong layer to keep patching. Exec completion delivery is not a heartbeat. It is a system event dispatch problem. The current PR keeps adding exceptions inside heartbeat scheduling/runner code because exec completions are being routed through heartbeat wakes. Probably Continuing to patch heartbeat edge cases here doesn’t appear to address the root issue. Exec completion would be better handled as a separate system event path, reusing shared lower-level agent-turn/delivery helpers if needed, instead of depending on heartbeat interval semantics. |
1 participant
Problem:
Background exec completions could be lost when the heartbeat interval was disabled with
agents.defaults.heartbeat.every = "0m". The process completed successfully, but the user did not receive the completion output.Scenario:
A user asks OpenClaw to run a long command. The command runs longer than the exec foreground yield window, so the tool returns a background/running result and the assistant turn ends. The command still finishes before the hard exec timeout, so it exits successfully.
Normally, background exec completion is delivered through the heartbeat/event-wake path: exec enqueues an
exec-eventwake with the original chat delivery context, and the heartbeat runner starts a follow-up agent turn to relay the command output back to the chat. When the heartbeat interval was disabled, the runner treated the wake as disabled before dispatching it, so the follow-up turn never happened.Solution:
Allow
exec-eventwakes to bypass the normal per-agent heartbeat interval checks while preserving the global heartbeat enable switch. For targeted exec wakes, create a one-shot runner entry even when no interval-backed heartbeat agent is scheduled. For exec events without an explicit heartbeat target, default totarget: "last"so the captured delivery context can be used to send the completion output to the original chat.Added unit coverage for:
Verification:
pnpm test src/infra/heartbeat-runner.scheduler.test.ts -t "disabled heartbeat interval"pnpm test src/infra/heartbeat-runner.returns-default-unset.test.ts -t "disabled heartbeat interval"Relates to #62505