Suggest Lookup.load(): a load that throws leaves the lookup half-loaded, with the new count and part of the new model

The six suggest Lookups that implement store()/load() (TSTLookup, AnalyzingSuggester, FuzzySuggester, FreeTextSuggester, FSTCompletionLookup, WFSTCompletionLookup) can save their model to a stream and load it back; the infix suggesters' store()/load() just return false, so they are not affected. Lookup.load is documented as "Discard current lookup data and load it from a previously saved copy" and throws IOException "when fatal IO error occurs" (Lookup.java:246-252 on main 645d4fe14a20); it says nothing about the state after a throw. Each of the five load() methods (FuzzySuggester inherits one) assigns its fields one after another while it reads: TSTLookup.load sets count, replaces the tree with a new empty root and only then reads the tree into that live root (TSTLookup.java:276-278). So a load() that throws part-way (a stored copy cut short, an IOException while a valid copy is read, or FreeTextSuggester rejecting a model saved by a FreeTextSuggester with other grams or separator settings) leaves a mix of the old or initial state and the new model. A new TSTLookup loading a 2-word copy that is 1 byte short reports getCount() 2 instead of 0, suggests a/1 and throws NullPointerException for "b"; for most other classes only getCount() is wrong, with narrower payload and score mixes and a narrow window in which a new AnalyzingSuggester or FuzzySuggester throws NoSuchElementException from some lookups. TSTLookup has done this since its first load() (SOLR-2375, 3.1.0, 2011), all six classes since LUCENE-5404 (4.7.0, 2014). Reading into locals and assigning at the end fixes it (5 methods, +30/-16).

• TSTLookup: a suggester on a ternary search tree. Each node (TernaryTreeNode.java:28-48) holds one character (splitchar), an optional token and val (the word and its weight) and up to three children. getCount() is "the number of entries the lookup was built with" (Lookup.java:172-176).
• Stored copy: store() writes count as a VLong and then the tree in pre-order: per node its character, a flag byte, then the token and an 8-byte weight when the node has them (TSTLookup.java:241-272). For {a, b} that is 30 bytes; the last byte belongs to b's weight.

1. load(InputStream) wraps the stream, calls load(DataInput) and closes the stream in a finally block, also when the load fails (Lookup.java:150-157).
2. TSTLookup.load (TSTLookup.java:275-280): count = input.readVLong(); (:276), root = new TernaryTreeNode(); (:277), readRecursively(input, root); (:278). The count and the new, empty root are live before a single node is read.
3. readRecursively (:218-239) reads a node's character, flags, token and weight, then allocates each child and recurses. With the copy 1 byte short, readLong() for b's weight (:225) hits the end: EOFException. Node b exists with its token but val == null.
4. lookup() collects every node with a token (TSTAutocomplete.java:97-148) and unboxes its weight, ((Number) ttn.val).longValue() (TSTLookup.java:199, :205): "a" gives a/1, "b" throws NullPointerException.
5. The other load() methods assign in stream order too: AnalyzingSuggester.java:614-619 (count, fst, maxAnalyzedPathsForOneInput, hasPayloads; FuzzySuggester inherits it), FreeTextSuggester.java:363-379 (count before its own separator and grams checks, totTokens before the FST), FSTCompletionLookup.java:304-308 and WFSTCompletionLookup.java:145-146 (count before the FST). In a new AnalyzingSuggester or FuzzySuggester whose stream ends or fails right after the FST, the new FST is live while maxAnalyzedPathsForOneInput is still 0, and some lookups throw NoSuchElementException. Nothing restores a field on the exception path.

Every cut stream is rejected (0 silent loads in the hunt's 6,210-check exhaustive run): the claim is failure atomicity, not acceptance of damaged data.

• Who is hit. Code that keeps using a Lookup after its load() threw, including a new instance it meant to swap in. Solr's SolrSuggester.init loads the stored file into the lookup it will serve, catches the IOException, logs "Loading stored lookup data failed, possibly not cached yet" and continues with that instance (it catches only IOException, so this does not cover FreeText's IllegalStateException); its build() stores straight into the target file, so a store cut short by a crash leaves exactly this input for the next start.
• Default or opt-in. Opt-in: the suggest module's Lookups, with default settings (TSTLookup and FSTCompletionLookup have no-argument constructors; the others take an analyzer and/or a temporary directory). Lookup and five of the six classes are @lucene.experimental (TSTLookup carries no tag).
• Effect. After an exception the caller saw: TSTLookup serves part of the new tree and some lookups throw NullPointerException, also in a new instance. For FSTCompletionLookup, WFSTCompletionLookup, a new FreeTextSuggester, the FreeText rejected-model case and most Analyzing/Fuzzy cut points only getCount() is wrong. Analyzing/Fuzzy cut in their last 1-2 bytes after a model with the other payload setting glue the payload to the key or return the key as the payload (an AssertionError with -ea). A new Analyzing/Fuzzy instance whose stream ends or fails right after the FST throws NoSuchElementException from some lookups. FreeTextSuggester over a built model scores the old FST with the new token total.
• Scale. None needed: 2 entries, one thread, assertions on or off.
• How likely: rare. It needs a load() that throws and continued use of that instance; upstream tests load only complete copies, and the recorded search found no report since 2011.

TestTSTLookupFailedLoad (46 lines with the license header, one test method, package org.apache.lucene.search.suggest). Public API only: TSTLookup, PlainTextDictionary, ByteBuffersDirectory, store(OutputStream), load(InputStream), getCount(). Deterministic: fixed input, an in-memory directory, one thread. The test asserts only the count, the smallest stable symptom; the NullPointerException from lookup("b") in the same state comes from a separate evidence probe (a main() that prints the state), not from the test.

Use the same test as the regression test of a fix, as its own class or as one method in TestPersistence, with its one comment line wrapped to google-java-format's 100 columns.

The hunt's larger evidence, not part of the repro: an 8-test JUnit over all six classes (cut copies, a stream that fails part-way, the FreeText rejected model), an independent 7-test JUnit over every failure point, and a bounded-exhaustive fault-injection check (6,210 checks per assertion mode).

cp $REPRO/TestTSTLookupFailedLoad.java $LUCENE/lucene/suggest/src/test/org/apache/lucene/search/suggest/
cd $LUCENE && ./gradlew :lucene:suggest:test --tests TestTSTLookupFailedLoad -Ptests.seed=DEADBEEF -Ptests.asserts=true

TestTSTLookupFailedLoad > testFailedLoadIntoNewLookup FAILED
    java.lang.AssertionError: getCount() after a failed load() expected:<0> but was:<2>

# the same message through the standalone runner on main (assertions on and off) and on 10.5.2 and 9.12.3
# with the fix (gradle, assertions on and off): 1 test, 0 failures

# evidence probe of the same state (not the repro)
built:          getCount()=2 | lookup("a") -> [a/1] | lookup("b") -> [b/1]
stored copy:    30 bytes
loaded whole:   getCount()=2 | lookup("a") -> [a/1] | lookup("b") -> [b/1]
new TSTLookup:  getCount()=0 | lookup("a") -> [] | lookup("b") -> []
load(cut 29 bytes) threw java.io.EOFException
after the throw: getCount()=2 | lookup("a") -> [a/1] | lookup("b") -> threw java.lang.NullPointerException

• A new instance is left half-loaded too. #5558 (LUCENE-4492, open) describes loading a new instance and swapping it in as today's way to reload a suggester (the issue is about safe publication of that swap). In the repro that instance ends with getCount() 2, one suggestion and a NullPointerException: neither its previous (empty) state nor the stored model.
• The trigger is in contract. An IOException while a valid copy is read is what load() declares (Lookup.java:250). These classes have no closed or unusable state.
• FreeTextSuggester's validation is out of order. Its first load() (LUCENE-5214, b50a2edbb23, 2013) checked separator and grams before assigning any field; LUCENE-5404 (b5926da2d67, 2014) put count = input.readVLong() ahead of those checks (FreeTextSuggester.java:363), so a model it rejects with its own IllegalStateException still changes getCount().
• Lucene reloads in place itself. TestAnalyzingSuggester calls suggester.load(is) on the instance it just built and stored (TestAnalyzingSuggester.java:1048, :1099, :1147, :1187). The module's 6 load() calls in tests all read complete copies, so no test asserts or contradicts any post-failure state.
• The build side was fixed as a bug. LUCENE-10292 (#11328, 9.2.0, "Bug Fixes"): "Fix other FST based suggesters so that getCount() returned results consistent with lookup() during concurrent build()" (lucene/CHANGES.txt:4289-4291). load() was not touched.
• Lucene's newer suggester already loads this way. NRTSuggester.load (the document suggester, NRTSuggester.java:309-327) reads every field into locals and constructs the object at the end, and CompletionsTermsReader.suggester() assigns it in one step (:64-69): the shape of the recommended fix.

The intended-behaviour checker, a reviewer from another model family (OpenAI GPT-5.6) and the adjudication built the strongest case: Lookup.load promises nothing after it throws, and "fatal IO error" can be read as "the instance is unusable afterwards"; Lookup is @lucene.experimental; a cut stream is not "a previously saved copy", a trigram model is not a copy for a bigram suggester, and every damaged stream is detected; #5558 describes a new instance and a swap; on #6467 (LUCENE-5404) McCandless (they) called loading a previously built suggester and then building a new one over it "strange", and wrote about the repro's own pattern: "it's sort of weird to 1) create a new suggester class, and 2) call its load API; it's more natural to create the suggester, passing in a Dir/File where it should load its state from"; on #11328 dweiss (they) said the Lookups "seemed to be designed with a single-thread model in mind"; and for most classes only getCount() is wrong.

Why it fails: the classes define no unusable state, and the first consumer of TSTLookup.load (Solr's Suggester.init, added by the same SOLR-2375 commit) caught the exception and kept the instance; the "not a saved copy" point does not cover an IOException while a valid copy is read, and the FreeText rejection is the suggester's own check; the new-instance pattern is the repro, and that instance is not left empty; Lucene's own tests reload in place. The "sort of weird" remark is an API-design opinion, and the API as shipped is built for construct-then-load: FSTCompletionLookup's no-argument constructor is documented "This constructor should only be used to read a previously saved suggester." (FSTCompletionLookup.java:105), TestPersistence (:86-87) and TestFreeTextSuggester (:90-91) load into a new instance, and so does Solr's SolrSuggester.init. The count-only point is true and is why the priority is low, but TSTLookup throws from lookup(). What remains supports a low priority and possibly an "improvement" label, not "works as designed".

One fix was built and validated; four alternatives were considered and not built.

Five load() methods (TSTLookup, AnalyzingSuggester, FreeTextSuggester, FSTCompletionLookup, WFSTCompletionLookup) read into locals and assign the fields at the end; FuzzySuggester inherits it. In FreeTextSuggester this also puts the separator and grams checks back before any assignment. Lucene's document suggester already loads this way (NRTSuggester.load). The TSTLookup part:

   public synchronized boolean load(DataInput input) throws IOException {
-    count = input.readVLong();
-    root = new TernaryTreeNode();
-    readRecursively(input, root);
+    long newCount = input.readVLong();
+    TernaryTreeNode newRoot = new TernaryTreeNode();
+    readRecursively(input, newRoot);
+    root = newRoot;
+    count = newCount;
     return true;
   }

Trade-offs

• Several plain field writes at the end, as the build side does since LUCENE-10292: not one atomic snapshot for a thread that looks up while load() publishes.
• TSTLookup keeps the old tree until the new one is read, so peak memory during a load is old + new tree (today the old tree is dropped first); the FST classes already hold the old FST until their assignment.
• No stored-format change and no cost on the lookup path.

What the checks validated

• Scratch worktree of 645d4fe14a20, gradle, OpenJDK 25: the repro test fails on the stock tree and passes with the fix, with assertions on and off.
• With the fix and the test, the whole lucene/suggest module passes with assertions on and off: 32 suites, 237 tests, 7 skipped, 0 failures (TestPersistence, TestAnalyzingSuggester, TestFuzzySuggester, TestFreeTextSuggester, TestFSTCompletion, TestWFSTCompletion included).
• :lucene:suggest:check -x test flagged one line only, the test's 101-character comment (google-java-format); with it wrapped onto two lines (no code change) check passes, so the five fixed sources are clean. Wrap that comment before the test goes into a PR; the assertions-off module run used the wrapped copy.
• Earlier, the finder's 8 tests and an independent reproducer's 7 tests passed with the same change, and the exhaustive fault-injection check found 0 mixed states in 6,210 checks.
• An independent fact check repeated the stock and fixed gradle runs, the module tests and the check in its own scratch worktree, with the same results.

The same change in TSTLookup alone. Not built.

Trade-offs

Removes the broadest throwing symptom, TSTLookup's NullPointerException at most failure points, but leaves the narrow window in which a new AnalyzingSuggester or FuzzySuggester throws NoSuchElementException from some lookups, and the count-only and the score and payload mixes of the other five classes.

Catch, clear every field, rethrow. Not built.

Trade-offs

Matches "Discard current lookup data" literally, but drops the previous model a caller may still want and needs a per-class reset that is easy to get wrong.

Javadoc only. Not recommended alone.

Trade-offs

Changes no behaviour: a caller that swaps in a new instance already discards it, and one that does not still serves the half-loaded state. A maintainer may still prefer it: #16951 (open, 2026-10-29) proposes the same kind of answer one level down, a javadoc saying that an IndexInput is in an undetermined state after any IOException while reading.

#6493 (LUCENE-5430, open) proposes a header check before loading; a checksum footer would also catch a cut copy. Not built.

Trade-offs

Does not cover an IOException in the middle of a valid copy, and FreeTextSuggester already checks its header yet assigns count early, so it would sit on top of the recommended fix, not replace it.

Method: non-formal first signal, then semi-formal: a planning probe followed by a bounded-exhaustive fault-injection check (the hunt's hunt unit for suggest lookups whose failed build or load leaves mixed state, 2026-10-28, main a9779ef3c193).

• Lead. A planning probe for exception safety loaded a stored model cut at every prefix length into a built instance on the stock jars (assertions on and off): all 7 Lookup configurations were left mixed (for example TSTLookup 68 of 69 prefixes, AnalyzingSuggester 87 of 88); the same on the branch_10_6 RC jars.
• Exhaustive check. A translation-validation harness on the stock jar: 6 Lookups x payload flags x previous state (loaded, built, new) x every cut length and an IOException at every byte; 6,210 checks per assertion mode, the oracle accepting the previous or the empty state: 78 violating configurations, 0 silent loads, 0 violations with the fix.
• JUnit. Reduced to an 8-test public-API JUnit; the hunt's repro-minimisation step reduced it further to the one test above.