Fix millis() rollover in deadline, interval, and timestamp handling

.github/copilot-instructions.md

@@ -336,7 +336,18 @@ firmware/
 - Use `assert()` for invariants that should never fail
 - C++17 features are available (`std::optional`, structured bindings, `if constexpr`, etc.)
 - **Keep code comments minimal - one or two lines, max.** Comment only when the _why_ isn't obvious from the code; never restate what the next line does. No multi-paragraph block comments explaining straightforward changes. The diff and commit message carry the rationale; the code carries the behavior.
-- **Use `Throttle` for time-based rate limiting, not raw `millis()` math.** `src/mesh/Throttle.h` provides `Throttle::isWithinTimespanMs(lastMs, intervalMs)` (returns true while inside the cooldown) and `Throttle::execute(&lastMs, intervalMs, func)` (function-pointer form that updates the timestamp on fire). Use these for any "did N ms pass since X" check - raw `millis() > lastMs + N` is rollover-unsafe (breaks after ~49.7 days) and inconsistent with the rest of the codebase. The helpers compute `now - lastMs` with unsigned subtraction, which wraps correctly.
+- **Never compare against `millis()` directly. Use `Throttle`.** `src/mesh/Throttle.h` is the sanctioned way to ask about time, and CI enforces this (`millis-deadline-check` in `.github/workflows/test_native.yml` fails the PR on a new `millis() >` / `< millis()` comparison).
+  - `Throttle::isWithinTimespanMs(lastMs, intervalMs)` - true while still inside the cooldown.
+  - `Throttle::hasElapsed(lastMs, intervalMs)` - its complement, true once the interval has passed (inclusive `>=`). Prefer this to spelling `!isWithinTimespanMs(...)`.
+  - `Throttle::execute(&lastMs, intervalMs, func)` - function-pointer form that updates the timestamp on fire.
+  - `Throttle::deadlinePassed(deadlineMs)` - for a stored absolute deadline that cannot be re-expressed as "interval since an event". Uses an unsigned half-range compare; reads deadlines more than ~24.8 days out as already passed, which no interval in this firmware approaches (the longest is 24 h).
+  - `Throttle::deadlinePassedAt(nowMs, deadlineMs)` - the same test against a caller-supplied `now`, for a loop that snapshots the clock once and then tests many deadlines (`NextHopRouter::doRetransmissions()`). Take the snapshot from `Time::getMillis()`, not `millis()`.
+
+  Raw `millis() > deadline` or `deadline < millis()` is rollover-unsafe: the comparison inverts while the deadline sits on the far side of the 32-bit wrap, so the action fires immediately (losing its whole wait) or blocks for roughly the interval it should have waited - days, for the nRF52 flash-corruption backoff. All five helpers subtract first, so unsigned wraparound cancels out. `Throttle` reads the clock through `Time::getMillis()` (`src/UptimeClock.h`), which means every one of its ~94 call sites is time-injectable - a native test can drive `Time::setTestMillis(0xFFFFFF00)` across the wrap. For _timestamps_ (not deadlines) there is `Time::getMillisMonotonic()` / `Time::getUptimeSecs()` - a 64-bit monotonic uptime read. Readers are pure: they add their own wrap-immune elapsed time to a snapshot published by `Time::serviceMonotonic()`, which the main loop calls every iteration and which is **the only writer**. Never call `serviceMonotonic()` from anywhere else - two writers can count one wrap twice, putting every uptime and wall-clock reading ~49.7 days into the future for the rest of the boot. Not ISR-safe (the snapshot is read under a seqlock); see the contract in `UptimeClock.h`. Deadline and interval checks should still use `Throttle`, which needs no carry state at all.
+
+  **Sentinel hazard.** If a deadline variable also encodes "inactive" - `0` for `rebootAtMsec`, `shutdownAtMsec`, `alertBannerUntil`, `fixHoldEnds`; `UINT32_MAX` for `nagCycleCutoff` - test that sentinel _before_ the elapsed comparison, and match the test to the sentinel actually in use. `if (deadline && Throttle::deadlinePassed(deadline))` covers the `0` family only; `nagCycleCutoff` needs `deadline != UINT32_MAX`, or a separate armed flag as `ExternalNotificationModule` does with `isNagging`. Every sentinel value is arithmetically far in the past, so a correct comparison reads it as "expired" and fires immediately: `rebootAtMsec = -1` meaning "never" is what would have become a reboot loop. Never fold the sentinel into the helper.
+
+  **And decide which way the sentinel should fall.** "Inactive" does not always mean "suppress". At the GPS fix-hold site `fixHoldEnds == 0` means _no hold is in force_, which is exactly when a new hold must be armed - the naive comparison it replaced was `(fixHoldEnds + GPS_THREAD_INTERVAL) < millis()`, always true when nothing was armed. Guarding it with `fixHoldEnds != 0 &&` looks like this rule and inverts the site: nothing re-arms, nothing publishes, and the receiver stays powered until the search timeout. Read the surrounding logic before adding the guard. `fixHoldInForce()` in `src/gps/GPS.cpp` is the worked example - state the predicate positively, so the sentinel has an honest answer, and derive both decisions from it - with `test/test_gps_fix_hold/` pinning both directions.
 
 ### Naming Conventions
 

.github/millis-deadline-allowlist.txt

@@ -0,0 +1,22 @@
+# Allowlist for the millis-deadline-check guard in .github/workflows/test_native.yml.
+#
+# That guard rejects comparisons made directly against millis(), because they invert while the
+# deadline sits on the far side of the 32-bit wrap. Use Throttle::deadlinePassed(deadline) or
+# Throttle::hasElapsed(lastEvent, intervalMs) instead - see .github/copilot-instructions.md.
+#
+# Only add a line here when the comparison genuinely is not a deadline test. The usual valid case is
+# an *uptime threshold*: "has the device been up for at least N ms", where there is no stored
+# deadline and no event to measure from. Those still misbehave briefly after a wrap - the threshold
+# is simply re-crossed - which is harmless for boot-holdoff logic and not worth new state.
+#
+# Format: <path><TAB><exact trimmed source line, comments stripped>
+# Line numbers are deliberately absent so edits above an entry do not invalidate it. A `#` comment
+# on the code line is stripped before matching, so do not include one here.
+
+# Boot holdoff, not a deadline: suppresses a phantom shutdown from floating pins during the first
+# 30s of uptime. Pairs with the buttonPressStartTime > 30000 test on the same line.
+src/input/ButtonThread.cpp	if (millis() > 30000 && buttonPressStartTime > 30000 && _longLongPress != INPUT_BROKER_NONE &&
+
+# Boot-window check, not a deadline: draws the custom OEM logo only during the first 10s of uptime,
+# so the ordinary Meshtastic logo is used at shutdown.
+src/graphics/niche/InkHUD/Applets/System/Logo/LogoApplet.cpp	if (millis() < 10 * 1000UL) {

.github/workflows/test_native.yml

@@ -57,6 +57,72 @@ jobs:
           fi
           echo "native-suite-count matches the $expected_count suite directories."
 
+  # Reject naive deadline comparisons against the 32-bit uptime clocks. `millis() > deadline` and
+  # `deadline < millis()` invert while the deadline sits on the far side of the 32-bit wrap: the
+  # action fires immediately, or blocks for about the interval it should have waited. The correct
+  # forms are
+  # Throttle::isWithinTimespanMs / hasElapsed (elapsed since a stored event) and
+  # Throttle::deadlinePassed (an absolute deadline). See .github/copilot-instructions.md.
+  millis-deadline-check:
+    # Name is load-bearing: upstream branch protection matches the check by name. Widen the guard,
+    # not this string.
+    name: Naive millis() Deadline Compare
+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+    steps:
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
+        with:
+          persist-credentials: false
+
+      - name: Reject 32-bit uptime clocks used directly in a deadline comparison
+        shell: bash
+        run: |
+          set -euo pipefail
+          allowlist=".github/millis-deadline-allowlist.txt"
+
+          # Flag millis() or its Time::getMillis() wrapper directly adjacent to a comparison
+          # operator, in either order. The correct idioms subtract first, so they are not matched.
+          #
+          # Line comments are stripped before matching, so prose may name the broken idiom (this
+          # guard's own documentation does). Block comments are not stripped; keep `millis() >` out
+          # of /* */ blocks. mawk-compatible - ubuntu-latest has no gawk.
+          find src -type f \( -name '*.cpp' -o -name '*.h' -o -name '*.hpp' -o -name '*.ino' \) \
+               ! -path 'src/mesh/generated/*' -print0 |
+            xargs -0 awk '
+              {
+                line = $0
+                sub(/\/\/.*/, "", line)
+                if (line ~ /((millis|getMillis)\(\)[ \t]*[<>]=?)|([<>]=?[ \t]*(millis|getMillis)\(\))/) {
+                  code = line
+                  sub(/^[ \t]+/, "", code); sub(/[ \t]+$/, "", code)
+                  printf "%s\t%s\t%s\n", FILENAME, FNR, code
+                }
+              }' > /tmp/millis-hits.tsv
+
+          # Allowlisted entries are keyed on file + exact source text, deliberately without a line
+          # number, so unrelated edits above them do not invalidate the entry.
+          : > /tmp/millis-allowed.tsv
+          if [[ -f $allowlist ]]; then
+            grep -vE '^[[:space:]]*(#|$)' "$allowlist" > /tmp/millis-allowed.tsv || true
+          fi
+
+          violations=0
+          while IFS=$'\t' read -r file line code; do
+            [[ -n ${file:-} ]] || continue
+            if grep -qxF "$(printf '%s\t%s' "$file" "$code")" /tmp/millis-allowed.tsv; then
+              continue
+            fi
+            echo "$file:$line: $code"
+            violations=$((violations + 1))
+          done < /tmp/millis-hits.tsv
+
+          if [[ $violations -gt 0 ]]; then
+            echo "::error title=Naive uptime deadline compare::$violations line(s) compare a 32-bit uptime clock directly, which inverts while the deadline is on the far side of the 32-bit wrap - the action fires immediately, or blocks for about the interval it should have waited. Use Throttle::deadlinePassed(deadline) for a stored absolute deadline, or Throttle::hasElapsed(lastEvent, intervalMs) for an interval. If a match genuinely is not a deadline test (an uptime threshold, say), add it to $allowlist with a reason."
+            exit 1
+          fi
+          echo "No naive 32-bit uptime deadline comparisons in src/ (allowlist: $(wc -l < /tmp/millis-allowed.tsv) entr(y/ies))."
+
   simulator-tests:
     name: Native Simulator Tests
     runs-on: ubuntu-24.04-arm

AGENTS.md

@@ -81,7 +81,18 @@ Key rotation to never trigger casually: only the **full** factory reset (`factor
 - **Never edit or commit files under `src/mesh/generated/`.** They are regenerated from the [`meshtastic/protobufs`](https://github.com/meshtastic/protobufs) repo by the `update_protobufs.yml` workflow (entry point: `bin/regen-protos.sh`). Local edits will be overwritten and create merge conflicts. If a `.proto` change is needed, open a PR against the protobufs repo first, then let the workflow re-sync this repo.
 - **`confirm=True` on destructive MCP tools is a real gate, not a formality.** Don't bypass it via auto-approve settings.
 - **Keep code comments minimal - one or two lines, max.** Comment only when the _why_ isn't obvious from the code; never restate what the next line does. No multi-paragraph block comments explaining straightforward changes. The diff and commit message carry the rationale; the code carries the behavior.
-- **Use `Throttle` for time-based rate limiting, not raw `millis()` math.** `src/mesh/Throttle.h` provides `Throttle::isWithinTimespanMs(lastMs, intervalMs)` (returns true while inside the cooldown) and `Throttle::execute(&lastMs, intervalMs, func)` (function-pointer form that updates the timestamp on fire). Use these for any "did N ms pass since X" check - raw `millis() > lastMs + N` is rollover-unsafe (breaks after ~49.7 days) and inconsistent with the rest of the codebase. The helpers compute `now - lastMs` with unsigned subtraction, which wraps correctly.
+- **Never compare against `millis()` directly. Use `Throttle`.** `src/mesh/Throttle.h` is the sanctioned way to ask about time, and CI enforces this (`millis-deadline-check` in `.github/workflows/test_native.yml` fails the PR on a new `millis() >` / `< millis()` comparison).
+  - `Throttle::isWithinTimespanMs(lastMs, intervalMs)` - true while still inside the cooldown.
+  - `Throttle::hasElapsed(lastMs, intervalMs)` - its complement, true once the interval has passed (inclusive `>=`). Prefer this to spelling `!isWithinTimespanMs(...)`.
+  - `Throttle::execute(&lastMs, intervalMs, func)` - function-pointer form that updates the timestamp on fire.
+  - `Throttle::deadlinePassed(deadlineMs)` - for a stored absolute deadline that cannot be re-expressed as "interval since an event".
+  - `Throttle::deadlinePassedAt(nowMs, deadlineMs)` - the same test against a caller-supplied `now`, for a loop that snapshots the clock once and tests many deadlines. Snapshot from `Time::getMillis()`.
+
+  Raw `millis() > deadline` or `deadline < millis()` is rollover-unsafe: the comparison inverts while the deadline sits on the far side of the 32-bit wrap, so the action fires immediately or blocks for roughly the interval it should have waited. All five helpers subtract first, so unsigned wraparound cancels out. `Throttle` reads the clock through `Time::getMillis()` (`src/UptimeClock.h`), so all ~94 of its call sites are time-injectable and a native test can drive the wrap with `Time::setTestMillis()`.
+
+  **Sentinel hazard.** If a deadline variable also encodes "inactive" (`0` for `rebootAtMsec`, `shutdownAtMsec`, `alertBannerUntil`, `fixHoldEnds`; `UINT32_MAX` for `nagCycleCutoff`), test that sentinel _before_ the elapsed comparison - every such value is arithmetically far in the past, so a correct comparison fires on it immediately. Match the test to the sentinel in use: `if (deadline && Throttle::deadlinePassed(deadline))` covers the `0` family, `nagCycleCutoff` needs `deadline != UINT32_MAX` or a separate armed flag (`isNagging`).
+
+  Then decide which way the sentinel should fall - "inactive" does not always mean "suppress". At the GPS fix-hold site `fixHoldEnds == 0` means _no hold is in force_, which is exactly when one must be armed; guarding it with `fixHoldEnds != 0 &&` looks like this rule and inverts the site. See `fixHoldInForce()` in `src/gps/GPS.cpp` and `test/test_gps_fix_hold/`.
 
 ## Typical agent workflows
 

CLAUDE.md

@@ -11,12 +11,13 @@
 >
 > **Need this? It's here.**
 >
-> |                                             |                                                            |
-> | ------------------------------------------- | ---------------------------------------------------------- |
-> | General helpers (clamp, UTF-8, string fmt…) | `src/meshUtils.h`                                          |
-> | Logging macros (LOG_DEBUG / INFO / WARN…)   | `src/DebugConfiguration.h`                                 |
-> | New module skeleton                         | inherit `ProtobufModule<T>` in `src/mesh/ProtobufModule.h` |
-> | Observer / event wiring                     | `src/Observer.h`                                           |
+> |                                                           |                                                            |
+> | --------------------------------------------------------- | ---------------------------------------------------------- |
+> | General helpers (clamp, UTF-8, string fmt…)               | `src/meshUtils.h`                                          |
+> | Logging macros (LOG_DEBUG / INFO / WARN…)                 | `src/DebugConfiguration.h`                                 |
+> | Elapsed time / deadlines (never bare `millis()` compares) | `src/mesh/Throttle.h`                                      |
+> | New module skeleton                                       | inherit `ProtobufModule<T>` in `src/mesh/ProtobufModule.h` |
+> | Observer / event wiring                                   | `src/Observer.h`                                           |
 
 **Read `.github/copilot-instructions.md` first.** That file is the canonical agent-facing document for this repo. It covers project layout, coding conventions, the build system, CI/CD, the native C++ test suite, and the MCP Server & Hardware Test Harness. Read it top-to-bottom before starting any non-trivial change.
 

src/Power.cpp

@@ -831,12 +831,14 @@ bool Power::setup()
 
 void Power::powerCommandsCheck()
 {
-    if (rebootAtMsec && millis() > rebootAtMsec) {
+    // 0 means "not scheduled" for both, and reads as long expired - test it first.
+    // TODO(deadline-type): the plain 0-sentinel pair, and the cheapest pair to convert first.
+    if (rebootAtMsec && Throttle::deadlinePassed(rebootAtMsec)) {
         LOG_INFO("Rebooting");
         reboot();
     }
 
-    if (shutdownAtMsec && millis() > shutdownAtMsec) {
+    if (shutdownAtMsec && Throttle::deadlinePassed(shutdownAtMsec)) {
         shutdownAtMsec = 0;
         shutdown();
     }
@@ -878,7 +880,8 @@ void Power::reboot()
 #elif defined(ARCH_STM32)
     HAL_NVIC_SystemReset();
 #else
-    rebootAtMsec = -1;
+    // 0 disarms; UINT32_MAX would read as long expired and reboot-loop.
+    rebootAtMsec = 0;
     LOG_WARN("FIXME implement reboot for this platform. Note that some settings "
              "require a restart to be applied");
 #endif

src/PowerFSMThread.h

@@ -5,6 +5,7 @@
 #include "concurrency/OSThread.h"
 #include "configuration.h"
 #include "main.h"
+#include "mesh/Throttle.h"
 
 namespace concurrency
 {
@@ -29,9 +30,9 @@ class PowerFSMThread : public OSThread
         if (powerStatus->getHasUSB()) {
             timeLastPowered = millis();
         } else if (config.power.on_battery_shutdown_after_secs > 0 && config.power.on_battery_shutdown_after_secs != UINT32_MAX &&
-                   millis() > (timeLastPowered +
-                               Default::getConfiguredOrDefaultMs(
-                                   config.power.on_battery_shutdown_after_secs))) { // shutdown after 30 minutes unpowered
+                   Throttle::hasElapsed(
+                       timeLastPowered,
+                       Default::getConfiguredOrDefaultMs(config.power.on_battery_shutdown_after_secs))) { // unpowered too long
             powerFSM.trigger(EVENT_SHUTDOWN);
         }