Repository navigation
fix: retry lazy publisher init after a failed attempt - #653
ihlokalise wants to merge 2 commits into
Conversation
A failed lazy init left its rejected promise cached on the publisher, so every later publish re-awaited the same rejection until the process restarted. Clear the cached promise on failure as well as on success, in the SQS, SNS, GCP Pub/Sub and AMQP publishers.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: retrying lazy publisher initialization after a failed attempt. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 8… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 8 files.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
| } | ||
| await this.initPromise | ||
| this.initPromise = undefined | ||
| const initPromise = this.initPromise |
There was a problem hiding this comment.
Not caused by this PR, but it's close to it: in SQS, SNS and PubSub, close() doesn't clear initPromise (AMQP does). If close() runs while a lazy init is still going, the init finishes afterwards and sets isInitted = true again, so the closed publisher looks ready. A later publish() could also wait on the old promise.
Fine to leave it for a follow-up if you prefer. I just wanted to mention it since we're touching this code.
There was a problem hiding this comment.
@CarlosGamero How we would want publish() to behave when close() is triggered? Should we reject publishing or still send?
There was a problem hiding this comment.
Good question! I’d keep today’s behaviour: after close(), isInitted is false, so the next publish() just runs the lazy init again and sends. Rejecting would be a breaking change and isn’t related to this fix.
With that in mind, the case I mentioned ends up almost the same as a normal re-init, so it’s low risk. The only small thing would be clearing initPromise in close() like AMQP already does, to keep the publishers consistent, but that’s optional and fine for a follow-up. Not a blocker for this PR 👍
- AMQP: clear the cached init promise only on an init rejection, not when the publish after a successful init throws - close the publishers created in the new SNS and SQS tests - SQS: cover a real init failure (missing queue, then created) and concurrent publishes sharing one failed init
In plain words
Before a publisher can send its first message, it has to get ready: find the queue or topic, check it exists, and so on. If you don't call
init()yourself,publish()does this for you the first time it's called. This is the "lazy init".The bug: if that first get-ready step fails once, the publisher remembers the failure and never tries again. Every later
publish()gets the same old error until the process restarts, even after the cause has gone away.Think of calling a shop that doesn't answer. You write "they're closed" on a sticky note, and from then on you read the note instead of calling again. The shop opened an hour ago, but you never find out.
When this bites: anything short-lived during that first attempt, such as a network blip, AWS throttling
GetQueueUrl, credentials that aren't ready yet at startup, or a queue that is still being created. A one-second hiccup turns into every message failing until the pod restarts. Fixing the queue or the permissions doesn't help, because the publisher never looks again.Why it happened
The code already meant to forget the attempt once it finished. It just never got there when the attempt failed:
The fix
The attempt is now forgotten in a
finally, so it is cleared whether init succeeds or fails, and the nextpublish()simply tries again:The
=== initPromisecheck makes sure we only clear our own attempt and never wipe out a newer retry that another caller has already started.The same pattern was in four publishers, and all four are fixed:
AbstractSqsPublisherAbstractSnsPublisherAbstractPubSubPublisherAbstractAmqpPublisher: this one doesn't await (itspublishis synchronous), so the attempt is cleared in its existing.catchNothing else changes. A successful init behaves exactly as before. Concurrent publishes still share a single init attempt. A failed
publish()still throws the init error to its caller, as it does today. It just doesn't keep throwing it forever.Tests
One test per publisher: the first
init()fails, and the nextpublish()succeeds and the message is delivered.Each new test fails without the fix and passes with it. I checked this by reverting the library change and re-running the tests. The full publisher suites for
sqs,sns,amqpandgcp-pubsubpass, and lint (biome+tsc) is clean for all four packages.Summary by CodeRabbit