From 28a197cdfdab7977eecf591df39612009d725e17 Mon Sep 17 00:00:00 2001 From: Dotta Date: Sat, 24 Oct 2026 15:27:55 -0700 Subject: [PATCH] fix(server): preserve terminal workspace recovery cursor precision Co-Authored-By: Paperclip --- doc/idle-terminal-workspace-pagination.md | 35 +++++++++++++++++++ ...2026-10-24-recurring-server-sleep-stack.md | 24 +++++++++++++ .../execution-workspaces-service.test.ts | 11 ++++-- server/src/services/execution-workspaces.ts | 19 ++++------ 4 files changed, 74 insertions(+), 15 deletions(-) create mode 100644 doc/idle-terminal-workspace-pagination.md diff --git a/doc/idle-terminal-workspace-pagination.md b/doc/idle-terminal-workspace-pagination.md new file mode 100644 index 00000000000..e7e5f7f32cd --- /dev/null +++ b/doc/idle-terminal-workspace-pagination.md @@ -0,0 +1,35 @@ +# Terminal workspace recovery pagination + +Terminal workspace maintenance already uses the shared delivery coordinator and +retains deadlines only for outstanding work. Its bounded scan must progress past +ineligible rows so that later eligible work can finish and the worker can park. + +The source query orders candidates by `(updated_at, id)`. PostgreSQL timestamps +can contain microseconds. The former cursor used a JavaScript Date, which loses +that precision. A skipped row could then compare greater than its own rounded +cursor. Repeated full pages kept recovery active and prevented later cleanup. + +The query now carries the database timestamp as text alongside each workspace. +Only the private scan cursor uses that value. Workspace data, business deadlines, +and the frozen rotation boundary keep their existing types and behavior. The +cursor comparison uses the same timestamp/ID tuple as the ordering. The source +service continues to own cleanup eligibility, authorization, concurrency fences, +page limits, abort handling, failed-page recovery, and rotation under churn. +No scheduler API, queue, migration, producer signal, or external alarm changes. + +## Verification + +The existing full-page starvation regression now gives its skipped rows actual +PostgreSQL microsecond timestamps. Before the repair, the eligible workspace was +never archived. After the repair, the full workspace service and terminal worker +selection passes all 87 tests, including bounded rotation under continuous churn, +changed rows behind the cursor, failed-page recovery, quiet workers, and shutdown. +The service suite now supports the repository's validated disposable external +PostgreSQL URL so this path executes where embedded PostgreSQL is unavailable. +The database connection closes during teardown. Server typechecking passes. + +This fixes recovery progress. It does not establish provider suspension or change +which active or unresolved resources prevent instance sleep. + +Workspace-wide typechecking and the production build also pass. Full sharded CI +and fresh review remain required before this PR is ready. diff --git a/doc/plans/2026-10-24-recurring-server-sleep-stack.md b/doc/plans/2026-10-24-recurring-server-sleep-stack.md index 09ebcc6a1de..e1b421845f3 100644 --- a/doc/plans/2026-10-24-recurring-server-sleep-stack.md +++ b/doc/plans/2026-10-24-recurring-server-sleep-stack.md @@ -842,3 +842,27 @@ paging in the earlier TTL and effect-recovery owners, propagating parent changes through later PRs; qualify the terminal-workspace cursor; then implement the join-request retention audit on this branch. Every original inventory item and remaining retention source remains required. No merge is authorized. + +## Decision and workspace precision follow-up + +Expiry #17479 now preserves the PostgreSQL timestamp in its cursor, head +`e89c682ba5`. Its regression failed before repair; all 15 expiry cases and server +typechecking pass. Fresh CI is green and Greptile is 5/5 at that head. +Effect recovery #17482 has the same verified repair at `cebcd9c427`: its invalid +legacy claim regression failed before repair, then all 18 cases and typechecking +passed. Both changes have been merged through #17486, #17490, and #17497. +The combined 33 expiry/effect cases pass on the latest stack. Fresh dependent +CI/review is running. Superseded runs on the Grok/retention heads were cancelled +when both base and head updated; replacement workflows exist. Do not rerun or +claim those heads green until their current runs finish. + +Approval retention previously passed all checks and fresh 5/5 at `a29ce265b0`. +Its new head is `2c5babcabc`, with only parent paging repairs added. Join retention +still has no implementation; its branch now includes the current parents. + +The terminal-workspace audit also reproduced starvation using real microsecond +source timestamps. The focused branch `codex/sleep-terminal-workspace-precision` +preserves cursor precision in the owning query. All 87 workspace service/worker +cases and server typechecking pass. See +[workspace pagination](../idle-terminal-workspace-pagination.md). Publication and +fresh CI/review are next. Join retention remains the next new migration duty. diff --git a/server/src/__tests__/execution-workspaces-service.test.ts b/server/src/__tests__/execution-workspaces-service.test.ts index faf0f68bff8..7ae1e4ed3d9 100644 --- a/server/src/__tests__/execution-workspaces-service.test.ts +++ b/server/src/__tests__/execution-workspaces-service.test.ts @@ -1,3 +1,4 @@ +import { resolveExternalTestDatabaseUrl } from "./helpers/external-test-database.js"; import * as localServiceSupervisor from "../services/local-service-supervisor.js"; import { EMBEDDED_POSTGRES_TEST_TIMEOUT_MS } from "@paperclipai/db"; import { execFile } from "node:child_process"; @@ -160,7 +161,8 @@ describe("execution workspace config helpers", () => { }); }); -const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const externalUrl = resolveExternalTestDatabaseUrl("PAPERCLIP_EXECUTION_WORKSPACES_TEST_DATABASE_URL"); +const embeddedPostgresSupport = externalUrl ? { supported: true, reason: undefined } : await getEmbeddedPostgresTestSupport(); const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; if (!embeddedPostgresSupport.supported) { @@ -258,8 +260,8 @@ describeEmbeddedPostgres("executionWorkspaceService.getCloseReadiness", () => { }>(); beforeAll(async () => { - tempDb = await startEmbeddedPostgresTestDatabase("paperclip-execution-workspaces-service-"); - db = createDb(tempDb.connectionString); + if (!externalUrl) tempDb = await startEmbeddedPostgresTestDatabase("paperclip-execution-workspaces-service-"); + db = createDb(externalUrl ?? tempDb!.connectionString); svc = executionWorkspaceService(db, { resolvePullRequestDetails: vi.fn(async (companyId, reference) => pullRequestDetailsByKey.get(`${companyId}:${reference.number}`) @@ -295,6 +297,7 @@ describeEmbeddedPostgres("executionWorkspaceService.getCloseReadiness", () => { }); afterAll(async () => { + await db?.$client.end({ timeout: 1 }); await tempDb?.cleanup(); }); @@ -1124,6 +1127,8 @@ describeEmbeddedPostgres("executionWorkspaceService.getCloseReadiness", () => { .set({ sourceIssueId: openIssueId, updatedAt: new Date(Date.UTC(2020, 0, index + 1)) }) .where(eq(executionWorkspaces.id, workspaceId)); } + await db.execute(sql`update execution_workspaces set updated_at = updated_at + interval '456 microseconds' + where id in (${sql.join(skippedWorkspaceIds.map(id => sql`${id}::uuid`), sql`, `)})`); // The eligible workspace is newest, so it sorts after the whole skipped page. const eligible = await seedAncestryTerminalWorkspace({ updatedAt: new Date(Date.UTC(2020, 0, 9)) }); diff --git a/server/src/services/execution-workspaces.ts b/server/src/services/execution-workspaces.ts index 9d42edcc6e1..510d28e9945 100644 --- a/server/src/services/execution-workspaces.ts +++ b/server/src/services/execution-workspaces.ts @@ -40,7 +40,7 @@ import { createHash } from "node:crypto"; import fs from "node:fs/promises"; import path from "node:path"; import { isDeepStrictEqual, promisify } from "node:util"; -import { and, asc, desc, eq, gt, inArray, isNull, lte, ne, or, sql } from "drizzle-orm"; +import { and, asc, desc, eq, inArray, isNull, lte, ne, or, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { executionWorkspaces, @@ -1746,7 +1746,7 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic // required: without it the query re-selects the oldest ineligible rows every // sweep and starves eligible rows behind them. The cursor resets to the start // when a sweep reaches the end of the candidate set. - let terminalSweepCursor: { updatedAt: Date; id: string } | null = null; + let terminalSweepCursor: { updatedAt: string; id: string } | null = null; const terminalInspections = new Map; nextAt: number }>(); const terminalWorkspaceDeadlines = new Map(); const isolatedRehomeDeadlines = new Map(); @@ -3312,17 +3312,11 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic ? lte(executionWorkspaces.updatedAt, boundary) : undefined; const cursorFilter = cursor - ? or( - gt(executionWorkspaces.updatedAt, cursor.updatedAt), - and( - eq(executionWorkspaces.updatedAt, cursor.updatedAt), - gt(executionWorkspaces.id, cursor.id), - ), - ) + ? sql`(${executionWorkspaces.updatedAt}, ${executionWorkspaces.id}) > (${cursor.updatedAt}::timestamptz, ${cursor.id}::uuid)` : undefined; const scanFilter = and(baseCandidateFilter, boundaryFilter, cursorFilter); const candidates = await db - .select() + .select({ workspace: executionWorkspaces, cursorAt: sql`${executionWorkspaces.updatedAt}::text` }) .from(executionWorkspaces) .where(scanFilter) .orderBy(asc(executionWorkspaces.updatedAt), asc(executionWorkspaces.id)) @@ -3335,7 +3329,7 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic terminalSweepBoundary = null; } else { const lastCandidate = candidates[candidates.length - 1]!; - terminalSweepCursor = { updatedAt: lastCandidate.updatedAt, id: lastCandidate.id }; + terminalSweepCursor = { updatedAt: lastCandidate.cursorAt, id: lastCandidate.workspace.id }; } const result = { checked: candidates.length, @@ -3355,7 +3349,8 @@ export function executionWorkspaceService(db: Db, opts: ExecutionWorkspaceServic }; const readinessByWorkingTree = new Map>(); - for (let workspace of candidates) { + for (const candidate of candidates) { + let workspace = candidate.workspace; signal?.throwIfAborted(); // Trash deletion is part of compaction, so a sweep that skips compaction // also leaves leftover trash for a later sweep.