diff --git a/packages/plugins/plugin-workspace-diff/package.json b/packages/plugins/plugin-workspace-diff/package.json index 4acccdfd25..186b11b13d 100644 --- a/packages/plugins/plugin-workspace-diff/package.json +++ b/packages/plugins/plugin-workspace-diff/package.json @@ -52,7 +52,7 @@ }, "dependencies": { "@paperclipai/plugin-sdk": "workspace:*", - "@pierre/diffs": "^1.3.5" + "@pierre/diffs": "^1.3.6" }, "devDependencies": { "@types/node": "^24.0.0", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 250a7e0238..fee99a970b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -749,8 +749,8 @@ importers: specifier: workspace:* version: link:../sdk '@pierre/diffs': - specifier: ^1.3.5 - version: 1.3.5(@shikijs/themes@4.4.3)(react-dom@19.2.8(react@19.2.8))(react@19.2.8) + specifier: ^1.3.6 + version: 1.3.6(@shikijs/themes@4.4.3)(react-dom@19.2.8(react@19.2.8))(react@19.2.8) devDependencies: '@types/node': specifier: ^24.0.0 @@ -3207,8 +3207,8 @@ packages: '@paralleldrive/cuid2@2.3.1': resolution: {integrity: sha512-XO7cAxhnTZl0Yggq6jOgjiOHhbgcO4NqFqwSmQpjK3b6TEE6Uj/jfSk6wzYyemh3+I0sHirKSetjQwn5cZktFw==} - '@pierre/diffs@1.3.5': - resolution: {integrity: sha512-BhaLEiUvR+BdIyOYdogA4JLQjluWPubuwySmmIqEkcE0FwIWRbgJgHNC/r884dlxEr89fO4hTk/sBln0a7NSOw==} + '@pierre/diffs@1.3.6': + resolution: {integrity: sha512-a3woaW2QHy78JDxPJK0OJzwZUN4xoQLLIS/pceO8X6+L8gA5D682mP7/w3YxxEVRPXOaoe/p/RJ5Oj/3nrEzew==} peerDependencies: react: ^19.2.8 react-dom: ^19.2.8 @@ -10574,7 +10574,7 @@ snapshots: dependencies: '@noble/hashes': 1.8.0 - '@pierre/diffs@1.3.5(@shikijs/themes@4.4.3)(react-dom@19.2.8(react@19.2.8))(react@19.2.8)': + '@pierre/diffs@1.3.6(@shikijs/themes@4.4.3)(react-dom@19.2.8(react@19.2.8))(react@19.2.8)': dependencies: '@pierre/theme': 2.0.0 '@pierre/theming': 1.0.1(@pierre/theme@2.0.0)(@shikijs/themes@4.4.3)(react-dom@19.2.8(react@19.2.8))(react@19.2.8)(shiki@4.4.3) diff --git a/server/src/__tests__/instance-settings-routes.test.ts b/server/src/__tests__/instance-settings-routes.test.ts index 4c5bc17579..92c82751ab 100644 --- a/server/src/__tests__/instance-settings-routes.test.ts +++ b/server/src/__tests__/instance-settings-routes.test.ts @@ -1,6 +1,7 @@ import express from "express"; import request from "supertest"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { hoistModuleGraph } from "./helpers/hoist-module-graph.js"; const mockInstanceSettingsService = vi.hoisted(() => ({ get: vi.fn(), @@ -40,41 +41,51 @@ function registerModuleMocks() { // Identity object the mocked db.transaction hands to writers; tests assert // both the marker clear and the settings update receive THIS same tx. const TX_SENTINEL = { __tx: true }; +// Runs the callback with a sentinel tx and propagates throws, so a failing +// write inside rejects the whole request exactly like a real transaction +// rollback. This is the default mockDb.transaction implementation; a test +// that installs its own mockImplementation loses this default, so +// beforeEach below reinstalls it before every test. +function defaultTransactionImplementation(fn: (tx: unknown) => Promise) { + return fn(TX_SENTINEL); +} // Module-scoped (not rebuilt per createApp call) so a test can assert how // many times a request opened a transaction — the task-drain audit writes // for every company must share ONE transaction, not one each. const mockDb = { - // Runs the callback with a sentinel tx and propagates throws, so a - // failing write inside rejects the whole request exactly like a real - // transaction rollback. - transaction: vi.fn(async (fn: (tx: unknown) => Promise) => fn(TX_SENTINEL)), + transaction: vi.fn(defaultTransactionImplementation), }; -async function createApp(actor: any) { - const [{ errorHandler }, { instanceSettingsRoutes }] = await Promise.all([ - vi.importActual("../middleware/index.js"), - vi.importActual("../routes/instance-settings.js"), - ]); - const app = express(); - app.use(express.json()); - app.use((req, _res, next) => { - req.actor = actor; - next(); - }); - app.use("/api", instanceSettingsRoutes(mockDb as any)); - app.use(errorHandler); - return app; -} - describe("instance settings routes", () => { + const routeModules = hoistModuleGraph(registerModuleMocks, async () => { + const [{ errorHandler }, { instanceSettingsRoutes }] = await Promise.all([ + vi.importActual("../middleware/index.js"), + vi.importActual("../routes/instance-settings.js"), + ]); + return { errorHandler, instanceSettingsRoutes }; + }); + + function createApp(actor: any) { + const { errorHandler, instanceSettingsRoutes } = routeModules.value; + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.actor = actor; + next(); + }); + app.use("/api", instanceSettingsRoutes(mockDb as any)); + app.use(errorHandler); + return app; + } + beforeEach(() => { - vi.resetModules(); - vi.doUnmock("../services/index.js"); - vi.doUnmock("../routes/instance-settings.js"); - vi.doUnmock("../routes/authz.js"); - vi.doUnmock("../middleware/index.js"); - registerModuleMocks(); vi.clearAllMocks(); + // vi.clearAllMocks() clears recorded calls only; it does not remove a + // mockImplementation a prior test installed. Reinstall the default here + // so a stateful implementation from one test can never leak into the + // next one. + mockDb.transaction.mockReset(); + mockDb.transaction.mockImplementation(defaultTransactionImplementation); mockInstanceSettingsService.get.mockReset(); mockInstanceSettingsService.getGeneral.mockReset(); mockInstanceSettingsService.getExperimental.mockReset(); @@ -1151,10 +1162,19 @@ describe("instance settings routes", () => { const transactionCalls: string[] = []; let releasePostTransaction: (() => void) | undefined; let sawFirstCall = false; + // Resolves the instant the first (blocked) transaction call starts. + // The test then waits for this real event, not a fixed duration. + // Under CPU contention the event loop can take far longer than any + // fixed budget to reach this call, so a timer would flake here. + let notifyFirstTransactionStarted: (() => void) | undefined; + const firstTransactionStarted = new Promise((resolve) => { + notifyFirstTransactionStarted = resolve; + }); mockDb.transaction.mockImplementation((fn: (tx: unknown) => Promise) => { if (!sawFirstCall) { sawFirstCall = true; transactionCalls.push("post-start"); + notifyFirstTransactionStarted?.(); return new Promise((resolve) => { releasePostTransaction = () => { transactionCalls.push("post-commit"); @@ -1166,19 +1186,44 @@ describe("instance settings routes", () => { return fn(TX_SENTINEL); }); + // Each route handler awaits listCompanyIds as its last step before it + // enters the task-drain transition queue, so a second call proves the + // DELETE passed authorization and reached the queue — not merely that + // it has not arrived yet. + let listCompanyIdsCallCount = 0; + let notifySecondListCompanyIdsCall: (() => void) | undefined; + const secondListCompanyIdsCall = new Promise((resolve) => { + notifySecondListCompanyIdsCall = resolve; + }); + mockInstanceSettingsService.listCompanyIds.mockImplementation(async () => { + listCompanyIdsCallCount += 1; + if (listCompanyIdsCallCount === 2) notifySecondListCompanyIdsCall?.(); + return ["company-1", "company-2"]; + }); + const app = await createApp(adminActor); - // supertest only sends the request once something calls .then() on - // it, so kick both off eagerly instead of waiting for the final - // Promise.all below to do it — otherwise neither request would even - // reach the (still-pending) POST transaction during the wait. + // supertest only sends a request once something calls .then() on it, + // so force the POST to send now instead of waiting for the final + // Promise.all below to do it. const postPromise = request(app).post("/api/instance/task-drain").send({}); postPromise.then(() => {}, () => {}); + // Wait for the POST's transaction call to start before the test sends + // the DELETE. At that point the POST already called listCompanyIds, + // already entered the task-drain transition queue, and sits blocked + // inside the mocked db.transaction call — the POST holds the queue. + // Only a real event proves this; a fixed wait would not, because the + // event loop can take far longer than any fixed budget under CPU + // contention. Sending the DELETE only after this event fixes the + // request order by the queue, not by which socket the operating + // system happens to service first. + await firstTransactionStarted; const deletePromise = request(app).delete("/api/instance/task-drain"); deletePromise.then(() => {}, () => {}); - // Give both requests time to reach as far as they can go before the - // POST's transaction is released. - await new Promise((resolve) => setTimeout(resolve, 30)); + // Wait for the DELETE's own listCompanyIds call. It proves the DELETE + // passed authorization and reached the transition queue behind the + // POST — not merely that it has not shown up yet. + await secondListCompanyIdsCall; expect(transactionCalls).toEqual(["post-start"]); expect(mockHeartbeatService.applyTaskDrain).not.toHaveBeenCalled(); expect(mockHeartbeatService.stopTaskDrain).not.toHaveBeenCalled();