fix(test): drain in-flight heartbeat runs before liveness teardown (#10040)
## Thinking Path > - Paperclip runs AI agent heartbeats to manage work; each heartbeat dispatches `executeRun` fire-and-forget, which is intentional for concurrency > - The server escalation test suite (`heartbeat-issue-liveness-escalation.test.ts`) exercises `reconcileIssueGraphLiveness`, which heals a resolved-dependency wake by enqueuing an on-demand heartbeat run > - `enqueueWakeup` → `startNextQueuedRunForAgent` dispatches the run fire-and-forget (`void executeRun(...)`), so the background run outlives the awaited reconcile call > - The test's `afterEach` polled `heartbeat_runs.status` to wait for idle, but that flips to `completed` while `executeRun`'s finally block is still flushing events — the escaping `heartbeat_run_events` insert could land between the events delete and the runs delete, tripping the FK constraint > - This PR fixes the race deterministically by tracking in-flight `executeRun` promises and exposing `heartbeatService.drainActiveRunExecutions()`, which the suite awaits before clearing tables > - The benefit is a permanently reliable escalation test suite with no sleeps, no retry bumps, and no production behavior change ## Linked Issues or Issue Description **What happened?** The `heartbeat-issue-liveness-escalation.test.ts` suite intermittently failed in CI with: ``` delete on table "heartbeat_runs" violates foreign key constraint "heartbeat_run_events_run_id_heartbeat_runs_id_fk" ``` **Expected behavior** `afterEach` cleanup should complete without FK violations. **Steps to reproduce** The race is timing-dependent but surfaces reliably when the teardown window is artificially widened. `reconcileIssueGraphLiveness()` heals resolved-dependency wakes by dispatching a heartbeat run fire-and-forget (`void executeRun(...)`). The old `afterEach` polled `heartbeat_runs.status` — but that flips to `completed` while `executeRun`'s finally block still has pending `heartbeat_run_events` row writes. The escaping insert can land between the events delete and the runs delete. **Paperclip version or commit** Reproducible on current `master` (commit `b57aa9950c707a024156c34b79326a82b2dcca31`) ## What Changed - **`server/src/services/heartbeat.ts`** — tracks all in-flight `executeRun` promises in a module-level `Set`; exposes `heartbeatService(db).drainActiveRunExecutions()`, which loops until the set drains (a completing run can enqueue the next queued run in its finally, so a single `await` is not enough) - **`server/src/server-suites/heartbeat-issue-liveness-escalation.test.ts`** — replaces the poll-on-`heartbeat_runs.status` teardown with `await heartbeatService(db).drainActiveRunExecutions()` before clearing tables; removes the now-unnecessary `waitForHeartbeatRunToComplete` helper ## Verification ```bash # Full file (22 tests) npx vitest run server/src/server-suites/heartbeat-issue-liveness-escalation.test.ts # 12x stress loop (264 test-runs, 0 failures) for i in $(seq 1 12); do npx vitest run server/src/server-suites/heartbeat-issue-liveness-escalation.test.ts || break done # Type check the changed files npx tsc --noEmit ``` - 22/22 tests green locally - 12/12 full-file loop iterations: 264 test-runs / 264 afterEach cycles, 0 failures - Widened-teardown stress variant (failed deterministically before the fix) now passes with the drain ## Risks Low risk. The drain mechanism is additive — it only affects test teardown and could also be wired into graceful shutdown. The fire-and-forget dispatch in production is unchanged. The `Set`-based tracking adds negligible overhead per run dispatch (insert on dispatch, delete on completion). ## Model Used - **Provider:** Anthropic - **Model:** Claude Sonnet 4.6 (`claude-sonnet-4-6`) - **Context window:** 200K tokens - **Mode:** Tool use, code execution, extended reasoning ## 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 - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes - [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: Harold Kim <harold@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
parent
b57aa9950c
commit
8af70b9fae
|
|
@ -93,21 +93,15 @@ describeEmbeddedPostgres("heartbeat issue graph liveness escalation", () => {
|
|||
afterEach(async () => {
|
||||
vi.clearAllMocks();
|
||||
runningProcesses.clear();
|
||||
let idlePolls = 0;
|
||||
for (let attempt = 0; attempt < 100; attempt += 1) {
|
||||
const runs = await db
|
||||
.select({ status: heartbeatRuns.status })
|
||||
.from(heartbeatRuns);
|
||||
const hasActiveRun = runs.some((run) => run.status === "queued" || run.status === "running");
|
||||
if (!hasActiveRun) {
|
||||
idlePolls += 1;
|
||||
if (idlePolls >= 3) break;
|
||||
} else {
|
||||
idlePolls = 0;
|
||||
}
|
||||
await new Promise((resolve) => setTimeout(resolve, 50));
|
||||
}
|
||||
await new Promise((resolve) => setTimeout(resolve, 50));
|
||||
// reconcileIssueGraphLiveness heals dependency wakes by enqueuing an
|
||||
// on-demand wake, which dispatches a heartbeat run fire-and-forget (see
|
||||
// startNextQueuedRunForAgent → executeRun in the heartbeat service). That
|
||||
// background run keeps writing rows (workspace_operations, heartbeat_run_events)
|
||||
// after the awaited call resolves. Deterministically await those in-flight
|
||||
// executions before clearing tables — otherwise an escaping heartbeat_run_events
|
||||
// insert can land between the events delete and the heartbeat_runs delete and
|
||||
// trip the run_events → runs foreign key.
|
||||
await heartbeatService(db).drainActiveRunExecutions();
|
||||
await db.delete(activityLog);
|
||||
await db.delete(heartbeatRunEvents);
|
||||
await db.delete(costEvents);
|
||||
|
|
|
|||
|
|
@ -554,6 +554,13 @@ const SESSIONED_LOCAL_ADAPTERS = new Set([
|
|||
// 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<string>();
|
||||
// Background heartbeat executions are dispatched fire-and-forget (see
|
||||
// startNextQueuedRunForAgent), so the promise that resolves once a run's DB
|
||||
// writes are fully flushed is otherwise unobservable. Track those promises here
|
||||
// — shared across service instances like activeRunExecutions above — so callers
|
||||
// that must guarantee no run write is still in flight (graceful shutdown, and
|
||||
// tests tearing down a shared database) can await drainActiveRunExecutions().
|
||||
const activeRunExecutionPromises = new Set<Promise<void>>();
|
||||
const INLINE_BASE64_IMAGE_DATA_RE = /("type":"image","source":\{"type":"base64","data":")([A-Za-z0-9+/=]{1024,})(")/g;
|
||||
|
||||
type RuntimeConfigSecretResolver = Pick<
|
||||
|
|
@ -11748,14 +11755,34 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {})
|
|||
if (claimedRuns.length === 0) return [];
|
||||
|
||||
for (const claimedRun of claimedRuns) {
|
||||
void executeRun(claimedRun.id).catch((err) => {
|
||||
const execution = executeRun(claimedRun.id).catch((err) => {
|
||||
logger.error({ err, runId: claimedRun.id }, "queued heartbeat execution failed");
|
||||
});
|
||||
// Register the in-flight execution so drainActiveRunExecutions() can await
|
||||
// it. executeRun resolves only after its finally block finishes flushing
|
||||
// run rows/events, so awaiting this promise guarantees the run's writes
|
||||
// have landed before a caller (e.g. a test's afterEach) mutates the DB.
|
||||
activeRunExecutionPromises.add(execution);
|
||||
void execution.finally(() => {
|
||||
activeRunExecutionPromises.delete(execution);
|
||||
});
|
||||
}
|
||||
return claimedRuns;
|
||||
});
|
||||
}
|
||||
|
||||
// Await every background heartbeat execution that is currently in flight. A
|
||||
// draining run can, in its finally block, promote and dispatch the next queued
|
||||
// run for the same agent — that follow-up execution is registered in the set
|
||||
// before the parent promise settles, so we loop until the set is empty rather
|
||||
// than snapshotting once. Callers use this to guarantee no run is still
|
||||
// writing rows/events (graceful shutdown, deterministic test teardown).
|
||||
async function drainActiveRunExecutions() {
|
||||
while (activeRunExecutionPromises.size > 0) {
|
||||
await Promise.all([...activeRunExecutionPromises]);
|
||||
}
|
||||
}
|
||||
|
||||
async function executeRun(runId: string) {
|
||||
if ((await getSchedulingSuppression()).suppressed) return;
|
||||
|
||||
|
|
@ -16912,6 +16939,7 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {})
|
|||
// gate on suppression should prefer this over the env-only resolver.
|
||||
resolveSchedulingSuppression: getSchedulingSuppression,
|
||||
drainRunningRunsForShutdown,
|
||||
drainActiveRunExecutions,
|
||||
|
||||
promoteDueScheduledRetries,
|
||||
retryScheduledRetryNow,
|
||||
|
|
|
|||
Loading…
Reference in New Issue