Claude finished @Yeraze's task —— View job

PR Review: fix(nodes): guard lastHeard against replayed/stale packets

Tasks:

• Gather context and understand the PR
• Read changed files
• Analyze implementation for code quality, bugs, performance, security
• Check test coverage
• Post review feedback

Overall this is a well-scoped, conservative fix for a real field-reported issue. The helper is cleanly isolated, the documentation is excellent, and the "fail open" (stamp now when rxTime is absent/boot-relative) design avoids the worst false-positive class. A few things worth addressing:

Minor Bug — Mixed float/floor in resolveLastHeardSec

src/server/utils/replayGuard.ts:73-74:

const nowSec = Math.floor(nowMs / 1000);
return isStaleReplayRxTime(rxTimeSec, nowMs / 1000) ? undefined : nowSec;

nowSec is floored to an integer, but isStaleReplayRxTime is called with nowMs / 1000 (a float, e.g. 1781913600.123). This is a sub-second inconsistency and won't affect correctness in practice (the threshold is 6 hours), but the two expressions should use the same value for consistency and to avoid confusion. The fix is trivial:

const nowSec = Math.floor(nowMs / 1000);
return isStaleReplayRxTime(rxTimeSec, nowSec) ? undefined : nowSec;

Design Note — Text messages from replayed packets are still stored

In mqttIngestion.ts, the TEXT_MESSAGE_APP case (around line 341) inserts the message into the DB regardless of whether it's a stale replay — only the lastHeard stamp on the sender node is suppressed. This is likely intentional (you don't want to lose a message that might have been legitimately delayed), but it's worth a comment confirming this, since the PR description focuses entirely on lastHeard and the "don't resurrect a dead node" goal. The same applies to TELEMETRY_APP — data is still stored even for stale replays.

Missing integration test coverage for most packet types

mqttIngestion.test.ts only tests the replay guard for TELEMETRY_APP. The helpers ingestTraceroute, ingestNeighborInfo, ingestPaxcounter, and ingestStoreForward all call resolveLastHeardSec independently and are not covered by the new replay-guard describe block. Given that ingestTraceroute and ingestStoreForward call resolveLastHeardSec from packet.rxTime directly (rather than using the pre-computed lastHeardSec from the envelope level), a test verifying those paths also suppress lastHeard correctly would add confidence. This is non-blocking but worth noting.

Inconsistent guard call-site pattern (ingestTraceroute, ingestPaxcounter, ingestStoreForward)

The top-level ingestServiceEnvelope computes lastHeardSec once and passes it down to ingestNeighborInfo (good). But ingestTraceroute (mqttIngestion.ts:460), ingestPaxcounter (line 773), and ingestStoreForward (line 866) each call resolveLastHeardSec(packet.rxTime, nowMs) again internally. This is redundant but not wrong — it produces the same result since packet and nowMs are the same object references. Passing the pre-computed lastHeardSec as a parameter (as ingestNeighborInfo already does) would be more consistent. Not blocking.

Threshold / floor choice — looks reasonable

The 6-hour threshold is defensible: it comfortably covers multi-day MQTT broker retention scenarios while being generous to legitimate delivery delays and clock skew. The 2020 floor (MIN_PLAUSIBLE_UNIX_SEC = 1_600_000_000) correctly catches boot-relative timestamps (uptime counters that look like small integers).

One edge case not explicitly tested: if rxTimeSec is a very large future timestamp (e.g. year 2100 from a misconfigured clock), nowSec - rxTimeSec will be deeply negative, which correctly returns false from isStaleReplayRxTime. This is covered implicitly by the "never flags a future-dated rx_time" test but is worth keeping in mind.

Security / correctness

No security concerns. The guard is purely defensive — it suppresses a lastHeard update; it doesn't drop packets, alter stored telemetry values, or affect authentication paths.

Summary

The float/floor inconsistency in replayGuard.ts:74 is the only item I'd call out for a fix before merge — everything else is polish.