Repository navigation
chore: Adding protected DLQ props getter - #443
Merged
CarlosGamero merged 9 commits intoMay 26, 2026
Merged
Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 23 minutes and 48 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR models queue and DLQ identities as structured resources, makes initSqs optionally return undefined (requiring ARN when present), and updates services, consumers, publishers, fakes, exports, and tests to use the new resources. ChangesDLQ Resource and InitSqs Contract Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sqs/lib/sqs/AbstractSqsConsumer.ts (1)
346-349:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard DLQ setup on unresolved base init and reset stale DLQ handle.
super.init()can now exit without queue initialization; this method still proceeds, and_deadLetterQueuealso isn’t cleared when DLQ init returns no result. That can preserve stale DLQ routing across re-inits.Suggested fix
override async init(): Promise<void> { await super.init() + if (!this.isInitted) { + this._deadLetterQueue = undefined + return + } await this.initDeadLetterQueue() } protected async initDeadLetterQueue() { + this._deadLetterQueue = undefined if (!this.deadLetterQueueOptions) return @@ const result = await initSqs(this.sqsClient, locatorConfig, creationConfig, this.isFifoQueue) if (!result) return @@ this._deadLetterQueue = { url: result.queueUrl, arn: result.queueArn } }Also applies to: 363-363, 377-377
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sqs/lib/sqs/AbstractSqsConsumer.ts` around lines 346 - 349, super.init() can return without initializing the base queue so calling initDeadLetterQueue() unguarded can preserve stale routing; change override async init() to first ensure the base init actually created the queue (e.g. check the instance’s queue handle/state produced by super.init(), such as this._queue / this.queueUrl / similar) before calling initDeadLetterQueue(), and when calling initDeadLetterQueue() capture its return and only assign it to this._deadLetterQueue if truthy; if initDeadLetterQueue() returns no result, explicitly clear this._deadLetterQueue (set to undefined/null) to avoid stale DLQ handles; apply the same guard-and-clear pattern to the other init overrides referenced in this class (the other override implementations noted in the review).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/sqs/lib/sqs/AbstractSqsService.ts`:
- Around line 120-124: The early-return when result is falsy leaves stale values
in this.queueName/this.queueUrl/this.queueArn; update the init flow in
AbstractSqsService to explicitly clear these fields (set to
undefined/null/empty) before returning on !result so a failed/unresolved init
cannot reuse prior queueName/queueUrl/queueArn; locate the block that checks if
(!result) and add clearing of this.queueName, this.queueUrl, and this.queueArn
immediately before the return.
---
Outside diff comments:
In `@packages/sqs/lib/sqs/AbstractSqsConsumer.ts`:
- Around line 346-349: super.init() can return without initializing the base
queue so calling initDeadLetterQueue() unguarded can preserve stale routing;
change override async init() to first ensure the base init actually created the
queue (e.g. check the instance’s queue handle/state produced by super.init(),
such as this._queue / this.queueUrl / similar) before calling
initDeadLetterQueue(), and when calling initDeadLetterQueue() capture its return
and only assign it to this._deadLetterQueue if truthy; if initDeadLetterQueue()
returns no result, explicitly clear this._deadLetterQueue (set to
undefined/null) to avoid stale DLQ handles; apply the same guard-and-clear
pattern to the other init overrides referenced in this class (the other override
implementations noted in the review).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c7fd6b8-beae-410b-b2d9-8d110ab0d547
📒 Files selected for processing (6)
packages/sqs/lib/sqs/AbstractSqsConsumer.tspackages/sqs/lib/sqs/AbstractSqsService.tspackages/sqs/lib/utils/sqsInitter.tspackages/sqs/test/consumers/SqsPermissionConsumer.startupResourcePolling.spec.tspackages/sqs/test/consumers/SqsPermissionConsumer.tspackages/sqs/test/consumers/SqsPermissionConsumerFifo.ts
kibertoad
approved these changes
May 25, 2026
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/sqs/lib/sqs/AbstractSqsConsumer.ts (1)
347-360:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset DLQ resource to avoid stale failover routing across re-init/close.
_deadLetterQueueis never cleared when DLQ init is skipped/falsy (Line 359) and also not cleared on close (Line 407). A restarted/reconfigured consumer can keep sending failed messages to an old DLQ URL.Proposed fix
protected async initDeadLetterQueue() { + // Prevent stale DLQ routing across re-init attempts. + this._deadLetterQueue = undefined if (!this.deadLetterQueueOptions) return @@ const result = await initSqs(this.sqsClient, locatorConfig, creationConfig, this.isFifoQueue) if (!result) return @@ this._deadLetterQueue = { url: result.queueUrl, arn: result.queueArn } } public override async close(abort?: boolean): Promise<void> { + this._deadLetterQueue = undefined await super.close() await this.stopExistingConsumers(abort ?? false) }Also applies to: 407-410
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sqs/lib/sqs/AbstractSqsConsumer.ts` around lines 347 - 360, The _deadLetterQueue field is left stale when initDeadLetterQueue exits early or when DLQ init is skipped, causing rerouted failures to an old DLQ; update initDeadLetterQueue to explicitly clear this._deadLetterQueue when deadLetterQueueOptions is falsy and also when initSqs returns no result (after the deleteSqs/initSqs calls) so the consumer no longer holds an outdated URL, and add a clear of this._deadLetterQueue in the class close method (the same place other shutdown cleanup happens) to ensure re-initialized consumers don’t reuse a previous DLQ; reference initDeadLetterQueue, this._deadLetterQueue, initSqs, deleteSqs, and the close method when applying the change.packages/sqs/lib/sqs/AbstractSqsPublisher.ts (1)
107-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard publish flow when init completes without queue resource.
With non-blocking init,
this.init()can resolve whileisInittedis still false. The method continues andthis.queueaccess can throw; then Line 175/176 can throw again while building error details, masking the original failure.Proposed fix
if (!this.isInitted) { // avoid multiple concurrent inits if (!this.initPromise) { this.initPromise = this.init() } await this.initPromise this.initPromise = undefined + if (!this.isInitted) { + throw new InternalError({ + message: 'Queue is not ready yet', + errorCode: 'SQS_QUEUE_NOT_READY', + details: { publisher: this.constructor.name }, + }) + } } @@ } catch (error) { const err = error as Error this.handleError(err) + const queueDetails = this.isInitted + ? { queueArn: this.queue.arn, queueName: this.queue.name } + : {} throw new InternalError({ message: `Error while publishing to SQS: ${err.message}`, errorCode: 'SQS_PUBLISH_ERROR', details: { publisher: this.constructor.name, - queueArn: this.queue.arn, - queueName: this.queue.name, + ...queueDetails, messageType: this.resolveMessageTypeFromMessage(message) ?? 'unknown', }, cause: err, }) }Also applies to: 170-177
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sqs/lib/sqs/AbstractSqsPublisher.ts` around lines 107 - 114, The publish flow can race with the non-blocking init: after awaiting this.initPromise the code assumes this.isInitted and this.queue exist, but init() may have resolved without creating the queue causing subsequent accesses to throw and obscure the original failure; update the logic in the publish path (where isInitted, initPromise, init() and this.queue are used—e.g., the block around isInitted and the code that builds error details at/near publish()) to re-check this.isInitted after awaiting this.initPromise and bail out with a clear error (or return) if still not initted, and when constructing error details guard against this.queue being undefined (use explicit checks/optional chaining) so you never dereference this.queue after a failed init.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/sqs/lib/sqs/AbstractSqsConsumer.ts`:
- Around line 347-360: The _deadLetterQueue field is left stale when
initDeadLetterQueue exits early or when DLQ init is skipped, causing rerouted
failures to an old DLQ; update initDeadLetterQueue to explicitly clear
this._deadLetterQueue when deadLetterQueueOptions is falsy and also when initSqs
returns no result (after the deleteSqs/initSqs calls) so the consumer no longer
holds an outdated URL, and add a clear of this._deadLetterQueue in the class
close method (the same place other shutdown cleanup happens) to ensure
re-initialized consumers don’t reuse a previous DLQ; reference
initDeadLetterQueue, this._deadLetterQueue, initSqs, deleteSqs, and the close
method when applying the change.
In `@packages/sqs/lib/sqs/AbstractSqsPublisher.ts`:
- Around line 107-114: The publish flow can race with the non-blocking init:
after awaiting this.initPromise the code assumes this.isInitted and this.queue
exist, but init() may have resolved without creating the queue causing
subsequent accesses to throw and obscure the original failure; update the logic
in the publish path (where isInitted, initPromise, init() and this.queue are
used—e.g., the block around isInitted and the code that builds error details
at/near publish()) to re-check this.isInitted after awaiting this.initPromise
and bail out with a clear error (or return) if still not initted, and when
constructing error details guard against this.queue being undefined (use
explicit checks/optional chaining) so you never dereference this.queue after a
failed init.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f2da178-11d3-4e4e-aa43-69dff4b5dc27
📒 Files selected for processing (13)
packages/sqs/lib/fakes/TestSqsPublisher.tspackages/sqs/lib/index.tspackages/sqs/lib/sqs/AbstractSqsConsumer.tspackages/sqs/lib/sqs/AbstractSqsPublisher.tspackages/sqs/lib/sqs/AbstractSqsService.tspackages/sqs/test/consumers/SqsEventBridgeConsumer.spec.tspackages/sqs/test/consumers/SqsEventBridgeConsumer.tspackages/sqs/test/consumers/SqsPermissionConsumer.payloadOffloading.spec.tspackages/sqs/test/consumers/SqsPermissionConsumer.startupResourcePolling.spec.tspackages/sqs/test/consumers/SqsPermissionConsumer.tspackages/sqs/test/consumers/SqsPermissionConsumerFifo.tspackages/sqs/test/publishers/SqsPermissionPublisher.tspackages/sqs/test/publishers/SqsPermissionPublisherFifo.ts
CarlosGamero
force-pushed
the
chore/protected_dead_letter_queue_props_getter
branch
from
May 25, 2026 20:28
92f80b1 to
d5e9768
Compare
kibertoad
approved these changes
May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context -> https://lokalise.slack.com/archives/C0194K4AC1L/p1779712247913279
In order to use DLQ in susbcription we need to expose queue are
Summary by CodeRabbit
Refactor
Tests
Chores