fix(drivers): preempt blocked commands for autonomous defaults - #798
Conversation
|
PAUSED / NOT READY TO MERGE. Remote head är 328e087. PR:s registrerade base är 369d120; aktuell live master är 081ea9e, så PR:n är bakom och ska rebasas före nästa push. Lokalt passerade make verify-all med Python 3.12.13 och 37 drivers, full internal/drivers -race, full internal/api -race, samt nya stale-queue-testet 50x med -race. Oberoende kvarvarande P1: defaultCh kan inte leverera default medan samma runLoop sitter fast i en legacy Command som ignorerar cancellation och aldrig returnerar; köprioritet räcker inte. P2: 2/50 statuskontroller läste före registry-statusuppdateringen; testerna måste synka på registry-status. Read-only scratch-merge mot Claude #608 head 8f19c7a var konfliktfri, utan direkt filöverlapp, och relevanta manual-hold/API/control- samt webtester passerade. Worktree har inga lokala ocommittade ändringar. Nästa steg är faktisk Lua-preemption eller säker generation-quarantine/restart, en oändlig legacy-driver-regression och status-testsynkning, sedan rebase mot 081ea9e och ny race/verify/#608-kontroll. #608 rörs inte. |
… command (#805) #800 established the law — a driver that cannot actuate gets its autonomous default and stops being counted on — and wired exactly one path into it. The reviewer named the gap and its author confirmed it: only the storage loop filed an outcome. PV curtail discarded its error outright and the loadpoint controller only logged one. So the bug #800 fixed for batteries was still live for the other two. A wallbox that answers every poll and refuses every setpoint held the current it last accepted while the plan went on booking the charge and the surplus reserve went on being held back from the battery for an EV that was not drawing. An inverter that refused every cap kept exporting into a negative price while the plan booked the saving. Route both into the same tracker. The wiring is small; what took the work was deciding which command on each path is the one whose refusal means core cannot put power where it asked. Getting that wrong excludes a healthy device, which is worse than the bug. PV curtail: the cap counts, the release does not. A `curtail_disable` is core letting go of the inverter, so refusing it proves nothing about the device — the same reading #800 gives a rejected set_self_consumption in sungrow.lua. It would also seal shut: ComputePVCurtail emits a release the moment a driver drops offline, so a counted refusal would let an excluded inverter hold itself out on its own exclusion. Loadpoint: the periodic ev_set_current counts. Four other sends do not, and each for its own reason. The 0 W standdown is core withdrawing under a stale site meter, a transition the staleness tracker already owns. `charge_start` goes to the bound vehicle driver, and a parked car refuses it whenever it is asleep — counting that would take the car's SoC out of the plan for napping, and wakeVehicleAuto already backs off on its own. The ev_pause/ev_resume contactor cycle is documented as free for any charger implementing those actions, so a charger that implements neither returns an error and is behaving correctly; it also runs on its own goroutine, which the tracker is not built to take. The operator's force-start and refresh are not dispatch at all. Nothing about what counts as a refusal changes: ErrObserveOnly, ErrControlBlocked and deadline/cancel remain faults another mechanism owns. Observe-only drivers still receive no command of any kind. Deliberately still out of scope, as in #800: a refusal joined with ErrControlBlocked is not counted, so such a driver stays in the MPC fleet. #798 owns that registry state machine. Risk direction, stated the way #800 did it: commands now reach hardware on paths that previously sent none. An excluded PV inverter receives its driver's declared default where it previously received nothing. Both shipped EV chargers declare a no-op default — easee_cloud.lua and ctek.lua both say a wallbox has no autonomous equivalent and should hold its last current — so an excluded charger receives no new write at all, and the exclusion is pure accounting. Golden corpus: no record moved, as predicted. This changes command outcome accounting, not dispatch arithmetic, and the replay never goes through the control tick. Co-authored-by: Claude Opus 5 <[email protected]>
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Fixes a P1 concurrency safety bug: autonomous "default"/watchdog commands now flow through a dedicated one-slot channel instead of sharing the normal command queue, so a stale/blocked queue can no longer starve the safety fallback path. The new TestRegistryDefaultBypassesStaleCommandQueue directly exercises the race (blocked active command + full stale queue + a default whose own caller times out) and asserts the default is accepted durably and retried to success, while confirming no stale command crosses the driver boundary.
- Nit (info): the enqueue-time guard relies on a generation-blocked check (
controlIsBlockedper the comment) outside this diff to discard a normal command that races the transition — not shown here, presumably pre-existing. Worth a quick gut-check that it behaves as claimed, though the new test empirically covers the scenario end to end. - No secrets, no new deps, no new network destinations.
Safe to merge from my read — you flagged this needs a human look at the safety path regardless, which I'd honor given the P1 history on #791.
|
Stable-release coordination: draft PR #922 now overlaps #922 does not claim to solve #798's remaining hard case where a legacy runtime ignores cancellation forever. It fixes separate release gaps: command-block exclusion, per-action refusal tracking, loadpoint health gating, and the EV pause/resume boundary across Core default, exclusion, removal, timeout, and shutdown. It keeps calls under the registry's one per-driver owner and leaves the driver JSON/API unchanged. Race tests and full #798 is already marked PAUSED / NOT READY and remains behind. Please keep its current head out of the release candidate. If its runtime-preemption work resumes, rebase and redesign it on top of the command owner that lands from #922 rather than merging the two registry models independently. |
328e087 to
a9dfe4f
Compare
frahlg
changed the title
|
@codex review |
![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: a9dfe4fa0c
ℹ️ 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".
a9dfe4f to
a2f7941
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2f7941ff4
ℹ️ 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".
| select { | ||
| case cmd := <-rd.defaultCh: | ||
| handleDefault(cmd) |
There was a problem hiding this comment.
When a long-lived normal command is already waiting in cmdCh, this new priority path processes the later SendDefault first; handleDefault then clears controlBlocked, so the older queued command subsequently passes the blocked check and returns the device to controlled mode. This occurs when a command is queued behind blocked active work before the watchdog/default request, and reverses the previous FIFO ordering in which the default remained the final operation; discard or supersede commands predating the accepted default before confirming recovery.
AGENTS.md reference: AGENTS.md:L35-L36
Useful? React with 👍 / 👎.
2 participants
Important
Fresh review on
a9dfe4fafound that canceling a mutating HTTP request could let it finish after default.a2f7941fkeeps POST/PATCH in the registry actor until the host transport returns. This remains draft until fresh review and CI confirm the fix.What changed
host.sleepWhy
The old queue-only fix could not run a default while a legacy Lua command ignored cancellation. This patch makes the Lua host calls used by drivers honor the active command context. The same registry actor then runs the default, so Core does not call one driver in parallel.
Scope and order
This is one commit on master
4b7ad49e, after #922. It changes seven files ingo/internal/driversplus one changeset.#927 and #928 only show
registry.gobecause their draft stacks still include #922. Their own patches do not change the runtime owner. #728 has reallua.goandregistry.gooverlap and must rebase after this safety fix; its resolver transport remains separate.Proof
host.sleepreturned after 250.8 ms and failed the old-red testa9dfe4fa: default returned success while an older mutating PATCH still ran at the device-race: 20 runsgo test -race ./internal/driversgo test -race ./cmd/ftwmake verifygit range-diffshows the already reviewed local patch unchanged on live masterDo not merge until fresh review and CI pass on
a2f7941f.