fix(occupancy): PR #53 review follow-ups — unlisted-group state, typed camera rows, naming #65

New issue

Closed

opened 2026-09-23 17:48:19 +00:00 by gabogg · 0 comments

gabogg commented

2026-09-23 17:48:19 +00:00

Owner

Deferred, non-blocking findings from the round-3 pre-merge review of PR #53 (comment #53 (comment)). The one merge blocker from that review was fixed in the PR itself (039ae91). Line numbers refer to the PR branch at 039ae91; re-check them after merge.

Behaviour (Spec P3s)

Unlisted flag is not retried after a failed write — reproduced. _skip_group_async (app/services/occupancy_service.py) records the new skip reason in memory before mark_group_unlisted_async writes. If that write fails once, later polls see the same reason and never retry, so the group never appears in GET /api/occupancy/camera-groups. Fix: write first, record the reason only after the write succeeds.
Stale unlisted state for groups that disappear — reproduced. is_unlisted is only cleared when HikCentral lists the group again. A group that is unlisted and then drops out of count_list entirely stays is_unlisted=True in the endpoint forever, and its stale _group_skip_reasons entry suppresses the warning if it returns with the same reason. Decide: clear or age out the flag when a group is absent from both lists, and prune _group_skip_reasons for codes not seen in the current poll.
Startup coupling — not reproduced. In app/main.py, if enforce_counting_integrity_async raises at startup, sync_initial_counts_from_artemis_async and reconcile_and_quarantine_historical_anomalies_async are skipped because they share its try. Give the integrity check its own try.

Code quality (Standards)

P2, raw dict where a schema exists (code-standards §2.3). OccupancyRepository.get_camera_async returns dict[str, Any] | None although CountingCameraItem exists; cam_payload in the camera sync carries an untyped resource_group_code key.
In-memory state deciding a DB write. _group_skip_reasons controls whether mark_group_unlisted_async runs, while camera sync clears the DB flag without touching the dict, so the two can drift. Related to items 1–2; consider deriving the "already flagged" decision from the stored row instead.
Possible Data Clump. group_cam_map[g_code] = {"name", "in_cams", "out_cams"} in the passenger flow sync is a raw dict; a small typed structure would match GroupMember.
Type placement. GroupMember and GroupSkipReason live in the service while QuarantineReason lives in app/schemas/. Pick one home. GroupSkipReason values are log sentences; consider short codes plus a message map.
Naming. QuarantineTally.total means passages; rename to passages and add a docstring.
Constant consistency. app/main.py (lines ~83 and ~100) still uses status.HTTP_422_UNPROCESSABLE_ENTITY (deprecated in the installed Starlette) while the occupancy controller uses HTTP_422_UNPROCESSABLE_CONTENT. Production runs Starlette 1.6.0, so both work; unify on _CONTENT and consider pinning a minimum Starlette version in requirements.txt, since fastapi>=0.115.0 alone does not guarantee _CONTENT exists.

Acceptance criteria

Items 1–3: each fixed with a regression test (1 and 2 were reproduced in review).
Items 4–9 addressed or explicitly declined with a reason.
Full suite green (pytest + node --test).

Related: #29, PR #53.

> Deferred, non-blocking findings from the round-3 pre-merge review of PR #53 (comment https://git.gaboggamer.online/gabogg/hikcentral/pulls/53#issuecomment-1225). The one merge blocker from that review was fixed in the PR itself (`039ae91`). Line numbers refer to the PR branch at `039ae91`; re-check them after merge. ## Behaviour (Spec P3s) 1. **Unlisted flag is not retried after a failed write** — reproduced. `_skip_group_async` (`app/services/occupancy_service.py`) records the new skip reason in memory **before** `mark_group_unlisted_async` writes. If that write fails once, later polls see the same reason and never retry, so the group never appears in `GET /api/occupancy/camera-groups`. Fix: write first, record the reason only after the write succeeds. 2. **Stale unlisted state for groups that disappear** — reproduced. `is_unlisted` is only cleared when HikCentral lists the group again. A group that is unlisted and then drops out of `count_list` entirely stays `is_unlisted=True` in the endpoint forever, and its stale `_group_skip_reasons` entry suppresses the warning if it returns with the same reason. Decide: clear or age out the flag when a group is absent from both lists, and prune `_group_skip_reasons` for codes not seen in the current poll. 3. **Startup coupling** — not reproduced. In `app/main.py`, if `enforce_counting_integrity_async` raises at startup, `sync_initial_counts_from_artemis_async` and `reconcile_and_quarantine_historical_anomalies_async` are skipped because they share its `try`. Give the integrity check its own `try`. ## Code quality (Standards) 4. **P2, raw dict where a schema exists (code-standards §2.3).** `OccupancyRepository.get_camera_async` returns `dict[str, Any] | None` although `CountingCameraItem` exists; `cam_payload` in the camera sync carries an untyped `resource_group_code` key. 5. **In-memory state deciding a DB write.** `_group_skip_reasons` controls whether `mark_group_unlisted_async` runs, while camera sync clears the DB flag without touching the dict, so the two can drift. Related to items 1–2; consider deriving the "already flagged" decision from the stored row instead. 6. **Possible Data Clump.** `group_cam_map[g_code] = {"name", "in_cams", "out_cams"}` in the passenger flow sync is a raw dict; a small typed structure would match `GroupMember`. 7. **Type placement.** `GroupMember` and `GroupSkipReason` live in the service while `QuarantineReason` lives in `app/schemas/`. Pick one home. `GroupSkipReason` values are log sentences; consider short codes plus a message map. 8. **Naming.** `QuarantineTally.total` means passages; rename to `passages` and add a docstring. 9. **Constant consistency.** `app/main.py` (lines ~83 and ~100) still uses `status.HTTP_422_UNPROCESSABLE_ENTITY` (deprecated in the installed Starlette) while the occupancy controller uses `HTTP_422_UNPROCESSABLE_CONTENT`. Production runs Starlette 1.6.0, so both work; unify on `_CONTENT` and consider pinning a minimum Starlette version in `requirements.txt`, since `fastapi>=0.115.0` alone does not guarantee `_CONTENT` exists. ## Acceptance criteria - [ ] Items 1–3: each fixed with a regression test (1 and 2 were reproduced in review). - [ ] Items 4–9 addressed or explicitly declined with a reason. - [ ] Full suite green (pytest + `node --test`). Related: #29, PR #53.

gabogg referenced this issue

2026-09-23 17:48:44 +00:00

fix(occupancy): camera-group schema, orphan guard and migration — phase 1 (#29) #53

gabogg changed title from ~~Follow-ups from PR #53 review: unlisted-group state, typed camera rows, naming~~ to fix(occupancy): PR #53 review follow-ups — unlisted-group state, typed camera rows, naming

2026-09-24 10:15:35 +00:00

gabogg added the

ready-for-agent

label

2026-09-24 10:15:36 +00:00

gabogg referenced this issue from a commit

2026-09-25 18:48:44 +00:00

fix(occupancy): address review follow-ups from PR #53 and #54 (#65, #66)

gabogg referenced this issue from a pull request that will close it,

2026-09-25 18:49:04 +00:00

fix(occupancy): address review follow-ups from PR #53 and #54 (#65, #66) #117

gabogg referenced this issue