fix: Event Store Tests for Large Event Counts (>100 events)#224
Conversation
WalkthroughThe pull request introduces array chunking functionality, refactors event position advancement logic in Postgres event streaming, optimizes query ordering, and restructures test infrastructure to use batch insertion patterns. Test coverage expands to include high-volume event scenarios, and timing adjustments enhance test reliability. Changes
Sequence Diagram(s)sequenceDiagram
participant Reader as Forward Reader
participant Pos as Position State
participant DB as Database
rect rgb(220, 240, 255)
Note over Reader,Pos: Direction-Aware Position Advancement
end
Reader->>DB: Query events from position P
DB-->>Reader: Events returned (success or error)
alt Forward Read
Reader->>Pos: Advance position by +1
else Backward Read
alt GlobalPosition
Reader->>Pos: Decrement position by -1
else LocalPosition
Reader->>Pos: Maintain prior position
end
end
Pos-->>Reader: Updated position for next batch
sequenceDiagram
participant Setup as Test Setup
participant Batch as Batch Insert
participant Derive as Position Derivation
participant Context as Test Context
rect rgb(240, 220, 255)
Note over Setup,Context: Old Pattern (Per-Entity Incremental)
end
Setup->>Setup: Generate event for entity1
Setup->>Batch: Insert event
Batch-->>Setup: Position from result
Setup->>Setup: Generate event for entity2
Setup->>Batch: Insert event
Batch-->>Setup: Position from result
Setup->>Context: Construct with individual positions
rect rgb(220, 255, 220)
Note over Setup,Context: New Pattern (Batch with Precomputed Positions)
end
Setup->>Setup: Precompute all insertions for entity1
Setup->>Setup: Precompute all insertions for entity2
Setup->>Batch: Batch insert entity1 in chunks of 100
Setup->>Batch: Batch insert entity2 in chunks of 100
Setup->>Derive: Derive positions from index ranges
Derive-->>Setup: StreamPosition array
Setup->>Context: Construct with allInsertions + derived positions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @NickSeagull's task —— View job PR Review ProgressReviewing PR #224: Event Store Tests for Large Event Counts Todo List:
✅ Code Review SummaryThis PR successfully addresses event store test failures with large event counts by implementing proper batch processing and fixing position tracking bugs. The implementation is well-structured and follows NeoHaskell conventions. 🎯 Key Improvements1. Array.chunksOf Implementation (
2. Event Store Batch Processing Fix
3. Position Tracking Fixes (
4. SQL Query Fix (
5. Test Infrastructure Improvements
🔍 Code Quality AssessmentNeoHaskell Style Compliance: ✅
Performance: ✅
Security: ✅
Test Coverage: ✅
🐛 Issues Found: None CriticalMinor Observations:
🎯 Recommendations
🚀 Performance Impact
Overall Assessment: ✅ APPROVED This is a high-quality fix that addresses a critical data integrity issue. The implementation follows NeoHaskell conventions, includes comprehensive tests, and properly handles edge cases. The code is production-ready. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (10)
core/core/Array.hs(2 hunks)core/service/Service/EventStore/Postgres/Internal.hs(4 hunks)core/service/Service/EventStore/Postgres/Internal/Sessions.hs(1 hunks)core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Context.hs(2 hunks)core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Spec.hs(2 hunks)core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Context.hs(1 hunks)core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Spec.hs(1 hunks)core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Context.hs(1 hunks)core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Spec.hs(1 hunks)core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests-linux
- GitHub Check: tests-macos
- GitHub Check: claude-review
🔇 Additional comments (13)
core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Spec.hs (1)
28-32: Behold, the expansion of test realms into high-volume territories!The addition of
TEST_EVENT_COUNT_HIGHenvironment-gated tests (10k, 100k, 1M events) aligns perfectly with the PR objectives. This separation ensures swift regular test runs while enabling stress testing when summoned.core/testlib/Test/Service/EventStore/Subscriptions/Spec.hs (1)
53-53: A wise augmentation of temporal tolerance!The increase from 10ms to 100ms provides appropriate breathing room for asynchronous notification processing, enhancing test reliability across diverse execution environments without materially impacting overall test duration.
core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Spec.hs (1)
28-32: The backwards realm receives equal fortification!This mirrors the high-volume test expansion in the forward-reading tests, ensuring comprehensive coverage for both traversal directions. The implementation is consistent and correct.
core/core/Array.hs (3)
52-52: The public interface expands with purpose!The export of
chunksOfis properly positioned and documented, making this essential batching utility available to all who seek it.
662-663: A trivial yet tidy refinement!Minor formatting adjustment with no functional impact.
665-691: A well-crafted partition function emerges!The
chunksOfimplementation is correct and well-documented:
- Properly handles edge cases (empty input, uneven final chunk)
- Clear recursive logic via the local
chunkListhelper- Comprehensive documentation with illustrative examples
The conversion path (Vector → List → chunked sublists → Vectors → Array of Arrays) is straightforward and appropriate for the batching use case, where the overhead is negligible compared to I/O operations.
core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Context.hs (1)
37-49: The transition to batched insertions is executed with precision!The refactoring respects the 100-event batch limit enforced by the event store:
- Insertions are precomputed and chunked appropriately
- Both entities are processed in batches
- Results are discarded as positions are now derived independently
This approach eliminates the silent truncation that plagued the previous implementation when attempting to insert 1,000+ events in a single operation.
core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Context.hs (2)
38-60: Batched insertions with result collection, executed flawlessly!The implementation correctly:
- Chunks insertions into batches of 100
- Collects insertion results via
Task.mapArrayfor both entities- Preserves result metadata needed to determine
maxGlobalPositionThis approach aligns with the event store's batch limit while enabling backward-read tests to start from the correct end position.
62-67: The maximum position is determined with sound logic!The derivation correctly:
- Combines results from both entities
- Extracts global positions from insertion results
- Finds the maximum to establish the end position for backward reads
The default of
StreamPosition 0serves as a reasonable fallback, though in practice it should never be reached with non-empty test data.core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Context.hs (1)
22-45: A more elegant design manifests itself!This Context structure is notably cleaner than its ReadAll counterparts:
- Directly exposes
allInsertionswithout unnecessary wrapper layers- Eliminates the redundant
generatedEventsfield- Positions are derived straightforwardly from the range
The batched insertion pattern (chunks of 100) is consistently applied, and the simpler structure makes the test data flow more transparent.
core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Spec.hs (2)
27-30: The high-volume test expansion reaches this domain as well!Consistent with the pattern established in other test files, enabling stress testing of individual stream ordering at scale.
79-89: The test validation adapts to the refined Context structure!The changes correctly align with the Context refactoring:
- Event pairing now uses
context.allInsertionsdirectly- Simplified from
((payload, insertion), event)to(insertion, event)- Entity and stream IDs sourced from
contextfields rather than per-payload wrappersThis cleaner approach improves code clarity while maintaining equivalent validation logic.
core/service/Service/EventStore/Postgres/Internal/Sessions.hs (1)
351-357: LocalPosition ordering is the correct key for per‑stream paginationAligning the
ORDER BYwith theLocalPositionfilter is consistent with the rest of the stream‑scoped logic (including truncation), and is required for the new local‑position pagination inperformReadStreamEventsto behave deterministically per stream. This change is correct and well‑scoped.
| -- Update position for next batch | ||
| -- For forward: increment by 1 (since we use >= comparison) | ||
| -- For backward: decrement by 1 (since we use <= comparison for global position) | ||
| case readDirection of | ||
| Just Backwards -> positionRef |> Var.set (record.globalPosition - 1) | ||
| _ -> positionRef |> Var.set (record.globalPosition + 1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Direction‑aware global position advancement is logically sound but tightly coupled
The new positionRef updates in performReadAllStreamEvents correctly mirror the intended pagination semantics:
- Forward reads:
positionRef := record.globalPosition + 1matches aGlobalPosition >= positionfilter and prevents re‑reading the last item of the previous batch. - Backward reads:
positionRef := record.globalPosition - 1matches aGlobalPosition <= positionfilter and steps strictly past the item just read.
Mirroring this logic in both the Ok and toxic branches is essential to avoid getting stuck on toxic records and is done correctly here.
Be aware that this logic is tightly coupled to toPostgresGlobalPositionComparison using >= for forwards and <= for backwards as the comments state. If those comparisons ever change (e.g., to > / <), this block will silently become off‑by‑one. At minimum, keep tests around these edge cases to guard that contract.
Also applies to: 543-548
🤖 Prompt for AI Agents
In core/service/Service/EventStore/Postgres/Internal.hs around lines 523-528
(and also apply same change at lines 543-548), the positionRef advancement must
be kept identical for both Ok and toxic branches and remain direction-aware:
ensure that when readDirection is Just Backwards you set positionRef to
(record.globalPosition - 1) and otherwise to (record.globalPosition + 1) in
every branch that advances the global position (including toxic/error handling
paths), and add/keep unit/integration tests that assert correct forward/backward
pagination behavior to guard the coupling with
toPostgresGlobalPositionComparison (>= for forwards, <= for backwards).
| -- For forward: increment by 1 (since we use >= comparison) | ||
| -- For backward: keep same position (since we use < comparison which already excludes it) | ||
| case readDirection of | ||
| Just Backwards -> positionRef |> Var.set record.localPosition | ||
| _ -> positionRef |> Var.set (record.localPosition + 1) | ||
| Err err -> do |
There was a problem hiding this comment.
Local‑position pagination is correct, but Start handling will not advance for large streams
The per‑stream pagination logic in performReadStreamEvents is internally consistent:
- For forward reads,
positionRef := record.localPosition + 1matches aLocalPosition >= positionfilter and avoids re‑reading the last item in the previous batch. - For backward reads,
positionRef := record.localPositionmatches aLocalPosition < positionfilter and naturally walks backwards without re‑reading the same event.
Again, duplicating this logic in both Ok and toxic branches is correct and prevents getting stuck on malformed events.
However, there is a deeper behavioral issue when combined with selectStreamEventBatch:
- In
selectStreamEventBatch,positionFilteris disabled entirely whenrelativeisJust StartorJust End. readAllStreamEventsImplcallsperformReadStreamEventswithrelative = Just StartandLimit maxValue.
For streams with more than batchSize events, this means:
- Every iteration of the loop queries
WHERE Entity = ... AND InlinedStreamId = ...without aLocalPositionbound. - Because
ORDER BY LocalPosition ASC LIMIT batchSizeis used, each iteration returns the same firstbatchSizeevents. positionRefis updated, but never respected by the SQL forJust Start, so pagination does not advance.- The loop terminates only when
remainingLimitRefhits zero, soreadAllStreamEventsImplwill emit many duplicate events and perform unnecessary queries.
You must adjust one side of this contract:
- Either remove the
Just Start/Just Endspecial‑case inselectStreamEventBatchso theLocalPositionfilter is always applied based onpositionRef(withpositionRefinitialised appropriately for “start” / “end”), or - Change
readAllStreamEventsImplto callperformReadStreamEventswith a relative that uses the filter (FromAndAfter (StreamPosition 0)for forward “from the beginning”, and a correspondingBeforeform for backwards), letting the existing pagination logic drive the bounds.
Without such a change, readAllStreamEventsImpl will not behave correctly on large streams.
Also applies to: 825-830
🤖 Prompt for AI Agents
In core/service/Service/EventStore/Postgres/Internal.hs around lines 808-813
(also apply same change at 825-830): calling performReadStreamEvents with
relative = Just Start disables the SQL position filter in selectStreamEventBatch
so pagination never advances on large streams; instead change
readAllStreamEventsImpl to pass a relative value that enforces the position
filter (e.g. use Just (FromAndAfter (StreamPosition 0)) for forward/from-start
reads, and the corresponding Before/FromAndBefore form for backward/end reads)
so the database WHERE LocalPosition clause is respected and positionRef updates
drive pagination.
| -- Create payload wrappers for compatibility (not used in tests, but kept for API consistency) | ||
| let entity1Events = | ||
| entity1Insertions | ||
| |> Array.map | ||
| ( \insertion -> | ||
| Event.InsertionPayload | ||
| { streamId, | ||
| entityName = entity1Id, | ||
| insertionType = Event.AnyStreamState, | ||
| insertions = Array.wrap insertion | ||
| } | ||
| ) | ||
|
|
||
| let entity2Events = | ||
| entity2Insertions | ||
| |> Array.map | ||
| ( \insertion -> | ||
| Event.InsertionPayload | ||
| { streamId, | ||
| entityName = entity2Id, | ||
| insertionType = Event.AnyStreamState, | ||
| insertions = Array.wrap insertion | ||
| } | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Compatibility wrappers appear once more, their purpose obscure!
As with the forward-reading context, these wrappers reconstruct single-insertion payloads that are not used for actual insertion. The generatedEvents field may be unnecessary if no tests depend on it.
🏁 Script executed:
#!/bin/bash
# Verify usage of generatedEvents in backwards reading tests
rg -n -C3 'generatedEvents' --type=hs -g '!Context.hs' -g 'Test/Service/EventStore/ReadAllBackwardsFromEnd/*.hs'Length of output: 148
🏁 Script executed:
#!/bin/bash
# First, locate the test directory and list relevant files
echo "=== Finding ReadAllBackwardsFromEnd test files ==="
fd -t f -e hs 'ReadAllBackwardsFromEnd' . | head -20
echo ""
echo "=== Searching for generatedEvents usage in ReadAllBackwardsFromEnd directory ==="
rg -n 'generatedEvents' --type=hs 'core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/' 2>/dev/null || echo "Search completed"
echo ""
echo "=== Broader search: generatedEvents in all event store test contexts ==="
rg -n 'generatedEvents' --type=hs 'core/testlib/Test/Service/EventStore/' 2>/dev/null || echo "Search completed"Length of output: 1026
🏁 Script executed:
#!/bin/bash
# Search for actual usage of generatedEvents field in test implementations
echo "=== Searching for field access to generatedEvents ==="
rg -n '\.generatedEvents|generatedEvents\s*->' --type=hs 'core/testlib/Test/Service/EventStore/' 2>/dev/null
echo ""
echo "=== Search for any reference to generatedEvents outside Context.hs ==="
rg -n 'generatedEvents' --type=hs 'core/testlib/Test/Service/EventStore/' --invert-match -l | xargs -I {} rg -n 'generatedEvents' {}
echo ""
echo "=== List all test files in ReadAllBackwardsFromEnd directory ==="
fd -t f -e hs . 'core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/'Length of output: 578
🏁 Script executed:
#!/bin/bash
# Examine the Spec.hs file to see how context is used in tests
echo "=== Content of ReadAllBackwardsFromEnd/Spec.hs ==="
cat 'core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Spec.hs'Length of output: 16303
Remove unused generatedEvents field from Context record.
Verification confirms the field is dead code: it is assigned at line 99 but never accessed anywhere in the test suite or broader codebase. The comment claiming "API consistency" cannot be justified when no consumers exist. Lines 69-92 (the entity event wrapper creation) and the assignment at line 99 should be deleted entirely.
🤖 Prompt for AI Agents
In core/testlib/Test/Service/EventStore/ReadAllBackwardsFromEnd/Context.hs
around lines 69 to 99, remove the dead/unused generatedEvents field and the
creation of the entity1Events/entity2Events wrappers: delete lines 69–92 (the
Array.map blocks that build Event.InsertionPayloads) and remove the assignment
that sets generatedEvents at line 99; also remove the generatedEvents field from
the Context record definition and any related pattern matches or construction
sites in this file so the record no longer includes that unused field.
| -- Create payload wrappers for compatibility (not used in tests, but kept for API consistency) | ||
| let entity1Events = | ||
| entity1Insertions | ||
| |> Array.map | ||
| ( \insertion -> | ||
| Event.InsertionPayload | ||
| { streamId, | ||
| entityName = entity1Id, | ||
| insertionType = Event.AnyStreamState, | ||
| insertions = Array.wrap insertion | ||
| } | ||
| ) | ||
|
|
||
| let entity2Events = | ||
| entity2Insertions | ||
| |> Array.map | ||
| ( \insertion -> | ||
| Event.InsertionPayload | ||
| { streamId, | ||
| entityName = entity2Id, | ||
| insertionType = Event.AnyStreamState, | ||
| insertions = Array.wrap insertion | ||
| } | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Behold, compatibility wrappers that serve no active purpose!
These wrappers reconstruct the old single-insertion payload structure but are not used for actual insertion. The comment acknowledges they exist "for API consistency."
However, I observe no usage of context.generatedEvents in the associated test file. If downstream consumers do not rely on this field, consider removing it to simplify the Context structure and eliminate unnecessary allocations.
🏁 Script executed:
#!/bin/bash
# Search for usage of generatedEvents field in test files
rg -n -C3 'generatedEvents' --type=hs -g '!Context.hs'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for generatedEvents across entire codebase
rg -n 'generatedEvents' --type=hsLength of output: 535
🏁 Script executed:
#!/bin/bash
# Find the definition of generatedEvents in Context.hs
ast-grep --pattern 'generatedEvents'Length of output: 535
🏁 Script executed:
#!/bin/bash
# Check Context.hs for the field definition and exports
fd -e hs -p '*Context.hs' | xargs -I {} bash -c "echo '=== {} ===' && head -50 {} && echo '...' && tail -50 {}"Length of output: 290
🏁 Script executed:
#!/bin/bash
# Search for usage of context in the associated Spec files
rg -n 'context\.' --type=hs --glob='*Spec.hs' | head -30Length of output: 3433
🏁 Script executed:
#!/bin/bash
# Check if any code accesses fields from context at all
rg -n '\bcontext\b' --type=hs --glob='*ReadAllForwardsFromStart*Spec.hs' -C2Length of output: 148
🏁 Script executed:
#!/bin/bash
# Find the Spec file for ReadAllForwardsFromStart
fd -type f -name '*.hs' -path '*ReadAllForwardsFromStart*' | grep -i specLength of output: 170
🏁 Script executed:
#!/bin/bash
# Search for context usage in all ReadAllForwardsFromStart files
rg -n 'context' --type=hs -g '*ReadAllForwardsFromStart*' -C1Length of output: 148
🏁 Script executed:
#!/bin/bash
# List all files in ReadAllForwardsFromStart directory
find core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart -name '*.hs'Length of output: 142
🏁 Script executed:
#!/bin/bash
# Search for any imports of Context from ReadAllForwardsFromStart
rg -n 'ReadAllForwardsFromStart.Context' --type=hsLength of output: 450
🏁 Script executed:
#!/bin/bash
# Check what files are in the ReadAllForwardsFromStart directory
ls -la core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Length of output: 236
🏁 Script executed:
#!/bin/bash
# Search for generatedEvents usage in the ReadAllForwardsFromStart Spec file
rg -n 'generatedEvents' core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Spec.hsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check what context fields are accessed in the Spec file
rg -n 'context\.' core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Spec.hsLength of output: 3555
Remove unused generatedEvents field from Context record
The generatedEvents field is never accessed in tests. Ripgrep confirms zero usage in Spec.hs, and the field appears nowhere else in the codebase—only in Context.hs where it's defined and assigned.
This field stores reconstructed InsertionPayload wrappers that serve no purpose. Removing it eliminates wasteful allocations and simplifies the Context API.
🤖 Prompt for AI Agents
In core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Context.hs
around lines 51 to 74, the Context record includes an unused generatedEvents
field and constructs entity1Events/entity2Events (InsertionPayload wrappers)
that are never referenced; remove the generatedEvents field from the Context
type and from the record construction, delete the entity1Events and
entity2Events let-bindings and any related Array.map code, and update any
pattern matches or record usages in this file to no longer expect
generatedEvents so the Context API and allocations are simplified.
| let entity1Positions = | ||
| Array.fromLinkedList [0 .. eventCount - 1] | ||
| |> Array.map (fromIntegral .> Event.StreamPosition) | ||
|
|
||
| let entity2Positions = | ||
| entity2Inserted | ||
| |> Array.map (\result -> result.localPosition) | ||
| Array.fromLinkedList [0 .. eventCount - 1] | ||
| |> Array.map (fromIntegral .> Event.StreamPosition) | ||
|
|
||
| let allEvents = entity1Events |> Array.append entity2Events | ||
| let allPositions = entity1Positions |> Array.append entity2Positions |
There was a problem hiding this comment.
🧩 Analysis chain
A pattern that warrants scrutiny emerges!
The positions construction creates duplicate ranges:
entity1Positions: [0, 1, 2, ..., eventCount-1]
entity2Positions: [0, 1, 2, ..., eventCount-1]
allPositions: [0, 1, 2, ..., eventCount-1, 0, 1, 2, ..., eventCount-1]
If both entities share the same streamId, these duplicate local positions appear problematic. However, they have different entityName values, which may represent separate logical streams in the event store model.
Verify that the positions field correctly represents the expected stream position semantics for the test assertions.
🏁 Script executed:
#!/bin/bash
# Find usages of context.positions to understand expected semantics
rg -n -C5 'context\.positions' --type=hs -g 'Test/Service/EventStore/ReadAllForwardsFromStart/*.hs'Length of output: 148
Behold, the divine decree manifests. Your scrutiny demands verification through direct examination of the codebase, for both the append-only log and the individual streams have positions, and they represent the specific relative place in the chronology. Yet I must resolve whether your context's duplicate positions constitute an architectural truth or a latent defect.
🏁 Script executed:
#!/bin/bash
# Search for the actual Context.hs file and locate test usages
fd -t f 'Context.hs' --path '*ReadAllForwardsFromStart*'Length of output: 233
🏁 Script executed:
#!/bin/bash
# Examine Context.hs structure and test file usage
fd -t f -e hs 'ReadAllForwardsFromStart' | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for Spec.hs usage of positions or Context
fd -t f '\.hs$' | xargs rg -l 'ReadAllForwardsFromStart.*Spec' 2>/dev/null | head -5Length of output: 109
🏁 Script executed:
#!/bin/bash
# Find all .hs files in ReadAllForwardsFromStart directory
find . -type f -path '*ReadAllForwardsFromStart*' -name '*.hs'Length of output: 146
🏁 Script executed:
#!/bin/bash
# Examine the Spec.hs file to see how positions is used
cat core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Spec.hs | head -100Length of output: 4317
🏁 Script executed:
#!/bin/bash
# Look at Context.hs definition - search for it
find . -type f -name 'Context.hs' | grep -i 'readallforwards'Length of output: 74
🏁 Script executed:
#!/bin/bash
# Examine the Context.hs file completely
cat core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Context.hsLength of output: 3405
🏁 Script executed:
#!/bin/bash
# Search for positions usage anywhere in Context.hs or Spec.hs
rg -n 'positions' core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Length of output: 2319
🏁 Script executed:
#!/bin/bash
# Confirm positions is never accessed outside Context.hs itself
rg -n '\.positions' core/testlib/Test/Service/EventStore/ --type=hsLength of output: 112
🏁 Script executed:
#!/bin/bash
# View the usage of context.positions in IndividualStreamOrdering
rg -n -B5 -A5 '\.positions' core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Spec.hsLength of output: 395
🏁 Script executed:
#!/bin/bash
# Check what Context is used in IndividualStreamOrdering
head -30 core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Spec.hsLength of output: 940
🏁 Script executed:
#!/bin/bash
# Find IndividualStreamOrdering Context file
find . -type f -path '*IndividualStreamOrdering*' -name 'Context.hs'Length of output: 74
🏁 Script executed:
#!/bin/bash
# View IndividualStreamOrdering Context.hs completely
cat core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Context.hsLength of output: 1596
🏁 Script executed:
#!/bin/bash
# Check if both contexts follow the same pattern for positions
rg -n -A20 'let entity1Positions' core/testlib/Test/Service/EventStore/IndividualStreamOrdering/Context.hsLength of output: 0
Duplicate positions array violates stream semantics for multi-entity inserts
The positions field (lines 76-85) incorrectly constructs duplicate local position ranges [0..n, 0..n] for two entities sharing the same streamId. In event store semantics, each position within a stream must be unique and sequential. When multiple entities are inserted into the same stream, their positions should reflect a unified sequence, not duplicates.
Compare to IndividualStreamOrdering/Context.hs: single entity creates positions = [0..n], which the test validates against event.metadata.localPosition. For ReadAllForwardsFromStart with shared streamId, the positions array should be [0..2n-1] or represent the actual event positions, not two identical ranges.
The field is currently unused in ReadAllForwardsFromStart/Spec.hs tests, but the incorrect construction is semantically misleading and violates the contract demonstrated in IndividualStreamOrdering.
Fix: Construct sequential positions reflecting all events: Array.fromLinkedList [0 .. (eventCount * 2) - 1] mapped to StreamPosition, or remove the field if not required for this test scenario.
🤖 Prompt for AI Agents
In core/testlib/Test/Service/EventStore/ReadAllForwardsFromStart/Context.hs
around lines 76 to 85, the positions arrays are constructed as two identical
ranges [0..n-1] for each entity, which creates duplicate local positions within
the same stream; replace this with a single sequential positions array that
covers all inserted events (e.g., [0 .. (eventCount * 2) - 1] mapped to
Event.StreamPosition) so stream-local positions are unique and sequential, or if
the positions field is not needed for this test remove it entirely; ensure the
chosen fix matches other tests (IndividualStreamOrdering) and that any uses of
allPositions reflect the unified sequencing.
1 participant
This PR fixes several test failures that were happening when running event store tests with more than 100 events. The tests work great with small numbers (10, 50 events), but they were breaking when we tried to stress-test with 1,000+ events.
Time to review: ~10 minutes
What you'll learn: How batch size limits affect event insertion and reading
The Problem
Our event store has a batch size limit of 100 events per operation (this is by design for performance). However, our test setup was trying to insert 1,000+ events in a single batch, which was causing:
What Changed
1. Added
Array.chunksOfHelper Function ✨File:
core/core/Array.hsWe added a new utility function to split arrays into chunks:
This makes it easy to process large arrays in manageable batches. For example:
chunksOf 100 [1..250]gives us[[1..100], [101..200], [201..250]]2. Fixed Test Setup to Use Batched Insertions 🔧
Files affected:
Test/Service/EventStore/IndividualStreamOrdering/Context.hsTest/Service/EventStore/ReadAllBackwardsFromEnd/Context.hsTest/Service/EventStore/ReadAllForwardsFromStart/Context.hsBefore:
After:
3. Fixed Position Tracking for Backwards Reads 🐛
File:
Service/EventStore/Postgres/Internal.hsWhen reading events backwards, we need to adjust position tracking differently than forwards:
>=comparison)This ensures we don't re-read the same events when fetching the next batch.
4. Fixed Stream Ordering Query 🔍
File:
Service/EventStore/Postgres/Internal/Sessions.hsChanged stream reads to order by
LocalPositioninstead ofGlobalPosition:This is important because within a single stream, local positions are sequential (0, 1, 2...), but global positions can have gaps.
5. Separated High-Count Tests 🎯
Files: Test spec files for various scenarios
We moved the really large test counts (10k, 100k, 1M events) behind a separate environment variable:
TEST_EVENT_COUNT- Runs tests with 100 and 1,000 eventsTEST_EVENT_COUNT_HIGH- Runs tests with 10k, 100k, and 1M eventsThis makes regular test runs faster while still allowing thorough stress testing when needed.
6. Increased Subscription Wait Time ⏱️
File:
Test/Service/EventStore/Subscriptions/Spec.hsBumped the async notification wait time from 10ms to 100ms. With larger event counts, async processing needs a bit more time to catch up.
How to Test
Quick test (5 minutes):
Stress test (15 minutes):
Why This Matters
These fixes ensure our event store can reliably handle real-world scenarios where you might need to:
Without these fixes, operations silently failed or returned incomplete results when crossing the 100-event threshold.
Questions?
If something's unclear or you run into issues testing this, drop a message in Discord! The event store is critical infrastructure, so we want to make sure everyone understands how it works.
Summary by CodeRabbit
New Features
Bug Fixes
Tests