Repository navigation
Conversation
A driver that went offline or failed to start stayed down until a config reload or a manual restart. The watchdog retries with backoff, and leaves disabled, observe-only, waiting push, and running cloud drivers alone. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce8fffd49e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cfgMu.RLock() | ||
| defer cfgMu.RUnlock() | ||
| timeout := time.Duration(cfg.Site.WatchdogTimeoutS) * time.Second | ||
| return config.WithBatterySoCBounds(cfg.ModbusControlDrivers(), cfg.Batteries), timeout |
There was a problem hiding this comment.
The callback releases cfgMu after returning a copied driver list, before MaintainDrivers acquires the per-driver lifecycle gate. If a failed driver's retry is already due and the operator disables or changes it during that window, configreload.Apply can install and reload the new configuration first, after which the watchdog still starts the driver using the stale snapshot. Because later watchdog passes deliberately leave a running disabled driver alone, it can remain active indefinitely despite the saved setting; recheck the current configuration after acquiring the lifecycle gate or otherwise reject stale snapshots.
AGENTS.md reference: AGENTS.md:L20-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a9dd434. The snapshot is still copied under cfgMu and that lock is dropped before the restart, so a settings save is not blocked for the whole reconnect.
After acquireLifecycle, and before a restart is scheduled, the watchdog reads the installed config again. It skips the restart when that driver is gone, disabled, or observe-only, or when sameDriverConfig says the snapshot is stale. Reload already owns that transition, and a later pass no longer has a driver to leave running. An unchanged config is still restarted.
TestRestartWatchdogRechecksConfigAfterLifecycleGate holds the lifecycle gate, checks that the second read has not happened yet, then releases it with a disabled, observe-only, changed, or unchanged config. CI is green.
The watchdog copied the driver list, dropped the config lock, and could then start that copy after a reload had already disabled or edited the driver. It now re-reads the installed config after the lifecycle gate and leaves a disabled, observe-only, or changed driver to reload. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Problem and result
A Pixii stayed offline after a power outage until someone restarted the driver by hand. Core only restarted a driver on a config change or a manual
RestartByName, and a driver that failed to start at boot was logged once and never retried.The registry now restarts a driver that has been offline or stale, or that failed to start, with backoff 30s, 1 min, 2 min, 5 min, then every 5 min. A fresh reading resets the backoff. Each automatic restart is logged, and
/api/statusreportsauto_restarts,auto_restart_reason,auto_restart_error, andnext_auto_restart_ms.Related to #522. That GoodWe case is a dead Modbus TCP session after the controller reboots. TCP keepalive already landed; this does not change the dialer. It does replace a Modbus driver, including a Pixii or a GoodWe, whose session has gone stale, by stopping it (which runs
driver_default_mode) and starting it again.Scope and safety
Disabled drivers and observe-only drivers are left alone. A disabled entry does not stop a driver that is still running; reload owns that.
A running cloud or OAuth driver is not torn down when it goes quiet.
driver_initis where a rotated refresh token is applied, and restarting a live login would repeat the re-auth loopRegistry.Addwarns about. A cloud driver that failed to start is retried on the same backoff.MQTT, WebSocket, and TCP drivers that are polling and have never delivered a reading are waiting, not dead. A push driver that has delivered data and then goes stale is restarted. That memory survives the health record being wiped on remove.
The watchdog takes the same per-driver lifecycle lock as Reload and Restart, then uses the same remove-and-add path, including
driver_default_modeon the way out and again at startup. It does not run inside the control tick. On process exit it is cancelled and waited for beforeShutdownAll, so it cannot start a driver shutdown has already removed.The existing control-loop watchdog still marks a driver offline and sends its default mode. This one replaces the process when that is not enough. Stale site-meter data still stops dispatch; planner output is still not sent to hardware.
Open PRs #1003, #1364, #1337, and #1546 also touch
main.goorapi.go, in other regions. This branch is based on master and does not include those changes.Verification
All passed. The watchdog tests cover offline then restart then recovered (including
driver_default_mode), backoff growth through 30s/1m/2m/5m/5m and reset to 30s, disabled and observe-only left alone, start failure retried, a running cloud driver left alone with start failures still retried, MQTT waiting left alone, the lifecycle lock, and a cancelled context./api/statuscoversauto_restarts, the reason, the error, and the next attempt. GitHub CI on this PR is green, includingtest / go test + vet.No hardware run. A Pixii or GoodWe that stays offline after a power loss or a controller reboot still needs a site check.
Checklist