Freeze MessageMappingRegistry.ModuleKey at construction - #164
Merged
Conversation
VDBBjorn
force-pushed
the
arch/freeze-mapping-module-key
branch
2 times, most recently
from
August 6, 2026 14:48
d759a9f to
a130347
Compare
In a sharded module, `WithOutbox` constructed one `MessageMappingRegistry`
eagerly — before any `Register` callbacks ran — and then registered a
`Register` callback that mutated `registry.ModuleKey` to `context.ModuleKey`.
`RegisterModule` runs that same callback list once per shard, so for a two-shard
module the sequence was:
1. Shard db1's callback: `registry.ModuleKey = "orders#db1"`
2. Shard db2's callback: `registry.ModuleKey = "orders#db2"` ← overwrites
Both shards' `OutboxHandler` instances held a reference to the same registry
object. After registration completed, `registry.ModuleKey` was `"orders#db2"`
regardless of which handler was resolving it. Every mapper in every shard called
`GetKeyedService<EventSerializer>("orders#db2")` — the first shard's keyed
serializer was never reached and its upcaster chain was silently bypassed.
For an event stored at an old schema version, the EV-1 guard in
`DeserializeEvent` would then throw `InvalidOperationException` on the first
shard's outbox entries. For events at the current version, the wrong serializer
ran without error — a silent data-integrity defect.
## What changed
**`IMessageMappingRegistry.ModuleKey`** — setter removed; get-only. The seam
that enabled last-write-wins is gone.
**`MessageMappingRegistry`** — default constructor removed; replaced with
`MessageMappingRegistry(string? moduleKey)`. `ModuleKey` is read-only. Passing
null is legitimate for an unsharded module that has no module key.
**`MessagingBuilderExtensions.WithOutbox`** — construction and configuration of
the registry moved inside the `Register` callback. Each shard's callback now
creates its own `MessageMappingRegistry(context.ModuleKey)`, so the registered
handler for `"orders#db1"` holds a registry whose key is immutably
`"orders#db1"`, and the one for `"orders#db2"` holds its own.
**`PublicAPI.Unshipped.txt`** (`Alberto.Messaging`) — three `*REMOVED*` entries
for the deleted setter and default constructor; the new constructor signature
declared.
**`OutboxHandlerTests`** — replaced the original single-module positive test
with two tests:
- `Map_WithMessageType_ResolvesModuleKeyedSerializerSoUpcasterChainFires` — a
single-module positive test confirming the keyed-serializer path works when
the registry carries the correct key. XML doc revised to remove the false
"before the fix" narrative (the single-module case did not actually break in
production; the real defect was in sharding).
- `WithOutbox_in_sharded_module_each_shard_resolves_its_own_keyed_serializer` —
the regression test. Runs the deferred registrations for two shards directly
(no Postgres or full `AddAlberto` plumbing) so it stays in the fast suite.
Asserts: (a) `configureMappings` is invoked once per shard, producing two
separate registries; (b) each registry carries the correct shard key; (c) each
registry resolves its own shard-keyed `EventSerializer` when mapping a v1
event, delivering the shard-specific upcasted value to the mapper.
**`ConsumerUpcastingTests`** and **`UpcasterPublicApiReviewTests`** — callers of
the removed default constructor updated to `new MessageMappingRegistry(null)`;
doc comments updated to reflect the actual mechanism.
## Verification
Red run (sharding regression test, against origin/main code):
Failed WithOutbox_in_sharded_module_each_shard_resolves_its_own_keyed_serializer [17 ms]
Assert.Equal() Failure: Values differ
Expected: 2
Actual: 1
Failed! - Failed: 1, Passed: 0, Skipped: 0, Total: 1, Duration: 26 ms
The count of 1 proves configureMappings was called only once (eagerly), creating
a single registry that both shards mutated. After the fix, configureMappings runs
once per shard inside the callback, producing one registry per shard.
Green run (same test, after fix):
Passed! - Failed: 0, Passed: 1, Skipped: 0, Total: 1, Duration: 28 ms
Full fast suite after fix:
Passed! - Failed: 0, Passed: 1492, Skipped: 3, Total: 1495, Duration: 2 s
Release build: `dotnet build -c Release` — 0 errors, 94 warnings (all
pre-existing xUnit1051)
…ions Four Statement-mutation survivors in WithOutbox, all in src/Alberto.Messaging/MessagingBuilderExtensions.cs: 1. L60 ArgumentNullException.ThrowIfNull(configureMappings) deleted → pinned by WithOutbox_ThrowsWhenConfigureMappingsIsNull 2. L61 ArgumentNullException.ThrowIfNull(outboxStore) deleted → pinned by WithOutbox_ThrowsWhenOutboxStoreIsNull 3. L83 AddKeyedSingleton<IEventProcessor> call deleted (silent no-op outbox) → pinned by WithOutbox_RegistersOutboxHandlerAsKeyedEventProcessor 4. L86 AddSingleton<IHostedService>(OutboxRetentionService) call deleted (stated contract: retention sweep registers regardless of transport) → pinned by WithOutbox_RegistersRetentionServiceEvenWithoutTransport Two additional tests cover the transport branch in both directions: WithOutbox_RegistersRelayWhenTransportProvided WithOutbox_DoesNotRegisterRelayWhenNoTransport All six tests drive the deferred Register callbacks directly (same pattern as the existing sharded-module regression test) so they run in the fast (Category!=Integration) suite with no Docker. Mutation gate result: 9/9 killed, 100% (was 66.67%).
VDBBjorn
force-pushed
the
arch/freeze-mapping-module-key
branch
from
August 6, 2026 18:01
a130347 to
316dd10
Compare
Member
Author
|
CI note: the red The same tree passed this gate before the rebase: run 31124238686 on Merging on the two required contexts, |
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
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.
Closes #154
In a sharded module,
WithOutboxbuilt oneMessageMappingRegistryeagerly — before anyRegistercallback ran — then registered a callback that assignedregistry.ModuleKey = context.ModuleKey.RegisterModuleruns that callback list once per shard, over the same object:db1's callback setsModuleKey = "orders#db1"db2's callback setsModuleKey = "orders#db2"← overwritesEvery shard's
OutboxHandlerheld a reference to that one registry, so after registration it read"orders#db2"no matter which handler asked. Every mapper in every shard resolvedGetKeyedService<EventSerializer>("orders#db2"); the first shard's keyed serializer was never reached and its upcaster chain was silently bypassed.The failure mode splits on schema version. An event stored at an old version trips the EV-1 guard in
DeserializeEventand throwsInvalidOperationExceptionon the first shard's outbox entries. An event at the current version deserializes cleanly under the wrong serializer and no one finds out — a silent data-integrity defect.What changed
The mutability that made last-write-wins expressible is gone, rather than being guarded against.
IMessageMappingRegistry.ModuleKey— setter removed, get-only.MessageMappingRegistry— default constructor replaced withMessageMappingRegistry(string? moduleKey); the key is now fixed at construction. Null stays legitimate for an unsharded module that has no module key.WithOutbox— registry construction and configuration moved inside theRegistercallback, so each shard builds its own. The handler registered for"orders#db1"holds a registry whose key is immutably"orders#db1".PublicAPI.Unshipped.txt(Alberto.Messaging) — three*REMOVED*entries for the setter and default constructor, plus the new constructor.Tests
WithOutbox_in_sharded_module_each_shard_resolves_its_own_keyed_serializeris the regression test. It drives the deferred registrations for two shards directly — no Postgres, no fullAddAlberto— so it stays in the fast suite. It assertsconfigureMappingsruns once per shard, that each registry carries its own shard key, and that each resolves its own shard-keyedEventSerializer, delivering the shard-specific upcasted value.The existing single-module positive test was kept and renamed to
Map_WithMessageType_ResolvesModuleKeyedSerializerSoUpcasterChainFires. Its doc comment previously narrated a "before the fix" break that never happened — the single-module case worked in production; the defect was specific to sharding. That narration is corrected rather than preserved.Verified by breaking
Against
origin/main's production code the regression test fails:The
1is the diagnostic:configureMappingsran once, eagerly, producing the single shared registry both shards then mutated. With the fix it runs once per shard and the test passes.Full fast suite on the rebased branch: 1611 passed, 3 skipped, 0 failed. Release build: 0 errors.