diff --git a/README.md b/README.md index b47dad1..c7195fd 100644 --- a/README.md +++ b/README.md @@ -154,6 +154,95 @@ Other safety behavior worth knowing: `intensity` to `low`/`medium`/`high`, and `action` to `start`/`stop`; everything else is rejected with `400`. +## Home Assistant (MQTT) + +Optional. Off by default. When enabled, the controller connects to your MQTT +broker, self-registers in Home Assistant via **MQTT Discovery**, publishes its +state every control tick, and accepts fan overrides and workload hints over MQTT +— full parity with the HTTP API. + +Enable it in `config.yaml`: + +```yaml +mqtt: + enabled: true + broker: "tcp://192.168.1.10:1883" + username: "homeassistant" + password: "your-broker-password" + client_id: "only-fan-controller" + base_topic: "only-fan-controller" + discovery_prefix: "homeassistant" +``` + +Or via environment variables: `MQTT_ENABLED=true`, `MQTT_BROKER=tcp://…`, +`MQTT_USERNAME=…`, `MQTT_PASSWORD=…`. + +### What appears in Home Assistant + +All entities are grouped under one device, **Only Fan Controller**: + +| Entity | Type | Notes | +|--------|------|-------| +| CPU Temperature | sensor | °C | +| GPU Temperature (Max) | sensor | °C; hottest card across all GPUs (drives fan logic) | +| GPU _N_ (_model_) Temperature / Utilization / Power | sensor | one set per detected GPU (°C / % / W) | +| Fan Speed, Target Fan Speed | sensor | % | +| Thermal Zone, Failsafe Reason | sensor | text | +| Failsafe Active, Restore Pending, Last Fan Write Failed | binary_sensor | `problem` class | +| Override Fan Speed | number | slider bound to `min_speed`/`max_speed`; sends a 1-hour override | +| Clear Fan Override | button | clears any active override | + +**Per-GPU sensors are dynamic in card count.** On a two-GPU box you get two sets +of Temperature/Utilization/Power sensors, on a three-GPU box three, and so on. +Each card is announced as soon as the controller's first GPU read completes (a +few seconds after start — MQTT connects before the sensors are read, so the +per-card entities appear on the first control tick, not instantly). A GPU added +later is picked up automatically on the next tick. If the card count *shrinks*, +the stale per-card entities linger in Home Assistant as *unavailable* until you +delete them (the controller does not remove discovery entries). + +The device is marked **unavailable** the instant the process dies — the broker +publishes the Last Will (`offline`) on ungraceful disconnect, giving you free +external monitoring for the fail-safe scenarios. + +`broker` is forgiving about format: a bare host, `host:port`, or even a browser +URL pasted from Home Assistant (`http://ha.local:8123`) is normalized to +`tcp://host:port` at startup (the interpretation is logged). If the broker turns +out to be unreachable — e.g. you pointed it at HA's web UI port `8123` instead of +the MQTT broker's `1883` — the log calls that out explicitly instead of silently +timing out. + +### Topics + +- `only-fan-controller/availability` — retained `online`/`offline`. +- `only-fan-controller/state` — retained JSON, one document per control tick. +- `only-fan-controller/cmd/override` — `{"speed": 60, "duration_seconds": 3600, "reason": "..."}` +- `only-fan-controller/cmd/override/clear` — any payload clears the override. +- `only-fan-controller/cmd/hint` — `{"type": "transcode", "action": "start|stop", "intensity": "high", "source": "plex", "duration_estimate": 120}` + +Commands go through the exact same safety clamps and validation as the HTTP API: +speed is clamped to `min_speed`/`max_speed`, override duration is capped at 24h, +the critical-temperature ramp overrides everything, and hint fields are charset/ +length checked. A malformed command is logged and dropped, never applied. + +Command topics must be published **without** the retain flag — a retained +command would be replayed by the broker on every reconnect and restart. The +bridge deliberately drops retained messages on the command topics (logging the +drop once per topic) so a stray `mosquitto_pub -r` or a misconfigured automation +cannot silently re-fire the fans forever. + +Hints have no dedicated HA entity (a source/type/intensity tuple doesn't map to +one) — drive `cmd/hint` from an HA automation for the full-parity escape hatch. + +### Security + +**MQTT command authorization is your broker's authentication.** The HTTP bearer +token is HTTP-only and does not apply here — anyone who can publish to the broker +can command the fans, so protect it with broker credentials/ACLs. The blast +radius is still bounded by the controller-level clamps and the critical-temp +override. **There is no TLS in v1**; keep the broker on a trusted LAN or in front +of a TLS-terminating proxy. + ## API Endpoints ### GET /api/status @@ -328,6 +417,10 @@ All config options can be overridden via environment variables: | `CHECK_INTERVAL` | Seconds between checks | 10 | | `API_PORT` | API/Dashboard port | 8086 | | `API_TOKEN` | Bearer token for mutating endpoints | - (loopback-only) | +| `MQTT_ENABLED` | Enable Home Assistant MQTT bridge | false | +| `MQTT_BROKER` | MQTT broker URL (`tcp://host:port`) | - (required when enabled) | +| `MQTT_USERNAME` | MQTT broker username | - | +| `MQTT_PASSWORD` | MQTT broker password | - | ## Unraid Installation diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 59ccf80..73acbe8 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -17,6 +17,7 @@ import ( "github.com/sethpjohnson/only-fan-controller/internal/config" "github.com/sethpjohnson/only-fan-controller/internal/controller" "github.com/sethpjohnson/only-fan-controller/internal/monitor" + "github.com/sethpjohnson/only-fan-controller/internal/mqtt" "github.com/sethpjohnson/only-fan-controller/internal/storage" ) @@ -93,6 +94,20 @@ func applyEnvOverrides(cfg *config.Config) { if v := os.Getenv("API_TOKEN"); v != "" { cfg.API.Token = v } + + // MQTT / Home Assistant bridge + if v := os.Getenv("MQTT_ENABLED"); v != "" { + cfg.MQTT.Enabled = strings.ToLower(v) == "true" || v == "1" + } + if v := os.Getenv("MQTT_BROKER"); v != "" { + cfg.MQTT.Broker = v + } + if v := os.Getenv("MQTT_USERNAME"); v != "" { + cfg.MQTT.Username = v + } + if v := os.Getenv("MQTT_PASSWORD"); v != "" { + cfg.MQTT.Password = v + } } func main() { @@ -134,6 +149,11 @@ func restoreOnce(restore func() error) func() error { // this wait starts, so a hung Cleanup query must not stall process exit. const cleanupShutdownTimeout = 5 * time.Second +// mqttShutdownTimeout bounds how long shutdown waits for the MQTT bridge to +// publish offline + disconnect. Thermal safety (restore()) always runs before +// this, so a wedged broker must never stall process exit. +const mqttShutdownTimeout = 3 * time.Second + // runHistoryCleanup prunes readings older than retention on a fixed daily // interval until stopCh is closed. The initial cleanup (so a fresh start // doesn't wait a full day before its first prune) is run by the caller before @@ -273,6 +293,17 @@ func run() int { log.Printf("API server listening on %s:%d", cfg.API.Host, cfg.API.Port) log.Printf("Dashboard: http://localhost:%d/dashboard/", cfg.API.Port) + // Optional MQTT / Home Assistant bridge. Off unless mqtt.enabled. It talks to + // the controller only through the exported Consumer methods (the same ones the + // HTTP handlers use), so a hung/unreachable broker can never stall fan control. + // fanCtrl is a *controller.FanController in both real and demo mode, so the + // bridge (and thus HA) works in demo mode too. + var mqttBridge *mqtt.Bridge + if cfg.MQTT.Enabled { + mqttBridge = mqtt.New(cfg, fanCtrl, mqtt.NewPahoClient) + mqttBridge.Start() + } + // Wait for a shutdown signal or a fatal error. sigChan := make(chan os.Signal, 1) signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) @@ -294,6 +325,21 @@ func run() int { log.Printf("Warning: Failed to restore auto fan mode: %v", err) } + // Tear down the MQTT bridge AFTER the BMC hand-back choke point, bounded so a + // wedged broker cannot delay exit. Mirrors the history-cleanup shutdown pattern. + if mqttBridge != nil { + stopped := make(chan struct{}) + go func() { + mqttBridge.Stop() + close(stopped) + }() + select { + case <-stopped: + case <-time.After(mqttShutdownTimeout): + log.Printf("Warning: MQTT bridge did not stop within %s; abandoning it", mqttShutdownTimeout) + } + } + // Stop the history cleanup goroutine before the deferred store.Close() runs. // Bounded: if a Cleanup query happens to be in flight, don't let it stall // exit indefinitely (thermal safety already happened above, in restore()). diff --git a/cmd/controller/main_test.go b/cmd/controller/main_test.go index 04eea7c..f1b3099 100644 --- a/cmd/controller/main_test.go +++ b/cmd/controller/main_test.go @@ -6,8 +6,30 @@ import ( "path/filepath" "testing" "time" + + "github.com/sethpjohnson/only-fan-controller/internal/config" ) +func TestApplyEnvOverridesMQTT(t *testing.T) { + t.Setenv("MQTT_ENABLED", "true") + t.Setenv("MQTT_BROKER", "tcp://10.0.0.9:1883") + t.Setenv("MQTT_USERNAME", "ha") + t.Setenv("MQTT_PASSWORD", "brokerpw") + + cfg := config.Default() + applyEnvOverrides(cfg) + + if !cfg.MQTT.Enabled { + t.Fatal("MQTT_ENABLED=true should enable mqtt") + } + if cfg.MQTT.Broker != "tcp://10.0.0.9:1883" { + t.Fatalf("broker override not applied: %q", cfg.MQTT.Broker) + } + if cfg.MQTT.Username != "ha" || cfg.MQTT.Password != "brokerpw" { + t.Fatalf("credentials override not applied: %q / %q", cfg.MQTT.Username, cfg.MQTT.Password) + } +} + func TestResolveConfigMissingFileFallsBackToDefaults(t *testing.T) { missing := filepath.Join(t.TempDir(), "does-not-exist.yaml") cfg, err := resolveConfig(missing) diff --git a/config.example.yaml b/config.example.yaml index d86dc29..252866d 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -67,6 +67,20 @@ storage: # at startup and then once a day. Must be > 0. retention_days: 30 +# Optional Home Assistant integration over MQTT. Off by default: when disabled +# there is zero MQTT activity and no behavior change. When enabled, `broker` is +# REQUIRED (the service refuses to start otherwise). There is NO TLS support — +# anyone who can publish to the broker can command the fans, so protect it with +# broker credentials/ACLs. Publish cadence reuses monitoring.interval above. +mqtt: + enabled: false # off by default + broker: "tcp://192.168.1.10:1883" # required when enabled (tcp://host:port) + username: "" # optional broker username + password: "" # optional broker password; never exposed via /api/config + client_id: "only-fan-controller" # MQTT client id and HA device identifier + base_topic: "only-fan-controller" # root for state/command/availability topics + discovery_prefix: "homeassistant" # HA MQTT Discovery root + # Logs go to stderr; there is no file-logging config. Docker/systemd/your # process supervisor is responsible for capturing and rotating them (e.g. # `docker logs`, or the json-file/local log driver's rotation options). diff --git a/docs/superpowers/plans/2026-07-17-mqtt-ha-support.md b/docs/superpowers/plans/2026-07-17-mqtt-ha-support.md new file mode 100644 index 0000000..929c8a5 --- /dev/null +++ b/docs/superpowers/plans/2026-07-17-mqtt-ha-support.md @@ -0,0 +1,2456 @@ +# MQTT / Home Assistant Support Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add optional, off-by-default MQTT support so the controller self-registers in Home Assistant via MQTT Discovery, publishing retained state every control tick and accepting override/hint commands with the same safety semantics as the HTTP API. + +**Architecture:** A new decoupled `internal/mqtt` package exposes a `Bridge` that talks to the controller through a small `Consumer` interface (exactly `GetStatus`/`SetOverride`/`ClearOverride`/`AddHint`/`RemoveHint`) and to the broker through a `Client` interface (real implementation wraps `paho.mqtt.golang`; tests use a fake). The bridge runs in its own goroutines with bounded publishes and no shared locks beyond the existing `GetStatus` read path, so a hung or unreachable broker can never stall the control loop. It is wired in `cmd/controller/main.go` `run()`, gated on `mqtt.enabled`, and torn down after the BMC `restore()` choke point with a bounded timeout. + +**Tech Stack:** Go 1.25, `github.com/eclipse/paho.mqtt.golang` (MQTT client, built-in auto-reconnect), `github.com/mochi-mqtt/server/v2` (in-process broker, integration tests only), `gopkg.in/yaml.v3` (existing config), Go stdlib `testing`. + +## Global Constraints + +Copied verbatim from the approved spec (`docs/superpowers/specs/2026-07-17-mqtt-ha-design.md`). Every task's requirements implicitly include this section. + +- **Off by default:** `mqtt.enabled` defaults to `false`; when disabled there is zero MQTT activity and zero behavior change. +- **Broker required when enabled:** `config.Validate()` requires `mqtt.broker` to be non-empty and parseable when `mqtt.enabled` is true; an invalid config keeps the existing refuse-to-start behavior. +- **Password never leaked:** `MQTTConfig.Password` carries `json:"-"`; `/api/config` must not expose it (regression test required). +- **No TLS in v1:** documented limitation; broker auth is username/password only. MQTT command authorization = broker authentication. +- **Publish cadence = `monitoring.interval`:** deliberately reuse the existing control-tick interval; no separate MQTT interval knob (YAGNI). +- **Discovery:** retained config topics under `/…` (default `homeassistant`), republished on every (re)connect; all entities grouped into one HA device named "Only Fan Controller", identifier from `client_id`. +- **Availability:** `/availability`, retained `online`, LWT `offline`. +- **State:** `/state`, one retained JSON document per tick, derived from `GetStatus()`. +- **Commands (QoS 1):** `/cmd/override`, `/cmd/override/clear`, `/cmd/hint` — all safety clamping/validation lives in the controller and the shared validator; the bridge re-implements none of it. +- **Shutdown:** MQTT teardown is sequenced AFTER the BMC `restore()` choke point, publishes `offline` + disconnects with a short bounded timeout (abandon and log if exceeded). MQTT can never delay the safety-critical hand-back. +- **Client library:** `paho.mqtt.golang` for the client; `mochi-mqtt/server` for in-process integration tests (no Docker). +- **Env overrides:** `MQTT_ENABLED`, `MQTT_BROKER`, `MQTT_USERNAME`, `MQTT_PASSWORD`, following the existing `applyEnvOverrides` pattern. + +--- + +## File Structure + +**New files** +- `internal/config/config.go` (modify) — `MQTTConfig` struct, `Config.MQTT` field, defaults, validation. +- `internal/mqtt/client.go` — `Client` interface, `MessageHandler`, `ClientOptions`, `ClientFactory`, `NewPahoClient` (paho wrapper). +- `internal/mqtt/bridge.go` — `Consumer` interface, `Bridge`, `New`, lifecycle (`Start`/`Stop`), topic helpers, availability, rate limiter. +- `internal/mqtt/state.go` — `statePayload`, `buildStatePayload`, `publishState`, `publishLoop`. +- `internal/mqtt/discovery.go` — discovery entity builders, `discoveryEntities`, `publishDiscovery`. +- `internal/mqtt/command.go` — command structs, handlers, `subscribeCommands`. +- `internal/validate/validate.go` — shared request validation (used by both `api` and `mqtt`). +- `internal/mqtt/bridge_test.go`, `state_test.go`, `discovery_test.go`, `command_test.go`, `integration_test.go` — tests (the fakes `fakeClient`/`fakeConsumer`/`clientHolder` live in `bridge_test.go`, package-visible to all `_test.go` in `internal/mqtt`). + +**Modified files** +- `cmd/controller/main.go` — `applyEnvOverrides` (MQTT vars), bridge wiring + shutdown sequencing. +- `cmd/controller/main_test.go` — env-override test. +- `internal/config/config_test.go` — example-yaml regression + validation cases + password-marshal test. +- `internal/api/server.go` + `internal/api/server_test.go` — switch to `internal/validate`. +- `config.example.yaml` — `mqtt:` block. +- `unraid/only-fan-controller.xml` — MQTT template fields. +- `README.md` — "Home Assistant (MQTT)" section + env-var table rows. + +--- + +## Task 1: MQTT config section, validation, env overrides, example YAML + +**Files:** +- Modify: `internal/config/config.go` +- Modify: `cmd/controller/main.go:24-96` (`applyEnvOverrides`) +- Modify: `config.example.yaml` +- Test: `internal/config/config_test.go` +- Test: `cmd/controller/main_test.go` + +**Interfaces:** +- Consumes: nothing (first task). +- Produces: + - `type config.MQTTConfig struct { Enabled bool; Broker string; Username string; Password string; ClientID string; BaseTopic string; DiscoveryPrefix string }` + - `config.Config` gains field `MQTT MQTTConfig` (yaml `mqtt`). + - `config.Default()` populates `MQTT` with `Enabled:false, ClientID:"only-fan-controller", BaseTopic:"only-fan-controller", DiscoveryPrefix:"homeassistant"`. + - `config.Config.Validate()` enforces the broker rule when enabled. + - `applyEnvOverrides` handles `MQTT_ENABLED/BROKER/USERNAME/PASSWORD`. + +- [x] **Step 1: Write the failing config tests** + +Add to `internal/config/config_test.go`: + +```go +func TestMQTTValidation(t *testing.T) { + tests := []struct { + name string + mutate func(c *Config) + wantErr bool + }{ + { + name: "disabled needs no broker", + mutate: func(c *Config) { c.MQTT.Enabled = false; c.MQTT.Broker = "" }, + wantErr: false, + }, + { + name: "enabled with valid tcp broker is ok", + mutate: func(c *Config) { c.MQTT.Enabled = true; c.MQTT.Broker = "tcp://192.168.1.5:1883" }, + wantErr: false, + }, + { + name: "enabled with empty broker is rejected", + mutate: func(c *Config) { c.MQTT.Enabled = true; c.MQTT.Broker = "" }, + wantErr: true, + }, + { + name: "enabled with unparseable broker is rejected", + mutate: func(c *Config) { c.MQTT.Enabled = true; c.MQTT.Broker = "://nope" }, + wantErr: true, + }, + { + name: "enabled with schemeless broker is rejected", + mutate: func(c *Config) { c.MQTT.Enabled = true; c.MQTT.Broker = "192.168.1.5:1883" }, + wantErr: true, + }, + { + name: "enabled with blank client_id is rejected", + mutate: func(c *Config) { c.MQTT.Enabled = true; c.MQTT.Broker = "tcp://h:1883"; c.MQTT.ClientID = "" }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := Default() + tt.mutate(c) + err := c.Validate() + if tt.wantErr && err == nil { + t.Fatal("expected validation error, got nil") + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected validation error: %v", err) + } + }) + } +} + +func TestMQTTPasswordNotMarshaled(t *testing.T) { + c := Default() + c.MQTT.Password = "s3cr3t-broker-pw" + b, err := json.Marshal(c.MQTT) + if err != nil { + t.Fatalf("marshal failed: %v", err) + } + if strings.Contains(string(b), "s3cr3t-broker-pw") { + t.Fatalf("mqtt password leaked into JSON: %s", b) + } +} +``` + +Add imports `encoding/json` and `strings` to the test file's import block. + +- [x] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/config/ -run 'TestMQTTValidation|TestMQTTPasswordNotMarshaled' -v` +Expected: FAIL — `c.MQTT undefined (type *Config has no field or method MQTT)` (compile error). + +- [x] **Step 3: Add the MQTT config struct, field, defaults, and validation** + +In `internal/config/config.go`, add `"net/url"` to the import block. Add the `MQTT` field to `Config`: + +```go +type Config struct { + IDRAC IDRACConfig `yaml:"idrac"` + Monitoring MonitoringConfig `yaml:"monitoring"` + GPU GPUConfig `yaml:"gpu"` + Zones []Zone `yaml:"zones"` + FanControl FanControlConfig `yaml:"fan_control"` + API APIConfig `yaml:"api"` + Dashboard DashboardConfig `yaml:"dashboard"` + Storage StorageConfig `yaml:"storage"` + MQTT MQTTConfig `yaml:"mqtt"` +} +``` + +Add the struct (near the other config structs): + +```go +// MQTTConfig configures the optional Home Assistant MQTT bridge. It is off by +// default; when Enabled, Broker is required and validated. Password carries +// json:"-" so it is never exposed via /api/config (same treatment as the iDRAC +// password and API token). +type MQTTConfig struct { + Enabled bool `yaml:"enabled" json:"enabled"` + Broker string `yaml:"broker" json:"broker"` + Username string `yaml:"username" json:"username"` + Password string `yaml:"password" json:"-"` + ClientID string `yaml:"client_id" json:"client_id"` + BaseTopic string `yaml:"base_topic" json:"base_topic"` + DiscoveryPrefix string `yaml:"discovery_prefix" json:"discovery_prefix"` +} +``` + +In `Default()`, add the `MQTT` field to the returned `&Config{...}` (after `Storage`): + +```go + MQTT: MQTTConfig{ + Enabled: false, + ClientID: "only-fan-controller", + BaseTopic: "only-fan-controller", + DiscoveryPrefix: "homeassistant", + }, +``` + +In `Validate()`, add this block just before the final `return nil`: + +```go + // MQTT is optional. When enabled, the broker must be a parseable URL with a + // scheme and host, and the identity/topic roots must be non-empty (they + // default to non-empty values, so this only trips if an operator blanks + // them). When disabled, none of this is checked. + if c.MQTT.Enabled { + if c.MQTT.Broker == "" { + return fmt.Errorf("mqtt.enabled is true but mqtt.broker is empty") + } + u, err := url.Parse(c.MQTT.Broker) + if err != nil { + return fmt.Errorf("invalid mqtt.broker %q: %v", c.MQTT.Broker, err) + } + switch u.Scheme { + case "tcp", "ssl", "ws", "wss", "mqtt", "mqtts": + default: + return fmt.Errorf("invalid mqtt.broker %q: scheme must be one of tcp/ssl/ws/wss/mqtt/mqtts", c.MQTT.Broker) + } + if u.Host == "" { + return fmt.Errorf("invalid mqtt.broker %q: missing host", c.MQTT.Broker) + } + if c.MQTT.ClientID == "" || c.MQTT.BaseTopic == "" || c.MQTT.DiscoveryPrefix == "" { + return fmt.Errorf("mqtt.client_id, mqtt.base_topic and mqtt.discovery_prefix must be non-empty when mqtt is enabled") + } + } +``` + +- [x] **Step 4: Run config tests to verify they pass** + +Run: `go test ./internal/config/ -run 'TestMQTTValidation|TestMQTTPasswordNotMarshaled|TestExampleConfigIsValid' -v` +Expected: PASS (all three). + +- [x] **Step 5: Write the failing env-override test** + +Add to `cmd/controller/main_test.go`: + +```go +func TestApplyEnvOverridesMQTT(t *testing.T) { + t.Setenv("MQTT_ENABLED", "true") + t.Setenv("MQTT_BROKER", "tcp://10.0.0.9:1883") + t.Setenv("MQTT_USERNAME", "ha") + t.Setenv("MQTT_PASSWORD", "brokerpw") + + cfg := config.Default() + applyEnvOverrides(cfg) + + if !cfg.MQTT.Enabled { + t.Fatal("MQTT_ENABLED=true should enable mqtt") + } + if cfg.MQTT.Broker != "tcp://10.0.0.9:1883" { + t.Fatalf("broker override not applied: %q", cfg.MQTT.Broker) + } + if cfg.MQTT.Username != "ha" || cfg.MQTT.Password != "brokerpw" { + t.Fatalf("credentials override not applied: %q / %q", cfg.MQTT.Username, cfg.MQTT.Password) + } +} +``` + +Add `"github.com/sethpjohnson/only-fan-controller/internal/config"` to the test file's import block. + +- [x] **Step 6: Run env-override test to verify it fails** + +Run: `go test ./cmd/controller/ -run TestApplyEnvOverridesMQTT -v` +Expected: FAIL — env vars are ignored (`cfg.MQTT.Enabled` stays false). + +- [x] **Step 7: Add MQTT env overrides** + +In `cmd/controller/main.go`, inside `applyEnvOverrides`, after the `API_TOKEN` block (before the closing brace of the function): + +```go + // MQTT / Home Assistant bridge + if v := os.Getenv("MQTT_ENABLED"); v != "" { + cfg.MQTT.Enabled = strings.ToLower(v) == "true" || v == "1" + } + if v := os.Getenv("MQTT_BROKER"); v != "" { + cfg.MQTT.Broker = v + } + if v := os.Getenv("MQTT_USERNAME"); v != "" { + cfg.MQTT.Username = v + } + if v := os.Getenv("MQTT_PASSWORD"); v != "" { + cfg.MQTT.Password = v + } +``` + +(`os` and `strings` are already imported in `main.go`.) + +- [x] **Step 8: Run env-override test to verify it passes** + +Run: `go test ./cmd/controller/ -run TestApplyEnvOverridesMQTT -v` +Expected: PASS. + +- [x] **Step 9: Add the `mqtt:` block to `config.example.yaml`** + +Append to `config.example.yaml` (after the `storage:` block, before the trailing logging comment): + +```yaml + +# Optional Home Assistant integration over MQTT. Off by default: when disabled +# there is zero MQTT activity and no behavior change. When enabled, `broker` is +# REQUIRED (the service refuses to start otherwise). There is NO TLS support — +# anyone who can publish to the broker can command the fans, so protect it with +# broker credentials/ACLs. Publish cadence reuses monitoring.interval above. +mqtt: + enabled: false # off by default + broker: "tcp://192.168.1.10:1883" # required when enabled (tcp://host:port) + username: "" # optional broker username + password: "" # optional broker password; never exposed via /api/config + client_id: "only-fan-controller" # MQTT client id and HA device identifier + base_topic: "only-fan-controller" # root for state/command/availability topics + discovery_prefix: "homeassistant" # HA MQTT Discovery root +``` + +- [x] **Step 10: Verify the example config still loads/validates verbatim** + +Run: `go test ./internal/config/ -run TestExampleConfigIsValid -v` +Expected: PASS (the example has `enabled: false`, so the broker is not required). + +- [x] **Step 11: Commit** + +```bash +git add internal/config/config.go internal/config/config_test.go cmd/controller/main.go cmd/controller/main_test.go config.example.yaml +git commit --no-gpg-sign -m "feat(config): add optional MQTT config section, validation, and env overrides" +``` + +--- + +## Task 2: Bridge skeleton — Consumer/Client interfaces, lifecycle, LWT, availability + +**Files:** +- Create: `internal/mqtt/client.go` +- Create: `internal/mqtt/bridge.go` +- Test: `internal/mqtt/bridge_test.go` +- Modify: `go.mod`, `go.sum` (add `github.com/eclipse/paho.mqtt.golang`) + +**Interfaces:** +- Consumes: `config.Config` / `config.MQTTConfig` (Task 1); `controller.Status`, `controller.WorkloadHint` (existing). +- Produces: + - `type Consumer interface { GetStatus() *controller.Status; SetOverride(speed int, duration time.Duration, reason string); ClearOverride(); AddHint(hint *controller.WorkloadHint); RemoveHint(source string) }` + - `type MessageHandler func(topic string, payload []byte)` + - `type Client interface { Connect() error; Publish(topic string, qos byte, retained bool, payload []byte) error; Subscribe(topic string, qos byte, handler MessageHandler) error; Disconnect(quiesceMs uint) }` + - `type ClientOptions struct { Broker, ClientID, Username, Password, AvailabilityTopic, OnlinePayload, OfflinePayload string; OnConnect func() }` + - `type ClientFactory func(opts ClientOptions) Client` + - `func NewPahoClient(opts ClientOptions) Client` + - `type Bridge struct { ... }`, `func New(cfg *config.Config, consumer Consumer, factory ClientFactory) *Bridge` + - Methods: `(*Bridge).Start()`, `(*Bridge).Stop()`, `(*Bridge).onConnect()`, `(*Bridge).publishAvailability(online bool)`, topic helpers `availabilityTopic/stateTopic/cmdOverrideTopic/cmdOverrideClearTopic/cmdHintTopic() string`. + +- [x] **Step 1: Add the paho dependency** + +Run: +```bash +go get github.com/eclipse/paho.mqtt.golang@v1.5.0 +``` +Expected: `go.mod`/`go.sum` updated with `github.com/eclipse/paho.mqtt.golang v1.5.0` (plus `golang.org/x/sync` indirect, already present or added). + +- [x] **Step 2: Write the failing lifecycle test** + +Create `internal/mqtt/bridge_test.go`: + +```go +package mqtt + +import ( + "sync" + "testing" + "time" + + "github.com/sethpjohnson/only-fan-controller/internal/config" + "github.com/sethpjohnson/only-fan-controller/internal/controller" +) + +// publishedMsg records a single Publish call for assertions. +type publishedMsg struct { + topic string + qos byte + retained bool + payload []byte +} + +// fakeClient is an in-memory Client used across the mqtt package tests. It +// records publishes/subscriptions and invokes OnConnect synchronously from +// Connect (mirroring paho's OnConnect firing on every successful connection). +type fakeClient struct { + mu sync.Mutex + opts ClientOptions + published []publishedMsg + subs map[string]MessageHandler + disconnectCalled bool +} + +func (f *fakeClient) Connect() error { + if f.opts.OnConnect != nil { + f.opts.OnConnect() + } + return nil +} + +func (f *fakeClient) Publish(topic string, qos byte, retained bool, payload []byte) error { + f.mu.Lock() + defer f.mu.Unlock() + f.published = append(f.published, publishedMsg{topic, qos, retained, append([]byte(nil), payload...)}) + return nil +} + +func (f *fakeClient) Subscribe(topic string, qos byte, h MessageHandler) error { + f.mu.Lock() + defer f.mu.Unlock() + f.subs[topic] = h + return nil +} + +func (f *fakeClient) Disconnect(quiesceMs uint) { + f.mu.Lock() + defer f.mu.Unlock() + f.disconnectCalled = true +} + +// lastPublishOn returns the most recent publish to topic. +func (f *fakeClient) lastPublishOn(topic string) (publishedMsg, bool) { + f.mu.Lock() + defer f.mu.Unlock() + for i := len(f.published) - 1; i >= 0; i-- { + if f.published[i].topic == topic { + return f.published[i], true + } + } + return publishedMsg{}, false +} + +// deliver invokes the handler registered for topic, simulating an inbound +// broker message. +func (f *fakeClient) deliver(topic string, payload []byte) { + f.mu.Lock() + h := f.subs[topic] + f.mu.Unlock() + if h != nil { + h(topic, payload) + } +} + +// clientHolder captures the fakeClient the Bridge creates via the factory. +type clientHolder struct{ client *fakeClient } + +func (h *clientHolder) factory(opts ClientOptions) Client { + h.client = &fakeClient{opts: opts, subs: map[string]MessageHandler{}} + return h.client +} + +// fakeConsumer records controller calls and returns a canned status. +type fakeConsumer struct { + mu sync.Mutex + status *controller.Status + overrideSpeed int + overrideDur time.Duration + overrideReason string + overrideSet bool + cleared bool + addedHints []*controller.WorkloadHint + removedSources []string +} + +func (c *fakeConsumer) GetStatus() *controller.Status { + c.mu.Lock() + defer c.mu.Unlock() + if c.status != nil { + return c.status + } + return &controller.Status{} +} + +func (c *fakeConsumer) SetOverride(speed int, duration time.Duration, reason string) { + c.mu.Lock() + defer c.mu.Unlock() + c.overrideSpeed, c.overrideDur, c.overrideReason, c.overrideSet = speed, duration, reason, true +} + +func (c *fakeConsumer) ClearOverride() { + c.mu.Lock() + defer c.mu.Unlock() + c.cleared = true +} + +func (c *fakeConsumer) AddHint(hint *controller.WorkloadHint) { + c.mu.Lock() + defer c.mu.Unlock() + c.addedHints = append(c.addedHints, hint) +} + +func (c *fakeConsumer) RemoveHint(source string) { + c.mu.Lock() + defer c.mu.Unlock() + c.removedSources = append(c.removedSources, source) +} + +// testConfig returns a valid enabled-MQTT config for tests. +func testConfig() *config.Config { + cfg := config.Default() + cfg.MQTT.Enabled = true + cfg.MQTT.Broker = "tcp://127.0.0.1:1883" + return cfg +} + +func TestStartConfiguresLWTAndPublishesOnline(t *testing.T) { + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{}, h.factory) + b.Start() + + if h.client.opts.AvailabilityTopic != "only-fan-controller/availability" { + t.Fatalf("LWT topic = %q", h.client.opts.AvailabilityTopic) + } + if h.client.opts.OfflinePayload != "offline" { + t.Fatalf("LWT payload = %q, want offline", h.client.opts.OfflinePayload) + } + msg, ok := h.client.lastPublishOn("only-fan-controller/availability") + if !ok { + t.Fatal("no availability publish on connect") + } + if string(msg.payload) != "online" || !msg.retained { + t.Fatalf("availability publish = %q retained=%v, want online/true", msg.payload, msg.retained) + } +} + +func TestStopPublishesOfflineAndDisconnects(t *testing.T) { + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{}, h.factory) + b.Start() + b.Stop() + + msg, ok := h.client.lastPublishOn("only-fan-controller/availability") + if !ok || string(msg.payload) != "offline" || !msg.retained { + t.Fatalf("expected retained offline on stop, got %q retained=%v ok=%v", msg.payload, msg.retained, ok) + } + if !h.client.disconnectCalled { + t.Fatal("Disconnect was not called on Stop") + } +} +``` + +- [x] **Step 3: Run tests to verify they fail** + +Run: `go test ./internal/mqtt/ -run 'TestStart|TestStop' -v` +Expected: FAIL — package does not compile (`New`, `Bridge`, `Client`, etc. undefined). + +- [x] **Step 4: Create the Client interface and paho wrapper** + +Create `internal/mqtt/client.go`: + +```go +package mqtt + +import ( + "fmt" + "log" + "time" + + paho "github.com/eclipse/paho.mqtt.golang" +) + +// MessageHandler receives an inbound message's topic and raw payload. +type MessageHandler func(topic string, payload []byte) + +// Client is the minimal broker surface the Bridge needs. The real +// implementation wraps paho; tests use an in-memory fake. All methods must be +// bounded: no call may block the control loop or shutdown indefinitely. +type Client interface { + // Connect starts (or resumes) the connection. It must NOT block on an + // unreachable broker — the real implementation is fire-and-forget and lets + // paho's auto-reconnect handle background connection. + Connect() error + Publish(topic string, qos byte, retained bool, payload []byte) error + Subscribe(topic string, qos byte, handler MessageHandler) error + Disconnect(quiesceMs uint) +} + +// ClientOptions carries everything NewPahoClient needs, including the LWT +// (AvailabilityTopic + OfflinePayload) and an OnConnect callback invoked on +// every successful (re)connection. +type ClientOptions struct { + Broker string + ClientID string + Username string + Password string + AvailabilityTopic string + OnlinePayload string + OfflinePayload string + OnConnect func() +} + +// ClientFactory builds a Client from options. Production passes NewPahoClient; +// tests pass a fake factory. +type ClientFactory func(opts ClientOptions) Client + +// pahoClient adapts the paho client to the Client interface. +type pahoClient struct { + client paho.Client +} + +// NewPahoClient builds a paho-backed Client with auto-reconnect, connect-retry, +// and the LWT configured. OnConnect is wired so discovery/availability are +// republished on every (re)connection. +func NewPahoClient(opts ClientOptions) Client { + o := paho.NewClientOptions() + o.AddBroker(opts.Broker) + o.SetClientID(opts.ClientID) + if opts.Username != "" { + o.SetUsername(opts.Username) + } + if opts.Password != "" { + o.SetPassword(opts.Password) + } + o.SetWill(opts.AvailabilityTopic, opts.OfflinePayload, 1, true) + o.SetAutoReconnect(true) + o.SetConnectRetry(true) + o.SetConnectRetryInterval(5 * time.Second) + o.SetMaxReconnectInterval(30 * time.Second) + o.SetCleanSession(true) + o.SetOnConnectHandler(func(_ paho.Client) { + log.Printf("MQTT: connected to %s", opts.Broker) + if opts.OnConnect != nil { + opts.OnConnect() + } + }) + o.SetConnectionLostHandler(func(_ paho.Client, err error) { + log.Printf("MQTT: connection lost (auto-reconnecting): %v", err) + }) + return &pahoClient{client: paho.NewClient(o)} +} + +// Connect is fire-and-forget: it starts paho's connect loop and returns +// immediately so an unreachable broker never blocks startup. Success/failure is +// surfaced via the OnConnect / ConnectionLost handlers. +func (p *pahoClient) Connect() error { + p.client.Connect() + return nil +} + +// Publish is bounded so a wedged broker can never stall the publish ticker. +func (p *pahoClient) Publish(topic string, qos byte, retained bool, payload []byte) error { + token := p.client.Publish(topic, qos, retained, payload) + if !token.WaitTimeout(2 * time.Second) { + return fmt.Errorf("publish to %s timed out", topic) + } + return token.Error() +} + +// Subscribe runs from the OnConnect handler (paho's goroutine), so waiting on +// the token here is safe. +func (p *pahoClient) Subscribe(topic string, qos byte, handler MessageHandler) error { + token := p.client.Subscribe(topic, qos, func(_ paho.Client, m paho.Message) { + handler(m.Topic(), m.Payload()) + }) + token.Wait() + return token.Error() +} + +func (p *pahoClient) Disconnect(quiesceMs uint) { + p.client.Disconnect(quiesceMs) +} +``` + +- [x] **Step 5: Create the Bridge skeleton and lifecycle** + +Create `internal/mqtt/bridge.go`: + +```go +package mqtt + +import ( + "log" + "time" + + "github.com/sethpjohnson/only-fan-controller/internal/config" + "github.com/sethpjohnson/only-fan-controller/internal/controller" +) + +// Consumer is the exact controller surface the bridge needs. *controller. +// FanController satisfies it. All safety semantics (clamping, the 24h override +// cap, the critical-temp ramp, hint validation) live behind these methods, so +// MQTT commands inherit them for free — the bridge re-implements none of them. +type Consumer interface { + GetStatus() *controller.Status + SetOverride(speed int, duration time.Duration, reason string) + ClearOverride() + AddHint(hint *controller.WorkloadHint) + RemoveHint(source string) +} + +// disconnectQuiesceMs bounds the graceful disconnect wait so shutdown cannot be +// delayed by a slow broker. +const disconnectQuiesceMs = 250 + +// Bridge publishes controller state to MQTT and routes MQTT commands into the +// controller. It owns its own goroutines and never shares a lock with the +// control loop beyond the Consumer.GetStatus read path. +type Bridge struct { + cfg *config.Config + consumer Consumer + newClient ClientFactory + client Client +} + +// New builds a Bridge. factory is NewPahoClient in production and a fake in +// tests. Start actually connects. +func New(cfg *config.Config, consumer Consumer, factory ClientFactory) *Bridge { + return &Bridge{ + cfg: cfg, + consumer: consumer, + newClient: factory, + } +} + +func (b *Bridge) availabilityTopic() string { return b.cfg.MQTT.BaseTopic + "/availability" } +func (b *Bridge) stateTopic() string { return b.cfg.MQTT.BaseTopic + "/state" } +func (b *Bridge) cmdOverrideTopic() string { return b.cfg.MQTT.BaseTopic + "/cmd/override" } +func (b *Bridge) cmdOverrideClearTopic() string { return b.cfg.MQTT.BaseTopic + "/cmd/override/clear" } +func (b *Bridge) cmdHintTopic() string { return b.cfg.MQTT.BaseTopic + "/cmd/hint" } + +// Start builds the client and initiates connection. Connect is non-blocking, so +// an unreachable broker does not delay startup. onConnect (fired on every +// (re)connection) republishes availability. +func (b *Bridge) Start() { + opts := ClientOptions{ + Broker: b.cfg.MQTT.Broker, + ClientID: b.cfg.MQTT.ClientID, + Username: b.cfg.MQTT.Username, + Password: b.cfg.MQTT.Password, + AvailabilityTopic: b.availabilityTopic(), + OnlinePayload: "online", + OfflinePayload: "offline", + OnConnect: b.onConnect, + } + b.client = b.newClient(opts) + log.Printf("MQTT: bridge starting (broker %s, base topic %s)", b.cfg.MQTT.Broker, b.cfg.MQTT.BaseTopic) + if err := b.client.Connect(); err != nil { + log.Printf("MQTT: initial connect error (will keep retrying in background): %v", err) + } +} + +// onConnect runs on every successful (re)connection. Task 4 adds discovery and +// Task 6 adds command subscriptions here. +func (b *Bridge) onConnect() { + b.publishAvailability(true) +} + +// publishAvailability publishes the retained availability state. +func (b *Bridge) publishAvailability(online bool) { + payload := "offline" + if online { + payload = "online" + } + if err := b.client.Publish(b.availabilityTopic(), 1, true, []byte(payload)); err != nil { + log.Printf("MQTT: failed to publish availability: %v", err) + } +} + +// Stop publishes the offline availability state and disconnects. Task 3 extends +// this to stop the publish loop first. It is bounded (disconnectQuiesceMs) so it +// cannot delay the safety-critical shutdown path. +func (b *Bridge) Stop() { + if b.client == nil { + return + } + b.publishAvailability(false) + b.client.Disconnect(disconnectQuiesceMs) + log.Printf("MQTT: bridge stopped") +} +``` + +- [x] **Step 6: Run tests to verify they pass** + +Run: `go test ./internal/mqtt/ -run 'TestStart|TestStop' -v` +Expected: PASS. + +- [x] **Step 7: Verify the whole module still builds and vets** + +Run: `go build ./... && go vet ./internal/mqtt/` +Expected: no output (success). + +- [x] **Step 8: Commit** + +```bash +git add go.mod go.sum internal/mqtt/client.go internal/mqtt/bridge.go internal/mqtt/bridge_test.go +git commit --no-gpg-sign -m "feat(mqtt): add bridge skeleton, client interface, and connect/LWT lifecycle" +``` + +--- + +## Task 3: State publishing — retained JSON per tick + +**Files:** +- Create: `internal/mqtt/state.go` +- Modify: `internal/mqtt/bridge.go` (add `stopCh`/`doneCh`/`pubLimiter` fields; start/stop the loop) +- Test: `internal/mqtt/state_test.go` + +**Interfaces:** +- Consumes: `Bridge` + topic helpers (Task 2); `Consumer.GetStatus() *controller.Status`; `controller.Status`, `controller.CPUReading`/`GPUReading`, `controller.Override`. +- Produces: + - `type statePayload struct { ... }` (JSON keys: `cpu_temp, gpu_temp, fan_speed, target_speed, zone, mode, failsafe_active, failsafe_reason, restore_pending, last_write_failed, override_speed, override_reason, override_expires, active_hint_count`). + - `func buildStatePayload(status *controller.Status) statePayload` + - `func (b *Bridge) publishState()` + - `func (b *Bridge) publishLoop()` + - `type rateLimiter struct{...}`, `func (r *rateLimiter) allow() bool` + +- [x] **Step 1: Write the failing state tests** + +Create `internal/mqtt/state_test.go`: + +```go +package mqtt + +import ( + "encoding/json" + "testing" + "time" + + "github.com/sethpjohnson/only-fan-controller/internal/controller" + "github.com/sethpjohnson/only-fan-controller/internal/monitor" +) + +func TestBuildStatePayloadFull(t *testing.T) { + exp := time.Date(2026, 7, 17, 12, 0, 0, 0, time.UTC) + status := &controller.Status{ + CPU: &monitor.CPUReading{Max: 71}, + GPU: &monitor.GPUReading{Max: 63}, + CurrentSpeed: 40, + TargetSpeed: 55, + Zone: "warm", + Mode: "override", + FailsafeActive: true, + FailsafeReason: "sensor-loss", + RestorePending: true, + LastWriteFailed: false, + Override: &controller.Override{Speed: 55, Reason: "burn-in", ExpiresAt: exp}, + ActiveHints: []*controller.WorkloadHint{{Source: "a"}, {Source: "b"}}, + } + + p := buildStatePayload(status) + + if p.CPUTemp == nil || *p.CPUTemp != 71 { + t.Fatalf("cpu_temp = %v, want 71", p.CPUTemp) + } + if p.GPUTemp == nil || *p.GPUTemp != 63 { + t.Fatalf("gpu_temp = %v, want 63", p.GPUTemp) + } + if p.FanSpeed != 40 || p.TargetSpeed != 55 { + t.Fatalf("speeds = %d/%d, want 40/55", p.FanSpeed, p.TargetSpeed) + } + if p.OverrideSpeed == nil || *p.OverrideSpeed != 55 { + t.Fatalf("override_speed = %v, want 55", p.OverrideSpeed) + } + if p.OverrideReason == nil || *p.OverrideReason != "burn-in" { + t.Fatalf("override_reason = %v, want burn-in", p.OverrideReason) + } + if p.OverrideExpires == nil || *p.OverrideExpires != exp.Format(time.RFC3339) { + t.Fatalf("override_expires = %v", p.OverrideExpires) + } + if p.ActiveHintCount != 2 { + t.Fatalf("active_hint_count = %d, want 2", p.ActiveHintCount) + } + if !p.FailsafeActive || p.FailsafeReason != "sensor-loss" || !p.RestorePending { + t.Fatalf("failsafe fields wrong: %+v", p) + } +} + +func TestBuildStatePayloadNilsBecomeNull(t *testing.T) { + p := buildStatePayload(&controller.Status{}) + b, err := json.Marshal(p) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var m map[string]any + if err := json.Unmarshal(b, &m); err != nil { + t.Fatalf("unmarshal: %v", err) + } + for _, k := range []string{"cpu_temp", "gpu_temp", "override_speed", "override_reason", "override_expires"} { + if m[k] != nil { + t.Fatalf("%s = %v, want null", k, m[k]) + } + } +} + +func TestPublishStatePublishesRetainedJSON(t *testing.T) { + h := &clientHolder{} + consumer := &fakeConsumer{status: &controller.Status{CurrentSpeed: 33, Zone: "idle"}} + b := New(testConfig(), consumer, h.factory) + b.Start() + + b.publishState() + + msg, ok := h.client.lastPublishOn("only-fan-controller/state") + if !ok { + t.Fatal("no publish on state topic") + } + if !msg.retained || msg.qos != 1 { + t.Fatalf("state publish retained=%v qos=%d, want true/1", msg.retained, msg.qos) + } + var m map[string]any + if err := json.Unmarshal(msg.payload, &m); err != nil { + t.Fatalf("state payload not valid JSON: %v", err) + } + if m["fan_speed"].(float64) != 33 || m["zone"].(string) != "idle" { + t.Fatalf("state payload wrong: %v", m) + } +} +``` + +- [x] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/mqtt/ -run 'TestBuildStatePayload|TestPublishState' -v` +Expected: FAIL — `buildStatePayload` and `publishState` undefined. + +- [x] **Step 3: Create the state payload and publisher** + +Create `internal/mqtt/state.go`: + +```go +package mqtt + +import ( + "encoding/json" + "log" + "time" + + "github.com/sethpjohnson/only-fan-controller/internal/controller" +) + +// statePayload is the retained JSON document published to /state on +// every tick. Pointer fields marshal to null when absent (nil sensor reading / +// no override) so HA templates can distinguish "no data" from zero. +type statePayload struct { + CPUTemp *int `json:"cpu_temp"` + GPUTemp *int `json:"gpu_temp"` + FanSpeed int `json:"fan_speed"` + TargetSpeed int `json:"target_speed"` + Zone string `json:"zone"` + Mode string `json:"mode"` + FailsafeActive bool `json:"failsafe_active"` + FailsafeReason string `json:"failsafe_reason"` + RestorePending bool `json:"restore_pending"` + LastWriteFailed bool `json:"last_write_failed"` + OverrideSpeed *int `json:"override_speed"` + OverrideReason *string `json:"override_reason"` + OverrideExpires *string `json:"override_expires"` + ActiveHintCount int `json:"active_hint_count"` +} + +// buildStatePayload derives the wire payload from a controller status snapshot. +func buildStatePayload(status *controller.Status) statePayload { + p := statePayload{ + FanSpeed: status.CurrentSpeed, + TargetSpeed: status.TargetSpeed, + Zone: status.Zone, + Mode: status.Mode, + FailsafeActive: status.FailsafeActive, + FailsafeReason: status.FailsafeReason, + RestorePending: status.RestorePending, + LastWriteFailed: status.LastWriteFailed, + ActiveHintCount: len(status.ActiveHints), + } + if status.CPU != nil { + v := status.CPU.Max + p.CPUTemp = &v + } + if status.GPU != nil { + v := status.GPU.Max + p.GPUTemp = &v + } + if status.Override != nil { + s := status.Override.Speed + r := status.Override.Reason + e := status.Override.ExpiresAt.Format(time.RFC3339) + p.OverrideSpeed = &s + p.OverrideReason = &r + p.OverrideExpires = &e + } + return p +} + +// publishState marshals the current status and publishes it retained to the +// state topic. A marshal or publish error is rate-limited and dropped — the +// next tick republishes anyway, so no buffering is needed. +func (b *Bridge) publishState() { + payload, err := json.Marshal(buildStatePayload(b.consumer.GetStatus())) + if err != nil { + if b.pubLimiter.allow() { + log.Printf("MQTT: failed to marshal state: %v", err) + } + return + } + if err := b.client.Publish(b.stateTopic(), 1, true, payload); err != nil { + if b.pubLimiter.allow() { + log.Printf("MQTT: failed to publish state: %v", err) + } + } +} + +// publishLoop publishes state on the monitoring interval until stopCh closes. +func (b *Bridge) publishLoop() { + defer close(b.doneCh) + interval := time.Duration(b.cfg.Monitoring.Interval) * time.Second + if interval <= 0 { + interval = 10 * time.Second + } + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-ticker.C: + b.publishState() + case <-b.stopCh: + return + } + } +} +``` + +- [x] **Step 4: Add the loop plumbing and rate limiter to the Bridge** + +In `internal/mqtt/bridge.go`, add `"sync"` to the import block, extend the `Bridge` struct, add the `rateLimiter` type, and wire the loop into `Start`/`Stop`. + +Replace the `Bridge` struct with: + +```go +type Bridge struct { + cfg *config.Config + consumer Consumer + newClient ClientFactory + client Client + stopCh chan struct{} + doneCh chan struct{} + pubLimiter rateLimiter +} +``` + +Add near the top of the file (after the constants): + +```go +// stopWaitTimeout bounds how long Stop waits for publishLoop to exit. +const stopWaitTimeout = 2 * time.Second + +// rateLimiter permits an action at most once per min interval. Used to keep +// repeated broker failures from flooding the log. +type rateLimiter struct { + mu sync.Mutex + last time.Time + min time.Duration +} + +func (r *rateLimiter) allow() bool { + r.mu.Lock() + defer r.mu.Unlock() + now := time.Now() + if r.min == 0 { + r.min = 30 * time.Second + } + if !r.last.IsZero() && now.Sub(r.last) < r.min { + return false + } + r.last = now + return true +} +``` + +In `Start`, after the `Connect` block, start the loop: + +```go + b.stopCh = make(chan struct{}) + b.doneCh = make(chan struct{}) + go b.publishLoop() +``` + +Replace `Stop` with a version that stops the loop first (bounded), then publishes offline and disconnects: + +```go +// Stop halts the publish loop (bounded by stopWaitTimeout), publishes the +// offline availability state, and disconnects. It is safe to call once. All +// waits are bounded so it can never delay the safety-critical shutdown path. +func (b *Bridge) Stop() { + if b.client == nil { + return + } + if b.stopCh != nil { + close(b.stopCh) + select { + case <-b.doneCh: + case <-time.After(stopWaitTimeout): + log.Printf("MQTT: publish loop did not stop within %s; abandoning it", stopWaitTimeout) + } + b.stopCh = nil + } + b.publishAvailability(false) + b.client.Disconnect(disconnectQuiesceMs) + log.Printf("MQTT: bridge stopped") +} +``` + +- [x] **Step 5: Run tests to verify they pass** + +Run: `go test ./internal/mqtt/ -run 'TestBuildStatePayload|TestPublishState|TestStart|TestStop' -v` +Expected: PASS (state tests plus the Task 2 lifecycle tests still green). + +- [x] **Step 6: Commit** + +```bash +git add internal/mqtt/state.go internal/mqtt/bridge.go internal/mqtt/state_test.go +git commit --no-gpg-sign -m "feat(mqtt): publish retained state JSON on the monitoring interval" +``` + +--- + +## Task 4: Discovery payloads — sensors, binary_sensors, number, button + +**Files:** +- Create: `internal/mqtt/discovery.go` +- Modify: `internal/mqtt/bridge.go` (`onConnect` publishes discovery) +- Test: `internal/mqtt/discovery_test.go` + +**Interfaces:** +- Consumes: `Bridge` + topic helpers (Task 2); `cfg.FanControl.MinSpeed/MaxSpeed`, `cfg.MQTT.ClientID/DiscoveryPrefix/BaseTopic`. +- Produces: + - `type discoveryEntity struct { topic string; payload []byte }` + - `func (b *Bridge) discoveryEntities() []discoveryEntity` + - `func (b *Bridge) publishDiscovery()` + - helpers: `deviceBlock()`, `discoveryTopic(component, objectID string) string`, `sensorConfig(...)`, `binarySensorConfig(...)`, `numberConfig()`, `buttonConfig()`. + +- [x] **Step 1: Write the failing discovery tests** + +Create `internal/mqtt/discovery_test.go`: + +```go +package mqtt + +import ( + "encoding/json" + "testing" +) + +func TestDiscoveryEntitiesCoverAllEntities(t *testing.T) { + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{}, h.factory) + + entities := b.discoveryEntities() + + wantTopics := []string{ + "homeassistant/sensor/only-fan-controller/cpu_temp/config", + "homeassistant/sensor/only-fan-controller/gpu_temp/config", + "homeassistant/sensor/only-fan-controller/fan_speed/config", + "homeassistant/sensor/only-fan-controller/target_speed/config", + "homeassistant/sensor/only-fan-controller/zone/config", + "homeassistant/sensor/only-fan-controller/failsafe_reason/config", + "homeassistant/binary_sensor/only-fan-controller/failsafe_active/config", + "homeassistant/binary_sensor/only-fan-controller/restore_pending/config", + "homeassistant/binary_sensor/only-fan-controller/last_write_failed/config", + "homeassistant/number/only-fan-controller/override_speed/config", + "homeassistant/button/only-fan-controller/override_clear/config", + } + got := map[string]bool{} + for _, e := range entities { + got[e.topic] = true + } + if len(entities) != len(wantTopics) { + t.Fatalf("got %d entities, want %d", len(entities), len(wantTopics)) + } + for _, w := range wantTopics { + if !got[w] { + t.Fatalf("missing discovery topic %q", w) + } + } +} + +func TestNumberEntityBoundsToConfiguredSpeeds(t *testing.T) { + cfg := testConfig() + cfg.FanControl.MinSpeed = 15 + cfg.FanControl.MaxSpeed = 90 + h := &clientHolder{} + b := New(cfg, &fakeConsumer{}, h.factory) + + var payload map[string]any + for _, e := range b.discoveryEntities() { + if e.topic == "homeassistant/number/only-fan-controller/override_speed/config" { + if err := json.Unmarshal(e.payload, &payload); err != nil { + t.Fatalf("unmarshal number config: %v", err) + } + } + } + if payload == nil { + t.Fatal("number entity not found") + } + if payload["min"].(float64) != 15 || payload["max"].(float64) != 90 { + t.Fatalf("number min/max = %v/%v, want 15/90", payload["min"], payload["max"]) + } + if payload["command_topic"].(string) != "only-fan-controller/cmd/override" { + t.Fatalf("number command_topic = %v", payload["command_topic"]) + } + dev := payload["device"].(map[string]any) + ids := dev["identifiers"].([]any) + if ids[0].(string) != "only-fan-controller" { + t.Fatalf("device identifier = %v, want only-fan-controller", ids[0]) + } +} + +func TestBinarySensorUsesProblemClassAndOnOff(t *testing.T) { + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{}, h.factory) + var payload map[string]any + for _, e := range b.discoveryEntities() { + if e.topic == "homeassistant/binary_sensor/only-fan-controller/failsafe_active/config" { + _ = json.Unmarshal(e.payload, &payload) + } + } + if payload["device_class"].(string) != "problem" { + t.Fatalf("device_class = %v, want problem", payload["device_class"]) + } + if payload["payload_on"].(string) != "ON" || payload["payload_off"].(string) != "OFF" { + t.Fatalf("payload_on/off = %v/%v", payload["payload_on"], payload["payload_off"]) + } +} + +func TestPublishDiscoveryOnConnect(t *testing.T) { + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{}, h.factory) + b.Start() + + msg, ok := h.client.lastPublishOn("homeassistant/sensor/only-fan-controller/cpu_temp/config") + if !ok { + t.Fatal("discovery not published on connect") + } + if !msg.retained || msg.qos != 1 { + t.Fatalf("discovery publish retained=%v qos=%d, want true/1", msg.retained, msg.qos) + } +} +``` + +- [x] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/mqtt/ -run 'TestDiscovery|TestNumberEntity|TestBinarySensor|TestPublishDiscovery' -v` +Expected: FAIL — `discoveryEntities` undefined. + +- [x] **Step 3: Create the discovery builders** + +Create `internal/mqtt/discovery.go`: + +```go +package mqtt + +import ( + "encoding/json" + "fmt" + "log" +) + +// discoveryEntity is a single retained HA MQTT Discovery config message. +type discoveryEntity struct { + topic string + payload []byte +} + +// deviceBlock is the shared HA device all entities belong to. Its identifier is +// the MQTT client_id, so every entity is grouped under one "Only Fan Controller" +// device in Home Assistant. +func (b *Bridge) deviceBlock() map[string]any { + return map[string]any{ + "identifiers": []string{b.cfg.MQTT.ClientID}, + "name": "Only Fan Controller", + "manufacturer": "only-fan-controller", + "model": "IPMI Fan Controller", + } +} + +// discoveryTopic builds ////config. +func (b *Bridge) discoveryTopic(component, objectID string) string { + return fmt.Sprintf("%s/%s/%s/%s/config", b.cfg.MQTT.DiscoveryPrefix, component, b.cfg.MQTT.ClientID, objectID) +} + +// baseEntity returns the availability/device fields common to every entity. +func (b *Bridge) baseEntity(objectID, name string) map[string]any { + return map[string]any{ + "name": name, + "unique_id": b.cfg.MQTT.ClientID + "_" + objectID, + "object_id": b.cfg.MQTT.ClientID + "_" + objectID, + "availability_topic": b.availabilityTopic(), + "payload_available": "online", + "payload_not_available": "offline", + "device": b.deviceBlock(), + } +} + +// sensorConfig builds a read-only sensor bound to a value_json key. +func (b *Bridge) sensorConfig(objectID, name, valueKey, deviceClass, unit, icon string) map[string]any { + e := b.baseEntity(objectID, name) + e["state_topic"] = b.stateTopic() + e["value_template"] = "{{ value_json." + valueKey + " }}" + if deviceClass != "" { + e["device_class"] = deviceClass + } + if unit != "" { + e["unit_of_measurement"] = unit + } + if icon != "" { + e["icon"] = icon + } + return e +} + +// binarySensorConfig builds an ON/OFF binary sensor from a boolean value_json +// key, using device_class "problem" (HA shows red when true). +func (b *Bridge) binarySensorConfig(objectID, name, valueKey string) map[string]any { + e := b.baseEntity(objectID, name) + e["state_topic"] = b.stateTopic() + e["value_template"] = "{{ 'ON' if value_json." + valueKey + " else 'OFF' }}" + e["payload_on"] = "ON" + e["payload_off"] = "OFF" + e["device_class"] = "problem" + return e +} + +// numberConfig builds the override-fan-speed number. min/max are bound to the +// configured MinSpeed/MaxSpeed so the HA UI cannot request an out-of-range value +// (the controller still clamps regardless). The command wraps the raw value into +// the cmd/override JSON with a default 1h duration. +func (b *Bridge) numberConfig() map[string]any { + e := b.baseEntity("override_speed", "Override Fan Speed") + e["command_topic"] = b.cmdOverrideTopic() + e["command_template"] = `{"speed": {{ value }}, "duration_seconds": 3600, "reason": "home assistant"}` + e["state_topic"] = b.stateTopic() + e["value_template"] = "{{ value_json.override_speed | default(0) }}" + e["min"] = b.cfg.FanControl.MinSpeed + e["max"] = b.cfg.FanControl.MaxSpeed + e["step"] = 1 + e["unit_of_measurement"] = "%" + e["mode"] = "slider" + e["icon"] = "mdi:fan" + return e +} + +// buttonConfig builds the clear-override button. +func (b *Bridge) buttonConfig() map[string]any { + e := b.baseEntity("override_clear", "Clear Fan Override") + e["command_topic"] = b.cmdOverrideClearTopic() + e["payload_press"] = "PRESS" + e["icon"] = "mdi:fan-off" + return e +} + +// discoveryEntities returns every HA discovery config message. Marshal failures +// (not expected for these primitive maps) are logged and skipped. +func (b *Bridge) discoveryEntities() []discoveryEntity { + type spec struct { + component string + objectID string + config map[string]any + } + specs := []spec{ + {"sensor", "cpu_temp", b.sensorConfig("cpu_temp", "CPU Temperature", "cpu_temp", "temperature", "°C", "")}, + {"sensor", "gpu_temp", b.sensorConfig("gpu_temp", "GPU Temperature", "gpu_temp", "temperature", "°C", "")}, + {"sensor", "fan_speed", b.sensorConfig("fan_speed", "Fan Speed", "fan_speed", "", "%", "mdi:fan")}, + {"sensor", "target_speed", b.sensorConfig("target_speed", "Target Fan Speed", "target_speed", "", "%", "mdi:fan")}, + {"sensor", "zone", b.sensorConfig("zone", "Thermal Zone", "zone", "", "", "mdi:thermometer")}, + {"sensor", "failsafe_reason", b.sensorConfig("failsafe_reason", "Failsafe Reason", "failsafe_reason", "", "", "mdi:shield-alert")}, + {"binary_sensor", "failsafe_active", b.binarySensorConfig("failsafe_active", "Failsafe Active", "failsafe_active")}, + {"binary_sensor", "restore_pending", b.binarySensorConfig("restore_pending", "Restore Pending", "restore_pending")}, + {"binary_sensor", "last_write_failed", b.binarySensorConfig("last_write_failed", "Last Fan Write Failed", "last_write_failed")}, + {"number", "override_speed", b.numberConfig()}, + {"button", "override_clear", b.buttonConfig()}, + } + entities := make([]discoveryEntity, 0, len(specs)) + for _, s := range specs { + payload, err := json.Marshal(s.config) + if err != nil { + log.Printf("MQTT: failed to marshal discovery for %s/%s: %v", s.component, s.objectID, err) + continue + } + entities = append(entities, discoveryEntity{ + topic: b.discoveryTopic(s.component, s.objectID), + payload: payload, + }) + } + return entities +} + +// publishDiscovery publishes all discovery config messages, retained. +func (b *Bridge) publishDiscovery() { + for _, e := range b.discoveryEntities() { + if err := b.client.Publish(e.topic, 1, true, e.payload); err != nil { + log.Printf("MQTT: failed to publish discovery %s: %v", e.topic, err) + } + } +} +``` + +- [x] **Step 4: Publish discovery on connect** + +In `internal/mqtt/bridge.go`, update `onConnect` to publish discovery before availability (so HA has the entities registered before it sees them go online): + +```go +func (b *Bridge) onConnect() { + b.publishDiscovery() + b.publishAvailability(true) +} +``` + +- [x] **Step 5: Run tests to verify they pass** + +Run: `go test ./internal/mqtt/ -run 'TestDiscovery|TestNumberEntity|TestBinarySensor|TestPublishDiscovery' -v` +Expected: PASS. + +- [x] **Step 6: Commit** + +```bash +git add internal/mqtt/discovery.go internal/mqtt/bridge.go internal/mqtt/discovery_test.go +git commit --no-gpg-sign -m "feat(mqtt): publish HA MQTT Discovery for all entities on connect" +``` + +--- + +## Task 5: Extract shared request validation into `internal/validate` + +**Files:** +- Create: `internal/validate/validate.go` +- Modify: `internal/api/server.go` +- Test: `internal/validate/validate_test.go` +- Modify: `internal/api/server_test.go:210` + +**Interfaces:** +- Consumes: nothing new. +- Produces: + - `const validate.MaxHintFieldLen = 64`, `const validate.MaxOverrideReasonLen = 128` + - `func validate.HintField(name, value string) error` + - `func validate.HintAction(action string) error` + - `func validate.Intensity(intensity string) error` + - `func validate.OverrideSpeed(speed int) error` + - `func validate.OverrideReason(reason string) error` + +- [x] **Step 1: Write the failing validation tests** + +Create `internal/validate/validate_test.go`: + +```go +package validate + +import "testing" + +func TestHintField(t *testing.T) { + if err := HintField("source", "plex.transcode-1"); err != nil { + t.Fatalf("valid source rejected: %v", err) + } + if err := HintField("source", "bad source!"); err == nil { + t.Fatal("source with space/bang should be rejected") + } + if err := HintField("type", string(make([]byte, MaxHintFieldLen+1))); err == nil { + t.Fatal("overlong type should be rejected") + } +} + +func TestHintAction(t *testing.T) { + if err := HintAction("start"); err != nil { + t.Fatalf("start rejected: %v", err) + } + if err := HintAction("stop"); err != nil { + t.Fatalf("stop rejected: %v", err) + } + if err := HintAction("pause"); err == nil { + t.Fatal("pause should be rejected") + } +} + +func TestIntensity(t *testing.T) { + for _, ok := range []string{"", "low", "medium", "high"} { + if err := Intensity(ok); err != nil { + t.Fatalf("intensity %q rejected: %v", ok, err) + } + } + if err := Intensity("extreme"); err == nil { + t.Fatal("extreme should be rejected") + } +} + +func TestOverrideSpeed(t *testing.T) { + for _, ok := range []int{0, 50, 100} { + if err := OverrideSpeed(ok); err != nil { + t.Fatalf("speed %d rejected: %v", ok, err) + } + } + if err := OverrideSpeed(-1); err == nil { + t.Fatal("negative speed should be rejected") + } + if err := OverrideSpeed(101); err == nil { + t.Fatal("speed > 100 should be rejected") + } +} + +func TestOverrideReason(t *testing.T) { + if err := OverrideReason("manual burn-in test (2h)"); err != nil { + t.Fatalf("normal prose rejected: %v", err) + } + if err := OverrideReason("bad\x00null"); err == nil { + t.Fatal("control character should be rejected") + } + if err := OverrideReason(string(make([]byte, MaxOverrideReasonLen+1))); err == nil { + t.Fatal("overlong reason should be rejected") + } +} +``` + +- [x] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/validate/ -v` +Expected: FAIL — package `internal/validate` does not exist (build error). + +- [x] **Step 3: Create the validate package** + +Create `internal/validate/validate.go`: + +```go +// Package validate holds request-field validation shared by the HTTP API and +// the MQTT bridge, so both control surfaces enforce identical rules (charset, +// length, closed sets, control-character rejection) on operator-supplied input. +package validate + +import ( + "fmt" + "regexp" + "unicode" +) + +const ( + // MaxHintFieldLen bounds free-form hint identifiers (source/type). Small on + // purpose: these are process names, not prose. + MaxHintFieldLen = 64 + // MaxOverrideReasonLen bounds the free-text override reason. + MaxOverrideReasonLen = 128 +) + +// hintFieldPattern is the allowed charset for hint source/type. Restricting to +// this set means no stored hint string can carry HTML/script even if a dashboard +// interpolation is ever missed. +var hintFieldPattern = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`) + +// allowedIntensities is the closed set of intensity values the controller +// understands ("" means "unspecified"). +var allowedIntensities = map[string]bool{"": true, "low": true, "medium": true, "high": true} + +// allowedHintActions is the closed set of hint actions the controller acts on. +var allowedHintActions = map[string]bool{"start": true, "stop": true} + +// HintField enforces length and charset on a hint source/type. name is used only +// for the error message. +func HintField(name, value string) error { + if len(value) > MaxHintFieldLen { + return fmt.Errorf("%s exceeds %d characters", name, MaxHintFieldLen) + } + if !hintFieldPattern.MatchString(value) { + return fmt.Errorf("%s must match [A-Za-z0-9_.-]", name) + } + return nil +} + +// HintAction enforces the closed set of hint actions. +func HintAction(action string) error { + if !allowedHintActions[action] { + return fmt.Errorf("action must be one of start, stop") + } + return nil +} + +// Intensity enforces the closed set of hint intensities. +func Intensity(intensity string) error { + if !allowedIntensities[intensity] { + return fmt.Errorf("intensity must be one of low, medium, high") + } + return nil +} + +// OverrideSpeed enforces the 0..100 percentage range. The controller still +// clamps to the configured min/max band; this only rejects nonsensical input. +func OverrideSpeed(speed int) error { + if speed < 0 || speed > 100 { + return fmt.Errorf("speed must be 0-100") + } + return nil +} + +// OverrideReason enforces a length cap and rejects control characters on the +// human-readable override reason. Normal punctuation, spaces, and quotes are +// valid free text. +func OverrideReason(reason string) error { + if len(reason) > MaxOverrideReasonLen { + return fmt.Errorf("reason exceeds %d characters", MaxOverrideReasonLen) + } + for _, r := range reason { + if unicode.IsControl(r) { + return fmt.Errorf("reason must not contain control characters") + } + } + return nil +} +``` + +- [x] **Step 4: Run validate tests to verify they pass** + +Run: `go test ./internal/validate/ -v` +Expected: PASS (all five tests). + +- [x] **Step 5: Refactor the API to use `internal/validate`** + +In `internal/api/server.go`: + +1. Delete these now-relocated declarations: `maxHintFieldLen`, `hintFieldPattern`, `allowedIntensities`, `allowedHintActions`, `maxOverrideReasonLen`, `validateOverrideReason`, and `validateHintField` (lines 23-56 and 223-231 in the original file). +2. Remove `"regexp"` and `"unicode"` from the import block (no longer used) and add `"github.com/sethpjohnson/only-fan-controller/internal/validate"`. +3. Replace `validateHintRequest` with: + +```go +// validateHintRequest enforces length, charset, and closed-set bounds on the +// client-controlled hint fields before they are stored or echoed back, using the +// shared validate package so the HTTP and MQTT surfaces agree. +func validateHintRequest(req *HintRequest) error { + if err := validate.HintField("source", req.Source); err != nil { + return err + } + if err := validate.HintField("type", req.Type); err != nil { + return err + } + if err := validate.HintAction(req.Action); err != nil { + return err + } + if err := validate.Intensity(req.Intensity); err != nil { + return err + } + if req.DurationEstimate < 0 { + return fmt.Errorf("duration_estimate must not be negative") + } + return nil +} +``` + +4. In `handleOverride`, replace the inline speed check and reason check: + +```go + if err := validate.OverrideSpeed(req.Speed); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + + if err := validate.OverrideReason(req.Reason); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } +``` + +- [x] **Step 6: Update the one API test that referenced the moved constant** + +In `internal/api/server_test.go`, line 210 references `maxOverrideReasonLen`. Change it to `validate.MaxOverrideReasonLen` and add `"github.com/sethpjohnson/only-fan-controller/internal/validate"` to that test file's import block: + +```go + {"overlong reason rejected", strings.Repeat("a", validate.MaxOverrideReasonLen+1), http.StatusBadRequest}, +``` + +- [x] **Step 7: Run the API and validate suites to verify nothing regressed** + +Run: `go test ./internal/api/ ./internal/validate/ -v` +Expected: PASS (API behavior unchanged; validation centralized). + +- [x] **Step 8: Commit** + +```bash +git add internal/validate/validate.go internal/validate/validate_test.go internal/api/server.go internal/api/server_test.go +git commit --no-gpg-sign -m "refactor: extract shared request validation into internal/validate" +``` + +--- + +## Task 6: Command handling — override / clear / hint over MQTT + +**Files:** +- Create: `internal/mqtt/command.go` +- Modify: `internal/mqtt/bridge.go` (`onConnect` subscribes to commands) +- Test: `internal/mqtt/command_test.go` + +**Interfaces:** +- Consumes: `Bridge`, topic helpers, `Consumer` (Task 2); `fakeClient.deliver` (Task 2 test helper); `validate.*` (Task 5); `controller.WorkloadHint`. +- Produces: + - `type overrideCommand struct { Speed int; DurationSeconds int; Reason string }` + - `type hintCommand struct { Type, Action, Intensity, Source string; DurationEstimate int }` + - `func (b *Bridge) subscribeCommands()` + - `func (b *Bridge) handleOverrideCommand(payload []byte)` + - `func (b *Bridge) handleClearCommand(payload []byte)` + - `func (b *Bridge) handleHintCommand(payload []byte)` + +- [x] **Step 1: Write the failing command tests** + +Create `internal/mqtt/command_test.go`: + +```go +package mqtt + +import ( + "testing" + "time" +) + +func startBridge(t *testing.T, consumer *fakeConsumer) *fakeClient { + t.Helper() + h := &clientHolder{} + b := New(testConfig(), consumer, h.factory) + b.Start() + return h.client +} + +func TestOverrideCommandRoutesToConsumer(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/override", + []byte(`{"speed": 60, "duration_seconds": 1800, "reason": "burn-in"}`)) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if !consumer.overrideSet { + t.Fatal("SetOverride was not called") + } + if consumer.overrideSpeed != 60 { + t.Fatalf("speed = %d, want 60", consumer.overrideSpeed) + } + if consumer.overrideDur != 1800*time.Second { + t.Fatalf("duration = %v, want 30m", consumer.overrideDur) + } + if consumer.overrideReason != "burn-in" { + t.Fatalf("reason = %q, want burn-in", consumer.overrideReason) + } +} + +func TestOverrideCommandRejectsOutOfRangeSpeed(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/override", []byte(`{"speed": 150}`)) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if consumer.overrideSet { + t.Fatal("SetOverride should not be called for out-of-range speed") + } +} + +func TestOverrideCommandRejectsBadJSON(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/override", []byte(`not json`)) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if consumer.overrideSet { + t.Fatal("SetOverride should not be called for invalid JSON") + } +} + +func TestClearCommandRoutesToConsumer(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/override/clear", []byte("PRESS")) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if !consumer.cleared { + t.Fatal("ClearOverride was not called") + } +} + +func TestHintCommandStartAddsHint(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/hint", + []byte(`{"type": "transcode", "action": "start", "intensity": "high", "source": "plex", "duration_estimate": 120}`)) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if len(consumer.addedHints) != 1 { + t.Fatalf("added hints = %d, want 1", len(consumer.addedHints)) + } + if consumer.addedHints[0].Source != "plex" || consumer.addedHints[0].Intensity != "high" { + t.Fatalf("hint wrong: %+v", consumer.addedHints[0]) + } + if consumer.addedHints[0].ExpiresAt.IsZero() { + t.Fatal("expected an expiry from duration_estimate") + } +} + +func TestHintCommandStopRemovesHint(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/hint", + []byte(`{"type": "transcode", "action": "stop", "source": "plex"}`)) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if len(consumer.removedSources) != 1 || consumer.removedSources[0] != "plex" { + t.Fatalf("removed sources = %v, want [plex]", consumer.removedSources) + } +} + +func TestHintCommandRejectsBadSource(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/hint", + []byte(`{"type": "transcode", "action": "start", "source": "bad source!"}`)) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if len(consumer.addedHints) != 0 { + t.Fatal("hint with invalid source should be rejected") + } +} + +func TestSubscribeCommandsRegistersAllTopics(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.mu.Lock() + defer client.mu.Unlock() + for _, topic := range []string{ + "only-fan-controller/cmd/override", + "only-fan-controller/cmd/override/clear", + "only-fan-controller/cmd/hint", + } { + if _, ok := client.subs[topic]; !ok { + t.Fatalf("not subscribed to %q", topic) + } + } +} +``` + +- [x] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/mqtt/ -run 'TestOverrideCommand|TestClearCommand|TestHintCommand|TestSubscribeCommands' -v` +Expected: FAIL — `subscribeCommands`/`handle*Command` undefined and no command subscriptions registered. + +- [x] **Step 3: Create the command handlers** + +Create `internal/mqtt/command.go`: + +```go +package mqtt + +import ( + "encoding/json" + "log" + "time" + + "github.com/sethpjohnson/only-fan-controller/internal/controller" + "github.com/sethpjohnson/only-fan-controller/internal/validate" +) + +// overrideCommand is the JSON schema for /cmd/override. +type overrideCommand struct { + Speed int `json:"speed"` + DurationSeconds int `json:"duration_seconds"` + Reason string `json:"reason"` +} + +// hintCommand is the JSON schema for /cmd/hint, mirroring the HTTP +// POST /api/hint body. +type hintCommand struct { + Type string `json:"type"` + Action string `json:"action"` + Intensity string `json:"intensity"` + Source string `json:"source"` + DurationEstimate int `json:"duration_estimate"` +} + +// subscribeCommands registers QoS-1 handlers for the three command topics. It +// runs from onConnect, so it re-subscribes on every (re)connection. +func (b *Bridge) subscribeCommands() { + subs := []struct { + topic string + handler MessageHandler + }{ + {b.cmdOverrideTopic(), func(_ string, p []byte) { b.handleOverrideCommand(p) }}, + {b.cmdOverrideClearTopic(), func(_ string, p []byte) { b.handleClearCommand(p) }}, + {b.cmdHintTopic(), func(_ string, p []byte) { b.handleHintCommand(p) }}, + } + for _, s := range subs { + if err := b.client.Subscribe(s.topic, 1, s.handler); err != nil { + log.Printf("MQTT: failed to subscribe to %s: %v", s.topic, err) + } + } +} + +// handleOverrideCommand parses, validates, and applies an override. Validation +// reuses the shared validate package; final clamping/cap live in the controller. +func (b *Bridge) handleOverrideCommand(payload []byte) { + var cmd overrideCommand + if err := json.Unmarshal(payload, &cmd); err != nil { + log.Printf("MQTT: invalid override command: %v", err) + return + } + if err := validate.OverrideSpeed(cmd.Speed); err != nil { + log.Printf("MQTT: rejecting override command: %v", err) + return + } + if err := validate.OverrideReason(cmd.Reason); err != nil { + log.Printf("MQTT: rejecting override command: %v", err) + return + } + duration := time.Duration(cmd.DurationSeconds) * time.Second + b.consumer.SetOverride(cmd.Speed, duration, cmd.Reason) + log.Printf("MQTT: override set via command: %d%% (%s)", cmd.Speed, cmd.Reason) +} + +// handleClearCommand clears any active override. The payload is ignored (any +// message on the clear topic triggers it), mirroring an HA button press. +func (b *Bridge) handleClearCommand(_ []byte) { + b.consumer.ClearOverride() + log.Printf("MQTT: override cleared via command") +} + +// handleHintCommand parses, validates, and applies a workload hint. action +// "stop" removes the hint by source; anything else starts it. +func (b *Bridge) handleHintCommand(payload []byte) { + var cmd hintCommand + if err := json.Unmarshal(payload, &cmd); err != nil { + log.Printf("MQTT: invalid hint command: %v", err) + return + } + if err := validate.HintField("source", cmd.Source); err != nil { + log.Printf("MQTT: rejecting hint command: %v", err) + return + } + if err := validate.HintField("type", cmd.Type); err != nil { + log.Printf("MQTT: rejecting hint command: %v", err) + return + } + if err := validate.HintAction(cmd.Action); err != nil { + log.Printf("MQTT: rejecting hint command: %v", err) + return + } + if err := validate.Intensity(cmd.Intensity); err != nil { + log.Printf("MQTT: rejecting hint command: %v", err) + return + } + if cmd.DurationEstimate < 0 { + log.Printf("MQTT: rejecting hint command: duration_estimate must not be negative") + return + } + + if cmd.Action == "stop" { + b.consumer.RemoveHint(cmd.Source) + log.Printf("MQTT: hint removed via command: %s", cmd.Source) + return + } + + hint := &controller.WorkloadHint{ + Type: cmd.Type, + Action: cmd.Action, + Intensity: cmd.Intensity, + Source: cmd.Source, + } + if cmd.DurationEstimate > 0 { + hint.ExpiresAt = time.Now().Add(time.Duration(cmd.DurationEstimate) * time.Second) + } + b.consumer.AddHint(hint) + log.Printf("MQTT: hint registered via command: %s from %s", cmd.Action, cmd.Source) +} +``` + +- [x] **Step 4: Subscribe to commands on connect** + +In `internal/mqtt/bridge.go`, update `onConnect` to also subscribe: + +```go +func (b *Bridge) onConnect() { + b.publishDiscovery() + b.subscribeCommands() + b.publishAvailability(true) +} +``` + +- [x] **Step 5: Run tests to verify they pass** + +Run: `go test ./internal/mqtt/ -run 'TestOverrideCommand|TestClearCommand|TestHintCommand|TestSubscribeCommands' -v` +Expected: PASS. + +- [x] **Step 6: Run the whole mqtt package suite** + +Run: `go test ./internal/mqtt/ -v` +Expected: PASS (all unit tests across Tasks 2-6). + +- [x] **Step 7: Commit** + +```bash +git add internal/mqtt/command.go internal/mqtt/bridge.go internal/mqtt/command_test.go +git commit --no-gpg-sign -m "feat(mqtt): route override/clear/hint commands with shared validation" +``` + +--- + +## Task 7: Wire the bridge into `main.go` with shutdown sequencing + +**Files:** +- Modify: `cmd/controller/main.go` (`run()`) + +**Interfaces:** +- Consumes: `mqtt.New`, `mqtt.NewPahoClient`, `(*Bridge).Start`, `(*Bridge).Stop` (Tasks 2-6); `*controller.FanController` (satisfies `mqtt.Consumer`); `cfg.MQTT.Enabled` (Task 1). +- Produces: no new exported API; the running program starts/stops the bridge. + +- [x] **Step 1: Add the mqtt import** + +In `cmd/controller/main.go`, add to the import block: + +```go + "github.com/sethpjohnson/only-fan-controller/internal/mqtt" +``` + +- [x] **Step 2: Add the shutdown-timeout constant** + +Near `cleanupShutdownTimeout` (line 135), add: + +```go +// mqttShutdownTimeout bounds how long shutdown waits for the MQTT bridge to +// publish offline + disconnect. Thermal safety (restore()) always runs before +// this, so a wedged broker must never stall process exit. +const mqttShutdownTimeout = 3 * time.Second +``` + +- [x] **Step 3: Start the bridge after the API server starts** + +In `run()`, after the `log.Printf("Dashboard: ...")` line (around line 274) and before the signal-handling block, add: + +```go + // Optional MQTT / Home Assistant bridge. Off unless mqtt.enabled. It talks to + // the controller only through the exported Consumer methods (the same ones the + // HTTP handlers use), so a hung/unreachable broker can never stall fan control. + // fanCtrl is a *controller.FanController in both real and demo mode, so the + // bridge (and thus HA) works in demo mode too. + var mqttBridge *mqtt.Bridge + if cfg.MQTT.Enabled { + mqttBridge = mqtt.New(cfg, fanCtrl, mqtt.NewPahoClient) + mqttBridge.Start() + } +``` + +- [x] **Step 4: Stop the bridge after restore(), bounded** + +In `run()`, immediately after the `restore()` block (the `if err := restore(); err != nil { ... }` around lines 293-295) and before the history-cleanup shutdown, add: + +```go + // Tear down the MQTT bridge AFTER the BMC hand-back choke point, bounded so a + // wedged broker cannot delay exit. Mirrors the history-cleanup shutdown pattern. + if mqttBridge != nil { + stopped := make(chan struct{}) + go func() { + mqttBridge.Stop() + close(stopped) + }() + select { + case <-stopped: + case <-time.After(mqttShutdownTimeout): + log.Printf("Warning: MQTT bridge did not stop within %s; abandoning it", mqttShutdownTimeout) + } + } +``` + +- [x] **Step 5: Verify the program builds and the existing main tests pass** + +Run: `go build ./... && go test ./cmd/controller/ -v` +Expected: build succeeds; all `cmd/controller` tests PASS (including `TestApplyEnvOverridesMQTT` from Task 1). + +- [x] **Step 6: Smoke-test that demo mode with MQTT disabled behaves identically** + +Run: `go run ./cmd/controller -demo -config /nonexistent.yaml 2>&1 | head -n 12` +Expected: normal demo startup logs; NO `MQTT:` lines appear (bridge is off by default). Stop with Ctrl-C. + +- [x] **Step 7: Commit** + +```bash +git add cmd/controller/main.go +git commit --no-gpg-sign -m "feat(mqtt): wire bridge into run() with bounded post-restore shutdown" +``` + +--- + +## Task 8: Integration test with an in-process mochi-mqtt broker + +**Files:** +- Create: `internal/mqtt/integration_test.go` +- Modify: `go.mod`, `go.sum` (add `github.com/mochi-mqtt/server/v2` test dep) + +**Interfaces:** +- Consumes: everything from Tasks 2-6 (real `NewPahoClient`, `New`, `Start`, `Stop`, `publishState`); `fakeConsumer` (Task 2 test helper). +- Produces: no production code; a real connect → discovery → command → state → LWT round-trip. + +- [x] **Step 1: Add the mochi-mqtt test dependency** + +Run: +```bash +go get github.com/mochi-mqtt/server/v2@v2.6.6 +``` +Expected: `go.mod`/`go.sum` updated with `github.com/mochi-mqtt/server/v2 v2.6.6`. + +- [x] **Step 2: Write the integration test** + +Create `internal/mqtt/integration_test.go`: + +```go +package mqtt + +import ( + "errors" + "net" + "sync" + "testing" + "time" + + paho "github.com/eclipse/paho.mqtt.golang" + mochi "github.com/mochi-mqtt/server/v2" + "github.com/mochi-mqtt/server/v2/hooks/auth" + "github.com/mochi-mqtt/server/v2/listeners" + + "github.com/sethpjohnson/only-fan-controller/internal/config" + "github.com/sethpjohnson/only-fan-controller/internal/controller" + "github.com/sethpjohnson/only-fan-controller/internal/monitor" +) + +// freeAddr returns a currently-free 127.0.0.1 TCP address for the broker. +func freeAddr(t *testing.T) string { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("failed to grab a free port: %v", err) + } + addr := ln.Addr().String() + _ = ln.Close() + return addr +} + +// startBroker starts an in-process mochi broker on addr and returns it. +func startBroker(t *testing.T, addr string) *mochi.Server { + t.Helper() + server := mochi.New(&mochi.Options{InlineClient: false}) + if err := server.AddHook(new(auth.AllowHook), nil); err != nil { + t.Fatalf("add auth hook: %v", err) + } + tcp := listeners.NewTCP(listeners.Config{ID: "t1", Address: addr}) + if err := server.AddListener(tcp); err != nil { + t.Fatalf("add listener: %v", err) + } + go func() { _ = server.Serve() }() + t.Cleanup(func() { _ = server.Close() }) + return server +} + +// observer subscribes to "#" and records the FULL per-topic history, so the test +// can assert on discovery/state/availability traffic. History (not just the +// latest value) matters for the LWT check: paho auto-reconnects and republishes +// "online" moments after the forced disconnect, so we must assert that "offline" +// appeared at some point, not that it is the latest value. +type observer struct { + mu sync.Mutex + history map[string][]string + client paho.Client +} + +func newObserver(t *testing.T, broker string) *observer { + t.Helper() + o := &observer{history: map[string][]string{}} + opts := paho.NewClientOptions().AddBroker(broker).SetClientID("observer") + o.client = paho.NewClient(opts) + tok := o.client.Connect() + if !tok.WaitTimeout(3*time.Second) || tok.Error() != nil { + t.Fatalf("observer connect failed: %v", tok.Error()) + } + sub := o.client.Subscribe("#", 1, func(_ paho.Client, m paho.Message) { + o.mu.Lock() + o.history[m.Topic()] = append(o.history[m.Topic()], string(m.Payload())) + o.mu.Unlock() + }) + sub.Wait() + t.Cleanup(func() { o.client.Disconnect(100) }) + return o +} + +// latest returns the most recent payload seen on topic. +func (o *observer) latest(topic string) (string, bool) { + o.mu.Lock() + defer o.mu.Unlock() + h := o.history[topic] + if len(h) == 0 { + return "", false + } + return h[len(h)-1], true +} + +// seen reports whether payload was ever observed on topic. +func (o *observer) seen(topic, payload string) bool { + o.mu.Lock() + defer o.mu.Unlock() + for _, p := range o.history[topic] { + if p == payload { + return true + } + } + return false +} + +// waitFor polls cond up to 3s; fails the test with msg otherwise. +func waitFor(t *testing.T, msg string, cond func() bool) { + t.Helper() + deadline := time.Now().Add(3 * time.Second) + for time.Now().Before(deadline) { + if cond() { + return + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("timed out waiting for %s", msg) +} + +func TestIntegrationConnectDiscoveryCommandStateLWT(t *testing.T) { + addr := freeAddr(t) + server := startBroker(t, addr) + broker := "tcp://" + addr + + obs := newObserver(t, broker) + + cfg := config.Default() + cfg.MQTT.Enabled = true + cfg.MQTT.Broker = broker + cfg.Monitoring.Interval = 1 + consumer := &fakeConsumer{status: &controller.Status{ + CPU: &monitor.CPUReading{Max: 55}, + CurrentSpeed: 42, + Zone: "warm", + }} + + bridge := New(cfg, consumer, NewPahoClient) + bridge.Start() + t.Cleanup(func() { bridge.Stop() }) + + // 1. Availability online + discovery published on connect. + waitFor(t, "availability online", func() bool { + return obs.seen("only-fan-controller/availability", "online") + }) + waitFor(t, "cpu_temp discovery", func() bool { + _, ok := obs.latest("homeassistant/sensor/only-fan-controller/cpu_temp/config") + return ok + }) + + // 2. Command round-trip: publish an override command, expect SetOverride. + pub := obs.client.Publish("only-fan-controller/cmd/override", 1, false, + []byte(`{"speed": 70, "duration_seconds": 600, "reason": "integration"}`)) + pub.Wait() + waitFor(t, "SetOverride called", func() bool { + consumer.mu.Lock() + defer consumer.mu.Unlock() + return consumer.overrideSet && consumer.overrideSpeed == 70 + }) + + // 3. State publish (ticker at 1s interval) reaches the state topic. + waitFor(t, "state published", func() bool { + v, ok := obs.latest("only-fan-controller/state") + return ok && len(v) > 0 + }) + + // 4. LWT: forcibly stop the bridge's broker-side connection (ungraceful) and + // expect the retained will "offline" to be published. + waitFor(t, "bridge client registered on broker", func() bool { + _, ok := server.Clients.Get(cfg.MQTT.ClientID) + return ok + }) + cl, _ := server.Clients.Get(cfg.MQTT.ClientID) + cl.Stop(errors.New("integration: force ungraceful disconnect")) + waitFor(t, "LWT offline", func() bool { + return obs.seen("only-fan-controller/availability", "offline") + }) +} +``` + +The import block above is complete: `config` (`config.Default()`), `controller` (`controller.Status`), and `monitor` (`monitor.CPUReading`) are all listed, and `fmt` is deliberately absent. If a future edit adds an unused import, `go vet` will flag it — keep the block tight. + +- [x] **Step 3: Run the integration test** + +Run: `go test ./internal/mqtt/ -run TestIntegration -v` +Expected: PASS — the log shows connect, discovery, command routing, state, and the LWT offline message observed. + +- [x] **Step 4: Run the full mqtt suite (unit + integration) with the race detector** + +Run: `go test ./internal/mqtt/ -race -v` +Expected: PASS with no data races. + +- [x] **Step 5: Commit** + +```bash +git add go.mod go.sum internal/mqtt/integration_test.go +git commit --no-gpg-sign -m "test(mqtt): in-process mochi broker integration (discovery/command/state/LWT)" +``` + +--- + +## Task 9: Documentation, Unraid template, and `/api/config` leak regression + +**Files:** +- Modify: `README.md` +- Modify: `unraid/only-fan-controller.xml` +- Test: `internal/api/server_test.go` (add `/api/config` password-leak regression) + +**Interfaces:** +- Consumes: `handleGetConfig` (existing); the running server test harness in `server_test.go`. +- Produces: docs + a regression test guaranteeing `/api/config` never exposes `mqtt.password`. + +- [x] **Step 1: Write the failing `/api/config` regression test** + +Add to `internal/api/server_test.go`. This assumes the existing test harness exposes a way to build a server and issue a request; use the same pattern the other tests in that file use (a `*config.Config`, `NewServer`, and `httptest`). Concretely: + +```go +func TestGetConfigDoesNotLeakMQTTPassword(t *testing.T) { + cfg := config.Default() + cfg.MQTT.Enabled = true + cfg.MQTT.Broker = "tcp://10.0.0.5:1883" + cfg.MQTT.Password = "super-secret-broker-pw" + + srv := NewServer(cfg, &controller.FanController{}, &storage.Store{}) + + req := httptest.NewRequest(http.MethodGet, "/api/config", nil) + rec := httptest.NewRecorder() + srv.router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("GET /api/config = %d, want 200", rec.Code) + } + if strings.Contains(rec.Body.String(), "super-secret-broker-pw") { + t.Fatalf("/api/config leaked mqtt.password: %s", rec.Body.String()) + } +} +``` + +Ensure the test file imports `net/http`, `net/http/httptest`, `strings`, `github.com/sethpjohnson/only-fan-controller/internal/config`, `.../internal/controller`, and `.../internal/storage`. If `server_test.go` already constructs servers via a local helper (e.g. `newTestServer(t, cfg)`), use that helper instead of calling `NewServer` directly with zero-value dependencies, and drop the unused imports. (Read the top of `server_test.go` first and match its established construction pattern; the assertion — body must not contain the password — is the load-bearing part.) + +- [x] **Step 2: Run the regression test to verify it passes** + +Run: `go test ./internal/api/ -run TestGetConfigDoesNotLeakMQTTPassword -v` +Expected: PASS immediately — `handleGetConfig` builds a hand-picked map that never includes `mqtt`, so the password cannot appear. (This test locks that behavior in against future edits; if it FAILS, `handleGetConfig` was changed to include MQTT and must be corrected.) + +- [x] **Step 3: Add the "Home Assistant (MQTT)" section to the README** + +Insert a new section in `README.md` after the `## API Security` section (before `## API Endpoints`, around line 157): + +````markdown +## Home Assistant (MQTT) + +Optional. Off by default. When enabled, the controller connects to your MQTT +broker, self-registers in Home Assistant via **MQTT Discovery**, publishes its +state every control tick, and accepts fan overrides and workload hints over MQTT +— full parity with the HTTP API. + +Enable it in `config.yaml`: + +```yaml +mqtt: + enabled: true + broker: "tcp://192.168.1.10:1883" + username: "homeassistant" + password: "your-broker-password" + client_id: "only-fan-controller" + base_topic: "only-fan-controller" + discovery_prefix: "homeassistant" +``` + +Or via environment variables: `MQTT_ENABLED=true`, `MQTT_BROKER=tcp://…`, +`MQTT_USERNAME=…`, `MQTT_PASSWORD=…`. + +### What appears in Home Assistant + +All entities are grouped under one device, **Only Fan Controller**: + +| Entity | Type | Notes | +|--------|------|-------| +| CPU Temperature, GPU Temperature | sensor | °C | +| Fan Speed, Target Fan Speed | sensor | % | +| Thermal Zone, Failsafe Reason | sensor | text | +| Failsafe Active, Restore Pending, Last Fan Write Failed | binary_sensor | `problem` class | +| Override Fan Speed | number | slider bound to `min_speed`/`max_speed`; sends a 1-hour override | +| Clear Fan Override | button | clears any active override | + +The device is marked **unavailable** the instant the process dies — the broker +publishes the Last Will (`offline`) on ungraceful disconnect, giving you free +external monitoring for the fail-safe scenarios. + +### Topics + +- `only-fan-controller/availability` — retained `online`/`offline`. +- `only-fan-controller/state` — retained JSON, one document per control tick. +- `only-fan-controller/cmd/override` — `{"speed": 60, "duration_seconds": 3600, "reason": "..."}` +- `only-fan-controller/cmd/override/clear` — any payload clears the override. +- `only-fan-controller/cmd/hint` — `{"type": "transcode", "action": "start|stop", "intensity": "high", "source": "plex", "duration_estimate": 120}` + +Commands go through the exact same safety clamps and validation as the HTTP API: +speed is clamped to `min_speed`/`max_speed`, override duration is capped at 24h, +the critical-temperature ramp overrides everything, and hint fields are charset/ +length checked. A malformed command is logged and dropped, never applied. + +Hints have no dedicated HA entity (a source/type/intensity tuple doesn't map to +one) — drive `cmd/hint` from an HA automation for the full-parity escape hatch. + +### Security + +**MQTT command authorization is your broker's authentication.** The HTTP bearer +token is HTTP-only and does not apply here — anyone who can publish to the broker +can command the fans, so protect it with broker credentials/ACLs. The blast +radius is still bounded by the controller-level clamps and the critical-temp +override. **There is no TLS in v1**; keep the broker on a trusted LAN or in front +of a TLS-terminating proxy. +```` + +- [x] **Step 4: Add the MQTT env-var rows to the README table** + +In the `## Environment Variables` table (around line 319), add after the `API_TOKEN` row: + +```markdown +| `MQTT_ENABLED` | Enable Home Assistant MQTT bridge | false | +| `MQTT_BROKER` | MQTT broker URL (`tcp://host:port`) | - (required when enabled) | +| `MQTT_USERNAME` | MQTT broker username | - | +| `MQTT_PASSWORD` | MQTT broker password | - | +``` + +- [x] **Step 5: Add the MQTT fields to the Unraid template** + +In `unraid/only-fan-controller.xml`, add these `` elements after the `API Token` entry (before `Enable GPU Monitoring`): + +```xml + false + + + +``` + +- [x] **Step 6: Verify the full test suite passes** + +Run: `go test ./...` +Expected: PASS across all packages (`config`, `api`, `validate`, `mqtt`, `controller`, `cmd/controller`, …). + +- [x] **Step 7: Commit** + +```bash +git add README.md unraid/only-fan-controller.xml internal/api/server_test.go +git commit --no-gpg-sign -m "docs(mqtt): README HA section, Unraid MQTT fields, /api/config leak regression" +``` + +--- + +## Final verification + +- [ ] Run the whole suite with the race detector: `go test ./... -race` + Expected: PASS, no data races. +- [ ] `go vet ./...` — no findings. +- [ ] `go build ./...` — succeeds. +- [ ] Confirm `mqtt.enabled: false` (default) produces zero `MQTT:` log lines in a demo run (Task 7 Step 6) — the off-by-default guarantee. diff --git a/docs/superpowers/specs/2026-07-17-mqtt-ha-design.md b/docs/superpowers/specs/2026-07-17-mqtt-ha-design.md new file mode 100644 index 0000000..fdc7207 --- /dev/null +++ b/docs/superpowers/specs/2026-07-17-mqtt-ha-design.md @@ -0,0 +1,149 @@ +# MQTT / Home Assistant Support — Design + +Date: 2026-07-17 +Status: Approved by Seth (brainstorm session) +Base: stacked on the fail-safe hardening, API security, deps, and housekeeping branches (PRs #1–#5) + +## Goal + +Optional MQTT support so the controller's status appears in Home Assistant as a +self-registered device (MQTT Discovery), with full control parity: everything +the HTTP API can do (overrides, hints) is also possible over MQTT. Off by +default; zero behavior change when disabled. + +## Decisions (from brainstorm Q&A) + +| Question | Decision | +|---|---| +| Control scope | Full parity: sensors + override control + hints | +| HA registration | MQTT Discovery (retained config topics under `homeassistant/`) | +| Broker security | Username/password, no TLS (documented; consistent with project stance) | +| Publish cadence | Every control tick (`monitoring.interval`), retained state, LWT availability | +| Architecture | Decoupled bridge package — broker problems can never stall the control loop | + +## Architecture + +New package `internal/mqtt` using `github.com/eclipse/paho.mqtt.golang` +(standard Go client, built-in auto-reconnect). + +- `mqtt.Bridge` consumes a small, bridge-defined consumer interface with + exactly the controller methods it needs: `GetStatus()`, `SetOverride()`, + `ClearOverride()`, `AddHint()`, `RemoveHint()` — the same methods the HTTP + handlers call. All safety semantics (min/max clamping, 24h override cap, + critical-temp ramp overriding everything, hint validation) live in the + controller and therefore apply to MQTT commands automatically. The bridge + re-implements none of them. +- Wired in `cmd/controller/main.go` `run()`, gated on `mqtt.enabled` + (default `false`). The controller package remains MQTT-unaware. +- The bridge runs in its own goroutine(s). A hung, slow, or unreachable broker + cannot affect fan control by construction: no shared locks with the control + loop beyond the existing `GetStatus` read path, no unbounded queues, no + blocking publishes from loop context. + +## Configuration + +New `mqtt:` section in the YAML config: + +```yaml +mqtt: + enabled: false # off by default + broker: "tcp://192.168.1.x:1883" # required when enabled + username: "" # optional + password: "" # optional; never exposed via /api/config + client_id: "only-fan-controller" # default + base_topic: "only-fan-controller" # default; state/command topic root + discovery_prefix: "homeassistant" # default; HA discovery root +``` + +- Env overrides: `MQTT_ENABLED`, `MQTT_BROKER`, `MQTT_USERNAME`, + `MQTT_PASSWORD` (following the existing `applyEnvOverrides` pattern). +- `config.Validate()`: `broker` must be non-empty and parseable when + `enabled`; invalid config keeps the existing refuse-to-start behavior. +- `Password` gets `json:"-"` (same treatment as the iDRAC password and API + token). `/api/config` must not leak it (regression test). +- Publish cadence deliberately reuses `monitoring.interval` — no separate + interval knob (YAGNI). + +## Topics & entities + +Availability: `/availability` — retained `online`, LWT `offline`. +HA marks the entire device unavailable the moment the process dies (free +external monitoring for the fail-safe scenarios). + +State: `/state` — one retained JSON document per tick, derived +from `GetStatus()`: cpu max temp, gpu max temp, current fan %, target %, zone, +failsafe_active, failsafe_reason, restore_pending, last_write_failed, override +(speed/reason/expires), active hint count. + +Commands (QoS 1 subscriptions): +- `/cmd/override` — JSON `{"speed": N, "duration_seconds": N, "reason": "..."}` + → `SetOverride` (controller clamps speed, caps duration, validates reason + using the same rules as HTTP). +- `/cmd/override/clear` — any payload → `ClearOverride`. +- `/cmd/hint` — JSON matching `POST /api/hint`'s schema, same + validation (charset/length/closed sets), `action: start|stop`. + +Discovery (retained, republished on every (re)connect, under +`/…`), all entities grouped into one HA device +("Only Fan Controller", identifier from `client_id`): +- `sensor`: cpu_temp, gpu_temp, fan_speed, target_speed, zone, failsafe_reason +- `binary_sensor`: failsafe_active, restore_pending, last_write_failed +- `number`: override fan speed — min/max bound to the configured + `min_speed`/`max_speed` so the HA UI cannot request an out-of-range value + (the controller still clamps regardless); command topic maps to + `cmd/override` with a default duration (1h, documented). +- `button`: clear override → `cmd/override/clear`. + +Hints intentionally have no dedicated HA entity (a source/type/intensity tuple +doesn't map to an entity type); HA automations use the JSON command topic. +This is the "full parity" escape hatch. + +## Error handling & lifecycle + +- Auto-reconnect with paho's built-in backoff; discovery + availability + republished on reconnect. +- Broker unreachable at startup: service starts normally; bridge connects in + the background and logs (rate-limited) until it succeeds. +- Publish failure: log (rate-limited) and drop — state is republished next + tick anyway. No unbounded buffering. +- Shutdown: mirrors the history-cleanup pattern — sequenced AFTER the BMC + `restore()` choke point, publishes `offline` + disconnects with a short + bounded timeout (abandon and log if exceeded). MQTT can never delay the + safety-critical hand-back. + +## Security + +- MQTT command authorization = broker authentication. The HTTP bearer token is + HTTP-only. README states plainly: anyone who can publish to the broker can + command the fans — use broker credentials/ACLs. Blast radius remains bounded + by controller-level clamps and the critical-temp override in all cases. +- No TLS support in v1 (documented limitation, consistent with project + stance; revisit if a remote broker is ever needed). + +## Testing + +- Unit: fake client behind a bridge-defined publisher/subscriber interface — + discovery payload correctness, state JSON shape, command validation and + routing (including rejection paths), config validation. +- Integration: in-process broker (`mochi-mqtt/server`) — real + connect → discovery → command → state round-trip, LWT verification on + ungraceful disconnect. No Docker required. +- Demo mode works with MQTT enabled (bridge publishes mock data), so + acceptance can drive the full flow live against a real or in-process broker. +- Regression: `/api/config` does not leak `mqtt.password`; `mqtt.enabled: + false` produces zero MQTT activity; control-loop behavior identical with + bridge enabled and broker unreachable. + +## Documentation + +- README: "Home Assistant (MQTT)" section — broker setup, discovery behavior, + entity list, command topic examples, the security note above. +- `config.example.yaml`: full `mqtt:` block, commented (must still validate + verbatim — extend the existing regression test). +- Unraid template: `MQTT_ENABLED`, `MQTT_BROKER`, `MQTT_USERNAME`, + `MQTT_PASSWORD` (masked) fields. + +## Out of scope (explicit) + +- TLS/mqtts, HA entity for hints, separate publish interval, MQTT v5-specific + features, bridging history data, HA config-flow/native integration. diff --git a/go.mod b/go.mod index da8fc80..209cc54 100644 --- a/go.mod +++ b/go.mod @@ -5,8 +5,10 @@ go 1.25.0 toolchain go1.25.12 require ( + github.com/eclipse/paho.mqtt.golang v1.5.0 github.com/gin-gonic/gin v1.12.0 github.com/mattn/go-sqlite3 v1.14.48 + github.com/mochi-mqtt/server/v2 v2.6.6 gopkg.in/yaml.v3 v3.0.1 ) @@ -22,6 +24,7 @@ require ( github.com/go-playground/validator/v10 v10.30.3 // indirect github.com/goccy/go-json v0.10.6 // indirect github.com/goccy/go-yaml v1.19.2 // indirect + github.com/gorilla/websocket v1.5.3 // indirect github.com/json-iterator/go v1.1.12 // indirect github.com/klauspost/cpuid/v2 v2.4.0 // indirect github.com/leodido/go-urn v1.4.0 // indirect @@ -31,12 +34,14 @@ require ( github.com/pelletier/go-toml/v2 v2.4.3 // indirect github.com/quic-go/qpack v0.6.0 // indirect github.com/quic-go/quic-go v0.60.0 // indirect + github.com/rs/xid v1.4.0 // indirect github.com/twitchyliquid64/golang-asm v0.15.1 // indirect github.com/ugorji/go/codec v1.3.1 // indirect go.mongodb.org/mongo-driver/v2 v2.8.0 // indirect golang.org/x/arch v0.29.0 // indirect golang.org/x/crypto v0.54.0 // indirect golang.org/x/net v0.57.0 // indirect + golang.org/x/sync v0.22.0 // indirect golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.40.0 // indirect google.golang.org/protobuf v1.36.11 // indirect diff --git a/go.sum b/go.sum index 4628431..fd63f27 100644 --- a/go.sum +++ b/go.sum @@ -9,6 +9,8 @@ github.com/cloudwego/base64x v0.1.7/go.mod h1:Cu1PV9zfrSf7ET2tIbWbbEy7jO7HHJ13q4 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/eclipse/paho.mqtt.golang v1.5.0 h1:EH+bUVJNgttidWFkLLVKaQPGmkTUfQQqjOsyvMGvD6o= +github.com/eclipse/paho.mqtt.golang v1.5.0/go.mod h1:du/2qNQVqJf/Sqs4MEL77kR8QTqANF7XU7Fk0aOTAgk= github.com/gabriel-vasile/mimetype v1.4.13 h1:46nXokslUBsAJE/wMsp5gtO500a4F3Nkz9Ufpk2AcUM= github.com/gabriel-vasile/mimetype v1.4.13/go.mod h1:d+9Oxyo1wTzWdyVUPMmXFvp4F9tea18J8ufA774AB3s= github.com/gin-contrib/sse v1.1.1 h1:uGYpNwTacv5R68bSGMapo62iLTRa9l5zxGCps4hK6ko= @@ -30,6 +32,10 @@ github.com/goccy/go-yaml v1.19.2/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7Lk github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= +github.com/gorilla/websocket v1.5.3 h1:saDtZ6Pbx/0u+bgYQ3q96pZgCzfhKXGPqt7kZ72aNNg= +github.com/gorilla/websocket v1.5.3/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE= +github.com/jinzhu/copier v0.3.5 h1:GlvfUwHk62RokgqVNvYsku0TATCF7bAHVwEXoBh3iJg= +github.com/jinzhu/copier v0.3.5/go.mod h1:DfbEm0FYsaqBcKcFuvmOZb218JkPGtvSHsKg8S8hyyg= github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnrnM= github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= github.com/klauspost/cpuid/v2 v2.4.0 h1:S6Hrbc7+ywsr0r+RLapfGBHfyefhCTwEh3A0tV913Dw= @@ -44,6 +50,8 @@ github.com/mattn/go-isatty v0.0.23 h1:cYwCQTQf3HB6xUC+BtyCLZNr7IzbOmoZbmssVNzSyi github.com/mattn/go-isatty v0.0.23/go.mod h1:nMCL3Zebbrt45jsMDgnfIwz6ydEQApk5oEI3HqDio6A= github.com/mattn/go-sqlite3 v1.14.48 h1:7XHIgl0a8HwOaiK4E47ozLkST78rR9+OtNGx27D/TFs= github.com/mattn/go-sqlite3 v1.14.48/go.mod h1:6JTjA44L93a0QCyJef5YvlPoKXntQPjzWv5gtm9sB6w= +github.com/mochi-mqtt/server/v2 v2.6.6 h1:FmL5ebeIIA+AKo/nX0DF8Yc2MMWFLQCwh3FZBEmg6dQ= +github.com/mochi-mqtt/server/v2 v2.6.6/go.mod h1:TqztjKGO0/ArOjJt9x9idk0kqPT3CVN8Pb+l+PS5Gdo= github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= @@ -61,6 +69,8 @@ github.com/quic-go/quic-go v0.60.0 h1:xcQioE8OM66UQLeUMHltK1CCcOu3JbVB4JAQdDQSB+ github.com/quic-go/quic-go v0.60.0/go.mod h1:wpKpjmPpftl30sL6pFh7REVpjbcCVy4zt2vDyK1TuJk= github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ= github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog= +github.com/rs/xid v1.4.0 h1:qd7wPTDkN6KQx2VmMBLrpHkiyQwgFXRnkOLacUiaSNY= +github.com/rs/xid v1.4.0/go.mod h1:trrq9SKmegXys3aeAKXMUTdJsYXVwGY3RLcfgqegfbg= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= @@ -86,6 +96,8 @@ golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= golang.org/x/net v0.57.0 h1:K5+3DljvIuDG9/Jv9rvyMywYNFCQ9RSUY6OOTTkT+tE= golang.org/x/net v0.57.0/go.mod h1:KpXc8iv+r3XplLAG/f7Jsf9RPszJzdR0f58q9vGOuEU= +golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= +golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/text v0.40.0 h1:Ub2Z6/xjgF1WrYQz2nuITOEegKFtiIy+rieRJ5lHZKs= diff --git a/internal/api/server.go b/internal/api/server.go index b6ea15e..552a804 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -8,53 +8,17 @@ import ( "log" "net" "net/http" - "regexp" "strconv" "strings" "time" - "unicode" "github.com/gin-gonic/gin" "github.com/sethpjohnson/only-fan-controller/internal/config" "github.com/sethpjohnson/only-fan-controller/internal/controller" "github.com/sethpjohnson/only-fan-controller/internal/storage" + "github.com/sethpjohnson/only-fan-controller/internal/validate" ) -// maxHintFieldLen bounds free-form hint identifiers (source/type). Kept small: -// these are process names, not prose. -const maxHintFieldLen = 64 - -// hintFieldPattern is the allowed charset for hint source/type. Restricting to -// this set means no server-derived hint string can carry HTML/script even if a -// dashboard interpolation is ever missed. -var hintFieldPattern = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`) - -// allowedIntensities is the closed set of intensity values AddHint understands. -var allowedIntensities = map[string]bool{"": true, "low": true, "medium": true, "high": true} - -// allowedHintActions is the closed set of hint actions the controller acts on. -var allowedHintActions = map[string]bool{"start": true, "stop": true} - -// maxOverrideReasonLen bounds the free-text override reason. -const maxOverrideReasonLen = 128 - -// validateOverrideReason enforces a length cap and rejects control characters -// on the override reason. Unlike hint source/type, this is human-readable -// prose (shown to an operator, not interpolated as an identifier), so normal -// punctuation, spaces, and quotes are all valid free text -- only control -// characters and excessive length are rejected. -func validateOverrideReason(reason string) error { - if len(reason) > maxOverrideReasonLen { - return fmt.Errorf("reason exceeds %d characters", maxOverrideReasonLen) - } - for _, r := range reason { - if unicode.IsControl(r) { - return fmt.Errorf("reason must not contain control characters") - } - } - return nil -} - //go:embed static/* var staticFiles embed.FS @@ -199,20 +163,20 @@ func bearerTokenMatches(header, expected string) bool { } // validateHintRequest enforces length, charset, and closed-set bounds on the -// client-controlled hint fields before they are stored or echoed back. This is -// the server-side half of the XSS defense (the dashboard escapes on render). +// client-controlled hint fields before they are stored or echoed back, using the +// shared validate package so the HTTP and MQTT surfaces agree. func validateHintRequest(req *HintRequest) error { - if err := validateHintField("source", req.Source); err != nil { + if err := validate.HintField("source", req.Source); err != nil { return err } - if err := validateHintField("type", req.Type); err != nil { + if err := validate.HintField("type", req.Type); err != nil { return err } - if !allowedHintActions[req.Action] { - return fmt.Errorf("action must be one of start, stop") + if err := validate.HintAction(req.Action); err != nil { + return err } - if !allowedIntensities[req.Intensity] { - return fmt.Errorf("intensity must be one of low, medium, high") + if err := validate.Intensity(req.Intensity); err != nil { + return err } if req.DurationEstimate < 0 { return fmt.Errorf("duration_estimate must not be negative") @@ -220,16 +184,6 @@ func validateHintRequest(req *HintRequest) error { return nil } -func validateHintField(name, value string) error { - if len(value) > maxHintFieldLen { - return fmt.Errorf("%s exceeds %d characters", name, maxHintFieldLen) - } - if !hintFieldPattern.MatchString(value) { - return fmt.Errorf("%s must match [A-Za-z0-9_.-]", name) - } - return nil -} - // GET /api/status func (s *Server) handleStatus(c *gin.Context) { status := s.ctrl.GetStatus() @@ -306,12 +260,12 @@ func (s *Server) handleOverride(c *gin.Context) { return } - if req.Speed < 0 || req.Speed > 100 { - c.JSON(http.StatusBadRequest, gin.H{"error": "speed must be 0-100"}) + if err := validate.OverrideSpeed(req.Speed); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } - if err := validateOverrideReason(req.Reason); err != nil { + if err := validate.OverrideReason(req.Reason); err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } diff --git a/internal/api/server_test.go b/internal/api/server_test.go index 4b1b580..1df98e4 100644 --- a/internal/api/server_test.go +++ b/internal/api/server_test.go @@ -11,6 +11,7 @@ import ( "github.com/gin-gonic/gin" "github.com/sethpjohnson/only-fan-controller/internal/config" "github.com/sethpjohnson/only-fan-controller/internal/controller" + "github.com/sethpjohnson/only-fan-controller/internal/validate" ) func init() { gin.SetMode(gin.TestMode) } @@ -27,6 +28,30 @@ func newTestServer(t *testing.T, token string) *Server { return NewServer(cfg, ctrl, nil) } +// TestGetConfigDoesNotLeakMQTTPassword locks in that /api/config never exposes +// the MQTT broker password (MQTTConfig.Password carries json:"-", and +// handleGetConfig builds a hand-picked map that omits the mqtt section entirely). +func TestGetConfigDoesNotLeakMQTTPassword(t *testing.T) { + cfg := config.Default() + cfg.Dashboard.Enabled = false + cfg.MQTT.Enabled = true + cfg.MQTT.Broker = "tcp://10.0.0.5:1883" + cfg.MQTT.Password = "super-secret-broker-pw" + ctrl := controller.NewFanController(cfg, nil, nil, nil) + srv := NewServer(cfg, ctrl, nil) + + req := httptest.NewRequest(http.MethodGet, "/api/config", nil) + rec := httptest.NewRecorder() + srv.router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("GET /api/config = %d, want 200", rec.Code) + } + if strings.Contains(rec.Body.String(), "super-secret-broker-pw") { + t.Fatalf("/api/config leaked mqtt.password: %s", rec.Body.String()) + } +} + // doRequest issues a request against the server's router. A non-empty token is // sent as a bearer header; a non-empty remoteAddr overrides the connection peer // (httptest defaults to a non-loopback TEST-NET address). @@ -207,7 +232,7 @@ func TestOverrideReasonValidation(t *testing.T) { {"empty reason allowed", "", http.StatusOK}, {"newline rejected", "line1\nline2", http.StatusBadRequest}, {"tab rejected", "reason\twith\ttab", http.StatusBadRequest}, - {"overlong reason rejected", strings.Repeat("a", maxOverrideReasonLen+1), http.StatusBadRequest}, + {"overlong reason rejected", strings.Repeat("a", validate.MaxOverrideReasonLen+1), http.StatusBadRequest}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/config/config.go b/internal/config/config.go index 1b34ccc..ed7737d 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2,6 +2,7 @@ package config import ( "fmt" + "net/url" "os" "gopkg.in/yaml.v3" @@ -16,6 +17,21 @@ type Config struct { API APIConfig `yaml:"api"` Dashboard DashboardConfig `yaml:"dashboard"` Storage StorageConfig `yaml:"storage"` + MQTT MQTTConfig `yaml:"mqtt"` +} + +// MQTTConfig configures the optional Home Assistant MQTT bridge. It is off by +// default; when Enabled, Broker is required and validated. Password carries +// json:"-" so it is never exposed via /api/config (same treatment as the iDRAC +// password and API token). +type MQTTConfig struct { + Enabled bool `yaml:"enabled" json:"enabled"` + Broker string `yaml:"broker" json:"broker"` + Username string `yaml:"username" json:"username"` + Password string `yaml:"password" json:"-"` + ClientID string `yaml:"client_id" json:"client_id"` + BaseTopic string `yaml:"base_topic" json:"base_topic"` + DiscoveryPrefix string `yaml:"discovery_prefix" json:"discovery_prefix"` } type IDRACConfig struct { @@ -174,6 +190,30 @@ func (c *Config) Validate() error { if c.Storage.RetentionDays <= 0 { return fmt.Errorf("invalid storage.retention_days: %d (require > 0)", c.Storage.RetentionDays) } + // MQTT is optional. When enabled, the broker must be a parseable URL with a + // scheme and host, and the identity/topic roots must be non-empty (they + // default to non-empty values, so this only trips if an operator blanks + // them). When disabled, none of this is checked. + if c.MQTT.Enabled { + if c.MQTT.Broker == "" { + return fmt.Errorf("mqtt.enabled is true but mqtt.broker is empty") + } + u, err := url.Parse(c.MQTT.Broker) + if err != nil { + return fmt.Errorf("invalid mqtt.broker %q: %v", c.MQTT.Broker, err) + } + switch u.Scheme { + case "tcp", "ssl", "ws", "wss", "mqtt", "mqtts": + default: + return fmt.Errorf("invalid mqtt.broker %q: scheme must be one of tcp/ssl/ws/wss/mqtt/mqtts", c.MQTT.Broker) + } + if u.Host == "" { + return fmt.Errorf("invalid mqtt.broker %q: missing host", c.MQTT.Broker) + } + if c.MQTT.ClientID == "" || c.MQTT.BaseTopic == "" || c.MQTT.DiscoveryPrefix == "" { + return fmt.Errorf("mqtt.client_id, mqtt.base_topic and mqtt.discovery_prefix must be non-empty when mqtt is enabled") + } + } return nil } @@ -228,6 +268,12 @@ func Default() *Config { Path: "/var/lib/only-fan-controller/history.db", RetentionDays: 30, }, + MQTT: MQTTConfig{ + Enabled: false, + ClientID: "only-fan-controller", + BaseTopic: "only-fan-controller", + DiscoveryPrefix: "homeassistant", + }, } } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index c6ad87b..317a199 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,11 +1,77 @@ package config import ( + "encoding/json" "os" "path/filepath" + "strings" "testing" ) +func TestMQTTValidation(t *testing.T) { + tests := []struct { + name string + mutate func(c *Config) + wantErr bool + }{ + { + name: "disabled needs no broker", + mutate: func(c *Config) { c.MQTT.Enabled = false; c.MQTT.Broker = "" }, + wantErr: false, + }, + { + name: "enabled with valid tcp broker is ok", + mutate: func(c *Config) { c.MQTT.Enabled = true; c.MQTT.Broker = "tcp://192.168.1.5:1883" }, + wantErr: false, + }, + { + name: "enabled with empty broker is rejected", + mutate: func(c *Config) { c.MQTT.Enabled = true; c.MQTT.Broker = "" }, + wantErr: true, + }, + { + name: "enabled with unparseable broker is rejected", + mutate: func(c *Config) { c.MQTT.Enabled = true; c.MQTT.Broker = "://nope" }, + wantErr: true, + }, + { + name: "enabled with schemeless broker is rejected", + mutate: func(c *Config) { c.MQTT.Enabled = true; c.MQTT.Broker = "192.168.1.5:1883" }, + wantErr: true, + }, + { + name: "enabled with blank client_id is rejected", + mutate: func(c *Config) { c.MQTT.Enabled = true; c.MQTT.Broker = "tcp://h:1883"; c.MQTT.ClientID = "" }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := Default() + tt.mutate(c) + err := c.Validate() + if tt.wantErr && err == nil { + t.Fatal("expected validation error, got nil") + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected validation error: %v", err) + } + }) + } +} + +func TestMQTTPasswordNotMarshaled(t *testing.T) { + c := Default() + c.MQTT.Password = "s3cr3t-broker-pw" + b, err := json.Marshal(c.MQTT) + if err != nil { + t.Fatalf("marshal failed: %v", err) + } + if strings.Contains(string(b), "s3cr3t-broker-pw") { + t.Fatalf("mqtt password leaked into JSON: %s", b) + } +} + // TestExampleConfigIsValid loads config.example.yaml verbatim (as a fresh // install would) and confirms it passes Validate(), so the shipped example // never silently bit-rots into a config the service refuses to start with. diff --git a/internal/mqtt/bridge.go b/internal/mqtt/bridge.go new file mode 100644 index 0000000..1a95a2c --- /dev/null +++ b/internal/mqtt/bridge.go @@ -0,0 +1,215 @@ +package mqtt + +import ( + "log" + "sync" + "time" + + "github.com/sethpjohnson/only-fan-controller/internal/config" + "github.com/sethpjohnson/only-fan-controller/internal/controller" +) + +// Consumer is the exact controller surface the bridge needs. *controller. +// FanController satisfies it. All safety semantics (clamping, the 24h override +// cap, the critical-temp ramp, hint validation) live behind these methods, so +// MQTT commands inherit them for free — the bridge re-implements none of them. +type Consumer interface { + GetStatus() *controller.Status + SetOverride(speed int, duration time.Duration, reason string) + ClearOverride() + AddHint(hint *controller.WorkloadHint) + RemoveHint(source string) +} + +// disconnectQuiesceMs bounds the graceful disconnect wait so shutdown cannot be +// delayed by a slow broker. +const disconnectQuiesceMs = 250 + +// stopWaitTimeout bounds how long Stop waits for publishLoop to exit. +const stopWaitTimeout = 2 * time.Second + +// rateLimiter permits an action at most once per min interval. Used to keep +// repeated broker failures from flooding the log. +type rateLimiter struct { + mu sync.Mutex + last time.Time + min time.Duration +} + +func (r *rateLimiter) allow() bool { + r.mu.Lock() + defer r.mu.Unlock() + now := time.Now() + if r.min == 0 { + r.min = 30 * time.Second + } + if !r.last.IsZero() && now.Sub(r.last) < r.min { + return false + } + r.last = now + return true +} + +// Bridge publishes controller state to MQTT and routes MQTT commands into the +// controller. It owns its own goroutines and never shares a lock with the +// control loop beyond the Consumer.GetStatus read path. +type Bridge struct { + cfg *config.Config + consumer Consumer + newClient ClientFactory + client Client + stopCh chan struct{} + doneCh chan struct{} + pubLimiter rateLimiter + // broker is the normalized broker address (set in Start) used in log + // messages, including the unreachable-broker hint. + broker string + // unreachableHint gates the one-time "still unable to reach broker" hint so a + // wrong host/port surfaces once instead of on every failing publish tick. + unreachableHint onceHint + // gpuDiscovered tracks which GPU device indices have had their discovery + // configs published on the CURRENT connection, so each card is announced + // exactly once per connection rather than on every publish tick. It is guarded + // by gpuDiscMu because it is cleared from the OnConnect goroutine and + // checked/updated from the publish-loop goroutine. onConnect clears it so a + // reconnect re-announces every card (keeping HA's retained configs fresh). + gpuDiscMu sync.Mutex + gpuDiscovered map[int]bool + // retainWarned tracks command topics we have already logged a dropped + // retained message for, so reconnect replays warn once per topic instead of + // flooding the log. + retainWarnMu sync.Mutex + retainWarned map[string]bool +} + +// onceHint fires at most once until it is rearmed. It backs the one-time +// unreachable-broker hint: arm()-ing on a failing publish returns true only the +// first time, and rearm() (called after a successful publish) lets the hint fire +// again if the broker later drops out. +type onceHint struct { + mu sync.Mutex + shown bool +} + +func (h *onceHint) arm() bool { + h.mu.Lock() + defer h.mu.Unlock() + if h.shown { + return false + } + h.shown = true + return true +} + +func (h *onceHint) rearm() { + h.mu.Lock() + defer h.mu.Unlock() + h.shown = false +} + +// New builds a Bridge. factory is NewPahoClient in production and a fake in +// tests. Start actually connects. +func New(cfg *config.Config, consumer Consumer, factory ClientFactory) *Bridge { + return &Bridge{ + cfg: cfg, + consumer: consumer, + newClient: factory, + gpuDiscovered: map[int]bool{}, + retainWarned: map[string]bool{}, + } +} + +func (b *Bridge) availabilityTopic() string { return b.cfg.MQTT.BaseTopic + "/availability" } +func (b *Bridge) stateTopic() string { return b.cfg.MQTT.BaseTopic + "/state" } +func (b *Bridge) cmdOverrideTopic() string { return b.cfg.MQTT.BaseTopic + "/cmd/override" } +func (b *Bridge) cmdOverrideClearTopic() string { return b.cfg.MQTT.BaseTopic + "/cmd/override/clear" } +func (b *Bridge) cmdHintTopic() string { return b.cfg.MQTT.BaseTopic + "/cmd/hint" } + +// Start builds the client and initiates connection. Connect is non-blocking, so +// an unreachable broker does not delay startup. onConnect (fired on every +// (re)connection) republishes availability. +func (b *Bridge) Start() { + broker, note := normalizeBrokerURL(b.cfg.MQTT.Broker) + if note != "" { + log.Printf("MQTT: broker %q interpreted as %q (%s)", b.cfg.MQTT.Broker, broker, note) + } + if hostPortIsHAWebUI(broker) { + log.Printf("MQTT: WARNING broker %q uses port 8123, which is typically Home Assistant's web UI, not the MQTT broker — Mosquitto usually listens on 1883", broker) + } + b.broker = broker + opts := ClientOptions{ + Broker: broker, + ClientID: b.cfg.MQTT.ClientID, + Username: b.cfg.MQTT.Username, + Password: b.cfg.MQTT.Password, + AvailabilityTopic: b.availabilityTopic(), + OnlinePayload: "online", + OfflinePayload: "offline", + OnConnect: b.onConnect, + } + b.client = b.newClient(opts) + log.Printf("MQTT: bridge starting (broker %s, base topic %s)", broker, b.cfg.MQTT.BaseTopic) + if err := b.client.Connect(); err != nil { + log.Printf("MQTT: initial connect error (will keep retrying in background): %v", err) + } + b.stopCh = make(chan struct{}) + b.doneCh = make(chan struct{}) + go b.publishLoop() +} + +// onConnect runs on every successful (re)connection. It publishes discovery, +// subscribes to the command topics, then marks availability online — so HA has +// the entities registered and the command paths live before it sees them go +// online. +// +// Per-GPU discovery is deliberately NOT published here: at cold start MQTT +// connects before the control loop's first GPU read, so the device set is still +// empty. It is announced lazily from the publish loop as devices appear (see +// publishNewGPUDiscovery). Clearing gpuDiscovered on every (re)connect makes that +// loop re-announce every card on the new connection. +func (b *Bridge) onConnect() { + b.resetGPUDiscovery() + b.publishDiscovery() + b.subscribeCommands() + b.publishAvailability(true) +} + +// resetGPUDiscovery forgets which GPU indices have been announced, so the publish +// loop re-publishes per-GPU discovery on the next tick. Called on (re)connect. +func (b *Bridge) resetGPUDiscovery() { + b.gpuDiscMu.Lock() + b.gpuDiscovered = map[int]bool{} + b.gpuDiscMu.Unlock() +} + +// publishAvailability publishes the retained availability state. +func (b *Bridge) publishAvailability(online bool) { + payload := "offline" + if online { + payload = "online" + } + if err := b.client.Publish(b.availabilityTopic(), 1, true, []byte(payload)); err != nil { + log.Printf("MQTT: failed to publish availability: %v", err) + } +} + +// Stop halts the publish loop (bounded by stopWaitTimeout), publishes the +// offline availability state, and disconnects. It is safe to call once. All +// waits are bounded so it can never delay the safety-critical shutdown path. +func (b *Bridge) Stop() { + if b.client == nil { + return + } + if b.stopCh != nil { + close(b.stopCh) + select { + case <-b.doneCh: + case <-time.After(stopWaitTimeout): + log.Printf("MQTT: publish loop did not stop within %s; abandoning it", stopWaitTimeout) + } + b.stopCh = nil + } + b.publishAvailability(false) + b.client.Disconnect(disconnectQuiesceMs) + log.Printf("MQTT: bridge stopped") +} diff --git a/internal/mqtt/bridge_test.go b/internal/mqtt/bridge_test.go new file mode 100644 index 0000000..90069f4 --- /dev/null +++ b/internal/mqtt/bridge_test.go @@ -0,0 +1,284 @@ +package mqtt + +import ( + "bytes" + "errors" + "log" + "os" + "strings" + "sync" + "testing" + "time" + + "github.com/sethpjohnson/only-fan-controller/internal/config" + "github.com/sethpjohnson/only-fan-controller/internal/controller" +) + +// publishedMsg records a single Publish call for assertions. +type publishedMsg struct { + topic string + qos byte + retained bool + payload []byte +} + +// fakeClient is an in-memory Client used across the mqtt package tests. It +// records publishes/subscriptions and invokes OnConnect synchronously from +// Connect (mirroring paho's OnConnect firing on every successful connection). +type fakeClient struct { + mu sync.Mutex + opts ClientOptions + published []publishedMsg + subs map[string]MessageHandler + disconnectCalled bool + // publishErr, when non-nil, makes every Publish fail with it (simulating an + // unreachable/wedged broker where the paho token times out). + publishErr error +} + +func (f *fakeClient) Connect() error { + if f.opts.OnConnect != nil { + f.opts.OnConnect() + } + return nil +} + +func (f *fakeClient) Publish(topic string, qos byte, retained bool, payload []byte) error { + f.mu.Lock() + defer f.mu.Unlock() + f.published = append(f.published, publishedMsg{topic, qos, retained, append([]byte(nil), payload...)}) + return f.publishErr +} + +// setPublishErr toggles whether subsequent Publish calls fail. +func (f *fakeClient) setPublishErr(err error) { + f.mu.Lock() + defer f.mu.Unlock() + f.publishErr = err +} + +func (f *fakeClient) Subscribe(topic string, qos byte, h MessageHandler) error { + f.mu.Lock() + defer f.mu.Unlock() + f.subs[topic] = h + return nil +} + +func (f *fakeClient) Disconnect(quiesceMs uint) { + f.mu.Lock() + defer f.mu.Unlock() + f.disconnectCalled = true +} + +// lastPublishOn returns the most recent publish to topic. +func (f *fakeClient) lastPublishOn(topic string) (publishedMsg, bool) { + f.mu.Lock() + defer f.mu.Unlock() + for i := len(f.published) - 1; i >= 0; i-- { + if f.published[i].topic == topic { + return f.published[i], true + } + } + return publishedMsg{}, false +} + +// deliver invokes the handler registered for topic, simulating a live (non- +// retained) inbound broker message. +func (f *fakeClient) deliver(topic string, payload []byte) { + f.deliverMsg(topic, payload, false) +} + +// deliverRetained simulates the broker replaying a retained message on +// (re)subscribe, so tests can assert that command handlers reject it. +func (f *fakeClient) deliverRetained(topic string, payload []byte) { + f.deliverMsg(topic, payload, true) +} + +func (f *fakeClient) deliverMsg(topic string, payload []byte, retained bool) { + f.mu.Lock() + h := f.subs[topic] + f.mu.Unlock() + if h != nil { + h(topic, payload, retained) + } +} + +// clientHolder captures the fakeClient the Bridge creates via the factory. +type clientHolder struct{ client *fakeClient } + +func (h *clientHolder) factory(opts ClientOptions) Client { + h.client = &fakeClient{opts: opts, subs: map[string]MessageHandler{}} + return h.client +} + +// fakeConsumer records controller calls and returns a canned status. +type fakeConsumer struct { + mu sync.Mutex + status *controller.Status + overrideSpeed int + overrideDur time.Duration + overrideReason string + overrideSet bool + cleared bool + addedHints []*controller.WorkloadHint + removedSources []string +} + +func (c *fakeConsumer) GetStatus() *controller.Status { + c.mu.Lock() + defer c.mu.Unlock() + if c.status != nil { + return c.status + } + return &controller.Status{} +} + +// setStatus swaps the status returned by GetStatus, simulating the control loop +// populating GPU devices after the first read. +func (c *fakeConsumer) setStatus(s *controller.Status) { + c.mu.Lock() + defer c.mu.Unlock() + c.status = s +} + +func (c *fakeConsumer) SetOverride(speed int, duration time.Duration, reason string) { + c.mu.Lock() + defer c.mu.Unlock() + c.overrideSpeed, c.overrideDur, c.overrideReason, c.overrideSet = speed, duration, reason, true +} + +func (c *fakeConsumer) ClearOverride() { + c.mu.Lock() + defer c.mu.Unlock() + c.cleared = true +} + +func (c *fakeConsumer) AddHint(hint *controller.WorkloadHint) { + c.mu.Lock() + defer c.mu.Unlock() + c.addedHints = append(c.addedHints, hint) +} + +func (c *fakeConsumer) RemoveHint(source string) { + c.mu.Lock() + defer c.mu.Unlock() + c.removedSources = append(c.removedSources, source) +} + +// testConfig returns a valid enabled-MQTT config for tests. +func testConfig() *config.Config { + cfg := config.Default() + cfg.MQTT.Enabled = true + cfg.MQTT.Broker = "tcp://127.0.0.1:1883" + return cfg +} + +func TestStartConfiguresLWTAndPublishesOnline(t *testing.T) { + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{}, h.factory) + b.Start() + + if h.client.opts.AvailabilityTopic != "only-fan-controller/availability" { + t.Fatalf("LWT topic = %q", h.client.opts.AvailabilityTopic) + } + if h.client.opts.OfflinePayload != "offline" { + t.Fatalf("LWT payload = %q, want offline", h.client.opts.OfflinePayload) + } + msg, ok := h.client.lastPublishOn("only-fan-controller/availability") + if !ok { + t.Fatal("no availability publish on connect") + } + if string(msg.payload) != "online" || !msg.retained { + t.Fatalf("availability publish = %q retained=%v, want online/true", msg.payload, msg.retained) + } +} + +func TestStopPublishesOfflineAndDisconnects(t *testing.T) { + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{}, h.factory) + b.Start() + b.Stop() + + msg, ok := h.client.lastPublishOn("only-fan-controller/availability") + if !ok || string(msg.payload) != "offline" || !msg.retained { + t.Fatalf("expected retained offline on stop, got %q retained=%v ok=%v", msg.payload, msg.retained, ok) + } + if !h.client.disconnectCalled { + t.Fatal("Disconnect was not called on Stop") + } +} + +// TestStartNormalizesBroker verifies the bridge hands the paho client a +// normalized broker (bare host -> tcp://host:1883) rather than the raw config +// value. +func TestStartNormalizesBroker(t *testing.T) { + cfg := testConfig() + cfg.MQTT.Broker = "192.168.1.50" + h := &clientHolder{} + b := New(cfg, &fakeConsumer{}, h.factory) + b.Start() + + if h.client.opts.Broker != "tcp://192.168.1.50:1883" { + t.Fatalf("client broker = %q, want tcp://192.168.1.50:1883", h.client.opts.Broker) + } + if b.broker != "tcp://192.168.1.50:1883" { + t.Fatalf("bridge broker = %q, want tcp://192.168.1.50:1883", b.broker) + } +} + +// TestUnreachableBrokerHintOnce verifies the "unable to reach broker" hint is +// logged once while publishing keeps failing, then rearms after a success. +func TestUnreachableBrokerHintOnce(t *testing.T) { + var buf bytes.Buffer + log.SetOutput(&buf) + log.SetFlags(0) + t.Cleanup(func() { + log.SetOutput(os.Stderr) + log.SetFlags(log.LstdFlags) + }) + + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{}, h.factory) + b.Start() + h.client.setPublishErr(errors.New("publish to x timed out")) + + const hint = "still unable to reach broker" + for i := 0; i < 3; i++ { + b.publishState() + } + if got := strings.Count(buf.String(), hint); got != 1 { + t.Fatalf("hint logged %d times across 3 failing publishes, want 1", got) + } + + // A successful publish rearms the hint so a later outage surfaces again. + h.client.setPublishErr(nil) + b.publishState() + buf.Reset() + h.client.setPublishErr(errors.New("publish to x timed out")) + b.publishState() + if got := strings.Count(buf.String(), hint); got != 1 { + t.Fatalf("hint logged %d times after rearm, want 1", got) + } +} + +// TestHAWebUIPortWarning verifies a broker resolving to port 8123 triggers the +// startup warning about Home Assistant's web UI port. +func TestHAWebUIPortWarning(t *testing.T) { + var buf bytes.Buffer + log.SetOutput(&buf) + log.SetFlags(0) + t.Cleanup(func() { + log.SetOutput(os.Stderr) + log.SetFlags(log.LstdFlags) + }) + + cfg := testConfig() + cfg.MQTT.Broker = "tcp://homeassistant.local:8123" + h := &clientHolder{} + b := New(cfg, &fakeConsumer{}, h.factory) + b.Start() + + if !strings.Contains(buf.String(), "8123") || !strings.Contains(buf.String(), "web UI") { + t.Fatalf("expected HA web UI port warning, log was:\n%s", buf.String()) + } +} diff --git a/internal/mqtt/broker.go b/internal/mqtt/broker.go new file mode 100644 index 0000000..c1b9344 --- /dev/null +++ b/internal/mqtt/broker.go @@ -0,0 +1,153 @@ +package mqtt + +import ( + "fmt" + "strings" +) + +// defaultMQTTPort is the plaintext MQTT port used when the broker address omits +// one. defaultMQTTSPort is used instead when the scheme resolves to TLS (ssl). +const ( + defaultMQTTPort = "1883" + defaultMQTTSPort = "8883" +) + +// normalizeBrokerURL turns a loosely-typed broker address into the canonical +// "scheme://host:port" form paho expects, and returns a human-readable note +// describing any transformation it applied (empty when the input was already +// canonical). It is intentionally forgiving of the ways a user might paste a +// broker address — a bare host, a host:port pair, or even a browser URL copied +// out of the address bar (http://ha.local:8123). +// +// Recognized transformations: +// - surrounding whitespace is trimmed and a single trailing slash removed +// - the scheme is lowercased +// - a missing scheme defaults to tcp:// +// - http:// / https:// / mqtt:// resolve to tcp://; mqtts:// / tls:// resolve +// to ssl://; tcp/ssl/ws/wss are kept as-is +// - a missing port defaults to 1883 (or 8883 when the scheme resolves to ssl) +// - IPv6 literals are kept/placed in brackets ([::1]:1883) +// +// It never panics: if the address cannot be parsed it is returned unchanged with +// a note explaining that it was left as-is, so a malformed value degrades to the +// previous behavior rather than crashing startup. +func normalizeBrokerURL(raw string) (normalized string, note string) { + s := strings.TrimSpace(raw) + var notes []string + if s != raw { + notes = append(notes, "trimmed surrounding whitespace") + } + if s == "" { + return raw, "empty broker address; left unchanged" + } + if strings.HasSuffix(s, "/") { + s = strings.TrimSuffix(s, "/") + notes = append(notes, "removed trailing slash") + } + + // Split an optional "scheme://" prefix off the authority. + rawScheme, authority := splitScheme(s) + + // Resolve the scheme. An empty rawScheme means none was supplied. + scheme, schemeNote, ok := resolveScheme(rawScheme) + if !ok { + return raw, fmt.Sprintf("unrecognized scheme %q; left unchanged", rawScheme) + } + if schemeNote != "" { + notes = append(notes, schemeNote) + } + + host, port, bracketsAdded := splitAuthority(authority) + if host == "" { + return raw, fmt.Sprintf("could not parse broker address %q; left unchanged", raw) + } + if bracketsAdded { + notes = append(notes, "added brackets around IPv6 literal") + } + if port == "" { + port = defaultMQTTPort + if scheme == "ssl" { + port = defaultMQTTSPort + } + notes = append(notes, fmt.Sprintf("defaulted to port %s", port)) + } + + normalized = scheme + "://" + host + ":" + port + return normalized, strings.Join(notes, "; ") +} + +// hostPortIsHAWebUI reports whether a normalized broker address resolves to port +// 8123 — Home Assistant's web UI port, which users commonly paste by mistake in +// place of the MQTT broker's port (usually 1883). +func hostPortIsHAWebUI(normalized string) bool { + _, authority := splitScheme(normalized) + _, port, _ := splitAuthority(authority) + return port == "8123" +} + +// splitScheme separates an optional "scheme://" prefix from the authority. +// scheme is "" when the input carries no "://". +func splitScheme(s string) (scheme, authority string) { + if i := strings.Index(s, "://"); i >= 0 { + return s[:i], s[i+len("://"):] + } + return "", s +} + +// resolveScheme maps a raw (as-typed) scheme to the canonical paho scheme and a +// note describing the mapping. ok is false for a scheme we do not recognize, so +// the caller can leave the address untouched rather than guess. +func resolveScheme(rawScheme string) (scheme, note string, ok bool) { + if rawScheme == "" { + return "tcp", "assumed tcp:// scheme", true + } + lower := strings.ToLower(rawScheme) + switch lower { + case "tcp", "ssl", "ws", "wss": + if rawScheme != lower { + return lower, "lowercased scheme to " + lower + "://", true + } + return lower, "", true + case "http", "https": + return "tcp", fmt.Sprintf("treated %s:// as tcp:// (that looks like a web address, not an MQTT broker)", lower), true + case "mqtt": + return "tcp", "converted mqtt:// to tcp://", true + case "mqtts": + return "ssl", "converted mqtts:// to ssl://", true + case "tls": + return "ssl", "converted tls:// to ssl://", true + default: + return "", "", false + } +} + +// splitAuthority splits "host", "host:port", "[ipv6]", or "[ipv6]:port" into its +// host and port parts. A bare IPv6 literal (more than one colon, no brackets) is +// wrapped in brackets and bracketsAdded is set. host is "" when the input is +// unparseable. +func splitAuthority(authority string) (host, port string, bracketsAdded bool) { + if authority == "" { + return "", "", false + } + if strings.HasPrefix(authority, "[") { + end := strings.Index(authority, "]") + if end < 0 { + // Malformed bracketed literal; hand it back as-is for the caller to reject. + return "", "", false + } + host = authority[:end+1] + rest := authority[end+1:] + if strings.HasPrefix(rest, ":") { + port = rest[1:] + } + return host, port, false + } + // A bare IPv6 literal has multiple colons and no port we can distinguish. + if strings.Count(authority, ":") > 1 { + return "[" + authority + "]", "", true + } + if i := strings.LastIndex(authority, ":"); i >= 0 { + return authority[:i], authority[i+1:], false + } + return authority, "", false +} diff --git a/internal/mqtt/broker_test.go b/internal/mqtt/broker_test.go new file mode 100644 index 0000000..dcbf842 --- /dev/null +++ b/internal/mqtt/broker_test.go @@ -0,0 +1,68 @@ +package mqtt + +import "testing" + +func TestNormalizeBrokerURL(t *testing.T) { + tests := []struct { + name string + in string + want string + wantNote bool // true if a transformation note is expected + }{ + {"bare IPv4 defaults scheme and port", "192.168.1.10", "tcp://192.168.1.10:1883", true}, + {"host:port assumes tcp scheme", "192.168.1.10:1883", "tcp://192.168.1.10:1883", true}, + {"bare hostname defaults scheme and port", "broker.example.com", "tcp://broker.example.com:1883", true}, + {"http browser paste becomes tcp", "http://homeassistant.local:8123", "tcp://homeassistant.local:8123", true}, + {"https browser paste becomes tcp and defaults port", "https://broker.example.com", "tcp://broker.example.com:1883", true}, + {"canonical tcp is unchanged", "tcp://127.0.0.1:1883", "tcp://127.0.0.1:1883", false}, + {"canonical ssl is unchanged", "ssl://broker.example.com:8883", "ssl://broker.example.com:8883", false}, + {"canonical ws is unchanged", "ws://broker.example.com:9001", "ws://broker.example.com:9001", false}, + {"mqtt scheme becomes tcp", "mqtt://broker.example.com:1883", "tcp://broker.example.com:1883", true}, + {"mqtts scheme becomes ssl", "mqtts://broker.example.com:8883", "ssl://broker.example.com:8883", true}, + {"mqtts without port defaults to 8883", "mqtts://broker.example.com", "ssl://broker.example.com:8883", true}, + {"tls scheme becomes ssl", "tls://broker.example.com:8883", "ssl://broker.example.com:8883", true}, + {"ssl without port defaults to 8883", "ssl://broker.example.com", "ssl://broker.example.com:8883", true}, + {"uppercase scheme is lowercased", "TCP://broker.example.com:1883", "tcp://broker.example.com:1883", true}, + {"surrounding whitespace is trimmed", " tcp://broker.example.com:1883 ", "tcp://broker.example.com:1883", true}, + {"trailing slash is stripped", "tcp://broker.example.com:1883/", "tcp://broker.example.com:1883", true}, + {"IPv6 literal with port is preserved", "[::1]:1883", "tcp://[::1]:1883", true}, + {"IPv6 literal without port defaults port", "[::1]", "tcp://[::1]:1883", true}, + {"bare IPv6 literal gets brackets", "fe80::1", "tcp://[fe80::1]:1883", true}, + {"canonical IPv6 tcp is unchanged", "tcp://[::1]:1883", "tcp://[::1]:1883", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, note := normalizeBrokerURL(tt.in) + if got != tt.want { + t.Errorf("normalizeBrokerURL(%q) = %q, want %q", tt.in, got, tt.want) + } + if (note != "") != tt.wantNote { + t.Errorf("normalizeBrokerURL(%q) note = %q, wantNote=%v", tt.in, note, tt.wantNote) + } + }) + } +} + +// TestNormalizeBrokerURLDefensive covers the never-crash contract: unparseable +// or empty input is returned unchanged with an explanatory note. +func TestNormalizeBrokerURLDefensive(t *testing.T) { + tests := []struct { + name string + in string + }{ + {"empty string", ""}, + {"whitespace only", " "}, + {"unrecognized scheme", "carrier-pigeon://broker.example.com"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, note := normalizeBrokerURL(tt.in) + if got != tt.in { + t.Errorf("normalizeBrokerURL(%q) = %q, want input returned unchanged", tt.in, got) + } + if note == "" { + t.Errorf("normalizeBrokerURL(%q) expected a note explaining it was left unchanged", tt.in) + } + }) + } +} diff --git a/internal/mqtt/client.go b/internal/mqtt/client.go new file mode 100644 index 0000000..0de84d8 --- /dev/null +++ b/internal/mqtt/client.go @@ -0,0 +1,116 @@ +package mqtt + +import ( + "fmt" + "log" + "time" + + paho "github.com/eclipse/paho.mqtt.golang" +) + +// MessageHandler receives an inbound message's topic, raw payload, and whether +// the broker delivered it as a retained message. Command subscribers use the +// retained flag to reject replayed commands (see Bridge.subscribeCommands). +type MessageHandler func(topic string, payload []byte, retained bool) + +// Client is the minimal broker surface the Bridge needs. The real +// implementation wraps paho; tests use an in-memory fake. All methods must be +// bounded: no call may block the control loop or shutdown indefinitely. +type Client interface { + // Connect starts (or resumes) the connection. It must NOT block on an + // unreachable broker — the real implementation is fire-and-forget and lets + // paho's auto-reconnect handle background connection. + Connect() error + Publish(topic string, qos byte, retained bool, payload []byte) error + Subscribe(topic string, qos byte, handler MessageHandler) error + Disconnect(quiesceMs uint) +} + +// ClientOptions carries everything NewPahoClient needs, including the LWT +// (AvailabilityTopic + OfflinePayload) and an OnConnect callback invoked on +// every successful (re)connection. +type ClientOptions struct { + Broker string + ClientID string + Username string + Password string + AvailabilityTopic string + OnlinePayload string + OfflinePayload string + OnConnect func() +} + +// ClientFactory builds a Client from options. Production passes NewPahoClient; +// tests pass a fake factory. +type ClientFactory func(opts ClientOptions) Client + +// pahoClient adapts the paho client to the Client interface. +type pahoClient struct { + client paho.Client +} + +// NewPahoClient builds a paho-backed Client with auto-reconnect, connect-retry, +// and the LWT configured. OnConnect is wired so discovery/availability are +// republished on every (re)connection. +func NewPahoClient(opts ClientOptions) Client { + o := paho.NewClientOptions() + o.AddBroker(opts.Broker) + o.SetClientID(opts.ClientID) + if opts.Username != "" { + o.SetUsername(opts.Username) + } + if opts.Password != "" { + o.SetPassword(opts.Password) + } + o.SetWill(opts.AvailabilityTopic, opts.OfflinePayload, 1, true) + o.SetAutoReconnect(true) + o.SetConnectRetry(true) + o.SetConnectRetryInterval(5 * time.Second) + o.SetMaxReconnectInterval(30 * time.Second) + o.SetCleanSession(true) + o.SetOnConnectHandler(func(_ paho.Client) { + log.Printf("MQTT: connected to %s", opts.Broker) + if opts.OnConnect != nil { + opts.OnConnect() + } + }) + o.SetConnectionLostHandler(func(_ paho.Client, err error) { + log.Printf("MQTT: connection lost (auto-reconnecting): %v", err) + }) + return &pahoClient{client: paho.NewClient(o)} +} + +// Connect is fire-and-forget: it starts paho's connect loop and returns +// immediately so an unreachable broker never blocks startup. Success/failure is +// surfaced via the OnConnect / ConnectionLost handlers. +func (p *pahoClient) Connect() error { + p.client.Connect() + return nil +} + +// Publish is bounded so a wedged broker can never stall the publish ticker. +func (p *pahoClient) Publish(topic string, qos byte, retained bool, payload []byte) error { + token := p.client.Publish(topic, qos, retained, payload) + if !token.WaitTimeout(2 * time.Second) { + return fmt.Errorf("publish to %s timed out", topic) + } + return token.Error() +} + +// Subscribe runs from the OnConnect handler (paho's goroutine). The token wait +// is bounded (like Publish) so a wedged broker can never stall the connection +// callback. The retained flag is forwarded so command subscribers can drop +// replayed messages. +func (p *pahoClient) Subscribe(topic string, qos byte, handler MessageHandler) error { + token := p.client.Subscribe(topic, qos, func(_ paho.Client, m paho.Message) { + handler(m.Topic(), m.Payload(), m.Retained()) + }) + if !token.WaitTimeout(2 * time.Second) { + return fmt.Errorf("subscribe to %s timed out", topic) + } + return token.Error() +} + +func (p *pahoClient) Disconnect(quiesceMs uint) { + p.client.Disconnect(quiesceMs) +} diff --git a/internal/mqtt/command.go b/internal/mqtt/command.go new file mode 100644 index 0000000..c0580df --- /dev/null +++ b/internal/mqtt/command.go @@ -0,0 +1,147 @@ +package mqtt + +import ( + "encoding/json" + "log" + "time" + + "github.com/sethpjohnson/only-fan-controller/internal/controller" + "github.com/sethpjohnson/only-fan-controller/internal/validate" +) + +// overrideCommand is the JSON schema for /cmd/override. +type overrideCommand struct { + Speed int `json:"speed"` + DurationSeconds int `json:"duration_seconds"` + Reason string `json:"reason"` +} + +// hintCommand is the JSON schema for /cmd/hint, mirroring the HTTP +// POST /api/hint body. +type hintCommand struct { + Type string `json:"type"` + Action string `json:"action"` + Intensity string `json:"intensity"` + Source string `json:"source"` + DurationEstimate int `json:"duration_estimate"` +} + +// subscribeCommands registers QoS-1 handlers for the three command topics. It +// runs from onConnect, so it re-subscribes on every (re)connection. +// +// Every handler drops RETAINED deliveries: commands are live imperatives, not +// state. A command ever published with the retain flag (a `mosquitto_pub -r` +// test, a misconfigured automation) would otherwise be replayed by the broker on +// every reconnect and process restart — silently re-executing forever. Dropping +// them here (once-per-topic warning) keeps that from happening while still +// surfacing the misconfiguration to an operator. +func (b *Bridge) subscribeCommands() { + subs := []struct { + topic string + apply func(payload []byte) + }{ + {b.cmdOverrideTopic(), b.handleOverrideCommand}, + {b.cmdOverrideClearTopic(), b.handleClearCommand}, + {b.cmdHintTopic(), b.handleHintCommand}, + } + for _, s := range subs { + apply := s.apply + handler := func(topic string, payload []byte, retained bool) { + if retained { + b.warnRetainedCommand(topic) + return + } + apply(payload) + } + if err := b.client.Subscribe(s.topic, 1, handler); err != nil { + log.Printf("MQTT: failed to subscribe to %s: %v", s.topic, err) + } + } +} + +// warnRetainedCommand logs a dropped retained command at most once per topic. +func (b *Bridge) warnRetainedCommand(topic string) { + b.retainWarnMu.Lock() + first := !b.retainWarned[topic] + b.retainWarned[topic] = true + b.retainWarnMu.Unlock() + if first { + log.Printf("MQTT: dropping RETAINED message on command topic %s; commands must be published WITHOUT the retain flag (a retained command would replay on every reconnect)", topic) + } +} + +// handleOverrideCommand parses, validates, and applies an override. Validation +// reuses the shared validate package; final clamping/cap live in the controller. +func (b *Bridge) handleOverrideCommand(payload []byte) { + var cmd overrideCommand + if err := json.Unmarshal(payload, &cmd); err != nil { + log.Printf("MQTT: invalid override command: %v", err) + return + } + if err := validate.OverrideSpeed(cmd.Speed); err != nil { + log.Printf("MQTT: rejecting override command: %v", err) + return + } + if err := validate.OverrideReason(cmd.Reason); err != nil { + log.Printf("MQTT: rejecting override command: %v", err) + return + } + duration := time.Duration(cmd.DurationSeconds) * time.Second + b.consumer.SetOverride(cmd.Speed, duration, cmd.Reason) + log.Printf("MQTT: override set via command: %d%% (%s)", cmd.Speed, cmd.Reason) +} + +// handleClearCommand clears any active override. The payload is ignored (any +// message on the clear topic triggers it), mirroring an HA button press. +func (b *Bridge) handleClearCommand(_ []byte) { + b.consumer.ClearOverride() + log.Printf("MQTT: override cleared via command") +} + +// handleHintCommand parses, validates, and applies a workload hint. action +// "stop" removes the hint by source; anything else starts it. +func (b *Bridge) handleHintCommand(payload []byte) { + var cmd hintCommand + if err := json.Unmarshal(payload, &cmd); err != nil { + log.Printf("MQTT: invalid hint command: %v", err) + return + } + if err := validate.HintField("source", cmd.Source); err != nil { + log.Printf("MQTT: rejecting hint command: %v", err) + return + } + if err := validate.HintField("type", cmd.Type); err != nil { + log.Printf("MQTT: rejecting hint command: %v", err) + return + } + if err := validate.HintAction(cmd.Action); err != nil { + log.Printf("MQTT: rejecting hint command: %v", err) + return + } + if err := validate.Intensity(cmd.Intensity); err != nil { + log.Printf("MQTT: rejecting hint command: %v", err) + return + } + if cmd.DurationEstimate < 0 { + log.Printf("MQTT: rejecting hint command: duration_estimate must not be negative") + return + } + + if cmd.Action == "stop" { + b.consumer.RemoveHint(cmd.Source) + log.Printf("MQTT: hint removed via command: %s", cmd.Source) + return + } + + hint := &controller.WorkloadHint{ + Type: cmd.Type, + Action: cmd.Action, + Intensity: cmd.Intensity, + Source: cmd.Source, + } + if cmd.DurationEstimate > 0 { + hint.ExpiresAt = time.Now().Add(time.Duration(cmd.DurationEstimate) * time.Second) + } + b.consumer.AddHint(hint) + log.Printf("MQTT: hint registered via command: %s from %s", cmd.Action, cmd.Source) +} diff --git a/internal/mqtt/command_test.go b/internal/mqtt/command_test.go new file mode 100644 index 0000000..9f9b8ed --- /dev/null +++ b/internal/mqtt/command_test.go @@ -0,0 +1,188 @@ +package mqtt + +import ( + "bytes" + "log" + "strings" + "testing" + "time" +) + +func startBridge(t *testing.T, consumer *fakeConsumer) *fakeClient { + t.Helper() + h := &clientHolder{} + b := New(testConfig(), consumer, h.factory) + b.Start() + return h.client +} + +func TestOverrideCommandRoutesToConsumer(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/override", + []byte(`{"speed": 60, "duration_seconds": 1800, "reason": "burn-in"}`)) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if !consumer.overrideSet { + t.Fatal("SetOverride was not called") + } + if consumer.overrideSpeed != 60 { + t.Fatalf("speed = %d, want 60", consumer.overrideSpeed) + } + if consumer.overrideDur != 1800*time.Second { + t.Fatalf("duration = %v, want 30m", consumer.overrideDur) + } + if consumer.overrideReason != "burn-in" { + t.Fatalf("reason = %q, want burn-in", consumer.overrideReason) + } +} + +func TestOverrideCommandRejectsOutOfRangeSpeed(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/override", []byte(`{"speed": 150}`)) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if consumer.overrideSet { + t.Fatal("SetOverride should not be called for out-of-range speed") + } +} + +func TestOverrideCommandRejectsBadJSON(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/override", []byte(`not json`)) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if consumer.overrideSet { + t.Fatal("SetOverride should not be called for invalid JSON") + } +} + +func TestClearCommandRoutesToConsumer(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/override/clear", []byte("PRESS")) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if !consumer.cleared { + t.Fatal("ClearOverride was not called") + } +} + +func TestHintCommandStartAddsHint(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/hint", + []byte(`{"type": "transcode", "action": "start", "intensity": "high", "source": "plex", "duration_estimate": 120}`)) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if len(consumer.addedHints) != 1 { + t.Fatalf("added hints = %d, want 1", len(consumer.addedHints)) + } + if consumer.addedHints[0].Source != "plex" || consumer.addedHints[0].Intensity != "high" { + t.Fatalf("hint wrong: %+v", consumer.addedHints[0]) + } + if consumer.addedHints[0].ExpiresAt.IsZero() { + t.Fatal("expected an expiry from duration_estimate") + } +} + +func TestHintCommandStopRemovesHint(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/hint", + []byte(`{"type": "transcode", "action": "stop", "source": "plex"}`)) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if len(consumer.removedSources) != 1 || consumer.removedSources[0] != "plex" { + t.Fatalf("removed sources = %v, want [plex]", consumer.removedSources) + } +} + +func TestHintCommandRejectsBadSource(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.deliver("only-fan-controller/cmd/hint", + []byte(`{"type": "transcode", "action": "start", "source": "bad source!"}`)) + + consumer.mu.Lock() + defer consumer.mu.Unlock() + if len(consumer.addedHints) != 0 { + t.Fatal("hint with invalid source should be rejected") + } +} + +func TestRetainedCommandIsDroppedAndLoggedOnce(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + var buf bytes.Buffer + old := log.Writer() + log.SetOutput(&buf) + defer log.SetOutput(old) + + // A retained override command must NOT reach the controller (it would replay + // on every reconnect/restart otherwise). + client.deliverRetained("only-fan-controller/cmd/override", + []byte(`{"speed": 88, "duration_seconds": 600, "reason": "retained-replay"}`)) + // A second retained delivery on the same topic must not log again. + client.deliverRetained("only-fan-controller/cmd/override", + []byte(`{"speed": 88, "duration_seconds": 600, "reason": "retained-replay"}`)) + + consumer.mu.Lock() + set := consumer.overrideSet + consumer.mu.Unlock() + if set { + t.Fatal("retained override command must not be applied to the controller") + } + + logged := buf.String() + if !strings.Contains(logged, "dropping RETAINED message") || + !strings.Contains(logged, "only-fan-controller/cmd/override") { + t.Fatalf("expected a dropped-retained log line, got: %q", logged) + } + if got := strings.Count(logged, "dropping RETAINED message"); got != 1 { + t.Fatalf("expected the drop to be logged exactly once per topic, got %d", got) + } + + // A subsequent live (non-retained) command on the same topic must still work, + // proving the drop is specific to retained delivery. + client.deliver("only-fan-controller/cmd/override", + []byte(`{"speed": 61, "duration_seconds": 600, "reason": "live"}`)) + consumer.mu.Lock() + defer consumer.mu.Unlock() + if !consumer.overrideSet || consumer.overrideSpeed != 61 { + t.Fatalf("live command after a retained drop was not applied: set=%v speed=%d", consumer.overrideSet, consumer.overrideSpeed) + } +} + +func TestSubscribeCommandsRegistersAllTopics(t *testing.T) { + consumer := &fakeConsumer{} + client := startBridge(t, consumer) + + client.mu.Lock() + defer client.mu.Unlock() + for _, topic := range []string{ + "only-fan-controller/cmd/override", + "only-fan-controller/cmd/override/clear", + "only-fan-controller/cmd/hint", + } { + if _, ok := client.subs[topic]; !ok { + t.Fatalf("not subscribed to %q", topic) + } + } +} diff --git a/internal/mqtt/discovery.go b/internal/mqtt/discovery.go new file mode 100644 index 0000000..f53bb35 --- /dev/null +++ b/internal/mqtt/discovery.go @@ -0,0 +1,194 @@ +package mqtt + +import ( + "encoding/json" + "fmt" + "log" + + "github.com/sethpjohnson/only-fan-controller/internal/monitor" +) + +// discoveryEntity is a single retained HA MQTT Discovery config message. +type discoveryEntity struct { + topic string + payload []byte +} + +// deviceBlock is the shared HA device all entities belong to. Its identifier is +// the MQTT client_id, so every entity is grouped under one "Only Fan Controller" +// device in Home Assistant. +func (b *Bridge) deviceBlock() map[string]any { + return map[string]any{ + "identifiers": []string{b.cfg.MQTT.ClientID}, + "name": "Only Fan Controller", + "manufacturer": "only-fan-controller", + "model": "IPMI Fan Controller", + } +} + +// discoveryTopic builds ////config. +func (b *Bridge) discoveryTopic(component, objectID string) string { + return fmt.Sprintf("%s/%s/%s/%s/config", b.cfg.MQTT.DiscoveryPrefix, component, b.cfg.MQTT.ClientID, objectID) +} + +// baseEntity returns the availability/device fields common to every entity. +func (b *Bridge) baseEntity(objectID, name string) map[string]any { + return map[string]any{ + "name": name, + "unique_id": b.cfg.MQTT.ClientID + "_" + objectID, + "object_id": b.cfg.MQTT.ClientID + "_" + objectID, + "availability_topic": b.availabilityTopic(), + "payload_available": "online", + "payload_not_available": "offline", + "device": b.deviceBlock(), + } +} + +// sensorConfig builds a read-only sensor bound to a value_json key. +func (b *Bridge) sensorConfig(objectID, name, valueKey, deviceClass, unit, icon string) map[string]any { + e := b.baseEntity(objectID, name) + e["state_topic"] = b.stateTopic() + e["value_template"] = "{{ value_json." + valueKey + " }}" + if deviceClass != "" { + e["device_class"] = deviceClass + } + if unit != "" { + e["unit_of_measurement"] = unit + } + if icon != "" { + e["icon"] = icon + } + return e +} + +// gpuSensorConfig builds a per-card GPU sensor bound to a value_json expression. +// Unlike sensorConfig, the value_template wraps the expression in a `default('')` +// filter so it renders cleanly (rather than erroring) during the brief window +// after (re)connect where discovery is published but no state has landed yet, or +// if the indexed device is momentarily absent from the gpus array. +func (b *Bridge) gpuSensorConfig(objectID, name, valueExpr, deviceClass, unit string) map[string]any { + e := b.baseEntity(objectID, name) + e["state_topic"] = b.stateTopic() + e["value_template"] = "{{ " + valueExpr + " | default('') }}" + if deviceClass != "" { + e["device_class"] = deviceClass + } + if unit != "" { + e["unit_of_measurement"] = unit + } + return e +} + +// binarySensorConfig builds an ON/OFF binary sensor from a boolean value_json +// key, using device_class "problem" (HA shows red when true). +func (b *Bridge) binarySensorConfig(objectID, name, valueKey string) map[string]any { + e := b.baseEntity(objectID, name) + e["state_topic"] = b.stateTopic() + e["value_template"] = "{{ 'ON' if value_json." + valueKey + " else 'OFF' }}" + e["payload_on"] = "ON" + e["payload_off"] = "OFF" + e["device_class"] = "problem" + return e +} + +// numberConfig builds the override-fan-speed number. min/max are bound to the +// configured MinSpeed/MaxSpeed so the HA UI cannot request an out-of-range value +// (the controller still clamps regardless). The command wraps the raw value into +// the cmd/override JSON with a default 1h duration. +func (b *Bridge) numberConfig() map[string]any { + e := b.baseEntity("override_speed", "Override Fan Speed") + e["command_topic"] = b.cmdOverrideTopic() + e["command_template"] = `{"speed": {{ value }}, "duration_seconds": 3600, "reason": "home assistant"}` + e["state_topic"] = b.stateTopic() + e["value_template"] = "{{ value_json.override_speed | default(0) }}" + e["min"] = b.cfg.FanControl.MinSpeed + e["max"] = b.cfg.FanControl.MaxSpeed + e["step"] = 1 + e["unit_of_measurement"] = "%" + e["mode"] = "slider" + e["icon"] = "mdi:fan" + return e +} + +// buttonConfig builds the clear-override button. +func (b *Bridge) buttonConfig() map[string]any { + e := b.baseEntity("override_clear", "Clear Fan Override") + e["command_topic"] = b.cmdOverrideClearTopic() + e["payload_press"] = "PRESS" + e["icon"] = "mdi:fan-off" + return e +} + +// discoverySpec is one HA discovery config message before it is marshaled. +type discoverySpec struct { + component string + objectID string + config map[string]any +} + +// discoveryEntities returns every HA discovery config message. Marshal failures +// (not expected for these primitive maps) are logged and skipped. +func (b *Bridge) discoveryEntities() []discoveryEntity { + specs := []discoverySpec{ + {"sensor", "cpu_temp", b.sensorConfig("cpu_temp", "CPU Temperature", "cpu_temp", "temperature", "°C", "")}, + {"sensor", "gpu_temp", b.sensorConfig("gpu_temp", "GPU Temperature (Max)", "gpu_temp", "temperature", "°C", "")}, + {"sensor", "fan_speed", b.sensorConfig("fan_speed", "Fan Speed", "fan_speed", "", "%", "mdi:fan")}, + {"sensor", "target_speed", b.sensorConfig("target_speed", "Target Fan Speed", "target_speed", "", "%", "mdi:fan")}, + {"sensor", "zone", b.sensorConfig("zone", "Thermal Zone", "zone", "", "", "mdi:thermometer")}, + {"sensor", "failsafe_reason", b.sensorConfig("failsafe_reason", "Failsafe Reason", "failsafe_reason", "", "", "mdi:shield-alert")}, + {"binary_sensor", "failsafe_active", b.binarySensorConfig("failsafe_active", "Failsafe Active", "failsafe_active")}, + {"binary_sensor", "restore_pending", b.binarySensorConfig("restore_pending", "Restore Pending", "restore_pending")}, + {"binary_sensor", "last_write_failed", b.binarySensorConfig("last_write_failed", "Last Fan Write Failed", "last_write_failed")}, + {"number", "override_speed", b.numberConfig()}, + {"button", "override_clear", b.buttonConfig()}, + } + entities := make([]discoveryEntity, 0, len(specs)) + for _, s := range specs { + payload, err := json.Marshal(s.config) + if err != nil { + log.Printf("MQTT: failed to marshal discovery for %s/%s: %v", s.component, s.objectID, err) + continue + } + entities = append(entities, discoveryEntity{ + topic: b.discoveryTopic(s.component, s.objectID), + payload: payload, + }) + } + return entities +} + +// gpuDeviceSpecs builds the three discovery sensors (temperature, utilization, +// power) for a single GPU device. +// +// arrayIndex is the device's position in the state `gpus` array (what the +// value_templates index into, matching the order buildStatePayload appends +// devices); d.Index keys the stable object_ids/unique_ids (gpu0_temp, +// gpu1_temp, ...) so entities survive restarts and preserve HA history. All +// per-card sensors share the one HA device block and availability topic used by +// every other entity. +func (b *Bridge) gpuDeviceSpecs(arrayIndex int, d monitor.GPUDevice) []discoverySpec { + tempID := fmt.Sprintf("gpu%d_temp", d.Index) + utilID := fmt.Sprintf("gpu%d_utilization", d.Index) + powerID := fmt.Sprintf("gpu%d_power", d.Index) + label := fmt.Sprintf("GPU %d (%s)", d.Index, d.Name) + return []discoverySpec{ + {"sensor", tempID, b.gpuSensorConfig( + tempID, label+" Temperature", + fmt.Sprintf("value_json.gpus[%d].temp", arrayIndex), "temperature", "°C")}, + {"sensor", utilID, b.gpuSensorConfig( + utilID, label+" Utilization", + fmt.Sprintf("value_json.gpus[%d].utilization", arrayIndex), "", "%")}, + {"sensor", powerID, b.gpuSensorConfig( + powerID, label+" Power", + fmt.Sprintf("value_json.gpus[%d].power", arrayIndex), "power", "W")}, + } +} + +// publishDiscovery publishes all discovery config messages, retained. +func (b *Bridge) publishDiscovery() { + for _, e := range b.discoveryEntities() { + if err := b.client.Publish(e.topic, 1, true, e.payload); err != nil { + log.Printf("MQTT: failed to publish discovery %s: %v", e.topic, err) + } + } +} diff --git a/internal/mqtt/discovery_test.go b/internal/mqtt/discovery_test.go new file mode 100644 index 0000000..5990a88 --- /dev/null +++ b/internal/mqtt/discovery_test.go @@ -0,0 +1,101 @@ +package mqtt + +import ( + "encoding/json" + "testing" +) + +func TestDiscoveryEntitiesCoverAllEntities(t *testing.T) { + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{}, h.factory) + + entities := b.discoveryEntities() + + wantTopics := []string{ + "homeassistant/sensor/only-fan-controller/cpu_temp/config", + "homeassistant/sensor/only-fan-controller/gpu_temp/config", + "homeassistant/sensor/only-fan-controller/fan_speed/config", + "homeassistant/sensor/only-fan-controller/target_speed/config", + "homeassistant/sensor/only-fan-controller/zone/config", + "homeassistant/sensor/only-fan-controller/failsafe_reason/config", + "homeassistant/binary_sensor/only-fan-controller/failsafe_active/config", + "homeassistant/binary_sensor/only-fan-controller/restore_pending/config", + "homeassistant/binary_sensor/only-fan-controller/last_write_failed/config", + "homeassistant/number/only-fan-controller/override_speed/config", + "homeassistant/button/only-fan-controller/override_clear/config", + } + got := map[string]bool{} + for _, e := range entities { + got[e.topic] = true + } + if len(entities) != len(wantTopics) { + t.Fatalf("got %d entities, want %d", len(entities), len(wantTopics)) + } + for _, w := range wantTopics { + if !got[w] { + t.Fatalf("missing discovery topic %q", w) + } + } +} + +func TestNumberEntityBoundsToConfiguredSpeeds(t *testing.T) { + cfg := testConfig() + cfg.FanControl.MinSpeed = 15 + cfg.FanControl.MaxSpeed = 90 + h := &clientHolder{} + b := New(cfg, &fakeConsumer{}, h.factory) + + var payload map[string]any + for _, e := range b.discoveryEntities() { + if e.topic == "homeassistant/number/only-fan-controller/override_speed/config" { + if err := json.Unmarshal(e.payload, &payload); err != nil { + t.Fatalf("unmarshal number config: %v", err) + } + } + } + if payload == nil { + t.Fatal("number entity not found") + } + if payload["min"].(float64) != 15 || payload["max"].(float64) != 90 { + t.Fatalf("number min/max = %v/%v, want 15/90", payload["min"], payload["max"]) + } + if payload["command_topic"].(string) != "only-fan-controller/cmd/override" { + t.Fatalf("number command_topic = %v", payload["command_topic"]) + } + dev := payload["device"].(map[string]any) + ids := dev["identifiers"].([]any) + if ids[0].(string) != "only-fan-controller" { + t.Fatalf("device identifier = %v, want only-fan-controller", ids[0]) + } +} + +func TestBinarySensorUsesProblemClassAndOnOff(t *testing.T) { + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{}, h.factory) + var payload map[string]any + for _, e := range b.discoveryEntities() { + if e.topic == "homeassistant/binary_sensor/only-fan-controller/failsafe_active/config" { + _ = json.Unmarshal(e.payload, &payload) + } + } + if payload["device_class"].(string) != "problem" { + t.Fatalf("device_class = %v, want problem", payload["device_class"]) + } + if payload["payload_on"].(string) != "ON" || payload["payload_off"].(string) != "OFF" { + t.Fatalf("payload_on/off = %v/%v", payload["payload_on"], payload["payload_off"]) + } +} + +func TestPublishDiscoveryOnConnect(t *testing.T) { + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{}, h.factory) + b.Start() + + msg, ok := h.client.lastPublishOn("homeassistant/sensor/only-fan-controller/cpu_temp/config") + if !ok { + t.Fatal("discovery not published on connect") + } + if !msg.retained || msg.qos != 1 { + t.Fatalf("discovery publish retained=%v qos=%d, want true/1", msg.retained, msg.qos) + } +} diff --git a/internal/mqtt/gpu_test.go b/internal/mqtt/gpu_test.go new file mode 100644 index 0000000..6e6300f --- /dev/null +++ b/internal/mqtt/gpu_test.go @@ -0,0 +1,258 @@ +package mqtt + +import ( + "encoding/json" + "fmt" + "regexp" + "testing" + + "github.com/sethpjohnson/only-fan-controller/internal/controller" + "github.com/sethpjohnson/only-fan-controller/internal/monitor" +) + +// gpuStatus builds a controller status with n GPU devices carrying distinct, +// recognizable per-card values. +func gpuStatus(n int) *controller.Status { + devices := make([]monitor.GPUDevice, n) + maxTemp := 0 + for i := 0; i < n; i++ { + temp := 40 + i + if temp > maxTemp { + maxTemp = temp + } + devices[i] = monitor.GPUDevice{ + Index: i, + Name: "Tesla P40", + Temp: temp, + Utilization: i * 5, + MemoryUsed: 1024, + PowerDraw: 50 + i, + } + } + return &controller.Status{GPU: &monitor.GPUReading{Devices: devices, Max: maxTemp}} +} + +// gpuDiscTopicRe matches a per-GPU discovery config topic (gpu_), +// so it does NOT match the aggregate gpu_temp sensor. +var gpuDiscTopicRe = regexp.MustCompile(`/gpu\d+_(temp|utilization|power)/config$`) + +// countGPUDiscovery counts per-GPU discovery configs published so far (the fake +// client keeps full history, so this is cumulative across ticks/reconnects). +func countGPUDiscovery(f *fakeClient) int { + f.mu.Lock() + defer f.mu.Unlock() + n := 0 + for _, m := range f.published { + if gpuDiscTopicRe.MatchString(m.topic) { + n++ + } + } + return n +} + +// TestGPUDiscoveryColdStart is the regression test for the bug found on the live +// R730: MQTT connects before the control loop's first GPU read, so no per-GPU +// discovery was ever published. It asserts the lazy publish path fixes that. +func TestGPUDiscoveryColdStart(t *testing.T) { + // Start with NO GPU devices known (connect-before-first-read). + consumer := &fakeConsumer{status: &controller.Status{}} + h := &clientHolder{} + b := New(testConfig(), consumer, h.factory) + b.Start() + + // (1) At connect, zero per-GPU discovery is published (device set empty). + if n := countGPUDiscovery(h.client); n != 0 { + t.Fatalf("per-GPU discovery at connect = %d, want 0", n) + } + // A tick while the device set is still empty must not publish anything either. + b.publishState() + if n := countGPUDiscovery(h.client); n != 0 { + t.Fatalf("per-GPU discovery after empty tick = %d, want 0", n) + } + + // (2) The first GPU read completes: two devices appear. The next tick must + // publish all three sensors for each of the two cards (6 total). + consumer.setStatus(gpuStatus(2)) + b.publishState() + if n := countGPUDiscovery(h.client); n != 6 { + t.Fatalf("per-GPU discovery after devices appear = %d, want 6", n) + } + for i := 0; i < 2; i++ { + for _, suffix := range []string{"temp", "utilization", "power"} { + topic := fmt.Sprintf("homeassistant/sensor/only-fan-controller/gpu%d_%s/config", i, suffix) + if _, ok := h.client.lastPublishOn(topic); !ok { + t.Errorf("missing per-GPU discovery topic %q", topic) + } + } + } + + // (3) Each card is announced exactly once per connection: further ticks do + // not republish. + b.publishState() + b.publishState() + if n := countGPUDiscovery(h.client); n != 6 { + t.Fatalf("per-GPU discovery republished on later ticks = %d, want 6 (once per index)", n) + } + + // (4) On reconnect the published set is cleared, so the next tick re-announces + // every card (keeping HA's retained configs fresh after a broker restart). + b.onConnect() // simulate paho's OnConnect firing again on reconnect + b.publishState() + if n := countGPUDiscovery(h.client); n != 12 { + t.Fatalf("per-GPU discovery after reconnect = %d, want 12 (re-published)", n) + } +} + +// TestGPUDiscoveryScalesWithDeviceCount verifies the per-card sensor set scales +// with the detected device count (1, 2, and 3 cards) and that onConnect alone +// never publishes per-GPU discovery. +func TestGPUDiscoveryScalesWithDeviceCount(t *testing.T) { + for _, n := range []int{1, 2, 3} { + t.Run(fmt.Sprintf("%d_devices", n), func(t *testing.T) { + consumer := &fakeConsumer{status: gpuStatus(n)} + h := &clientHolder{} + b := New(testConfig(), consumer, h.factory) + b.Start() + + // Even with devices already available, onConnect must not publish them. + if got := countGPUDiscovery(h.client); got != 0 { + t.Fatalf("onConnect published %d per-GPU configs, want 0 (publish loop owns this)", got) + } + b.publishState() + if got := countGPUDiscovery(h.client); got != n*3 { + t.Fatalf("per-GPU discovery after tick = %d, want %d", got, n*3) + } + }) + } +} + +// TestGPUDeviceSpecsClassesAndUnits verifies each per-card sensor carries the +// right device_class/unit and shares the common device block + availability. +func TestGPUDeviceSpecsClassesAndUnits(t *testing.T) { + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{}, h.factory) + + specs := b.gpuDeviceSpecs(0, monitor.GPUDevice{Index: 0, Name: "Tesla P40"}) + want := map[string]struct { + deviceClass string + unit string + }{ + "gpu0_temp": {"temperature", "°C"}, + "gpu0_utilization": {"", "%"}, + "gpu0_power": {"power", "W"}, + } + seen := map[string]bool{} + for _, s := range specs { + exp, ok := want[s.objectID] + if !ok { + t.Fatalf("unexpected per-GPU objectID %q", s.objectID) + } + seen[s.objectID] = true + if dc, _ := s.config["device_class"].(string); dc != exp.deviceClass { + t.Errorf("%s device_class = %q, want %q", s.objectID, dc, exp.deviceClass) + } + if u, _ := s.config["unit_of_measurement"].(string); u != exp.unit { + t.Errorf("%s unit = %q, want %q", s.objectID, u, exp.unit) + } + if at, _ := s.config["availability_topic"].(string); at != "only-fan-controller/availability" { + t.Errorf("%s availability_topic = %q", s.objectID, at) + } + dev, ok := s.config["device"].(map[string]any) + if !ok { + t.Fatalf("%s missing device block", s.objectID) + } + if ids := dev["identifiers"].([]string); ids[0] != "only-fan-controller" { + t.Errorf("%s device identifier = %v", s.objectID, ids[0]) + } + } + for id := range want { + if !seen[id] { + t.Errorf("per-GPU sensor %q not emitted", id) + } + } +} + +// TestBuildStatePayloadGPUs verifies the gpus array marshals with the expected +// keys and per-card values, and that the aggregate gpu_temp (max) is preserved. +func TestBuildStatePayloadGPUs(t *testing.T) { + p := buildStatePayload(gpuStatus(2)) + if p.GPUTemp == nil || *p.GPUTemp != 41 { + t.Fatalf("gpu_temp (max) = %v, want 41", p.GPUTemp) + } + b, err := json.Marshal(p) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var m struct { + GPUs []map[string]any `json:"gpus"` + } + if err := json.Unmarshal(b, &m); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(m.GPUs) != 2 { + t.Fatalf("gpus length = %d, want 2", len(m.GPUs)) + } + for i, g := range m.GPUs { + for _, k := range []string{"index", "name", "temp", "utilization", "power"} { + if _, ok := g[k]; !ok { + t.Errorf("gpus[%d] missing key %q", i, k) + } + } + if int(g["index"].(float64)) != i { + t.Errorf("gpus[%d].index = %v", i, g["index"]) + } + if g["name"].(string) != "Tesla P40" { + t.Errorf("gpus[%d].name = %v", i, g["name"]) + } + } +} + +// valueJSONKeyRe pulls "" and "" out of a value_template like +// "{{ value_json.gpus[0].temp | default('') }}". +var valueJSONKeyRe = regexp.MustCompile(`value_json\.gpus\[(\d+)\]\.(\w+)`) + +// TestGPUDiscoveryTemplatesMatchStateKeys guards against drift between the +// per-GPU discovery value_templates and the state JSON: every key a template +// references must actually exist in the marshaled gpus array at that index. +func TestGPUDiscoveryTemplatesMatchStateKeys(t *testing.T) { + status := gpuStatus(2) + h := &clientHolder{} + b := New(testConfig(), &fakeConsumer{status: status}, h.factory) + + // Marshal the state payload and index the gpus array as generic maps. + stateJSON, err := json.Marshal(buildStatePayload(status)) + if err != nil { + t.Fatalf("marshal state: %v", err) + } + var state struct { + GPUs []map[string]any `json:"gpus"` + } + if err := json.Unmarshal(stateJSON, &state); err != nil { + t.Fatalf("unmarshal state: %v", err) + } + + checked := 0 + for i, d := range status.GPU.Devices { + for _, s := range b.gpuDeviceSpecs(i, d) { + tmpl, _ := s.config["value_template"].(string) + match := valueJSONKeyRe.FindStringSubmatch(tmpl) + if match == nil { + t.Fatalf("per-GPU sensor %q has no value_json.gpus[..] template: %q", s.objectID, tmpl) + } + var arrIdx int + fmt.Sscanf(match[1], "%d", &arrIdx) + key := match[2] + if arrIdx < 0 || arrIdx >= len(state.GPUs) { + t.Errorf("template %q indexes gpus[%d] but state has %d entries", tmpl, arrIdx, len(state.GPUs)) + continue + } + if _, ok := state.GPUs[arrIdx][key]; !ok { + t.Errorf("template %q references gpus[%d].%s which is absent from state JSON", tmpl, arrIdx, key) + } + checked++ + } + } + if checked != 6 { // 2 devices * 3 sensors + t.Fatalf("checked %d per-GPU templates, want 6", checked) + } +} diff --git a/internal/mqtt/integration_test.go b/internal/mqtt/integration_test.go new file mode 100644 index 0000000..bec990f --- /dev/null +++ b/internal/mqtt/integration_test.go @@ -0,0 +1,276 @@ +package mqtt + +import ( + "errors" + "fmt" + "net" + "sync" + "testing" + "time" + + paho "github.com/eclipse/paho.mqtt.golang" + mochi "github.com/mochi-mqtt/server/v2" + "github.com/mochi-mqtt/server/v2/hooks/auth" + "github.com/mochi-mqtt/server/v2/listeners" + + "github.com/sethpjohnson/only-fan-controller/internal/config" + "github.com/sethpjohnson/only-fan-controller/internal/controller" + "github.com/sethpjohnson/only-fan-controller/internal/monitor" +) + +// freeAddr returns a currently-free 127.0.0.1 TCP address for the broker. +func freeAddr(t *testing.T) string { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("failed to grab a free port: %v", err) + } + addr := ln.Addr().String() + _ = ln.Close() + return addr +} + +// startBroker starts an in-process mochi broker on addr and returns it. +func startBroker(t *testing.T, addr string) *mochi.Server { + t.Helper() + server := mochi.New(&mochi.Options{InlineClient: false}) + if err := server.AddHook(new(auth.AllowHook), nil); err != nil { + t.Fatalf("add auth hook: %v", err) + } + tcp := listeners.NewTCP(listeners.Config{ID: "t1", Address: addr}) + if err := server.AddListener(tcp); err != nil { + t.Fatalf("add listener: %v", err) + } + go func() { _ = server.Serve() }() + t.Cleanup(func() { _ = server.Close() }) + return server +} + +// observer subscribes to "#" and records the FULL per-topic history, so the test +// can assert on discovery/state/availability traffic. History (not just the +// latest value) matters for the LWT check: paho auto-reconnects and republishes +// "online" moments after the forced disconnect, so we must assert that "offline" +// appeared at some point, not that it is the latest value. +type observer struct { + mu sync.Mutex + history map[string][]string + client paho.Client +} + +func newObserver(t *testing.T, broker string) *observer { + t.Helper() + o := &observer{history: map[string][]string{}} + opts := paho.NewClientOptions().AddBroker(broker).SetClientID("observer") + o.client = paho.NewClient(opts) + tok := o.client.Connect() + if !tok.WaitTimeout(3*time.Second) || tok.Error() != nil { + t.Fatalf("observer connect failed: %v", tok.Error()) + } + sub := o.client.Subscribe("#", 1, func(_ paho.Client, m paho.Message) { + o.mu.Lock() + o.history[m.Topic()] = append(o.history[m.Topic()], string(m.Payload())) + o.mu.Unlock() + }) + sub.Wait() + t.Cleanup(func() { o.client.Disconnect(100) }) + return o +} + +// latest returns the most recent payload seen on topic. +func (o *observer) latest(topic string) (string, bool) { + o.mu.Lock() + defer o.mu.Unlock() + h := o.history[topic] + if len(h) == 0 { + return "", false + } + return h[len(h)-1], true +} + +// seen reports whether payload was ever observed on topic. +func (o *observer) seen(topic, payload string) bool { + o.mu.Lock() + defer o.mu.Unlock() + for _, p := range o.history[topic] { + if p == payload { + return true + } + } + return false +} + +// waitFor polls cond up to 3s; fails the test with msg otherwise. +func waitFor(t *testing.T, msg string, cond func() bool) { + t.Helper() + deadline := time.Now().Add(3 * time.Second) + for time.Now().Before(deadline) { + if cond() { + return + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("timed out waiting for %s", msg) +} + +func TestIntegrationRetainedCommandIsDropped(t *testing.T) { + addr := freeAddr(t) + startBroker(t, addr) + broker := "tcp://" + addr + + obs := newObserver(t, broker) + + // Pre-publish a RETAINED override command. The broker will replay it to the + // bridge the instant it subscribes on connect. + pub := paho.NewClient(paho.NewClientOptions().AddBroker(broker).SetClientID("retain-pub")) + if tok := pub.Connect(); !tok.WaitTimeout(3*time.Second) || tok.Error() != nil { + t.Fatalf("retain publisher connect failed: %v", tok.Error()) + } + rt := pub.Publish("only-fan-controller/cmd/override", 1, true, + []byte(`{"speed": 88, "duration_seconds": 600, "reason": "retained-replay"}`)) + rt.Wait() + pub.Disconnect(100) + + cfg := config.Default() + cfg.MQTT.Enabled = true + cfg.MQTT.Broker = broker + consumer := &fakeConsumer{status: &controller.Status{}} + bridge := New(cfg, consumer, NewPahoClient) + bridge.Start() + t.Cleanup(func() { bridge.Stop() }) + + // onConnect subscribes to the command topics BEFORE it publishes + // availability=online, so once we observe "online" the broker has already + // delivered the retained command to the bridge's subscription. + waitFor(t, "bridge online", func() bool { + return obs.seen("only-fan-controller/availability", "online") + }) + // Give the retained delivery a moment to be processed (and dropped). + time.Sleep(300 * time.Millisecond) + + consumer.mu.Lock() + replayed := consumer.overrideSet + consumer.mu.Unlock() + if replayed { + t.Fatal("retained command was replayed into the controller on connect") + } + + // A normal (non-retained) command must still be applied, proving the drop is + // specific to retained delivery and not a dead command path. + live := obs.client.Publish("only-fan-controller/cmd/override", 1, false, + []byte(`{"speed": 71, "duration_seconds": 600, "reason": "live"}`)) + live.Wait() + waitFor(t, "live command applied", func() bool { + consumer.mu.Lock() + defer consumer.mu.Unlock() + return consumer.overrideSet && consumer.overrideSpeed == 71 + }) +} + +// TestIntegrationGPUDiscoveryAppearsAfterFirstRead reproduces the live-hardware +// cold-start bug against a real broker: MQTT connects before the first GPU read +// (no devices at connect), so per-GPU discovery must NOT be published at connect, +// but MUST appear on a later tick once the devices are known. +func TestIntegrationGPUDiscoveryAppearsAfterFirstRead(t *testing.T) { + addr := freeAddr(t) + startBroker(t, addr) + broker := "tcp://" + addr + + obs := newObserver(t, broker) + + cfg := config.Default() + cfg.MQTT.Enabled = true + cfg.MQTT.Broker = broker + cfg.Monitoring.Interval = 1 + // Connect-before-first-read: GPU is nil at startup. + consumer := &fakeConsumer{status: &controller.Status{CPU: &monitor.CPUReading{Max: 50}}} + + bridge := New(cfg, consumer, NewPahoClient) + bridge.Start() + t.Cleanup(func() { bridge.Stop() }) + + // Wait until the bridge is connected (availability online). + waitFor(t, "availability online", func() bool { + return obs.seen("only-fan-controller/availability", "online") + }) + // The aggregate discovery is present, but NO per-GPU discovery yet. + waitFor(t, "aggregate gpu_temp discovery", func() bool { + _, ok := obs.latest("homeassistant/sensor/only-fan-controller/gpu_temp/config") + return ok + }) + if _, ok := obs.latest("homeassistant/sensor/only-fan-controller/gpu0_temp/config"); ok { + t.Fatal("per-GPU discovery published at connect, before the first GPU read") + } + + // The first GPU read completes: two cards appear. + consumer.setStatus(gpuStatus(2)) + + // Within a tick or two, all three sensors for both cards must be announced. + for i := 0; i < 2; i++ { + for _, suffix := range []string{"temp", "utilization", "power"} { + topic := fmt.Sprintf("homeassistant/sensor/only-fan-controller/gpu%d_%s/config", i, suffix) + waitFor(t, "per-GPU discovery "+topic, func() bool { + _, ok := obs.latest(topic) + return ok + }) + } + } +} + +func TestIntegrationConnectDiscoveryCommandStateLWT(t *testing.T) { + addr := freeAddr(t) + server := startBroker(t, addr) + broker := "tcp://" + addr + + obs := newObserver(t, broker) + + cfg := config.Default() + cfg.MQTT.Enabled = true + cfg.MQTT.Broker = broker + cfg.Monitoring.Interval = 1 + consumer := &fakeConsumer{status: &controller.Status{ + CPU: &monitor.CPUReading{Max: 55}, + CurrentSpeed: 42, + Zone: "warm", + }} + + bridge := New(cfg, consumer, NewPahoClient) + bridge.Start() + t.Cleanup(func() { bridge.Stop() }) + + // 1. Availability online + discovery published on connect. + waitFor(t, "availability online", func() bool { + return obs.seen("only-fan-controller/availability", "online") + }) + waitFor(t, "cpu_temp discovery", func() bool { + _, ok := obs.latest("homeassistant/sensor/only-fan-controller/cpu_temp/config") + return ok + }) + + // 2. Command round-trip: publish an override command, expect SetOverride. + pub := obs.client.Publish("only-fan-controller/cmd/override", 1, false, + []byte(`{"speed": 70, "duration_seconds": 600, "reason": "integration"}`)) + pub.Wait() + waitFor(t, "SetOverride called", func() bool { + consumer.mu.Lock() + defer consumer.mu.Unlock() + return consumer.overrideSet && consumer.overrideSpeed == 70 + }) + + // 3. State publish (ticker at 1s interval) reaches the state topic. + waitFor(t, "state published", func() bool { + v, ok := obs.latest("only-fan-controller/state") + return ok && len(v) > 0 + }) + + // 4. LWT: forcibly stop the bridge's broker-side connection (ungraceful) and + // expect the retained will "offline" to be published. + waitFor(t, "bridge client registered on broker", func() bool { + _, ok := server.Clients.Get(cfg.MQTT.ClientID) + return ok + }) + cl, _ := server.Clients.Get(cfg.MQTT.ClientID) + cl.Stop(errors.New("integration: force ungraceful disconnect")) + waitFor(t, "LWT offline", func() bool { + return obs.seen("only-fan-controller/availability", "offline") + }) +} diff --git a/internal/mqtt/state.go b/internal/mqtt/state.go new file mode 100644 index 0000000..b2c0425 --- /dev/null +++ b/internal/mqtt/state.go @@ -0,0 +1,182 @@ +package mqtt + +import ( + "encoding/json" + "log" + "time" + + "github.com/sethpjohnson/only-fan-controller/internal/controller" +) + +// statePayload is the retained JSON document published to /state on +// every tick. Pointer fields marshal to null when absent (nil sensor reading / +// no override) so HA templates can distinguish "no data" from zero. +type statePayload struct { + CPUTemp *int `json:"cpu_temp"` + GPUTemp *int `json:"gpu_temp"` + FanSpeed int `json:"fan_speed"` + TargetSpeed int `json:"target_speed"` + Zone string `json:"zone"` + Mode string `json:"mode"` + FailsafeActive bool `json:"failsafe_active"` + FailsafeReason string `json:"failsafe_reason"` + RestorePending bool `json:"restore_pending"` + LastWriteFailed bool `json:"last_write_failed"` + OverrideSpeed *int `json:"override_speed"` + OverrideReason *string `json:"override_reason"` + OverrideExpires *string `json:"override_expires"` + ActiveHintCount int `json:"active_hint_count"` + // GPUs carries one entry per detected GPU device, so Home Assistant can show + // per-card temperature/utilization/power. GPUTemp above stays the overall max + // (fan logic and the aggregate sensor depend on it). The per-card discovery + // entities index into this array by position (gpus[0], gpus[1], ...). + GPUs []gpuState `json:"gpus"` +} + +// gpuState is one per-card entry in statePayload.GPUs. Keys here MUST stay in +// sync with the value_templates emitted by the per-GPU discovery sensors (see +// discovery.go); the key-correspondence test guards against drift. +type gpuState struct { + Index int `json:"index"` + Name string `json:"name"` + Temp int `json:"temp"` + Utilization int `json:"utilization"` + Power int `json:"power"` +} + +// buildStatePayload derives the wire payload from a controller status snapshot. +func buildStatePayload(status *controller.Status) statePayload { + p := statePayload{ + FanSpeed: status.CurrentSpeed, + TargetSpeed: status.TargetSpeed, + Zone: status.Zone, + Mode: status.Mode, + FailsafeActive: status.FailsafeActive, + FailsafeReason: status.FailsafeReason, + RestorePending: status.RestorePending, + LastWriteFailed: status.LastWriteFailed, + ActiveHintCount: len(status.ActiveHints), + } + if status.CPU != nil { + v := status.CPU.Max + p.CPUTemp = &v + } + if status.GPU != nil { + v := status.GPU.Max + p.GPUTemp = &v + for _, d := range status.GPU.Devices { + p.GPUs = append(p.GPUs, gpuState{ + Index: d.Index, + Name: d.Name, + Temp: d.Temp, + Utilization: d.Utilization, + Power: d.PowerDraw, + }) + } + } + if status.Override != nil { + s := status.Override.Speed + r := status.Override.Reason + e := status.Override.ExpiresAt.Format(time.RFC3339) + p.OverrideSpeed = &s + p.OverrideReason = &r + p.OverrideExpires = &e + } + return p +} + +// publishState marshals the current status and publishes it retained to the +// state topic. A marshal or publish error is rate-limited and dropped — the +// next tick republishes anyway, so no buffering is needed. +func (b *Bridge) publishState() { + status := b.consumer.GetStatus() + payload, err := json.Marshal(buildStatePayload(status)) + if err != nil { + if b.pubLimiter.allow() { + log.Printf("MQTT: failed to marshal state: %v", err) + } + return + } + if err := b.client.Publish(b.stateTopic(), 1, true, payload); err != nil { + if b.pubLimiter.allow() { + log.Printf("MQTT: failed to publish state: %v", err) + } + // Surface a targeted hint the first time publishing fails (rearmed after + // the next success). The rate-limited error above says *that* publishing + // failed; this says the most likely *why* — a wrong broker host/port — the + // exact pain the 8123-instead-of-1883 mistake causes. + if b.unreachableHint.arm() { + log.Printf("MQTT: still unable to reach broker %s — is the host/port correct? Home Assistant's MQTT broker is usually port 1883, not the 8123 web UI", b.broker) + } + return + } + b.unreachableHint.rearm() + // The state publish just succeeded, so the broker is reachable. Announce + // discovery for any GPU that has appeared since the last connect. Doing this + // only after a good state publish (rather than in onConnect) is what makes + // per-GPU entities show up: at cold start MQTT connects before the first GPU + // read, so the device set is empty at connect and only populated by later + // ticks. Gating on the successful publish also avoids retrying/logging + // discovery while the broker is down. + b.publishNewGPUDiscovery(status) +} + +// publishNewGPUDiscovery publishes retained discovery configs for any GPU device +// in status whose index has not yet been announced on the current connection, +// then records it so it is not republished every tick. It handles a GPU that +// appears seconds after startup (the cold-start case) or is hot-added later. +// gpuDiscovered is mutex-guarded because onConnect clears it from paho's +// goroutine while this runs on the publish-loop goroutine. A device is marked +// announced only if all of its configs published cleanly, so a transient failure +// is retried on a later tick. +func (b *Bridge) publishNewGPUDiscovery(status *controller.Status) { + if status == nil || status.GPU == nil { + return + } + for i, d := range status.GPU.Devices { + b.gpuDiscMu.Lock() + already := b.gpuDiscovered[d.Index] + b.gpuDiscMu.Unlock() + if already { + continue + } + published := true + for _, s := range b.gpuDeviceSpecs(i, d) { + payload, err := json.Marshal(s.config) + if err != nil { + log.Printf("MQTT: failed to marshal GPU discovery for %s/%s: %v", s.component, s.objectID, err) + published = false + continue + } + topic := b.discoveryTopic(s.component, s.objectID) + if err := b.client.Publish(topic, 1, true, payload); err != nil { + log.Printf("MQTT: failed to publish GPU discovery %s: %v", topic, err) + published = false + } + } + if published { + b.gpuDiscMu.Lock() + b.gpuDiscovered[d.Index] = true + b.gpuDiscMu.Unlock() + } + } +} + +// publishLoop publishes state on the monitoring interval until stopCh closes. +func (b *Bridge) publishLoop() { + defer close(b.doneCh) + interval := time.Duration(b.cfg.Monitoring.Interval) * time.Second + if interval <= 0 { + interval = 10 * time.Second + } + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-ticker.C: + b.publishState() + case <-b.stopCh: + return + } + } +} diff --git a/internal/mqtt/state_test.go b/internal/mqtt/state_test.go new file mode 100644 index 0000000..26226db --- /dev/null +++ b/internal/mqtt/state_test.go @@ -0,0 +1,96 @@ +package mqtt + +import ( + "encoding/json" + "testing" + "time" + + "github.com/sethpjohnson/only-fan-controller/internal/controller" + "github.com/sethpjohnson/only-fan-controller/internal/monitor" +) + +func TestBuildStatePayloadFull(t *testing.T) { + exp := time.Date(2026, 7, 17, 12, 0, 0, 0, time.UTC) + status := &controller.Status{ + CPU: &monitor.CPUReading{Max: 71}, + GPU: &monitor.GPUReading{Max: 63}, + CurrentSpeed: 40, + TargetSpeed: 55, + Zone: "warm", + Mode: "override", + FailsafeActive: true, + FailsafeReason: "sensor-loss", + RestorePending: true, + LastWriteFailed: false, + Override: &controller.Override{Speed: 55, Reason: "burn-in", ExpiresAt: exp}, + ActiveHints: []*controller.WorkloadHint{{Source: "a"}, {Source: "b"}}, + } + + p := buildStatePayload(status) + + if p.CPUTemp == nil || *p.CPUTemp != 71 { + t.Fatalf("cpu_temp = %v, want 71", p.CPUTemp) + } + if p.GPUTemp == nil || *p.GPUTemp != 63 { + t.Fatalf("gpu_temp = %v, want 63", p.GPUTemp) + } + if p.FanSpeed != 40 || p.TargetSpeed != 55 { + t.Fatalf("speeds = %d/%d, want 40/55", p.FanSpeed, p.TargetSpeed) + } + if p.OverrideSpeed == nil || *p.OverrideSpeed != 55 { + t.Fatalf("override_speed = %v, want 55", p.OverrideSpeed) + } + if p.OverrideReason == nil || *p.OverrideReason != "burn-in" { + t.Fatalf("override_reason = %v, want burn-in", p.OverrideReason) + } + if p.OverrideExpires == nil || *p.OverrideExpires != exp.Format(time.RFC3339) { + t.Fatalf("override_expires = %v", p.OverrideExpires) + } + if p.ActiveHintCount != 2 { + t.Fatalf("active_hint_count = %d, want 2", p.ActiveHintCount) + } + if !p.FailsafeActive || p.FailsafeReason != "sensor-loss" || !p.RestorePending { + t.Fatalf("failsafe fields wrong: %+v", p) + } +} + +func TestBuildStatePayloadNilsBecomeNull(t *testing.T) { + p := buildStatePayload(&controller.Status{}) + b, err := json.Marshal(p) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var m map[string]any + if err := json.Unmarshal(b, &m); err != nil { + t.Fatalf("unmarshal: %v", err) + } + for _, k := range []string{"cpu_temp", "gpu_temp", "override_speed", "override_reason", "override_expires"} { + if m[k] != nil { + t.Fatalf("%s = %v, want null", k, m[k]) + } + } +} + +func TestPublishStatePublishesRetainedJSON(t *testing.T) { + h := &clientHolder{} + consumer := &fakeConsumer{status: &controller.Status{CurrentSpeed: 33, Zone: "idle"}} + b := New(testConfig(), consumer, h.factory) + b.Start() + + b.publishState() + + msg, ok := h.client.lastPublishOn("only-fan-controller/state") + if !ok { + t.Fatal("no publish on state topic") + } + if !msg.retained || msg.qos != 1 { + t.Fatalf("state publish retained=%v qos=%d, want true/1", msg.retained, msg.qos) + } + var m map[string]any + if err := json.Unmarshal(msg.payload, &m); err != nil { + t.Fatalf("state payload not valid JSON: %v", err) + } + if m["fan_speed"].(float64) != 33 || m["zone"].(string) != "idle" { + t.Fatalf("state payload wrong: %v", m) + } +} diff --git a/internal/validate/validate.go b/internal/validate/validate.go new file mode 100644 index 0000000..eb9833f --- /dev/null +++ b/internal/validate/validate.go @@ -0,0 +1,82 @@ +// Package validate holds request-field validation shared by the HTTP API and +// the MQTT bridge, so both control surfaces enforce identical rules (charset, +// length, closed sets, control-character rejection) on operator-supplied input. +package validate + +import ( + "fmt" + "regexp" + "unicode" +) + +const ( + // MaxHintFieldLen bounds free-form hint identifiers (source/type). Small on + // purpose: these are process names, not prose. + MaxHintFieldLen = 64 + // MaxOverrideReasonLen bounds the free-text override reason. + MaxOverrideReasonLen = 128 +) + +// hintFieldPattern is the allowed charset for hint source/type. Restricting to +// this set means no stored hint string can carry HTML/script even if a dashboard +// interpolation is ever missed. +var hintFieldPattern = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`) + +// allowedIntensities is the closed set of intensity values the controller +// understands ("" means "unspecified"). +var allowedIntensities = map[string]bool{"": true, "low": true, "medium": true, "high": true} + +// allowedHintActions is the closed set of hint actions the controller acts on. +var allowedHintActions = map[string]bool{"start": true, "stop": true} + +// HintField enforces length and charset on a hint source/type. name is used only +// for the error message. +func HintField(name, value string) error { + if len(value) > MaxHintFieldLen { + return fmt.Errorf("%s exceeds %d characters", name, MaxHintFieldLen) + } + if !hintFieldPattern.MatchString(value) { + return fmt.Errorf("%s must match [A-Za-z0-9_.-]", name) + } + return nil +} + +// HintAction enforces the closed set of hint actions. +func HintAction(action string) error { + if !allowedHintActions[action] { + return fmt.Errorf("action must be one of start, stop") + } + return nil +} + +// Intensity enforces the closed set of hint intensities. +func Intensity(intensity string) error { + if !allowedIntensities[intensity] { + return fmt.Errorf("intensity must be one of low, medium, high") + } + return nil +} + +// OverrideSpeed enforces the 0..100 percentage range. The controller still +// clamps to the configured min/max band; this only rejects nonsensical input. +func OverrideSpeed(speed int) error { + if speed < 0 || speed > 100 { + return fmt.Errorf("speed must be 0-100") + } + return nil +} + +// OverrideReason enforces a length cap and rejects control characters on the +// human-readable override reason. Normal punctuation, spaces, and quotes are +// valid free text. +func OverrideReason(reason string) error { + if len(reason) > MaxOverrideReasonLen { + return fmt.Errorf("reason exceeds %d characters", MaxOverrideReasonLen) + } + for _, r := range reason { + if unicode.IsControl(r) { + return fmt.Errorf("reason must not contain control characters") + } + } + return nil +} diff --git a/internal/validate/validate_test.go b/internal/validate/validate_test.go new file mode 100644 index 0000000..5f89cb0 --- /dev/null +++ b/internal/validate/validate_test.go @@ -0,0 +1,64 @@ +package validate + +import "testing" + +func TestHintField(t *testing.T) { + if err := HintField("source", "plex.transcode-1"); err != nil { + t.Fatalf("valid source rejected: %v", err) + } + if err := HintField("source", "bad source!"); err == nil { + t.Fatal("source with space/bang should be rejected") + } + if err := HintField("type", string(make([]byte, MaxHintFieldLen+1))); err == nil { + t.Fatal("overlong type should be rejected") + } +} + +func TestHintAction(t *testing.T) { + if err := HintAction("start"); err != nil { + t.Fatalf("start rejected: %v", err) + } + if err := HintAction("stop"); err != nil { + t.Fatalf("stop rejected: %v", err) + } + if err := HintAction("pause"); err == nil { + t.Fatal("pause should be rejected") + } +} + +func TestIntensity(t *testing.T) { + for _, ok := range []string{"", "low", "medium", "high"} { + if err := Intensity(ok); err != nil { + t.Fatalf("intensity %q rejected: %v", ok, err) + } + } + if err := Intensity("extreme"); err == nil { + t.Fatal("extreme should be rejected") + } +} + +func TestOverrideSpeed(t *testing.T) { + for _, ok := range []int{0, 50, 100} { + if err := OverrideSpeed(ok); err != nil { + t.Fatalf("speed %d rejected: %v", ok, err) + } + } + if err := OverrideSpeed(-1); err == nil { + t.Fatal("negative speed should be rejected") + } + if err := OverrideSpeed(101); err == nil { + t.Fatal("speed > 100 should be rejected") + } +} + +func TestOverrideReason(t *testing.T) { + if err := OverrideReason("manual burn-in test (2h)"); err != nil { + t.Fatalf("normal prose rejected: %v", err) + } + if err := OverrideReason("bad\x00null"); err == nil { + t.Fatal("control character should be rejected") + } + if err := OverrideReason(string(make([]byte, MaxOverrideReasonLen+1))); err == nil { + t.Fatal("overlong reason should be rejected") + } +} diff --git a/unraid/only-fan-controller.xml b/unraid/only-fan-controller.xml index 65d8076..501fbde 100644 --- a/unraid/only-fan-controller.xml +++ b/unraid/only-fan-controller.xml @@ -34,6 +34,10 @@ root + false + + + true 20 65