Skip to content

Fix raised events going to every stream, plus IsValidNumber and ReadExpectedBufferAsync false positives - #1098

Merged
jeremydmiller merged 3 commits into
mainfrom
fix/raised-events-and-read-helpers
Nov 2, 2026
Merged

jeremydmiller merged 3 commits into
mainfrom
fix/raised-events-and-read-helpers

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

Three defects on main and in released 2.x packages, found by reading code and confirmed here with tests that failed first. One commit each.

Raised events went to every stream

EventSlice.BuildOperations grouped RaisedEvents by stream, then gave every group all of them. Beyond the duplication, StreamAction.AddEvent stamps the stream identity onto the IEvent, and the instances were shared between actions, so each later group re-tagged events already queued for earlier ones. Both identity branches had this.

There was a second defect beneath it. A single stream slice that starts its stream numbers raised events from _events.Count, which is only true of its own stream, but it applied that to every group. This is the shape Marten's docs recommend in TripProjectionWithEventMetadata: a raise to this stream plus a raise to another. The other stream got a version and expected version predicted from an unrelated event count.

Now each stream gets only its events, only the slice's own stream is numbered, and other streams go through QuickAppendEvents, as multi stream projections already do. Marten's mt_quick_append_events creates a missing stream, so a foreign stream that does not exist yet works. A slice raising only to itself is unchanged, and two tests pin that.

The tests drive RaiseSideEffects through the real AggregationRunner into a recording batch, and read the actions after all of them are built, because a store executes them at commit. This has not run against a real store: the in-memory store has no daemon and does not enrol ProjectionSideEffectCompliance, and that suite has no cross-stream fact, so nothing in this repository's gate covers it end to end.

IsValidNumber: fixed, not deprecated

The unescaped group separator was a wildcard on de-DE, so "1x234" was valid. "" and "-" were valid too, and so was "123\n", which was not in the original report: $ matches before a trailing newline.

Nothing in Marten, Polecat, Fisher, Weasel, Wolverine or CritterWatch calls it. It is fixed anyway because [Obsolete] would leave the wrong answers in place for outside callers for the rest of 2.x, and there is no equivalent to point them at: decimal.TryParse accepts "1,00", which this rejects by design. Whether to delete it in 3.0 is a separate question this does not prejudge.

ReadExpectedBufferAsync matched padding

Confirmed: a stream holding [1] matched [1, 0]. An empty stream also matched [0, 0]. It now uses Stream.ReadExactlyAsync, and the existing catch turns end of stream into false.

ReadBytesAsync still zero-pads, deliberately. Its contract is now documented, and a test pins it. Throwing looks like the obvious fix, but Wolverine depends on the padding (src/Wolverine/Transports/Tcp/WireProtocol.cs):

Line Call Today on end of stream If ReadBytesAsync threw
86 length prefix zeros, so length == 0 and a quiet return LogError, then a write to the closed socket
99 frame body zero-padded frame gets parsed clean failure
59 send confirmation matches no branch, batch gets no callback exception out of SendBatchAsync
128, 137 ReadExpectedBufferAsync result discarded no change

Line 86 is why this stays: every connection that closes without sending, such as a TCP health probe, would become an error in any installed Wolverine that takes a newer JasperFx transitively. Returning a short array has the same result, because BitConverter rejects it. This comes from reading that code, not from running Wolverine against this branch.

Lines 59 and 99 remain real hazards. Fixing them properly starts in Wolverine: move to ReadExactlyAsync and handle end of stream at the length prefix deliberately. After that, ReadBytesAsync can throw or go away in 3.0.

Reaching 3.0

A trial merge conflicts in two files, because #1041 already changed them on 3.0:

  • StreamExtensions.cs: keep this branch's ReadExactlyAsync read and 3.0's expected.AsSpan().SequenceEqual(bytes). Taking 3.0's side of the hunk silently brings the bug back.
  • StreamExtensionsTests.cs: both sides append tests, so keep both. The names do not collide.

StringExtensions.cs auto-merges correctly, and EventSlice.cs is untouched on 3.0.

Verification

./build.sh Test is green in this worktree: all 14 targets, on net9.0 and net10.0 where the project targets both. EventTests 1655 passed and CoreTests 810 passed with 1 skipped. This branch adds no skips.

🤖 Generated with Claude Code

jeremydmiller and others added 3 commits November 1, 2026 06:27
EventSlice.BuildOperations grouped RaisedEvents by stream and then handed
every group all of them, so a slice raising to two streams wrote each event
to both. It was worse than duplication: StreamAction.AddEvent stamps the
stream identity onto the IEvent, and the instances were shared between the
actions, so each later group re-tagged the events already queued for the
earlier ones. The string identity branch had the same defect.

The version numbering had a second one. A single stream slice that starts
its stream numbers raised events from _events.Count, which is only true of
the slice's own stream, but it was applied to every group. That is the shape
Marten's docs recommend, a raise to this stream plus a raise to another, and
it predicted the other stream's version and expected version from an
unrelated event count. Only the slice's own stream is numbered now, and the
store numbers the rest through QuickAppendEvents, as it already does for
multi stream projections.

The two branches differed only in their identity, so the append moves to one
helper. A slice raising only to itself is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The group separator went into the pattern unescaped, so on de-DE, where it
is ".", it matched any character and "1x234" was valid. Both parts of the
pattern were optional, so "" and "-" were valid too, and $ matches before a
trailing newline, so "123\n" was as well.

Both separators are escaped now, the pattern has to find a digit, and it
ends at \z. Everything that was a number before still is, including a bare
fraction like ".5".

This is fixed rather than deprecated even though nothing in the Critter
Stack calls it. [Obsolete] would leave the wrong answers in place for any
outside caller for the rest of 2.x, and there is no equivalent to send them
to: decimal.TryParse accepts "1,00", which this rejects by design.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ReadBytesAsync hands back a zero-padded array when the stream ends early, so
ReadExpectedBufferAsync compared the padding against what it expected. A
stream holding [1] matched [1, 0], and an empty stream matched [0, 0]. It
now reads with Stream.ReadExactlyAsync, and its existing catch turns the
EndOfStreamException into false.

ReadBytesAsync itself is left as it is on 2.x, with its contract documented
and pinned by a test, because Wolverine depends on it. WireProtocol reads its
length prefix through it and returns quietly on a zero length, which is how
a connection that closes without sending anything, like a TCP health probe,
is ignored. Throwing there, or returning a short array that BitConverter
then rejects, would turn each of those into an error log and a write to a
closed socket in every installed Wolverine that takes a newer JasperFx.
Wolverine's acknowledgement reads discard this method's result, so they do
not change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jeremydmiller
jeremydmiller merged commit 1f717c9 into main Nov 2, 2026
1 check passed
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