From f45d23df9706e594201fb87bc3ab1c37ba7ea6c4 Mon Sep 17 00:00:00 2001 From: Lutz Finsterle Date: Wed, 8 Apr 2026 20:44:08 +0200 Subject: [PATCH] Fix Shelly unreachable: roll back engine state on failed actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without this, a failed Execute() left engine state diverged from hardware. SyncHardwareState would then misread the mismatch as a manual override and apply a 1-hour lockout — causing either a stuck-on or stuck-off loop. Now Execute() returns per-action []error. The control loop calls Engine.RollbackAction() for each failed action, keeping engine state in sync with hardware so the next cycle simply retries. Co-Authored-By: Claude Sonnet 4.6 --- internal/actuator/actuator.go | 16 ++++++++++------ internal/actuator/actuator_test.go | 21 +++++++++++---------- internal/engine/engine.go | 28 ++++++++++++++++++++++++++++ main.go | 14 ++++++++------ 4 files changed, 57 insertions(+), 22 deletions(-) diff --git a/internal/actuator/actuator.go b/internal/actuator/actuator.go index 9019592..60b4535 100644 --- a/internal/actuator/actuator.go +++ b/internal/actuator/actuator.go @@ -38,26 +38,30 @@ func NewActuator(cfg *config.Config, vc *viessmann.Client, logger *slog.Logger) } } -// Execute performs a list of switching actions. -func (a *Actuator) Execute(ctx context.Context, actions []engine.Action) error { - for _, action := range actions { +// Execute performs a list of switching actions and returns one error per action +// (nil on success). Failed actions do not prevent subsequent actions from running. +// The caller should roll back engine state for any failed action using +// Engine.RollbackAction so that the next SyncHardwareState cycle does not mistake +// the divergence for a manual override. +func (a *Actuator) Execute(ctx context.Context, actions []engine.Action) []error { + errs := make([]error, len(actions)) + for i, action := range actions { if err := a.executeOne(ctx, action); err != nil { a.logger.Error("action failed", "consumer", action.Consumer, "turn_on", action.TurnOn, "error", err, ) - // Continue with other actions even if one fails + errs[i] = err continue } - a.logger.Info("action executed", "consumer", action.Consumer, "turn_on", action.TurnOn, "reason", action.Reason, ) } - return nil + return errs } func (a *Actuator) executeOne(ctx context.Context, action engine.Action) error { diff --git a/internal/actuator/actuator_test.go b/internal/actuator/actuator_test.go index f18d87e..1a5e1db 100644 --- a/internal/actuator/actuator_test.go +++ b/internal/actuator/actuator_test.go @@ -88,11 +88,11 @@ func TestExecuteTurnOnSGReady(t *testing.T) { act := testActuator(t, ipFrom(sgSrv), ipFrom(dummySrv), ipFrom(dummySrv)) - err := act.Execute(context.Background(), []engine.Action{ + errs := act.Execute(context.Background(), []engine.Action{ {Consumer: engine.ConsumerSGReady, TurnOn: true, Reason: "test"}, }) - if err != nil { - t.Fatalf("Execute failed: %v", err) + if errs[0] != nil { + t.Fatalf("Execute failed: %v", errs[0]) } if !sg.state { @@ -112,11 +112,11 @@ func TestExecuteTurnOffWallboxA(t *testing.T) { act := testActuator(t, ipFrom(dummySrv), ipFrom(wbASrv), ipFrom(dummySrv)) - err := act.Execute(context.Background(), []engine.Action{ + errs := act.Execute(context.Background(), []engine.Action{ {Consumer: engine.ConsumerWallboxA, TurnOn: false, Reason: "import"}, }) - if err != nil { - t.Fatalf("Execute failed: %v", err) + if errs[0] != nil { + t.Fatalf("Execute failed: %v", errs[0]) } if wbA.state { @@ -164,13 +164,14 @@ func TestExecuteMultipleActions(t *testing.T) { act := testActuator(t, ipFrom(sgSrv), ipFrom(wbASrv), ipFrom(wbBSrv)) - err := act.Execute(context.Background(), []engine.Action{ + for i, err := range act.Execute(context.Background(), []engine.Action{ {Consumer: engine.ConsumerSGReady, TurnOn: true}, {Consumer: engine.ConsumerWallboxA, TurnOn: true}, {Consumer: engine.ConsumerWallboxB, TurnOn: true}, - }) - if err != nil { - t.Fatalf("Execute failed: %v", err) + }) { + if err != nil { + t.Fatalf("Execute action %d failed: %v", i, err) + } } if !sg.state || !wbA.state || !wbB.state { diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 5472dfe..084a944 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -755,6 +755,34 @@ func (e *Engine) ApplyOverride(consumer Consumer, on bool, duration time.Duratio ) } +// RollbackAction reverts the engine's internal state for an action that the actuator +// failed to execute. Without this, the engine believes the switch happened, diverges +// from hardware, and SyncHardwareState will misinterpret the next read-back as a +// manual override and apply a 1-hour lockout. +// +// After rollback the engine state matches hardware again, so the next cycle's +// SyncHardwareState sees no mismatch and the action is simply retried. +func (e *Engine) RollbackAction(action Action) { + cs, ok := e.consumers[action.Consumer] + if !ok { + return + } + e.logger.Warn("rolling back engine state after failed action", + "consumer", action.Consumer, + "turn_on", action.TurnOn, + ) + cs.Active = !action.TurnOn + if action.TurnOn { + // Turn-on failed: undo activation side-effects + cs.ActivatedAt = time.Time{} + cs.ProactiveCharging = false + cs.ProbeStartGridW = 0 + cs.LowPowerCycles = 0 + } + // Turn-off failed: just restore Active=true. ActivatedAt is preserved + // (shutdown code doesn't reset it), so min-runtime stays correct. +} + // isHeatingPeriod returns true if heating is appropriate given the current month // and outdoor temperature. If HeatingMinAmbientC is configured (> 0), ambient // temperatures above that threshold suppress SG-Ready even within the heating months. diff --git a/main.go b/main.go index f291e90..437597c 100644 --- a/main.go +++ b/main.go @@ -361,8 +361,10 @@ func runCycle( return } - if err := act.Execute(ctx, actions); err != nil { - logger.Error("execution failed", "error", err) + for i, err := range act.Execute(ctx, actions) { + if err != nil { + eng.RollbackAction(actions[i]) + } } } @@ -520,8 +522,8 @@ func wwResetHandler(act *actuator.Actuator, eng *engine.Engine, cfg *config.Conf TargetTempC: cfg.Strategic.WWBaseC, Reason: "manual WW reset via web UI", } - if err := act.Execute(ctx, []engine.Action{action}); err != nil { - logger.Error("WW reset: actuator failed", "error", err) + if errs := act.Execute(ctx, []engine.Action{action}); errs[0] != nil { + logger.Error("WW reset: actuator failed", "error", errs[0]) } else { logger.Info("WW boost reset", "base_c", cfg.Strategic.WWBaseC, "locked_until", midnight.Format("15:04")) } @@ -611,8 +613,8 @@ func overrideHandler(act *actuator.Actuator, eng *engine.Engine, logger *slog.Lo TurnOn: turnOn, Reason: fmt.Sprintf("manual override via web UI (%s)", duration), } - if err := act.Execute(ctx, []engine.Action{action}); err != nil { - logger.Error("web UI override failed", "consumer", consumerKey, "state", stateVal, "error", err) + if errs := act.Execute(ctx, []engine.Action{action}); errs[0] != nil { + logger.Error("web UI override failed", "consumer", consumerKey, "state", stateVal, "error", errs[0]) http.Error(w, "switch failed — check logs", http.StatusInternalServerError) return }