feat(EN-1550): idempotent mirror ingest on v2LogId#1587
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
🛑 Changes requested — automated reviewAll previously raised blockers and major issues (contiguous-prefix enforcement, checker bidirectional equality, zero v2LogId rejection, nil-slot idempotency, absent boundary row detection) have been addressed in successive commits and confirmed resolved in the PR discussion. The one remaining open thread (Thread 11) concerns a false-positive in the checker for legitimately deleted mirror ledgers: after the absent-boundary-row fix, |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1587 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Two High-severity blockers remain on head 7ea7abd56694dc7bb883fa27716b5f7573801e78:
- The new persisted high-water mark gates future FSM application but is not checker-verified against the audit chain, violating the repository projection invariant: #1587 (comment)
- Existing mirror ledgers decode the new field as zero and receive no migration/backfill, so a rolled-back cursor can still replay and double-apply historical postings until a newer ingest advances it: #1587 (comment)
I am not duplicating the existing inline comments. Targeted processing tests pass under the Nix toolchain; these are correctness and upgrade-safety blockers.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (65.47%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release/v3.0 #1587 +/- ##
================================================
+ Coverage 74.97% 75.00% +0.02%
================================================
Files 429 429
Lines 44942 45037 +95
================================================
+ Hits 33696 33780 +84
- Misses 8305 8309 +4
- Partials 2941 2948 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1587 (comment)
|
I reviewed the current head ( GitHub will not let me submit a formal “request changes” review on my own PR identity, so I am posting the blocking review as a PR comment. Findings:
I also checked the generated protobuf files. I did not find an issue there: Verification run locally: GOROOT= go test ./internal/domain/processing ./internal/application/check -run 'Mirror|V2Log|ProcessOrders' -count=1 |
…aseline boundary rows
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1587 (comment)
|
Update — all three review findings are now addressed (head `d360d6fef`), following the product decision that we do not support backward-compat with existing mirror clusters (no backfill; a pre-field cluster re-mirrors):
Independently re-reviewed at |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1587 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Current head dd8bff930ebcb026639ac00f856541e8fe09fe12 still has one High/Critical-blocking issue: malformed mirror ingest orders with v2LogId == 0 bypass the replay/gap guards and do not advance LastMirrorV2LogId, so they can be replayed and applied repeatedly. I replied on the existing inline thread instead of opening a duplicate: #1587 (comment)
The previous contiguity, checker equality, and baseline-boundary blockers look addressed. The remaining missing-boundary checker point is minor and not an approval blocker under the current High/Critical threshold. Targeted tests pass under Nix: GOROOT= go test ./internal/domain/processing ./internal/application/check ./internal/infra/attributes -run 'Mirror|V2Log|Baseline|ProcessOrders' -count=1.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1587 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1587 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1587 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Approved on current head 30330d3fdb8d67437c681ad1734f09af42106926.
Re-reviewed the follow-up fixes: the zero v2LogId path now rejects before mutation, the mirror high-water mark is checked as audit equality, absent boundary rows are detected for active mirror ledgers, and legitimately deleted mirror ledgers are excluded from the absent-row branch via deletedInReplay. I resolved the remaining duplicate thread after confirming TestCompareMirrorV2LogID_DeletedMirrorLedgerNotFlagged covers that case.
Targeted verification passed under Nix: GOROOT= go test ./internal/domain/processing ./internal/application/check ./internal/infra/attributes -run 'Mirror|V2Log|Baseline|ProcessOrders|DeletedMirror|AbsentRow|ZeroV2LogId' -count=1.
3 participants
What
Makes mirror ingest idempotent on
v2LogIdat the FSM apply path, so replaying an already-applied source log is a deterministic no-op. Closes the "tampered/rolled-backMirrorCursor→ worker re-emits already-applied logs →processMirrorIngestforce-adds postings twice → doubled balances" concern raised on #1581.How — the guard
LedgerBoundaries.last_mirror_v2_log_id(field 11,fixed64): the highest sourcev2LogIdalready applied to this mirror ledger.processMirrorIngest(internal/domain/processing/processor_mirror.go), before any mutation (no ledger re-touch, no cache write), an entry whosev2LogId <= last_mirror_v2_log_idreturns a deterministic no-op(nil, nil).last_mirror_v2_log_idis advanced to the entry'sv2LogId— set once on the wrappingMirrorLogEntry, so it covers every inner kind (CreatedTransaction, SavedMetadata, DeletedMetadata, RevertedTransaction, FillGap).ProcessOrders(internal/domain/processing/processor.go) now treats a(nil, nil)processor return as a no-log outcome: it consumes no sequence id, appends no degenerateLog{Payload:nil}, records noCreatedLog, and does not absorb into the sink. The per-order slot stays nil and is skipped byWriteSet.Mergeexactly like an idempotency-replayReferenceSequence.Invariant safety (FSM hot path)
last_mirror_v2_log_id) and the order (v2LogId). No time, no randomness, no map iteration.LedgerBoundaries, already preloaded (SubAttrBoundary) and coverage-gated on every mirror-ingest order (Worker.extractMirrorNeeds) and already mutated on this path.return nil, nilis correct per invariant Add Postgres storage #7's expected-vs-impossible distinction (notassert.Unreachable, which is reserved for impossible-by-design branches).adapter/v2/translator.go: TranslateBatch), so<= 0never wrongly skips the first ingest (default 0 → first real id >= 1 applies).Checker
last_mirror_v2_log_idneeds no new tamper-vector checker pass: it is derivable from the auditedMirrorIngestorders (the max appliedv2LogId— the same quantity the still-owedMirrorCursorequality re-derives) and only gates future application. It is not independently business-authoritative, so it rides on the (pending) mirror-cursor coverage. Documented indocs/technical/architecture/audit-vs-technical-state.md(Mirror Cursor section).Out of scope
The ahead direction (a cursor beyond the true source head) is worker-side source-head recovery and is explicitly out of scope here — only the behind/replay direction is closed.
Tests
internal/domain/processing:v2LogId(== high-water mark) is a no-op: no volume writes (balances not doubled), no ledger re-touch, no boundary Put,(nil, nil)returned.v2LogIdafter a higher one is skipped.last_mirror_v2_log_id.ProcessOrders): a replay emits no log slot, consumes no sequence id, and never reaches the sink.Verified:
go build ./...,go test ./internal/domain/processing/... ./internal/infra/state/... ./internal/proto/... -count=1all green;golangci-lintclean on the touched package.Relates to #1581.