Repository navigation
Snapshot support (as an event) - #479
alexey-troshkin-xpress wants to merge 27 commits into
Conversation
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||||||||||
alexey-troshkin-xpress
changed the title
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
Test Results 54 files + 35 54 suites +35 27m 45s ⏱️ + 16m 37s For more details on these failures, see this check. Results for commit a06a726. ± Comparison against base commit 2bcac37. This pull request removes 5 and adds 12 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
alexey-troshkin-xpress
changed the title
fe1f82c to
8cfd469
Compare
…age strategies. Added logic to retrieve and register storage strategies for snapshots, ensuring proper handling during snapshot type registration
…vice; implement snapshot handling logic for SeparateStream and SeparateStore strategies
# Conflicts: # Eventuous.slnx # src/Core/gen/Eventuous.Shared.Generators/Constants.cs
nmummau
left a comment
nmummau
left a comment
There was a problem hiding this comment.
I’ve tested this implementation locally against commit a06a726 using Testcontainers with PostgreSQL, Redis, MongoDB, SQL Server, and KurrentDB, plus focused source-generator tests.
The regression tests, reproduction commands, and captured test reports are available in this commit. They run against the unchanged snapshot implementation at a06a726.
The tests reproduce several correctness issues: backward pagination repeats pages, Redis can combine one snapshot’s revision with another’s payload, snapshot persistence failures report command failure after domain events commit, and generic snapshot attributes emit no registration.
I also reproduced incorrect revision handling for snapshot-only commands and explicitly returned snapshots inside a command batch. I’ll add the specific reproductions inline so they can be addressed separately.
This looks worth moving forward. My main concern is ensuring snapshot loading reproduces both the state and stream revision of full replay.
Test-by-test outcomes and captured failures: RESULTS.md. Reproduction commands: README.md. The raw final report is archived in the same commit.
| var database = _getDatabase(); | ||
| var key = GetSnapshotKey(streamName); | ||
|
|
||
| var revisionValue = await database.HashGetAsync(key, Revision).NoContext(); |
There was a problem hiding this comment.
These separate reads can combine the revision from one snapshot with the payload from a newer snapshot if a write occurs between them.
I reproduced this against real Redis by replacing the snapshot after the revision read completes. The loader receives revision 9 with state already covering revision 10, then replays event 10 again. Expected: 11 applied events, balance 5.00. Actual: 12 applied events, balance 6.25.
Could revision, event type, and payload be fetched together using a single multi-field HashGetAsync call?
Regression test · Passing no-replacement control · Recorded results
|
|
||
| if (events.Length < pageSize) break; | ||
|
|
||
| position = new(position.Value - events.Length); |
There was a problem hiding this comment.
Reproduced against KurrentDB: when the snapshot is behind at least 500 tail events, this repeatedly reads the same page. A stream containing exactly 500 events and no snapshot also reproduces it. The second cursor is long.MaxValue - 500, rather than the revision before the oldest returned event. Could the next cursor derive from the returned revision, with an explicit beginning-of-stream check?
There was a problem hiding this comment.
Regression tests: snapshot beyond the first page and no snapshot at page boundaries. All six parameterized cases fail against the reviewed implementation by exceeding the bounded backward-read budget.
|
|
||
| // Handle snapshots based on strategy | ||
| if (snapshotEvents.Length > 0 && storageStrategy != SnapshotStorageStrategy.SameStream) { | ||
| await HandleSnapshots(streamName, snapshotEvents, storeResult.NextExpectedVersion, storageStrategy, resolvedWriter, cancellationToken).NoContext(); |
There was a problem hiding this comment.
When a command produces a snapshot but no domain events, the empty append returns revision -1. Both command-service styles then use that as the snapshot revision. With an existing stream at revision 2, PostgreSQL and SQL Server reject the write, while Redis and MongoDB persist -1. Could this path preserve the loaded stream revision when no domain events are appended?
There was a problem hiding this comment.
Regression test: snapshot-only command preserves the existing revision. It covers both command-service styles and all four external stores; all eight cases fail against the reviewed implementation.
| var changes = result.Select(x => Change.FromEvent(x, _typeMap)); | ||
|
|
||
| // Handle snapshots based on strategy | ||
| if (snapshotEvents.Length > 0 && storageStrategy != SnapshotStorageStrategy.SameStream) { |
There was a problem hiding this comment.
This new snapshot operation runs after the domain events have committed, but inside the same exception handler. I reproduced that a snapshot-write failure makes both command-service styles report command failure despite the durable domain event. Retrying a deposit of 10 then produces balance 20. Could snapshot persistence failure be distinguished from domain-command failure so callers don’t retry an already committed operation?
There was a problem hiding this comment.
Reproduction test: snapshot failure after commit and duplicate retry. Both service-style cases pass because the test explicitly asserts the undesirable outcome: a reported failure, a durable event, and a duplicate business effect on retry.
| }; | ||
| } | ||
|
|
||
| static HashSet<string> GetTypesFromSnapshotsAttribute(AttributeData attributeData) { |
There was a problem hiding this comment.
Compiling declarations using the generic snapshot attribute produces no expected snapshot registration. Equivalent non-generic declarations pass, including multiple snapshot contracts. This extraction inspects constructor and named arguments but does not extract AttributeClass.TypeArguments. The generic declaration shape used by the banking sample needs coverage here.
| // Separate snapshots from regular events based on storage strategy | ||
| var snapshotTypes = SnapshotTypeMap.GetSnapshotTypes<TState>(); | ||
| var storageStrategy = SnapshotTypeMap.GetStorageStrategy<TState>(); | ||
| var (regularEvents, snapshotEvents) = SeparateSnapshots(newEvents, snapshotTypes, storageStrategy); |
There was a problem hiding this comment.
If a command returns a snapshot between two domain events, this separation loses the snapshot’s position in the batch. For example, add 10 → snapshot balance 10 → add 5 should reload as 15. Instead, the snapshot is saved with the revision of the second addition, so loading skips that addition and returns 10. Could snapshots be required to come last, or retain the revision they actually represent?
There was a problem hiding this comment.
Regression test: explicit snapshot inside a batch. The test handler deliberately returns those three objects from one command. The test fails because the reloaded balance is 10 instead of 15. This is the manually returned snapshot path; automatic snapshot production appends its snapshot last.
|
@alexey-troshkin-xpress could you rebase or merge current dev into the PR? Its common ancestor with One concrete consequence: our tests reproduced PostgreSQL concurrent appends reporting success while dropping an event, but that is an existing provider bug already fixed in merged PR #582, commit Updating the branch would bring in that fix and let us distinguish current snapshot defects from old provider behavior. Our existing evidence remains pinned to |
|
|
||
| public string ReadSnapshot => $"SELECT revision, event_type, json_data FROM {schema}.snapshots WHERE stream_name = @stream_name"; | ||
| public string WriteSnapshot | ||
| => $"INSERT INTO {schema}.snapshots (stream_name, revision, event_type, json_data) VALUES (@stream_name, @revision, @event_type, @json_data) ON CONFLICT (stream_name) DO UPDATE SET revision = @revision, event_type = @event_type, json_data = @json_data, created = now() at time zone 'utc'"; |
There was a problem hiding this comment.
Should ISnapshotStore guarantee that writes cannot move the represented revision backwards? I reproduced a delayed revision-100 write replacing an already stored revision-110 snapshot in PostgreSQL, Redis, MongoDB, and SQL Server. Each payload can be individually valid, but the older write becomes the one returned by Read.
With complete event history retained, this increases replay work rather than inherently corrupting state. If applications truncate history based on a newer snapshot, accepting an older snapshot would no longer be safe. Could the adapters atomically ignore older revisions, with an explicit policy for equal-revision writes? Alternatively, the contract should document last-completed-write behavior and the requirement to retain replayable history.
| /// <summary> | ||
| /// Store snapshots in the same stream as other aggregate events | ||
| /// </summary> | ||
| SameStream, |
There was a problem hiding this comment.
With SameStream, snapshots will also be delivered to subscriptions. Typed handlers can ignore unhandled types, but catch-all handlers, gateways, and independent consumers may need explicit filtering.
Should this PR provide a way for subscriptions to identify and ignore snapshot events—perhaps through metadata, allowing filtering before deserialization—while still advancing checkpoints correctly?
This also seems relevant to SeparateStream when subscribing to all streams. Some consumers may intentionally want snapshots, so making this configurable seems useful.
|
|
||
| var snapshotAppend = new ProposedAppend( | ||
| snapshotStreamName, | ||
| ExpectedStreamVersion.Any, |
There was a problem hiding this comment.
Could a delayed snapshot write move the represented domain revision backwards here? For example, writer A commits domain revision 100 and pauses; writer B commits through 110 and saves its snapshot; then A appends snapshot(100) using Any.
The loader appears to select the last appended snapshot, so it would choose snapshot(100). The subsequent truncation could also remove snapshot(110), since it occupies an earlier snapshot-stream position.
With complete domain history this means extra replay, but it could become unsafe if an application truncates domain history based on the newer snapshot.
Should we compare represented domain revisions and use the snapshot stream’s expected version together to prevent this? The aggregate command service appears to have the same path. This is based on reading the code; I haven’t reproduced the SeparateStream interleaving yet.
User description
Added snapshot support according to the description in #468, except that the attribute is called Snapshots instead of HasRollingSnapshots, registration is done through source generators, and also added a project with an example of usage
PR Type
Enhancement
Description
Added rolling snapshot support with
Snapshotsattribute for state typesImplemented source generator to auto-register snapshot type mappings
Added
ReadStreamAfterSnapshotmethod to read events from latest snapshotCreated banking sample demonstrating snapshot usage with deposit/withdraw operations
Diagram Walkthrough
File Walkthrough
7 files
Core snapshot type registration and attribute definitionSource generator for automatic snapshot type mappingHelper utility for global type name formattingRefactored to use shared MakeGlobal helper methodImplemented ReadStreamAfterSnapshot for backward event readingIntegrated snapshot support in LoadState methodIntegrated snapshot support in LoadAggregate method9 files
Added snapshot attribute constants for generatorsDomain project file with core dependenciesAPI project file with KurrentDB and generator referencesAppHost project file with Aspire configurationAPI application settings configurationAPI development logging configurationAppHost application settings configurationAppHost development logging configurationAdded banking sample projects to solution5 files
State with Snapshots attribute and snapshot event handlerDomain events including snapshot event definitionCommand service with snapshot creation logicAPI setup with KurrentDB and command service registrationAspire AppHost configuration for banking sample1 files
Documentation for snapshot feature usage1 files
Added KurrentDB Aspire package versions