Skip to content

Commit 09a1a3b

Browse files
committed
fix(webapp): reject locked triggers when the task is missing on that worker, clear stale cache on zero-task deploys
Two CodeRabbit findings: 1. The locked + queue-override branch silently accepted a missing task slug on the locked worker version, leaving `taskKind` / `ttl` as undefined instead of erroring. Now matches the no-override branch and throws a ServiceValidationError if the task isn't on the locked worker. 2. The cache populate methods short-circuited on empty entries, leaving stale hashes in Redis when a worker registered or got promoted with zero tasks. The Lua scripts already handle the empty-entries case (DEL + no HSET), so just drop the gates at the cache class and at the four call sites so promotion correctly clears prior data.
1 parent 344135a commit 09a1a3b

5 files changed

Lines changed: 26 additions & 20 deletions

File tree

apps/webapp/app/runEngine/concerns/queues.server.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,10 +124,17 @@ export class DefaultQueueManager implements QueueManager {
124124
request.taskId
125125
);
126126

127+
if (!lockedMeta) {
128+
throw new ServiceValidationError(
129+
`Task '${request.taskId}' not found on locked version '${lockedBackgroundWorker.version ?? "<unknown>"
130+
}'.`
131+
);
132+
}
133+
127134
if (request.body.options?.ttl === undefined) {
128-
taskTtl = lockedMeta?.ttl ?? undefined;
135+
taskTtl = lockedMeta.ttl ?? undefined;
129136
}
130-
taskKind = lockedMeta?.triggerSource;
137+
taskKind = lockedMeta.triggerSource;
131138
} else {
132139
// No queue override - resolve default queue + TTL + triggerSource via cache,
133140
// falling back to a single BackgroundWorkerTask lookup on miss.

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -267,8 +267,10 @@ export class RedisTaskMetadataCache implements TaskMetadataCache {
267267
workerId: string,
268268
entries: TaskMetadataEntry[]
269269
): Promise<void> {
270-
if (entries.length === 0) return;
271270
try {
271+
// Always invoke the script — empty `entries` is valid and causes both
272+
// keyspaces to be cleared (DEL + no HSET), which is the right behavior
273+
// when promoting a worker with no tasks.
272274
const argv: string[] = [
273275
String(this.currentEnvTtlSeconds),
274276
String(this.byWorkerTtlSeconds),
@@ -291,8 +293,8 @@ export class RedisTaskMetadataCache implements TaskMetadataCache {
291293
}
292294

293295
async populateByWorker(workerId: string, entries: TaskMetadataEntry[]): Promise<void> {
294-
if (entries.length === 0) return;
295296
try {
297+
// Always invoke the script — empty `entries` clears the keyspace.
296298
const argv: string[] = [String(this.byWorkerTtlSeconds)];
297299
for (const entry of entries) {
298300
argv.push(entry.slug, encode(entry));

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -234,8 +234,9 @@ export class CreateBackgroundWorkerService extends BaseService {
234234
// here — promotion writes the `:env:` keyspace later in
235235
// changeCurrentDeployment / createDeploymentBackgroundWorkerV3.
236236
// Cache calls log+swallow internally, so a Redis blip can't break
237-
// anything else here.
238-
if (workerTaskEntries && workerTaskEntries.length > 0) {
237+
// anything else here. Empty `workerTaskEntries` is intentional — the
238+
// populate methods clear stale hashes for zero-task deploys.
239+
if (workerTaskEntries) {
239240
if (environment.type === "DEVELOPMENT") {
240241
await this._taskMetaCache.populateByCurrentWorker(
241242
environment.id,

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

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -168,14 +168,13 @@ export class CreateDeploymentBackgroundWorkerServiceV3 extends BaseService {
168168

169169
// V3 promotes the deployment immediately above, so this worker is now
170170
// current for the env — write both keyspaces atomically. Cache calls
171-
// log+swallow internally.
172-
if (workerTaskEntries.length > 0) {
173-
await this._taskMetaCache.populateByCurrentWorker(
174-
environment.id,
175-
backgroundWorker.id,
176-
workerTaskEntries
177-
);
178-
}
171+
// log+swallow internally. Empty `workerTaskEntries` is intentional: the
172+
// populate methods clear stale hashes for zero-task deploys.
173+
await this._taskMetaCache.populateByCurrentWorker(
174+
environment.id,
175+
backgroundWorker.id,
176+
workerTaskEntries
177+
);
179178

180179
try {
181180
//send a notification that a new worker has been created

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -152,12 +152,9 @@ export class CreateDeploymentBackgroundWorkerServiceV4 extends BaseService {
152152
// triggers against this build hit the cache. Promotion (which writes the
153153
// env keyspace) happens later via finalizeDeployment → changeCurrentDeployment.
154154
// Cache calls log+swallow internally, so a Redis blip can't stall the
155-
// deployment state machine.
156-
if (workerTaskEntries && workerTaskEntries.length > 0) {
157-
await this._taskMetaCache.populateByWorker(
158-
backgroundWorker.id,
159-
workerTaskEntries
160-
);
155+
// deployment state machine. Empty entries clears stale hashes.
156+
if (workerTaskEntries) {
157+
await this._taskMetaCache.populateByWorker(backgroundWorker.id, workerTaskEntries);
161158
}
162159

163160
const [schedulesError] = await tryCatch(

0 commit comments

Comments
 (0)