From f1931d0e14d1e7888e921bc1596cd8a014cb5be6 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Wed, 12 Aug 2026 11:29:51 -0700 Subject: [PATCH] test(server): fix flaky workspace-busy retry-row read race (#11293) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The server heartbeat system records each agent run and its retry state > - A workspace-busy deferral cancels one run before it inserts the scheduled retry row > - The test helper can return after the cancel write and before the retry-row insert > - A direct read can then return no row and fail a valid retry assertion > - This pull request makes presence reads wait for the retry row > - The benefit is stable test coverage without a production behavior change ## Linked Issues or Issue Description Refs: #10806 **What happened?** The workspace-busy test read the retry row after the first deferral write. The helper returned before the scheduled-retry insert completed. The read then returned no row and failed the retry assertions. **Expected behavior** The test must wait until the scheduled-retry row exists before it checks retry-row fields. The production write order must stay unchanged. **Steps to reproduce** 1. Add a 300 ms delay between the deferral writes. 2. Run `server/src/__tests__/heartbeat-workspace-busy.test.ts`. 3. Observe failures at retry-row presence checks. 4. Add the bounded polling helper. 5. Run the test file again and observe that all presence checks pass. **Paperclip version or commit** Commit `d9b6e8a6e62b9b56919fc9c52d294e8ac569f70f`. **Deployment mode** Local test run from source. ## What Changed - Add `waitForRetryRun`, which polls for the retry row with a 10 second timeout and a 50 millisecond interval. - Use the helper at every test site that reads a retry row after deferral. - Keep direct reads at absence assertions. - Keep production code unchanged. ## Verification - Injected a temporary 300 millisecond delay between the two production writes and reproduced the five presence-site failures. - Applied the helper with the delay and passed the test file 15 out of 15 times. - Removed the temporary production delay. - Ran the changed test file 25 consecutive times with 0 failures. - Ran TypeScript checks for the changed test file with no errors. ## Risks Low risk. This pull request changes test code only. The helper has a bounded timeout. Production behavior and retry-row assertions remain unchanged. ## Model Used OpenAI Codex, GPT-5, reasoning mode, tool use, and code execution. The runtime does not expose the context window size. ## 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] 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 --- .../heartbeat-workspace-busy.test.ts | 53 ++++++++++--------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/server/src/__tests__/heartbeat-workspace-busy.test.ts b/server/src/__tests__/heartbeat-workspace-busy.test.ts index 15ab976c55..1b6ec47da7 100644 --- a/server/src/__tests__/heartbeat-workspace-busy.test.ts +++ b/server/src/__tests__/heartbeat-workspace-busy.test.ts @@ -176,6 +176,29 @@ describeEmbeddedPostgres("shared-workspace run serialization", () => { return await heartbeat.getRun(runId); } + // A deferral does two writes in order: first it cancels the original run, + // then it inserts the scheduled-retry row. waitForRunToLeaveActiveStates + // returns after the first write, so a read of the retry row can land before + // the second write and find nothing. Poll until the retry row exists so the + // retry-row assertions never observe the gap between the two writes. + async function waitForRetryRun(originalRunId: string, timeoutMs = 10_000) { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + const retryRun = await db + .select() + .from(heartbeatRuns) + .where(eq(heartbeatRuns.retryOfRunId, originalRunId)) + .then((rows) => rows[0] ?? null); + if (retryRun) return retryRun; + await new Promise((resolve) => setTimeout(resolve, 50)); + } + return await db + .select() + .from(heartbeatRuns) + .where(eq(heartbeatRuns.retryOfRunId, originalRunId)) + .then((rows) => rows[0] ?? null); + } + interface WorkspaceFixture { companyId: string; projectId: string; @@ -491,11 +514,7 @@ describeEmbeddedPostgres("shared-workspace run serialization", () => { // The deferred run's adapter never executed — the whole point of the gate. expect(executedRunIds).not.toContain(run!.id); - const retryRun = await db - .select() - .from(heartbeatRuns) - .where(eq(heartbeatRuns.retryOfRunId, run!.id)) - .then((rows) => rows[0] ?? null); + const retryRun = await waitForRetryRun(run!.id); expect(retryRun).toMatchObject({ status: "scheduled_retry", scheduledRetryAttempt: 1, @@ -560,11 +579,7 @@ describeEmbeddedPostgres("shared-workspace run serialization", () => { const deferred = await waitForRunToLeaveActiveStates(run!.id); expect(deferred?.status).toBe("cancelled"); - const retryRun = await db - .select() - .from(heartbeatRuns) - .where(eq(heartbeatRuns.retryOfRunId, run!.id)) - .then((rows) => rows[0] ?? null); + const retryRun = await waitForRetryRun(run!.id); expect(retryRun?.status).toBe("scheduled_retry"); // Holder finishes; the due retry promotes, queues, and executes. @@ -606,11 +621,7 @@ describeEmbeddedPostgres("shared-workspace run serialization", () => { expect(deferred?.errorCode).toBe(WORKSPACE_BUSY_ERROR_CODE); expect(executedRunIds).not.toContain(run!.id); - const retryRun = await db - .select() - .from(heartbeatRuns) - .where(eq(heartbeatRuns.retryOfRunId, run!.id)) - .then((rows) => rows[0] ?? null); + const retryRun = await waitForRetryRun(run!.id); expect(retryRun).toMatchObject({ status: "scheduled_retry", scheduledRetryReason: WORKSPACE_BUSY_RETRY_REASON, @@ -658,11 +669,7 @@ describeEmbeddedPostgres("shared-workspace run serialization", () => { const deferred = await waitForRunToLeaveActiveStates(run!.id); expect(deferred?.status).toBe("cancelled"); - const retryRun = await db - .select() - .from(heartbeatRuns) - .where(eq(heartbeatRuns.retryOfRunId, run!.id)) - .then((rows) => rows[0] ?? null); + const retryRun = await waitForRetryRun(run!.id); expect(retryRun?.status).toBe("scheduled_retry"); expect( (retryRun?.contextSnapshot as Record | null)?.workspaceBusyDeferredWhileAssignee, @@ -849,11 +856,7 @@ describeEmbeddedPostgres("shared-workspace run serialization", () => { expect(finishedRun?.errorCode).toBe(WORKSPACE_BUSY_ERROR_CODE); expect(executedRunIds).not.toContain(retryRunId); - const nextRetry = await db - .select() - .from(heartbeatRuns) - .where(eq(heartbeatRuns.retryOfRunId, retryRunId)) - .then((rows) => rows[0] ?? null); + const nextRetry = await waitForRetryRun(retryRunId); expect(nextRetry).toMatchObject({ status: "scheduled_retry", scheduledRetryAttempt: priorAttempts + 1,