refactor!: async-only consolidation (one async bus + drop the sync repository API) - #49
Conversation
First step of Phase 1 (legacy sync bus removal): the pub/sub transport test now publishes events to InMemoryBus and drains them via bus.subscribe, instead of Bus::from_queue(InMemoryQueue) + microsvc::subscribe. Proves the migration pattern; the legacy bus src stays until all consumers are migrated. Refs [[tasks/async-only-consolidation]] Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughConverts the crate to async-first: adds async locks/managers, async repository traits/implementations, async read-model and snapshot APIs, async outbox commit; removes synchronous layers and the entire bus module. Microsvc handlers become async, transports await dispatch. Tests are migrated to async patterns. ChangesAsync Platform Migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant AsyncRepo
participant ReadModelStore
Client->>Service: Command
Service->>AsyncRepo: commit(...)
AsyncRepo->>AsyncRepo: lock streams
AsyncRepo->>ReadModelStore: commit_write_plan_async
AsyncRepo-->>Service: Result
Service-->>Client: Response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
…arity)
The load -> mutate -> sync -> commit workspace ergonomic existed only over
the sync store traits; the async path used the bare write-plan builder. This
restores parity: the mutation/sync/diff surface is store-independent, so the
same `ReadModelWorkspace` now gains `load_async`/`commit_async` over the
`Async{ReadModelWritePlanStore,RelationalReadModelQueryStore}` traits, plus
`AsyncReadModelLoadBuilder` and `AsyncReadModelWorkspaceExt::workspace_async()`.
No struct extraction or duplicated diff logic: `load`/`commit` move to small
sync- and async-bound impl blocks; everything else stays shared and unbounded.
Proven with async mirrors of the include-hydration and sync-roundtrip tests
on `InMemoryReadModelStore` (impls both async store traits). Sync workspace
API and its tests unchanged.
Part of [[tasks/async-only-consolidation]] (Phase 2).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…on over the async surface Async paths previously bypassed QueuedRepository entirely (AsyncCommitBuilder commits straight through commit_batch_async), so two concurrent async commits to the same aggregate could interleave. This restores the queueing ability for async: `repo.queued_async().async_aggregate::<T>()` serializes per-aggregate get/commit exactly like the sync `.queued().aggregate::<T>()`. Lock primitive (runtime-agnostic — no tokio dep, matching the crate's RPITIT async surface): - AsyncLock / AsyncLockManager traits + InMemoryAsyncLock / InMemoryAsyncLockManager, a hand-rolled waker-based async mutex (try_lock/unlock stay sync; only acquire awaits). QueuedRepository<R, AsyncLockManager> (struct/Clone bound moved to the impls so an async lock manager is accepted): - AsyncGetStream / AsyncTransactionalCommit with the sync locking contract: reads acquire+hold the per-stream lock, commit releases on success and holds on error, multi-locks acquired in sorted/deduped order. Keyed by StreamIdentity::storage_key consistently across get/commit/unlock. - Non-locking forwards (drop-in completeness): AsyncSnapshotStore, AsyncReadModelWritePlanStore, AsyncRelationalReadModelQueryStore, AsyncInboxStore. - AsyncGetWithOpts / AsyncGetAllWithOpts (no_lock opt-out) + AsyncUnlockableRepository. - Queueable::queued_async() / queued_async_with(); AsyncAggregateRepository gains get_with/peek/get_all_with/peek_all/abort/unlock mirroring the sync layer. Adversarial review (3 lenses) found two latent defects in unlock(), both fixed: waking wakers while holding the std Mutex guard could (1) poison/brick the lock if a waker panics and (2) deadlock if a waker synchronously re-polls. unlock() now drains under the guard and wakes outside it; regression tests cover both. Tests: async lock unit tests (incl. re-entrant + panicking waker regressions) and tests/queued_repo_async (mutual exclusion, per-aggregate granularity, no_lock peek, abort release). Sync QueuedRepository API and its tests unchanged. Part of [[tasks/async-only-consolidation]] (Phase 2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces the legacy `Bus::from_queue`/`microsvc::listen` queue tests with the
async `InMemoryBus` + `BusConsumer::listen` (competing-consumer queues keyed by
command name). The legacy `stats.handled`/`stats.failed` handle has no async
analogue, so:
- success is asserted via domain outcomes (committed aggregate state), not counts;
- failure tolerance is asserted by showing the consumer drains past a failing
message and still processes the rest;
- metadata->Session is verified through `whoami` over the bus (works via
run_source -> dispatch_message -> message_to_session), with a negative control
under FailurePolicy::Stop;
- arbitrary queue names ("counters"/"creates") become command-name routing, so
two services on one bus consume disjoint command queues without competing.
Confirms Phase 1 needs no new runtime capability — metadata->Session already
works and the stats gap is a test-rewrite. microsvc crate: 15 passed.
Part of [[tasks/async-only-consolidation]] (Phase 1).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…InMemoryBus Replaces the threaded legacy-bus choreography (InMemoryQueue + OutboxWorkerThread::spawn_routed + microsvc::listen per service + sleep-poll) with a deterministic, thread-free drive over the async InMemoryBus: - `publish_pending_outbox` claims each service's outbox and forwards messages by destination — worker-addressed messages are point-to-point commands (send_message → consumed via `listen`), saga-addressed messages are events (publish_message → consumed via `subscribe`). - Each round uses a FRESH bus (the in-memory topic log is retained across reads, so a shared bus would re-deliver every prior event to the saga), forwards the pending outbox backlog, then drains the consumers. The loop ends when no service has pending work — i.e. the saga reached Completed. The `stats.handled` assertions (no async analogue) are dropped in favor of the existing domain assertions (saga/order Completed, inventory 95 available / 5 reserved, payment successful). Test 1 (saga_orchestrated) was already bus-free and is unchanged. Both tests pass. Part of [[tasks/async-only-consolidation]] (Phase 1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ed.rs) tests/sagas/distributed.rs drove the order-fulfillment saga over the raw `bus::Bus`/`Subscribable` API with hand-spawned threads and manual aggregate handling (bus.subscribe(&[names]) -> events.recv() loops). The async InMemoryBus has no raw-receiver equivalent — listen/subscribe are Service-driven — so the file cannot be faithfully migrated; a rewrite would duplicate the async microsvc_saga::saga_distributed test (same saga) plus the matrix metadata coverage. Removed as superseded (owner-confirmed): no coverage is lost. Also drops the now-unused event payloads in tests/sagas/order/events.rs (only distributed.rs constructed them). sagas crate: 7 passed, no warnings. Part of [[tasks/async-only-consolidation]] (Phase 1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…board onto InMemoryBus The projection handlers of BOTH the gold-standard matrix and the board still decoded via bus::Event (`Event::try_from(ctx.message())` + event.decode/ json_decode + event.id) — so the legacy bus could not be removed without touching the gold-standard test. Decoupled both (refactor, not delete): - decode straight from ctx.message().payload(): serde_json::from_slice for the matrix (JSON), BitcodePayloadCodec::decode for the board (bitcode — identical bytes to the old event.decode()); match on ctx.message().name(); event id from ctx.message().id(). Dropped the bus::Event `event()` helper from both projection handlers/mod.rs. - board main.rs: replaced InMemoryQueue + OutboxWorkerThread + the threaded start_board_projection_service + wait_for_* polling with publish_pending_outbox (fan-out events) + a single bus.subscribe; the projection's monotonic source_version guard makes the per-event-type drain order-independent. Added projections_service::load_board (direct read) replacing the poll loop. matrix: 2 passed (in-memory cell + refactored saga, both exercise the decode); board: 3 passed; sagas: 7 passed. clippy/fmt clean. Part of [[tasks/async-only-consolidation]] (Phase 1 — last test migration before the src removal). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The legacy sync bus is fully superseded by the async bus facade (InMemoryBus + the BusConsumer listen/subscribe + OutboxSource) and had no remaining consumers after the test migrations. Removed: - `src/bus/` entirely (Bus/Subscribable/InMemoryQueue/Listener/Sender/EventBus/ Event/Publisher, ~1.4k lines). - `OutboxWorkerThread` + WorkerStats + OutboxWorkerJoinError (the threaded outbox->bus bridge) and `src/outbox_worker/thread.rs`. - The bus-gated `microsvc::service` surface: `dispatch_event`, `dispatch_listened_event`, `subscribe`/`listen`, `TransportHandle` + `TransportStats`/`TransportJoinError`, and the `From<&Event> for Message` / `TryFrom<&Message> for Event` / `from_bus_event` bridges (+ their unit tests). - The `bus` Cargo feature (out of `default`); `http`/`grpc` no longer depend on it — they use the unconditional `microsvc::Message`, confirmed by building `--features http,grpc`. - The bus-gated crate-root re-exports (`InMemoryQueue`, `bus::Message`, the threaded-worker types). All consumers were migrated first (transport_subscribe/listen, microsvc_saga, the board) or removed as superseded (sagas/distributed.rs), and both projection handlers were decoupled from `bus::Event`. Default test sweep: 238 lib + all integration crates green; `--features http,grpc` builds; clippy/fmt clean. Closes Phase 1 of [[tasks/async-only-consolidation]] — one async bus facade, no sync bus path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Converts the microsvc handler model from sync to async, the foundation for dropping the sync repository API so all backends are async-only (the sync/async mix was the source of subtle bugs). Core (lib green; integration test crates migrated in follow-up commits): - HandlerFn is now `dyn for<'a> Fn(&'a Context<'a, D>) -> Pin<Box<dyn Future< Output=Result<Value, HandlerError>> + Send + 'a>>`, with an `AsyncHandler<'a,D>` HRTB helper trait so `async fn handle(ctx: &Context<D>)` registers directly. Guards stay synchronous. - Service::dispatch / dispatch_message / dispatch_request / invoke are async. - dependencies.rs: HasRepo/HasReadModelStore now resolve via the ASYNC repo + read-model traits (+ HasRepo for AsyncAggregateRepository / AsyncSnapshotAggregateRepository). - run_source + the http/grpc/knative transports await dispatch. - src unit tests converted (async-closure handlers + awaited dispatch). Handler authors write `async fn handle`; closures need an explicit ctx type annotation and must extract owned values before the `async move` (the future cannot borrow ctx across the await — an HRTB-closure limitation). cargo build (default + --features http,grpc) green; 238 lib tests pass. NOTE: tests/ integration crates still use the sync handler API and are migrated in the following commits (all-or-nothing handler switch). Part of [[tasks/async-only-consolidation]] (Phase 3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Completes the integration half of the async handler switch: all 21 test crates
(microsvc, sagas, the gold-standard distributed_read_model matrix, the board,
the transport conformance crates, and the ~15 direct-repo crates) now use the
async handler + async repo API exclusively:
- handlers are `async fn handle(ctx: &Context<'_, D>)` with awaited
ctx.repo().get/commit/peek and ctx.repo().outbox(msg).commit(&mut a).await;
read-model handlers use workspace_async()/load_async()/commit_async().await.
- services build with .queued_async().async_aggregate(); inline handler closures
use the `|ctx: &Context<D>| { extract ctx reads; async move { ... } }` form.
- test bodies await dispatch and the now-async repo reads.
Guards stay synchronous. Assertions and domain logic unchanged. The sync repo
trait surface is still present (deleted next); 502 default tests pass, the gold
-standard matrix's gated cells compile, http/grpc/sqlite cells pass.
Part of [[tasks/async-only-consolidation]] (Phase 3).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deletes the entire synchronous repository/read-model/snapshot trait surface, now unused after the async handler switch. This eliminates the sync/async mix that was the source of subtle combination bugs: there is exactly one (async) path for every backend. Removed (traits + all backend impls + re-exports): - repository: Get/Commit/Repository (repository.rs), GetOne/GetMany/Gettable (gettable.rs), the TransactionalCommit trait (batch.rs; CommitBatch kept). - snapshot: sync SnapshotStore + sync SnapshotAggregateRepository/SnapshotOutboxCommit. - read_model: sync ReadModelWritePlanStore/RelationalReadModelQueryStore, the sync ReadModelWorkspace load/commit impl, ReadModelLoadBuilder, ReadModelWorkspaceExt, and ReadModelWritePlanBuilder::commit (async equivalents kept). - aggregate: GetAggregate/GetAllAggregates/CommitAggregate + the sync AggregateRepository/AggregateBuilder (AsyncAggregateRepository/Builder kept). - commit_builder: SyncCommitBuilder/SyncStagedCommitBuilder/exts. - outbox: SyncOutboxCommit/SyncOutboxCommitExt (outbox_sync/commit_sync). - hashmap/postgres/sqlite/in-memory backends: their sync impls. - queued_repo: the sync QueuedRepository impls + sync Queueable::queued; the sync lock module (Lock/LockManager/InMemoryLock/InMemoryLockManager) is now fully unused and deleted (Async lock variants kept; LockError kept). - src/ unit tests that exercised the removed sync surface, converted to async. Also converted 5 remaining fully-sync integration crates the earlier sweep missed (bomberman [19 files], read_model_relationship_includes, read_model_commit_bridge, sourced_upcasting, transport_conformance's store_outbox). cargo test: 490 passed / 0 failed; --features http,grpc / postgres / sqlite all build; clippy clean; no sync trait remains in src/. Completes Phase 3 of [[tasks/async-only-consolidation]] — HashMap/SQLite/Postgres all async-only and consistent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
It is only used by the postgres/sqlite-gated matrix cells, so it (and its `TableSchemaRegistry` import) tripped a dead-code warning on the default build. Gate both with cfg(any(feature = "postgres", feature = "sqlite")) to match the call sites. Default clippy is now fully clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/lock/mod.rs (1)
27-34: ⚡ Quick winUpdate module docs to match the async-only API surface.
The exported surface is async-only now, but the module-level architecture docs still describe sync
Lock/LockManager/InMemoryLock, which is confusing for users.📘 Proposed doc update
-//! │ LockManager (per repository) │ -//! │ - get_lock(id) → Arc<Lock> │ +//! │ AsyncLockManager (per repository) │ +//! │ - get_lock(id) → Arc<AsyncLock> │ ... -//! │ Lock Trait │ +//! │ AsyncLock Trait │ //! │ lock() / try_lock() / unlock() │ ... -//! │InMemoryLock │ │ RedisLock │ │ PostgresAdvisory │ +//! │InMemoryAsync│ │ RedisLock │ │ PostgresAdvisory │ +//! │Lock (included)│ │ (external) │ │ (external) │ -//! │ (included) │ │ (external) │ │ (external) │🤖 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/lock/mod.rs` around lines 27 - 34, Update the module-level documentation in src/lock/mod.rs to reflect the async-only API: remove or replace any references to sync types "Lock", "LockManager", and "InMemoryLock" with the async equivalents "AsyncLock", "AsyncLockManager", and "InMemoryAsyncLock" and adjust prose to describe async behavior (futures, non-blocking semantics, and the InMemoryAsyncLock/ InMemoryAsyncLockManager testing use-case); also ensure exported symbols listed (AsyncLock, AsyncLockManager, InMemoryAsyncLock, InMemoryAsyncLockFuture, InMemoryAsyncLockManager) are accurately described in the docs so consumers aren’t misled by legacy sync terminology.src/commit_builder/mod.rs (1)
325-342: 💤 Low valueBusy-poll
block_onworks only for immediately-ready futures.This no-op waker implementation spins until
Poll::Ready. It's safe here becauseHashMapRepositoryandInMemoryReadModelStorecomplete synchronously, but would deadlock on any future that genuinely yields.Consider adding a brief doc comment noting the limitation, or use
futures::executor::block_onwhich handles real yields.🤖 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/commit_builder/mod.rs` around lines 325 - 342, The custom busy-polling function block_on spins with a no-op waker and only works for futures that are immediately Ready; replace its use or document its limitation: either switch to a proper executor (e.g., use futures::executor::block_on) wherever block_on is used, or add a clear doc comment on the block_on function explaining that it only supports synchronously-completing futures and will deadlock on yielding futures (mention HashMapRepository and InMemoryReadModelStore as currently safe examples). Ensure calls to block_on are updated to use the executor if you choose replacement.src/queued_repo/repository.rs (1)
177-187: ⚡ Quick winDeduplicate committed stream locks before releasing.
commit_batch_asynccurrently resolves/unlocks perbatch.streamsentry; duplicate stream identities can cause repeated unlock attempts for the same lock.♻️ Proposed hardening
- let mut locks = Vec::with_capacity(batch.streams.len()); - for stream in &batch.streams { - locks.push(self.ensure_async_lock(&stream.identity.storage_key())?); - } + let mut lock_ids: Vec<String> = batch + .streams + .iter() + .map(|s| s.identity.storage_key()) + .collect(); + lock_ids.sort_unstable(); + lock_ids.dedup(); + + let mut locks = Vec::with_capacity(lock_ids.len()); + for id in &lock_ids { + locks.push(self.ensure_async_lock(id)?); + }🤖 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/queued_repo/repository.rs` around lines 177 - 187, The unlock loop in commit_batch_async can call unlock multiple times for the same stream when batch.streams contains duplicates; change the collection of locks from a plain Vec to a keyed collection (e.g., HashMap or deduplicated Vec) keyed by stream.identity.storage_key() when calling ensure_async_lock so you only store one lock per unique storage_key, then iterate that deduplicated set to call unlock() once per key; update references in this block (ensure_async_lock, locks, batch.streams, and the unlock loop) accordingly to prevent duplicate unlock attempts.src/read_model/in_memory.rs (1)
573-590: 💤 Low valueTest
block_onhelper will spin indefinitely if the future yieldsPending.This no-op waker
block_onspins in a busy loop. It works for the current in-memory store futures (which never actually suspend), but if future refactoring introduces a real.awaitthat returnsPending, this loop will spin forever. Consider adding a panic or a bounded iteration count as a safety net.💡 Suggested defensive guard
let mut future = std::pin::pin!(future); + let mut iterations = 0; loop { if let Poll::Ready(output) = future.as_mut().poll(&mut cx) { return output; } + iterations += 1; + if iterations > 1000 { + panic!("block_on: future did not resolve after 1000 polls"); + } }🤖 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/read_model/in_memory.rs` around lines 573 - 590, The block_on helper (function block_on) uses a no-op waker and busy-loops, which will spin forever if the polled future ever returns Pending; change it to include a defensive safety net such as tracking loop iterations or elapsed time and panic (or return an error) after a reasonable bound, or yield to the OS with a short sleep per iteration to avoid busy spinning; update the block_on implementation to detect when a future hasn’t completed after the bound and abort with a clear message including "block_on: future did not complete" so callers of block_on/block_on in this module can surface the failure safely.tests/read_model_relationship_includes/main.rs (1)
11-28: 💤 Low valueThird occurrence of the same
block_onhelper.This is duplicated across three test files (
src/read_model/in_memory.rs,src/snapshot/in_memory.rs, and here). Consider extracting to a shared test utility module to reduce duplication.🤖 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 `@tests/read_model_relationship_includes/main.rs` around lines 11 - 28, The helper fn block_on is duplicated across tests; extract it to a shared test utility module (e.g., tests/common.rs or tests/util/mod.rs) as a pub fn block_on<F: Future>(...) and remove the local duplicates in read_model_relationship_includes, snapshot/in_memory.rs and read_model/in_memory.rs; update those test files to import the shared function (use crate::common::block_on or appropriate path) and ensure the signature and visibility match so callers compile without changing behavior.src/snapshot/in_memory.rs (1)
85-102: 💤 Low valueSame
block_onspinning concern as inin_memory.rs.This is the same no-op waker spinning pattern. Consider extracting a shared test utility or adding the same defensive guard.
🤖 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/snapshot/in_memory.rs` around lines 85 - 102, The block_on function in snapshot::in_memory.rs uses a no-op RawWaker that spins indefinitely; replace it with the shared test utility or the same defensive pattern used elsewhere: extract this into a common test helper (e.g., a shared block_on_noop_waker) or modify block_on to track poll iterations and call thread::yield_now periodically and panic after a reasonable max-iterations with a clear error message; update references to use the shared helper and keep the RawWaker creation logic centralized so both in_memory implementations share the same safe, non-spinning behavior.
🤖 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/commit.rs`:
- Around line 49-66: The custom block_on function creates a no-op RawWaker and
busy-polls, which can hang on Pending futures; remove this function and replace
uses of block_on with a proper executor (e.g., call
tokio::runtime::Runtime::new().unwrap().block_on(...) in tests or use
futures::executor::block_on(...)) so futures can be scheduled and woken
correctly; delete the unsafe RawWaker implementation in block_on and update any
calls that referenced block_on to use the chosen executor API (or convert tests
to #[tokio::test]/async tests) so waking and Pending are handled properly.
In `@tests/distributed_read_model_board/projections_service/handlers/board.rs`:
- Around line 32-33: The handler currently panics on malformed transport IDs
because event_version(message_id) and later code use expect(...) instead of
returning errors; update the code so message_id parsing returns a Result and
propagate parsing failures as a HandlerError instead of panicking. Concretely,
change usage of event_version(message_id) to handle its Result (or update
event_version to return Result<u64, ParseError>), replace any expect(...) calls
around Transport/MessageId parsing with map_err or match that converts the parse
error into a HandlerError (e.g., HandlerError::MalformedMessageId or a
descriptive variant), and ensure updated_board_view(&snapshot, version) is only
called after successful parsing so the handler returns Err(HandlerError) on
malformed IDs rather than panicking.
In `@tests/todos/main.rs`:
- Around line 146-154: The test is ignoring commit results and allowing an empty
path to pass; change the calls to repo.commit_all(&mut [&mut todo2, &mut
todo3]).await so failures are not ignored (unwrap or assert the Result is Ok)
and update the subsequent check of all_todos from repo.peek_all(&[&id1, &id2,
&id3]).await.unwrap() to fail fast (remove the if/else) by asserting the result
is not empty and that all_todos.len() == 3 (e.g. assert!(!all_todos.is_empty());
assert_eq!(all_todos.len(), 3)); apply the same pattern for the other
occurrences mentioned (the commit_all calls at the other locations and any
conditional empty-path checks).
---
Nitpick comments:
In `@src/commit_builder/mod.rs`:
- Around line 325-342: The custom busy-polling function block_on spins with a
no-op waker and only works for futures that are immediately Ready; replace its
use or document its limitation: either switch to a proper executor (e.g., use
futures::executor::block_on) wherever block_on is used, or add a clear doc
comment on the block_on function explaining that it only supports
synchronously-completing futures and will deadlock on yielding futures (mention
HashMapRepository and InMemoryReadModelStore as currently safe examples). Ensure
calls to block_on are updated to use the executor if you choose replacement.
In `@src/lock/mod.rs`:
- Around line 27-34: Update the module-level documentation in src/lock/mod.rs to
reflect the async-only API: remove or replace any references to sync types
"Lock", "LockManager", and "InMemoryLock" with the async equivalents
"AsyncLock", "AsyncLockManager", and "InMemoryAsyncLock" and adjust prose to
describe async behavior (futures, non-blocking semantics, and the
InMemoryAsyncLock/ InMemoryAsyncLockManager testing use-case); also ensure
exported symbols listed (AsyncLock, AsyncLockManager, InMemoryAsyncLock,
InMemoryAsyncLockFuture, InMemoryAsyncLockManager) are accurately described in
the docs so consumers aren’t misled by legacy sync terminology.
In `@src/queued_repo/repository.rs`:
- Around line 177-187: The unlock loop in commit_batch_async can call unlock
multiple times for the same stream when batch.streams contains duplicates;
change the collection of locks from a plain Vec to a keyed collection (e.g.,
HashMap or deduplicated Vec) keyed by stream.identity.storage_key() when calling
ensure_async_lock so you only store one lock per unique storage_key, then
iterate that deduplicated set to call unlock() once per key; update references
in this block (ensure_async_lock, locks, batch.streams, and the unlock loop)
accordingly to prevent duplicate unlock attempts.
In `@src/read_model/in_memory.rs`:
- Around line 573-590: The block_on helper (function block_on) uses a no-op
waker and busy-loops, which will spin forever if the polled future ever returns
Pending; change it to include a defensive safety net such as tracking loop
iterations or elapsed time and panic (or return an error) after a reasonable
bound, or yield to the OS with a short sleep per iteration to avoid busy
spinning; update the block_on implementation to detect when a future hasn’t
completed after the bound and abort with a clear message including "block_on:
future did not complete" so callers of block_on/block_on in this module can
surface the failure safely.
In `@src/snapshot/in_memory.rs`:
- Around line 85-102: The block_on function in snapshot::in_memory.rs uses a
no-op RawWaker that spins indefinitely; replace it with the shared test utility
or the same defensive pattern used elsewhere: extract this into a common test
helper (e.g., a shared block_on_noop_waker) or modify block_on to track poll
iterations and call thread::yield_now periodically and panic after a reasonable
max-iterations with a clear error message; update references to use the shared
helper and keep the RawWaker creation logic centralized so both in_memory
implementations share the same safe, non-spinning behavior.
In `@tests/read_model_relationship_includes/main.rs`:
- Around line 11-28: The helper fn block_on is duplicated across tests; extract
it to a shared test utility module (e.g., tests/common.rs or tests/util/mod.rs)
as a pub fn block_on<F: Future>(...) and remove the local duplicates in
read_model_relationship_includes, snapshot/in_memory.rs and
read_model/in_memory.rs; update those test files to import the shared function
(use crate::common::block_on or appropriate path) and ensure the signature and
visibility match so callers compile without changing behavior.
🪄 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: 61233f48-07b5-4c61-8d96-82a012393114
📒 Files selected for processing (135)
Cargo.tomlsrc/aggregate/aggregate.rssrc/aggregate/async_aggregate.rssrc/aggregate/mod.rssrc/bus/bus.rssrc/bus/event_bus.rssrc/bus/in_memory_queue.rssrc/bus/listener.rssrc/bus/mod.rssrc/bus/publisher.rssrc/bus/sender.rssrc/bus/subscriber.rssrc/commit_builder/mod.rssrc/hashmap_repo/repository.rssrc/lib.rssrc/lock/async_in_memory.rssrc/lock/async_lock.rssrc/lock/async_lock_manager.rssrc/lock/in_memory.rssrc/lock/lock.rssrc/lock/lock_manager.rssrc/lock/mod.rssrc/microsvc/dependencies.rssrc/microsvc/grpc.rssrc/microsvc/http.rssrc/microsvc/mod.rssrc/microsvc/service.rssrc/microsvc/transport/in_memory_bus.rssrc/microsvc/transport/knative.rssrc/microsvc/transport/outbox_dispatch.rssrc/microsvc/transport/outbox_source.rssrc/microsvc/transport/runner.rssrc/outbox/commit.rssrc/outbox/mod.rssrc/outbox_worker/mod.rssrc/outbox_worker/store.rssrc/outbox_worker/thread.rssrc/queued_repo/mod.rssrc/queued_repo/repository.rssrc/read_model/in_memory.rssrc/read_model/mod.rssrc/read_model/session.rssrc/repository/batch.rssrc/repository/gettable.rssrc/repository/mod.rssrc/repository/repository.rssrc/snapshot/in_memory.rssrc/snapshot/mod.rssrc/snapshot/repository.rssrc/snapshot/store.rstests/blob_game/main.rstests/bomberman/handlers/create_game.rstests/bomberman/handlers/get_player.rstests/bomberman/handlers/join_game.rstests/bomberman/handlers/mod.rstests/bomberman/handlers/move_player.rstests/bomberman/handlers/place_bomb.rstests/bomberman/handlers/shared.rstests/bomberman/handlers/tick.rstests/bomberman/main.rstests/bomberman/sim.rstests/distributed_read_model/checkout_saga_service/handlers/record_seat_reserved.rstests/distributed_read_model/checkout_saga_service/handlers/start.rstests/distributed_read_model/checkout_saga_service/mod.rstests/distributed_read_model/main.rstests/distributed_read_model/projection_service/handlers/checkout.rstests/distributed_read_model/projection_service/handlers/mod.rstests/distributed_read_model/projection_service/handlers/seat.rstests/distributed_read_model/query_service/mod.rstests/distributed_read_model/read_models/mod.rstests/distributed_read_model/seat_inventory_service/handlers/add.rstests/distributed_read_model/seat_inventory_service/handlers/reserve_started_checkout_seat.rstests/distributed_read_model/seat_inventory_service/mod.rstests/distributed_read_model_board/board_service/handlers/board_add_card.rstests/distributed_read_model_board/board_service/handlers/board_move_card.rstests/distributed_read_model_board/board_service/handlers/board_open.rstests/distributed_read_model_board/board_service/handlers/board_remove_card.rstests/distributed_read_model_board/board_service/mod.rstests/distributed_read_model_board/main.rstests/distributed_read_model_board/projections_service/handlers/board.rstests/distributed_read_model_board/projections_service/handlers/mod.rstests/distributed_read_model_board/projections_service/mod.rstests/distributed_read_model_board/query_service/mod.rstests/enqueue/main.rstests/event_store/main.rstests/kafka_transport/main.rstests/knative_cloudevents/main.rstests/microsvc/basic.rstests/microsvc/convention.rstests/microsvc/handlers/counter_create.rstests/microsvc/handlers/counter_increment.rstests/microsvc/handlers/mod.rstests/microsvc/handlers/whoami.rstests/microsvc/session.rstests/microsvc/transport_grpc.rstests/microsvc/transport_http.rstests/microsvc/transport_listen.rstests/microsvc/transport_subscribe.rstests/nats_transport/main.rstests/postgres_transport/main.rstests/queued_repo_async/main.rstests/rabbitmq_transport/main.rstests/read_model_commit_bridge/main.rstests/read_model_relationship_includes/main.rstests/read_model_session/main.rstests/sagas/distributed.rstests/sagas/handlers/inventory/init.rstests/sagas/handlers/inventory/mod.rstests/sagas/handlers/inventory/reserve.rstests/sagas/handlers/messages.rstests/sagas/handlers/orders/complete.rstests/sagas/handlers/orders/create.rstests/sagas/handlers/orders/mod.rstests/sagas/handlers/payments/mod.rstests/sagas/handlers/payments/process.rstests/sagas/handlers/saga/mod.rstests/sagas/handlers/saga/on_inventory_reserved.rstests/sagas/handlers/saga/on_order_completed.rstests/sagas/handlers/saga/on_order_created.rstests/sagas/handlers/saga/on_payment_succeeded.rstests/sagas/handlers/saga/start.rstests/sagas/main.rstests/sagas/microsvc_saga.rstests/sagas/orchestration.rstests/sagas/order/events.rstests/sagas/order/mod.rstests/snapshots/main.rstests/sourced/main.rstests/sourced_enqueue/main.rstests/sourced_snapshot/main.rstests/sourced_upcasting/main.rstests/todos/main.rstests/transport_conformance/mod.rstests/upcasting/aggregate.rstests/upcasting/main.rs
💤 Files with no reviewable changes (23)
- src/bus/subscriber.rs
- src/bus/event_bus.rs
- src/lock/lock_manager.rs
- src/bus/sender.rs
- tests/sagas/main.rs
- src/bus/listener.rs
- src/bus/publisher.rs
- src/bus/mod.rs
- tests/sagas/order/events.rs
- src/lock/lock.rs
- src/bus/bus.rs
- src/outbox_worker/mod.rs
- src/lock/in_memory.rs
- src/outbox_worker/thread.rs
- tests/sagas/order/mod.rs
- tests/sagas/distributed.rs
- src/repository/batch.rs
- src/repository/gettable.rs
- src/repository/repository.rs
- src/snapshot/store.rs
- tests/distributed_read_model_board/projections_service/handlers/mod.rs
- src/bus/in_memory_queue.rs
- src/microsvc/mod.rs
…ion) - src/ unit tests (outbox/commit, snapshot/in_memory, snapshot/repository, read_model/in_memory, commit_builder, outbox_worker/store, hashmap_repo): replace the custom busy-poll `block_on` (no-op waker, ignores Poll::Pending — would spin on any yielding future) with `#[tokio::test]`. Transport modules keep their intentionally runtime-free block_on. - board projection handler: `event_version` returns Result<_, HandlerError> instead of panicking on a malformed message id; the handler propagates with `?`. Its unit test now asserts the error path. - tests/todos: the bulk-commit roundtrip now asserts the commit succeeds and that exactly 3 todos are present (was: ignored result + an `if !empty` that masked failures). The concurrency-race commits (deliberately may lose the lock) keep their `let _ =`. 490 tests pass; clippy --all-targets clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
b7e1125
into
feat/transport-persistence-matrix
Makes the crate async-only and consistent: one async bus facade and one async repository surface, with HashMap/SQLite/Postgres all behaving the same. Removes the sync/async mix that was the source of subtle combination bugs. Stacked on the matrix branch (#48).
Net
135 files, +3,935 / −7,092(−3,157 lines). All green:cargo test490 passed / 0 failed;--features http,grpc / postgres / sqlitebuild; clippy--all-targetsclean.Phase 1 — remove the legacy sync bus
src/bus/(~1.4k lines),OutboxWorkerThread, the bus-gatedservice.rssurface (dispatch_event/listen/subscribe/TransportHandle+ theEvent↔Messagebridges), and thebusCargo feature.http/grpcno longer depend on it (they use the unconditionalmicrosvc::Message).transport_subscribe/transport_listen/microsvc_saga/the board onto the asyncInMemoryBus; removed the superseded raw-bus::Bussaga test (coverage preserved by the async tests); decoupled both projection handlers (incl. the gold-standard matrix) frombus::Event.Phase 2 — async ergonomic builders
ReadModelWorkspace: addedload_async/commit_async+AsyncReadModelWorkspaceExt::workspace_async()(the load→mutate→sync→commit ergonomic, previously sync-only) with no duplicated diff logic.QueuedRepository: a runtime-agnostic, waker-based async lock (AsyncLock/AsyncLockManager, no tokio dep) + the full async repo surface, so.queued_async().async_aggregate::<T>()serializes per-aggregateget/commitover the async path (which previously bypassed locking entirely). An adversarial review caught + fixed two latentunlockdefects (poison-on-panic and reentrant-wake deadlock from waking under the guard); regression tests added.Phase 3 — async handler model + drop the sync repo API
HandlerFnis now a boxedSendfuture with anAsyncHandlerHRTB helper trait, soasync fn handle(ctx: &Context<'_, D>)registers directly; dispatch/invoke anddependencies.rsare async; guards stay sync. (Required because dropping the sync repo traits meant handlers could no longer call syncctx.repo().get/commit.).queued_async().async_aggregate(), and test bodies).Get/Commit/Repository/GetOne/GetMany/TransactionalCommit, syncSnapshotStore/ReadModelWritePlanStore/RelationalReadModelQueryStore, syncAggregateRepository/QueuedRepository/CommitBuilder/OutboxCommit, and the now-unused sync lock module — across HashMap/SQLite/Postgres/in-memory backends.Breaking changes
async fn;Service::dispatch/dispatch_messageare async.Async*equivalents and.queued_async()/.async_aggregate().Tracked by
tasks/async-only-consolidation.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Breaking Changes
Removals
busmodule no longer enabled by default in Cargo features.