feat(EN-1334): usagebuilder subsystem + event-counter cutover (EN-1420)#1464
feat(EN-1334): usagebuilder subsystem + event-counter cutover (EN-1420)#1464gfyrag wants to merge 39 commits into
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 reviewThe consolidated review covers the single round of new findings from the current head. All previously raised issues (HTTP DTO camelCase serialization, epoch timestamp handling, stale cursor boot guard, VolumeCount seeding for pre-upgrade ledgers, checker/usagestore scope, LedgerStats.metadata_count proto reservation, and volume annotation category verification) have been resolved or explicitly accepted by design through prior discussion threads and are not repeated here. Two new major issues were identified in the current diff, both stemming from the color dimension being dropped in two separate volume-processing paths. First, the indexbuilder's purged/ephemeral exclusion key is constructed without the volume color, so the |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 4 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 6 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 5 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 8 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 5 new inline findings.
Summary: #1464 (comment)
gfyrag
force-pushed
the
feat/EN-1334-usagebuilder
branch
from
a705c93 to
5733eb9
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1464 (comment)
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (68.32%) 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 #1464 +/- ##
==============================================
Coverage 75.02% 75.03%
==============================================
Files 432 441 +9
Lines 45925 46582 +657
==============================================
+ Hits 34455 34951 +496
- Misses 8405 8508 +103
- Partials 3065 3123 +58
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 4 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 8 new inline findings.
Summary: #1464 (comment)
gfyrag
force-pushed
the
feat/EN-1334-usagebuilder
branch
from
c638a14 to
41c8ba7
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 7 new inline findings.
Summary: #1464 (comment)
gfyrag
force-pushed
the
feat/EN-1334-usagebuilder
branch
from
41c8ba7 to
4c64ab8
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 6 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 7 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 9 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 11 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 6 new inline findings.
Summary: #1464 (comment)
gfyrag
force-pushed
the
feat/EN-1334-usagebuilder
branch
from
aaa6014 to
cedcab0
Compare
|
Rebased on Addressed the 9 outstanding NumaryBot threads inline; summary:
|
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 9 new inline findings.
Summary: #1464 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1464 (comment)
gfyrag
left a comment
gfyrag
left a comment
There was a problem hiding this comment.
Found one invariant-level issue.
This PR introduces a new persisted projection store (internal/storage/usagestore) for template usage and the ledger usage counters, but internal/application/check/checker.go does not add any compare pass that re-derives those values from the audit chain and compares them against the stored usagestore rows. The current Check flow still stops at the existing projections (compareVolumes, compareMetadata, compareTransactions, compareExclusionProjections, compareIdempotencyOutcomes, compareIndexes, etc.); rg finds no usagestore / usage-counter / template-usage verification in the checker.
That violates invariant #8 from CLAUDE.md: the audit log is the only source of truth, every other persisted dataset is a projection, and every new persisted projection must be verified by the checker. As written, tampering with PrefixTemplate or PrefixCounter rows can change GetTemplateUsage / GetLedgerStats results without CheckStore surfacing a mismatch. Please add checker coverage that replays the audit-derived usagebuilder state and compares every persisted usage projection, emitting an appropriate CHECK_STORE_ERROR_TYPE_* on divergence.
gfyrag
force-pushed
the
feat/EN-1334-usagebuilder
branch
from
0649e1a to
18a8629
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1464 (comment)
gfyrag
force-pushed
the
feat/EN-1334-usagebuilder
branch
from
18a8629 to
1443eb1
Compare
…usagebuilder # Conflicts: # internal/application/ctrl/controller_default.go # internal/bootstrap/module.go
After the watermark/rollback revert (b09f824) the docs still described a rollback-rewind mechanism and a checker backstop that no longer exist: - AGENTS.md: the volume-preload invariant claimed 'the checker's volume-count pass would surface the drift'. usagestore is a rebuildable peer side-store outside checker scope (invariant #8 = primary-store projections only), so there is no such backstop — reworded to reflect that (which makes preload correctness more load-bearing, not less). - module.go: the NewBuilder comment claimed a shared 'rebuild threshold' + a rollback-then-catch-up gap heuristic. The final arg is batchSize (0=default) and there is no rollback guard (append-only audit). - usagestore/store.go: Reset() doc framed it as builder-triggered on rollback detection; it is a full-rebuild primitive not called for rollbacks. Also corrected the crash-safety note to rely on single-batch atomicity. - counters.md: dropped the 'automatic boot/tick rewind / Reset() on rollback detection' claim; the fold only moves forward from the cursor. Docs/comments only; build clean.
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Current-head re-review: the High usagestore integrity finding remains. PR #1581 does not clear it under its own proposed classification because these API-visible rows and their cursor affect correctness independently and have no executable archival-safe rebuild/detection path. Details are posted in the existing inline thread.
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Current-head, conversation-aware re-review: the existing High remains. The latest response reclassifies the side-store but does not provide an executable, archival-safe rebuild: Reset() has no production caller, the CLI was removed, and replay after archival is explicitly known to undercount VolumeCount. Details and current-head evidence are in the existing inline thread: #1464 (comment). No additional distinct High/Critical finding was found.
…usagebuilder # Conflicts: # tests/e2e/business/transient_accounts_test.go
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1464 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Current-head re-review at 6f3685135: the release/v3.0 merge resolves only a test conflict and does not address the existing High. The usagestore remains served without checker coverage or a wired archival-safe rebuild/detection path; current-head evidence is in the existing inline thread: #1464 (comment). The new NumaryBot migration-reset finding matches the explicitly accepted cutover semantics and is not an additional High/Critical blocker.
|
Proto cleanup finding from the EN-1551 review: the Please add |
…usagebuilder # Conflicts: # internal/proto/raftcmdpb/raft_cmd.pb.go # internal/proto/raftcmdpb/raft_cmd_reader.pb.go # internal/proto/raftcmdpb/raft_cmd_vtproto.pb.go # misc/proto/raft_cmd.proto
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1464 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Current-head re-review at 47dce64: the release/v3.0 merge does not touch the usagebuilder or usagestore recovery/integrity paths, so the existing High blocker remains. API-visible usage rows are still unchecked, Store.Reset() still has no production caller, and the documented rebuild cannot be exact after archival. Please address the existing thread before re-requesting review: #1464 (comment). The two latest skipped-order accounting findings are already covered by NumaryBot and are not duplicated here.
paul-nicolas
left a comment
paul-nicolas
left a comment
There was a problem hiding this comment.
Multi-model PR review (Claude + Codex)
Two independent reviewers (Claude, grounded in the PR-head tree; Codex via codex exec) covered architecture, correctness, concurrency, error handling, security, the audit-chain cursor semantics, the checker invariant, the proto cutover, API/contract changes, tests, and doc drift. Findings were then re-verified adversarially against the exact PR head (47dce641d) before posting.
Important: the local review worktree was 8 commits behind the pushed head, so several early findings referenced stale code. All findings below were re-checked against the real head; the stale ones were rejected.
Verification outcome
12 Critical/Major/Medium candidates entered adversarial verification: 3 survived (posted inline), 4 were downgraded below the posting threshold (listed below), and 5 were rejected as false:
- Rejected — timestamp µs-vs-ns bug in the usage handler: refuted. At head the handler uses
ts.AsTime().UTC().Format(...)(the correct microsecond path) andTestToTemplateUsageJSON_MicrosecondUnitAndCamelCaselocks it. The buggytime.Unix(0, ...)was only in stale code. - Rejected —
readLogsilently skips missing/non-Apply logs (invariant #7): refuted at head. - Rejected — usage endpoint missing 404 in openapi: the 404 is present (openapi.yml:1704) and was added by this PR; the 401/403 comparison used the wrong sibling endpoint (Numscript-Library siblings also carry none).
- Rejected — lag gauge conflates log vs audit sequence: refuted against head code.
- Rejected —
GetLedgerStatscomposes two unsynchronized stores: the usagestore is by design an eventually-consistent lagging projection; no contract requires cross-store atomicity, and intra-counter atomicity is already provided by the single snapshot.
Posted inline (3)
- Major —
misc/proto/raft_cmd.proto— merge into current release/v3.0 won't compile (compareBoundariesreads the deleted counter getters). - Medium —
internal/adapter/http/handlers_get_ledger_stats.go— openapiLedgerStatsschema mismatches the handler DTO. - Medium —
internal/application/usagebuilder/process_audit_test.go— audit-processing pipeline (processAuditEntries+ dispatch fan-out + dedup) has no end-to-end test.
Notable point of agreement / adjustment
Both reviewers flagged that the new usagestore projection is not verified by the checker (invariant #8). On verification this was downgraded to Minor / documented tech-debt: usagestore is a physically-separate secondary Pebble store (peer to readstore, which is itself not content-verified — checker.go:485-488), it is explicitly documented as out of checker scope in the invariant-#6 note and the new docs/.../usage/ pages, and it is rebuildable from the audit chain. It is not an unacknowledged violation — but a client-served projection with no cryptographic backstop is a real residual item, same as the one readstore already carries.
Not posted inline — Minor / Low (would still be worth addressing)
internal/application/check/checker.go(Minor) — nocompareUsage*/collectUsage*pass for the usagestore counters (documented gap; see above).internal/application/usagebuilder/builder.go:172,216(Minor) — the PR description's "self-heal on boot / automatic rollback-reset" is not implemented as an automatic mechanism.usagestore.Store.Reset()exists (store.go:204) but is unwired — called only from tests, no boot hook, no rollback detection, noledgerctlcommand.boot()computes the cursor-vs-audit-head gap and discards it. The append-only justification holds for follower-sync and, via the fresh-data-dir guard (module_restore.go:57-64), for the documented backup-restore flow — but that guard scanscheckpoints/, notusage/, so an in-place restore where an operator clears onlycheckpoints/would strand a stale usage cursor with no wired remedy. Consider wiringReset()to aledgerctl store rebuild-usagecommand (its own doc claims it exists) or a boot-timecursor > pebbleLastself-heal.misc/proto/common.proto:1647(Low) —LedgerStatsdrops field 3 (metadata_count) without areserved 3;clause, unlikeLedgerBoundariesin the same PR.LedgerStatsis never persisted (fresh per-RPC DTO), so impact is low, but addingreserved 3;restores consistency and proto hygiene.internal/application/ctrl/controller_default.go:645(Low) — checkpoint-scopedGetLedgerStatsnow returns zero usage counters (pre-PR it returned real checkpointed counts). This is documented and intentional (usagestore is live-only), but silent zeros are indistinguishable from an empty ledger — consider omitting the fields or signaling "not available for historical scope."
Event kept as COMMENT (no approve/request-changes).
| // unknown-field bytes and re-emits them on MarshalVT, so an old persisted | ||
| // Pebble record carrying e.g. volume_count at tag 3 would be mis-decoded | ||
| // as whatever new field claimed tag 3. | ||
| reserved 3, 4, 5, 6, 7, 8, 9, 10; |
There was a problem hiding this comment.
[Major] Merging this branch into current release/v3.0 will not compile — compareBoundaries reads the getters this PR deletes
This branch's merge base is ea3dfba, which predates compareBoundaries in internal/application/check/checker.go (added on release/v3.0 by 310ba1f, after the base). Current release/v3.0's compareBoundaries (checker.go ~5331-5336) calls GetPostingCount(), GetRevertCount(), GetNumscriptExecutionCount(), GetVolumeCount(), GetMetadataCount(), GetReferenceCount() on LedgerBoundaries — the exact fields reserved here (tags 3-10). A 3-way merge takes both sides with no textual conflict (proto and checker are different files), so the merged tree keeps compareBoundaries while those getters are gone → a deterministic go build failure on six undefined method calls. Verified: the getters no longer exist in the generated raftcmdpb at this head.
Fix: rebase onto current release/v3.0 and reconcile compareBoundaries — drop the removed counters from the boundary compare and re-home their verification (see the usagestore/checker gap in the review summary). Re-run GOROOT= go build ./... after the rebase.
| @@ -10,7 +10,6 @@ import ( | |||
| type ledgerStatsJSON struct { | |||
| TransactionCount uint64 `json:"transactionCount"` | |||
There was a problem hiding this comment.
[Medium] openapi LedgerStats schema does not match this DTO
This handler serializes nine fields (transactionCount, volumeCount, referenceCount, postingCount, logCount, ephemeralEvictedCount, transientUsedCount, revertCount, numscriptExecutionCount) and never emits accountCount or metadataCount. But openapi.yml's LedgerStats schema (openapi.yml:3634-3649; endpoint description at :291) declares only accountCount (marked required) and transactionCount — a required field the server never returns, plus all eight real counters missing. Clients generated from the spec will mis-model this response.
The drift is pre-existing (the schema block is not in this diff), but this PR reworks the stats contract — it drops metadataCount and re-sources the counters from usagestore — so this is the right change to also sync the spec, per the repo's "update openapi.yml if HTTP endpoints change" rule. Fix: replace the schema properties with the nine fields actually serialized, drop accountCount from properties/required, and update the endpoint description.
| "github.com/formancehq/ledger/v3/internal/proto/commonpb" | ||
| "github.com/formancehq/ledger/v3/internal/storage/usagestore" | ||
| ) | ||
|
|
There was a problem hiding this comment.
[Medium] The audit-processing pipeline has no end-to-end test coverage
The tests here cover the pure aggregation helpers (addCounter, addTemplateUsage, applyDelta, mergeTemplateUsage, timestampGreater), and commitBatch + the store layer are integration-tested against a temp-dir store (TestCommitBatch_SameBatchDeleteRecreateDoesNotResurrect, usagestore/store_test.go). But the load-bearing ingestion path above commitBatch is untested: processAuditEntries (batch boundaries, EOF break, cursor advance), the dispatchOrder/dispatchCreateTransaction/dispatchRevertTransaction/dispatchMirrorIngest fan-out, applyVolumeAnnotations per-audit-entry dedup (the "shared account counted once across N orders" invariant), and readLog. These are exactly the paths most likely to regress silently — and per invariant #8 there is no checker backstop for the usagestore counters. Add table-driven tests that feed synthetic AuditEntry/AuditItem/Log fixtures through processAuditEntries against a real temp-dir usagestore and assert the resulting counters, including the multi-order dedup case.
…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.
paul-nicolas
left a comment
paul-nicolas
left a comment
There was a problem hiding this comment.
My bad, wrong approve, please fix the comments if you can
* Clarify checker store invariant * Address review: keep emission-gating cursors verified; require wired 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. * Reclassify MirrorCursor as technical state, not a checker business invariant 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. * MirrorCursor: classify as technical state with precise tamper analysis 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). * Invariant #8: classify by store — peer secondary stores out of checker 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. * docs: align readstore framing with main-store-only checker scope 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). * docs(checker): align linked pages + exemption bases with invariant #8 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. * docs(checker): mark usagestore as forthcoming (EN-1334), not an existing 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. * docs(checker): frame ahead-cursor as a current gap; fix usagestore refs 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. * docs(checker): align peer-store & MirrorCursor gap wording across linked 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. --------- Co-authored-by: Geoffrey Ragot <geoffrey@formance.com>
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.
|
@flemzord PR rebasée sur Le cadrage peer-store est désormais acté et mergé dans En rebasant, le merge a fait remonter le
Vérif locale : Restent hors périmètre de cette PR : |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1464 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Requesting changes on current head 6f4519f. I validated the current NumaryBot findings against the code and am not duplicating its inline threads.
Blocking issues:
-
Skipped CreateTransaction orders still inflate usage counters.
processAuditEntriesdispatches any audit item with a non-zeroLogSequence, includingOrderSkippedlogs.readLog()returns empty annotations for anOrderSkippedpayload, butdispatchCreateTransactionstill incrementsCounterReference,CounterNumscriptExecution, and template usage directly from the raw order atinternal/application/usagebuilder/process_audit.go:469-491. A skipped transaction should contribute no successful transaction/template usage counters; gate those increments on the produced log being a realCreatedTransaction. -
Skipped RevertTransaction orders still inflate
RevertCount.dispatchRevertTransactionincrementsCounterRevertbefore inspecting the produced log atinternal/application/usagebuilder/process_audit.go:508. If the audit item points at anOrderSkippedlog,readLog()returns no reverted transaction, but the revert counter is already bumped. Only increment after confirming the log payload isRevertedTransaction. -
LedgerStats.metadata_count = 3was removed without reserving the tag/name.misc/proto/common.proto:1647-1655jumps from field 2 to field 4 but does not reserve3or"metadata_count". This is a removed public/protobuf field; please addreserved 3;andreserved "metadata_count";and regenerate.
Non-blocking but should be cleaned: docs/technical/architecture/subsystems/usage/counters.md still opens by saying the page explains how the checker verifies these counters, while the current design explicitly keeps usagestore out of main-store checker scope. Please align that intro with the accepted peer-store/rebuild-health framing.
flemzord review: dispatch* incremented success-scoped usage counters from the raw order even when the produced log was an OrderSkipped (order committed nothing). - readLog now reports the produced log kind (isCreatedTx / isRevertedTx). - dispatchCreateTransaction: gate the reference / numscript-execution / template-usage increments on a produced CreatedTransaction (skips contribute nothing; posting/volume counters already derive from the produced log). - dispatchRevertTransaction: gate CounterRevert on a produced RevertedTransaction (previously bumped unconditionally before reading the log). - docs/usage/counters.md: intro aligned to the peer-store framing (usagestore is out of main-store checker scope; integrity is rebuild-health, not verified by the checker); Revert/Numscript/Reference deltas documented as per-successful-order. go build ./..., usagebuilder tests, golangci-lint — green.
|
@flemzord merci pour la re-review — traité dans 1. CreateTransaction skippé gonflait les compteurs ✅ — 2. RevertTransaction skippé gonflait 3. 4. (non-bloquant)
|
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1464 (comment)
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Re-checking current head 0510143f.
Thanks, the skipped-order counter bugs and the usagebuilder counters doc issue are fixed in this head.
One blocker remains: LedgerStats.metadata_count = 3 is still removed from misc/proto/common.proto without reserving the field number and name. Current LedgerStats jumps from field 2 to field 4, but public/protobuf removals must reserve both the tag and the name to prevent future incompatible reuse.
Please add:
reserved 3;
reserved "metadata_count";inside message LedgerStats, then regenerate the protobuf outputs.
…usagebuilder # Conflicts: # internal/infra/state/write_set_ephemeral_purge.go # internal/proto/commonpb/common.pb.go
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1464 (comment)
|
|
||
| out := make(map[domain.AccountAssetKey]struct{}, len(purged)+len(ephemeral)) | ||
| for _, v := range purged { | ||
| out[domain.AccountAssetKey{Account: v.GetAccount(), Asset: v.GetAsset()}] = struct{}{} |
There was a problem hiding this comment.
🟠 [major] Color dimension dropped when building purged/ephemeral volume exclusion keys
When a purged or ephemeral volume is colored, the exclusion key is built with the zero color. The isExcluded lookup later uses the full (account, asset, color) tuple, so colored purged postings are never excluded from the readstore account-to-transaction mappings. Uncolored postings for the same account/asset can also be over-excluded. This causes colored-balance queries to return stale index entries after an eviction.
Suggestion: Include the volume's color field when constructing the exclusion key so that the lookup tuple (account, asset, color) matches exactly what isExcluded expects.
|
|
||
| // volumeSetKey mirrors state.volumeSetKey — kept local to avoid crossing | ||
| // package boundaries just for a triple of strings. | ||
| type volumeSetKey struct { |
There was a problem hiding this comment.
🟠 [major] Color dimension omitted from usage volume dedup keys causes undercounting
The usagebuilder deduplicates volume annotations per audit entry using only (ledger, account, asset), but volume identity now includes color. If the same account/asset pair has two distinct colors created, purged, or marked ephemeral within a single audit entry, the second color is silently dropped, causing VolumeCount and/or EphemeralEvictedCount in GetLedgerStats to undercount.
Suggestion: Add the color field to the deduplication key so that each (ledger, account, asset, color) tuple is treated independently.
gfyrag
left a comment
gfyrag
left a comment
There was a problem hiding this comment.
Re-reviewing current head ea05119a: not approvable yet.
I revalidated the current diff and agree with the two latest NumaryBot color findings; I am not duplicating those inline threads. I am also not blocking on the LedgerStats.metadata_count reservation request under the current pre-GA/no-compat direction.
Blocking issues:
- The indexbuilder drops
TouchedVolume.colorwhen building purged/ephemeral exclusion keys.
misc/proto/common.proto defines TouchedVolume as (account, asset, color), and internal/application/indexbuilder/process_logs.go later checks exclusions with the full (account, asset, color) tuple. However, internal/application/indexbuilder/applied_proposal_sync.go builds the purged/ephemeral exclusion set using only account and asset. A colored purged/ephemeral bucket therefore does not match the exclusion lookup for that same colored posting, leaving stale account-to-transaction mappings in the readstore; the uncolored bucket for the same account/asset can also be over-excluded.
- The usagebuilder deduplicates per-entry volume annotations without
color.
internal/application/usagebuilder/process_audit.go defines its local volumeSetKey as (ledger, account, asset), then uses it for newKept, purged, and ephemeral volume annotations. Volume identity is now (ledger, account, asset, color), so two colors of the same account/asset in the same audit entry collapse into one counter delta. That undercounts VolumeCount and/or EphemeralEvictedCount in GetLedgerStats.
Non-blocking but should be fixed in this PR:
openapi.ymlstill documentsTemplateUsage.countasint64, while the proto/Go/JSON surface isfixed64/uint64. Generated clients can use the wrong signed type.- The
LedgerStatsOpenAPI schema still describes onlyaccountCountandtransactionCount, but the HTTP DTO now returnstransactionCount,volumeCount,referenceCount,postingCount,logCount,ephemeralEvictedCount,transientUsedCount,revertCount, andnumscriptExecutionCount. docs/technical/architecture/subsystems/usage/usagebuilder.mdstill says a primary-store rollback/cursor-ahead situation triggersusagestore.Reset()on the next boot/tick, while the code explicitly documents that no rollback/cursor-ahead re-check is needed andReset()has no production caller.
Checks run locally:
git diff --check origin/release/v3.0...HEAD
GOROOT= go test ./internal/application/usagebuilder ./internal/storage/usagestore ./internal/adapter/http -run 'Test.*(Usage|Counter|Template|LedgerStats|Numscript|Store|Process)' -count=1
GOROOT= go test ./internal/application/usagebuilder ./internal/application/indexbuilder -run 'Test.*(Color|Volume|Purged|Ephemeral|Excluded|Dedup)' -count=1They pass, but the last command reports [no tests to run] for usagebuilder, so the colored-volume counter bug is currently uncovered.
4 participants
Summary
usagebuildersubsystem materialises per-Numscript-template invocation counters (count + lastUsed) from the FSM audit chain into a dedicated peer secondary Pebble store (usagestore). Exposed viaGET /v3/{ledger}/numscripts/{name}/usage.LedgerBoundariesintousagestore. This is the full cutover: the event counters (posting, revert, numscript_execution, reference) AND the attribute-derived counters (Volume, EphemeralEvicted, TransientUsed) now live inusagestore;MetadataCountis dropped for now (no sound foundation — see below). The FSM increment sites are removed and the proto fields deleted + reserved.LedgerStatssources every migrated counter fromusagestore.Architecture notes
CreatedTransactionpayload doesn't carryNumscriptReference— the raw order inAuditItem.SerializedOrderdoes. When a posting count is needed (revert / script-backed create) the specific log is fetched viaReadLogBySequence— a single point read per item on hot Pebble cache.LedgerLog—NewKeptVolumes(+1),PurgedVolumes(−1),EphemeralVolumes(0, tracked forEphemeralEvictedCount) — split this way (vs. the earlier 2-list encoding) to keep ephemeral-heavy workloads from paying 2× bytes on the log payload (EN-1422).[0xFE][0x01]. LogCommitted notification wakes the loop; the audit chain advances in the same batch as the log. If the primary store is rolled back beneath the cursor, the builder self-heals: it wipes the projection and replays from sequence 0 (usagestore.Store.Reset()on boot).usagestoreopens its own Pebble instance at<data-dir>/usage/(peer toreadstore). Rebuild = drop the directory (orReset()); the builder replays from cursor=0.feedback_constructor_injectionin project memory), and the existingsignal.FanOutis extended to a 4th target.Cutover semantics (EN-1420 / EN-1422)
On production upgrade, every migrated counter resets to 0 and repopulates from the earliest audit entry still reachable in Pebble. Historic pre-upgrade counter values are lost. This is the trade-off — accepted by product to avoid a stateful migration. A one-time seed from
LedgerBoundarieswas explicitly considered and rejected because theLedgerBoundariescounters were themselves inaccurate on any ledger with pre-existing history.processor_transaction.go,processor_revert_transaction.go,processor_mirror.go, andwrite_set_counters.go.LedgerBoundaries(reserved 3..10+ names) — the vtprotobuf unmarshaller retains unknown-field bytes and re-emits them on MarshalVT, so the tags must never be reused.GetLedgerStatsreads posting / revert / numscript_execution / reference / ephemeral_evicted / transient_used / volume fromusagestore.GetCounter. Missing key = 0. Checkpoint-scoped (historical) reads short-circuit before touching the usagestore and return only the boundary-derivedTransactionCount+LogCount.MetadataCountis intentionally not returned. The admission preload no longer injects old metadata values, so the FSM-side counter can no longer distinguish "new key" from "overwrite". It is disabled until it can be rebuilt on a sound foundation (open question, no ticket yet).Test plan
just pre-commit— 0 issues (verified locally)GOROOT= go test ./internal/...— all pass locally (verified)GOROOT= go build ./...— clean (verified)usagestore/store_test.gocovers round-trip Get/Put + DeleteLedger cascade +Reset;usagebuilder/process_audit_test.gocovers batchState aggregation, applyDelta, mergeTemplateUsage, timestampGreater./v3/{ledger}/numscripts/{name}/usageand inGetLedgerStats.ledgerctl store rebuild-usage --data-dir <dir>drops and rebuilds the store.LedgerStatsvalues on a ledger with existing history — expect the migrated counters to be lower than pre-cutover values (that's the reset).Files
internal/storage/usagestore/,internal/application/usagebuilder/internal/adapter/http/handlers_get_numscript_usage.go,cmd/ledgerctl/store/rebuild_usage.gomisc/proto/common.proto(+TemplateUsage),misc/proto/bucket.proto(+RPC + request),misc/proto/raft_cmd.proto(LedgerBoundaries fields 3–10 deleted + reserved)internal/bootstrap/module.go,internal/application/ctrl/controller.go,internal/application/ctrl/controller_default.go,internal/adapter/grpc/{server,client}_bucket.go,internal/bootstrap/controller_routed.gointernal/domain/processing/processor_{transaction,revert_transaction,mirror}.go,internal/infra/state/write_set{,_counters,_new_volumes,_ephemeral_purge}.gointernal/application/check/checker.go(exclusion projection now foldsEphemeralVolumesalongsidePurgedVolumes).golangci.yaml,misc/devenv/monitoring-dashboards/jsonnet/lib/metrics.libsonnetopenapi.yml,docs/technical/contributing/api-comparison.md,docs/ops/cli.md,docs/technical/architecture/subsystems/usage/Follow-ups
MetadataCount— decide how to rebuild it on a sound foundation (the preload no longer carries old metadata values). No ticket yet.PebbleLastAuditSequenceobservability once accuracy on truncated-audit ledgers is understood.