Fix MAVLink listener socket leak and missing thread-signal reset

[mengelh]

app/mavlink/src/main/cpp/mavlink.cpp's listen() never closed its UDP
socket on any exit path (bind failure, setsockopt failure, recv error,
peer shutdown, or the normal loop exit once nativeStop() sets
mavlink_thread_signal). The bound fd leaked for the life of the
process, silently soaking up every datagram the kernel delivered to
port 14550 even after the reading thread had exited -- so once the
parser had been started and stopped once, nothing could ever read that
port's traffic again for the rest of the process's life, confirmed via
netstat showing an ever-growing, undrained receive queue.

Separately, mavlink_thread_signal was never reset when starting again,
so a second nativeStart() call would see the exit flag already set
from a previous nativeStop() and exit its loop before reading a single
packet.

Neither was reachable through stock PixelPilot's own call pattern
(nativeStart()/nativeStop() are each called at most once per app
lifetime today: once at onCreate, once on the first backgrounding), so
both bugs were latent. They matter for a caller that starts/stops the
listener repeatedly during the same process -- verified on-device by
toggling the listener on and off multiple times and confirming the
receive queue no longer grows unbounded and every restart parses new
traffic (rather than exiting immediately).

🤖 Generated with Claude Code

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix MAVLink listener socket cleanup and restart state

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Closes MAVLink UDP sockets after setup failures, receive termination, and normal listener
 shutdown.
• Resets the stop signal during startup, enabling repeated listener restarts within one process.

Diagram

sequenceDiagram
    actor Caller
    participant JNI as JNI lifecycle
    participant Thread as Listener thread
    participant UDP as UDP socket
    Caller->>JNI: nativeStart
    JNI->>JNI: Reset stop signal
    JNI->>Thread: Launch listener
    Thread->>UDP: Open and bind
    alt Setup or receive error
        Thread->>UDP: Close descriptor
    else Normal stop
        Caller->>JNI: nativeStop
        JNI->>Thread: Set stop signal
        Thread->>UDP: Close descriptor
    end
    Caller->>JNI: nativeStart again

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. RAII socket ownership

• ➕ Guarantees descriptor cleanup for current and future return paths
• ➕ Removes duplicated close calls
• ➖ Requires a small native resource-wrapper refactor
• ➖ May be broader than the immediate bug fix

2. Managed thread lifecycle with atomic cancellation

• ➕ Avoids cross-thread access through a plain integer
• ➕ Can coordinate shutdown completion before restarting
• ➕ Prevents overlapping detached listeners during rapid stop-start sequences
• ➖ Requires persistent thread ownership and synchronization
• ➖ Changes lifecycle behavior beyond the reported failures

Recommendation: The targeted changes are appropriate for fixing the immediate leak and stale stop signal with minimal scope. A follow-up should consider RAII descriptor ownership plus an atomic cancellation flag and joinable thread, which would make cleanup exception-safe and eliminate potential stop/start synchronization races.

Files changed (1) +10 / -0

Bug fix (1) +10 / -0

mavlink.cppClose listener sockets and reset restart state +10/-0

Close listener sockets and reset restart state

• Closes the MAVLink UDP descriptor after bind or timeout-configuration failures, receive errors, peer shutdown, and normal loop termination. Clears the previous stop signal before launching a new detached listener so subsequent starts can receive packets.

app/mavlink/src/main/cpp/mavlink.cpp

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Quick restarts can stop telemetry 🐞 Bug ☼ Reliability

Description

nativeStart clears mavlink_thread_signal and launches another detached listener without waiting
for the previous listener to close its socket. If the old thread has observed the stop signal but
has not yet closed the descriptor, the new thread can fail to bind before the old one exits, leaving
no thread receiving telemetry.

Code

app/mavlink/src/main/cpp/mavlink.cpp[387]

+    mavlink_thread_signal = 0;

Evidence

The stop state is a plain shared integer, while the receive loop can remain blocked for 100
milliseconds before checking it. nativeStop only increments that state, and nativeStart resets
it before creating a detached thread, so neither operation waits for the old listener's final close;
the new listener can consequently attempt its bind during that interval and terminate on failure.

app/mavlink/src/main/cpp/mavlink.cpp[57-57]
app/mavlink/src/main/cpp/mavlink.cpp[85-104]
app/mavlink/src/main/cpp/mavlink.cpp[319-324]
app/mavlink/src/main/cpp/mavlink.cpp[382-397]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A restart can reset the shared stop signal and launch a new listener before the previous detached listener has closed its bound socket. Retain and synchronize ownership of the listener thread so starting waits for prior shutdown, and use an atomic or otherwise synchronized stop state.

## Issue Context
The listener may take up to the receive timeout to observe a stop request. The replacement thread can therefore encounter the still-bound port and exit immediately, followed by the previous thread closing its socket and leaving no active listener.

## Fix Focus Areas
- app/mavlink/src/main/cpp/mavlink.cpp[57-57]
- app/mavlink/src/main/cpp/mavlink.cpp[85-104]
- app/mavlink/src/main/cpp/mavlink.cpp[322-324]
- app/mavlink/src/main/cpp/mavlink.cpp[382-397]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

Review mode: ⚖️ Balanced: This is a localized but behaviorally significant native networking/thread-lifecycle fix involving socket cleanup and restart signaling, so it warrants a careful single-pass review.

Tip of the day

💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-free-for-open-source-projects]

1. Quick restarts can stop telemetry 🐞 Bug ☼ Reliability

nativeStart clears mavlink_thread_signal and launches another detached listener without waiting
for the previous listener to close its socket. If the old thread has observed the stop signal but
has not yet closed the descriptor, the new thread can fail to bind before the old one exits, leaving
no thread receiving telemetry.

Agent Prompt

## Issue description
A restart can reset the shared stop signal and launch a new listener before the previous detached listener has closed its bound socket. Retain and synchronize ownership of the listener thread so starting waits for prior shutdown, and use an atomic or otherwise synchronized stop state.

## Issue Context
The listener may take up to the receive timeout to observe a stop request. The replacement thread can therefore encounter the still-bound port and exit immediately, followed by the previous thread closing its socket and leaving no active listener.

## Fix Focus Areas
- app/mavlink/src/main/cpp/mavlink.cpp[57-57]
- app/mavlink/src/main/cpp/mavlink.cpp[85-104]
- app/mavlink/src/main/cpp/mavlink.cpp[322-324]
- app/mavlink/src/main/cpp/mavlink.cpp[382-397]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools