Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/quick-otters-dlq.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@trigger.dev/redis-worker": patch
---

Move unrecoverable items (unknown job, missing schema, or invalid payload) to the dead-letter queue instead of redelivering them indefinitely
118 changes: 118 additions & 0 deletions packages/redis-worker/src/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,124 @@ describe("SimpleQueue", () => {
}
});

redisTest(
"dequeue moves items with an unknown job to the DLQ",
{ timeout: 20_000 },
async ({ redisContainer }) => {
const redisOptions = {
host: redisContainer.getHost(),
port: redisContainer.getPort(),
password: redisContainer.getPassword(),
};

// Producer enqueues an item for the "test" job.
const producer = new SimpleQueue({
name: "test-invalid-job",
schema: {
test: z.object({
value: z.number(),
}),
},
redisOptions,
logger: new Logger("test", "log"),
});

// Consumer shares the same queue name but doesn't know about the "test" job.
const consumer = new SimpleQueue({
name: "test-invalid-job",
schema: {
other: z.object({
value: z.number(),
}),
},
redisOptions,
logger: new Logger("test", "log"),
});

try {
await producer.enqueue({
id: "1",
job: "test",
item: { value: 1 },
visibilityTimeoutMs: 2000,
});
expect(await consumer.size()).toBe(1);
expect(await consumer.sizeOfDeadLetterQueue()).toBe(0);

// Dequeue can't find a schema for the job, so it should be moved to the DLQ.
const dequeued = await consumer.dequeue(1);
expect(dequeued).toEqual([]);
expect(await consumer.size({ includeFuture: true })).toBe(0);
expect(await consumer.sizeOfDeadLetterQueue()).toBe(1);

// The item must not resurface on a subsequent dequeue.
expect(await consumer.dequeue(1)).toEqual([]);
} finally {
await producer.close();
await consumer.close();
}
}
);

redisTest(
"dequeue moves items with an unparseable payload to the DLQ",
{ timeout: 20_000 },
async ({ redisContainer }) => {
const redisOptions = {
host: redisContainer.getHost(),
port: redisContainer.getPort(),
password: redisContainer.getPassword(),
};

// Producer enqueues a numeric payload.
const producer = new SimpleQueue({
name: "test-invalid-payload",
schema: {
test: z.object({
value: z.number(),
}),
},
redisOptions,
logger: new Logger("test", "log"),
});

// Consumer expects a string payload for the same job, so validation fails.
const consumer = new SimpleQueue({
name: "test-invalid-payload",
schema: {
test: z.object({
value: z.string(),
}),
},
redisOptions,
logger: new Logger("test", "log"),
});

try {
await producer.enqueue({
id: "1",
job: "test",
item: { value: 1 },
visibilityTimeoutMs: 2000,
});
expect(await consumer.size()).toBe(1);
expect(await consumer.sizeOfDeadLetterQueue()).toBe(0);

// The payload fails schema validation, so it should be moved to the DLQ.
const dequeued = await consumer.dequeue(1);
expect(dequeued).toEqual([]);
expect(await consumer.size({ includeFuture: true })).toBe(0);
expect(await consumer.sizeOfDeadLetterQueue()).toBe(1);

// The item must not resurface on a subsequent dequeue.
expect(await consumer.dequeue(1)).toEqual([]);
} finally {
await producer.close();
await consumer.close();
}
}
);

redisTest("cleanup orphaned queue entries", { timeout: 20_000 }, async ({ redisContainer }) => {
const queue = new SimpleQueue({
name: "test-orphaned",
Expand Down
9 changes: 9 additions & 0 deletions packages/redis-worker/src/queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ export class SimpleQueue<TMessageCatalog extends MessageCatalogSchema> {
const parsedItem = JSON.parse(serializedItem) as any;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. I think the malformed-JSON case still needs the same DLQ treatment becase right now JSON.parse(serializedItem) runs before the new invalid-item checks. And if the queue hash contains a corrupt/non-JSON value, dequeue() will hit the outer catch and throw, so the item can remain in the queue and keep blocking/redelivering instead of moving to the DLQ.

Could you, please, wrap the parse for each item and move that item to the DLQ when parsing fails? A small regression test with a manually seeded items hash value like "not-json" would cover the remaining poison-message path.

if (typeof parsedItem.job !== "string") {
this.logger.error(`Invalid item in queue`, { queue: this.name, id, item: parsedItem });
await this.moveToDeadLetterQueue(id, "Invalid item in queue: 'job' is not a string");
continue;
}

Expand All @@ -214,6 +215,10 @@ export class SimpleQueue<TMessageCatalog extends MessageCatalogSchema> {
timestamp,
availableJobs: Object.keys(this.schema),
});
await this.moveToDeadLetterQueue(
id,
`Invalid item in queue: no schema found for job "${parsedItem.job}"`
);
continue;
}

Expand All @@ -228,6 +233,10 @@ export class SimpleQueue<TMessageCatalog extends MessageCatalogSchema> {
attempt: parsedItem.attempt,
timestamp,
});
await this.moveToDeadLetterQueue(
id,
`Invalid item in queue: payload failed schema validation: ${validatedItem.error.message}`
);
continue;
}

Expand Down