Fix SmartLink disconnect teardown#2278
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates SmartLink/WAN disconnect handling so remote sessions use the same RadioModel::onDisconnected() teardown path as LAN sessions. That fits the connection lifecycle code by ensuring radio-owned model state and downstream UI observers are cleaned up consistently, which is also needed for the reconnect behavior described in #2254.
Changes:
- Replace the WAN disconnect fast-path that nulled
m_wanConndirectly with the normalonDisconnected()cleanup flow. - Ensure intentional SmartLink disconnects emit the standard disconnect-side signals and clear session-owned state.
- Align WAN teardown behavior with existing LAN disconnect semantics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m_wanConn->disconnect(this); // remove signal connections to prevent duplicates on reconnect (#224) | ||
| m_wanConn->disconnectFromRadio(); | ||
| m_wanConn = nullptr; | ||
| onDisconnected(); |
|
Hi @rfoust — thanks for the contribution! 🙌 Quick note on the red ✗ you're seeing: the only job that failed on While I'm here, one thing worth flagging from the Copilot review on
That race matches the kind of failure mode users hit with rapid disconnect→reconnect on SmartLink. A few options to consider:
Option 1 is probably the cleanest if Either way — really nice catch on the underlying bug. The fact that |
|
Addressed the WAN teardown race review:
Rebuilt successfully with: |
There was a problem hiding this comment.
Thanks @rfoust — clean fix for a real bug. Routing through onDisconnected() is clearly correct: without it the WAN path skipped panadapter/slice/meter/stream cleanup, model state reset, and the connectionStateChanged(false) emission, which is also why #2254's RX applet teardown couldn't fire. Pairing a Qt::SingleShotConnection lambda with the isSocketIdle() synchronous fast path covers both the normal async close and the already-unconnected case, and the m_wanConn == wan re-check inside the lambda prevents a stale fire from clobbering a fresh session.
A few notes:
Copilot's race concern — mostly mitigated, worth a small follow-up. Copilot flagged that a quick disconnect→reconnect could race against the still-closing QSslSocket. The new guard in WanConnection::connectToRadio():
if (m_connected || m_socket.state() != QAbstractSocket::UnconnectedState) {
qCWarning(lcSmartLink) << "WanConnection: already connected or still closing";
return;
}handles that correctly — the second-call returns silently. Note though that connectionStateChanged(false) does not fire "immediately" in the async path as Copilot's comment implies; it only fires once the socket finishes closing and the lambda runs onDisconnected(), so the UI shouldn't re-enable Connect mid-teardown in the first place. So the concrete race is narrow, but if a user does manage to click Connect during the close window the warning gets logged and the click is silently dropped — consider whether MainWindow's WAN connect path (src/gui/MainWindow.cpp:1159) should surface that state to the user, or block the button until connectionStateChanged(false) lands. Not blocking; just a follow-up.
One small nit: the comment on the lambda connect (// remove stale signal connections before adding the one-shot teardown (#224)) attaches the #224 ticket to the new line, but #224 was about preventing duplicate connections on reconnect. The original intent of that issue is preserved by the leading wan->disconnect(this), so the ticket reference reads more naturally on that line. Minor.
Files in scope match the stated fix — no scope creep. LGTM as a COMMENT review.
The v0.9.5.1 entry was originally cut for just the TCI TX hotfix (#2285), but six additional fixes had already landed in main between the v0.9.5 tag and v0.9.5.1: - #2275 NR2 wisdom generation off the audio thread - #2278 SmartLink disconnect teardown - #2279 Reset RX slice tabs on disconnect (#2254) - #2280 macOS panadapter pop-out refresh + multi-pan dock layout - #2282 SmartLink reconnect after WAN drop - #2284 Qt log handler serialization fixes macOS tune-time crash CHANGELOG.md gets a per-fix entry with root cause + behavior change. WhatsNewData.cpp regenerated via scripts/gen_whatsnew.py so the in-app What's New dialog reflects the full v0.9.5.1 contents. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Match the v0.9.5 `(#NNNN, contributor)` heading style and call out @rfoust and @jensenpat in the summary for the bulk of this release. Authors per PR: - @rfoust: #2278, #2279, #2280, #2282 (SmartLink + macOS popout) - @jensenpat: #2275, #2284 (NR2 wisdom + Qt log serialization) WhatsNewData.cpp regenerated. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bug
SmartLink/WAN disconnect closed the WAN socket after disconnecting RadioModel's WanConnection signals, then nulled m_wanConn directly. That skipped RadioModel::onDisconnected(), leaving radio-owned state such as panadapters, slices, meters, streams, and downstream UI state alive after the user clicked Disconnect.
Fix
Run the normal RadioModel::onDisconnected() cleanup path after intentionally closing a WAN session. This preserves the existing duplicate-signal guard while making WAN disconnects emit connectionStateChanged(false) and clear model state like LAN disconnects.
Testing
Codex
Codex model used: GPT-5.
Related
This is required for the #2254 UI fix to take effect for SmartLink/WAN sessions, because the RX applet teardown is triggered from connectionStateChanged(false).