Retire the previous natural key row when the key changes (#5041) - #5049
Merged
Conversation
NaturalKeyProjection only ever upserted, so an event that renamed an aggregate's [NaturalKey] left the row carrying the old value behind, still pointing at the same stream. The table then accumulated one dead row per rename and the retired value permanently squatted on its slot in the primary key, so no other stream could claim it. A projection rebuild replays through the same upsert builder and reproduced the same set of rows after teardown. Queue a scoped DELETE ahead of each upsert that clears any other row for this stream (and tenant, when conjoined). QueueSqlCommand preserves order within the batch, so a create-then-rename inside a single batch still lands on the newest value, and the rebuild path gets the same treatment for free since it shares the builder. The remaining two symptoms reported on #5041 are [NaturalKeySource] discovery defects in JasperFx.Events, not Marten: an IEvent<T> handler yields no extractor at all, and an instance Apply(TEvent) handler is invoked against a fabricated blank aggregate so any body touching other state throws out of SaveChangesAsync. Filed as JasperFx/jasperfx#569; the repros from #5042 land here skipped, to be unskipped on the bump. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremydmiller
added a commit
that referenced
this pull request
Jul 26, 2026
#5041 reported three natural key defects. Two are fixed in code across this PR and an already-merged one; the third is a "cannot be supported here" that was never written down. Closing it out means saying so with coverage. Item 1 - static IEvent<T> handlers not discovered - is the #5052 commit in this PR (#571 widened the extraction contract to the whole event). Item 2 - the old natural key not cleaned up - shipped in #5049. Its tests only exercise a source shape discovery could ALWAYS bind, though: a static handler taking the raw event. On the two paths #5052 newly enables the question could not even be asked before, because item 1 meant nothing was written for those event types at all. So assert retirement there too - the lookup table holds only the new key, and the retired one no longer resolves - on both the IEvent<T> source and the explicit NaturalKeyFor registration. Item 3 - handlers not receiving the current snapshot state - stays unsupported, because the lookup table is maintained inline at append time. That is exactly what makes a natural key lookup work under an Async snapshot lifecycle, where no aggregate exists yet, and it is why the key has to be a function of the event alone. #571 stopped papering over it: the blank-aggregate strategy is ranked last, skipped when the aggregate cannot be safely constructed, and reported loudly rather than silently producing a wrong key. The docs now say the method never sees the current aggregate, why that follows from when the write happens, and what to do instead (carry the value on the event). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeremydmiller
added a commit
that referenced
this pull request
Jul 26, 2026
…eout docs, natural key extraction (#5054) * Bump JasperFx/JasperFx.Events 2.35.0 -> 2.36.0 Picks up the three upstream changes the follow-on commits build on: - #564 (#5047): DaemonSettings.StopAndDrainTimeout, a per-shard bound on the graceful stop-and-drain. Default 5s == the old hardcoded value, so behavior is unchanged unless configured. - #565/#567 (#5048): ShardFailure, ShardFailureCategory, EventFailureDetails and the IEventFailureContext seam, plus ShardState.Failure on the paused/stopped states. - #569/#571 (#5052): NaturalKeyEventMapping.Extractor widens from Func<object, object?> to Func<IEvent, object?>. That last one is source-breaking, so the two mechanical call-site adaptations in NaturalKeyProjection ride along here to keep this commit compiling on its own. The rest of #5052 - the regression test it unblocks and the docs for the explicit registration path - is its own commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(#5047): document DaemonSettings.StopAndDrainTimeout #564 added a per-shard bound on the daemon's graceful stop-and-drain, and nothing in the async daemon docs explained what a drain is or why the bound matters. New "Graceful Shutdown and the Drain Timeout" section covers what the timeout bounds (the in-flight page plus the progression flush, on all three stop paths), the failure it prevents when the drain is cut short (ProgressionProgressOutOfOrderException on the next start), why a large agent universe draining inside a Kubernetes grace window is the motivating case, pairing it with HostOptions.ShutdownTimeout and terminationGracePeriodSeconds, why a deployment might instead lower it, and what opting out with Timeout.InfiniteTimeSpan costs. Docs only; the setting itself ships in JasperFx.Events 2.36.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(#5048): classify why a shard is down and persist it (#565) Before this, an external supervisor could see that a shard was Paused and nothing else. #565 adds ShardFailure so the daemon can say WHY, but it deliberately has no fallback type-name sniffing: a store's exception declares its own category, or the failure classifies as Other with no event details. Both halves of that land here. Part 1 - the read-path exceptions implement IEventFailureContext: - EventDeserializationFailureException declares EventSerialization. It has always been handed the IEventType and used it only to build the message string; the alias is now retained so the failing event type is reported as data rather than prose. - UnknownEventTypeException declares UnknownEventType, kept separate from serialization on purpose - a missing registration or a rollback is a deployment fix, not a data fix. It now also carries the sequence, threaded down from the mt_events row being read (-1 where the throw site has no row, which is already how Marten spells an undeterminable sequence). - EventDeserializationFailureException.ToDeadLetterEvent assigns its Id up front instead of leaving Guid.Empty for identity generation at write time, so the creating process can correlate the ShardFailure it reported with the row it produced before that background write lands. Matches what jasperfx's DeadLetterEvent constructor now does on the ApplyEventException path. Part 2 - four extended-progression columns (failure_category, failure_event_sequence, failure_event_type, failure_event_tenant_id) behind the existing EnableExtendedProgressionTracking gate, written and read back onto ShardState.Failure so a consumer polling the database - which is the only channel left when the publishing node is down - gets the same shape a live observer does. The category persists as the enum NAME, so reordering ShardFailureCategory can never silently re-label old rows. No column for the reason text: ShardFailure.Detail is exactly what pause_reason already carried. The failure columns follow a different write rule from the rest of extended progression: written when the state carries a Failure, cleared on a Started that has none (a recovered shard must stop reporting the reason it paused an hour ago), and otherwise left alone. That last case is load-bearing - SubscriptionAgent publishes a plain Stopped right behind a Paused, and an unconditional write would erase the reason microseconds after recording it. Both write overloads now share one UPDATE ... FROM unnest, so the conditional semantics live in exactly one place; the single-state overload delegates to the batch. This retires Marten's use of mt_mark_event_progression_extended rather than growing its signature, which follows the reasoning already recorded on the batch overload: no schema object to migrate, so AutoCreate.None deployments pick the change up. The function itself stays installed for anything calling it directly. Note that the four new columns DO require a migration for stores that already have EnableExtendedProgressionTracking on. They are nullable, so no backfill is needed, and until the migration runs the write degrades to no telemetry rather than failing anything - extended progression is best-effort by contract. Tests: exception contract + ShardFailure.For classification through wrapped exceptions (CoreTests), and write/clear/preserve/rehydrate plus the acceptance case from the issue - a projection paused by a corrupted event body reports EventSerialization with the failing sequence and event type (DaemonTests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#5052): close out natural key extraction on the widened contract (#571) The mechanical half - NaturalKeyProjection passing the whole IEvent to mapping.Extractor at both call sites - rode along with the 2.36.0 bump, because widening Func<object, object?> to Func<IEvent, object?> is source breaking and the bump had to compile. This is the rest of what #571 makes possible or observable. Unblocks the two skipped tests from #5042, but not by simply unskipping them. Widening to the whole event is what makes an IEvent<T> [NaturalKeySource] bindable at all, and that half now works. The other half does not, by design: the reporter's aggregate declares `required` members, so the fallback strategy of invoking their handler against a fabricated blank aggregate is now skipped rather than silently handing them an instance C# itself would never have let them construct - which was the original ArgumentNullException out of SaveChangesAsync. That shape is a configuration-time error now, so the file pins three things instead of two: - an unbindable source fails loudly, naming the method, the reason, and both ways out. The original bug was that NOTHING happened - no mapping, no log, no error - and the user found out when a lookup returned null at runtime, so silence is the regression actually worth guarding. - the rename scenario works when the key comes from the event alone: a static [NaturalKeySource] returning the key type and taking IEvent<T>. This is #5042's failing case, and the strategy that could not bind before. - the rename scenario works for the reporter's own aggregate through the explicit NaturalKeyFor(x => x.SetBy/SetByEvent(...)) registration, which replaces what discovery found and clears the error. Swept the rest of the suite for projections quietly relying on a mapping that never existed - 34 natural key tests across EventSourcingTests and the other DaemonTests files, all green, so nothing else was depending on the silence. Docs: the handler-signature section described one strategy and warned that IEvent<T> "is not currently supported here, and silently ignored". Replaced with the three ranked strategies, when the blank-aggregate fallback is skipped and why, the loud failure, and a new section for NaturalKeyFor as the supported explicit path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(#5041): tie off the umbrella issue across all three reported items #5041 reported three natural key defects. Two are fixed in code across this PR and an already-merged one; the third is a "cannot be supported here" that was never written down. Closing it out means saying so with coverage. Item 1 - static IEvent<T> handlers not discovered - is the #5052 commit in this PR (#571 widened the extraction contract to the whole event). Item 2 - the old natural key not cleaned up - shipped in #5049. Its tests only exercise a source shape discovery could ALWAYS bind, though: a static handler taking the raw event. On the two paths #5052 newly enables the question could not even be asked before, because item 1 meant nothing was written for those event types at all. So assert retirement there too - the lookup table holds only the new key, and the retired one no longer resolves - on both the IEvent<T> source and the explicit NaturalKeyFor registration. Item 3 - handlers not receiving the current snapshot state - stays unsupported, because the lookup table is maintained inline at append time. That is exactly what makes a natural key lookup work under an Async snapshot lifecycle, where no aggregate exists yet, and it is why the key has to be a function of the event alone. #571 stopped papering over it: the blank-aggregate strategy is ranked last, skipped when the aggregate cannot be safely constructed, and reported loudly rather than silently producing a wrong key. The docs now say the method never sees the current aggregate, why that follows from when the write happens, and what to do instead (carry the value on the event). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Bump JasperFx/JasperFx.Events 2.36.0 -> 2.36.1 Picks up #572/#573, the lost-wakeup race this PR's CI found. 2.36.0's #568 narrowed the window between a ShardStatusWatcher's two ways of learning that its shard reached a sequence - an OnNext from the publication walk, or the current-state snapshot it reads right after subscribing - but left it open. A watcher could subscribe after the walk captured the listener list (so no OnNext) and read the snapshot before that state was recorded (so not there either), seeing it on neither path. Once a high water agent reaches the head it has nothing further to publish, so the wait could then only end in a timeout, however generous. 2.36.1 takes one lock across recording the state and capturing the listener list, and has the watcher subscribe and read the snapshot through it as a single atomic step, so the two sides are totally ordered: either the watcher wins and the following walk sees it, or the publication wins and the state is in the snapshot it gets back. That fixes HighWaterAgentTests.skips_multiple_gaps_and_keeps_advancing, which failed ~8 of 10 runs on net10.0 Release under 2.36.0. No Marten change was needed - deliberately, since widening the test's timeouts (tried, 9 of 10 still failed) would have masked a real defect on the public WaitForHighWaterMark path rather than fixing it. Verified on net10.0 Release: 10 of 10 runs of that test, and the full DaemonTests suite 250/250. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 27, 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.
Closes part of #5041. Repro credit to @ytqsl in #5042.
What was broken
NaturalKeyProjectiononly ever upserted intomt_natural_key_X. When an event renamed an aggregate's[NaturalKey], the row carrying the previous value stayed behind, still pointing at the same stream:Consequences:
natural_key_valueis part of the table's primary key, so the retired value permanently squats on its slot and no other stream can claim it;The docs already promised that "the old key value is replaced" — it wasn't.
The fix
NaturalKeyProjection.queueUpsertSqlnow queues a scopedDELETEahead of each upsert, clearing any other row for this stream (and tenant, when conjoined). It never touches a key legitimately owned by a different stream.QueueSqlCommandpreserves order within the batch, so a create-then-rename inside a single batch still lands on the newest value. The rebuild path (StartProjectionBatchAsync→QueueUpsertsForEvents) shares the same builder and is fixed for free.Tests
Three new tests in
rebuild_projection_with_natural_key_update.cs, all red before / green after:Existing
bug_4966_natural_key_should_be_updated_during_rebuildplus the 34 natural-key tests inEventSourcingTestsstill pass.What is not fixed here
The other two symptoms reported on #5041 are
[NaturalKeySource]discovery defects in JasperFx.Events, filed upstream as JasperFx/jasperfx#569:IEvent<T>handlers yield no extractor at all.buildExtractorbails out for anIEvent<>parameter (the extractor contract only receives event data), then the property-matching fallback finds nothing for a strong-typed key, so no mapping is registered — silently. The lookup table is simply never written for that event type.Instance
Apply(TEvent)handlers are invoked against a fabricated blank aggregate.Expression.New(TDoc)also bypassesrequiredmember enforcement, so any handler body touching other state throws — and it throws out of the caller'sSaveChangesAsync, i.e. a plain event append fails:Both repros from #5042 land here as
Bug_5041_natural_key_source_discovery, skipped with a pointer to the upstream issue, ready to unskip on the JasperFx.Events bump.The docs gain a "Handler Signature Requirements" section covering why the key must be derivable from the event alone and which signatures are actually supported today.
🤖 Generated with Claude Code