Skip to content

Commit 0e86a90

Browse files
fix(query-engine): make abort-on-drop test actually falsifiable
roborev (job 200) on d90a0a7: the regression test checked a flag set after a 60s sleep, but only waited 50ms before asserting — so it passed even with AbortOnDrop::drop() doing nothing, since 50ms never approaches 60s either way. Confirmed by temporarily neutering the abort call: the old test still passed. Rewrote it to check ongoing progress instead: the wrapped task increments a counter on a 5ms cadence, the test waits for it to start, drops the guard, then asserts the counter stops advancing. Verified this version fails without the real abort call and passes with it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PR8ENqodwuB1o8k4YAcjcJ
1 parent d90a0a7 commit 0e86a90

1 file changed

Lines changed: 30 additions & 9 deletions

File tree

‎asap-query-engine/src/precompute_engine/ingest_handler.rs‎

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ async fn handle_victoriametrics_ingest(
141141
#[cfg(test)]
142142
mod tests {
143143
use super::AbortOnDrop;
144-
use std::sync::atomic::{AtomicBool, Ordering};
144+
use std::sync::atomic::{AtomicU64, Ordering};
145145
use std::sync::Arc;
146146
use std::time::Duration;
147147

@@ -150,21 +150,42 @@ mod tests {
150150
/// (and keep the ingest state alive) if `HttpIngestSource::run` were ever
151151
/// dropped/cancelled mid-flight by a future caller. `AbortOnDrop` must
152152
/// actually abort the task, not just detach it.
153+
///
154+
/// Checks ongoing progress rather than a single far-future flag (roborev
155+
/// job 200): the wrapped task increments a counter on a fast, observable
156+
/// cadence, so a merely-detached task (bug) keeps advancing the counter
157+
/// after drop, while a genuinely aborted task (fix) does not. An earlier
158+
/// version of this test only checked a flag set after a 60s sleep — that
159+
/// passed even with `AbortOnDrop::drop` doing nothing, since the check
160+
/// window (50ms) never reached the 60s mark either way.
153161
#[tokio::test]
154162
async fn abort_on_drop_cancels_the_wrapped_task() {
155-
let ran_to_completion = Arc::new(AtomicBool::new(false));
156-
let flag = ran_to_completion.clone();
163+
let counter = Arc::new(AtomicU64::new(0));
164+
let task_counter = counter.clone();
157165
let guard = AbortOnDrop(tokio::spawn(async move {
158-
tokio::time::sleep(Duration::from_secs(60)).await;
159-
flag.store(true, Ordering::SeqCst);
166+
loop {
167+
task_counter.fetch_add(1, Ordering::SeqCst);
168+
tokio::time::sleep(Duration::from_millis(5)).await;
169+
}
160170
}));
161171

172+
// Synchronize with task startup before measuring.
173+
while counter.load(Ordering::SeqCst) == 0 {
174+
tokio::task::yield_now().await;
175+
}
176+
162177
drop(guard);
163-
tokio::time::sleep(Duration::from_millis(50)).await;
178+
let count_at_drop = counter.load(Ordering::SeqCst);
179+
180+
// A merely-detached task has ample time here to advance the counter
181+
// several more times (100ms / 5ms per tick); an aborted task cannot
182+
// make any further progress at all.
183+
tokio::time::sleep(Duration::from_millis(100)).await;
164184

165-
assert!(
166-
!ran_to_completion.load(Ordering::SeqCst),
167-
"task should have been aborted, not left to run to completion"
185+
assert_eq!(
186+
counter.load(Ordering::SeqCst),
187+
count_at_drop,
188+
"task kept running after the guard was dropped — it was merely detached, not aborted"
168189
);
169190
}
170191
}

0 commit comments

Comments
 (0)