Allow cancellation of IntersectGSL, optimise skip block check

[neilalexander]

Cancelling intersection mid-flight allows us to optimise the checkSkipFirstBlockMulti check by exiting early if we find that the optimisation is wasted because we're going to end up at the next block anyway.

[synadia-claude-reviewer]

Claude finished @neilalexander's task in 7m 8s —— View job

---

PR Review: Allow cancellation of IntersectGSL, optimise skip block check

• Gathering context and reading PR diff
• Reviewing server/stree/stree.go changes
• Reviewing server/filestore.go changes
• Reviewing server/memstore.go changes
• Reviewing server/stree/stree_test.go changes
• Posting final review

---

Overview

The core idea — making IntersectGSL's callback return bool to allow early termination — is a clean, well-motivated design change. The stree implementation and memstore changes look correct. However there is a correctness bug in one of the filestore callsites, and a subtle type-mismatch in the new skip-block optimisation.

---

Bug: havePartial is never set in NumPendingMulti — dead scan code

server/filestore.go, around line 4450–4500 (the seqStart >= len(fs.blks)/2 path)

var havePartial bool   // declared…
stree.IntersectGSL[SimpleState](mb.fss, sl, func(bsubj []byte, ss *SimpleState) bool {
    ...
    if sseq <= ss.First {
        t += ss.Msgs
    } else if sseq <= ss.Last {
        // We matched but its a partial.
        return false   // …but havePartial is NEVER set to true
    }
    return true
})

if havePartial {       // always false — this block is now dead code
    t = 0
    // full sequential scan from sseq…
}

The original code set havePartial = true and stopped further callback iteration (by checking it at the top of the next callback invocation). In the PR, havePartial = true was replaced entirely by return false. The early-exit optimization is fine, but the flag must still be set so the subsequent full-scan block is reached.

Impact: When a subject in the block has messages that straddle sseq (partial match), the partial subject's pending messages from sseq to ss.Last are silently dropped from the count. NumPendingMulti returns an under-count.

Fix: Combine the flag set with the early exit:

} else if sseq <= ss.Last {
    havePartial = true   // keep this
    return false         // and add this
}

Fix this →

---

Subtle concern: slice index vs block index in checkSkipFirstBlockMulti

server/filestore.go, line 3460

guard := uint32(bi + 1)   // bi is a *slice* index into fs.blks
...
if psi.fblk < start {
    start = psi.fblk      // psi.fblk is a *block* index (mb.index)
}
if start == guard {       // comparing slice-index+1 with a block-index
    return false
}

bi is a position in the fs.blks slice; psi.fblk is mb.index, a monotonically-increasing block serial number. After stream purges these diverge: e.g., on a stream that has had its first 99 blocks purged, blks[0].index == 100 while bi + 1 == 1. A subject whose stale fblk happens to equal 1 would trigger the guard and return bi+1, nil instead of what selectSkipFirstBlock (which correctly uses mbi = blks[bi].getIndex()) would compute.

The existing selectSkipFirstBlock handles this correctly via mbi. The guard optimisation bypasses that protection.

In practice: the worst outcome is returning bi+1, nil when ErrStoreEOF is correct (extra unnecessary block scanning, no data loss). Given psi.fblk is documented as "lazy" and can be stale, this edge-case is reachable on long-lived streams.

Suggested guard:

guard := fs.blks[bi].getIndex() + 1  // use block index, not slice index

(This requires holding the FS lock, which callers already do.)

---

stree.go / stree_test.go

The _intersectGSL propagation logic is correct — false bubbles up through all recursive calls and the outer IntersectGSL wrapper correctly discards the bool (callers don't need it). The new StopsWhenCallbackReturnsFalse test is a good addition.

---

memstore.go

Mechanical change only; havePartial is still set correctly here (line 972). No issues.

---

Summary: One correctness bug (havePartial never set) that causes NumPendingMulti to under-count pending messages on partial blocks, and one minor index-type mismatch in the skip-block guard that could cause unnecessary scanning on heavily-purged streams. Both are straightforward to fix.

[neilalexander]

@claude re-review please

[synadia-claude-reviewer]

PR Re-Review: Allow cancellation of IntersectGSL, optimise skip block check

• Gathering context
• Reading current state of changed files
• Checking if previous review issues are addressed
• Reviewing for any new issues
• Posting final review

View job run

[neilalexander]

@claude re-review please

[synadia-claude-reviewer]

Claude finished @neilalexander's task in 2m 28s —— View job

---

PR Re-Review: Allow cancellation of IntersectGSL, optimise skip block check

• Gathering context
• Reading current state of changed files
• Checking if previous review issues are addressed
• Reviewing for any new issues
• Posting final review

---

Both issues raised in the previous review have been addressed. Here's a full account:

Issue 1 — havePartial never set: Fixed ✓

server/filestore.go:4465–4468 now correctly does:

} else if sseq <= ss.Last {
    // We matched but its a partial.
    havePartial = true
    return false
}

havePartial = true is set before the early exit, so the subsequent full sequential scan is correctly triggered when a partial block is encountered.

Issue 2 — Slice index vs block index in checkSkipFirstBlockMulti: Fixed ✓

server/filestore.go:3464 now uses:

guard := fs.blks[bi].getIndex() + 1

getIndex() returns the block's monotonic serial number (mb.index), matching the type of psi.fblk. The comparison is now semantically correct even on heavily-purged streams where slice indices and block indices diverge.

---

New code in checkSkipFirstBlockMulti

Two new optimisations were added. Both look correct:

MatchesFullWildcard() shortcut (filestore.go:3458–3461): If the sublist matches everything, we know the next block always qualifies, so the loop is skipped entirely. This is a straightforward win.

Guard-based early exit in the IntersectGSL callback (filestore.go:3465–3476): The callback updates start (running minimum of psi.fblk), then checks if start == guard. This correctly detects that a subject's first block is the next block, making further scanning pointless. Crucially, stop is left partially computed when the early exit fires, but the post-loop if start == guard { return bi+1, nil } short-circuits before stop is used. The path to selectSkipFirstBlock(bi, start, stop) is only reached when no early exit occurred, so stop is always fully populated when it's actually consumed. Logic is sound.

---

stree.go / stree_test.go / memstore.go

No changes from the previous review — all correct.

---

All previously raised issues are resolved. No new issues found. LGTM.

[MauriceVanVeen]

LGTM