fix(startup): harden boot count persistence against hard-reset corruption - #470
Merged
Conversation
…tion HWIL evidence (integration-uart runs 29894110572 / 29979002023, PR #460): the boot count file was read back as 0x02FE191005000001 after a sequence of watchdog-commanded hard resets, and the next boot persisted exactly garbage+1 - the increment machinery works, but nothing guards against a torn flash write poisoning the count forever. Four changes to StartupManager: - Corruption guard: values > 1,000,000 are treated as a failed read (mirrors the existing quiescence-file 0xFF-fill guard) and reported via new WARNING_HI BootCountCorrupted with the raw value. - GET_BOOT_COUNT is now read-only: the unconditional write-back on every query was the largest source of boot-count write traffic, and each write is a window for a reset to tear the file. - Increment retry: if the first-tick persist fails (e.g. filesystem not ready), run_handler retries each 1Hz tick until it sticks - the increment is delayed, not lost. Failure warning de-duplicated per streak. - Atomic persist: write temp file then Os::FileSystem::rename over the target; littlefs renames are atomic so a mid-update reset leaves either the old or new file, never a torn one. Regression seam: test_safe_09_command_loss_triggers_safe_mode_and_reboot asserts boot count == initial+1 across a watchdog reset (with the retry poll from PR #460). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughStartupManager now detects implausible boot counts, reports corruption, persists counts through flushed temporary-file replacement, and retries failed persistence on later run-handler ticks. ChangesBoot Count Reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant StartupManager
participant FileSystem
participant BootCountFile
StartupManager->>FileSystem: Read boot count
FileSystem->>BootCountFile: Load stored value
StartupManager->>FileSystem: Write and flush temporary value
FileSystem->>BootCountFile: Store temporary value
StartupManager->>FileSystem: Rename temporary file
FileSystem-->>StartupManager: Return persistence status
StartupManager->>StartupManager: Retry on later run-handler ticks after failure
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@PROVESFlightControllerReference/Components/StartupManager/StartupManager.cpp`:
- Around line 140-150: The write path used by persist_boot_count must flush the
temporary file before considering it persisted. Update the underlying write
helper invoked by persist_boot_count to call file.flush() before file.close(),
propagate any flush failure as a write failure, and only allow
persist_boot_count to rename the temporary file after a successful flush; do not
compare file.close() with a status value because it returns void.
In
`@PROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp`:
- Around line 111-118: Initialize StartupManager::m_boot_count to 0 before any
run_handler logic reads it, using either an in-class member initializer or the
constructor initializer list. Preserve the existing boot-count persistence and
telemetry behavior after initialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2eb78bd6-6e19-4588-a248-57c0eafab100
📒 Files selected for processing (3)
PROVESFlightControllerReference/Components/StartupManager/StartupManager.cppPROVESFlightControllerReference/Components/StartupManager/StartupManager.fppPROVESFlightControllerReference/Components/StartupManager/StartupManager.hpp
…ot_count Addresses CodeRabbit review on #470: Os::File::close() is void and cannot report a deferred flush failure, so the write helper now requires file.flush() == OP_OK before reporting SUCCESS - persist_boot_count no longer renames a temp file whose data may not have reached storage. Also gives m_boot_count an in-class zero initializer; run_handler's first-tick guard reads it before any assignment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mikefly123
added a commit
that referenced
this pull request
Jul 23, 2026
Second flake in test_safe_09, seen on PR #470's CI (run 29983198334): CommandLossDetected asserted fine, but the immediate history scrape ran before EnteringSafeMode (emitted ~40ms later in the same firmware code path) reached the GDS. Use assert_event with a timeout so the check waits for the event instead of racing it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ineskhou
previously approved these changes
Jul 25, 2026
ineskhou
left a comment
ineskhou
left a comment
Contributor
There was a problem hiding this comment.
Passing Relevant Testing, and I agree with the post-merge monitoriing of the test this should fix
The code itself doesn't seem like it would impact any non-testable components in the rest of the system
Would like a sdd update before merge, but can also track separately
LGTM
Conflict resolution: kept main's hardcoded-startup additions (multi-sequencer ports, transmit-enable countdown) alongside this branch's boot-count hardening (zero-init sentinel, persist retry branch, Os/FileSystem include). SDD: documents the hardened persistence path (corruption guard, read-only GET_BOOT_COUNT, per-tick increment retry, atomic temp+rename), the two new state variables, BootCountCorrupted event, and REQ-SM-010/011. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nto harden/boot-count-persistence
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PROVESFlightControllerReference/Components/StartupManager/docs/sdd.md (1)
65-70: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate the documented port contract.
Both tables advertise nonexistent
sequenceStartedandcompleteSequenceports. Document the FPP-definedstartupCompleteSequence,startupsequenceStarted, safe-mode, payload, andloraFirstStartports so topology integrations use valid names.
PROVESFlightControllerReference/Components/StartupManager/docs/sdd.md#L65-L70: replace the obsolete generic sequencer rows with the actual FPP ports.docs-site/components/StartupManager.md#L65-L70: make the same replacement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PROVESFlightControllerReference/Components/StartupManager/docs/sdd.md` around lines 65 - 70, Update the port tables in PROVESFlightControllerReference/Components/StartupManager/docs/sdd.md lines 65-70 and docs-site/components/StartupManager.md lines 65-70 by replacing the obsolete sequenceStarted and completeSequence rows with the FPP-defined startupCompleteSequence, startupsequenceStarted, safe-mode, payload, and loraFirstStart ports, preserving their correct directions, types, and descriptions in both files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@PROVESFlightControllerReference/Components/StartupManager/docs/sdd.md`:
- Around line 65-70: Update the port tables in
PROVESFlightControllerReference/Components/StartupManager/docs/sdd.md lines
65-70 and docs-site/components/StartupManager.md lines 65-70 by replacing the
obsolete sequenceStarted and completeSequence rows with the FPP-defined
startupCompleteSequence, startupsequenceStarted, safe-mode, payload, and
loraFirstStart ports, preserving their correct directions, types, and
descriptions in both files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 80a9613a-de12-45db-95b7-dba7bfaa1ac4
📒 Files selected for processing (5)
PROVESFlightControllerReference/Components/StartupManager/StartupManager.cppPROVESFlightControllerReference/Components/StartupManager/StartupManager.fppPROVESFlightControllerReference/Components/StartupManager/StartupManager.hppPROVESFlightControllerReference/Components/StartupManager/docs/sdd.mddocs-site/components/StartupManager.md
The persist design claimed littlefs-atomic renames; the flight filesystem is FAT (CONFIG_FAT_FILESYSTEM_ELM, zephyr,fstab,fatfs). FAT renames are not guaranteed power-cut atomic. The write-then-rename design still closes the observed torn-in-place-write window (data fully flushed before it replaces the old file); the residual worst case is a missing file, which reads as a failed read and re-initializes visibly instead of propagating silent garbage. Comments and SDD updated to state this accurately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ineskhou
approved these changes
Jul 27, 2026
ineskhou
left a comment
ineskhou
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the merge and the docs udpates! LGTM
8 tasks
wrong-formatt
added a commit
to Open-Source-Space-Foundation/broncospace-cubesat
that referenced
this pull request
Aug 2, 2026
…t corruption (Open-Source-Space-Foundation#470)" This reverts commit c03b206.
nateinaction
added a commit
that referenced
this pull request
Aug 31, 2026
Unlike the crypto bump, this moves fatfs off the revision Zephyr 4.4.2 pins. The fixes landed in the Zephyr FatFs tree in July 2026 but did not make the 4.4.2 release and are not on v4.4-branch, so waiting for 4.4.3 would not pick them up. Our previous pin dates to October 2025 and is a pristine ChaN R0.16 with none of them applied. Three are mount-time validation, which is the reason to take this. The flight filesystem is FAT with exFAT enabled (prj.conf), and the realistic trigger here is not an attacker-supplied volume but a superblock torn by a hard reset -- the same corruption class that motivated the boot count hardening in #470. Today a malformed BPB faults; with these it is rejected at mount, and CONFIG_FS_FATFS_MOUNT_MKFS reformats and recovers. - CVE-2026-6682: BPB_FATSz32 times the FAT count overflowed a DWORD, laying the data region inside the FAT. Now capped before the multiply. - CVE-2026-6683 (exFAT): a BPB reporting zero clusters made sync_fs() divide by zero on first write or unmount. Rejected at mount instead. - CVE-2026-6687 (exFAT): f_getlabel() looped over an unvalidated on-disk count and overran the caller's buffer. Clamped to the 11-char maximum. The fourth, CVE-2026-6686, zeroes the range exposed when f_lseek() extends a file past end-of-file; that range previously read back stale flash contents. The fifth, CVE-2026-6685, upstream documents as a readability change rather than a real fix. The six commits touch only ff.c and documentation, so there is no Zephyr-side glue to adapt. Costs 280 bytes of flash (64.67% -> 64.70%) for the fill_zero helper; RAM is unchanged.
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
test_safe_09_command_loss_triggers_safe_mode_and_rebootHWIL failures on PR #460 turned out not to be test flakiness — the test was correctly detecting that boot-count persistence is unreliable across watchdog hard resets.Evidence (from the debug session on integration-uart runs 29894110572 / 29979002023)
Reconstructed from the GDS event/command/channel logs:
GET_BOOT_COUNTanswered atTB_PROC_TIME 4.492s— 80 ms before StartupManager's first 1 Hz tick performed the lazy increment (evidenced by the tick'sstartup.binsequence dispatch atTB 4.571s). PR Make mode manager test more reliable #460 now polls, closing that race.STOP_WATCHDOGhard resets,GET_BOOT_COUNTreturned215637388754419713(0x02FE191005000001) — a successful full-length 8-byte read of torn data. Theread<>helper's status/size checks rule out an unmounted filesystem or short read: this was a well-formed file containing junk, i.e. a torn flash write.215637388754419714(garbage + 1). The increment machinery works perfectly — and will carry poisoned data forever, because nothing validates the value.Contributing factor:
GET_BOOT_COUNTperformed an unconditional write-back on every query (increment=falseincluded), so every ground/test query of the boot count was a flash write that a reset could tear mid-commit.(Ruled out during the session: watchdog non-rearm —
Watchdog::m_runinitializestrue, so feeding resumes on every boot; the repeated resets in the logs were commanded by subsequent tests.)Changes (StartupManager only)
WARNING_HI BootCountCorrupted(raw)— mirrors the existing quiescence-file guard for "partial flash write leaving 0xFF bytes".GET_BOOT_COUNTis now read-only: removes the largest source of boot-count write traffic and its tear window.run_handlerretries each 1 Hz tick — the increment is delayed, not lost. The failure EVR is de-duplicated per failure streak to avoid 1 Hz spam.<path>.tmp, flush, thenOs::FileSystem::renameover the target. Correction (2026-07-26): the flight FS is FAT (ELM FatFs,CONFIG_FAT_FILESYSTEM_ELM), not littlefs — FAT renames are not guaranteed power-cut atomic. The design still closes the observed torn-in-place-write window (data fully on storage before it replaces the old file); the residual worst case during the rename window is a missing file, which reads as a failed read and re-initializes visibly rather than propagating silent garbage.Test plan
integration-uartgreen, specificallytest_safe_09(asserts boot count == initial+1 across a watchdog reset — the regression seam for all four changes)integration-uart2–3× for flake confidenceRelated: PR #460 (test-side retry), and a CI follow-up worth filing separately: the pre-test filesystem format (#446) leaves
/seq/startup.binand/seq/enter_safe.binmissing for every integration run.🤖 Generated with Claude Code