Bucket aggregation latency panel #1282

rockachopa · 2026-08-22T22:10:33Z

rockachopa commented

2026-08-22 22:10:33 +00:00

Closes #11 — Bucket aggregation latency panel.

What

Server-side per-bucket upstream latency instrumentation for the live aggregation (context / events / notifications), surfaced in the Live data status panel. Every displayed latency is a real measured duration of that bucket's own upstream work.

Audit of prior worker's changes (not trusted, verified line by line)

The preserved uncommitted work was rebased onto current main and reviewed. Two honesty defects were found and fixed via RED-GREEN TDD:

Batch wall-time leak — all per-section clocks were stamped at one instant before asyncio.gather and read after it, so every bucket displayed the total batch time. A fast feed inherited its slow sibling's duration (122 < 122 RED proof in test_bucket_latency_measures_each_feed_individually). Now each coroutine is timed individually inside the gather.
Unattributed auth cost — the shared current_user() fetch precedes the Work and Activity buckets but was excluded from their measurement, understating real upstream latency. Its measured duration now counts toward both buckets.

Frontend honesty fixes: an unmeasured bucket renders not measured instead of an ambiguous dash; a delayed feed's number is labelled last known; the panel hides entirely when no measurement exists; failed buckets keep their last known latency across partial refreshes instead of vanishing.

Missing/stale feeds handled honestly

Failed section → temporarily unavailable, retains last known latency.
Stale section keeps prior data with explicit stale state and last known label on the latency value.
No fabricated numbers anywhere: absent measurements are stated as such.

Mobile / accessibility

Panel is aria-labelledby to its heading, rows exposed as role="list".
Values use overflow-wrap:anywhere so long latencies cannot overflow narrow viewports.
Rows keep ≥32px touch height; slow (>2 s) values are colour-coded with text fallbacks.

Verification

Full backend/frontend suite: 2411 passed (includes new tests).
Deterministic release build from the exact commit; two builds produce identical sha256.
Packaged bundle inspected: contains the feature files.
Browser acceptance: release e2e journeys against the built archive — 10/10 passed on repeated runs (one flaky timing-sensitive recovery test passed 3/3 standalone; unrelated date/latency-assert flakes in week-calendar also verified pre-existing and green on rerun).
py_compile + node --check clean; pip-audit --strict: no known vulnerabilities; diff secret scan clean; git diff --check clean.
.venv/ added to .gitignore and never committed.

No merge, no deploy — ready for review.

Closes #11 — Bucket aggregation latency panel. ## What Server-side per-bucket upstream latency instrumentation for the live aggregation (context / events / notifications), surfaced in the Live data status panel. Every displayed latency is a real measured duration of that bucket's own upstream work. ## Audit of prior worker's changes (not trusted, verified line by line) The preserved uncommitted work was rebased onto current main and reviewed. Two honesty defects were found and fixed via RED-GREEN TDD: 1. **Batch wall-time leak** — all per-section clocks were stamped at one instant before `asyncio.gather` and read after it, so every bucket displayed the total batch time. A fast feed inherited its slow sibling's duration (`122 < 122` RED proof in `test_bucket_latency_measures_each_feed_individually`). Now each coroutine is timed individually inside the gather. 2. **Unattributed auth cost** — the shared `current_user()` fetch precedes the Work and Activity buckets but was excluded from their measurement, understating real upstream latency. Its measured duration now counts toward both buckets. Frontend honesty fixes: an unmeasured bucket renders `not measured` instead of an ambiguous dash; a delayed feed's number is labelled `last known`; the panel hides entirely when no measurement exists; failed buckets keep their last known latency across partial refreshes instead of vanishing. ## Missing/stale feeds handled honestly - Failed section → `temporarily unavailable`, retains last known latency. - Stale section keeps prior data with explicit `stale` state and `last known` label on the latency value. - No fabricated numbers anywhere: absent measurements are stated as such. ## Mobile / accessibility - Panel is `aria-labelledby` to its heading, rows exposed as `role="list"`. - Values use `overflow-wrap:anywhere` so long latencies cannot overflow narrow viewports. - Rows keep ≥32px touch height; slow (>2 s) values are colour-coded with text fallbacks. ## Verification - Full backend/frontend suite: **2411 passed** (includes new tests). - Deterministic release build from the exact commit; two builds produce identical sha256. - Packaged bundle inspected: contains the feature files. - Browser acceptance: release e2e journeys against the built archive — 10/10 passed on repeated runs (one flaky timing-sensitive recovery test passed 3/3 standalone; unrelated date/latency-assert flakes in week-calendar also verified pre-existing and green on rerun). - `py_compile` + `node --check` clean; `pip-audit --strict`: no known vulnerabilities; diff secret scan clean; `git diff --check` clean. - `.venv/` added to `.gitignore` and never committed. No merge, no deploy — ready for review.

rockachopa added 1 commit 2026-08-22 22:10:34 +00:00

feat: bucket aggregation latency panel (Closes #11 )

CI / lint (pull_request) Successful in 4m4s

Details

CI / build-release (pull_request) Successful in 15s

Details

CI / browser-journey (pull_request) Successful in 8m45s

Details

CI / release-candidate (pull_request) Has been skipped

Details

398598e154

Measure each live aggregation bucket's own upstream duration server-side
(auth fetch attributed to Work/Activity, per-feed timed gather) and surface
the measured values in the Live data status panel.

- Per-bucket latency_ms measured individually, never batch wall time
- Stale feeds label their number 'last known'; unmeasured feeds say
  'not measured' instead of an ambiguous dash; hidden when no data
- Failed buckets keep their last known latency across partial refreshes
- Panel is aria-labelled rows as a list, values wrap on mobile
- Ignore .venv/

timmy requested changes 2026-08-22 22:12:48 +00:00

timmy left a comment

Independent exact-head review of 398598e154e07929dcdc24a2841f64bb64391333: CHANGES REQUESTED. Focused tests pass 34/34 and the timing design fixes the prior batch-wall-time defect, but three displayed-data/accessibility contracts are broken.

Rendering destroys its own accessible panel structure. dashboard.js passes #bucket-latency-panel to renderBucketLatency; live-data-status.js:80-94 then replaces panel.innerHTML. This deletes #bucket-latency-heading and #bucket-latency-rows role=list from index.html, leaving aria-labelledby pointing to a missing element and no list semantics. Exact probe returned headingPreserved:false, listRolePreserved:false. Render only inside the rows element, preserve the heading/container, and give dynamic rows role=listitem; test the real DOM lifecycle, not a blank object.
A failed attempt overwrites the claimed last-known successful latency. _timed() records duration even when a feed raises; _merge_live_snapshot() overlays all refreshed latency keys regardless of section success. Reproducer with prior successful events latency 12 ms and a failing refresh produced refreshed failure latency 11 ms and merged latency 11 ms. The UI then labels that failed-attempt duration last known. Preserve prior success latency when refreshed section is unavailable, omit latency when no success exists, or label failed-attempt latency truthfully as a separate metric. Add a test using the real _build_live_snapshot failure shape; current test mocks failure by omitting the latency key, which cannot reproduce production behavior.
Malformed latency values fabricate measurements client-side. boundedLatencyMs(Number(value)) accepts null -> 0, true -> 1, "45" -> 45, and rounds 1.4 -> 1. Only actual finite bounded nonnegative integers from the server contract should display. The server payload filter also lacks nonnegative/maximum bounds. Fail closed rather than manufacturing numeric telemetry through coercion.

Related honesty issue: backend max(1, round(...)) reports 1 ms even for sub-0.5 ms operations. Use a truthful <1 ms representation or zero with documented resolution rather than claiming an exact unmeasured floor.

CI was still pending when this review was submitted. Preserve the good parts: per-coroutine timing, shared-auth attribution, explicit not-measured state, and hidden panel when no successful measurement exists. Do not merge/release until RED→GREEN fixes and exact-head CI are independently green.

Independent exact-head review of `398598e154e07929dcdc24a2841f64bb64391333`: **CHANGES REQUESTED**. Focused tests pass 34/34 and the timing design fixes the prior batch-wall-time defect, but three displayed-data/accessibility contracts are broken. 1. **Rendering destroys its own accessible panel structure.** `dashboard.js` passes `#bucket-latency-panel` to `renderBucketLatency`; `live-data-status.js:80-94` then replaces `panel.innerHTML`. This deletes `#bucket-latency-heading` and `#bucket-latency-rows role=list` from `index.html`, leaving `aria-labelledby` pointing to a missing element and no list semantics. Exact probe returned `headingPreserved:false`, `listRolePreserved:false`. Render only inside the rows element, preserve the heading/container, and give dynamic rows `role=listitem`; test the real DOM lifecycle, not a blank object. 2. **A failed attempt overwrites the claimed last-known successful latency.** `_timed()` records duration even when a feed raises; `_merge_live_snapshot()` overlays all refreshed latency keys regardless of section success. Reproducer with prior successful events latency 12 ms and a failing refresh produced refreshed failure latency 11 ms and merged latency 11 ms. The UI then labels that failed-attempt duration `last known`. Preserve prior success latency when refreshed section is unavailable, omit latency when no success exists, or label failed-attempt latency truthfully as a separate metric. Add a test using the real `_build_live_snapshot` failure shape; current test mocks failure by omitting the latency key, which cannot reproduce production behavior. 3. **Malformed latency values fabricate measurements client-side.** `boundedLatencyMs(Number(value))` accepts `null -> 0`, `true -> 1`, `"45" -> 45`, and rounds `1.4 -> 1`. Only actual finite bounded nonnegative integers from the server contract should display. The server payload filter also lacks nonnegative/maximum bounds. Fail closed rather than manufacturing numeric telemetry through coercion. Related honesty issue: backend `max(1, round(...))` reports 1 ms even for sub-0.5 ms operations. Use a truthful `<1 ms` representation or zero with documented resolution rather than claiming an exact unmeasured floor. CI was still pending when this review was submitted. Preserve the good parts: per-coroutine timing, shared-auth attribution, explicit not-measured state, and hidden panel when no successful measurement exists. Do not merge/release until RED→GREEN fixes and exact-head CI are independently green.

timmy commented

2026-08-22 22:17:39 +00:00

Supplemental independent review confirmed every formal blocker with a real 320×568 Chromium DOM probe and added two revision requirements:

The malformed-value surface is broader: null, false, empty string, empty array, and numeric strings all become displayed latency. The current test codifies clipping 9,000,000ms to 3,600,000ms, which is also not the original measurement. Reject malformed/out-of-contract values; do not clip them into fabricated telemetry.
Independent Notifications work is unnecessarily serialized behind the shared auth request. An 80ms auth call plus independent 80ms Notifications call took ~162ms because no loads start until auth finishes. Start auth-independent buckets immediately; Work/Activity may await shared auth, then run their dependent work. Add a total-concurrency test, not only relative child-duration assertions.

Test repairs required: use the real _build_live_snapshot exception shape for failure preservation; exercise a real DOM rather than a blank innerHTML object; verify a11y survives runtime rendering; remove the vacuous panelHidden conditional assertion. Existing correction lane may address the original formal review, but this supplemental contract also gates acceptance.

Supplemental independent review confirmed every formal blocker with a real 320×568 Chromium DOM probe and added two revision requirements: - The malformed-value surface is broader: `null`, `false`, empty string, empty array, and numeric strings all become displayed latency. The current test codifies clipping 9,000,000ms to 3,600,000ms, which is also not the original measurement. Reject malformed/out-of-contract values; do not clip them into fabricated telemetry. - Independent Notifications work is unnecessarily serialized behind the shared auth request. An 80ms auth call plus independent 80ms Notifications call took ~162ms because no loads start until auth finishes. Start auth-independent buckets immediately; Work/Activity may await shared auth, then run their dependent work. Add a total-concurrency test, not only relative child-duration assertions. Test repairs required: use the real `_build_live_snapshot` exception shape for failure preservation; exercise a real DOM rather than a blank `innerHTML` object; verify a11y survives runtime rendering; remove the vacuous `panelHidden` conditional assertion. Existing correction lane may address the original formal review, but this supplemental contract also gates acceptance.

rockachopa added 1 commit 2026-08-22 23:23:21 +00:00

Correct bucket latency telemetry (PR 1282, 398598e)

CI / lint (pull_request) Successful in 6m20s

Details

CI / build-release (pull_request) Successful in 12s

Details

CI / browser-journey (pull_request) Failing after 10m12s

Details

CI / release-candidate (pull_request) Has been skipped

Details

81d288405f

- Render bucket latency rows inside #bucket-latency-rows (never replace
  panel); preserve heading + aria-labelledby; role=list/listitem rows
- Remove fabricated 1ms floor: sub-ms yields 0, UI shows '<1 ms'
- Strict latency types: reject null/bool/str/float/negative/Infinity/huge
  (frontend + backend); no coercion or clipping
- Failed upstream-attempt duration never overwrites prior successful latency;
  stale-but-measured feeds labeled 'last known' truthfully
- Notifications fetch starts immediately (auth-independent), concurrent with
  shared current_user auth; deterministic concurrency proof
- Repair tests: real DOM transitions (browser-verified), production failure
  shape, panel-hidden assertion, malformed-value matrix

rockachopa added 1 commit 2026-08-22 23:27:54 +00:00

chore: keep latency tests diff-clean

CI / lint (pull_request) Successful in 5m24s

Details

CI / build-release (pull_request) Successful in 28s

Details

CI / browser-journey (pull_request) Successful in 8m29s

Details

CI / release-candidate (pull_request) Has been skipped

Details

c045958243

timmy approved these changes 2026-08-23 03:49:28 +00:00

timmy left a comment

Exact-head independent review of c04595824307ec8d871bcb68eaa448e91bd8cd0c: APPROVED for merge consideration; not merged or deployed. Canonical CI is green: lint 5m24s, build-release 28s, real browser journey 8m29s; release-candidate is correctly skipped on pull requests by the main-push-only condition. Independently reproduced 53/53 focused live-snapshot/status/context tests, real Chromium accessibility/latency journey, Python/JS syntax, diff hygiene, clean merge with current origin/main, and the same focused suite after that true-base merge. The full local run under heavy host contention produced 2416 pass / 12 skip / 6 timing failures; the four non-calendar failures passed in isolated reproduction, and the two calendar <1000ms assertions are known load-sensitive baseline failures. Corrected behavior is strict finite integer latency data, truthful <1 ms, accessible named status lists/panels, correct unavailable error taxonomy, and Notifications starting independently of auth. No merge/deploy.

Exact-head independent review of `c04595824307ec8d871bcb68eaa448e91bd8cd0c`: **APPROVED for merge consideration; not merged or deployed**. Canonical CI is green: lint 5m24s, build-release 28s, real browser journey 8m29s; release-candidate is correctly skipped on pull requests by the main-push-only condition. Independently reproduced 53/53 focused live-snapshot/status/context tests, real Chromium accessibility/latency journey, Python/JS syntax, diff hygiene, clean merge with current `origin/main`, and the same focused suite after that true-base merge. The full local run under heavy host contention produced 2416 pass / 12 skip / 6 timing failures; the four non-calendar failures passed in isolated reproduction, and the two calendar `<1000ms` assertions are known load-sensitive baseline failures. Corrected behavior is strict finite integer latency data, truthful `<1 ms`, accessible named status lists/panels, correct unavailable error taxonomy, and Notifications starting independently of auth. No merge/deploy.

timmy approved these changes 2026-08-23 03:49:28 +00:00

timmy referenced this pull request

2026-08-24 05:38:46 +00:00

Show requested-change feedback in mobile pull detail #1337

rockachopa referenced this pull request

2026-08-24 10:53:20 +00:00

Surface authored pull requests in My Work #1346

timmy referenced this pull request

2026-08-24 21:19:33 +00:00

Recover failed post-merge checks from the mobile release receipt #1366

timmy referenced this pull request

2026-08-26 04:34:03 +00:00