fix(precompute-engine): wall-clock watermark fallback closes idle windows - #83
Merged
Merged
Conversation
…dows When event-time stagnates (e.g. agents stamp every sketch with the same `time_unix_nano`), `flush_all`'s `+1ms` watermark advance is a no-op: `closed_windows(prev_wm, prev_wm+1)` returns empty forever, the 30s window never closes, and warm-tier queries come back empty even though `worker_process_accumulator` keeps logging — exactly the live sweep blocker #2 symptom (8000 sketch arrivals, 0 store entries, query empty). PR #82 pinned the post-window-close persistence path is correct given event-time advances; it explicitly punted this fix as "watermark- semantics design change". This PR lands it. The fix tracks each pane's wall-clock birth time in `GroupState::pane_wall_clock_starts_ms` and, in `flush_all`, force- advances `effective_wm` past `pane_start + window_size_ms` for any pane older than `window_size_ms + grace_period_ms` of WALL-CLOCK time. Event-time-driven closure remains the primary path; wall-clock is fallback. Monotonicity is preserved — the fallback only ever pushes the watermark forward. `PrecomputeEngineConfig::wall_clock_grace_period_ms` (default 5_000ms, matching the existing `allowed_lateness_ms` default) tunes the grace period; set to `<= 0` to opt out and keep strict event-time-only semantics. For testability, `Worker` carries an injectable `now_ms_fn` (default `SystemTime::now`-backed). The new `wall_clock_fallback_closes_idle_window` test injects a fake clock and pins the fix in milliseconds rather than needing `std::thread::sleep(35s)`. The 3 PR #82 pin tests still pass (regression baseline). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Land the actual fix for sweep blocker #2 that PR #82 (just merged) explicitly punted as "watermark-semantics design change". PR #82 pinned that the post-window-close persistence pathway is correct given timestamps advance; the live e2e symptom (8000 sketch arrivals, 0 store entries, query empty) means timestamps don't advance enough to close 30s windows.
When the agent stamps every sketch with the same
time_unix_nano(e.g. window-start instead of flush-time),flush_all's+1mswatermark advance is a no-op:closed_windows(prev_wm, prev_wm+1)returns empty forever, the window never closes, and warm-tier queries come back empty even thoughworker_process_accumulatorkeeps logging.Fix shape
GroupStatenow tracks each open pane's wall-clock birth time in a newpane_wall_clock_starts_ms: BTreeMap<i64, i64>. The first sample/sketch routed into a pane stamps it with the current wall-clock time.flush_allforce-advances each group'seffective_wmpastpane_start + window_size_msfor any pane whose wall-clock age exceedswindow_size_ms + wall_clock_grace_period_ms. The existing event-time path (closed_windows + merge_*_for_window + emit_batch) consumes the bumped watermark exactly as before — no duplicated close logic.effective_wmforward, never backward.PrecomputeEngineConfig::wall_clock_grace_period_ms(default 5_000 ms — matching the existingallowed_lateness_msdefault) tunes the grace. Setting it to<= 0opts out and reverts to strict event-time-only semantics (pin test included).Workercarries an injectablenow_ms_fn(defaultSystemTime::now-backed) so tests can drive the fallback in milliseconds rather than needing real-timesleep(35s).Diff size
worker.rs+ ~7 inconfig.rs+ 1 inengine.rs+ 7 across bins/tests for the newPrecomputeEngineConfigfield. Net actual code: ~70 LOC, well under the 100-LOC autopilot threshold. Heavy on doc comments because the failure mode is subtle.make_worker_with_gracehelper).New tests
wall_clock_fallback_closes_idle_window— ingests 10 DDSketches stamped with frozen event-timet=0, then advances a fake clock bywindow_size + grace = 35sand assertsflush_allcloses + emits + persists the[0, 30_000)window with aDDSketchAccumulatorcarrying all 10 sketches. Re-asserts idempotency: a secondflush_allwith no new data does not re-emit.wall_clock_fallback_disabled_preserves_event_time_only_semantics— pins thatwall_clock_grace_period_ms = 0opts out of the fallback (event-time-only behaviour, matching pre-fix semantics). Even after a 24h fake-clock advance, no emit happens.Subtleties
allowed_lateness_ms = 5_000default. Configurable via the YAML / CLI like every otherPrecomputeEngineConfigfield. Setting<= 0is the explicit opt-out.propagated_wm + 1) and the wall-clock fallback only pusheffective_wmforward. Once a window closes via the fallback, its pane is drained fromsketch_panes(andactive_panes) and frompane_wall_clock_starts_msviaprune_pane_wall_clock_starts, so subsequentflush_alls have no candidate for re-closure. The "close at most once" invariant is preserved.closed_windows(prev, force_to)naturally yields all windows that closed in the bumped jump, including overlapping ones for sliding-window configs. No special-case logic needed.Box<dyn Fn() -> i64 + Send + Sync>) over a trait so we don't introduce a new test pattern for a single use site. Theset_now_ms_fnsetter is#[cfg(test)]-gated so production code never touches it.now_msmatched to its actual creation time, not its event-time window-start. The fallback fires afterwindow_size + graceof real elapsed time — which is the desired "the agent stopped progressing event-time" semantics.Open question (separate hardening pass)
PR #82 pinned hypothesis (1) — frozen
time_unix_nano— as the most likely production root cause. Hypothesis (3) flagged a related case: if the agent'stime_unix_nanois0,(0 / 1_000_000) as i64 = 0becomes the timestamp, all sketches land inpane_start_for(0) = 0, and the wall-clock fallback now correctly fires atnow - pane_birth >= 35s. Worth a separate look atdecode_modified_otlp_sketch_bytes's timestamp handling — clock-skew between agent and backend could still produce panes whosepane_startis wildly different fromnow_ms, and the fallback's "wall-clock age" semantics implicitly assume the backend's clock is the authoritative one. For agents shipping batched sketches with their own clock embedded, that assumption is fine. For pathological clock skew, amax(event-time-wall-clock-equivalent, ingest-wall-clock)heuristic might be more robust.Test plan
cargo test --release -p query_engine_rust --lib precompute_engine::worker— 29 passed (all 3 PR fix(precompute-engine): warm-tier persists ingested sketches under queryable key #82 pin tests + both new tests + 24 prior).cargo test --release -p query_engine_rust --lib precompute_engine— 68 passed (was 66 on main; +2 new).cargo test --release -p query_engine_rust --lib— 796 passed / 34 failed (matches main's 794 / 34 with +2 new tests, zero regressions).cargo clippy --release -p query_engine_rust --lib --tests— 5 pre-existing warnings, zero new warnings from this PR's code.cargo build --release -p query_engine_rust— all bins build clean (added the new field at everyPrecomputeEngineConfigliteral).🤖 Generated with Claude Code