Skip to content

Commit 1a06ef6

Browse files
committed
fix(webapp): address CodeRabbit review on the task metadata cache
- TaskMetadataCache: interface → type alias, matching the repo's 'prefer type unless extending' standard. - syncTaskMetadataCache calls in createBackgroundWorker, createDeploymentBackgroundWorkerV3, and createDeploymentBackgroundWorkerV4 are now wrapped in tryCatch and log on failure. Matches the pattern in changeCurrentDeployment so a Redis blip can't strand a successful worker build or deploy promotion. - Back-fill assertion in 'cache miss falls through to PG' now polls with a bounded timeout instead of a fixed sleep, removing the CI flake surface. - Promotion assertion in 'ChangeCurrentDeploymentService promotes the env cache' now pre-clears the by-worker + env keys and asserts they're null before the call, so the test proves the service did the write.
1 parent bdaaf06 commit 1a06ef6

5 files changed

Lines changed: 74 additions & 27 deletions

File tree

apps/webapp/app/services/taskMetadataCache.server.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ export type TaskMetadataEntry = {
1010
queueName: string;
1111
};
1212

13-
export interface TaskMetadataCache {
13+
export type TaskMetadataCache = {
1414
getCurrent(envId: string, slug: string): Promise<TaskMetadataEntry | null>;
1515
getByWorker(workerId: string, slug: string): Promise<TaskMetadataEntry | null>;
1616
populateCurrent(envId: string, entries: TaskMetadataEntry[]): Promise<void>;
@@ -20,7 +20,7 @@ export interface TaskMetadataCache {
2020
/** Add a single field to the by-worker keyspace and refresh the hash TTL. */
2121
setByWorker(workerId: string, entry: TaskMetadataEntry): Promise<void>;
2222
invalidateCurrent(envId: string): Promise<void>;
23-
}
23+
};
2424

2525
export type RedisTaskMetadataCacheOptions = {
2626
redis: Redis;

apps/webapp/app/v3/services/createBackgroundWorker.server.ts

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -234,14 +234,26 @@ export class CreateBackgroundWorkerService extends BaseService {
234234
// worker by createdAt. Non-DEV (deploy-built) workers are not promoted
235235
// here — promotion writes the `:env:` keyspace later in
236236
// changeCurrentDeployment / createDeploymentBackgroundWorkerV3.
237+
// Wrap in tryCatch so a Redis blip can't break the post-cache side
238+
// effects below.
237239
if (workerTaskEntries && workerTaskEntries.length > 0) {
238-
await syncTaskMetadataCache(
239-
environment.id,
240-
backgroundWorker.id,
241-
environment.type === "DEVELOPMENT",
242-
workerTaskEntries,
243-
this._taskMetaCache
240+
const [metaCacheError] = await tryCatch(
241+
syncTaskMetadataCache(
242+
environment.id,
243+
backgroundWorker.id,
244+
environment.type === "DEVELOPMENT",
245+
workerTaskEntries,
246+
this._taskMetaCache
247+
)
244248
);
249+
250+
if (metaCacheError) {
251+
logger.error("Error syncing task metadata cache", {
252+
error: metaCacheError,
253+
backgroundWorker,
254+
environment,
255+
});
256+
}
245257
}
246258

247259
const [updateConcurrencyLimitsError] = await tryCatch(

apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV3.server.ts

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -168,15 +168,27 @@ export class CreateDeploymentBackgroundWorkerServiceV3 extends BaseService {
168168
}
169169

170170
// V3 promotes the deployment immediately above, so this worker is now
171-
// current for the env — write both keyspaces.
171+
// current for the env — write both keyspaces. Wrap in tryCatch so a
172+
// Redis blip can't strand the post-cache side effects below (waiting
173+
// runs flush, alerts, timeout cancellation).
172174
if (workerTaskEntries.length > 0) {
173-
await syncTaskMetadataCache(
174-
environment.id,
175-
backgroundWorker.id,
176-
true,
177-
workerTaskEntries,
178-
this._taskMetaCache
175+
const [metaCacheError] = await tryCatch(
176+
syncTaskMetadataCache(
177+
environment.id,
178+
backgroundWorker.id,
179+
true,
180+
workerTaskEntries,
181+
this._taskMetaCache
182+
)
179183
);
184+
185+
if (metaCacheError) {
186+
logger.error("Error syncing task metadata cache on deployment", {
187+
error: metaCacheError,
188+
deploymentId: deployment.id,
189+
workerId: backgroundWorker.id,
190+
});
191+
}
180192
}
181193

182194
try {

apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -152,14 +152,27 @@ export class CreateDeploymentBackgroundWorkerServiceV4 extends BaseService {
152152
// only the `task-meta:by-worker:{workerId}` keyspace so locked-version
153153
// triggers against this build hit the cache. Promotion (which writes the
154154
// env keyspace) happens later via finalizeDeployment → changeCurrentDeployment.
155+
// Wrap in tryCatch so a Redis blip can't break the deployment state
156+
// machine — without this the build would stall in BUILDING and only
157+
// surface as a misleading "Indexing timed out".
155158
if (workerTaskEntries && workerTaskEntries.length > 0) {
156-
await syncTaskMetadataCache(
157-
environment.id,
158-
backgroundWorker.id,
159-
false,
160-
workerTaskEntries,
161-
this._taskMetaCache
159+
const [metaCacheError] = await tryCatch(
160+
syncTaskMetadataCache(
161+
environment.id,
162+
backgroundWorker.id,
163+
false,
164+
workerTaskEntries,
165+
this._taskMetaCache
166+
)
162167
);
168+
169+
if (metaCacheError) {
170+
logger.error("Error syncing task metadata cache on build", {
171+
error: metaCacheError,
172+
deploymentId: deployment.id,
173+
workerId: backgroundWorker.id,
174+
});
175+
}
163176
}
164177

165178
const [schedulesError] = await tryCatch(

apps/webapp/test/engine/triggerTask.test.ts

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1294,10 +1294,12 @@ describe("DefaultQueueManager task metadata cache", () => {
12941294
assertNonNullable(result);
12951295
expect((result.run.annotations as { taskKind?: string } | null)?.taskKind).toBe("STANDARD");
12961296

1297-
// Back-fill is fire-and-forget; allow a turn of the event loop for it to land.
1298-
await setTimeout(50);
1299-
1300-
const backfilled = await cache.getCurrent(environment.id, taskIdentifier);
1297+
// Back-fill is fire-and-forget; poll with a bounded timeout to avoid CI flakes.
1298+
let backfilled = await cache.getCurrent(environment.id, taskIdentifier);
1299+
for (let i = 0; i < 40 && !backfilled; i++) {
1300+
await setTimeout(25);
1301+
backfilled = await cache.getCurrent(environment.id, taskIdentifier);
1302+
}
13011303
expect(backfilled).not.toBeNull();
13021304
expect(backfilled?.triggerSource).toBe("STANDARD");
13031305
expect(backfilled?.queueName).toBe(`task/${taskIdentifier}`);
@@ -1495,14 +1497,22 @@ describe("DefaultQueueManager task metadata cache", () => {
14951497
const redis = new Redis(redisOptions);
14961498
const cache = new RedisTaskMetadataCache({ redis });
14971499

1500+
// Pre-clear any pre-existing cache state so the assertions below prove
1501+
// the rollback service did the write — not some other path. The test
1502+
// helpers don't currently populate the cache, but pre-clearing keeps the
1503+
// test bulletproof against future helper changes.
1504+
await redis.del(`task-meta:by-worker:${workerA.worker.id}`);
1505+
await redis.del(`task-meta:env:${environment.id}`);
1506+
expect(await cache.getByWorker(workerA.worker.id, taskIdentifier)).toBeNull();
1507+
expect(await cache.getCurrent(environment.id, taskIdentifier)).toBeNull();
1508+
14981509
// Manually rollback to A to exercise the cache-write side effect.
14991510
const service = new ChangeCurrentDeploymentService(prisma, undefined, cache);
15001511
await service.call(workerA.deployment, "rollback", true /* disableVersionCheck */);
15011512

1502-
// Allow the awaited cache write to settle.
1513+
// Both keyspaces should now reflect workerA.
15031514
const entry = await cache.getCurrent(environment.id, taskIdentifier);
15041515
expect(entry).not.toBeNull();
1505-
// by-worker keyspace also written by syncTaskMetadataCache(isCurrent=true)
15061516
const byWorkerEntry = await cache.getByWorker(workerA.worker.id, taskIdentifier);
15071517
expect(byWorkerEntry).not.toBeNull();
15081518
expect(byWorkerEntry?.queueName).toBe(`task/${taskIdentifier}`);

0 commit comments

Comments
 (0)