Skip to content

fix: prevent duplicate packets in API for multi-source deployments (#3051)#3052

Merged
Yeraze merged 1 commit into
mainfrom
claude/great-dijkstra-yYpws
May 17, 2026
Merged

fix: prevent duplicate packets in API for multi-source deployments (#3051)#3052
Yeraze merged 1 commit into
mainfrom
claude/great-dijkstra-yYpws

Conversation

@Yeraze
Copy link
Copy Markdown
Owner

@Yeraze Yeraze commented May 17, 2026

Summary

  • Fixes duplicate rows returned by /api/v1/packets (and the legacy /api/packets) when the same nodeNum exists in multiple sources.
  • Root cause: getPacketLogs and getPacketLogById joined the nodes table on nodeNum alone, but nodes has a composite PK (nodeNum, sourceId) since migration 029 — one packet row was multiplied by the number of sources that knew the node.
  • Fix: add AND pl.sourceId = from_nodes.sourceId (and to_nodes) to both LEFT JOINs, matching the per-source scoping pattern already used by getPacketCountsByNode.
  • Updated existing tests to use the production composite-PK schema (they were using an old single-source schema without sourceId).
  • Added a dedicated regression suite (MiscRepository - getPacketLogs / getPacketLogById multi-source dedup (#3051)) with three tests that would have caught this.

Closes #3051

Test plan

  • Full Vitest suite passes (292 test files, 0 failures — verified locally before PR)
  • /api/v1/packets returns exactly one row per physical packet in a multi-source deployment
  • Node longName in API response comes from the correct source (the one that received the packet)
  • Single-source deployments unaffected

https://claude.ai/code/session_01BPPK2EUX6W7VwuTEPMYf6G


Generated by Claude Code

…ti-source deployments (#3051)

The LEFT JOINs on the nodes table were keyed only on nodeNum, but nodes
uses a composite PK (nodeNum, sourceId) since migration 029.  When the
same nodeNum existed in N sources the JOIN multiplied each packet_log row
by N, producing 100%-identical duplicate entries in /api/v1/packets
responses.  The Packet Monitor UI masked the bug by deduplicating by id
client-side; direct API consumers received inflated results.

Fix: add `AND pl.sourceId = from_nodes.sourceId` (and `to_nodes.sourceId`)
to both JOINs in getPacketLogs and getPacketLogById, mirroring the
per-source scoping already used by getPacketCountsByNode.

Update existing tests to use the production composite-PK schema and add
a dedicated regression suite that verifies no duplicate rows are returned
when the same nodeNum is present in multiple sources.

https://claude.ai/code/session_01BPPK2EUX6W7VwuTEPMYf6G
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 17, 2026

Claude finished @Yeraze's task —— View job


PR Review: Fix duplicate packets in multi-source deployments

Todo List

  • Read and analyze changed files
  • Review database query changes for correctness
  • Evaluate test coverage and regression tests
  • Check for performance and security considerations
  • Provide comprehensive feedback

Code Quality Review

This is an excellent fix for a significant bug in multi-source deployments. The changes are well-implemented and thoroughly tested.

Strengths

1. Correct Root Cause Analysis

  • Properly identified that getPacketLogs and getPacketLogById were joining on nodeNum alone
  • Understood that the composite primary key (nodeNum, sourceId) introduced in migration 029 created the multiplication issue
  • Applied the same pattern already used successfully in getPacketCountsByNode

2. Precise Fix Implementation (src/db/repositories/misc.ts:1043-1044, 1070-1071)

LEFT JOIN nodes from_nodes ON pl.from_node = from_nodes.${nodeNum} AND pl.${sourceIdCol} = from_nodes.${sourceIdCol}
LEFT JOIN nodes to_nodes ON pl.to_node = to_nodes.${nodeNum} AND pl.${sourceIdCol} = to_nodes.${sourceIdCol}
  • Minimal, surgical change that adds the missing sourceId constraint
  • Maintains consistency with existing patterns in the codebase
  • Preserves all existing functionality while fixing the bug

3. Comprehensive Test Coverage

  • Updated existing tests to use production-accurate schema with composite PK
  • Added dedicated regression test suite with 3 specific test cases
  • Tests verify both deduplication and correct source resolution
  • Would have caught this bug during development

🔍 Code Quality Assessment

Security: ✅ No security concerns

  • No SQL injection risks - uses proper Drizzle ORM parameter binding
  • No sensitive data exposure

Performance: ✅ Neutral to positive impact

  • Fix eliminates unnecessary duplicate row processing
  • JOIN conditions are properly indexed (nodeNum, sourceId composite PK)
  • Query complexity unchanged, just more precise

Maintainability: ✅ Excellent

  • Uses established patterns from getPacketCountsByNode
  • Clear, descriptive variable names (sourceIdCol, longName, etc.)
  • Comprehensive test coverage prevents regressions

🧪 Test Quality

The regression test suite (misc.packetlog.test.ts:366-469) is particularly well-designed:

  1. Realistic test data: Uses production-like multi-source scenario
  2. Specific assertions: Verifies exact row counts and correct source resolution
  3. Comprehensive coverage: Tests both getPacketLogs and getPacketLogById
  4. Clear documentation: Test names and comments explain the bug scenario

⚠️ Minor Considerations

1. Consider Backfill Validation
Since this affects existing multi-source deployments, consider documenting or testing scenarios where historical data might have been affected by the duplicate API responses.

2. API Response Consistency
The fix ensures /api/v1/packets returns correct data, but verify that any cached or paginated responses are also cleared/updated appropriately.

Overall Assessment

This is a high-quality fix that:

  • ✅ Correctly identifies and fixes the root cause
  • ✅ Uses minimal, surgical changes
  • ✅ Includes comprehensive test coverage
  • ✅ Follows established codebase patterns
  • ✅ Has no negative performance or security impact

The bug was subtle but significant - causing API consumers to see inflated packet counts in multi-source deployments. The fix is precise and well-tested.

Recommendation: Approve - This PR is ready for merge.


| Branch: claude/great-dijkstra-yYpws

@github-actions
Copy link
Copy Markdown

System Test Results

MeshMonitor System Test Results

Test Run: 2026-05-17 10:01:07 EDT

Test Summary

Test Suite Result
Configuration Import ✅ PASSED
Quick Start Test ✅ PASSED
Security Test ✅ PASSED
V1 API Test ✅ PASSED
Reverse Proxy Test ✅ PASSED
Reverse Proxy + OIDC ✅ PASSED
Virtual Node CLI Test ✅ PASSED
Backup & Restore Test ✅ PASSED
Database Migration Test ✅ PASSED
DB Backing Consistency ✅ PASSED
API Exercise (3 DBs) ✅ PASSED

✅ Overall Result: PASSED

All deployment configurations are working correctly!

Test Details

Configuration Import:

  • Tests configuration import and device reboot cycle
  • Verifies channel roles, PSKs, and LoRa configuration
  • Note: Channel name verification skipped due to architectural limitation

Quick Start Test:

  • Zero-config deployment (no SESSION_SECRET or COOKIE_SECURE required)
  • HTTP access without HSTS
  • Auto-generated admin user with default credentials
  • Session cookies work over HTTP
  • Meshtastic node connection and message exchange verified

Security Test:

  • Verifies Node IP address hidden from anonymous users in API responses
  • Verifies MQTT configuration hidden from anonymous users
  • Verifies Node IP address visible to authenticated users
  • Verifies MQTT configuration visible to authenticated users
  • Verifies protected endpoints require authentication

V1 API Test:

  • Tests v1 REST API endpoints with Bearer token authentication
  • Verifies Bearer token requests bypass CSRF protection
  • Verifies POST/PUT/DELETE work without CSRF token when using Bearer auth
  • Verifies session-based requests still require CSRF token

Reverse Proxy Test:

  • Production deployment with COOKIE_SECURE=true
  • HTTPS-ready configuration
  • Trust proxy enabled for reverse proxy compatibility
  • CORS configured for HTTPS domain
  • Meshtastic node connection and message exchange verified

Reverse Proxy + OIDC Test:

  • OIDC authentication integration
  • Mock OIDC provider health checks
  • Authorization flow and session creation
  • Hybrid mode (OIDC + local auth)
  • Meshtastic node connection verified

Virtual Node CLI Test:

  • Virtual Node Server enabled on TCP port 4404
  • Meshtastic Python client successfully connects
  • Node data download and synchronization verified
  • Test message sent on gauntlet channel (index 3)
  • Message delivery confirmed via Web UI API
  • Virtual Node Server connection logging verified

Backup & Restore Test:

  • System backup created from running dev container
  • New container spun up with RESTORE_FROM_BACKUP env var
  • Data integrity verified (node count, message count, settings)
  • Restore event logged in audit log
  • Dev container unaffected by restore test

Database Migration Test:

  • SQLite to PostgreSQL migration verified
  • SQLite to MySQL migration verified
  • Data integrity confirmed for both target databases
  • Row counts match between source and target

DB Backing Consistency Test:

  • SQLite, PostgreSQL, and MySQL backends tested with same device
  • Node counts within ±10 across all three backends
  • Favorite counts identical across all backends
  • Key station verified as favorite on all backends

@Yeraze Yeraze merged commit ee199ea into main May 17, 2026
20 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.

[BUG] API: Dupe packets

2 participants