fix: accept new nodeNum on reboot and fix isLocked desync

[Yeraze]

Summary

• Accept new nodeNum on same-device reboot instead of rejecting it. The old approach (from PR fix: prevent ghost duplicate node on reboot-induced nodeNum change #2091) caused identity mismatches because the firmware was already broadcasting on the new nodeNum while MeshMonitor kept using the old one — resulting in conflicting NodeInfo packets, wrong VN client identities, and mismatched admin packets.
• Fix isLocked desync where node names changed outside MeshMonitor (e.g., via the Meshtastic app or after factory reset + reconfiguration) were never picked up. The isLocked flag on localNodeInfo was blocking updates from processNodeInfoProtobuf — the authoritative source for node identity.

Changes

src/server/meshtasticManager.ts

1. processMyNodeInfo same-device-reboot branch (~line 2876): Rewritten from "reject new nodeNum" to "accept + merge". Fetches old node data, merges metadata into the new node entry (new node's data takes priority), deletes the old ghost, updates settings, clears initConfigCache, and schedules a deferred sendRemoveNode to clean up the old nodeNum from the physical device's NodeDB.

2. processNodeInfoProtobuf (~line 5608): Removed the !isLocked guard so NodeInfo always updates localNodeInfo for the local node. NodeInfo is the device's source of truth for names — the lock should only prevent processMyNodeInfo (which doesn't carry names) from overwriting with incomplete data. Adds an info log when names change.

3. refreshNodeDatabase (~line 11737): Clears isLocked before sending want_config_id so processMyNodeInfo can also run during manual refresh to update hwModel, rebootCount, etc.

src/server/meshtasticManager.test.ts

• Updated simulateProcessMyNodeInfo helper: new merged_and_accepted action replaces rejected_new_nodenum
• Test 1 renamed and rewritten: verifies merge behavior (new nodeNum accepted, old ghost deleted, metadata merged)
• Test 8 rewritten: verifies no duplicate nodes after merge (one node with merged data)
• 4 new tests for NodeInfo name update behavior: updates when locked, updates on initial config, skips different node, skips missing user data

Test plan

• npx vitest run — 2771 tests passed (4 new)
• Docker build succeeds
• Dev container deployed and verified new code is running
• tests/system-tests.sh — run before merge
• Manual: Connect device, simulate reboot with new nodeNum, verify merge
• Manual: Change node name via Meshtastic app, click Reload Config, verify name updates

🤖 Generated with Claude Code

[github-actions]

Claude finished @Yeraze's task —— View job

---

Code Review Complete ✅

I've thoroughly analyzed the PR changes. Here's my comprehensive feedback:

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

Overall Assessment

This is a well-implemented fix that addresses real protocol synchronization issues. The solution properly handles Meshtastic firmware behavior and maintains data integrity.

---

🟢 Strengths

1. Sound Architecture Decision

• Correctly identifies that NodeInfo packets are the authoritative source for node identity
• Proper separation of concerns: isLocked prevents incomplete data overwrites but allows authoritative updates
• Aligns with Meshtastic protocol where firmware broadcasts on new nodeNum immediately after reboot

2. Robust Merge Strategy (meshtasticManager.ts:2876-2944)

• Comprehensive metadata merging with sensible fallback priority (new → old → default)
• Proper cleanup of ghost nodes and cascaded data
• Transaction-like approach: fetch → merge → delete → update settings

3. Excellent Test Coverage

• 4 new focused tests covering edge cases
• Helper function simulateNodeInfoUpdate makes tests readable and maintainable
• Tests verify both positive and negative scenarios

---

🟡 Areas for Improvement

1. Error Handling in Async Operations

// Current (line 2935-2942)
setTimeout(async () => {
  try {
    await this.sendRemoveNode(prevNumToRemove);
  } catch (err) {
    logger.warn(`⚠️ Could not remove old nodeNum...`, err);
  }
}, 5000);

Issue: If sendRemoveNode fails, the old nodeNum persists in device NodeDB, potentially causing future confusion.
Suggestion: Consider retry logic or add a manual cleanup endpoint for failed removals.

2. Magic Numbers

setTimeout(async () => {
  // ...cleanup logic
}, 5000); // Why 5 seconds?

Suggestion: Extract to named constant: NODEDB_CLEANUP_DELAY_MS = 5000 with comment explaining why this delay is needed.

3. Type Safety (meshtasticManager.ts:2896)

firmwareVersion: (newNode as any)?.firmwareVersion || (oldNode as any)?.firmwareVersion || undefined,

Issue: Using as any bypasses TypeScript safety
Suggestion: Define proper types or use type guards for these optional properties.

---

🔵 Minor Issues

1. Logging Consistency

• Mix of logger.info and logger.warn levels
• Consider standardizing log levels for similar events

2. Test Organization

• New tests are well-written but could benefit from more descriptive scenario names
• Consider grouping related test cases under shared describe blocks

---

🟢 Security & Performance

Security: ✅ Clean

• No security vulnerabilities identified
• Proper device ID validation prevents unauthorized node takeovers
• Safe fallback behaviors for edge cases

Performance: ✅ Acceptable

• Single database operations (no N+1 queries)
• Efficient ghost node cleanup
• Reasonable 5s delay for device cleanup doesn't block main thread

---

🔍 Potential Edge Cases

1. Race Condition: What if multiple rapid reboots occur before the 5s cleanup timer fires?
2. Network Partition: If sendRemoveNode times out, should we retry or have a periodic cleanup?
3. Database Rollback: If any step in the merge process fails, previous state isn't restored

---

📋 Recommendations Before Merge

1. Run system tests as noted in the test plan
2. Consider adding constants for magic numbers
3. Test manual scenario: Connect device, change name via Meshtastic app, verify name updates
4. Monitor logs in production to ensure deferred cleanup succeeds consistently

---

✅ Verdict

APPROVE - This PR solves real synchronization issues with a thoughtful approach. The benefits far outweigh the minor improvements suggested above. The code quality is high and the test coverage is comprehensive.

The solution correctly handles the fundamental challenge of keeping MeshMonitor's state synchronized with firmware that can change its identity autonomously.

---