fix(statesync): wire the never-connected state sync metrics into /status

[PastaPastaPasta]

Issue being fixed or feature implemented

The state-sync counters in /status sync_info (total_snapshots, chunk_process_avg_time, snapshot_height, snapshot_chunks_count, backfilled_blocks, backfill_blocks_total) have been zero since they were introduced: env.StateSyncMetricer was never assigned anywhere, *statesync.Reactor never actually satisfied the Metricer interface (two methods unimplemented, no compile-time assertion, so it silently compiled), and even wired, four of the values would have zeroed the moment a sync completed because syncComplete() drops the syncer they're read through. Found during Dash Platform state-sync QA when a freshly-restored node reported all zeros through DAPI.

What was done?

• *statesync.Reactor now fully implements Metricer: the two missing methods are implemented from the syncer's chunk queue, and var _ Metricer = (*Reactor)(nil) prevents silent drift.
• Final sync values (total snapshots, avg chunk time, snapshot height, chunk counts) are snapshotted into reactor fields before the syncer is dropped, so a node reports real progress during a sync and retains the final figures for the life of the process afterwards.
• The statesync reactor is kept and wired into the RPC env (node.rpcEnv.StateSyncMetricer), mirroring the existing ConsensusReactor wiring — the missing assignment that skipped the counters entirely.
• Two latent data races fixed (backfilledBlocks/backfillBlockTotal written without the mutex the readers take; chunk-timing/height made atomic), verified with go test -race.
• OpenAPI drift fixed: snapshot_chunks_total was documented but never existed in the Go struct.

Out of scope, documented in-code: persisting the completed-sync record across restart (needs a state-store change — maintainer design decision; after a reboot the counters are zero again, and the sync never re-runs by design).

How Has This Been Tested?

New unit tests for the full Metricer lifecycle, post-sync assertions added to the real end-to-end TestReactor_Sync, and a new /status test proving the counters are copied into sync_info when wired. go test -race ./internal/statesync/... ./internal/rpc/... ./node/... -short all pass; full non-short statesync suite passes.

Companion: dashpay/platform#4532 already gates DAPI's stateSync status section on these counters being non-zero — once this ships, DAPI status lights up with no further changes.

Breaking Changes

None.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation

---

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 62423536-c78e-4c06-8733-058b78cb0339

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

⛔ Final review complete — 1 blocking finding(s) (commit 9e1fba7)

[thepastaclaw]

Final validation — GLM Flash + Sol

The reactor/RPC wiring and completed-sync retention are otherwise sound, but total_snapshots is still sourced from a mutable pool and can be snapshotted as zero after a successful sync, defeating the PR's core status fix. A node-level assertion should also protect the newly added wiring assignment.
Source: reviewer backends glm-5.3-flash and gpt-5.6-sol; final verifier backend gpt-5.6-sol.

Review provenance

• Phase 1 reviewers (GLM Flash): glm-5.3-flash — general (completed), glm-5.3-flash — tenderdash-consensus-security (completed)
• Fresh verifier (Sol): gpt-5.6-sol — final-verifier
• Phase 2 reviewers (Sol): gpt-5.6-sol — general (completed), gpt-5.6-sol — tenderdash-consensus-security (completed)

🔴 1 blocking | 🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `internal/statesync/syncer.go`:
- [BLOCKING] internal/statesync/syncer.go:525-526: TotalSnapshots reports the current pool size instead of the total discovered
  The documented value and the corresponding Prometheus counter represent snapshots discovered over the syncer's lifetime, but this getter returns `snapshotPool.Len()`, which counts only snapshots currently retained. `Reject`, `RejectVersion`, `RejectPeer`, and `RemovePeer` all delete pool entries. Because the syncer remains installed through backfill, a peer disconnect after its snapshot has been restored can remove that snapshot before `syncComplete` captures the final metrics. `/status` then retains `total_snapshots: 0` despite a successful state sync, reproducing the condition this PR is intended to fix and potentially keeping DAPI's state-sync section disabled. Store a cumulative accepted-snapshot count in the syncer, increment it whenever `AddSnapshot` returns `added == true`, and return that synchronized or atomic count here instead of the mutable pool length.

In `node/node.go`:
- [SUGGESTION] node/node.go:427-428: Add a regression test for the node-to-RPC metricer wiring
  The new status test injects a fake `Metricer`, and the reactor tests call the concrete getters directly; neither test constructs a node and verifies that `RPCEnvironment().StateSyncMetricer` receives the state-sync reactor. Removing this assignment would therefore leave all new tests passing while restoring the original all-zero `/status` behavior. Add a non-nil assertion to an existing node-construction test such as `TestNodeStartStop` to protect the exact integration boundary fixed by this PR.

[thepastaclaw]

🔴 Blocking: TotalSnapshots reports the current pool size instead of the total discovered

The documented value and the corresponding Prometheus counter represent snapshots discovered over the syncer's lifetime, but this getter returns snapshotPool.Len(), which counts only snapshots currently retained. Reject, RejectVersion, RejectPeer, and RemovePeer all delete pool entries. Because the syncer remains installed through backfill, a peer disconnect after its snapshot has been restored can remove that snapshot before syncComplete captures the final metrics. /status then retains total_snapshots: 0 despite a successful state sync, reproducing the condition this PR is intended to fix and potentially keeping DAPI's state-sync section disabled. Store a cumulative accepted-snapshot count in the syncer, increment it whenever AddSnapshot returns added == true, and return that synchronized or atomic count here instead of the mutable pool length.

_{source: ['claude']}

[thepastaclaw]

🟡 Suggestion: Add a regression test for the node-to-RPC metricer wiring

The new status test injects a fake Metricer, and the reactor tests call the concrete getters directly; neither test constructs a node and verifies that RPCEnvironment().StateSyncMetricer receives the state-sync reactor. Removing this assignment would therefore leave all new tests passing while restoring the original all-zero /status behavior. Add a non-nil assertion to an existing node-construction test such as TestNodeStartStop to protect the exact integration boundary fixed by this PR.

_{source: ['codex']}

[PastaPastaPasta]

Both findings verified valid and fixed (pushed through 93700762):

• TotalSnapshots pool-mutation race — confirmed and fixed: the getter returned snapshotPool.Len(), and since syncComplete is deferred until after backfill, a peer disconnect post-restore could shrink the pool to zero before the final metrics were captured — reproducing exactly the all-zero /status this PR fixes. Now backed by an atomic cumulative counter incremented when AddSnapshot accepts a new snapshot; the metrics test pins that duplicates don't bump it and that the value survives emptying the pool via RemovePeer followed by syncComplete.
• Wiring regression test — added: TestNodeStartStop now asserts RPCEnvironment().StateSyncMetricer is non-nil after construction, so removing the one-line wiring can no longer pass the suite.

go build + go test -race -short green on node, statesync, and rpc/core.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[lklimek]

to check: ensure there is no deadlock possible.

[Claudius-Maginificent]

Grumpy review pass — 10 actionable findings posted inline (full report: severity/merge-class rationale for all 19 findings, including deferred ones, is in the consolidated report). One pre-existing issue this PR newly exposes (SEC-002, chunk-queue teardown deadlock) is filed as a separate GitHub issue rather than commented here, since fixing it exceeds this PR's stated scope.

_{🤖 Reviewed by Claudius the Magnificent (security-engineer-smythe, project-reviewer-adams, qa-engineer-marvin)}

[Claudius-Maginificent]

[SEC-001 · MEDIUM, non-blocking] This assignment is the one line that turns the /status state-sync block from dead code into a live one — before this PR env.StateSyncMetricer was never set, so the branch in internal/rpc/core/status.go:91-97 never ran.

With it live, /status now walks Reactor.mtx → syncer.mtx → chunkQueue.mtx, the innermost held across chunk-file disk I/O (chunkQueue.Add/Next/Close in chunks.go do os.WriteFile/os.ReadFile/os.RemoveAll under q.mtx). An RPC call can therefore block on chunk-file I/O while holding a reactor read lock that a pending backfill writer (r.mtx, taken per block) then queues behind — back-pressuring backfill through an unauthenticated status call.

reactor.go already has the right pattern one call away: r.getSyncer() copies the pointer under lock and releases before doing anything with it. The metric getters (SnapshotChunksCount, SnapshotHeight, etc.) descend through the nested locks directly instead. Recommend routing them through getSyncer(), or better, publishing the counts into the syncer's existing atomics so /status never has to touch chunkQueue.mtx at all.

[Claudius-Maginificent]

[SEC-004 · LOW, non-blocking] avgChunkTime, lastSyncedSnapshotHeight and totalSnapshots are bare int64 fields accessed via atomic.LoadInt64/StoreInt64, at offsets 14-16 of syncer. Go only guarantees 64-bit alignment for the first word of an allocated struct on 32-bit platforms. .goreleaser.yml ships a goarch: arm (32-bit) release target while the arm entry in build.yml's CI matrix is commented out, so a misalignment wouldn't be caught by CI.

Measured, not assumed: a layout replica compiled for GOARCH=arm places these fields at offset 104 — aligned today, but only by accident. Inserting one pointer-sized field ahead of this group in a future change silently turns these loads into a panic loop on 32-bit ARM.

go.mod declares go 1.26.6; atomic.Int64 (available since Go 1.19) is the idiomatic fix — it carries its own alignment guarantee structurally instead of by convention, and replaces the doc comment below with a compiler-enforced contract.

[Claudius-Maginificent]

[GO-005 · LOW, non-blocking] This comment declares a new invariant ("access them atomically"), but existing in-repo readers already violate it: internal/statesync/syncer_test.go:279-280 and internal/statesync/reactor_test.go:759-776 read avgChunkTime/lastSyncedSnapshotHeight/backfillBlockTotal/backfilledBlocks as plain fields rather than through the accessors this PR added.

Convert those six raw reads to suite.syncer.LastSyncedSnapshotHeight(), suite.syncer.AvgChunkTime(), rts.reactor.BackFillBlocksTotal(), rts.reactor.BackFilledBlocks(). Where a test just compares the field to its own getter, drop the assertion — it tests the compiler, not the code.

[Claudius-Maginificent]

[CALL-001 · MEDIUM, non-blocking] When SyncAny abandons a snapshot (rejection, chunk timeout — the normal failure mode on a live network, not an exotic path) after releaseChunkQueue has already stamped lastChunksCount/lastChunksTotal for that attempt, nothing clears them. /status then permanently reports a nonzero snapshot_chunks_count next to snapshot_height: 0 for a snapshot that was rejected — a self-contradictory pair for as long as the process runs.

Reset lastChunksCount/lastChunksTotal in SyncAny's abandonment cleanup, alongside snapshot = nil; queue = nil; s.processingSnapshot = nil, so the retained counts only ever describe a completed restore. (If the intent is instead to show progress on failed attempts, snapshot_height needs to be populated too, so the two fields stop contradicting each other.)

[Claudius-Maginificent]

[GO-001 · MEDIUM, non-blocking] This is tier two of a two-tier "remember the last value before dropping the owner" cache: syncer.releaseChunkQueue() copies chunk-queue counts into syncer.lastChunksCount/lastChunksTotal; syncComplete then copies the syncer's numbers again into these five Reactor fields. Same idea, implemented twice, one layer apart, for five values each.

Collapse to a single value type, e.g. type syncMetrics struct { TotalSnapshots int64; ChunkAvgTime time.Duration; SnapshotHeight int64; ChunksCount, ChunksTotal int64 }, with one func (s *syncer) metricsSnapshot() syncMetrics. Reactor keeps a single last syncMetrics, syncComplete does r.last = r.syncer.metricsSnapshot(), and every getter becomes a one-line field read. Removes the syncer-level cache tier entirely and roughly halves this code.

[Claudius-Maginificent]

[GO-006 · LOW, non-blocking] TotalSnapshots() is the only one of five sibling Metricer getters carrying a second guard clause (r.syncer.snapshots != nil). It was correct before commit ffdcd7e94, when the body read len(r.syncer.snapshots.snapshots) directly; now that it just calls r.syncer.TotalSnapshots(), the snapshots != nil half of the guard checks a field this branch no longer touches. Drop && r.syncer.snapshots != nil so all five getters share the same shape.

[Claudius-Maginificent]

[GO-004 · LOW, non-blocking] snapshotPool.Len() was added by commit 9b0beb0e0 for an approach (TotalSnapshots() reading a locked pool.Len()) that commit ffdcd7e94 superseded with a cumulative atomic counter, because pool size shrinks on rejection. Len() now has zero production callers — only reactor_metrics_test.go:85 calls it, to assert emptiness.

Delete Len() and assert emptiness through an existing accessor (p.Ranked()/p.Best()) instead — or keep it and actually use it somewhere, but not neither.

[Claudius-Maginificent]

[PROJ-001 · MEDIUM, non-blocking] This hunk deletes snapshot_chunks_total because coretypes.SyncInfo never had that field — correct. But the PR also adds a complete five-layer implementation for it in the same diff: chunkQueue.TotalChunksCount(), syncer.lastChunksTotal/SnapshotChunksTotal(), Reactor.lastSnapshotChunksTotal/SnapshotChunksTotal() (~45 lines across three files), which nothing now consumes. The PR's own test comment says as much (status_test.go: "SnapshotChunksTotal only satisfies the interface: SyncInfo has no snapshot_chunks_total field, so /status never reads it").

Pick one ending: (preferred) add snapshot_chunks_total back to SyncInfo/openapi and wire it in status.go — the plumbing is already built and tested; or remove SnapshotChunksTotal() from the Metricer interface and delete the now-pointless implementation.

[Claudius-Maginificent]

[PROJ-004 · MEDIUM, non-blocking] Every state-sync property in this SyncInfo block is type: string plus an example, with no description — and no unit is guessable from the schema. Until this PR these fields were always zero so nobody noticed; they carry real values now. In particular chunk_process_avg_time is a time.Duration marshalled as a nanoseconds-as-string integer ("1000000000"), which reads like a Unix timestamp without a comment saying otherwise.

Add a one-line description: to each state-sync property here, naming the unit for the duration fields ("average chunk processing time, in nanoseconds") and the subject for snapshot_height ("height of the last successfully restored snapshot").

[Claudius-Maginificent]

[PROJ-007 · LOW, non-blocking] STYLE_GUIDE.md § Testing says to use Testify mock with Mockery for autogeneration, and .mockery.yaml already registers two internal/statesync interfaces into internal/statesync/mocks/. Metricer isn't registered there, so this test hand-rolls a fakeStateSyncMetricer stub instead.

Add Metricer: {} to the internal/statesync block in .mockery.yaml, regenerate, and use mocks.NewMetricer(t) with .On(...).Return(...) — or keep the hand-rolled stub and note in a comment why the generated mock wasn't used, so the next reader doesn't wonder.