🤖 fix: stop heartbeats and background wakes from pausing goals - #3955
🤖 fix: stop heartbeats and background wakes from pausing goals#3955ThomasK33 wants to merge 45 commits into
Conversation
Three mechanisms made scheduled heartbeats appear to pause active goals (diagnosed from live session data): 1. Queue race: a message typed while the goal-creating turn was still streaming dispatched right after the queued set_goal applied and auto-paused the brand-new goal. MessageQueue now stamps entries with enqueuedAtMs, and goal safety skips the pause when the goal was created after the message was typed. 2. Fragile kickoff window: chat-tail reconciliation paused active goals whose in-memory kickoff candidate was lost (restart, eviction) before the first continuation fired — the next getGoal (heartbeat/wake tool assembly runs one every turn) silently flipped them to paused. Goals with lastContinuationFiredAtMs == null are now exempt from the active→paused manual_user reconciliation, making the kickoff window durable and self-healing. 3. Accounting noise: recordStreamAccounting charged paused/complete goals for maintenance streams (heartbeats, background wake turns), bumping updatedAtMs so every heartbeat looked like it had just touched the paused goal. Only goal-driven origins (goal_continuation / goal_budget_limit) now charge non-active goals, mirroring attributeChildReport.
|
@codex review |
![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: e67e217ff9
ℹ️ 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".
This comment has been minimized.
This comment has been minimized.
…econciliation, budget_limited accounting) - P1: preserve the projected goal's createdAtMs through the mid-stream drain so interventions queued against the visible optimistic goal are not misread as pre-goal input. - P2: persist enqueuedAtMs on queue-dispatched user rows and scope the never-driven kickoff exemption to rows authored before the goal existed; post-goal rows pause on reconciliation even when the dispatch-time auto-pause was lost to a crash. - P2 (accounting): extend the maintenance-stream skip to budget_limited goals so background wakes cannot inflate the recorded overshoot.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02ab4595ee
ℹ️ 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".
…e maintenance cost previews - Rejected manual sends (pricing gate) now persist timestamp + enqueuedAtMs so chat-tail reconciliation can classify the row as pre-goal after a restart instead of pausing a never-driven goal. - previewStreamAccounting receives the stream origin kind and mirrors final accounting's budget_limited maintenance skip, so the Goal UI no longer shows climbing heartbeat/wake cost that snaps back at stream end.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bd9a185f4
ℹ️ 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".
Codex P2: createGoal() stamped createdAtMs before the kickoff-model validation and streaming re-check awaits, so a message queued during those awaits postdated the stamp yet predated goal visibility, and the pre-goal guard misread it as an intervention. Re-stamp fresh projected goals immediately before publishPendingGoalSnapshot.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e712711382
ℹ️ 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".
This comment has been minimized.
This comment has been minimized.
…off candidate on ack failure Codex P2 x2: (1) the round-3 stamp still preceded the async activity-snapshot read inside publishPendingGoalSnapshot, so a message queued during that read postdated the stamp while the renderer had not yet received onActivityChange — publish first, then stamp fresh goals. (2) reordering acknowledgeUser before the candidate clear meant an acknowledgment throw skipped the clear entirely, letting a stale kickoff candidate dispatch a continuation despite the user's persisted intervention — clear conservatively on failure, then rethrow.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 930f18846d
ℹ️ 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".
…ure authoring time at request entry Codex P1: round 4 moved pendingGoalMutations.set after the publication await, so a user abort landing during publication deleted nothing and the setter then installed the mutation anyway — resurrecting a goal the abort discarded and silently applying it at the next stream end. Install before publish; guard the post-publication re-stamp on mutation identity so an interleaved abort (or competing setter) is never overwritten. Codex P2: lastAddedAtMs was sampled at enqueue, after WorkspaceService preflight awaits (pricing gate, settings persistence) — a goal becoming visible during those awaits postdated the user's authoring. Capture authoredAtMs at request entry and thread it through queueMessage / MessageQueue, the pricing-rejection delegation, and direct sends.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1340998494
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 1340998494
Only the user who started this review can view the report in Codex.
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
…ublication setter Codex P2: a stream ending during the setter's publication await let applyPendingAfterStreamEnd take the mutation synchronously (outside the goal file lock) and persist the pre-publication construction stamp — a message authored during the publication await was then misclassified as a post-goal intervention. The drain now flushes the lock and re-reads the mutation, draining the finalized publication stamp (or honoring a discard that landed in the same window).
…hed sends Codex security P2: overlapping sends can complete preflight out of authoring order, letting an older pre-goal message overwrite a later post-goal stop/correction's authoring time — the batch then satisfied the pre-goal guard and kept the goal running despite the intervention. Fold each add's authoring time in via max(); seed entry creation with 0 so an authoredAtMs captured before slow preflight is never swallowed by the creation wall clock.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9b94d898d
ℹ️ 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".
…al-safety comparisons Codex P2: chat.jsonl rows are unchecked JSON, so a malformed enqueuedAtMs (negative/NaN/string) could beat a valid row timestamp in the nullish selection and misclassify a genuine post-goal intervention as pre-goal, leaving a never-driven goal running after restart. Only finite non-negative numbers participate; invalid values fall back to the validated row timestamp.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04e46a95a8
ℹ️ 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".
|
@codex review |
|
@codex security review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27927435b9
ℹ️ 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".
Reject malformed goal attribution at the unchecked compaction-summary boundary, clear the corrupt pending follow-up, and forward only validated values into goal recovery. --- _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh` • Cost: `$502.42`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh costs=502.42 -->
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
xum/src/node/services/agentSession.ts
Lines 1718 to 1722 in f308112
Scope auto-pause to the acknowledged goal
When acknowledgeUser() returns active goal A but a replacement active goal B persists before this call acquires the goal lock, the unguarded status mutation re-reads and pauses B even though the manual row predates B. The earlier pre-goal classification used A's creation time, and the unscoped candidate clear can also remove B's kickoff, leaving the replacement stranded. Fresh evidence beyond the resolved goal-scoped wrap-up-suppression thread is that this durable auto-pause path still carries no goal identity; pass A's expectedGoalId and clear only A's candidate, or re-read and classify the replacement before applying either side effect.
AGENTS.md reference: AGENTS.md:L150-L150
ℹ️ 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".
…idate retry goal IDs Carry the queue/busy idle rule through the redispatched follow-up's send-admission gates, treat stale-admission refusals as skips, and discard startup-retry goal attribution when the persisted goal ID is present but malformed. --- _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh` • Cost: `$502.42`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh costs=502.42 -->
|
@codex review |
|
@codex security review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f4cad147d
ℹ️ 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".
…ation; recovered wrap-up reservation Expose WorkspaceService send preflights to redispatched follow-up idle probes, validate goal-scoping IDs against the durable UUID contract, and install the missing wrap-up reservation when a crash-recovered budget follow-up dispatches. --- _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh` • Cost: `$502.42`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh costs=502.42 -->
|
@codex review |
|
@codex security review |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c205e21ba1
ℹ️ 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".
…ollback on preflight contention
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
xum/src/node/services/agentSession.ts
Line 1732 in af84428
Scope the automatic pause to the acknowledged goal
When a manual message acknowledges goal A while a concurrent replacement B is queued behind that acknowledgment's goal-file lock, B can become durable before this setGoal() obtains the lock. Because the pause carries no expectedGoalId, it then resolves the current objective and pauses B, even though the message predates B; the earlier workspace-wide candidate deletion can also remove B's kickoff. Fresh evidence beyond the resolved goal-scoped wrap-up suppression is that the subsequent automatic pause remains unscoped; pass A's goal ID as the expected identity and treat a replacement conflict as a no-op.
AGENTS.md reference: AGENTS.md:L150-L150
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: af844281ac
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| // silently pausing it half a second after creation (user report: goals | ||
| // "paused by heartbeats" were actually killed here, then heartbeat turns | ||
| // kept the workspace moving while the goal sat paused). | ||
| if (input.enqueuedAtMs != null && goal != null && goal.createdAtMs >= input.enqueuedAtMs) { |
There was a problem hiding this comment.
When a user queues a stop/correction while the current model turn is still streaming and that model publishes a new goal afterward, this timestamp-only branch treats the message as non-intervention, restores the kickoff candidate, and returns without pausing. The durable tail code makes the same pre-goal exemption, so the next idle pass can dispatch an exec continuation even if the model ignores the corrective turn. This lets a prompt-injected/untrusted model resume file/shell work after the documented “any manual turn pauses” boundary. Only bypass this pause for an explicit user opt-in, not merely enqueuedAtMs <= createdAtMs.
Useful? React with 👍 / 👎.
|
@codex review |
|
@codex security review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b37d4b6ed1
ℹ️ 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".
| const persisted = await this.persistGoalMutationLocked(input, options); | ||
| // Arm under the same lock tenure as the paused write so no other locked | ||
| // writer can observe the paused record before the hold exists. | ||
| if (this.pauseFinalizationHoldApplies(input, persisted) && persisted.success) { | ||
| this.armPauseFinalizationHold(input.workspaceId, persisted.data.goalId); |
There was a problem hiding this comment.
When a captured continuation is in send preflight and Pause commits goal.json, persistGoalMutationLocked() still awaits snapshot/preview publication before returning here and bumping explicitPauseGenerations; during that window the candidate, pause generation, terminal generation, and identity generation all remain unchanged, so admissionStale can admit an autonomous exec turn after the durable Pause. Fresh evidence beyond the resolved Pause-admission thread is this publication gap before armPauseFinalizationHold() is reached; arm the hold/generation immediately at the paused write commit.
AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.
| }); | ||
| } | ||
|
|
||
| sessionInvisiblePreflight.release(); |
There was a problem hiding this comment.
When startup follow-up recovery races a user/API Resume, this releases the only hasExternalSendPreflight signal before AgentSession.resumeStream() runs its second asynchronous pricing check. The session is still idle during that await, so dispatchPendingFollowUp() can admit the recovered synthetic turn; after pricing resolves, resumeStream() does not recheck isBusy() and can start the resumed turn concurrently. Fresh evidence beyond the resolved send-preflight threads is this separate resume handoff; retain the reservation until session admission settles or recheck busy state after the inner preflight.
AGENTS.md reference: AGENTS.md:L150-L150
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: b37d4b6ed1
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| // Handoff: from here the send is the session's own admission problem — | ||
| // release the probe reservation so a follow-up redispatched from within | ||
| // this very turn (on-send compaction completion) does not veto itself. | ||
| sessionInvisiblePreflight.release(); |
There was a problem hiding this comment.
Requires a goal-scoped compaction follow-up being recovered while an accepted manual send enters AgentSession. This line releases the only follow-up-visible reservation before AgentSession claims PREPARING; fresh evidence beyond the prior preflight threads is that AgentSession immediately yields at await cancelBeforeAcceptance() and does not claim PREPARING until line 3892. Recovery can therefore see no preflight, queue, or busy state and admit the exec follow-up ahead of the manual pause, allowing normal shell/file tools to run. Retain visibility until the session is synchronously busy, while excluding only the originating turn's own follow-up.
Useful? React with 👍 / 👎.
1 participant
Summary
Scheduled heartbeats (and background wake turns) could leave active goals paused — or make already-paused goals look like they were still being touched. This PR fixes the three mechanisms behind the symptom: a queue race that auto-paused a freshly created goal, a fragile in-memory kickoff window that let any maintenance turn's
getGoal()flip a never-driven goal to paused, and stream accounting that charged paused/complete goals for maintenance streams.Background
Diagnosed from live session data (user report: "heartbeats are pausing goals"). In one session, a model-created goal was auto-paused ~500ms after creation by a user message that had been queued while the goal-creating turn was still streaming; heartbeat turns then ran for hours charging the paused goal ($7.11 / 8 turns,
updatedAtMsbumped every turn) without ever driving it. In another, an active goal whose kickoff continuation was blocked byactive_descendant_taskssurvived only via the in-memory kickoff candidate — any candidate loss (restart, eviction) let the nextgetGoal()(tool assembly runs one on every heartbeat/wake turn) silently pause it via chat-tail reconciliation.Implementation
Queue-race guard —
MessageQueuestamps each entry withlastAddedAtMsanddequeueNext()exposes it asenqueuedAtMs.applyManualUserMessageGoalSafetyskips the auto-pause (and candidate clear) whengoal.createdAtMs >= enqueuedAtMs: the user cannot have been intervening against a goal that did not exist when they typed. Messages typed after the goal exists still pause it; explicit Pause is unaffected.Durable kickoff window —
applyChatTailGoalModeno longer reconciles an active goal to paused off a manual-user chat tail whenlastContinuationFiredAtMs == null. A never-driven goal has nogoal_continuationrow yet by construction, so its tail always ends at a pre-goal manual row; pausing on that is a false positive. This makes the kickoff window durable across restarts and candidate eviction, and self-healing: the next stream end re-arms the continuation instead of stranding the goal.Maintenance-stream accounting skip —
recordStreamAccountingnow skips paused/complete goals for all origins exceptgoal_continuation/goal_budget_limit(previously onlyuserwas skipped). Heartbeats resolve touserorigin and wake turns toother; neither should charge turns/cost or bumpupdatedAtMson a goal that is not running. MirrorsattributeChildReport's existing paused/complete skip.Validation
"other"-origin accounting.src/node/servicessweep: the only failures (BackupRepoCache, TaskService recovery, agent_skill_delete) reproduce identically on a clean-HEAD probe worktree — pre-existing host issues, not regressions.Risks
Goal lifecycle logic; moderate regression surface:
enqueuedAtMsonly being present on queue-dispatched sends; direct (idle) sends never carry it and keep today's behavior.AgentSession, so this is bounded to cost accounting, not control flow.Generated with
xum• Model:anthropic:claude-fable-5• Thinking:xhigh• Cost:$49.89