From d086ee81f0ae57cd687288d57971ce349ce6f426 Mon Sep 17 00:00:00 2001 From: Thibault Le Ouay Ducasse Date: Sat, 17 Oct 2026 16:22:14 +0200 Subject: [PATCH] fix pr --- .../api/src/router/page.custom-domain.test.ts | 5 ++- .../__tests__/dedupe-and-atomicity.test.ts | 28 ++++++++++++ packages/services/src/import/phase-writers.ts | 6 ++- .../services/src/incident/postmortem-draft.ts | 10 ++++- packages/services/src/invitation/constants.ts | 6 +-- packages/services/src/monitor/create.ts | 4 ++ packages/services/src/monitor/internal.ts | 6 ++- packages/services/src/monitor/update.ts | 2 + .../services/src/sso/__tests__/sso.test.ts | 44 ++++++++++++++++++- packages/services/src/sso/enable.ts | 16 +++++-- 10 files changed, 113 insertions(+), 14 deletions(-) diff --git a/packages/api/src/router/page.custom-domain.test.ts b/packages/api/src/router/page.custom-domain.test.ts index 4f16bd64d..14882194e 100644 --- a/packages/api/src/router/page.custom-domain.test.ts +++ b/packages/api/src/router/page.custom-domain.test.ts @@ -24,9 +24,10 @@ test("page.updateCustomDomain rejects a taken domain before touching Vercel", as const realFetch = globalThis.fetch; let fetchCalls = 0; - globalThis.fetch = (...args) => { + // Count, never send: a regression must not reach the real Vercel API. + globalThis.fetch = () => { fetchCalls++; - return realFetch(...args); + return Promise.reject(new Error("unexpected Vercel request")); }; try { await caller.page.updateCustomDomain({ diff --git a/packages/services/src/import/__tests__/dedupe-and-atomicity.test.ts b/packages/services/src/import/__tests__/dedupe-and-atomicity.test.ts index c0174f80d..f65d68880 100644 --- a/packages/services/src/import/__tests__/dedupe-and-atomicity.test.ts +++ b/packages/services/src/import/__tests__/dedupe-and-atomicity.test.ts @@ -169,6 +169,34 @@ describe("writeIncidentsPhase first-update match", () => { ); }); }); + + test("a first update with milliseconds still matches on rerun", async () => { + await withTestTransaction(async (tx) => { + const ctx = { ...teamCtx, db: tx }; + const pageRow = await insertPage(tx, ctx.workspace.id, "millis"); + const title = `${TEST_PREFIX}-millis`; + const pc = { ctx, tx, provider: "statuspage" as const }; + const makePhase = (): PhaseResult => ({ + phase: "incidents", + status: "completed", + resources: [ + incident("inc_ms", title, pageRow.id, ctx.workspace.id, [ + new Date("2024-03-01T00:00:00.123Z"), + ]), + ], + }); + + const firstRun = makePhase(); + await writeIncidentsPhase(pc, firstRun, pageRow.id, new Map()); + const secondRun = makePhase(); + await writeIncidentsPhase(pc, secondRun, pageRow.id, new Map()); + + expect(secondRun.resources[0].status).toBe("skipped"); + expect(secondRun.resources[0].openstatusId).toBe( + firstRun.resources[0].openstatusId, + ); + }); + }); }); // These run on the committed db: inside `withTestTransaction` the writers diff --git a/packages/services/src/import/phase-writers.ts b/packages/services/src/import/phase-writers.ts index 9d0cf3bce..40f177c90 100644 --- a/packages/services/src/import/phase-writers.ts +++ b/packages/services/src/import/phase-writers.ts @@ -431,7 +431,11 @@ async function findReportByFirstUpdate( .orderBy(asc(statusReportUpdate.date)) .limit(1) .get(); - if (earliest?.date.getTime() === args.firstUpdateDate.getTime()) { + // The column stores whole seconds; provider dates may carry milliseconds. + if ( + earliest?.date.getTime() === + Math.floor(args.firstUpdateDate.getTime() / 1000) * 1000 + ) { return candidate; } } diff --git a/packages/services/src/incident/postmortem-draft.ts b/packages/services/src/incident/postmortem-draft.ts index af47dff07..b447c9304 100644 --- a/packages/services/src/incident/postmortem-draft.ts +++ b/packages/services/src/incident/postmortem-draft.ts @@ -100,9 +100,14 @@ export async function collectChannelTranscript( let count = 0; for (const message of top) { const block: string[] = []; + // An older block over the remaining budget is dropped whole, so stop + // paging its thread early. Only the newest block needs every page, to + // keep its newest replies. + const overBudget = () => + blocks.length > 0 && count + block.length > MAX_MESSAGES; // A bot parent (the incident card) is left out, its human replies are not. if (!message.bot_id && message.text) block.push(line(message)); - if (message.reply_count && message.ts) { + if (message.reply_count && message.ts && !overBudget()) { let replyCursor: string | undefined; do { const thread = await client.conversations.replies({ @@ -114,11 +119,12 @@ export async function collectChannelTranscript( for (const reply of thread.messages ?? []) { if (reply.ts === message.ts || reply.bot_id || !reply.text) continue; block.push(` ${line(reply)}`); + if (overBudget()) break; } replyCursor = thread.has_more ? thread.response_metadata?.next_cursor : undefined; - } while (replyCursor); + } while (replyCursor && !overBudget()); } if (block.length === 0) continue; if (count + block.length > MAX_MESSAGES) { diff --git a/packages/services/src/invitation/constants.ts b/packages/services/src/invitation/constants.ts index ed8edaba8..f47d4a44b 100644 --- a/packages/services/src/invitation/constants.ts +++ b/packages/services/src/invitation/constants.ts @@ -1,6 +1,6 @@ /** - * "Unlimited" members resolves to this effective cap — matches the legacy - * behavior in `invitationRouter.create`. Large enough that enterprise plans - * never brush against it in practice. + * "Unlimited" members resolves to this effective cap, shared by + * `createInvitation` and `acceptInvitation`. Large enough that enterprise + * plans never brush against it in practice. */ export const UNLIMITED_MEMBERS_EFFECTIVE_CAP = 420; diff --git a/packages/services/src/monitor/create.ts b/packages/services/src/monitor/create.ts index 8bc94be77..f3f953a32 100644 --- a/packages/services/src/monitor/create.ts +++ b/packages/services/src/monitor/create.ts @@ -6,6 +6,7 @@ import { type ServiceContext, withTransaction } from "../context"; import { assertWithinLimit } from "../limits"; import type { Monitor } from "../types"; import { + assertSchedulingWithinPlan, headersToDbJson, pickDefaultRegions, serialiseAssertions, @@ -22,6 +23,9 @@ export async function createMonitor(args: { const input = CreateMonitorInput.parse(args.input); assertMonitorUrlSafe({ jobType: input.jobType, url: input.url }); + // Only caller-supplied values: the plan defaults below are picked from the + // plan, and a workspace's stored limits can lag behind newly added regions. + assertSchedulingWithinPlan(ctx.workspace.limits, input); return withTransaction(ctx, async (tx) => { await assertWithinLimit({ diff --git a/packages/services/src/monitor/internal.ts b/packages/services/src/monitor/internal.ts index afb9068c3..1a68d076f 100644 --- a/packages/services/src/monitor/internal.ts +++ b/packages/services/src/monitor/internal.ts @@ -185,19 +185,23 @@ export function pickDefaultRegions(workspace: Workspace): { * Enforce the plan's periodicity, region-count and region-access limits on * a monitor's scheduling. Shared by every path that writes them so a clone * (or a source that predates a downgrade) can't carry out-of-plan values. + * A field left `undefined` isn't being written and isn't checked, so a + * partial update of a monitor that predates a downgrade still goes through. */ export function assertSchedulingWithinPlan( limits: Workspace["limits"], - scheduling: { periodicity: string; regions: ReadonlyArray }, + scheduling: { periodicity?: string; regions?: ReadonlyArray }, ): void { const { periodicity, regions } = scheduling; if ( + periodicity !== undefined && !limits.periodicity.includes( periodicity as (typeof limits)["periodicity"][number], ) ) { throw new LimitExceededError("periodicity", limits.periodicity.length); } + if (regions === undefined) return; if (limits["max-regions"] < regions.length) { throw new LimitExceededError("max-regions", limits["max-regions"]); } diff --git a/packages/services/src/monitor/update.ts b/packages/services/src/monitor/update.ts index a66e5ead1..68c171c95 100644 --- a/packages/services/src/monitor/update.ts +++ b/packages/services/src/monitor/update.ts @@ -6,6 +6,7 @@ import { requireScope } from "../auth"; import { type ServiceContext, withTransaction } from "../context"; import type { Monitor } from "../types"; import { + assertSchedulingWithinPlan, getMonitorInWorkspace, headersToDbJson, serialiseAssertions, @@ -33,6 +34,7 @@ export async function updateMonitorConfig(args: { const { ctx } = args; requireScope(ctx, "write"); const input = UpdateMonitorConfigInput.parse(args.input); + assertSchedulingWithinPlan(ctx.workspace.limits, input); return withTransaction(ctx, async (tx) => { const existing = await getMonitorInWorkspace({ diff --git a/packages/services/src/sso/__tests__/sso.test.ts b/packages/services/src/sso/__tests__/sso.test.ts index cb35addfb..77b672e58 100644 --- a/packages/services/src/sso/__tests__/sso.test.ts +++ b/packages/services/src/sso/__tests__/sso.test.ts @@ -1,4 +1,4 @@ -import { and, eq } from "@openstatus/db"; +import { and, db, eq } from "@openstatus/db"; import { usersToWorkspaces, workspace, @@ -219,6 +219,48 @@ describe("enableSso", () => { }); }); + // Runs on the committed db: the keep branch only applies to a transaction + // enableSso owns. A fresh workspace keeps the commit away from seeded ones. + test("keeps the org when its commit landed despite an error", async () => { + const fixture = await createWorkspaceFixture("scale", { + limits: JSON.stringify({ sso: true }), + }); + // Org ids are unique across workspaces and this commit outlives the test. + const organizationId = `org_landed_${fixture.workspace.id}`; + const deleted: string[] = []; + let transactions = 0; + // The persisting transaction commits, then the response is "lost". + const lossyDb = new Proxy(db, { + get(target, prop, receiver) { + if (prop !== "transaction") return Reflect.get(target, prop, receiver); + return async (fn: Parameters[0]) => { + const result = await target.transaction(fn); + transactions++; + if (transactions === 2) throw new Error("lost response"); + return result; + }; + }, + }); + + await expect( + enableSso({ + ctx: { + ...makeUserCtx(fixture.workspace, { userId: fixture.userId }), + db: lossyDb, + workos: fakeWorkOS({ organizationId, deleted }), + }, + }), + ).rejects.toThrow("lost response"); + expect(deleted).toEqual([]); + + const row = await db + .select() + .from(workspace) + .where(eq(workspace.id, fixture.workspace.id)) + .get(); + expect(row?.workosOrganizationId).toBe(organizationId); + }); + test("refuses to enable when the add-on is removed mid-enable", async () => { await withTestTransaction(async (tx) => { const deleted: string[] = []; diff --git a/packages/services/src/sso/enable.ts b/packages/services/src/sso/enable.ts index ca7de60b0..5de986060 100644 --- a/packages/services/src/sso/enable.ts +++ b/packages/services/src/sso/enable.ts @@ -3,7 +3,7 @@ import { selectWorkspaceSchema, workspace } from "@openstatus/db/src/schema"; import { emitAudit } from "../audit"; import { requireScope } from "../auth"; -import { type ServiceContext, withTransaction } from "../context"; +import { isTx, type ServiceContext, withTransaction } from "../context"; import { ConflictError, NotFoundError, @@ -124,9 +124,17 @@ export async function enableSso(args: { }; }); } catch (err) { - // The commit may have landed even though we saw an error (e.g. a lost - // response) — never delete an org the workspace now references. - const row = await (ctx.db ?? defaultDb) + const db = ctx.db ?? defaultDb; + // Inside a caller's transaction the write is still uncommitted and the + // caller rolls back on this error, so the org is always an orphan. + if (isTx(db)) { + await discardCreated(); + throw err; + } + // Our own commit may have landed even though we saw an error (e.g. a lost + // response) — never delete an org the workspace now references. If the + // re-read fails too, keep the org: a leak beats a dangling reference. + const row = await db .select({ workosOrganizationId: workspace.workosOrganizationId }) .from(workspace) .where(eq(workspace.id, ctx.workspace.id)) -- 2.51.2