AnalyzingSuggester and FuzzySuggester: a failed rebuild with the other payload setting leaves the old suggestions decoded in the wrong format

AnalyzingSuggester.build() writes the new input's payload setting into the field hasPayloads right after its contexts check, before it reads any input (AnalyzingSuggester.java:394), before the input loop, the analysis, the offline sort and the FST compilation, all of which can throw; the new FST and count are published only at the end (:589-590). Lookups and store() read the field to decide whether each FST output holds text<U+001F>payload or just the text. So when a rebuild whose input has the other payload setting throws, the instance keeps the old FST but decodes it with the new flag: built from foo<TAB>10<TAB>p1, then a failed rebuild from a file without payloads, lookup("fo") returns the text foo<U+001F>p1 with payload null, and store()/load() keep it (shown by a separate public-API probe); the other direction returns empty text with foo as the payload, or trips the class's own assert sepIndex != -1 (:633). FuzzySuggester inherits build(). A lookup made while such a rebuild is still reading its input sees the same. Every release since payloads were added in 4.3.0 (LUCENE-4820, 2013); LUCENE-10292 (9.2.0) moved count next to the FST but left this flag. Fix: keep the flag in a local and assign the field next to fst and count.

• AnalyzingSuggester / FuzzySuggester: FST-based suggesters in the suggest module (FuzzySuggester extends AnalyzingSuggester and inherits build()). build(InputIterator) (or build(Dictionary)) builds the suggestions; lookup(key, ...) returns LookupResults with a text (key), a weight (value) and an optional payload; store() / load() save and restore the built state.
• Payloads: optional bytes per suggestion. An input says whether it has them with hasPayloads(). FileDictionary decides from the first line of the file ("In order to have payload enabled, the first entry has to have a payload"); DocumentDictionary from whether a payloadField is configured.
• How the FST stores them: with payloads, each FST output is text bytes, U+001F, payload bytes; without, just the text bytes. U+001F is reserved: build() rejects it in a payload-enabled text and lookup rejects it in keys.

1. Right after its contexts check (AnalyzingSuggester.java:390-392), build() assigns the field: hasPayloads = iterator.hasPayloads(); (:394).
2. Then it reads the input (:415-485: next() may throw IOException; the analysis may throw; an entry is rejected with IllegalArgumentException for U+001F with payloads on (:465-470), an over-long analyzed form (:424) or, with payloads on, an over-long surface form (:441), or with UnsupportedOperationException for a weight outside [0, Integer.MAX_VALUE] (:913-914)), sorts it through the temp directory (:490) and compiles the FST.
3. Only then are the new FST and count published: fst = ...; count = newCount; (:589-590). If anything above throws, fst and count still describe the previous build.
4. getLookupResult (:623-648) splits each output at U+001F only if (hasPayloads) and asserts the separator is there (assert sepIndex != -1, :633); sameSurfaceForm (:650-665) branches on the same field; store() writes the flag next to the FST (:608) and load() reads it back (:619).
5. Result: flag newly false: the old output foo<U+001F>p1 is returned whole as the text, payload null. Flag newly true: the old output foo has no separator; with assertions on lookup throws AssertionError, with them off the text is empty and foo is returned as the payload (the payload is copied from the start of the output's bytes, :638).
6. The flag is written before the input is read, so a lookup on another thread while such a rebuild is still in next() sees the same mixture without any failure.
7. A failed build can also raise maxAnalyzedPathsForOneInput (:484), as every build does (it is never reset in build()); it only sizes the lookup's top-N search queue (:786) and plays no part in decoding.

• Who is exposed. Applications that keep one AnalyzingSuggester or FuzzySuggester and rebuild it in place (an established pattern for successful rebuilds: a committer wrote on java-user@ in 2013 that build() "fully replaces all internal suggester state every time you call it"; the state after a failed build is not documented either way), whose input changes its payload setting between builds, and that keep serving the instance after that rebuild throws, or serve lookups while it runs.
• What goes wrong. Every suggestion of the instance, not some: all old outputs are decoded in the wrong format. Payload bytes appear inside the user-visible suggestion text behind U+001F, or the text is empty and the suggestion comes back as the payload. With assertions on, lookup throws AssertionError. store() writes the state to disk, so load() brings it back.
• Ordinary triggers. An IOException from the input (BytesRefIterator.next() declares it; for example a truncated file), or the suggester rejecting one bad entry (weight -1, U+001F in a payload-enabled text, a form over 32765 bytes). Users do hit build failures: #7074 (LUCENE-6012, open) reports build() failing on records over 32767 bytes.
• Default vs opt-in. Default constructors and settings, but one of the two builds must use the optional payload feature; both classes are @lucene.experimental. A suggester that never uses payloads cannot hit it.
• Scale. None needed: one entry per build, deterministic.
• Not affected. Rebuilds that keep the payload setting (the control passes), and every successful build, which repairs the state.
• How likely. Rare: the format change and the failure (or a concurrent lookup) must coincide, and the searches found no report since 4.3.0. A configuration change that reloads into a new instance avoids it.

# Lucene main 645d4fe14a20, assertions on (-ea), seed DEADBEEF: 1 test(s) run, 1 failed
    => org.junit.ComparisonFailure: expected:<foo[ payload=p1]> but was:<foo[<U+001F>p1 payload=null]>
       org.junit.Assert.assertEquals(Assert.java:117)
       org.junit.Assert.assertEquals(Assert.java:146)
       org.apache.lucene.search.suggest.analyzing.TestAnalyzingSuggesterFailedRebuild.testFailedRebuildWithoutPayloadsKeepsPayload(TestAnalyzingSuggesterFailedRebuild.java:46)

• The result is incoherent, not stale. It matches neither the previous build nor the attempted one. The previous build had payloads, where U+001F is the reserved internal separator (build() rejects it in a payload-enabled text, "this character is reserved", AnalyzingSuggester.java:465-470, and lookup rejects it in keys, :688-690), yet that separator and the payload come back inside the returned text. LookupResult documents key as "the key's text" and payload as "the key's payload (null if not present)" (Lookup.java:48-58); here the payload is returned as the text, or the text as the payload. The class's own invariant assert sepIndex != -1 (:633) fires.
• Late publication is the design. fst and count are assigned only after compilation (:589-590). LUCENE-10292 (CHANGES 9.2.0: "Fix other FST based suggesters so that getCount() returned results consistent with lookup() during concurrent build()") moved count there (that change was about concurrent readers and says nothing about failed builds, but it shows the intended order); in that issue (#11328) the author described build() as using temporary data structures until its logic is complete and then replacing the ones lookup() uses. hasPayloads is the derived field that move missed.
• Upstream tests the property during a rebuild. SuggestRebuildTestUtil.java:78: "at every stage of the slow rebuild, we should still be able to get our original suggestions", run for AnalyzingSuggester by TestAnalyzingSuggester#testLookupsDuringReBuild (:173-177), during a successful rebuild with no payloads in either build. No upstream test rebuilds with another payload setting or looks up after a failed build.
• Reuse is an established pattern. Lookup#build "Builds up a new internal Lookup representation" (Lookup.java:179-182); a committer said on java-user@ (2013) that build() "fully replaces all internal suggester state every time you call it". Both are about successful rebuilds; nothing documents the state after a failed one.
• Ordinary failure paths. FileDictionary documents both line formats and takes the payload setting from the first line, and every line the failing stream delivers is well-formed; next() declares IOException; the other triggers are entries the suggester's own validation rejects on purpose. A rejected input should not corrupt the instance the suggester already has.
• History. The early assignment came with payload support itself (LUCENE-4820, 2013) and was later only rewritten in place (LUCENE-5260); no commit, issue or test found in the searches discusses it.

The checkers built the strongest case for "intended": Lookup and AnalyzingSuggester are @lucene.experimental and document no failure atomicity; sibling lookups (TSTLookup, FreeTextSuggester) also change state before their fallible steps; so a maintainer could answer "after build() throws, rebuild or discard the instance". For the during-rebuild variant, committers said in 2022 (#11328 and the dev@ thread on it) that concurrent build and lookup are not guaranteed or documented.

Why it fails: the result is not stale or empty (either would be acceptable); it matches neither the previous build nor the attempted one, violates the class's own assertion and reserved-character rule, and store() persists it. The fix costs nothing and follows the order the class already uses for fst and count. Failure atomicity is not a documented contract; the case rests on the class's own invariants and its existing publication order. The concurrency objection only weakens the secondary variant; the failed-build path is single-threaded. These checkers judged it real: an independent reproducer, a cross-model intended-behaviour review, a devil's advocate and an adjudication; the cross-model review rated the intended case moderate and outweighed. The two read-only cross-model refute reviews returned a procedural "not real" (see Independent checks).

Option A was applied in a scratch worktree of main a9779ef3c193 and run through gradle with assertions on and off; options B to D were not run.

Replace the field write at AnalyzingSuggester.java:394 with a local of the same name and assign the field right after fst and count (:589-590); +3/-1 lines, one of them a comment.

The local shadows the field, so every use inside build() (:397, :439, :465, :532, :576) reads the new input's setting and nothing else changes; no helper called from build() reads the field. If reviewers dislike the shadowing, name the local newHasPayloads and update those five uses. FuzzySuggester is fixed with it.

-    hasPayloads = iterator.hasPayloads();
+    // published with the new FST below, so a failed build keeps the flag matching the old FST
+    final boolean hasPayloads = iterator.hasPayloads();
 ...
       fst = FST.fromFSTReader(fstCompiler.compile(), fstCompiler.getFSTReader());
       count = newCount;
+      this.hasPayloads = hasPayloads;

Trade-offs

• None for single-threaded use; no API or format change.
• Concurrent readers still get no safe publication: fst, count and hasPayloads are written separately, only count is volatile, and lookup() reads fst and hasPayloads, both plain. A lookup that races the end of build() can still see the new FST with the old flag (the Java memory model does not confine this to one instant), as getCount() and lookup() can already disagree today. What closes is the long, deterministic window: the whole input read, sort and compilation.

What the checks validated

• The new test class with every org.apache.lucene.search.suggest.analyzing.* suite: stock 94 tests, 4 failed: the 4 new bug cases, and 1 skipped; the new control passes and none of the 89 upstream tests fails (88 pass, 1 skipped); with the fix 94 tests, 0 failed, 1 skipped, assertions on and off.
• all of :lucene:suggest:test with the fix: 32 classes, 241 tests, 0 failed (7 skipped), assertions on and off.
• :lucene:suggest:check -x test passes with the fix (google-java-format, ecjLint, forbiddenApis; error-prone is skipped in the default build).
• Through the repro harness: the new test 5 of 5 pass with the finder's equivalent fix shadowed (-ea and -da). Earlier, the finder's and the reproducer's JUnits passed with it, and the exhaustive fault-injection harness (run with assertions off) reported no mixture for either suggester (only the expected temp files left by its injected delete failure).
• The comment line was reworded after these runs to say exactly what the change keeps (the flag; a failed build can still raise maxAnalyzedPathsForOneInput). The code lines are the validated ones; fix.diff and fix-and-test.diff apply cleanly to a9779ef3c193, and google-java-format 1.37.0 leaves the patched file unchanged.

Hold fst, count, hasPayloads and maxAnalyzedPathsForOneInput in one final object, published with a single (volatile) write at the end of build() and in load(); lookup, getCount, store and ramBytesUsed read one snapshot. In the spirit of LUCENE-4492 (#5558, "Immutable Suggester impls. should be immutable", open).

Trade-offs

Also gives concurrent readers one consistent snapshot, but a larger diff that touches every reader of the four fields; no behaviour change for single-threaded callers.

Clear fst and count if build() throws, so lookups return nothing.

Trade-offs

Coherent, but it throws away the previous suggestions, which the class keeps today when a same-setting rebuild fails (the control).

Say in the Lookup#build javadoc that callers must discard the instance after build() throws.

Trade-offs

The state stays corrupt and store() still persists it; it leaves a trap in the established in-place rebuild pattern and goes against the class's own late publication of fst and count.

• Lead (code reading). The unit plan noted that AnalyzingSuggester.java:394 writes the flag before the input loop, sort and compilation, while :589-590 publish fst and count only after them, so a mixture needs a failed rebuild whose input differs in hasPayloads().
• Witness (semi-formal): bounded-exhaustive fault-injection translation validation on the stock jar, over five Lookups: a first build from every dictionary of up to 2 entries and a second build from every dictionary of up to 3 entries over a 4-term alphabet (with a 4-byte and a 3-byte UTF-8 letter), with and without payloads, the second build failing at every injected point: an input IOException or RuntimeException after k entries, an analyzer exception, temp-directory create/open/delete failures, an input with contexts, and invalid entries (weight -1 or 2^31, U+001F in a payload-enabled text, a 40,000-character term, a 40,000-byte payload).
• Oracle. Afterwards the lookups and getCount must match a fresh instance built from the previous input, a never-built instance, or, only if the second build did not throw, a fresh build of the new input; store()+load() must then match the same reference; and a third, successful build from the previous input must match a fresh build of it.
• Signal. 7,220 of 19,470 checks each for AnalyzingSuggester and FuzzySuggester showed the mixture, with the same totals with assertions on and off (for FuzzySuggester with assertions on only the summary totals were kept), for every injected failure that throws after the flag write (all kinds except the contexts input, which is rejected before line 394; the U+001F-surface and long-payload entries are not invalid without payloads), and only when the payload settings differ. With fix A (run with assertions off) none: the 660 remaining reports are the expected temp files left by the injected delete failure. On stock no third build differed from a fresh one, so a successful build repairs the state. The finder then wrote the public-API JUnit and the fix, and independent checkers followed.