fix: harden outbox claim leases - #28
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThis PR adds time-explicit lease/claim checks to OutboxMessage, worker-scoped repository operations (complete/release/record failure) with a retry ceiling, new RepositoryError variants for validation, updates worker threads to use the new APIs, and updates README/module docs and re-exports. ChangesOutbox lease semantics and worker-scoped failure handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
613-618:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the Outbox Worker example with the new worker-scoped contract.
This section now documents lease ownership and retry-ceiling failure handling, but the example still directly commits
message.entity. That bypasses the worker-scoped APIs the prose recommends and can mislead users into skipping lease/worker validation.Suggested README adjustment
let mut claimed = repo.claim_outbox_messages("worker-1", 100, Duration::from_secs(30))?; -let _ = worker.process_batch(&mut claimed); +let _ = worker.process_batch(&mut claimed); for message in &mut claimed { - repo.commit(&mut message.entity)?; + match message.status() { + OutboxMessageStatus::Published => { + repo.complete_outbox_message_for_worker(message.id(), "worker-1")?; + } + OutboxMessageStatus::InFlight => { + let _action = repo.record_outbox_publish_failure(message.id(), "worker-1", 5)?; + } + _ => {} + } }Also applies to: 621-625
🤖 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 `@README.md` around lines 613 - 618, The example bypasses the new worker-scoped contract by calling repo.commit(&mut message.entity) directly; update the snippet to use the repository's worker-scoped commit API (the one that accepts the claimed outbox message and worker/lease info) instead of committing message.entity so lease ownership and retry-ceiling checks are enforced; after worker.process_batch(&mut claimed) call the worker-scoped commit method for each message (pass the claimed message and the worker id/lease token) and handle any lease/ retry-ceiling errors per the README prose rather than mutating message.entity directly.
🧹 Nitpick comments (2)
src/outbox_worker/repository_ext.rs (1)
36-47: ⚡ Quick winConstrain legacy non-worker mutation APIs to avoid ownership bypass.
Line 224 and Line 244 validate lease freshness but skip claimant ownership (
ensure_active_claim(..., None, ...)), so callers can still complete/release another worker’s active claim through the non-worker methods. Consider deprecating these methods (or making them internal/admin-only) and steering callers to*_for_worker.Also applies to: 46-55, 222-248
🤖 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 `@src/outbox_worker/repository_ext.rs` around lines 36 - 47, The non-worker mutation APIs (complete_outbox_message, release_outbox_message) allow bypassing claimant ownership because ensure_active_claim is called with None; restrict or deprecate these to prevent one worker completing/releasing another’s claim. Mark complete_outbox_message and release_outbox_message as deprecated/internal (or change their visibility), update implementations to either call through to the *_for_worker variants only after verifying the caller is an admin, or remove their public surface; change ensure_active_claim calls so they require a claimant (pass worker_id) where appropriate and update all call sites to use complete_outbox_message_for_worker and release_outbox_message_for_worker (and ensure implementations of those methods enforce claimant checks).src/outbox_worker/thread.rs (1)
117-122: ⚡ Quick winPersist the actual publish error.
Recording a fixed
"publish failed"message throws away the broker error that explains why the message exhaustedDEFAULT_MAX_ATTEMPTS. Capture theErr(err)value here and persist its text instead.Also applies to: 208-213
🤖 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 `@src/outbox_worker/thread.rs` around lines 117 - 122, The call to repo.record_outbox_publish_failure currently records the fixed string "publish failed" which discards the broker error; change the code around the publish failure handling (the branches that call repo.record_outbox_publish_failure with msg.id(), &worker_id, "publish failed", DEFAULT_MAX_ATTEMPTS) to capture the Err(err) returned by the publish attempt and pass err.to_string() (or format!("{}", err)) as the error message argument instead of the literal "publish failed", and apply the same change for the other occurrence around the 208-213 block so the real broker error text is persisted.
🤖 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 `@src/outbox_worker/mod.rs`:
- Around line 26-28: The example loop binds msg immutably but calls
worker.process_message(&mut msg); change the loop to bind msg as mutable (e.g.,
use a mutable iterator so the loop variable is mutable) so that
worker.process_message(&mut msg) compiles; update the for ... in messages line
that precedes calls to worker.process_message(&mut msg) and
repo.complete_outbox_message_for_worker(msg.id(), "worker-1") accordingly (use
either "for mut msg in messages" or "for msg in &mut messages" depending on
whether messages is owned or borrowed).
In `@src/outbox_worker/thread.rs`:
- Around line 109-123: The repo calls currently drop all errors from
complete_outbox_message_for_worker and record_outbox_publish_failure; change the
logic in the worker loop to explicitly match and handle the repo error variants
returned by complete_outbox_message_for_worker and record_outbox_publish_failure
(e.g., stale-lease / wrong-worker / invalid-state vs other failures) instead of
ignoring Err(_): for complete_outbox_message_for_worker, if you get a
stale-lease/wrong-worker/invalid-state error treat the message as already
reclaimed (log at debug/warn and do not increment messages_published), but for
transient/errors that indicate DB failure log an error and increment
messages_failed (or emit a metric) so it won’t silently be re-published;
similarly for record_outbox_publish_failure, detect and handle
lease/worker/state errors separately (log and advance attempts or surface the
error) and only swallow benign concurrency errors—ensure you call the concrete
error-match arms on the Result from repo.complete_outbox_message_for_worker and
repo.record_outbox_publish_failure (use their returned error enum/variants) so
retries/attempt counters and reclaim behavior remain correct.
---
Outside diff comments:
In `@README.md`:
- Around line 613-618: The example bypasses the new worker-scoped contract by
calling repo.commit(&mut message.entity) directly; update the snippet to use the
repository's worker-scoped commit API (the one that accepts the claimed outbox
message and worker/lease info) instead of committing message.entity so lease
ownership and retry-ceiling checks are enforced; after worker.process_batch(&mut
claimed) call the worker-scoped commit method for each message (pass the claimed
message and the worker id/lease token) and handle any lease/ retry-ceiling
errors per the README prose rather than mutating message.entity directly.
---
Nitpick comments:
In `@src/outbox_worker/repository_ext.rs`:
- Around line 36-47: The non-worker mutation APIs (complete_outbox_message,
release_outbox_message) allow bypassing claimant ownership because
ensure_active_claim is called with None; restrict or deprecate these to prevent
one worker completing/releasing another’s claim. Mark complete_outbox_message
and release_outbox_message as deprecated/internal (or change their visibility),
update implementations to either call through to the *_for_worker variants only
after verifying the caller is an admin, or remove their public surface; change
ensure_active_claim calls so they require a claimant (pass worker_id) where
appropriate and update all call sites to use complete_outbox_message_for_worker
and release_outbox_message_for_worker (and ensure implementations of those
methods enforce claimant checks).
In `@src/outbox_worker/thread.rs`:
- Around line 117-122: The call to repo.record_outbox_publish_failure currently
records the fixed string "publish failed" which discards the broker error;
change the code around the publish failure handling (the branches that call
repo.record_outbox_publish_failure with msg.id(), &worker_id, "publish failed",
DEFAULT_MAX_ATTEMPTS) to capture the Err(err) returned by the publish attempt
and pass err.to_string() (or format!("{}", err)) as the error message argument
instead of the literal "publish failed", and apply the same change for the other
occurrence around the 208-213 block so the real broker error text is persisted.
🪄 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 Plus
Run ID: 3fa88cc7-0e11-4eb2-9023-c61365c26a87
📒 Files selected for processing (8)
README.mdsrc/lib.rssrc/outbox/message.rssrc/outbox/mod.rssrc/outbox_worker/mod.rssrc/outbox_worker/repository_ext.rssrc/outbox_worker/thread.rssrc/repository/error.rs
Implements [[tasks/durable-outbox-claim-leases]]
patrickleet
force-pushed
the
fix/outbox-claim-leases
branch
from
May 21, 2026 04:11
c8a076e to
0fa4373
Compare
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This was referenced May 25, 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
1 participant
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.
Summary
Verification
Note: all-features still reports the existing Reservation dead-code warning in tests/sagas/order/inventory.rs.
Summary by CodeRabbit
Documentation
New Features
Tests