Skip to content

[1.4] core: services: ardupilot_manager: Cap consecutive start failures - #4121

Open
joaoantoniocardoso wants to merge 1 commit into
bluerobotics:1.4-devfrom
joaoantoniocardoso:fix/1.4-autopilot-start-failure-cap
Open

[1.4] core: services: ardupilot_manager: Cap consecutive start failures#4121
joaoantoniocardoso wants to merge 1 commit into
bluerobotics:1.4-devfrom
joaoantoniocardoso:fix/1.4-autopilot-start-failure-cap

Conversation

@joaoantoniocardoso

@joaoantoniocardoso joaoantoniocardoso commented Aug 11, 2026

Copy link
Copy Markdown
Member

Relates to #4070. Rebased onto 1.4-dev so it sits on top of 71500f9 (fix(i2c): prevent file descriptor leak in SMBus usage), which matters for the story below.

Problem

A customer moved an SD card from a Pi 4 with a Navigator to a Pi 4 without one. The autopilot service pegged the CPU, became unresponsive, and recovered only when the card went back to the original Pi.

From the logs of that incident, in auto_restart_ardupilot():

Time What happened
17:43:11 Only SITL detected, so get_board_to_be_used() refuses to start: Only available board is SITL, and it wasn't explicitly chosen.
17:43:11 - 17:46:25 32 restart cycles, ~5.8s apart
17:46:09 GET /available_boards still returns 200
17:46:16 First OSError: [Errno 24] Too many open files
17:46:15+ GET /available_boards returns 500
after uvicorn's socket.accept() logs a full traceback per retry, forever, ~150MB in ten minutes

Two things went wrong, and the second was a consequence of the first:

  1. 100% CPU. Not the retry loop itself, which sleeps, but asyncio's accept handler after EMFILE: it does not break out of its retry loop, so it emitted a loguru traceback per readiness event.
  2. The service became unresponsive, so switching to SITL was not an option. SITL is always in available_boards, but that endpoint started returning 500 once descriptors ran out, and the frontend clears its board list on error, leaving an empty "Available boards" dropdown. POST /board calls available_boards() too, so it would have failed as well.

What actually exhausted the descriptors

I reproduced this on a Pi 4 running this branch, by booting a Navigator-configured SD card with the HAT removed. The service settled at 338 open descriptors against a soft limit of 1024, of which 330 were handles on /dev/i2c-1 — about 32 leaked per retry cycle.

That arithmetic matches the incident exactly: at 32 per cycle the limit arrives at ~31 cycles, and the customer hit EMFILE after 32.

The leak was check_for_i2c_device() calling SMBus(bus_number) without ever closing it. With a HAT present, detection succeeds on the first pass and is cached, so it leaks 4 descriptors once and nobody notices. With the HAT absent, detection never caches, so all 5 passes re-probe on every call and every probe leaks.

That leak is already fixed on 1.4-dev by 71500f9, which this PR is rebased on. I verified the fixed version leaks nothing: 200 probes against the Navigator's four addresses on a bare Pi, all failing with [Errno 121] Remote I/O error, zero descriptor growth.

So 71500f9 is what prevents the EMFILE storm and keeps the API responsive. This PR is no longer the root-cause fix, and the remaining justification is narrower.

What this PR still fixes

With the leak fixed, a Pi that cannot start any board still retries forever, every 5 seconds, indefinitely. Each cycle runs a full kill_ardupilot() (including a MAVLink disarm attempt that times out), tears down and recreates the MavlinkManager, and runs a 5-pass i2c detection sweep.

Measured on hardware: ~8KB of logs per cycle, ~12 cycles/minute, so roughly 140MB/day written to the SD card, forever, for a machine that will never succeed without user action.

This gives up after 10 consecutive failures instead:

  • Transient failures still self-heal, with 10 attempts over ~50s of grace.
  • An explicit POST /start or POST /board re-arms the watchdog, since start_ardupilot() still sets should_be_running = True in its finally. That gives one further attempt per user action.
  • Observing the autopilot running clears the counter, so a board started outside the loop gets the full budget back.
  • The counter lives in __init__ rather than setup(), because setup() runs on every start attempt and would reset it.

It also bounds the blast radius of any future resource leak on this path, which is how the original incident escalated from "no board" to "unrecoverable".

Hardware validation

Pi 4, this branch, Navigator-configured SD card, HAT physically removed, preferred_board unset (matching the customer's settings).

Result
Detection probed Navigator, NavigatorPi5, NavigatorPi4, Argonot, found none
Failures climbed cleanly 1/10 to 10/10
Gave up Consecutive start failures threshold reached, ~2m45s after boot
Log size 80KB (a pre-cap run on the same hardware produced 220MB)
EMFILE none
CPU 1.0% service average, system load flat at ~1.2
Stability flat for 12+ minutes after giving up, no drift

Note the 338 descriptors above were measured on a build predating the rebase, so without 71500f9. With both changes the i2c descriptors should stay flat.

Considered and deliberately left out

  • Moving should_be_running = True out of the finally in start_ardupilot(). Smaller diff, larger behaviour change: it removes auto-retry for transient failures entirely and flips is_running() for serial boards. Worth doing on its own merits, not in a release-candidate branch.
  • Exponential backoff. With a cap of 10 it adds code without changing the outcome.
  • Invalidating the Linux board detection cache. Not involved: the service restarted fresh at 17:43, and the cache only ever stores a successful detection.
  • Frontend changes (banner, SITL fallback in the dropdown). Would not have helped here, since POST /board depends on the same failing endpoint.

Known limitation, not addressed here

"Change board", "Start autopilot", and "Stop autopilot" are all gated behind pirate mode in core/frontend/src/views/Autopilot.vue. After the cap trips, support and pirate-mode users can select SITL, but a regular user has no recovery path in the UI. That is pre-existing behaviour, unrelated to this incident, and worth discussing separately.

Test plan

  • black, isort, ruff, pylint 10.00/10, mypy all pass on the changed file
  • pytest output matches the base commit exactly, including a pre-existing local ardupilot_fw_decoder collection error and 74.32% coverage
  • Standalone simulation of the loop: cap trips after 10 watchdog attempts; a user-triggered start afterwards gets exactly one more attempt; a successful start clears the counter; a transient failure clearing on the 4th attempt still self-heals
  • On hardware: Navigator-configured SD card booted on a Pi 4 with no Navigator, counter climbs to 10, gives up, CPU stays idle, logs stay at 80KB
  • Normal Navigator boot unaffected (detected immediately, ArduSub 4.7.0, 14 descriptors)
  • Re-run the HAT-removed boot on the rebased build and confirm i2c descriptors stay flat
  • Confirm SITL can be selected from the Autopilot page while the service sits in its given-up state

@github-actions

Copy link
Copy Markdown

Automated PR Review

0. Summary

  • Verdict: LGTM :shipit:

Caps auto_restart_ardupilot() at 10 consecutive start failures by adding a counter incremented in the watchdog's except, clearing should_be_running when the threshold is hit, and resetting the counter whenever is_running() is observed True. The failure counter lives in __init__ (not setup()) so it survives restart attempts. Prevents the file-descriptor exhaustion / unresponsive-API scenario from #4070.

No further comments, nice job 👍

Generated by PR Review Bot. This is advisory, a human reviewer must still approve.

@joaoantoniocardoso joaoantoniocardoso changed the title core: services: ardupilot_manager: Cap consecutive start failures [1.4] core: services: ardupilot_manager: Cap consecutive start failures Aug 11, 2026
When no board can be started, auto_restart_ardupilot() retried every 5s
forever. Each cycle leaks file descriptors, so after ~30 minutes the
service hit EMFILE and uvicorn's accept loop began logging a traceback
per retry, pegging the CPU and making the API unresponsive. That also
removed the only recovery path, since selecting SITL requires a working
/available_boards.

Give up after 10 consecutive failures instead. Transient failures still
self-heal, and an explicit start or board change re-arms the watchdog.

The counter lives in __init__ rather than setup(), because setup() runs
on every start attempt and would reset it.

Fixes bluerobotics#4070
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