Repository navigation
feat(api): harden HAF indexer replay safety - #31
KareemN1936 wants to merge 1 commit into
Conversation
|
Vercel preview is ready: https://hivelore-cegmk9g0h-kareemn1936s-projects.vercel.app Commit: 9e41579 |
mcfarhat
left a comment
There was a problem hiding this comment.
Requesting changes on one defect. I want to lead with what's right, because most of this is.
The hard part — idempotent replay — you got correct. I verified rather than assumed it: the upsert is keyed on (transactionId, operationIndex), and the replay boundary is genuinely inclusive — replayFromBlock seeds the watermark with lastProcessedOperationIndex: -1, so shouldProcessRow lets operation index 0 through and the rewound block really is re-projected rather than skipped. Your crash-replay test is testing the thing it claims to test.
The rewind is also complete, which I checked separately because it's the obvious way this design goes wrong. projectHiveOperation writes only HiveEvent; I grepped for anything else materialising from it and there's nothing outside runner.ts. So deleting HiveEvent from the fork point really does undo the projection — there's no derived table left holding stale counts.
And you fixed a latent bug nobody asked you to. On develop, normalizeHafOperation resolved the operation index via operation_id ?? operationIndex ?? op_in_trx ?? op_pos, but shouldProcessRow used operation_id ?? operationIndex ?? op_pos. Any row carrying op_in_trx but not op_pos would have had the watermark and the stored index disagree. You aligned them in this PR and didn't mention it. Worth mentioning next time — that's a real find.
The migration is additive and safe, and the README is honest about rebuild bounds instead of implying an SLO you can't hold.
Blocking
findForkBlock treats stream-adjacency as chain-adjacency
haf-sync.ts:342 compares a row's previous_block_hash against the checkpoint hash without first checking that the row is the block after the checkpoint:
if (previousBlockHash && previousBlockHash !== watermark.lastProcessedBlockHash) {
return blockNumber - 1;
}The operation stream is filtered to HiveLore operations, so the next row is very often not the next block. When the checkpoint is at block 100 and the next relevant operation is at block 104, that row's parent hash is block 103's — which of course doesn't match block 100's. A healthy chain reads as a fork.
That alone would only cost a wasted DELETE. What makes it blocking is the multi-page case. Checkpoint at 100, page 1 carries block 101, page 2 carries block 102 — contiguous blocks, healthy chain, no reorg anywhere:
PROBE E -> projected = 2 | rolledBack = 1 | surviving = block-102-op:1
Page 1 projects block 101. Page 2 computes a fork at 101 and deletes it. The event is gone and is never re-projected, because the watermark has already advanced past it — no ordinary run will re-fetch that block. Recovery needs a manual --from-block replay, and you'd have to already know which blocks you lost.
Note the run reports projected = 2 while one row survives. It logs success.
Two things are compounding here. The first is the missing contiguity check. The second is at haf-sync.ts:138: watermark is read once per run and never advanced as rows are projected — only the database row moves, via saveWatermark. So every page after the first compares against the checkpoint the run started with, which is what puts page 2 a block behind reality.
There's a second-order effect worth knowing about: when the spurious rewind fires it sets lastProcessedBlockHash to null, and findForkBlock returns early on a null hash. So after one false positive, fork detection is off for the rest of that run — a genuine reorg arriving on a later page produces no rollback at all. Two bugs partly masking each other, which is why this isn't obvious from reading it.
Why the suite is green
row() in haf-sync.test.ts hardcodes previousBlockHash: \block-${input.block - 1}-a``, and every scenario uses consecutive or identical block numbers. There's no test where the stream skips a block, so the one assumption the fork logic rests on is never exercised.
What I'd like to see
- Guard the parent-hash comparison so it only applies to the block immediately after the checkpoint —
blockNumber === lastProcessedBlock + 1. I tried it locally:PROBE Egoes torolledBack = 0with both events surviving, and all four of your tests still pass. - Advance the in-memory watermark as you project, so pages after the first compare against current state. The guard is still needed for real gaps, but this removes the underlying class of bug rather than the symptom.
- A test with a gap in it — checkpoint at 100, next operation at 104, healthy chain, assert no rewind is issued. And one with contiguous blocks split across two pages, asserting the page-1 event survives. Both fail on the current code.
Not blocking
deleteManyis typedPromise<{ count: number } | unknown>with a runtime shape check athaf-sync.ts:298. Prisma always returns{ count }; the defensive branch reads as uncertainty about an API that is actually stable. Type it and drop the check.- Even with the guard, a reorg that arrives mid-run on a later page won't be rewound in that run. The next run converges, so I'd leave it — but it's worth a comment so the next reader doesn't think it's covered.
The design is sound and the replay semantics are right. It's one conceptual slip: the next block in the stream and the next block on the chain are only the same thing when nothing in between was filtered out.
Summary
indexer:replay -- <from-block>path plus documentation for rebuild bounds.Trello
Verification
Note: avoid running API commands that invoke
prisma generatein parallel locally; Prisma can collide while replacingsrc/generated/prismaon Windows.