Skip to content

fix(precompute-engine): warm-tier persists ingested sketches under queryable key - #82

Merged
zzylol merged 1 commit into
mainfrom
fix/sweep-blocker-2-warmtier-persistence-or-rollup-alignment
May 5, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/sweep-blocker-2-warmtier-persistence-or-rollup-alignment

Conversation

@zzylol

@zzylol zzylol commented May 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Sweep blocker #2 diagnostic claimed the warm-tier query path returned empty
even when sketches were arriving at the backend worker
(worker_process_accumulator log lines fire ~8000 times but
Metric ... not found in store). Three hypotheses were proposed:
(A) streaming.yaml grouping/rollup mismatch, (B) _quantile metric-name
suffix mismatch, (C) worker → per_key store persistence regression.

Verification verdict — at the unit level all three hypotheses are
disproven
, and three new pinning tests prove the existing
process_accumulator_input → window-close → emit_batch → per_key store
path is correct given that timestamps advance enough to close windows.

What was checked

  • (B) per_key store is keyed by aggregation_id only
    (per_key.rs:817 store_key = output.aggregation_id). The metric field
    is diagnostic-only. So _quantile suffix in the agent emit cannot
    cause a query miss as long as the agent's metric matches the
    streaming.yaml's metric for agg_id=1. Not the bug.

  • (A) backend's extract_group_key reads only grouping_labels
    rollup_labels is parsed by aggregation_config.rs but never read in
    precompute_engine/
    (verified via grep). 1000 per-tuple agent
    sketches with grouping: [zone] collapse into ~zones-many groups,
    each merged via AggregateCore::merge_with. The new
    test_grouping_labels_roll_up_per_tuple_sketches pins this. Not the bug.

  • (C) the new test_process_accumulator_input_persists_after_window_close

    • test_sketch_ingest_persists_and_query_returns_non_empty exercise the
      full ingest→worker→SimpleMapStorePerKeyquery_precomputed_output
      round-trip with real DDSketchAccumulator values. They pass on
      origin/main, so the persistence pathway itself is not regressed.

What this PR ships

A pure test-only addition (no production-code change) that pins the
warm-tier sketch persistence contract so any future regression to:

  • WorkerMessage::AccumulatorInput handling in worker.rs,
  • the sketch_panesmerge_sketch_panes_for_windowemit_batch chain,
  • or the per_key store's agg_id-keyed insert/query path

will trip a unit test instead of being silently masked by the existing
process_group_samples test coverage (which never exercised the sketch path).

The diff is +302 / -0 lines, contained entirely to the existing
worker.rs::tests module — no new files, no API surface changes.

Hypothesis verdict (per task spec)

  • (A) rollup mismatch: NOrollup_labels is unused in
    precompute_engine; backend rollup happens implicitly via
    grouping_labels. Test pins the contract.
  • (B) _quantile suffix: NO — store is keyed by agg_id, not metric.
  • (C) persistence regression: NO — full round-trip works in unit test.

The actual root cause is therefore NEITHER (A), (B), NOR (C). Per the
task spec's escalation rule, I'm pinging the user.

Most likely live root cause (ping the user)

Given that the unit-level path is provably correct, the live e2e blocker
is most likely one of these production-only conditions, which a unit test
cannot reproduce:

  1. Event-time watermark stagnation. process_accumulator_input only
    closes windows when closed_windows(prev_wm, current_wm) returns a
    non-empty set, which requires current_wm to advance past
    pane_start + window_size_ms. If the agent stamps every sketch with
    the same time_unix_nano (e.g. window-start instead of flush-time),
    the wm stalls. flush_all only adds +1ms to the wm, which is
    insufficient to close a 30s window. A wall-clock-based fallback in
    flush_all would defend against this.
    That is a design change so
    I have not made it; flagging here per task escalation rule.

  2. Schema barrier dropping writes silently. A reconcile transition
    could move agg_id=1 to non-Active, after which is_writable(1) is
    false and route_modified_otlp_sketches_to_precompute (otel.rs:750)
    buckets the sketch into barrier_drops instead of forwarding to the
    worker. But this contradicts the diagnostic's claim that
    worker_process_accumulator{agg_id=1} fires 8000 times. So unlikely.

  3. OTLP decode failure path. If the agent's time_unix_nano is 0
    or unset, (0 / 1_000_000) as i64 = 0 becomes the timestamp, all
    sketches land in the pane at pane_start_for(0), and unless a later
    batch advances the wm past 0 + 30s = 30000, no window closes.
    This is a real edge case; the proposed wall-clock fallback (1) covers
    it too.

Open follow-ups (separate from this PR)

  • rollup_labels is parsed from streaming.yaml but unused in
    precompute_engine. This is a UX trap: operators who write
    rollup: [rack, node, pod] expecting it to drive backend rollup
    semantics get the right answer for the wrong reason (it's
    grouping_labels doing the work). Worth either deleting the field
    from the YAML schema or wiring it into capability matching properly.

  • flush_all's +1ms watermark advance is a no-op for stuck-event-time
    scenarios. Adding a wall-clock fallback would close idle windows
    even when event-time freezes — but this is a watermark-semantics
    design change, escalated above.

Test plan

  • cargo test --release -p query_engine_rust --lib precompute_engine — 66 passed (was 63 on main, +3 new)
  • cargo test --release -p query_engine_rust --lib — 794 passed / 34 failed (matches main's 791 passed / 34 failed; +3 new tests, zero regressions)
  • No production-code change → no risk of breaking an existing caller
  • Sweep agent will re-run live e2e on retry to validate

🤖 Generated with Claude Code

Adds three regression tests against the
`process_accumulator_input → window-close → emit_batch → per_key store`
path the OTLP sketch ingest dispatcher relies on:

- `test_process_accumulator_input_persists_after_window_close` — a single
  group emits exactly one persisted output once a sketch in a later window
  advances the group watermark past the prior window boundary.
- `test_sketch_ingest_persists_and_query_returns_non_empty` — a real
  `SimpleMapStorePerKey` sees non-empty results from a `query_precomputed_output`
  call right after sketch ingest completes a window. This is the unit-level
  repro of sweep blocker #2's symptom (`No precomputed outputs found for
  metric: ..., aggregation_id: 1`).
- `test_grouping_labels_roll_up_per_tuple_sketches` — pins the
  agent-emit-shape vs. backend-grouping-config invariant: backend's
  `grouping_labels = [zone]` collapses 5 per-`(zone,rack,node,pod)`
  sketches into 2 per-zone outputs (one per zone).

The tests pass against `origin/main`, so all three sweep-blocker
hypotheses (A-rollup, B-metric-suffix, C-persistence-regression) are
disproven at the unit level. The `process_accumulator_input → per_key
store` plumbing is correct given that timestamps advance enough to close
windows. Live e2e remediation likely requires diagnosing event-time
stagnation or a schema-barrier transition in production.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol merged commit 4e4d4b3 into main May 5, 2026
@zzylol
zzylol deleted the fix/sweep-blocker-2-warmtier-persistence-or-rollup-alignment branch May 5, 2026 22:05
zzylol added a commit that referenced this pull request May 5, 2026
…dows (#83)

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant