refactor(telemetry): remove push-then-pull round-trip and batch the sync broadcast (#14) #52
No reviewers
Labels
No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gabogg/hikcentral!52
Loading…
Reference in a new issue
No description provided.
Delete branch "refactor/telemetry-streaming-deepening"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #14.
🔒 Blocked: waits for #53 to merge. Can proceed in parallel with #54 once #53 lands.
This description replaces the original draft scope following the grilling session of 2026-09-22. The original body treated #14 as a live three-candidate menu. It isn't: Candidates 3 and 4 shipped on 2026-09-10, and Candidate 2's stated friction is factually wrong about the code. See the correction on #14.
What is actually left
Issue #14's Candidate 2 claims "the current backend WebSocket broadcast in
monitor_service.pydrops individual directional passage vector events and emits coarse aggregates." That is false atmaster:occupancy_service.py:528-552builds a complete directional event —camera_index_code,camera_name,direction,count,timestamp_epoch,timestamp_formatted— and broadcasts it asrecent_event.PassengerFlowEvent(app/schemas/occupancy_models.py:234) already is the proposedPassageFluxVectorshape, and ships in bulk viaOccupancyLiveResponse.recent_passages(:285).TelemetryEngine.ingestMessageabsorbsrecent_eventinto_recentFluxVectors(telemetry_engine.js:564-572,:605-615) with no follow-up fetch.The candidate's second friction point is real, and it has a server-side twin. That pair is this PR.
1. Kill the client's push-then-pull round-trip
app/static/js/app.js:522-524:refreshOccupancyData()issuesGET /api/occupancy/overview?preset=...(app.js:1590) on everyoccupancy_update. The live card and flux stream do not need it — the broadcast carriesliveand the new events.But the fetch cannot simply be deleted. It also redraws the selected-timespan dashboard (
renderOccupancyDashboard(data): camera table and window totals forcurrentOccupancyTimespan), which the broadcast does not carry and cannot carry cheaply (it depends on each client's selected preset).Settled (2026-09-23): throttle it. The live card and flux stream are driven purely by the push. The overview fetch becomes a trailing throttle — at most one per 15 s window, only while the occupancy view is visible. Window aggregates do not need 3-second freshness; a burst still shows within seconds.
2. One recomputation and one broadcast per batch
record_counting_event_asyncrecomputesget_live_occupancy_async()and broadcasts per event (occupancy_service.py:543-552). It has several callers — the sync tick, the simulator (occupancy_controller.py:374/386), the manual endpoint (:341). (The webhook caller is deleted by #53: it was misreading door alarms as passages.)Settled (2026-09-23): batch ingestion at the interface. Add
record_counting_events_async(events: list): persist all, recompute live once, broadcast once withrecent_events: [...]. The single-event method becomes a batch of one. Every caller hands over what it has (the sync tick's events, the simulator's loop). Rejected: abroadcast=Falseflag (shallow — callers must remember an ordering rule) and timer-based coalescing (hidden state, timing-sensitive tests).The payload key changes from
recent_eventtorecent_events; updateTelemetryEngine.ingestMessage(telemetry_engine.js:564-572,:605-615) accordingly.Sequencing
Lands after #53. #53 edits the same sync loop (
occupancy_service.py:1814-1888) and deletes the webhook counting block. Under the revised plan the full group-join cutover is deferred to #62, so the even split stays — with every camera group holding exactly one camera (ADR 0005) it produces one event per group per tick. #54 no longer touches the sync loop beyond #53, so #52 and #54 can proceed in parallel after #53.No longer blocked by #26 or #31. This scope publishes nothing new — it throttles a redundant fetch and batches an existing broadcast. It reads no count, timestamp or camera code.
Out of scope — spun out
door_hardware_state_transitions. The table exists (database.py:276) and is written atdoor_repository.py:486/527/577/596, but never broadcast. That is a new capability, not a deepening.requestAnimationFramebatching inCommandDeckAdapter.render().render()writesinnerHTMLsynchronously. Deferred on cost: all 22 tests intests/frontend/test_command_deck_adapter.test.jscall private renderers directly and assert on rawinnerHTMLsubstrings, so nearly every assertion would need rewriting — against a file that took +198 lines in7ba208dand further changes ine7bf76b,bbc435aand3368f4f, all on 2026-09-21.scripts/tailwindcss. Candidate 4 shipped, but the binary is gitignored (.gitignore:9), so a fresh clone cannot rebuildtactical-bundle.min.css.Acceptance criteria
occupancy_updateno longer triggers an overview fetch per message; at most oneGET /api/occupancy/overviewper 15 s window, trailing, only while the occupancy view is visible.record_counting_events_async(list)exists; one sync tick → one live recomputation and oneoccupancy_updatebroadcast carryingrecent_events.TelemetryEngineconsumesrecent_events.node --test).🤖 Generated with Claude Code
Deliberate additions beyond the settled spec
Recorded so they are not mistaken for scope creep (review round 1):
visibilitychange. Fetching an overview nobody can see was the same waste the spec targets.content-doors) or the occupancy admin deck (content-occupancy-admin) is on screen, becauserenderOccupancyDashboardfills both.CountingEventInputschema. The typed input to the batch interface.PASSdirection removed fromPOST /api/occupancy/eventand its i18n messages.CountingEventInput.directionis nowLiteral["IN", "OUT"], andPASSrows were never counted by any aggregate. This takes one item off #64.TelemetryEngine.ingestMessage()no longer echoes locally sourced data toonRawMessage. This was already an unbounded fetch loop onmaster.app.jsbutton-label template literals (no rendered change).WIP: refactor(architecture): deepen telemetry streaming, command deck, and CSS tooling (#14)to WIP: refactor(telemetry): remove push-then-pull round-trip and batch the sync broadcast (#14)🔒 Blocked — waiting for #53 to merge.
Settled in the grilling session of 2026-09-22/23: merge order is #53 → then #52 and #54 in parallel. #53 rewrites the sync loop (
occupancy_service.py:1814-1888) and deletes the webhook counting block, both of which this PR depends on. Do not start implementation until #53 is merged; then mergemasterinto this branch first.Description updated to the settled scope. The deferred full group-join cutover is #62.
🔓 Unblocked — #53 merged (
1ed0f45, 2026-09-23).The sync loop rewrite and the webhook counting-block deletion this PR depended on are now on
master. Mergingmasterinto this branch and starting implementation. Draft status removed at the maintainer's request.Note for the implementation: after #53,
record_counting_event_asyncraisesUnregisteredCameraErrorfor unknown cameras, and the webhook is no longer a caller, so the batch interface only has three callers to migrate: the sync tick, the simulator and the manual event endpoint (#64 may delete the last one).WIP: refactor(telemetry): remove push-then-pull round-trip and batch the sync broadcast (#14)to refactor(telemetry): remove push-then-pull round-trip and batch the sync broadcast (#14)Implemented —
35783c3Backend
record_counting_events_async(list[CountingEventInput]): persists the batch, recomputes live occupancy once, broadcasts oneoccupancy_updatewithrecent_events(oldest first, capped at 50 = the client's flux buffer). Cameras are validated before any write, so a batch naming an unregistered camera records nothing.record_counting_event_asyncis now a batch of one (the manual endpoint keeps its signature).Frontend
app.jsno longer fetches/api/occupancy/overviewon every push. The live card and flux stream come from the broadcast; the selected-timespan panels refresh through a trailing throttle: at most one fetch per 15 s, only while the doors deck and the browser tab are visible, caught up onvisibilitychange.createTrailingThrottleadded tosrc/utils.js(injectable clock and timer, so it's unit-tested);app.jscreates it lazily becauseapp.jsloads before the module bootstrap.TelemetryEnginereadsrecent_events, ingesting oldest-first so the newest ends up on top; duplicates still dropped.Tests
tests/test_batched_occupancy_broadcast.py(7): one recompute and one broadcast per batch; empty batch is silent; unregistered camera → nothing written; payload cap keeps the newest; single event = batch of one; a sync tick with 8 deltas → 1 broadcast, 1 recompute; simulator → 1 broadcast. Uses the realws_manager.broadcastwith a recording client.tests/frontend/test_trailing_throttle.test.js(4) and one newTelemetryEnginetest for batched ingestion order and dedup.Verification: pytest 236 passed / 1 skipped,
node --test64/64, ruff clean, pre-commit hooks passed. The whitespace hook also trimmed trailing spaces in three existingapp.jsbutton-label template literals (no rendered change).Not covered by automated tests: the
app.jswiring itself (it is DOM glue with no test harness). Worth a quick manual check on the dashboard: the live card updates every tick, the camera/timespan panels update at most every ~15 s, and returning to a background tab refreshes them.Code review — round 1
Reviewed
35783c3againstmaster(4115881) with two independent passes: Standards (docs/standards/code-standards.md,ui-design-guidelines.md, ADR 0002,AGENTS.md, plus a code-smell baseline) and Spec (this PR's description with its two settled decisions, and #14). The Spec pass re-ran the suites in a throwaway worktree: pytest 236 passed / 1 skipped,node --test64/64.Verdict: not ready to merge. Spec has a P2 that defeats the PR's goal.
Standards
occupancy_service.py:643and the return type at:615:record_counting_events_asyncbuilds each event as a raw dict and returnslist[dict[str, Any]], althoughPassengerFlowEvent(occupancy_models.py:308) has exactly those fields. The shape predates the PR, but this diff rewrites it and spreads it to the sync tick and the controller.occupancy_models.py:301:CountingEventInput.direction: stris uppercased later in the service; aLiteral["IN","OUT"]would validate on input.occupancy_controller.py:390-415,occupancy_service.py:2060-2087: the IN and OUT branches build the sameCountingEventInput(...)block, differing only in the direction.occupancy_service.py:40:RECENT_EVENTS_BROADCAST_LIMIT = 50stays in step with the client's flux buffer only by a comment.app.js:545: a missingwindow.createTrailingThrottledrops the refresh silently.app.js:535:isOccupancyViewVisibleactually checkscontent-doors._count_live_recomputationswraps the real method, so it complies with AGENTS.md §3; the lazywindow.*lookup matches the existing module-to-app.jsbridge (ADR 0002).Spec
Correct: one recompute and one broadcast per batch on every counting path; the other
occupancy_updatebroadcasts (calibration, multiplier, camera exclusion) are one per admin action and now throttled; no reader ofrecent_eventremains; oldest-first + cap 50 matches the engine; the occupancy deck iscontent-doors; #53's guard means an unregistered camera cannot abort a sync tick.app.js:547with:1620-1629. Spec: "occupancy_updateno longer triggers an overview fetch per message; at most one … per 15 s window, trailing".refreshOccupancyData()feeds its result back throughtelemetryEngine.ingestMessage({type: 'occupancy_update'})→_handleMessage→onRawMessage→handleIncomingWsMessage→requestOccupancyRefresh(): each fetch schedules the next. With zero pushes, a fake-clock simulation fetched at t=0, 0, 15 s, 30 s … 120 s: a permanent 15 s poll, plus a duplicate at login and onswitchTab('doors'). The fake messages also bump the analytics "new records pending" counter. Onmasterthis path already loops with no limit; the throttle caps it but doesn't break it.app.js:536-539:renderOccupancyDashboardalso fills the admin camera IN/OUT table (#occ-admin-cameras-body,index.html:932), which used to refresh on every push and now never refreshes from pushes whilecontent-occupancy-adminis open. Matches the spec's wording, but should be deliberate.telemetry_engine.js:609: flux dedup also treats events with the same(camera_index_code, timestamp_epoch)as duplicates. Every event in a tick sharesnow, so on a bidirectional one-camera group (ADR 0005) the OUT event is dropped when an IN arrives in the same tick. Conflicts with "No regression to the flux stream".visibilitychangecatch-up (spec asks only for view visibility); three trimmedapp.jstemplate literals; theCountingEventInputschema (justified by the batch interface).Standards: 7 findings, worst P2 (raw event dicts). Spec: 4 findings, worst P2 (the refresh feedback loop becomes a permanent 15 s poll). All being addressed in a follow-up commit on this branch.
Review round 1 addressed —
2b78fdbSpec
TelemetryEngine.ingestMessage()is for locally sourced data and no longer callsonRawMessage; only transport (network) messages do. A fetch result can no longer schedule the next fetch. This also removes the duplicate doors render and the fake analytics "new records" bumps. Test:locally ingested data is not echoed to onRawMessage.switchTab('doors')already does).renderOccupancyDashboardfills both.PASSremoval, the loop fix, the dedup and the whitespace trim.Standards
record_counting_events_asyncbuilds and returnsPassengerFlowEventmodels;model_dump()only at the repository and broadcast boundaries.CountingEventInput.directionisLiteral["IN", "OUT"], normalised on input.POST /api/occupancy/eventand its i18n messages drop the unusedPASSdirection (noted on #64).FLUX_VECTOR_BUFFER_SIZE, and a Python test asserts it equalsRECENT_EVENTS_BROADCAST_LIMIT.app.jswarns once ifcreateTrailingThrottleis missing.isOccupancyViewVisible→isOccupancyOverviewOnScreen, checking an explicit list of decks.Verification: pytest 239 passed / 1 skipped,
node --test67/67, ruff clean, pre-commit hooks passed.Code review — round 2 (follow-up)
Reviewed
2b78fdbagainstmasterwith the same two independent passes. CI green; merges cleanly.Verdict: ready to merge. Every round-1 finding holds; no P1 or P2 on either axis.
Standards
Round 1: all hold. The
PassengerFlowEventreturn serialises to the same JSON;divmodequals the old///%; both locales updated consistently; the regex test reading the JS file fails loudly if the line changes, which is acceptable./simulatethe loop variablecams(occupancy_controller.py:~392) reuses the name of the all-cameras list.ingestMessage/onRawMessagecontract is an inline comment, not JSDoc.Literal).PassengerFlowEvent.directionand the single-event method'sdirectionstaystr.member.codein the same function.ZeroDivisionErrorin the split: not reachable. Since #53 an empty direction list falls back to the member cameras, and memberless groups are skipped first.Spec
Round 1: all 4 hold. Fake-clock simulation with the real engine and throttle: zero pushes over 120 s → 1 fetch (login), 0 echoes; a push every 3 s → fetches at 0, 0, 15, 30 … 120 s. Nothing depended on the removed echo: both local fetches render directly; the overview has no
server_time; real admin actions still reach analytics over the network. Login fetches hold for every role (doorsis inOPERATOR_ALLOWED_DECKS). No producer ofPASSremains. pytest 239 passed,node --test67/67.refreshOccupancyData()calls don't reset the throttle window, so a push right after one causes a second fetch.PASSremoval touches #64's endpoint. #64 already records it (comment 1373).Standards: 5 findings, worst P3. Spec: 1 finding, worst P3. All filed in #67. Merging.