fix: reset fetch_notes cursor stranded above the seq high-water

[WiktorStarczewski]

Problem

A wallet whose stored transport cursor is higher than the server's current max seq fetches zero notes forever. Every fetch_notes(seq > cursor) matches nothing, and the handler echoes rcursor = max(cursor, max_seq_returned) — so the too-high cursor is returned verbatim and never decreases. Notes correctly addressed to that wallet sit on the server, undeliverable, with no self-healing on either side.

This was found in the wild: a testnet wallet holding cursor 3487 against a server whose max seq is 812. FetchNotes(tags, cursor=0) returns the notes; cursor=3487 returns nothing. Full root-cause writeup: the mechanism is verified against origin/main (client persists only server-echoed cursors; LEGACY_CURSOR_THRESHOLD = 1e12 doesn't catch a small value like 3487).

How a cursor ends up above the max seq

The client only ever persists a server-echoed rcursor (= max(seen seqs)), so a stored cursor above the current max seq can only mean the server's seq space regressed: the backing DB was recreated (volume reset / restore-from-empty / endpoint swap) and AUTOINCREMENT restarted low, while the client still holds a cursor from the previous, larger epoch. The add_seq_cursor migration itself backfills seq in created_at order and its comment assumes a single deployment lifetime — this is the blind spot of that assumption.

Fix

Detect a stranded cursor — one at/below the legacy threshold but strictly above the current seq high-water — and reset it to 0 so the client re-scans the current epoch. sqlite_sequence.seq (the AUTOINCREMENT high-water) is the right signal: it only decreases across a DB recreation, never within a lifetime (survives DELETE/VACUUM/cleanup_old_notes). A legitimately caught-up client sits at cursor == high_water and is untouched — so no false positives.

Crucially, the reset must also heal the echoed cursor: fetch_notes_by_tags now returns the effective cursor it used, and the gRPC handler bases rcursor on that (not the client's claimed cursor). Without this, the handler would keep echoing the stranded 3487 and the client would re-download the whole epoch on every poll and never converge. Basing the echo on the effective cursor lets a stranded client heal in 1–2 polls and paginate normally.

Bonus: this same echo change fixes the pre-existing legacy-µs-cursor path, which had the identical "re-download every poll, never heal" behavior.

Both the pull path (fetch_notes) and the push path (streaming.rs) are covered.

Changes

• sqlite/mod.rs: high_water_seq() helper (reads sqlite_sequence, fail-safe None on any error → never falsely resets); stranded-cursor detection in fetch_notes_by_tags, run in the same snapshot as the query; returns (notes, effective_cursor).
• grpc/mod.rs: base the response cursor on the effective cursor.
• streaming.rs: advance the subscription cursor from the effective cursor.
• metrics.rs: db_fetch_notes_stranded_cursor_reset_count counter (mirrors the legacy-reset counter) so operators can see it fire.
• database/mod.rs: trait/wrapper signature update; new test test_fetch_notes_resets_cursor_stranded_above_high_water; updated the legacy-reset test's sanity check (see below).

Behavior change (intentional)

A fetch_notes cursor strictly above the current high-water is now reset to 0 instead of returning empty. The existing test_fetch_notes_resets_legacy_cursor had a sanity check asserting that cursor=1000 against a one-note DB (seq 1) returns empty; that scenario is exactly a stranded cursor, so its assertion was updated to use a caught-up cursor (== high_water), which is the genuine "no reset" case.

Assumption

The reset assumes a single shared seq space (single writer / shared volume). A sharded deployment with independent per-instance seq spaces behind a naive load balancer would thrash — but such a deployment is already incompatible with cursor semantics, and the analysis confirmed a single logical seq space (all backend IPs returned identical results). A sharded setup would need epoch-in-cursor instead; noted as a follow-up if that ever changes.

Verification

• cargo test -p miden-note-transport-node — 17/17 pass (new + updated tests included; test_fetch_notes_paginates_at_batch_limit confirms normal backlog pagination is undisturbed — during pagination the cursor is always ≤ high-water).
• CLIPPY_CONF_DIR=configs cargo clippy --locked --all-targets --workspace -- -D warnings — clean.
• cargo +nightly fmt --all --check (repo config) — clean.

Draft: opening for review. No migration and no wire-format change; fixes every already-deployed client with no client update. A complementary client-side change (SDK fetch_all_private_notes never-regress guard) is optional defense-in-depth but not required once this lands.

[WiktorStarczewski]

Review round (two independent passes)

Ran an internal adversarial code review and an independent Codex review. Both concluded the primary logic is correct — neither could construct a false-positive reset for a legitimate client (within a single shared seq space, every client cursor derives from a server-echoed rcursor ≤ high_water, and sqlite_sequence.seq is monotonic-non-decreasing within a DB lifetime; the high-water read and the notes SELECT share one transaction snapshot). Fixes applied:

[MEDIUM — Codex] Streaming reset-storm. In streaming.rs, the stranded reset fired every 500ms but the subscription cursor only healed when notes were pushed. A subscriber whose tag has no notes in the new epoch (while other tags do → high-water non-None) would re-fire the reset (warn log + metric) every tick and never heal. Fixed: query_updates now emits a cursor-only heal entry when the effective cursor changed with no notes, so update_timestamps advances the stored cursor once; forward_updates skips empty batches so no empty update reaches subscribers. Storm stops after one tick.

[SUGGESTION — internal] gRPC echo-heal was untested. Added test_fetch_notes_response_cursor_heals_stranded_client: proves the handler builds FetchNotesResponse.cursor from the effective cursor (recovered seq), not the stranded value echoed back. A regression reverting rcursor = effective_cursor → = cursor now fails a test instead of silently re-breaking the fix (and re-introducing the analogous latent legacy-cursor bug).

[NIT] Docs/comments. Documented that an empty tag set short-circuits before the stranded check; added a comment that the strict > boundary is deliberate (a caught-up client at cursor == high_water must not be reset) with its bounded residual edge.

Not changed (reviewer agreed no action): two now-dead defensive int conversions (post_legacy_cursor.try_into::<i64>() can't fail since it's ≤ 1e12; effective_i64.try_into::<u64>() is always non-negative).

Follow-up: a streaming-path integration test for the heal would lock in the Finding-2 fix, but there's no existing StreamManager test harness — deferring rather than building one here.

Verification after fixes: cargo test -p miden-note-transport-node 18/18 pass; clippy --locked --all-targets --workspace -D warnings clean; nightly fmt --check clean.

[WiktorStarczewski]

Review round 2 (both passes, focused on the streaming heal)

Re-ran an internal reviewer pass and an independent Codex pass, this time scrutinizing the round-1 streaming fix (the freshest, least-reviewed code) and re-confirming the whole diff. Both concluded the change is correct — no false-reset, no note loss/duplication, and the reset storm provably converges (after a heal the cursor lands at 0, and the if effective > 0 guard permanently blocks re-entry). No Critical/Warning findings from either. Applied the actionable items:

• [MEDIUM] high_water_seq doc contradiction (sqlite/mod.rs): the None-path comment read as "treat as stranded" (backwards). Reworded to state the check is skipped on None, leaving the cursor unchanged — so no maintainer adds a wrong reset there.
• [MEDIUM] Streaming heal+delivery was untested. The round-1 test only covered a stranded tag with no notes. Added test_streaming_heals_stranded_cursor_and_delivers_notes: stranded tag with notes → reset re-scans from 0, notes are forwarded to the subscriber (waker consumed), and the stored cursor advances to the max seq. Covers the non-empty forward_updates path.
• [LOW] Pull-path heal-with-no-matching-notes was untested. Added test_fetch_notes_response_cursor_heals_when_no_matching_notes: stranded cursor on a tag with no notes → response cursor heals to 0 (not the stranded value echoed back).
• [LOW] Comments added on the load-bearing if effective > 0 convergence guard and on the forward_updates continue waker behavior.

Not changed (both reviewers agreed no action): the two dead defensive int conversions; the metric-description "at or below" wording (accurate).

Verification after fixes: cargo test -p miden-note-transport-node 21/21 pass · clippy --locked --all-targets --workspace -D warnings clean · nightly fmt --check clean.

This resolves the round-1 follow-up ("no streaming test harness") — the streaming manager now has direct heal tests.

[Kubudak90]

Heads up: this does not currently pass make clippy. collapsible_if fires on fetch_notes_by_tags, and the workspace lint config denies warnings, so it is an error rather than a warning.

error: this `if` statement can be collapsed
   --> crates/node/src/database/sqlite/mod.rs:243:17

Reproduced on this branch rebased onto current main, with nothing else applied:

CLIPPY_CONF_DIR=configs cargo clippy --locked --all-targets --workspace -- -D warnings

Collapsing the effective > 0 and if let Some(high_water) arms into a single let-chain clears it and keeps the comment attached to the > comparison:

if effective > 0
    && let Some(high_water) = high_water_seq(conn)
    // Strict `>` is deliberate: ...
    && effective > high_water
{
    effective = 0;
}

Context for why I was in here: I have been reconciling this with a per-subscriber stream cursor for #96, #122 and finding 3 of #123, discussed on #96. The two conflict in query_updates and forward_updates - your heal runs through TagData.cursor and update_timestamps, and the per-subscriber change removes both - so they need reconciling rather than a mechanical rebase. An integrated branch with both, including your streaming heal test rewritten in per-subscriber terms, is here if it is useful. No claim on your work - just flagging the overlap so whoever lands first is not a surprise to the other.

[Dominik1999]

Full review across five lenses: logical correctness, adversarial/security, API design, client-side semantics (verified against the rust-sdk consumer code), and hands-on verification (branch built and tested in isolation, including reverting each component of the fix to prove the new tests are non-vacuous).

Verdict

The approach is right and the core logic is verified sound - approving the design, but requesting changes on three items below before merge.

What was verified positively:

• The invariant is airtight: any echoed cursor is <= the AUTOINCREMENT high-water, which is monotonic within a DB lifetime and survives retention DELETEs and VACUUM, so strict > only fires cross-epoch - no false positives. The high-water read and the notes SELECT share one transaction snapshot (no race), and the streaming heal converges (reset fires at most once per stranding; no replay storm).
• 21/21 tests pass on the branch; each of the five new tests fails when its specific component of the fix is reverted (checked component-by-component). Extra probes confirmed: cursor == high_water is not reset; deleting all rows via retention does not cause a false reset (sqlite_sequence retains the high water); legacy-and-stranded cursors take the legacy path exactly once with a sane echoed cursor.
• Client behavior confirmed in the rust-sdk source: the client adopts a decreased cursor unconditionally and dedupes by NoteId, so the heal works end to end. The StreamNotesUpdate cursor never reaches client persistence, so streaming resets cannot re-strand a client.

Required changes

1. Note-volume disclosure oracle (new with this PR) - crates/node/src/node/grpc/mod.rs / crates/node/src/database/sqlite/mod.rs. Echoing the reset cursor when zero notes match creates an unauthenticated probe: pick a tag with no notes, binary-search cursor values, and response.cursor == cursor vs == 0 distinguishes below/above the high-water - the exact cumulative sqlite_sequence value (notes ever stored, which survives retention and is strictly more than Stats exposes) leaks in ~40 requests, plus DB-recreation timing. For a privacy-focused transport this is meaningful metadata. Suggested fix: only surface effective_cursor when notes were actually returned; on a no-match query echo the claimed cursor unchanged. Healing is unaffected - it only matters when there is something to deliver, and the first note that exists makes the reset query find it and heal the echo. Note this inverts test_fetch_notes_response_cursor_heals_when_no_matching_notes, which currently pins the leaking behavior.

2. Clippy failure on current stable - crates/node/src/database/sqlite/mod.rs. The nested if in the stranded check fires clippy::collapsible_if under current stable clippy (let-chains era) and the workspace promotes warnings to errors (already reproduced by a commenter here). It passes on 1.96.1, which is why local runs look clean. Collapse into a let-chain, keeping the boundary comment on the > comparison.

3. Document the cursor contract; withdraw the client-side guard suggestion - proto/proto/miden_note_transport.proto. This PR makes FetchNotesResponse.cursor able to decrease relative to the request cursor - a semver-invisible behavioral change on a public contract. Add to the proto docs (also on StreamNotesUpdate.cursor): the response cursor MAY be lower than the request cursor and clients MUST persist it verbatim, MUST NOT clamp it to a monotonic maximum. Related: the PR description proposes a client-side "never-regress guard" as defense-in-depth - applied to the global cursor, that guard would silently re-introduce the exact stranding this PR fixes (the SDK's unguarded global cursor is a correctness requirement, not an accident). Recommend removing that suggestion from the description.

Recommended (non-blocking)

• Replace the (Vec<StoredNote>, u64) return with a named struct (FetchResult { notes, effective_cursor }) - the API break is happening anyway; a struct makes it the last one and gives fix (1) a natural home.
• Reword the "one-insert-wide coincidence" comment: the miss window is "any first poll after the regrown high-water reaches the stranded cursor" - still low-probability given polling frequency, but the current framing understates it.
• The "server seq space regressed; DB recreated" WARN is client-forgeable on demand (any crafted cursor) - consider DEBUG or rate-limiting it, and dashboard db_fetch_notes_stranded_cursor_reset_count: it is the only operator signal for a restore or a misconfigured load balancer.
• Follow-ups worth filing: reset policy now lives in the DB trait return, which a future Postgres backend (#157) must reimplement per backend - exposing high_water_seq() as the primitive and moving the policy to the handler would keep it written once; an additive bool cursor_reset response field for client debuggability; thundering-herd after a restore (all clients replay simultaneously, compounded by #121's missing byte caps).

Checked and fine

Replay amplification is unchanged by this PR (cursor 0 always returned the full backlog; caps are #121, pre-existing). The fail-safe None path is not attacker-inducible. Streaming subscriber cursors cannot be client-injected. Pushing the high water via SendNote can suppress a victim's heal, but never leaves them worse than pre-PR (worth a one-line note that "always heals" is best-effort against concurrent writers). The waker-gated delivery drop in forward_updates is pre-existing #122, not this PR's problem.