diff --git a/.changeset/modbus-reprobe-review-fixes.md b/.changeset/modbus-reprobe-review-fixes.md new file mode 100644 index 000000000..2568b9eea --- /dev/null +++ b/.changeset/modbus-reprobe-review-fixes.md @@ -0,0 +1,5 @@ +--- +"ftw": patch +--- + +Modbus give-up recovery no longer reload-loops a missing driver file or a device that never answered, and a failed `driver_init` during that reload keeps the previous VM so default-mode still works. diff --git a/go/internal/drivers/lua.go b/go/internal/drivers/lua.go index ef597ae89..5c7906e69 100644 --- a/go/internal/drivers/lua.go +++ b/go/internal/drivers/lua.go @@ -81,11 +81,15 @@ type LuaDriver struct { mu sync.Mutex L *lua.LState - restricted bool - initConfig map[string]any + restricted bool + initConfig map[string]any + // sawModbusRead is true only after a poll that successfully read a + // register. Failed attempts do not count: a device that never answered + // is not reloaded every time its give-up tables go quiet. sawModbusRead bool - // skipReprobe latches after a reload that still produced no reads, so a - // driver that legitimately stops probing is not reloaded on every poll. + // skipReprobe latches after a failed reload, or after a reload whose + // immediate retry still made zero reads, so a missing file or a driver + // that legitimately stops probing is not reloaded on every poll. skipReprobe bool reprobeCount int } @@ -235,6 +239,7 @@ func (d *LuaDriver) Poll(ctx context.Context) (time.Duration, error) { // retry, not a shutdown. d.Env.Logger.Info("modbus driver stopped probing registers; reloading to retry") if rerr := d.reprobeLocked(ctx); rerr != nil { + d.skipReprobe = true return 0, fmt.Errorf("reprobe: %w", rerr) } d.reprobeCount++ @@ -291,7 +296,7 @@ func (d *LuaDriver) notePollModbusActivity() { if !d.Env.requiresFreshModbusRead { return } - if d.Env.lastPollEvidence.Attempts > 0 { + if d.Env.lastPollEvidence.Successes > 0 { d.sawModbusRead = true d.skipReprobe = false } @@ -300,6 +305,12 @@ func (d *LuaDriver) notePollModbusActivity() { // reprobeLocked re-executes the driver file in a new VM and re-runs // driver_init. Caller holds d.mu. The previous VM is closed without // driver_cleanup so a live setpoint is not cleared. +// +// The file is read from disk so a hot-edited driver is what we retry +// with; catalog drivers are already hot-editable, and this path is a +// probe retry rather than a process restart. The new VM is initialized +// before the swap: a failed driver_init keeps the previous state so +// default-mode still has its locals. func (d *LuaDriver) reprobeLocked(ctx context.Context) error { src, err := os.ReadFile(d.Path) if err != nil { @@ -327,12 +338,17 @@ func (d *LuaDriver) reprobeLocked(ctx context.Context) error { } old := d.L d.L = L + if err := d.callInitLocked(ctx); err != nil { + d.L = old + L.Close() + return err + } old.Close() d.Env.requiresFreshModbusRead = driverRequiresFreshModbusRead(L, d.Env.Modbus != nil) if driverDeclaresReadOnlyBattery(L) { d.Env.BatteryTelemetryOnly = true } - return d.callInitLocked(ctx) + return nil } func (d *LuaDriver) reprobes() int { diff --git a/go/internal/drivers/modbus_failure_test.go b/go/internal/drivers/modbus_failure_test.go index c125ec7be..64348b367 100644 --- a/go/internal/drivers/modbus_failure_test.go +++ b/go/internal/drivers/modbus_failure_test.go @@ -362,3 +362,165 @@ func TestGiveUpOnAbsentRegisterDoesNotReload(t *testing.T) { t.Fatalf("meter = %+v, want 321 W kept while 11 is absent", reading) } } + +func pollLiveThenGiveUp(t *testing.T, driver *LuaDriver, bus *toggleModbus) { + t.Helper() + if _, err := driver.Poll(context.Background()); err != nil { + t.Fatalf("live poll: %v", err) + } + bus.down = true + for poll := 1; poll <= 3; poll++ { + if _, err := driver.Poll(context.Background()); err == nil { + t.Fatalf("outage poll %d succeeded", poll) + } + } +} + +// A missing driver file must not become a per-poll reload loop. The PR +// promised that a failed reload latches until process restart. +func TestGiveUpFailedReloadLatches(t *testing.T) { + tel := telemetry.NewStore() + bus := &toggleModbus{registers: []uint16{321}} + driver := newGiveUpDriver(t, tel, bus) + pollLiveThenGiveUp(t, driver, bus) + + src, err := os.ReadFile(driver.Path) + if err != nil { + t.Fatal(err) + } + if err := os.Remove(driver.Path); err != nil { + t.Fatal(err) + } + if _, err := driver.Poll(context.Background()); err == nil { + t.Fatal("expected reprobe error after the driver file disappeared") + } + if err := os.WriteFile(driver.Path, src, 0o644); err != nil { + t.Fatal(err) + } + if _, err := driver.Poll(context.Background()); err != nil { + t.Fatalf("given-up poll after a failed reload: %v", err) + } + if driver.reprobes() != 0 { + t.Fatalf("reprobes = %d, want 0: a failed reload latches", driver.reprobes()) + } +} + +// driver_init must succeed on the candidate VM before we throw the old one +// away. Otherwise default-mode runs against an uninitialized state. +func TestGiveUpKeepsOldVMWhenReprobeInitFails(t *testing.T) { + const withDefault = probeGiveUpSource + ` +function driver_default_mode() + host.emit("meter", { w = 42 }) +end +` + const failingInit = ` +PROTOCOL = "modbus" +function driver_init() + error("init failed") +end +function driver_poll() + host.modbus_read(10, 1, "holding") + host.emit("meter", { w = 999 }) + return 1000 +end +function driver_default_mode() + error("new vm") +end +` + tel := telemetry.NewStore() + bus := &toggleModbus{registers: []uint16{321}} + path := filepath.Join(t.TempDir(), "give_up.lua") + if err := os.WriteFile(path, []byte(withDefault), 0o644); err != nil { + t.Fatal(err) + } + driver, err := NewLuaDriver(path, NewHostEnv("give-up", tel).WithModbus(bus)) + if err != nil { + t.Fatalf("load driver: %v", err) + } + t.Cleanup(driver.Cleanup) + if err := driver.Init(context.Background(), nil); err != nil { + t.Fatalf("init driver: %v", err) + } + + pollLiveThenGiveUp(t, driver, bus) + readsAfterGiveUp := bus.reads[10] + if err := os.WriteFile(path, []byte(failingInit), 0o644); err != nil { + t.Fatal(err) + } + if _, err := driver.Poll(context.Background()); err == nil { + t.Fatal("expected reprobe to fail when driver_init errors") + } + if driver.reprobes() != 0 { + t.Fatalf("reprobes = %d, want 0", driver.reprobes()) + } + if err := driver.DefaultMode(); err != nil { + t.Fatalf("default-mode on the kept VM: %v", err) + } + if _, err := driver.Poll(context.Background()); err != nil { + t.Fatalf("given-up poll on the kept VM: %v", err) + } + if bus.reads[10] != readsAfterGiveUp { + t.Fatalf("register 10 reads = %d, want %d: new VM would have probed again", bus.reads[10], readsAfterGiveUp) + } +} + +// Failed attempts must not arm reprobe. A device that never answered would +// otherwise reload-loop once its give-up tables emptied. +func TestGiveUpNeverOnlineDoesNotReload(t *testing.T) { + tel := telemetry.NewStore() + bus := &toggleModbus{registers: []uint16{321}, down: true} + driver := newGiveUpDriver(t, tel, bus) + + for poll := 1; poll <= 3; poll++ { + if _, err := driver.Poll(context.Background()); err == nil { + t.Fatalf("never-online poll %d succeeded", poll) + } + } + for poll := 4; poll <= 6; poll++ { + if _, err := driver.Poll(context.Background()); err != nil { + t.Fatalf("given-up poll %d: %v", poll, err) + } + } + if driver.reprobes() != 0 { + t.Fatalf("reprobes = %d, want 0 before the device has ever been read", driver.reprobes()) + } + + bus.down = false + if _, err := driver.Poll(context.Background()); err != nil { + t.Fatalf("quiet poll after the link appeared: %v", err) + } + if driver.reprobes() != 0 { + t.Fatalf("reprobes = %d, want 0: never-online stays given-up until restart", driver.reprobes()) + } + if reading := tel.Get("give-up", telemetry.DerMeter); reading != nil { + t.Fatalf("never-online driver stored meter %+v", reading) + } +} + +// A blip that outlasts the first reload must keep retrying. Latch-after- +// every-reload would recover a 15-second flap and miss the 9-minute one +// that took Pixii offline. +func TestGiveUpDriverRecoversAfterSustainedOutage(t *testing.T) { + tel := telemetry.NewStore() + bus := &toggleModbus{registers: []uint16{321}} + driver := newGiveUpDriver(t, tel, bus) + pollLiveThenGiveUp(t, driver, bus) + + for poll := 1; poll <= 6; poll++ { + if _, err := driver.Poll(context.Background()); err == nil { + t.Fatalf("sustained-outage poll %d succeeded", poll) + } + } + if driver.reprobes() < 1 { + t.Fatalf("reprobes = %d, want at least one retry while the link stayed down", driver.reprobes()) + } + + bus.down = false + if _, err := driver.Poll(context.Background()); err != nil { + t.Fatalf("recovery poll after a sustained outage: %v", err) + } + reading := tel.Get("give-up", telemetry.DerMeter) + if reading == nil || reading.RawW != 321 { + t.Fatalf("recovered meter = %+v, want 321 W", reading) + } +}