refactor(occupancy): PR #68 review follow-ups — sync method size, ingestion flag and anomaly-kind types, cold-start labelling #75

New issue

Open

opened 2026-09-24 22:31:45 +00:00 by gabogg · 0 comments

gabogg commented

2026-09-24 22:31:45 +00:00

Owner

Non-blocking findings from the round-3 review of PR #68 (comment 1714), deferred per the review policy. Line numbers refer to PR #68's branch before merge; re-check them after merge.

Code quality

sync_passenger_flow_from_artemis_async is ~210 lines (polling, reset detection, drift, gap reconstruction, persistence). Split into per-group reconciliation helpers. Deferred since round 2 because it touched all four stacked branches.
OccupancyManager.get_ingestion_anomalies_async is a thin pass-through to the repository. Keep as the service API or inline, but decide explicitly.
Ingestion flags as bare strings: {"FLAG_INGESTION_GAP", "FLAG_INGESTION_STALLED"} appears twice without using CalibrationAnomalyFlag. One module-level set of enum members.
Stall sentinel group_code="*" is written in the service and hardcoded again in extend_ingestion_stall_async. Name it once.
Anomaly kind declared three times: Literal["reset", "stall", "gap"] in the repository and the schema, plus the SQL CHECK. One IngestionAnomalyKind alias.
Duplicated PassengerFlowReading(...) construction in the reset and normal branches.
The stall log says "five" while the count comes from STALL_POLL_THRESHOLD.

Behaviour

A true cold start is labelled reset (e.g. a fresh deployment mid-day). Use a distinct kind (cold_start) or record it differently.
The trailing rate that sizes the drift cap counts reconstructed events, which inflates the cap right after a gap. Exclude reconstructed rows from get_group_recent_rate_async.
Stall rows start at the first failed poll, not the last successful one, and aren't linked to the gap row that follows.
A persisted reading several days old is spread across all intervening cycles with no length bound. Consider capping reconstruction length.

Acceptance criteria

Items 1–11 fixed or explicitly declined with a reason.
Full suite green.

Related: #26, #31, #30, PR #68, #74 (inline values from the same review).

> Non-blocking findings from the round-3 review of PR #68 (comment 1714), deferred per the review policy. Line numbers refer to PR #68's branch before merge; re-check them after merge. ## Code quality 1. **`sync_passenger_flow_from_artemis_async` is ~210 lines** (polling, reset detection, drift, gap reconstruction, persistence). Split into per-group reconciliation helpers. Deferred since round 2 because it touched all four stacked branches. 2. **`OccupancyManager.get_ingestion_anomalies_async` is a thin pass-through** to the repository. Keep as the service API or inline, but decide explicitly. 3. **Ingestion flags as bare strings:** `{"FLAG_INGESTION_GAP", "FLAG_INGESTION_STALLED"}` appears twice without using `CalibrationAnomalyFlag`. One module-level set of enum members. 4. **Stall sentinel `group_code="*"`** is written in the service and hardcoded again in `extend_ingestion_stall_async`. Name it once. 5. **Anomaly kind declared three times:** `Literal["reset", "stall", "gap"]` in the repository and the schema, plus the SQL `CHECK`. One `IngestionAnomalyKind` alias. 6. **Duplicated `PassengerFlowReading(...)` construction** in the reset and normal branches. 7. **The stall log says "five"** while the count comes from `STALL_POLL_THRESHOLD`. ## Behaviour 8. **A true cold start is labelled `reset`** (e.g. a fresh deployment mid-day). Use a distinct kind (`cold_start`) or record it differently. 9. **The trailing rate that sizes the drift cap counts reconstructed events**, which inflates the cap right after a gap. Exclude `reconstructed` rows from `get_group_recent_rate_async`. 10. **Stall rows start at the first failed poll**, not the last successful one, and aren't linked to the gap row that follows. 11. **A persisted reading several days old** is spread across all intervening cycles with no length bound. Consider capping reconstruction length. ## Acceptance criteria - [ ] Items 1–11 fixed or explicitly declined with a reason. - [ ] Full suite green. Related: #26, #31, #30, PR #68, #74 (inline values from the same review).

gabogg added the

ready-for-agent

label

2026-09-24 22:31:46 +00:00

gabogg referenced this issue

2026-09-24 22:32:32 +00:00

fix(occupancy): passenger flow ingestion honesty — counter resets, outage gaps, peak ordering (#26, #31, #30) #68