From fad475273065ad89a6f4c295bc7c9a01d44a495c Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Fri, 31 Jul 2026 13:49:12 +0200 Subject: [PATCH 01/10] feat(api): send a declared driver command, and hold it for a bounded time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/drivers/{name}/control sends one command the driver declared and holds it; DELETE ends the hold early; /api/drivers/{name} shows what is set and until when. Deliberately outside control v2. Synthesising a RuntimePolicy for an unsigned driver is worse than doing nothing: HostEnv.permissionAllowed grants everything only while the policy is nil, so a policy without permissions silently blocks the driver's own MQTT, and LuaDriver.Command refuses a control v2 driver on the legacy path — v2 wants driver_command_v2 entrypoints no community driver has. Signed packages keep CommandV2 unchanged. What that costs: no host-enforced write scope, no host-verified evidence. What it keeps is the part that protects hardware. Core clamps to the declared bounds rather than trusting the Lua, and the declaration is the whole allowlist — an undeclared control is a 400, not a 200 for a command the Lua ignored, which is what the registry cannot otherwise tell apart. Every hold ends by itself, into the driver's own driver_default_mode rather than a value Core invented: only the driver knows what neutral is. Default 4 h, maximum 24 h, nothing survives a restart. Because the policy is nil the registry's lease machinery never arms, so the 300 s Lease.MaxDuration ceiling does not apply and the hold does not need to fight it. Tests drive a real registry and a real Lua driver, so they distinguish "Core sent it" from "Core said it sent it", and cover the case that bites: replacing a hold must stop the old timer from defaulting the device out from under the new setting. Co-Authored-By: Claude Opus 5 --- .changeset/driver-control-path.md | 31 +++ go/internal/api/api.go | 10 +- go/internal/api/api_driver_control.go | 239 ++++++++++++++++++++ go/internal/api/api_driver_control_test.go | 242 +++++++++++++++++++++ go/internal/api/api_drivers_debug.go | 6 +- 5 files changed, 525 insertions(+), 3 deletions(-) create mode 100644 .changeset/driver-control-path.md create mode 100644 go/internal/api/api_driver_control.go create mode 100644 go/internal/api/api_driver_control_test.go diff --git a/.changeset/driver-control-path.md b/.changeset/driver-control-path.md new file mode 100644 index 00000000..627b64bb --- /dev/null +++ b/.changeset/driver-control-path.md @@ -0,0 +1,31 @@ +--- +"ftw": minor +--- + +An operator can now send a driver's declared command and hold it for a bounded +time. `POST /api/drivers/{name}/control` takes `{control, value, duration_s}`; +`DELETE` on the same path ends the hold early. The active hold appears on +`/api/drivers/{name}` so a UI can show what is set and until when. + +Deliberately outside control v2. A signed package binds a RuntimePolicy and +goes through `CommandV2` with its write scope, lease and evidence, unchanged. +A bundled or local driver has no policy, and synthesising one would be worse +than doing nothing: `HostEnv.permissionAllowed` grants everything only while +the policy is nil, so a policy without permissions silently blocks the driver's +own MQTT, and `LuaDriver.Command` refuses a control v2 driver on the legacy +path — v2 wants `driver_command_v2` entrypoints no community driver has. This +path leaves the policy layer untouched and validates against the catalog +declaration instead. + +What that costs, stated plainly: no host-enforced write scope, no host-verified +evidence. What it keeps is the part that protects hardware. Core clamps every +value to the declared bounds rather than trusting the Lua to do it — a driver +that forgets to clamp is exactly the driver this protects — and the driver's +own declaration is the whole allowlist, so an undeclared control is a 400 +rather than a 200 for a command the Lua silently ignored. + +Every hold ends by itself, and ending means calling the driver's own +`driver_default_mode` rather than writing a value Core invented: only the +driver knows what neutral is. Default 4 h, maximum 24 h, and nothing survives a +restart. An offset left behind by a browser tab that closed is a house heated +wrong for weeks. diff --git a/go/internal/api/api.go b/go/internal/api/api.go index 3e13313c..3b7cf3d4 100644 --- a/go/internal/api/api.go +++ b/go/internal/api/api.go @@ -206,6 +206,12 @@ type Server struct { savingsCacheMu sync.Mutex savingsCache map[string]daySavings + // controlHolds is the one operator setting in force per driver, with the + // timer that ends it. Process-lifetime only, deliberately: a restart + // should leave no device held by a setting nobody remembers making. + controlHoldMu sync.Mutex + controlHolds map[string]*controlHold + versionUpdateMu sync.Mutex driverUpdateMu sync.Mutex backupMu sync.Mutex @@ -277,7 +283,9 @@ func (s *Server) routes() { s.handle("GET /api/drivers/{name}/logs", s.handleDriverLogs) s.handle("GET /api/logs", s.handleGlobalLogs) s.handle("GET /api/support/dump", s.handleSupportDump) - s.handle("GET /api/support/report", s.handleSupportReport) + s.handle("GET /api/support/report", s.handleSupportReport) + s.handle("POST /api/drivers/{name}/control", s.handleDriverControl) + s.handle("DELETE /api/drivers/{name}/control", s.handleDriverControlRelease) s.handle("POST /api/drivers/{name}/restart", s.handleDriverRestart) s.handle("POST /api/drivers/{name}/disable", s.handleDriverDisable) s.handle("POST /api/drivers/{name}/enable", s.handleDriverEnable) diff --git a/go/internal/api/api_driver_control.go b/go/internal/api/api_driver_control.go new file mode 100644 index 00000000..190080ce --- /dev/null +++ b/go/internal/api/api_driver_control.go @@ -0,0 +1,239 @@ +// Operator-facing driver controls: send one declared command, hold it for a +// bounded time, and hand the device back to itself when the hold ends. +// +// Deliberately outside control v2. A signed package binds a RuntimePolicy and +// goes through CommandV2 with its write scope, lease and evidence. A bundled +// or local driver has no policy, and giving it a synthesised one would be +// worse than doing nothing: HostEnv.permissionAllowed grants everything only +// while the policy is nil, so a policy without permissions silently blocks +// the driver's own MQTT, and LuaDriver.Command refuses a control v2 driver on +// the legacy path — v2 needs driver_command_v2 entrypoints that no community +// driver has. So this path leaves the policy layer untouched and validates +// against the driver's catalog declaration instead. +// +// What that costs is honest and worth stating: no host-enforced write scope +// and no host-verified evidence. What it keeps is the part that protects +// hardware — Core clamps every value to the declared bounds rather than +// trusting the Lua to do it, and every hold ends by itself. +package api + +import ( + "context" + "encoding/json" + "log/slog" + "net/http" + "time" + + "github.com/srcfl/ftw/go/internal/drivers" +) + +// A hold has to end on its own. An offset left behind by a browser tab that +// closed, or by an FTW that stopped answering, is a house heated wrong for +// weeks — the failure nobody notices until the bill. 24 h is long enough for +// "warm through the cold snap" and short enough that forgetting is cheap. +const ( + maxControlHoldSeconds = 24 * 60 * 60 + defaultControlHoldSeconds = 4 * 60 * 60 +) + +// controlHold is one active operator setting. The timer is what releases it; +// the fields are what the UI shows meanwhile. +type controlHold struct { + Control string `json:"control"` + Value *float64 `json:"value,omitempty"` + ExpiresAt int64 `json:"expires_at_ms"` + + timer *time.Timer +} + +type controlRequest struct { + Control string `json:"control"` + Value *float64 `json:"value"` + DurationS int `json:"duration_s"` +} + +// POST /api/drivers/{name}/control — send one declared command and hold it. +// +// The command must be declared by the driver's catalog entry. That is the +// whole allowlist: a driver that declares nothing can be commanded by nobody, +// and a typo'd control name is a 400 rather than a silent success. The Lua +// command hook returns no value for an action it does not know, which the +// registry cannot tell apart from a command that worked. +func (s *Server) handleDriverControl(w http.ResponseWriter, r *http.Request) { + name := r.PathValue("name") + if name == "" { + writeJSON(w, 400, map[string]string{"error": "missing driver name"}) + return + } + var req controlRequest + if err := readJSON(r, &req); err != nil { + writeJSON(w, 400, map[string]string{"error": "invalid request"}) + return + } + + declared := s.driverControls(name) + if len(declared) == 0 { + writeJSON(w, 404, map[string]string{"error": "driver declares no controls"}) + return + } + var control *drivers.CatalogControl + for i := range declared { + if declared[i].ID == req.Control { + control = &declared[i] + break + } + } + if control == nil { + writeJSON(w, 400, map[string]string{"error": "unknown control"}) + return + } + if s.deps.Registry == nil { + writeJSON(w, 503, map[string]string{"error": "driver registry not available"}) + return + } + + payload := map[string]any{"action": control.ID} + var applied *float64 + if control.Input.Type == "number" { + if req.Value == nil { + writeJSON(w, 400, map[string]string{"error": "control requires a value"}) + return + } + // Clamp here rather than reject: the bounds came from the driver, and + // a UI that rounds differently should not fail an operator's click. + // Clamping in Core is the point — a driver that forgets to clamp is + // exactly the driver this protects. + value := clampToDeclared(*req.Value, control.Input) + applied = &value + payload["value"] = value + // Drivers written before this endpoint read their own key names. + // Sending both costs one JSON field and saves every such driver a + // rewrite; heishamon reads cmd.offset or cmd.value. + payload["offset"] = value + } + + body, err := json.Marshal(payload) + if err != nil { + writeJSON(w, 500, map[string]string{"error": err.Error()}) + return + } + if err := s.deps.Registry.Send(r.Context(), name, body); err != nil { + writeJSON(w, 500, map[string]string{"error": err.Error()}) + return + } + + seconds := req.DurationS + if seconds <= 0 { + seconds = defaultControlHoldSeconds + } + if seconds > maxControlHoldSeconds { + seconds = maxControlHoldSeconds + } + hold := s.armControlHold(name, control.ID, applied, time.Duration(seconds)*time.Second) + + writeJSON(w, 200, map[string]any{ + "control": control.ID, + "applied": applied, + "evidence": control.Evidence, + "expires_at_ms": hold.ExpiresAt, + }) +} + +// DELETE /api/drivers/{name}/control — end the hold now. +// +// Releasing means calling the driver's own default mode, not writing a value +// this package invented. Only the driver knows what neutral is: heishamon's +// is its configured safe_offset, which an operator may have moved. +func (s *Server) handleDriverControlRelease(w http.ResponseWriter, r *http.Request) { + name := r.PathValue("name") + if name == "" { + writeJSON(w, 400, map[string]string{"error": "missing driver name"}) + return + } + if s.deps.Registry == nil { + writeJSON(w, 503, map[string]string{"error": "driver registry not available"}) + return + } + s.clearControlHold(name) + if err := s.deps.Registry.SendDefault(r.Context(), name); err != nil { + writeJSON(w, 500, map[string]string{"error": err.Error()}) + return + } + writeJSON(w, 200, map[string]string{"status": "released"}) +} + +func clampToDeclared(value float64, in drivers.CatalogControlInput) float64 { + if in.Min != nil && value < *in.Min { + value = *in.Min + } + if in.Max != nil && value > *in.Max { + value = *in.Max + } + return value +} + +// armControlHold replaces any existing hold for the driver. One driver holds +// one control at a time: two overlapping holds on the same device would each +// expire into a default that undoes the other. +func (s *Server) armControlHold(name, control string, value *float64, d time.Duration) *controlHold { + s.controlHoldMu.Lock() + defer s.controlHoldMu.Unlock() + if s.controlHolds == nil { + s.controlHolds = make(map[string]*controlHold) + } + if existing, ok := s.controlHolds[name]; ok && existing.timer != nil { + existing.timer.Stop() + } + hold := &controlHold{ + Control: control, + Value: value, + ExpiresAt: time.Now().Add(d).UnixMilli(), + } + hold.timer = time.AfterFunc(d, func() { s.expireControlHold(name, hold) }) + s.controlHolds[name] = hold + return hold +} + +// expireControlHold hands the device back to itself. It checks identity +// first: a hold that was replaced or released already had its timer stopped, +// but a timer that had begun firing cannot be stopped, and defaulting a +// driver that an operator has just set again is the one wrong answer here. +func (s *Server) expireControlHold(name string, fired *controlHold) { + s.controlHoldMu.Lock() + current, ok := s.controlHolds[name] + if !ok || current != fired { + s.controlHoldMu.Unlock() + return + } + delete(s.controlHolds, name) + s.controlHoldMu.Unlock() + + if s.deps.Registry == nil { + return + } + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if err := s.deps.Registry.SendDefault(ctx, name); err != nil { + // Nothing to retry into: the driver is gone, wedged, or already in + // its default. Log rather than reschedule, so a dead driver does not + // leave a timer firing every ten seconds for the process lifetime. + slog.Warn("control hold expiry failed", "driver", name, "err", err) + } +} + +func (s *Server) clearControlHold(name string) { + s.controlHoldMu.Lock() + defer s.controlHoldMu.Unlock() + if hold, ok := s.controlHolds[name]; ok { + if hold.timer != nil { + hold.timer.Stop() + } + delete(s.controlHolds, name) + } +} + +func (s *Server) activeControlHold(name string) *controlHold { + s.controlHoldMu.Lock() + defer s.controlHoldMu.Unlock() + return s.controlHolds[name] +} diff --git a/go/internal/api/api_driver_control_test.go b/go/internal/api/api_driver_control_test.go new file mode 100644 index 00000000..8e4a644c --- /dev/null +++ b/go/internal/api/api_driver_control_test.go @@ -0,0 +1,242 @@ +package api + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/srcfl/ftw/go/internal/config" + "github.com/srcfl/ftw/go/internal/drivers" + "github.com/srcfl/ftw/go/internal/telemetry" +) + +// A driver that declares one control, records what it was commanded and +// counts its own default-mode calls, so a test can tell "Core sent it" from +// "Core said it sent it". +const controlProbeLua = `DRIVER = { + id = "probe", + name = "Probe", + version = "1.0.0", + controls = { + { + id = "set_offset", + label = "Offset", + evidence = "readback", + input = { type = "number", min = -3, max = 3, step = 1, unit = "C" }, + }, + }, +} + +local applied = nil +local defaulted = 0 + +function driver_init(config) + host.set_make("Probe") + -- The default first poll is 5 s away, which would make every assertion + -- here a five-second wait for a value that was already set. + host.set_poll_interval(100) +end + +function driver_poll() + if applied ~= nil then host.emit_metric("applied", applied, "C") end + host.emit_metric("defaulted", defaulted, "n") + return 100 +end + +function driver_command(action, power_w, cmd) + if action == "set_offset" then + applied = tonumber(cmd and (cmd.offset or cmd.value)) + return true + end + return false +end + +function driver_default_mode() + defaulted = defaulted + 1 + applied = 0 +end +` + +func controlServer(t *testing.T) (*Server, *telemetry.Store) { + t.Helper() + dir := t.TempDir() + lua := filepath.Join(dir, "probe.lua") + if err := os.WriteFile(lua, []byte(controlProbeLua), 0o600); err != nil { + t.Fatal(err) + } + tel := telemetry.NewStore() + reg := drivers.NewRegistry(tel) + cfg := config.Driver{Name: "heat", Lua: lua} + if err := reg.Add(context.Background(), cfg); err != nil { + t.Fatalf("add driver: %v", err) + } + t.Cleanup(reg.ShutdownAll) + srv := New(&Deps{ + Tel: tel, + Registry: reg, + Cfg: &config.Config{Drivers: []config.Driver{cfg}}, + CfgMu: &sync.RWMutex{}, + DriverDir: dir, + ConfigPath: filepath.Join(dir, "config.yaml"), + }) + return srv, tel +} + +func post(t *testing.T, srv *Server, path, body string) *httptest.ResponseRecorder { + t.Helper() + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, path, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + srv.Handler().ServeHTTP(rec, req) + return rec +} + +// waitMetric polls for a metric to reach want, so the test follows the +// driver's own poll loop rather than a sleep chosen by guess. +func waitMetric(t *testing.T, tel *telemetry.Store, driver, metric string, want float64) { + t.Helper() + deadline := time.Now().Add(3 * time.Second) + var last float64 + for time.Now().Before(deadline) { + if got, _, ok := tel.LatestMetric(driver, metric); ok { + last = got + if got == want { + return + } + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("%s/%s = %v, want %v", driver, metric, last, want) +} + +// The value reaches the driver, and Core clamps it to the declared bound +// rather than trusting the Lua to do it. +func TestDriverControlClampsAndReachesDriver(t *testing.T) { + srv, tel := controlServer(t) + + rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":99,"duration_s":600}`) + if rec.Code != http.StatusOK { + t.Fatalf("POST = %d, body %s", rec.Code, rec.Body.String()) + } + var resp struct { + Applied *float64 `json:"applied"` + Evidence string `json:"evidence"` + Expires int64 `json:"expires_at_ms"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatal(err) + } + if resp.Applied == nil || *resp.Applied != 3 { + t.Errorf("applied = %v, want 3 (clamped from 99)", resp.Applied) + } + if resp.Evidence != "readback" { + t.Errorf("evidence = %q", resp.Evidence) + } + if resp.Expires <= time.Now().UnixMilli() { + t.Errorf("expires_at_ms = %d, want in the future", resp.Expires) + } + waitMetric(t, tel, "heat", "applied", 3) +} + +// The declaration is the allowlist. A control the driver never declared is a +// 400, not a 200 for a command the Lua silently ignored. +func TestDriverControlRejectsUndeclared(t *testing.T) { + srv, _ := controlServer(t) + + rec := post(t, srv, "/api/drivers/heat/control", `{"control":"set_fan","value":1}`) + if rec.Code != http.StatusBadRequest { + t.Errorf("unknown control = %d, want 400 (body %s)", rec.Code, rec.Body.String()) + } + rec = post(t, srv, "/api/drivers/heat/control", `{"control":"set_offset"}`) + if rec.Code != http.StatusBadRequest { + t.Errorf("missing value = %d, want 400", rec.Code) + } + rec = post(t, srv, "/api/drivers/nosuch/control", `{"control":"set_offset","value":1}`) + if rec.Code != http.StatusNotFound { + t.Errorf("unknown driver = %d, want 404", rec.Code) + } +} + +func TestDriverControlHoldIsVisibleAndReleasable(t *testing.T) { + srv, tel := controlServer(t) + + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":2,"duration_s":600}`); rec.Code != http.StatusOK { + t.Fatalf("POST = %d", rec.Code) + } + + rec := httptest.NewRecorder() + srv.Handler().ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/api/drivers/heat", nil)) + var detail driverDetailResp + if err := json.Unmarshal(rec.Body.Bytes(), &detail); err != nil { + t.Fatal(err) + } + if detail.Hold == nil || detail.Hold.Control != "set_offset" { + t.Fatalf("hold = %+v, want set_offset", detail.Hold) + } + if detail.Hold.Value == nil || *detail.Hold.Value != 2 { + t.Errorf("hold value = %v, want 2", detail.Hold.Value) + } + + rec = httptest.NewRecorder() + srv.Handler().ServeHTTP(rec, + httptest.NewRequest(http.MethodDelete, "/api/drivers/heat/control", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("DELETE = %d, body %s", rec.Code, rec.Body.String()) + } + // Releasing calls the driver's own default mode, not a value this + // package invented. + waitMetric(t, tel, "heat", "defaulted", 1) + if hold := srv.activeControlHold("heat"); hold != nil { + t.Errorf("hold survived release: %+v", hold) + } +} + +// The whole reason a hold is bounded: it has to end by itself. An offset that +// outlives the browser tab that set it heats a house wrong for weeks. +func TestDriverControlHoldExpiresIntoDefault(t *testing.T) { + srv, tel := controlServer(t) + + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":2,"duration_s":1}`); rec.Code != http.StatusOK { + t.Fatalf("POST = %d", rec.Code) + } + waitMetric(t, tel, "heat", "applied", 2) + waitMetric(t, tel, "heat", "defaulted", 1) + if hold := srv.activeControlHold("heat"); hold != nil { + t.Errorf("hold survived expiry: %+v", hold) + } +} + +// Replacing a hold must not leave the old timer able to default the device +// out from under the new setting. +func TestDriverControlReplacingHoldCancelsTheOldTimer(t *testing.T) { + srv, tel := controlServer(t) + + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":1,"duration_s":1}`); rec.Code != http.StatusOK { + t.Fatalf("first POST = %d", rec.Code) + } + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":-2,"duration_s":600}`); rec.Code != http.StatusOK { + t.Fatalf("second POST = %d", rec.Code) + } + waitMetric(t, tel, "heat", "applied", -2) + + // Past when the first hold would have fired. + time.Sleep(1500 * time.Millisecond) + if got, _, ok := tel.LatestMetric("heat", "defaulted"); ok && got != 0 { + t.Errorf("defaulted = %v, want 0 — the replaced timer still fired", got) + } + if got, _, ok := tel.LatestMetric("heat", "applied"); !ok || got != -2 { + t.Errorf("applied = %v, want -2 to survive", got) + } +} diff --git a/go/internal/api/api_drivers_debug.go b/go/internal/api/api_drivers_debug.go index 8c0eb2b2..ef559d71 100644 --- a/go/internal/api/api_drivers_debug.go +++ b/go/internal/api/api_drivers_debug.go @@ -35,9 +35,10 @@ type driverDetailResp struct { Metrics []telemetry.MetricSnapshot `json:"metrics"` Identity driverIdentityDTO `json:"identity"` // Controls are what this driver says an operator may command. Absent - // for every driver that only reports. Nothing sends them yet — this is - // the description, not the path. + // for every driver that only reports. Controls []drivers.CatalogControl `json:"controls,omitempty"` + // Hold is the operator setting in force, if any, and when it ends. + Hold *controlHold `json:"hold,omitempty"` } type readingDTO struct { @@ -106,6 +107,7 @@ func (s *Server) handleDriverDetail(w http.ResponseWriter, r *http.Request) { } } resp.Controls = s.driverControls(name) + resp.Hold = s.activeControlHold(name) writeJSON(w, 200, resp) } From b0cfec30840f5dcf9dce9faf600cd57d95c86097 Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Tue, 4 Aug 2026 09:50:09 +0200 Subject: [PATCH 02/10] fix(api): make driver control holds fail safe --- go/cmd/ftw/main.go | 8 +- go/internal/api/api.go | 71 ++++---- go/internal/api/api_driver_control.go | 198 ++++++++++++++------- go/internal/api/api_driver_control_test.go | 184 ++++++++++++++++++- 4 files changed, 360 insertions(+), 101 deletions(-) diff --git a/go/cmd/ftw/main.go b/go/cmd/ftw/main.go index 1b75564c..261aa7f6 100644 --- a/go/cmd/ftw/main.go +++ b/go/cmd/ftw/main.go @@ -2442,7 +2442,7 @@ func main() { if !tr.Online { slog.Warn("driver telemetry stale — marking offline + reverting to autonomous", "name", tr.Name, "timeout", watchdogTimeout) - sendDriverDefault(ctx, reg, tr.Name, "watchdog", observeOnlySnap) + sendDriverDefault(ctx, srv, tr.Name, "watchdog", observeOnlySnap) watchdogDefaulted[tr.Name] = struct{}{} bus.Publish(events.DriverLost{Driver: tr.Name, At: time.Now()}) } else { @@ -2488,7 +2488,7 @@ func main() { if _, alreadyDefaulted := watchdogDefaulted[name]; alreadyDefaulted { continue } - sendDriverDefault(ctx, reg, name, freshness.Reason, observeOnlySnap) + sendDriverDefault(ctx, srv, name, freshness.Reason, observeOnlySnap) } // Loadpoint observation and schedule rolling stay live while the @@ -2967,13 +2967,13 @@ func registerAllDevices(st *state.Store, reg *drivers.Registry) { const driverDefaultTimeout = 2 * time.Second -func sendDriverDefault(ctx context.Context, reg *drivers.Registry, name, reason string, observeOnly map[string]bool) { +func sendDriverDefault(ctx context.Context, srv *api.Server, name, reason string, observeOnly map[string]bool) { if observeOnly[name] { return } cmdCtx, cancel := context.WithTimeout(ctx, driverDefaultTimeout) defer cancel() - if err := reg.SendDefault(cmdCtx, name); err != nil { + if err := srv.SendDriverDefault(cmdCtx, name); err != nil { slog.Warn("driver default command failed", "name", name, "reason", reason, "timeout", driverDefaultTimeout, "err", err) } diff --git a/go/internal/api/api.go b/go/internal/api/api.go index 3b7cf3d4..498bcfaa 100644 --- a/go/internal/api/api.go +++ b/go/internal/api/api.go @@ -72,23 +72,23 @@ type Deps struct { Tel *telemetry.Store // LogRing is the in-memory log buffer wired in main.go. Nil makes // /api/drivers/{name}/logs and /api/support/dump return 503. - LogRing *telemetry.LogRing - Ctrl *control.State - CtrlMu *sync.Mutex - State *state.Store - CapMu *sync.RWMutex - Capacities map[string]float64 // driver → battery_capacity_wh (controllable pool) - TelemetryCapacities map[string]float64 // all site batteries incl. observe_only (SoC weighting) - CfgMu *sync.RWMutex - Cfg *config.Config - ConfigPath string - DriverDir string // where to scan for Lua drivers (default: /drivers) - UserDriverDir string // persistent user-drivers overlay; searched before DriverDir - Models map[string]*battery.Model - ModelsMu *sync.Mutex - SelfTune *selftune.Coordinator - DtS float64 // control interval seconds (for model τ / age displays) - SaveConfig func(path string, c *config.Config) error // injection for testability + LogRing *telemetry.LogRing + Ctrl *control.State + CtrlMu *sync.Mutex + State *state.Store + CapMu *sync.RWMutex + Capacities map[string]float64 // driver → battery_capacity_wh (controllable pool) + TelemetryCapacities map[string]float64 // all site batteries incl. observe_only (SoC weighting) + CfgMu *sync.RWMutex + Cfg *config.Config + ConfigPath string + DriverDir string // where to scan for Lua drivers (default: /drivers) + UserDriverDir string // persistent user-drivers overlay; searched before DriverDir + Models map[string]*battery.Model + ModelsMu *sync.Mutex + SelfTune *selftune.Coordinator + DtS float64 // control interval seconds (for model τ / age displays) + SaveConfig func(path string, c *config.Config) error // injection for testability // ConfigApplier is main.go's config-applied callback — the same // closure the configreload watcher runs (registry reload with SoC // bounds, capacities, inverter groups, fuse and mpc/loadmodel @@ -96,13 +96,13 @@ type Deps struct { // config exactly like a file edit would. Nil (tests, minimal // embeddings) still applies control-level fields via // configreload.Apply; only the callback's extras are skipped. - ConfigApplier configreload.Applier - WebDir string // static assets root (default "web") - ColdDir string // cold-storage root for parquet rolloff; empty disables cold fallback - DataDir string // complete persistent-data root used by portable backups - StatePath string // absolute primary SQLite path used by portable backups - BackupDir string // full .ftwbak output; may be an externally mounted path - DataMaintenanceMu *sync.Mutex // excludes Parquet rolloff/pruning while a full backup is captured + ConfigApplier configreload.Applier + WebDir string // static assets root (default "web") + ColdDir string // cold-storage root for parquet rolloff; empty disables cold fallback + DataDir string // complete persistent-data root used by portable backups + StatePath string // absolute primary SQLite path used by portable backups + BackupDir string // full .ftwbak output; may be an externally mounted path + DataMaintenanceMu *sync.Mutex // excludes Parquet rolloff/pruning while a full backup is captured // SnapshotDir is where pre-update snapshots of state.db + config.yaml // are written by the self-update flow. Defaults to // `/snapshots`; main.go is responsible for passing @@ -206,11 +206,11 @@ type Server struct { savingsCacheMu sync.Mutex savingsCache map[string]daySavings - // controlHolds is the one operator setting in force per driver, with the - // timer that ends it. Process-lifetime only, deliberately: a restart + // controlStates serializes command dispatch, default dispatch, and hold + // transitions per driver. Process-lifetime only, deliberately: a restart // should leave no device held by a setting nobody remembers making. - controlHoldMu sync.Mutex - controlHolds map[string]*controlHold + controlStateMu sync.Mutex + controlStates map[string]*controlDriverState versionUpdateMu sync.Mutex driverUpdateMu sync.Mutex @@ -231,10 +231,11 @@ func New(deps *Deps) *Server { deps.WebDir = "web" } s := &Server{ - deps: deps, - mux: http.NewServeMux(), - dailyCache: make(map[string]state.DayEnergy), - drafts: newDriverDrafts(), + deps: deps, + mux: http.NewServeMux(), + dailyCache: make(map[string]state.DayEnergy), + controlStates: make(map[string]*controlDriverState), + drafts: newDriverDrafts(), } s.routes() // A draft's timer died with the previous process, so anything left behind @@ -283,9 +284,9 @@ func (s *Server) routes() { s.handle("GET /api/drivers/{name}/logs", s.handleDriverLogs) s.handle("GET /api/logs", s.handleGlobalLogs) s.handle("GET /api/support/dump", s.handleSupportDump) - s.handle("GET /api/support/report", s.handleSupportReport) - s.handle("POST /api/drivers/{name}/control", s.handleDriverControl) - s.handle("DELETE /api/drivers/{name}/control", s.handleDriverControlRelease) + s.handle("GET /api/support/report", s.handleSupportReport) + s.handle("POST /api/drivers/{name}/control", s.handleDriverControl) + s.handle("DELETE /api/drivers/{name}/control", s.handleDriverControlRelease) s.handle("POST /api/drivers/{name}/restart", s.handleDriverRestart) s.handle("POST /api/drivers/{name}/disable", s.handleDriverDisable) s.handle("POST /api/drivers/{name}/enable", s.handleDriverEnable) diff --git a/go/internal/api/api_driver_control.go b/go/internal/api/api_driver_control.go index 190080ce..2cad47fe 100644 --- a/go/internal/api/api_driver_control.go +++ b/go/internal/api/api_driver_control.go @@ -18,10 +18,15 @@ package api import ( + "bytes" "context" "encoding/json" + "errors" + "fmt" "log/slog" + "math" "net/http" + "sync" "time" "github.com/srcfl/ftw/go/internal/drivers" @@ -34,22 +39,28 @@ import ( const ( maxControlHoldSeconds = 24 * 60 * 60 defaultControlHoldSeconds = 4 * 60 * 60 + controlDefaultTimeout = 10 * time.Second ) +type controlDriverState struct { + mu sync.Mutex + hold *controlHold +} + // controlHold is one active operator setting. The timer is what releases it; // the fields are what the UI shows meanwhile. type controlHold struct { - Control string `json:"control"` - Value *float64 `json:"value,omitempty"` - ExpiresAt int64 `json:"expires_at_ms"` + Control string `json:"control"` + Value any `json:"value,omitempty"` + ExpiresAt int64 `json:"expires_at_ms"` timer *time.Timer } type controlRequest struct { - Control string `json:"control"` - Value *float64 `json:"value"` - DurationS int `json:"duration_s"` + Control string `json:"control"` + Value json.RawMessage `json:"value"` + DurationS int `json:"duration_s"` } // POST /api/drivers/{name}/control — send one declared command and hold it. @@ -92,20 +103,14 @@ func (s *Server) handleDriverControl(w http.ResponseWriter, r *http.Request) { return } - payload := map[string]any{"action": control.ID} - var applied *float64 - if control.Input.Type == "number" { - if req.Value == nil { - writeJSON(w, 400, map[string]string{"error": "control requires a value"}) - return - } - // Clamp here rather than reject: the bounds came from the driver, and - // a UI that rounds differently should not fail an operator's click. - // Clamping in Core is the point — a driver that forgets to clamp is - // exactly the driver this protects. - value := clampToDeclared(*req.Value, control.Input) - applied = &value - payload["value"] = value + applied, err := decodeControlValue(req.Value, control.Input) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": err.Error()}) + return + } + + payload := map[string]any{"action": control.ID, "value": applied} + if value, ok := applied.(float64); ok { // Drivers written before this endpoint read their own key names. // Sending both costs one JSON field and saves every such driver a // rewrite; heishamon reads cmd.offset or cmd.value. @@ -117,11 +122,6 @@ func (s *Server) handleDriverControl(w http.ResponseWriter, r *http.Request) { writeJSON(w, 500, map[string]string{"error": err.Error()}) return } - if err := s.deps.Registry.Send(r.Context(), name, body); err != nil { - writeJSON(w, 500, map[string]string{"error": err.Error()}) - return - } - seconds := req.DurationS if seconds <= 0 { seconds = defaultControlHoldSeconds @@ -129,7 +129,28 @@ func (s *Server) handleDriverControl(w http.ResponseWriter, r *http.Request) { if seconds > maxControlHoldSeconds { seconds = maxControlHoldSeconds } - hold := s.armControlHold(name, control.ID, applied, time.Duration(seconds)*time.Second) + + // Reserve the hold before dispatch. Registry.Send can return after the + // request is canceled even while the driver is still applying the command; + // the reservation guarantees that an ambiguous result still has a bounded + // safety path. The per-driver lock also keeps expiry/default from racing a + // replacement command. + state := s.controlState(name) + state.mu.Lock() + defer state.mu.Unlock() + hold := s.armControlHoldLocked(name, state, control.ID, applied, time.Duration(seconds)*time.Second) + if err := s.deps.Registry.Send(r.Context(), name, body); err != nil { + s.clearControlHoldLocked(state) + defaultCtx, cancel := context.WithTimeout(context.Background(), controlDefaultTimeout) + defaultErr := s.sendDefaultLocked(defaultCtx, name) + cancel() + if defaultErr != nil { + err = errors.Join(err, fmt.Errorf("restore default after ambiguous command: %w", defaultErr)) + slog.Error("ambiguous driver control could not restore default", "driver", name, "err", defaultErr) + } + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) + return + } writeJSON(w, 200, map[string]any{ "control": control.ID, @@ -154,14 +175,41 @@ func (s *Server) handleDriverControlRelease(w http.ResponseWriter, r *http.Reque writeJSON(w, 503, map[string]string{"error": "driver registry not available"}) return } - s.clearControlHold(name) - if err := s.deps.Registry.SendDefault(r.Context(), name); err != nil { + if err := s.SendDriverDefault(context.Background(), name); err != nil { writeJSON(w, 500, map[string]string{"error": err.Error()}) return } writeJSON(w, 200, map[string]string{"status": "released"}) } +func decodeControlValue(raw json.RawMessage, in drivers.CatalogControlInput) (any, error) { + if len(raw) == 0 || bytes.Equal(bytes.TrimSpace(raw), []byte("null")) { + return nil, fmt.Errorf("control requires a %s value", in.Type) + } + switch in.Type { + case "number": + var value float64 + if err := json.Unmarshal(raw, &value); err != nil || math.IsNaN(value) || math.IsInf(value, 0) { + return nil, errors.New("control requires a numeric value") + } + return clampToDeclared(value, in), nil + case "boolean": + var value bool + if err := json.Unmarshal(raw, &value); err != nil { + return nil, errors.New("control requires a boolean value") + } + return value, nil + case "string": + var value string + if err := json.Unmarshal(raw, &value); err != nil { + return nil, errors.New("control requires a string value") + } + return value, nil + default: + return nil, fmt.Errorf("control has unsupported input type %q", in.Type) + } +} + func clampToDeclared(value float64, in drivers.CatalogControlInput) float64 { if in.Min != nil && value < *in.Min { value = *in.Min @@ -175,45 +223,53 @@ func clampToDeclared(value float64, in drivers.CatalogControlInput) float64 { // armControlHold replaces any existing hold for the driver. One driver holds // one control at a time: two overlapping holds on the same device would each // expire into a default that undoes the other. -func (s *Server) armControlHold(name, control string, value *float64, d time.Duration) *controlHold { - s.controlHoldMu.Lock() - defer s.controlHoldMu.Unlock() - if s.controlHolds == nil { - s.controlHolds = make(map[string]*controlHold) +func (s *Server) controlState(name string) *controlDriverState { + s.controlStateMu.Lock() + defer s.controlStateMu.Unlock() + if s.controlStates == nil { + s.controlStates = make(map[string]*controlDriverState) } - if existing, ok := s.controlHolds[name]; ok && existing.timer != nil { - existing.timer.Stop() + state := s.controlStates[name] + if state == nil { + state = &controlDriverState{} + s.controlStates[name] = state } + return state +} + +func (s *Server) armControlHoldLocked(name string, state *controlDriverState, control string, value any, d time.Duration) *controlHold { + s.clearControlHoldLocked(state) hold := &controlHold{ Control: control, Value: value, ExpiresAt: time.Now().Add(d).UnixMilli(), } - hold.timer = time.AfterFunc(d, func() { s.expireControlHold(name, hold) }) - s.controlHolds[name] = hold + hold.timer = time.AfterFunc(d, func() { s.expireControlHold(name, state, hold) }) + state.hold = hold return hold } +func (s *Server) clearControlHoldLocked(state *controlDriverState) { + if state.hold != nil && state.hold.timer != nil { + state.hold.timer.Stop() + } + state.hold = nil +} + // expireControlHold hands the device back to itself. It checks identity // first: a hold that was replaced or released already had its timer stopped, // but a timer that had begun firing cannot be stopped, and defaulting a // driver that an operator has just set again is the one wrong answer here. -func (s *Server) expireControlHold(name string, fired *controlHold) { - s.controlHoldMu.Lock() - current, ok := s.controlHolds[name] - if !ok || current != fired { - s.controlHoldMu.Unlock() - return - } - delete(s.controlHolds, name) - s.controlHoldMu.Unlock() - - if s.deps.Registry == nil { +func (s *Server) expireControlHold(name string, state *controlDriverState, fired *controlHold) { + state.mu.Lock() + defer state.mu.Unlock() + if state.hold != fired { return } + s.clearControlHoldLocked(state) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - if err := s.deps.Registry.SendDefault(ctx, name); err != nil { + if err := s.sendDefaultLocked(ctx, name); err != nil { // Nothing to retry into: the driver is gone, wedged, or already in // its default. Log rather than reschedule, so a dead driver does not // leave a timer firing every ten seconds for the process lifetime. @@ -221,19 +277,41 @@ func (s *Server) expireControlHold(name string, fired *controlHold) { } } -func (s *Server) clearControlHold(name string) { - s.controlHoldMu.Lock() - defer s.controlHoldMu.Unlock() - if hold, ok := s.controlHolds[name]; ok { - if hold.timer != nil { - hold.timer.Stop() - } - delete(s.controlHolds, name) +// SendDriverDefault is the shared safety path for watchdogs and API release. +// It clears the operator hold while holding the same per-driver lock used by +// command dispatch, then sends the driver's own default with a bounded +// context. A caller without a deadline gets the 10-second safety deadline. +func (s *Server) SendDriverDefault(ctx context.Context, name string) error { + state := s.controlState(name) + state.mu.Lock() + defer state.mu.Unlock() + s.clearControlHoldLocked(state) + return s.sendDefaultLocked(ctx, name) +} + +func (s *Server) sendDefaultLocked(ctx context.Context, name string) error { + if s.deps == nil || s.deps.Registry == nil { + return errors.New("driver registry not available") + } + if ctx == nil { + ctx = context.Background() } + if _, ok := ctx.Deadline(); !ok { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, controlDefaultTimeout) + defer cancel() + } + return s.deps.Registry.SendDefault(ctx, name) } func (s *Server) activeControlHold(name string) *controlHold { - s.controlHoldMu.Lock() - defer s.controlHoldMu.Unlock() - return s.controlHolds[name] + state := s.controlState(name) + state.mu.Lock() + defer state.mu.Unlock() + if state.hold == nil { + return nil + } + hold := *state.hold + hold.timer = nil + return &hold } diff --git a/go/internal/api/api_driver_control_test.go b/go/internal/api/api_driver_control_test.go index 8e4a644c..f5495e28 100644 --- a/go/internal/api/api_driver_control_test.go +++ b/go/internal/api/api_driver_control_test.go @@ -64,11 +64,99 @@ function driver_default_mode() end ` +const controlTypesProbeLua = `DRIVER = { + id = "probe_types", + name = "Probe types", + version = "1.0.0", + controls = { + { id = "set_boost", input = { type = "boolean" } }, + { id = "set_mode", input = { type = "string" } }, + }, +} + +local applied = 0 + +function driver_init(config) + host.set_make("Probe types") + host.set_poll_interval(100) +end + +function driver_poll() + host.emit_metric("applied", applied, "n") + return 100 +end + +function driver_command(action, power_w, cmd) + if action == "set_boost" then + if cmd.value == true then applied = 1 else applied = 2 end + return true + end + if action == "set_mode" then + if cmd.value == "eco" then applied = 3 else applied = 4 end + return true + end + return false +end + +function driver_default_mode() + applied = 0 +end +` + +const controlSafetyProbeLua = `DRIVER = { + id = "probe_safety", + name = "Probe safety", + version = "1.0.0", + controls = { + { id = "set_offset", input = { type = "number", min = -3, max = 3 } }, + { id = "set_offset_fail", input = { type = "number", min = -3, max = 3 } }, + }, +} + +local applied = nil +local defaulted = 0 + +function driver_init(config) + host.set_make("Probe safety") + host.set_poll_interval(100) +end + +function driver_poll() + if applied ~= nil then host.emit_metric("applied", applied, "n") end + host.emit_metric("defaulted", defaulted, "n") + return 100 +end + +function driver_command(action, power_w, cmd) + if action == "set_offset" then + applied = tonumber(cmd.value) + return true + end + if action == "set_offset_fail" then + applied = tonumber(cmd.value) + host.sleep(100) + return false + end + return false +end + +function driver_default_mode() + defaulted = defaulted + 1 + host.emit_metric("default_started", defaulted, "n") + host.sleep(200) + applied = 0 +end +` + func controlServer(t *testing.T) (*Server, *telemetry.Store) { + return controlServerWithLua(t, controlProbeLua) +} + +func controlServerWithLua(t *testing.T, source string) (*Server, *telemetry.Store) { t.Helper() dir := t.TempDir() lua := filepath.Join(dir, "probe.lua") - if err := os.WriteFile(lua, []byte(controlProbeLua), 0o600); err != nil { + if err := os.WriteFile(lua, []byte(source), 0o600); err != nil { t.Fatal(err) } tel := telemetry.NewStore() @@ -165,6 +253,28 @@ func TestDriverControlRejectsUndeclared(t *testing.T) { } } +func TestDriverControlPreservesDeclaredBooleanAndStringValues(t *testing.T) { + srv, tel := controlServerWithLua(t, controlTypesProbeLua) + + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_boost","value":true,"duration_s":600}`); rec.Code != http.StatusOK { + t.Fatalf("boolean POST = %d, body %s", rec.Code, rec.Body.String()) + } + waitMetric(t, tel, "heat", "applied", 1) + if hold := srv.activeControlHold("heat"); hold == nil || hold.Value != true { + t.Fatalf("boolean hold = %+v, want true", hold) + } + + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_mode","value":"eco","duration_s":600}`); rec.Code != http.StatusOK { + t.Fatalf("string POST = %d, body %s", rec.Code, rec.Body.String()) + } + waitMetric(t, tel, "heat", "applied", 3) + if hold := srv.activeControlHold("heat"); hold == nil || hold.Value != "eco" { + t.Fatalf("string hold = %+v, want eco", hold) + } +} + func TestDriverControlHoldIsVisibleAndReleasable(t *testing.T) { srv, tel := controlServer(t) @@ -182,7 +292,8 @@ func TestDriverControlHoldIsVisibleAndReleasable(t *testing.T) { if detail.Hold == nil || detail.Hold.Control != "set_offset" { t.Fatalf("hold = %+v, want set_offset", detail.Hold) } - if detail.Hold.Value == nil || *detail.Hold.Value != 2 { + value, ok := detail.Hold.Value.(float64) + if !ok || value != 2 { t.Errorf("hold value = %v, want 2", detail.Hold.Value) } @@ -240,3 +351,72 @@ func TestDriverControlReplacingHoldCancelsTheOldTimer(t *testing.T) { t.Errorf("applied = %v, want -2 to survive", got) } } + +func TestDriverControlAmbiguousCommandRestoresDefault(t *testing.T) { + srv, tel := controlServerWithLua(t, controlSafetyProbeLua) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) + defer cancel() + req := httptest.NewRequest(http.MethodPost, "/api/drivers/heat/control", strings.NewReader( + `{"control":"set_offset_fail","value":2,"duration_s":600}`)).WithContext(ctx) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + srv.Handler().ServeHTTP(rec, req) + if rec.Code != http.StatusInternalServerError { + t.Fatalf("ambiguous POST = %d, body %s", rec.Code, rec.Body.String()) + } + waitMetric(t, tel, "heat", "defaulted", 1) + if hold := srv.activeControlHold("heat"); hold != nil { + t.Fatalf("ambiguous command left hold active: %+v", hold) + } + waitMetric(t, tel, "heat", "applied", 0) +} + +func TestDriverControlDefaultPathClearsHold(t *testing.T) { + srv, tel := controlServerWithLua(t, controlSafetyProbeLua) + + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":2,"duration_s":600}`); rec.Code != http.StatusOK { + t.Fatalf("POST = %d, body %s", rec.Code, rec.Body.String()) + } + if err := srv.SendDriverDefault(context.Background(), "heat"); err != nil { + t.Fatalf("SendDriverDefault = %v", err) + } + if hold := srv.activeControlHold("heat"); hold != nil { + t.Fatalf("default path left hold active: %+v", hold) + } + waitMetric(t, tel, "heat", "defaulted", 1) +} + +// Expiry must hold the per-driver lock through the actual default command. +// Otherwise a replacement can be sent after the old hold is deleted but +// before its default reaches the device, and the old default wins last. +func TestDriverControlSerializesExpiryAndReplacement(t *testing.T) { + srv, tel := controlServerWithLua(t, controlSafetyProbeLua) + + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":1,"duration_s":600}`); rec.Code != http.StatusOK { + t.Fatalf("POST = %d, body %s", rec.Code, rec.Body.String()) + } + state := srv.controlState("heat") + state.mu.Lock() + hold := state.hold + state.mu.Unlock() + if hold == nil { + t.Fatal("missing hold") + } + + done := make(chan struct{}) + go func() { + srv.expireControlHold("heat", state, hold) + close(done) + }() + waitMetric(t, tel, "heat", "default_started", 1) + + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":-2,"duration_s":600}`); rec.Code != http.StatusOK { + t.Fatalf("replacement POST = %d, body %s", rec.Code, rec.Body.String()) + } + <-done + waitMetric(t, tel, "heat", "applied", -2) +} From 9531cfb248d3bd3a01aa964d31e952b094a7cb25 Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Tue, 4 Aug 2026 09:53:27 +0200 Subject: [PATCH 03/10] ci: trigger checks on master-base PR From 1f6686b471d8fbf92978730fa9a7f33c14d6a84a Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Tue, 4 Aug 2026 10:54:36 +0200 Subject: [PATCH 04/10] fix: close driver control safety gaps --- go/internal/api/api.go | 5 + go/internal/api/api_driver_control.go | 128 +++++-- go/internal/api/api_driver_control_test.go | 207 ++++++++++- go/internal/api/api_drivers_debug.go | 72 +++- go/internal/drivers/lua.go | 42 ++- go/internal/drivers/lua_test.go | 38 ++ go/internal/drivers/registry.go | 357 +++++++++++++++---- go/internal/drivers/registry_restart_test.go | 79 ++++ 8 files changed, 823 insertions(+), 105 deletions(-) diff --git a/go/internal/api/api.go b/go/internal/api/api.go index 498bcfaa..430730e8 100644 --- a/go/internal/api/api.go +++ b/go/internal/api/api.go @@ -237,6 +237,11 @@ func New(deps *Deps) *Server { controlStates: make(map[string]*controlDriverState), drafts: newDriverDrafts(), } + if deps.Registry != nil { + // Registry removal is the lifecycle boundary for a driver generation. + // Clear the API hold before a replacement instance can be added. + deps.Registry.SetLifecycleHook(s.clearDriverControl) + } s.routes() // A draft's timer died with the previous process, so anything left behind // goes back now. What runs after a restart should be the driver that was diff --git a/go/internal/api/api_driver_control.go b/go/internal/api/api_driver_control.go index 2cad47fe..ca62de4e 100644 --- a/go/internal/api/api_driver_control.go +++ b/go/internal/api/api_driver_control.go @@ -23,7 +23,6 @@ import ( "encoding/json" "errors" "fmt" - "log/slog" "math" "net/http" "sync" @@ -50,9 +49,10 @@ type controlDriverState struct { // controlHold is one active operator setting. The timer is what releases it; // the fields are what the UI shows meanwhile. type controlHold struct { - Control string `json:"control"` - Value any `json:"value,omitempty"` - ExpiresAt int64 `json:"expires_at_ms"` + Control string `json:"control"` + Value any `json:"value,omitempty"` + ExpiresAt int64 `json:"expires_at_ms"` + Generation uint64 `json:"-"` timer *time.Timer } @@ -81,6 +81,13 @@ func (s *Server) handleDriverControl(w http.ResponseWriter, r *http.Request) { writeJSON(w, 400, map[string]string{"error": "invalid request"}) return } + if cfg, ok := s.configuredDriver(name); ok && cfg.ObserveOnly { + writeJSON(w, http.StatusForbidden, map[string]any{ + "error": "driver is observe_only and cannot be controlled", + "observe_only": true, + }) + return + } declared := s.driverControls(name) if len(declared) == 0 { @@ -102,6 +109,20 @@ func (s *Server) handleDriverControl(w http.ResponseWriter, r *http.Request) { writeJSON(w, 503, map[string]string{"error": "driver registry not available"}) return } + status, ok := s.deps.Registry.ControlStatus(name) + if !ok { + writeJSON(w, http.StatusNotFound, map[string]string{"error": "driver not running"}) + return + } + if status.Blocked { + writeJSON(w, http.StatusConflict, map[string]any{ + "error": drivers.ErrControlBlocked.Error(), + "control_blocked": true, + "default_confirmed": false, + "recovery_pending": status.RecoveryPending, + }) + return + } applied, err := decodeControlValue(req.Value, control.Input) if err != nil { @@ -138,15 +159,24 @@ func (s *Server) handleDriverControl(w http.ResponseWriter, r *http.Request) { state := s.controlState(name) state.mu.Lock() defer state.mu.Unlock() - hold := s.armControlHoldLocked(name, state, control.ID, applied, time.Duration(seconds)*time.Second) + hold := s.armControlHoldLocked(name, state, control.ID, applied, status.Generation, time.Duration(seconds)*time.Second) if err := s.deps.Registry.Send(r.Context(), name, body); err != nil { s.clearControlHoldLocked(state) - defaultCtx, cancel := context.WithTimeout(context.Background(), controlDefaultTimeout) - defaultErr := s.sendDefaultLocked(defaultCtx, name) - cancel() - if defaultErr != nil { - err = errors.Join(err, fmt.Errorf("restore default after ambiguous command: %w", defaultErr)) - slog.Error("ambiguous driver control could not restore default", "driver", name, "err", defaultErr) + if errors.Is(err, drivers.ErrObserveOnly) { + writeJSON(w, http.StatusForbidden, map[string]any{ + "error": err.Error(), + "observe_only": true, + }) + return + } + if errors.Is(err, drivers.ErrControlBlocked) { + writeJSON(w, http.StatusConflict, map[string]any{ + "error": err.Error(), + "control_blocked": true, + "default_confirmed": false, + "recovery_pending": true, + }) + return } writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) return @@ -237,12 +267,13 @@ func (s *Server) controlState(name string) *controlDriverState { return state } -func (s *Server) armControlHoldLocked(name string, state *controlDriverState, control string, value any, d time.Duration) *controlHold { +func (s *Server) armControlHoldLocked(name string, state *controlDriverState, control string, value any, generation uint64, d time.Duration) *controlHold { s.clearControlHoldLocked(state) hold := &controlHold{ - Control: control, - Value: value, - ExpiresAt: time.Now().Add(d).UnixMilli(), + Control: control, + Value: value, + ExpiresAt: time.Now().Add(d).UnixMilli(), + Generation: generation, } hold.timer = time.AfterFunc(d, func() { s.expireControlHold(name, state, hold) }) state.hold = hold @@ -266,15 +297,20 @@ func (s *Server) expireControlHold(name string, state *controlDriverState, fired if state.hold != fired { return } + if s.deps.Registry == nil { + s.clearControlHoldLocked(state) + return + } + status, ok := s.deps.Registry.ControlStatus(name) + if !ok || status.Generation != fired.Generation { + // A stopped generation must never default a replacement instance. + s.clearControlHoldLocked(state) + return + } s.clearControlHoldLocked(state) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - if err := s.sendDefaultLocked(ctx, name); err != nil { - // Nothing to retry into: the driver is gone, wedged, or already in - // its default. Log rather than reschedule, so a dead driver does not - // leave a timer firing every ten seconds for the process lifetime. - slog.Warn("control hold expiry failed", "driver", name, "err", err) - } + _ = s.sendDefaultLocked(ctx, name) } // SendDriverDefault is the shared safety path for watchdogs and API release. @@ -282,6 +318,12 @@ func (s *Server) expireControlHold(name string, state *controlDriverState, fired // command dispatch, then sends the driver's own default with a bounded // context. A caller without a deadline gets the 10-second safety deadline. func (s *Server) SendDriverDefault(ctx context.Context, name string) error { + if s.deps == nil || s.deps.Registry == nil { + return errors.New("driver registry not available") + } + if _, ok := s.deps.Registry.ControlStatus(name); !ok { + return fmt.Errorf("driver %q not found", name) + } state := s.controlState(name) state.mu.Lock() defer state.mu.Unlock() @@ -305,13 +347,55 @@ func (s *Server) sendDefaultLocked(ctx context.Context, name string) error { } func (s *Server) activeControlHold(name string) *controlHold { - state := s.controlState(name) + state := s.peekControlState(name) + if state == nil { + return nil + } state.mu.Lock() defer state.mu.Unlock() if state.hold == nil { return nil } + if s.deps == nil || s.deps.Registry == nil { + s.clearControlHoldLocked(state) + return nil + } + status, ok := s.deps.Registry.ControlStatus(name) + if !ok || status.Generation != state.hold.Generation { + s.clearControlHoldLocked(state) + return nil + } hold := *state.hold hold.timer = nil return &hold } + +func (s *Server) peekControlState(name string) *controlDriverState { + s.controlStateMu.Lock() + state := s.controlStates[name] + s.controlStateMu.Unlock() + return state +} + +// clearDriverControl is called by Registry before a driver generation is +// removed. It stops the old timer and drops the map entry so a re-added driver +// cannot inherit an operator hold from the previous instance. +func (s *Server) clearDriverControl(name string) { + s.clearDriverControlState(name, nil) +} + +func (s *Server) clearDriverControlState(name string, expected *controlDriverState) { + s.controlStateMu.Lock() + state := s.controlStates[name] + if expected != nil && state != expected { + s.controlStateMu.Unlock() + return + } + delete(s.controlStates, name) + s.controlStateMu.Unlock() + if state != nil { + state.mu.Lock() + s.clearControlHoldLocked(state) + state.mu.Unlock() + } +} diff --git a/go/internal/api/api_driver_control_test.go b/go/internal/api/api_driver_control_test.go index f5495e28..cb6e075a 100644 --- a/go/internal/api/api_driver_control_test.go +++ b/go/internal/api/api_driver_control_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strconv" "strings" "sync" "testing" @@ -148,11 +149,54 @@ function driver_default_mode() end ` +const controlRecoveryProbeLua = `DRIVER = { + id = "probe_recovery", + name = "Probe recovery", + version = "1.0.0", + controls = { + { id = "set_offset", input = { type = "number", min = -3, max = 3 } }, + }, +} + +local applied = nil +local defaults = 0 + +function driver_init(config) + host.set_make("Probe recovery") + host.set_poll_interval(100) +end + +function driver_poll() + if applied ~= nil then host.emit_metric("applied", applied, "n") end + host.emit_metric("defaulted", defaults, "n") + return 100 +end + +function driver_command(action, power_w, cmd) + applied = tonumber(cmd and (cmd.value or cmd.offset)) + host.emit_metric("command_applied", applied, "n") + if defaults < 2 then return false end + return true +end + +function driver_default_mode() + defaults = defaults + 1 + host.emit_metric("default_attempt", defaults, "n") + if defaults == 1 then return false end + if defaults == 2 then host.sleep(500) end + applied = 0 +end +` + func controlServer(t *testing.T) (*Server, *telemetry.Store) { return controlServerWithLua(t, controlProbeLua) } func controlServerWithLua(t *testing.T, source string) (*Server, *telemetry.Store) { + return controlServerWithLuaConfig(t, source, config.Driver{Name: "heat"}) +} + +func controlServerWithLuaConfig(t *testing.T, source string, cfg config.Driver) (*Server, *telemetry.Store) { t.Helper() dir := t.TempDir() lua := filepath.Join(dir, "probe.lua") @@ -161,7 +205,7 @@ func controlServerWithLua(t *testing.T, source string) (*Server, *telemetry.Stor } tel := telemetry.NewStore() reg := drivers.NewRegistry(tel) - cfg := config.Driver{Name: "heat", Lua: lua} + cfg.Lua = lua if err := reg.Add(context.Background(), cfg); err != nil { t.Fatalf("add driver: %v", err) } @@ -420,3 +464,164 @@ func TestDriverControlSerializesExpiryAndReplacement(t *testing.T) { <-done waitMetric(t, tel, "heat", "applied", -2) } + +func TestDriverControlRejectsObserveOnlyWithoutSending(t *testing.T) { + srv, tel := controlServerWithLuaConfig(t, controlProbeLua, config.Driver{ + Name: "heat", + ObserveOnly: true, + }) + + rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":2,"duration_s":600}`) + if rec.Code != http.StatusForbidden { + t.Fatalf("observe_only POST = %d, body %s", rec.Code, rec.Body.String()) + } + var body map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatal(err) + } + if body["observe_only"] != true { + t.Fatalf("observe_only response = %v", body) + } + if _, _, ok := tel.LatestMetric("heat", "applied"); ok { + t.Fatal("observe_only control reached the driver") + } + if err := srv.deps.Registry.Send(context.Background(), "heat", []byte(`{"action":"set_offset","value":2}`)); err != drivers.ErrObserveOnly { + t.Fatalf("direct observe_only Send = %v, want %v", err, drivers.ErrObserveOnly) + } +} + +func TestDriverControlBlocksUntilDefaultRecovery(t *testing.T) { + srv, tel := controlServerWithLua(t, controlRecoveryProbeLua) + + rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":2,"duration_s":600}`) + if rec.Code != http.StatusConflict { + t.Fatalf("partial-effect POST = %d, body %s", rec.Code, rec.Body.String()) + } + var blockedBody map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &blockedBody); err != nil { + t.Fatal(err) + } + if blockedBody["control_blocked"] != true || blockedBody["default_confirmed"] != false { + t.Fatalf("partial-effect safety response = %v", blockedBody) + } + waitMetric(t, tel, "heat", "command_applied", 2) + waitMetric(t, tel, "heat", "default_attempt", 1) + + status, ok := srv.deps.Registry.ControlStatus("heat") + if !ok || !status.Blocked || !status.RecoveryPending || status.DefaultConfirmed { + t.Fatalf("control status after failed default = %+v, running=%v", status, ok) + } + rec = post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":3,"duration_s":600}`) + if rec.Code != http.StatusConflict { + t.Fatalf("POST while default recovery is pending = %d, body %s", rec.Code, rec.Body.String()) + } + + detail := driverDetail(t, srv, "heat") + if detail.ControlState.State != "default_recovery" || !detail.ControlState.Blocked || + detail.ControlState.DefaultConfirmed || !detail.ControlState.RecoveryPending { + t.Fatalf("recovery control state = %+v", detail.ControlState) + } + + waitMetric(t, tel, "heat", "default_attempt", 2) + waitMetric(t, tel, "heat", "defaulted", 2) + status, ok = srv.deps.Registry.ControlStatus("heat") + if !ok || status.Blocked || !status.DefaultConfirmed || status.RecoveryPending { + t.Fatalf("control status after recovery = %+v, running=%v", status, ok) + } + rec = post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":3,"duration_s":600}`) + if rec.Code != http.StatusOK { + t.Fatalf("POST after default recovery = %d, body %s", rec.Code, rec.Body.String()) + } + waitMetric(t, tel, "heat", "applied", 3) +} + +func TestDriverControlClearsHoldAcrossLifecycle(t *testing.T) { + srv, tel := controlServer(t) + cfg := srv.deps.Cfg.Drivers[0] + + rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":2,"duration_s":600}`) + if rec.Code != http.StatusOK { + t.Fatalf("initial POST = %d, body %s", rec.Code, rec.Body.String()) + } + waitMetric(t, tel, "heat", "applied", 2) + oldState := srv.peekControlState("heat") + if oldState == nil { + t.Fatal("initial control state is missing") + } + oldState.mu.Lock() + oldHold := oldState.hold + oldState.mu.Unlock() + if oldHold == nil { + t.Fatal("initial control hold is missing") + } + + if err := srv.deps.Registry.Restart(context.Background(), cfg); err != nil { + t.Fatalf("restart = %v", err) + } + if got := srv.activeControlHold("heat"); got != nil { + t.Fatalf("hold survived restart: %+v", got) + } + if got := srv.peekControlState("heat"); got != nil { + t.Fatal("control state map entry survived restart") + } + if got := driverDetail(t, srv, "heat"); got.Hold != nil { + t.Fatalf("GET after restart exposed old hold: %+v", got.Hold) + } + + rec = post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":-2,"duration_s":600}`) + if rec.Code != http.StatusOK { + t.Fatalf("POST after restart = %d, body %s", rec.Code, rec.Body.String()) + } + waitMetric(t, tel, "heat", "applied", -2) + // Simulate a timer callback that was already in flight when the old + // generation was removed. It must not default the new instance. + srv.expireControlHold("heat", oldState, oldHold) + time.Sleep(250 * time.Millisecond) + if got, _, ok := tel.LatestMetric("heat", "defaulted"); !ok || got != 0 { + t.Fatalf("old timer changed the replacement: defaulted=%v/%v", got, ok) + } + if got, _, ok := tel.LatestMetric("heat", "applied"); !ok || got != -2 { + t.Fatalf("replacement applied value = %v/%v, want -2", got, ok) + } + + disabled := cfg + disabled.Disabled = true + srv.deps.Registry.Reload(context.Background(), []config.Driver{disabled}, false) + if got := srv.peekControlState("heat"); got != nil { + t.Fatal("control state map entry survived disable") + } + if got := driverDetail(t, srv, "heat"); got.Hold != nil { + t.Fatalf("GET while disabled exposed a hold: %+v", got.Hold) + } + + srv.deps.Registry.Reload(context.Background(), []config.Driver{cfg}, false) + if got := srv.peekControlState("heat"); got != nil { + t.Fatal("control state map entry survived re-add") + } + if got := driverDetail(t, srv, "heat"); got.Hold != nil { + t.Fatalf("GET after re-add exposed an old hold: %+v", got.Hold) + } +} + +func TestDriverDetailUnknownNamesDoNotCreateControlState(t *testing.T) { + srv, _ := controlServer(t) + for i := 0; i < 1000; i++ { + name := "missing-" + strconv.Itoa(i) + rec := httptest.NewRecorder() + srv.Handler().ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/api/drivers/"+name, nil)) + if rec.Code != http.StatusOK { + t.Fatalf("GET %s = %d, body %s", name, rec.Code, rec.Body.String()) + } + } + srv.controlStateMu.Lock() + defer srv.controlStateMu.Unlock() + if got := len(srv.controlStates); got != 0 { + t.Fatalf("unknown GETs created %d control states", got) + } +} diff --git a/go/internal/api/api_drivers_debug.go b/go/internal/api/api_drivers_debug.go index ef559d71..85ecc17d 100644 --- a/go/internal/api/api_drivers_debug.go +++ b/go/internal/api/api_drivers_debug.go @@ -39,6 +39,16 @@ type driverDetailResp struct { Controls []drivers.CatalogControl `json:"controls,omitempty"` // Hold is the operator setting in force, if any, and when it ends. Hold *controlHold `json:"hold,omitempty"` + // ControlState says whether Core has confirmed autonomous default mode. + // It must not call a failed default safe or settled. + ControlState driverControlStateResp `json:"control_state"` +} + +type driverControlStateResp struct { + State string `json:"state"` + Blocked bool `json:"blocked"` + DefaultConfirmed bool `json:"default_confirmed"` + RecoveryPending bool `json:"recovery_pending"` } type readingDTO struct { @@ -108,9 +118,35 @@ func (s *Server) handleDriverDetail(w http.ResponseWriter, r *http.Request) { } resp.Controls = s.driverControls(name) resp.Hold = s.activeControlHold(name) + resp.ControlState = s.driverControlState(name, resp.Hold) writeJSON(w, 200, resp) } +func (s *Server) driverControlState(name string, hold *controlHold) driverControlStateResp { + if s.deps == nil || s.deps.Registry == nil { + return driverControlStateResp{State: "unknown"} + } + status, ok := s.deps.Registry.ControlStatus(name) + if !ok { + return driverControlStateResp{State: "unknown"} + } + state := "unknown" + switch { + case status.Blocked: + state = "default_recovery" + case hold != nil: + state = "held" + case status.DefaultConfirmed: + state = "default_confirmed" + } + return driverControlStateResp{ + State: state, + Blocked: status.Blocked, + DefaultConfirmed: status.DefaultConfirmed, + RecoveryPending: status.RecoveryPending, + } +} + // driverControls returns the controls the configured driver `name` declares. // // The lookup goes name → configured lua path → catalog entry. A driver that @@ -119,25 +155,11 @@ func (s *Server) handleDriverDetail(w http.ResponseWriter, r *http.Request) { // failure belongs in the catalog endpoint, where an operator is looking at // driver files, not here. func (s *Server) driverControls(name string) []drivers.CatalogControl { - if s.deps.Cfg == nil { - return nil - } - lua := "" - if s.deps.CfgMu != nil { - s.deps.CfgMu.RLock() - } - for _, d := range s.deps.Cfg.Drivers { - if d.Name == name { - lua = d.Lua - break - } - } - if s.deps.CfgMu != nil { - s.deps.CfgMu.RUnlock() - } - if lua == "" { + cfg, ok := s.configuredDriver(name) + if !ok || cfg.Lua == "" { return nil } + lua := cfg.Lua // Config.ResolveDriverPaths normally makes lua absolute. Read that exact // file first when it is available: a local overlay may contain the same // filename as a deliberately selected managed or bundled driver. @@ -160,6 +182,22 @@ func (s *Server) driverControls(name string) []drivers.CatalogControl { return drivers.ControlsForDriver(entries, lua) } +func (s *Server) configuredDriver(name string) (config.Driver, bool) { + if s.deps == nil || s.deps.Cfg == nil { + return config.Driver{}, false + } + if s.deps.CfgMu != nil { + s.deps.CfgMu.RLock() + defer s.deps.CfgMu.RUnlock() + } + for _, d := range s.deps.Cfg.Drivers { + if d.Name == name { + return d, true + } + } + return config.Driver{}, false +} + // POST /api/drivers/test — start one short-lived driver instance from the // posted config, wait briefly for telemetry, and return whatever live values // it emitted. This lets Settings validate an unsaved driver without writing it diff --git a/go/internal/drivers/lua.go b/go/internal/drivers/lua.go index 25286fef..0ec70f2f 100644 --- a/go/internal/drivers/lua.go +++ b/go/internal/drivers/lua.go @@ -269,6 +269,14 @@ func (d *LuaDriver) Command(ctx context.Context, cmdJSON []byte) error { } d.mu.Lock() defer d.mu.Unlock() + if ctx == nil { + ctx = context.Background() + } + // Legacy commands are allowed to write hardware before they return. Give + // the VM the caller's context so a cancelled request or a driver lifecycle + // stop can terminate a Lua loop and let the registry run the default path. + d.L.SetContext(ctx) + defer d.L.RemoveContext() fn := d.L.GetGlobal("driver_command") if fn == lua.LNil { return nil @@ -486,6 +494,18 @@ func containsEvidence(evidence []string, want string) bool { return false } +func (d *LuaDriver) setLuaCallContext(parent context.Context, timeout time.Duration) func() { + if parent == nil { + parent = context.Background() + } + ctx, cancel := context.WithTimeout(parent, timeout) + d.L.SetContext(ctx) + return func() { + d.L.RemoveContext() + cancel() + } +} + func (d *LuaDriver) setLifecycleContext(parent context.Context, timeout time.Duration) func() { if d.Env.RuntimePolicy == nil || !d.Env.RuntimePolicy.IsControlV2() { return func() {} @@ -518,7 +538,14 @@ func (d *LuaDriver) commandResult(cmd DriverCommandV1, now time.Time) DriverComm // Cleanup calls driver_cleanup() and closes the VM. func (d *LuaDriver) Cleanup() { - _ = d.call("driver_cleanup") + d.CleanupContext(context.Background()) +} + +// CleanupContext runs driver_cleanup with a bounded, cancellable VM context +// before closing the state. The no-argument Cleanup method remains for tests +// and direct embedders that do not have a lifecycle context. +func (d *LuaDriver) CleanupContext(ctx context.Context) { + _ = d.call(ctx, "driver_cleanup") d.mu.Lock() d.L.Close() d.mu.Unlock() @@ -527,18 +554,25 @@ func (d *LuaDriver) Cleanup() { // DefaultMode calls driver_default_mode() — typically tells the device // to revert to autonomous self-consumption when the EMS is offline. func (d *LuaDriver) DefaultMode() error { - return d.call("driver_default_mode") + return d.DefaultModeContext(context.Background()) +} + +// DefaultModeContext calls driver_default_mode with a caller-owned context. +// This is the safety path after an ambiguous or failed command, so it must +// not leave a legacy Lua loop holding the VM lock forever. +func (d *LuaDriver) DefaultModeContext(ctx context.Context) error { + return d.call(ctx, "driver_default_mode") } // call is a convenience for parameter-less void-returning lifecycle funcs. -func (d *LuaDriver) call(name string) error { +func (d *LuaDriver) call(ctx context.Context, name string) error { d.mu.Lock() defer d.mu.Unlock() fn := d.L.GetGlobal(name) if fn == lua.LNil { return nil } - cleanup := d.setLifecycleContext(context.Background(), 5*time.Second) + cleanup := d.setLuaCallContext(ctx, 5*time.Second) defer cleanup() if err := d.L.CallByParam(lua.P{Fn: fn, NRet: 1, Protect: true}); err != nil { return err diff --git a/go/internal/drivers/lua_test.go b/go/internal/drivers/lua_test.go index a837ef04..18c1a520 100644 --- a/go/internal/drivers/lua_test.go +++ b/go/internal/drivers/lua_test.go @@ -117,6 +117,44 @@ end } } +func TestLuaLegacyCommandHonorsContextAndDefaultCanRunAfterCancel(t *testing.T) { + path := filepath.Join(t.TempDir(), "loop.lua") + src := ` +function driver_command(action, w, cmd) + host.emit_metric("command_side_effect", 1) + while true do end +end +function driver_default_mode() + host.emit_metric("default_called", 1) +end +` + if err := os.WriteFile(path, []byte(src), 0o644); err != nil { + t.Fatal(err) + } + tel := telemetry.NewStore() + d, err := NewLuaDriver(path, NewHostEnv("loop", tel)) + if err != nil { + t.Fatal(err) + } + defer d.Cleanup() + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + err = d.Command(ctx, []byte(`{"action":"set_offset","value":2}`)) + cancel() + if err == nil || !strings.Contains(err.Error(), "context deadline exceeded") { + t.Fatalf("cancellable legacy command error = %v, want context deadline exceeded", err) + } + if got, _, ok := tel.LatestMetric("loop", "command_side_effect"); !ok || got != 1 { + t.Fatalf("command side effect metric = %v/%v, want 1", got, ok) + } + if err := d.DefaultModeContext(context.Background()); err != nil { + t.Fatalf("default after cancelled command: %v", err) + } + if got, _, ok := tel.LatestMetric("loop", "default_called"); !ok || got != 1 { + t.Fatalf("default metric = %v/%v, want 1", got, ok) + } +} + type luaKindTestModbus struct { called bool } diff --git a/go/internal/drivers/registry.go b/go/internal/drivers/registry.go index 29f99c31..5ffea6ff 100644 --- a/go/internal/drivers/registry.go +++ b/go/internal/drivers/registry.go @@ -18,6 +18,31 @@ import ( "github.com/srcfl/ftw/go/internal/telemetry" ) +var ( + // ErrControlBlocked is returned until the driver's autonomous default has + // completed successfully after an ambiguous or failed control command. + ErrControlBlocked = errors.New("driver control is blocked until autonomous default is confirmed") + // ErrObserveOnly is returned when a configured telemetry-only driver is + // reached through a generic command path instead of the API guard. + ErrObserveOnly = errors.New("driver is observe_only and cannot be controlled") +) + +const ( + defaultRecoveryTimeout = 5 * time.Second + defaultRetryInitial = 100 * time.Millisecond + defaultRetryMax = 30 * time.Second +) + +// DriverControlStatus describes the safety state that the API may expose to +// an operator. DefaultConfirmed is false unless the registry has completed a +// default-mode call successfully for the current driver instance. +type DriverControlStatus struct { + Blocked bool + DefaultConfirmed bool + RecoveryPending bool + Generation uint64 +} + // Registry manages running Lua driver instances — spawn, poll, command, stop. // Thread-safe. type Registry struct { @@ -51,8 +76,10 @@ type Registry struct { // A nil sink keeps tests and legacy setups simple. CommandResultSink func(driverName string, result DriverCommandResultV1) - mu sync.Mutex - rec map[string]*runningDriver + mu sync.Mutex + rec map[string]*runningDriver + nextGeneration uint64 + lifecycleHook func(name string) } // NewRegistry builds a driver registry. @@ -71,6 +98,14 @@ func (r *Registry) SetTroubleshootingMode(enabled bool) { r.mu.Unlock() } +// SetLifecycleHook installs the callback used by API owners to clear state +// before a driver instance is removed. The callback runs without r.mu held. +func (r *Registry) SetLifecycleHook(hook func(name string)) { + r.mu.Lock() + r.lifecycleHook = hook + r.mu.Unlock() +} + // driverRuntime abstracts the Lua driver lifecycle so the registry's // run-loop, command dispatch, and health tracking stay clean. type driverRuntime interface { @@ -99,9 +134,14 @@ func (l *luaRuntime) Init(ctx context.Context, cfg []byte) error { } return l.LuaDriver.Init(ctx, m) } -func (l *luaRuntime) DefaultMode(ctx context.Context) error { return l.LuaDriver.DefaultMode() } -func (l *luaRuntime) Cleanup(ctx context.Context) error { l.LuaDriver.Cleanup(); return nil } -func (l *luaRuntime) Env() *HostEnv { return l.LuaDriver.Env } +func (l *luaRuntime) DefaultMode(ctx context.Context) error { + return l.LuaDriver.DefaultModeContext(ctx) +} +func (l *luaRuntime) Cleanup(ctx context.Context) error { + l.LuaDriver.CleanupContext(ctx) + return nil +} +func (l *luaRuntime) Env() *HostEnv { return l.LuaDriver.Env } func (l *luaRuntime) CommandV2(ctx context.Context, cmd DriverCommandV1, now time.Time) (DriverCommandResultV1, error) { return l.LuaDriver.CommandV2(ctx, cmd, now) } @@ -131,18 +171,103 @@ func driverInitConfigJSON(cfg config.Driver, troubleshootingMode bool) []byte { } type runningDriver struct { - driver driverRuntime - env *HostEnv - cfg config.Driver - policy *RuntimePolicy - leaseExpiresAt time.Time - controlBlocked bool + driver driverRuntime + env *HostEnv + cfg config.Driver + policy *RuntimePolicy + leaseExpiresAt time.Time + generation uint64 + statusMu sync.RWMutex + controlBlocked bool + defaultConfirmed bool + recoveryPending bool + activeMu sync.Mutex + activeCancel context.CancelFunc + lifecycleCtx context.Context + lifecycleCancel context.CancelFunc // Poll loop coordination cmdCh chan driverCmd stop chan bool done chan struct{} } +func (rd *runningDriver) controlStatus() DriverControlStatus { + rd.statusMu.RLock() + defer rd.statusMu.RUnlock() + return DriverControlStatus{ + Blocked: rd.controlBlocked, + DefaultConfirmed: rd.defaultConfirmed, + RecoveryPending: rd.recoveryPending, + Generation: rd.generation, + } +} + +func (rd *runningDriver) controlIsBlocked() bool { + rd.statusMu.RLock() + blocked := rd.controlBlocked + rd.statusMu.RUnlock() + return blocked +} + +func (rd *runningDriver) markCommandApplied() { + rd.statusMu.Lock() + rd.defaultConfirmed = false + rd.statusMu.Unlock() +} + +func (rd *runningDriver) markDefaultConfirmed() { + rd.statusMu.Lock() + rd.controlBlocked = false + rd.defaultConfirmed = true + rd.recoveryPending = false + rd.statusMu.Unlock() +} + +func (rd *runningDriver) markDefaultRecoveryPending() { + rd.statusMu.Lock() + rd.controlBlocked = true + rd.defaultConfirmed = false + rd.recoveryPending = true + rd.statusMu.Unlock() +} + +func (rd *runningDriver) beginCommand(ctx context.Context) (context.Context, func()) { + if ctx == nil { + ctx = context.Background() + } + parent := rd.lifecycleCtx + if parent == nil { + parent = context.Background() + } + commandCtx, cancel := context.WithCancel(parent) + stopCaller := context.AfterFunc(ctx, cancel) + rd.activeMu.Lock() + rd.activeCancel = cancel + rd.activeMu.Unlock() + return commandCtx, func() { + rd.activeMu.Lock() + rd.activeCancel = nil + rd.activeMu.Unlock() + stopCaller() + cancel() + } +} + +func (rd *runningDriver) cancelActiveCommand() { + rd.activeMu.Lock() + cancel := rd.activeCancel + rd.activeMu.Unlock() + if cancel != nil { + cancel() + } +} + +func (rd *runningDriver) cancelLifecycle() { + if rd.lifecycleCancel != nil { + rd.lifecycleCancel() + } +} + type driverCmd struct { kind string ctx context.Context @@ -302,16 +427,24 @@ func (r *Registry) Add(ctx context.Context, cfg config.Driver) error { } } + lifecycleCtx, lifecycleCancel := context.WithCancel(context.Background()) rd := &runningDriver{ - driver: drv, - env: env, - cfg: cfg, - policy: policy, - cmdCh: make(chan driverCmd, 8), - stop: make(chan bool, 1), - done: make(chan struct{}), + driver: drv, + env: env, + cfg: cfg, + policy: policy, + lifecycleCtx: lifecycleCtx, + lifecycleCancel: lifecycleCancel, + cmdCh: make(chan driverCmd, 8), + stop: make(chan bool, 1), + done: make(chan struct{}), } r.mu.Lock() + r.nextGeneration++ + rd.generation = r.nextGeneration + if policy != nil && policy.IsControlV2() { + rd.defaultConfirmed = true + } r.rec[cfg.Name] = rd r.mu.Unlock() // Create the health record eagerly so /api/status reflects @@ -332,7 +465,10 @@ func (r *Registry) Add(ctx context.Context, cfg config.Driver) error { // runLoop polls the driver at its requested cadence and handles commands. func (r *Registry) runLoop(rd *runningDriver) { defer close(rd.done) - ctx := context.Background() + ctx := rd.lifecycleCtx + if ctx == nil { + ctx = context.Background() + } // The first tick can be held back by host.set_warmup_s for a device // that answers Modbus before its registers mean anything. Every // later tick uses the plain poll interval. @@ -345,6 +481,9 @@ func (r *Registry) runLoop(rd *runningDriver) { } defer leaseTimer.Stop() var leaseC <-chan time.Time + var recoveryTimer *time.Timer + var recoveryC <-chan time.Time + retryDelay := defaultRetryInitial clearLease := func() { rd.leaseExpiresAt = time.Time{} if !leaseTimer.Stop() { @@ -365,13 +504,74 @@ func (r *Registry) runLoop(rd *runningDriver) { leaseTimer.Reset(d) leaseC = leaseTimer.C } + clearRecoveryTimer := func() { + if recoveryTimer != nil { + if !recoveryTimer.Stop() { + select { + case <-recoveryTimer.C: + default: + } + } + } + recoveryC = nil + retryDelay = defaultRetryInitial + } + scheduleRecovery := func() { + d := retryDelay + if d <= 0 { + d = defaultRetryInitial + } + if recoveryTimer == nil { + recoveryTimer = time.NewTimer(d) + } else { + if !recoveryTimer.Stop() { + select { + case <-recoveryTimer.C: + default: + } + } + recoveryTimer.Reset(d) + } + recoveryC = recoveryTimer.C + rd.markDefaultRecoveryPending() + if retryDelay < defaultRetryMax { + retryDelay *= 2 + if retryDelay > defaultRetryMax { + retryDelay = defaultRetryMax + } + } + } + defer clearRecoveryTimer() + attemptDefault := func(reason string) error { + defaultCtx, cancel := context.WithTimeout(context.Background(), defaultRecoveryTimeout) + defaultErr := r.defaultDriver(defaultCtx, rd, reason) + cancel() + if defaultErr != nil { + scheduleRecovery() + return defaultErr + } + rd.markDefaultConfirmed() + clearLease() + clearRecoveryTimer() + return nil + } + restoreAfterCommand := func(commandErr error) error { + clearLease() + defaultErr := attemptDefault("command_failed") + if defaultErr != nil { + return errors.Join(commandErr, fmt.Errorf("%w: restore default after ambiguous command: %v", ErrControlBlocked, defaultErr)) + } + return commandErr + } for { select { case skipDefault := <-rd.stop: if !skipDefault { - if err := r.defaultDriver(ctx, rd, "host_shutdown"); err != nil { + shutdownCtx, cancel := context.WithTimeout(context.Background(), defaultRecoveryTimeout) + if err := r.defaultDriver(shutdownCtx, rd, "host_shutdown"); err != nil { slog.Error("driver failed to enter default mode during shutdown", "name", rd.cfg.Name, "err", err) } + cancel() } _ = rd.driver.Cleanup(ctx) // Tear down capability connections so a subsequent Add @@ -404,41 +604,43 @@ func (r *Registry) runLoop(rd *runningDriver) { } switch cmd.kind { case "command": + if rd.controlIsBlocked() { + err = ErrControlBlocked + break + } + commandCtx, finishCommand := rd.beginCommand(cmdCtx) if rd.policy != nil && rd.policy.IsControlV2() { - if rd.controlBlocked { - err = errors.New("driver control is blocked because default mode failed") - } else { - var result DriverCommandResultV1 - var leaseExpiresAt time.Time - result, leaseExpiresAt, err = r.dispatchV2Command(cmdCtx, rd, cmd.payload) - r.recordCommandResult(rd.cfg.Name, result) - if err == nil && result.Status == "applied" && result.DeviceState == "controlled" { - armLease(leaseExpiresAt) - } else if err == nil && result.Status == "applied" && result.DeviceState == "default" { - clearLease() - } else if err != nil && result.Writes > 0 { - // A failed call may still have changed the device. End any old - // lease and restore the signed default before more control. - clearLease() - defaultCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defaultErr := r.defaultDriver(defaultCtx, rd, "command_failed_after_write") - cancel() - if defaultErr != nil { - rd.controlBlocked = true - err = errors.Join(err, fmt.Errorf("restore default after partial command: %w", defaultErr)) - } - } + var result DriverCommandResultV1 + var leaseExpiresAt time.Time + result, leaseExpiresAt, err = r.dispatchV2Command(commandCtx, rd, cmd.payload) + r.recordCommandResult(rd.cfg.Name, result) + if err == nil && result.Status == "applied" && result.DeviceState == "controlled" { + armLease(leaseExpiresAt) + rd.markCommandApplied() + } else if err == nil && result.Status == "applied" && result.DeviceState == "default" { + clearLease() + rd.markDefaultConfirmed() + clearRecoveryTimer() + } else if err != nil { + err = restoreAfterCommand(err) } } else { - err = rd.driver.Command(cmdCtx, cmd.payload) + err = rd.driver.Command(commandCtx, cmd.payload) + if err != nil { + err = restoreAfterCommand(err) + } else { + rd.markCommandApplied() + } } + finishCommand() case "default": err = r.defaultDriver(cmdCtx, rd, "host_request") if err == nil { clearLease() - rd.controlBlocked = false - } else if rd.policy != nil && rd.policy.IsControlV2() { - rd.controlBlocked = true + rd.markDefaultConfirmed() + clearRecoveryTimer() + } else { + scheduleRecovery() } } if cmd.result != nil { @@ -449,7 +651,9 @@ func (r *Registry) runLoop(rd *runningDriver) { if _, err := rd.driver.Poll(ctx); err != nil { pollFailed = true slog.Warn("driver poll failed", "name", rd.cfg.Name, "err", err) - r.tel.RecordDriverError(rd.cfg.Name, err.Error()) + if r.tel != nil { + r.tel.RecordDriverError(rd.cfg.Name, err.Error()) + } } else if r.tel != nil { // Bump TickCount so the loop is visibly alive in // /api/status, but DON'T touch LastSuccess — that @@ -462,16 +666,14 @@ func (r *Registry) runLoop(rd *runningDriver) { r.tel.RecordDriverTick(rd.cfg.Name) } if rd.policy != nil && rd.policy.IsControlV2() && !rd.leaseExpiresAt.IsZero() { - health := r.tel.DriverHealth(rd.cfg.Name) + var health *telemetry.DriverHealth + if r.tel != nil { + health = r.tel.DriverHealth(rd.cfg.Name) + } if pollFailed || health == nil || !health.IsOnline() { - defaultCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - err := r.defaultDriver(defaultCtx, rd, "driver_stale") - cancel() - if err != nil { - rd.controlBlocked = true + clearLease() + if err := attemptDefault("driver_stale"); err != nil { slog.Error("driver stale default mode failed; control blocked", "name", rd.cfg.Name, "err", err) - } else { - clearLease() } } } @@ -479,14 +681,14 @@ func (r *Registry) runLoop(rd *runningDriver) { interval = rd.env.PollInterval() timer.Reset(interval) case <-leaseC: - defaultCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - err := r.defaultDriver(defaultCtx, rd, "lease_expired") - cancel() clearLease() - if err != nil { - rd.controlBlocked = true + if err := attemptDefault("lease_expired"); err != nil { slog.Error("driver lease expiry default mode failed; control blocked", "name", rd.cfg.Name, "err", err) } + case <-recoveryC: + if err := attemptDefault("control_recovery"); err != nil { + slog.Error("driver default recovery failed; control remains blocked", "name", rd.cfg.Name, "err", err) + } } } } @@ -597,7 +799,16 @@ func (r *Registry) remove(name string, skipDefault bool) { return } delete(r.rec, name) + hook := r.lifecycleHook r.mu.Unlock() + // A legacy Lua command may be looping after it has already written the + // device. Cancel it before waiting for the lifecycle callback or stop + // signal, otherwise restart and shutdown can wait forever for runLoop. + rd.cancelLifecycle() + rd.cancelActiveCommand() + if hook != nil { + hook(name) + } rd.stop <- skipDefault <-rd.done if r.tel != nil { @@ -609,12 +820,21 @@ func (r *Registry) remove(name string, skipDefault bool) { // Send dispatches a command JSON blob to a specific driver. Blocks until the // driver's runLoop processes it or ctx expires. func (r *Registry) Send(ctx context.Context, name string, payload []byte) error { + if ctx == nil { + ctx = context.Background() + } r.mu.Lock() rd, ok := r.rec[name] r.mu.Unlock() if !ok { return fmt.Errorf("driver %q not found", name) } + if rd.cfg.ObserveOnly { + return ErrObserveOnly + } + if rd.controlIsBlocked() { + return ErrControlBlocked + } resCh := make(chan error, 1) select { case rd.cmdCh <- driverCmd{kind: "command", ctx: ctx, payload: payload, result: resCh}: @@ -636,6 +856,9 @@ func (r *Registry) Send(ctx context.Context, name string, payload []byte) error // path runs on every dispatch tick, so an unblocked send into a wedged // driver deadlocks the entire control loop. func (r *Registry) SendDefault(ctx context.Context, name string) error { + if ctx == nil { + ctx = context.Background() + } r.mu.Lock() rd, ok := r.rec[name] r.mu.Unlock() @@ -656,6 +879,18 @@ func (r *Registry) SendDefault(ctx context.Context, name string) error { } } +// ControlStatus returns the command safety state for the current driver +// generation without creating or changing any state. +func (r *Registry) ControlStatus(name string) (DriverControlStatus, bool) { + r.mu.Lock() + rd, ok := r.rec[name] + r.mu.Unlock() + if !ok { + return DriverControlStatus{}, false + } + return rd.controlStatus(), true +} + // Names returns the currently registered driver names. // Env returns the HostEnv for a driver, or nil if not registered. // Used by main to read identity (make/sn/mac/endpoint) after init. diff --git a/go/internal/drivers/registry_restart_test.go b/go/internal/drivers/registry_restart_test.go index 2031b1e1..502d3c26 100644 --- a/go/internal/drivers/registry_restart_test.go +++ b/go/internal/drivers/registry_restart_test.go @@ -125,6 +125,85 @@ func TestSendDefaultPassesCallerContextToRuntime(t *testing.T) { r.remove("d1", true) } +func waitRegistryMetric(t *testing.T, tel *telemetry.Store, driver, metric string, want float64) { + t.Helper() + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if got, _, ok := tel.LatestMetric(driver, metric); ok && got == want { + return + } + time.Sleep(10 * time.Millisecond) + } + got, _, ok := tel.LatestMetric(driver, metric) + t.Fatalf("%s/%s = %v/%v, want %v", driver, metric, got, ok, want) +} + +func TestRegistryCancelsLegacyCommandOnRestartAndShutdown(t *testing.T) { + src := ` +function driver_init(config) + host.set_poll_interval(1000) +end +function driver_poll() return 1000 end +function driver_command(action, w, cmd) + host.emit_metric("command_started", 1) + while true do end +end +function driver_default_mode() + host.emit_metric("default_called", 1) +end +` + path := writeTestDriver(t, src) + cfg := config.Driver{Name: "d1", Lua: path} + tel := telemetry.NewStore() + r := NewRegistry(tel) + if err := r.Add(context.Background(), cfg); err != nil { + t.Fatal(err) + } + + commandDone := make(chan error, 1) + go func() { + commandDone <- r.Send(context.Background(), "d1", []byte(`{"action":"loop"}`)) + }() + waitRegistryMetric(t, tel, "d1", "command_started", 1) + + restartDone := make(chan error, 1) + go func() { restartDone <- r.Restart(context.Background(), cfg) }() + select { + case err := <-restartDone: + if err != nil { + t.Fatalf("restart = %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("restart waited for a legacy Lua command that should have been cancelled") + } + select { + case <-commandDone: + case <-time.After(2 * time.Second): + t.Fatal("cancelled command did not return") + } + + commandDone = make(chan error, 1) + go func() { + commandDone <- r.Send(context.Background(), "d1", []byte(`{"action":"loop"}`)) + }() + waitRegistryMetric(t, tel, "d1", "command_started", 1) + shutdownDone := make(chan struct{}) + go func() { + r.ShutdownAll() + close(shutdownDone) + }() + select { + case <-shutdownDone: + case <-time.After(2 * time.Second): + t.Fatal("shutdown waited for a legacy Lua command that should have been cancelled") + } + select { + case <-commandDone: + case <-time.After(2 * time.Second): + t.Fatal("shutdown-cancelled command did not return") + } +} + // Reset all — used to test a series of adds / removes in the same // registry without the mocks carrying state across calls. func writeTestDriver(t *testing.T, src string) string { From 0f068164c74a1b5654e720a29c2fb48365f1665c Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Tue, 4 Aug 2026 11:45:12 +0200 Subject: [PATCH 05/10] fix: fail closed across driver lifecycle races --- go/internal/drivers/registry.go | 215 ++++++++++++++++--- go/internal/drivers/registry_restart_test.go | 136 ++++++++++++ 2 files changed, 326 insertions(+), 25 deletions(-) diff --git a/go/internal/drivers/registry.go b/go/internal/drivers/registry.go index 5ffea6ff..d6f9d136 100644 --- a/go/internal/drivers/registry.go +++ b/go/internal/drivers/registry.go @@ -22,11 +22,29 @@ var ( // ErrControlBlocked is returned until the driver's autonomous default has // completed successfully after an ambiguous or failed control command. ErrControlBlocked = errors.New("driver control is blocked until autonomous default is confirmed") + // ErrCommandMayHaveRun is returned when the caller canceled while the + // driver command had already started but its final result was not yet + // available. The registry restores the driver's default independently. + ErrCommandMayHaveRun = errors.New("driver command may have run") // ErrObserveOnly is returned when a configured telemetry-only driver is // reached through a generic command path instead of the API guard. ErrObserveOnly = errors.New("driver is observe_only and cannot be controlled") ) +type commandMayHaveRunError struct { + cause error +} + +func (e *commandMayHaveRunError) Error() string { + return fmt.Sprintf("%s: %v", ErrCommandMayHaveRun, e.cause) +} + +func (e *commandMayHaveRunError) Unwrap() error { return e.cause } + +func (e *commandMayHaveRunError) Is(target error) bool { + return target == ErrCommandMayHaveRun +} + const ( defaultRecoveryTimeout = 5 * time.Second defaultRetryInitial = 100 * time.Millisecond @@ -76,17 +94,19 @@ type Registry struct { // A nil sink keeps tests and legacy setups simple. CommandResultSink func(driverName string, result DriverCommandResultV1) - mu sync.Mutex - rec map[string]*runningDriver - nextGeneration uint64 - lifecycleHook func(name string) + mu sync.Mutex + rec map[string]*runningDriver + recoveryRequired map[string]bool + nextGeneration uint64 + lifecycleHook func(name string) } // NewRegistry builds a driver registry. func NewRegistry(tel *telemetry.Store) *Registry { return &Registry{ - tel: tel, - rec: map[string]*runningDriver{}, + tel: tel, + rec: map[string]*runningDriver{}, + recoveryRequired: map[string]bool{}, } } @@ -171,20 +191,22 @@ func driverInitConfigJSON(cfg config.Driver, troubleshootingMode bool) []byte { } type runningDriver struct { - driver driverRuntime - env *HostEnv - cfg config.Driver - policy *RuntimePolicy - leaseExpiresAt time.Time - generation uint64 - statusMu sync.RWMutex - controlBlocked bool - defaultConfirmed bool - recoveryPending bool - activeMu sync.Mutex - activeCancel context.CancelFunc - lifecycleCtx context.Context - lifecycleCancel context.CancelFunc + driver driverRuntime + env *HostEnv + cfg config.Driver + policy *RuntimePolicy + leaseExpiresAt time.Time + generation uint64 + statusMu sync.RWMutex + controlBlocked bool + defaultConfirmed bool + recoveryPending bool + activeMu sync.Mutex + activeCancel context.CancelFunc + lifecycleCtx context.Context + lifecycleCancel context.CancelFunc + shutdownMu sync.Mutex + shutdownDefaultErr error // Poll loop coordination cmdCh chan driverCmd stop chan bool @@ -268,11 +290,66 @@ func (rd *runningDriver) cancelLifecycle() { } } +func (rd *runningDriver) setShutdownDefaultError(err error) { + rd.shutdownMu.Lock() + rd.shutdownDefaultErr = err + rd.shutdownMu.Unlock() +} + +func (rd *runningDriver) shutdownDefaultFailed() bool { + rd.shutdownMu.Lock() + failed := rd.shutdownDefaultErr != nil + rd.shutdownMu.Unlock() + return failed +} + +type commandState struct { + mu sync.Mutex + started bool + completed bool + err error +} + +func (s *commandState) markStarted() { + s.mu.Lock() + s.started = true + s.mu.Unlock() +} + +func (s *commandState) finish(err error) { + s.mu.Lock() + s.completed = true + s.err = err + s.mu.Unlock() +} + +func (s *commandState) snapshot() (err error, started, completed bool) { + s.mu.Lock() + err, started, completed = s.err, s.started, s.completed + s.mu.Unlock() + return err, started, completed +} + +func commandContextError(requestCtx, executionCtx context.Context) error { + if requestCtx != nil { + if err := requestCtx.Err(); err != nil { + return err + } + } + if executionCtx != nil { + if err := executionCtx.Err(); err != nil { + return err + } + } + return nil +} + type driverCmd struct { kind string ctx context.Context payload []byte result chan error + state *commandState } // Add spawns a driver. Returns error if the driver config is invalid or @@ -442,8 +519,20 @@ func (r *Registry) Add(ctx context.Context, cfg config.Driver) error { r.mu.Lock() r.nextGeneration++ rd.generation = r.nextGeneration + inheritsRecovery := r.recoveryRequired[cfg.Name] if policy != nil && policy.IsControlV2() { rd.defaultConfirmed = true + // The v2 startup default above is a confirmed recovery for a + // replacement generation. It does not need the legacy retry path. + if inheritsRecovery { + delete(r.recoveryRequired, cfg.Name) + } + } else if inheritsRecovery { + // Legacy drivers have no host-owned startup default call. Keep the + // new generation unavailable until its run loop confirms one. + rd.controlBlocked = true + rd.defaultConfirmed = false + rd.recoveryPending = true } r.rec[cfg.Name] = rd r.mu.Unlock() @@ -542,6 +631,9 @@ func (r *Registry) runLoop(rd *runningDriver) { } } defer clearRecoveryTimer() + if rd.controlIsBlocked() { + scheduleRecovery() + } attemptDefault := func(reason string) error { defaultCtx, cancel := context.WithTimeout(context.Background(), defaultRecoveryTimeout) defaultErr := r.defaultDriver(defaultCtx, rd, reason) @@ -553,23 +645,27 @@ func (r *Registry) runLoop(rd *runningDriver) { rd.markDefaultConfirmed() clearLease() clearRecoveryTimer() + r.clearRecoveryRequired(rd.cfg.Name, rd) return nil } restoreAfterCommand := func(commandErr error) error { clearLease() defaultErr := attemptDefault("command_failed") + commandOutcome := &commandMayHaveRunError{cause: commandErr} if defaultErr != nil { - return errors.Join(commandErr, fmt.Errorf("%w: restore default after ambiguous command: %v", ErrControlBlocked, defaultErr)) + return errors.Join(commandOutcome, fmt.Errorf("%w: restore default after ambiguous command: %v", ErrControlBlocked, defaultErr)) } - return commandErr + return commandOutcome } for { select { case skipDefault := <-rd.stop: if !skipDefault { shutdownCtx, cancel := context.WithTimeout(context.Background(), defaultRecoveryTimeout) - if err := r.defaultDriver(shutdownCtx, rd, "host_shutdown"); err != nil { - slog.Error("driver failed to enter default mode during shutdown", "name", rd.cfg.Name, "err", err) + defaultErr := r.defaultDriver(shutdownCtx, rd, "host_shutdown") + rd.setShutdownDefaultError(defaultErr) + if defaultErr != nil { + slog.Error("driver failed to enter default mode during shutdown", "name", rd.cfg.Name, "err", defaultErr) } cancel() } @@ -608,12 +704,22 @@ func (r *Registry) runLoop(rd *runningDriver) { err = ErrControlBlocked break } + if cancelErr := cmdCtx.Err(); cancelErr != nil { + err = cancelErr + break + } + if cmd.state != nil { + cmd.state.markStarted() + } commandCtx, finishCommand := rd.beginCommand(cmdCtx) if rd.policy != nil && rd.policy.IsControlV2() { var result DriverCommandResultV1 var leaseExpiresAt time.Time result, leaseExpiresAt, err = r.dispatchV2Command(commandCtx, rd, cmd.payload) r.recordCommandResult(rd.cfg.Name, result) + if err == nil { + err = commandContextError(cmdCtx, commandCtx) + } if err == nil && result.Status == "applied" && result.DeviceState == "controlled" { armLease(leaseExpiresAt) rd.markCommandApplied() @@ -626,6 +732,9 @@ func (r *Registry) runLoop(rd *runningDriver) { } } else { err = rd.driver.Command(commandCtx, cmd.payload) + if err == nil { + err = commandContextError(cmdCtx, commandCtx) + } if err != nil { err = restoreAfterCommand(err) } else { @@ -639,10 +748,14 @@ func (r *Registry) runLoop(rd *runningDriver) { clearLease() rd.markDefaultConfirmed() clearRecoveryTimer() + r.clearRecoveryRequired(rd.cfg.Name, rd) } else { scheduleRecovery() } } + if cmd.state != nil { + cmd.state.finish(err) + } if cmd.result != nil { cmd.result <- err } @@ -791,6 +904,20 @@ func (r *Registry) RemoveProbe(name string) { r.remove(name, true) } +// clearRecoveryRequired removes the name-level lifecycle gate only when the +// generation that confirmed default is still the current one. If a new +// generation was added while the old one was shutting down, that replacement +// must keep its own recovery gate and retry. +func (r *Registry) clearRecoveryRequired(name string, rd *runningDriver) { + r.mu.Lock() + if current, ok := r.rec[name]; ok && current != rd { + r.mu.Unlock() + return + } + delete(r.recoveryRequired, name) + r.mu.Unlock() +} + func (r *Registry) remove(name string, skipDefault bool) { r.mu.Lock() rd, ok := r.rec[name] @@ -798,6 +925,15 @@ func (r *Registry) remove(name string, skipDefault bool) { r.mu.Unlock() return } + if !skipDefault { + if r.recoveryRequired == nil { + r.recoveryRequired = make(map[string]bool) + } + // Gate a replacement before the old generation is canceled. A + // concurrent Add must not become controllable while shutdown default + // is still unknown. + r.recoveryRequired[name] = true + } delete(r.rec, name) hook := r.lifecycleHook r.mu.Unlock() @@ -811,6 +947,21 @@ func (r *Registry) remove(name string, skipDefault bool) { } rd.stop <- skipDefault <-rd.done + if !skipDefault { + if rd.shutdownDefaultFailed() { + // A recovery timer may have succeeded while shutdown was + // waiting. The shutdown result is the final lifecycle boundary, + // so reassert the gate if that default failed. + r.mu.Lock() + if r.recoveryRequired == nil { + r.recoveryRequired = make(map[string]bool) + } + r.recoveryRequired[name] = true + r.mu.Unlock() + } else { + r.clearRecoveryRequired(name, rd) + } + } if r.tel != nil { r.tel.Remove(name) } @@ -823,6 +974,9 @@ func (r *Registry) Send(ctx context.Context, name string, payload []byte) error if ctx == nil { ctx = context.Background() } + if err := ctx.Err(); err != nil { + return err + } r.mu.Lock() rd, ok := r.rec[name] r.mu.Unlock() @@ -835,9 +989,13 @@ func (r *Registry) Send(ctx context.Context, name string, payload []byte) error if rd.controlIsBlocked() { return ErrControlBlocked } + if err := ctx.Err(); err != nil { + return err + } resCh := make(chan error, 1) + state := &commandState{} select { - case rd.cmdCh <- driverCmd{kind: "command", ctx: ctx, payload: payload, result: resCh}: + case rd.cmdCh <- driverCmd{kind: "command", ctx: ctx, payload: payload, result: resCh, state: state}: case <-ctx.Done(): return ctx.Err() } @@ -845,6 +1003,13 @@ func (r *Registry) Send(ctx context.Context, name string, payload []byte) error case err := <-resCh: return err case <-ctx.Done(): + err, started, completed := state.snapshot() + if completed { + return err + } + if started { + return &commandMayHaveRunError{cause: ctx.Err()} + } return ctx.Err() } } diff --git a/go/internal/drivers/registry_restart_test.go b/go/internal/drivers/registry_restart_test.go index 502d3c26..daea5cc3 100644 --- a/go/internal/drivers/registry_restart_test.go +++ b/go/internal/drivers/registry_restart_test.go @@ -93,6 +93,34 @@ func (r *ctxAwareRuntime) DefaultMode(ctx context.Context) error { func (r *ctxAwareRuntime) Cleanup(ctx context.Context) error { return nil } func (r *ctxAwareRuntime) Env() *HostEnv { return r.env } +type cancelAfterStartRuntime struct { + env *HostEnv + started chan struct{} + sideEffect chan struct{} + defaulted chan struct{} + startOnce sync.Once + effectOnce sync.Once + defaultOnce sync.Once +} + +func (r *cancelAfterStartRuntime) Init(ctx context.Context, configJSON []byte) error { return nil } +func (r *cancelAfterStartRuntime) Poll(ctx context.Context) (time.Duration, error) { + return time.Hour, nil +} +func (r *cancelAfterStartRuntime) Command(ctx context.Context, cmdJSON []byte) error { + r.startOnce.Do(func() { close(r.started) }) + // The command has crossed the driver boundary and may have written hardware. + r.effectOnce.Do(func() { close(r.sideEffect) }) + <-ctx.Done() + return nil +} +func (r *cancelAfterStartRuntime) DefaultMode(ctx context.Context) error { + r.defaultOnce.Do(func() { close(r.defaulted) }) + return nil +} +func (r *cancelAfterStartRuntime) Cleanup(ctx context.Context) error { return nil } +func (r *cancelAfterStartRuntime) Env() *HostEnv { return r.env } + func TestSendDefaultPassesCallerContextToRuntime(t *testing.T) { tel := telemetry.NewStore() r := NewRegistry(tel) @@ -125,6 +153,114 @@ func TestSendDefaultPassesCallerContextToRuntime(t *testing.T) { r.remove("d1", true) } +func TestRegistryCancelAfterCommandStartedRestoresDefault(t *testing.T) { + tel := telemetry.NewStore() + r := NewRegistry(tel) + rt := &cancelAfterStartRuntime{ + env: NewHostEnv("d1", tel), + started: make(chan struct{}), + sideEffect: make(chan struct{}), + defaulted: make(chan struct{}), + } + lifecycleCtx, lifecycleCancel := context.WithCancel(context.Background()) + rd := &runningDriver{ + driver: rt, + env: rt.env, + cfg: config.Driver{Name: "d1"}, + lifecycleCtx: lifecycleCtx, + lifecycleCancel: lifecycleCancel, + cmdCh: make(chan driverCmd, 1), + stop: make(chan bool, 1), + done: make(chan struct{}), + } + r.mu.Lock() + r.nextGeneration++ + rd.generation = r.nextGeneration + r.rec["d1"] = rd + r.mu.Unlock() + go r.runLoop(rd) + t.Cleanup(func() { r.remove("d1", true) }) + + ctx, cancel := context.WithCancel(context.Background()) + commandDone := make(chan error, 1) + go func() { commandDone <- r.Send(ctx, "d1", []byte(`{"action":"set"}`)) }() + select { + case <-rt.started: + case <-time.After(time.Second): + t.Fatal("command did not start") + } + select { + case <-rt.sideEffect: + case <-time.After(time.Second): + t.Fatal("command did not cross the driver boundary") + } + cancel() + select { + case err := <-commandDone: + if !errors.Is(err, context.Canceled) { + t.Fatalf("Send after started-command cancel = %v, want context canceled", err) + } + if !errors.Is(err, ErrCommandMayHaveRun) { + t.Fatalf("Send after started-command cancel = %v, want may-have-run outcome", err) + } + case <-time.After(time.Second): + t.Fatal("Send did not return after caller cancellation") + } + select { + case <-rt.defaulted: + case <-time.After(time.Second): + t.Fatal("started command did not trigger autonomous default") + } +} + +func TestRegistryRestartDefaultFailureBlocksNewGeneration(t *testing.T) { + src := ` +local defaults = 0 +function driver_init(config) + host.set_poll_interval(1000) +end +function driver_poll() return 1000 end +function driver_command(action, w, cmd) + host.emit_metric("command_called", 1) + return true +end +function driver_default_mode() + defaults = defaults + 1 + host.emit_metric("default_attempt", defaults) + if defaults == 1 then return false end +end +` + path := writeTestDriver(t, src) + cfg := config.Driver{Name: "d1", Lua: path} + tel := telemetry.NewStore() + r := NewRegistry(tel) + if err := r.Add(context.Background(), cfg); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { r.remove("d1", true) }) + + if err := r.Restart(context.Background(), cfg); err != nil { + t.Fatalf("restart = %v", err) + } + status, ok := r.ControlStatus("d1") + if !ok || !status.Blocked || !status.RecoveryPending { + t.Fatalf("new generation status after failed restart default = %+v, running=%v", status, ok) + } + if err := r.Send(context.Background(), "d1", []byte(`{"action":"set"}`)); !errors.Is(err, ErrControlBlocked) { + t.Fatalf("control after failed restart default = %v, want ErrControlBlocked", err) + } + + waitRegistryMetric(t, tel, "d1", "default_attempt", 2) + status, ok = r.ControlStatus("d1") + if !ok || status.Blocked || !status.DefaultConfirmed || status.RecoveryPending { + t.Fatalf("new generation status after recovery = %+v, running=%v", status, ok) + } + if err := r.Send(context.Background(), "d1", []byte(`{"action":"set"}`)); err != nil { + t.Fatalf("control after confirmed recovery = %v", err) + } + waitRegistryMetric(t, tel, "d1", "command_called", 1) +} + func waitRegistryMetric(t *testing.T, tel *telemetry.Store, driver, metric string, want float64) { t.Helper() deadline := time.Now().Add(2 * time.Second) From 008e468d5f3e2c2a84101616e38039654de95582 Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Tue, 4 Aug 2026 12:20:55 +0200 Subject: [PATCH 06/10] fix: require legacy startup default before control --- .changeset/driver-control-path.md | 6 +- go/internal/api/api_driver_control_test.go | 18 +++ go/internal/api/api_drivers_debug.go | 2 +- go/internal/drivers/registry.go | 51 +++++- go/internal/drivers/registry_restart_test.go | 162 ++++++++++++++++++- 5 files changed, 226 insertions(+), 13 deletions(-) diff --git a/.changeset/driver-control-path.md b/.changeset/driver-control-path.md index 627b64bb..4b627c67 100644 --- a/.changeset/driver-control-path.md +++ b/.changeset/driver-control-path.md @@ -27,5 +27,7 @@ rather than a 200 for a command the Lua silently ignored. Every hold ends by itself, and ending means calling the driver's own `driver_default_mode` rather than writing a value Core invented: only the driver knows what neutral is. Default 4 h, maximum 24 h, and nothing survives a -restart. An offset left behind by a browser tab that closed is a house heated -wrong for weeks. +restart. On process start or driver re-add, a legacy driver must also confirm +that default before control opens; a failed confirmation keeps control blocked +and retries with a bounded backoff. An offset left behind by a browser tab that +closed is a house heated wrong for weeks. diff --git a/go/internal/api/api_driver_control_test.go b/go/internal/api/api_driver_control_test.go index cb6e075a..a7574eca 100644 --- a/go/internal/api/api_driver_control_test.go +++ b/go/internal/api/api_driver_control_test.go @@ -37,6 +37,7 @@ const controlProbeLua = `DRIVER = { local applied = nil local defaulted = 0 +local startup_default = true function driver_init(config) host.set_make("Probe") @@ -60,6 +61,11 @@ function driver_command(action, power_w, cmd) end function driver_default_mode() + if startup_default then + startup_default = false + applied = 0 + return true + end defaulted = defaulted + 1 applied = 0 end @@ -116,6 +122,7 @@ const controlSafetyProbeLua = `DRIVER = { local applied = nil local defaulted = 0 +local startup_default = true function driver_init(config) host.set_make("Probe safety") @@ -142,6 +149,11 @@ function driver_command(action, power_w, cmd) end function driver_default_mode() + if startup_default then + startup_default = false + applied = 0 + return true + end defaulted = defaulted + 1 host.emit_metric("default_started", defaulted, "n") host.sleep(200) @@ -160,6 +172,7 @@ const controlRecoveryProbeLua = `DRIVER = { local applied = nil local defaults = 0 +local startup_default = true function driver_init(config) host.set_make("Probe recovery") @@ -180,6 +193,11 @@ function driver_command(action, power_w, cmd) end function driver_default_mode() + if startup_default then + startup_default = false + applied = 0 + return true + end defaults = defaults + 1 host.emit_metric("default_attempt", defaults, "n") if defaults == 1 then return false end diff --git a/go/internal/api/api_drivers_debug.go b/go/internal/api/api_drivers_debug.go index 85ecc17d..a4a1b7ca 100644 --- a/go/internal/api/api_drivers_debug.go +++ b/go/internal/api/api_drivers_debug.go @@ -277,7 +277,7 @@ func (s *Server) handleDriverTest(w http.ResponseWriter, r *http.Request) { ctx, cancel := context.WithTimeout(r.Context(), 12*time.Second) defer cancel() started := time.Now() - if err := reg.Add(ctx, cfg); err != nil { + if err := reg.AddProbe(ctx, cfg); err != nil { writeJSON(w, 200, driverProbeResp{ Name: displayName, OK: false, diff --git a/go/internal/drivers/registry.go b/go/internal/drivers/registry.go index d6f9d136..1cdb7e11 100644 --- a/go/internal/drivers/registry.go +++ b/go/internal/drivers/registry.go @@ -352,9 +352,28 @@ type driverCmd struct { state *commandState } -// Add spawns a driver. Returns error if the driver config is invalid or -// the Lua script can't be loaded. +// Add spawns an operational driver. Before a legacy driver becomes +// controllable, it must confirm its autonomous default once for this +// process. A failed default leaves the driver registered but blocked so the +// recovery loop can retry without opening a control window. func (r *Registry) Add(ctx context.Context, cfg config.Driver) error { + return r.add(ctx, cfg, true) +} + +// AddProbe spawns a short-lived, read-only probe. Probes must not write a +// device's default mode during connection testing, and Send rejects them via +// the observe-only flag on the private config copy. +func (r *Registry) AddProbe(ctx context.Context, cfg config.Driver) error { + cfg.ObserveOnly = true + return r.add(ctx, cfg, false) +} + +// add is the shared driver construction path. startupDefault is false only +// for connection probes; operational drivers use the startup safety gate. +func (r *Registry) add(ctx context.Context, cfg config.Driver, startupDefault bool) error { + if ctx == nil { + ctx = context.Background() + } r.mu.Lock() if _, exists := r.rec[cfg.Name]; exists { r.mu.Unlock() @@ -494,7 +513,7 @@ func (r *Registry) Add(ctx context.Context, cfg config.Driver) error { drv.Cleanup(ctx) return fmt.Errorf("driver_init: %w", err) } - if policy != nil && policy.IsControlV2() { + if startupDefault && !cfg.ObserveOnly && policy != nil && policy.IsControlV2() { v2 := drv.(controlV2Runtime) result, defaultErr := v2.DefaultModeV2(ctx, newControlID("default"), "host_start", time.Now()) r.recordCommandResult(cfg.Name, result) @@ -503,6 +522,15 @@ func (r *Registry) Add(ctx context.Context, cfg config.Driver) error { return fmt.Errorf("driver_default_mode_v2 on startup: %w", defaultErr) } } + var startupDefaultErr error + if startupDefault && !cfg.ObserveOnly && (policy == nil || !policy.IsControlV2()) { + defaultCtx, cancel := context.WithTimeout(ctx, defaultRecoveryTimeout) + startupDefaultErr = drv.DefaultMode(defaultCtx) + cancel() + if startupDefaultErr != nil { + slog.Error("driver failed to confirm autonomous default at startup; control remains blocked", "name", cfg.Name, "err", startupDefaultErr) + } + } lifecycleCtx, lifecycleCancel := context.WithCancel(context.Background()) rd := &runningDriver{ @@ -520,16 +548,27 @@ func (r *Registry) Add(ctx context.Context, cfg config.Driver) error { r.nextGeneration++ rd.generation = r.nextGeneration inheritsRecovery := r.recoveryRequired[cfg.Name] - if policy != nil && policy.IsControlV2() { + if startupDefault && !cfg.ObserveOnly && policy != nil && policy.IsControlV2() { rd.defaultConfirmed = true // The v2 startup default above is a confirmed recovery for a // replacement generation. It does not need the legacy retry path. if inheritsRecovery { delete(r.recoveryRequired, cfg.Name) } + } else if startupDefault && !cfg.ObserveOnly && (policy == nil || !policy.IsControlV2()) { + if startupDefaultErr == nil { + rd.defaultConfirmed = true + if inheritsRecovery { + delete(r.recoveryRequired, cfg.Name) + } + } else { + rd.controlBlocked = true + rd.defaultConfirmed = false + rd.recoveryPending = true + } } else if inheritsRecovery { - // Legacy drivers have no host-owned startup default call. Keep the - // new generation unavailable until its run loop confirms one. + // A replacement that cannot run a startup default (observe_only or a + // probe) remains unavailable if its predecessor left a lifecycle gate. rd.controlBlocked = true rd.defaultConfirmed = false rd.recoveryPending = true diff --git a/go/internal/drivers/registry_restart_test.go b/go/internal/drivers/registry_restart_test.go index daea5cc3..f608b29d 100644 --- a/go/internal/drivers/registry_restart_test.go +++ b/go/internal/drivers/registry_restart_test.go @@ -261,6 +261,153 @@ end waitRegistryMetric(t, tel, "d1", "command_called", 1) } +func TestFreshRegistryBlocksLegacyControlUntilStartupDefault(t *testing.T) { + src := ` +local defaults = 0 +local fail_first_default = false +function driver_init(config) + if config ~= nil and config.process == "new" then + fail_first_default = true + end + host.set_poll_interval(1000) +end +function driver_poll() return 1000 end +function driver_command(action, w, cmd) + host.emit_metric("command_called", 1) + return true +end +function driver_default_mode() + defaults = defaults + 1 + host.emit_metric("default_attempt", defaults) + if fail_first_default and defaults == 1 then return false end +end +` + path := writeTestDriver(t, src) + tel := telemetry.NewStore() + + // The first process may have crossed the driver boundary before it + // stopped. RemoveProbe models that loss of in-memory recovery state: it + // tears down the old runtime without sending another default. + old := NewRegistry(tel) + oldCfg := config.Driver{ + Name: "d1", + Lua: path, + Config: map[string]any{"process": "old"}, + } + if err := old.Add(context.Background(), oldCfg); err != nil { + t.Fatal(err) + } + if err := old.Send(context.Background(), "d1", []byte(`{"action":"set"}`)); err != nil { + t.Fatalf("old process control = %v", err) + } + waitRegistryMetric(t, tel, "d1", "command_called", 1) + old.RemoveProbe("d1") + + fresh := NewRegistry(tel) + freshCfg := config.Driver{ + Name: "d1", + Lua: path, + Config: map[string]any{"process": "new"}, + } + if err := fresh.Add(context.Background(), freshCfg); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { fresh.remove("d1", true) }) + + status, ok := fresh.ControlStatus("d1") + if !ok || !status.Blocked || !status.RecoveryPending || status.DefaultConfirmed { + t.Fatalf("fresh process opened control before startup default: status=%+v, running=%v", status, ok) + } + if err := fresh.Send(context.Background(), "d1", []byte(`{"action":"set"}`)); !errors.Is(err, ErrControlBlocked) { + t.Fatalf("control during startup-default recovery = %v, want ErrControlBlocked", err) + } + + waitRegistryMetric(t, tel, "d1", "default_attempt", 2) + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + status, ok = fresh.ControlStatus("d1") + if ok && !status.Blocked && status.DefaultConfirmed && !status.RecoveryPending { + break + } + time.Sleep(10 * time.Millisecond) + } + if !ok || status.Blocked || !status.DefaultConfirmed || status.RecoveryPending { + t.Fatalf("fresh process status after startup-default recovery = %+v, running=%v", status, ok) + } + if err := fresh.Send(context.Background(), "d1", []byte(`{"action":"set"}`)); err != nil { + t.Fatalf("control after confirmed startup default = %v", err) + } + waitRegistryMetric(t, tel, "d1", "command_called", 1) +} + +func TestFreshRegistryConfirmsSuccessfulLegacyStartupDefault(t *testing.T) { + src := ` +function driver_init(config) + host.set_poll_interval(1000) +end +function driver_poll() return 1000 end +function driver_command(action, w, cmd) + host.emit_metric("command_called", 1) + return true +end +function driver_default_mode() + host.emit_metric("startup_default", 1) +end +` + path := writeTestDriver(t, src) + tel := telemetry.NewStore() + r := NewRegistry(tel) + cfg := config.Driver{Name: "d1", Lua: path} + if err := r.Add(context.Background(), cfg); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { r.remove("d1", true) }) + + status, ok := r.ControlStatus("d1") + if !ok || status.Blocked || !status.DefaultConfirmed || status.RecoveryPending { + t.Fatalf("legacy control opened without confirmed startup default: status=%+v, running=%v", status, ok) + } + waitRegistryMetric(t, tel, "d1", "startup_default", 1) + if err := r.Send(context.Background(), "d1", []byte(`{"action":"set"}`)); err != nil { + t.Fatalf("control after successful startup default = %v", err) + } + waitRegistryMetric(t, tel, "d1", "command_called", 1) +} + +func TestObserveOnlySkipsStartupDefaultAndRejectsControl(t *testing.T) { + src := ` +function driver_init(config) + host.set_poll_interval(1000) +end +function driver_poll() return 1000 end +function driver_command(action, w, cmd) + host.emit_metric("command_called", 1) + return true +end +function driver_default_mode() + host.emit_metric("startup_default", 1) +end +` + path := writeTestDriver(t, src) + tel := telemetry.NewStore() + r := NewRegistry(tel) + cfg := config.Driver{Name: "d1", Lua: path, ObserveOnly: true} + if err := r.Add(context.Background(), cfg); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { r.remove("d1", true) }) + + if err := r.Send(context.Background(), "d1", []byte(`{"action":"set"}`)); !errors.Is(err, ErrObserveOnly) { + t.Fatalf("observe_only Send = %v, want ErrObserveOnly", err) + } + if _, _, ok := tel.LatestMetric("d1", "startup_default"); ok { + t.Fatal("observe_only Add invoked driver_default_mode") + } + if _, _, ok := tel.LatestMetric("d1", "command_called"); ok { + t.Fatal("observe_only Send reached driver_command") + } +} + func waitRegistryMetric(t *testing.T, tel *telemetry.Store, driver, metric string, want float64) { t.Helper() deadline := time.Now().Add(2 * time.Second) @@ -276,12 +423,17 @@ func waitRegistryMetric(t *testing.T, tel *telemetry.Store, driver, metric strin func TestRegistryCancelsLegacyCommandOnRestartAndShutdown(t *testing.T) { src := ` +local command_count = 0 function driver_init(config) + if config ~= nil and config.phase == "restart" then + command_count = 1 + end host.set_poll_interval(1000) end function driver_poll() return 1000 end function driver_command(action, w, cmd) - host.emit_metric("command_started", 1) + command_count = command_count + 1 + host.emit_metric("command_started", command_count) while true do end end function driver_default_mode() @@ -289,7 +441,7 @@ function driver_default_mode() end ` path := writeTestDriver(t, src) - cfg := config.Driver{Name: "d1", Lua: path} + cfg := config.Driver{Name: "d1", Lua: path, Config: map[string]any{"phase": "initial"}} tel := telemetry.NewStore() r := NewRegistry(tel) if err := r.Add(context.Background(), cfg); err != nil { @@ -303,7 +455,9 @@ end waitRegistryMetric(t, tel, "d1", "command_started", 1) restartDone := make(chan error, 1) - go func() { restartDone <- r.Restart(context.Background(), cfg) }() + restartCfg := cfg + restartCfg.Config = map[string]any{"phase": "restart"} + go func() { restartDone <- r.Restart(context.Background(), restartCfg) }() select { case err := <-restartDone: if err != nil { @@ -322,7 +476,7 @@ end go func() { commandDone <- r.Send(context.Background(), "d1", []byte(`{"action":"loop"}`)) }() - waitRegistryMetric(t, tel, "d1", "command_started", 1) + waitRegistryMetric(t, tel, "d1", "command_started", 2) shutdownDone := make(chan struct{}) go func() { r.ShutdownAll() From efd19fb9b233155f7a837f8b845d42c3617f6d9a Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Tue, 4 Aug 2026 12:54:02 +0200 Subject: [PATCH 07/10] fix: close driver control generation safety gaps --- go/internal/api/api.go | 6 + go/internal/api/api_driver_control.go | 62 +++---- go/internal/api/api_driver_control_test.go | 171 +++++++++++++++++++ go/internal/drivers/lua.go | 11 ++ go/internal/drivers/registry.go | 53 ++++-- go/internal/drivers/registry_restart_test.go | 63 +++++++ 6 files changed, 326 insertions(+), 40 deletions(-) diff --git a/go/internal/api/api.go b/go/internal/api/api.go index 430730e8..9b05be83 100644 --- a/go/internal/api/api.go +++ b/go/internal/api/api.go @@ -212,6 +212,12 @@ type Server struct { controlStateMu sync.Mutex controlStates map[string]*controlDriverState + // beforeDriverControlSend is a package-test seam for reproducing a + // lifecycle change between request validation and registry dispatch. It is + // nil in production; SendWithGeneration still binds every real dispatch to + // the selected running generation. + beforeDriverControlSend func() + versionUpdateMu sync.Mutex driverUpdateMu sync.Mutex backupMu sync.Mutex diff --git a/go/internal/api/api_driver_control.go b/go/internal/api/api_driver_control.go index ca62de4e..9d9dde2f 100644 --- a/go/internal/api/api_driver_control.go +++ b/go/internal/api/api_driver_control.go @@ -23,6 +23,7 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "math" "net/http" "sync" @@ -109,21 +110,6 @@ func (s *Server) handleDriverControl(w http.ResponseWriter, r *http.Request) { writeJSON(w, 503, map[string]string{"error": "driver registry not available"}) return } - status, ok := s.deps.Registry.ControlStatus(name) - if !ok { - writeJSON(w, http.StatusNotFound, map[string]string{"error": "driver not running"}) - return - } - if status.Blocked { - writeJSON(w, http.StatusConflict, map[string]any{ - "error": drivers.ErrControlBlocked.Error(), - "control_blocked": true, - "default_confirmed": false, - "recovery_pending": status.RecoveryPending, - }) - return - } - applied, err := decodeControlValue(req.Value, control.Input) if err != nil { writeJSON(w, http.StatusBadRequest, map[string]string{"error": err.Error()}) @@ -151,16 +137,22 @@ func (s *Server) handleDriverControl(w http.ResponseWriter, r *http.Request) { seconds = maxControlHoldSeconds } - // Reserve the hold before dispatch. Registry.Send can return after the - // request is canceled even while the driver is still applying the command; - // the reservation guarantees that an ambiguous result still has a bounded - // safety path. The per-driver lock also keeps expiry/default from racing a - // replacement command. + // Allow lifecycle tests to pause at the same boundary as a real request. + // The registry lookup happens after this hook, so a restart here cannot make + // the request carry a stale generation into the hold. + if s.beforeDriverControlSend != nil { + s.beforeDriverControlSend() + } + + // Keep command dispatch and hold transitions under one per-driver lock. + // Registry.SendWithGeneration selects the concrete running instance and + // returns its generation, so expiry can never pair a new command with an + // old status snapshot. state := s.controlState(name) state.mu.Lock() defer state.mu.Unlock() - hold := s.armControlHoldLocked(name, state, control.ID, applied, status.Generation, time.Duration(seconds)*time.Second) - if err := s.deps.Registry.Send(r.Context(), name, body); err != nil { + generation, err := s.deps.Registry.SendWithGeneration(r.Context(), name, body) + if err != nil { s.clearControlHoldLocked(state) if errors.Is(err, drivers.ErrObserveOnly) { writeJSON(w, http.StatusForbidden, map[string]any{ @@ -170,17 +162,23 @@ func (s *Server) handleDriverControl(w http.ResponseWriter, r *http.Request) { return } if errors.Is(err, drivers.ErrControlBlocked) { + status, _ := s.deps.Registry.ControlStatus(name) writeJSON(w, http.StatusConflict, map[string]any{ "error": err.Error(), "control_blocked": true, "default_confirmed": false, - "recovery_pending": true, + "recovery_pending": status.RecoveryPending, }) return } + if _, running := s.deps.Registry.ControlStatus(name); !running { + writeJSON(w, http.StatusNotFound, map[string]string{"error": "driver not running"}) + return + } writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) return } + hold := s.armControlHoldLocked(name, state, control.ID, applied, generation, time.Duration(seconds)*time.Second) writeJSON(w, 200, map[string]any{ "control": control.ID, @@ -275,7 +273,11 @@ func (s *Server) armControlHoldLocked(name string, state *controlDriverState, co ExpiresAt: time.Now().Add(d).UnixMilli(), Generation: generation, } - hold.timer = time.AfterFunc(d, func() { s.expireControlHold(name, state, hold) }) + hold.timer = time.AfterFunc(d, func() { + if err := s.expireControlHold(name, state, hold); err != nil { + slog.Error("control hold expiry default failed; registry recovery gate remains active", "driver", name, "err", err) + } + }) state.hold = hold return hold } @@ -291,26 +293,26 @@ func (s *Server) clearControlHoldLocked(state *controlDriverState) { // first: a hold that was replaced or released already had its timer stopped, // but a timer that had begun firing cannot be stopped, and defaulting a // driver that an operator has just set again is the one wrong answer here. -func (s *Server) expireControlHold(name string, state *controlDriverState, fired *controlHold) { +func (s *Server) expireControlHold(name string, state *controlDriverState, fired *controlHold) error { state.mu.Lock() defer state.mu.Unlock() if state.hold != fired { - return + return nil } - if s.deps.Registry == nil { + if s.deps == nil || s.deps.Registry == nil { s.clearControlHoldLocked(state) - return + return errors.New("driver registry not available") } status, ok := s.deps.Registry.ControlStatus(name) if !ok || status.Generation != fired.Generation { // A stopped generation must never default a replacement instance. s.clearControlHoldLocked(state) - return + return nil } s.clearControlHoldLocked(state) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - _ = s.sendDefaultLocked(ctx, name) + return s.sendDefaultLocked(ctx, name) } // SendDriverDefault is the shared safety path for watchdogs and API release. diff --git a/go/internal/api/api_driver_control_test.go b/go/internal/api/api_driver_control_test.go index a7574eca..c74587de 100644 --- a/go/internal/api/api_driver_control_test.go +++ b/go/internal/api/api_driver_control_test.go @@ -206,6 +206,47 @@ function driver_default_mode() end ` +const controlExpiryRecoveryProbeLua = `DRIVER = { + id = "probe_expiry_recovery", + name = "Probe expiry recovery", + version = "1.0.0", + controls = { + { id = "set_offset", input = { type = "number", min = -3, max = 3 } }, + }, +} + +local applied = nil +local defaults = 0 +local startup_default = true + +function driver_init(config) + host.set_poll_interval(100) +end + +function driver_poll() + if applied ~= nil then host.emit_metric("applied", applied, "n") end + return 100 +end + +function driver_command(action, power_w, cmd) + applied = tonumber(cmd and (cmd.value or cmd.offset)) + return true +end + +function driver_default_mode() + if startup_default then + startup_default = false + applied = 0 + return true + end + defaults = defaults + 1 + host.emit_metric("default_attempt", defaults, "n") + if defaults == 1 then return false end + if defaults == 2 then host.sleep(500) end + applied = 0 +end +` + func controlServer(t *testing.T) (*Server, *telemetry.Store) { return controlServerWithLua(t, controlProbeLua) } @@ -389,6 +430,136 @@ func TestDriverControlHoldExpiresIntoDefault(t *testing.T) { } } +// The request may be paused after validation while a lifecycle reload swaps +// the running instance. The hold must use the generation that received the +// command, so expiry defaults that instance instead of discarding the hold as +// stale. +func TestDriverControlBindsHoldToRestartedGeneration(t *testing.T) { + srv, tel := controlServer(t) + cfg := srv.deps.Cfg.Drivers[0] + oldStatus, ok := srv.deps.Registry.ControlStatus("heat") + if !ok { + t.Fatal("driver is not running before generation race") + } + + paused := make(chan struct{}) + resume := make(chan struct{}) + var pauseOnce sync.Once + srv.beforeDriverControlSend = func() { + pauseOnce.Do(func() { + close(paused) + <-resume + }) + } + + postDone := make(chan *httptest.ResponseRecorder, 1) + go func() { + postDone <- post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":2,"duration_s":600}`) + }() + select { + case <-paused: + case <-time.After(time.Second): + t.Fatal("control request did not reach the pre-send pause") + } + + if err := srv.deps.Registry.Restart(context.Background(), cfg); err != nil { + t.Fatalf("restart during control dispatch = %v", err) + } + newStatus, ok := srv.deps.Registry.ControlStatus("heat") + if !ok { + t.Fatal("replacement driver is not running") + } + if newStatus.Generation == oldStatus.Generation { + t.Fatalf("restart kept generation %d", newStatus.Generation) + } + close(resume) + + var rec *httptest.ResponseRecorder + select { + case rec = <-postDone: + case <-time.After(2 * time.Second): + t.Fatal("control request did not finish after restart") + } + if rec.Code != http.StatusOK { + t.Fatalf("POST after restart race = %d, body %s", rec.Code, rec.Body.String()) + } + waitMetric(t, tel, "heat", "applied", 2) + hold := srv.activeControlHold("heat") + if hold == nil { + t.Fatal("control request did not create a hold") + } + if hold.Generation != newStatus.Generation { + t.Fatalf("hold generation = %d, want replacement generation %d", hold.Generation, newStatus.Generation) + } + + state := srv.peekControlState("heat") + if state == nil { + t.Fatal("control state disappeared before expiry") + } + state.mu.Lock() + fired := state.hold + state.mu.Unlock() + if err := srv.expireControlHold("heat", state, fired); err != nil { + t.Fatalf("expiry after generation-bound command = %v", err) + } + waitMetric(t, tel, "heat", "defaulted", 1) + waitMetric(t, tel, "heat", "applied", 0) + if got := srv.activeControlHold("heat"); got != nil { + t.Fatalf("hold survived successful expiry: %+v", got) + } +} + +func TestDriverControlExpiryDefaultFailureBlocksUntilRecovery(t *testing.T) { + srv, _ := controlServerWithLua(t, controlExpiryRecoveryProbeLua) + + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":2,"duration_s":600}`); rec.Code != http.StatusOK { + t.Fatalf("POST = %d, body %s", rec.Code, rec.Body.String()) + } + state := srv.peekControlState("heat") + if state == nil { + t.Fatal("missing control state") + } + state.mu.Lock() + hold := state.hold + state.mu.Unlock() + if hold == nil { + t.Fatal("missing control hold") + } + + if err := srv.expireControlHold("heat", state, hold); err == nil { + t.Fatal("expiry hid a failed default") + } + if got := srv.activeControlHold("heat"); got != nil { + t.Fatalf("failed expiry left hold active: %+v", got) + } + status, ok := srv.deps.Registry.ControlStatus("heat") + if !ok || !status.Blocked || !status.RecoveryPending || status.DefaultConfirmed { + t.Fatalf("status after failed expiry default = %+v, running=%v", status, ok) + } + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":3,"duration_s":600}`); rec.Code != http.StatusConflict { + t.Fatalf("control during expiry recovery = %d, body %s", rec.Code, rec.Body.String()) + } + + deadline := time.Now().Add(3 * time.Second) + for time.Now().Before(deadline) { + status, ok = srv.deps.Registry.ControlStatus("heat") + if ok && !status.Blocked && status.DefaultConfirmed && !status.RecoveryPending { + break + } + time.Sleep(20 * time.Millisecond) + } + if !ok || status.Blocked || !status.DefaultConfirmed || status.RecoveryPending { + t.Fatalf("status after expiry default recovery = %+v, running=%v", status, ok) + } + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":3,"duration_s":600}`); rec.Code != http.StatusOK { + t.Fatalf("control after expiry default recovery = %d, body %s", rec.Code, rec.Body.String()) + } +} + // Replacing a hold must not leave the old timer able to default the device // out from under the new setting. func TestDriverControlReplacingHoldCancelsTheOldTimer(t *testing.T) { diff --git a/go/internal/drivers/lua.go b/go/internal/drivers/lua.go index 0ec70f2f..d8ab865a 100644 --- a/go/internal/drivers/lua.go +++ b/go/internal/drivers/lua.go @@ -564,6 +564,17 @@ func (d *LuaDriver) DefaultModeContext(ctx context.Context) error { return d.call(ctx, "driver_default_mode") } +// hasEntrypoint reports whether the loaded driver defines a callable global. +// Missing lifecycle hooks remain optional for reporting-only drivers, so the +// registry uses this only when it has already established that an operator +// control declaration makes the default hook a safety requirement. +func (d *LuaDriver) hasEntrypoint(name string) bool { + d.mu.Lock() + defer d.mu.Unlock() + _, ok := d.L.GetGlobal(name).(*lua.LFunction) + return ok +} + // call is a convenience for parameter-less void-returning lifecycle funcs. func (d *LuaDriver) call(ctx context.Context, name string) error { d.mu.Lock() diff --git a/go/internal/drivers/registry.go b/go/internal/drivers/registry.go index 1cdb7e11..3e362aee 100644 --- a/go/internal/drivers/registry.go +++ b/go/internal/drivers/registry.go @@ -368,6 +368,14 @@ func (r *Registry) AddProbe(ctx context.Context, cfg config.Driver) error { return r.add(ctx, cfg, false) } +func legacyDriverDeclaresControls(path string) (bool, error) { + entry, err := ParseCatalogFile(path) + if err != nil { + return false, err + } + return len(entry.Controls) > 0, nil +} + // add is the shared driver construction path. startupDefault is false only // for connection probes; operational drivers use the startup safety gate. func (r *Registry) add(ctx context.Context, cfg config.Driver, startupDefault bool) error { @@ -481,6 +489,17 @@ func (r *Registry) add(ctx context.Context, cfg config.Driver, startupDefault bo if err != nil { return fmt.Errorf("load lua: %w", err) } + if !cfg.ObserveOnly && (policy == nil || !policy.IsControlV2()) { + declaresControls, catalogErr := legacyDriverDeclaresControls(cfg.Lua) + if catalogErr != nil { + luaDrv.CleanupContext(ctx) + return fmt.Errorf("validate legacy driver controls: %w", catalogErr) + } + if declaresControls && !luaDrv.hasEntrypoint("driver_default_mode") { + luaDrv.CleanupContext(ctx) + return fmt.Errorf("driver %q declares operator controls but is missing required driver_default_mode", cfg.Name) + } + } var drv driverRuntime = &luaRuntime{LuaDriver: luaDrv} r.mu.Lock() @@ -1010,46 +1029,60 @@ func (r *Registry) remove(name string, skipDefault bool) { // Send dispatches a command JSON blob to a specific driver. Blocks until the // driver's runLoop processes it or ctx expires. func (r *Registry) Send(ctx context.Context, name string, payload []byte) error { + _, err := r.SendWithGeneration(ctx, name, payload) + return err +} + +// SendWithGeneration dispatches a command to the concrete running driver that +// was selected for this call and returns that instance's generation. Selecting +// the instance and recording its generation under the registry lock prevents a +// caller from pairing a command sent to a replacement with an older status +// snapshot. +func (r *Registry) SendWithGeneration(ctx context.Context, name string, payload []byte) (uint64, error) { if ctx == nil { ctx = context.Background() } if err := ctx.Err(); err != nil { - return err + return 0, err } r.mu.Lock() rd, ok := r.rec[name] + generation := uint64(0) + if ok { + generation = rd.generation + } r.mu.Unlock() if !ok { - return fmt.Errorf("driver %q not found", name) + return 0, fmt.Errorf("driver %q not found", name) } if rd.cfg.ObserveOnly { - return ErrObserveOnly + return generation, ErrObserveOnly } if rd.controlIsBlocked() { - return ErrControlBlocked + return generation, ErrControlBlocked } if err := ctx.Err(); err != nil { - return err + return generation, err } resCh := make(chan error, 1) state := &commandState{} select { case rd.cmdCh <- driverCmd{kind: "command", ctx: ctx, payload: payload, result: resCh, state: state}: case <-ctx.Done(): - return ctx.Err() + return generation, ctx.Err() } select { case err := <-resCh: - return err + return generation, err case <-ctx.Done(): err, started, completed := state.snapshot() if completed { - return err + return generation, err } if started { - return &commandMayHaveRunError{cause: ctx.Err()} + return generation, &commandMayHaveRunError{cause: ctx.Err()} } - return ctx.Err() + return generation, ctx.Err() } } diff --git a/go/internal/drivers/registry_restart_test.go b/go/internal/drivers/registry_restart_test.go index f608b29d..a6d88b48 100644 --- a/go/internal/drivers/registry_restart_test.go +++ b/go/internal/drivers/registry_restart_test.go @@ -5,6 +5,7 @@ import ( "errors" "os" "path/filepath" + "strings" "sync" "sync/atomic" "testing" @@ -374,6 +375,68 @@ end waitRegistryMetric(t, tel, "d1", "command_called", 1) } +func TestLegacyControlDriverRequiresDefaultMode(t *testing.T) { + src := ` +DRIVER = { + id = "control_without_default", + controls = { + { id = "set_offset", input = { type = "number", min = -3, max = 3 } }, + }, +} +function driver_init(config) host.set_poll_interval(1000) end +function driver_poll() return 1000 end +function driver_command(action, w, cmd) return true end +` + path := writeTestDriver(t, src) + r := NewRegistry(telemetry.NewStore()) + err := r.Add(context.Background(), config.Driver{Name: "d1", Lua: path}) + if err == nil { + t.Fatal("control-capable legacy driver without driver_default_mode was accepted") + } + if !strings.Contains(err.Error(), "driver_default_mode") { + t.Fatalf("missing-default error = %v, want driver_default_mode", err) + } + if _, ok := r.ControlStatus("d1"); ok { + t.Fatal("driver without driver_default_mode was registered") + } +} + +func TestLegacyNoControlDriverMayOmitDefaultMode(t *testing.T) { + path := writeTestDriver(t, ` +function driver_init(config) host.set_poll_interval(1000) end +function driver_poll() return 1000 end +function driver_command(action, w, cmd) return true end +`) + r := NewRegistry(telemetry.NewStore()) + if err := r.Add(context.Background(), config.Driver{Name: "d1", Lua: path}); err != nil { + t.Fatalf("reporting-only legacy driver without default = %v", err) + } + t.Cleanup(func() { r.Remove("d1") }) +} + +func TestObserveOnlyControlDriverMayOmitDefaultMode(t *testing.T) { + src := ` +DRIVER = { + id = "observe_only_control", + controls = { + { id = "set_offset", input = { type = "number", min = -3, max = 3 } }, + }, +} +function driver_init(config) host.set_poll_interval(1000) end +function driver_poll() return 1000 end +function driver_command(action, w, cmd) return true end +` + path := writeTestDriver(t, src) + r := NewRegistry(telemetry.NewStore()) + if err := r.Add(context.Background(), config.Driver{Name: "d1", Lua: path, ObserveOnly: true}); err != nil { + t.Fatalf("observe-only legacy driver without default = %v", err) + } + t.Cleanup(func() { r.Remove("d1") }) + if err := r.Send(context.Background(), "d1", []byte(`{"action":"set_offset","value":2}`)); !errors.Is(err, ErrObserveOnly) { + t.Fatalf("observe-only control = %v, want ErrObserveOnly", err) + } +} + func TestObserveOnlySkipsStartupDefaultAndRejectsControl(t *testing.T) { src := ` function driver_init(config) From 83b3bc4a205a1bbfe495dfd88fe4ca0f87acfd52 Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Tue, 4 Aug 2026 13:19:47 +0200 Subject: [PATCH 08/10] fix: serialize driver lifecycle by name --- go/internal/drivers/registry.go | 117 +++++-- go/internal/drivers/registry_restart_test.go | 338 +++++++++++++++++++ 2 files changed, 432 insertions(+), 23 deletions(-) diff --git a/go/internal/drivers/registry.go b/go/internal/drivers/registry.go index 3e362aee..55f858c8 100644 --- a/go/internal/drivers/registry.go +++ b/go/internal/drivers/registry.go @@ -99,6 +99,16 @@ type Registry struct { recoveryRequired map[string]bool nextGeneration uint64 lifecycleHook func(name string) + lifecycleMu sync.Mutex + lifecycleGates map[string]*lifecycleGate +} + +// lifecycleGate serializes every lifecycle transition for one driver name. +// The reference count lets unused gates leave the map, while an acquire that +// races the final release still keeps the same gate alive until it owns it. +type lifecycleGate struct { + mu sync.Mutex + refs int } // NewRegistry builds a driver registry. @@ -107,6 +117,32 @@ func NewRegistry(tel *telemetry.Store) *Registry { tel: tel, rec: map[string]*runningDriver{}, recoveryRequired: map[string]bool{}, + lifecycleGates: map[string]*lifecycleGate{}, + } +} + +func (r *Registry) acquireLifecycle(name string) func() { + r.lifecycleMu.Lock() + if r.lifecycleGates == nil { + r.lifecycleGates = make(map[string]*lifecycleGate) + } + gate := r.lifecycleGates[name] + if gate == nil { + gate = &lifecycleGate{} + r.lifecycleGates[name] = gate + } + gate.refs++ + r.lifecycleMu.Unlock() + + gate.mu.Lock() + return func() { + gate.mu.Unlock() + r.lifecycleMu.Lock() + gate.refs-- + if gate.refs == 0 && r.lifecycleGates[name] == gate { + delete(r.lifecycleGates, name) + } + r.lifecycleMu.Unlock() } } @@ -357,6 +393,8 @@ type driverCmd struct { // process. A failed default leaves the driver registered but blocked so the // recovery loop can retry without opening a control window. func (r *Registry) Add(ctx context.Context, cfg config.Driver) error { + release := r.acquireLifecycle(cfg.Name) + defer release() return r.add(ctx, cfg, true) } @@ -365,6 +403,8 @@ func (r *Registry) Add(ctx context.Context, cfg config.Driver) error { // the observe-only flag on the private config copy. func (r *Registry) AddProbe(ctx context.Context, cfg config.Driver) error { cfg.ObserveOnly = true + release := r.acquireLifecycle(cfg.Name) + defer release() return r.add(ctx, cfg, false) } @@ -376,8 +416,9 @@ func legacyDriverDeclaresControls(path string) (bool, error) { return len(entry.Controls) > 0, nil } -// add is the shared driver construction path. startupDefault is false only -// for connection probes; operational drivers use the startup safety gate. +// add is the shared driver construction path. The caller must hold the +// lifecycle gate for cfg.Name. startupDefault is false only for connection +// probes; operational drivers use the startup safety gate. func (r *Registry) add(ctx context.Context, cfg config.Driver, startupDefault bool) error { if ctx == nil { ctx = context.Background() @@ -977,6 +1018,15 @@ func (r *Registry) clearRecoveryRequired(name string, rd *runningDriver) { } func (r *Registry) remove(name string, skipDefault bool) { + release := r.acquireLifecycle(name) + defer release() + r.removeLocked(name, skipDefault) +} + +// removeLocked stops a driver while its per-name lifecycle gate is held. This +// lets Restart and Reload keep the old generation and its replacement in one +// serialized transition instead of opening an Add/Remove gap. +func (r *Registry) removeLocked(name string, skipDefault bool) { r.mu.Lock() rd, ok := r.rec[name] if !ok { @@ -1182,43 +1232,56 @@ func (r *Registry) Reload(ctx context.Context, newDrivers []config.Driver, troub r.mu.Lock() troubleshootingChanged := r.troubleshootingMode != troubleshootingMode r.troubleshootingMode = troubleshootingMode - oldNames := make(map[string]bool, len(r.rec)) oldCfgs := make(map[string]config.Driver, len(r.rec)) for n, rd := range r.rec { - oldNames[n] = true oldCfgs[n] = rd.cfg } r.mu.Unlock() - newNames := make(map[string]bool, len(active)) - for _, d := range active { - newNames[d.Name] = true - } - - // Remove or restart + // Remove or restart. Keep the same name gate through the whole transition; + // otherwise an Add can register between Remove and the replacement Add. for n, old := range oldCfgs { newCfg, stillThere := findDriver(active, n) + requiresRestart := stillThere && (troubleshootingChanged || !sameDriverConfig(old, newCfg)) if !stillThere { - r.Remove(n) - } else if troubleshootingChanged { - slog.Info("driver troubleshooting mode changed, restarting", "name", n, "enabled", troubleshootingMode) - r.Remove(n) - } else if !sameDriverConfig(old, newCfg) { - slog.Info("driver config changed, restarting", "name", n) - r.Remove(n) + release := r.acquireLifecycle(n) + r.removeLocked(n, false) + release() + } else if requiresRestart { + if troubleshootingChanged { + slog.Info("driver troubleshooting mode changed, restarting", "name", n, "enabled", troubleshootingMode) + } else { + slog.Info("driver config changed, restarting", "name", n) + } + release := r.acquireLifecycle(n) + r.removeLocked(n, false) + if err := r.add(ctx, newCfg, true); err != nil { + slog.Warn("reload driver failed", "name", n, "err", err) + } + release() } } // Add new for _, d := range active { + release := r.acquireLifecycle(d.Name) r.mu.Lock() - _, exists := r.rec[d.Name] - r.mu.Unlock() + current, exists := r.rec[d.Name] + var currentCfg config.Driver if exists { + currentCfg = current.cfg + } + r.mu.Unlock() + if exists && !troubleshootingChanged && sameDriverConfig(currentCfg, d) { + release() continue } - if err := r.Add(ctx, d); err != nil { + if exists { + r.removeLocked(d.Name, false) + } + if err := r.add(ctx, d, true); err != nil { slog.Warn("add driver failed", "name", d.Name, "err", err) } + release() } } @@ -1226,17 +1289,21 @@ func (r *Registry) Reload(ctx context.Context, newDrivers []config.Driver, troub // If cfg.Disabled is true, this is a no-op after the stop. Used by the API // restart endpoint so the driver picks up fresh credentials / re-auths. func (r *Registry) Restart(ctx context.Context, cfg config.Driver) error { - r.Remove(cfg.Name) + release := r.acquireLifecycle(cfg.Name) + defer release() + r.removeLocked(cfg.Name, false) if cfg.Disabled { return nil } - return r.Add(ctx, cfg) + return r.add(ctx, cfg, true) } // Restart a driver by name using whatever cfg it was last started with. // Returns an error if the driver isn't running (use Restart with a cfg // to spawn from scratch). func (r *Registry) RestartByName(ctx context.Context, name string) error { + release := r.acquireLifecycle(name) + defer release() r.mu.Lock() rd, ok := r.rec[name] r.mu.Unlock() @@ -1244,7 +1311,11 @@ func (r *Registry) RestartByName(ctx context.Context, name string) error { return fmt.Errorf("driver %q not running", name) } cfg := rd.cfg - return r.Restart(ctx, cfg) + r.removeLocked(name, false) + if cfg.Disabled { + return nil + } + return r.add(ctx, cfg, true) } // PollInterval returns the currently requested cadence for a running driver. diff --git a/go/internal/drivers/registry_restart_test.go b/go/internal/drivers/registry_restart_test.go index a6d88b48..ed0865f1 100644 --- a/go/internal/drivers/registry_restart_test.go +++ b/go/internal/drivers/registry_restart_test.go @@ -262,6 +262,344 @@ end waitRegistryMetric(t, tel, "d1", "command_called", 1) } +const concurrentLifecycleDriver = `DRIVER = { + id = "concurrent_lifecycle", + name = "Concurrent lifecycle", + version = "1.0.0", + controls = { + { id = "set_offset", input = { type = "number", min = -3, max = 3 } }, + }, +} + +local instance = "unknown" + +function driver_init(config) + instance = config and config.instance or "unknown" + host.set_poll_interval(1000) +end + +function driver_poll() return 1000 end + +function driver_command(action, w, cmd) + if action == "set_offset" then + host.emit_metric("applied_" .. instance, tonumber(cmd.value or cmd.offset)) + return true + end + return false +end + +function driver_default_mode() + host.emit_metric("defaulted_" .. instance, 1) +end +` + +func concurrentLifecycleConfig(path, name, instance string) config.Driver { + return config.Driver{ + Name: name, + Lua: path, + Config: map[string]any{ + "instance": instance, + }, + Capabilities: config.Capabilities{ + MQTT: &config.MQTTConfig{Host: "localhost", Port: 1883}, + }, + } +} + +func TestConcurrentAddSameNameHasSingleOwner(t *testing.T) { + path := writeTestDriver(t, concurrentLifecycleDriver) + tel := telemetry.NewStore() + r := NewRegistry(tel) + entered := make(chan struct{}, 2) + release := make(chan struct{}) + var factoryCalls atomic.Int32 + r.MQTTFactory = func(name string, c *config.MQTTConfig) (MQTTCap, error) { + factoryCalls.Add(1) + entered <- struct{}{} + <-release + return &mockMQTT{}, nil + } + var releaseOnce sync.Once + t.Cleanup(func() { releaseOnce.Do(func() { close(release) }); r.ShutdownAll() }) + + results := make(chan error, 2) + go func() { results <- r.Add(context.Background(), concurrentLifecycleConfig(path, "d1", "A")) }() + go func() { results <- r.Add(context.Background(), concurrentLifecycleConfig(path, "d1", "B")) }() + select { + case <-entered: + case <-time.After(time.Second): + t.Fatal("first Add did not reach initialization") + } + releaseOnce.Do(func() { close(release) }) + + successes := 0 + for i := 0; i < 2; i++ { + select { + case err := <-results: + if err == nil { + successes++ + } + case <-time.After(2 * time.Second): + t.Fatal("concurrent Add did not finish") + } + } + if successes != 1 { + t.Fatalf("concurrent Add successes = %d, want exactly one", successes) + } + if got := factoryCalls.Load(); got != 1 { + t.Fatalf("MQTT factory calls = %d, want one owner", got) + } + + r.mu.Lock() + rd := r.rec["d1"] + var winner string + if rd != nil { + winner, _ = rd.cfg.Config["instance"].(string) + } + r.mu.Unlock() + if rd == nil || winner == "" { + t.Fatal("successful Add left no current generation") + } + if err := r.Send(context.Background(), "d1", []byte(`{"action":"set_offset","value":2}`)); err != nil { + t.Fatalf("control on winning generation = %v", err) + } + waitRegistryMetric(t, tel, "d1", "applied_"+winner, 2) + loser := "A" + if winner == loser { + loser = "B" + } + if _, _, ok := tel.LatestMetric("d1", "applied_"+loser); ok { + t.Fatalf("orphan generation %s accepted a command", loser) + } +} + +func TestAddWaitsForInFlightAddBeforeRemove(t *testing.T) { + path := writeTestDriver(t, concurrentLifecycleDriver) + r := NewRegistry(telemetry.NewStore()) + entered := make(chan struct{}, 1) + release := make(chan struct{}) + r.MQTTFactory = func(name string, c *config.MQTTConfig) (MQTTCap, error) { + entered <- struct{}{} + <-release + return &mockMQTT{}, nil + } + var releaseOnce sync.Once + t.Cleanup(func() { releaseOnce.Do(func() { close(release) }); r.ShutdownAll() }) + + addDone := make(chan error, 1) + go func() { + addDone <- r.Add(context.Background(), concurrentLifecycleConfig(path, "d1", "A")) + }() + select { + case <-entered: + case <-time.After(time.Second): + t.Fatal("Add did not reach initialization") + } + removeDone := make(chan struct{}) + go func() { + r.Remove("d1") + close(removeDone) + }() + select { + case <-removeDone: + releaseOnce.Do(func() { close(release) }) + t.Fatal("Remove returned before the in-flight Add completed") + case <-time.After(100 * time.Millisecond): + } + releaseOnce.Do(func() { close(release) }) + if err := <-addDone; err != nil { + t.Fatalf("Add = %v", err) + } + select { + case <-removeDone: + case <-time.After(2 * time.Second): + t.Fatal("Remove did not finish after Add") + } + if _, ok := r.ControlStatus("d1"); ok { + t.Fatal("Remove left the Add-owned generation registered") + } +} + +func TestAddWaitsForInFlightAddBeforeRestart(t *testing.T) { + path := writeTestDriver(t, concurrentLifecycleDriver) + r := NewRegistry(telemetry.NewStore()) + entered := make(chan struct{}, 1) + release := make(chan struct{}) + var factoryCalls atomic.Int32 + r.MQTTFactory = func(name string, c *config.MQTTConfig) (MQTTCap, error) { + if factoryCalls.Add(1) == 1 { + entered <- struct{}{} + <-release + } + return &mockMQTT{}, nil + } + var releaseOnce sync.Once + t.Cleanup(func() { releaseOnce.Do(func() { close(release) }); r.ShutdownAll() }) + + addDone := make(chan error, 1) + go func() { + addDone <- r.Add(context.Background(), concurrentLifecycleConfig(path, "d1", "A")) + }() + select { + case <-entered: + case <-time.After(time.Second): + t.Fatal("Add did not reach initialization") + } + restartDone := make(chan error, 1) + go func() { + restartDone <- r.Restart(context.Background(), concurrentLifecycleConfig(path, "d1", "restart")) + }() + select { + case <-restartDone: + releaseOnce.Do(func() { close(release) }) + t.Fatal("Restart returned before the in-flight Add completed") + case <-time.After(100 * time.Millisecond): + } + releaseOnce.Do(func() { close(release) }) + if err := <-addDone; err != nil { + t.Fatalf("Add = %v", err) + } + select { + case err := <-restartDone: + if err != nil { + t.Fatalf("Restart = %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("Restart did not finish after Add") + } + r.mu.Lock() + rd := r.rec["d1"] + var instance string + if rd != nil { + instance, _ = rd.cfg.Config["instance"].(string) + } + r.mu.Unlock() + if instance != "restart" { + t.Fatalf("current generation instance = %q, want restart", instance) + } +} + +func TestReloadWaitsForInFlightAddBeforeReplacement(t *testing.T) { + path := writeTestDriver(t, concurrentLifecycleDriver) + r := NewRegistry(telemetry.NewStore()) + entered := make(chan struct{}, 1) + release := make(chan struct{}) + var factoryCalls atomic.Int32 + r.MQTTFactory = func(name string, c *config.MQTTConfig) (MQTTCap, error) { + if factoryCalls.Add(1) == 1 { + entered <- struct{}{} + <-release + } + return &mockMQTT{}, nil + } + var releaseOnce sync.Once + t.Cleanup(func() { releaseOnce.Do(func() { close(release) }); r.ShutdownAll() }) + + addDone := make(chan error, 1) + go func() { + addDone <- r.Add(context.Background(), concurrentLifecycleConfig(path, "d1", "A")) + }() + select { + case <-entered: + case <-time.After(time.Second): + t.Fatal("Add did not reach initialization") + } + reloadDone := make(chan struct{}) + go func() { + r.Reload(context.Background(), []config.Driver{ + concurrentLifecycleConfig(path, "d1", "reload"), + }, false) + close(reloadDone) + }() + select { + case <-reloadDone: + releaseOnce.Do(func() { close(release) }) + t.Fatal("Reload returned before the in-flight Add completed") + case <-time.After(100 * time.Millisecond): + } + releaseOnce.Do(func() { close(release) }) + if err := <-addDone; err != nil { + t.Fatalf("Add = %v", err) + } + select { + case <-reloadDone: + case <-time.After(2 * time.Second): + t.Fatal("Reload did not finish after Add") + } + r.mu.Lock() + rd := r.rec["d1"] + var instance string + if rd != nil { + instance, _ = rd.cfg.Config["instance"].(string) + } + r.mu.Unlock() + if instance != "reload" { + t.Fatalf("current generation instance = %q, want reload", instance) + } +} + +func TestFailedAddReleasesNameReservation(t *testing.T) { + path := writeTestDriver(t, ` +function driver_init(config) + if config ~= nil and config.fail == true then error("intentional init failure") end + host.set_poll_interval(1000) +end +function driver_poll() return 1000 end +function driver_command(action, w, cmd) return true end +function driver_default_mode() end +`) + r := NewRegistry(telemetry.NewStore()) + failing := config.Driver{Name: "d1", Lua: path, Config: map[string]any{"fail": true}} + if err := r.Add(context.Background(), failing); err == nil { + t.Fatal("failing Add unexpectedly succeeded") + } + completed := make(chan error, 1) + go func() { + completed <- r.Add(context.Background(), config.Driver{Name: "d1", Lua: path}) + }() + select { + case err := <-completed: + if err != nil { + t.Fatalf("Add after failed initialization = %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("name reservation was not released after failed initialization") + } + t.Cleanup(r.ShutdownAll) +} + +func TestDifferentNamesCanInitializeInParallel(t *testing.T) { + path := writeTestDriver(t, concurrentLifecycleDriver) + r := NewRegistry(telemetry.NewStore()) + entered := make(chan struct{}, 2) + release := make(chan struct{}) + r.MQTTFactory = func(name string, c *config.MQTTConfig) (MQTTCap, error) { + entered <- struct{}{} + <-release + return &mockMQTT{}, nil + } + var releaseOnce sync.Once + t.Cleanup(func() { releaseOnce.Do(func() { close(release) }); r.ShutdownAll() }) + + results := make(chan error, 2) + go func() { results <- r.Add(context.Background(), concurrentLifecycleConfig(path, "d1", "one")) }() + go func() { results <- r.Add(context.Background(), concurrentLifecycleConfig(path, "d2", "two")) }() + for i := 0; i < 2; i++ { + select { + case <-entered: + case <-time.After(time.Second): + releaseOnce.Do(func() { close(release) }) + t.Fatal("different driver names did not initialize in parallel") + } + } + releaseOnce.Do(func() { close(release) }) + for i := 0; i < 2; i++ { + if err := <-results; err != nil { + t.Fatalf("parallel Add = %v", err) + } + } +} + func TestFreshRegistryBlocksLegacyControlUntilStartupDefault(t *testing.T) { src := ` local defaults = 0 From a5e702dd72a36f503069c05f2851bd5ebd088e74 Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Tue, 4 Aug 2026 13:54:10 +0200 Subject: [PATCH 09/10] fix: serialize control state lifecycle --- go/internal/api/api.go | 4 + go/internal/api/api_driver_control.go | 44 +++++--- go/internal/api/api_driver_control_test.go | 120 +++++++++++++++++++++ 3 files changed, 156 insertions(+), 12 deletions(-) diff --git a/go/internal/api/api.go b/go/internal/api/api.go index 9b05be83..70be2314 100644 --- a/go/internal/api/api.go +++ b/go/internal/api/api.go @@ -217,6 +217,10 @@ type Server struct { // nil in production; SendWithGeneration still binds every real dispatch to // the selected running generation. beforeDriverControlSend func() + // beforeDriverControlStateLock is a package-test seam for the narrower + // lookup-to-lock lifecycle race. It runs after the state map lookup while + // the map lock is still held, before the per-driver state lock is taken. + beforeDriverControlStateLock func() versionUpdateMu sync.Mutex driverUpdateMu sync.Mutex diff --git a/go/internal/api/api_driver_control.go b/go/internal/api/api_driver_control.go index 9d9dde2f..0d45ae96 100644 --- a/go/internal/api/api_driver_control.go +++ b/go/internal/api/api_driver_control.go @@ -147,9 +147,10 @@ func (s *Server) handleDriverControl(w http.ResponseWriter, r *http.Request) { // Keep command dispatch and hold transitions under one per-driver lock. // Registry.SendWithGeneration selects the concrete running instance and // returns its generation, so expiry can never pair a new command with an - // old status snapshot. - state := s.controlState(name) - state.mu.Lock() + // old status snapshot. The state lookup and lock are also one lifecycle + // transaction: clearDriverControlState takes the same locks in the same + // order, so it cannot remove the map entry between these operations. + state := s.lockControlState(name, s.beforeDriverControlStateLock) defer state.mu.Unlock() generation, err := s.deps.Registry.SendWithGeneration(r.Context(), name, body) if err != nil { @@ -254,6 +255,10 @@ func clampToDeclared(value float64, in drivers.CatalogControlInput) float64 { func (s *Server) controlState(name string) *controlDriverState { s.controlStateMu.Lock() defer s.controlStateMu.Unlock() + return s.controlStateLocked(name) +} + +func (s *Server) controlStateLocked(name string) *controlDriverState { if s.controlStates == nil { s.controlStates = make(map[string]*controlDriverState) } @@ -265,6 +270,22 @@ func (s *Server) controlState(name string) *controlDriverState { return state } +// lockControlState serializes a state map lookup with acquisition of that +// driver's mutex. Lifecycle invalidation uses controlStateMu -> state.mu as +// well, so a request can either own the state before removal starts or see a +// removed state after removal finishes; it cannot keep a pointer that was +// deleted in between. +func (s *Server) lockControlState(name string, beforeStateLock func()) *controlDriverState { + s.controlStateMu.Lock() + state := s.controlStateLocked(name) + if beforeStateLock != nil { + beforeStateLock() + } + state.mu.Lock() + s.controlStateMu.Unlock() + return state +} + func (s *Server) armControlHoldLocked(name string, state *controlDriverState, control string, value any, generation uint64, d time.Duration) *controlHold { s.clearControlHoldLocked(state) hold := &controlHold{ @@ -326,8 +347,7 @@ func (s *Server) SendDriverDefault(ctx context.Context, name string) error { if _, ok := s.deps.Registry.ControlStatus(name); !ok { return fmt.Errorf("driver %q not found", name) } - state := s.controlState(name) - state.mu.Lock() + state := s.lockControlState(name, nil) defer state.mu.Unlock() s.clearControlHoldLocked(state) return s.sendDefaultLocked(ctx, name) @@ -388,16 +408,16 @@ func (s *Server) clearDriverControl(name string) { func (s *Server) clearDriverControlState(name string, expected *controlDriverState) { s.controlStateMu.Lock() + defer s.controlStateMu.Unlock() state := s.controlStates[name] if expected != nil && state != expected { - s.controlStateMu.Unlock() return } - delete(s.controlStates, name) - s.controlStateMu.Unlock() - if state != nil { - state.mu.Lock() - s.clearControlHoldLocked(state) - state.mu.Unlock() + if state == nil { + return } + state.mu.Lock() + s.clearControlHoldLocked(state) + delete(s.controlStates, name) + state.mu.Unlock() } diff --git a/go/internal/api/api_driver_control_test.go b/go/internal/api/api_driver_control_test.go index c74587de..2dc81338 100644 --- a/go/internal/api/api_driver_control_test.go +++ b/go/internal/api/api_driver_control_test.go @@ -510,6 +510,126 @@ func TestDriverControlBindsHoldToRestartedGeneration(t *testing.T) { } } +func TestDriverControlLookupRaceCannotOrphanStateOrTimer(t *testing.T) { + tests := []struct { + name string + lifecycle func(*Server, config.Driver) error + }{ + { + name: "remove", + lifecycle: func(srv *Server, _ config.Driver) error { + srv.deps.Registry.Remove("heat") + return nil + }, + }, + { + name: "restart", + lifecycle: func(srv *Server, cfg config.Driver) error { + return srv.deps.Registry.Restart(context.Background(), cfg) + }, + }, + { + name: "reload", + lifecycle: func(srv *Server, cfg config.Driver) error { + srv.deps.Registry.Reload(context.Background(), []config.Driver{cfg}, true) + return nil + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + srv, tel := controlServerWithLua(t, controlSafetyProbeLua) + cfg := srv.deps.Cfg.Drivers[0] + lookedUp := make(chan struct{}) + resume := make(chan struct{}) + var pauseOnce sync.Once + var resumeOnce sync.Once + resumeRequest := func() { resumeOnce.Do(func() { close(resume) }) } + defer resumeRequest() + srv.beforeDriverControlStateLock = func() { + pauseOnce.Do(func() { + close(lookedUp) + <-resume + }) + } + + postDone := make(chan *httptest.ResponseRecorder, 1) + go func() { + postDone <- post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":1,"duration_s":1}`) + }() + select { + case <-lookedUp: + case <-time.After(time.Second): + t.Fatal("control request did not reach the post-lookup pause") + } + + lifecycleDone := make(chan error, 1) + go func() { lifecycleDone <- test.lifecycle(srv, cfg) }() + mapProbeDone := make(chan bool, 1) + go func() { mapProbeDone <- srv.peekControlState("heat") == nil }() + select { + case removed := <-mapProbeDone: + if removed { + t.Fatalf("%s removed control state between lookup and state.mu", test.name) + } + case <-time.After(100 * time.Millisecond): + } + + resumeRequest() + var rec *httptest.ResponseRecorder + select { + case rec = <-postDone: + case <-time.After(2 * time.Second): + t.Fatal("control request did not finish after lifecycle release") + } + if rec.Code != http.StatusNotFound { + t.Fatalf("raced POST = %d, body %s; request must not arm a stale state", rec.Code, rec.Body.String()) + } + select { + case err := <-lifecycleDone: + if err != nil { + t.Fatalf("%s = %v", test.name, err) + } + case <-time.After(2 * time.Second): + t.Fatal("lifecycle deadlocked after the request released state.mu") + } + + if got := srv.activeControlHold("heat"); got != nil { + t.Fatalf("%s left an orphaned hold: %+v", test.name, got) + } + if got := driverDetail(t, srv, "heat"); got.Hold != nil { + t.Fatalf("GET after %s exposed an orphaned hold: %+v", test.name, got.Hold) + } + + if test.name == "remove" { + time.Sleep(1500 * time.Millisecond) + if got, _, ok := tel.LatestMetric("heat", "defaulted"); ok && got != 0 { + t.Fatalf("removed driver received orphaned timer default: %v", got) + } + return + } + + if rec := post(t, srv, "/api/drivers/heat/control", + `{"control":"set_offset","value":-2,"duration_s":600}`); rec.Code != http.StatusOK { + t.Fatalf("new-generation POST after %s = %d, body %s", test.name, rec.Code, rec.Body.String()) + } + waitMetric(t, tel, "heat", "applied", -2) + if got := driverDetail(t, srv, "heat"); got.Hold == nil { + t.Fatalf("GET after %s lost the valid replacement hold", test.name) + } + time.Sleep(1500 * time.Millisecond) + if got, _, ok := tel.LatestMetric("heat", "defaulted"); ok && got != 0 { + t.Fatalf("old timer defaulted the new generation after %s: %v", test.name, got) + } + if got, _, ok := tel.LatestMetric("heat", "applied"); !ok || got != -2 { + t.Fatalf("new generation after %s applied %v/%v, want -2", test.name, got, ok) + } + }) + } +} + func TestDriverControlExpiryDefaultFailureBlocksUntilRecovery(t *testing.T) { srv, _ := controlServerWithLua(t, controlExpiryRecoveryProbeLua) From ecabb30b2e2665892617c413c78612be0878d325 Mon Sep 17 00:00:00 2001 From: Fredrik Ahlgren Date: Tue, 4 Aug 2026 14:18:30 +0200 Subject: [PATCH 10/10] fix: avoid recreating removed control state --- go/internal/api/api.go | 3 + go/internal/api/api_driver_control.go | 25 ++++- go/internal/api/api_driver_control_test.go | 106 +++++++++++++++++++++ 3 files changed, 131 insertions(+), 3 deletions(-) diff --git a/go/internal/api/api.go b/go/internal/api/api.go index 70be2314..d0efaf19 100644 --- a/go/internal/api/api.go +++ b/go/internal/api/api.go @@ -221,6 +221,9 @@ type Server struct { // lookup-to-lock lifecycle race. It runs after the state map lookup while // the map lock is still held, before the per-driver state lock is taken. beforeDriverControlStateLock func() + // beforeDriverDefaultStateLock is a package-test seam for the default path's + // lookup-to-lock lifecycle race. It is nil in production. + beforeDriverDefaultStateLock func() versionUpdateMu sync.Mutex driverUpdateMu sync.Mutex diff --git a/go/internal/api/api_driver_control.go b/go/internal/api/api_driver_control.go index 0d45ae96..d995687e 100644 --- a/go/internal/api/api_driver_control.go +++ b/go/internal/api/api_driver_control.go @@ -286,6 +286,21 @@ func (s *Server) lockControlState(name string, beforeStateLock func()) *controlD return state } +// lockExistingControlState serializes access to a state that already exists +// without creating one for a driver that may have been removed concurrently. +// Lifecycle invalidation uses the same controlStateMu -> state.mu order. +func (s *Server) lockExistingControlState(name string) (*controlDriverState, bool) { + s.controlStateMu.Lock() + state := s.controlStates[name] + if state == nil { + s.controlStateMu.Unlock() + return nil, false + } + state.mu.Lock() + s.controlStateMu.Unlock() + return state, true +} + func (s *Server) armControlHoldLocked(name string, state *controlDriverState, control string, value any, generation uint64, d time.Duration) *controlHold { s.clearControlHoldLocked(state) hold := &controlHold{ @@ -347,9 +362,13 @@ func (s *Server) SendDriverDefault(ctx context.Context, name string) error { if _, ok := s.deps.Registry.ControlStatus(name); !ok { return fmt.Errorf("driver %q not found", name) } - state := s.lockControlState(name, nil) - defer state.mu.Unlock() - s.clearControlHoldLocked(state) + if s.beforeDriverDefaultStateLock != nil { + s.beforeDriverDefaultStateLock() + } + if state, ok := s.lockExistingControlState(name); ok { + defer state.mu.Unlock() + s.clearControlHoldLocked(state) + } return s.sendDefaultLocked(ctx, name) } diff --git a/go/internal/api/api_driver_control_test.go b/go/internal/api/api_driver_control_test.go index 2dc81338..de6bd2cb 100644 --- a/go/internal/api/api_driver_control_test.go +++ b/go/internal/api/api_driver_control_test.go @@ -738,9 +738,115 @@ func TestDriverControlDefaultPathClearsHold(t *testing.T) { if hold := srv.activeControlHold("heat"); hold != nil { t.Fatalf("default path left hold active: %+v", hold) } + if state := srv.peekControlState("heat"); state == nil { + t.Fatal("default path removed the existing control state") + } waitMetric(t, tel, "heat", "defaulted", 1) } +func TestSendDriverDefaultDoesNotRecreateStateAfterRemove(t *testing.T) { + srv, _ := controlServer(t) + srv.controlState("heat") + + paused := make(chan struct{}) + resume := make(chan struct{}) + srv.beforeDriverDefaultStateLock = func() { + close(paused) + <-resume + } + + result := make(chan error, 1) + go func() { + result <- srv.SendDriverDefault(context.Background(), "heat") + }() + select { + case <-paused: + case <-time.After(time.Second): + t.Fatal("SendDriverDefault did not reach the state-lock seam") + } + + removed := make(chan struct{}) + go func() { + srv.deps.Registry.RemoveProbe("heat") + close(removed) + }() + select { + case <-removed: + case <-time.After(time.Second): + close(resume) + t.Fatal("RemoveProbe deadlocked behind SendDriverDefault") + } + if state := srv.peekControlState("heat"); state != nil { + t.Fatalf("removed driver still has control state before default resumes: %p", state) + } + close(resume) + + select { + case err := <-result: + if err == nil { + t.Fatal("SendDriverDefault succeeded for removed driver") + } + case <-time.After(time.Second): + t.Fatal("SendDriverDefault did not finish after lifecycle removal") + } + if state := srv.peekControlState("heat"); state != nil { + t.Fatalf("default path recreated removed control state: %p", state) + } +} + +func TestSendDriverDefaultDoesNotAccumulateRemovedStates(t *testing.T) { + srv, _ := controlServer(t) + srv.controlState("heat") + base := srv.deps.Cfg.Drivers[0] + + for i := 0; i < 64; i++ { + name := "removed-" + strconv.Itoa(i) + cfg := base + cfg.Name = name + if err := srv.deps.Registry.Add(context.Background(), cfg); err != nil { + t.Fatalf("add %s: %v", name, err) + } + srv.controlState(name) + + paused := make(chan struct{}) + resume := make(chan struct{}) + srv.beforeDriverDefaultStateLock = func() { + close(paused) + <-resume + } + result := make(chan error, 1) + go func() { + result <- srv.SendDriverDefault(context.Background(), name) + }() + select { + case <-paused: + case <-time.After(time.Second): + close(resume) + t.Fatalf("SendDriverDefault(%s) did not reach the state-lock seam", name) + } + srv.deps.Registry.RemoveProbe(name) + close(resume) + select { + case err := <-result: + if err == nil { + t.Fatalf("SendDriverDefault(%s) succeeded after removal", name) + } + case <-time.After(time.Second): + t.Fatalf("SendDriverDefault(%s) did not finish", name) + } + srv.beforeDriverDefaultStateLock = nil + if state := srv.peekControlState(name); state != nil { + t.Fatalf("removed driver %s left a control state", name) + } + } + + srv.controlStateMu.Lock() + defer srv.controlStateMu.Unlock() + if got := len(srv.controlStates); got != 1 { + t.Fatalf("control state map has %d entries after removed names, want only heat", got) + } +} + // Expiry must hold the per-driver lock through the actual default command. // Otherwise a replacement can be sent after the old hold is deleted but // before its default reaches the device, and the old default wins last.