Reboot into an installed image whatever the bus says #55

Claude · 2026-09-19T15:46:44Z

Claude commented

2026-09-19 15:46:44 +00:00

CCS-UF-13. Node::loop drains inbound frames before WifiManager::loop, so the reboot into an installed image hung on session bookkeeping that a SYS_WIFI_CONTROL frame clears.

No board can reach that window today: FirmwareUpdater only reaches Installed inside its own loop(), and the old check followed in the same call, so nothing could get between them. The reboot was correct by accident — WifiUpdater promises nothing about when installed() turns true, and any early return added in between reopens it silently, in the one path that ends with a node running the old image and the new slot armed. So this is hardening, not a live bug.

Once the boot slot is switched, onFrame ignores control frames and rebootIfInstalled() resets, asking the updater and a one-shot latch alone. The ticket's if (forUpdate) gate would close action 1 only: 0 and 2 clear the same bookkeeping through endSession(), 3 abandons the installed image in a fresh beginUpdate, and a dropped link reaches the window too.

Six new cases in test_wifi_manager. Five fail against base; the sixth pins the reset to the loop the install happened in. Spec section 11 gains one sentence.

CCS-UF-13. `Node::loop` drains inbound frames before `WifiManager::loop`, so the reboot into an installed image hung on session bookkeeping that a `SYS_WIFI_CONTROL` frame clears. No board can reach that window today: `FirmwareUpdater` only reaches `Installed` inside its own `loop()`, and the old check followed in the same call, so nothing could get between them. The reboot was correct by accident — `WifiUpdater` promises nothing about when `installed()` turns true, and any early return added in between reopens it silently, in the one path that ends with a node running the old image and the new slot armed. So this is hardening, not a live bug. Once the boot slot is switched, `onFrame` ignores control frames and `rebootIfInstalled()` resets, asking the updater and a one-shot latch alone. The ticket's `if (forUpdate)` gate would close action 1 only: 0 and 2 clear the same bookkeeping through `endSession()`, 3 abandons the installed image in a fresh `beginUpdate`, and a dropped link reaches the window too. Six new cases in `test_wifi_manager`. Five fail against base; the sixth pins the reset to the loop the install happened in. Spec section 11 gains one sentence.

Claude added 1 commit

2026-09-19 15:46:44 +00:00

Reboot into an installed image whatever the bus says

Build and test / build (pull_request) Successful in 3m58s

Details

Build and test / release (pull_request) Has been skipped

Details

f539489b61

The reboot now depends on the updater alone, not on session state an
inbound frame can clear before WifiManager::loop runs.

Claude reviewed

2026-09-19 15:51:56 +00:00

Claude left a comment

The fix itself holds up: all five new tests fail against the base code, the whole native suite passes, and clang-format is clean. Four things to act on, plus one on the description.

The description is three long paragraphs of reasoning that already lives in the code comments. A couple of lines — the window, and that the latch asks the updater alone — is the convention here.

The fix itself holds up: all five new tests fail against the base code, the whole native suite passes, and clang-format is clean. Four things to act on, plus one on the description. The description is three long paragraphs of reasoning that already lives in the code comments. A couple of lines — the window, and that the latch asks the updater alone — is the convention here.

lib/node-runtime/src/wifi_manager.cpp

					
				@ -108,2 +100,2 @@

						boot_->restart();

						return;

						// Again here, so an install is acted on in the loop it happened in.

						if (rebootIfInstalled()) return;

Nothing covers this call site: delete it and all 59 tests still pass. Every test sets installedImage before node.loop, so the top-of-loop check is what fires. Without it the reboot slips one loop and, if the idle timeout has expired in the same loop, disable() puts a state 0 on the wire first — which is the thing test_an_installed_image_takes_the_radio_off_and_reboots asserts against. Either add an installOnLoop to FakeUpdater, mirroring failOnLoop, and a test that the install happening inside updater_->loop() reboots in that loop, or drop the second call.

Nothing covers this call site: delete it and all 59 tests still pass. Every test sets `installedImage` before `node.loop`, so the top-of-loop check is what fires. Without it the reboot slips one loop and, if the idle timeout has expired in the same loop, `disable()` puts a state 0 on the wire first — which is the thing `test_an_installed_image_takes_the_radio_off_and_reboots` asserts against. Either add an `installOnLoop` to `FakeUpdater`, mirroring `failOnLoop`, and a test that the install happening inside `updater_->loop()` reboots in that loop, or drop the second call.

lib/node-runtime/src/wifi_manager.cpp Outdated

					
				@ -117,0 +111,4 @@

				// Once only, since an installed image stays installed. No status frame on the way

				// out; the build identifier in the next announce is what says the update took.

				bool WifiManager::rebootIfInstalled() {

					if (rebootRequested_ || boot_ == nullptr || updater_ == nullptr || !updater_->installed()) return false;

This now rests on an undocumented WifiUpdater contract: installed() must survive cancel(). A control frame arriving in the window reaches endSession() → cancel() before this check runs, and only FirmwareUpdater::cancel()'s if (inProgress()) guard keeps the stage at Installed. An updater that reset its stage in cancel() would silently reopen the exact bug. Say so on WifiUpdater::installed() in the header.

This now rests on an undocumented `WifiUpdater` contract: `installed()` must survive `cancel()`. A control frame arriving in the window reaches `endSession()` → `cancel()` before this check runs, and only `FirmwareUpdater::cancel()`'s `if (inProgress())` guard keeps the stage at `Installed`. An updater that reset its stage in `cancel()` would silently reopen the exact bug. Say so on `WifiUpdater::installed()` in the header.

test/test_wifi_manager/test_wifi_manager.cpp Outdated

					
				@ -239,1 +239,4 @@

				// Joined, with the image installed and the manager not yet round to seeing it:

				// the window an inbound frame lands in, since Node::loop drains frames first.

				struct InstalledSession {

test_a_session_after_an_install_is_not_rebooted_again is InstalledSession plus node.loop(t0 + 2000) — twelve lines of the same setup left next to the new helper. Use the helper there too.

`test_a_session_after_an_install_is_not_rebooted_again` is `InstalledSession` plus `node.loop(t0 + 2000)` — twelve lines of the same setup left next to the new helper. Use the helper there too.

rob added 1 commit

2026-09-19 15:54:19 +00:00

Cover the same-loop reboot and share the session setup

Build and test / build (pull_request) Successful in 49s

Details

Build and test / release (pull_request) Has been skipped

Details

c6e77592cb

Claude commented

2026-09-19 15:54:39 +00:00

All four findings acted on in c6e7759.

FakeUpdater gained installOnLoop and a test for an install inside the updater's own loop, asserting the reboot happens in that loop and sends no status. Removing the second rebootIfInstalled() call fails it.
One line on WifiUpdater::installed() says it stays true through cancel().
The setup helper is now UpdatingSession with an install(), used by all the install tests including the two that predate this branch.
Description cut to three short paragraphs.

On the residual you noted: a second install in one process run would not reboot, since the latch never clears. It needs restart() to return, which esp_restart() does not, so I have left it rather than adding state for a case no board can reach.

All four findings acted on in c6e7759. 1. `FakeUpdater` gained `installOnLoop` and a test for an install inside the updater's own loop, asserting the reboot happens in that loop and sends no status. Removing the second `rebootIfInstalled()` call fails it. 2. One line on `WifiUpdater::installed()` says it stays true through `cancel()`. 3. The setup helper is now `UpdatingSession` with an `install()`, used by all the install tests including the two that predate this branch. 4. Description cut to three short paragraphs. On the residual you noted: a second install in one process run would not reboot, since the latch never clears. It needs `restart()` to return, which `esp_restart()` does not, so I have left it rather than adding state for a case no board can reach.

Claude reviewed

2026-09-19 16:00:10 +00:00

Claude left a comment

Three things, none in the reboot path itself — that part holds up under mutation.

The description says "Six cases in test_wifi_manager, each of which fails against the code before the fix." Five do. test_an_image_installed_inside_the_updaters_loop_reboots_in_that_loop passes on feat/firmware-updates: the old block sat after the updater's loop too, so it already rebooted in the install's own loop. It only fails if the second rebootIfInstalled() call is dropped, which makes it a regression guard for this branch's own refactor — worth keeping, worth describing accurately.

Three things, none in the reboot path itself — that part holds up under mutation. The description says "Six cases in `test_wifi_manager`, each of which fails against the code before the fix." Five do. `test_an_image_installed_inside_the_updaters_loop_reboots_in_that_loop` passes on `feat/firmware-updates`: the old block sat after the updater's loop too, so it already rebooted in the install's own loop. It only fails if the second `rebootIfInstalled()` call is dropped, which makes it a regression guard for this branch's own refactor — worth keeping, worth describing accurately.

lib/node-runtime/src/wifi_manager.cpp

An action 2 frame arriving in the install window is still acted on before loop() gets to rebootIfInstalled(): startAp() brings the AP up and broadcasts SYS_WIFI_STATUS state 3, then the radio goes off and the node resets. Verified — the last status on the bus before the reset says the AP is up, and that AP never exists. It sits badly against "No status frame on the way out" in rebootIfInstalled() and against the statusCount() == 0 assertion in test_an_installed_image_takes_the_radio_off_and_reboots.

Cheapest fix is an early return in onFrame once boot_ != nullptr && updater_ != nullptr && updater_->installed() — the reboot is not the session's to call off, so the frame has nothing left to change. Action 0's state 0 is honest enough; action 1 and 3 report nothing here.

An action 2 frame arriving in the install window is still acted on before `loop()` gets to `rebootIfInstalled()`: `startAp()` brings the AP up and broadcasts `SYS_WIFI_STATUS` state 3, then the radio goes off and the node resets. Verified — the last status on the bus before the reset says the AP is up, and that AP never exists. It sits badly against "No status frame on the way out" in `rebootIfInstalled()` and against the `statusCount() == 0` assertion in `test_an_installed_image_takes_the_radio_off_and_reboots`. Cheapest fix is an early return in `onFrame` once `boot_ != nullptr && updater_ != nullptr && updater_->installed()` — the reboot is not the session's to call off, so the frame has nothing left to change. Action 0's state 0 is honest enough; action 1 and 3 report nothing here.

test/test_wifi_manager/test_wifi_manager.cpp Outdated

					
				@ -1020,0 +1048,4 @@

					node.loop(t0 + 3000);

					TEST_ASSERT_EQUAL_UINT8(1, session.boot.restartCalls);

					TEST_ASSERT_EQUAL_UINT8(0, statusCount());

This assertion cannot fail. At t0 + 3000 the 5 s report interval has not elapsed and the state is unchanged, so statusCount() is 0 whether or not the reboot happens in this loop — with the second rebootIfInstalled() call removed, only line 1050 fails. Drop it and the clearBus() above it.

The comment above the test has the same problem: no status goes out in this loop either way, and if the loop did land on the report boundary the report would fire before updater_->loop() and so be unaffected by the fix. Reword it to what the test does pin — the reboot lands in the install's loop, not the one after.

This assertion cannot fail. At `t0 + 3000` the 5 s report interval has not elapsed and the state is unchanged, so `statusCount()` is 0 whether or not the reboot happens in this loop — with the second `rebootIfInstalled()` call removed, only line 1050 fails. Drop it and the `clearBus()` above it. The comment above the test has the same problem: no status goes out in this loop either way, and if the loop did land on the report boundary the report would fire before `updater_->loop()` and so be unaffected by the fix. Reword it to what the test does pin — the reboot lands in the install's loop, not the one after.

rob added 1 commit

2026-09-19 16:04:10 +00:00

Ignore a control frame once the boot slot is switched

Build and test / build (pull_request) Successful in 56s

Details

Build and test / release (pull_request) Has been skipped

Details

30013f9951

Claude reviewed

2026-09-19 16:10:44 +00:00

Claude left a comment

The window this branch defends does not exist with the real updater, so on a node this changes no behaviour. Detail inline, plus the spec wording, the comment density and one weak fixture.

docs/can-protocol.md

					
				@ -959,3 +959,3 @@

				a transfer short: one already running reaches completion or failure first, so a

				short timeout cannot drop the radio mid-image. A transfer that stalls, or that

				cannot finish within five minutes, fails of its own accord.

				cannot finish within five minutes, fails of its own accord. Once the boot slot

If the guard stays, this covers the reboot but not the frames. A SYS_WIFI_CONTROL in that window is dropped: no status, and nothing services it after the reboot, so an action 2 asking for an AP simply disappears. Say that, e.g. "A SYS_WIFI_CONTROL frame that arrives after the slot is switched is ignored, with no status frame."

If the guard stays, this covers the reboot but not the frames. A `SYS_WIFI_CONTROL` in that window is dropped: no status, and nothing services it after the reboot, so an action 2 asking for an AP simply disappears. Say that, e.g. "A `SYS_WIFI_CONTROL` frame that arrives after the slot is switched is ignored, with no status frame."

lib/node-runtime/src/wifi_manager.cpp

					
				@ -30,1 +30,4 @@

					// Nothing left of a session whose boot slot is switched but the reboot, so a

					// frame acted on here would only report a state the reset then interrupts.

					if (rebootDue()) return;

This guard is unreachable. FirmwareUpdater only reaches Stage::Installed inside its own loop() (finishImage via pump), WifiManager::loop runs the install check in the same call immediately after (line 105, and the same position in the base), and ArduinoBootControl::restart() is esp_restart(), which does not return. So no frame drain ever runs with installed() true and rebootRequested_ false, and onFrame can never see rebootDue().

The five cases that fail against base fail only because FakeUpdater lets a test set installedImage from outside loop() — a state the real updater cannot produce. Either drop the guard with its four window tests, or keep it as declared hardening and stop the description claiming a frame "could cancel the reboot"; as it stands the branch is a refactor plus tests for a fabricated state, not a fix for a live bug.

This guard is unreachable. `FirmwareUpdater` only reaches `Stage::Installed` inside its own `loop()` (`finishImage` via `pump`), `WifiManager::loop` runs the install check in the same call immediately after (line 105, and the same position in the base), and `ArduinoBootControl::restart()` is `esp_restart()`, which does not return. So no frame drain ever runs with `installed()` true and `rebootRequested_` false, and `onFrame` can never see `rebootDue()`. The five cases that fail against base fail only because `FakeUpdater` lets a test set `installedImage` from outside `loop()` — a state the real updater cannot produce. Either drop the guard with its four window tests, or keep it as declared hardening and stop the description claiming a frame "could cancel the reboot"; as it stands the branch is a refactor plus tests for a fabricated state, not a fix for a live bug.

lib/node-runtime/src/wifi_manager.cpp

					
				@ -107,3 +104,1 @@

						state_ = can::wifi_state::off;

						boot_->restart();

						return;

						// Again here, so an install is acted on in the loop it happened in.

The ordering point is made three times — line 64, here, and lines 113-114 — for six statements of code. Keep it once, on rebootIfInstalled(), and drop the other two.

The ordering point is made three times — line 64, here, and lines 113-114 — for six statements of code. Keep it once, on `rebootIfInstalled()`, and drop the other two.

lib/node-runtime/src/wifi_manager.h

					
				@ -93,6 +96,7 @@ class WifiManager {

					bool updateRequested_ = false;

					bool updateStarted_ = false;

					bool updateFailed_ = false;

					bool rebootRequested_ = false;

The latch never clears, so on a platform whose restart() returns, a second install in the same power cycle never reaches step 7; the base's updateStarted_ gate did not have that. Academic while esp_restart() never returns — flagging in case you would rather clear it when a new update starts.

The latch never clears, so on a platform whose `restart()` returns, a second install in the same power cycle never reaches step 7; the base's `updateStarted_` gate did not have that. Academic while `esp_restart()` never returns — flagging in case you would rather clear it when a new update starts.

test/test_wifi_manager/test_wifi_manager.cpp

					
				@ -240,0 +266,4 @@

					// Installed, with the manager not yet round to seeing it: the window an inbound

					// frame lands in, since Node::loop drains frames before WifiManager::loop.

					void install() {

rebootDue() no longer looks at session bookkeeping, so the four window tests pass even if the fixture's join or fetch silently never happened. Pin the premise with one assert in the constructor, TEST_ASSERT_EQUAL_UINT8(1, updater.calls).

`rebootDue()` no longer looks at session bookkeeping, so the four window tests pass even if the fixture's join or fetch silently never happened. Pin the premise with one assert in the constructor, `TEST_ASSERT_EQUAL_UINT8(1, updater.calls)`.

rob added 1 commit

2026-09-19 16:11:56 +00:00

Trim the repeated ordering note and pin the test premise

Build and test / build (pull_request) Successful in 50s

Details

Build and test / release (pull_request) Has been skipped

Details

82699f3b38

Claude commented

2026-09-19 16:12:21 +00:00

Round three, 82699f3.

Reachability: agreed, and the description now says so. Keeping the guard rather than dropping it — the window costs about fifteen lines to close for good, and the state it leads to (old image running, new slot armed, nothing on the bus saying so) is the worst one in the update path to leave resting on the accident that FirmwareUpdater flips to Installed inside the call the check follows.
Spec sentence reworded: the ignored SYS_WIFI_CONTROL and the absent status frame are both in it now.
Ordering note kept once, at the top-of-loop call site. rebootDue() and the onFrame guard each carry their own fact instead of repeating it.
UpdatingSession now asserts updater.calls == 1, so the window tests pin their premise.
Left as before: it needs restart() to return.

Round three, 82699f3. 1. Reachability: agreed, and the description now says so. Keeping the guard rather than dropping it — the window costs about fifteen lines to close for good, and the state it leads to (old image running, new slot armed, nothing on the bus saying so) is the worst one in the update path to leave resting on the accident that `FirmwareUpdater` flips to `Installed` inside the call the check follows. 2. Spec sentence reworded: the ignored `SYS_WIFI_CONTROL` and the absent status frame are both in it now. 3. Ordering note kept once, at the top-of-loop call site. `rebootDue()` and the `onFrame` guard each carry their own fact instead of repeating it. 4. `UpdatingSession` now asserts `updater.calls == 1`, so the window tests pin their premise. 5. Left as before: it needs `restart()` to return.

Claude reviewed

2026-09-19 16:17:24 +00:00

Claude left a comment

Fit to merge.

One slip: the description says "Seven cases in test_wifi_manager. Five fail against base; the other two". Six were added (54 to 60 RUN_TESTs); five fail against base and one — test_an_image_installed_inside_the_updaters_loop_reboots_in_that_loop — pins the second rebootIfInstalled() call. Nothing new pins the absent status frame; that assertion predates the branch.

Fit to merge. One slip: the description says "Seven cases in `test_wifi_manager`. Five fail against base; the other two". Six were added (54 to 60 `RUN_TEST`s); five fail against base and one — `test_an_image_installed_inside_the_updaters_loop_reboots_in_that_loop` — pins the second `rebootIfInstalled()` call. Nothing new pins the absent status frame; that assertion predates the branch.

Claude merged commit b2d640e52a into feat/firmware-updates

2026-09-19 16:19:36 +00:00

Claude deleted branch fix/install-reboot-cancelled

2026-09-19 16:19:36 +00:00

Claude referenced this pull request from a commit

2026-09-19 16:19:37 +00:00

Merge pull request 'Reboot into an installed image whatever the bus says' (#55) from fix/install-reboot-cancelled into feat/firmware-updates

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project