From f019f54bb33e5e07525fc019f06377de6adf35d5 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Tue, 30 Jun 2026 00:40:56 -0700 Subject: [PATCH] Fix active heartbeat run reaping (#8776) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Heartbeat monitoring is the subsystem that keeps agent execution visible and recovers work only when execution continuity is genuinely lost. > - Routes and scheduler paths can construct separate heartbeat service instances inside the same server process. > - Active adapter execution tracking was scoped to each service instance, so the periodic orphan reaper could miss a run that another service instance was actively executing. > - Remote or sandbox adapters are especially exposed because they may not persist a local process PID/group and may go quiet while the remote command is still alive. > - This pull request makes active in-process adapter execution tracking shared across heartbeat service instances and adds a regression for the cross-instance reaper case. > - The benefit is fewer false `process_lost` failures for long-running or quiet sandbox/remote agent runs. ## Linked Issues or Issue Description No public GitHub issue exists. This PR describes the bug inline using the bug report template fields. ### What happened? An actively executing heartbeat run could be finalized as `process_lost` by the orphan reaper when adapter execution was active through one `heartbeatService()` instance but the reaper ran through another instance in the same server process. ### Expected behavior The orphan reaper should skip runs that are still actively executing in-process, regardless of which `heartbeatService()` instance is doing the reaping. ### Steps to reproduce 1. Create two `heartbeatService()` instances in the same process. 2. Start an adapter run through the first instance. 3. Backdate the run row enough for orphan reaping to consider it stale. 4. Run orphan reaping through the second instance while the first instance is still awaiting adapter execution. 5. Observe that the old instance-local tracking can mark the live run as `process_lost`. ### Paperclip version or commit Reproduced against `master` before commit `44ba6d8bb4f7ae1ca3715697f750844d770d83a3`. ### Deployment mode Self-hosted/local server process with route and scheduler code paths constructing separate heartbeat service instances. Remote or sandbox adapters are the highest-risk case because they may not have local PID metadata and can be quiet while still running. ## What Changed - Moved active adapter execution tracking from the `heartbeatService()` closure to module-level process state shared by heartbeat service instances. - Added a regression test that starts a run through one heartbeat service instance and runs orphan reaping through another, proving the active run is not reaped and can finish normally. ## Verification - `pnpm exec vitest run server/src/__tests__/heartbeat-process-recovery.test.ts` passes: 61 tests. - `pnpm -r typecheck` passes. - `pnpm build` passes; existing UI build warnings remain for `::highlight(...)`, large chunks, and a mixed static/dynamic import. - `pnpm test:run` does not fully pass in this local environment: 1 unrelated existing failure in `server/src/__tests__/workspace-runtime.test.ts` for `auto-detects the default branch via symbolic-ref when origin/HEAD is set`. The fixture command fails with `git push -u origin main master` because the temp repo has no `master` ref. - Reran the isolated failing test with `pnpm exec vitest run server/src/__tests__/workspace-runtime.test.ts -t "auto-detects the default branch via symbolic-ref when origin/HEAD is set"`; it reproduces the same missing-`master` ref failure. ## Risks - Low risk for single-process Paperclip servers: this only broadens in-process active run tracking across service instances. - Multi-process deployments still need persisted or distributed execution liveness to coordinate reaping across processes; this PR does not claim to solve cross-process recovery. - A run could be skipped by the reaper while its adapter promise is active, but the existing `finally` path removes the active marker after execution settles. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used OpenAI GPT-5 via Codex, with tool use and local command execution. The runtime did not expose a separate context-window value. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [ ] I have run tests locally and they pass — targeted regression, typecheck, and build pass; full `pnpm test:run` has the unrelated missing-`master` ref fixture failure documented above - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes — no docs change needed for this internal bug fix - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip Agent --- .../heartbeat-process-recovery.test.ts | 66 +++++++++++++++++++ server/src/services/heartbeat.ts | 4 +- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/server/src/__tests__/heartbeat-process-recovery.test.ts b/server/src/__tests__/heartbeat-process-recovery.test.ts index df7ffae7d9..af0c89a8bd 100644 --- a/server/src/__tests__/heartbeat-process-recovery.test.ts +++ b/server/src/__tests__/heartbeat-process-recovery.test.ts @@ -575,6 +575,72 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { return { environmentId, leaseId }; } + it("does not reap active adapter executions started by another heartbeat service instance", async () => { + let releaseAdapter: (() => void) | null = null; + const adapterStarted = new Promise((resolve) => { + mockAdapterExecute.mockImplementationOnce(async () => { + resolve(); + await new Promise((release) => { + releaseAdapter = release; + }); + return { + exitCode: 0, + signal: null, + timedOut: false, + errorMessage: null, + summary: "Remote run completed.", + provider: "test", + model: "test-model", + }; + }); + }); + + const { runId, wakeupRequestId } = await seedRunFixture({ + adapterType: "openclaw_gateway", + agentStatus: "idle", + runStatus: "queued", + processPid: null, + processGroupId: null, + includeIssue: false, + }); + const executorHeartbeat = heartbeatService(db); + const reaperHeartbeat = heartbeatService(db); + + await executorHeartbeat.resumeQueuedRuns(); + await Promise.race([ + adapterStarted, + new Promise((_, reject) => { + setTimeout(() => reject(new Error("Timed out waiting for adapter execution to start")), 3_000); + }), + ]); + + await db + .update(heartbeatRuns) + .set({ + updatedAt: new Date("2026-03-19T00:00:00.000Z"), + }) + .where(eq(heartbeatRuns.id, runId)); + + const result = await reaperHeartbeat.reapOrphanedRuns({ staleThresholdMs: 1 }); + expect(result).toEqual({ reaped: 0, runIds: [] }); + + const activeRun = await reaperHeartbeat.getRun(runId); + expect(activeRun?.status).toBe("running"); + expect(activeRun?.errorCode).toBeNull(); + + const wakeup = await db + .select() + .from(agentWakeupRequests) + .where(eq(agentWakeupRequests.id, wakeupRequestId)) + .then((rows) => rows[0] ?? null); + expect(wakeup?.status).toBe("claimed"); + + if (!releaseAdapter) throw new Error("Adapter release handle was not captured"); + releaseAdapter(); + const settledRun = await waitForRunToSettle(executorHeartbeat, runId, 5_000); + expect(settledRun?.status).toBe("succeeded"); + }); + async function seedStrandedIssueFixture(input: { status: "todo" | "in_progress"; runStatus: "failed" | "timed_out" | "cancelled" | "succeeded"; diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index c530f09513..739b4dc47e 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -409,6 +409,9 @@ const SESSIONED_LOCAL_ADAPTERS = new Set([ "opencode_local", "pi_local", ]); +// Routes and the scheduler construct separate heartbeatService instances, but +// they must agree on in-process adapter executions when reaping stale runs. +const activeRunExecutions = new Set(); const INLINE_BASE64_IMAGE_DATA_RE = /("type":"image","source":\{"type":"base64","data":")([A-Za-z0-9+/=]{1024,})(")/g; type RuntimeConfigSecretResolver = Pick< @@ -3556,7 +3559,6 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) environmentRuntime, }); const workspaceOperationsSvc = workspaceOperationService(db); - const activeRunExecutions = new Set(); const liveRunExecutions = { has(id: string) { return runningProcesses.has(id) || activeRunExecutions.has(id);