Skip to content

Adopt JasperFx.Events 2.36.0: shard failure classification, drain timeout docs, natural key extraction - #5054

Merged
jeremydmiller merged 6 commits into
masterfrom
fix/5047-5048-5052-jasperfx-2.36.0
Jul 26, 2026
Merged

Adopt JasperFx.Events 2.36.0: shard failure classification, drain timeout docs, natural key extraction#5054
jeremydmiller merged 6 commits into
masterfrom
fix/5047-5048-5052-jasperfx-2.36.0

Conversation

@jeremydmiller

@jeremydmiller jeremydmiller commented Jul 26, 2026

Copy link
Copy Markdown
Member

Adopts JasperFx/JasperFx.Events 2.36.0 and lands the Marten-side issues that ride with it. One commit per issue, plus the bump.

Closes #5047. Closes #5048. Closes #5052. Closes #5041.

Commits

Bump JasperFx/JasperFx.Events 2.35.0 -> 2.36.0 #564 + #565/#567 + #569/#571
docs(#5047) DaemonSettings.StopAndDrainTimeout
feat(#5048) IEventFailureContext + extended progression failure columns
fix(#5052) natural key extraction on the widened contract
test(#5041) ties off the umbrella issue across all three reported items

Why the bump is not purely a version change

NaturalKeyEventMapping.Extractor widens from Func<object, object?> to Func<IEvent, object?>, which is source breaking. Marten does not compile on 2.36.0 without adapting NaturalKeyProjection's two call sites, so those two lines ride in the bump commit to keep every commit buildable. The rest of #5052 is its own commit.

#5047 — drain timeout docs

New Graceful Shutdown and the Drain Timeout section: what the timeout bounds (the in-flight page plus the progression flush, across all three stop paths), the ProgressionProgressOutOfOrderException that follows a drain cut short, pairing a raised value with HostOptions.ShutdownTimeout and terminationGracePeriodSeconds, why a deployment might instead lower it, and what opting out with Timeout.InfiniteTimeSpan costs. Docs only.

#5048 — why a shard is down

Part 1. EventDeserializationFailureException declares EventSerialization and now retains the IEventType it was always handed but only used for the message string. UnknownEventTypeException declares UnknownEventType — kept separate on purpose, since a missing registration is a deployment fix rather than a data fix — and carries the sequence, threaded down from the row being read. ToDeadLetterEvent assigns its Id up front instead of leaving Guid.Empty for identity generation, so the creating process can correlate its ShardFailure with the row before that background write lands.

Part 2. Four columns behind the existing EnableExtendedProgressionTracking gate, read back onto ShardState.Failure. 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, 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 that conditional lives in one place. This retires Marten's use of mt_mark_event_progression_extended rather than growing its signature, following the reasoning already recorded on the batch overload: no schema object to migrate. The function stays installed for anything calling it directly.

Note

This adds a schema migration. Stores that already have EnableExtendedProgressionTracking on need ApplyAllConfiguredChangesToDatabaseAsync. The columns are nullable so no backfill is required, and until the migration runs the write degrades to no telemetry rather than failing anything — extended progression is best-effort by contract.

#5052 — natural key extraction

The two skipped tests from #5042 could not simply be un-skipped. Widening to the whole event fixes one half; the other half is now an error by design, because the reporter's aggregate declares required members and #571 stopped fabricating blank aggregates that C# itself would never have let you construct — that fabrication was the original ArgumentNullException out of SaveChangesAsync. So the file pins three things:

  • 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 — so silence is the regression actually worth guarding;
  • the rename works when the key comes from the event alone (a static [NaturalKeySource] returning the key type, taking IEvent<T>repro for https://github.com/JasperFx/marten/issues/5041 #5042's failing case, and the strategy that could not bind before);
  • the rename works for the reporter's own aggregate through the explicit NaturalKeyFor(x => x.SetBy/SetByEvent(...)) registration.

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.

#5041 — the umbrella issue

Three defects were reported. Two are fixed in code; the third is a cannot be supported here that had never been written down.

Item Where
1 static IEvent<T> handlers not discovered the #5052 commit here
2 old natural key not cleaned up shipped in #5049
3 handlers do not receive the current snapshot state unsupported by design — now documented

Item 2's existing tests only exercise a source shape discovery could always bind (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 this PR asserts retirement there too, on both the IEvent<T> source and the explicit registration.

Item 3 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 state that the method never sees the current aggregate, why that follows from when the write happens, and what to do instead.

Swept the rest of the suite for projections quietly relying on a mapping that never existed — 34 natural key tests in EventSourcingTests plus 7 in DaemonTests, all green, so nothing else depended on the silence.

Verification

Suite Result
CoreTests 493 passed, 1 skipped
EventSourcingTests 1470 passed, 7 skipped
DaemonTests 249 passed, 1 pre-existing failure
markdownlint + cspell clean over all of docs/

The DaemonTests failure is blue_green_side_effect_gate, which flaps pass/fail run-to-run on a clean worktree at the merge-base before any change here. Unrelated to this work — it touches neither natural keys, extended progression, nor event deserialization.

🤖 Generated with Claude Code

jeremydmiller and others added 5 commits July 26, 2026 07:39
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>
#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>
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>
…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>
#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

Copy link
Copy Markdown
Member Author

CI: HighWaterAgentTests.skips_multiple_gaps_and_keeps_advancing

The one failing job (test-daemon, net10.0) is not caused by anything in this PR's own commits — it comes from the JasperFx.Events 2.36.0 bump, and I've filed it upstream as JasperFx/jasperfx#572.

Bisected to the bump alone; a commit changing nothing but the pin reproduces it.

Result
This branch (2.36.0), net10.0 Release, unmodified test 8 of 10 fail
Clean master (2.35.0), same test 0 of 8 fail
Interleaved, to rule out DB/ordering effects 2.36.0 fails 2/3, 2.35.0 passes 3/3

It is not a daemon slowdown. With generous budgets both versions reach the head at an identical ~5.05s on the same per-gap cadence, and when the test passes Tracker.HighWaterMark equals the expected head — the mark is reached, the waiter just never observes it.

It is not a tight test deadline either. I tried raising the per-step budget 2s → 10s; still 9 of 10 failures. The wakeup is lost, not late, so widening the deadline cannot fix it — which is why this PR carries no test-timeout change.

Root cause is a race in ShardStateTracker/ShardStatusWatcher that #568 narrowed but did not close: publish walks a captured listener snapshot and refreshes _states from inside that same walk, so a watcher that subscribes mid-walk gets neither the OnNext nor a current-state snapshot containing it. Details and a suggested direction are in the upstream issue.

Holding this PR until an upstream fix ships.

jeremydmiller added a commit to JasperFx/jasperfx that referenced this pull request Jul 26, 2026
#568 gave ShardStatusWatcher a snapshot re-check so a state published before the
wait was set up could still be found. It narrowed the window but could not close
it, because the snapshot it re-read was written by the very publication walk it
was racing. ShardStateTracker subscribed itself first and updated its state map
from OnNext on the block's consumer thread, so a watcher could subscribe too late
for the walk AND read the snapshot before that walk had recorded the state.
Neither path saw it. With nothing further published for that shard -- exactly a
high water agent that has reached the head and has nothing left to detect -- the
wait could only end in a timeout, however generous. Marten's
HighWaterAgentTests.skips_multiple_gaps_and_keeps_advancing hit this reproducibly
on 2.36.0 and never on 2.35.0, because the new snapshot path let the earlier waits
complete instantly and the test raced ahead into the window.

Two changes close it:

PublishAsync now records the state synchronously, before posting to the block.
Delivery to listeners stays asynchronous, but "what the tracker knows" is now
synchronous with the caller, so a snapshot read can never lag a publication that
has already happened. That also removes the second writer of the state map -- the
consumer thread replaying an older state could momentarily walk it backwards.

Subscribing and reading the snapshot is now one atomic step
(SubscribeAndCaptureCurrentStates) taken under the same lock that records the
state and captures the listener list for a walk. That totally orders the two
sides: either the watcher wins the lock and the following walk sees it as a
listener, or the publication wins and the state is in the snapshot handed back.
Observers are still notified outside the lock, so user code in OnNext cannot
deadlock the tracker by publishing or subscribing.

The listener list and state map were also both read-modify-write on plain fields,
so two concurrent Subscribe calls could drop one of the subscribers outright --
a permanently lost wakeup rather than a delayed one. All mutations now take the
lock, and HighWaterMark is read and written volatile.

Reported from JasperFx/marten#5054.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
@jeremydmiller

Copy link
Copy Markdown
Member Author

Resolved by JasperFx.Events 2.36.1

jasperfx#572 is fixed and shipped (jasperfx#573, "close the lost-wakeup race rather than narrowing it"). Pins bumped 2.36.0 → 2.36.1.

The fix makes subscribe-and-read-snapshot atomic against the publication walk: ShardStateTracker now takes one lock across recording a published state and capturing its listener list, and ShardStatusWatcher goes through it via a new SubscribeAndCaptureCurrentStates. The two sides are totally ordered, so a watcher can no longer fall between them and see the state on neither path.

Verification on net10.0 Release — the same protocol that produced the failures:

2.36.0 2.36.1
skips_multiple_gaps_and_keeps_advancing, 10 runs 2 passed / 8 failed 10 passed / 0 failed
Full DaemonTests suite 249 / 1 failed 250 / 0 failed

Ten consecutive runs rather than one, deliberately: this bug passed roughly 1 in 10 by chance, so a single green run would have proved nothing.

No Marten code changed for this. The test is untouched — widening its timeouts was tried and failed (9 of 10 still failed), and would have masked a real defect on the public WaitForHighWaterMark path rather than fixing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant