Skip to content

fix: removing schedule instance no longer crashes #1430

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 24, 2024
Merged

fix: removing schedule instance no longer crashes #1430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions ...pp/routes/resources.orgs.$organizationSlug.projects.$projectParam.schedules.new/route.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import { CronPattern, UpsertSchedule } from "~/v3/schedules";
import { UpsertTaskScheduleService } from "~/v3/services/upsertTaskSchedule.server";
import { AIGeneratedCronField } from "../resources.orgs.$organizationSlug.projects.$projectParam.schedules.new.natural-language";
import { TimezoneList } from "~/components/scheduled/timezones";
import { logger } from "~/services/logger.server";

const cronFormat = `* * * * *
┬ ┬ ┬ ┬ ┬
Expand Down Expand Up @@ -94,9 +95,9 @@ export const action = async ({ request, params }: ActionFunctionArgs) => {
submission.value?.friendlyId === result.id ? "Schedule updated" : "Schedule created"
);
} catch (error: any) {
const errorMessage = `Failed: ${
error instanceof Error ? error.message : JSON.stringify(error)
}`;
logger.error("Failed to create schedule", error);

const errorMessage = `Something went wrong. Please try again.`;
Comment on lines +98 to +100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling with error categorization.

While the current changes improve security by not exposing internal errors to users, consider categorizing known errors to provide more specific user feedback when appropriate.

Consider implementing error categorization like this:

- logger.error("Failed to create schedule", error);
+ logger.error("Failed to create schedule", { error, projectId: project.id });
+ 
+ let errorMessage = "Something went wrong. Please try again.";
+ if (error instanceof ServiceValidationError) {
+   errorMessage = error.message; // Safe to show validation errors
+ } else if (error instanceof PrismaClientKnownRequestError && error.code === "P2002") {
+   errorMessage = "A schedule with this deduplication key already exists.";
+ }

This approach:

  1. Adds context (projectId) to error logs
  2. Shows friendly messages for known error cases
  3. Maintains generic message for unexpected errors

Committable suggestion was skipped due to low confidence.

return redirectWithErrorMessage(
v3SchedulesPath({ slug: organizationSlug }, { slug: projectParam }),
request,
Expand Down
290 changes: 146 additions & 144 deletions apps/webapp/app/v3/services/upsertTaskSchedule.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Prisma, TaskSchedule } from "@trigger.dev/database";
import cronstrue from "cronstrue";
import { nanoid } from "nanoid";
import { $transaction, PrismaClientOrTransaction } from "~/db.server";
import { $transaction } from "~/db.server";
import { generateFriendlyId } from "../friendlyIdentifiers";
import { UpsertSchedule } from "../schedules";
import { calculateNextScheduledTimestamp } from "../utils/calculateNextSchedule.server";
Expand Down Expand Up @@ -31,124 +31,45 @@ export class UpsertTaskScheduleService extends BaseService {
const checkSchedule = new CheckScheduleService(this._prisma);
await checkSchedule.call(projectId, schedule);

const result = await $transaction(this._prisma, async (tx) => {
const deduplicationKey =
typeof schedule.deduplicationKey === "string" && schedule.deduplicationKey !== ""
? schedule.deduplicationKey
: nanoid(24);
const deduplicationKey =
typeof schedule.deduplicationKey === "string" && schedule.deduplicationKey !== ""
? schedule.deduplicationKey
: nanoid(24);

const existingSchedule = schedule.friendlyId
? await tx.taskSchedule.findUnique({
where: {
friendlyId: schedule.friendlyId,
},
})
: await tx.taskSchedule.findUnique({
where: {
projectId_deduplicationKey: {
projectId,
deduplicationKey,
},
const existingSchedule = schedule.friendlyId
? await this._prisma.taskSchedule.findUnique({
where: {
friendlyId: schedule.friendlyId,
},
})
: await this._prisma.taskSchedule.findUnique({
where: {
projectId_deduplicationKey: {
projectId,
deduplicationKey,
},
});
},
});

const result = await (async (tx) => {
if (existingSchedule) {
if (existingSchedule.type === "DECLARATIVE") {
throw new ServiceValidationError("Cannot update a declarative schedule");
}

return await this.#updateExistingSchedule(tx, existingSchedule, schedule, projectId);
return await this.#updateExistingSchedule(existingSchedule, schedule);
} else {
return await this.#createNewSchedule(tx, schedule, projectId, deduplicationKey);
return await this.#createNewSchedule(schedule, projectId, deduplicationKey);
}
});
})();
Comment on lines +54 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove unused parameter tx from the async function

The parameter tx in the async function is not used and can be removed to simplify the code.

Apply this diff to remove the unused parameter:

-    const result = await (async (tx) => {
+    const result = await (async () => {
       if (existingSchedule) {
         if (existingSchedule.type === "DECLARATIVE") {
           throw new ServiceValidationError("Cannot update a declarative schedule");
         }
         return await this.#updateExistingSchedule(existingSchedule, schedule);
       } else {
         return await this.#createNewSchedule(schedule, projectId, deduplicationKey);
       }
     })();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const result = await (async (tx) => {
if (existingSchedule) {
if (existingSchedule.type === "DECLARATIVE") {
throw new ServiceValidationError("Cannot update a declarative schedule");
}
return await this.#updateExistingSchedule(tx, existingSchedule, schedule, projectId);
return await this.#updateExistingSchedule(existingSchedule, schedule);
} else {
return await this.#createNewSchedule(tx, schedule, projectId, deduplicationKey);
return await this.#createNewSchedule(schedule, projectId, deduplicationKey);
}
});
})();
const result = await (async () => {
if (existingSchedule) {
if (existingSchedule.type === "DECLARATIVE") {
throw new ServiceValidationError("Cannot update a declarative schedule");
}
return await this.#updateExistingSchedule(existingSchedule, schedule);
} else {
return await this.#createNewSchedule(schedule, projectId, deduplicationKey);
}
})();


if (!result) {
throw new Error("Failed to create or update the schedule");
}

const { scheduleRecord, instances } = result;

return this.#createReturnObject(scheduleRecord, instances);
}

async #createNewSchedule(
tx: PrismaClientOrTransaction,
options: UpsertTaskScheduleServiceOptions,
projectId: string,
deduplicationKey: string
) {
const scheduleRecord = await tx.taskSchedule.create({
data: {
projectId,
friendlyId: generateFriendlyId("sched"),
taskIdentifier: options.taskIdentifier,
deduplicationKey,
userProvidedDeduplicationKey:
options.deduplicationKey !== undefined && options.deduplicationKey !== "",
generatorExpression: options.cron,
generatorDescription: cronstrue.toString(options.cron),
timezone: options.timezone ?? "UTC",
externalId: options.externalId ? options.externalId : undefined,
},
});

const registerNextService = new RegisterNextTaskScheduleInstanceService(tx);

//create the instances (links to environments)
let instances: InstanceWithEnvironment[] = [];
for (const environmentId of options.environments) {
const instance = await tx.taskScheduleInstance.create({
data: {
taskScheduleId: scheduleRecord.id,
environmentId,
},
include: {
environment: {
include: {
orgMember: {
include: {
user: true,
},
},
},
},
},
});

await registerNextService.call(instance.id);

instances.push(instance);
throw new ServiceValidationError("Failed to create or update schedule");
}

return { scheduleRecord, instances };
}
const { scheduleRecord } = result;

async #updateExistingSchedule(
tx: PrismaClientOrTransaction,
existingSchedule: TaskSchedule,
options: UpsertTaskScheduleServiceOptions,
projectId: string
) {
//update the schedule
const scheduleRecord = await tx.taskSchedule.update({
where: {
id: existingSchedule.id,
},
data: {
generatorExpression: options.cron,
generatorDescription: cronstrue.toString(options.cron),
timezone: options.timezone ?? "UTC",
externalId: options.externalId ? options.externalId : null,
},
});

const scheduleHasChanged =
scheduleRecord.generatorExpression !== existingSchedule.generatorExpression ||
scheduleRecord.timezone !== existingSchedule.timezone;

// find the existing instances
const existingInstances = await tx.taskScheduleInstance.findMany({
const instances = await this._prisma.taskScheduleInstance.findMany({
where: {
taskScheduleId: scheduleRecord.id,
},
Expand All @@ -165,18 +86,35 @@ export class UpsertTaskScheduleService extends BaseService {
},
});

// create the new instances
const newInstances: InstanceWithEnvironment[] = [];
const updatingInstances: InstanceWithEnvironment[] = [];
return this.#createReturnObject(scheduleRecord, instances);
}

async #createNewSchedule(
options: UpsertTaskScheduleServiceOptions,
projectId: string,
deduplicationKey: string
) {
return await $transaction(this._prisma, async (tx) => {
const scheduleRecord = await tx.taskSchedule.create({
data: {
projectId,
friendlyId: generateFriendlyId("sched"),
taskIdentifier: options.taskIdentifier,
deduplicationKey,
userProvidedDeduplicationKey:
options.deduplicationKey !== undefined && options.deduplicationKey !== "",
generatorExpression: options.cron,
generatorDescription: cronstrue.toString(options.cron),
timezone: options.timezone ?? "UTC",
externalId: options.externalId ? options.externalId : undefined,
},
});

const registerNextService = new RegisterNextTaskScheduleInstanceService(tx);

for (const environmentId of options.environments) {
const existingInstance = existingInstances.find((i) => i.environmentId === environmentId);
//create the instances (links to environments)

if (existingInstance) {
// Update the existing instance
updatingInstances.push(existingInstance);
} else {
// Create a new instance
for (const environmentId of options.environments) {
const instance = await tx.taskScheduleInstance.create({
data: {
taskScheduleId: scheduleRecord.id,
Expand All @@ -195,39 +133,21 @@ export class UpsertTaskScheduleService extends BaseService {
},
});

newInstances.push(instance);
await registerNextService.call(instance.id);
}
}

// find the instances that need to be removed
const instancesToDeleted = existingInstances.filter(
(i) => !options.environments.includes(i.environmentId)
);

// delete the instances no longer selected
for (const instance of instancesToDeleted) {
await tx.taskScheduleInstance.delete({
where: {
id: instance.id,
},
});
}

const registerService = new RegisterNextTaskScheduleInstanceService(tx);

for (const instance of newInstances) {
await registerService.call(instance.id);
}

if (scheduleHasChanged) {
for (const instance of updatingInstances) {
await registerService.call(instance.id);
}
}
return { scheduleRecord };
});
}

const instances = await tx.taskScheduleInstance.findMany({
async #updateExistingSchedule(
existingSchedule: TaskSchedule,
options: UpsertTaskScheduleServiceOptions
) {
// find the existing instances
const existingInstances = await this._prisma.taskScheduleInstance.findMany({
where: {
taskScheduleId: scheduleRecord.id,
taskScheduleId: existingSchedule.id,
},
include: {
environment: {
Expand All @@ -242,7 +162,89 @@ export class UpsertTaskScheduleService extends BaseService {
},
});

return { scheduleRecord, instances };
return await $transaction(
this._prisma,
async (tx) => {
const scheduleRecord = await tx.taskSchedule.update({
where: {
id: existingSchedule.id,
},
data: {
generatorExpression: options.cron,
generatorDescription: cronstrue.toString(options.cron),
timezone: options.timezone ?? "UTC",
externalId: options.externalId ? options.externalId : null,
},
});

const scheduleHasChanged =
scheduleRecord.generatorExpression !== existingSchedule.generatorExpression ||
scheduleRecord.timezone !== existingSchedule.timezone;

// create the new instances
const newInstances: InstanceWithEnvironment[] = [];
const updatingInstances: InstanceWithEnvironment[] = [];

for (const environmentId of options.environments) {
const existingInstance = existingInstances.find((i) => i.environmentId === environmentId);

if (existingInstance) {
// Update the existing instance
updatingInstances.push(existingInstance);
} else {
// Create a new instance
const instance = await tx.taskScheduleInstance.create({
data: {
taskScheduleId: scheduleRecord.id,
environmentId,
},
include: {
environment: {
include: {
orgMember: {
include: {
user: true,
},
},
},
},
},
});

newInstances.push(instance);
}
}

// find the instances that need to be removed
const instancesToDeleted = existingInstances.filter(
(i) => !options.environments.includes(i.environmentId)
);
Comment on lines +219 to +221
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct variable name from instancesToDeleted to instancesToDelete

There's a typo in the variable name instancesToDeleted. It should be instancesToDelete for clarity and consistency.

Apply this diff to fix the typo:

     // find the instances that need to be removed
-    const instancesToDeleted = existingInstances.filter(
+    const instancesToDelete = existingInstances.filter(
       (i) => !options.environments.includes(i.environmentId)
     );

     // delete the instances no longer selected
-    for (const instance of instancesToDeleted) {
+    for (const instance of instancesToDelete) {
       await tx.taskScheduleInstance.delete({
         where: {
           id: instance.id,
         },
       });
     }

Also applies to: 224-230


// delete the instances no longer selected
for (const instance of instancesToDeleted) {
await tx.taskScheduleInstance.delete({
where: {
id: instance.id,
},
});
}

const registerService = new RegisterNextTaskScheduleInstanceService(tx);

for (const instance of newInstances) {
await registerService.call(instance.id);
}

if (scheduleHasChanged) {
for (const instance of updatingInstances) {
await registerService.call(instance.id);
}
}

return { scheduleRecord };
},
{ timeout: 10_000 }
);
}

#createReturnObject(taskSchedule: TaskSchedule, instances: InstanceWithEnvironment[]) {
Expand Down
2 changes: 1 addition & 1 deletion internal-packages/database/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ This is the internal database package for the Trigger.dev project. It exports a
### How to add a new index on a large table

1. Modify the Prisma.schema with a single index change (no other changes, just one index at a time)
2. Create a Prisma migration using `cd packages/database && pnpm run db:migrate:dev --create-only`
2. Create a Prisma migration using `cd internal-packages/database && pnpm run db:migrate:dev --create-only`
3. Modify the SQL file: add IF NOT EXISTS to it and CONCURRENTLY:

```sql
Expand Down
2 changes: 2 additions & 0 deletions ...prisma/migrations/20241023154826_add_schedule_instance_id_index_to_task_run/migration.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- CreateIndex
CREATE INDEX CONCURRENTLY IF NOT EXISTS "TaskRun_scheduleInstanceId_idx" ON "TaskRun"("scheduleInstanceId");
1 change: 1 addition & 0 deletions internal-packages/database/prisma/schema.prisma
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1793,6 +1793,7 @@ model TaskRun {
@@index([projectId, taskIdentifier, status])
//Schedules
@@index([scheduleId])
@@index([scheduleInstanceId])
// Run page inspector
@@index([spanId])
@@index([parentSpanId])
Expand Down
4 changes: 2 additions & 2 deletions references/v3-catalog/src/management.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,8 @@ async function doTriggerUnfriendlyTaskId() {
}

// doRuns().catch(console.error);
doListRuns().catch(console.error);
// doListRuns().catch(console.error);
// doScheduleLists().catch(console.error);
// doSchedules().catch(console.error);
doSchedules().catch(console.error);
// doEnvVars().catch(console.error);
// doTriggerUnfriendlyTaskId().catch(console.error);