Fix parser and command callback state leaking across reconnects

[rfoust]

Problem and fix

A TCP disconnect in the middle of a Flex status line left RadioConnection::m_readBuffer intact. The next session's version line was appended to the old bytes: R42| fabricated a successful response for sequence 42, while H produced an extra zero-handle connection notification. Same-backend reconnects also retained the old model command callbacks.

Reset the partial buffer, client handle, and pending ping timing at disconnect/error and connection-start boundaries. Expire model callbacks on disconnect using the existing terminal failure code. Remove a response callback before invoking it so a callback-triggered disconnect cannot expire it twice or invalidate the iterator. The existing subscription and occupied-client callback chains stop on that terminal result, while ordinary radio-result behavior is preserved.

Fixes #5649.

Validation

• Full native macOS application built with the local ARM64 toolchain, RADE enabled. Host/system processors and Mach-O executable are ARM64; no RNNoise x86 sources in the Ninja graph.
• Four focused CTests pass: radio_connection_session_test, tx_operation_integration_test, flex_backend_lifecycle_test, and backend_family_switch_test.
• New parser coverage injects bytes through production onReadyRead() using an in-memory QTcpSocket subclass. It covers V/H/R/S/M prefixes across remote, explicit, and terminal-error boundaries; split fresh handshakes; one valid handle; no fabricated response/status; expired ping timing; and the real TCP connection-start method with its final virtual transport call intercepted. No socket descriptor, listener, network peer, or simulated radio firmware is opened by this regression.
• Callback coverage uses production model response wiring and checks terminal expiration, late response suppression, repeated disconnect, reentrant disconnect, and subscription/client-disconnect chains.
• Three mutations each fail the relevant regression: remove buffer clearing; remove TCP-start reset; remove disconnect callback expiration. Restoring the fixes returns the tests to green.
• Engine-boundary, command-plane, test-registration, frozen CI-gate and generated-touchpoint checks pass. Independent review findings were corrected and re-reviewed.
• FlexLib does the same thing, for the same reason: TcpCommandCommunication keeps its line-assembly buffer as a member of a long-lived object and clears it under _tcpReadSyncObj in Disconnect() (reference/FlexLib_API_v4.1.5.39794/FlexLib/TcpCommandCommunication.cs:249). It is not per-session by lifetime. This patch also resets at the connect edge, which additionally covers a hard kill that never reaches a disconnect path. (An earlier revision of this body claimed a fresh reader lifetime and cited FlexLib 4.2.18; both were wrong — the vendored reference is 4.1.5.39794, and the real upstream behaviour is stronger support for this fix. Corrected in review.)

Agent automation bridge coverage

The native demo smoke test was attempted with isolated settings and TX disabled, but is not claimed as passed. The first isolated launch did not expose its bridge socket and was stopped. A retry using an ephemeral token then waited 570 seconds for the shared application slot and timed out while another AetherSDR instance remained active; that instance was left untouched.

The agent automation bridge cannot inject a truncated TCP line or observe the private pending-callback map. The socket-free production-path tests above are the load-bearing proof. No live Flex firmware convergence, partial-wire bridge proof, or completed demo reconnect smoke coverage is claimed. No screenshot is needed for this non-visual transport fix.

The default all-target build was narrowed to the complete application and relevant tests after compiling much of the unrelated suite. The full test suite was not run. macOS iconutil required the native build context rather than the sandbox; no source workaround was added.

Generated with OpenAI Codex.

---

Review round 2 — all findings addressed

Three reviews (@aethersdr-agent, Copilot, @K5PTB) plus a fourth pass raised two blockers and a set of
non-blocking findings. Every one is now fixed in this PR, with regression coverage for each.

Blockers

1. A network blip during GUI registration permanently disabled auto-reconnect.
The in-flight client gui callback treats any non-zero result as a radio rejection and routes into
handleGuiClientRegistrationFailure(), which latches m_intentionalDisconnect and stops the reconnect
timer. The new disconnect-edge expiry handed it the terminal code, so a mid-handshake TCP drop — the exact
recovery path #5649 protects — ended the session for good and told the operator a GUI-client slot was taken.

Fixed by a commandNeverReachedRadio(code) helper next to kNoCommandPlaneCode, checked in the client gui
callback before the rejection branch. The three sentinel comparisons this PR already had now use the same
helper, so the distinction between "the radio refused" and "the command never got there" is named once rather
than open-coded at each site.

2. Expiry re-inserted callbacks into the map it had just cleared.
hasCommandPlane() is only a pointer check — m_connection outlives the socket — so a drained callback that
chained another sendCmd() (createAudioStream()'s stream remove → createRxAudioStream()) landed a fresh
entry in the cleared map and queued a write to a dead socket. Measured 1 → 1 rather than 1 → 0.

Fixed centrally rather than per-site: expirePendingCallbacks() sets m_expiringPendingCallbacks for the
duration of the drain (saved/restored, so a nested disconnect cannot lift it early) and sendCmd() drops
commands issued under it. No call site can opt out by forgetting a guard, which is what blocker 1 was.

Also fixed

• The SmartLink sibling had the identical defect. WanConnection runs its own line assembly and cleared
  m_readBuffer only in its overflow guard — never at a session boundary — while onTlsDisconnected() reset
  nothing but m_connected. Since MainWindow::m_wanConnection is a by-value member alive for the whole
  process, a WAN reconnect after a mid-line drop carried the same fabricated-response/zero-handle failure.
  Now has a resetSessionState() called at all three edges. This makes the title's "across reconnects" honest.
• The multiFLEX peek was not session-scoped (Copilot). Its 400 ms singleShot could fire after a drop and
  consume a continuation belonging to the dead session. Now guarded by a session generation, with the
  continuation dropped in onDisconnected(). The test that asserted the stale continuation survives has been
  flipped — it was pinning the leak as correct.
• The synthetic-demo handshake timers were not session-scoped (Copilot). They tested only m_syntheticDemo,
  which a fast reconnect sets straight back to true, so an old session's timers could replay
  version/connected/status into the new one. Now pinned to the same session generation.
• Dead store removed: m_handle = 0 in the demo teardown branch, already zeroed by resetSessionState().
  The deliberate double resetSessionState() in disconnectFromRadio() now says why it is not a duplicate,
  so a future cleanup does not delete the load-bearing one.

Not changed, with reasons

• Copilot's RadioConnection.cpp:528 (terminal socket error reaching UnconnectedState without emitting
  disconnected()): @K5PTB's loopback probe showed the client emits errorOccurred while still ConnectedState
  and then disconnected(), so the expiry does run; that branch belongs to connects that never completed and
  have no pending callbacks. No change needed.
• "The new test never runs in CI": accurate that ci.yml's ctest steps are -R-filtered and this target
  matches none, but that is the project's design (ci.yml's own header says so). Registration in tests.cmake
  is the correct and complete step; the sanitizer lane runs the suite. No change needed.

Round-2 verification

• New regression tests, all socket-free: guiRegistrationDropIsNotARejection (asserts the reconnect timer stays
  armed and no guiClientRegistrationFailed fires), expiringCallbackCannotRepopulateTheMap (asserts 1 → 0),
  and staleDemoTimersCannotReplayIntoANewSession (asserts exactly one handshake across a fast demo reconnect).
• Each new test was confirmed to fail with its fix reverted.
• Demo smoke test completed this round — the one the first revision could not finish: offscreen, isolated
  AETHER_SETTINGS_DIR, AETHER_AUTOMATION_NO_TX=1, attached explicitly to DEMO-0001. Three disconnect/
  reconnect cycles returned connected=true serial=DEMO-0001 sliceCount=1 panCount=1 with no state
  accumulation; sim malformed left the session intact; sim disconnect dropped and recovered cleanly.
• The WAN change is reasoned and compiles but is not exercised against live SmartLink hardware.

[aethersdr-agent]

1. Issue fit

Partially. The RadioConnection half of #5649 is solid and I could not break it: resetSessionState() covers all four session edges (connectToHost, disconnectFromRadio, onSocketDisconnected, and the onSocketError → unconnected path), and the new radio_connection_session_test genuinely fails against unfixed code — with m_readBuffer retained, feed("0.0\nH00AB") leaves versions.size() == 0 because the leaked R42|/H prefix eats the version line, which is exactly the assertion at line 108.

The RadioModel half is where I think this breaks. The issue asked to "inspect model pending command callback expiration and fix only demonstrated related leakage." Calling expirePendingCallbacks() from onDisconnected() changes the contract for all ~131 sendCmd() call sites — a callback that used to be silently dropped on a network blip now runs its error branch on every disconnect. The PR audits three of them (disconnectClientHandlesThen, the two MultiFlex subscriptions). I found at least two more that misbehave, one of which regresses the very recovery path this issue is about. Details in blockers 1 and 2.

2. Scope

File	What it changes	Claimed?	Verdict
src/core/RadioConnection.{h,cpp}	resetSessionState() helper + calls on all four session edges; friend RadioConnectionSessionTestAccess	Yes — issue's "Proposed fix" verbatim	In scope
src/models/RadioModel.cpp — expirePendingCallbacks() extraction + onDisconnected() call	Drains pending callbacks at the disconnect edge	Yes	In scope (but see blockers)
src/models/RadioModel.cpp:1402 — move-then-erase in the commandResponse lambda	Detaches the callback before erasing so a reentrant sendCmd can't invalidate it	Not named in the issue or the commit message	In scope-adjacent and correct — the old it.value()(code, body); m_pendingCallbacks.erase(it); is a real use-after-invalidate if the callback touches the map. Worth a sentence in the PR body rather than arriving unannounced
src/models/RadioModel.cpp:6923, 6960, 6964 — kNoCommandPlaneCode early returns	Stops three chains from advancing on expiry	Yes	In scope; incomplete (blockers)
tests/radio_connection_session_test.cpp, tests/tests.cmake	New socket-free regression target	Yes	In scope, registered
tests/tx_operation_integration_test.cpp	Access shims + pendingCallbackDisconnectExpiry()	Yes	In scope

No CHANGELOG.md entry (correct). No new protocol verb, settings key, capability field, config document, or public API. No UI element or default touched, so nothing to classify under the preference check. No sibling copy of the parser left behind — WanConnection maintains its own buffer, and its callbacks live in WanConnection's own map, not m_pendingCallbacks (see nit 1).

Socket-test disclosure

The PR adds tests/radio_connection_session_test.cpp, which subclasses QTcpSocket (MemorySocket). It is not a socket-owning test: connectToHost is overridden to ++connectAttempts; open(mode); and never delegates to the base, readData/bytesAvailable are served from an in-memory QByteArray, and both TCP cases assert socketDescriptor() == qintptr(-1) as standing proof. No bind, listen, peer process, or fake radio. The tests.cmake block says so at line 3632. Recording it here per the reporting rule; it is not a finding.

3. Blockers

1. A network blip during GUI registration now permanently disables auto-reconnect. (inline on RadioModel.cpp:7677) The client gui response callback at RadioModel.cpp:7206 branches on code != 0 into handleGuiClientRegistrationFailure(), which at RadioModel.cpp:7154-7172 does m_intentionalDisconnect = true; m_reconnectTimer.stop(); … emit connectionError(message); closeConnectionForTerminalDisconnect();. With this PR, a TCP drop while client gui is in flight expires that callback with kNoCommandPlaneCode, so the operator gets a modal-grade "GUI client registration failed (0x50000063): the radio connection was disconnected. AetherSDR disconnected without retrying" and the 5 s reconnect timer is stopped. Before this PR the callback was simply never invoked and the reconnect timer fired normally. The issue's own framing is "the affected path is the network-blip recovery path" — this makes that path terminal. Fix: same if (code == kNoCommandPlaneCode) return; guard the PR already applies at 6923/6960/6964, or (better, see blocker 2) an expiry code the callbacks can't confuse with a radio rejection.

2. Expiry re-creates the leak it is closing, because hasCommandPlane() is still true inside onDisconnected(). (inline on RadioModel.cpp:1909) hasCommandPlane() is m_wanConn != nullptr || m_connection != nullptr (RadioModel.h:614) — it says nothing about whether the link is up, and both pointers are live throughout onDisconnected(). So any expired callback that chains a sendCmd inserts a fresh entry into the map expirePendingCallbacks() just cleared, and queues a write to a dead socket. createAudioStream() at RadioModel.cpp:12208 is a concrete one: sendCmd("stream remove …", [this](int, const QString&) { createRxAudioStream(); }) ignores the result code entirely, and createRxAudioStream() (9223) sets m_rxAudio.createPending = true and fires two more sendCmds. A disconnect landing on an in-flight stream remove therefore leaves a stream create callback stranded in m_pendingCallbacks for the life of the process — the exact condition this PR exists to eliminate. The PR's own tests assert this hazard for the MultiFlex and client-disconnect chains ("an expired client subscription cannot continue the MultiFlex handshake"), so the omission is inconsistent rather than a judgment call. I'd rather see this fixed centrally than by a fourth per-site guard: e.g. set a m_expiringPendingCallbacks flag that sendCmd() checks and refuses under, so no call site can opt out of the invariant by forgetting.

4. Nits (non-blocking)

• WAN sessions get none of this. sendCmd() returns through m_wanConn->sendCommand(...) before ever touching m_pendingCallbacks (RadioModel.cpp:9538), so on SmartLink the disconnect-edge drain is a no-op and WanConnection's own callback map keeps the old behavior. Not a regression, but the PR title says "across reconnects" without qualification — worth a line in the body scoping it to LAN/Flex.
• The new test never runs in CI. Every ctest invocation in ci.yml is -R-filtered (lines 771, 784, 1126, 1141) and radio_connection_session_test matches none of them. It will compile-gate only. Standard for this repo, just noting that "CI green" here means "it builds."
• tx_operation_integration_test.cpp asserts hasMultiFlexContinuation(radio) is still true after disconnect — i.e. it pins a stale continuation surviving the session boundary as correct. It's benign (line 6941 overwrites it on the next probe), but as written the test reads like it's endorsing the leak rather than documenting indifference. A comment saying which would help.

5. What I tried to break (and couldn't)

• resetSessionState() clearing m_handle before emit disconnected(). This is new — onSocketDisconnected() previously left the dead session's handle readable. I walked the whole body of RadioModel::onDisconnected() (7673-7951) and it never calls clientHandle(); m_ownSessionHandle is captured separately in stageSessionModelsForReconnect(), whose comment at 7791 already assumes the handle is zeroed by then. No consumer of the disconnected() edge reads it. Clean.
• resetSessionState() at the top of disconnectFromRadio(), zeroing m_handle before the teardown writes. writeDisconnectMarker() (689) writes a bare \x04 and never reads the handle, and gracefulDisconnect() takes its handle as a parameter and does its stream remove before calling disconnectFromRadio(). No ordering hazard.
• The double resetSessionState() in disconnectFromRadio(). Deliberate and correct: the first covers the synthetic-demo early return, the second catches bytes that arrive during waitForDisconnected(2000).
• Whether the new parser test would pass against unfixed code. It would not — the partialLineAcrossDisconnect rows feed V1.4. / 0.0\n split across the boundary and assert versions.size() transitions 0→1, which only holds with the buffer cleared; the pings.size() == 0 assertion likewise only holds with m_lastPingSeq reset. Both are real guards, not implementation echoes.
• Reentrancy inside expirePendingCallbacks() itself. The detach-clear-then-iterate ordering is right; a callback that inserts during the loop cannot invalidate pending. The 1402 move-before-erase closes the matching hazard on the normal-response path. This part I have no complaint about.
• shouldRetryLanUdpPortRegistration on an expired client udpport (RadioModel.cpp:7361): it gates on isUdpPortInUseError(code, body), which 0x50000063 does not match, so no spurious rebind. Survived.

Everything above is reasoned from reading the head checkout at /tmp/aetherclaude/pr-5653; I have no build and ran nothing, so blockers 1 and 2 are traced through the code paths, not reproduced at runtime.

6. Recommendation

Request changes — on the RadioModel half only. The RadioConnection fix is well-targeted, well-tested, and I'd take it as-is. The problem is that expiring callbacks at the disconnect edge is a much broader semantic change than the three audited call sites acknowledge, and one of the unaudited ones (client gui) turns a recoverable blip into a terminal, reconnect-disabling error — a worse outcome on the same path the issue is trying to protect. Concrete next step: either split the RadioConnection fix out and land it now, or add the central guard from blocker 2 (sendCmd() refuses while expiring, plus a code every callback treats as "session gone, do nothing") and sweep the sendCmd call sites that branch on code != 0 into user-visible or state-changing actions.

Nice work on the transport injection — socketDescriptor() == -1 as a standing assertion is a good pattern and I'd like to see it copied.

---

_{🤖 aethersdr-agent · cost: $5.0254 · model: claude-opus-5}

[Copilot]

🟡 Changes recommended

Unresolved disconnect and session-lifecycle paths can retain or replay stale callbacks across reconnects.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes #5649 by resetting connection state and expiring pending callbacks across Flex reconnects.

Changes:

• Resets parser, handle, and ping state at session boundaries.
• Expires callbacks and removes response callbacks before invocation.
• Adds parser and callback lifecycle regression tests.

File summaries

File	Review
tests/tx_operation_integration_test.cpp	Adds callback lifecycle coverage. Moderate (1 vote): the MultiFLEX continuation remains alive after expiration and should be invalidated.
tests/tests.cmake	Registers the parser session test.
tests/radio_connection_session_test.cpp	Adds parser and reconnect-boundary coverage.
src/models/RadioModel.h	Declares callback-expiration support.
src/models/RadioModel.cpp	Moderate (2 votes): terminal expiration can be treated as a radio rejection; moderate (2 votes): callbacks can enqueue new callbacks during expiration; moderate (1 vote): the MultiFLEX continuation and timer can survive disconnect.
src/core/RadioConnection.h	Declares connection-state reset support and test access.
src/core/RadioConnection.cpp	Moderate (1 vote): terminal socket errors may not emit disconnect cleanup; moderate (1 vote): queued synthetic-demo callbacks are not session-scoped.

Review details

Suppressed comments (4)

src/core/RadioConnection.cpp:528

• When the socket is already UnconnectedState, this branch changes the connection state to Disconnected but never emits disconnected(). RadioModel only expires m_pendingCallbacks from its disconnected handler, so a terminal socket error can leave callbacks from the old session queued while onConnectionError() arms the auto-reconnect; the same stale-callback leak then survives into the next session. Route this terminal error through the same disconnect cleanup (or otherwise expire the model callbacks) and add coverage for the error-without-disconnected-signal path.

    if (m_socket->state() == QAbstractSocket::UnconnectedState) {
        resetSessionState();
        setState(ConnectionState::Disconnected);
    }

src/core/RadioConnection.cpp:120

• The boolean m_syntheticDemo does not invalidate the queued singleShot callbacks from an earlier demo session. If the demo is disconnected and reconnected before the old 0 ms or 50 ms callbacks run, those callbacks see the new session's true flag and emit duplicate version/connected/status events into the new session. Capture a per-session generation (or use cancelable timers) in both lambdas and cover a rapid demo reconnect.

    resetSessionState();

src/models/RadioModel.cpp:7677

• Disconnect expiration does not invalidate m_multiFlexContinuation or the 400 ms timer created by peekForMultiFlexConflictThen(). If the link drops after sub client all succeeds but before that timer fires, it can run while disconnected or during the next session, consume the old/new continuation, and issue client gui plus new pending callbacks. Clear the continuation and cancel or generation-guard the timer at the session boundary; the added test currently asserts that the continuation remains set after disconnect, so it does not catch this stale-session execution.

    expirePendingCallbacks(QStringLiteral("the radio connection was disconnected"));

tests/tx_operation_integration_test.cpp:414

• This expectation leaves m_multiFlexContinuation alive after the subscription callback is expired. That continuation belongs to the dead session; if the second subscription reply had already armed the 400 ms singleShot in peekForMultiFlexConflictThen(), a disconnect before the timer fires can still invoke registerAsGuiClient() after the session ended, and the stale continuation is retained across reconnects. Clear/invalidate this session continuation on disconnect (or guard the timer with a session generation) and assert that it is absent here.

            check(TxOperationIntegrationTestAccess::pendingReplyCount(radio) == 0
                      && TxOperationIntegrationTestAccess::hasMultiFlexContinuation(radio),
                  "an expired client subscription cannot continue the MultiFlex handshake");

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[K5PTB]

Review of 495a33d: independent pass with a local CMake build, runtime probes of both bot blockers, and verified fixes

Copilot and aethersdr-agent reviewed this PR without building it, and both raised the same two blockers. This review builds the code and runs production-path probes. Both blockers reproduce, and both are fixed by changes I verified. I also tested the PR author's three claimed mutations and Copilot's socket-error claim.

Issue fit: partially. The RadioConnection half of #5649 is correct and well tested. The RadioModel half turns on callback expiry at the disconnect edge. That closes one leak, but it opens a regression on the network-blip recovery path, and it leaves a second leak in place.

What passes. I built the four focused tests the body names on macOS arm64, Homebrew Qt 6.11.1, Debug. All four pass: radio_connection_session_test, tx_operation_integration_test, flex_backend_lifecycle_test, and backend_family_switch_test. CI filters ctest by name and never selects them.

The PR body's mutation claims reproduce.

Mutation	Result
don't clear m_readBuffer in resetSessionState()	partialLineAcrossDisconnect fails on 15 rows
no resetSessionState() at TCP start	newTcpSessionResetsOldBytes fails
no expirePendingCallbacks() in onDisconnected()	7 pendingCallbackDisconnectExpiry checks fail

Scope

File	Change	Claimed	Verdict
src/core/RadioConnection.{h,cpp}	resetSessionState() on every session edge	Yes	In scope
src/models/RadioModel.{h,cpp}	expirePendingCallbacks() on disconnect, move-before-erase on response, 3 expiry guards	Yes. aethersdr-agent notes move-before-erase is unmentioned	In scope
tests/radio_connection_session_test.cpp, tests/tests.cmake, tests/tx_operation_integration_test.cpp	new regression coverage	Yes	In scope

Everything in the diff is explained by the issue.

Blockers

1. A disconnect during GUI registration permanently disables auto-reconnect. Confirmed at runtime; see the inline comment on RadioModel.cpp:7677. With the PR, a disconnect while client gui is pending emits guiClientRegistrationFailed, sets m_intentionalDisconnect, and leaves the reconnect timer stopped. Without the expiry line, the same probe arms the timer. This regresses the recovery path #5649 exists to protect. A one-line guard in that callback fixes it; see the verified code block inline.

2. Expiry re-inserts callbacks into the map it just cleared. Confirmed at runtime; see the inline comment on RadioModel.cpp:1909. The in-flight stream remove chain from createAudioStream() leaves 1 pending callback after onDisconnected(). A central guard in sendCmd() brings that to 0; see the verified code block inline.

The two fixes are independent, and both are needed.

Build	GUI-registration blip: reconnect armed	stream-remove chain: callbacks left	focused tests
this PR	no	1	pass
client gui guard only	yes	1	pass
central sendCmd() guard only	no	0	pass
both	yes	0	pass

Findings (non-blocking)

3. Copilot's socket-error claim at RadioConnection.cpp:528 does not hold for a connected socket. I ran a standalone QTcpSocket probe on loopback, with the server aborting and closing the connection. The client emitted errorOccurred while still ConnectedState, then disconnected(), so onDisconnected() and the expiry do run. The UnconnectedState branch Copilot flagged is reached when a connect never completes, and that session has no pending callbacks. I didn't probe other error kinds, such as a network interface going down.

4. Two issues that predate this PR, noted but out of scope. The first is Copilot's point that the MultiFlex 400 ms singleShot in peekForMultiFlexConflictThen() isn't session-scoped. It can consume m_multiFlexContinuation after a disconnect, and the new test asserts that continuation survives. The second is that the synthetic demo's queued singleShots check only the m_syntheticDemo boolean, so a very fast demo reconnect could replay them. Main already had both behaviors, and this PR doesn't make either worse. They'd make good follow-up issues.

5. aethersdr-agent's other claims — WAN sessions are unaffected, the move-before-erase change is correct, and resetting m_handle before disconnected() is safe — are consistent with what I read. I didn't run them.

What was verified and what was only read

• Verified locally on macOS arm64. The four focused tests; the three claimed mutations; both blocker probes; both fixes, alone and together, with the focused tests passing under each; and the loopback socket-signal probe.
• Not verified. Real Flex hardware, SmartLink/WAN, Windows or Linux, and the full suite.
• Read only. Finding 4 and aethersdr-agent's other claims.

Recommendation: request changes. The RadioConnection fix is ready. The disconnect-edge expiry needs the two verified guards before merge. The first stops a network blip during registration from ending auto-reconnect. The second stops expired callbacks from leaking new ones. Alternatively, as aethersdr-agent suggests, split out the RadioConnection half and land it now.

[ten9876]

Issue fit — partially

#5649 has two halves. The RadioConnection half is correct and I could not break it: resetSessionState() covers all four session edges, and radio_connection_session_test genuinely pins the fix rather than echoing it — I reverted each of the three mutations the body claims and all three fail (table below). I'd take that half as-is.

The RadioModel half — expiring pending callbacks at the disconnect edge — closes the stale-callback leak, but it opens a regression on the network-blip recovery path that #5649 exists to protect, and it leaves a second leak in place.

Both problems were already found by @aethersdr-agent and Copilot (by reading) and reproduced by @K5PTB on macOS. What this pass adds: both reproduced on Linux against the PR head, and against the actual merge base 85c9413b — the "before" measurement nobody had run, which is what distinguishes a regression from pre-existing behaviour. Plus one governance correction and one sibling implementation that no pass has flagged.

Scope

File / group	What it changes	Claimed in title or body?	Verdict
src/core/RadioConnection.{h,cpp}	resetSessionState() helper; called on all four session edges; friend RadioConnectionSessionTestAccess	Yes — the issue's "Proposed fix" almost verbatim	In scope, and the strong part of the PR
src/models/RadioModel.cpp — expirePendingCallbacks() extraction + onDisconnected() call	Drains pending callbacks at the disconnect edge	Yes	In scope; blockers 1 and 2
src/models/RadioModel.cpp:1402 — move-then-erase in the commandResponse lambda	Detaches the callback before erasing it	Only obliquely ("Remove a response callback before invoking it")	In scope and correct — the old it.value()(code, body); erase(it); is a real use-after-invalidate. Fine to keep; worth naming in the body as its own fix
src/models/RadioModel.cpp:6923, 6960, 6964 — kNoCommandPlaneCode early returns	Stops two callback chains advancing on expiry	Yes	In scope; incomplete — a third chain is unguarded (blocker 1)
src/models/RadioModel.h	declares expirePendingCallbacks()	Yes	In scope
tests/radio_connection_session_test.cpp, tests/tests.cmake	New socket-free regression target	Yes	In scope, registered
tests/tx_operation_integration_test.cpp	Access shims + pendingCallbackDisconnectExpiry()	Yes	In scope

Everything in the diff is explained by #5649. One commit, authoredDate 2026-09-12 — the issue's own date, no rebase-flattened outlier. No CHANGELOG.md entry (correct — it is a release-prep file and an ordinary PR must not add one). No CI or workflow edits. No new protocol verb, wire message, settings key, capability field, CLI flag, or exported API. No existing UI element, default value, slider range, timer interval, or keyboard behaviour is touched, so there is nothing to classify under the preference check.

Test-layer boundary (AGENTS.md). radio_connection_session_test subclasses QTcpSocket, so I checked it before running it: connectToHost() is overridden to ++connectAttempts; open(mode); and never delegates to the base, readData()/bytesAvailable() are served from an in-memory QByteArray, and both TCP cases assert socketDescriptor() == qintptr(-1) as a standing guard. No bind, listen, peer process, or fake radio firmware — this is injected transport, which is exactly the layer AGENTS.md routes "a dropped/malformed/disconnected input" to. The tests.cmake block says so. Recording it per the disclosure rule; it is not a finding. I also confirmed empirically that it writes nothing to a settings store (ran under a scratch HOME/XDG_CONFIG_HOME: zero files created), so the absent TestSettingsProfile.h is not a live hazard.

Blockers

1. A network blip during GUI registration permanently disables auto-reconnect. Inline on src/models/RadioModel.cpp:7677. A TCP drop while client gui is in flight expires that callback with kNoCommandPlaneCode; the callback treats any non-zero code as a terminal rejection, sets m_intentionalDisconnect and stops the reconnect timer before onDisconnected() reaches its own m_reconnectTimer.start(). Measured: reconnect armed = 1 on the merge base, 0 on this PR, with a user-visible notice blaming a GUI-client slot conflict for a network drop. Full evidence and a minimal guard in the inline comment.

2. Expiry re-inserts callbacks into the map it just cleared. Inline on src/models/RadioModel.cpp:1909. hasCommandPlane() is only a pointer check, so an expired callback that chains another sendCmd() lands a fresh entry in the just-cleared map. Measured on the createAudioStream() stream remove chain: pending goes 1 -> 1, not 1 -> 0. A central guard in sendCmd() is the right shape; four per-site guards is already one too many, as blocker 1 shows.

Nits — all explicitly non-blocking

1. The Principle I citation in the body is wrong, and the truth is a better argument for this PR. Inline on src/core/RadioConnection.cpp:279-280. FlexLib does not avoid this via a fresh reader lifetime — it keeps _tcpReadStringBuffer as a member of a long-lived object and explicitly clears it in Disconnect() at TcpCommandCommunication.cs:249, i.e. it does exactly what this patch does. Also the vendored reference is FlexLib_API_v4.1.5.39794; there is no 4.2.18 in the tree.

2. The same defect is unfixed in the sibling transport. WanConnection (SmartLink) runs its own identical line assembly — m_readBuffer appended at src/core/WanConnection.cpp:334 and consumed at :347-349 — and clears it in exactly one place, the overflow guard at :342. Neither connectToRadio() (:140-167, which resets m_wanHandle, m_validated, m_handle but not the buffer) nor disconnectFromRadio() (:169-184, which clears m_pendingCallbacks, m_seqCounter, m_handle, m_connected but not the buffer) touches it. And the object is reused across sessions — WanConnection m_wanConnection; is a by-value member of MainWindow (src/gui/MainWindow.h:1135), alive for the life of the process. So a WAN reconnect after a mid-line drop carries the same fabricated-response/zero-handle failure #5649 describes. Not a blocker and not for this PR — but it means the PR's unqualified "across reconnects" framing over-claims, and it is worth a follow-up issue. (This also corrects @aethersdr-agent's "no sibling copy of the parser left behind": the callback map is indeed separate, but the read buffer is not.)

3. m_handle = 0; in the synthetic-demo branch of disconnectFromRadio() (RadioConnection.cpp:292) is now dead — the resetSessionState() at the top of the function already zeroed it. Inline suggestion on :288 also pins down why the double reset is deliberate, so a future cleanup doesn't delete the load-bearing one.

4. tx_operation_integration_test.cpp asserts hasMultiFlexContinuation(radio) is still true after disconnect, which reads as endorsing a stale continuation surviving a session boundary rather than being indifferent to it. Copilot and @aethersdr-agent both flagged this; a comment saying which it means would settle it.

5. One prior nit I'd push back on. @aethersdr-agent notes the new test "never runs in CI" because every ctest in ci.yml is -R-filtered. The filter claim is accurate — the only -R steps are ci.yml:771, 784, 1126, 1141, and radio_connection_session_test matches none. But that is the project's design, not a gap: ci.yml's own header comment says so, registration in tests.cmake is the correct and complete step, and the sanitizer lane is what runs the suite. Nothing to do here.

What I tried to break, and couldn't

• The three mutation claims in the body — all three reproduce, independently. Reverting m_readBuffer.clear() fails 17 of 19 radio_connection_session_test checks; removing resetSessionState() from connectToHost() fails exactly newTcpSessionResetsOldBytes; removing expirePendingCallbacks() from onDisconnected() fails 7 pendingCallbackDisconnectExpiry checks. These are real guards, not implementation echoes. The four focused CTests the body names all pass on Linux / Qt 6.11.2 (radio_connection_session_test 19/19 in 4 ms).
• The demo session, driven for real — the smoke test the body says was attempted and not completed. Built the PR head, launched offscreen with AETHER_SETTINGS_DIR isolated and AETHER_AUTOMATION_NO_TX=1 (whoami confirms txAllowed: false), and attached the demo explicitly: get radio -> serial DEMO-0001, model "AetherSDR Demo", nickname "Simulator (not on the air)". Worth noting the isolation mattered — connect list did offer the operator's live FLEX-8600 at 192.168.50.100, and with a real settings store AutoConnectToLastRadio would have taken it. Nothing connected to it. Then, since this PR rewrites startSyntheticDemoConnect() and the demo branch of disconnectFromRadio(): three full disconnect/reconnect cycles, each returning connected=true serial=DEMO-0001 sliceCount=1 panCount=1 with no slice or pan accumulation; sim malformed (the parser path) left the session intact; sim disconnect (a radio-initiated drop) dropped to connectState: idle and reconnected cleanly. No registration failed, no command plane, or dropped-command lines in the log — only ALSA noise from the headless host. The demo half of this PR is fine.
• The demo path at the unit layer too, since the bridge cannot inject a truncated TCP line (the body is right about that): sim_backend_test — which includes an explicit testDisconnectAndReconnect() — and demo_backend_swap_test both pass on the PR head, and radio_connection_session_test::newDemoSessionResetsOldBytes drives startSyntheticDemoConnect() directly.
• resetSessionState() zeroing m_handle before emit disconnected(). New behaviour — onSocketDisconnected() previously left the dead handle readable. I walked all 280 lines of RadioModel::onDisconnected() (7673-7952): it never calls clientHandle(). No consumer of that edge reads it. Clean.
• A new data race on m_readBuffer. The PR adds writes to it from connectToHost() and disconnectFromRadio(), which are the two entry points called from outside the connection thread — so I checked every caller. All of them go through QMetaObject::invokeMethod (FlexBackend.cpp:85, SimBackend.cpp:188 and :428-429, RadioModel::closeConnectionForTerminalDisconnect), Blocking or Queued, so every resetSessionState() runs on the connection thread. m_handle is std::atomic<quint32> already. No new race.
• resetSessionState() at the top of disconnectFromRadio() zeroing the handle before teardown writes. writeDisconnectMarker() (:689-697) writes a bare \x04 and never reads the handle; gracefulDisconnect() takes its handle as a parameter. No ordering hazard.
• The two guards this PR does add stalling a connect when sendCmd answers kNoCommandPlaneCode synchronously. Both peekForMultiFlexConflictThen and disconnectClientHandlesThen are only reachable from registerAsGuiClient(), which early-returns unless m_connection && m_panStream — so hasCommandPlane() is true and the synchronous path can't fire. disconnectClient() (:3937) passes no continuation, so the guard is a no-op there. Survived.
• The sendCmd callback audit. Exactly four callbacks branch on code != 0. Two are guarded, one is blocker 1, and the fourth (client set local_ptt=1, :9206) only logs a warning — noisier on every disconnect now, but harmless.
• Whether the new test quietly touches the real settings store. It links aethercore without TestSettingsProfile.h, unlike its neighbours. Ran it under a scratch HOME: zero files created, and RadioConnection.cpp references AppSettings nowhere. Not a hazard.
• Copilot's RadioConnection.cpp:528 claim (a terminal socket error reaching UnconnectedState without emitting disconnected(), so callbacks never expire). I did not re-run @K5PTB's loopback probe; their result — the client emits errorOccurred while still ConnectedState and then disconnected(), so the expiry does run, and the UnconnectedState branch belongs to connects that never completed and have no pending callbacks — matches what I read of the code. Reported as read, not reproduced.

Verified empirically: the four focused tests, all three mutations, both blockers on the PR head, both blockers absent on merge base 85c9413b, the demo bridge session above, sim_backend_test / demo_backend_swap_test, the settings-isolation probe, and the socket-freeness of the new test. Built and run on Arch Linux, Qt 6.11.2, RelWithDebInfo, in a scratch worktree at 495a33dc. Read only: the WAN sibling (nit 2, traced through the code, not run — SmartLink needs hardware), Copilot's socket-error claim, and the macOS/Windows build claims in the body.

Recommendation

Request changes — on the RadioModel half only, agreeing with @K5PTB and @aethersdr-agent. The RadioConnection fix is well-targeted, genuinely tested, and matches what FlexLib itself does; I'd land it today. The disconnect-edge expiry is the right idea with too narrow an audit: it changes the contract for every sendCmd() caller, and the one call site it missed turns a recoverable 3-second blip into a terminal, reconnect-disabling error on the very path #5649 is about.

Concrete next step, either: (a) split the RadioConnection half out and land it now, leaving the callback-expiry work to its own PR; or (b) add both guards — the client gui early return and the central sendCmd()-during-expiry refusal — with a regression check for each, since the measured matrix shows they are independent and both needed. Option (b) with the central guard is my preference: it makes the invariant impossible to opt out of, and it lets the expiry code be something a callback can distinguish from a radio rejection.

Nice work on the transport injection, by the way — socketDescriptor() == -1 as a standing assertion is a good pattern and worth copying into the next socket-free test.

[aethersdr-agent]

Read the diff against current main. The core of this is right, and the two load-bearing pieces check out:

• Erase-before-invoke in the commandResponse lambda (RadioModel.cpp:1402) is the correct shape — the old it.value()(code, body); m_pendingCallbacks.erase(it); invalidates it if the callback synchronously disconnects or issues another command that rehashes the map. Moving the callback out and erasing first fixes both the double-expire and the iterator hazard.
• Branching on kNoCommandPlaneCode is safe: it's an explicit internal sentinel (RadioModel.cpp:68, 0x50000063, numbered alongside kProfileLoadSuppressedCommandCode), not a SmartSDR response code, so a real firmware reply can't be mistaken for a terminal drop and silently abort the subscription chain. That was my first worry and it doesn't hold.
• The test's transport interception is legitimate rather than a trick — QAbstractSocket::connectToHost(const QHostAddress&, ...) forwards to the virtual connectToHost(const QString&, ...) overload, so overriding the string form really does stop at the transport boundary with no descriptor opened. The socketDescriptor() == -1 assertions pin that.

Three things worth addressing, one of them a real (narrow) hazard:

1. m_multiFlexContinuation is not part of the disconnect reset — and this PR makes that reachable.

peekForMultiFlexConflictThen() stores the continuation at RadioModel.cpp:7175 and only clears it at :7207, :7241, :7294, :7309. onDisconnected() never touches it. Before this patch, a disconnect mid-probe let the sub radio all → sub client all chain run to completion with a nonzero code and clear the continuation on the way through (badly — that's part of what #5649 is about). With the new early returns the chain now stops before those clear points, so the saved continuation survives the drop.

Concrete failure: conflict dialog is up (multiFlexConflictDetected emitted at :7236), the TCP session drops while it's open, the operator then answers the dialog after auto-reconnect has re-established the session. resolveMultiFlexConflict() moves out the stale continuation and issues client disconnect 0x… plus registration against the new session, evicting a handle from the previous session's client list. Suggested fix, alongside the new expiry call:

void RadioModel::onDisconnected()
{
    resetTxOperations();
    (void)m_txCoordinator.acknowledgeStopped(m_txOperation);
    expirePendingCallbacks(QStringLiteral("the radio connection was disconnected"));
    // The saved continuation belongs to the probe of the session that just
    // ended. The chains that would have cleared it now stop on the terminal
    // code, so drop it here or a dialog answered after reconnect resumes
    // registration against a session it never inspected.
    m_multiFlexContinuation = nullptr;
    ...

2. Redundant reset at the head of disconnectFromRadio().

The leading resetSessionState() is immediately re-dirtiable: waitForDisconnected(2000) a few lines down pumps the event loop, so onReadyRead() can refill m_readBuffer and even parse a trailing H line before the socket actually closes. The tail call (and onSocketDisconnected()) is what makes this correct, so the head call buys nothing and reads as if it did. It also leaves m_handle = 0; in the demo branch dead. Not a bug — just consider dropping the head call and the now-redundant demo assignment so the invariant ("reset happens after the transport is down, and at connect-start") stays legible.

3. Pre-existing, but the new early return sits right on top of it.

disconnectClientHandlesThen() builds auto step = std::make_shared<std::function<void()>>(); then *step = [this, remaining, completion, step]() {...} (RadioModel.cpp:7143-7144). That self-capture is a shared_ptr cycle that is never broken — on the success path either, since the terminal branch at :7145 just returns with *step still holding a copy of step. Every invocation leaks the QList, the completion, and the lambda. Not a regression from this PR, and not in scope for #5649, but the terminal branch and the new kNoCommandPlaneCode return are exactly where it would be released:

        if (remaining->isEmpty()) {
            if (*completion) { /* … QTimer::singleShot … */ }
            *step = nullptr;   // break the self-referencing shared_ptr cycle
            return;
        }

One observation rather than a request: sendCmd() still inserts a pending callback whenever the Flex command plane exists, regardless of socket state (RadioModel.cpp:9772 only short-circuits on !hasCommandPlane()). expirePendingCallbacks() sweeps what was already pending at the disconnect edge, so a command issued after the drop still writes into a dead socket and waits until the next disconnect or teardownBackend(). That's the remaining half of this bug class; fine to leave for a follow-up, but worth naming in the PR body so it doesn't read as fully closed.

The validation write-up is unusually honest about what wasn't proven — the mutation testing (three mutations, each failing the matching regression) is the part that makes this reviewable, and the explicit non-claim on the demo smoke test is the right call rather than papering over the shared-slot timeout.

A maintainer can authorize the orchestrator to land items 1–3 by adding the aetherclaude-eligible label; item 1 is the only one I'd consider blocking.

73, Jeremy KK7GWY & Claude (AI dev partner)

---

_{🤖 aethersdr-agent · cost: $1.8750 · model: claude-opus-5}

[ten9876]

Approve — every finding from four review passes is fixed and pinned

Re-reviewed at 3f66334. The two blockers are gone, the non-blocking findings are addressed rather than deferred, and each fix now has a regression test that I confirmed fails when the fix alone is reverted.

Blockers — verified closed

1. Network blip during GUI registration no longer kills auto-reconnect. commandNeverReachedRadio(code) names the distinction between "the radio refused" and "the command never got there", and the client gui callback checks it before its rejection branch. The three sentinel comparisons already in the PR now use the same helper, so the concept is defined once.

Re-ran the probe that produced the original finding. Merge base 85c9413b: reconnect armed 1. PR head before the fix: 0. PR head now: 1 — back to correct, with no guiClientRegistrationFailed and no connectionError.

2. Expiry can no longer repopulate the map it is draining. Fixed centrally, which is what @aethersdr-agent, @K5PTB and I all argued for over a fourth per-site guard: expirePendingCallbacks() sets m_expiringPendingCallbacks (saved and restored, so a nested disconnect can't lift it early) and sendCmd() refuses while it is set. The createAudioStream() stream remove chain measured 1 → 1 before; it is now 1 → 0.

Non-blocking findings — all fixed

• The SmartLink sibling defect is fixed, not deferred. WanConnection now has resetSessionState() on all three edges — including onTlsDisconnected(), the radio-initiated drop, which previously reset nothing but m_connected while the object lives for the whole process as a by-value MainWindow member. The PR title's "across reconnects" is now accurate.
• MultiFLEX peek is session-scoped (Copilot). The 400 ms singleShot captures a session generation; onDisconnected() bumps it and drops the continuation. The test that asserted the stale continuation survives has been inverted — it was pinning the leak as correct.
• Synthetic-demo handshake timers are session-scoped (Copilot). They tested only m_syntheticDemo, which a fast reconnect sets straight back to true.
• Principle I citation corrected in both the code comment and the PR body. FlexLib does not get this from reader lifetime — TcpCommandCommunication keeps its line buffer as a member and clears it in Disconnect() (reference/FlexLib_API_v4.1.5.39794/FlexLib/TcpCommandCommunication.cs:249). Upstream is direct precedent for this fix, and this patch goes further by also resetting at the connect edge.
• Dead m_handle = 0 removed; the deliberate double resetSessionState() now records why it is not a duplicate.

Deliberately unchanged

• Copilot's RadioConnection.cpp:528 claim — @K5PTB's loopback probe showed the client emits errorOccurred while still ConnectedState and then disconnected(), so the expiry does run. That branch belongs to connects that never completed and carry no pending callbacks.
• "The new test never runs in CI" — true that ci.yml's ctest steps are -R-filtered and this target matches none, but that is the project's design (ci.yml's own header says so). tests.cmake registration is the correct and complete step.

Verification

Linux / Qt 6.11.2, RelWithDebInfo, scratch worktree at 3f66334.

Check	Result
radio_connection_session_test	20/20 pass
tx_operation_integration_test	pass
flex_backend_lifecycle_test, backend_family_switch_test	pass
sim_backend_test, demo_backend_swap_test	pass
All 7 Static Checks gates, run locally	pass

Mutation-tested — each fix reverted individually, leaving the others in place:

Fix reverted	Result
client gui guard + sendCmd() refusal	5 checks fail
demo session-generation guard	staleDemoTimersCannotReplayIntoANewSession fails
multiFLEX continuation clear	an expired client subscription drops the dead session's MultiFlex continuation fails

Not verified: the WanConnection change compiles and is reasoned from the code, but SmartLink needs hardware and was not exercised end-to-end. It is the same shape as the RadioConnection fix this PR already proves, on a path that had no reset at all.

Approving. Auto-merge armed once the build lanes finish.