fix(heartbeat): guard finalizeAgentStatus against clobbering a concurrent pause/terminate (RBR-932)

[PraeSynBH]

Thinking Path

• Paperclip orchestrates AI agents that run as persistent server processes with heartbeat-driven lifecycle management
• finalizeAgentStatus in server/src/services/heartbeat.ts is the write path that sets an agent's status when a run finishes (succeeded/failed/cancelled/etc.)
• It reads the agent, checks paused/terminated in JS against that snapshot, awaits countRunningRunsForAgent, then writes status back with an unguarded where(eq(agents.id, agentId)) — an operator pause/terminate landing in that window is silently clobbered
• recovery/service.ts's finalizeAgentAfterSourceResolvedRun already solves the identical problem correctly by putting the guard in the WHERE clause instead of relying on a stale JS check
• This PR ports that same compare-and-set predicate into finalizeAgentStatus, closing the TOCTOU window so an operator's pause/terminate always wins
• The benefit is that operator intent (pause/terminate) can no longer be silently reverted by an in-flight run finishing concurrently

Linked Issues or Issue Description

No public GitHub issue exists for this internally-tracked defect. Following the Bug report template:

• What happened: finalizeAgentStatus reads an agent's status, performs an early-out JS check for paused/terminated, then several awaits later writes the agent's status with a WHERE clause scoped only to agents.id. If an operator pauses or terminates the agent in that window, the write silently overwrites their action back to idle/running/error.
• Expected: an operator pause/terminate should never be clobbered by a run-completion write that started before the operator's action.
• Where: server/src/services/heartbeat.ts, finalizeAgentStatus.
• Prior art: open PR fix(heartbeat): make agent pause status guard atomic with status update #4503 fixes the identical defect with the identical predicate, but has been open since 2026-04-29, is now CONFLICTING/stale against current master, and is from an external contributor fork. This PR is a fresh, mergeable, currently-passing-tests implementation of the same fix rebased on current master, with a regression test that drives the real production call path. Recommend closing fix(heartbeat): make agent pause status guard atomic with status update #4503 as superseded once this lands (or vice versa, whichever reviewers prefer — the code intent is identical).

What Changed

• server/src/services/heartbeat.ts: finalizeAgentStatus's UPDATE now carries notInArray(agents.status, ["paused", "terminated"]) in its WHERE clause alongside eq(agents.id, agentId), making the guard atomic with the write. The existing JS-level early-out check is kept as a cheap early exit and commented as non-authoritative.
• server/src/__tests__/heartbeat-finalize-agent-status-pause-race.test.ts (new): 4 embedded-Postgres regression tests that drive the real production path — heartbeatService(db).cancelRun(runId) → finalizeAgentStatus — with an injected race (a db.update proxy that commits the operator's pause/terminate write between the service's snapshot read and its own finalize write). Asserts final persisted agents.status, not call shapes:
  • pause survives a concurrently-racing run finalization
  • terminate survives a concurrently-racing run finalization
  • the normal no-race case still finalizes to idle (control)
  • an already-paused agent stays paused via the cheap JS early-out

Verification

cd server && npx vitest run src/__tests__/heartbeat-finalize-agent-status-pause-race.test.ts --no-coverage

✓ src/__tests__/heartbeat-finalize-agent-status-pause-race.test.ts (4 tests) 9447ms
  ✓ keeps the agent paused when an operator pause lands between the snapshot read and the finalize write  427ms
  ✓ keeps the agent terminated when a terminate lands in the same window
  ✓ still finalizes to idle when no pause races the write (control)
  ✓ leaves an already-paused agent paused (cheap JS early-out still holds)

Test Files  1 passed (1)
     Tests  4 passed (4)

npx tsc --noEmit -p . shows zero new errors from this change (pre-existing, unrelated @paperclipai/plugin-sdk module-resolution errors in other files are untouched — confirmed zero hits for heartbeat.ts or the new test file in the tsc output).

Risks

Low. This makes an existing early-return guard atomic with its own write; it does not change behavior for any caller that wasn't already relying on the (incorrect) racy overwrite. recovery/service.ts already uses the identical predicate for the analogous write, so this brings finalizeAgentStatus in line with established precedent in the same codebase rather than introducing a new pattern.

Model Used

• Provider: Anthropic
• Model: Claude Sonnet (claude-sonnet-5), agentic tool use via Claude Code
• Reasoning: standard effort, no extended thinking
• Tool use: file read/edit, shell execution, git, gh CLI

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above (fix(heartbeat): make agent pause status guard atomic with status update #4503, noted as stale/conflicting prior art)
• I have either (a) linked existing issues OR (b) described the issue in-PR following the relevant issue template
• I have not referenced internal/instance-local Paperclip issues or links (only public GitHub references above)
• My branch name describes the change and contains no internal ticket id
• I have run tests locally and they pass
• I have added or updated tests where applicable
• I have updated relevant documentation to reflect my changes (N/A — internal service-layer fix, no user-facing docs)
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups (re-review requested after strengthening the regression test to exercise the real call path)
• I will address all Greptile and reviewer comments before requesting merge

[commitperclip]

✅ All checks passing — ready for Greptile review and maintainer approval.

— commitperclip

[greptile-apps]

Greptile Summary

The PR makes the agent-status finalization write atomic with concurrent pause and termination operations.

• Adds a guarded status-update predicate so operator pause or termination takes precedence.
• Replaces the predicate-copying test with production-path race tests through heartbeatService.cancelRun.
• Covers pause, termination, normal finalization, and an already-paused agent.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
server/src/services/heartbeat.ts	Adds the paused/terminated compare-and-set predicate to the production finalization update without changing normal finalization behavior.
server/src/tests/heartbeat-finalize-agent-status-pause-race.test.ts	Exercises the production finalization path and deterministically injects pause or termination immediately before the guarded update executes.

_{Reviews (2): Last reviewed commit: "fix(heartbeat): guard finalizeAgentStatu..." | Re-trigger Greptile}

[greptile-apps]

PR description omits required sections

The PR description includes the problem, fix, and testing details, but it omits the required top-down Thinking Path and explicit Risks section. Please update it to follow the template required by CONTRIBUTING.md so reviewers have the project-level rationale, benefits, and risk assessment.

Context Used: CONTRIBUTING.md has a guide for a good PR message ... (source)

Prompt To Fix With AI

This is a comment left during a code review.
Path: server/src/__tests__/heartbeat-agent-status-cas.test.ts
Line: 19

Comment:
**PR description omits required sections**

The PR description includes the problem, fix, and testing details, but it omits the required top-down Thinking Path and explicit Risks section. Please update it to follow the template required by `CONTRIBUTING.md` so reviewers have the project-level rationale, benefits, and risk assessment.

**Context Used:** CONTRIBUTING.md has a guide for a good PR message ... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!