feat(storage): run the ledger drift checks in the background

[atharrva01]

Fixes #2166

The drift checks (CheckTransactions, CheckUnspentTokens, CheckTokenSpendability) already existed but nothing on a running node ever called them, only integration test views did. This adds a background sweep, modeled on recovery and cleanup: runs on an interval, leader election through a PostgreSQL advisory lock, one sweep per store a node owns (owner over ttxdb, auditor over auditdb).

What changed:

• Findings are now structured (checker, code, severity, tx/token id) instead of plain strings, persisted in a findings table keyed by a stable finding key. A repeated problem ages instead of getting re-reported, and closes once a sweep stops seeing it. A check that fails never closes its own findings, so an unreachable ledger can't look like a clean bill of health.
• CheckUnspentTokens now resolves tokens against the ledger in batches instead of one call for everything followed by a linear scan per token.
• New CheckLocalCompleteness check covers the direction the others can't: tokens the ledger says the node owns that never landed locally. That's the case that actually costs the owner money, so it runs in the background sweep.
• The on-demand Check API on auditor/owner is unchanged in shape (still plain messages) but now backed by the same finding checkers, prefixed with severity and code.

Docs at docs/services/storage/checks.md, wired into docs/services/storage.md and docs/configuration.md.

Test plan

• go test ./token/services/storage/... ./token/sdk/... (postgres, sqlite, race where applicable)
• New unit tests for the checks manager/scheduler (leader election, sweep lifecycle, resolvable-checker semantics)
• golangci-lint run clean on everything touched
• Manually verified the findings upsert/aging against a real Postgres container

[AkramBitar]

Three issues before this is ready to merge:

1. CI failure — TestTMSScopedProviderWiringIsIntact (blocking)

token/sdk/db/checks.go adds a new call to metrics.NewTMSProvider but the tokenDrivers list in token/services/metricsdoc/reference_test.go was not updated. Until it is, every metric this PR introduces is exported under the wrong name (panurus_core_common_metrics_ prefix instead of its own package). The test error message says exactly what to do: verify the new metrics against docs/development/metrics.md, then add token/sdk/db/checks.go to tokenDrivers.

2. Reintroduces the per-TMS lock-ID bug that PR #2085 just fixed

checks/config.go has a static defaultLockID = 0x74746b636865636b and an operator-configurable AdvisoryLockID. Two TMSes sharing a persistence configuration both derive the same constant, so only one wins the advisory lock per tick and the other silently skips its sweep forever — the exact same bug #2085 fixed for recovery and cleanup. The fix is the same: derive the lock ID from the fully-qualified table name at construction time and drop AdvisoryLockID from Config.

3. Interface conflict with PR #2085

checks.Storage declares AcquireRecoveryLeadership(ctx context.Context, lockID int64) — the signature #2085 is removing. The two PRs cannot both merge without a compile error. They need to be coordinated: either this PR adopts the no-parameter signature from #2085, or they merge in a defined order with the interface aligned before the second one lands.

[AkramBitar]

@atharrva01

Thanks a lot for this PR.

See my comments.

Regards,
Akram

[atharrva01]

Pushed a rebase onto main plus two fix commits addressing your review:

1. CI failure (blocking): fixed in 0647194. token/sdk/db/checks.go's NewTMSProvider call is now in tokenDrivers, plus a new "Checks: ledger drift" group in reference_test.go, the regenerated golden file, and the five panurus_core_common_metrics_storage_checks_* names documented in docs/development/metrics.md (with a pointer from checks.md's own table, which only had the bare names). Turned out tokenDrivers assumed every TMS-scoped call site used the identical literal expression the two driver.go files share, which doesn't hold for checks.go's differently-named locals, so it's a small struct per file now instead of a bare string list.

2. Per-TMS lock ID bug: fixed in e2afa1e, same shape as #2085 — derives the lock id from network/channel/namespace/role at manager construction instead of a node-wide constant, drops AdvisoryLockID from Config. Added TestManager_LockIDDistinctPerTMS covering both the per-TMS and per-role cases (owner vs auditor sweeps over the same TMS).

3. Interface conflict with #2085: still open, and I don't think it's fixable correctly from this side yet. checks.Storage.AcquireRecoveryLeadership(ctx, lockID) and ttxdb/auditdb's AcquireRecoveryLeadership(ctx, lockID) are the literal same method, so once #2085 drops the lockID param there, checks can't keep calling it with an arbitrary id at all — it needs its own leadership-acquisition path (mirroring what #2085 did for recovery/cleanup), which only makes sense to design against #2085's actual merged shape rather than guess at it now. Agree with your suggested sequencing: once #2085 merges I'll rebase this branch and align the interface then, rather than block on it now.

[atharrva01]

Found and fixed the CI failure, pushed 9966417.

Root cause: `AsNamedChecker` (the downgrade from structured findings to the legacy plain-message Checker API) was including every finding regardless of severity. `SeverityInfo` is documented as "expected to resolve on its own" (a transaction the ledger has not caught up with yet, for example) and that's exactly what `CodeTxStatusUnavailable` reports when a status lookup fails right after a node restart. Since the plain-message contract has no way to carry severity, that info-level finding showed up as a plain "error" string to every legacy caller, including `CheckOwnerStore`'s "expect zero errors" assertion in the integration suite, which is why it failed across nearly the whole itest matrix rather than one flaky spec.

Fixed by dropping Info findings at that one downgrade point, so the structured findings table (and the background sweep) still see them, only the lossy plain-message path filters them out. Added unit tests for AsNamedChecker, which had none before.

[atharrva01]

hi @AkramBitar , CI passes now and the reviews are also addressed across this and all my other pr's Thanks :)

[AkramBitar]

Review: background ledger drift checks

Solid, well-tested feature — build, go vet, and all new tests pass at 3de2ff56c. I verified the generated upsert SQL against both backends (occurrences is correctly table-qualified, first_seen correctly excluded from the conflict update, and the Lt("last_seen", seenAt) predicate correctly avoids resolving rows the same sweep just wrote), and confirmed positional comparison in checkUnspentBatch is safe since both backends preserve request order.

That said, I found four defects that break the guarantee the service exists to provide — a checker that reports "clean" when it isn't, or records nothing at all. These are inline and marked Blocker:

1. db/common/checks.go:430 — a ledger that can't answer Status causes the next sweep to close previously recorded critical findings.
2. sql/common/findings.go:74 — a sweep with >~3000 findings exceeds the bind-parameter ceiling and persists nothing, repeatedly.
3. services/checks/config.go:70 — any checks: block without an explicit enabled silently disables the sweep, contradicting its own godoc and the docs.
4. db/common/checks.go:901 — a transient local-DB error is reported as a critical token_missing_locally.

Plus four more inline (one-by-one fallback unreachable, timeout covering the writes, nil metrics-provider panic, Stop() never wired) and one minor.

One finding with no line to anchor to

The auditor sweep is not leader-elected on Postgres. sqlcommon.NewAuditTransactionStore delegates to NewOwnerTransactionStore, which passes recoveryLeaderFactory = nil; AcquireRecoveryLeadership (sql/common/transactions.go:388) then returns noopRecoveryLeadership{}, true unconditionally. Both services/checks/manager.go:74 and docs/services/storage/checks.md:117 promise "only one replica sweeps a given store at a time, decided by a PostgreSQL advisory lock".

With N auditor replicas on one database, all N run the full sweep every interval — multiplying ledger traffic — and the slower replica's ResolveFindingsNotSeenSince can close findings the faster one just recorded. Either wire the advisory-lock factory into the audit store, or stop claiming election for that role. (The mechanism lives in code this PR doesn't touch, hence no inline anchor.)

[AkramBitar]

Blocker: a ledger that cannot answer Status closes previously recorded critical findings.

This branch emits CodeTxStatusUnavailable at SeverityInfo — not CodeCheckFailed. But resolvableCheckers (services/checks/manager.go:394) only suppresses resolution for checkers that reported CodeCheckFailed, so CheckerTransactions stays resolvable.

Concretely:

1. Sweep 1 records Transaction Check|tx_status_mismatch|T| as critical.
2. Sweep 2 hits a chaincode/query error for T, emits this Info finding.
3. ResolveFindingsNotSeenSince closes the critical row, because its last_seen predates seenAt.

A ledger outage is turned into a clean bill of health — the exact failure mode this comment, resolvableCheckers' own godoc, and docs/services/storage/checks.md:85 all say the design prevents.

Either give the unavailable case CodeCheckFailed, or exclude a checker from resolution when it reported any inconclusive code.

[atharrva01]

Fixed in 1ab94db. resolvableCheckers now checks dbcommon.IsInconclusive(code) instead of just == CodeCheckFailed, so a checker reporting tx_status_unavailable stays excluded from resolution too.

[AkramBitar]

Blocker: one statement for every finding, so a large sweep records nothing.

11 bind parameters per row against SQLite's 32766-variable ceiling (~2977 rows) and Postgres's 65535 (~5957 rows). Verified on SQLite: n=1000 succeeds, n=3000 fails with too many SQL variables.

The error aborts sweep() before ResolveFindingsNotSeenSince, so the sweep fails and re-fails every interval having persisted nothing.

This is reachable, not theoretical: CheckTokenSpendability emits one critical finding per unspent token when a public-parameters upgrade makes the stored format unsupported, and CheckLocalCompleteness one per missing token. A node with >3000 unspent tokens loses the entire report exactly when it matters most.

Suggest chunking into BatchSize-sized batches that all share one seenAt.

[atharrva01]

Fixed in 1ab94db. UpsertFindings now chunks at 1000 rows per statement, all sharing the sweep's one seenAt.

[AkramBitar]

Blocker: any checks: block without an explicit enabled silently disables the sweep.

result.Enabled = loaded.Enabled is unconditional, so a config that only tunes e.g. scanInterval: 2h yields Enabled: false.

That contradicts three things at once: LoadConfig's own godoc four lines above ("falling back to DefaultConfig for anything it does not set"), the Config.Enabled field comment ("Default true"), and docs/configuration.md:334 ("Default: true").

The result is a node whose operator believes drift checking is on while nothing runs — the state this service exists to end. TestLoadConfig_EnabledDefaultsFalseWhenKeyPresentButUnset documents the behaviour deliberately, but a *bool (or cfg.IsSet(ConfigKeyChecks+".enabled")) removes the trap. Note the same fix would be needed in recovery/cleanup for consistency.

[atharrva01]

Fixed in 1ab94db. LoadConfig now gates Enabled on cfg.IsSet(ConfigKeyChecks + ".enabled"), same pattern already used for NotFoundGracePeriod in recovery. Left recovery/cleanup alone for this PR since they're actively touched by other open PRs right now, happy to open a follow-up for those.

[AkramBitar]

Blocker: a local store query failure is reported as missing money.

checkPresenceBatch treats any error from qe.WhoDeletedTokens as "token absent". The SQL implementation (sql/common/tokens.go:1108) returns token not found for a genuine miss but also propagates raw driver errors (connection reset, statement timeout) identically.

So a transient local DB failure puts every id in the batch through reportMissingLocally, and since the ledger still holds them unspent, each becomes a SeverityCritical token_missing_locally finding logged at error level — "the node owns money it cannot see".

This is the precise conflation that checkUnspentBatch:557 and checkUnspentOneByOne:593 go out of their way to avoid. Distinguish not-found from a query error (a sentinel, or a presence query returning a bool).

[atharrva01]

Fixed in 1ab94db. Added driver.ErrTokenNotFound, WhoDeletedTokens wraps it for a genuine miss, checkPresenceBatch now only treats errors.Is(err, ErrTokenNotFound) as absence and propagates anything else as a real failure.

[AkramBitar]

checkUnspentOneByOne is unreachable, and token_missing_on_ledger is never produced.

Its godoc says "a failed batch is resolved one token at a time", but the error branch two lines up does return nil, err; the fallback is gated only on this length mismatch — which neither backend produces:

• the Fabric translator (translator.go:167) returns an error when any key is absent;
• fabricx (qe.go:145) returns a nil entry at that position, preserving length.

So on Fabric a single absent token fails the whole Unspent Tokens Check (check_failed), losing every other token's comparison; on fabricx it surfaces as token_content_mismatch rather than token_missing_on_ledger.

Worse: a token spent concurrently while the sweep walks its local snapshot triggers this on a perfectly healthy busy node — a routine false check_failed on Fabric, a false critical on fabricx. The fallback should be driven by the error, not by the length.

[atharrva01]

Fixed in 1ab94db. checkUnspentBatch now falls back to checkUnspentOneByOne on the batch error itself, not the length check, and a nil entry in a same-length successful response is reported as token_missing_on_ledger instead of going through the content comparison.

[AkramBitar]

The sweep timeout also covers the writes, so the longest sweeps record nothing.

Check, UpsertFindings and ResolveFindingsNotSeenSince all share one Timeout-bounded context. A sweep whose checks consume nearly the full 30 minutes reaches UpsertFindings with almost no budget; when the deadline hits, the findings are discarded and the sweep is counted failed.

The nodes with the most drift and the longest histories are exactly the ones that will never persist a finding. Suggest bounding only checker.Check with Timeout, giving the two writes their own short deadline off the parent context.

[atharrva01]

Fixed in 1ab94db. checker.Check is still bounded by Config.Timeout, but UpsertFindings/ResolveFindingsNotSeenSince now run under their own 2m deadline off the parent context instead of sharing it.

[AkramBitar]

The documented nil-provider guard is defeated by the wrapper.

The comment states "A nil provider discards everything, so a caller with no metrics configured needs no special case" — but metrics.NewTMSProvider(tmsID, nil) returns a non-nil *tmsProvider holding a nil provider, so the guard never fires and p.provider.NewCounter nil-derefs.

Reachable for the hand-wired setups this file explicitly contemplates: NewOwnerCheckServiceProvider(..., configuration, nil) with a non-nil configuration panics inside CheckService, i.e. during ttx/auditor service construction. Guard s.metricsProvider == nil before wrapping.

[atharrva01]

Fixed in 1ab94db, at the root: NewTMSProvider now returns the Provider interface and returns a true nil when given a nil provider, instead of *tmsProvider (which wrapped nil into a non-nil interface value). That covers this call site and the two driver.go ones without needing a nil check at every caller.

[AkramBitar]

Stop() on the check service providers is never wired into the SDK lifecycle.

sweeper.Stop, AuditorCheckServiceProvider.Stop and OwnerCheckServiceProvider.Stop all exist, but nothing invokes them and SDK.Start registers no corresponding shutdown.

Each running sweep therefore keeps its goroutine — and on Postgres its dedicated advisory-lock connection — past SDK shutdown. An in-process SDK restart leaves the old sweep holding the lock and the new one permanently reporting not_leader.

[atharrva01]

Fixed in 1ab94db. Added SDK.Stop(), which stops both check service providers (via the dig container) before delegating to the underlying SDK's Stop.

[AkramBitar]

Minor: this doc comment is detached from the interface.

The block ends with a bare // followed by a blank line, so exported FindingsStore has no godoc at all. AGENTS.md requires one, and revive's exported rule isn't enabled so lint won't catch it.

Two other small things while here:

• validateConfig doesn't reject Timeout > ScanInterval, which yields back-to-back sweeping.
• mockTransactionsStore in sql/{sqlite,postgres}/transactions_test.go still builds TableNames without Findings — harmless only because those tests never call GetSchema, which would now emit CREATE TABLE IF NOT EXISTS (.

[atharrva01]

Fixed in 1ab94db: the orphaned doc comment is reattached, validateConfig now rejects Timeout > ScanInterval, and both mockTransactionsStore helpers include the Findings table name.

[atharrva01]

Fixed the auditor leader-election gap too, in 1ab94db. Postgres's NewAuditTransactionStore was building its store through sqlcommon.NewAuditTransactionStore -> NewOwnerTransactionStore, which hardcodes recoveryLeaderFactory: nil, so AcquireRecoveryLeadership always fell back to the no-op leadership. It now goes through NewTransactionStoreWithNotifierAndRecovery directly with NewAdvisoryLockFactory(), the same path the owner store already used. Added TestAuditRecoveryIntegration mirroring the existing owner-side TestRecoveryIntegration, confirms a second replica no longer acquires the lock while the first holds it.

All eight inline findings fixed too, replied on each thread with the specifics. Full make checks + make lint + go test ./token/... clean.

[atharrva01]

@AkramBitar would love your thoughts on this too