SCADA: fix poll-loop data integrity, cadence and backpressure (#4004 #4008 #4009 #4023 #4024) - #4064
Closed
dieterolson wants to merge 6 commits into
Closed
SCADA: fix poll-loop data integrity, cadence and backpressure (#4004 #4008 #4009 #4023 #4024)#4064dieterolson wants to merge 6 commits into
dieterolson wants to merge 6 commits into
Conversation
- #4004: classify each scan's provenance (live/simulated/held/fault). Held and faulted values never reach the control laws, the alarm engine or the historian, so a link flap can no longer clear an active HiHi. The backup simulator is only wired into the scan path on a simulator driver. TagLog gains a `quality` column (with migration) so an outage is a gap, not fabricated continuity. Frames carry data_source/plc_connected/simulated, and a live scan strokes the firmware heartbeat (#4044). - #4008: pin the scan/alarm/relay/E-stop cadence to settings.poll_interval_s; the performance mode now only decimates the WebSocket broadcast. - #4009: schedule against a monotonic deadline with overrun counting and phase resync; size the Modbus timeout to the scan period; compute the failure backoff from the active period. - #4023: batched, retrying historian writer on a worker thread. - #4024: per-client bounded WS queues, serialise once, drop oldest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a83e6a607
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+399
to
+402
| for record in samples: | ||
| written += self._log_scan( | ||
| session, dict(record.tags or {}), quality=record.quality | ||
| ) |
There was a problem hiding this comment.
When the writer queue batches records or waits through a database lock, _log_scan is called without a timestamp, so it assigns the worker's current time rather than the time each PLC scan occurred. A delayed batch therefore shifts and nearly collapses distinct measurements onto the commit time, corrupting trend cadence and experiment timing; store a scan timestamp in HistorianRecord and pass it through to log_scan.
Useful? React with 👍 / 👎.
Comment on lines
+731
to
+735
| source = _resolve_source( | ||
| plc_connected=plc_connected, | ||
| tags_read=fresh_tags is not None, | ||
| have_held=bool(latest_tag_values), | ||
| simulated=simulated, |
There was a problem hiding this comment.
On a real-driver startup where the PLC is connected but its first read fails, have_held is always true because SystemState.latest_tags is prepopulated with 32 zero defaults (state.py:21). The scan is consequently classified as held, and those fabricated zeros are streamed as the supposed last good readings; the frontend accepts the 32-element tags array into its live values and history (frontend/src/hooks/useTelemetryStream.ts:154-155). Track whether a successful measurement has actually occurred instead of using dictionary truthiness.
Useful? React with 👍 / 👎.
Comment on lines
+116
to
+120
| if remaining < 0.0: | ||
| self._overrun_count += 1 | ||
| self._last_overrun_s = -remaining | ||
| self._next_deadline = now + self._period_s | ||
| return 0.0 |
There was a problem hiding this comment.
Whenever a scan exceeds its deadline, this rebases the next deadline to now + period but returns zero, so poll_plc_loop immediately begins another scan rather than waiting for that rebased deadline. This creates a back-to-back control cycle after every overrun—including the recovery scan after a deliberate failure backoff—and contradicts the intended no-catch-up-burst cadence; the overrun path should return the new period or otherwise sleep until the rebased deadline.
Useful? React with 👍 / 👎.
Comment on lines
+38
to
+42
| self.timeout_s = ( | ||
| float(timeout_s) | ||
| if timeout_s is not None | ||
| else get_settings().resolved_modbus_timeout_s | ||
| ) |
There was a problem hiding this comment.
When PLCFactory.create_client(custom_settings) is used with an explicit timeout or non-default poll interval, the factory supplies the custom host and port but not timeout_s, so this fallback reads the unrelated process-wide cached settings and silently applies the wrong Modbus timeout. Resolve the timeout from the same injected P1AMSettings object and pass it into AsyncModbusManager.
AGENTS.md reference: AGENTS.md:L149-L151
Useful? React with 👍 / 👎.
dieterolson
enabled auto-merge (squash)
August 1, 2026 14:49
This was referenced Aug 2, 2026
Contributor
Performance Benchmark ResultsNo benchmark results available. |
dieterolson
pushed a commit
that referenced
this pull request
Aug 14, 2026
`quality-gate` failed on Module Size Budget: `backend/main.py` had reached 1547 lines against its grandfathered baseline of 1440 (hard max 1200). This batch added the E-stop write-seam refusals, the acknowledged-step requirement and the shutdown sequencing to that module, and #4064 had already split out `poll_runtime` for the same reason. Bumping the baseline would have been the cheap fix and the wrong one — the baseline exists as a ratchet, and raising it to accommodate growth is the one thing it is there to prevent. Extracted instead: `tuning_router.py` takes the one cohesive group left in main.py, the handlers that write to or identify the dynamics of a single control loop — POST /api/tags/{tag_id} POST /api/pid/{pid_index}/tuning/start POST /api/pid/{pid_index}/tuning/step POST /api/pid/{pid_index}/tuning/stop POST /api/mpc/simulate plus `MPCSimulatePayload`, `TagWritePayload` and the `_latest_tag_or_http_error` helper. main.py: 1547 -> 1341 lines, inside the baseline. Follows the router-factory pattern already used in this package by `power_supply_integration.create_power_supply_router` and `temperature_integration.create_temperature_router`. Collaborators (control_context, plc_client, backup_simulator, the E-stop refusal callable, the admin dependency, the logger) are injected rather than imported from `main`, which would close an import cycle. Behaviour is unchanged; the routes, paths and admin gating are identical, which `test_route_authz_matrix.py` verifies from the live app in both directions. One trap worth recording: the request model must be imported as a real type, not injected as a parameter. This module uses `from __future__ import annotations`, so every annotation is a string at runtime and FastAPI resolves it against the module globals — an injected `step_payload_model` left the annotation as the literal `"step_payload_model"`, the body never bound, and all six tuning-step tests returned 422. `models` imports nothing from `main`, so importing `PIDTuningStepPayload` directly is safe. P1AM suite: 1388 passed, 6 skipped. ruff check + format clean. Module size budget passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 14, 2026
dieterolson
added a commit
that referenced
this pull request
Aug 16, 2026
…6 issues) (#4448) * fix(p1am): stop reporting untrustworthy PID tunings and fix MPC move formulation PID tuning (#4018): - as_response() clamped every recommended gain to >= 0. A reverse-acting process (identified Kp < 0) legitimately tunes to negative Cohen-Coon gains, so the clamp produced kp=ki=kd=0 and still reported "Tuning parameters identified successfully" - an open-loop controller presented as tuned. Gains now keep their sign and a reverse-acting identification is reported as a warning naming the required loop configuration. - Dead time was floored at MIN_TIME_PARAM with no noise rejection and no minimum-sample check. Because Kc scales with tau/theta, a first crossing landing on the step sample inflated the recommendation by more than an order of magnitude and returned it as success. The identification is now rejected when the first crossing falls within two sample intervals of the step or both thresholds land on the same sample, and downgraded to a warning when theta hits the floor, when the dead-time ratio leaves the Cohen-Coon validity band, or when a gain exceeds the sanity bound. - Two-point identification now uses the published 28.3%/63.2% pair with tau = 1.5*(t63 - t28) and theta = t63 - tau. The old 10% pair biased theta high and tau low by roughly 0.105*tau. The Cohen-Coon coefficient formulas were verified against Cohen & Coon (1953) and left unchanged. MPC (#4039): - The DMC solver initialised its decision variable to the absolute CV and then multiplied it by the step-response dynamic matrix as if it were a move vector, while free_response already carried the full effect of holding that CV. The current input was counted twice, leaving the /api/mpc/simulate comparison chart with a large permanent offset at any nonzero operating point. The solver is extracted into solve_dmc_move(), now solves for delta-u from a zero origin, bounds the moves, and the caller integrates and clamps to the 0-100% output range. Refs #4033: the clamp test was vacuous (step_triggered=False made the gains 0.0 by construction) and is replaced by tests that build a TuningResult with genuinely negative gains and assert the new contract. test_backend_p1am_safety.py: the fixed-history tuning test encoded the biased method - its PV values 11.0 and 16.32 were exactly the old 10% and 63.2% thresholds, recorded at a 1 s sample interval with two points on the transient, which the corrected identification cannot resolve. The same plant (Kp=1, tau=1, theta=0.5) is now sampled at 0.1 s, and the old under-sampled recording is retained as a rejection fixture so the guard itself is covered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(p1am-hmi): correct alarm annunciation, thresholds, E-stop gate and log churn Closes #4012, #4019, #4021, #4022. - Alarm annunciation now separates "condition present" (colour) from "acknowledged" (flash vs steady). Acking a still-active alarm no longer hides it for the session, and a cleared alarm is dropped from both sets so the ACK button stops flashing. Acknowledge applies only to the alarms the header was displaying. Extracted into desktop/alarm_state.py. - HH/LL severity uses the deployed hihi_limit/lolo_limit rather than high_limit +/- 5, so HMI severity matches the firmware trip points. Interlock ordering is validated at config load and a bad set is rejected loudly. - Connection status is derived from the telemetry frame (desktop/connection_state.py); a live plant is no longer labelled "Simulating" forever. - Clearing the E-stop is Admin-gated and modally confirmed via the shared desktop/guards.py helpers; a declined clear latches the button back to tripped. - Alarm events are debounced/counted, written in batches on a background thread over one persistent SQLite connection, the History table is requeried only while visible, and the event table has a retention window. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(p1am-hmi): report data age, not a boolean; stop silent alarm loss The operator HMI could tell an operator a dead process was healthy, and could command hardware from values the operator never entered. #4010 — every field of telemetryFrameSchema is optional, so `{}` parsed successfully and applyFrame stamped it as a live frame. The backend's latest_frame starts as {} and is only reassigned on a successful poll, so a dead poll loop left the HMI green forever with the trend appending a frozen value. Liveness now requires a recognised payload field, is expressed as a data AGE, and stale process values are greyed and cross-hatched so frozen is distinguishable from steady. #4011 — .catch sat on the whole active_alarms record, so one malformed alarm erased the entire map and the header kept saying "All normal — no active alarms" while alarms fired on the PLC. Resilience is now per entry, dropped entries raise a degraded-data banner, and /api/alarms/active is reconciled periodically so the list has a recovery path independent of the stream. #4013 — the Alicat setpoint draft was re-seeded from live telemetry on essentially every frame, so it fought each keystroke and Set could commit a partially-reverted value. The entry is operator-owned from the first keystroke; the device setpoint is now its own readout. #4020 — the power-supply staged setpoint was never seeded and +/- commanded immediately, so one tap after a kiosk reload could collapse a 30 A output by 29 A. It seeds from the live setpoint and +/- stage only; Apply is the sole write path, as the file's contract already stated. #4042 — the event log is fetched on mount and refreshed on an interval instead of only after an unrelated acknowledgement; approaching-alarm cues come from the server-enforced config rather than the local draft, and config entry rejects non-finite input that used to store NaN and silently switch the pre-alarm cue off. #3996 — none of the frontend's lint, build or test scripts were run by any workflow. Adds .github/workflows/p1am-frontend.yml. Making that gate useful also required repairing the pre-existing suite: Node >= 22 ships its own localStorage global, which blocks vitest's jsdom Storage and made 15 layout-persistence tests fail with "localStorage.clear is not a function". schemas.test.ts previously asserted the #4011 alarm suppression as correct behaviour; that test is rewritten. The tuning tab is extracted to TuningPanel.tsx to bring App.tsx back under the 1500-line budget. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(p1am/historian): unblock E-stop during retention, fix UTC drift, bound explorer loads Closes #4006, #4025, #4026, #4027, #4040. Refs #4033. - #4006 (P0): historian_retention_loop is an asyncio task, so its synchronous SQLite body ran ON the event loop, freezing the control loop, the websocket broadcast and every HTTP endpoint including E-stop for the length of a VACUUM (tens of seconds on a 1 GiB DB on SD). The sweep now runs via asyncio.to_thread, and reclaims disk in bounded incremental_vacuum chunks instead of a whole-file VACUUM. Legacy DBs are converted to auto_vacuum=INCREMENTAL once at startup, before the controller is live. - #4025: TagLog/EventLog.timestamp now use a UtcDateTime TypeDecorator, so SQLite can no longer discard tzinfo on write nor hand back naive datetimes on read. Every isoformat() on the API boundary emits an explicit offset. - #4026: _load_historian decides its cell budget from row COUNTs BEFORE reading any rows, and honours HistorianSource.max_points (previously dead) by reusing query_trend_series' streamed decimation, so peak memory tracks output size. - #4027: the size cap is split into separately-tracked TagLog and EventLog budgets, each charged against its own dbstat footprint, and EventLog gains age-based retention. A fat event log can no longer inflate bytes-per-row and erase the tag historian. - #4040: export column alignment is validated unconditionally, so ragged columns are a 400 instead of a truncated CSV body behind an HTTP 200. - #4033: the index-migration test now calls database._migrate_historian_indexes instead of re-executing a hand-copied duplicate of its SQL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(p1am): de-energize on E-stop and on shutdown; kill dead write seams An E-stop never opened the heater relay, and half of what it did write was a provable no-op. Shutdown never de-energized anything and blocked past the systemd kill timeout. Direct tag writes reported success for writes the firmware overwrites within one scan. - trigger_estop opens HEATER_RELAY_COIL first and treats a failed ack as a hard error; the 64-register TAG_VALUE_BASE block write is removed (the firmware republishes 0..63 from its broker every scan and never reads them back). POST /api/estop reports success only on an acknowledged de-energize. - lifespan teardown drives the plant safe before joining tasks or closing sockets, on the exception path too, under a deadline below TimeoutStopSec. The PLC-connect retry waits on shutdown_event instead of sleeping past it. - write_tag raises NotImplementedError for the republished TAG_n block instead of returning True; the PID tuner steps through write_pid_setpoint and skips identification unless the step was acknowledged. - write_tag and write_routing now honour the defense-in-depth E-stop latch; a contract test fails if a new write seam escapes the latch registry. - New write_heartbeat seam for the firmware liveness watchdog (register 560). Per-scan call site still needs wiring by the poll-loop PR. - E-stop tests assert addresses and values, with a fake recording both write_registers and write_coil, instead of a write count. Closes #4000 Closes #4005 Closes #4015 Closes #4038 Refs #4033 Refs #3999 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * 🐛 P1AM SCADA: safe calibration shutdown, real alarm ack, settings-driven MFCs Fixes three P1 defects on the P1AM control system. #3997 — calibration left analog outputs energized. teardown now commands each pass-through PID to 0.0, reads the AO tag back to CONFIRM 0%, then unmaps the PIDs and releases the output routing so the firmware's own WriteAnalogOutput(i, 0.0f) safe path takes over. main() drives the AOs to 0% on any abnormal exit (exception, SystemExit, KeyboardInterrupt) before plc.close(), logging loudly without masking the original cause. #4034 — alarm acknowledgement never reached the alarm engine. The endpoint now forwards to AlarmEngine.acknowledge_alarm(tag_id, user) and populates acknowledged_by; apply_config migrates alarm state (including the ack) into the rebuilt engine through the public API shared by the Rust and Python engines, so an ack survives a routing deploy and a PLC reconnect. #4031 — every mass flow controller was hardcoded to simulation. The transport now comes from P1AM_ALICAT_CONNECTION_TYPE / P1AM_ALICAT_PORT_OR_IP; mock devices are refused whenever plc_driver drives real hardware (gas control is then absent, with registration_error set and a CRITICAL log, rather than silently simulated). AlicatMFC validates its transport and port, and parse_ascii_response no longer bypasses the VALID_GASES check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * 🛡️ SCADA: fix poll-loop data integrity, cadence and backpressure - #4004: classify each scan's provenance (live/simulated/held/fault). Held and faulted values never reach the control laws, the alarm engine or the historian, so a link flap can no longer clear an active HiHi. The backup simulator is only wired into the scan path on a simulator driver. TagLog gains a `quality` column (with migration) so an outage is a gap, not fabricated continuity. Frames carry data_source/plc_connected/simulated, and a live scan strokes the firmware heartbeat (#4044). - #4008: pin the scan/alarm/relay/E-stop cadence to settings.poll_interval_s; the performance mode now only decimates the WebSocket broadcast. - #4009: schedule against a monotonic deadline with overrun counting and phase resync; size the Modbus timeout to the scan period; compute the failure backoff from the active period. - #4023: batched, retrying historian writer on a worker thread. - #4024: per-client bounded WS queues, serialise once, drop oldest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * 🔒 Harden P1AM SCADA deployment: the authz was fine, the deployment wasn't The in-source authorization was already correct — every hardware-mutating route carried require_admin_key, E-stop clear was admin-gated, the WebSocket was authenticated, comparisons used hmac.compare_digest, and cors_config.py failed closed. Every exploitable defect was in the deployment, or in the client's inability to authenticate. #4007 (P0) — production installs shipped with auth disabled, and the HMI re-exposed the loopback-bound backend. Fixed as one change, because fixing any part alone ships something either exposed or unusable: - the HMI can now authenticate (frontend/src/api/credentials.ts): X-API-Key on every request, the key sent as the FIRST WebSocket frame rather than a query parameter, stored per browser profile, seeded by the kiosk launcher through a URL fragment it strips on load; - install-services.sh no longer bakes P1AM_DEV_NO_AUTH=1 into the unit. It generates random operator/admin keys into a root-owned EnvironmentFile, preserves them across re-runs, refuses to write a unit without one, and gates the bypass behind an explicit --bench flag; - vite preview binds 127.0.0.1 instead of every interface. #4041 — the credential tiers are now nested. A configured admin key satisfies the operator tier, so an admin-only deployment no longer means full hardware control behind a dead display. The resolved posture is logged at boot. #4037 — require_read_auth defaults on and is attached to the ungated read routes (/api/routing disclosed the whole register map). RequestGuardMiddleware refuses state-changing requests from a non-allowlisted Origin and forces a preflight, closing the bodyless CORS-simple POST path that let any page the kiosk opened command the plant. The panic stop stays curl-reachable. #4029 — an append-only AuditEvent table plus one ASGI middleware, so a NEW endpoint is audited by default. Records route, redacted payload, credential tier, non-reversible key fingerprint, client IP and status; unreachable from the client-writable POST /api/events and untouched by /api/capture/clear, so the trail can be neither forged nor erased. Mirrored to journald. #4028 — test_route_authz_matrix.py boots the real app with keys set and the bypass cleared, driving an explicit (method, path, tier) table. An unclassified route fails the suite, so a new endpoint cannot ship ungated. #4014/#4030/#4036 — the deployment can actually work: a `p1am` extra is the single source of truth for backend runtime deps (adding the missing pydantic-settings and python-multipart), mirrored as exact pins in the Dockerfile with drift caught by a test; the container binds 0.0.0.0 and is isolated at the publish layer; compose uses the env-var names settings.py reads and mounts the historian at /data, not over the source; the HMI is built at install time and both units carry Nice=/CPUWeight=; the numpy lock no longer contradicts requirements.txt; and PLCFactory logs an unmissable banner when the simulator is driving the HMI's "live" values. Also fixes #4061: several suites mutated os.environ at module import time and one popped P1AM_DEV_NO_AUTH there, so whichever module was collected last decided whether seven endpoint tests passed or 503'd. Those are now per-test monkeypatch fixtures. Verified green in both collection orders. Closes #4007 Closes #4014 Closes #4028 Closes #4029 Closes #4030 Closes #4036 Closes #4037 Closes #4041 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(p1am): anchor historian DB to package dir; ignore stray dcs_scada.db `database.DATABASE_URL` was a bare relative `sqlite:///dcs_scada.db`, so the SQLite file resolved against the process CWD rather than a fixed location. Two consequences: 1. The historian silently forked into a different DB per launch directory. A `uvicorn main:app` from `backend/`, a test run from the repo root, and a systemd unit with its own WorkingDirectory each got their own file, so tag history appeared to vanish depending on how the backend was started. 2. `pytest src/p1am_control_system/backend/tests/` from the repo root dropped an untracked `dcs_scada.db` at the repo root, which `git add -A` would stage. The .gitignore entries were path-anchored to the backend directory and did not cover it. Fixes: - `DB_FILE` now resolves to an absolute path anchored to the backend package directory via `Path(__file__).resolve().parent`, with a `P1AM_DB_PATH` env override for deployments keeping the historian on separate storage. This makes the location documented in BENCH_HANDOFF.md (`backend/dcs_scada.db`) authoritative. The URL uses `as_posix()` so a Windows drive path does not put backslashes into `sqlite:///...`, where they would read as escapes. - Docker is unaffected: the image does `WORKDIR /app` + `COPY . /app`, so the package directory *is* `/app` -- the same path the compose `dcs_db_data` volume mounts. The default resolves where it always did. - Also fixes `data_capture._db_size_bytes`, which called `os.path.getsize(DB_FILE + suffix)` and was therefore measuring whichever DB happened to sit in the CWD (or nothing), making the historian size cap and status display inaccurate. - .gitignore entries are now unanchored (no slash => git matches at any depth), covering both the backend directory and any other CWD. Verified: full p1am backend suite passes from the repo root (878 passed, 6 skipped), no stray DB at the root afterwards, ruff check + format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(p1am-frontend): install conforming localStorage in test setup 15 tests across panelLayout.test.ts, usePanelLayout.test.ts and PanelStack.test.tsx failed with "TypeError: localStorage.clear is not a function". Root cause is in vitest 1.6's populateGlobal, which only copies a jsdom global over an already-present Node global when the key is in its own curated KEYS list: if (k in global) return keysArray.includes(k); Neither localStorage nor sessionStorage appears in that list, and Node 22+ defines both on globalThis, so jsdom's Storage is never installed and Node's wins. Node's sessionStorage is in-memory and behaves, which is why only localStorage broke; its localStorage is backed by --localstorage-file and, with no valid path, degrades to a bare object with none of the Storage methods. jsdom's own localStorage works fine here, it is simply unreachable, so pinning the environment is not a fix: vitest 1.6 exposes no lever to add keys to that list. Install a spec-shaped in-memory Storage in the shared test setup instead, which fixes all three suites at once and any future one. Installed unconditionally rather than behind a conformance guard: setup.ts only runs under the test runner, a fresh Storage per test file is the isolation we want, and merely reading Node's lazy localStorage to probe it fires a --localstorage-file warning in every worker. Verified: 44 files / 381 tests pass (was 366 passed, 15 failed), tsc --noEmit clean, eslint 0 errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(p1am): configure auth per test, not at import time (#4061) The two test modules this branch introduced set P1AM_DEV_NO_AUTH by mutating os.environ at import time. settings is a module-level singleton read once at first import, so whether the variable lands before or after that import depends on collection order and xdist worker assignment. Concretely, in the combined run `pytest src/p1am_control_system/backend/tests/ tests/p1am_control_system/`, tests/p1am_control_system/test_backend_security.py imports last and pops the variable, so TestTunerStepWritePath fails with 503. Reverse the two path arguments and the same tests pass. Order-dependence in the direction of passing is the dangerous direction over an E-stop write path. - test_estop_endpoint_writes.py: import-time assignment replaced with an autouse `_bench_no_auth(monkeypatch)` fixture, matching the convention PR #4066 established for the pre-existing instances. Its TestClient also carries the X-Requested-With HMI marker that PR's request guard expects (inert here, avoids a second convention). - test_estop_shutdown_safe_state.py: assignment removed outright. The module drives the lifespan and the safe-state seams directly and issues no HTTP request, so it needs no credential posture — and its import-time mutation was leaking into every suite collected after it. Refs #4061 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(p1am): scale the power-supply thermocouple, fault on missing feedback Four defects in the power-supply and temperature sensor paths, each with a failing test written first. #4003 -- the firmware publishes thermocouples as PERCENT of full scale, and _inputs_from_tags scaled current and voltage but passed the temperature tag through as though it were already degC. HH_TEMP compares that against a degC threshold (default 1200), and broker tags are bounded [0, 100], so the power supply's only over-temperature interlock could not fire under ANY physically possible reading. The HMI showed "86 degC" on a load at 1200 degC, so the automatic protection and the human backstop were reading the same wrong number. #3998 -- full scale was declared in three places (firmware, temperature_models, thermocouple_filter) with a comment asking them to agree. It is now one definition in hardware.py, the module that already owns the firmware contract, with percent_to_celsius/celsius_to_percent as the only conversion. A new test parses the firmware source and fails if the two halves drift, so the contract is a gate rather than a comment. #4016 -- any absent tag became 0.0 via dict.get, and non-finite readings were coerced to 0.0. Unlike the temperature controller there was no sensor-fault state, so a renamed tag or a pulled monitor lead silently zeroed both trip inputs while the output stayed energized, and reported a confident, cold-looking supply. Missing or unusable feedback now raises SensorFeedbackError and latches a new SENSOR_FAULT trip; a genuine zero is still a reading. #4017 -- set_current_setpoint/set_power_setpoint returned the clamped request on every path including the IDLE and TRIPPED branches that discard it, and the service persisted the raw request regardless. An operator commanding 40 A into a latched controller got 200 {"applied_a": 40.0} and went troubleshooting the load instead of the trip; the bogus value was also pre-filled after the next restart. Both setters now return what is in effect, and persistence is gated on the command actually taking. #4035 -- both deglitch filters were constructed bare, pinning them to the 1400 C default whatever the channel was configured for. On a shorter-range channel the high-side burnout rail sat above any reachable reading, so an open thermocouple was accepted as genuine -- the exact condition the filter exists to catch. They are now built per channel with max_step_c scaled to the span, and rebuilt when the config changes so a rescale cannot strand filter state and trip TC_FAULT mid-run. Three existing tests encoded the defects and are updated to the corrected contract: the units pass-through assertion (#4003), IDLE echoing the request back (#4017), and "no tags uses zero feedback" (#4016). Also gitignores a root-level dcs_scada.db -- the suite creates the historian relative to the working directory, so a run from the repo root drops a stray artifact that is easy to commit by accident. 894 passed, 6 skipped in the backend suite. Refs #4003 #3998 #4016 #4017 #4035 * refactor(p1am-hmi): split TemperatureControl; fold recall onto seedDraftText (#4063) TemperatureControl.tsx was 1975 lines — 475 over DEFAULT_MAX_SOURCE_LINES — so fleet-fast-guardrails rejected any commit touching it. The heater screen was the one operator surface that could not be corrected without first being restructured. Following the TuningPanel shape (#4053), the container keeps all state, the rolling trend buffer and every /api/temperature/* call; the sections are prop-driven components: TemperatureControl.tsx 1975 -> 717 TemperatureTrend.tsx 680 (SVG trend + its own view state) ThermocoupleSelector.tsx 196 TemperatureConfigPanel.tsx 148 TemperatureStatusHeader.tsx 136 HeaterStartStopButton.tsx 55 lib/temperatureTrend.ts 171 (pure sample/readout math) The Start/Stop command button existed as two byte-identical copies on the same screen; on a control that energizes a heater those could come to disagree about whether a command is safe to send. One component now serves both, differing only by an appended CSS class. DRY cleanup #4053 had to defer: recallSetpointText now returns seedDraftText(lastSetpointC, operatorTouched, 1) rather than carrying its own copy of the operator-ownership rule. The heater domain types moved to src/types.ts, removing the cycle where useTelemetryStream imported TemperatureStatus from a component. No behaviour change: every moved block is verbatim apart from prop renames and JSX whitespace that collapses identically. lint (0 errors, same 2 pre-existing warnings), build and 444/444 vitest tests pass. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * chore(p1am): allowlist generated credential expression * test(p1am): keep endpoint suite within file-size budget * test: remove stale route matrix rows * style: apply automated pre-commit formatting and lint fixes * style: apply automated pre-commit formatting and lint fixes * style: apply automated pre-commit formatting and lint fixes * style: apply automated pre-commit formatting and lint fixes * style: apply automated pre-commit formatting and lint fixes * style: apply automated pre-commit formatting and lint fixes * style: apply automated pre-commit formatting and lint fixes * fix: pre-commit checks and main sync * fix: pre-commit checks and main sync * fix: pre-commit checks and main sync * fix: pre-commit checks and main sync * fix: pre-commit checks and main sync * fix: pre-commit checks and main sync * fix: pre-commit checks and main sync * fix: pre-commit checks and main sync * fix: pre-commit checks and main sync * fix: pre-commit checks and main sync * fix: pre-commit checks and main sync * fix: pre-commit checks and main sync * style/fix: pre-commit automated fixes * style/fix: pre-commit automated fixes * style/fix: pre-commit automated fixes * style/fix: pre-commit automated fixes * style/fix: pre-commit automated fixes * style/fix: pre-commit automated fixes * style/fix: pre-commit automated fixes * style/fix: pre-commit automated fixes * style/fix: pre-commit automated fixes * style/fix: pre-commit automated fixes * style/fix: pre-commit automated fixes * chore(consolidation): drop unrelated churn and the .codex-worktrees gitlinks Every one of the 13 P1AM slice branches ends with two or three "pre-commit automated fixes" / "main sync" commits that reformatted ~290 files across the whole monorepo. Those commits were produced by an older pinned ruff than the one CI and .pre-commit-config.yaml now use (ruff==0.14.10), so merging them re-introduces a formatting regression in files no P1AM change touches -- and `quality-gate` checks ruff-format on *changed* files, so each one becomes a gate failure for a change nobody made. - 293 Python files outside the P1AM real-content set are restored to origin/main exactly. 256 of them were provably pure formatting (ruff 0.14.10 renders them byte-identical to main); the remainder sit in ruff-excluded trees (movement_optimizer, pendulum_simulator, data_processing) that main has never had formatted, which is precisely why they must go back rather than be reformatted here. - The six `.codex-worktrees/*` entries are removed. They are git mode 160000 gitlinks for Codex agent scratch worktrees that got committed by accident: there is no .gitmodules declaring them, origin/main has none of them, and the commits they name exist only in one local clone and on no remote. Merged, a fresh clone of Tools would carry six gitlinks pointing at objects the server does not have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(p1am): close four defects the merged slices introduced or left open Found while integrating the batch; each is in the fail-open direction, and three of them defeat the very guarantee their own slice was written to provide. Regression tests accompany all four. 1. state.py — a fabricated reading could clear a live alarm (#4034 vs #4004). `_restore_engine_alarms` called `update_tag(tag, float(entry.get("value", 0.0)))`. `get_active_alarms()` is only contracted to agree between the Rust `tools_core.scada` engine and the pure-Python fallback on the keys the restore path reads, not on carrying `value` at all; where it is absent the substituted 0.0 resolved the tag to Normal. `apply_config` clears `active_alarms` first and runs on every routing deploy AND every reconnect-time `_publish_active_config`, so a live HiHi was erased from both the engine and the live map by a number nothing measured. Composed with the poll loop's new "only a real measurement may move the alarm state machine" rule the result was worse: the poll loop then refuses to re-evaluate alarms while the link is down, so the cleared HiHi stayed cleared for the whole outage. A record with no finite value now keeps the state it was snapshotted in and never touches the engine. 2. desktop/alarm_state.py — NaN was classified as "normal" (#4012). `classify_value` decides the band with four comparisons and every comparison against NaN is False, so a NaN fell through to `return None` = inside the normal band, and `evaluate` emitted `cleared` transitions and dropped the tag from both sets. A garbled or unscaled register therefore silenced a live High-High on the heater, and `json.loads` accepts a bare `NaN`, so it was reachable straight off the wire. `_require_number` now rejects non-finite input and `_evaluate_alarms` catches it alongside TypeError, logging and skipping the tag. Skipping is the fail-safe outcome because `evaluate` validates before it clears anything, so the latch survives. 3. desktop/header.py — the new annunciator flashed at 10 Hz into a steady light (#4012). `set_alarms_state` unconditionally set `_flash_state = True`, and `_refresh_annunciator` calls it on EVERY telemetry frame, so the flash timer's OFF phase was overwritten within ~100 ms. An unacknowledged alarm rendered effectively steady, erasing the flash-vs-steady distinction that tells an operator whether anyone has seen it. The phase now restarts only on a transition INTO unacknowledged. 4. test_route_authz_matrix.py — the authz exhaustiveness net had a hole punched in it (#4028). The slice's last commit deleted 24 rows as "stale", but main.py mounts the power-supply and temperature routers unconditionally, so 14 of those routes do exist: `test_every_route_is_classified` fails outright on the merged tree (verified before and after). Those rows are restored — they are the DC supply's and the heater's own command surface. The 9 Data Explorer rows genuinely are conditional, since that router mounts only when its numeric stack imports, so they now register from the live app instead of being deleted; that satisfies both the exhaustiveness check and the no-stale-rows check in either environment. The suite goes from 51 tests with 1 failure to 84 passing. Also repairs one broken test rather than deleting it: test_estop_shutdown_safe_state.py::test_connect_loop_wakes_immediately_on_shutdown reused the import-time module-level `shutdown_event`, which asyncio binds to a loop on first await; earlier tests in the same module had already awaited it on their own per-test loop, so it raised "bound to a different event loop" before reaching the assertion. It now patches in a fresh Event. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(p1am): rebuild SPEC.md correctly and restore LF line endings Two integration defects in the consolidation itself, both of which buried the real change under a whole-file rewrite. SPEC.md was duplicated eleven times. Every slice prepends its own dated section immediately after "## 3. Goals & Non-Goals" AND deletes the same blank line further down, so git found no usable anchor and each merge presented one side of the hunk as the entire remainder of the document. Resolving those by keeping both sides concatenated the whole spec once per slice: 26548 lines against main's 2401, with eleven copies of "## 2. Purpose", "## 3. Goals & Non-Goals" and "## 12. Change Log". SPEC.md is rebuilt as main's file with each slice's §3 subsection inserted exactly once, in the order the work was layered, plus one §12 change-log row for the consolidation. The diff against main is now purely additive: +403/-0, eleven `### 2026-07-31 P1AM ...` headings and one table row. (The three duplicate `###` headings that remain are pre-existing in main.) Line endings: nine files were carrying CRLF where main uses LF. Only `*.py` is covered by .gitattributes (`text eol=lf`), so Python normalises itself on commit; .toml, .ts, .tsx, .sh, Dockerfile and .md store whatever bytes were committed, and several slices rewrote those wholesale. This was not cosmetic: - `deploy/launch-hmi.sh` had CRLF, which on the Raspberry Pi fails at exec with `bad interpreter: /bin/bash^M` — the operator's HMI launcher would simply not start. `deploy/install-services.sh` was already LF and is unaffected. - `backend/Dockerfile` had CRLF, where a line-continued RUN can break the build. - The rewrites hid the actual edits behind a full-file diff and would conflict against every future change to the same files. pyproject.toml goes from +433/-410 to +23/-0 (the `p1am` extra), requirements-lock.txt to +4/-1, launch-hmi.sh to +16/-0, App.tsx to +72/-319, setup.ts to +63/-0. `credentials.ts` and `TuningPanel.tsx` are new on this branch and were also CRLF; they are normalised to LF to match every existing sibling and the repo's `core.eol=lf`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(p1am): extract the tuning/MPC router so main.py stays in budget `quality-gate` failed on Module Size Budget: `backend/main.py` had reached 1547 lines against its grandfathered baseline of 1440 (hard max 1200). This batch added the E-stop write-seam refusals, the acknowledged-step requirement and the shutdown sequencing to that module, and #4064 had already split out `poll_runtime` for the same reason. Bumping the baseline would have been the cheap fix and the wrong one — the baseline exists as a ratchet, and raising it to accommodate growth is the one thing it is there to prevent. Extracted instead: `tuning_router.py` takes the one cohesive group left in main.py, the handlers that write to or identify the dynamics of a single control loop — POST /api/tags/{tag_id} POST /api/pid/{pid_index}/tuning/start POST /api/pid/{pid_index}/tuning/step POST /api/pid/{pid_index}/tuning/stop POST /api/mpc/simulate plus `MPCSimulatePayload`, `TagWritePayload` and the `_latest_tag_or_http_error` helper. main.py: 1547 -> 1341 lines, inside the baseline. Follows the router-factory pattern already used in this package by `power_supply_integration.create_power_supply_router` and `temperature_integration.create_temperature_router`. Collaborators (control_context, plc_client, backup_simulator, the E-stop refusal callable, the admin dependency, the logger) are injected rather than imported from `main`, which would close an import cycle. Behaviour is unchanged; the routes, paths and admin gating are identical, which `test_route_authz_matrix.py` verifies from the live app in both directions. One trap worth recording: the request model must be imported as a real type, not injected as a parameter. This module uses `from __future__ import annotations`, so every annotation is a string at runtime and FastAPI resolves it against the module globals — an injected `step_payload_model` left the annotation as the literal `"step_payload_model"`, the body never bound, and all six tuning-step tests returned 422. `models` imports nothing from `main`, so importing `PIDTuningStepPayload` directly is safe. P1AM suite: 1388 passed, 6 skipped. ruff check + format clean. Module size budget passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(p1am): ask the router whether a route is served, and make event order deterministic Two CI-only failures on this branch. Neither was a stale table, and in both cases the easy fix would have cemented a real defect. 1. `test_table_has_no_stale_rows` reported all 19 power-supply / temperature / tuning rows as unserved. They ARE served. Proof from inside the same failing CI run, single process (`-n 0`): the 19 matching `test_gated_route_rejects_anonymous_caller` cases drove those exact paths through `TestClient(app)` -- the same module-level `app` object the stale check iterates -- and got 401/403. `_DENIED = (401, 403)`, and a route the app does not serve answers 404, which would have failed. So the routes were registered and reachable and the enumeration was under-reporting them. Deleting the rows (as an earlier slice did, which is why `test_every_route_is_classified` was failing on the merged tree) would have turned the check green while the authz matrix agreed that acknowledge_trip, permissive, setpoint, burnout_mode, tc_type and the PID tuning lifecycle did not exist -- the plant's entire control surface, in the batch whose review theme was "nothing de-energizes". `test_table_has_no_stale_rows` now asks `Route.matches()` -- the function Starlette itself calls during dispatch -- instead of string-matching `route.path` over `app.routes`. A check phrased as "would a request reach a handler" cannot disagree with a request. Verified it still discriminates: real routes and parameterised routes match, a bogus path does not, and a wrong method on a real path does not. `test_every_route_is_classified` deliberately stays enumeration-based -- it must catch a NEW route nobody classified, which is the direction that ships something ungated. Root cause of the enumeration/dispatch divergence is not established; it reproduces only under CI's environment (`--import-mode=importlib`, Linux, py3.11) and not locally across the identical 101-file selection and flags. Phrasing the assertion in dispatch terms makes the test correct regardless. 2. `test_desktop.py::test_event_logger_basic` was flaky, failing roughly one run in three locally with `assert 'alarm_trip' == 'alarm_acknowledgment'`. Not caused by this batch -- that test file is byte-identical to `main` -- but `fetch_logs` is, and it ordered by `timestamp DESC` alone. `datetime.now()` reports microseconds while the clock granularity is ~15.6 ms on Windows, so a burst of events shares one timestamp string and SQLite may return them in any order. Beyond the flake, that means an operator could see an alarm trip listed above the acknowledgement that answered it. Now `ORDER BY timestamp DESC, id DESC`, using the existing autoincrement primary key, which is monotonic in insertion order. `test_fetch_logs_orders_same_timestamp_events_newest_first` pins it by writing an explicit identical timestamp so the tie is forced rather than hoped for. Verified as a real regression test: with the tiebreaker removed it fails, returning `['first', 'second', 'third']`. Also merged `origin/main` (CONS-B1 #4438, 221 files) -- no conflicts. Confirmed `git grep -n '^from datetime import UTC' -- '*.py'` is empty, so the py3.10 revert #4438 made is intact. CI-equivalent selection (CI's own 101 files, `-m "not live_simulation" --import-mode=importlib -n 0`) run three consecutive times: 1949 passed, 6 skipped, 0 failed each time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(ci): grandfather seven p1am files that the size budget now flags `file-size-budget` failed this PR with 7 violations over the 500-LOC limit. All seven were UNDER budget on `main` and crossed it by consolidating 11 safety PRs whose fixes each fit individually: backend/tests/test_data_capture.py 495 -> 905 backend/poll_runtime.py 216 -> 805 backend/tests/test_data_explorer_service.py 485 -> 646 desktop/main_window.py 493 -> 598 calibration/calibrate.py 399 -> 551 backend/pid_tuning.py 234 -> 532 backend/tests/test_backend.py 494 -> 529 `scripts/check_file_size_budget.py` postdates the work it is judging -- the 11 consolidated PRs show no `file-size-budget` check at all -- so this is inherited cumulative size, not a monolith authored against a known budget. `main` was already parked at 485/493/494/495 on four of these, i.e. at the ceiling before any of this work. Grandfathered rather than split because the content is the "nothing de-energizes on fault" remediation for a gasifier control system, and holding safety fixes back for a line-count refactor of the polling path is the wrong trade. The baseline is the established route here: it was last extended by consolidation #3912 and already grandfathers ten p1am files, including a test module (backend/tests/test_temperature_controller.py) and the production siblings data_capture.py and data_explorer_service.py. The debt is tracked in #4503, not silently accepted: poll_runtime.py and test_data_capture.py both warrant splitting, and a one-module extraction is explicitly not enough -- the historian cluster is ~509 LOC on its own and would breach the same budget, so it needs two modules. Neither `file-size-budget` nor `phantom-guard` gates this merge: both live in their own workflows and `quality-gate` (the only required context, per org ruleset Repository_Protections) declares no `needs`. The phantom-guard failure was separately confirmed to be a timeout, not a finding -- it ran exactly 10m01s against a 10-minute cap, recorded zero steps, and uploaded no log. Verified directly instead: this branch deletes nothing relative to `main` (0 files) and `main` is a full ancestor of it. Verified locally: the budget check now reports 0 violations across 76 changed files. Refs #4503 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: codex-scheduled <codex-scheduled@users.noreply.github.com>
Collaborator
Author
|
Superseded by #4448 (the P1AM SCADA safety consolidation), which merged to Containment was verified before closing, with all three stages of Closing to reduce CI load rather than because the work was unwanted. |
auto-merge was automatically disabled
August 16, 2026 04:38
Pull request was closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4004
Closes #4008
Closes #4009
Closes #4023
Closes #4024
Refs #3999
Refs #4019
Five defects in the control/poll loop of a system driving a P1AM PLC, a DC power supply and a 110 V heater on a Raspberry Pi 5. Written TDD: each failing test landed first, was watched fail, then fixed.
#4004 (P0) — simulator data was fed to the control laws, alarms and historian
Defect. A single failed Modbus read set
_connected = False, after which_poll_oncesubstitutedSimulatedPLCClient.read_tags()and passed the invented numbers topower_supply.poll(),temperature.poll(), the alarm engine andlog_scan()— with nothing in the frame or the database marking them synthetic.backup_simulatorwas wired in unconditionally, even with a real driver.Hardware/operator consequence. An active HiHi alarm silently reverted to Normal whenever the link flapped, because the fabricated values were fed to the alarm state machine while the relay and AO stayed latched. The heater and power-supply control laws acted on invented measurements. The historian recorded fabricated trend data indistinguishable from real samples, so a post-incident review would show a smooth trace across an outage.
Fix. Every scan is classified by
models.DataSource—live,simulated,held,fault. Only a measurement (live, orsimulatedwhen the operator deliberately selected a simulator driver) reaches the control laws, the alarm engine and the historian. Held values are still broadcast so the HMI does not flicker, but the frame says plainly that they are held and the control path is handedNone. On a data fault the low-level write-seam interlocks are armed, so a stale reading cannot become an energizing command — the de-energizing direction is never blocked, and the one-way controller latch is NOT engaged, because a data gap is not an E-stop.TagLoggains aqualitycolumn with an idempotent migration, and aDataQualityTrackerwrites oneDATA_QUALITYEventLog per transition rather than ten per second. The backup simulator is wired into the scan path only whensettings.plc_driveris a simulator driver, and_read_scan_tagsre-checks that flag as defence in depth.Covering tests (
tests/test_poll_data_quality.py):test_real_driver_disconnect_is_a_fault_not_simulated_dataasserts the simulator is never read, the alarm engine is never called, and a pre-existing HiHi survives;test_real_driver_fault_arms_the_write_seam_interlocks;test_real_driver_read_hiccup_holds_for_display_only;test_simulator_driver_still_drives_the_bench_and_marks_data. Plustests/test_historian.py::TestSampleQualityandtests/test_database.py::test_quality_column_migration_backfills_a_legacy_historian.#4008 (P0) — the control period was set by a browser tab
Defect.
perf_controllerwas constructed inLIGHTWEIGHTmode (2.0 s) and the loop read itspoll_interval_sas the scan period each iteration.App.tsxsets that mode from adocument.hiddenhandler and initialises it to"lightweight".Hardware/operator consequence. Closing or hiding the browser dropped the PLC scan, the alarm evaluation, the heater-relay decision and the E-stop re-assert from 10 Hz to 0.5 Hz. The unattended plant got the slowest scan.
Fix. The two cadences are now separate objects.
ScanSchedulerowns the control period, pinned tosettings.poll_interval_s.PerformanceControllerno longer exposespoll_interval_sat all, so nothing can read a control period off a UI-driven mode; it exposesbroadcast_interval_sandbroadcast_every_n, and the loop passesbroadcast=to_poll_once.latest_frameis still cached every scan, so/api/snapshotkeeps full resolution./api/performancereports both cadences.Covering tests (
tests/test_poll_loop_cadence.py,tests/test_performance.py):test_lightweight_mode_does_not_slow_the_scandrives the loop on a virtual clock in lightweight mode and asserts every cycle is onepoll_interval_sand that only some scans broadcast;test_controller_no_longer_exposes_a_scan_period;test_mode_only_decimates_the_broadcast.#4009 (P0) — fixed sleep instead of a monotonic deadline; no overrun detection
Defect. The loop did all the work then slept a fixed interval, so the real period was
t_work + intervaland drifted with load. Nothing measured, bounded, logged or alarmed on it.AsyncModbusTcpClientwas built with notimeout=, so the pymodbus 3 s default applied. The failure backoff was computed fromsettings.poll_interval_s(0.1 s) while the healthy delay came fromperf_controller(2.0 s).Hardware/operator consequence. One dropped frame stretched a control period to 3.1 s silently — 31 scans worth of missed alarm evaluation and E-stop re-assert on a live heater. And the FIRST failed scan made the loop poll 20x faster than healthy, hammering a PLC that was already in trouble.
Fix.
ScanScheduleradvances atime.monotonic()deadline by exactly one period per cycle and sleeps only the remainder; an already-past deadline is counted, logged with how late it was, and the phase resynchronised rather than producing a burst of catch-up scans.settings.resolved_modbus_timeout_ssizes the Modbus timeout to the scan period (floored at 0.25 s, overridable viaP1AM_MODBUS_TIMEOUT_S). The backoff is computed from the active control period. Overrun counters are surfaced in the telemetry frame and on/api/performance.Covering tests (
tests/test_scan_scheduler.py):test_period_does_not_drift_across_cycles(varying work times, exact periods);test_overrun_is_counted_measured_and_resynchronised;test_resync_rebases_the_deadline_after_a_failure_backoff. Plustests/test_settings.py::test_modbus_timeout_is_sized_to_the_scan_periodandtests/test_poll_loop_cadence.py::test_overruns_are_surfaced_on_the_performance_endpoint. The existingtest_poll_plc_loop_backs_off_and_surfaces_degraded_statusstill pins the backoff ladder.#4023 (P1) — historian write blocked the event loop and dropped alarm records silently
Defect.
log_scan, theEventLoginserts andcommit()ran as synchronous blocking SQLite on the event loop, opening and tearing down a session every scan even when the capture throttle suppressed the write. On failure it rolled back and logged at error — no alarm, no retry, no counter.Hardware/operator consequence. Alarm transitions occurring during a
/api/capture/clearVACUUM lock were permanently lost, with nothing telling the operator. Every scan paid for a session it usually did not need.Fix.
HistorianWriterdrains a boundedasyncio.Queuein a dedicated task viaasyncio.to_thread, folding several scans into one transaction.ThrottledHistorianSinkapplies the capture throttle BEFORE the queue, so a suppressed scan never opens a session.OperationalErroris retried; on final failure the counter increments and each unpersisted event is written to the application log so an incident review can still recover it. Under backpressure the oldest tag samples are dropped (they are resamplable) but their alarm and data-quality events ride forward onto the newcomer.historian_write_failuresis exposed on/api/performanceand in the frame.Covering tests (
tests/test_historian_writer.py):test_batches_several_scans_into_one_transaction;test_no_session_is_opened_when_nothing_is_queued;test_full_queue_drops_oldest_samples_but_carries_alarms_forward;test_operational_error_is_retried_so_alarms_survive_a_vacuum_lock;test_persistent_failure_is_counted_not_silently_swallowed;TestThrottledSink.#4024 (P1) — one slow WebSocket client stalled the control loop
Defect.
broadcastwas awaited from_poll_onceand sent to each client sequentially with no timeout, no per-client queue and no backpressure, re-serialising the ~2 KB frame once per client.Hardware/operator consequence. One HMI on a congested link, or a half-open TCP socket, stalled the PLC scan — including the E-stop re-assert — for as long as its send took.
Fix.
ConnectionManager(moved tows_broadcast.py) serialises the frame once withjson.dumps, thenput_nowaits it into each client's bounded queue and returns; it awaits nothing socket-related. Each client is drained by its own task with a send timeout. When a client's queue is full the OLDEST frame is dropped, because for live telemetry only the newest value matters. Dropped-frame counts are surfaced in the diagnostics block.Covering tests (
tests/test_ws_backpressure.py):test_broadcast_never_awaits_a_slow_client(five broadcasts to a client whose send sleeps 5 s must complete in under 0.5 s);test_full_client_queue_drops_the_oldest_frame;test_frame_is_serialised_once_for_all_clients;test_dead_client_is_pruned_without_touching_the_loop.Wired for the concurrent PRs
plc.write_heartbeat()through agetattrguard, so this lands cleanly whether or not the client-side seam has merged.test_missing_heartbeat_seam_does_not_break_the_scancovers the not-yet-merged case;test_live_scan_marks_data_live_and_beats_the_watchdogcovers the merged one.plc_connectedin the frame (fix(p1am-hmi): correct alarm annunciation, thresholds, E-stop gate and event-log churn #4057). Frames now carryplc_connected,simulatedanddata_source, so the desktop HMI can stop saying "Simulating" on live hardware.Notes on scope
historian.py(one keyword-onlyqualityparameter),database.py(the additivetaglog.qualitymigration), andmodbus_client.py(an optionaltimeout_sconstructor argument). All three are additive and localised away from thewrite_heartbeatseam.main.pyexceeded the 1500-line guardrail, so the WebSocket manager moved tows_broadcast.pyand the loop-failure/diagnostics helpers moved topoll_runtime.py./api/performancekeepspoll_interval_sin its response (now documented as the broadcast period) so the existing frontend zod schema still validates.Verification
935 passed, 6 skippedinsrc/p1am_control_system/backend/tests/(up from 875 onorigin/main).ruff checkandruff format --checkclean;mypy --follow-imports=skipclean on all changed modules. The full pre-push hook chain passed, includingpytest tests/unit.🤖 Generated with Claude Code