Skip to content

compute: add configurable peek row iteration limit - #38158

Closed
aljoscha wants to merge 1 commit into
MaterializeInc:mainfrom
aljoscha:compute-peek-row-iteration-limit
Closed

compute: add configurable peek row iteration limit#38158
aljoscha wants to merge 1 commit into
MaterializeInc:mainfrom
aljoscha:compute-peek-row-iteration-limit

Conversation

@aljoscha

@aljoscha aljoscha commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Motivation

Compute workers synchronously iterate arrangements while fulfilling index-backed peeks. A query that scans far more rows than it returns can hold a worker for a long time and delay everything else on the cluster. Persist fast-path peeks have the same shape, since filtering happens after the rows have been read.

This change adds an off-by-default failsafe that bounds how many rows a worker may examine for one peek.

Description

Two dynamic configuration parameters: a feature gate that defaults to false, and a per-worker row threshold that defaults to 1,000. Both are read through handles, so an UpdateConfiguration reaches peeks that are already in flight without resetting work they have already performed.

The budget covers the index result trace, the index error trace and the Persist fast path, and it counts rows before literal and MFP filtering: a row that is read and then discarded costs the same scan time as one that is returned. Exactly the configured number of rows may be examined, and a peek fails only when it asks for the row after that.

The limit deliberately stops at the peek stash. A stashed peek restarts its scan and produces in bounded bursts, so bounding it means carrying the count across the hand-off, and the restart then charges the same rows twice. That is worth doing properly rather than quickly, and it can be added later. The peeks that motivate the failsafe, large filtered scans, fail well before they reach the stash threshold.

Structured peek errors. Reporting the limit needs an error type that survives the trip from the worker. PeekResponse::Error carried a bare String, so every peek failure reached the adapter as AdapterError::Unstructured and was reported as XX000. It now carries a PeekError (Dataflow / Unstructured / RowIterationLimitExceeded) and PeekResponseUnary::Error carries an AdapterError, so the conversion happens once instead of once per frontend. The limit reports SQLSTATE 54000 (PROGRAM_LIMIT_EXCEEDED) with detail about the per-worker scan and a hint naming the threshold parameter. Worker responses merge by precedence: cancellation, then ordinary errors, then the limit.

Two user-visible consequences of carrying the dataflow error structurally:

  • Evaluation errors raised while reading a collection get a precise SQLSTATE. SELECT a / b FROM t reports 22012, matching its constant-folded counterpart, instead of XX000.
  • Such an error renders the way DataflowError renders it, so one that used to come back bare from an index or Persist fast-path MFP gains the Evaluation error: prefix that the error-trace path already produced.

The compute protocol is bincode, which cannot skip an unknown variant, so PeekResponse serializes through a mirror type that keeps Error(String) in place for the unstructured case and appends the structured one. Existing frames, Canceled in particular, encode exactly as before, and a test asserts that.

The test and CI configuration enables the feature with a high threshold, so existing suites exercise the guarded path without constraining ordinary queries.

Verification

sqllogictest coverage exercises enabled and disabled behavior, the exact bound, filtered rows, and index-backed and Persist fast-path peeks. Unit coverage verifies dynamic configuration updates, structured error metadata, deterministic multi-worker response precedence, and that the wire encoding of the pre-existing response variants is unchanged. test_dataflow_error_codes covers the fast-path and error-trace SQLSTATEs end to end.

The full suite is left to CI, which is also where the Evaluation error: message change will surface if any golden still expects the bare form.

}
}

fn absorb(&mut self, shard_id: usize, response: PeekResponse, max_result_size: u64) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this all feels a bit too bespoke, and maybe nudges us in the direction of instead making the existing error type properly typed, and we fix this once and for all instead of glomming on workaround solutions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, and done in 380031f — the bespoke accumulator is gone.

PeekResponse::Error now carries a PeekError (Dataflow / Unstructured / RowIterationLimitExceeded) and PeekResponseUnary::Error carries an AdapterError, so compute-to-adapter conversion happens once instead of once per frontend. The row limit is just one variant of that type now, so PendingPeek is back to eager merging plus error-vs-error precedence.

One thing that did have to stay: the running inline_byte_len. Merging drops a worker's rows as soon as any worker errors, so without tracking the byte total separately, whether a peek reports the aggregate max-result-size error depends on the order the responses arrive in. The permutation test covers that.

Falling out of this: evaluation errors from reading a collection now get a real SQLSTATE (SELECT a / b FROM t -> 22012 instead of XX000), which is what the typed error was worth doing for.

@aljoscha
aljoscha force-pushed the compute-peek-row-iteration-limit branch from 380031f to a42d56c Compare August 11, 2026 13:14
@aljoscha

Copy link
Copy Markdown
Contributor Author

Scope trimmed and the branch is now a single commit.

The row iteration limit no longer follows a peek into the response stash. Bounding a stashed peek means carrying the count across the hand-off, and since the stash restarts the scan from the beginning, the prefix then gets charged twice. Doing that properly means handing the existing iterator and its buffered prefix to StashingPeek instead of building a fresh cursor, which is a bigger change than the failsafe itself. The queries this protects against, large filtered scans, fail long before they reach the stash threshold, so the guard still does its job. There is a NOTE at the hand-off and a sentence in the dyncfg description recording the gap.

That also removed the speculative-production bound in pump_rows, which only existed so that stash batching could not trip the limit on rows the finishing did not need.

Also folded the structured-error work into the same commit, since it reads as an add-then-rewrite otherwise: the typed PeekError is what makes the limit reportable, so they belong together.

@antiguru antiguru left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this looks fine. I'd increase the default from 1k. Why do we not extend it to the peek stash?

Comment on lines +522 to +534
/// Whether compute should stop peeks that iterate over too many rows.
pub const ENABLE_PEEK_ROW_ITERATION_LIMIT: Config<bool> = Config::new(
"enable_compute_peek_row_iteration_limit",
false,
"Whether compute should stop peeks that exceed compute_peek_row_iteration_limit.",
);

/// The maximum number of rows a peek may iterate over on each worker.
pub const PEEK_ROW_ITERATION_LIMIT: Config<usize> = Config::new(
"compute_peek_row_iteration_limit",
1000,
"The maximum number of rows a peek may iterate over on each worker when enable_compute_peek_row_iteration_limit is enabled. Does not apply once a peek's results move to the peek stash.",
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could the limit be a Option<NonZeroUsize> instead so that we just need one flag? Also, I think the default should be larger, say one million or so. How much times does it cost to scan 1 million values?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could be, if dyncfgs support that, totally

Compute workers iterate arrangements synchronously while serving
index-backed peeks, so a query that scans far more rows than it returns
can hold a worker for a long time and delay everything else on the
cluster. Persist fast-path peeks have the same shape: filtering happens
after the rows have been read.

Add an off-by-default failsafe that bounds how many rows a worker may
examine for one peek. Two dyncfgs, a feature gate and a threshold that
defaults to 1000 rows, both read through handles so that an
`UpdateConfiguration` reaches peeks that are already in flight. The
budget covers the index result trace, the index error trace and the
Persist fast path, and counts rows before literal and MFP filtering,
because a row that is read and then discarded costs the same scan time
as one that is returned. Exactly the configured number of rows may be
examined. A peek fails only when it asks for the row after that.

The limit deliberately stops at the peek stash. A stashed peek restarts
its scan and produces in bounded bursts, so bounding it needs the count
to survive the hand-off, and the restart makes that count charge the
same rows twice. Leaving it out keeps this change small. The peeks that
motivate the failsafe, large filtered scans, fail before they ever reach
the stash threshold.

Reporting the limit needs an error type that survives the trip from the
worker. `PeekResponse::Error` carried a bare `String`, so every peek
failure reached the adapter as `AdapterError::Unstructured` and was
reported as XX000. Give it a `PeekError` of `Dataflow`, `Unstructured`
or `RowIterationLimitExceeded`, and let `PeekResponseUnary::Error` carry
an `AdapterError`, so the conversion happens once instead of once per
frontend. The limit then reports SQLSTATE 54000 with a hint naming the
threshold parameter, and worker responses merge by error precedence:
cancellation, then ordinary errors, then the limit.

Carrying the dataflow error structurally also fixes the SQLSTATE of
evaluation errors raised while reading a collection: `SELECT a / b FROM
t` now reports 22012 like its constant-folded counterpart. Such an error
keeps the message `DataflowError` renders, so one that used to come back
bare from an index or Persist fast-path MFP now carries the `Evaluation
error:` prefix the error-trace path already used.

The wire encoding is bincode, which cannot skip a variant it does not
know, so `PeekResponse` serializes through a mirror type that keeps
`Error(String)` where it was for the unstructured case and appends the
structured one. Existing frames, `Canceled` in particular, encode
exactly as before.

The test and CI configuration enables the feature with a high threshold,
so the guarded path is exercised broadly without constraining ordinary
queries.
@antiguru
antiguru force-pushed the compute-peek-row-iteration-limit branch from a42d56c to 23fa821 Compare August 24, 2026 18:59
@aljoscha

Copy link
Copy Markdown
Contributor Author

I think this looks fine. I'd increase the default from 1k. Why do we not extend it to the peek stash?

I didn't do it do keep the diff minimal, and they're already somewhat cooperative so not the issue we had at hand right now

antiguru added a commit that referenced this pull request Aug 26, 2026
…ts it

The design document was audited against the tree and found materially out of
sync. It described four branches and named a top layer that exists nowhere, it
claimed a #38040 guard that a slice always advances the cursor at least once,
which no version of the code has, and it counted four new dyncfgs against three
removed where the stack adds seven and removes two. It also credited #38429 with
removing an inflight cap that never landed, and said #38158 merges separately
where the stack carries it as a layer.

Three claims described mechanism that has since changed. Cancellation on tokio
is driven by the abort handle the pending peek owns, with the closed result
channel backing it up at a slice boundary rather than driving it. Writer-side
blob cleanup now exists, so state 5 states its cost instead of asking for the
work. And the semaphore is neither process-wide nor replica-wide: it is built
per serve call, so a process running two compute runtimes admits its worker
count twice, which is recorded as a known over-admission rather than left as a
contradiction between two sections.

Four surfaces the document never had are added, because they are what a reader
needs to operate the thing. The metric names that distinguish the offload
working from the offload never having been reached, the parameter scopes and the
broadcast-peek reasoning that decides them, the uuid resume ring behind "served
first next activation", and the two answers a failure produces. The stashed
answer's size bound is documented as well, since max_result_size now measures the
sum of the batches handed off rather than the prefix a scan happens to retain.

The problem section gains the open-loop measurement of this stack on staging, so
its central claim rests on the branches it heads rather than only on the earlier
two-runtime prototype.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
antiguru added a commit that referenced this pull request Aug 27, 2026
Introduces `PeekError` as a structured peek failure and adds a
configurable limit on how many rows a peek may iterate.

The first commit is @aljoscha's, taken from #38158 and authored by him.
It carries the uncommitted fixes that accompanied that patch, because
the mailbox commit alone leaves a state where the golden in
`src/environmentd/tests/testdata/http/ws` contradicts its own code: the
double-wrap in `src/environmentd/src/http/sql.rs` re-flattens the
structured error to `XX000`. Splitting the fixes out would have
attributed his work to someone else and left a red intermediate commit.
The version gate was moved to this branch's release.

`PeekError::Dataflow` is produced unconditionally, so structured error
frames flow on the wire regardless of whether the row-iteration limit is
enabled.

The second commit resets `enable_compute_peek_row_iteration_limit` after
use in the two slt files that set it.

🤖 Opened by [Claude Code](https://claude.com/claude-code) on behalf of
@antiguru


Replaces #38451, which GitHub closed when the design document moved from
the bottom of the stack to the top and its branch was force-pushed past
these commits. Same content, new base.

---------

Co-authored-by: Aljoscha Krettek <aljoscha.krettek@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
antiguru added a commit that referenced this pull request Aug 31, 2026
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 offloaded
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, and never returns
to the worker to reach the stash.

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 and 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. `PendingPeek::Stash`, `StashingPeek` and its worker-driven pump,
`start_stash_upload`, `PeekStatus::UsePeekStash` 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 the budget-based offload only: a scan that suspends
because its prefix is batch-ready is handed to a task whichever way the switch is
set, because that offload 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.

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 offloaded 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.

`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.
@aljoscha

aljoscha commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

superseded

@aljoscha aljoscha closed this Sep 1, 2026
@aljoscha
aljoscha deleted the compute-peek-row-iteration-limit branch September 1, 2026 11:03
antiguru added a commit that referenced this pull request Sep 1, 2026
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 offloaded
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, and never returns
to the worker to reach the stash.

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 and 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. `PendingPeek::Stash`, `StashingPeek` and its worker-driven pump,
`start_stash_upload`, `PeekStatus::UsePeekStash` 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 the budget-based offload only: a scan that suspends
because its prefix is batch-ready is handed to a task whichever way the switch is
set, because that offload 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.

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 offloaded 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.

`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.
antiguru added a commit that referenced this pull request Sep 1, 2026
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 offloaded
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, and never returns
to the worker to reach the stash.

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 and 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. `PendingPeek::Stash`, `StashingPeek` and its worker-driven pump,
`start_stash_upload`, `PeekStatus::UsePeekStash` 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 the budget-based offload only: a scan that suspends
because its prefix is batch-ready is handed to a task whichever way the switch is
set, because that offload 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.

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 offloaded 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.

`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.
antiguru added a commit that referenced this pull request Sep 1, 2026
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 offloaded
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, and never returns
to the worker to reach the stash.

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 and 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. `PendingPeek::Stash`, `StashingPeek` and its worker-driven pump,
`start_stash_upload`, `PeekStatus::UsePeekStash` 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 the budget-based offload only: a scan that suspends
because its prefix is batch-ready is handed to a task whichever way the switch is
set, because that offload 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.

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 offloaded 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.

`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.
antiguru added a commit that referenced this pull request Sep 1, 2026
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 offloaded
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, and never returns
to the worker to reach the stash.

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 and 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. `PendingPeek::Stash`, `StashingPeek` and its worker-driven pump,
`start_stash_upload`, `PeekStatus::UsePeekStash` 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 the budget-based offload only: a scan that suspends
because its prefix is batch-ready is handed to a task whichever way the switch is
set, because that offload 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.

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 offloaded 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.

`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.
antiguru added a commit that referenced this pull request Sep 2, 2026
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 offloaded
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, and never returns
to the worker to reach the stash.

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 and 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. `PendingPeek::Stash`, `StashingPeek` and its worker-driven pump,
`start_stash_upload`, `PeekStatus::UsePeekStash` 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 the budget-based offload only: a scan that suspends
because its prefix is batch-ready is handed to a task whichever way the switch is
set, because that offload 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.

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 offloaded 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.

`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.
antiguru added a commit that referenced this pull request Sep 2, 2026
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 offloaded
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, and never returns
to the worker to reach the stash.

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 and 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. `PendingPeek::Stash`, `StashingPeek` and its worker-driven pump,
`start_stash_upload`, `PeekStatus::UsePeekStash` 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 the budget-based offload only: a scan that suspends
because its prefix is batch-ready is handed to a task whichever way the switch is
set, because that offload 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.

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 offloaded 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.

`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.
antiguru added a commit that referenced this pull request Sep 2, 2026
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 offloaded
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, and never returns
to the worker to reach the stash.

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 and 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. `PendingPeek::Stash`, `StashingPeek` and its worker-driven pump,
`start_stash_upload`, `PeekStatus::UsePeekStash` 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 the budget-based offload only: a scan that suspends
because its prefix is batch-ready is handed to a task whichever way the switch is
set, because that offload 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.

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 offloaded 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.

`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.
antiguru added a commit that referenced this pull request Sep 2, 2026
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 offloaded
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, and never returns
to the worker to reach the stash.

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 and 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. `PendingPeek::Stash`, `StashingPeek` and its worker-driven pump,
`start_stash_upload`, `PeekStatus::UsePeekStash` 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 the budget-based offload only: a scan that suspends
because its prefix is batch-ready is handed to a task whichever way the switch is
set, because that offload 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.

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 offloaded 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.

`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.
antiguru added a commit that referenced this pull request Sep 2, 2026
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 offloaded
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, and never returns
to the worker to reach the stash.

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 and 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. `PendingPeek::Stash`, `StashingPeek` and its worker-driven pump,
`start_stash_upload`, `PeekStatus::UsePeekStash` 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 the budget-based offload only: a scan that suspends
because its prefix is batch-ready is handed to a task whichever way the switch is
set, because that offload 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.

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 offloaded 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.

`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.
antiguru added a commit that referenced this pull request Sep 3, 2026
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 offloaded
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, and never returns
to the worker to reach the stash.

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 and 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. `PendingPeek::Stash`, `StashingPeek` and its worker-driven pump,
`start_stash_upload`, `PeekStatus::UsePeekStash` 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 the budget-based offload only: a scan that suspends
because its prefix is batch-ready is handed to a task whichever way the switch is
set, because that offload 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.

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 offloaded 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.

`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.
antiguru added a commit that referenced this pull request Sep 4, 2026
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 offloaded 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, and never returns to the worker to reach the
stash. 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 and the batch builder, and takes rows through
`push` instead of through a channel the worker pumps. `Finished` 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 pushes those rows too and answers with the
stashed handle, so a stashed answer carries no rows inline. 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`, `PeekStatus::UsePeekStash` and
`PEEK_STASH_NUM_BATCHES` are all gone. `PEEK_STASH_BATCH_SIZE`, which
counted rows, becomes `compute_peek_response_stash_batch_bytes`: the
first batch is cut at the stash threshold, since that is the size that
decides an answer is not an inline one, and every later batch at this
size, so how much one hand-over carries is set apart from when to stash.
Each hand-over is a round trip through the blocking pool, and a scan
retains up to this much between them.

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 the budget-based offload
only: a scan that suspends because its prefix is batch-ready is handed
to a task whichever way the switch is set, because that offload 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 offloaded 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. `DeliveredBatch` covers the window
inside `finish`, where persist has taken the builder and no upload holds
the parts any more: dropping one undelivered deletes the batch.

Reclaiming those blobs finishes a builder whose part write may have been
in flight when its walk was aborted. Persist used to panic on that, and
a panic in this replica aborts the process; #38577 made the pending
write cancel safe, so `abandon` finishes the builder and deletes the
batch without a guard. A replica that dies mid-upload still leaves its
parts behind, which this path tolerates.

`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:

* `max_result_size.slt` drives a stashed fast-path peek through the
adapter reader only as far as the `max_query_result_size` rejection. No
test in `test/` reads a stashed answer back whole.
* 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.

🤖 Opened by [Claude Code](https://claude.com/claude-code) on behalf of
@antiguru


Replaces #38482, which GitHub closed when the design document moved from
the bottom of the stack to the top and its branch was force-pushed past
these commits. Same content, new base.
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.

2 participants