fix(doors): async webhook persistence and transition cleanup (#95, #102)

gabogg commented

2026-09-25 18:14:27 +00:00

Owner

Summary

Addresses #95 and #102. The async webhook route previously ran synchronous SQLite ingestion on the event loop. Door transition inputs also traveled as untyped dictionaries, and the telemetry engine had duplicate state mappings with labels that differed from the server.

Architectural Impact

DoorStateManager.handle_webhook_event_async runs the existing synchronous ingestion in a worker thread, serializes concurrent webhooks, and returns persisted transition batches to the event loop for broadcast before the overview message. The controller also builds the overview in a worker thread. DoorTransitionInput is the typed contract from polling, webhook, and reconciliation through the repository. The transition broadcaster uses the door cache's canonical exclusionSource and no longer performs a synchronous database fallback on async paths. The frontend now shares one numeric state presentation map and uses the server's door-state labels.

Verification

rtk pytest -q: 330 passed, 1 skipped.
node --test tests/frontend/*.test.js: all 6 test files passed.
Ruff lint and format pre-commit hooks passed.
python3 scripts/check_docs.py: 33 Markdown files and 72 HTTP operations checked.

Checklist

Webhook persistence and overview reads leave the event loop responsive; an ASGI test delays a real SQLite transition write and checks the loop remains free.
Persisted webhook transitions are broadcast after the write; failed writes emit nothing.
Polling followed by reconciliation does not duplicate a transition.
Typed transition input replaces raw records in all four producer paths.
Canonical exclusion source, shared engine state mapping, and commanded/offline labels match the server.

Addresses #95. Addresses #102.

Second-pass follow-ups

Small follow-ups are recorded on #95 and #102. These issues remain open after this PR merges. The second-pass review found no merge blocker.

## Summary Addresses #95 and #102. The async webhook route previously ran synchronous SQLite ingestion on the event loop. Door transition inputs also traveled as untyped dictionaries, and the telemetry engine had duplicate state mappings with labels that differed from the server. ## Architectural Impact `DoorStateManager.handle_webhook_event_async` runs the existing synchronous ingestion in a worker thread, serializes concurrent webhooks, and returns persisted transition batches to the event loop for broadcast before the overview message. The controller also builds the overview in a worker thread. `DoorTransitionInput` is the typed contract from polling, webhook, and reconciliation through the repository. The transition broadcaster uses the door cache's canonical `exclusionSource` and no longer performs a synchronous database fallback on async paths. The frontend now shares one numeric state presentation map and uses the server's door-state labels. ## Verification - `rtk pytest -q`: 330 passed, 1 skipped. - `node --test tests/frontend/*.test.js`: all 6 test files passed. - Ruff lint and format pre-commit hooks passed. - `python3 scripts/check_docs.py`: 33 Markdown files and 72 HTTP operations checked. ## Checklist - [x] Webhook persistence and overview reads leave the event loop responsive; an ASGI test delays a real SQLite transition write and checks the loop remains free. - [x] Persisted webhook transitions are broadcast after the write; failed writes emit nothing. - [x] Polling followed by reconciliation does not duplicate a transition. - [x] Typed transition input replaces raw records in all four producer paths. - [x] Canonical exclusion source, shared engine state mapping, and commanded/offline labels match the server. Addresses #95. Addresses #102. ## Second-pass follow-ups Small follow-ups are recorded on #95 and #102. These issues remain open after this PR merges. The second-pass review found no merge blocker.

gabogg added 1 commit

2026-09-25 18:14:28 +00:00

fix(doors): move webhook persistence off loop and type transitions

CI / lint-and-test (pull_request) Successful in 1m31s

Details

ea81ad356f

gabogg commented

2026-09-25 18:20:37 +00:00

Author

Owner

Review of PR #112 (634db84...ea81ad3) against issues #95 and #102.

Standards

Judgement call — possible Duplicated Code (low severity): app/services/door_service.py:567-592 and :752-777 each build a near-identical DoorTransitionInput(...) with the same state fields, trigger_source="POLLING_SYNC", timestamp formatting, and details_json. This duplication predates the PR as dict literals, but remains in the changed hunks. A shared constructor could prevent the two polling paths from drifting. No hard documented-standard violations found.

Spec

Cancellation can lose a persisted transition broadcast: Issue #95 requires preserving the “one-broadcast-per-persisted-transition rule” and event ordering. In app/services/door_service.py:806-814, cancelling the request during await asyncio.to_thread(...) runs finally immediately. The worker thread can continue after the batch list is drained and the lock released. If it persists and appends a transition afterward, that transition is never broadcast; a second webhook can also overlap the first worker. Keep the lock and batch collection active until the worker completes, including on cancellation, then emit the completed batches on the event loop.

Summary: Standards: 1 low severity judgement call (worst: duplicated polling construction). Spec: 1 functional finding (worst: lost broadcast on cancellation).

Review of PR #112 (`634db84...ea81ad3`) against issues #95 and #102. ## Standards - **Judgement call — possible Duplicated Code (low severity):** `app/services/door_service.py:567-592` and `:752-777` each build a near-identical `DoorTransitionInput(...)` with the same state fields, `trigger_source="POLLING_SYNC"`, timestamp formatting, and `details_json`. This duplication predates the PR as dict literals, but remains in the changed hunks. A shared constructor could prevent the two polling paths from drifting. No hard documented-standard violations found. ## Spec - **Cancellation can lose a persisted transition broadcast:** Issue #95 requires preserving the “one-broadcast-per-persisted-transition rule” and event ordering. In `app/services/door_service.py:806-814`, cancelling the request during `await asyncio.to_thread(...)` runs `finally` immediately. The worker thread can continue after the batch list is drained and the lock released. If it persists and appends a transition afterward, that transition is never broadcast; a second webhook can also overlap the first worker. Keep the lock and batch collection active until the worker completes, including on cancellation, then emit the completed batches on the event loop. Summary: Standards: 1 low severity judgement call (worst: duplicated polling construction). Spec: 1 functional finding (worst: lost broadcast on cancellation).

gabogg added 1 commit

2026-09-25 18:33:45 +00:00

fix(doors): preserve webhook broadcasts after cancellation

CI / lint-and-test (pull_request) Successful in 1m36s

Details

f11feea63c

Keep cancelled webhook persistence and broadcasts in an owned task so the worker completes under the serialization lock. Reuse one polling transition constructor across sync and async paths. Add a regression for cancellation during a real SQLite write. Addresses #95 and #102.

gabogg referenced this pull request

2026-09-25 18:43:43 +00:00

fix(webhook): avoid synchronous SQLite writes in async event route #95

gabogg referenced this pull request

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

follow-up(telemetry): P3 cleanups from #94 review (door transition broadcast) #102

gabogg commented

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

Author

Owner

Second-pass review of PR #112 at f11feea against issues #95 and #102. The cancellation fix and shared polling transition constructor address the first review's findings. No major merge blocker found.

Standards

Minor (documented standard): Several new transition test functions/helpers omit explicit parameter or return annotations (tests/test_door_transition_broadcast.py:153,160,189,296), contrary to docs/standards/code-standards.md §2.2. Tracked in issue #102.

Spec

Minor (#95): The webhook's threaded overview can race async polling when a new door is inserted into self.doors, raising RuntimeError: dictionary changed size during iteration. A deterministic reproduction confirmed it; the already persisted transition still broadcasts. Tracked in issue #95.
Minor (#95): A card-only webhook's immediate threaded overview cannot start person_directory.request() because it has no running event loop. The monitor can start lookup later. Tracked in issue #95.
Minor (#102): An OFFLINE or REMAIN_CLOSED doors_update is still normalized to CLOSED by _ingestDoors() despite the new state presentation helper. This behavior predates this PR; a direct frontend reproduction confirmed it. Tracked in issue #102.

Verification: 330 Python tests passed, 1 skipped; all 6 frontend test files passed; Ruff checks and PR CI passed. Summary: Standards 1 minor (worst: missing test annotations); Spec 3 minor (worst: intermittent threaded-overview race).

Second-pass review of PR #112 at `f11feea` against issues #95 and #102. The cancellation fix and shared polling transition constructor address the first review's findings. No major merge blocker found. ## Standards - **Minor (documented standard):** Several new transition test functions/helpers omit explicit parameter or return annotations (`tests/test_door_transition_broadcast.py:153,160,189,296`), contrary to `docs/standards/code-standards.md` §2.2. Tracked in [issue #102](https://git.gaboggamer.online/gabogg/hikcentral/issues/102#issuecomment-2240). ## Spec - **Minor (#95):** The webhook's threaded overview can race async polling when a new door is inserted into `self.doors`, raising `RuntimeError: dictionary changed size during iteration`. A deterministic reproduction confirmed it; the already persisted transition still broadcasts. Tracked in [issue #95](https://git.gaboggamer.online/gabogg/hikcentral/issues/95#issuecomment-2238). - **Minor (#95):** A card-only webhook's immediate threaded overview cannot start `person_directory.request()` because it has no running event loop. The monitor can start lookup later. Tracked in [issue #95](https://git.gaboggamer.online/gabogg/hikcentral/issues/95#issuecomment-2238). - **Minor (#102):** An OFFLINE or REMAIN_CLOSED `doors_update` is still normalized to CLOSED by `_ingestDoors()` despite the new state presentation helper. This behavior predates this PR; a direct frontend reproduction confirmed it. Tracked in [issue #102](https://git.gaboggamer.online/gabogg/hikcentral/issues/102#issuecomment-2240). Verification: 330 Python tests passed, 1 skipped; all 6 frontend test files passed; Ruff checks and PR CI passed. Summary: Standards 1 minor (worst: missing test annotations); Spec 3 minor (worst: intermittent threaded-overview race).

gabogg merged commit 1b9e406b46 into master

2026-09-25 18:45:10 +00:00

gabogg deleted branch fix/door-transition-async-cleanup

2026-09-25 18:45:11 +00:00

gabogg referenced this pull request from a commit

2026-09-25 18:45:11 +00:00

Merge pull request 'fix(doors): async webhook persistence and transition cleanup (#95, #102)' (#112) from fix/door-transition-async-cleanup into master