Skip to content

refactor!: outbox dedup + perf cleanup (remove OutboxWorker stack, batch settlement) - #106

Merged
patrickleet merged 7 commits into
mainfrom
review/outbox-dedup-perf
Jul 2, 2026
Merged

patrickleet merged 7 commits into
mainfrom
review/outbox-dedup-perf

Conversation

@patrickleet

Copy link
Copy Markdown
Collaborator

Summary

Cleanup of the outbox layer from two independent dedup/perf audits. One commit per finding.

refactor!: remove the vestigial OutboxWorker publish stack (7355637)

Deletes src/outbox_worker/worker.rs + publisher.rs (OutboxWorker, DrainResult, ProcessOneResult, OutboxPublisher, LogPublisher, LocalEmitterPublisher) — a complete second publish pipeline with its own degenerate message shape and an in-memory claim→publish→settle state machine without durable settle, duplicating OutboxDispatcher::dispatch_claimed. Only consumer was tests/todos, now ported to OutboxDispatcher + a recording MessagePublisher. LocalEmitterPublisher was not needed by anything else (the emitter feature's real path is the src/emitter enqueue flow), so it is not recreated; the one test that existed purely to exercise it is dropped. README's two stale mentions updated. No deprecation shims (pre-release).

perf: convert outbox rows to transport messages by value (0bcc085)

The immediate-publish path cloned each staged row into the hook, then From<&OutboxMessage> re-cloned payload + every metadata string. Every dispatch path owns its row at map time, so the conversion is now From<OutboxMessage> and payload/metadata/id/event-type all move.

refactor: one shared publish-then-settle path (5120a10)

dispatch_claimed and BusOutboxPublishHook carried the same publish → complete-or-record_failure sequence. Extracted as publish_and_settle, used by both; the hook never re-claims (rows are claimed inside the commit transaction). OutboxPublishHook::publish_claimed now takes the commit's rows as one Vec batch.

perf: batch outbox settlement + bounded publish concurrency (6e9689a)

  • OutboxStore::complete_many (default = serial loop): Postgres settles a batch in one UPDATE … FROM unnest(ids, workers, attempts) with the same per-claim predicate as complete (unapplied claims diagnosed to the same NotFound/InvalidState errors); SQLite runs the per-claim UPDATEs in one transaction (one commit/fsync per batch — an IN-list can't carry the per-claim worker/attempt predicate, so the transaction is the honest batching there); hashmap settles under a single write lock. Backend impls are additive-only (new fns, no restructuring) per the parallel repo-internals work.
  • publish_and_settle publishes with bounded concurrency (futures-util buffer_unordered) and settles all successes in one complete_many; failures settle individually.
  • OutboxDispatcher::with_publish_concurrency(NonZeroUsize)defaults to 1 because outbox claim order is created-at order and consumers may rely on it; the after-commit hook always uses 1 (a commit's rows are one aggregate's events). Documented on the option.
  • If the batched complete fails, published-but-unsettled rows stay in flight and re-publish at lease expiry — the same at-least-once window a crash between publish and settle always leaves.
  • New dep: futures-util (no default features) in core; the runtime-agnostic core stays executor-free.
  • New conformance case worker_completes_claims_in_one_batch runs complete_many against hashmap, Postgres, and SQLite.

refactor: shared claim_order_key (cf02f7c)

sort_by_claim_order and claim_order_ids each restated the (created-at, id) ordering; one key fn now defines it.

chore: consolidate test block_on (e5fddcd)

The busy-poll test executor was copy-pasted across outbox_worker test modules; now one #[cfg(test)] util. src/bus/ copies untouched (other agent's area).

Test output

  • cargo fmt clean, cargo clippy --workspace --all-features --all-targets clean
  • cargo test --workspace --all-features --all-targets (DATABASE_URL → dedicated postgres): all 45 suites pass, including todos, enqueue, sourced_enqueue, durable_enqueue_sqlite, postgres/sqlite repository + conformance (incl. new outbox batch case on both real backends), postgres_transport
  • lib tests: 270 passed (+5 new: mixed-outcome settle, concurrency>1, claim-order-on-the-wire, hashmap complete_many happy/stale)
  • Note: one unrelated flake observed once in a fully parallel run — distributed_read_model_board::board_service_feeds_a_normalized_card_read_model (in-memory bus/projection test; touches none of the changed paths, passes standalone 5/5 and in the full rerun)

🤖 Generated with Claude Code

https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n

@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@patrickleet, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 36 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ad386f04-5956-40da-8722-0078726bbd34

📥 Commits

Reviewing files that changed from the base of the PR and between 6d3272c and fd1be28.

📒 Files selected for processing (21)
  • Cargo.toml
  • README.md
  • src/lib.rs
  • src/outbox/commit.rs
  • src/outbox/message.rs
  • src/outbox_worker/bus_publisher.rs
  • src/outbox_worker/mod.rs
  • src/outbox_worker/outbox_dispatch.rs
  • src/outbox_worker/outbox_source.rs
  • src/outbox_worker/publish_hook.rs
  • src/outbox_worker/publisher.rs
  • src/outbox_worker/store.rs
  • src/outbox_worker/testing.rs
  • src/outbox_worker/worker.rs
  • src/postgres_repo/mod.rs
  • src/sqlite_repo/mod.rs
  • tests/hashmap_repository_conformance/main.rs
  • tests/persistent_repository_conformance/outbox.rs
  • tests/postgres_repository_conformance/main.rs
  • tests/sqlite_repository_conformance/main.rs
  • tests/todos/main.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch review/outbox-dedup-perf

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

patrickleet and others added 7 commits July 2, 2026 17:27
Delete the parallel outbox publish pipeline (OutboxWorker, DrainResult,
ProcessOneResult, OutboxPublisher, LogPublisher, LocalEmitterPublisher):
a complete second claim -> publish -> settle state machine that mutated
loaded rows in memory without durable settle, duplicating the production
drain path in OutboxDispatcher::dispatch_claimed with its own degenerate
message shape (event_type + raw bytes + metadata map instead of the
canonical bus Message).

Its only consumers were the todos integration tests, now ported to
OutboxDispatcher with a recording MessagePublisher. LocalEmitterPublisher
had no other users (the emitter feature's real path is src/emitter
enqueue), so it is not recreated. Pre-release: no deprecation shims.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
The immediate-publish path cloned each staged row into the publish hook,
then From<&OutboxMessage> for Message cloned the payload bytes and every
metadata string a second time. Every dispatch path (dispatch_claimed,
publish hook, outbox source) owns its row by the time it maps, so the
conversion is now From<OutboxMessage> (by value) and the payload,
event type, id, and metadata strings move instead of being re-cloned.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
OutboxDispatcher::dispatch_claimed and BusOutboxPublishHook carried the
same publish -> complete-or-record_failure sequence in two places.
Extract it as publish_and_settle, used by both. The hook still never
re-claims: it is handed rows already claimed inside the commit
transaction, and publish_and_settle only settles claims it is given.

OutboxPublishHook::publish_claimed now takes the commit's claimed rows
as one batch (callers looped per row anyway), which also sets up
batched settlement for the after-commit path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
dispatch_claimed settled strictly serially: one publish round trip plus
one complete UPDATE round trip per message.

- Add OutboxStore::complete_many (default: serial loop). Postgres settles
  the batch in one UPDATE ... FROM unnest(ids, workers, attempts)
  statement with the same per-claim predicate as complete, diagnosing
  unapplied claims to the same NotFound/InvalidState errors; SQLite runs
  the per-claim UPDATEs in one transaction (one commit/fsync per batch;
  an IN-list cannot carry the per-claim worker/attempt predicate); the
  hashmap store settles under a single write lock. Additive only in the
  repo backends: new fns, no restructuring.
- publish_and_settle now publishes with bounded concurrency
  (buffer_unordered) and settles all successes in one complete_many call;
  publish failures remain settled individually via record_failure.
- OutboxDispatcher::with_publish_concurrency(NonZeroUsize) exposes the
  publish window. It defaults to 1 because outbox claim order is
  created-at order and consumers may rely on it: 1 preserves strict
  ordering on the wire; higher values overlap publish round trips but may
  deliver out of order. The after-commit hook always uses 1 — a commit's
  rows are one aggregate's events, where relative order matters.
- If the batched complete fails, published-but-unsettled rows stay in
  flight and are re-published after lease expiry — the same at-least-once
  window a crash between publish and settle always leaves.
- Adds futures-util (no default features) to the core crate for
  buffer_unordered; the runtime-agnostic core stays executor-free.
- Conformance: worker_completes_claims_in_one_batch runs complete_many
  against hashmap, Postgres, and SQLite.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
sort_by_claim_order and claim_order_ids each restated the claim ordering
(created-at, then message id). One claim_order_key fn now defines it and
both helpers sort by it, so the two orderings cannot drift apart.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
The busy-poll test executor was copy-pasted into three outbox_worker
test modules (two more copies died with the OutboxWorker stack). One
#[cfg(test)] testing module now owns it; src/bus copies are untouched.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
lease_deadline_secs truncated now + lease to whole seconds (.as_secs()),
and claim() rebuilt the SystemTime from that u64 — so a sub-second lease
produced a deadline at or before now (expired at birth), and every lease
lost up to a second. claim() now takes the deadline as a SystemTime and
lease_deadline computes now + lease at full precision, keeping the
overflow and before-epoch validation. The stores already persist
leased_until with sub-second precision (f64 epoch / nanosecond storage),
so only the in-memory computation was lossy.

Unit test covers a sub-second lease: deadline is exactly now + lease and
is not expired until the lease elapses.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
@patrickleet
patrickleet force-pushed the review/outbox-dedup-perf branch from 75b5408 to fd1be28 Compare July 2, 2026 22:30
@patrickleet
patrickleet merged commit d25161b into main Jul 2, 2026
10 checks passed
patrickleet added a commit that referenced this pull request Jul 3, 2026
…ayer

Extends the existing SqlxReadModelBackend / lock/sqlx_common dialect-trait
pattern to the event-store/snapshot/outbox/inbox layers. postgres_repo and
sqlite_repo shrink from ~1700 lines each to ~460-line dialect shims over a
shared SqlxRepository/SqlxOutboxStore in src/sqlx_repo/repo.rs. Outbox `claim`
stays per-backend (postgres SKIP LOCKED CTE vs sqlite scan-loop).

Squashed from 14 commits for a single rebase reconciliation onto main (after
#106/#107/#109 merged). Notable changes rolled up:

- fix: surface malformed sqlite timestamps as errors (was silent UNIX_EPOCH);
  align null-bind error message with postgres (includes field_name).
- refactor: share commit-batch validation across all backends (fixes hashmap
  vs SQL snapshot-identity drift).
- feat!: widen postgres integer columns to BIGINT so both backends decode i64;
  deletes the width-conversion helpers.
- perf: batch snapshot loads for get_all hydration (fixes N+1); get_streams is
  a GetStream default method.
- perf: batch the commit_batch concurrency pre-check into one query.
- perf!: borrow events in PreparedEventAppend instead of cloning.
- feat!: bound outbox status listings (messages_by_status/pending) with a limit.
- perf!: store EventRecord.payload_codec as Cow<'static, str>.
- feat: run migrations through sqlx's Migrator with a _sqlx_migrations ledger.
- fix: chunk postgres event/outbox inserts under the 65535 bind-param cap.
- fix: classify postgres 57P01/57P02/57P03 and class-08 SQLSTATEs as transient.

Merge-reconciliation notes:
- table/ is now the canonical vocabulary (#109); backend shims import the
  renamed types via local aliases (TableColumn as ColumnDef, TableStoreError
  as ReadModelError) to keep the collapsed bodies unchanged.
- #106's batched OutboxStore::complete_many overrides lived in the old backend
  files and were collapsed away; SqlxOutboxStore inherits the serial default
  (correct, conformance-tested). Re-adding a batched override to the shared
  layer is a follow-up (see tasks/outbox-sqlx-batched-complete-many).

NOTE for #110: this flips the 57P01 classification, so the "flip this assertion"
fault-injection test on review/tests-dedup-coverage must be updated when both
merge.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
patrickleet added a commit that referenced this pull request Jul 3, 2026
…ayer

Extends the existing SqlxReadModelBackend / lock/sqlx_common dialect-trait
pattern to the event-store/snapshot/outbox/inbox layers. postgres_repo and
sqlite_repo shrink from ~1700 lines each to ~460-line dialect shims over a
shared SqlxRepository/SqlxOutboxStore in src/sqlx_repo/repo.rs. Outbox `claim`
stays per-backend (postgres SKIP LOCKED CTE vs sqlite scan-loop).

Squashed from 14 commits for a single rebase reconciliation onto main (after

- fix: surface malformed sqlite timestamps as errors (was silent UNIX_EPOCH);
  align null-bind error message with postgres (includes field_name).
- refactor: share commit-batch validation across all backends (fixes hashmap
  vs SQL snapshot-identity drift).
- feat!: widen postgres integer columns to BIGINT so both backends decode i64;
  deletes the width-conversion helpers.
- perf: batch snapshot loads for get_all hydration (fixes N+1); get_streams is
  a GetStream default method.
- perf: batch the commit_batch concurrency pre-check into one query.
- perf!: borrow events in PreparedEventAppend instead of cloning.
- feat!: bound outbox status listings (messages_by_status/pending) with a limit.
- perf!: store EventRecord.payload_codec as Cow<'static, str>.
- feat: run migrations through sqlx's Migrator with a _sqlx_migrations ledger.
- fix: chunk postgres event/outbox inserts under the 65535 bind-param cap.
- fix: classify postgres 57P01/57P02/57P03 and class-08 SQLSTATEs as transient.

Merge-reconciliation notes:
- table/ is now the canonical vocabulary (#109); backend shims import the
  renamed types via local aliases (TableColumn as ColumnDef, TableStoreError
  as ReadModelError) to keep the collapsed bodies unchanged.
- #106's batched OutboxStore::complete_many overrides lived in the old backend
  files and were collapsed away; SqlxOutboxStore inherits the serial default
  (correct, conformance-tested). Re-adding a batched override to the shared
  layer is a follow-up (see tasks/outbox-sqlx-batched-complete-many).

REBASED onto main+#110 (9f34f0a): #110 already merged, so its fault-injection
test that pinned the OLD 57P01 (mis)classification is flipped here to
`assert!(err.is_retryable())` to match this PR's classification fix; #110's
shared outbox test helpers are updated for the bounded messages_by_status(limit)
API. Main stays green on merge.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n
patrickleet added a commit that referenced this pull request Jul 3, 2026
…ayer (#111)

Extends the existing SqlxReadModelBackend / lock/sqlx_common dialect-trait
pattern to the event-store/snapshot/outbox/inbox layers. postgres_repo and
sqlite_repo shrink from ~1700 lines each to ~460-line dialect shims over a
shared SqlxRepository/SqlxOutboxStore in src/sqlx_repo/repo.rs. Outbox `claim`
stays per-backend (postgres SKIP LOCKED CTE vs sqlite scan-loop).

Squashed from 14 commits for a single rebase reconciliation onto main (after

- fix: surface malformed sqlite timestamps as errors (was silent UNIX_EPOCH);
  align null-bind error message with postgres (includes field_name).
- refactor: share commit-batch validation across all backends (fixes hashmap
  vs SQL snapshot-identity drift).
- feat!: widen postgres integer columns to BIGINT so both backends decode i64;
  deletes the width-conversion helpers.
- perf: batch snapshot loads for get_all hydration (fixes N+1); get_streams is
  a GetStream default method.
- perf: batch the commit_batch concurrency pre-check into one query.
- perf!: borrow events in PreparedEventAppend instead of cloning.
- feat!: bound outbox status listings (messages_by_status/pending) with a limit.
- perf!: store EventRecord.payload_codec as Cow<'static, str>.
- feat: run migrations through sqlx's Migrator with a _sqlx_migrations ledger.
- fix: chunk postgres event/outbox inserts under the 65535 bind-param cap.
- fix: classify postgres 57P01/57P02/57P03 and class-08 SQLSTATEs as transient.

Merge-reconciliation notes:
- table/ is now the canonical vocabulary (#109); backend shims import the
  renamed types via local aliases (TableColumn as ColumnDef, TableStoreError
  as ReadModelError) to keep the collapsed bodies unchanged.
- #106's batched OutboxStore::complete_many overrides lived in the old backend
  files and were collapsed away; SqlxOutboxStore inherits the serial default
  (correct, conformance-tested). Re-adding a batched override to the shared
  layer is a follow-up (see tasks/outbox-sqlx-batched-complete-many).

REBASED onto main+#110 (9f34f0a): #110 already merged, so its fault-injection
test that pinned the OLD 57P01 (mis)classification is flipped here to
`assert!(err.is_retryable())` to match this PR's classification fix; #110's
shared outbox test helpers are updated for the bounded messages_by_status(limit)
API. Main stays green on merge.


Claude-Session: https://claude.ai/code/session_01DzYSVLas93c7LbgHJWsW7n

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant