compute: walk an expensive index peek off the serving worker

[antiguru]

Moves an expensive fast-path index peek's walk off the timely worker that received it, so a long scan no longer delays the peeks queued behind it. This is the layer the ones beneath it were built for.

A peek runs its first slice inline under a small budget, sized so point lookups finish there and nothing else does. If it completes, the peek never leaves the worker. If it outruns the budget it is offloaded to a task that walks the rest on the blocking pool, holding a permit from a process-wide semaphore. The walk stays on its blocking thread, checking for cancellation every few thousand positions, and returns to the runtime only to answer: a round trip per slice costs two thread wakes, which is more than the slice itself on a small machine. Cost is measured rather than predicted, so a skewed point lookup over a hot key needs no special case: it enters as a point lookup, overruns, and offloads.

The offload is for a scan that suspended with its budget spent, never for one holding a full batch. An offloaded task has nowhere to write a batch until the stash becomes a state transition of the same scan, and stepping a batch-ready scan returns without spending fuel or advancing, so offloading one would spin. A scan that fills a batch while offloaded hands back and the worker starts the existing stash walk.

The permit travels with the scan rather than with the pending peek, so releasing it coincides with releasing the batches it accounts for. Permits default to this runtime's worker count, which preserves the ceiling that exists today: a peek blocking its worker already costs one core per worker. Excess scans queue in the semaphore, so a peek storm costs queue entries rather than threads.

Cancellation needs no new mechanism at any stage. Removing the pending peek drops the result channel's receiver, and the walk observes a closed sender at its next cancellation check; a scan still waiting for a permit leaves the queue the same way. Permits release on drop, including on panic.

The peeks awaiting a turn on the worker and the peeks a driver has taken over sit in separate queues, because they need opposite treatment. The former all draw on one per-activation aggregate that does not refill within an activation, so the sweep serves them from the front and stops at the first peek the budget cannot serve: every peek behind it would be passed over for the same reason. The latter draw no budget, since their work is not on the worker, so the sweep polls all of them. A peek that gets no turn keeps its place and is served first on the next activation.

Off by default in production and on in the test configuration. With the switch off nothing is offloaded and placement is unchanged.

Both substrate counters are pre-resolved, so mz_index_peek_walks_total{substrate="offloaded"} reads zero rather than being absent, which keeps "the offload changed nothing" distinguishable from "the offload never engaged".

Reading mz_index_peek_total_seconds across the flag is misleading: an offloaded peek contributes only its inline slice, which the inline budget bounds, so the expensive peeks leave a bounded sample where they used to leave the whole walk. mz_index_peek_offload_seconds and the per-phase histograms are the honest pairing. mz_index_peek_offload_seconds is wall clock away from the worker, so it counts the wait for a permit, not the walk's own time.

🤖 Opened by Claude Code on behalf of @antiguru

Replaces #38478, 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.

[antiguru]

Review findings, posted by Claude Code on behalf of @antiguru. Line numbers are at 51b8da6c22.

1. The offloaded diversion drops a soundness guard the inline driver keeps

peek_offload.rs:289-292:

ScanOutcome::Suspended if scan.batch_ready() => {
    metrics.observe_error_phase(&scan.phases());
    return Some(OffloadOutcome::NeedsStash);
}

The inline driver, at the same decision point (compute_state.rs:2043-2052), refuses the diversion unless the error trace is read out:

if !scan.error_trace_clean() {
    soft_panic_or_log!("peek on {} suspended before its error trace was read out", ...);
    return PeekStatus::Ready(PeekResponse::Error(...));
}

Both drivers make the same decision, handing a peek to a stash walk that reads the ok trace only. Its own comment says "the guard states that rather than assuming it", and the offload path assumes it. If anything ever lets the error phase accumulate rows, or batch_ready stops consulting peek_stash_eligible, the inline path fails the peek and the promoted path silently returns rows for a collection that contains an error.

Worth noting for whoever fixes it: git grep error_trace_clean at the top of the stack (3cd9c996a3) finds it only in peek_metrics.rs and peek_scan.rs. The guard is gone from both drivers by the end of the stack, which is consistent, because #38510 makes the stash a state transition of the same scan and there is no second ok-trace-only walk to divert into. So the guard is load-bearing for exactly this layer's lifetime and then deleted, and no commit message says so. Either mirror it here or record why it is unnecessary.

2. The permit does not bound the memory the module doc says it bounds

peek_offload.rs:9-11:

The task owns both the scan and the permit that admitted it, which is what ties the bound on concurrent walks to the memory those walks retain

False for a queued walk. The permit is acquired inside the task at :191, after the scan has already moved in, and the queue behind the semaphore is deliberately unbounded. A queued scan retains its accumulated RowBatch and the Vec<Tr::Batch> its cursors were opened over, which pins those arrangement batches against merging.

Concretely: 500 expensive peeks promote on a two-worker process, two run, 498 sit in the queue, each holding pinned batch handles and up to the stash threshold of rows. Retained memory scales with promoted-and-queued walks, which nothing bounds. INDEX_PEEK_PERMITS's "costs queue entries rather than threads" understates an entry by a whole scan. Either bound the queue or say what an entry costs, because as written the doc invites sizing the permit count as a memory bound.

3. The permit count equals the tokio runtime's worker-thread count

server.rs:185-192 justifies the default by the CPU ceiling, one core per worker. That accounts for cores but not for which threads. The walk is an async task, deliberately not spawn_blocking, so it runs on the process's shared runtime alongside persist IO, gRPC and heartbeats. Where workers_per_process == cores, a saturated cap puts a CPU-bound walk on every tokio worker thread at once, and at the default INDEX_PEEK_YIELD_GRANULARITY of 10000 with the dyncfg's own 100ns to 1us per position a slice runs up to ~10ms between yields. The two-runtime note doubles the count.

The PR already knows this: misc/python/materialize/parallel_workload/action.py caps its coarse granularity at 100000 precisely because "the permit count is not sized to leave threads over for the rest of the process's async work". The production default does not act on it. This is the number worth measuring before the flag is armed anywhere real; at minimum server.rs's comment should state the thread-occupancy consequence and not only the core count.

4. The semaphore's reach is right in one place and wrong in four

server.rs:187-191 is correct: the bound covers the workers of one serve call, and the process admits the count once per role. Contradicted by:

• INDEX_PEEK_PERMITS's doc, "How many promoted index peek scans may run at once in one replica process", and "The count is per process rather than per replica because the semaphore that enforces it lives in one process" — the stated reason is the exact fact that is false;
• the same config's user-visible description, which is what reaches LaunchDarkly and the docs;
• compute_state.rs:244-245, "shared with every other worker in this process";
• server.rs:331-332, "shared by every worker in this process".

An operator sizing the flag from its description under-counts admissions by 2x on a two-runtime process.

5. Test coverage: permit accounting is pinned only on the cancellation path, and the tokio cancellation mechanism nowhere

• peek_offload/tests.rs:453-507 is the only test where a walk runs to an answer with a semaphore in scope, and it never checks available_permits() afterwards. A walk leaking its permit on the Complete arm would consume one per successful peek until the replica promoted nothing ever again, and the suite stays green. Same gap on NeedsStash. The module doc's "every way the task ends, including a panic, drops the two together" has no test at all.
• peek_sweep_tests.rs:804-838 is the only test that drops a real PendingPeek::Offloaded, and its own comment concedes the cancellation lands before the task has been polled. Dropping the entry also closes the oneshot, so the task's first poll takes the result_tx.closed() branch at peek_offload.rs:198 and produces an identical walks() == (0, 0). Delete _abort_handle from OffloadedPeek and every test still passes. The documented mechanism is unpinned; only its backstop is.
• Cancellation state 5, mid stash upload, has no test. handle_cancel_peek appears four times in peek_sweep_tests.rs and never against a PendingPeek::Stash, which this PR newly makes reachable through a promoted hand-back.
• No test runs two promoted walks against one permit. peek_offload/tests.rs:448-451 says outright that the permit is held by the test rather than by a second walk, so a promote that acquired nothing would pass.
• promote's permits.resize(config.permits.get()) is untested, since every test runs at the default of 0. Nothing pins that default_permits is workers_per_process either.
• The TryRecvError::Closed arm at compute_state.rs:1353-1361 is unreached by any test and emits a user-visible error.

6. A fixture disagrees with its own schema

index_peek_tests.rs:125-126 builds result_desc with one non-nullable SqlScalarType::UInt64; ok_row at :49-51 packs Datum::UInt8. Every index_peek plus ok_row fixture is a peek whose result description disagrees with the rows answering it. peek_sweep_tests.rs:1019-1020 documents the problem and routes around it with wide_ok_rows rather than fixing it. It bites the first time an ok_row test is extended through the stash, where the description is the write schema.

Minor

• peek_offload.rs:174-178 says a precondition "is checked rather than assumed", but debug_assert! compiles out under [profile.optimized] and release, which is what bin/environmentd and mzcompose build. Not a side-effect violation, and the caller does guarantee it, but the doc should say the check is CI-only.
• peek_stash.rs:45-47 removes the comment explaining why the channel exists ("the underlying trace reader is not Send/Sync", now false since peek_scan.rs asserts IndexPeekScan: Send) without supplying a replacement, so the channel reads as unnecessary.
• ResponseSender::new and set_nonce widen from private to pub(crate) solely for tests. Worth a #[cfg(test)] or a comment.
• A walk whose task panics counts on neither substrate label, yet the peek is answered with an error. mz_index_peek_walks_total's help mentions only cancellation.

Checked and clean

Permit lifetime and drop order in the running task: walk's _permit: OwnedSemaphorePermit first parameter genuinely outlives the scan by reverse-order parameter drop, every exit including an unwind drops both, and the tokio::select! over acquire_owned() against result_tx.closed() is cancel-safe on both arms. PermitWait decrements the depth gauge from Drop, so admission, cancellation and abort all leave the queue. PeekPermits::resize cannot overflow Semaphore::MAX_PERMITS.

The resume ring: no starvation found. get_or_insert preserves an earlier sweep's resume point against a later-arriving lower uuid, a resume point implies remaining == 0 so an arriving peek cannot jump ahead of a passed-over one, a stale uuid resolves via partition_point to the next live peek, the is_empty early return clears what handle_cancel_peek and reconcile leave behind, and every activation grants at least one full per-peek budget so the re-activation loop always progresses.

The kill switch restores placement byte-for-byte: Unbounded makes grant() return usize::MAX, promotion becomes unreachable, no peek is passed over, no resume point is left, and process_peeks takes the cheap arm that iterates the map in its own order. Both zero sentinels behave as documented. The substrate label pair sums to walks that ended. assert_send::<IndexPeekScan>() is real, since the scan stores cursors and Arc'd batches rather than trace handles.

Conventions clean: no as conversions, no std::collections::HashMap/HashSet, no em-dashes or structuring semicolons in new comments, no side effects in any debug_assert!, metrics.yml regenerated consistently for all four new metrics and all six edited helps, the flag off in code and on in bin/sqllogictest and get_minimal_system_parameters gated at the current version, and all five names in both LaunchDarkly allowlists.

[antiguru]

Findings addressed, posted by Claude Code on behalf of @antiguru. Your inline comments are answered on their own threads.

The diversion guard is no longer asymmetric. The inline driver refuses to divert a peek to the stash unless its error walk is over, because the stash answers from the ok trace alone and a peek diverted with its error trace half-read would return rows where it owes an error. The promoted driver made the same decision without the guard, so it now mirrors it. It is load-bearing for this layer only: #38510 keeps one scan across the stash boundary, and both guards go with the diversion they guarded.

Two properties the tests did not pin. A walk that answers has to return its permit, or the bound shrinks by one per promoted peek until the replica admits nothing. And dropping a promoted peek has to abort the walk: every cancellation test also dropped the receiving end of the answer channel, so the walk stopped on the closed-channel backstop and deleting _abort_handle passed the whole suite. The new test keeps that channel open, so only the handle can end the walk, and it fails without it.

What the permit bounds, and how far it reaches. The module doc said the permit ties the concurrency bound to the memory those walks retain. It does not: a promoted walk that has not been admitted sits in the queue holding its scan, which retains its accumulated rows and pins the batches its cursors were opened over, so retained memory grows with promoted walks rather than running ones. And the semaphore covers the workers of one serve call, which four comments called a per-process bound. Both corrected, along with a note that a promoted walk is an async task, so a saturated bound can occupy every runtime worker thread for up to a yield granularity at a time.

Smaller. The debug_assert on promotion is a CI-only check, not one an optimized build makes. The stash channel carries rows across threads because the walk stays on the worker, not because a trace reader is !Send. A walk whose task panics is counted on neither substrate, which the counter's help attributed to cancellation alone. ResponseSender::new says why it is crate-visible. And the fixture peeks now pack the datum type their result description declares, which one test already routed around.

[antiguru]

Second round addressed, posted by Claude Code on behalf of @antiguru. Each of your eight comments is answered on its own thread; this is what changed beyond them.

The permit bound is a plain fraction. Config<f64>, default 1.0, no nano-fractions and no unit constant. COLUMN_PAGED_BATCHER_BUDGET_FRACTION and MEMORY_LIMITER_BURST_FACTOR in the same file already spell a fraction that way. A negative or NaN value lands on the floor of one scan rather than being rejected, because a bound nobody can read should pace promotion rather than stop it.

Three clones became moves. The sweep matched the pending peek by reference, so the arms that hand a peek to another driver cloned it and its trace bundle instead of taking them. TraceBundle::clone clones two trace handles, each registering a reader. The arms take ownership now, and start_stash_upload takes the stash location as an argument so the caller that must answer a peek when the replica has none settles that before the move rather than needing the peek back after.

The dyncfg defaults test is gone. Its scope and value assertions restated the literals a few lines above them, and its two ordering assertions constrain only the defaults, which an operator may override without either check running.

A verbosity pass over the stack, not only the lines you marked. peek_offload.rs 330 lines to 303 for the same code, peek_metrics.rs 197 to 183, and the same pass over the five dyncfg docs, the sweep's comments in compute_state.rs, and both places in server.rs. Every metric help this stack writes or edits is one sentence, across both layers. The exception is mz_index_peek_total_seconds, which keeps a clause: that metric changes meaning when the flag flips, so a reader comparing windows either side of a flip would read a latency drop that is really a change of subject.

#38510 and #38449 are rebased on top and repushed.

[def-]

QA LLM Review

1. MEDIUM -- parallel-workload still varies the permit bound in nano-fractions

misc/python/materialize/parallel_workload/action.py:3109

Details

compute_index_peek_permit_fraction was converted from a nano-fraction usize to a plain f64 fraction in 200bda2137, but the stress values in FlipFlagsAction were left at the old units. All three values now land on "effectively unbounded" or "the default", so the permit-contention path this knob was added to stress is never exercised.

The list is:

# One permit per worker (the default), a bound that serializes every
# promoted walk, and one that never queues. Expressed in
# nano-fractions of the process's worker count.
self.flags_with_values["compute_index_peek_permit_fraction"] = [
    "1000000000",
    "1",
    "1000000000000",
]

Against INDEX_PEEK_PERMIT_FRACTION: Config<f64> (default 1.0) and PeekPermits::permits_for, these read as 1e9, 1.0 and 1e12, i.e. workers * 1e9 permits (clamped to Semaphore::MAX_PERMITS), the production default, and workers * 1e12 permits. The intended "serializes every promoted walk" case is gone: nothing in the workload ever configures a bound low enough to force a walk to wait, so PeekPermits' queueing and the cancel-while-queued path (result_tx.closed() in the tokio::select!) get no concurrent stress. The neighbouring f64 flag in the same file, column_paged_batcher_pool_rss_target_fraction, is already written as plain decimals ("0.0", "0.01", "0.02"), which is the convention this one should follow.

permits_for floors at one permit total, so a tiny fraction is the serializing value:

# One permit per worker (the default), a bound that serializes every
# promoted walk, and one that never queues.
self.flags_with_values["compute_index_peek_permit_fraction"] = [
    "1.0",
    "0.0001",
    "1000.0",
]

2. LOW -- the new .slt never runs its queries with the offload off

test/sqllogictest/index_peek_offload.slt:15

Details

The file states that every query "has to answer exactly as it does with the offload off, and each is run both ways", but sqllogictest runs with enable_compute_index_peek_offload=true because this same PR adds it to get_minimal_system_parameters, which SqlLogicTest passes through MZ_SYSTEM_PARAMETER_DEFAULT. The first half of the file therefore runs with the flag already on, and the trailing ALTER SYSTEM RESET enable_compute_index_peek_offload (line 181) restores true, not false.

The assertions are still correct answers, so nothing passes that should fail; what is lost is the A/B the file says it performs. The flag-off placement is covered by peek_sweep_tests::the_kill_switch_answers_the_same_scan_inline, so the gap is in this file's stated purpose rather than in overall coverage. Either drop the "both ways" framing, or set enable_compute_index_peek_offload to false explicitly at the top of the baseline section and back to the default at the end.

[antiguru]

Both findings are right and both are fixed in 7466a3ffb5, the commit "compute: make the offload's two test knobs exercise what they claim". The branch is also rebased onto upstream/main at 951d074c87.

1. The permit bound in nano-fractions. Confirmed: the values were left behind when the parameter became a plain f64, so read as fractions they are 1e9, 1.0 and 1e12. Every one of them is either the production default or effectively unbounded, which means nothing in the workload ever configures a bound low enough to make a promoted walk queue. The permit-contention path the knob exists to stress, including cancelling a walk while it waits on result_tx.closed(), went unexercised. Now:

# One permit per worker (the default), a bound that serializes every
# promoted walk, and one that never queues. A fraction of the process's
# worker count, floored at one permit, so any tiny fraction serializes.
self.flags_with_values["compute_index_peek_permit_fraction"] = [
    "1.0",
    "0.0001",
    "1000.0",
]

The serializing value relies on the floor in permits_for, as you noted, and the plain decimals match column_paged_batcher_pool_rss_target_fraction in the same file.

2. The .slt never runs with the offload off. Also confirmed: this PR adds the flag to get_minimal_system_parameters (misc/python/materialize/mzcompose/__init__.py:150), so the baseline section inherited true and the file compared the offloaded walk against itself. Rather than drop the "both ways" framing I made it true, since the A/B is the point of the file: the baseline now sets the flag to false explicitly, with a comment saying why, and the existing ALTER SYSTEM RESET at the end restores the configured default as before. The file passes 30/30.

Posted by Claude Code on behalf of @antiguru.

[def-]

QA LLM Review

1. MEDIUM -- the offloaded walk builds its answer unsliced on a tokio runtime worker thread

src/compute/src/compute_state/peek_offload.rs:288

Details

Every slice of an offloaded walk is stepped on the blocking pool, but the terminal step that turns the accumulated rows into the answer runs inline in the async task: metrics.rows_response(rows, order_by) sorts and re-copies the whole row set on a tokio runtime worker thread, with no fuel bound and no yield point. A peek whose finishing is not streamable accumulates until it approaches max_result_size (1 GB by default), so one such peek can hold a runtime worker thread for seconds, and the default permit bound admits one of these per timely worker at once.

walk routes each slice through mz_ore::task::spawn_blocking (peek_offload.rs:269) precisely because "a slice is CPU-bound for its whole length and would otherwise hold an async worker there". The ScanOutcome::Finished(Ok(rows)) arm then calls rows_response directly (peek_offload.rs:283-289), which converts every count and calls RowCollection::new (peek_metrics.rs:130-146). That sorts the whole Vec<(Row, NonZeroUsize)> with a RowComparator and copies every row into a fresh buffer (src/expr/src/row/collection.rs:70-88) -- row_collection_seconds exists to measure exactly this cost.

How large the row set gets: PeekScan::batch_ready fires only for a stash-eligible scan, and RowSetFinishing::is_streamable requires an empty order_by and an identity projection. A fast-path index peek carrying ORDER BY with no LIMIT is therefore never diverted to the stash, and with no limit num_rows_needed() is None so thin() never runs. It accumulates until total_size > max_result_size, and it outruns the 1024-position inline budget long before that, so it is offloaded and the sort lands on the runtime rather than on the worker.

This is the one step the module doc's "neither the timely worker nor an async one carries the walk" (peek_offload.rs:8-9) does not hold for, and the one INDEX_PEEK_YIELD_GRANULARITY does not bound. With INDEX_PEEK_PERMIT_FRACTION at its default of one walk per timely worker, a replica whose worker count matches the runtime's worker-thread count can have every runtime thread inside one of these sorts, stalling persist IO completions, the controller's command/response stream and heartbeats in the same process. Off by default in production, on in the test configuration.

Fix: give the terminal answer the same treatment as a slice. Either build it inside the spawn_blocking closure (return a finished PeekResponse rather than a RowBatch, moving a cloned PeekWalkMetrics and the order_by in), or wrap the single rows_response call in its own spawn_blocking after the loop.

[antiguru]

Confirmed and fixed in 00926d8a24, with the merge carried through 9bf89cf5cf and d4a82496de.

The finding is right on every step. RowCollection::new sorts the whole Vec<(Row, NonZeroUsize)> with a RowComparator and then copies every row into a fresh buffer, and the arm called it directly on the async task. The size argument holds too: is_streamable requires an empty order_by, so a peek carrying ORDER BY is never diverted to the stash, and without a LIMIT its num_rows_needed() is None, so nothing thins the accumulation before max_result_size. That is the one step yield_granularity does not bound, and the module doc's claim that neither the timely worker nor an async one carries the walk did not hold for it.

Fixed with the second of the two suggested shapes, wrapping the single call:

return Some(match upload {
    // Onto the blocking pool for the same reason a slice goes there:
    // building the answer sorts and copies the whole row set, and a
    // finishing that carries an order never reaches the stash, so it
    // accumulates the whole result before it can.
    None => {
        let answer_metrics = metrics.clone();
        let order_by = order_by.to_vec();
        mz_ore::task::spawn_blocking(
            || "peek_offload::answer",
            move || answer_metrics.rows_response(rows, &order_by),
        )
        .await
    }
    Some(upload) => stashed_answer(peek_uuid, upload, rows).await,
});

Building it inside the slice closure would have to move the metrics and the order into every slice to serve the one that turns out to be terminal, so the wrapper keeps the slice loop as it is. PeekWalkMetrics is already Clone for exactly this reason, and the clone plus the order_by copy happen once per walk.

The stashed arm is left as it is. StashUpload::finish also builds a RowCollection, but only over the inline rows, which are what remains below the batch threshold the scan already cut on, so that one is bounded by construction rather than by max_result_size.

Posted by Claude Code on behalf of @moritz*.*

[antiguru]

Two fixes here, both found while reading rather than reported.

compute_state/tests.rs was compiling nowhere. This layer adds index_peek_tests and peek_sweep_tests to the foot of compute_state.rs and dropped the #[cfg(test)] mod tests; that stood there. main declares it, peek/bounded-scan and peek/scan still declare it, and from this commit up the file was orphaned: row_iteration_limit_observes_updates_and_disabled_rows, which covers PeekRowIterationTracker against live config updates, never built and never ran. The declaration is back and the test passes.

The offloaded-walk counter comment said the opposite of what the code does. It read as though counting past the walk was what caught a cancelled walk. It is what excludes one: walk returns None on cancellation and the task returns before the counter, and an aborted task never reaches the line at all. walked_offloaded's own doc owns that contract ("A walk cancelled while queued or while running counts on neither substrate"), and an_aborted_walk_deletes_the_parts_its_upload_wrote asserts both counters stay at zero, so the call site now states the placement decision and points at the doc for the rest.

Posted by Claude Code on behalf of @antiguru.

[antiguru]

Three things from the same read.

order_by is shared now, not copied twice. It was cloned out of the peek in start, handed to walk as a slice, and copied back into a Vec inside the answer arm because spawn_blocking needs it owned. It is an Arc<[ColumnOrder]> created once, passed into walk by value, and cloned as a pointer where the blocking closure needs its own handle. A walk that answers from the stash never touches it at all, and a walk that answers inline no longer allocates for it a second time.

ActivationBudget's Unbounded stays, and here is why it is not a redundant enum. Collapsing it to Bounded { per_peek: usize::MAX, remaining: usize::MAX } would make charge accumulate under the kill switch, where today it is a no-op. In practice nothing reaches the difference: charge is handed fuel - unspent, which is the positions actually walked, so draining usize::MAX needs about 1.8e19 of them in one activation. But the_kill_switch_grants_every_peek_an_unbounded_slice pins the stronger property on purpose, charging usize::MAX three times and still expecting an unbounded grant, and the kill switch is the thing you reach for when the offload is misbehaving in production. "Provably inert" is worth an enum and two match arms over "inert by an arithmetic argument about how many cursor positions fit in a usize".

InlineBudget::activation stays an Option, but its doc gave the wrong reason. It said the constructor cannot arm the budget because CreateInstance precedes the UpdateConfiguration carrying the parameters. CreateInstance carries initial_config and handle_create_instance applies it, so that reading is stale. The reason it actually cannot is one step earlier: InlineBudget::new runs inside ComputeState::new, before handle_create_instance applies that snapshot, and an empty snapshot leaves dyncfg defaults in place until the first UpdateConfiguration. The offload's own flag defaults off, so an eagerly armed budget is an unbounded one.

Refilling at start_activation alone does not close that either, because handle_pending_commands drains before the sweep that begins an activation, so a peek arriving on that path is granted a slice before any activation has begun. Option plus get_or_insert_with is what reads the parameters as late as anything can: at the first peek that actually asks for fuel. The doc says that now.

Posted by Claude Code on behalf of @antiguru.

[petrosagg]

We should leave a note here about why we take it out of self. I assume it's to be able to call walk_index_peek below which is a &mut Self receiver method

[antiguru]

Right, that is the reason. Noted in 2be9565e11:

// Both queues are taken out of the state for the sweep, because serving a peek borrows
// `self` mutably. A peek the sweep returns for another turn lands in the emptied queue.

The second sentence is the part worth knowing: walk_index_peek pushes a not-yet-ready peek back into self.compute_state.queued_peeks while the sweep holds the taken copy, which is why the tail of the function merges the two rather than assigning.

🤖 Posted by Claude Code on behalf of @antiguru

[petrosagg]

What ensures that we never suspend mid-error walk. From my understanding we fuel the error walk so it should be possible to stop while not having read everything.

What would happen if we unconditionally returned PeekStatus::Offload(_) in both cases?

[antiguru]

Nothing prevents suspending mid-error walk, and that case is fine: it takes the arm above this one. A suspension during the error walk has no batch, because only the ok walk accumulates rows, so !scan.batch_ready() holds and the scan goes to PeekStatus::Offload, which resumes it where it stopped. The guard here is on the other arm only, the one that hands the peek to the stash, and it can fail only if a scan holds a full batch without having finished the error walk, which the scan's structure rules out. It states the invariant rather than assuming it.

Unconditional Offload in both cases would wedge here: at this layer the offloaded driver writes nothing, and a scan holding a full batch makes no progress until the batch is taken, so the walk would spin on a suspension it cannot resolve. #38510 is where that becomes the right answer. There the offloaded walk owns the stash upload, so it does exactly what you describe: both suspensions go to Offload, and UsePeekStash and this guard disappear with the second walk.

🤖 Posted by Claude Code on behalf of @antiguru

[linear-code]

CPU-195

[antiguru]

45f8a74901 changes how the offloaded walk uses the blocking pool, after the feature benchmark flagged FastPathFilterNoIndex at 2.2x slower with the offload on.

That scenario is 100 filter peeks over a million-row index on one worker, each rejecting every row. Every peek offloads, and the walk was cut into slices of yield_granularity positions, each one a spawn_blocking round trip: blocking thread finishes, tokio worker wakes, spawns the next slice, blocking thread wakes. A hundred slices per peek, two thread wakes each, on a machine where a wake is not cheap. Locally the same binary with the flag on ran 13% slower than with it off.

The walk now stays on its blocking thread for as long as it has nothing to await. It checks for cancellation and re-reads its configuration every yield_granularity positions, and returns to the async task only with an answer or, in #38510, a batch to write. The scan, the permit and the result channel's sender cross to the pool and back together as one WalkState, so an aborted task cannot separate them: the permit is released when the thread that was walking is done, not when a task that was merely awaiting it is dropped. Locally the flag now costs 3% on that scenario, under the 10% threshold.

compute_index_peek_yield_granularity keeps its name, but it no longer yields to the runtime, so its description now says what it does bound.

🤖 Posted by Claude Code on behalf of @antiguru

[def-]

QA LLM Review

1. MEDIUM -- lowering INDEX_PEEK_PERMIT_FRACTION never takes effect while walks are queued

src/compute/src/compute_state/peek_offload.rs:93

Details

PeekPermits::resize shrinks the bound with Semaphore::forget_permits, which only reduces the semaphore's available permits. Once every permit is held and a walk is queued behind them, released permits go straight to the queued waiter and never become available, so the shrink returns 0 forever and the concurrency bound stays at whatever it was. The lever is inert in exactly the overload it exists for, and a raise cannot be rolled back under load.

The doc on resize (peek_offload.rs:82-84) states the opposite: "The next call takes back what the walks that finished returned." Tokio's forget_permits computes available.saturating_sub(n) and returns min(available, n), and OwnedSemaphorePermit::drop → Semaphore::add_permits → add_permits_locked assigns to waiters before it touches the available count. So with any waiter present at each release, available stays 0.

Concretely, with workers = 2: two walks admitted, a third queued, operator sets the fraction to 0.5 (target 1). Every subsequent offload calls resize(0.5), each forget_permits(1) returns 0, granted stays 2, and the two-walk bound persists for as long as a walk is queued at each hand-back. The reduction lands only once the queue has drained, at the next offload. resize is also the sole trigger: with no further offloads nothing re-attempts the shrink.

permits_resize_around_the_walks_holding_them (peek_offload/tests.rs) covers the case where the shrink finds a free permit, so the waiter case is untested.

Fix: record the target alongside granted and absorb the reduction on release rather than on the next resize. Wrap the permit WalkState holds so that its drop asks PeekPermits whether a shrink is pending, and calls OwnedSemaphorePermit::forget() (drops the permit without returning it to the semaphore) with granted decremented when it is. The shrink then converges as the walks holding permits finish, which is what the doc promises, and a walk under way is still never interrupted.

[antiguru]

Confirmed and fixed in a0ec303b24. The mechanism is as you describe: add_permits_locked assigns a released permit to a waiter before it touches the available count, so with a walk queued at every release forget_permits finds nothing to forget and the shrink never lands. The old resize doc promised the opposite.

PeekPermits now keeps target beside granted. resize still forgets whatever is free at once, and the rest is absorbed on release: the permit a walk holds is a WalkPermit whose drop asks whether a shrink is pending and calls OwnedSemaphorePermit::forget instead of returning it, decrementing granted, until granted meets target. A raise still takes effect immediately, and a walk under way is still never interrupted.

Pinned by a_shrink_that_finds_every_permit_held_lands_as_walks_finish: two permits held, a third walk queued, resize to one. The first release is absorbed and the queued walk stays queued, the second admits it, and the bound is one afterwards. It fails against the old code at the first of those assertions.

🤖 Posted by Claude Code on behalf of @antiguru