-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(run-engine,webapp): return unclaimed runs to the queue when pausing #4398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
352c612
0a4aa58
b717937
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| area: webapp | ||
| type: fix | ||
| --- | ||
|
|
||
| Pausing a queue or an environment now also holds back runs that were already waiting to start. Previously those runs would still go ahead, so a pause could take effect a little later than expected. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -49,6 +49,86 @@ describe("convergeBillingLimitEnvironmentsForOrg", () => { | |||||
| expect(envAfter.pauseSource).toBeNull(); | ||||||
| }); | ||||||
|
|
||||||
| postgresTest("returns unclaimed runs after pausing for a billing limit", async ({ prisma }) => { | ||||||
| const { organization, project } = await createTestOrgProjectWithMember(prisma); | ||||||
| const environment = await createRuntimeEnvironment(prisma, { | ||||||
| projectId: project.id, | ||||||
| organizationId: organization.id, | ||||||
| type: "PRODUCTION", | ||||||
| slug: uniqueId("prod"), | ||||||
| }); | ||||||
|
|
||||||
| const calls: Array<{ concurrency?: number; returnedFor?: string }> = []; | ||||||
|
|
||||||
|
Comment on lines
+61
to
+62
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win Duplicate The 🐛 Proposed fix- const calls: Array<{ concurrency?: number; returnedFor?: string }> = [];
const calls: Array<{ concurrency?: number; returnedFor?: string }> = [];📝 Committable suggestion
Suggested change
|
||||||
| const result = await convergeBillingLimitEnvironmentsForOrg(organization.id, "grace", { | ||||||
| prismaClient: prisma, | ||||||
| updateConcurrency: async (_env, maximumConcurrencyLimit) => { | ||||||
| calls.push({ concurrency: maximumConcurrencyLimit }); | ||||||
| }, | ||||||
| returnUnclaimed: async (env) => { | ||||||
| calls.push({ returnedFor: env.id }); | ||||||
| }, | ||||||
| }); | ||||||
|
|
||||||
| expect(result).toEqual({ paused: 1, unpaused: 0 }); | ||||||
|
|
||||||
| expect(calls).toEqual([{ concurrency: 0 }, { returnedFor: environment.id }]); | ||||||
|
|
||||||
| const envAfter = await prisma.runtimeEnvironment.findUniqueOrThrow({ | ||||||
| where: { id: environment.id }, | ||||||
| }); | ||||||
| expect(envAfter.paused).toBe(true); | ||||||
| expect(envAfter.pauseSource).toBe(EnvironmentPauseSource.BILLING_LIMIT); | ||||||
| }); | ||||||
|
|
||||||
| postgresTest("does not return unclaimed runs when unpausing", async ({ prisma }) => { | ||||||
| const { organization } = await createBillingPausedProductionEnv(prisma); | ||||||
|
|
||||||
| const returnUnclaimed = vi.fn(async () => undefined); | ||||||
|
|
||||||
| await convergeBillingLimitEnvironmentsForOrg(organization.id, "ok", { | ||||||
| prismaClient: prisma, | ||||||
| updateConcurrency: async () => undefined, | ||||||
| returnUnclaimed, | ||||||
| }); | ||||||
|
|
||||||
| expect(returnUnclaimed).not.toHaveBeenCalled(); | ||||||
| }); | ||||||
|
|
||||||
| postgresTest("keeps the pause when returning unclaimed runs fails", async ({ prisma }) => { | ||||||
| const { organization, project } = await createTestOrgProjectWithMember(prisma); | ||||||
| const environment = await createRuntimeEnvironment(prisma, { | ||||||
| projectId: project.id, | ||||||
| organizationId: organization.id, | ||||||
| type: "PRODUCTION", | ||||||
| slug: uniqueId("prod"), | ||||||
| }); | ||||||
| const second = await createRuntimeEnvironment(prisma, { | ||||||
| projectId: project.id, | ||||||
| organizationId: organization.id, | ||||||
| type: "PRODUCTION", | ||||||
| slug: uniqueId("prod"), | ||||||
| }); | ||||||
|
|
||||||
| const result = await convergeBillingLimitEnvironmentsForOrg(organization.id, "grace", { | ||||||
| prismaClient: prisma, | ||||||
| updateConcurrency: async () => undefined, | ||||||
| returnUnclaimed: async () => { | ||||||
| throw new Error("run queue unavailable"); | ||||||
| }, | ||||||
| }); | ||||||
|
|
||||||
| expect(result).toEqual({ paused: 2, unpaused: 0 }); | ||||||
|
|
||||||
| for (const env of [environment, second]) { | ||||||
| const envAfter = await prisma.runtimeEnvironment.findUniqueOrThrow({ | ||||||
| where: { id: env.id }, | ||||||
| }); | ||||||
| expect(envAfter.paused).toBe(true); | ||||||
| expect(envAfter.pauseSource).toBe(EnvironmentPauseSource.BILLING_LIMIT); | ||||||
| } | ||||||
| }); | ||||||
|
|
||||||
| postgresTest("rolls back pause when concurrency update fails", async ({ prisma }) => { | ||||||
| const { organization, project } = await createTestOrgProjectWithMember(prisma); | ||||||
| const environment = await createRuntimeEnvironment(prisma, { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| import { type PrismaClient } from "@trigger.dev/database"; | ||
| import { describe, expect, vi } from "vitest"; | ||
| import { postgresTest } from "@internal/testcontainers"; | ||
| import type { AuthenticatedEnvironment } from "~/services/apiAuth.server"; | ||
| import { | ||
| createRuntimeEnvironment, | ||
| createTestOrgProjectWithMember, | ||
| uniqueId, | ||
| } from "./fixtures/environmentVariablesFixtures"; | ||
|
|
||
| vi.setConfig({ testTimeout: 60_000 }); | ||
|
|
||
| const calls: string[] = []; | ||
|
|
||
| vi.mock("~/v3/runQueue.server", () => ({ | ||
| updateEnvConcurrencyLimits: vi.fn(async (_env: unknown, limit?: number) => { | ||
| calls.push(`updateEnvConcurrencyLimits:${limit}`); | ||
| }), | ||
| updateQueueConcurrencyLimits: vi.fn(async (_env: unknown, name: string, limit: number) => { | ||
| calls.push(`updateQueueConcurrencyLimits:${name}:${limit}`); | ||
| }), | ||
| removeQueueConcurrencyLimits: vi.fn(async () => { | ||
| calls.push("removeQueueConcurrencyLimits"); | ||
| }), | ||
| returnUnclaimedMessagesToQueue: vi.fn(async () => ({ | ||
| returned: 0, | ||
| skipped: 0, | ||
| errors: 0, | ||
| passes: 1, | ||
| })), | ||
| sweepUnclaimedRuns: vi.fn(async (_env: unknown, queue?: string) => { | ||
| calls.push(`sweepUnclaimedRuns:${queue ?? "*"}`); | ||
| }), | ||
| })); | ||
|
|
||
| async function loadServices() { | ||
| const [{ PauseEnvironmentService }, { authIncludeBase, toAuthenticated }] = await Promise.all([ | ||
| import("~/v3/services/pauseEnvironment.server"), | ||
| import("~/models/runtimeEnvironment.server"), | ||
| ]); | ||
| return { PauseEnvironmentService, authIncludeBase, toAuthenticated }; | ||
| } | ||
|
|
||
| type Loaded = Awaited<ReturnType<typeof loadServices>>; | ||
|
|
||
| async function seedEnv( | ||
| loaded: Loaded, | ||
| prisma: PrismaClient | ||
| ): Promise<{ environment: AuthenticatedEnvironment; environmentId: string }> { | ||
| const { organization, project } = await createTestOrgProjectWithMember(prisma); | ||
| const created = await createRuntimeEnvironment(prisma, { | ||
| projectId: project.id, | ||
| organizationId: organization.id, | ||
| type: "PRODUCTION", | ||
| slug: uniqueId("prod"), | ||
| }); | ||
|
|
||
| const row = await prisma.runtimeEnvironment.findFirstOrThrow({ | ||
| where: { id: created.id }, | ||
| include: loaded.authIncludeBase, | ||
| }); | ||
|
|
||
| return { environment: loaded.toAuthenticated(row), environmentId: created.id }; | ||
| } | ||
|
|
||
| describe("pause sweep wiring", () => { | ||
| postgresTest("pausing an environment sweeps after the limit is zeroed", async ({ prisma }) => { | ||
| calls.length = 0; | ||
| const loaded = await loadServices(); | ||
| const { environment } = await seedEnv(loaded, prisma); | ||
|
|
||
| const result = await new loaded.PauseEnvironmentService(prisma).call(environment, "paused"); | ||
|
|
||
| expect(result).toEqual({ success: true, state: "paused" }); | ||
| expect(calls).toEqual(["updateEnvConcurrencyLimits:0", "sweepUnclaimedRuns:*"]); | ||
| }); | ||
|
|
||
| postgresTest("resuming an environment does not sweep", async ({ prisma }) => { | ||
| const loaded = await loadServices(); | ||
| const { environment, environmentId } = await seedEnv(loaded, prisma); | ||
|
|
||
| await prisma.runtimeEnvironment.update({ | ||
| where: { id: environmentId }, | ||
| data: { paused: true }, | ||
| }); | ||
|
|
||
| calls.length = 0; | ||
|
|
||
| const result = await new loaded.PauseEnvironmentService(prisma).call(environment, "resumed"); | ||
|
|
||
| expect(result).toEqual({ success: true, state: "resumed" }); | ||
| expect(calls).toEqual(["updateEnvConcurrencyLimits:undefined"]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing rollback on failure, unlike sibling pause paths.
If
updateQueueConcurrencyLimitsor the newreturnUnclaimedMessagesToQueuecall throws here, the outer catch just logs and returnssuccess: false— buttaskQueue.pausedwas already committed totrueabove, and there's no rollback.pauseEnvironment.server.tsandbillingLimitConvergeEnvironments.server.tsboth wrap the equivalent concurrency-update + returnUnclaimed sequence in an inner try/catch that reverts the DB pause state on error; this file lacks that, so a failed pause here can leave the queue marked paused in the DB while the caller is told the operation failed.Consider wrapping the
action === "paused"branch's two calls in a try/catch that revertstaskQueue.paused(and the concurrency limit, if previously unset) on failure, mirroring the other two pause flows.Also applies to: 55-73