From 6a5b293240b7e8c327fde7ca8978de1ac35dc35e Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Wed, 12 Aug 2026 10:42:58 -0700 Subject: [PATCH] test(server): fix onboarding first-task teardown foreign-key race (#11284) 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. > - The server coordinates issue work and heartbeat runs. > - The onboarding first-task route sends an assignment wake in the background. > - The route test removes related database rows during teardown. > - A late heartbeat run can keep foreign-key child rows alive during teardown. > - This pull request drains the wake and deletes run rows in foreign-key order. > - The benefit is a stable test that keeps the onboarding behavior unchanged. ## Linked Issues or Issue Description **What happened?** The onboarding first-task route sent a background assignment wake. The test teardown removed parent rows before the wake-created heartbeat rows finished. **Expected behavior** The test teardown should wait for the background wake and remove heartbeat rows before it removes their parent rows. **Steps to reproduce** 1. Run the onboarding first-task route test. 2. Repeat the test many times. 3. Observe an intermittent foreign-key error during teardown. **Paperclip version or commit** Commit `c30fe965920eeb7e7fb88e17574a65bed8fc01a4`. **Deployment mode** Local dev (pnpm dev). **Installation method** Built from source (pnpm dev / pnpm build). **Agent adapter(s) involved** Not adapter-specific (core bug). **Database mode** Embedded PGlite (default — DATABASE_URL unset). ## What Changed - Stub the server adapter in the route test so the dispatched run finishes at once. - Drain heartbeat runs to quiescence before teardown. - Delete heartbeat runs and child rows before their parent rows. - Delete runtime state and company skill rows in foreign-key order. - Keep the route behavior and all three test assertions unchanged. ## Verification - Run `pnpm exec vitest run src/__tests__/issue-onboarding-first-task-routes.test.ts` from the `server` package. - The author ran the suite 25 times with 25 passes. - The suite reproduced the teardown foreign-key error before this change. ## Risks Low risk. This change affects one test file and does not change product code or route behavior. ## Model Used OpenAI GPT-5. The model used tool calls and code review assistance. The exact context window and reasoning mode were not exposed in this run. ## 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 - [x] I have updated relevant documentation to reflect my changes - [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 --- ...issue-onboarding-first-task-routes.test.ts | 54 ++++++++++++++++++- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/issue-onboarding-first-task-routes.test.ts b/server/src/__tests__/issue-onboarding-first-task-routes.test.ts index 01b3078ceb..094ad03694 100644 --- a/server/src/__tests__/issue-onboarding-first-task-routes.test.ts +++ b/server/src/__tests__/issue-onboarding-first-task-routes.test.ts @@ -2,13 +2,17 @@ import { randomUUID } from "node:crypto"; import express from "express"; import request from "supertest"; import { and, eq } from "drizzle-orm"; -import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { activityLog, + agentRuntimeState, agents, agentWakeupRequests, companies, + companySkills, createDb, + heartbeatRunEvents, + heartbeatRuns, issueComments, issues, } from "@paperclipai/db"; @@ -19,7 +23,38 @@ import { } from "./helpers/embedded-postgres.js"; import { actorMiddleware } from "../middleware/auth.js"; import { errorHandler } from "../middleware/index.js"; +import { runningProcesses } from "../adapters/index.ts"; import { issueRoutes } from "../routes/issues.js"; +import { heartbeatService } from "../services/heartbeat.js"; +import { drainHeartbeatRunsToQuiescence } from "./helpers/drain-heartbeat-runs.js"; + +// The issue-create route dispatches an assignment wake fire-and-forget for an +// assigned agent. The wake dispatches a heartbeat run that calls the server +// adapter. Stub the adapter so the run finishes at once and does not start a +// real agent session. The stub keeps the wake path realistic and fast, so the +// afterEach drain reaches quiescence without a long real run. +const mockAdapterExecute = vi.hoisted(() => + vi.fn(async () => ({ + exitCode: 0, + signal: null, + timedOut: false, + errorMessage: null, + summary: "Onboarding first-task route test run.", + provider: "test", + model: "test-model", + })), +); + +vi.mock("../adapters/index.ts", async () => { + const actual = await vi.importActual("../adapters/index.ts"); + return { + ...actual, + getServerAdapter: vi.fn(() => ({ + supportsLocalAgentJwt: false, + execute: mockAdapterExecute, + })), + }; +}); const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; @@ -42,11 +77,26 @@ describeEmbeddedPostgres("issue create onboarding first-task routes", () => { }, 20_000); afterEach(async () => { + // Wait for the background assignment wake to reach quiescence before the + // deletes. The wake inserts agent_wakeup_requests and heartbeat_runs rows + // asynchronously, and a late row races the teardown deletes. The module + // global promise sets in heartbeat.ts are shared, so a fresh heartbeatService + // drains the route wake that this suite started. + mockAdapterExecute.mockClear(); + runningProcesses.clear(); + await drainHeartbeatRunsToQuiescence(db, heartbeatService(db)); + // Delete heartbeat_runs and its child rows before agents and + // agent_wakeup_requests. heartbeat_runs references both, so a completed run + // row blocks a parent delete with a foreign-key violation. await db.delete(activityLog); - await db.delete(agentWakeupRequests); await db.delete(issueComments); + await db.delete(heartbeatRunEvents); + await db.delete(heartbeatRuns); + await db.delete(agentWakeupRequests); + await db.delete(agentRuntimeState); await db.delete(issues); await db.delete(agents); + await db.delete(companySkills); await db.delete(companies); });