Skip to content

Commit 8470de5

Browse files
authored
fix: tolerate a deleted job row when recording the terminal state (#187)
When a job's row is deleted while it runs (cleanup routine, explicit delete, or a test truncating the table), the final transition's updateJob throws "Cannot update job, not found." Since execute() is fire-and-forget, that surfaced as an unhandled rejection that could crash and restart the engine fork (which in turn made scheduled jobs re-fire). Apply the terminal transition through a helper that swallows and logs such failures (recording the state of a job that no longer exists is safe to skip), and guard the dispatcher's fire-and-forget execute() with a .catch so a single job can never crash the engine.
1 parent e669e1b commit 8470de5

3 files changed

Lines changed: 54 additions & 5 deletions

File tree

packages/engine/src/execution/dispatcher.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,11 @@ export class Dispatcher {
6060
// because the execution is not awaited. This way we ensure that available slots
6161
// are correctly calculated.
6262
this.executorManager.queueJob(queue, job);
63-
// does not await for job execution.
64-
void this.executorManager.execute(queue, job);
63+
// does not await for job execution. Guard against any unexpected rejection so a single
64+
// job can never crash the engine with an unhandled promise rejection.
65+
void this.executorManager.execute(queue, job).catch((error: unknown) => {
66+
logger("Dispatcher").error(`Unexpected error executing job ${job.id}:`, error);
67+
});
6568
}
6669
}
6770

packages/engine/src/execution/executor-manager.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,30 @@ describe("ExecutorManager", () => {
261261
getJobSpy.mockRestore();
262262
});
263263

264+
sidequestTest("does not crash when the final transition fails (job row gone)", async ({ backend, config }) => {
265+
const queryConfig = await grantQueueConfig(backend, { name: "default", concurrency: 1 });
266+
const executorManager = new ExecutorManager(backend, config);
267+
268+
// RunTransition succeeds; the terminal transition fails because the row was deleted mid-run.
269+
// eslint-disable-next-line @typescript-eslint/unbound-method
270+
vi.mocked(JobTransitioner.apply)
271+
.mockImplementationOnce((_backend: Backend, job: JobData) => job)
272+
.mockImplementationOnce(() => {
273+
throw new Error("Cannot update job, not found.");
274+
});
275+
runMock.mockResolvedValue({
276+
__is_job_transition__: true,
277+
type: "completed",
278+
result: "ok",
279+
} satisfies CompletedResult);
280+
281+
// The fire-and-forget executor must not reject, and must free the job from the active set.
282+
await expect(executorManager.execute(queryConfig, jobData)).resolves.toBeUndefined();
283+
expect(executorManager.totalActiveWorkers()).toBe(0);
284+
285+
await executorManager.destroy();
286+
});
287+
264288
sidequestTest("should abort job execution on timeout", async ({ backend, config }) => {
265289
jobData = await backend.updateJob({ ...jobData, state: "claimed", claimed_at: new Date(), timeout: 100 });
266290

packages/engine/src/execution/executor-manager.ts

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
JobCanceled,
55
JobData,
66
JobTimeout,
7+
JobTransition,
78
JobTransitionFactory,
89
logger,
910
QueueConfig,
@@ -151,7 +152,7 @@ export class ExecutorManager {
151152
// The job ran to a conclusion and returned a state (even if a timeout/cancel was signaled);
152153
// respect it and transition accordingly.
153154
logger("Executor Manager").debug(`Job ${job.id} settled with result: ${inspect(result)}`);
154-
await JobTransitioner.apply(this.backend, job, JobTransitionFactory.create(result));
155+
await this.applyTerminalTransition(job, JobTransitionFactory.create(result));
155156
} catch (error: unknown) {
156157
isRunning = false;
157158
const err = error as Error;
@@ -162,10 +163,10 @@ export class ExecutorManager {
162163
const reason: unknown = controller.signal.reason;
163164
logger("Executor Manager").debug(`Job ${job.id} was hard-killed (${String(reason)}): ${err.message}`);
164165
const transition = reason instanceof JobTimeout ? new RetryTransition(reason) : new CancelTransition();
165-
await JobTransitioner.apply(this.backend, job, transition);
166+
await this.applyTerminalTransition(job, transition);
166167
} else {
167168
logger("Executor Manager").error(`Unhandled error while executing job ${job.id}: ${err.message}`);
168-
await JobTransitioner.apply(this.backend, job, new RetryTransition(err));
169+
await this.applyTerminalTransition(job, new RetryTransition(err));
169170
}
170171
} finally {
171172
isRunning = false;
@@ -177,6 +178,27 @@ export class ExecutorManager {
177178
}
178179
}
179180

181+
/**
182+
* Applies a job's final transition, tolerating the job row having disappeared.
183+
*
184+
* A job's row can be deleted while it runs (cleanup routine, an explicit delete, or a test
185+
* truncating the table). Recording its terminal state is then impossible and safe to skip. This
186+
* must never throw: `execute` is fire-and-forget, so an error here would surface as an unhandled
187+
* rejection.
188+
*
189+
* @param job The job being finalized.
190+
* @param transition The terminal transition to apply.
191+
*/
192+
private async applyTerminalTransition(job: JobData, transition: JobTransition): Promise<void> {
193+
try {
194+
await JobTransitioner.apply(this.backend, job, transition);
195+
} catch (error) {
196+
logger("Executor Manager").warn(
197+
`Could not record terminal state for job ${job.id} (it may no longer exist): ${error instanceof Error ? error.message : String(error)}`,
198+
);
199+
}
200+
}
201+
180202
/**
181203
* Destroys the runner pool and releases resources.
182204
*/

0 commit comments

Comments
 (0)