Skip to content

Commit e3e956b

Browse files
committed
Correct the described failure window of a naive millis() compare
The comments and agent docs said a bare `millis() > deadline` "breaks for ~24 days after the wrap". That figure belongs to the fix, not the bug: it is the half-range limit of deadlinePassed(), which reads deadlines more than 2^31 ms ahead as already passed, and the range over which a UINT32_MAX sentinel reads as passed. The naive compare's actual failure is an inversion lasting only while the deadline sits on the far side of the wrap, so it is bounded by the interval: the action fires immediately and loses its wait, or blocks for about the wait it should have performed - days for the nRF52 flash-corruption backoff, one skipped cycle for a seconds-long retransmit timer. Comments and docs only; the ~24.8 day statements that correctly describe deadlinePassed()'s own range are left as they were. clod helped out here
1 parent 513a059 commit e3e956b

5 files changed

Lines changed: 10 additions & 9 deletions

File tree

.github/copilot-instructions.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -343,7 +343,7 @@ firmware/
343343
- `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).
344344
- `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()`.
345345

346-
Raw `millis() > deadline` or `deadline < millis()` is rollover-unsafe: for ~24 days after the 32-bit wrap it either stalls the action or fires it immediately. 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. There is deliberately no 64-bit millis; see the note in `UptimeClock.h`.
346+
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. There is deliberately no 64-bit millis; see the note in `UptimeClock.h`.
347347

348348
**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 reads it as "expired" and fires immediately: `rebootAtMsec = -1` meaning "never" is what would have become a reboot loop. Write `if (deadline && Throttle::deadlinePassed(deadline))`, and never fold the sentinel into the helper.
349349

.github/millis-deadline-allowlist.txt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
# Allowlist for the millis-deadline-check guard in .github/workflows/test_native.yml.
22
#
3-
# That guard rejects comparisons made directly against millis(), because they break for ~24 days
4-
# after the 32-bit wrap. Use Throttle::deadlinePassed(deadline) or
3+
# That guard rejects comparisons made directly against millis(), because they invert while the
4+
# deadline sits on the far side of the 32-bit wrap. Use Throttle::deadlinePassed(deadline) or
55
# Throttle::hasElapsed(lastEvent, intervalMs) instead - see .github/copilot-instructions.md.
66
#
77
# Only add a line here when the comparison genuinely is not a deadline test. The usual valid case is

.github/workflows/test_native.yml

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,9 @@ jobs:
5555
echo "native-suite-count matches the $expected_count suite directories."
5656
5757
# Reject naive deadline comparisons against millis(). `millis() > deadline` and
58-
# `deadline < millis()` are wrong for ~24 days after the 32-bit millis() wrap: depending on which
59-
# side wrapped they either stall the action or fire it immediately. The correct forms are
58+
# `deadline < millis()` invert while the deadline sits on the far side of the 32-bit wrap: the
59+
# action fires immediately, or blocks for about the interval it should have waited. The correct
60+
# forms are
6061
# Throttle::isWithinTimespanMs / hasElapsed (elapsed since a stored event) and
6162
# Throttle::deadlinePassed (an absolute deadline). See .github/copilot-instructions.md.
6263
millis-deadline-check:
@@ -113,7 +114,7 @@ jobs:
113114
done < /tmp/millis-hits.tsv
114115
115116
if [[ $violations -gt 0 ]]; then
116-
echo "::error title=Naive millis() deadline compare::$violations line(s) compare against millis() directly, which breaks for ~24 days after the 32-bit wrap - the action either stalls or fires immediately. 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."
117+
echo "::error title=Naive millis() deadline compare::$violations line(s) compare against millis() 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."
117118
exit 1
118119
fi
119120
echo "No naive millis() deadline comparisons in src/ (allowlist: $(wc -l < /tmp/millis-allowed.tsv) entr(y/ies))."

AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ Key rotation to never trigger casually: only the **full** factory reset (`factor
8888
- `Throttle::deadlinePassed(deadlineMs)` - for a stored absolute deadline that cannot be re-expressed as "interval since an event".
8989
- `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()`.
9090

91-
Raw `millis() > deadline` or `deadline < millis()` is rollover-unsafe: for ~24 days after the 32-bit wrap it either stalls the action or fires it immediately. 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()`.
91+
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()`.
9292

9393
**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. Write `if (deadline && Throttle::deadlinePassed(deadline))`.
9494

src/mesh/Throttle.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,8 @@ class Throttle
1818
}
1919

2020
/// True once an absolute deadline has arrived. Use this rather than comparing against millis()
21-
/// directly, which breaks for ~24 days after the 32-bit wrap - either stalling the action or
22-
/// firing it immediately, depending on which side wrapped.
21+
/// directly: that inverts while the deadline sits on the far side of the 32-bit wrap, so the
22+
/// action either fires immediately or blocks for about the interval it should have waited.
2323
///
2424
/// Use this when the site stores a deadline; use hasElapsed() when it stores the time of the
2525
/// last event, which allows the full ~49.7 day range instead of ~24.8 days ahead.

0 commit comments

Comments
 (0)