Skip to content

Bound the seismic build's memory by batching the term space - #45

Merged
zirui-song-18 merged 15 commits into
opensearch-project:mainfrom
chishui:seismic-batched-build
Sep 3, 2026
Merged

Bound the seismic build's memory by batching the term space#45
zirui-song-18 merged 15 commits into
opensearch-project:mainfrom
chishui:seismic-batched-build

Conversation

@chishui

@chishui chishui commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Fixes #44.

The build runs one term window at a time; for_each_clustered_window is the only place that work lives, so all four index types get it.

Two knobs on the BatchClusteringOption #25 added and never wired up:

  • inverted_list_batch_size=N — bounds the inverted-list intermediate.
  • batch_file_output_path=P — with N > 1, bounds the clustered lists too; build() still ends holding a usable index.

seismic/seismic_sq end their payload with their lists, so each window streams into P. The disk pair's forward index needs every list's doc ids, so they spill to P.lists and write the payload from that.

base_full (1.12B nnz), mapped corpus, peak RssAnon:

windows seismic λ=6000 disk_seismic λ=600
1 10281 MB 8671 MB
10 2816 MB 1440 MB
100 465 MB 434 MB

Byte-identical to build() + write_index at a fixed seed.

The index header is written centrally, by write_index, so an index type never
has to know its layout. A writer that streams a payload out without an Index
object -- the term-batched Seismic build in the next commit -- has no way in.
Moving write_header from the anonymous namespace into detail keeps one
definition of the layout rather than a second copy that can drift from
read_header.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
A whole-corpus build holds two intermediates that scale with the corpus's
non-zeros -- the inverted lists, then the clustered posting lists -- so peak
memory scales with the corpus, and a corpus whose posting lists do not fit in RAM
cannot be indexed at all.

The build now runs one contiguous term window at a time. for_each_clustered_window
in seismic_common.cpp is the single place that work lives, and every index type in
the family already reaches it: build_inverted_lists_clusters becomes a thin
wrapper whose sink appends into one vector, so the batched and unbatched paths
cannot drift. It takes the element width from SparseVectorsConfig, so a quantizing
index gets the same treatment as a float one -- add() has already encoded the
values by the time they arrive, and the previous float-only restriction was
gratuitous.

Two knobs, both on the BatchClusteringOption that opensearch-project#25 added and never wired up,
so this needs no new API and reaches Python through the factory description
unchanged:

  inverted_list_batch_size=N   build in N windows; bounds the inverted-list
                               intermediate. seismic, seismic_sq, disk_seismic
                               and disk_seismic_sq all get this from build().
  batch_file_output_path=P     also serialize each window to P and free it, so
                               the clustered lists are never all resident either.
                               The index becomes the file: nothing is retained.

The streaming write is write_seismic_index_batched, parameterized by the header
and a prefix writer, which is all that differs between SEIS and SESQ. The
disk-resident types keep their existing write for now: their payload interleaves
summaries with an inline forward index derived from the same clusters, so one of
those sections has to be held or spooled while the other streams.

Folded the duplicate inverted_list_batch_size / char* batch_file_output_path
fields on SeismicClusterParameters into the typed BatchClusteringOption, which
removes a raw char* lifetime hazard from a parameters struct. That makes
kDefaultSeismicClusterParams const rather than constexpr, the string not being a
literal type.

Measured on msmarco base_full (8.8M docs, dim 30109, 1.12B non-zeros,
lambda=6000 beta=400 alpha=0.4) on a 36-core/68GB host, corpus on the heap in
every row so the only difference is the build:

  windows    peak RSS    build time
  1 (whole)  24091 MB    105 s
  2          19300 MB    105 s
  10         10524 MB    107 s
  20          9284 MB    118 s
  50          7917 MB    164 s
  100         7425 MB    238 s

Ten windows is 2.3x less peak memory for the same build time. Past that memory
keeps falling but each window is another pass over the corpus, and by 100 those
passes have more than doubled the build. The floor is the 6.7GB corpus, which
read_csr can map instead of copying -- residency is SparseVectors' business, and
the build is indifferent to it.

Routing the ordinary build through the shared producer costs nothing: the
whole-corpus row reproduces the pre-refactor 24085 MB / 106 s, and the 10-window
row the pre-refactor 10516 MB / 103 s. The 2/20/50/100 rows are from the fuller
pre-refactor sweep, whose endpoints those two reproduce. A single window streamed
to a file rather than retained costs 24085 MB / 120 s -- the same memory, plus
15 s to put 14.9GB on disk.

Query performance does not move, because the index is the same index: two
independent unseeded builds, whole-corpus against 10 windows, over 6980 msmarco
dev queries at k=10, gave 69332 vs 70198 QPS, p50 0.290 vs 0.296 ms, p99 1.000 vs
1.055 ms, recall@10 0.8406 vs 0.8432.

Sameness is asserted, not assumed. At a fixed seed a streamed build is compared
against build() + write_index as files, for both a float and a quantizing index,
and across window counts; batch_size alone is compared the same way for a float,
a quantizing and a disk-resident index. That holds because each list's k-means
seed comes from its own GLOBAL term id rather than from the window it landed in or
the order the threads reached it, and lambda/beta are resolved once from the whole
corpus. It also needs each list's doc ids to arrive ascending, which is why the
fill walks documents serially: threading it would reorder them, and pruning sorts
by value with a non-stable sort.

Two smaller things the window structure needed. An up-front counting pass gives
the exact postings-per-term, which sizes every window's lists so none of them
grows and set_entries can adopt whole buffers instead of add_entry locking per
posting; it also range-checks the terms, which the mapped read does not. And the
cluster loop's OpenMP chunk is now derived from the window width rather than fixed
at 64, because a narrow window would otherwise be handed out as two chunks and
leave every thread but two idle -- that made 64 windows 3.5x slower than 16.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
google-benchmark measures throughput, and a memory high-water mark is only clean
in a process that has built nothing else, so this is a standalone driver that
runs one configuration per invocation and reports VmHWM plus wall time. It covers
the whole-corpus build, the batched build at any window count, and the
interchange -> native CSR conversion a mapped corpus needs.

Two things it has to get right to be worth trusting.

The corpus residency is a flag, not a mode. "batched inmem" streams the corpus
onto the heap through the same streaming_add the baseline uses, so a comparison
against the baseline isolates the batching. "batched mmap" has read_csr borrow a
native CSR instead, which is cheaper by the whole size of the corpus -- a real
saving, but one that comes from the residency, and reporting it against the
baseline would credit batching with it.

VmHWM is reset at the start of the build. Loading the corpus costs more than
holding it, because a streaming ingest stages a second copy of it: on msmarco
base_full the loader peaks at 13.0GB while the build at 100 windows peaks at
7.4GB. A whole-process high-water mark would report the loader for every
configuration below that and hide the batching entirely. The loader's peak is
reported alongside rather than dropped.

This is what produced the numbers in the previous commit.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Covers what batching is for, that it is a build option in the factory
description rather than a separate entry point, what each of the two knobs
bounds, and how to measure a build with the peak-RSS driver.

The guidance is the measured base_full numbers rather than the mechanism: where
the memory/time sweet spot actually is, that query latency and recall do not
move, and why the reported peak excludes corpus loading.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Peak RSS alone cannot answer the question the batching is for. It counts
anonymous pages the process allocated together with file pages it merely touched
of a mapping, and only the first are the process's to keep -- the kernel can
reclaim the second under pressure. So an RSS-only measurement of a build over a
mapped corpus reports memory the build does not really own.

On msmarco base_full at 10 windows, the same build over the same corpus:

  corpus residency   peak RSS    peak RssAnon   peak RssFile   build
  heap               10533 MB    10528 MB          4 MB        106 s
  mapped             10536 MB     4082 MB       6454 MB        123 s

Total RSS is identical, which is why mapping the corpus looked pointless in the
earlier numbers. What actually happens is that the 6.45GB corpus moves out of
anonymous memory into the page cache, and the memory the process is responsible
for falls to 4.1GB. Against the whole-corpus heap build's 24095 MB of anon, that
is a 5.9x reduction, for 17 s of extra page faults -- and it is invisible without
the split.

RssAnon has no kernel peak counter: /proc/self/status reports it as a current
value and only VmHWM as a peak, so a sampling thread tracks the maxima of RssAnon
and RssFile for the duration of the build. That makes those two lower bounds -- a
spike shorter than the 50ms interval is missed, and they can disagree with VmHWM
by a hair since VmHWM is itself only updated at certain fault paths. The interval
is small next to a build that runs for minutes and allocates in per-window steps,
so in practice they track the real peaks. Reported as such rather than as if they
were counters.

The sampler reads /proc/self/status into a stack buffer rather than through
iostreams, because it runs thousands of times while the thing being measured is
memory, and a per-sample allocation would show up in its own number.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Term frequencies are heavily skewed -- on msmarco base_full the heaviest term
holds 5.7M postings against a mean of 37K, and the top 1% of terms hold 19% of
them -- and peak memory is set by the largest window, not the average one. Equal
width therefore wastes most of what batching could save. The counting pass
already has the exact per-term counts, so the cut points can be chosen from them.

What to even out is min(count, lambda), not count. Pruning keeps at most lambda
doc ids per term before clustering, which on base_full is 115M of 1121M postings:
90% of them never reach the phase that dominates the peak, and a term with 5.7M
postings costs no more there than one with 6000. Measured imbalance of that load
at 10 windows, largest window over the mean:

  equal width                 1.53
  balanced by raw counts      3.31
  balanced by min(count, l)   1.00

Balancing raw counts is worse than doing nothing, which is not obvious and is why
this is measured rather than reasoned: it packs the heavy terms into narrow
windows and leaves the others holding thousands of light ones, which both
unbalances the phase that matters and starves the per-window parallel loop.
Measured that way, build time at 20 windows went from 118s to 128s and at 100
from 267s to 305s, for a 5% memory saving.

Weighted correctly, on base_full with the corpus mapped so the figure is the
build's own memory rather than the corpus:

  windows   peak RssAnon   build time
  whole      24095 MB       105 s
  10          3265 MB       109 s
  20          2148 MB       127 s
  100          734 MB       286 s

Anonymous memory now falls roughly as 1/N, which it did not before: at 10 windows
it is 3265 MB against 4082 MB for equal width, a 20% improvement, and at 100 the
build allocates under 1GB while indexing 1.12 billion postings. Build time did not
regress -- 109s against 123s for equal width at 10 windows.

Windows stay contiguous and ascending, which is what the streaming write needs:
the layout carries no per-list offsets, so a list's position in the file is its
term order, and a window's lists can only be appended once every earlier term is
written. Grouping by a hash of the term (term % batches) balances comparably --
1.03x at 10 windows -- but scatters each window's terms across the file, so it
cannot be streamed without buffering completed lists or adding an offset table. It
is also worse where the skew bites hardest, 1.64x at 100 windows against 1.07x,
because a single term heavier than the target dominates whichever window it lands
in.

The output is unchanged, as the existing byte-equality tests assert: which window
a term lands in cannot affect it. A new test covers the case the weighting
creates, a term heavier than a whole window's target, over window counts from 1 to
more than the dimension -- every term must still land in exactly one window, or
the streamed file would come out short and the write would refuse it.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
A window has two memory peaks and the split has to weigh both. Filling it holds
every posting of its terms at 8 bytes each; clustering it holds what survives
pruning -- min(count, lambda) per term, 10% of base_full's postings -- as clusters
and summaries, which come to 14.9GB for 115M pruned postings, about 16x bulkier
per posting.

Weighting either phase alone unbalances the other. min(count, lambda) balances the
clustering exactly but concentrates the heavy terms, leaving one window holding
3.7x the mean raw postings, and that window's fill then becomes the peak -- which
is what the previous commit's numbers were actually measuring. Raw counts do the
reverse. Weighting their sum at the relative cost balances what is resident.
Predicted peak of the largest window at 10 windows on base_full:

  equal width          2.76GB
  raw count            4.93GB
  min(count, lambda)   3.32GB
  both, as here        2.07GB

Measured, corpus mapped so the figure is the build's own memory:

  windows   peak RssAnon   was (min(count,l))   build time
  1          10270 MB              --             112 s
  10          2820 MB           3265 MB           107 s
  20          1424 MB           2148 MB           130 s
  100          458 MB            734 MB           299 s

14% better at 10 windows, 34% at 20, 38% at 100, for at most 4% of build time.
Against the unbatched build the anon figure is now 3.6x lower at 10 windows and
22x at 100 -- a build that allocates 458MB while indexing 1.12 billion postings.
The model over-predicts (2.07GB against 2.82GB measured), so it is used only to
rank the choices, not as a memory estimate.

The cluster-to-fill ratio is a constant rather than a function of alpha, beta and
dimension, because only its rough magnitude matters: the cost curve is a shallow
basin, and assuming 8x or 32x instead of 16x costs about a fifth of the benefit
while still beating either phase alone. Deriving it would mean modelling
summarize(), which this does not need to be right about.

The output is unchanged, which the existing byte-equality tests assert: which
window a term lands in cannot affect it.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Two errors, both mine, in the table added a few commits ago.

It carried the numbers from the previous weighting rather than the one that
shipped, because the edit that was meant to update them was in the same command as
a commit that a hook rejected, so it never ran.

Worse, it mixed units: every row was captioned as having the corpus mapped, but the
unbatched row was a heap build at 24095 MB, so the drop from 24 GB to 3 GB read as
a batching win when most of it was the corpus not being in anonymous memory. The
unbatched row is now the mapped measurement, 10274 MB, which makes the rows
comparable and the win 3.6x at ten windows rather than an implied 7x.

The heap and mapped figures also do not differ by a fixed offset, which the old
text implied: the same 1-window build measures 10274 MB mapped against 24084 MB
with a streaming ingest, 13.8 GB more for a 6.86 GB corpus, because the ingest
stages a second copy that the allocator retains rather than returning. How much of
that overlaps the build's own peak depends on how much the build asks for. Spelled
out rather than left to be inferred.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
A batched build retained nothing: clustered_inverted_lists stayed empty, so
search() found no lists and write_index() would have emitted a header with no
postings. The caller had to reopen the file by path. That is a sharp edge for no
good reason, and it made the batched path asymmetric with every other build.

With batch_size > 1 and an output path, build() now maps the file it just wrote and
borrows the posting lists out of it. batch_size <= 1 is untouched: an ordinary
build already holds its own lists, so there is nothing to stream and nothing to map
back.

Only the lists are read back. The forward vectors in the file are a copy of ones
the index already has, at whatever residency the caller chose, so re-reading them
would be work for nothing -- and it is what lets the corpus mapping be left alone.
write_seismic_index_batched returns the byte offset of the list section so the map
can seek straight there instead of parsing past the vectors.

MmapIndex gains a second MmapFile for that file rather than reusing mapped_file_.
The two genuinely coexist: an index that read its corpus with read_csr(kMmap) is
still scoring from that mapping, so giving it up for the index's own would leave
the index unable to score anything. Two members means no swap, and therefore none
of the ordering hazard a swap would carry -- whatever borrows from the new mapping
lives in the derived class, and derived members are destroyed before base ones, so
the borrowers always go first.

Borrowing is what the lists want rather than copying: measured on the 14.9GB
base_full index, 0.19s and 8MB of anonymous memory against 11.2s and 13.9GB to
copy them. The cursor reads only the size header before each array and skips the
bulk, so it faults in about a quarter of the file as page cache, which the kernel
can reclaim. Across the batched rows it costs about 4s and leaves peak RssAnon
unchanged -- 2816 MB at 10 windows against 2820 MB without it -- while peak RSS
rises by the index it touches. peak_rss_mb in the benchmark therefore now includes
that; the anon column is the one to read.

  windows       peak RssAnon   peak RSS   peak RssFile   build
  1 (unbatched)     10281 MB   16732 MB        6454 MB    105 s
  10                 2816 MB   12811 MB        9820 MB    111 s
  20                 1471 MB   11465 MB        9185 MB    130 s
  100                 465 MB   10396 MB        9725 MB    295 s

Tested: a batched build is searchable straight after build() and returns exactly
what an unbatched build at the same seed returns, including when the corpus is
itself mapped, which is the case the second mapping exists for. Python covers the
same through the factory description.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Comment thread DEVELOPER_GUIDE.md Outdated
Comment on lines +237 to +243
`lambda` and `beta`, and every type in the family gets it — `seismic`,
`seismic_sq`, `disk_seismic`, `disk_seismic_sq`:

| Option | Effect |
|---|---|
| `inverted_list_batch_size=N` | Build in `N` term windows. Bounds the inverted-list intermediate to one window; the index is still built in memory as usual. |
| `batch_file_output_path=P` | With `N > 1`, serialize each window to `P` and free it, so the clustered lists are never all resident either, then borrow them back from `P` by mapping it. Unused at `N <= 1`, which is an ordinary build and already holds its own lists. |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guide lists disk_seismic and disk_seismic_sq among the types that get batch_file_output_path, but neither honors it. DiskSeismicIndexBase::build() (which both inherit — neither overrides build()) unconditionally calls build_inverted_lists_clusters(...) and never reads batch_clustering.batch_file_output_path; only SeismicIndex::build() and SeismicScalarQuantizedIndex::build() have the streaming branch. So on a disk index the flag is a silent no-op: no file is written, the full clustered-list set stays resident (the memory bound is never applied), and no error is raised — precisely the large, disk-resident corpus this feature targets.

I can later implement this part for disk seismic.

Comment thread nsparse/seismic_batched_build.cpp Outdated
// writer that started at 0 carry the wrong padding once appended at some
// other offset. Streaming through a single writer keeps pos() the true
// absolute offset.
FileIOWriter writer(const_cast<char*>(out_path.c_str()));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

write_seismic_index_batched opens the destination with fopen(..., "wb"), which truncates out_path immediately, then writes incrementally. Several throw sites run after the open — count_postings_per_term (term outside dimension) on the batch_size>1 path, the WindowLists::add "corpus changed" guard, bad_alloc while clustering, a short fwrite on a full disk, and the final coverage-check throw — and none removes the file. So a failed build silently overwrites whatever was at out_path (possibly a valid index) and leaves a partial file read_index later rejects. Could we write to a temp path and std::filesystem::rename into place only after writer.close() succeeds, unlinking on any exception? That gives atomic publish and never clobbers a good index. (RejectsInvalidInput uses batch_size=1, taking the in-memory path, so this streaming-path clobber is untested — worth a batch_size>1 case asserting the pre-existing file is untouched on failure.)

std::vector<InvertedListClusters> clustered(window.size());
const auto chunk = static_cast<int64_t>(std::clamp<size_t>(
window.size() / kMinClusterChunks, 1, kMaxClusterChunk));
#pragma omp parallel for schedule(dynamic, chunk)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cluster_window's #pragma omp parallel for body (prune_and_keep_doc_ids / RandomKMeans::train / summarize) can throw std::bad_alloc, and an exception escaping an OpenMP structured block is UB — libgomp/libomp call std::terminate. This is inherited from the old build_inverted_lists_clusters so it's pre-existing, but since this feature is specifically for memory-pressured builds where a mid-clustering bad_alloc is realistic, it's more likely to bite here. Worth considering catching within the loop (capture the first exception + a flag, skip remaining iterations, rethrow after the region) so an OOM surfaces as a catchable exception. Non-blocking / pre-existing — flagging since it's on the exact failure path this feature is built for.

Comment thread nsparse/seismic_batched_build.h Outdated
//
// Reached through an index's build(), by setting
// SeismicClusterParameters::batch_clustering.batch_file_output_path. The index
// is then the file, not the object: nothing is retained to serve or to

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This says the batched build leaves "the file, not the object: nothing is retained to serve or to write_index afterwards," but build() calls map_streamed_lists() right after and borrows the posting lists back, so the index is servable in place (see BatchedBuildIsSearchableAfterBuild and the correct wording in SeismicIndex::build). Suggest rewording so a maintainer doesn't think they must re-read the file to serve.

The in-memory types end their payload with their posting lists, so a
batched build serializes each window straight into the output and drops
it. A DiskSeismic payload cannot be written that way: its summaries are
followed by an inline forward index whose blocks are laid out from the
doc-id membership of every list, which is not known until the last window
is clustered, so batch_file_output_path was silently ignored there.

What can be dropped is the lists' residency rather than the lists. The
clustering still runs once, a window at a time, into a spill next to the
output; the lists come back borrowed from that mapping, and the payload is
written from it, the forward index streaming its blocks as it lays them.
Neither phase holds more than one window of anonymous memory, and the
spill goes with the build.

On base_full (1.12B non-zeros) at lambda=600, corpus mapped, peak RssAnon
falls 8671 MB -> 1440 MB at 10 windows and 434 MB at 100. The benchmark
driver takes an index type now, so that is measurable rather than assumed.

The output is byte-identical to build() + write_index at a fixed seed, for
both disk types, and the index ends borrowing its own output rather than
the scratch it deleted -- so build() still leaves something that serves.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Comments and the guide carried figures from one benchmark run on one
dataset -- corpus names, per-term posting counts, peak-memory tables,
query-latency rows. They date the moment they are read: a reader cannot
tell whether they still hold, and the numbers are not what any of it is
explaining.

What is worth keeping is why the code is shaped this way -- that term
frequencies are skewed so windows are cut by cost, that clustered
postings are far bulkier than scattered ones, that anonymous memory is
the column batching moves. That survives here without a corpus attached.
The guide keeps the shape to expect and how to measure it; measurements
belong to a run, and a run belongs in its own report.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
batch_file_output_path is a directory the build may spill into, not an
index to produce. Serializing an index is write_index's job, and a build
that wrote one had two ways to make the same file -- the streamed writer
here and each type's write_index -- which is a layout to keep in step for
no benefit.

So there is one mechanism now, the one the disk-resident pair already
needed: cluster a window, spill it, drop it, and map the finished lists
back out of the spill. Every type reaches it through
MmapIndex::build_clustered_lists, which is also where the batching
decision is made, so a type's build() is one call again. The spill is
unlinked as soon as it is mapped, so nothing is left in the caller's
directory and a crash mid-build cannot strand scratch; where a mapped
file cannot be unlinked the index removes it when it goes.

That deletes more than it adds: write_seismic_index_batched, the
index-header exposure it needed (reverting 6affd30), the disk base's
payload-header hook and mapping-slot parameter, and its second write of
the payload.

A window count with no directory to spill into now resolves to one
window rather than being half-applied: it would bound the fill
intermediate while the clustered lists accumulated for the whole corpus
anyway, which is a corpus pass per window for a fraction of the peak.

Unchanged: at a fixed seed the index write_index produces is byte-for-byte
what a whole-corpus build would have produced, now asserted over all four
types in one parametrized test, and build() still leaves an index that
serves.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Four things the previous shape got wrong.

The spill's file and its mapping are one lifetime, so they are one type
now -- ClusteredListsSpill, which unmaps then removes, in that order, and
re-checks the name against the prefix and suffix it writes so it can only
ever delete a file it created. An index holds one instead of a mapping
plus a path, and MmapIndex needs no destructor.

build_clustered_lists was a method on MmapIndex, which is a mapping owner
rather than a builder. It is a free function beside the spill it uses, and
each type's build() passes its own corpus and dimension.

SparseVectorsConfig carried an element width the corpus already knows, so
callers were passing the same fact twice and the build checked the two
against each other. for_each_clustered_window now takes the corpus and the
dimension -- which is the list count, and the one thing an empty corpus
cannot supply -- and reads the width off SparseVectors.

Plus a test that the caller's scratch directory is left alone: files that
were there before the build survive it, including one named like a spill,
which the build did not create and so must not remove.

Comments trimmed to the reasons.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
The scratch-directory test read the spill's removal as immediate, which it
is only where a mapped file can be unlinked. On Windows it cannot, so the
spill sits there until the index releases it -- exactly what the code says
it does, and what the CI failure reported.

So the assertion is now the contract: the directory is empty once the index
is gone, and until then the spill is at most its sole occupant. The lists
stay readable throughout on both, which is the part that matters.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Comment thread nsparse/seismic_batched_build.cpp Outdated
Comment on lines +143 to +150
const std::string path = spill_path(scratch_dir);
{
// Closed before the file is mapped: the writer buffers.
FileIOWriter writer(const_cast<char*>(path.c_str()));
stream_clustered_lists(vectors, dimension, params, &writer);
writer.close();
}
into->adopt(path);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any possible chance that something throws between spill_path and adopt? Seems that adopt() here is the only code that removes it. I'm worried whether there would be partial scratch.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, and it was reachable: the counting pass rejects a term outside the dimension mid-write, and a failed flush or allocation would too. Fixed in 3586141 — the spill is owned from creation now and released on any throw, with a regression test.

The spill was written first and adopted afterwards, which left everything
in between unguarded: the counting pass rejecting a term outside the
dimension, a window arriving short, a full disk, a failed flush, an
allocation failing in a build that exists for memory-tight corpora. Any of
those left a half-written file in the caller's scratch directory. The first
of them is a case the tests already cover, so this was reachable, not
theoretical.

ClusteredListsSpill::write_and_map now creates the file, hands it to the
caller's writer, and maps it, owning it throughout: the path is recorded
before the file exists, and any throw releases it on the way out. Unlinking
after the map still covers a crash after the write; a crash during it is
what the distinctive name is for.

Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
@zirui-song-18
zirui-song-18 merged commit 9b3afa9 into opensearch-project:main Sep 3, 2026
10 checks passed
@chishui
chishui deleted the seismic-batched-build branch September 3, 2026 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bound the memory an index build needs, so a corpus larger than RAM can be indexed

2 participants