mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-29 21:07:40 +01:00
fix(server): preserve terminal workspace recovery cursor precision
Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
a1a7bfd915
commit
28a197cdfd
4 files changed
+74
-15
No files matched your search
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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)) });
|
||||
|
||||
|
||||
@@ -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<string, { revision: string; issueIds: Set<string>; nextAt: number }>();
|
||||
const terminalWorkspaceDeadlines = new Map<string, number>();
|
||||
const isolatedRehomeDeadlines = new Map<string, number>();
|
||||
@@ -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<string>`${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<string, ReturnType<typeof inspectGitCloseReadiness>>();
|
||||
|
||||
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.
|
||||
|
||||
Reference in new issue
Block a user