Guard the condition estimates that a zero RocksDB entry-count estimate turns into Infinity or a negative

[kriszyp]

#2163 switched RocksDB plan sizing from an exact getKeysCount() full-key-space scan to the O(1) rocksdb.estimate-num-keys read. That property reports 0 for a populated table once its accumulated tombstones reach its non-deletions, which the exact count could not do — a 0 there meant a genuinely empty store. Measured on the shipped binding: a store holding 200 live records reports an estimate of 0 after 5000 tombstones for keys that were never written, while getKeysCount() still returns 200.

Two arithmetic sites in estimateConditionForTable cannot take a 0 there:

site	with a 0 estimate	guard	what it fixes
the AND-group branch divides by it	estimated_count === Infinity	|| 1, the idiom already on the sibling relationship divisor two branches below	plan selection
the ne null branch subtracts the index null count from it	negative (e.g. -500)	Math.max(…, 0)	a negative public estimated total

The divisor breaks planning. The adaptive filter/index switch derives thresholdRemainingMisses as estimated_count >> 4, and Infinity >> 4 is 0 rather than NaN, so the isNaN check passes it through, canUseIndex stays true with a threshold the loop can never respect, and the first non-matching record flips the request onto searchByIndex plus a full Set build, memoized for the next 10 seconds — the inverse of the plan #2163 exists to protect, on exactly the eviction- and replication-heavy tables that accumulate tombstones. Guarded, the same group estimates 200 and the threshold is a usable 12.

The subtraction does not. -32 and 0 both trip the switch on the first miss, so clamping it changes no plan. It matters because Table.ts reports estimated_count verbatim as the estimated pagination total, so an empty page could answer Content-Range: */-500 — the clamp below that call only ever raises a total, never lowers it. Math.max(…, 0) rather than 1, because a table whose rows are all null has a true ne null count of zero and flooring at 1 would trade a negative total for a phantom row.

estimatedEntryCount itself keeps returning 0 for the same reason: it is not planner-private, so flooring it in the helper would make an empty table report a record count of 1 — pinned by a test.

The same two guards ship to v5.1 and v5.2 alongside the backport of #2163's estimatedEntryCount portion, so those branches do not inherit this.

For the human reviewer

1. Estimate accuracy is the standing trade, not this PR's. estimate-num-keys counts entries without reconciling overwrites, so it reads high on rewrite-heavy stores and collapses to 0 under tombstones. DESIGN.md records 37,775 against an exact 4,001 on a real HNSW index store, and notes it "validates clean in isolation and only misleads on a real index" — which is why HNSW node counting uses a monotonic id counter. This PR does not change that; it only removes the two values the >> 4 guard provably cannot handle. The four multiplier call sites stay unguarded on purpose: a 0 estimate makes them 1, which is finite and positive, so the guard still functions — they degrade ordering, which is Use storage-level statistical range estimates in the query planner #2163's trade.
2. Guard placement — arithmetic vs helper. The first draft floored inside estimatedEntryCount, which is the tidier-looking fix and covers all ten call sites at once. It is wrong for the reason above: the accessor has two contracts, a planner input and a user-visible cardinality, and only the former needs a positive value. If you'd rather see a separate planner-only accessor wrapping the raw one, that is a reasonable alternative and a bigger diff.
3. Two idioms for one invariant. || 1 on the divisor, Math.max(…, 0) on the subtraction. Deliberate: || 1 cannot catch a negative, and the sites want different floors — 1 for a divisor, 0 for a cardinality.
4. What this does not fix. estimated_count >> 4 is not a safe way to derive a threshold from an unbounded estimate. It wraps negative above 2³¹ (1e12 >> 4 === -45461248), and its isNaN guard is dead code regardless — NaN >> 4 is 0, so the check can never fire. Every pathology this PR removes passes through that one expression, which is the real thing to fix. Review also surfaced a separate pre-existing defect in the same accumulator, which this PR does not touch: a zero-match condition followed by a full-scan comparator makes it NaN (0 * Infinity), and isFinite(NaN) being false then discards every estimate accumulated before it. The || 1 divisor is not involved — it is NaN for any divisor. And the underlying estimate is inaccurate in both directions — inflated under overwrite churn, collapsed under tombstones — so condition ordering can still put a wide range ahead of a narrow indexed equals. Those are plan-quality problems, not correctness ones, and each wants its own change; this PR only removes the values the guard provably cannot handle.

Verification

unitTests/resources/estimatedEntryCount.test.js — 6 tests, all passing under RocksDB; 5 passing and 1 pending under HARPER_STORAGE_ENGINE=lmdb. The RocksDB-specific test gates on store instanceof RocksDatabase rather than on typeof store.getEstimatedKeyCount, so a renamed or dropped binding method fails the guard loudly instead of silently skipping the one test that covers it.

Fails on base against origin/main, with rm -rf dist before each build because tsc incremental will otherwise report a pass against stale output: Infinity !== 200 for the AND-group estimate and -500 !== 0 for the ne null estimate — the two values this PR removes. The other four pass on base and should: main already has the O(1) swap and the intersectionEstimate clamp, and the "count is reported as 0" test guards against the wrong fix rather than proving this one.

The tombstone premise was measured, not argued: on the shipped binding, 200 live rows plus 5000 tombstones for never-written keys gives {estimated: 0, exact: 200}.

test:unit:resources — 2003 passing, 0 failing. test:unit:main did not complete in this worktree: it wedges in the before all hook of a git-tag test (resolves a local tag to its commit SHA), which needs tag/remote state the worktree does not have, and never reaches a summary. Not related to this diff — the same suite runs to completion in a v5.1 worktree — but stating it rather than reporting a pass I did not observe. CI is the authoritative run.

Not covered: the tests prove no Infinity or negative escapes into a plan estimate. They do not prove the resulting plan is the better one on a churned table, and the 2,000-row fixture is a fresh store with simple puts — precisely the profile DESIGN.md says the estimate reads accurately on. Neither the rewrite-inflated nor the tombstone-collapsed real-store case is covered; both need an end-to-end query against a churned table and are additive.

Complexity: medium

_{Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=3 @ 64b408a}

_{Human-Review-Need: 3 (decisions: ne-null-clamp-to-zero, divisor-floor-vs-product-cap, guard-at-call-sites-vs-in-the-helper, stub-store-test-scope) @ 64b408a}

[gemini-code-assist]

Code Review

This pull request prevents division-by-zero and negative estimation issues in resources/search.ts when estimatedEntryCount returns zero. It adds a fallback divisor of 1 during condition estimation and ensures that ne null condition estimates are non-negative by wrapping the calculation in Math.max(..., 0). Additionally, a comprehensive suite of unit tests has been added in unitTests/resources/estimatedEntryCount.test.js to verify these behaviors, including memoization and edge cases for zero-entry stores. I have no feedback to provide as there are no review comments.

[claude]

Reviewed; no blockers found.