Files
MeshCore-mqtt-observer/UPSTREAM_BUGS.md
agessaman 5828a3c563 fix(mqtt-neighbors): report unusable heard ages as null
A neighbour heard before the clock is set carries the firmware's unset-clock
default (1715770351, 15 May 2024). Subtracting that from an NTP-synced clock
published ages of ~806 days for neighbours that had just answered a live scope
query, and a backwards clock step reported 0, i.e. "heard just now".

- finishNeighborDiscover() reports the age as unknown ("heard_secs_ago": null)
  when the stored stamp and the current clock come from different epochs, or
  when the clock has stepped backwards
- handleNeighborDiscoverResponse() re-stamps heard_timestamp on a zero-hop scope
  reply, in both the snapshot and the live table, so entries heal once per
  discovery cycle instead of waiting for the neighbour's next advert
- publish ordering places usable ages ahead of unknown ones so a poisoned stamp
  cannot displace fresh entries when the JSON buffer truncates
- document the null case, and the always-present total_neighbors /
  queried_neighbors / truncated fields the payload sample omitted
- add UPSTREAM_BUGS.md, tracking the monotonic-uptime fix to propose upstream
  plus the unclamped subtraction in the companion and CLI readouts

Containment only: upstream still stamps neighbours from the wall clock at
packet-reception time, which on a cold boot always precedes NTP.
2026-07-31 08:54:39 -07:00

5.4 KiB
Raw Permalink Blame History

Upstream Bug Candidates

Bugs in code this fork inherits from upstream (meshcore-dev/dev), each with the fix we would propose upstream. Any fork-local workaround is recorded so it can be dropped once upstream lands a real fix.

Status: open (not reported) · reported (issue/PR filed) · landed (upstream fixed, fork workaround removable).


1. Neighbour "heard" ages are derived from a steppable wall clock

Status: open Files: examples/simple_repeater/MyMesh.cpp, examples/simple_room_server/MyMesh.cpp Impact: wrong ages reported to companion apps, CLI, and (in this fork) MQTT. No crash.

Symptom

A repeater published a neighbour age of 69681995 seconds — 806 days — for two of three neighbours that had just answered a live query (capture abridged; see MQTT_IMPLEMENTATION.md for the full payload shape):

{"timestamp":"2026-07-30T22:59:09.000000+00:00","total_neighbors":3,
 "neighbors":[
   {"pubkey":"0CE5EA7C…","snr":12.5,"heard_secs_ago":57,"status":"responded"},
   {"pubkey":"B0D17C59…","snr":12.5,"heard_secs_ago":69681995,"status":"responded"},
   {"pubkey":"D19BEE16…","snr":12.25,"heard_secs_ago":69681995,"status":"responded"}]}

Root cause

heard_secs_ago is a relative quantity computed by subtracting two samples of an absolute wall clock that can step by years mid-session.

putNeighbour() stamps heard_timestamp = getRTCClock()->getCurrentTime() when a zero-hop advert or node-discover reply arrives (MyMesh.cpp:93). On a cold boot the radio is receiving within a second or two, while the clock is still at the firmware's unset default — 1715770351, 15 May 2024 (src/helpers/ArduinoHelpers.h, src/helpers/ESP32Board.h). NTP (or an admin over the CLI) then steps the clock forward by years, and every later reader subtracts the two:

publish epoch          1785452349   (2026-07-30T22:59:09Z)
 reported age           69681995
= stored heard_timestamp 1715770354   = unset-clock default + 3 s of uptime

So both stale entries were stamped 3 seconds after boot. The entries do not self-heal: putNeighbour() is the only writer of heard_timestamp, so an entry stays wrong until that specific neighbour is physically heard again. In the capture above the two stale neighbours missed the node-discover refresh (responder-side rate limit discover_limiter, a reply collision, or firmware without CTL_TYPE_NODE_DISCOVER_RESP) yet still answered a direct query — hence "status":"responded" next to a 2-year age.

Deferring the read until after time sync does not fix this. The bad value is written at packet-reception time, which on any cold boot precedes WiFi and NTP.

Affected readers

Path Location
Companion REQ_TYPE_GET_NEIGHBOURS binary reply MyMesh.cpp:404
CLI neighbors text output MyMesh.cpp:1350
MQTT neighbors JSON (fork-only) MyMesh.cpp:2036

Proposed upstream fix

Stamp neighbours from a monotonic source and derive every age from that:

  • add uint32_t heard_millis to NeighbourInfo, set from millis() in putNeighbour() alongside the existing wall-clock stamp;
  • compute every "seconds ago" readout as (now_millis - heard_millis) / 1000 using the unsigned-subtraction idiom already used by millisHasNowPassed() / futureMillis(), which is immune to clock steps and stays correct across one 32-bit rollover;
  • clamp the result to a documented maximum so an entry untouched for longer than the 49.7-day millis() period cannot alias back to a small age;
  • keep heard_timestamp for anything that needs an absolute instant — sort order is equivalent under either field.

Cost: 4 bytes × MAX_NEIGHBOURS of RAM.

Fork-local containment (in place)

Not a substitute for the above; it stops the fork from publishing a fabricated number while upstream still stamps from the wall clock.

  • finishNeighborDiscover() reports the age as unknown — "heard_secs_ago":null — when the stored stamp predates MQTTConnectionPolicy::kSyncedClockEpoch (2025-01-01, above the unset-clock default) while the current clock is past it, or when the clock has stepped backwards. Previously a backwards clock reported 0, i.e. "heard just now".
  • handleNeighborDiscoverResponse() re-stamps heard_timestamp on a zero-hop scope reply, in both the snapshot and the live table. The reply is proof of reception, so entries now heal once per discovery cycle instead of waiting for the neighbour's next advert. This would have made 2 of the 3 entries above correct.
  • Publish ordering places known ages before unknown ones, so an unusable stamp cannot masquerade as the freshest entry when the JSON buffer truncates.

2. Neighbour age subtraction is unclamped in the companion and CLI readouts

Status: open Files: examples/simple_repeater/MyMesh.cpp:404, :1350 (and the room-server twins) Impact: age underflows to ~4.29e9 instead of a sane value.

Both readers subtract without checking ordering:

uint32_t heard_seconds_ago = getRTCClock()->getCurrentTime() - neighbour->heard_timestamp;

Any state where heard_timestamp > now — clock stepped backwards by NTP or an admin, or a stamp taken before a backwards step — wraps the unsigned result to near UINT32_MAX. Distinct from #1 in that it is a missing guard rather than the choice of clock: worth fixing on its own even before the monotonic change, by clamping to 0 (or reporting "unknown", which is what the age actually is).