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

Close MAVLink listener sockets and reset the stop signal on restart

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Close the UDP socket on every listener exit to prevent leaked descriptors and undrained traffic.
• Reset the stop signal before starting a listener so subsequent starts can receive packets.

Diagram

graph TD
  A["Start and reset"] --> B["Open UDP socket"] --> C{"Setup succeeds?"}
  C -->|yes| D["Receive MAVLink"] -->|error or shutdown| E["Close socket"]
  C -->|no| E
  F["Stop signal"] -->|ends loop| E
  E -->|subsequent start| A

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Manage the listener thread and socket as one lifecycle

• ➕ Joining the prior thread before restart could prevent overlapping listeners and stop-signal races.
• ➕ Scoped socket ownership would make cleanup independent of individual return paths.
• ➖ Requires a broader change to thread ownership and synchronization than this targeted leak fix.

Recommendation: The targeted fix addresses the reported sequential stop-and-restart failures with little disruption. It does not make concurrent or rapid restarts safe: the listener is detached and its stop signal is a non-atomic shared int. Use a managed, synchronized lifecycle if repeated starts and stops become a supported API behavior.

Files changed (1) +10 / -0

Bug fix (1) +10 / -0

mavlink.cppRelease listener sockets and clear the prior stop signal +10/-0

Release listener sockets and clear the prior stop signal

• Closes the UDP socket after bind or timeout-configuration failure, receive error, zero-length receive, and normal loop exit. Clears mavlink_thread_signal before launching a new listener so a previous stop does not immediately terminate it.

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

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

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (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

---

2. Restarts race with the old listener 🐞 Bug ☼ Reliability

Description

nativeStart adds an unsynchronized write to the plain int mavlink_thread_signal while an
earlier listen thread may still read it. Calling start while that detached thread remains active
creates a C++ data race, so the loop’s behavior is undefined even apart from whether a stop request
is lost.

Code

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

+    mavlink_thread_signal = 0;

Evidence

The signal is a plain global integer; the old detached listener reads it without synchronization,
and the newly added start-time write has no ordering that ensures the old thread has exited.

app/mavlink/src/main/cpp/mavlink.cpp[57-57]
app/mavlink/src/main/cpp/mavlink.cpp[95-97]
app/mavlink/src/main/cpp/mavlink.cpp[387-392]

Agent prompt

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

## Issue description
The new start-time signal write can race with an active listener’s read.
## Fix Focus Areas
- app/mavlink/src/main/cpp/mavlink.cpp[57-57]
- app/mavlink/src/main/cpp/mavlink.cpp[95-97]
- app/mavlink/src/main/cpp/mavlink.cpp[387-397]
## Recommended Fix
Make all accesses to the stop state synchronized, for example with an atomic stop flag. Coordinate listener completion before resetting the flag for a new start.

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

---

Tip of the day

💡 Did you know, you can move or hide resolved findings to focus on what still needs action

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

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

Code review by qodo was updated up to the latest commit bdf370c

[josephnef]

Approving. Every exit path of listen() now closes the socket and nativeStart() resets the exit flag; both are correct and the change is minimal. CI is green on the reopened run.

Two non-blocking notes:

• The listener thread is detached, so a fast stop/start can race: the new thread's bind() can hit EADDRINUSE while the old one is still between recvfrom returning and close(fd). Setting SO_REUSEADDR before bind (or keeping the thread handle and joining in nativeStart) would close that gap. Qodo raised the same point.
• Worth knowing for the follow-ups: on master nativeStart is only called from onCreate and nativeStop from onStop, so after the first backgrounding the OSD telemetry is dead for the rest of the process anyway. This fix is a prerequisite for anything that restarts the listener, including #125.