Refactor rollback handling to track both target and diff checkpoints - #1256
Merged
Conversation
…y flag Replace the sticky `containsRollbackDiffChange` boolean threaded through every `InMemoryTable.Entity.set` call with a single `rollback` record on `InMemoryStore`, holding both the rollback target and diff checkpoint IDs. Whether an in-memory update originated from a rollback diff is derived at batch-assembly time by comparing change checkpoint IDs against the recorded `diffCheckpointId`. `InMemoryTable.Entity.set` no longer takes `~shouldSaveHistory` or `~containsRollbackDiffChange`. `history` is now defined as changes strictly older than `latestChange`: a new change with the same checkpoint overwrites `latestChange`; a newer checkpoint demotes the previous `latestChange` into `history`. Filtering by `shouldSaveHistory` and the rollback-diff checkpoint happens once, in `InMemoryStore.writeBatch`, when constructing the storage-bound shape. The rollback-diff change is written to the entity table when it is still `latestChange`, but never to the entity history table.
…ate type
`InMemoryStore.writeBatch` now passes raw `inMemoryStoreEntityUpdate`
records (`{latestChange, history}`) straight through to storage instead
of pre-computing a filtered shape with a `containsRollbackDiffChange`
flag. `Persistence.entityUpdate` is removed; `updatedEntity.updates` is
typed directly as `array<Internal.inMemoryStoreEntityUpdate>`.
PgStorage derives the diff classification locally from `~rollback` and
the change checkpoint IDs: skip backfill when the entity was touched by
the rollback diff (latestChange at diff, or history[0] at diff after
demotion), filter diff entries out of history-table writes, and skip the
latestChange history write when it equals the diff checkpoint. Behavior
is unchanged from the previous commit.
The `InMemoryTable.Entity.updates` helper is inlined at its single call
site in `InMemoryStore.writeBatch`. ClickHouse keeps its single-storage
signature without `~rollback` — it does not need rollback awareness;
follow-up work can let it iterate full history.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR removes ChangesRollback Representation & Entity History Refactor
🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/envio/src/InMemoryStore.res (1)
99-105: ⚡ Quick winKeep entity-update collection behind
InMemoryTable.Entity.
writeBatchnow reaches into.entitiesandrow.statusdirectly, which leaksEntity.tinternals intoInMemoryStore. Restoring a tinyupdatesaccessor/iterator would keep this refactor localized and avoid the nextEntitylayout change rippling into callers.♻️ Possible shape
// packages/envio/src/InMemoryTable.res + let updates = (inMemTable: t) => { + let acc = [] + inMemTable.entities->Utils.Dict.forEach(row => + switch row.status { + | Updated(update) => acc->Array.push(update) + | Loaded => () + } + ) + acc + } // packages/envio/src/InMemoryStore.res - let updates = [] - (inMemoryStore->getInMemTable(~entityConfig)).entities->Utils.Dict.forEach(row => - switch row.status { - | Updated(update) => updates->Array.push(update) - | Loaded => () - } - ) + let updates = inMemoryStore->getInMemTable(~entityConfig)->InMemoryTable.Entity.updates🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/InMemoryStore.res` around lines 99 - 105, The writeBatch code is accessing Entity internals (.entities and row.status) directly; restore an accessor on InMemoryTable.Entity that yields Updated entries so callers don't depend on Entity.t layout. Add or re-enable a method like InMemoryTable.Entity.getUpdates or an iterator on InMemoryTable.Entity that returns only the update payloads, then change writeBatch to call inMemoryStore->getInMemTable(~entityConfig)->Entity.getUpdates (or equivalent) instead of iterating .entities and matching row.status; update references in writeBatch to use that accessor and keep Entity.t internals encapsulated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/envio/src/InMemoryStore.res`:
- Around line 99-105: The writeBatch code is accessing Entity internals
(.entities and row.status) directly; restore an accessor on InMemoryTable.Entity
that yields Updated entries so callers don't depend on Entity.t layout. Add or
re-enable a method like InMemoryTable.Entity.getUpdates or an iterator on
InMemoryTable.Entity that returns only the update payloads, then change
writeBatch to call
inMemoryStore->getInMemTable(~entityConfig)->Entity.getUpdates (or equivalent)
instead of iterating .entities and matching row.status; update references in
writeBatch to use that accessor and keep Entity.t internals encapsulated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10020c0c-2f28-479f-befc-0c4630847f12
📒 Files selected for processing (11)
packages/envio/src/EventProcessing.respackages/envio/src/GlobalState.respackages/envio/src/InMemoryStore.respackages/envio/src/InMemoryTable.respackages/envio/src/Internal.respackages/envio/src/Persistence.respackages/envio/src/PgStorage.respackages/envio/src/TestIndexerProxyStorage.respackages/envio/src/UserContext.resscenarios/test_codegen/test/EventOrigin_test.resscenarios/test_codegen/test/helpers/MockIndexer.res
💤 Files with no reviewable changes (3)
- packages/envio/src/UserContext.res
- packages/envio/src/Internal.res
- scenarios/test_codegen/test/EventOrigin_test.res
This was referenced Jun 1, 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
2 participants
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.
Summary
Refactors the rollback mechanism to pass both the target checkpoint ID and the diff checkpoint ID through the persistence layer, enabling more precise filtering of rollback-diff changes during batch writes.
Key Changes
Rollback type consolidation: Replaced
rollbackTargetCheckpointId: option<checkpointId>withrollback: option<Persistence.rollback>throughout the codebase, whererollbackcontains bothtargetCheckpointIdanddiffCheckpointIdEntity update simplification: Removed
containsRollbackDiffChangefield fromInternal.inMemoryStoreEntityUpdatetype. This boolean flag is now computed on-the-fly by comparing checkpoint IDs against the diff checkpoint IDHistory tracking refactor: Simplified
InMemoryTable.Entity.setfunction by:shouldSaveHistoryandcontainsRollbackDiffChangeparameterslatestChangevaluesupdateshelper function (inlined at call sites)Batch write logic improvement: Enhanced
PgStorage.writeBatchto:diffCheckpointIdfrom the rollback optionEvent processing cleanup: Removed
shouldSaveHistoryparameter from event handler execution chain (EventProcessing.runEventHandlerOrThrow,runHandlerOrThrow,runBatchHandlersOrThrow)Storage interface update: Updated
Persistence.storage.writeBatchsignature to acceptrollback: option<Persistence.rollback>instead ofrollbackTargetCheckpointIdImplementation Details
The refactoring shifts from storing metadata about rollback-diff changes to computing it dynamically. When processing updates during batch writes, the code now:
latestChangecheckpoint matches the diff checkpointThis approach reduces state complexity while maintaining the same filtering behavior for rollback-diff changes.
https://claude.ai/code/session_015wCRQBjfLhGoJGNHiaGMQM
Summary by CodeRabbit
Refactor
Tests