Optimize rollback history prune - #700
Conversation
WalkthroughReplaces Internal.prettifyExn with Utils.prettifyExn across multiple logging and error paths, adds Utils.prettifyExn and removes Internal.prettifyExn. Introduces EntityHistory pruneStaleEntityHistory with safeReorgBlocks and SQL generator, updates ChainManager/GlobalState to the new API and flow, removes old pruning extern/JS implementation, adjusts Env defaults and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor GS as GlobalState
participant CM as ChainManager
participant EH as EntityHistory
participant PG as Postgres
participant EHdl as ErrorHandling
rect #f5f8ff
note over GS: Prune cycle
GS->>CM: getSafeReorgBlocks()
CM-->>GS: { chainIds[], blockNumbers[] }
end
alt chainIds not empty
loop Per-entity (delay ~1s)
GS->>EH: pruneStaleEntityHistory(sql, ~entityName, ~pgSchema, ~safeReorgBlocks)
EH->>PG: preparedUnsafe(makePruneStaleEntityHistoryQuery(...), params)
PG-->>EH: ok
EH-->>GS: unit
end
else no safe blocks
GS-->>GS: skip pruning
end
opt error
GS->>EHdl: mkLogAndRaise(logger.child(...), msg, exn)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codegenerator/cli/npm/envio/src/LoadManager.res (1)
69-81: Double settle risk: calls are rejected in catch, then resolved unconditionally.On load failure, you reject all currentInputKeys but subsequently still resolve them (and dereference getUnsafeInMemory), which can cause unnecessary lookups and potential throws. Cleanly separate success and failure paths, and always remove the calls from the group dictionary so they can be retried.
- if inputsToLoad->Utils.Array.isEmpty->not { - try { - await group.load(inputsToLoad) - } catch { - | exn => { - let exn = exn->Utils.prettifyExn - currentInputKeys->Array.forEach(inputKey => { - let call = calls->Js.Dict.unsafeGet(inputKey) - call.reject(exn) - }) - } - } - } + let hadLoadError = ref(false) + if inputsToLoad->Utils.Array.isEmpty->not { + try { + await group.load(inputsToLoad) + } catch { + | exn => { + let exn = exn->Utils.prettifyExn + hadLoadError := true + currentInputKeys->Array.forEach(inputKey => { + let call = calls->Js.Dict.unsafeGet(inputKey) + call.reject(exn) + }) + } + } + } - if currentInputKeys->Utils.Array.isEmpty->not { - currentInputKeys->Js.Array2.forEach(inputKey => { - let call = calls->Js.Dict.unsafeGet(inputKey) - calls->Utils.Dict.deleteInPlace(inputKey) - call.resolve(group.getUnsafeInMemory(inputKey)) - }) + if currentInputKeys->Utils.Array.isEmpty->not { + currentInputKeys->Js.Array2.forEach(inputKey => { + let call = calls->Js.Dict.unsafeGet(inputKey) + calls->Utils.Dict.deleteInPlace(inputKey) + if !hadLoadError.contents { + call.resolve(group.getUnsafeInMemory(inputKey)) + } + // else: already rejected above + })Also applies to: 83-97
🧹 Nitpick comments (13)
codegenerator/cli/npm/envio/src/PgStorage.res (1)
766-767: Include more context in the dump-cache error message (optional).The message “Failed to dump cache.” is terse. Consider adding the target cache directory and pgSchema to ease debugging runs where
psqlor permissions fail.Apply this minimal tweak:
- | exn => Logging.errorWithExn(exn->Utils.prettifyExn, `Failed to dump cache.`) + | exn => + Logging.errorWithExn( + exn->Utils.prettifyExn, + `Failed to dump cache for schema ${pgSchema} to ${cacheDirPath->NodeJs.Path.toString}.`, + )codegenerator/cli/npm/envio/src/Utils.res (1)
588-593: Add a short doc comment and (optionally) tests for edge cases.A brief description will help future contributors. Consider unit tests for:
- A plain ReScript exception (e.g.,
Not_found) remains unchanged.- A
Js.Exn.Error(new Error("x"))unwraps to the inner error.- An AggregateError remains usable (if surfaced as Js Error).
Proposed doc:
- let prettifyExn = exn => { + /** Unwrap Js.Exn.Error to expose the inner JS Error as a standard `exn`. + Returns the original `exn` when it's not a JS Error wrapper. */ + let prettifyExn = exn => { switch exn->Js.Exn.anyToExnInternal { | Js.Exn.Error(e) => e->(magic: Js.Exn.t => exn) | exn => exn } }I can also add a small spec exercising the three cases above—just say the word.
codegenerator/cli/templates/static/codegen/src/ink/components/CustomHooks.res (1)
104-105: Prefer errorWithExn to capture structured error + stack (optional).Using
Logging.errorWithExnwill emit the error in the structured “err” field with stack, which is more consistent with the rest of the migration.Apply this change:
- Logging.error({"msg": "Failed to load messages from envio server", "err": e->Utils.prettifyExn}) + Logging.errorWithExn(e->Utils.prettifyExn, "Failed to load messages from envio server")codegenerator/cli/templates/static/codegen/src/IO.res (2)
111-128: Return 0 on equal events in comparator to avoid unstable reorders.sortInPlaceWith currently returns -1 or 1 only. For equal events, returning 0 avoids unnecessary churn and preserves stability where possible.
- let _ = entityHistoryItemsToSet->Js.Array2.sortInPlaceWith((a, b) => { - EventUtils.isEarlierEvent( + let _ = entityHistoryItemsToSet->Js.Array2.sortInPlaceWith((a, b) => { + let aKey = { { timestamp: a.current.block_timestamp, chainId: a.current.chain_id, blockNumber: a.current.block_number, logIndex: a.current.log_index, - }, - { + } + } + let bKey = { timestamp: b.current.block_timestamp, chainId: b.current.chain_id, blockNumber: b.current.block_number, logIndex: b.current.log_index, - }, - ) - ? -1 - : 1 + } + } + if ( + aKey.timestamp === bKey.timestamp && + aKey.chainId === bKey.chainId && + aKey.blockNumber === bKey.blockNumber && + aKey.logIndex === bKey.logIndex + ) { + 0 + } else if EventUtils.isEarlierEvent(aKey, bKey) { + -1 + } else { + 1 + } })
200-205: Typo fixes in comments.Minor typos in comments; keeping comments sharp helps future maintenance.
- // Improtant: Don't rethrow here, since it'll result in + // Important: Don't rethrow here, since it'll result in ... - //Rollback tables need to happen first in the traction + //Rollback tables need to happen first in the transactionAlso applies to: 232-236
codegenerator/cli/npm/envio/src/sources/SourceManager.res (1)
3-3: Typo in variant name: Querieng.Consider renaming to Querying for clarity. This is cosmetic but improves readability; verify ripple effects before changing.
Would you like a follow-up PR to rename the variant and update references across modules?
codegenerator/cli/npm/envio/src/db/EntityHistory.res (2)
254-305: PL/pgSQL function formatting improved; fix “ElSIF” keyword casing for clarityThe reflow of DECLARE/BEGIN/END improves readability. Minor nit: “ElSIF” should be “ELSIF” (Postgres keywords are case-insensitive, so behavior is unaffected, but readability and greppability suffer). If you update it, also adjust the corresponding test expectation.
Apply this diff inside the string literal:
- ElSIF should_copy_current_entity THEN + ELSIF should_copy_current_entity THEN
326-357: Deep-prune SQL is correct and minimal; consider two small robustness tweaksThe query correctly:
- Computes keep_serial per id at or before safe blocks, per chain.
- Keeps keep_serial only when there are post-safe rows; otherwise deletes it as well.
- Deletes all rows older than keep_serial.
Two optional improvements:
- Add a comment near the generator describing the semantics (“keep latest safe row only if superseded by post-safe rows; otherwise delete everything for that id”).
- If entity names/schemas can be user-influenced, consider identifier sanitization (whitelisting or quoting helper) to harden against malformed identifiers. Today they come from codegen/env, so risk is low.
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res (1)
314-327: Add defensive assert to verify pairing in getSafeReorgBlocksScan confirms only the definition and a single usage remain, so no legacy calls are left.
- Locations to update:
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res(lines 314–327): insert the assertion after theforEachloop and before the returned object.codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res(line 861): only remaining call site ofgetSafeReorgBlocks.->Array.forEach((cf) => { chainIds->Js.Array2.push(cf.chainConfig.chain->ChainMap.Chain.toChainId)->ignore blockNumbers->Js.Array2.push(cf->ChainFetcher.getHighestBlockBelowThreshold)->ignore }) + Belt.Array.length(chainIds) === Belt.Array.length(blockNumbers) || { + Js.Exn.raiseError("ChainManager.getSafeReorgBlocks: chainIds and blockNumbers length mismatch") + }->ignore { chainIds, blockNumbers, }scenarios/test_codegen/test/lib_tests/EntityHistory_test.res (4)
189-223: String expectation mirrors PL/pgSQL — consider normalizing ELSIF casingThe expected function body includes “ElSIF”. If you adopt the nit in EntityHistory.res to switch to “ELSIF”, update the expectation here accordingly.
- ElSIF should_copy_current_entity THEN + ELSIF should_copy_current_entity THEN
798-805: API migration to EntityHistory.pruneStaleEntityHistory — LGTM; add edge-case testsGood use of Env.Db.publicSchema and the new safeReorgBlocks shape. Consider adding tests for:
- Empty arrays (should be a no-op).
- Mismatched lengths (should throw); aligns with my guard suggestion in EntityHistory.res.
If you want, I can add these two tests in a follow-up commit.
1070-1099: Duplicate prune test with fragile ordering — de-duplicate or stabilize comparatorThis repeats the previous test with a slightly different assertion shape and sorts only the actual array. It’s fragile when equal block_numbers exist across chains (tie order depends on DB return order and sort stability). Either remove this test (duplicative) or sort both arrays with a stable comparator that includes chain_id/log_index as tiebreakers.
Apply one of these diffs:
Option A — remove the duplicate test:
- Async.it("Prunes history correctly with items in reorg threshold", async () => { - let () = await Db.sql->EntityHistory.pruneStaleEntityHistory( - ~entityName=(module(TestEntity)->Entities.entityModToInternal).name, - ~pgSchema=Env.Db.publicSchema, - ~safeReorgBlocks={ - chainIds: [1, 2], - blockNumbers: [3, 2], - }, - ) - let currentHistoryItems = await Db.sql->getAllMockEntityHistory - - let parsedHistoryItems = - currentHistoryItems->S.parseJsonOrThrow(TestEntity.entityHistory.schemaRows) - - let sort = arr => - arr->Js.Array2.sortInPlaceWith( - (a, b) => a.EntityHistory.current.block_number - b.current.block_number, - ) - - Assert.deepEqual( - parsedHistoryItems->sort->stripUndefinedFieldsInPlace, - [ - Mocks.Chain1.historyRow2, - Mocks.Chain2.historyRow2, - Mocks.Chain2.historyRow3, - Mocks.Chain1.historyRow3, - ]->stripUndefinedFieldsInPlace, - ~message="Should have deleted the unneeded first items in history", - ) - })Option B — stabilize comparator and sort both sides:
- let sort = arr => - arr->Js.Array2.sortInPlaceWith( - (a, b) => a.EntityHistory.current.block_number - b.current.block_number, - ) + let cmp = (a, b) => { + let bn = a.EntityHistory.current.block_number - b.current.block_number + if bn != 0 { bn } + else { + let cid = a.EntityHistory.current.chain_id - b.current.chain_id + if cid != 0 { cid } + else a.EntityHistory.current.log_index - b.current.log_index + } + } + let sort = arr => arr->Js.Array2.sortInPlaceWith(cmp) @@ - parsedHistoryItems->sort->stripUndefinedFieldsInPlace, - [ + parsedHistoryItems->sort->stripUndefinedFieldsInPlace, + ([ Mocks.Chain1.historyRow2, Mocks.Chain2.historyRow2, Mocks.Chain2.historyRow3, Mocks.Chain1.historyRow3, - ]->stripUndefinedFieldsInPlace, + ]->sort)->stripUndefinedFieldsInPlace,
1172-1181: Prune performance harness — LGTMUseful smoke/benchmark; aligns with deep-prune approach. Consider logging row counts before/after to compute rows/s for future regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
codegenerator/cli/npm/envio/src/ErrorHandling.res(1 hunks)codegenerator/cli/npm/envio/src/Hasura.res(4 hunks)codegenerator/cli/npm/envio/src/Internal.res(0 hunks)codegenerator/cli/npm/envio/src/LoadManager.res(1 hunks)codegenerator/cli/npm/envio/src/Logging.res(1 hunks)codegenerator/cli/npm/envio/src/PgStorage.res(3 hunks)codegenerator/cli/npm/envio/src/Time.res(1 hunks)codegenerator/cli/npm/envio/src/Utils.res(1 hunks)codegenerator/cli/npm/envio/src/db/EntityHistory.res(2 hunks)codegenerator/cli/npm/envio/src/sources/SourceManager.res(3 hunks)codegenerator/cli/templates/static/codegen/src/Config.res(2 hunks)codegenerator/cli/templates/static/codegen/src/Env.res(1 hunks)codegenerator/cli/templates/static/codegen/src/IO.res(1 hunks)codegenerator/cli/templates/static/codegen/src/db/DbFunctions.res(0 hunks)codegenerator/cli/templates/static/codegen/src/db/DbFunctionsImplementation.js(0 hunks)codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res(1 hunks)codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res(3 hunks)codegenerator/cli/templates/static/codegen/src/ink/components/CustomHooks.res(1 hunks)scenarios/test_codegen/test/lib_tests/EntityHistory_test.res(6 hunks)
💤 Files with no reviewable changes (3)
- codegenerator/cli/templates/static/codegen/src/db/DbFunctionsImplementation.js
- codegenerator/cli/npm/envio/src/Internal.res
- codegenerator/cli/templates/static/codegen/src/db/DbFunctions.res
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{res,resi}
📄 CodeRabbit inference engine (.cursor/rules/rescript.mdc)
**/*.{res,resi}: Never use[| item |]to create an array. Use[ item ]instead.
Must always use=for setting value to a field. Use:=only for ref values created usingreffunction.
ReScript has record types which require a type definition before hand. You can access record fields by dot likefoo.myField.
It's also possible to define an inline object, it'll have quoted fields in this case.
Use records when working with structured data, and objects to conveniently pass payload data between functions.
Never use %raw to access object fields if you know the type.
Files:
codegenerator/cli/templates/static/codegen/src/ink/components/CustomHooks.rescodegenerator/cli/npm/envio/src/Hasura.rescodegenerator/cli/templates/static/codegen/src/Config.rescodegenerator/cli/npm/envio/src/Logging.rescodegenerator/cli/templates/static/codegen/src/IO.rescodegenerator/cli/npm/envio/src/sources/SourceManager.rescodegenerator/cli/npm/envio/src/ErrorHandling.rescodegenerator/cli/npm/envio/src/Utils.rescodegenerator/cli/npm/envio/src/Time.rescodegenerator/cli/npm/envio/src/PgStorage.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.rescodegenerator/cli/npm/envio/src/LoadManager.rescodegenerator/cli/templates/static/codegen/src/Env.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.rescodegenerator/cli/npm/envio/src/db/EntityHistory.resscenarios/test_codegen/test/lib_tests/EntityHistory_test.res
codegenerator/cli/templates/{dynamic/**/*.hbs,static/**}
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Templates live under codegenerator/cli/templates: dynamic/ for Handlebars (.hbs), static/ for raw Rescript files copied verbatim.
Files:
codegenerator/cli/templates/static/codegen/src/ink/components/CustomHooks.rescodegenerator/cli/templates/static/codegen/src/Config.rescodegenerator/cli/templates/static/codegen/src/IO.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.rescodegenerator/cli/templates/static/codegen/src/Env.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res
{**/generated/src/**/*.res,codegenerator/cli/templates/static/codegen/src/**/*.res,codegenerator/cli/templates/dynamic/codegen/src/**/*.res}
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Runtime code lives in each project’s generated/src, but template versions (good for editing) are under codegenerator/cli/templates/static/codegen/src or codegenerator/cli/templates/dynamic/codegen/src.
Files:
codegenerator/cli/templates/static/codegen/src/ink/components/CustomHooks.rescodegenerator/cli/templates/static/codegen/src/Config.rescodegenerator/cli/templates/static/codegen/src/IO.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.rescodegenerator/cli/templates/static/codegen/src/Env.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res
codegenerator/cli/npm/envio/**
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Library-fied runtime shared across indexers lives in codegenerator/cli/npm/envio.
Files:
codegenerator/cli/npm/envio/src/Hasura.rescodegenerator/cli/npm/envio/src/Logging.rescodegenerator/cli/npm/envio/src/sources/SourceManager.rescodegenerator/cli/npm/envio/src/ErrorHandling.rescodegenerator/cli/npm/envio/src/Utils.rescodegenerator/cli/npm/envio/src/Time.rescodegenerator/cli/npm/envio/src/PgStorage.rescodegenerator/cli/npm/envio/src/LoadManager.rescodegenerator/cli/npm/envio/src/db/EntityHistory.res
🧠 Learnings (1)
📚 Learning: 2025-05-27T17:07:12.878Z
Learnt from: JonoPrest
PR: enviodev/hyperindex#555
File: scenarios/test_codegen/test/lib_tests/EntityHistory_test.res:631-659
Timestamp: 2025-05-27T17:07:12.878Z
Learning: In isolated tests for EntityHistory rollbacks with controlled mock data setup, the ordering of getRollbackDiff results is deterministic and doesn't require sorting before comparison with deepStrictEqual.
Applied to files:
scenarios/test_codegen/test/lib_tests/EntityHistory_test.res
🔇 Additional comments (23)
codegenerator/cli/templates/static/codegen/src/Config.res (1)
116-118: Swap to Utils.prettifyExn is correct and consistent.Using Utils.prettifyExn keeps logs uniform and ensures Js.Exn.Error wrappers are unwrapped before passing to Logging.errorWithExn.
Also applies to: 140-142
codegenerator/cli/npm/envio/src/PgStorage.res (2)
390-392: Good: unwrap exceptions before wrapping in StorageError.Switching to
exn->Utils.prettifyExnpreserves the meaningful inner error for downstream handling and logging.
328-329: Keep pgErrorMessageSchema—it’s used by the codegen template
The symbolpgErrorMessageSchemais referenced incodegenerator/cli/templates/static/codegen/src/IO.res(around line 184) within the error‐parsing switch (S.parseOrThrow(PgStorage.pgErrorMessageSchema)). Removing this definition would break the generated IO module’s Postgres error handling, so it should remain as is.• Usage location:
– codegenerator/cli/templates/static/codegen/src/IO.res:184:switch error->S.parseOrThrow(PgStorage.pgErrorMessageSchema) { … }codegenerator/cli/npm/envio/src/Utils.res (2)
588-593: Unwrapping Js.Exn.Error at the boundary is the right move.The implementation correctly returns the inner JS Error as an
exn, avoiding double-wrapped errors throughout logging/handlers.
588-593: Sanity check passed – no lingeringInternal.prettifyExnreferences
Ranrg -n -C2 'Internal\.prettifyExn'across the repo; no matches were found, confirming the migration is complete.codegenerator/cli/npm/envio/src/Logging.res (1)
186-187: Consistent exception unwrapping in user logger.Switching to
Utils.prettifyExnaligns the user-facing logger with core logging behavior.codegenerator/cli/templates/static/codegen/src/IO.res (1)
194-195: No lingeringInternal.prettifyExnreferences found — ready to approveVerified across all
*.resfiles that there are no remaining calls toInternal.prettifyExn. The change cleanly replaces those withUtils.prettifyExn, meaning:
- Exceptions are now unwrapped via
Utils.prettifyExnconsistently.specificErrorremains anexn, with standardized formatting for downstream logging/raising.- There’s no behavioral change beyond normalized error messages.
No further action required.
codegenerator/cli/npm/envio/src/LoadManager.res (1)
74-75: Switch to Utils.prettifyExn — consistent with project-wide change.The unwrapping aligns with other modules and preserves error shape for logging.
codegenerator/cli/npm/envio/src/ErrorHandling.res (1)
9-11: Unified exception unwrapping — consistent and correct.Using Utils.prettifyExn in log/raise paths avoids double-wrapped JsError and keeps logs cleaner without losing stack. No functional regressions apparent.
Also applies to: 15-16, 19-22
codegenerator/cli/npm/envio/src/sources/SourceManager.res (3)
201-203: Log "err" now uses Utils.prettifyExn — good standardization.Keeps error objects uniform across logs and avoids wrapping artifacts.
411-416: Consistent error object in parsing failures.Passing the unwrapped exn improves signal in logs for triage.
468-474: Consistent error object in backoff path.Matches the rest of the logging flow and preserves stack.
codegenerator/cli/npm/envio/src/Time.res (1)
19-21: Switched to Utils.prettifyExn in retry logs — aligned with the new standard.No behavior change besides cleaner error representation.
codegenerator/cli/npm/envio/src/Hasura.res (1)
86-87: Consistent exn prettification — LGTMSwitching to Utils.prettifyExn keeps error formatting centralized and consistent with the rest of the refactor. No functional concerns.
Also applies to: 128-129, 175-176, 216-217
codegenerator/cli/npm/envio/src/db/EntityHistory.res (2)
321-324: Type for safe reorg window — LGTMThe safeReorgBlocks struct is clear and maps cleanly to the SQL unnest pairing. Nice.
359-364: Optional: ValidatesafeReorgBlocksinputs before calling SQLThe suggested guard to
- early‐return when both
chainIdsandblockNumbersare empty, and- raise an error on length mismatch
will indeed prevent the Postgres “each array must have the same cardinality” error and avoid a no‐op DB call. That said, in the current codebase:
ChainManager.getSafeReorgBlocksalways produces parallel arrays. It initializes bothchainIdsandblockNumberstogether and fills them in the same pass overself.chainFetchers, so their lengths are guaranteed to match.- GlobalState already skips empty inputs. In
GlobalState.res, the call topruneStaleEntityHistoryis gated byso you’ll never invoke the SQL with two empty arrays.if safeReorgBlocks.chainIds->Utils.Array.notEmpty { … }If you’d still like that extra safeguard (for future callers or unforeseen usages), you can apply:
let pruneStaleEntityHistory = (sql, ~entityName, ~pgSchema, ~safeReorgBlocks): promise<unit> => { - sql->Postgres.preparedUnsafe( - makePruneStaleEntityHistoryQuery(~entityName, ~pgSchema), - (safeReorgBlocks.chainIds, safeReorgBlocks.blockNumbers)->Utils.magic, - ) + let lenA = safeReorgBlocks.chainIds->Js.Array2.length + let lenB = safeReorgBlocks.blockNumbers->Js.Array2.length + if lenA == 0 && lenB == 0 { + Promise.resolve() + } else if lenA != lenB { + Js.Exn.raiseError("EntityHistory.pruneStaleEntityHistory: safeReorgBlocks arrays must have equal length") + } else { + sql->Postgres.preparedUnsafe( + makePruneStaleEntityHistoryQuery(~entityName, ~pgSchema), + (safeReorgBlocks.chainIds, safeReorgBlocks.blockNumbers)->Utils.magic, + ) + } }Please review any other call sites of
pruneStaleEntityHistory(beyondGlobalState) to ensure they also uphold this invariant before merging.codegenerator/cli/templates/static/codegen/src/Env.res (1)
221-221: Increase default prune interval to 30s — LGTMMatches the “deep prune less often” objective. Please update any operator docs or dashboards that reference the previous 10s default.
Would you like me to draft a short docs snippet reflecting the new default?
scenarios/test_codegen/test/lib_tests/EntityHistory_test.res (3)
419-454: SQL generator test — LGTMDirectly validating the generated deep-prune SQL is valuable. Assertion matches the semantics introduced in EntityHistory.makePruneStaleEntityHistoryQuery.
1038-1068: Primary prune-behavior test — LGTMCovers mixed pre/post-safe rows across chains and validates that only earliest items are pruned.
1102-1110: All-safe window prune — LGTMValidates the “delete everything” path when there are no post-safe rows. Good coverage.
codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (3)
566-567: Minor style simplification looks correctChanging to a single-line await of ChainFetcher.runContractRegistersOrThrow preserves behavior and improves readability. No functional issues spotted.
46-46: Resolved: No remaining references todeepCleanCountI’ve confirmed that:
- A global search (
rg -nP '\bdeepCleanCount\b') returns no occurrences ofdeepCleanCountin the repository.- All call sites of
WriteThrottlers.makeonly reference the three returned fields (chainMetaData,pruneStaleEndBlockData,pruneStaleEntityHistory) and have been updated accordingly.No further changes are required.
861-895: The next step is to locate theThrottler.resimplementation to understand howschedulehandles errors and whether rethrows are caught or surface as unhandled promise rejections.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
codegenerator/cli/npm/envio/src/db/EntityHistory.res (2)
347-377: Optional: consider DISTINCT ON for anchor selection and an aiding index if this ever regresses.If you see sort/agg pressure, replacing MAX(serial) with DISTINCT ON (id, chain_id) ORDER BY serial DESC can be faster with a supporting index on (id, entity_history_chain_id, serial DESC). I get the write-path sensitivity; keep this in your toolbox if deep prunes slow under load.
379-384: Add basic input validation and no-op fast path.UNNEST requires arrays of equal length; an empty prune batch can be a cheap no-op. Guard for both to avoid runtime errors and unnecessary round trips.
let pruneStaleEntityHistory = (sql, ~entityName, ~pgSchema, ~safeReorgBlocks): promise<unit> => { - sql->Postgres.preparedUnsafe( - makePruneStaleEntityHistoryQuery(~entityName, ~pgSchema), - (safeReorgBlocks.chainIds, safeReorgBlocks.blockNumbers)->Utils.magic, - ) + let chainsLen = safeReorgBlocks.chainIds->Belt.Array.length + let blocksLen = safeReorgBlocks.blockNumbers->Belt.Array.length + if chainsLen == 0 { + Js.Promise.resolve() + } else if chainsLen != blocksLen { + Js.Promise.reject(Js.Exn.raiseError("safeReorgBlocks: chainIds and blockNumbers must have equal length")) + } else { + sql->Postgres.preparedUnsafe( + makePruneStaleEntityHistoryQuery(~entityName, ~pgSchema), + (safeReorgBlocks.chainIds, safeReorgBlocks.blockNumbers)->Utils.magic, + ) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
codegenerator/cli/npm/envio/src/db/EntityHistory.res(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{res,resi}
📄 CodeRabbit inference engine (.cursor/rules/rescript.mdc)
**/*.{res,resi}: Never use[| item |]to create an array. Use[ item ]instead.
Must always use=for setting value to a field. Use:=only for ref values created usingreffunction.
ReScript has record types which require a type definition before hand. You can access record fields by dot likefoo.myField.
It's also possible to define an inline object, it'll have quoted fields in this case.
Use records when working with structured data, and objects to conveniently pass payload data between functions.
Never use %raw to access object fields if you know the type.
Files:
codegenerator/cli/npm/envio/src/db/EntityHistory.res
codegenerator/cli/npm/envio/**
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Library-fied runtime shared across indexers lives in codegenerator/cli/npm/envio.
Files:
codegenerator/cli/npm/envio/src/db/EntityHistory.res
⏰ 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). (1)
- GitHub Check: build_and_test
🔇 Additional comments (4)
codegenerator/cli/npm/envio/src/db/EntityHistory.res (4)
254-258: PL/pgSQL function prologue looks correct.Switching to RETURNS void with an explicit DECLARE/BEGIN block is the right shape for plpgsql and should remove parser ambiguity.
283-295: Confirm sentinel zeros won’t collide with valid values.You zero out current change fields in the synthetic backfill row and then set previous_* of the real row to 0. If chain_id=0 or block_number=0 (genesis) can ever be valid in your domain, this sentinel can blur ordering semantics or future predicates that rely on these fields. If 0 can be valid, consider using NULL (and adjusting predicates) or a dedicated “epoch” value outside the domain.
351-353: Parameter type alignment: confirm blockNumbers won’t overflow JS/ReScript int.You cast $2 to bigint[] (good for future-proofing), but safeReorgBlocks.blockNumbers is array. ReScript/JS int can’t represent >2^31-1 precisely. If any chain’s “safe block” can exceed that, you’ll want a BigInt-capable path. Otherwise, consider casting to int[] for perfect alignment.
348-349: Identifier interpolation: ensure entityName/pgSchema are sanitized.Identifiers are inlined into SQL with quotes. If these come purely from codegen, risk is low. If any user input can reach them, validate against /^[A-Za-z_][A-Za-z0-9_]*$/ before building SQL or generate via format('%I.%I', ...) on the server side.
| v_origin_record RECORD; | ||
| BEGIN | ||
| -- Check if previous values are not provided | ||
| IF ${previousHistoryFieldsAreNullStr} THEN |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use AND to detect “no previous values provided,” not OR.
The comment says “Check if previous values are not provided,” which implies all previous_* fields are NULL. Using OR will treat a partially filled previous_* as “not provided,” potentially overwriting caller-supplied values. Tighten the predicate to require all previous_* fields to be NULL.
Apply this minimal change in-place:
- IF ${previousHistoryFieldsAreNullStr} THEN
+ IF ${previousChangeFieldNames
+ ->Belt.Array.map(fieldName => `${historyRowArg}.${fieldName} IS NULL`)
+ ->Js.Array2.joinWith(" AND ")} THEN📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IF ${previousHistoryFieldsAreNullStr} THEN | |
| IF ${previousChangeFieldNames | |
| ->Belt.Array.map(fieldName => `${historyRowArg}.${fieldName} IS NULL`) | |
| ->Js.Array2.joinWith(" AND ")} THEN |
🤖 Prompt for AI Agents
In codegenerator/cli/npm/envio/src/db/EntityHistory.res at line ~260, the IF
condition uses OR to test previous_* fields being NULL which treats partially
provided previous values as “not provided”; change the predicate to require all
previous_* fields be NULL by replacing the OR operators with AND so the IF only
fires when every previous_* field is NULL.
| VALUES (${currentChangeFieldNames | ||
| ElSIF should_copy_current_entity THEN | ||
| -- Check if a value for the id exists in the origin table and if so, insert a history row for it. | ||
| SELECT ${dataFieldNamesCommaSeparated} FROM ${originTablePath} WHERE id = ${historyRowArg}.${id} INTO v_origin_record; |
There was a problem hiding this comment.
Fix PL/pgSQL SELECT … INTO syntax (order matters).
In plpgsql, INTO must appear immediately after the select list. The current placement at the end of the statement is invalid and will raise a syntax error.
- SELECT ${dataFieldNamesCommaSeparated} FROM ${originTablePath} WHERE id = ${historyRowArg}.${id} INTO v_origin_record;
+ SELECT ${dataFieldNamesCommaSeparated} INTO v_origin_record
+ FROM ${originTablePath}
+ WHERE ${id} = ${historyRowArg}.${id};🤖 Prompt for AI Agents
In codegenerator/cli/npm/envio/src/db/EntityHistory.res around line 279, the
PL/pgSQL SELECT places INTO at the end of the statement which is invalid; move
the INTO clause immediately after the select list so the statement reads: SELECT
${dataFieldNamesCommaSeparated} INTO v_origin_record FROM ${originTablePath}
WHERE id = ${historyRowArg}.${id}; ensuring the INTO appears directly after the
selected columns and before FROM.
| JOIN safe s | ||
| ON s.chain_id = t.entity_history_chain_id | ||
| AND t.entity_history_block_number <= s.block_number | ||
| GROUP BY t.id |
There was a problem hiding this comment.
Out of interest any reason to use group by here instead of distinct?
There was a problem hiding this comment.
In the end ChatGPT said that it's faster this way than having a order by for the whole data set.
There was a problem hiding this comment.
I didn't benchmark it though.
| // } | ||
|
|
||
| await Db.sql->DbFunctions.EntityHistory.pruneStaleEntityHistory( | ||
| let () = await Db.sql->EntityHistory.pruneStaleEntityHistory( |
There was a problem hiding this comment.
Is this a cursor change? Looks very ocaml esque 😝
There was a problem hiding this comment.
It started complaining about unresolved return or something
| [ | ||
| Mocks.Chain1.historyRow2, | ||
| Mocks.Chain2.historyRow2, | ||
| Mocks.Chain2.historyRow3, | ||
| Mocks.Chain1.historyRow3, | ||
| ]->stripUndefinedFieldsInPlace, |
There was a problem hiding this comment.
Ok so basically it deep cleans here 👍🏼, doesn't include historyRow1 anymore
JonoPrest
left a comment
JonoPrest
left a comment
There was a problem hiding this comment.
Cool, it looks good to me. Thanks for spending the time on this one 🙏🏼
2 participants
Now, deep query every time. Should be much faster than before even without an index. I decided not to add an index in the end to avoid slowing down writes. Reduced the rate we prune history, should be fine, since it's a deep prune anyways.