From 36844a458b199af491215f4b48199dedf1f6324b Mon Sep 17 00:00:00 2001 From: Dotta Date: Sat, 24 Oct 2026 09:43:47 -0700 Subject: [PATCH] test: cover concurrent setup expiry changes and startup ownership Co-Authored-By: Paperclip --- .../environment-custom-images-service.test.ts | 32 +++++++++++++++++-- .../server-startup-feedback-export.test.ts | 12 ++++--- .../src/services/custom-image-cleanup-work.ts | 1 - .../src/services/environment-custom-images.ts | 2 -- 4 files changed, 36 insertions(+), 11 deletions(-) diff --git a/server/src/__tests__/environment-custom-images-service.test.ts b/server/src/__tests__/environment-custom-images-service.test.ts index 1ce6ae8287a..b6a67e9e3f6 100644 --- a/server/src/__tests__/environment-custom-images-service.test.ts +++ b/server/src/__tests__/environment-custom-images-service.test.ts @@ -1,3 +1,4 @@ +import { resolveExternalTestDatabaseUrl } from "./helpers/external-test-database.js"; import { registerCustomImageCleanupWork } from "../services/custom-image-cleanup-work.js"; import { createDeliveryWorkCoordinator } from "../services/delivery-work-coordinator.js"; import { idleWorkSnapshot } from "../services/task-admission.js"; @@ -30,7 +31,7 @@ import { startEmbeddedPostgresTestDatabase, } from "./helpers/embedded-postgres.js"; -const externalTestDatabase = process.env.PAPERCLIP_CUSTOM_IMAGE_TEST_DATABASE_URL; +const externalTestDatabase = resolveExternalTestDatabaseUrl("PAPERCLIP_CUSTOM_IMAGE_TEST_DATABASE_URL"); const embeddedPostgresSupport = externalTestDatabase ? { supported: true, reason: undefined } : await getEmbeddedPostgresTestSupport(); const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; @@ -214,7 +215,6 @@ describeEmbeddedPostgres("environmentCustomImageService", () => { const errors: unknown[] = []; beforeEach(async () => { - // Open the pool before fake time can advance connection handshakes. await Promise.all(Array.from({ length: 6 }, () => db.execute(sql`select pg_sleep(0.01)`))); draining = false; errors.length = 0; @@ -304,6 +304,33 @@ describeEmbeddedPostgres("environmentCustomImageService", () => { await quiet(); }); + it("retains a producer commit that arrives during the deadline read", async () => { + await start(); const created = await create(); await settle(); + await coordinator!.stop(); + let release!: () => void, reading!: () => void; + const gate = new Promise(resolve => { release = resolve; }); + const entered = new Promise(resolve => { reading = resolve; }); + const readDeadline = service.nextSetupCleanupAt; + vi.spyOn(service, "nextSetupCleanupAt").mockImplementationOnce(async () => { + const stale = await readDeadline(); + reading(); await gate; + return stale; + }); + const starting = start(); + await entered; + const earlier = new Date(Date.now() + 5 * 60_000); + try { + provider.call.mockResolvedValueOnce({ providerLeaseId: created.session.providerLeaseId, + status: "waiting_for_user", expiresAt: earlier.toISOString() } as never); + await service.refreshSetupSession({ sessionId: created.session.id }); + } finally { release(); } + await starting; await settle(); + await vi.waitFor(() => expect(coordinator!.nextWakeAt()).toBe(earlier.getTime())); + await settle(5 * 60_000); + await vi.waitFor(() => expect(cancellations()).toHaveLength(1)); + await quiet(); + }); + it("reconstructs a saved expiry after restart", async () => { await start(); const created = await create(); await settle(); await coordinator!.stop(); @@ -324,7 +351,6 @@ describeEmbeddedPostgres("environmentCustomImageService", () => { await vi.waitFor(() => expect(errors).toHaveLength(1)); errors.length = 0; draining = true; } - // Let the committed signal reconcile before observing passive drain. await settle(); const reads = vi.spyOn(db, "select"); await settle(2 * 60 * 60_000); diff --git a/server/src/__tests__/server-startup-feedback-export.test.ts b/server/src/__tests__/server-startup-feedback-export.test.ts index db22ce7b2c2..81c689366c3 100644 --- a/server/src/__tests__/server-startup-feedback-export.test.ts +++ b/server/src/__tests__/server-startup-feedback-export.test.ts @@ -129,6 +129,7 @@ const { }; const issueThreadInteractionServiceFactoryMock = vi.fn(() => issueThreadInteractionServiceMock); const environmentCustomImagesServiceMock = { + nextSetupCleanupAt: vi.fn(async (): Promise => null), cleanupExpiredSetupSessions: vi.fn(async () => ({ scanned: 0, timedOut: 0, failed: 0 })), }; const environmentCustomImagesServiceFactoryMock = vi.fn(() => environmentCustomImagesServiceMock); @@ -843,7 +844,8 @@ describe("startServer feedback export wiring", () => { expect(issueThreadInteractionServiceMock.sweepMergedPullRequestConfirmations).toHaveBeenCalledTimes(1); expect(executionWorkspaceServiceMock.sweepTerminalWorkspaces).toHaveBeenCalledTimes(1); expect(routineServiceMock.tickScheduledTriggers).toHaveBeenCalledTimes(1); - expect(environmentCustomImagesServiceMock.cleanupExpiredSetupSessions).toHaveBeenCalledTimes(2); + expect(environmentCustomImagesServiceMock.cleanupExpiredSetupSessions).toHaveBeenCalledTimes(1); + expect(deliveryWorkRegisterMock.mock.calls.some(([queue]) => queue === DELIVERY_QUEUES.customImageExpiry)).toBe(true); } finally { setIntervalSpy.mockRestore(); } @@ -960,10 +962,10 @@ describe("startServer feedback export wiring", () => { try { await expect(startServer()).resolves.toBeDefined(); await settleStartupHousekeeping(); - expect(vi.mocked(logger.error)).toHaveBeenCalledWith( - expect.objectContaining({ startupStep: "custom_image_setup_cleanup" }), - "startup housekeeping step failed", - ); + const registration = deliveryWorkRegisterMock.mock.calls.find(([queue]) => queue === DELIVERY_QUEUES.customImageExpiry); + expect(registration).toBeDefined(); + expect(await registration![1].hasPending!()).toBe(true); + expect(environmentCustomImagesServiceMock.nextSetupCleanupAt).not.toHaveBeenCalled(); // The scheduler starts only after every housekeeping step settled. expect(heartbeatIntervals).toHaveLength(1); } finally { diff --git a/server/src/services/custom-image-cleanup-work.ts b/server/src/services/custom-image-cleanup-work.ts index e409bc45d42..0ad2de8fee6 100644 --- a/server/src/services/custom-image-cleanup-work.ts +++ b/server/src/services/custom-image-cleanup-work.ts @@ -3,7 +3,6 @@ import type { createDeliveryWorkCoordinator } from "./delivery-work-coordinator. import { DELIVERY_QUEUES } from "./delivery-work-notifications.js"; import type { environmentCustomImageService } from "./environment-custom-images.js"; -/** Existing setup rows own expiry; provider operations remain in their service. */ export function registerCustomImageCleanupWork( coordinator: ReturnType, service: Pick, "cleanupExpiredSetupSessions" | "nextSetupCleanupAt">, diff --git a/server/src/services/environment-custom-images.ts b/server/src/services/environment-custom-images.ts index 30232b077c7..9ff4fd1c1df 100644 --- a/server/src/services/environment-custom-images.ts +++ b/server/src/services/environment-custom-images.ts @@ -291,8 +291,6 @@ export function environmentCustomImageService( const environments = environmentService(db); const secrets = secretService(db); - // This service owns all setup-session state and expiry changes. Keep wakes - // inside the existing outer transaction, including historical cleanup hints. function writeSetup(write: (tx: Db) => PromiseLike): Promise { return db.transaction(async tx => { await notifyDeliveryWork(tx, DELIVERY_QUEUES.customImageExpiry);