Repository navigation
fix(mq): keep one tenant's failed queue join from ending ingest for all - #680
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)Source excerpt: **`mq/`** — the message-queue boundary: the **only** package that imports NATS/JetStream (Key Design Decision `#20`).📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: **Every code change updates its docs + `CHANGELOG.md` in the same PR** — a code change without its doc update is incomplete ([§Documentation Sync](`#documentation-sync`)).📄 CodeRabbit inference engine (AGENTS.md) Files:
🪛 LanguageToolCHANGELOG.md[typographical] ~93-~93: Consider using an em dash in dialogues and enumerations. (DASH_RULE) [style] ~93-~93: Consider using “who” when you are referring to people instead of objects. (THAT_WHO) [style] ~93-~93: Consider using “who” when you are referring to a person instead of an object. (THAT_WHO) 🔇 Additional comments (3)
📝 SummarySummary by CodeRabbit
WalkthroughTenant queues are recorded as open only after registered consumers join. Failed joins leave the queue closed to publishes and can be retried by a later publish or reload. Tests and documentation cover tenant-specific failures and continued delivery for other tenants. ChangesTenant queue join handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to No established issue remains that should delay merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is designed to contain a failed queue join to one tenant rather than interrupt shared ingestion. No new security exposure was established, but concurrent recovery and production behavior were not fully verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
github-actions
Bot
added
documentation
|
📚 Docs preview is live → https://e31d96d3-wavehouse-docs.wave-rf.workers.dev |
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit 1ade99c in the Show a line coverage summary of the most impacted files.
Updated |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: bb83d68e-c7f8-4a50-8b0b-c99d6555c788
📒 Files selected for processing (8)
CHANGELOG.mddocs/src/content/docs/architecture.mddocs/src/content/docs/durability.mddocs/src/content/docs/ingest-pipeline.mddocs/src/content/docs/settings-directory.mdxinternal/mq/embedded.gointernal/mq/embedded_test.gointernal/mq/mq.go
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
Source excerpt: **WH001 applies to every tracked Markdown file, with no carve-out** — `AGENTS.md`, `CHANGELOG.md`, `.github/` CI docs and `.claude/` agent prompts included.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/durability.mddocs/src/content/docs/ingest-pipeline.mddocs/src/content/docs/architecture.md
Source excerpt: **`mq/`** — the message-queue boundary: the **only** package that imports NATS/JetStream (Key Design Decision `#20`).
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal/mq/mq.gointernal/mq/embedded_test.gointernal/mq/embedded.go
Source excerpt: **Every code change updates its docs + `CHANGELOG.md` in the same PR** — a code change without its doc update is incomplete ([§Documentation Sync](`#documentation-sync`)).
📄 CodeRabbit inference engine (AGENTS.md)
Files:
CHANGELOG.md
Source excerpt: **In MDX, leave a blank line between a JSX tag and a code fence.**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/settings-directory.mdx
🪛 LanguageTool
docs/src/content/docs/durability.md
[style] ~105-~105: Consider using the typographical ellipsis character here instead.
Context: ...settings; the next reload retrieswithjoin the queue: ... context deadline exceeded`, and that te...
(ELLIPSIS)
CHANGELOG.md
[typographical] ~93-~93: Consider using an em dash in dialogues and enumerations.
Context: - **One tenant's failed queue join no lon...
(DASH_RULE)
[style] ~93-~93: Consider using “who” when you are referring to people instead of objects.
Context: ... so the next reload tries the consumers that had not joined again, and the tenant's ...
(THAT_WHO)
[style] ~93-~93: Consider using “who” when you are referring to a person instead of an object.
Context: ...uched, and nothing restarts. A consumer that had joined and whose delivery then ends...
(THAT_WHO)
docs/src/content/docs/settings-directory.mdx
[typographical] ~227-~227: Consider using an em dash in dialogues and enumerations.
Context: - mq.max_bytes_gb (seed default 50) —...
(DASH_RULE)
docs/src/content/docs/ingest-pipeline.md
[style] ~206-~206: Consider using “who” when you are referring to people instead of objects.
Context: ...rker's own failed channel. A consumer that cannot start at all takes the same path...
(THAT_WHO)
[style] ~208-~208: To elevate your writing, try using a synonym here.
Context: ...at could delete a durable) this path is hard to reach. A tenant's queue, opened at r...
(HARD_TO)
docs/src/content/docs/architecture.md
[typographical] ~173-~173: Consider using an em dash in dialogues and enumerations.
Context: - mq.go — The owned surface, stated a...
(DASH_RULE)
[style] ~173-~173: Since ownership is already implied, this phrasing may be redundant.
Context: ...o a blocking handler is backpressure on its own tenant alone, spreads the prefetch acro...
(PRP_OWN)
[style] ~173-~173: A comma is missing here.
Context: ...ending on its own — ErrDeliveryEnded, e.g. a deleted consumer or a closed connecti...
(EG_NO_COMMA)
[style] ~173-~173: Since ownership is already implied, this phrasing may be redundant.
Context: ...terer.DeadLetter` (park a message under its own topic, in its tenant's dead-letter queu...
(PRP_OWN)
[style] ~177-~177: ‘out of reach’ might be wordy. Consider a shorter alternative.
Context: ...75% of the free disk by default) is set out of reach, so a budget is a cap and never a reser...
(EN_WORDINESS_PREMIUM_OUT_OF_REACH)
[style] ~177-~177: Consider using “who” when you are referring to people instead of objects.
Context: ... so the next reload tries the consumers that had not joined again. A publish or park...
(THAT_WHO)
[style] ~177-~177: For a more expressive style, consider rephrasing the sentence in the active voice.
Context: ...rts the budget last applied in full, so a failed resize is retried by the next reload. The consumers CreateConsumer and `Su...
(PASSIVE_VOICE_SIMPLE)
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Brings in #614 (shared redis cache), #625 (dedupe reserve/commit, dynamodb, dedupe.lease, WithIdempotencyKey), #680 (a failed queue join is the tenant's alone) and #623's squash, whose content the stack already had from its PR head. Resolutions: - config: Warnings keeps the nats warnings for every role and main's api-only cache/redis/dedupe ones; the unknown-backend test lists nats beside redis and dynamodb. - mq: main's WithIdempotencyKey, ErrUnavailable and Consume contracts; the conformance suite is main's, idempotency case included. - testutil: main's storedir replaces this stack's StoreDir. - ingest: main's publishFailed already answers ErrUnavailable with 503. - integration setup keeps startNATS beside startDynamoDBLocal; the Makefile runs internal/cache with the tagged suites as main does. - docs and CHANGELOG keep both sides; the backend lists name nats, redis and dynamodb together. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyrXjhR7iDg33paioLQHFq
1 participant
Summary
When a tenant's queue opened at runtime and the ingest worker's consumer could not join it, the broker reported that as the consumer's delivery ending. The worker failed and the process restarted, so one tenant's failure took ingest down for every tenant. The stream hub's consumer failing the same way was only logged, and that tenant's streams got no live rows until a restart.
A queue a consumer cannot join is now left unrecorded as open, whichever consumer it is:
SetMaxBytesreturns the error andMaxBytesdoes not report the budget, so the next reload retries the join.503and retry the join too, through the existing pacing for a queue that cannot open (one shared attempt, then refusals for five seconds).Unchanged: a consumer that had already joined and whose delivery then ends on its own still fails the worker, and a flat directory whose queue cannot open still refuses boot.
Test plan
failed, and leaves a second tenant publishing and consumingRelated Issues
Closes #675
Part of #583