Monitor.register with the same query id twice in one call keeps both versions of the query

Registering a MonitorQuery whose id is already registered replaces the old query. When one Monitor.register call contains two versions of the same id, for example a query created and then edited in one batch, WritableQueryIndex.commitWithoutNotify deletes the id once and then adds every branch of both versions. Both stay in the query index, while the in-memory cache keeps one entry per cache id id_N, so the query runs a mix of the two versions. With q = apple OR banana then q = cherry, q matches {banana, cherry}, which neither version matches, and getDisjunctCount() is 3. On a persistent index the live Monitor, the reopened Monitor ({apple, banana}, the old version) and a read-only Monitor ({apple, banana, cherry}) disagree, and getQuery returns cherry. It is silent, needs only new Monitor(analyzer) with its default configuration for the in-memory case (the persistent views add an index path and a serializer), is reached through Lucene's own ConcurrentQueryLoader, lasts until the id is registered again on its own or deleted, and is in every release that ships the monitor module from 8.2.0 to 10.5.2, and on main.

• Monitor (lucene/monitor): stores queries under ids and reports which stored queries a document matches. It is backed by its own small Lucene index, the query index.
• Branches (disjuncts): QueryDecomposer splits a query into branches, one per clause of a disjunction. Each branch becomes one query-index document and one cache entry named id + "_" + ordinal (QueryCacheEntry.java:51-60): apple OR banana gives q_0 = apple and q_1 = banana. getDisjunctCount() counts these documents (Monitor.java:257-262).
• Matching: the document's terms select candidate query-index documents (the presearcher); for each candidate the Monitor looks its cache name up in the in-memory cache and runs that branch (WritableQueryIndex.java:380-386).

1. Monitor.register commits its input in chunks of up to queryUpdateBufferSize + 1 = 5001 entries (Monitor.java:152-162, MonitorConfiguration.java:35).
2. For one chunk, WritableQueryIndex.commitWithoutNotify collects the distinct ids, deletes each once, then adds every branch of every update and puts it in the cache by its cache name (WritableQueryIndex.java:113-125).
3. With two versions of q, the index keeps all 3 branch documents. Version 2's q_0 (cherry) overwrites version 1's q_0 (apple) in the cache; version 1's q_1 (banana) stays.
4. Live: "apple" selects version 1's q_0 document, whose slot now runs cherry: no match; "banana" runs q_1 = banana; "cherry" runs version 2's q_0. Result {banana, cherry}; 3 disjuncts.
5. Reopen: populateQueryCache rebuilds the cache from the first stored document of each id and skips the rest, as "a branch of a query that has already been reconstructed" (WritableQueryIndex.java:152-172). The first document the scan visits is version 1's doc 0 (in this fresh index, the order the documents were added; the code promises no "first update" rule), so every candidate runs version 1: {apple, banana}.
6. Read-only Monitor: each candidate document's own stored query is deserialized (ReadonlyQueryIndex.java:185-196), so both versions run: {apple, banana, cherry}.
7. getQuery keeps the stored query of the last document collected (QueryIndex.java:62-72), here version 2's doc 2: cherry.

With single-term versions (q = apple, then q = cherry) the live Monitor happens to be right ({cherry}), but after a reopen it runs apple only, and a read-only Monitor runs both. Main with -ea and -da, and 10.5.2; before 10.3.0 the recorded runs (8.2.0 to 10.2.2, JDK 25) give {apple, cherry} live: the branch order came from a HashSet and was not specified until #14885 (10.3.0) made it stable, so the surviving stale branch may differ on another JVM there.

• Trigger: two versions of one query id in the same register call and the same internal chunk of 5001 entries (default queryUpdateBufferSize 5000). Natural sources: an update stream or change log applied in batches (each register call commits and refreshes, WritableQueryIndex.java:127-128, so batching is the cheap way), or ConcurrentQueryLoader, whose worker passes everything it drained to one register call (ConcurrentQueryLoader.java:134-143): a one-thread loader fed a create and an edit reached the mixed state in 20 of 20 and 30 of 30 runs in independent checks (with several workers, both versions must be drained by the same worker into one call).
• Hidden boundary: the same id 5001 entries apart is handled correctly (executed: q, 5000 other ids, q, so 5002 entries; 5001 queries, 5001 disjuncts), so whether a repeated id breaks depends on a chunk boundary the caller cannot see.
• Default vs opt-in: the in-memory case uses new Monitor(analyzer), so the default MonitorConfiguration and TermFilteredPresearcher, assertions on or off. The persistent views need an index path and a MonitorQuerySerializer, the read-only view a read-only directory provider, and the metadata case a TermFilteredPresearcher built with a filter field (the default one has none, TermFilteredPresearcher.java:77-79). The monitor module itself is optional and less used.
• Scale: none; two entries suffice.
• Damage: silent wrong matches when the old version has a branch the new one lacks (a shrinking edit keeps matching what it removed) or a different filter-field value (the old value keeps selecting the query). When both versions have the same branches and metadata, the live Monitor happens to be right, and the damage shows as an inflated getDisjunctCount(), the old version running after a restart of a persistent Monitor, and both versions running in a read-only Monitor. getQuery names a version the reopened Monitor does not run.
• Bounds: separate calls are correct; deleteById removes every stale entry; a later single register repairs the id; purgeCache() does not; other ids in the batch are unaffected; repeating the batch does not add entries. No structural index corruption (the index stays readable; it holds stale query documents), hang or effect on IndexSearcher results was observed.
• How likely: rare (a judgement): it needs edits with repeated ids batched into one call; the two public downstream users found register one query per call (read only), and nobody has reported it since 8.2.0.

# Lucene main 645d4fe14a20, assertions on (-ea), seed DEADBEEF: 1 test(s) run, 1 failed
    => java.lang.AssertionError: q matches [banana, cherry], expected [apple, banana] or [cherry]
       org.junit.Assert.fail(Assert.java:89)
       org.junit.Assert.assertTrue(Assert.java:42)
       org.apache.lucene.monitor.TestRegisterDuplicateIdInOneCall.testOneBatchBehavesLikeOneOfTheVersions(TestRegisterDuplicateIdInOneCall.java:54)

• Contracts. Monitor class javadoc: "A Monitor contains a set of Query objects with associated IDs" (Monitor.java:38-41); getQuery: "@return the MonitorQuery stored for this id, or null if not found" (Monitor.java:245-255); register: "Add new queries to the monitor" (Monitor.java:147-151, :168-173). No uniqueness precondition in register, MonitorQuery (MonitorQuery.java:34-49), the package doc, CHANGES or MIGRATE.
• The chunk size is a buffer setting: setQueryUpdateBufferSize sets "how many queries will be buffered in memory before being committed to the queryindex" (MonitorConfiguration.java:150-160), yet it decides whether a repeated id breaks.
• Tests. TestMonitor#testUpdatesOverwriteOldQueries (TestMonitor.java:71-86) registers query1 as "this", then as "that" in a second call, and asserts that it matches a "that" document with one query run (it does not by itself show the old entry is gone, since "that" would not select it anyway); TestMonitorPersistence#testCacheIsRepopulated (TestMonitorPersistence.java:54-87) asserts that a reopened Monitor reports the same number of matches (4) and the same query count. No upstream test registers one id twice in one call.
• The code assumes it. The commit deletes the batch's ids before adding, so a commit is meant to replace each id's earlier documents (this supports the defect, not last-wins in particular); populateQueryCache assumes all documents of an id are branches of one query.
• History. The loop came with the module (LUCENE-8766, 2019) and moved unchanged in LUCENE-10422 (#679); neither discussion mentions repeated ids. Design context only, not a warning about this bug: in luwak#21 (2014) the author of the predecessor project Luwak explained why it keyed its query cache by the query string, not the id: "Just keying on the ID could result in weird results if a MonitorQuery is updated while a search is in progress - the presearcher could select a query based on it's current index, but then end up fetching an entirely different query from the updated QueryCache. Keying by the query string prevents this". That concern is an update racing a search. Luwak's QueryIndex.commit (the follow-up's copy) has the same delete-then-add-all loop with a hash-keyed cache, so by code reading it would also keep both versions' documents; only the slot overwrite comes from the Lucene module's id_ordinal key, used since its first commit.

The strongest case for "intended or misuse" (built by the devil's-advocate verifier; the GPT-5.6 reviews rated it moderate): register only says "Add new queries", replace-by-id is tested only across calls, so precedence inside one call is unspecified and callers should deduplicate; ConcurrentQueryLoader is documented as a startup loader ("useful to speed up startup times").

Why it fails: the precedence-free failure needs no rule about which entry wins, since {banana, cherry} is neither version and one persistent index gives three answers while getQuery names a fourth state; the commit loop deletes each id's earlier documents before adding; nothing rejects the input or documents a precondition; the library's own loader produces it (30 of 30 runs with one worker); and the outcome depends on an internal chunk boundary. At most it changes the fix (reject or last-wins), not whether today's state is a defect; last-wins is a proposal that matches consecutive calls, not an existing contract. Verdict: a defect.

One fix was built and validated in a scratch worktree at ea57360912ed; the reject alternative was run against the module's tests and a probe. Workaround in every affected release: deduplicate the batch before register (keep the last update per id) or register later edits in their own call; to repair an affected index, register each affected id again in its own call, or delete it.

Gives one call the meaning of consecutive calls, which the existing delete-then-add loop already assumes, and does not depend on the chunk boundary. Validation runs on every entry first, so an invalid earlier duplicate is still rejected (an earlier sketch deduplicated first and accepted it). Diffs: last-update-wins.diff, fix-with-test.diff (applies cleanly at ea57360912ed).

private List<Indexable> buildIndexables(List<MonitorQuery> updates) {
  // commitWithoutNotify deletes each id once and then adds every entry, so keep only the last
  // update of each id: a later update replaces an earlier one, as it does across commits
  Map<String, MonitorQuery> lastUpdates = new LinkedHashMap<>();
  for (MonitorQuery mq : updates) {
    if (serializer != null && mq.getQueryString() == null) {
      throw new IllegalArgumentException(
          "Cannot add a MonitorQuery with a null string representation to a non-ephemeral Monitor");
    }
    lastUpdates.put(mq.getId(), mq);
  }
  List<Indexable> indexables = new ArrayList<>();
  for (MonitorQuery mq : lastUpdates.values()) {
    // ... unchanged: serialize, decompose, one document per branch

Trade-offs

• Errors that only a dropped earlier entry would raise while it is serialized, decomposed or indexed are no longer raised, and serializer.serialize is not called for it; only the null-query-string check runs on every entry. Not tested.
• LinkedHashMap keeps each id at its first position, so surviving entries are indexed in first-occurrence order of their ids (read; no public result here depends on it).
• MonitorUpdateListener.afterUpdate still receives the full list, duplicates included (unchanged).
• Unchanged by the fix (read): a cache-only slot left by an earlier separate call (for example q_1 after a two-branch q is replaced by a one-branch q) stays cached until the normal purge; no index document refers to it, so matching is correct.
• An index written by an affected version keeps its extra documents until each affected id is registered again or deleted (a later single register repairs it, executed). Worth a CHANGES note.
• Not reviewed by a committer.

What the checks validated

• Stock: the doc's test fails 4 of 4 with assertions on and off; with the fix it passes 4 of 4 (gradle and the harness).
• The whole monitor module with the fix: 33 classes, 185 tests, 0 failures, 1 skipped, assertions on and off.
• :lucene:monitor:check -x test passes (google-java-format, ecjLint, forbiddenApis).
• Earlier hunt tests with the patched class shadowed, fresh seeds: the finder's 3, the independent reproducer's 6, the GPT-5.6 review stage's 2 (including its validation check) and the security probe's 4 (-ea, -da) pass. The devil's advocate's probes: 3 of 4 pass; the fourth asserts the stock state and fails because q now matches {cherry}; all its printed views, including ConcurrentQueryLoader, are {cherry} with 1 disjunct.

A Set of ids in buildIndexables throws on the second occurrence (option-reject.diff, +5).

Trade-offs

The check sees one chunk only, so behaviour still depends on the 5001-entry boundary; checking the whole call needs every id of the call in memory, and earlier chunks are already committed. ConcurrentQueryLoader's worker catches only IOException and InterruptedException, so the exception ends it and stays in an unread Future; by code reading, with one worker nothing queued after that is registered, and once every worker has died add() blocks forever when the bounded queue (2000 by default) is full (a pre-existing loader weakness this option routes repeated ids into; not executed).

What the checks validated

The 32 upstream monitor test classes pass (181 tests, 0 failures, 1 skipped), so no upstream test relies on duplicates in one call. Probe: a one-call duplicate throws and registers nothing; the same id 5001 entries apart (q, 5000 other ids, q: 5002 entries) is accepted; ConcurrentQueryLoader (1 thread) fed q, q, r: close() returns normally and no id at all is registered.

A javadoc line on both register overloads and on ConcurrentQueryLoader.

Trade-offs

Leaves the silent mixed state, and the chunk-dependent behaviour, for every caller who does not read it. Not recommended.

Method: code reading (the hunt's non-formal tier), in a hunt unit (monitor and presearcher, 2026-10-26).

• The finder was writing a TLA+ model of the WritableQueryIndex commit, purge and search protocol (QueryIndexConc.tla). Its header says cache ids are modelled as query ids and decomposition is not modelled, and it treats one update per commit, so the model does not witness this bug.
• Reading commitWithoutNotify for the model, the finder saw the delete-each-id-once-then-add-everything loop and wrote a public-API JUnit, which failed on the stock main jars and on 10.5.2 and 9.12.3.
• Then: a refute review by an independent reviewer from another model family (OpenAI GPT-5.6, reading only); an independent reproducer writing its own test from the claim alone; a GPT-5.6 "works as designed" review whose two side claims were executed; a devil's advocate; an adjudication; a release sweep, a prior-report search and a security screen; and this doc's own test and fix validation.