Skip to content

compute: make the peek stash a state transition of one scan - #38482

Merged
antiguru merged 19 commits into
peek/designfrom
peek/stash-transition
Aug 27, 2026
Merged

compute: make the peek stash a state transition of one scan#38482
antiguru merged 19 commits into
peek/designfrom
peek/stash-transition

Conversation

@antiguru

Copy link
Copy Markdown
Member

Deletes the second walk a stashed peek used to cost. A peek whose accumulated rows crossed the stash threshold abandoned its walk, and a separate path re-read the trace bundle from the beginning to stream rows into persist. The promoted driver now writes each full batch as the walk produces it, so a peek walks its arrangement exactly once whatever its answer turns out to be. This is the layer that makes the one-path claim in the design document true.

The upload becomes a handle the driver feeds. StashUpload keeps everything about the persist interaction, the shard derived from the peek uuid, the schemas, the batch builder, the max_rows early exit, and takes rows through push instead of through a channel the worker pumps. Complete carries whatever the scan still holds, and the driver assembles the answer around it: a driver that never opened an upload answers with a row collection, and one that did finishes and answers with the stashed handle, whose inline_rows carry the rows that never reached the stash. The hand-back the layer below needed goes away, because the task now has somewhere to write.

PendingPeek ends with fewer index-peek states than it had before this work started, which is the trade the design says the effort is making. PendingPeek::Stash, StashingPeek and its worker-driven pump, start_stash_upload, and PEEK_STASH_NUM_BATCHES are all gone. PEEK_STASH_BATCH_SIZE goes too, against the plan: it counted rows, the upload cuts a batch on the byte threshold the scan already tracks, and nothing read it. A live tunable that does nothing is worse than an absent one, because an operator who reaches for it gets silence rather than an error.

Two consequences worth stating plainly rather than discovering later.

The kill switch no longer means no peek ever leaves the worker. With UsePeekStash gone, a large streamable peek would otherwise have no route to the stash at all, which is a functional regression rather than a placement change. The switch therefore gates budget-based promotion only: a scan that suspends because its prefix is batch-ready is handed to a task whichever way the switch is set, because that promotion is for stashing rather than for latency. With the switch off, ordinary peeks behave exactly as they do today.

The row iteration limit now follows a peek into the peek stash. #38158 stopped at the stash because a stashed peek restarted its scan and the restart charged the same rows twice. Deleting the restart is what this change does, so the count simply continues because the scan does. The dyncfg description said otherwise and no longer does.

Cancellation deletes what it wrote. A cancelled peek used to leave the parts already written in blob storage, since impl Drop for Batch only logs the dangling keys and the reader-side delete runs after a successful read that a cancelled peek never reaches. Cancellation aborts the promoted task rather than signalling it, so an await placed after the cancellation check would never run, and the obligation lives on the upload itself: dropping one spawns a Batch::delete() onto a runtime handle captured when it opened. A guard covers the window inside finish, where persist has taken the builder and no upload holds the parts any more.

Reclaiming those blobs must not cost more than it saves. A builder whose part write was in flight when its walk was aborted holds a write persist has already marked as waited on, and finishing it panics rather than returning; this replica aborts the process on any uncaught panic, so reclaiming one query's blob storage could take the replica with it. The panic is caught and the shard logged. Failing to reclaim is an outcome this path already tolerates for a replica that dies mid-upload, which makes a leak the right answer there and an abort the wrong one. A Linear issue tracks the persist-side fix that would remove the need for the catch.

mz_index_peek_stashed_total counts the walks the stash answered. It is incremented beside the offloaded substrate counter, so it is a strict subset by construction, and every inline-driver test asserts it stays at zero. mz_stashed_peek_seconds goes: its only observer was the deleted walk, and a registered histogram that never observes reports a flat zero, which reads worse in a graph than an absent series. The duration is deliberately not reconstructed, because one loop now walks and writes, so the write is not separable from the walk.

What an upload can delete is not always everything it wrote. Persist merges runs once a builder holds more than peek_response_stash_batch_max_runs of them, and a merge writes a fresh output and drops the inputs it read without entering them into shard state. Parts flush at persist_blob_target_size, so an upload small enough never to merge deletes all of what it wrote and a large one deletes the output of its last merge. A reader deleting a response it has finished with reaches exactly the same parts, so this is a property of the builder rather than of abandonment, and it is unchanged by this PR.

Nothing bounds the total size of a stashed answer, which is also true today. max_result_size bounds the prefix a single scan retains between batches and never the sum across them. The only ceiling is the reader's own budget.

Known gaps, none of which this PR closes:

  • No sqllogictest or testdrive exercises a real stashed fast-path peek through the adapter reader. inline_rows was always empty from a worker on the old path and is now non-empty up to the threshold, and the adapter merges it across workers, so that merge has no coverage in CI.
  • Nothing bounds how many abandoned uploads hold a part at once. Each can hold up to persist_blob_target_size, and the abandon task outlives the walk's permit.
  • The caught persist panic is verified by construction rather than by a test, because the panic handler that turns it into an abort is not installed under cargo test.

🤖 Opened by Claude Code on behalf of @antiguru

@antiguru
antiguru force-pushed the peek/offload-driver branch from d8a92f4 to 25b64f4 Compare August 26, 2026 09:41
@antiguru
antiguru force-pushed the peek/stash-transition branch from 6a8b0f8 to aeed32d Compare August 26, 2026 09:41
antiguru and others added 19 commits August 27, 2026 13:35
The peek stash's persist interaction lived inside a loop over the channel the
worker pumps, so the only way to write a peek's rows was to hand them to that
channel. `StashUpload` lifts that interaction out: it opens the client, derives
the shard from the peek's uuid, builds the batch, and takes rows through
`push(RowBatch)` from whoever drives the walk that produced them.

The upload owns the IO, which is what lets a walk feed it without becoming
async itself. It reports demand rather than relying on a dropped receiver to
stop production: an upload that holds every row the finishing's offset plus
limit can use answers `Satisfied`, so a driver still walking learns it may stop,
and rows pushed after that are discarded because no answer built from the
upload can contain them.

The channel drain becomes a driver over the same handle, so the shard id, the
schemas, the `SourceData` wrapper, the batch runs, and the assembled
`StashedPeekResponse` are the ones it always wrote.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A fast-path index peek whose result is too large to answer inline went to the
stash by throwing its walk away: the scan that discovered the size was dropped,
and a fresh `PeekResultIterator` walked the same trace from the beginning while
the worker pumped rows into the upload. The promoted driver now writes instead.
It takes each full batch the scan hands over, pushes it to a `StashUpload`, and
steps the same scan again, so one walk produces the whole answer.

`ScanOutcome::Complete` carries the rows the scan still held, and the driver
assembles the response around them: a driver that never wrote answers with a
`RowCollection`, and one that did finishes its upload and answers with the
handle. Those trailing rows become the response's `inline_rows`, which is where
rows of a stashed answer that never reached a batch belong, so they cost no
write of their own. An upload that reports `Satisfied` stops the walk, because
the rows past a finishing's offset plus limit are rows no answer can contain.

With the restart gone, `OffloadOutcome::NeedsStash` and its worker-side arm have
nothing left to do, and the inline driver no longer needs to tell a batch-ready
suspension from one that ran out of fuel. Both are promotions now.

That changes what the kill switch covers, and the change is deliberate. It gates
promotion for latency only. A scan that suspends holding a batch bound for the
stash is promoted whichever way the switch is set, because the driver that
writes to the stash is the promoted one, and a worker that kept such a scan
would hold one that makes no progress until its batch is taken. Off therefore
means an ordinary peek runs where it used to, not that no peek leaves the
worker, and the dyncfg and the budget both say so.

`StashUpload::push` and `finish` report a rejected write rather than panicking.
The upload runs inside the promoted walk now, so a panic there kills the task
without an answer, which the worker reports through its dead-task arm as a
failed walk while soft-panicking the worker in test builds. The peek is the unit
that fails, and `open` already said so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The description claimed the limit does not apply once a peek's results
move to the peek stash. That described a peek that abandoned its inline
scan and restarted the walk from the trace, which no longer happens. A
peek walks its arrangement once, and the promoted walk re-reads the limit
at every slice against a count that is cumulative for the peek, so a
stash-bound walk can exceed the limit where it once could not. State the
rule the code now enforces and why it follows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A walk that hands persist a batch it rejects, or that fails to finish the
batch it built, returns the failure to the query and leaves whatever it
had already flushed in blob storage. The sibling paths that abandon an
upload say so where the code abandons it, so these two say the same.

An invalid usage from persist is a defect rather than a blip, and the
query's error message is the only place it showed. A warning makes it
diagnosable from the replica's logs without escalating a single peek's
failure to the worker.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The floor's justification named a suspension that holds no full batch as
what makes a promotion, and a trace bundle clone as part of its price.
Every suspension is a promotion now, and a promoted walk carries the scan
rather than a bundle to restart from. The floor is unchanged; only the
cost it avoids is restated.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The outcome's doc told a driver that cannot write batches to consult
batch_ready before stepping again. No such driver exists: a scan that
offers a batch is carried by the promoted walk, which takes every batch
it is offered. Say that taking a batch is what lets the walk go on, and
that a driver with nowhere to write one cannot reach an answer, rather
than describing a way to step around it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A walk that cannot open a shard, cannot hand over a batch, or cannot finish
one reports the failure to the query and warns the replica, but the warning
named no peek, which leaves an operator with a defect and no way to find which
query hit it. Carry the peek's uuid into the walk and into the answer it builds
from an upload.

Opening a shard failed silently before, which made the three stash failures
inconsistent about whether they left a trace at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PendingPeek::Stash and StashingPeek drove their own iterator over the ok
trace and pumped rows to an async upload task through a channel, a second
walk that duplicated what the promoted index-peek walk now does directly.
Remove StashingPeek's start_upload, do_upload and pump_rows along with its
peek_iterator and rows_tx fields, the PendingPeek::Stash state that held
one, and start_stash_upload, the compute_state.rs entry point that built
one. PEEK_STASH_NUM_BATCHES governed how many rows that pump moved per
worker activation, which is meaningless with no pump left, so it is
deleted along with its two Python allowlist entries. PEEK_STASH_BATCH_SIZE
stays, since the upload it fed still batches rows.

mz_stashed_peek_seconds observed only the deleted walk's upload and is
unreachable since the promoted walk took over stashing, so it is removed
with its ComputeMetrics and WorkerMetrics fields.
PEEK_STASH_BATCH_SIZE bounded how many rows one worker activation pumped
into a stash upload. The promoted walk cuts a batch on
peek_stash_threshold_bytes instead, the byte threshold the scan already
tracks, so nothing reads the row count and turning it has no effect. A
live tunable that does nothing is worse than an absent one, because an
operator who reaches for it gets silence rather than an error, so it goes
along with its two Python allowlist entries.

Regenerate the metrics catalog for mz_stashed_peek_seconds, whose
declaration went with the walk that observed it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deleting mz_stashed_peek_seconds left the peek stash with no signal of
its own: mz_index_peek_offload_seconds mixes stashed and non-stashed
walks under one name, and mz_index_peek_walks_total only splits inline
from offloaded. Whether a peek reached the stash was unanswerable, which
is the same failure mode the substrate counter exists to prevent.

Add mz_index_peek_stashed_total, incremented beside walked_offloaded so
it is a strict subset of that substrate, and assert it stays at zero in
the inline driver's tests, where a stashed answer would mean the inline
path wrote to persist. Duration is deliberately not restored: one loop
now walks and writes, so the write is not separable from the walk.

Correct mz_index_peek_total_seconds, which claimed a suspension may
divert a peek to the stash and that stash-bound peeks are excluded.
Neither holds: PeekStatus has no divert outcome, and the observation
runs before the status is matched, so a stash-bound peek's inline slice
is counted like any other.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A walk that gives up mid-upload used to leave the parts it had already written
in blob storage, and the reader-side deletion that does happen runs only after a
successful read. `StashUpload` now owns the obligation: `discard` consumes an
upload and deletes what it holds, and `Drop` does the same for an upload that
reaches neither `discard` nor `finish`.

`Drop` is what covers cancellation. Cancelling a peek drops the `OffloadedPeek`,
which aborts the walk's task, and an aborted task is dropped rather than polled
again, so an `await` on the path the walk takes would never run. The deletion is
spawned onto a runtime handle the upload holds, because a future's drop is not
guaranteed to run inside a runtime context.

Also records who bounds the size of a stashed answer, which is the reader's own
budget and nothing on the writing side.
Finishing an upload takes the builder out of it before awaiting the flush, so a
walk aborted inside that await left an upload with nothing to delete and every
part it had written behind. That await is the longest one an upload makes, which
makes it the likeliest place for a cancellation to land.

The finish now runs as a detached task and delivers its batch over a oneshot. A
batch nobody claims, because the send found no receiver or because the receiver
died before the take, deletes itself rather than leaving its blob keys behind.

Also states what abandoning an upload costs: a builder flushes only at
`persist_blob_target_size`, 128 MiB by default, and the walk's permit is gone
before the delete finishes, so nothing bounds how many abandoned uploads carry a
part at once.
The upload's contract named a rejected finish as the escape that leaves
parts in blob storage, and that case cannot arise here: persist refuses a
batch only when its bounds do not admit its own updates, and this upload
writes at a fixed lower, upper and timestamp. Meanwhile the escape that
does arise went unnamed. Once finish hands back a response the parts
belong to the response, so a cancellation that beats it to the
coordinator drops a transmittable batch nothing will delete. Rebuilding a
deletable batch from what the response carries needs a WriteHandle the
walk does not hold, so a reader-side sweep or persist's garbage
collection is what covers it, recorded as a TODO where the response is
dropped.

Build the inline rows before the finished batch leaves its guard, so that
a row whose count does not fit still deletes what was written.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An upload that reaches no reader owes blob storage a delete, and until now
nothing said so. The tests here count the blob keys an upload writes and the
ones it deletes, over a persist client cache configured to write every row out
as a part rather than inline it into shard state, which is what makes those
counts observable at all.

The four cleanup paths are covered separately, because they are separate code:
an upload handed to `discard`, one that is only dropped, a finished batch that
never reaches the response naming it, and a finished upload whose parts belong
to the response and must stay. The last of those is what keeps the other three
from being satisfied by cleanup that deletes unconditionally.

The rejection arms of `push` and `finish` are reached by building an upload
with bounds that opening never produces, since the lower, upper and timestamp
opening fixes are ones persist always accepts. They report rather than panic
because a panic in a promoted walk lands in the dead-task arm, which takes the
worker down in a test build.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cancelling a promoted peek aborts the task driving its walk, so the cleanup of
whatever that walk had already written to the peek stash runs from a `Drop`
that spawns onto a runtime handle the upload captured. Nothing exercised that
chain. The test here drives a real promotion past the stash threshold, waits
until a part has reached blob storage, aborts the walk the way a cancellation
does, and asserts that every key written is deleted.

Two further paths get the same treatment: a walk that observes its cancellation
between slices and gives its upload up, and a walk whose scan fails after a
batch has already been written, which is a failure this driver created by
making the walk that produces the rows the walk that writes them.

The walk counters gain the assertions the answers cannot make. A stashing walk
must reach `mz_index_peek_stashed_total`, and the count of cursor positions the
walk reports says the index was walked once, which a comparison of rows cannot:
a second walk of the same trace answers identically.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Aborting a walk while persist has a part write in flight leaves that
write marked as being waited on, and finishing the builder then panics
instead of returning. The cleanup that reclaims an abandoned upload's
blobs does exactly that finish, and this replica installs a panic handler
that aborts the process for any panic outside a catch, so reclaiming one
query's blob storage could take the whole replica with it. That window is
not narrow: parts flush at 128 MiB, so a large stashed answer spends most
of its time waiting on a write, which is the case the stash exists for.

Catch what the finish raises and log the shard it could not reclaim.
Failing to reclaim is an outcome this path already tolerates for a
replica that dies mid-upload, which makes a leak the right answer here
and an abort the wrong one.

Correct two claims the tests falsified. Persist merges runs once a
builder holds more than peek_response_stash_batch_max_runs of them, and a
merge drops the inputs it read without entering them into shard state, so
neither side can address them again: an upload deletes the parts a
finished batch reaches rather than everything it wrote, and a reader
deleting a response reaches the same subset. Drop a test's assertion-free
tail loop, which waited for a teardown whose failure lands in a detached
task that no test can observe.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three terminal arms of a promoted walk answer with an error when the
stash cannot take the rows: no stash location, a shard that will not
open, and a batch persist rejects. All three counted the walk on the
offloaded substrate and none reported the phases that precede the ok
walk, unlike the neighbouring failure arm and unlike the contract
observe_error_phase states. The error trace is complete by the time any
of them is reached, so report it.

Narrow two claims to what the code does. The substrates sum to the walks
that reached an outcome rather than to the peeks that were answered,
since a walk whose task dies without one is answered by the worker and
counted on neither. And the yield granularity is an upper bound rather
than a period, because a stash-bound walk suspends on the byte threshold
long before it spends its fuel.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
max_result_size was compared against the rows a scan happened to be
holding. Once a peek diverts to the stash that prefix is handed away and
reset on every batch, so the comparison never reached the ceiling and a
stashed answer could grow without bound. The ceiling appeared to hold
only because the whole answer used to reach the adapter as one chunk, and
RowSetFinishingIncremental checks the chunk it is given rather than the
running total: splitting the answer into inline rows plus threshold-sized
batches left every chunk under the ceiling and stopped it applying at all.

Track what the batches already handed off contained and measure the sum,
so the bound covers the answer the client receives rather than the prefix
the scan retains. Failing in the walk also spares the replica writing an
answer to blob storage that nothing may read.

The test that pinned the old reading asserted a stash-bound prefix escapes
the ceiling. It now asserts the opposite, alongside one that keeps a
stashed answer under the ceiling walking the whole trace, so the bound
does not cost the stash the results it exists for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@antiguru
antiguru force-pushed the peek/offload-driver branch from 25b64f4 to 08d2504 Compare August 27, 2026 11:38
@antiguru
antiguru force-pushed the peek/stash-transition branch from eb0ee27 to 3f5e531 Compare August 27, 2026 11:38
Base automatically changed from peek/offload-driver to peek/design August 27, 2026 11:39
@antiguru
antiguru merged commit 3f5e531 into peek/design Aug 27, 2026
1 of 2 checks passed
@antiguru
antiguru deleted the peek/stash-transition branch August 27, 2026 11:39
@antiguru
antiguru restored the peek/stash-transition branch August 27, 2026 11:44
antiguru added a commit that referenced this pull request Aug 31, 2026
`Pending::block_until_ready` replaced the state with a `Blocking`
placeholder before awaiting the write handle. Dropping the future at
that await left the placeholder in place permanently, and every later
`into_result` on that `Pending` panicked with "block_until_ready
cancelled?".

`BatchBuilder` reaches that await in its ordinary write path, in the
loop that bounds outstanding part writes, and `finish` reaches
`into_result` through the run-completion paths. A caller who drove
`BatchBuilder::add` inside a cancellable future and then called
`finish`, for example to obtain a `Batch` it could `delete()`, hit an
unconditional panic. Since `install_enhanced_handler` aborts the process
for any panic outside a catch, that lost the whole process rather than
the batch. The window is not narrow: for a large batch on object storage
the part writes are the bottleneck, so a builder past a few parts spends
most of its wall time inside exactly that await.

The fix awaits the handle through the borrow instead of taking it out.
Nothing is removed from the state, so there is no placeholder to leave
behind and the `Blocking` variant disappears along with its panic. The
spawned task keeps running across the cancellation, so a later
`block_until_ready` or `into_result` still yields the value.

Two items from [PER-70](https://linear.app/materializeinc/issue/PER-70)
stay open and are not part of this change: a `BatchBuilder` teardown
that surrenders the parts already written without flushing the buffered
one, and removing the `ore_catch_unwind` workaround that #38482 wraps
around its peek-stash blob cleanup, which this fix makes unnecessary.

Adds `test_pending_survives_cancellation`, which cancels a
`block_until_ready` at its await point and then reads the value; it
fails with the original panic before the fix.

Closes: [PER-70](https://linear.app/materializeinc/issue/PER-70)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant