Skip to content

Commit e46744e

Browse files
fix(worker): publish repo-index job pointer in onStarted before superseding
trackLatestJob removes a resource's previous job right after onStarted, which assumes the parent's latest...JobId pointer has already moved. repo-index wrote latestIndexingJobId inside process instead, so an abort or DB error before that write could leave the pointer on a removed job. - Move the pointer write to an onStarted hook (updateMany keyed by repo id), matching the other latestPerResource workloads. - Reject a latestPerResource workload without onStarted at registration so the contract cannot be silently skipped by a future workload. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 5040f09 commit e46744e

5 files changed

Lines changed: 78 additions & 45 deletions

File tree

‎packages/backend/src/jobManager.test.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,27 @@ describe("BullMQJobManager lifecycle", () => {
157157
);
158158
});
159159

160+
test("rejects a latestPerResource workload that does not publish its job id in onStarted", () => {
161+
const manager = new BullMQJobManager({} as Redis);
162+
const workload = createWorkload();
163+
workload.queueSpec.jobOptions = {
164+
...workload.queueSpec.jobOptions,
165+
retention: { mode: "latestPerResource", maxAgeSeconds: 60 },
166+
};
167+
168+
expect(() => manager.register(workload)).toThrow(
169+
/must publish its job id to the parent resource in onStarted/,
170+
);
171+
expect(() =>
172+
manager.register(
173+
createWorkload({
174+
queueSpec: workload.queueSpec,
175+
onStarted: vi.fn(),
176+
}),
177+
),
178+
).not.toThrow();
179+
});
180+
160181
test("delegates enqueueing to BullMQClient and returns its job id", async () => {
161182
const manager = new BullMQJobManager({} as Redis);
162183
const workload = createWorkload();

‎packages/backend/src/jobManager.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,14 @@ export class BullMQJobManager implements JobManager {
4242
if (this.workloads.has(name)) {
4343
throw new Error(`Workload "${name}" is already registered`);
4444
}
45+
if (
46+
workload.queueSpec.jobOptions.retention.mode === "latestPerResource"
47+
&& !workload.onStarted
48+
) {
49+
throw new Error(
50+
`Workload "${name}" uses latestPerResource retention and must publish its job id to the parent resource in onStarted`,
51+
);
52+
}
4553
this.workloads.set(name, workload);
4654
}
4755

@@ -151,8 +159,10 @@ export class BullMQJobManager implements JobManager {
151159

152160
const process = async (signal: AbortSignal) => {
153161
await workload.onStarted?.(lifecycleContext);
154-
// After onStarted so the parent's `latest...JobId` pointer has
155-
// moved to this job before its predecessor is removed.
162+
// Contract: `latestPerResource` workloads publish this job's id to
163+
// their parent's `latest...JobId` pointer in onStarted (enforced in
164+
// `register`), so the pointer has moved before the superseded job
165+
// is removed.
156166
await this.trackLatestJob(spec, job);
157167
return workload.process({
158168
...lifecycleContext,

‎packages/backend/src/repoIndexWorkload.test.ts‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,18 +19,10 @@ const repoFindUnique = vi.fn();
1919
const repoUpdate = vi.fn();
2020
const repoUpdateMany = vi.fn();
2121

22-
const transaction = vi.fn(async (callback: (tx: unknown) => Promise<unknown>) =>
23-
callback({
24-
repo: {
25-
findUnique: repoFindUnique,
26-
update: repoUpdate,
27-
},
28-
}),
29-
);
30-
3122
const db = {
32-
$transaction: transaction,
3323
repo: {
24+
findUnique: repoFindUnique,
25+
update: repoUpdate,
3426
updateMany: repoUpdateMany,
3527
},
3628
} as unknown as PrismaClient;
@@ -121,10 +113,20 @@ describe("repoIndexWorkload", () => {
121113
});
122114
});
123115

116+
test("publishes the job as the repository's latest indexing job when it starts", async () => {
117+
await workload.onStarted!(lifecycleContext);
118+
119+
expect(repoUpdateMany).toHaveBeenCalledWith({
120+
where: { id: 42 },
121+
data: { latestIndexingJobId: "job-1" },
122+
});
123+
});
124+
124125
test("skips an INDEX job when the repository no longer exists", async () => {
125126
await workload.process(processContext);
126127

127128
expect(repoUpdate).not.toHaveBeenCalled();
129+
expect(repoUpdateMany).not.toHaveBeenCalled();
128130
expect(lifecycleLogger.debug).toHaveBeenCalledWith(
129131
"Skipping INDEX job for repo 42: repository no longer exists",
130132
);

‎packages/backend/src/repoIndexWorkload.ts‎

Lines changed: 30 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,22 @@ export const createRepoIndexWorkload = ({
2525
queueSpec: REPO_INDEX_QUEUE,
2626
concurrency: settings.maxRepoIndexingJobConcurrency,
2727
executionLock: REPOSITORY_EXECUTION_LOCK,
28-
process: async ({ data, jobId, signal }) => {
28+
onStarted: async ({ data: { repoId }, jobId }) => {
29+
await db.repo.updateMany({
30+
where: {
31+
id: repoId,
32+
},
33+
data: {
34+
latestIndexingJobId: jobId,
35+
},
36+
});
37+
},
38+
process: async ({ data, signal }) => {
2939
signal.throwIfAborted();
3040

3141
const start = await prepareRepoIndexJob({
3242
db,
3343
repoId: data.repoId,
34-
jobId,
3544
});
3645

3746
if (start.action === "skip") {
@@ -119,45 +128,33 @@ type RepoIndexStartDecision =
119128
const prepareRepoIndexJob = async ({
120129
db,
121130
repoId,
122-
jobId,
123131
}: {
124132
db: PrismaClient;
125133
repoId: number;
126-
jobId: string;
127-
}): Promise<RepoIndexStartDecision> =>
128-
db.$transaction(async (tx) => {
129-
const repo = await tx.repo.findUnique({
130-
where: { id: repoId },
131-
include: {
132-
connections: {
133-
include: {
134-
connection: true,
135-
},
134+
}): Promise<RepoIndexStartDecision> => {
135+
const repo = await db.repo.findUnique({
136+
where: { id: repoId },
137+
include: {
138+
connections: {
139+
include: {
140+
connection: true,
136141
},
137142
},
138-
});
139-
140-
if (!repo) {
141-
return {
142-
action: "skip",
143-
reason: "repository no longer exists",
144-
};
145-
}
146-
147-
await tx.repo.update({
148-
where: {
149-
id: repoId,
150-
},
151-
data: {
152-
latestIndexingJobId: jobId,
153-
},
154-
});
143+
},
144+
});
155145

146+
if (!repo) {
156147
return {
157-
action: "run",
158-
repo,
148+
action: "skip",
149+
reason: "repository no longer exists",
159150
};
160-
});
151+
}
152+
153+
return {
154+
action: "run",
155+
repo,
156+
};
157+
};
161158

162159
const indexRepository = async (
163160
db: PrismaClient,

‎packages/shared/src/queue.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ export type JobRetention =
4848
| {
4949
// Keeps only the most recent job for each resource, keyed by the queue's
5050
// deduplication id. When a job starts, the job it supersedes is removed.
51+
// Workloads on these queues must publish the job id to their parent's
52+
// `latest...JobId` pointer in `onStarted`, so the pointer never
53+
// references a removed job.
5154
// `maxAgeSeconds` is an age-only backstop that reclaims jobs whose
5255
// resource stopped running (e.g. a deleted repo). It must exceed the
5356
// longest scheduler interval, or a resource's latest job can be

0 commit comments

Comments
 (0)