fix(icom): harden session lifecycle and IC-9700 CI-V recovery

[w5jwp]

Summary

Fixes #5119.

This PR hardens the network-Icom command-plane lifecycle without changing healthy-session dispatch policy. It gives queued/in-flight CI-V work and wait-idle callers explicit terminal outcomes during reset, rejects frames from disconnected or superseded session generations before any scope/slice/transmit/meter publication, makes partial and ordinary session teardown deterministic, and adds bounded IC-9700-only serial-stream recovery after a confirmed CI-V stall while RS-BA1 transport remains alive. If the targeted IC-9700 recovery does not verify after three one-second attempts, that IC-9700 session is replaced. IC-705, IC-7300MK2, and unknown Icom models retain main's existing warn-only stall behavior.

Review requested from @jensenpat and @aethersdr-agent. I attempted to claim #5119 before submission as required by AGENTS.md, but GitHub rejected @me assignment for this contributor account with ReplaceActorsForAssignable; the issue was unassigned and no competing claim was present.

Maintainer and bot feedback alignment

@aethersdr-agent: scheduler work and waiters were stranded or falsely reported idle

• IcomCivScheduler::reset() now returns every queued/in-flight request with an explicit terminal result instead of silently discarding it.
• Backend waiters now resolve as completed, timed out, failed, or cancelled. Terminal paths snapshot diagnostics, reset scheduler state, update accounting, and only then emit results so a re-entrant observer cannot have newly queued work erased.
• Unexpected disconnect and backend destruction exercise explicit cancellation paths.
• Deterministic tests cover queued and in-flight cancellation, failed waiters, disconnect, and destruction.

@aethersdr-agent: late frames could publish after disconnect

• Every session connection captures a generation value.
• onCivFrame() rejects disconnected or mismatched generations at entry, before hardware-scope, slice, PTT, or meter decoding can emit state.
• Old session signals are disconnected during teardown.
• Tests inject stale frames across scope, slice, transmit, and meter paths and retain positive controls for the current generation.

@aethersdr-agent: teardown, partial login, and fake-radio coverage

• Teardown remains wholly backend/session-owned; there is no GUI change and no nested event loop.
• Token removal is tracked safely during partial login and established logout.
• Tests cover partial login, ordinary disconnect, application-style shutdown, reconnect, and deferred failure cleanup.
• The existing fake-radio harness was extended for CI-V silence and recovery observations instead of adding a parallel harness.

@aethersdr-agent VOX and polling observations, as narrowed by Pat's approval

• This lifecycle PR deliberately adds no VOX-delay poll or write and synthesizes no literal-zero cleanup value. Model-specific VOX-delay work remains separate.
• Existing RIT/XIT polling is retained. The earlier suggestion to remove it is superseded by Pat's frozen healthy-session contract.
• The recovery frequency probe is emitted only after an actual CI-V-only stall; it is not part of healthy polling.

@jensenpat: freeze the IC-705 / IC-7300MK2 scheduler contract

• No changes were made to slot duration, read timeout, the single reply-bearing in-flight slot, priority/aging, semantic generations, coalescing, confirmation reads, or late/stale handling.
• Exact startup and steady-state request sequence/count/cadence tests were added for both IC-705 and IC-7300MK2, including their model-specific SET items and existing RIT/XIT reads.
• Ordinary traffic continues through IcomCivScheduler; emergency unkey ordering is preserved.
• Simulated CI-V stalls on IC-705 and IC-7300MK2 issue no IC-9700 restart packet and retain main's established warn-only behavior.

@jensenpat: lifecycle, state, and transmit boundaries

• The generation guard precedes every affected inbound publication.
• Recovery never keys or re-keys the transmitter. Teardown retains emergency unkey before the serial path is removed.
• No GUI, Flex, HL2, or Sim source or behavior is changed.
• The IC-9700 targeted restart is bounded to three attempts at one-second cadence and then falls back to full reconnect.

Pat's Discord concern: do not disturb known-good IC-705 / IC-7300MK2 timers

The observed failure was specifically a 5–6 second loss of CI-V replies on the IC-9700 while RS-BA1 packet counters continued and the scheduler queue grew. This PR does not infer that every Icom needs different scheduling. The serial data-stream restart (0x04) is gated to CI-V address 0xA2 (IC-9700), and explicit IC-705/IC-7300MK2 tests prove those models never send it. Shared healthy scheduling remains unchanged.

Public protocol provenance for the 0x04 CI-V data-start request

Icom's public RS-BA1 documentation confirms that the control, serial, and audio transports use UDP ports 50001, 50002, and 50003, but it does not publish the proprietary OpenClose packet layout or assign a meaning to byte 0x04. This PR therefore does not describe 0x04 as vendor-documented. Its clean-room provenance is the following public implementation and operational evidence:

1. wfview's Icom UDP CI-V implementation constructs the 22-byte OpenClose envelope with 0x04 as data start and 0x00 as close. Its CI-V watchdog resends the same data-start request after two seconds without CI-V data and stops when valid CI-V data resumes. Git history traces this behavior to wfview's public May 2022 transport implementation.
2. RigPlane Core's Icom LAN transport independently implements the same 22-byte envelope: 0x01C0 at offset 0x10, 0x04 at offset 0x15 for open/start, and 0x00 for close. Its published reliability semantics document that its two-second CI-V watchdog resends OpenClose to restart the stream.
3. A published wfview operational log from a physical IC-9700 shows that watchdog detecting CI-V silence and requesting data start on that model. The log does not expose the raw byte by itself; wfview's source above supplies the byte-to-operation mapping.
4. The Icom RS-BA1 Version 2 instruction manual is retained as the vendor source for the three UDP transports and their default ports, not for the undocumented OpenClose field.

RigPlane explicitly credits wfview for the watchdog behavior, so it is a second public implementation and interoperability reference, not a claim of a second independent protocol discovery. Together, these sources establish an open-source precedent plus physical IC-9700 operational evidence. They do not establish that Icom publicly documented the OpenClose field.

Independent review follow-ups

• Recovery verification now requires the selected radio's source address, a frequency-reply shape, and a scheduler observation that is not Stale. A reply arriving after the 350 ms matching window is Unmatched but remains radio-authoritative; a deterministic 500 ms regression test covers that exact case.
• The fake radio remains CI-V-silent until receipt of the 0x04 data-start packet, so recovery is now causally exercised rather than enabled manually by the test.
• Non-IC-9700 stalls preserve main's warn-only contract; IC-705, IC-7300MK2, and unknown-model tests pin both continued connection and absence of 0x04.
• Scheduler termination now resets before direct callback emission, with a regression test proving a waiter registered re-entrantly from the terminal callback survives.
• The retry-cadence assertion retains its meaningful lower bound and removes the loaded-runner upper bound; scheduler timeout-report accounting is reset with scheduler statistics.
• civ.scheduler.wait-idle now exposes the automation-visible keys outcome, failed, cancelled, cancelledRequests, and failedRequests; the existing timedOut key retains its prior semantics. Startup CI-V-address adoption deliberately terminates pending waiters because it replaces the scheduler's destination/generation rather than reporting that pre-adoption work as idle.
• Direct backend-destruction cancellation coverage was added.
• Cross-model tests prove the 0x04 stream restart is IC-9700-only.
• Tests pin the exact three-attempt limit and one-second retry cadence.

Scope boundaries and residual risk

• Only src/core/backends/icom/ and Icom test sources change.
• FM repeater/CTCSS behavior is not part of this lifecycle PR.
• CI-V provides no transaction identifier, so recovery correlation cannot be cryptographically unique; selected-radio source, frequency-reply shape, and rejection of scheduler-Stale observations constrain acceptance. An unsolicited frequency frame from that selected radio also demonstrates that its command plane is alive; another device on a shared bus cannot verify recovery.
• Final validation is deterministic fake-radio/unit coverage plus the original IC-9700 stall logs. The final rebased commit has not yet been exercised against physical IC-705 or IC-7300MK2 hardware; their risk is controlled by byte-sequence/count/cadence regression tests.

Constitution principle honored

Principle XI — Fixes Are Demonstrated. Each reported lifecycle failure has deterministic regression evidence, including cancellation outcomes, stale-generation suppression, partial teardown, transport-alive/CI-V-silent recovery, bounded fallback, and unchanged healthy IC-705/IC-7300MK2 traffic.

Principle II — Radio-authoritative live state. A disconnected or superseded session can no longer publish stale slice, transmit, meter, or scope state as current.

Principle IX — Evidence Over Assertion. The compatibility claim is backed by exact startup/steady-state wire inventories and explicit non-IC-9700 recovery tests rather than model-name assumptions alone.

Test plan

• Local build passes (cmake --build build)
• Behavior verified on a real radio if applicable — extensive physical IC-9700 connect/disconnect, PTT, shutdown, transmit, and spontaneous stall/reconnect testing established the lifecycle behavior and failure shape. The exact delayed-reply recovery edge is additionally deterministic in the fake-radio test; continued hardware soak remains appropriate for an intermittent fault.
• Existing focused tests pass locally:
  • icom_civ_scheduler_test
  • icom_protocol_test
  • icom_session_test
  • icom_backend_test
  • cwx_panel_test after rebasing Pat's fix(gui): prevent Icom CWK updates from stealing VFO focus #5134 fix
• Reproduction steps documented in Icom: harden session lifecycle and CI-V stall recovery #5119 and represented by the transport-alive/CI-V-silent fake-radio test
• git diff --check
• tools/check_test_registration.py --strict
• tools/check_engine_boundary.py --strict (no blockers; existing baseline warnings only)

Checklist

• Commits are signed (docs/COMMIT-SIGNING.md)
• No new flat-key AppSettings calls — no settings changes
• Code is clean-room — based on documented CI-V/RS-BA1 behavior and observed logs, not proprietary binary analysis (Principle IV)
• All meter UI uses MeterSmoother — not applicable; no meter UI changes
• Documentation updated if user-visible behavior changed — not applicable; backend reliability fix is fully documented here and CHANGELOG.md is intentionally untouched
• Security-sensitive changes reference a GHSA if applicable — not applicable
  \n\n### Maintainer adaptation after Icom: add evidence-backed model capability profiles #5151\n\nSuperseding the earlier address-gate wording above: this branch is rebased onto current main and the RS-BA1 data-pipe restart, scheduler termination, and bounded retry cadence are now gated by the authoritative IC-9700 CivDataRestart capability profile from Icom: add evidence-backed model capability profiles #5151. Only that profile carries the 3-attempt/1-second recovery facet. IC-705, IC-7300MK2, and unprofiled models declare no recovery capability, so their stall path stays diagnostic-only: no 0x04 command, recovery state, scheduler reset/failure, connection replacement, or link-timer change. Focused protocol/scheduler/meter/family/backend regressions pass 5/5. The timing-sensitive icom_session_test retains the current-main early-renewal baseline failure; no timer was changed to mask it.

[aethersdr-agent]

Issue fit

Partially. The lifecycle half of #5119 lands well: reset() now returns per-request terminal dispositions, waiters resolve as completed/timed-out/failed/cancelled, the session-generation gate sits ahead of every scope/slice/PTT/meter publication, and there are deterministic tests for cancellation, stale generations, partial login and app shutdown. Two acceptance criteria are not met by this diff: the VOX capture/restore criterion (explicitly deferred — fine, but the issue box stays unchecked), and "Poll inventory and cadence are documented before/after" is satisfied by tests rather than documentation (acceptable). The IC-9700 recovery half is where the problems are — see Blockers 1 and 2.

All findings below are reasoned from the head checkout at /tmp/aetherclaude/pr-5145; I have no build and ran nothing.

Scope

File / group	What it changes	Claimed?	Verdict
IcomCivBackend.{cpp,h}	generation gate, terminateScheduler, waiter outcomes, recovery state machine	yes	in scope
IcomCivScheduler.{cpp,h}	reset(TerminalOutcome) -> ResetResult; dispatch policy untouched (verified: takeNext/observe/priorityFor unchanged)	yes	in scope
IcomProtocol.{cpp,h}	new wire packet buildSerialRestart (magic 0x04)	yes	needs maintainer decision — new outbound protocol verb, no cited source (Blocker 2)
IcomSession.{cpp,h}	reopenCivPipe(); seq/sendTracked usage matches the existing open at IcomSession.cpp:593	yes	in scope
schedulerDiagnostics() keys	adds cancelledRequests, failedRequests, outcome, failed, cancelled to civ.scheduler.wait-idle results	not in body	nit — automation-visible surface; timedOut semantics verified backward-compatible
tests (5 files)	new coverage + deletion of the deaf-radio stall block	deletion not disclosed	see Blocker 1

Nothing unrelated to Icom is touched. CHANGELOG.md correctly untouched.

Blockers

1. A probe reply slower than 350 ms can never verify recovery — so a radio that has recovered gets force-disconnected. (inline: IcomCivBackend.cpp:1431)

IcomCivScheduler::observe() calls expireRead(nowMs) on entry (IcomCivScheduler.cpp:242), and expireRead retires the in-flight read at kReadTimeoutMs = 350 (IcomCivScheduler.h:113, IcomCivScheduler.cpp:107). A matching reply arriving after that falls into the m_expired branch and, with no newer generation behind it, returns Observation::Unmatched — not Accepted (IcomCivScheduler.cpp:255-271). The file's own comment at IcomCivScheduler.cpp:114 spells this out: the same frame is "Stale at 349 ms and unmatched-therefore-authoritative at 351 ms," and the backend adopts Unmatched replies as truth everywhere else.

So: IC-9700 stalls, the 0x04 restart works, the radio answers cmdReadFrequency in 400 ms — an unremarkable latency for a pipe that just came back — observation == Unmatched, the gate at line 1431 fails, m_civRecoveryStartedAtMs stays set. Because the recovery block early-returns at the top of onLinkTick (IcomCivBackend.cpp:4396), no ordinary polling runs either, so nothing else can clear it. Three attempts later the backend tears down a session whose command plane is demonstrably alive. The feature converts a recovered link into a dropped one.

The test does not catch this because c.radio.setCivSilent(false) makes the fake radio answer in microseconds. Suggested fix: Accepted is not the right predicate — Stale is the only outcome that means "superseded by newer intent", so gate on observation != IcomCivScheduler::Observation::Stale.

2. The 0x04 recovery packet has no cited source and has never been sent to a radio. (inline: IcomProtocol.cpp:539)

IcomProtocol.h asserts "The IC-9700 distinguishes this data-start request (magic 0x04) from the initial open above (magic 0x05)" with no spec page, no capture, no public reference. I grepped the tree: no other occurrence of a serial-stream 0x04 magic exists (IcomSession.cpp:142 documents type 0x05 only). Principle IV's clean inputs are "public documentation, open-source references, behavior observed on the wire" — this cites none of the three, and the PR's own "Behavior verified on a real radio" box is unchecked.

The tests cannot close the gap, because the evidence is circular in two directions:

• icom_protocol_test.cpp:326 asserts restart[0x15] == 0x04 — the encoder asserted against itself.
• IcomFakeRadio.h was taught by this PR that 0x04 means "restart", and the handler only timestamps it — it does not clear m_civSilent or resume CI-V. Recovery "succeeds" in icom_backend_test.cpp only because the test calls setCivSilent(false) by hand. The suite therefore proves the retry bookkeeping, not that the packet does anything.

This is a maintainer call, not a correctness claim on my part — but "we send a new, undocumented packet to real IC-9700 hardware on a stall" should be ratified with a provenance citation or a wire capture before merge, not inferred from a green fake-radio run.

3. Every Icom model now force-disconnects on 5 s of CI-V silence — and the PR describes this as pre-existing. (inline: IcomCivBackend.cpp:4533)

I diffed main: onLinkTick ends immediately after the CI-V STALL warning. There is no stall-triggered disconnect on main for any model. The PR body calls the non-IC-9700 path "the established full-session reconnect path" and "the existing full-session reconnect ... remains the fallback" — that path is introduced here. The diff also deletes the test block that pinned the old contract, including its comment:

"and isConnected() still reports true — the lie the detector exists to break"

Escalating a diagnostic warning to a teardown + RadioModel::onConnectionError reconnect timer (RadioModel.cpp:1431) is a defensible design, and I could not construct a false-positive: FB/FA acks, meter replies and the 1 s control poll all refresh m_lastInboundCivAtMs, so 5 s of true silence really does mean a dead command plane. But a change that turns "one log line" into "the operator's session drops" for IC-705 / IC-7300MK2 / IC-7760 needs to be stated as such in the body rather than framed as existing behavior, and the deleted assertion is a contract the maintainer should knowingly retire.

Nits (non-blocking)

• IcomCivBackend.cpp:2562 — terminateScheduler emits waiter results before reset(). extensionResult is a direct connection; a slot that re-enters and queues scheduler work has it silently wiped by the reset on the next line. The previous order was reset-then-service. Inline comment has a suggested reorder.
• adoptReportedCivAddress (IcomCivBackend.cpp:854, :910) now cancels pending wait-idle waiters during ordinary startup address adoption, not just teardown. Arguably right, but it is a new answer to an automation caller and isn't mentioned anywhere.
• icom_backend_test.cpp:2006 — elapsed >= 900 && elapsed <= 1300 is a wall-clock window around a 1000 ms QTimer in a test that also runs three 9–12 s waits. A loaded CI runner can easily push one tick past 1300 ms. Consider asserting only the lower bound (cadence is not faster than configured) and dropping the upper.
• terminateScheduler zeroes m_stats via reset() but leaves m_schedulerTimeoutsReported at its old high-water mark, so the "CI-V read timeout" warning stays suppressed after a stall until the fresh counter climbs back past it. disconnectRadio() resets it; the stall path doesn't.
• ~IcomCivBackend() emits extensionResult from the destructor body. Legal (the object is still a QObject), and the new test relies on it — but a receiver that calls back into the backend lands in a half-destroyed object. Worth a comment at minimum.
• m_civRecoveryProbeSent is set at both queue sites and cleared only on verify/teardown; it is never observable as false while m_civRecoveryStartedAtMs > 0. Redundant state.

What I tried to break (and could not)

• The 0xA2 model gate. Grepped IcomModels.cpp — 0xA2 is unique to IC-9700 in the table, and IcomModels.cpp:265 already uses civAddress == 0xA2 as the IC-9700 discriminator, so this matches house idiom. Brittle if a future model reuses the address, but correct today.
• buildSerialRestart bounds. framed(kLenOpenClose, …) yields a 0x16-byte packet, so packet[0x15] is the last valid index; seq/sendTracked usage is identical to the existing open at IcomSession.cpp:593.
• timedOut regression for existing wait-idle callers. Walked both non-terminal branches: idle → Completed → false, !idle → TimedOut → true. Byte-for-byte the old semantics.
• Null derefs on the new paths. m_model is &unknownModel() from the constructor (IcomCivBackend.cpp:86), so the m_model-less test fixture is safe through the scope branch; m_session is null-guarded in the recovery-candidate expression and non-null by the onLinkTick prologue.
• Reentrancy of disconnectRadio() from onLinkTick(). The slot only emits after the call and touches no session state; !m_connected gates subsequent ticks.
• Retry coalescing. The comment claims pumpCiv retires the unanswered probe first — correct: 1000 ms retry interval > 350 ms read timeout, so expireRead fires and enqueue issues a fresh generation. The old late reply then correctly classifies Stale.
• The "another device cannot verify the probe" claim. True as written (frame.from check). Note an unsolicited Transceive broadcast from this radio can verify it — which is the correct outcome anyway, so not a finding.
• "Healthy dispatch policy unchanged." Read takeNext, observe, matches, priorityFor, enqueue against main: only reset() changed. The claim holds.

Recommendation

Request changes. Blocker 1 is a concrete correctness defect that inverts the feature's purpose on exactly the hardware it targets, and it is masked by a fake radio that answers instantly — a one-line predicate change plus a test that delays the probe reply past 350 ms would settle it. Blockers 2 and 3 are maintainer decisions rather than my rulings: an unsourced new outbound packet, and a real behavior change for three shipping models presented as pre-existing. The lifecycle work (generations, terminal outcomes, teardown determinism) is genuinely good and I'd take it; if the IC-9700 recovery state machine needs another round, splitting it out would let the lifecycle half land now.

Nice work on the generation gate in particular — the positive controls in testStaleSessionFrameIsDropped are exactly what makes that test mean something.

---

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

[w5jwp]

@jensenpat @aethersdr-agent — review fixes are now pushed in signed commit d1f2bc9e; please re-review.

Alignment with Pat's compatibility requirement and the bot findings:

• IC-705, IC-7300MK2, and unknown Icom models retain main's warn-only CI-V-stall behavior. They neither receive the 0x04 data-start request nor a newly introduced stall disconnect.
• IC-9700 recovery remains model-gated to 0xA2, bounded to three attempts, and falls back to replacing only that IC-9700 session.
• A selected-radio frequency reply delayed 500 ms—past the scheduler's 350 ms matching window—now verifies recovery when authoritative (Unmatched) and is rejected only when Stale.
• The fake radio now resumes CI-V because it receives 0x04; the test no longer manually clears silence, making recovery causal.
• The PR body now cites public wfview and RigPlane OpenClose implementations plus a published physical IC-9700 watchdog log. It explicitly states that Icom documents the UDP transports but not this packet field, and that RigPlane credits wfview rather than representing a second independent discovery.
• Scheduler termination snapshots diagnostics, resets state and accounting, then emits direct callback results. A re-entrant waiter regression proves newly registered work is not erased.
• The timeout-report high-water mark resets with scheduler statistics, redundant recovery state was removed, and the retry-cadence test no longer has a loaded-runner upper bound.
• The PR body discloses the new automation-visible waiter-result keys and the intentional terminal result during startup CI-V-address adoption.
• The real-radio checkbox now reflects the extensive physical IC-9700 connect/disconnect, PTT, shutdown, transmit, and spontaneous stall/reconnect testing performed throughout bring-up, while retaining the need for continued soak of this intermittent condition.

Validation after these changes:

• icom_protocol_test, icom_civ_scheduler_test, icom_session_test, and icom_backend_test: 4/4 passed
• Full AetherSDR target build passed
• git diff --check passed
• strict test-registration and engine-boundary checks passed with no blockers

Each inline review thread also has a specific response tied to its fix and regression evidence.

[jensenpat]

Reviewed exact head 12f96e5 against current main after the #5151 capability-profile rebase.

The IC-9700 RS-BA1 data-pipe restart, scheduler termination, and bounded 3-attempt/1-second retry policy are now one CivDataRestart capability. IC-705, IC-7300MK2, and unprofiled models have no recovery facet; regression coverage pins no 0x04 command, no recovery state, no scheduler reset/failure, continued connection, and unchanged link-timer cadence. Healthy startup/poll inventory and shared scheduler dispatch constants remain unchanged.

All required CI checks pass, all commits are GitHub-verified, review threads are resolved, and the exact-head synthetic merge succeeds. No new live-radio run was performed on this rebased head; RF/runtime evidence remains the contributor's published IC-9700 stall/recovery context plus deterministic fake-radio coverage.