Skip to content

fix: prevent duplicate traceroute pending records (#2364)#2380

Merged
Yeraze merged 1 commit intomainfrom
fix/traceroute-duplicate-pending
Mar 22, 2026
Merged

fix: prevent duplicate traceroute pending records (#2364)#2380
Yeraze merged 1 commit intomainfrom
fix/traceroute-duplicate-pending

Conversation

@Yeraze
Copy link
Owner

@Yeraze Yeraze commented Mar 22, 2026

Summary

Fixes the remaining part of #2364 — traceroute history showed two entries per request, with the second being an empty "No response received" that overwrote the correct entry in the UI.

Root Cause

When MeshMonitor sends a traceroute:

  1. sendTraceroute() records a pending traceroute in the database ✓
  2. The physical node echoes the outgoing packet back
  3. processMeshPacket() catches the echo (undecodable TRACEROUTE_APP packet) and records it again as a second pending entry
  4. The response matches one entry, but the other stays as permanent "No response received"
  5. The UI shows the latest entry (the empty one), hiding the correct result

Fix

Skip recording in the echo path when the packet originates from our own node (fromNum === localNodeNum), since sendTraceroute() already recorded it. Virtual node client traceroutes (different from node) are still recorded correctly.

Files Changed

File Change
src/server/meshtasticManager.ts Skip duplicate recording for own-node traceroute echoes

Test plan

  • 3061 tests pass, 0 failures

🤖 Generated with Claude Code

When MeshMonitor sends a traceroute via sendTraceroute(), the request
is recorded in the database. When the physical node echoes back the
outgoing packet, the processMeshPacket else-if branch recorded it
again as a second pending entry. The response only matches one,
leaving the other as a permanent "No response received" entry that
overwrites the correct one in the UI.

Fix: skip recording in the echo path when the packet is from our own
node, since sendTraceroute() already recorded it.

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 ✅

I've thoroughly analyzed this pull request fixing duplicate traceroute pending records. Here's my comprehensive review:

Review Checklist:

  • Read and analyze the changed code
  • Assess code quality and best practices
  • Check for potential bugs or edge cases
  • Evaluate performance considerations
  • Review security implications
  • Assess test coverage adequacy
  • Provide final recommendations

🎯 Code Quality & Best Practices

✅ Excellent Implementation

  • Clear Logic: The fix correctly identifies that echo packets from our own node should not be recorded twice
  • Proper Guard Condition: if (fromNum !== localNodeNum) is the right approach to prevent duplicates
  • Good Context Preservation: Comment explains the purpose: "Skip if from our own node — sendTraceroute() already recorded the request"
  • Consistent Style: Follows existing code patterns and naming conventions

🐛 Bug Analysis

✅ Root Cause Correctly Identified
The fix addresses the exact issue described:

  1. sendTraceroute() records the request at src/server/meshtasticManager.ts:6410
  2. Physical node echo triggers processMeshPacket() else-if branch at src/server/meshtasticManager.ts:3823
  3. Original code recorded it again, causing duplicate entries
  4. Fix prevents the duplicate by checking fromNum !== localNodeNum

✅ Edge Cases Handled

  • Virtual node traceroutes (different fromNum) are preserved correctly
  • null/undefined localNodeNum handled gracefully (comparison will be false, maintaining safety)

🚀 Performance Considerations

✅ Minimal Performance Impact

  • Simple numeric comparison adds negligible overhead
  • Prevents unnecessary database writes for echo packets
  • Actually improves performance by reducing duplicate database operations

🔒 Security Implications

✅ No Security Concerns

  • Change is purely defensive (preventing duplicates)
  • No exposure of sensitive data
  • No changes to authentication/authorization logic
  • Maintains proper node identity validation

📋 Test Coverage Assessment

✅ Adequate Testing Coverage

  • All 3061 tests pass with 0 failures
  • Existing test infrastructure covers the traceroute functionality
  • Related test files include:
    • meshtasticManager.tracerouteScheduler.test.ts
    • server.test.ts (traceroute API endpoint)
    • meshtasticManager.node-identity-guards.test.ts (similar echo filtering patterns)

📝 Note: While no specific test was added for this exact edge case, the existing test suite validates the core functionality and the fix is small/low-risk.

🎖️ Final Recommendation: APPROVE

This is a well-implemented, targeted fix that:

  • ✅ Correctly solves the duplicate traceroute issue
  • ✅ Uses appropriate guard conditions
  • ✅ Maintains backward compatibility
  • ✅ Follows established code patterns
  • ✅ Has minimal risk and clear benefits
  • ✅ Passes all existing tests

The implementation is solid and ready for merge. The fix elegantly prevents echo packet duplication without affecting legitimate virtual node traceroute functionality.


@Yeraze Yeraze merged commit 1cad71a into main Mar 22, 2026
14 checks passed
@Yeraze Yeraze mentioned this pull request Mar 24, 2026
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