Skip to content

manager: restart self-crashed processes (backport of commaai/openpilot#36755) - #311

Open
PeterPhuTran wants to merge 3 commits into
FrogAi:MAKE-PRS-HEREfrom
PeterPhuTran:manager-restart-if-crash
Open

manager: restart self-crashed processes (backport of commaai/openpilot#36755)#311
PeterPhuTran wants to merge 3 commits into
FrogAi:MAKE-PRS-HEREfrom
PeterPhuTran:manager-restart-if-crash

Conversation

@PeterPhuTran

Copy link
Copy Markdown

Description

A managed process that dies on its own while its should_run predicate stays true
is never restarted:

  • stop() — the only place that logs the death and clears self.proc — is only
    reached when should_run flips false
  • start() early-returns whenever self.proc is set, even for a dead child
  • check_watchdog() only covers processes with watchdog_max_dt

So the process stays down for the rest of the onroad session while managerState
reports shouldBeRunning=True / running=False, and selfdrived raises
processNotRunning (NO_ENTRY + SOFT_DISABLE) permanently — engagement blocked
and force-disengaged until the car is restarted.

Observed live during the #309 investigation: loggerd SIGABRTed 3 seconds after
starting (NVMe failure) and stayed dead the whole session — alert on screen, no
restart, no death log, RouteCount frozen.

Fix: backport of commaai#36755 — an opt-in restart_if_crash flag on
ManagerProcess plus a reap in ensure_running() (verbatim upstream logic),
preserving FrogPilot's watchdog machinery and should_run signature. Unlike
upstream, the flag is also plumbed through NativeProcess, since FrogPilot's
crash-relevant targets (loggerd/encoderd/TICI ui) are native.

Flagged processes: ui (mirrors upstream's only flagged process), plus loggerd
and encoderd (per #309 — a logging-process crash must not permanently block
engagement).

Known tradeoff (accepted upstream in commaai#36755): a flagged process that crash-loops is
restarted once per manager loop pass. Opt-in, limited to these processes.

Verification

New test_restart_if_crash in system/manager/test/test_manager.py:

  • flagged process gets reaped and restarted with a new pid
  • unflagged process keeps opt-in semantics (stays dead)
  • fails on the unpatched base, passes with the fix; verified on-device
    (comma three, including a -W error run matching CI's -Werror)

🤖 Generated with Claude Code

A managed process that dies on its own while its should_run predicate
stays true is never reaped: stop() - the only place that logs the death
and clears self.proc - is only reached when should_run flips false, and
start() early-returns whenever self.proc is set, even for a dead child.
The process stays down for the rest of the onroad session while
managerState reports shouldBeRunning=True/running=False, so selfdrived
raises processNotRunning (NO_ENTRY + SOFT_DISABLE) permanently.

Observed on-device while investigating FrogAi#309: loggerd SIGABRTed 3s after
starting (NVMe failure) and stayed dead for the whole session with the
engagement-blocking alert on screen and no restart or death log.

Upstream fixed this in commaai#36755 with an opt-in
restart_if_crash flag and a reap in ensure_running; this backports that
mechanism verbatim while preserving FrogPilot's watchdog machinery and
should_run signature. Unlike upstream, the flag is also plumbed through
NativeProcess (FrogPilot's crash-relevant targets are native).

Flagged processes: ui (mirrors upstream's choice), plus loggerd and
encoderd (per FrogAi#309 - a logging process crash must not permanently block
engagement). Tradeoff accepted upstream: a flagged process that
crash-loops is restarted once per manager loop pass.

Adds test_restart_if_crash covering both the restart and the opt-in
(unflagged stays dead) semantics.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Thanks for contributing! A maintainer will review your pull request soon.

Field incident on 2026-07-10 (same device as FrogAi#309): updated logged a
130KB 'git diff output' record for a dirty working tree; logmessaged
crashed publishing it to the 256KB logMessage msgq (msgq asserts
3*msg_size <= queue_size, so anything over ~85KB is fatal while
logmessaged's own guard only rejects >2MB). Being unflagged, it stayed
dead for the rest of the session - permanent processNotRunning alert
and total swaglog loss (the crash of the logging daemon is the one
crash it can never log).

logmessaged is exactly the kind of always_run, non-safety process this
flag exists for.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@PeterPhuTran

Copy link
Copy Markdown
Author

Pushed one more flag: logmessaged — justified by a live incident on the same device today. updated logged a 130KB git diff output record for a dirty working tree; logmessaged crashed publishing it to the 256KB logMessage msgq (msgq asserts 3*msg_size <= queue_size, so anything over ~85KB is fatal, while logmessaged's own guard only rejects >2MB). Being unflagged, it stayed dead for the rest of the session — permanent "Process Not Running: logmessaged" and total swaglog loss. The crash of the logging daemon is the one crash it can never log, which makes it exactly the kind of always-run, non-safety process this flag exists for. (The guard/queue-size mismatch is a separate bug — happy to PR that fix too.)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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