-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
perf(webapp): scope declarative schedule sync to the current environment #4577
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
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: improvement | ||
| --- | ||
|
|
||
| Make background worker registration cheaper for projects with many scheduled tasks by scoping declarative schedule reconciliation to the current environment and dropping redundant schedule lookups. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -655,9 +655,21 @@ export async function syncDeclarativeSchedules( | |
| where: { | ||
| type: "DECLARATIVE", | ||
| projectId: environment.projectId, | ||
| instances: { | ||
| some: { | ||
| environmentId: environment.id, | ||
| }, | ||
| }, | ||
|
Comment on lines
+658
to
+662
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. 🟡 Leftover schedule records with no environments are no longer cleaned up Declarative schedules that have no environments attached at all are now excluded from the list that gets reviewed for deletion ( How the reconcile loop lost its orphan-cleanup pathPreviously the query fetched every If orphan cleanup is still desired, it needs a separate scoped pass (e.g. a periodic cleanup or a narrow query for Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| }, | ||
| include: { | ||
| instances: true, | ||
| select: { | ||
| id: true, | ||
| friendlyId: true, | ||
| taskIdentifier: true, | ||
| instances: { | ||
| select: { | ||
| environmentId: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
|
|
@@ -764,16 +776,9 @@ export async function syncDeclarativeSchedules( | |
|
|
||
| //Delete instances for this environment | ||
| //Delete schedules that have no instances left | ||
| const potentiallyDeletableSchedules = await prisma.taskSchedule.findMany({ | ||
| where: { | ||
| id: { | ||
| in: boundedIn(Array.from(missingSchedules)), | ||
| }, | ||
| }, | ||
| include: { | ||
| instances: true, | ||
| }, | ||
| }); | ||
| const potentiallyDeletableSchedules = existingDeclarativeSchedules.filter((schedule) => | ||
| missingSchedules.has(schedule.id) | ||
| ); | ||
|
Comment on lines
+779
to
+781
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. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift 🧩 Analysis chain🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
ast-grep outline apps/webapp/app/v3/services/createBackgroundWorker.server.ts \
--match 'syncDeclarativeSchedules' --view expanded
rg -n -C 12 '\bsyncDeclarativeSchedules\s*\(' \
apps/webapp --glob '*.ts' --glob '*.tsx'
rg -n -C 12 \
'taskScheduleInstance\.(create|createMany|update|updateMany|delete|deleteMany)|taskSchedule\.(delete|deleteMany)|\$transaction|Serializable|mutex|lock' \
apps/webapp --glob '*.ts' --glob '*.tsx'Repository: triggerdotdev/trigger.dev Length of output: 50382 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
file="apps/webapp/app/v3/services/createBackgroundWorker.server.ts"
printf '%s\n' '--- target function and call sites ---'
rg -n -C 8 'syncDeclarativeSchedules|potentiallyDeletableSchedules|canDeleteSchedule|missingSchedules' "$file"
printf '%s\n' '--- callers across the application ---'
rg -l '\bsyncDeclarativeSchedules\s*\(' apps/webapp --glob '*.ts' --glob '*.tsx' |
while read -r f; do
printf '\n### %s\n' "$f"
rg -n -C 15 '\bsyncDeclarativeSchedules\s*\(' "$f"
done
printf '%s\n' '--- schedule mutation sites ---'
rg -n -C 6 \
'taskScheduleInstance\.(create|createMany|update|updateMany|delete|deleteMany)|taskSchedule\.(create|update|delete|deleteMany)' \
apps/webapp --glob '*.ts' --glob '*.tsx'Repository: triggerdotdev/trigger.dev Length of output: 50381 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- reconciliation implementation ---'
sed -n '642,825p' apps/webapp/app/v3/services/createBackgroundWorker.server.ts
printf '%s\n' '--- deployment synchronization callers ---'
sed -n '120,235p' apps/webapp/app/v3/services/createBackgroundWorker.server.ts
sed -n '150,235p' apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts
sed -n '165,230p' apps/webapp/app/v3/services/changeCurrentDeployment.server.ts
printf '%s\n' '--- all transaction/lock context in the three service files ---'
rg -n -C 10 '\$transaction|Serializable|mutex|lock|upsertTaskSchedule|taskScheduleInstance\.create' \
apps/webapp/app/v3/services/createBackgroundWorker.server.ts \
apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts \
apps/webapp/app/v3/services/changeCurrentDeployment.server.ts \
apps/webapp/app/v3/services/upsertTaskSchedule.server.tsRepository: triggerdotdev/trigger.dev Length of output: 26106 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- production instance creation paths ---'
rg -n -C 12 'taskScheduleInstance\.(create|createMany)|instances:\s*\{\s*create' \
apps/webapp/app --glob '*.ts' --glob '*.tsx'
printf '%s\n' '--- CheckScheduleService definition and usages ---'
rg -n -C 20 'class CheckScheduleService|CheckScheduleService|checkSchedule\.call' \
apps/webapp/app --glob '*.ts' --glob '*.tsx'
printf '%s\n' '--- imperative schedule upsert implementation ---'
sed -n '1,245p' apps/webapp/app/v3/services/upsertTaskSchedule.server.tsRepository: triggerdotdev/trigger.dev Length of output: 50381 🏁 Script executed: python3 - <<'PY'
from dataclasses import dataclass, field
`@dataclass`
class Schedule:
id: str
instances: set[str] = field(default_factory=set)
def snapshot(schedules, project_id, environment_id):
# Matches the `findMany` relation predicate in syncDeclarativeSchedules.
return [
Schedule(s.id, set(s.instances))
for s in schedules
if s.instances and environment_id in s.instances
]
def plan_deletion(snapshot_rows, missing_ids, environment_id):
delete = []
detach = []
for row in snapshot_rows:
if row.id not in missing_ids:
continue
can_delete = (
not row.instances
or all(instance == environment_id for instance in row.instances)
)
if can_delete:
delete.append(row.id)
elif environment_id in row.instances:
detach.append(row.id)
return delete, detach
# A newer sync for the same environment can complete after an older sync reads.
schedules = [Schedule("sched-1", {"env-a"})]
old_snapshot = snapshot(schedules, "project-1", "env-a")
newer_sync_desired_ids = {"sched-1"} # newer sync includes the schedule
old_sync_missing_ids = {"sched-1"} # older sync does not include it
old_delete, old_detach = plan_deletion(
old_snapshot, old_sync_missing_ids, "env-a"
)
print("same-environment stale snapshot:", {
"old_delete": old_delete,
"old_detach": old_detach,
"newer_sync_desired": newer_sync_desired_ids,
})
# A different environment cannot attach to this schedule through this function
# unless it already has an instance, because the initial query requires it.
print("cross-environment attach from absent instance:", {
"env-b_snapshot": [s.id for s in snapshot(schedules, "env-b", "env-b")],
"can_attach_existing_schedule": False,
})
PYRepository: triggerdotdev/trigger.dev Length of output: 386 🏁 Script executed: python3 - <<'PY'
from dataclasses import dataclass, field
`@dataclass`
class Schedule:
id: str
instances: set[str] = field(default_factory=set)
def snapshot(schedules, environment_id):
return [
Schedule(s.id, set(s.instances))
for s in schedules
if environment_id in s.instances
]
def plan_deletion(rows, missing_ids, environment_id):
delete = []
detach = []
for row in rows:
if row.id not in missing_ids:
continue
can_delete = not row.instances or all(
instance == environment_id for instance in row.instances
)
if can_delete:
delete.append(row.id)
elif environment_id in row.instances:
detach.append(row.id)
return delete, detach
schedules = [Schedule("sched-1", {"env-a"})]
old_snapshot = snapshot(schedules, "env-a")
print("same-environment stale snapshot:", plan_deletion(
old_snapshot, {"sched-1"}, "env-a"
))
print("cross-environment snapshot:", snapshot(schedules, "env-b"))
PYRepository: triggerdotdev/trigger.dev Length of output: 245 Serialize declarative schedule reconciliation before deletion. Concurrent syncs for the same environment can use a stale |
||
|
|
||
| const scheduleIdsToDelete: string[] = []; | ||
| const scheduleIdsToDetachFromEnvironment: string[] = []; | ||
|
|
||
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.
🔍 Registering the wrong environment's schedule instance when a schedule spans environments
In the update branch the code re-reads all instances of the matched schedule and registers
schedule.instances.at(0)(apps/webapp/app/v3/services/createBackgroundWorker.server.ts:721-723). The nestedinstancesselection is not filtered byenvironmentId, so if a declarative schedule ever has instances in more than one environment, the instance registered may belong to a different environment than the one being deployed. This is pre-existing behavior (unchanged by the PR, since the nested select still returns all instances, which is also what keeps theevery(instance => instance.environmentId === environment.id)deletion check correct), but it is worth confirming that declarative schedules are always single-environment in practice.(Refers to lines 721-723)
Was this helpful? React with 👍 or 👎 to provide feedback.