fix: BM25 counter drift (#10184), concurrent commit retry (#10171), vector reranker loads (#10170), vector.neighbors where (#10169) - #10185

No actionable comments were generated in the recent review. 🎉

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Out of Scope Changes check		The WebSocket authentication test changes the expected wrong-credential response from 403 to 401 and references #10180. This change does not implement #10184, #10171, #10170, or #10169. The linked iss…	Remove the unrelated WebSocketEventBusIT authentication expectation change from this pull request, or link the change to an active issue that requires it.
Docstring Coverage		Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 152 functions across 29 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Title check		The title clearly identifies the four main fixes covered by the pull request and references their issue numbers. It is long but specific and related to the changeset.
Description check		The description explains the changes, motivation through the linked issues, related issues, implementation details, merged dependencies, and test coverage. It does not use the template headings or inc…
Linked Issues check		The PR meets the coding requirements for the active linked issues. For #10184, BM25 writes, drift checks, rescans, logical-index bucket scans, and crash recovery handling use consistent document-lengt…

Full details: Description check

Explanation

The description explains the changes, motivation through the linked issues, related issues, implementation details, merged dependencies, and test coverage. It does not use the template headings or include the checklist, but the required information is mostly present.

Full details: Linked Issues check

Explanation

The PR meets the coding requirements for the active linked issues. For #10184, BM25 writes, drift checks, rescans, logical-index bucket scans, and crash recovery handling use consistent document-length accounting. Regression tests cover null values, subtypes, mixed workloads, and recovery. For #10171, ended-session handling returns 204 only when the same user committed the session, and otherwise preserves 404 behavior. Integration tests cover both cases. For #10170, rerankers reuse carried records and use lazy score overrides. For #10169, both neighbor functions support map and SQL where predicates, combine where with filter, and test candidates through the predicate allow-list. Focused tests cover these behaviors.

Full details: Out of Scope Changes check

Explanation

The WebSocket authentication test changes the expected wrong-credential response from 403 to 401 and references #10180. This change does not implement #10184, #10171, #10170, or #10169. The linked issue set does not establish scope for this authentication change.

Resolution

Remove the unrelated WebSocketEventBusIT authentication expectation change from this pull request, or link the change to an active issue that requires it.

• Fix all pre-merge checks with AI

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

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

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Review of #10185. Overall a well-reasoned PR: the root-cause analysis of #10130 (sub-type bucket indexes writing their own totals into the shared counters) is good, and the tests are named per issue. I read the main-code changes but did not run the tests. Most important notes first.

1. RecordPredicateRidSet.contains() evaluates under synchronized(this). The record load and the SQL condition run inside the lock, yet the Javadoc says the set is meant for concurrent sparse fan-out. Workers serialize on the slowest lookup. Consider a ConcurrentHashMap<RID, Boolean> and evaluating outside the lock; a duplicated evaluation is idempotent.
2. The memo (HashMap<RID, Boolean>) is unbounded per call and boxes keys and values. Large efSearch or a sparse fan-out over millions of postings can grow it a lot. Document it, cap it, or use a compact structure.
3. A where SQL string runs with the caller's CommandContext, and lookupByRID(rid, true) loads records directly. Please confirm per-user read permissions are not bypassed, and add a test with a restricted user.
4. size() returns Integer.MAX_VALUE, isEmpty() returns false, and iterator() can throw. This is fragile if equals, hashCode, toString, addAll or stream ever touch the set. A narrower interface would be safer than subclassing AbstractSet.
5. performRecovery() forces an unconditional BM25 rescan at the first search after an unclean shutdown. That is correct, but it costs one full scan on large corpora. A log line when it triggers would help operators.
6. HTTP retry (A commit retry that races the still-running first commit of the same session answers 404 although it committed #10171): the logic is sound (recordCommitted runs before the session is removed, and a failed commit is not recorded, so it gives 404). Please confirm HttpSessionEndedException extends HttpSessionException so existing catch sites behave as before.

Style: the field and @PARAM alignment in RecordPredicateRidSet is off. Make sure no debug output remains in the new tests.

Tests: coverage is good (7 BM25, 2 ITs verified red without the fix, 7 plus 6 vector). Missing are a where test under concurrent sparse fan-out and the restricted-user test from point 3. The #10171 IT should avoid raw wall-clock assertions, per CLAUDE.md (StallAwareStopwatch).

Process: the PR bundles the unmerged #10181 and #10172, which makes review harder. A rebase after those land would help.

Nothing looks blocking. Points 1 to 3 are the ones I would address before merge.

Review (PR's own changes: BM25 counters, commit retry, rerankers, where. The merged-in #10181 and #10172 commits were only skimmed. I did not build or run the tests.)

Overall: well scoped, well commented, and each fix is pinned by a regression test named after its issue. The root-cause analysis for #10130 (a sub type's bucket index writing its own totals into the shared counters) is good. Mostly questions and minor suggestions below.

BM25 counters (#10184)

• documentLength() as the single definition for put, remove and rescan is the right fix for Inserting and deleting one document whose single indexed property is NULL permanently subtracts 1 from the BM25 sumDocLength, and no drift check can ever see it #10131.
• corpusBuckets() allocates a list and calls bucket.count() per bucket. It only runs behind claimStaleCheck(), so it is not hot. If count() can fall back to a scan on a cold cache, a large sub-typed index could pay for it on the first search. Worth a check.
• The rescan comment says it reads this transaction's own uncommitted writes. That used to be db.iterateType and is now bucket.iterator(). I believe both see the tx's modified pages, but please confirm, since the logic subtracting the tx's own contribution depends on it.
• After crash recovery a BM25 index whose record count did not change keeps the pre-crash sumDocLength forever: the once-per-session drift check only compares document counts, so half the corpus state it exists to repair is invisible to it #10133: arming requireRescan() on every BM25 index after any recovery means a full scan on the first search of each. Reasonable trade-off versus a schema format change, and the INFO log helps. It may be a latency spike on very large corpora, so mention it in the release notes.
• Existing indexes keep pre-fix drift until their next rescan. Consider whether a one-time rescan on upgrade is wanted.

Commit retry (#10171 / #10164)

• The generic answerForEndedSession() hook is cleaner than special-casing commit in HttpSession.
• HttpSessionEndedException is thrown inside the try of execute(), so the generic catch (Exception) arm runs for it. With rollbackOnFailure it calls rollbackIfActive() on a dead session and may set rolledBackByFailure. Harmless since the session is gone, but adding it next to RequestRefusedBeforeExecutionException in the pass-through catch would state the intent and avoid touching state.
• Behaviour change: /commit for an unknown or expired session used to be an idempotent 204 and is now a 404. That is the intended fix and the RemoteConsoleIT change reflects it. Call it out in the release notes, since clients that retried blindly after expiry will now see an error.
• recordCommitted runs in the finally only when committed is true, and the memory is bounded (max entries plus TTL) and keyed by user. Good.

where option (#10169)

• RecordPredicateRidSet is a nice way to reuse the allow-list plumbing without materializing RIDs.
• It breaks the Set contract on purpose: isEmpty() is always false, size() can be Integer.MAX_VALUE, and iterator() throws without a filter. It is documented, but any new consumer calling addAll, stream() or a copy constructor on the allow-list will fail. Consider a narrower interface, or a test asserting the index paths never enumerate it.
• The decided memo is unbounded for the call. The Javadoc argues the search bounds it, which is plausible, but a very wide ef or fan-out on a big type could hold many entries. Two RID sets (accepted and rejected) would be lighter than boxed Boolean values, per the project's GC guidance.
• The SQL-condition branch serializes on synchronized(this), including the record evaluation. Under sparse fan-out that is a contention point.
• Security: the user-supplied SQL string is parsed with SQLAntlrParser.parseCondition and evaluated with the caller's permissions. I found no injection concern, and SecurityException yields not accepted, so existence is not leaked.

Rerankers (#10170)

• carriedRecord checks identity against the RID, so a user property called record is not mistaken for the record. Good.
• ScoreOverrideRow is lazy and keeps the upstream row for downstream rerankers. Please confirm it serializes correctly to JSON over HTTP and through Result conversions. The old code returned plain LinkedHashMaps, so the type callers see has changed.

Process and tests

• The PR bundles commits from perf(vector): vector.fuse / vector.rerank stop loading every match twice #10181 and fix(#10164): commit naming an unknown or terminated session answers 404 #10172. Rebasing after those land would give a cleaner diff.
• I would like a test for the where set that goes through a real index search and checks the no-enumeration behaviour, and one for concurrent sparse fan-out with a SQL condition.

No blocking issues found. Nice work.

Review

Overall this is a careful, well-documented PR. I reviewed the PR's own commits (BM25 counters in LSMTreeFullTextIndex, HTTP session/commit retry), not the merged #10181 / #10172 content.

What looks good

• documentLength() as the single definition shared by put, remove and the rescan is the right fix for Inserting and deleting one document whose single indexed property is NULL permanently subtracts 1 from the BM25 sumDocLength, and no drift check can ever see it #10131, and removing the per-writer inline counting removes the class of bug.
• corpusBuckets() makes the drift check and the rescan read the same population as the writers, which fixes the root cause of A full-text index on a type that has subtypes reports a permanent BM25 "corpus counters diverged" warning and pays a full polymorphic type scan on the first search of every session, forever #10130 rather than the symptom. Arming requireRescan() in recovery (After crash recovery a BM25 index whose record count did not change keeps the pre-crash sumDocLength forever: the once-per-session drift check only compares document counts, so half the corpus state it exists to repair is invisible to it #10133) avoids a schema format change.
• recordCommitted happens before the session is removed, so there is no window where a retry sees the id as neither open nor committed. The map is bounded (10k entries plus time pruning), owner-gated, and cleared in removeSessionsForUser. Good security hygiene.
• HttpSessionEndedException is rethrown without rolling back, so the transaction stays with the request that ended the session.
• Regression tests exist for each issue, and the RemoteConsoleIT expectation is updated for the new semantics.

Points to consider

1. Behavior change in /commit for unknown sessions (A commit naming an unknown or terminated transaction session answers 204, as if it committed #10164). A /commit naming an unresolvable session now returns 404 unless this server remembers committing it. The memory is per server and in memory, so a retry after a restart, or one routed to another node in an HA cluster after the first commit succeeded, now fails with 404, where it used to get 204. The Javadoc says so, but please also make sure it is in the user-facing docs or release notes, since clients with retry loops may see this as a regression.
2. Wall clock in wasCommittedBy / pruneCommittedSessions. System.currentTimeMillis() is subject to clock adjustments. Using System.nanoTime() for committedAt would be more robust. This is minor, and consistent with lastUpdate elsewhere in the class.
3. corpusBuckets() allocates a list on every call. It runs only once per stale-check claim, so it is fine. Still, ensureCounters could sum directly without building the list. The rescan needs the list, so this is optional.
4. Rescan now iterates bucket.iterator() instead of db.iterateType(...). Please confirm that it still sees the current transaction's uncommitted writes, because the comment right after the loop ("The scan reads this transaction's own uncommitted writes...") depends on it. A test that triggers the rescan inside an open transaction would settle this if one isn't already there.
5. Mixed-scope PR. The PR carries unrelated work (perf(vector): vector.fuse / vector.rerank stop loading every match twice #10181, fix(#10164): commit naming an unknown or terminated session answers 404 #10172) plus three independent fixes (BM25, HTTP retry, vector). Reviewing and bisecting would be easier once the dependencies land and this is rebased, or split by area.
6. Style nit. PostCommitHandler.execute builds the HeaderValues lookup inline. A small helper such as findSessionId(exchange) shared with findSession would avoid duplicating the header parsing.

Tests

Coverage is good, including the concurrent retry IT (Issue10171ConcurrentCommitRetryIT). Please make sure nothing in it depends on raw wall-clock timing (see the StallAwareStopwatch guidance in CLAUDE.md). hasWaitingRequests() is a sensible deterministic hook for that.

No blocking issues from my side; point 1 (docs) and point 4 (confirm) are the ones I'd address before merge.

Review

I read the PR's own changes: LSMTreeFullTextIndex, HttpSession, HttpSessionManager, DatabaseAbstractHandler, PostCommitHandler and the new tests. I did not review the merged #10181 and #10172 content in depth, and I did not run the tests.

What looks good

• BM25 (Inserting and deleting one document whose single indexed property is NULL permanently subtracts 1 from the BM25 sumDocLength, and no drift check can ever see it #10131): a single documentLength() helper used by putWithStats, remove and the rescan is the right fix. Put and remove can no longer disagree about the null token.
• BM25 (A full-text index on a type that has subtypes reports a permanent BM25 "corpus counters diverged" warning and pays a full polymorphic type scan on the first search of every session, forever #10130): corpusBuckets() makes the drift check and the rescan count the same population as the writers. corpusTypeName() fixes the root cause, which was a sub type's bucket index writing its own totals into the shared counters.
• BM25 (After crash recovery a BM25 index whose record count did not change keeps the pre-crash sumDocLength forever: the once-per-session drift check only compares document counts, so half the corpus state it exists to repair is invisible to it #10133): arming requireRescan() in performRecovery() is cheaper than persisting a tx id and needs no schema change.
• Commit retry (A commit retry that races the still-running first commit of the same session answers 404 although it committed #10171, A commit naming an unknown or terminated transaction session answers 204, as if it committed #10164): HttpSessionEndedException plus the answerForEndedSession() hook keeps the route-specific policy out of HttpSession. The new exception is rethrown without rolling back, which is correct because the other request owns that transaction.
  • Recording the commit before the session is removed closes the "neither open nor committed" gap.
  • wasCommittedBy is gated on the owner, and removeSessionsForUser purges the entries, so there is no cross-principal leak.
  • The map is bounded at 10k entries and pruned by the idle sweep, so memory is fine.
• Tests: there is a regression test per issue, including the concurrency case and the sweep pruning.

Suggestions

1. Behaviour change. A /commit naming an unknown or expired session used to answer 204 and now answers 404. That is correct, and the RemoteConsoleIT change reflects it. It can still surprise clients that treated 204 as "safe to ignore", so please call it out in the release notes.
2. HA gap. committedSessions is per server and in memory, as the Javadoc says. After a leader failover, or a retry against another node, a retry of a successful commit now gets a 404, where before it got a silent 204. That is arguably more honest, but the client can't tell "committed" from "lost". Consider a short note in the docs or OpenAPI text saying a 404 on commit is ambiguous across failover.
3. corpusBuckets() allocation. It builds a new ArrayList on every stale check and rescan. This is cold, so it is fine. ensureCounters() only needs the count, so it could sum without materialising the list.
4. Leftover in putWithStats. final int len = docLen; is now redundant after docLen became final. docLen can be used directly in the lambda or loop.
5. pruneCommittedSessions takes the read lock and then the write lock, so the emptiness check can race. That is harmless, but the check can be dropped, since an empty iteration under the write lock is trivial.
6. Scope. This PR closes about seven issues across the BM25 index, the HTTP session layer and the vector functions, and it carries merged commits from perf(vector): vector.fuse / vector.rerank stop loading every match twice #10181 and fix(#10164): commit naming an unknown or terminated session answers 404 #10172. It would be easier to review and bisect as separate PRs. Rebasing once those two land would at least shrink the diff.
7. Timing in tests. Make sure Issue10171ConcurrentCommitRetryIT and the sweep tests wait with polling or hasWaitingRequests() and never use raw Thread.sleep bounds. Per CLAUDE.md, wall-clock assertions are only acceptable through StallAwareStopwatch.

No blocking bugs found. Approve once the merged dependencies land and the nits are addressed.

Review

I read the PR's own changes: the BM25 counters in LSMTreeFullTextIndex, the HTTP session and commit-retry work, and the new tests. I skimmed the vector where / ScoredRecordRow commits that came in through the merged PRs and did not review them in depth. I did not build the project or run any tests.

What looks good

• documentLength() is now the single definition used by putWithStats, remove and the rescan, which removes the null-token asymmetry (Inserting and deleting one document whose single indexed property is NULL permanently subtracts 1 from the BM25 sumDocLength, and no drift check can ever see it #10131) at the source.
• corpusBuckets() makes the drift check and the rescan count the same buckets the writers count. That fits the real cause of A full-text index on a type that has subtypes reports a permanent BM25 "corpus counters diverged" warning and pays a full polymorphic type scan on the first search of every session, forever #10130, which was bucket indexes writing sub-type totals into the shared counters. It also drops the countType / iterateType mismatch.
• In PostCommitHandler, recordCommitted runs inside the session lock and before the session is removed. A waiter can therefore never find the session neither open nor committed.
• wasCommittedBy checks the owner, applies the timeout, and the map is capped at 10k entries. removeSessionsForUser also clears the remembered commits, which closes the recreated-principal hole.
• Each fix has its own regression test (Issue10184..., Issue10164..., Issue10171...), and RemoteConsoleIT was updated to match the new behaviour.

Points to consider

1. Behaviour change on /commit. A commit that names an unknown session now returns 404 where it used to return 204. The new behaviour is more correct. It is still visible to clients that treated that 204 as "nothing to do", so I'd mention it in the release notes. The OpenAPI spec was updated, which is good.
2. Commit retries across HA nodes and restarts. Remembered commits live in memory on a single server, as the Javadoc says. A retry sent to a different node after a failover gets 404 even though the commit succeeded, which is the same lost-answer case [server audit] HTTP transaction sessions: not removed on commit/rollback, idle-sweep race leaks a transaction, ownership checked by name #5026 set out to cover. If clients commonly retry across nodes in HA, this gap is worth documenting or tracking in an issue.
3. Concurrent retry when the first commit fails. The first commit throws, so committed stays false and nothing is recorded. The waiting retry then gets 404 "not found or expired", while the real cause was a commit failure such as a conflict. That is acceptable, but the message may mislead.
4. Cost of the BM25 drift check. corpusBuckets() allocates a list and loops bucket.count(). It only runs behind claimStaleCheck, so I don't think it matters. The rescan now iterates buckets directly instead of iterateType. Please confirm that bucket.iterator() skips deleted records and behaves the same as iterateType for in-transaction deletes.
5. Crash recovery (After crash recovery a BM25 index whose record count did not change keeps the pre-crash sumDocLength forever: the once-per-session drift check only compares document counts, so half the corpus state it exists to repair is invisible to it #10133). The description says performRecovery() calls requireRescan() on every BM25 index. Please confirm there is a test that simulates an unclean open. I only skimmed the test list and did not look for one.
6. Merged branches. This PR carries commits from perf(vector): vector.fuse / vector.rerank stop loading every match twice #10181 and fix(#10164): commit naming an unknown or terminated session answers 404 #10172, so the diff is large (about 150KB) and is hard to review as one unit. Merging those two first would help reviewers.
7. Style. In HttpSession.execute, the single multi-catch of RequestRefusedBeforeExecutionException | HttpSessionEndedException is fine. The HttpSessionEndedException Javadoc has @author, which matches the other files. Nothing else to flag against CLAUDE.md. I saw no stray System.out and no fully qualified names.

I found no blocking issues. Points 2 and 5 are the ones I'd confirm before merge.

Review

I read the new code in RecordPredicateRidSet, HttpSession, PostCommitHandler and DatabaseAbstractHandler. I did not run the tests. I did not read the BM25 (LSMTreeFullTextIndex) or reranker (ScoreOverrideRow) changes in detail, so this says nothing about them. The PR description reports the new tests as passing.

What looks good

• The answerForEndedSession() hook is generic, so /commit is not special-cased inside HttpSession. Every other route and case stays 404.
• HttpSessionEndedException is rethrown before the catch (Exception) arm, so a session that another request already ended does not get a rollback attempt and a rolledBackByFailure mark.
• RecordPredicateRidSet is well documented. It fails closed on RecordNotFoundException and SecurityException, and it refuses positional ? parameters, which would otherwise bind silently to the wrong value. hasPositionalParameter handles quoting and escapes.
• There is regression coverage for each linked issue, and the Issue10171 test was verified red before the fix.

Points to consider

1. RecordPredicateRidSet (where, SQL-condition form)

• synchronized (this) around matchesFilters serializes every candidate test across the workers of a sparse fan-out. Each test also does a record load, because the load happens inside test() and the lock sits only around the condition. The class Javadoc is honest about this. Still, a condition-form where on vector.sparseNeighbors gets no parallelism from the fan-out. Could each worker use a cheap per-thread child context instead?
• The decided memo is unbounded for the life of the call. The Javadoc says the search bounds it. For a selective where on a large HNSW search with a big ef/fetch budget, it is worth confirming that bound. One boxed Boolean plus a RID key per candidate is what makes it heavy. A ConcurrentHashMap<RID, Boolean> is fine, but you could sidestep the boxing concern by noting Boolean.TRUE/FALSE are cached, so only the node and key cost anything.
• size() returns Integer.MAX_VALUE, and isEmpty() returns false regardless of content. That is deliberate and documented. It does mean generic AbstractSet methods (containsAll, removeAll, toArray, streams) misbehave or throw. A short guard that makes unsupported mutators and bulk operations throw explicitly would protect future callers.
• In of(...), a map where whose keys are not strings is stringified with String.valueOf. That is fine, but a nested-path key such as a.b depends on readNestedField. A test with a missing intermediate path would pin the behaviour. I did not check whether one exists.
• A malformed SQL condition is wrapped in CommandSQLParsingException. That is good. Check that the message does not echo untrusted text into a place that is rendered unescaped (Studio). It is probably fine.

2. HttpSession.execute

• HttpSessionEndedException is only thrown on the "not registered" path under the lock. That is correct, but the comment block above it still describes only the idle-sweep race. A one-line mention of the retried-commit case in that comment would help.
• /commit answers 204 via wasCommittedBy(user, id). The same-user check is in execute(), which throws SecurityException before reaching this. Please confirm a different principal retrying the same id still gets 404. The session == null path in PostCommitHandler already uses wasCommittedBy(user, ...), so this is probably covered. I would like an assertion for it in Issue10171ConcurrentCommitRetryIT.

3. PR hygiene

• The branch includes the commits of perf(vector): vector.fuse / vector.rerank stop loading every match twice #10181 and fix(#10164): commit naming an unknown or terminated session answers 404 #10172. That makes review harder and creates merge-order risk. Rebase once those land, or consider splitting into the BM25, HTTP and vector changes. They are unrelated to each other.
• Please confirm the vector tests that wait on background work carry the right @Tag. If Issue10169/Issue10170 do any index builds that wait, they may need vector.

4. CLAUDE.md conventions

• Prefer final and no braces on single-statement if. Both are mostly followed.
• The field alignment in RecordPredicateRidSet (extra spaces after the types) is inconsistent with its neighbours. That is cosmetic.
• No new dependency and no System.out found in the files I read.

Verdict

Solid, well-tested changes. The main thing I would address before merge is the lock contention of the condition-form where in the sparse fan-out, or an explicit note that it is accepted. Beyond that, add the cross-principal assertion for the retried commit, and split or rebase the PR so the diff shows only its own commits.

Review of #10185

I read the BM25 counter changes (LSMTreeFullTextIndex) and the HTTP concurrent-commit change (HttpSession, DatabaseAbstractHandler, PostCommitHandler, HttpSessionManager.wasCommittedBy). I did not read the vector changes (#10169/#10170) line by line, and I did not run the tests.

What looks good

• Inserting and deleting one document whose single indexed property is NULL permanently subtracts 1 from the BM25 sumDocLength, and no drift check can ever see it #10131: documentLength() is a single definition used by put, remove and the rescan, so the remove path can no longer subtract a null token the put never added. The comments say why.
• A full-text index on a type that has subtypes reports a permanent BM25 "corpus counters diverged" warning and pays a full polymorphic type scan on the first search of every session, forever #10130: corpusBuckets() makes the drift check and the rescan read the same population the writers count. That covers the real cause, which was a sub type's bucket index writing its own totals into the shared counters. The new corpusTypeName() falls back to getTypeName() when there is no logical index.
• After crash recovery a BM25 index whose record count did not change keeps the pre-crash sumDocLength forever: the once-per-session drift check only compares document counts, so half the corpus state it exists to repair is invisible to it #10133: arming requireRescan() in performRecovery() is cheap and needs no format change. It also avoids false positives on a clean open.
• Logging: passing the drift message as an argument ("%s") avoids format-string problems when an index name contains %.
• A commit retry that races the still-running first commit of the same session answers 404 although it committed #10171:
  • The retry answers 204 only when wasCommittedBy is true, and that check is gated on the owning user and the session timeout. A different principal still gets 404 and cannot learn the session existed.
  • HttpSessionEndedException is rethrown without touching the other request's transaction.
  • The default answerForEndedSession() returns null, so other routes keep the old behaviour.
• Tests: there are regression tests per issue (Issue10184..., Issue10169..., Issue10170..., Issue10171...IT), which matches the repo convention.

Suggestions (non-blocking)

1. In PostCommitHandler.answerForEndedSession, make sure the 204 body and headers match the normal commit success path. For example, check whether the normal path sets any bookmark header, such as emitCommitIndexBookmark. The retry short-circuits that call, so a client relying on read-your-writes might not get the bookmark. Consider echoing the committed index if it is available.
2. Check that committedSessions only records a session after the commit actually succeeds. If a failed commit is also recorded, a retry would get 204 for data that was never persisted. A test for the failed-commit-then-retry case would pin this down.
3. corpusBuckets() allocates a new ArrayList each call. That is fine on the once-per-session stale check, but it also runs in the rescan, so confirm it is not on a per-query hot path.
4. IT tests that wait on a race can flake. Make sure the concurrent-commit test forces the ordering, for example with latches, and does not depend on sleeps. Use StallAwareStopwatch for any timing bounds, per CLAUDE.md.
5. Process: the PR bundles the unmerged perf(vector): vector.fuse / vector.rerank stop loading every match twice #10181 and fix(#10164): commit naming an unknown or terminated session answers 404 #10172 commits, so the diff is large. Merging those first and rebasing would make this much easier to review, and it should keep git blame clean.

I found no security problems. Overall this looks good to merge once the points about the 204 bookmark header and failed-commit recording are checked.

Review of #10185

I reviewed the diff and did not build or run any tests. My read covered the BM25 changes, the HttpSession and commit changes, and RecordPredicateRidSet. It was not exhaustive: the vector function and rerank changes were only skimmed.

Overall

The fixes are well reasoned and the commit messages and Javadoc explain the "why" clearly. Each issue has a dedicated Issue10xxx...Test regression test, which matches the repo convention.

Scope

This branch bundles #10181 and #10172, so the diff is about 230 KB and mixes four unrelated concerns: BM25 counters, HTTP commit retry, vector.* rerank and the where option. That makes it hard to review. Once the two parent PRs land on main, rebasing will shrink it. Until then I'd suggest merging those first.

Correctness and design

• BM25 (Inserting and deleting one document whose single indexed property is NULL permanently subtracts 1 from the BM25 sumDocLength, and no drift check can ever see it #10131, A full-text index on a type that has subtypes reports a permanent BM25 "corpus counters diverged" warning and pays a full polymorphic type scan on the first search of every session, forever #10130): Routing every writer and the rescan through one documentLength() helper and one corpusBuckets() is the right fix. It removes the asymmetry at its source instead of patching it.
• BM25 (After crash recovery a BM25 index whose record count did not change keeps the pre-crash sumDocLength forever: the once-per-session drift check only compares document counts, so half the corpus state it exists to repair is invisible to it #10133): Calling requireRescan() in performRecovery() is simpler than persisting a tx id. The cost is a full scan of every BM25 index after each unclean shutdown. That is acceptable, but on large indexes the first search will be slow. The INFO log helps, so consider saying so in the docs too.
• HTTP (A commit retry that races the still-running first commit of the same session answers 404 although it committed #10171): Throwing HttpSessionEndedException and letting the route answer through answerForEndedSession() is clean. Re-throwing it before the rollback-on-exception path is correct, because the transaction now belongs to the request that ended the session. Please confirm there is no other catch (Exception) between execute and the handler that would swallow it as a generic 500. The IT covers the main path.
• RecordPredicateRidSet memory: The decided memo is a ConcurrentHashMap with one entry per candidate. Boxed Boolean values and RID keys are heavy on the GC, which CLAUDE.md asks us to avoid. The class comment says the search bounds the memo, but the exhaustive fallback for selective predicates tests every indexed vector. For a large index that memo grows to the index size per call.
• where cost: A highly selective or empty where can load every indexed record on every call. The Javadoc documents this, but it is a cheap way for a user to generate heavy load. A per-query cap or a metric would help. Alternatively, fall back to a property-index lookup when one exists for the equality map form.
• Parse errors: In the catch (CommandSQLParsingException e) { throw e; } plus catch (Exception e) pair, the first clause is only there to avoid re-wrapping. It's fine, but a short comment would help.
• Javadoc: The @param alignment in of(...) is off (functionName is wider than the others).
• Permissions: Good that unreadable candidates are silently rejected so the predicate can't act as an oracle. A test with a restricted user would pin that down. I couldn't tell whether one exists.

Tests

• Coverage looks good. I couldn't see in the files I read whether any test asserts on wall-clock time. If the concurrent-retry IT does, it should use StallAwareStopwatch per CLAUDE.md. The hasWaitingRequests() helper is package-private for tests, which is fine.
• Please make sure Issue10171ConcurrentCommitRetryIT gets its port from the fixture base and doesn't hardcode one.
• A BM25 test that does a real unclean shutdown and checks the rescan on first search would be the best check of After crash recovery a BM25 index whose record count did not change keeps the pre-crash sumDocLength forever: the once-per-session drift check only compares document counts, so half the corpus state it exists to repair is invisible to it #10133, if Issue10184Bm25CorpusCountersDriftTest doesn't already do that.

Minor

• The ALL-CAPS comment style in LocalDatabase matches the surrounding code.
• The ? scanner in hasPositionalParameter should also be tested with escaped quotes, such as 'it\'s ?'.

Nothing here is a blocker for me. The main points to resolve are the memory and cost profile of the where fallback, and the PR scope.

Review

I read the main-code diffs for the four fixes (BM25 counters, commit retry, rerankers, `where`). I did not build the branch or run the tests, so the comments below come from reading the code only. Overall this is careful work: each fix targets a root cause, has a regression test, and the comments say why.

BM25 counters (#10184)

• Good: `documentLength()` is now the single definition used by `putWithStats`, `remove` and the rescan. That fixes the put/remove asymmetry structurally.
• Good: The drift check and the rescan both read `corpusBuckets()`, so they count the same population the writers count. This is a better fix for A full-text index on a type that has subtypes reports a permanent BM25 "corpus counters diverged" warning and pays a full polymorphic type scan on the first search of every session, forever #10130 than special-casing the sub type name.
• Minor, `LSMTreeFullTextIndex.corpusBuckets()`:
  • It allocates a new `ArrayList` each call. That is only on the stale-check and rescan path, so it is fine.
  • The fallback branch, used when `typeIndex` is null, uses `getBuckets(true)`, which is polymorphic. The logical-index branch is not polymorphic: it includes only sub types that inherited the index. The two can disagree for a sub type that did not inherit the index. The Javadoc mentions this. Please confirm the fallback is only reachable transiently, before the wrapper is bound.
• Minor, `LocalDatabase.performRecovery`:
  • `requireRescan()` fires on every BM25 index after every unclean open. That costs a full scan on the first search, which is a real cost on large corpora. I think the trade-off is right, since it avoids a schema format change, and the INFO log makes it visible.
  • Because bucket indexes share one metadata instance, the loop calls it several times per logical index, and the `bm25Indexes` log count may overcount. If the count matters, dedupe by metadata instance or count only `TypeIndex` entries.

Concurrent commit retry (#10171)

• The design is clean: a dedicated `HttpSessionEndedException` plus an `answerForEndedSession` hook that defaults to null, so other routes keep the 404.
• Rethrowing it without rolling back is correct, since the transaction belongs to the request that ended the session.
• Race to verify: `wasCommittedBy` must be recorded before the session is unregistered or its lock released. Otherwise the waiter wakes, sees the session gone, and finds no commit record, so it answers 404 anyway. The IT covers the happy path, but please confirm the ordering in `HttpSessionManager` and that it holds on failure paths too.
• The OpenAPI text is updated and accurate. Minor: the "lock timeout error" status code is not stated.

Rerankers and `where` (#10170, #10169)

• `RecordPredicateRidSet`:
  • Memoising decisions in a `ConcurrentHashMap` and loading records outside the lock is good.
  • Evaluating the SQL condition under `synchronized(this)` is correct but serialises sparse fan-out workers on that predicate. If conditions are expensive, consider a per-thread context. Acceptable as is.
  • Refusing positional `?` is a sensible, safe choice.
  • Catching `SecurityException` and treating the record as "not accepted" avoids leaking existence. Good.
• Contract hazards:
  • `isEmpty()` is always false and `size()` is `Integer.MAX_VALUE` without a filter. The class Javadoc documents this. It is a fragile contract with `AbstractSet` (for example `equals`, `toString` and bulk ops), so a short test pinning that the indexes never call `iterator()`/`size()` on it without a filter would protect against future regressions.
  • A very selective predicate degrades to testing every indexed vector. This is documented, and the `filter` option remains the alternative.
• `carriedRecord()` correctly checks `rid.equals(document.getIdentity())`, so a user property named `record` is not misread.

Style and conventions (CLAUDE.md)

• Tests follow the `assertThat` style and the naming convention.
• Please check that no test asserts on raw wall-clock time (use `StallAwareStopwatch`), and that the IT waits on `hasWaitingRequests()` rather than sleeping.
• This PR bundles perf(vector): vector.fuse / vector.rerank stop loading every match twice #10181 and fix(#10164): commit naming an unknown or terminated session answers 404 #10172, which makes it large to review. Landing those first and rebasing would shrink the diff.

Test coverage

Coverage is good: 7 tests for the BM25 drift, 2 ITs for commit retry (verified red before the fix), 7 for the reranker carried record and 6 for `where`. Possible additions: a `where` test with a `filter` and an unreadable record, and a retry test where the first commit fails, expecting 404 rather than 204.

No blocking issues found. The main item I'd confirm is the commit-record ordering in the retry path.

🤖 Generated with Claude Code

Codecov Report

❌ Patch coverage is 73.07692% with 77 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.43%. Comparing base (80ef5d4) to head (9ca97a0).
⚠️ Report is 1 commits behind head on main.

@@             Coverage Diff              @@
##               main   #10185      +/-   ##
============================================
- Coverage     78.43%   78.43%   -0.01%     
+ Complexity     4652     4649       -3     
============================================
  Files          2249     2252       +3     
  Lines        238092   238386     +294     
  Branches      51540    51601      +61     
============================================
+ Hits         186745   186969     +224     
- Misses        32018    32062      +44     
- Partials      19329    19355      +26     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.