Clarify checker store invariant#1581
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v3.0 #1581 +/- ##
================================================
+ Coverage 74.80% 74.86% +0.05%
================================================
Files 430 430
Lines 45697 45722 +25
================================================
+ Hits 34185 34228 +43
+ Misses 8463 8448 -15
+ Partials 3049 3046 -3
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:
|
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Two High-severity gaps remain in the revised invariant. Both can allow persisted state that affects business outcomes to be treated as an unchecked projection.
| 7. **Never silently skip a "should not happen" branch** — A branch that is reachable only if an invariant is violated (nil where the contract says non-nil, a state we believe unreachable, a cache miss after a guaranteed preload, etc.) MUST surface a loud signal: `return fmt.Errorf("invariant: ...")` so it bubbles up, or `assert.Unreachable(...)` for SUT-level invariants exercised under antithesis. A silent `return nil` / `continue` on these branches hides real bugs — particularly catastrophic in the FSM apply path, where a no-op desyncs nodes from each other. Branches that represent genuine runtime conditions (cache miss as an expected outcome, stale proposal, deleted entity) keep their soft `return nil`. The distinction is whether the case is *expected* (soft skip OK) or *impossible by design* (must fail loudly). The comment must say *why* the case is impossible so a reader can decide whether to add a hard fail or relax the rule. | ||
|
|
||
| 8. **The audit log is the only source of truth — every other persisted dataset is a projection and must be verified by the checker** — Only `AuditEntry` (zone `Cold`, sub `Audit`) is cryptographically bound, via the hash chain that `state.BuildHashedHeaderPayload` + `processing.HashGenerator` produce and `checker.verifyAuditHashChain` verifies on every Check() run. Everything else stored in Pebble — `Log`, `AuditItem`, `AppliedProposal`, `LedgerLog.PurgedVolumes`, attribute caches (`Volume`, `Metadata`, `Transaction`, `Reference`, `Boundary`, etc.), reversion bitsets, idempotency keys, mirror cursors, chapters, bloom filters, signing keys, the read-side index — is a *projection* of orders that already live in the audit chain. Projections are rebuildable from the audit on demand, so we deliberately do NOT extend the hash chain to cover them (refactor over hash binding — see `feedback_audit_is_source_of_truth`). In exchange, **`internal/application/check/checker.go` MUST verify every projection it persists**: re-derive the value the projection should hold by replaying the audit (`ReplayLedgerLog`, `SimulateEphemeralPurge`, `partitionVolumes`, etc.) and compare to what is stored, emitting the matching `CHECK_STORE_ERROR_TYPE_*` event on divergence. A projection that the checker does not verify is a tampering vector — adding a new persisted projection without a matching compare* / collect* pass in the checker is the violation. The current passes are `compareVolumes`, `compareMetadata`, `compareTransactions`, `compareExclusionProjections` (AppliedProposal.TransientVolumes + LedgerLog.PurgedVolumes), `checkReversionInvariants`, `verifySealingHash`, `compareIdempotencyOutcomes` (frozen idempotency outcomes in SubIdempKeys vs the hash-chained AuditFailure/AuditSuccess that wrote them — the failure kind is re-derived from the chain-bound reason via `domain.KindForReason`, never stored), and `compareIndexes` (SubAttrIndex registry vs CreateIndex/DropIndex/RemovedMetadataFieldType/DeleteLedger logs — covers presence + identity; BuildStatus is intentionally excluded because it is purely informational on the cluster-wide registry entry — queries gate on the per-replica `IndexVersionState.CurrentVersion`, not on BuildStatus); extend the list as new persisted projections land. | ||
| 8. **The checker verifies the authoritative store, not every derived projection** — Only `AuditEntry` (zone `Cold`, sub `Audit`) is cryptographically bound, via the hash chain that `state.BuildHashedHeaderPayload` + `processing.HashGenerator` produce and `checker.verifyAuditHashChain` verifies on every Check() run. The checker must be able to validate the main persisted ledger state against the audit-derived state: when data is part of that authoritative store, or can affect correctness independently of the store it is derived from, `internal/application/check/checker.go` must re-derive the expected value from the audit chain or another already-verified source and compare it to what is stored, emitting the matching `CHECK_STORE_ERROR_TYPE_*` event on divergence. Persisted projections derived from the verified store — denormalized counters, rebuildable read models, informational build status, local cursors, caches, and other refreshable datasets — do not each require their own compare pass merely because they are persisted. The rule for new persisted data is to classify it explicitly: if it is authoritative, security-sensitive, or not deterministically rebuildable from verified state, add a compare* / collect* pass; if it is derived, document its rebuild source and keep recovery/checker code from treating it as an independent source of truth. The current checker passes include `compareVolumes`, `compareMetadata`, `compareTransactions`, `compareExclusionProjections` (AppliedProposal.TransientVolumes + LedgerLog.PurgedVolumes), `checkReversionInvariants`, `verifySealingHash`, `compareIdempotencyOutcomes` (frozen idempotency outcomes in SubIdempKeys vs the hash-chained AuditFailure/AuditSuccess that wrote them — the failure kind is re-derived from the chain-bound reason via `domain.KindForReason`, never stored), and `compareIndexes` (SubAttrIndex registry vs CreateIndex/DropIndex/RemovedMetadataFieldType/DeleteLedger logs — covers presence + identity; BuildStatus is intentionally excluded because it is purely informational on the cluster-wide registry entry — queries gate on the per-replica `IndexVersionState.CurrentVersion`, not on BuildStatus); extend the checker when new authoritative persisted state lands. |
There was a problem hiding this comment.
[High] A documented rebuild source is not sufficient when the persisted projection is trusted before any rebuild occurs. The repository already distinguishes projections that are actually discarded and rebuilt by a lifecycle path from ones that are only rebuildable in principle (docs/technical/architecture/audit-vs-technical-state.md). Require a tested executable detection/reset/rebuild path from still-retained verified state before serving the projection; otherwise it remains independently correctness-affecting and needs checker coverage. Please also update the linked checker architecture pages in this PR, since they currently state the opposite mandatory contract and this change rewrites a CRITICAL invariant.
There was a problem hiding this comment.
Addressed in f96ed6f (partial — see the doc-sync note below). The derived-exemption is tightened: a projection may skip a compare pass ONLY when it is deterministically rebuildable from still-retained verified state AND that rebuild is a tested, wired detect→reset→rebuild path that runs before the projection is served. A projection that is only rebuildable in principle (no executable rebuild yet) stays correctness-affecting and needs checker coverage. The dedicated rebuild wiring for currently-derived projections that lack one (e.g. usagestore, whose Reset() is not yet called by any lifecycle path) is explicitly tracked as a follow-up PR, and the invariant says not to rely on the derived-exemption for those until it lands.
On your second point — the linked checker architecture pages (docs/technical/architecture/.../checker*, audit-vs-technical-state.md) and CLAUDE.md invariant #8 still state the pre-clarification mandatory contract. Want me to sync them in THIS PR, or land the doc-page sync alongside the rebuild-wiring follow-up? Happy to do it here if you prefer a single coherent change.
…rebuild - Remove 'local cursors' from the derived-exempt category. A cursor that gates business proposal emission is correctness-affecting and stays under checker/recovery validation. Call out MirrorCursor explicitly (ahead of the audited v2LogId skips source logs; behind it replays, no dedup). - Tighten the derived-exemption: only when the projection is deterministically rebuildable from still-retained verified state AND that rebuild is a tested, wired detect->reset->rebuild path run before the projection is served; 'rebuildable in principle' is not enough. Note the dedicated rebuild path (e.g. usagestore) is a follow-up PR.
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
The update fixes the generic cursor exemption and now requires a wired pre-serve rebuild. One High factual safety issue remains on the current head; the canonical checker docs also still need to be aligned with the new contract.
…variant Re-verified the mirror mechanism: MirrorCursor is the worker's progress pointer into an external v2 source. The business effect (ingested transactions) is audit-bound (each MirrorIngest is an audit-chain order) and already checker-verified via recordMirrorIngestMutations, so ledger correctness holds regardless of the cursor. A wrong cursor only affects future ingestion fidelity (skip/re-emit) — an operational/recovery concern reconciled against source-head / max audited v2LogId, not a checker compare pass. The checker guards the business invariants of the main store, which are audit-derived and independent of replication cursors.
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Current-head re-review: the High MirrorCursor issue remains. The new classification relies on recovery reconciliation that is not implemented, and the cited checker helper does not verify cursor/source fidelity. Details are in the existing inline thread.
The cursor rides in the same atomic Raft-applied Pebble batch as the MirrorIngest orders it acknowledges (worker bundles the MirrorSync TU into the data proposal; applyProposal drains orders+TUs through one WriteSet.Merge and one batch.Commit). It can never lag/lead the ingested data operationally. - behind-tamper (replay): self-heals into an audit-consistent state, so a cursor/volume compare cannot see the doubling; the real fix is FSM v2LogId idempotency (functional follow-up), not a checker pass. - ahead-tamper (skip): a v2-parity property against the external source-head, not audit-vs-store integrity; home is the mirror worker's recovery reconciliation. Reconcile audit-vs-technical-state.md to match (was framed as a must-be- checker-enforced integrity gap).
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Current-head re-review: the High MirrorCursor classification issue remains. Atomic batching protects the normal commit/crash path, but the revised invariant still exempts a cursor whose tampering can skip or replay business proposals while the required idempotency and recovery reconciliation remain follow-ups. Evidence and the requested correction are in the existing inline thread.
…r scope Replace the ambiguous 'derived-exemption needs a wired rebuild' clause (which read as 'usagestore needs checker coverage until rebuilt is wired') with an explicit store-based classification: - Primary FSM store: in scope; derived-exemption needs a real wired rebuild. - Distinct peer secondary Pebble stores (usagestore, readstore): out of scope by construction — Check() never opens them. They are pure derived caches of the audit chain (the verified source of truth), so corruption is a rebuild event, not an undetected integrity violation. API-visibility != authoritative. Automating their drop+rebuild lifecycle is operational follow-up, not a checker-coverage gap.
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Current-head re-review found two High-severity blockers. The MirrorCursor protection is still not wired, so the existing correctness finding remains. In addition, the new store-based exemption contradicts the architecture page in this PR by treating undetected readstore corruption as an already-sound integrity contract. Details are in the existing thread and the new inline comment.
The checker verifies the authoritative MAIN store only. The readstore inverted index is a distinct per-replica peer Pebble store (not on the FSM apply path, populated locally by the indexbuilder) — out of invariant #8 scope by construction, same as the usagestore. Reframe the 'Readstore and Indexes' section, the rebuildable-projection table row, and the summary list: readstore integrity is the indexbuilder's rebuild-health concern (EN-1323), repaired by drop+re-index from the verified audit, not a compareIndexes / main-store checker obligation. compareIndexes still verifies the registry (which lives in the main store).
✅ Approve — automated reviewThis is a documentation-only PR that clarifies the checker store invariant. All previously raised major/blocker concerns from the prior discussion thread have been addressed through an iterative series of commits: the MirrorCursor exemption is now framed explicitly around atomic-batching crash paths with the tamper/corruption gap documented as open follow-up work; the readstore is classified as a peer secondary store out of checker scope with EN-1323 called out as the pending enforce-before-serving mechanism; and the linked architecture pages have been aligned or cross-referenced. The sole AI reviewer found no newly introduced correctness problems in the modified documentation. No kept finding rises to major or blocker severity. No findings. |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1581 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Current-head re-review: both High blockers remain. The new commit aligns the documentation to the proposed scope, but it provides neither the missing MirrorCursor safeguards nor readstore rebuild-health detection. Both paths still allow persisted corruption to change business-visible state or results without detection. Details are in the existing threads.
…-store-scope-claude
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1581 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Current-head re-review on 82bcb9970ba5: the merge from release/v3.0 is clean and does not change the PR-owned documentation or add any of the missing safeguards. The two existing High blockers remain, and the linked checker subsystem pages still state the opposite invariant. Details are kept in the existing inline threads to avoid duplicate findings.
…-store-scope-claude # Conflicts: # AGENTS.md # docs/technical/architecture/audit-vs-technical-state.md
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1581 (comment)
…-store-scope-claude # Conflicts: # AGENTS.md
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 5 new inline findings.
Summary: #1581 (comment)
…and merged EN-1550 Addresses NumaryBot review on the invariant clarification: - checker/README.md + audit-chain.md: stop listing the mirror cursor and readstore contents as 'tracked integrity gaps'. The mirror high-water mark (last_mirror_v2_log_id) is now verified by compareMirrorV2LogID (EN-1550); the readstore is a peer secondary store, out of checker scope by construction with automated detect/drop/rebuild tracked under EN-1323 (not yet wired). - README.md #16: add invariant #8's scope refinements (primary store; peer stores out by construction; wired-rebuild-or-informational exemption). - audit-vs-technical-state.md: update the MirrorCursor section — the *behind* tamper case is now closed by EN-1550 idempotency (only the ahead-case worker reconciliation remains); make the readstore EN-1323 open gap explicit. - AGENTS.md invariant #8 (b): split the two exemption bases — wired RebuildDelta rebuild vs purely-informational-carried-across-restore — so the informational counters are no longer incorrectly attributed to RebuildDelta.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1581 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
EN-1550 resolves the lowered-cursor replay path, and the linked checker/readstore documentation is now aligned. One blocker remains: a cursor corrupted beyond the source head is trusted by the worker, fetches no source logs, and is reported as FOLLOWING because cursor >= sourceHead. This leaves silent under-ingestion while the documentation exempts the main-store cursor from verification. Please keep this documented as a current correctness gap, or wire startup reconciliation/checker equality before approving the exemption.
paul-nicolas
left a comment
paul-nicolas
left a comment
There was a problem hiding this comment.
Multi-model PR review (Claude only — Codex unavailable)
Models: Claude (this repo's perspective) + independent verification subagent. The Codex leg could not run: the local omc ask codex CLI is broken on this machine (ENOENT: /opt/agents, Node stack trace), so this is a Claude-only review.
Scope note: all code claims were verified against the PR's actual base, origin/release/v3.0 — not a local working tree (which was on an unrelated branch with a different rebuild.go).
Verification outcome: 6 factual claims entered adversarial verification; 5 confirmed accurate, 1 flagged.
Confirmed accurate (no comment posted):
RebuildDeltagenuinely rebuildsSubAttrLedgerMetadata(internal/infra/backup/rebuild.go:272/:284,SavedLedgerMetadata/DeletedLedgerMetadata) andDefaultEnforcementMode(rebuild.go:822) — so the exemption basis (i) "wired rebuild path" holds.EphemeralEvictedCount/TransientUsedCountare not re-derived byRebuildDelta— basis (ii) holds.compareMirrorV2LogIDis a real equality check (internal/application/check/checker.go:967).processMirrorIngestimplements thev2LogID <= lastidempotent no-op (internal/domain/processing/processor_mirror.go).Check()'sCheckerholds only the main*dal.Storeand never opens the readstore — the "out of scope by construction" argument is factually sound.- No stale "integrity gap" references to the mirror cursor / readstore contents were left behind in other docs.
Posted inline: 1 Medium. Withheld (Minor/Nit): 0.
The readstore reclassification from "current integrity gap" to "peer-store rebuild-health concern (EN-1323, not yet wired)" is an intentional, transparent architectural decision with a correct factual basis, so it is not flagged.
| 7. **Never silently skip a "should not happen" branch** — A branch that is reachable only if an invariant is violated (nil where the contract says non-nil, a state we believe unreachable, a cache miss after a guaranteed preload, etc.) MUST surface a loud signal: `return fmt.Errorf("invariant: ...")` so it bubbles up, or `assert.Unreachable(...)` for SUT-level invariants exercised under antithesis. A silent `return nil` / `continue` on these branches hides real bugs — particularly catastrophic in the FSM apply path, where a no-op desyncs nodes from each other. Branches that represent genuine runtime conditions (cache miss as an expected outcome, stale proposal, deleted entity) keep their soft `return nil`. The distinction is whether the case is *expected* (soft skip OK) or *impossible by design* (must fail loudly). The comment must say *why* the case is impossible so a reader can decide whether to add a hard fail or relax the rule. | ||
|
|
||
| 8. **The audit log is the only source of truth — every other persisted dataset is a projection and must be verified by the checker** — Only `AuditEntry` (zone `Cold`, sub `Audit`) is cryptographically bound, via the hash chain that `state.BuildHashedHeaderPayload` + `processing.HashGenerator` produce and `checker.verifyAuditHashChain` verifies on every Check() run. Everything else stored in Pebble — `Log`, `AuditItem`, `AppliedProposal`, `LedgerLog.PurgedVolumes`, attribute caches (`Volume`, `Metadata`, `Transaction`, `Reference`, `Boundary`, etc.), reversion bitsets, idempotency keys, mirror cursors, chapters, bloom filters, signing keys, the read-side index — is a *projection* of orders that already live in the audit chain. Projections are rebuildable from the audit on demand, so we deliberately do NOT extend the hash chain to cover them (refactor over hash binding — see `feedback_audit_is_source_of_truth`). In exchange, **`internal/application/check/checker.go` MUST verify every projection it persists**: re-derive the value the projection should hold by replaying the audit (`ReplayLedgerLog`, `SimulateEphemeralPurge`, `partitionVolumes`, etc.) and compare to what is stored, emitting the matching `CHECK_STORE_ERROR_TYPE_*` event on divergence. A projection that the checker does not verify is a tampering vector — adding a new persisted projection without a matching compare* / collect* pass in the checker is the violation. The current passes are `compareVolumes`, `compareMetadata`, `compareTransactions`, `compareExclusionProjections` (AppliedProposal.TransientVolumes + LedgerLog.PurgedVolumes), `checkReversionInvariants`, `verifySealingHash`, `compareIdempotencyOutcomes` (frozen idempotency outcomes in SubIdempKeys vs the hash-chained AuditFailure/AuditSuccess that wrote them — the failure kind is re-derived from the chain-bound reason via `domain.KindForReason`, never stored), and `compareIndexes` (SubAttrIndex registry vs CreateIndex/DropIndex/RemovedMetadataFieldType/DeleteLedger logs — covers presence + identity; BuildStatus is intentionally excluded because it is purely informational on the cluster-wide registry entry — queries gate on the per-replica `IndexVersionState.CurrentVersion`, not on BuildStatus), and `compareMirrorV2LogID` (stored `LedgerBoundaries.last_mirror_v2_log_id` **==** `max(audited MirrorIngest.v2_log_id)` per ledger — a full equality check, `CHECK_STORE_ERROR_TYPE_MIRROR_V2LOGID_MISMATCH` on ANY divergence; the FSM enforces a contiguous applied prefix so at rest the two must be exactly equal, and the baseline floor is seeded from archived `Boundary` rows now included in the compact baseline snapshot; pre-field clusters are unsupported, no backfill leniency), and `compareSchema` (per-ledger `LedgerInfo.MetadataSchema` vs the replayed CreateLedger.initial_schema + SetMetadataFieldType/RemovedMetadataFieldType logs; under archived chapters the boundary schema is seeded from the baseline checkpoint, which carries LedgerInfo for this purpose), and `compareAccountTypes` (per-ledger `LedgerInfo.AccountTypes` vs the replayed AddAccountType/RemoveAccountType logs, baseline-seeded under archiving like the schema), and `compareLedgerPresence` (the live ledger set in the store must match the audit-derived set both ways: every audit-live ledger — CreateLedger with no later DeleteLedger, or a non-deleted baseline ledger under archiving — must have a *live* stored `LedgerInfo` else `MISSING_LEDGER` (covers an entry deleted outright OR tampered to a soft-deleted tombstone, which the compareSchema/compareAccountTypes loops skip), and every live stored `LedgerInfo` must be audit-backed else `UNAUDITED_LEDGER` (an injected empty ledger row carries no schema/types for the projection passes to flag). The audit-derived set is NEVER seeded from the live store — under archiving it comes from the baseline checkpoint, keeping the check independent of the data it verifies), and `compareReferences` (the SubAttrReference reference→txID uniqueness index vs the references replayed from CreatedTransaction/RevertedTransaction logs, baseline-seeded under archiving; verified both ways — missing, unaudited, and retargeted rows are all flagged; rows for deleted or cleanup-pending ledgers are skipped since they legitimately linger until a covering purge runs deleteLedgerData), and `compareBoundaries` (per-ledger `LedgerBoundaries` vs the checker’s re-derivation: NextTransactionId/NextLogId/PostingCount/RevertCount from the replayed logs plus the chain-bound AuditItem order effects — mirror fill-gap advances and NumscriptExecutionCount live on the orders, not the ledger-log stream — baseline-seeded under archiving; VolumeCount/MetadataCount/ReferenceCount are compared against a recount of the stored attribute rows they summarize, which their own passes verify entry-by-entry, so the chain is rows↔audit, counter↔rows; EphemeralEvictedCount/TransientUsedCount are intentionally excluded — informational, carried across restore, cf. BuildStatus); extend the list as new persisted projections land. **Known projection gaps**: `LedgerBoundaries.EphemeralEvictedCount` / `TransientUsedCount` are excluded from `compareBoundaries` and carried across restore rather than re-derived — informational counters (cf. BuildStatus); `LedgerInfo.DefaultEnforcementMode` and the ledger-metadata attribute (`SubAttrLedgerMetadata`) have no compare pass yet (both are rebuilt on restore by RebuildDelta). | ||
| 8. **The audit log is the only source of truth — every other persisted dataset is a projection and must be verified by the checker** — Only `AuditEntry` (zone `Cold`, sub `Audit`) is cryptographically bound, via the hash chain that `state.BuildHashedHeaderPayload` + `processing.HashGenerator` produce and `checker.verifyAuditHashChain` verifies on every Check() run. Everything else stored in Pebble — `Log`, `AuditItem`, `AppliedProposal`, `LedgerLog.PurgedVolumes`, attribute caches (`Volume`, `Metadata`, `Transaction`, `Reference`, `Boundary`, etc.), reversion bitsets, idempotency keys, mirror cursors, chapters, bloom filters, signing keys, the read-side index — is a *projection* of orders that already live in the audit chain. Projections are rebuildable from the audit on demand, so we deliberately do NOT extend the hash chain to cover them (refactor over hash binding — see `feedback_audit_is_source_of_truth`). In exchange, **`internal/application/check/checker.go` MUST verify every projection it persists**: re-derive the value the projection should hold by replaying the audit (`ReplayLedgerLog`, `SimulateEphemeralPurge`, `partitionVolumes`, etc.) and compare to what is stored, emitting the matching `CHECK_STORE_ERROR_TYPE_*` event on divergence. A projection that the checker does not verify is a tampering vector — adding a new persisted projection without a matching compare* / collect* pass in the checker is the violation. Two scope refinements: (a) this applies to the **primary FSM store** — the single store `Check()` opens and walks; **peer secondary stores** (`usagestore` counters, `readstore` index) are out of scope *by construction* since `Check()` never opens them and they are pure rebuildable caches whose corruption is a *rebuild* event, not an undetected integrity violation (serving a value through an API does not make it authoritative). (b) A primary-store projection may be exempted from a dedicated pass only when it is either (i) deterministically rebuildable from still-retained verified state through a real, *wired* rebuild path — not rebuildable merely in principle — or (ii) purely informational and intentionally carried across restore (cf. `BuildStatus`). These are distinct bases: of the *Known projection gaps* below, `DefaultEnforcementMode` and `SubAttrLedgerMetadata` qualify under (i) via `RebuildDelta`, while `EphemeralEvictedCount` / `TransientUsedCount` qualify under (ii) — they are NOT rebuilt by `RebuildDelta`, so the wired-rebuild basis must not be claimed for them. The current passes are `compareVolumes`, `compareMetadata`, `compareTransactions`, `compareExclusionProjections` (AppliedProposal.TransientVolumes + LedgerLog.PurgedVolumes), `checkReversionInvariants`, `verifySealingHash`, `compareIdempotencyOutcomes` (frozen idempotency outcomes in SubIdempKeys vs the hash-chained AuditFailure/AuditSuccess that wrote them — the failure kind is re-derived from the chain-bound reason via `domain.KindForReason`, never stored), and `compareIndexes` (SubAttrIndex registry vs CreateIndex/DropIndex/RemovedMetadataFieldType/DeleteLedger logs — covers presence + identity; BuildStatus is intentionally excluded because it is purely informational on the cluster-wide registry entry — queries gate on the per-replica `IndexVersionState.CurrentVersion`, not on BuildStatus), and `compareMirrorV2LogID` (stored `LedgerBoundaries.last_mirror_v2_log_id` **==** `max(audited MirrorIngest.v2_log_id)` per ledger — a full equality check, `CHECK_STORE_ERROR_TYPE_MIRROR_V2LOGID_MISMATCH` on ANY divergence; the FSM enforces a contiguous applied prefix so at rest the two must be exactly equal, and the baseline floor is seeded from archived `Boundary` rows now included in the compact baseline snapshot; pre-field clusters are unsupported, no backfill leniency), and `compareSchema` (per-ledger `LedgerInfo.MetadataSchema` vs the replayed CreateLedger.initial_schema + SetMetadataFieldType/RemovedMetadataFieldType logs; under archived chapters the boundary schema is seeded from the baseline checkpoint, which carries LedgerInfo for this purpose), and `compareAccountTypes` (per-ledger `LedgerInfo.AccountTypes` vs the replayed AddAccountType/RemoveAccountType logs, baseline-seeded under archiving like the schema), and `compareLedgerPresence` (the live ledger set in the store must match the audit-derived set both ways: every audit-live ledger — CreateLedger with no later DeleteLedger, or a non-deleted baseline ledger under archiving — must have a *live* stored `LedgerInfo` else `MISSING_LEDGER` (covers an entry deleted outright OR tampered to a soft-deleted tombstone, which the compareSchema/compareAccountTypes loops skip), and every live stored `LedgerInfo` must be audit-backed else `UNAUDITED_LEDGER` (an injected empty ledger row carries no schema/types for the projection passes to flag). The audit-derived set is NEVER seeded from the live store — under archiving it comes from the baseline checkpoint, keeping the check independent of the data it verifies), and `compareReferences` (the SubAttrReference reference→txID uniqueness index vs the references replayed from CreatedTransaction/RevertedTransaction logs, baseline-seeded under archiving; verified both ways — missing, unaudited, and retargeted rows are all flagged; rows for deleted or cleanup-pending ledgers are skipped since they legitimately linger until a covering purge runs deleteLedgerData), and `compareBoundaries` (per-ledger `LedgerBoundaries` vs the checker’s re-derivation: NextTransactionId/NextLogId/PostingCount/RevertCount from the replayed logs plus the chain-bound AuditItem order effects — mirror fill-gap advances and NumscriptExecutionCount live on the orders, not the ledger-log stream — baseline-seeded under archiving; VolumeCount/MetadataCount/ReferenceCount are compared against a recount of the stored attribute rows they summarize, which their own passes verify entry-by-entry, so the chain is rows↔audit, counter↔rows; EphemeralEvictedCount/TransientUsedCount are intentionally excluded — informational, carried across restore, cf. BuildStatus); extend the list as new persisted projections land. **Known projection gaps**: `LedgerBoundaries.EphemeralEvictedCount` / `TransientUsedCount` are excluded from `compareBoundaries` and carried across restore rather than re-derived — informational counters (cf. BuildStatus); `LedgerInfo.DefaultEnforcementMode` and the ledger-metadata attribute (`SubAttrLedgerMetadata`) have no compare pass yet (both are rebuilt on restore by RebuildDelta). |
There was a problem hiding this comment.
[Medium] usagestore is cited as an existing peer secondary store, but it does not exist on this PR's base branch
Scope refinement (a) lists usagestore counters alongside the readstore index as an existing "peer secondary store ... out of scope by construction." The same reference recurs in docs/technical/architecture/subsystems/checker/README.md:16 ("the usagestore counters") and docs/technical/architecture/audit-vs-technical-state.md:230 ("the same category as the usagestore counter cache").
But usagestore is not present on release/v3.0 (this PR's base): internal/storage/ contains only dal, pebblecfg, readstore, spool, wal, and git ls-tree origin/release/v3.0 | grep usagestore returns nothing. It exists only in the unmerged EN-1334 line. A reader on this branch who greps for it finds nothing, which undercuts a doc whose whole purpose is precise classification of which stores exist and their checker scope.
Fix: drop the usagestore mentions (the readstore inverted index is a real, sufficient example of the peer-secondary-store category), or mark usagestore explicitly as forthcoming (EN-1334). The readstore reclassification itself checks out — Check()'s Checker only holds the main *dal.Store and never opens the readstore.
There was a problem hiding this comment.
Fixed. Reworded to cite the readstore as the concrete peer secondary store today and mark the usagestore counters as forthcoming with EN-1334 — so the invariant is accurate against this branch's base (release/v3.0), where usagestore doesn't exist yet.
…ing peer store Per review: usagestore does not exist on release/v3.0 (introduced by the unmerged #1464/EN-1334). Cite the readstore as the concrete peer secondary store today and mark the usagestore counters as forthcoming, so the invariant is accurate on this branch's base.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1581 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Re-reviewed at 17253455. The new commit correctly marks usagestore as forthcoming in AGENTS.md, but it does not clear the current blockers. The advanced MirrorCursor path is unchanged: the worker trusts the persisted cursor, an empty FetchLogs result ends processing, and cursor >= sourceHead is reported as FOLLOWING. The documentation itself still acknowledges that an advanced cursor silently skips source logs while reconciliation remains a follow-up, so this must remain documented as a current correctness gap until the worker/startup safeguard is wired. Also, the same usagestore factual issue remains in checker/README.md and audit-vs-technical-state.md; please update those two references as already requested in the existing thread.
…fs in README + audit-vs-technical Per flemzord re-review of #1581: - audit-vs-technical-state.md: the *ahead* MirrorCursor case is now stated as a CURRENT correctness gap — an advanced/corrupted cursor fetches no source logs and reports FOLLOWING (cursor >= sourceHead), silently under-ingesting v2->v3 with no worker/startup safeguard wired yet. It stays a v2-parity concern (not checker scope), but is explicitly open until reconciliation lands, not framed as a tidy tracked follow-up. - checker/README.md + audit-vs-technical-state.md: usagestore marked as forthcoming (EN-1334), matching the AGENTS.md fix — it does not exist on release/v3.0 yet.
|
Addressed in the latest commit:
|
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1581 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Current-head re-review on 36675d44: the requested delta correctly documents the advanced MirrorCursor path as a current/open correctness gap and fixes all touched usagestore references to say forthcoming EN-1334. Two consistency blockers remain in the existing threads: the readstore is still described as sound/outside integrity concerns while the same architecture page admits corrupted READY indexes are served without detection, and the linked checker pages still contradict both the peer-store carve-out and the new MirrorCursor gap wording. I replied on the existing discussions rather than duplicating inline findings. Functional CI is fully green.
…ked pages flemzord re-review: the checker pages contradicted audit-vs-technical-state.md. Align all of them to one scope + gap classification: - readstore/peer stores are out of *main-store checker* scope by construction, but that is NOT a claim they are integrity-safe — their contents are a current open integrity gap until per-replica detect/drop/rebuild is wired (EN-1323). Dropped the 'rebuild event, not an undetected integrity violation' framing in AGENTS.md and the 'no longer integrity gaps' framing in checker/README.md. - MirrorCursor: the pointer/high-water mark is verified (compareMirrorV2LogID), not a gap; but the advanced-cursor path (cursor beyond source head → silent under-ingestion) remains a current open correctness gap until worker/startup reconciliation is wired. Stated consistently in README + audit-chain, matching audit-vs-technical-state.md. - audit-chain.md's normative 'verify each such projection' now scoped to the primary FSM store, with the peer-store carve-out called out.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 3 stale NumaryBot review threads (1 fixed, 2 outdated).
Summary: #1581 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Approved current head c12ad410. The follow-up now keeps the main-store checker scope distinction without hiding the remaining gaps: readstore contents are explicitly a current peer-store integrity gap until EN-1323 detect/drop/rebuild is wired, the advanced MirrorCursor path is explicitly open until worker/startup reconciliation exists, and the linked checker pages now match that classification. CI is green.
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Approved current head c12ad410. The latest docs pass resolves the remaining contradictions from my previous review: peer secondary stores are now scoped out of the main-store checker without claiming they are integrity-safe, the readstore gap remains explicitly open until EN-1323, and the advanced MirrorCursor path is consistently described as a current correctness gap. Required checks are green.
The release/v3.0 merge brought in compareBoundaries (checker) + RebuildDelta (backup) code that reads/writes the per-ledger usage counters (PostingCount, RevertCount, NumscriptExecutionCount, VolumeCount, MetadataCount, ReferenceCount) on LedgerBoundaries — but EN-1334 moved those counters to the usagestore peer secondary store, so the fields no longer exist there (compile break). Resolve in the peer-store direction, consistent with the checker-scope framing merged in #1581: the usagestore is a peer secondary store, out of main-store checker scope by construction; its counters are a rebuild-health concern, not an invariant-#8 main-store projection. - checker.go: compareBoundaries now verifies only NextTransactionId/NextLogId; dropped the counter folds in advanceExpectedBoundaries / collectAuditOrderBoundaryEffects (kept the NextTransactionId advances) and the 6 counter rows + attribute recounts; removed the now-unused countLedgerAttributeKeys helper. - rebuild.go: RebuildDelta no longer folds those counters onto LedgerBoundaries (kept NextTransactionId/NextLogId + mirror fill-gap advances); removed the countNetAttributes/countAttributeKeys helpers and the trimmed recordTransactionBoundary signature. - tests: dropped assertions on the removed main-store counters (kept the NextTransactionId/NextLogId + reference-index checks). - AGENTS.md invariant #8: compareBoundaries clause updated — usage counters live in the usagestore peer store, out of main-store checker scope. Verified: go build ./..., go test ./internal/application/check/... ./internal/infra/backup/..., golangci-lint — all green.
4 participants
Summary
Clarifies the checker invariant documented through
CLAUDE.md/AGENTS.md: the checker must verify the authoritative ledger store, not every persisted projection derived from that verified store.Details
Validation
git diff --checkGOROOT= go build ./...GOROOT= just pre-commitpassed, but generated proto header rewrites were restored because this PR is intentionally doc-only.nix develop --command bash -c "just pre-commit"failed before completion due to a local Go stdlib/tool mismatch (go1.26.3vsgo1.26.4).