fix(planner): bound MILP retention with CircularBuffer and spill evicted windows by milindsrivastava1997 · Pull Request #869 · ProjectASAP/ASAPQuery

milindsrivastava1997 · 2026-10-14T15:50:39Z

Closes #800. Closes #760.

Retention

Planner: MILP output now uses CircularBuffer instead of NoCleanup. Each query sets num_aggregates_to_retain to the closed windows its lookback needs, (L − x)/y + 1 (Deployment::closed_instance_count). Open windows live in the precompute engine, so the store plus the precompute engine hold what the plan charged. A lookback that isn't a whole number of windows is now an error.
Shared aggregations: retention takes the max across queries instead of the last one read, and an unbounded reference wins. ReadBased thresholds now sum read_count_threshold; previously they summed the wrong field, so ReadBased never evicted. Ported from Retention: take max across shared aggregation refs; sliding depth (n-1)*W/S+1 #760.
Engine: the store kept num_aggregates_to_retain × 4 windows (max_epochs = 4), and sealing an epoch mid-window split a window's series across two epochs, so the window was counted twice and its early series were evicted. CircularBuffer now never seals: every window stays in the current epoch, whose distinct-window set is exact, and the oldest windows beyond N are evicted. This holds however the precompute workers' batches interleave. Removing the now-unused sealed-epoch code is tracked in query-engine: remove the unused sealed-epoch machinery from SimpleMapStore #870.
Retention slack: the new engine setting store.retention_slack_ms (default 0) keeps N + ceil(slack_ms / step_ms) windows, where the step is the slide, or the window size when the slide is 0. It's for queries evaluated in the past: an instant quantile planned with retain 1 and evaluated 10s back fell back to Prometheus on every request.

Disk spill (write-only)

Engine: when store.spill_dir is set, every window cleanup evicts (CircularBuffer or ReadBased) is appended to <dir>/agg_<id>.bin before it leaves memory: window bounds, label key and serialized accumulator. Serialization and the write happen after the store lock is released, with one file lock per aggregation. A failed write is logged and counted in asap_store_spill_write_errors_total, and the window is still evicted. Spilled windows can't be queried yet.

asap-tools

query_engine.spill (default off) sets the engine's spill dir under its output dir. Spill files stay on the node; results syncs skip them. The run writes spill_report.json (bytes, failed writes) and fails, after rsync and teardown, if any spill write failed.
aggregate_cleanup.policy now defaults to circular_buffer. The ClickHouse runner passes it to the SQL planner instead of hardcoding read_based.
retention_slack_ms = the largest query_time_offset + one ingestion interval, passed to the engine automatically.

Testing

Unit and contract tests: planner retention for tumbling and sliding windows plus the partial-window error, shared max/sum, the store keeping exactly N windows and N + slack, spill contents and the error path for both lock strategies, the harness config and spill report.
E2E (Clemson, 2 nodes). ⚠️ These runs predate the series fix in 1c93243. The old store dropped series from retained windows, so "answered by ASAPQuery" below means the engine returned a result, not necessarily a complete one. A rerun on 1c93243 is in progress, and this section will be updated with its results.
Setup: generated2 workload 1 with 10 exporter ports (80k series), legacy planner, spill on, about 45 min in ASAPQuery mode.
- Without slack: both instant quantiles were forwarded to Prometheus on every request. All 1258 windows they needed were found in the spill files.
- With slack (11s): all 10 queries were answered by ASAPQuery. 8 had a 100% rate; the instant quantiles were at 98.8% (about one forward per heavy [15m] batch, which looks like a brief ingestion stall rather than eviction, left as is). Spill was 3.3 GB with 0 failed writes; engine memory was flat at about 7–8 GiB.

Details are in the #800 comments.

🤖 Generated with Claude Code

MILP output emitted NoCleanup, so the engine kept every window while the plan charged memory for only the retained ones. Each query now retains the closed windows its lookback needs, (L - x)/y + 1, under CircularBuffer. Shared aggregations keep the largest retention across their queries, and ReadBased thresholds sum read_count_threshold (ported from #760). The store keeps exactly num_aggregates_to_retain windows (one epoch) instead of 4x. Closes #760 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

With store.spill_dir set, every window cleanup evicts (CircularBuffer or ReadBased) is appended to <dir>/agg_<id>.bin before it leaves memory: window bounds, label key and serialized accumulator. Spill is write-only; a failed write is logged and counted in asap_store_spill_write_errors_total, and the window is still evicted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

query_engine.spill (off by default) sets the engine's store.spill_dir under its output dir. Runs then record the spill size and write-error count in spill_report.json and fail if any spill write failed. The engine registers the error counter as soon as spill is on, so /metrics reports 0 instead of omitting it. aggregate_cleanup.policy now defaults to circular_buffer, and the ClickHouse runner passes it to the SQL planner instead of hardcoding read_based. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Retention covered only a query's lookback, so a query evaluated in the past (the harness's query_time_offset) could find its windows already evicted. An instant quantile planned with retain=1 and evaluated 10s back fell back to Prometheus on every request. store.retention_slack_ms (default 0) makes CircularBuffer keep N + ceil(slack_ms / slide_ms) windows. asap-tools sets it to the largest query_time_offset plus one ingestion interval. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997 · 2026-10-14T16:33:25Z

 pub type EpochID = u64;
+
+/// Epochs (1 current + sealed) a key keeps. With one, the store retains
+/// exactly `num_aggregates_to_retain` windows, the count the planner sizes.


Bug: with MAX_EPOCHS = 1, a window whose keys straddle an epoch seal is counted twice, so maybe_rotate_epoch (global.rs and per_key.rs) evicts windows that are still in range.

The step-2 total sums distinct_window_count() per epoch, so the same window in two epochs counts twice. Example with retain=1 and two series:

(W,a) is inserted and the current epoch hits capacity 1, so it seals.

(W,b) is inserted and it seals again. The total is 1+1 = 2 > 1, so the epoch holding (W,a) is evicted.

Only the last series of each window survives. With N>1 the first key of W_N seals the epoch, the remaining keys of W_N go into a new epoch, and when that epoch fills, the early keys of W_N are evicted while W_N is still in range. Exact-window queries then find W_N and silently answer without those series, with no fallback. max_epochs=4 hid this behind headroom, and every test inserts a single key per window. A multi-series-per-window test would catch it.

Fixed in 1c93243. Reproduced first: with 3 series per window the newest window kept 1/3 (retain=1) and 2/3 (retain=3) under both strategies. A seal-on-new-window fix wouldn't hold, because each precompute worker sends its own batch per window, so windows arrive interleaved. CircularBuffer now never seals: all windows stay in the current epoch, whose distinct-window set is exact, and the oldest beyond the limit are evicted. Added test_circular_buffer_keeps_every_series_of_retained_windows (interleaved windows, retain 1 and 3, both strategies); it fails on the old store and passes now.

milindsrivastava1997 · 2026-10-14T16:33:25Z

+        if retention_slack_ms == 0 {
+            n
+        } else {
+            n + retention_slack_ms.div_ceil(slide_interval_ms)


Bug: divide-by-zero when slide_interval_ms == 0. Tumbling configs use slideIntervalMs: 0, and the engine treats that as tumbling elsewhere (bucket_step_ms, WindowManager). With any retention_slack_ms > 0, div_ceil(0) panics on the first insert while holding the store lock. Fall back to window_size_ms as SimpleEngine::bucket_step_ms does. That also makes the retention_slack_ms == 0 branch unnecessary, since 0.div_ceil(s) == 0.

Fixed in 1c93243: retained_windows steps by window_size_ms when slide_interval_ms == 0, and the slack == 0 branch is gone. Test: zero_slide_steps_by_window_size.

milindsrivastava1997 · 2026-10-14T16:33:25Z

                    if let Some(num_aggregates) = aggregation.num_aggregates_to_retain {
-                        // OLD: Keep last value only (for backwards compatibility)
-                        retention_map.insert(aggregation_id, num_aggregates);
+                        retention_map


Bug: an unbounded (None) retention loses to a bounded one in the merge. If query A uses agg 1 with no num_aggregates_to_retain (unlimited) and query B uses agg 1 with retain 2, then retention_map[1] = 2. The CircularBuffer keeps 2 windows, so A's longer lookback silently misses data or falls back. None should win the merge.

Fixed in 1c93243: the merge keeps Option<u64> and None wins in either order. Test: shared_references_keep_unbounded_retention.

milindsrivastava1997 · 2026-10-14T16:33:25Z

                pre_query_wait_seconds=steady_state_wait,
                monitor_interval_seconds=monitor_interval_seconds,
            )
+            query_engine_service.record_spill()


Bug: record_spill() can raise before sync and teardown. A failed spill write, a missing metric, or empty du output raises before rsync_experiment_data and query_engine_service.stop() run. Results are never rsynced back and the stack keeps running on the node, so the next run hits port conflicts or stale state. Record the failure and raise after sync and teardown. experiment_run_e2e.py:560 has the same problem.

Fixed in 1c93243: record_spill() never raises; it returns the failure (failed writes, missing metric, or the check itself failing). Both runners raise it after rsync and teardown. Test: test_failed_check_is_returned_not_raised.

milindsrivastava1997 · 2026-10-14T16:33:25Z

                    }
                }
            }
+            self.spill(store_key, &data.stores[&store_key].intern, &evicted);


Perf: spill does synchronous serialization and write_all on the insert path while holding the global store Mutex. per_key.rs:433 does the same while holding the DashMap entry lock. Every eviction batch serializes every evicted accumulator (MB-scale sketches across many series) while all inserts and queries wait, so latency spikes on slow disks. Collect the evicted entries under the lock and do serialization and I/O after releasing it, or on a background writer.

Fixed in 1c93243: evicted keys are resolved under the lock (resolve_evicted), then serialization and the write happen after the store lock / entry guard is released, in both stores.

milindsrivastava1997 · 2026-10-14T16:33:25Z

+            }
+            put_len_prefixed(&mut buf, &aggregate.serialize_to_bytes()?);
+        }
+        let mut file = self.files.entry(aggregation_id).or_try_insert_with(|| {


Perf: files.entry(aggregation_id) holds the DashMap shard write lock across write_all (line 83). Spills for unrelated aggregation ids that hash to the same shard wait on each other's disk writes, which removes the per-key strategy's concurrency whenever spill is on. Use one Mutex<File> per id, or clone the handle out before writing.

Fixed in 1c93243: files is now DashMap<u64, Arc<Mutex<File>>>; the handle is cloned out of the shard and the write happens under the per-file mutex only.

milindsrivastava1997 · 2026-10-14T16:33:25Z

            current_epoch_id: 0,
            epoch_capacity: None,
-            max_epochs: 4,
+            max_epochs: MAX_EPOCHS,


Design: setting MAX_EPOCHS = 1 leaves the multi-epoch machinery in place: the max_epochs field, the sealed-epoch BTreeMap, and the cross-epoch eviction loop, duplicated in PerKeyState and StoreKeyData. That machinery is what makes retention depend on seal timing instead of distinct windows, which causes the double-count bug in common.rs. Evicting the oldest distinct windows over the whole key once their count exceeds N would make "keep exactly N" hold by construction.

Agreed. #869 already stops sealing (see the double-count fix), which makes the sealed-epoch machinery dead code; removing it is tracked in #870. Until then seal and unique_windows carry #[allow(dead_code)].

CircularBuffer sealed an epoch once it held N windows. A window whose series arrived after the seal (each precompute worker sends its own batch) landed in two epochs, was counted twice, and lost its early series while still retained. CircularBuffer now keeps every window in the current epoch, whose distinct-window set is exact, and evicts the oldest windows beyond the limit. Also from review: - retention slack steps by window_size_ms when slide_interval_ms is 0 (tumbling) instead of dividing by zero; - an unbounded retention wins the shared-aggregation merge; - spill serializes and writes after releasing the store lock, with one file lock per aggregation; - record_spill() returns its failure so runners raise after sync and teardown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997

Code review findings (Claude Code).

milindsrivastava1997 · 2026-10-14T18:08:21Z

+    } else {
+        slide_interval_ms
+    };
+    num_aggregates_to_retain.map(|n| n + retention_slack_ms.div_ceil(step_ms))


Divide-by-zero panic. div_ceil(step_ms) panics when slide_interval_ms and window_size_ms are both 0, for example a DeltaSet key aggregation with planned retain under CircularBuffer. That happens even with the default retention_slack_ms=0, on the first insert_precomputed_output, while the store lock is held. Guard step_ms == 0, for example by returning num_aggregates_to_retain unchanged.

milindsrivastava1997 · 2026-10-14T18:08:21Z

-        }
-        evicted
+        let to_remove = self.current_epoch.oldest_windows(over);
+        self.current_epoch.remove_windows(&to_remove)


Late batches for an evicted window are spilled again. If a slow precompute worker sends more series for window W after W was evicted and spilled, W re-enters the epoch, the count goes to limit+1, W is the oldest, and it is evicted and appended to agg_<id>.bin a second time with only the late series. The spill then has duplicate partial records for W that can't be told apart. per_key.rs has the same problem. Consider tracking an eviction watermark per aggregation and dropping or merging late inserts for windows below it.

milindsrivastava1997 · 2026-10-14T18:08:21Z

+                    .evict_beyond_window_limit();
+                let mut dropped_windows: Vec<TimestampRange> =
+                    dropped.iter().map(|(window, _, _)| *window).collect();
+                dropped_windows.dedup();


dedup() only removes adjacent duplicates, but these windows come from evicted entries in column (insertion) order, so interleaved windows survive it and read_counts.remove and the debug log run once per series. evict_beyond_window_limit already computes to_remove, so returning it alongside the entries avoids rebuilding the list.

milindsrivastava1997 · 2026-10-14T18:08:21Z

    /// Consume this epoch and produce an immutable SealedEpoch by sorting in-place.
    /// Zips the three columns into tuples and sorts — moves Arcs without cloning.
    /// O(M log M) paid once at rotation time, not at query time.
+    /// Unused since CircularBuffer stopped sealing; the sealed-epoch paths remain


Perf: one mutable epoch holds every retained window. Since CircularBuffer never seals, all N+slack windows stay in a single MutableEpoch. Every insert invalidates the lazy index, so the next exact_query rebuilds a HashMap over every retained entry, and range_query_into scans them all linearly. For example, a 1s window with retain=1 and 11s of slack at 80k series is about 960k entries rebuilt per query under the store lock, and the sorted sealed-epoch fast path is never used. Worth benchmarking against the old behavior.

milindsrivastava1997 · 2026-10-14T18:08:21Z

+    /// Unused since CircularBuffer stopped sealing; the sealed-epoch paths remain
+    /// only until they are removed.
+    #[allow(dead_code)]
    pub fn seal(self) -> SealedEpoch {


Nit: this comment describes history and a planned removal ("Unused since CircularBuffer stopped sealing..."), which AGENTS.md says not to do. A few test comments do the same ("dividing by it panicked", "Sealing an epoch once it held N windows split a window..."). Also, seal() and unique_windows() are now dead code kept alive with #[allow(dead_code)]. Could they be removed?

milindsrivastava1997 · 2026-10-14T18:08:21Z

+                // configure_window_limit was already called before insert;
+                // eviction is handled by evict_beyond_window_limit after each insert.
                // Nothing additional needed here.
                let _ = (num_aggregates_to_retain, metric, aggregation_id);


This CircularBuffer arm is now a no-op (let _ = (...)). Together with the dead seal() path, this makes it hard to tell which epoch paths are live. sealed_epochs stays only for ReadBased. Removing the dead arm would make the single-epoch invariant clear.

milindsrivastava1997 · 2026-10-14T18:08:21Z

+            "retention_slack_ms": retention_slack_ms,
+        }
+        if spill:
+            store["spill_dir"] = os.path.join(output_dir, SPILL_SUBDIR)


The spill dir lives inside query_engine_output, and rsync_experiment_data copies that whole tree back with rsync -azh and no excludes. With about 3.3 GB of spill in the E2E run, that's a slow, compressed copy and a lot of local disk on every run, and only spill_report.json is needed locally. Add --exclude spill/ or put spill outside the synced dir.

milindsrivastava1997 · 2026-10-14T18:08:21Z

+                        let threshold = read_count_threshold_map.entry(aggregation_id).or_insert(0);
+                        *threshold = threshold
+                            .checked_add(read_count_threshold)
+                            .expect("read-count threshold overflowed");


from_yaml_data returns Result, so a summed threshold overflowing u64 should be returned as an error, not crash via .expect(...). A malformed planner output would then fail config load cleanly instead of panicking at startup.

milindsrivastava1997 · 2026-10-14T18:08:21Z

            lock_strategy=args.lock_strategy,
            backend_config=backend_config,
            http_port=http_port,
+            spill=args.spill,


Spill is turned on here, but this runner never calls query_engine_service.record_spill(). A failed spill write (disk full, missing dir) only bumps asap_store_spill_write_errors_total, with no spill_report.json and no failure. The e2e and ClickHouse runners do check this.

milindsrivastava1997 · 2026-10-14T18:08:21Z

+    /// retain=2, 11 inserts => windows 9 and 10 remain, windows 0..9 are
+    /// evicted.
    #[test]
    fn store_query_range_spans_sealed_and_mutable_epochs() {


This test, test_buckets_returned_in_chronological_order_after_epoch_rotation, and test_exact_query_correct_after_epoch_rotation now only use one mutable epoch, so the multi-epoch and ordering coverage they were written for (the 'current epoch checked first' prepend bug) is gone. ReadBased still walks sealed_epochs, so that regression could come back with no failing test. Consider a ReadBased variant that actually seals, or renaming the tests and fixing their assertion messages.

…e node - retained_windows adds no slack for a zero-width window instead of dividing by zero (also with the default slack of 0). - An overflowing summed read-count threshold is an error, not a panic. - Evicted windows are sorted before dedup, since a window's entries aren't adjacent. - Results syncs skip query_engine_output/spill; the Grafana demo never spills, as it has no end-of-run spill check. - Tests and comments no longer describe sealed epochs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

milindsrivastava1997 and others added 4 commits October 13, 2026 21:05

milindsrivastava1997 linked an issue Oct 14, 2026 that may be closed by this pull request

planner: MILP plan → InferenceConfig should use ReadBased cleanup instead of NoCleanup #800

Closed

milindsrivastava1997 commented Oct 14, 2026

View reviewed changes

milindsrivastava1997 mentioned this pull request Oct 14, 2026

query-engine: remove the unused sealed-epoch machinery from SimpleMapStore #870

Open

milindsrivastava1997 commented Oct 14, 2026

View reviewed changes

milindsrivastava1997 merged commit bbb3033 into main Oct 14, 2026
17 of 18 checks passed

Hidden character warning

Conversation

milindsrivastava1997 commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Retention

Disk spill (write-only)

asap-tools

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milindsrivastava1997 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

milindsrivastava1997 commented Oct 14, 2026 •

edited

Loading