Skip to content

fix: packet monitor uses server time for consistent ordering#2388

Merged
Yeraze merged 1 commit intomainfrom
fix/traceroute-packet-monitor-timestamps
Mar 22, 2026
Merged

fix: packet monitor uses server time for consistent ordering#2388
Yeraze merged 1 commit intomainfrom
fix/traceroute-packet-monitor-timestamps

Conversation

@Yeraze
Copy link
Owner

@Yeraze Yeraze commented Mar 22, 2026

Summary

  • Incoming packets in the packet monitor used meshPacket.rxTime (device clock) for timestamps, while outgoing packets used Date.now() (server clock)
  • When the connected device's clock drifts even slightly from the server, this causes responses to appear before their requests in the packet monitor
  • Now both incoming and outgoing packets use server time consistently
  • The original rxTime is still preserved in packet metadata (rx_time) for debugging

Addresses timing comments in #2364

Test Plan

  • npx vitest run — 3070 tests pass, 0 failures
  • npm run build — no TypeScript errors

🤖 Generated with Claude Code

Incoming packets used meshPacket.rxTime (device clock) for the packet
monitor timestamp, while outgoing packets used Date.now() (server clock).
When the device clock drifts even slightly, this causes traceroute
responses to appear before their requests in the packet monitor.

Now both incoming and outgoing packets use server time consistently.
The original rxTime is still preserved in packet metadata for debugging.

Fixes timing comments in #2364

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link

github-actions bot commented Mar 22, 2026

Claude finished @Yeraze's task —— View job


PR Review Analysis

Completed analysis of the timestamp consistency fix. This is a well-targeted improvement that addresses a real timing issue in the packet monitor.

Todo List:

  • Read and analyze the changed file
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Review test coverage adequacy

Review Findings

Code Quality & Best Practices

  • Clean, focused change: Single-line fix with clear intent and good commenting
  • Consistent with existing patterns: Uses the same Math.floor(Date.now() / 1000) pattern already used for outgoing packets at src/server/meshtasticManager.ts:3627
  • Good commenting: The inline comment explains both the "why" and notes that rxTime is preserved in metadata
  • Preserves data integrity: Original rxTime is still accessible in metadata.rx_time for debugging purposes

Bug Analysis

  • Addresses real timing issue: Clock drift between device and server can cause responses to appear before requests in the packet monitor
  • No regression risk: Change is conservative and maintains backward compatibility
  • Consistent behavior: Both incoming and outgoing packets now use the same time source

Performance Considerations

  • Minimal performance impact: Date.now() is already being called; just removing conditional logic
  • Slightly better performance: Eliminates conditional check on meshPacket.rxTime

Security Concerns

  • No security implications: Time source change doesn't introduce any security risks
  • Data preservation: Original timing data remains available for forensic analysis if needed

Test Coverage

  • Tests passing: All 3070 tests pass with 0 failures
  • Build clean: No TypeScript compilation errors
  • Adequate coverage: This change affects data logging behavior that's already covered by existing packet logging tests

Summary

This is an excellent fix that:

  1. Solves a real problem: Eliminates timing inconsistencies in packet monitor display
  2. Uses minimal code change: Single line modification with clear intent
  3. Maintains data integrity: Preserves original timing data for debugging
  4. Follows existing patterns: Consistent with outgoing packet timestamp handling
  5. Has comprehensive testing: All tests pass confirming no regressions

Recommendation: ✅ APPROVE - This is a well-implemented fix that addresses the timing issue described in #2364 without any apparent downsides.

@Yeraze Yeraze merged commit fb56617 into main Mar 22, 2026
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant