Skip to content

Commit d8a92f4

Browse files
antiguruclaude
andcommitted
compute: cover the cancellations that land after a peek stopped waiting
Three cases the sweep reasons about and asserted nowhere. A promoted walk is counted where it reached its outcome, and the outcome then travels back to a worker that reads it on its next sweep. A cancellation landing in that window answers the peek as cancelled while the walk stays counted, which is what the walk counters are documented with and what makes them the count of the walks each substrate finished rather than a lower bound on it. Cancelling a peek that was passed over for want of budget leaves the resume point naming a uuid the map no longer holds, which is the peek a caller is most likely to cancel because it is the one that has been waiting. The resume point is a position in the uuid ordering rather than a peek, so the sweep resumes at the first surviving peek that sorts at or after it, and one that sorts past every surviving peek wraps to the first of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 377c668 commit d8a92f4

1 file changed

Lines changed: 178 additions & 1 deletion

File tree

src/compute/src/compute_state.rs

Lines changed: 178 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2575,6 +2575,25 @@ mod peek_sweep_tests {
25752575
}
25762576
panic!("peeks were still pending after {SWEEP_BOUND} activations");
25772577
}
2578+
2579+
/// Runs the runtime until the two substrates have counted `walks` walks between them,
2580+
/// without sweeping.
2581+
///
2582+
/// Bounded, so a walk that never reaches an outcome fails here rather than hanging the
2583+
/// suite. No sweep runs, so a promoted walk that finishes here leaves its outcome sitting
2584+
/// in the channel that carries it back, which the worker has not yet read.
2585+
async fn drive_until_walks(&self, walks: (u64, u64)) {
2586+
for _ in 0..SWEEP_BOUND {
2587+
if self.walks() == walks {
2588+
return;
2589+
}
2590+
tokio::task::yield_now().await;
2591+
}
2592+
panic!(
2593+
"the substrates counted {:?} rather than {walks:?} within {SWEEP_BOUND} yields",
2594+
self.walks()
2595+
);
2596+
}
25782597
}
25792598

25802599
/// A harness with the offload on and every budget at its production default, which is the
@@ -2958,6 +2977,110 @@ mod peek_sweep_tests {
29582977
);
29592978
}
29602979

2980+
/// A resume point naming a peek that has since been cancelled still names where the next sweep
2981+
/// starts.
2982+
///
2983+
/// The peek a caller cancels is disproportionately one that was passed over for want of
2984+
/// budget, because that is the peek that has been waiting, and cancellation removes it from
2985+
/// the map without touching the resume point. What the resume point names is a position in the
2986+
/// uuid ordering rather than a peek, so the sweep resumes at the first surviving peek that
2987+
/// sorts at or after it and the peeks that were served ahead of it still go last.
2988+
#[mz_ore::test(tokio::test)]
2989+
async fn a_resume_point_outliving_its_peek_still_names_where_to_resume() {
2990+
let keys = wide_ok_rows(SMALL_INDEX_KEYS);
2991+
let answer = whole_index_answer(&keys);
2992+
2993+
let mut harness = with_activation_budget(1);
2994+
for uuid in [PEEK_B, PEEK_C] {
2995+
harness.add_pending(
2996+
index_peek_with_uuid(uuid, None),
2997+
trace_bundle(&keys, cancelling_errors(0)),
2998+
);
2999+
}
3000+
3001+
harness.sweep();
3002+
3003+
assert_eq!(harness.peek_responses(), vec![(PEEK_B, answer.clone())]);
3004+
assert_eq!(harness.state.peek_resume_at, Some(PEEK_C));
3005+
3006+
harness.active().handle_cancel_peek(PEEK_C);
3007+
3008+
assert_eq!(
3009+
harness.peek_responses(),
3010+
vec![(PEEK_C, PeekResponse::Canceled)]
3011+
);
3012+
assert_eq!(
3013+
harness.state.peek_resume_at,
3014+
Some(PEEK_C),
3015+
"cancelling a peek leaves the resume point naming it"
3016+
);
3017+
3018+
// Peeks arrive on both sides of the cancelled one in the uuid ordering.
3019+
for uuid in [PEEK_A, PEEK_D] {
3020+
harness.add_pending(
3021+
index_peek_with_uuid(uuid, None),
3022+
trace_bundle(&keys, cancelling_errors(0)),
3023+
);
3024+
}
3025+
3026+
harness.sweep();
3027+
3028+
assert_eq!(
3029+
harness.peek_responses(),
3030+
vec![(PEEK_D, answer)],
3031+
"the sweep resumes at the first peek sorting after the cancelled one"
3032+
);
3033+
assert_eq!(harness.state.peek_resume_at, Some(PEEK_A));
3034+
}
3035+
3036+
/// A resume point sorting past every pending peek starts the next sweep at the first of them,
3037+
/// which is where a ring wraps to.
3038+
///
3039+
/// This is the end of the ordering, where the rotation is by the whole length of it. Rotating
3040+
/// a sweep's peeks by their own count leaves them where they were, and where they were is the
3041+
/// wrap the ring owes the peeks that sort ahead of the resume point.
3042+
#[mz_ore::test(tokio::test)]
3043+
async fn a_resume_point_past_every_pending_peek_wraps_to_the_first() {
3044+
let keys = wide_ok_rows(SMALL_INDEX_KEYS);
3045+
let answer = whole_index_answer(&keys);
3046+
3047+
let mut harness = with_activation_budget(1);
3048+
for uuid in [PEEK_C, PEEK_D] {
3049+
harness.add_pending(
3050+
index_peek_with_uuid(uuid, None),
3051+
trace_bundle(&keys, cancelling_errors(0)),
3052+
);
3053+
}
3054+
3055+
harness.sweep();
3056+
3057+
assert_eq!(harness.peek_responses(), vec![(PEEK_C, answer.clone())]);
3058+
assert_eq!(harness.state.peek_resume_at, Some(PEEK_D));
3059+
3060+
harness.active().handle_cancel_peek(PEEK_D);
3061+
assert_eq!(
3062+
harness.peek_responses(),
3063+
vec![(PEEK_D, PeekResponse::Canceled)]
3064+
);
3065+
3066+
// Every peek that survives the cancellation sorts ahead of the resume point.
3067+
for uuid in [PEEK_A, PEEK_B] {
3068+
harness.add_pending(
3069+
index_peek_with_uuid(uuid, None),
3070+
trace_bundle(&keys, cancelling_errors(0)),
3071+
);
3072+
}
3073+
3074+
harness.sweep();
3075+
3076+
assert_eq!(
3077+
harness.peek_responses(),
3078+
vec![(PEEK_A, answer)],
3079+
"a resume point past every pending peek wraps to the first of them"
3080+
);
3081+
assert_eq!(harness.state.peek_resume_at, Some(PEEK_B));
3082+
}
3083+
29613084
/// A peek deferred as it arrives leaves an activation behind, so a worker with nothing else to
29623085
/// do does not park on it.
29633086
///
@@ -3018,7 +3141,9 @@ mod peek_sweep_tests {
30183141
/// a walk that never reached an outcome. The cancellation lands before the promoted task has
30193142
/// been polled, because nothing here awaits between the sweep that promoted it and the
30203143
/// cancellation. What a walk that was already running does with a cancellation is pinned by
3021-
/// `peek_offload::tests::a_walk_cancelled_while_running_reports_no_outcome`.
3144+
/// `peek_offload::tests::a_walk_cancelled_while_running_reports_no_outcome`, and what one that
3145+
/// had already reached an outcome does by
3146+
/// [`a_walk_cancelled_with_its_outcome_in_flight_is_counted`].
30223147
#[mz_ore::test(tokio::test)]
30233148
async fn a_cancelled_promoted_peek_is_answered_once() {
30243149
let keys = wide_ok_rows(WIDE_INDEX_KEYS);
@@ -3056,6 +3181,58 @@ mod peek_sweep_tests {
30563181
);
30573182
}
30583183

3184+
/// Cancelling a promoted peek whose walk has already reached its outcome answers it as
3185+
/// cancelled and counts the walk as offloaded.
3186+
///
3187+
/// This is the case the walk counters are documented with: a walk is counted where it reached
3188+
/// an outcome, and a cancellation that lands after that point does not take the count away.
3189+
/// The window is the one between the task sending its outcome and the worker's next sweep
3190+
/// reading it, which is as long as the worker takes to come back around, so a replica under
3191+
/// load spends real time in it. The counters would otherwise have to be read as a lower bound
3192+
/// on the walks each substrate finished rather than as the count of them.
3193+
#[mz_ore::test(tokio::test)]
3194+
async fn a_walk_cancelled_with_its_outcome_in_flight_is_counted() {
3195+
let keys = wide_ok_rows(WIDE_INDEX_KEYS);
3196+
3197+
let mut harness = at_production_defaults();
3198+
harness.add_pending(
3199+
index_peek_with_uuid(PEEK_A, None),
3200+
trace_bundle(&keys, cancelling_errors(0)),
3201+
);
3202+
3203+
harness.sweep();
3204+
assert_eq!(harness.pending(PEEK_A), Some("offloaded"));
3205+
3206+
// The walk runs to its outcome while no sweep collects it, which leaves the outcome in
3207+
// flight between the task and the worker.
3208+
harness.drive_until_walks((0, 1)).await;
3209+
assert_eq!(
3210+
harness.peek_responses(),
3211+
vec![],
3212+
"no sweep has read the outcome, so the peek is unanswered"
3213+
);
3214+
3215+
harness.active().handle_cancel_peek(PEEK_A);
3216+
3217+
assert_eq!(
3218+
harness.peek_responses(),
3219+
vec![(PEEK_A, PeekResponse::Canceled)],
3220+
"the cancellation answers the peek rather than the outcome in flight"
3221+
);
3222+
assert_eq!(
3223+
harness.walks(),
3224+
(0, 1),
3225+
"a walk that reached its outcome stays counted on the substrate that ended it"
3226+
);
3227+
3228+
harness.sweep();
3229+
assert_eq!(
3230+
harness.peek_responses(),
3231+
vec![],
3232+
"the outcome in flight is dropped with the peek rather than answered after it"
3233+
);
3234+
}
3235+
30593236
/// A harness whose peek of the whole wide index is promoted and whose promoted walk then
30603237
/// crosses the stash threshold, swept once so that the peek is already promoted.
30613238
///

0 commit comments

Comments
 (0)