feat(storage): run the ledger drift checks in the background - #2323
atharrva01 wants to merge 1 commit into
Conversation
|
Three issues before this is ready to merge: 1. CI failure —
2. Reintroduces the per-TMS lock-ID bug that PR #2085 just fixed
3. Interface conflict with PR #2085
|
1c9492c to
0647194
Compare
|
Pushed a rebase onto main plus two fix commits addressing your review: 1. CI failure (blocking): fixed in 0647194. 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 3. Interface conflict with #2085: still open, and I don't think it's fixable correctly from this side yet. |
|
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 |
|
hi @AkramBitar , CI passes now and the reviews are also addressed across this and all my other pr's Thanks :) |
AkramBitar
left a comment
AkramBitar
left a comment
There was a problem hiding this comment.
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:
db/common/checks.go:430— a ledger that can't answerStatuscauses the next sweep to close previously recorded critical findings.sql/common/findings.go:74— a sweep with >~3000 findings exceeds the bind-parameter ceiling and persists nothing, repeatedly.services/checks/config.go:70— anychecks:block without an explicitenabledsilently disables the sweep, contradicting its own godoc and the docs.db/common/checks.go:901— a transient local-DB error is reported as a criticaltoken_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.)
22e27da to
1ab94db
Compare
|
Fixed the auditor leader-election gap too, in 1ab94db. Postgres's All eight inline findings fixed too, replied on each thread with the specifics. Full |
8b4fcc2 to
a10f99d
Compare
926a70b to
f004847
Compare
f004847 to
2c51942
Compare
|
@Effi-S @AkramBitar , a gentle ping on this |
|
@Effi-S , any changes here? |
|
@Effi-S , ready for review |
|
Any update with this PR? Regards, |
|
@atharrva01, |
30c3de5 to
9cb9488
Compare
|
hi @Effi-S , i have done the changes , you can take a look now |
|
@atharrva01, A few more medium and minor changes |
9cb9488 to
e9e17b4
Compare
|
hi @Effi-S, pushed fixes for all four:
Squashed back to one commit, rebased onto main. |
|
@Effi-S PTAL |
Effi-S
left a comment
Effi-S
left a comment
There was a problem hiding this comment.
@atharrva01,
Thank you for the work here. 🙏
There are some more finding here..
08f7e02 to
4b1223e
Compare
|
Hello @atharrva01, Any updates with this PR? Regards, |
Adds a background sweep that periodically compares locally stored token transactions and unspent tokens against the ledger, recording findings for anything that drifted (a token missing on the ledger, a transaction status that disagrees, content mismatches, etc). One replica per store runs the sweep at a time via a PostgreSQL advisory lock, findings are aged and auto-resolved once a problem stops being observed, and the whole thing is a safety net that never fails the owning service. Includes the fixes from all review rounds: - clamp the default timeout to scanInterval (and now any explicit timeout that exceeds it too), so a misconfiguration cannot fail owner/auditor service startup - use FSC errors.Join instead of stdlib - stop dropping info-severity findings on the legacy on-demand check API, and only elevate tx_status_unavailable to Warning for confirmed records so it still surfaces through the on-demand path without reintroducing the transient-lookup flakiness the info-severity filter was added for - classic Fabric's token query now reports a missing token as a nil entry instead of erroring the whole call, matching fabricx, so checkUnspentOneByOne can tell a missing token apart from a read failure; GetTokenView and the interactive certifier backend both reject a nil entry explicitly since certification needs the token to actually exist, and the chaincode handler's own contract is documented rather than left implicit - order transaction queries by (stored_at, tx_id) so the transaction walk dedupes a multi-movement transaction in O(1) memory instead of a set that grows with total ledger history; documented on the interface, since it's shared by every QueryTransactions caller, not just the checks walk - reset the FindingsOpen gauge when leadership is not acquired or a leader sweep itself fails, so a replica that is not currently reporting current findings does not keep exporting a stale value - FindingsOpen is now read back from storage (a GROUP BY count, not a row fetch) instead of derived from one sweep's own findings, so an inconclusive checker during an outage cannot make an older, still- open critical read as zero - a failed sweeper.start is logged instead of failing CheckService, since the sweep is a safety net on top of the on-demand checks, not a precondition for them - create the sweep ticker after the initial sweep completes so a slow first sweep cannot cause back-to-back sweeps - document QueryTokens' positional/nil-for-absent contract on the interface itself, since checkUnspentBatch already relied on it - give a sweep that Stop() interrupts its own "canceled" outcome on the SweepsTotal metric instead of counting it as failed - cap how many critical findings report() logs individually per sweep, with a one-line "N more omitted" instead of flooding the log - surface a checker that overran its timeout: FindingsService.Check swallows a per-checker DeadlineExceeded into a finding rather than an error, so the timeout warning now checks the sweep's own context directly instead of trusting the Checker's error return - call out in the docs and in Config.Enabled that the sweep runs by default, so upgrading to a version carrying this service starts it on existing stores with no config change - a token spent between CheckUnspentTokens' snapshot and checkUnspentBatch's local read (routine on a busy node) no longer fails the whole batch: getLedgerToken wraps ErrTokenNotFound, and the batch resolves one id at a time on that error, skipping ids spent since the snapshot instead of reporting check_failed over them - sweeper.start now tracks a manager in s.managers under the same lock section as Start() itself, instead of after it returns, so a Stop() racing with startup can never miss a manager that did start and leak its goroutine (and, on postgres, its advisory-lock connection) - fixed FindingsOpen's doc comment and Help string, stale since it started being read back from storage; regenerated the metrics golden file - manager_test.go now uses the FSC errors wrapper instead of stdlib Signed-off-by: atharrva01 <atharvaborade568@gmail.com>
4b1223e to
ccc41e7
Compare
Effi-S
left a comment
Effi-S
left a comment
There was a problem hiding this comment.
@AkramBitar, Please take a look
|
hi @Effi-S @AkramBitar if this PR looks good now , can we move ahead with merge on this ? |
This branch has not been deployed
3 participants
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:
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)golangci-lint runclean on everything touched