Skip to content

fix(firmware): raise the httpd stack for OTA image validation - #1831

Open
clonea1 wants to merge 6 commits into
ruvnet:mainfrom
clonea1:contrib/rollback
Open

clonea1 wants to merge 6 commits into
ruvnet:mainfrom
clonea1:contrib/rollback

Conversation

@clonea1

@clonea1 clonea1 commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Adopted from upstream PR #1594 (Juan Kuscevic, open since 2026-08-11).

The OTA upload handler runs esp_ota_end() -> esp_image_verify() on the httpd
task's own stack, and HTTPD_DEFAULT_CONFIG allocates 4 KB. That overflows at
the very end of an upload: the transfer completes, validation panics, and the
node reboots into the old image. The symptom reads as 'the update did not
take' rather than as a crash, which makes it hard to attribute.

12 KB costs one page of RAM.


Stacked on #1827 (remote-config) — this branch carries that commit too, so review only the last two here, or merge #1827 first and this shrinks.

Rebased onto 33a9e908 (2026-09-14)

Two conflicts, both against files #1902 added at the same insertion points.
Both resolved additively:

  • main/main.c — #include "serial_onboarding.h" and #include "config_api.h"
    both kept.
  • main/CMakeLists.txt — serial_onboarding.c / serial_onboarding_protocol.c
    kept, config_api.c added. The branch's old side of this hunk re-listed
    main.c, csi_collector.c, stream_sender.c and nvs_config.c a second
    time in SRCS — a defect from an earlier resolution, not intent. SRCS now
    has 27 entries and no duplicates.

ota_update.c diffs as text again. The middle commit carried two literal
NUL bytes where '\0' was meant (s_rollback_reason[0] = '\0';, twice — a
lost backslash), which made git classify the file as binary: that commit and
this PR's headline commit both rendered as "Binary files differ", so the
change this PR is named after could not be read on GitHub. The escapes are
restored at their origin. The tree at the tip is bit-identical to a plain
rebase
(0c8ed84b) — this changes how the history reads, not what ships.

The ESP-IDF 6 lane needed #1827's cJSON fix. Rebasing exposed this: on
33a9e908 the Build XIAO C6 external antenna (ESP-IDF 6.0.2) job passes,
and with only the three original commits it failed with
Failed to resolve component 'json' required by component 'main': unknown name
— config_api.c needs cJSON, which is bundled through 5.x and moved to the
component manager in v6. #1827's fix(firmware): source cJSON from the component manager on ESP-IDF 6 is cherry-picked here so this branch is green
on its own; it is the same patch, so it collapses if #1827 merges first.

Verification

  • make -C firmware/esp32-csi-node/test host_tests — ALL PASS, 0 failures.
    The branch touches no file under test/, so that run is a regression control,
    not new coverage; config_api.c and ota_update.c are compiled by the
    firmware CI jobs, not by the host suite.
  • No commit in this PR touches anything outside firmware/esp32-csi-node/.

Known-red check, and it is not clean-inherited

Build firmware (esp32s3 / 8mb) fails its Verify binary size budget step.
The compile itself is green; the budget is a single global number that main
already exceeds.

binary 1152 KB gate over by
main @ 33a9e908 1,191,568 B 1,179,648 B 11,920 B
this branch 1,204,208 B 1,179,648 B 24,560 B

So the failure is inherited, but the byte counts do not match: this branch
adds 12,640 B of firmware (config_api.c plus the rollback path), so it would
trip the current global gate even on a green main. It is not over its actual
partition — ESP-IDF reports the smallest app partition as 0x200000 (2 MB) with
43% free. #1891 ("size the firmware budget to each variant's own app
partition") is what makes this measurable rather than global.

🤖 Generated with claude-flow

Joe and others added 3 commits September 14, 2026 14:53
Changing any node setting meant a USB cable and physical access. On a fleet
mounted on walls and ceilings that is the difference between a five-second
change and taking a board down.

Adds an authenticated HTTP config endpoint on each node. Settings are typed and
range-checked at the boundary, persisted to NVS, and applied without a reflash.
Includes LED mode and brightness: the status LED is bright enough to be
unwelcome in a bedroom, and that is a configuration question, not a reflash.

TRIAL AND REVERT. A setting that breaks connectivity would otherwise brick a
node remotely -- change the WiFi password and the node is simply gone. So a
change that could sever the uplink is applied on trial: the old values are
banked, the node reboots, and the change is confirmed only once the node
reassociates and gets an address. If it does not within the deadline, the node
restores the banked values and comes back on the old settings. Verified on
hardware with a deliberately wrong password: recovered on its own in 60.5 s.

The reply to a trial push is sent before the reboot rather than after it, so
the caller learns the trial was accepted instead of seeing a dropped
connection and having to guess.

Mutating requests require a pre-shared key checked in constant time, and the
endpoint FAILS CLOSED when no key is provisioned. The key is read from a file
path given at build time with no default, so no personal path is baked into a
published tree.

Co-Authored-By: claude-flow <ruv@ruv.net>
Config changes already self-heal: a node given credentials it cannot associate
with banks the old ones, reboots, and restores them. Firmware had no such
protection -- a bad image meant a boot loop and a USB cable, which for nodes
mounted around a house is the difference between a mistake and an afternoon.

CONFIG_BOOTLOADER_APP_ROLLBACK_ENABLE makes a freshly-OTA'd image boot as
PENDING_VERIFY; if it reboots without confirming, the bootloader reverts.

WHEN to confirm is the whole design. Confirming in app_main() would make the
mechanism decorative, since any image that starts would qualify. The failure
that actually costs someone a ladder is a node that cannot be REACHED, and
unreachable means no WiFi -- so the criterion is: obtained an IP, then stayed
up for a 60 s soak. The soak also catches an image that associates and then
crashes, which confirming on IP alone would miss. It shares the
IP_EVENT_STA_GOT_IP hook with the config trial: both mechanisms key off the
same evidence that the node can still talk.

Deliberately conservative -- a reboot for any other reason inside the soak
rolls back a good image. That errs toward a node that works over a node that
is new.

A silent revert is nearly useless operationally: a node that reappears on its
old firmware looks identical to one whose update never arrived. So the boot
check also detects an already-reverted slot and reports the failed partition,
its version and the reset reason, in the log, in NVS so a late poller still
sees it, and on /ota/status as `last_rollback` alongside `pending_verify`.

MEASURED on node 3 (esp32c6), not inferred:

  happy path -- OTA'd a good image, it booted PENDING_VERIFY, got an IP,
  soaked, confirmed; a subsequent reboot stayed on ota_1, which only happens
  if confirmation actually occurred.

  failure path -- OTA'd an image built to abort() 15 s after boot, inside the
  soak window:

    ota_update: OTA update successful! Rebooting to partition 'ota_0'...
    ota_update: new image on trial (ota_0): must reach the network and
                survive 60 s or the bootloader reverts
    main: Got IP: <node-ip>
    ota_update: image on trial reached the network; confirming in 60 s
    abort() was called at PC 0x4201ec43 on core 0
    ota_update: FIRMWARE ROLLBACK: ota_0 image 0.8.8 was aborted;
                recovered via panic
    main: Got IP: <node-ip>

  and /ota/status then returns
    "last_rollback": "ota_0 image 0.8.8 was aborted; recovered via panic"

The node recovered with no USB intervention. The panic used to produce the bad
image was a throwaway and is not in this commit.

Co-Authored-By: claude-flow <ruv@ruv.net>
Adopted from upstream PR ruvnet#1594 (Juan Kuscevic, open since 2026-08-11).

The OTA upload handler runs esp_ota_end() -> esp_image_verify() on the httpd
task's own stack, and HTTPD_DEFAULT_CONFIG allocates 4 KB. That overflows at
the very end of an upload: the transfer completes, validation panics, and the
node reboots into the old image. The symptom reads as 'the update did not
take' rather than as a crash, which makes it hard to attribute.

12 KB costs one page of RAM.

Co-Authored-By: claude-flow <ruv@ruv.net>
(cherry picked from commit cbfecfdb4b20dd37b40ab7d43f87504cad39024e)
Joe and others added 3 commits September 14, 2026 15:01
cJSON ships as the bundled `json` component through 5.x and moved out to the
component manager in v6, where requiring it fails outright:

    Failed to resolve component 'json' required by component 'main': unknown name.

Requiring it unconditionally kept the 5.4 lane green while breaking the 6.0.2
lane -- one toolchain is not the build matrix. `json` is now required only when
IDF_VERSION_MAJOR < 6, and `espressif/cjson` is declared in idf_component.yml
under an `idf_version >=6.0` rule so the two never coexist in one build.

Verified building on esp32c6/16MB with ESP-IDF 5.4.

Co-Authored-By: claude-flow <ruv@ruv.net>
Claude-Session: https://claude.ai/code/session_01PVWMiHQifoYXL7uL3bphrZ
… KiB

Unblocks the "Verify binary size budget" step of Build firmware
(esp32s3 / 8mb) -- this branch's head already carries the image growth
from 130afab but was still checked against the old 1152 KiB limit.
Does not address this branch's separate RUSTSEC-2026-0285 (rustls)
failure on Rust Dependency Audit.

Co-Authored-By: claude-flow <ruv@ruv.net>
… confirming an OTA image

The one-shot 60s soak this branch introduced confirms on wall-clock-plus-
network-presence alone -- it never checks whether the node can actually
send data. docs/ADR-360-node-crash-root-cause-2026-09-21.md found this gap
the hard way: four fleet nodes soaked and self-confirmed cleanly in
isolation during their solo OTA push, then panicked under later 9-node
concurrent load a solo soak can't reproduce, with no rollback safety net
left by the time the crash actually happened.

rollback_confirm()/ota_rollback_notify_connected() now require both the
60s soak AND stream_sender-observed send-health
(stream_sender_last_success_us() != 0 and stream_sender_failure_streak() <
3) before calling esp_ota_mark_app_valid_cancel_rollback(), rechecked
periodically (10s) instead of on a single timer fire, deferring up to 5
minutes past the base soak before giving up without force-confirming --
consistent with this file's existing "a node that works over a node that
is new" design.

stream_sender's backoff is broadened from errno==ENOMEM only to any send
failure (an EIO flood fell through the old ENOMEM-only gate entirely in
the incident above), and gains a new stream_sender_failure_streak()
accessor for the confirmation gate to read.

Full incident writeup, fix plan, and host-test coverage:
docs/ADR-360-crash-fix-plan-2026-09-22.md,
docs/ADR-360-node-crash-root-cause-2026-09-21.md (RuView main repo).

Co-Authored-By: claude-flow <ruv@ruv.net>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant