From 67f6d2506ee14f83794a6db43a35754a578b37ea Mon Sep 17 00:00:00 2001 From: Simon Keimer Date: Tue, 8 Sep 2026 16:05:40 +0200 Subject: [PATCH] fix(daemon): a repeater that is not running is no longer reported as running (#1298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `EnableRepeater` thread `take()`s the `CrossBandRepeater` out of the runtime state, so when that thread exits the repeater is gone. Nothing observed it: `repeater_enabled` stayed true, `EnableRepeater` answered "already enabled" forever, and no command sequence could get back to a truthful state. The no-repeater arm was worse: it logged a warning, then set `repeater_enabled = true` and emitted `RepeaterChanged { enabled: true }` regardless — so a daemon with nothing to run reported a running repeater. Now: enabling with nothing to run returns a `CommandError` naming why; a thread that has exited is reaped at the top of the repeater commands; and the thread reports its own unexpected exit, on the error path only, since a clean stop is already reported by `DisableRepeater`. `apply_repeater_enable_disable_emits_state_changes` was pinning the defect — it ran with `repeater: None` and asserted that enabling SUCCEEDED. It now builds a real repeater so the success path is the one it tests. The issue's own premise was corrected in the process: it claimed the loop exits on the first non-decoding capture, making enabled-forever the normal state on a rig. #1300 changed that. The normal state on a rig is a live thread that never relays and never says so above DEBUG — which is why this lands before #1297, where "no relay" and "thread dead" would otherwise be the same observation. Implements: REQ-OBS-02 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0188ATCj6DZ9aRVQ2vSirua6 --- CLAUDE.md | 1 + crates/openpulse-daemon/src/lib.rs | 216 +++++++++++++++++++++++++++-- docs/dev/project/traceability.md | 39 ++++++ 3 files changed, 244 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 94e3de57..041c3b90 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -238,6 +238,7 @@ Each requirement below is done when the linked test passes. Add new links as tes | No relay-path error leaves rig_b keyed **while the daemon is alive**: every path is an RAII guard under a watchdog, the §97.119 ID rides the frame's key instead of asserting a second one underneath it, and a full-duplex hold is bounded by **silence** (each relayed frame re-stamps the deadline) rather than held from session start. The watchdog is in-process, so it bounds a hang, NOT a dead daemon — on rigctld/CM108/GPIO nothing releases rig_b if the process dies, which is why the hold is no longer eager. The ID gate decodes `DE N0CALL` off the transmit side; the edge count it replaced was a proxy that passed while the double-key shipped. **Loopback tier only — the repeater cannot receive a frame on hardware at all (#1297)** | `cargo test -p openpulse-repeater --no-default-features --no-fail-fast` + `cargo test -p openpulse-radio --no-default-features --lib shared_ptt` | | `[radio.rig_b]` cannot alias the rigctld the main rig already uses — the daemon refuses to start **when the repeater is enabled at startup** (otherwise it warns and builds no repeater). Two controllers over one transmitter key and release each other, and #1263's refusal rule reaches only *within* one `SharedPtt`. Not exotic: both `RigConfig::default()` and `RadioConfig::default()` carry `127.0.0.1:4532`, so an empty `[radio.rig_b]` header IS the collision — and the shared endpoint is rigctld itself, so `cat_backend = "rigctld"` alone collides even with a non-rigctld `ptt_backend`. Scoped to string equality: it catches the shipped defaults, not `localhost` vs `127.0.0.1` | `cargo test -p openpulse-daemon --no-default-features --lib repeater_rig_b_tests` | | A **cap-flushed** burst is not evidence about the rate ladder (#1255) — the cap exceeds the longest candidate frame, so hitting it means the carrier was still up and the slab is not one transmission. A failed decode of one must not key an ACK or move `recommended_level`. The decode itself still runs: when the squelch sits below the band floor EVERY burst is a cap flush (#1254's regime), so skipping it would make the daemon deaf on a hot band — pinned by a control that decodes a frame at the head of a capped slab. Runs ~70 s, dominated by one `ota_decode_burst` over the candidate rungs | `cargo test -p openpulse-modem --no-default-features --test cap_flush_is_not_ladder_evidence` | +| A repeater that is not running is not reported as running (#1298) — enabling with nothing to run FAILS with a reason instead of emitting `RepeaterChanged { enabled: true }`, and a thread that exited is reaped so the next command sees the truth rather than "already enabled" forever. The thread OWNS the `CrossBandRepeater`, so its exit means the repeater is gone | `cargo test -p openpulse-daemon --no-default-features --lib command_apply_tests` | | `openpulse-kiss`'s `SharedPtt` has a **watchdog thread**, so its deadline is enforced — the crate built one and called `spawn_watchdog` nowhere, leaving `force_release_if_expired` with no caller in the crate. The guard covers an early return and an unwind; it cannot reach a transmit that BLOCKS, which is the case the watchdog exists for. Driven through the real constructor, since the defect was the wiring | `cargo test -p openpulse-kiss --no-default-features --test ptt_keys_every_transmit` | | `openpulse-mesh` has no route to a sound card — it beacons and relays automatically with no PTT controller, no carrier sense and no station-ID timer, and its beacon carries no callsign field, so the capability was REMOVED rather than guarded (a fourth hand-rolled keying path on a crate with no §97.221 mapping, no control point and no on-air record). Each check is validated against a planted input | `cargo test -p openpulse-mesh --no-default-features --test no_real_audio` | | A CONACK cannot select a signing mode the CONREQ never offered (F-1147-05 — v1 checked local policy only) | `cargo test -p openpulse-core --no-default-features --test handshake_integration conack_rejected_when_mode_not_offered` | diff --git a/crates/openpulse-daemon/src/lib.rs b/crates/openpulse-daemon/src/lib.rs index 986c6781..8cb46304 100644 --- a/crates/openpulse-daemon/src/lib.rs +++ b/crates/openpulse-daemon/src/lib.rs @@ -1609,6 +1609,39 @@ pub(crate) enum KeyedTxError { Transmit(openpulse_core::error::ModemError), } +/// Clear repeater state whose thread has already exited (#1298). +/// +/// The `EnableRepeater` thread OWNS the `CrossBandRepeater` — it is `take()`n out of the runtime +/// state — so when that thread exits the repeater is gone for good. Nothing observed that: +/// `repeater_enabled` stayed `true`, `EnableRepeater` answered "already enabled", and a +/// `DisableRepeater` + `EnableRepeater` pair reported success while starting nothing. +/// +/// Called at the top of both repeater commands rather than from a periodic tick: the events emitted +/// by the thread itself are what notify clients promptly, and this only has to make the *next* +/// command see the truth. A tick poll would need `runtime_state` on the tick path for no added +/// signal. +fn reap_finished_repeater( + runtime_state: &mut RuntimeControlState, + event_tx: &Arc>, +) { + let finished = runtime_state + .repeater_thread + .as_ref() + .is_some_and(|t| t.is_finished()); + if !finished { + return; + } + if let Some(t) = runtime_state.repeater_thread.take() { + let _ = t.join(); + } + runtime_state.repeater_stop = None; + if runtime_state.repeater_enabled { + runtime_state.repeater_enabled = false; + tracing::warn!("cross-band repeater thread has exited; marking the repeater disabled"); + let _ = event_tx.send(ControlEvent::RepeaterChanged { enabled: false }); + } +} + pub(crate) fn keyed_transmit( ptt: &crate::ptt::SharedPtt, event_tx: Option<&broadcast::Sender>, @@ -2567,6 +2600,7 @@ pub async fn apply_command_to_engine( } } ControlCommand::EnableRepeater => { + reap_finished_repeater(runtime_state, event_tx); if runtime_state.repeater_enabled { let _ = event_tx.send(ControlEvent::CommandError { command: "enable_repeater".to_string(), @@ -2575,24 +2609,46 @@ pub async fn apply_command_to_engine( return; } - if let Some(mut repeater) = runtime_state.repeater.take() { - let stop = Arc::new(AtomicBool::new(false)); - let stop_clone = Arc::clone(&stop); - let thread = std::thread::spawn(move || { - if let Err(e) = repeater.run_full_duplex(stop_clone) { - tracing::warn!(error = %e, "cross-band repeater exited with error"); - } + // No repeater to run is a FAILED enable, not a quiet one (#1298). This arm used to warn + // at `tracing::warn!` and then set `repeater_enabled = true` and emit + // `RepeaterChanged { enabled: true }` anyway — so a daemon with no repeater reported one + // as running, and no command sequence could get back to a truthful state. + let Some(mut repeater) = runtime_state.repeater.take() else { + let _ = event_tx.send(ControlEvent::CommandError { + command: "enable_repeater".to_string(), + reason: "no repeater is available — it was not built at startup (see the \ + startup log for why), or a previous session ended and consumed it" + .to_string(), }); - runtime_state.repeater_stop = Some(stop); - runtime_state.repeater_thread = Some(thread); - } else { - tracing::warn!("enable_repeater: no pre-built repeater in runtime state; audio routing not started"); - } + return; + }; + + let stop = Arc::new(AtomicBool::new(false)); + let stop_clone = Arc::clone(&stop); + // The thread owns the repeater, so when it exits the repeater is GONE. Report that: + // without it, "relaying nothing because the band is quiet" and "the thread died" are + // the same observation from outside (#1298). + let thread_tx = Arc::clone(event_tx); + let thread = std::thread::spawn(move || { + if let Err(e) = repeater.run_full_duplex(stop_clone) { + tracing::warn!(error = %e, "cross-band repeater exited with error"); + let _ = thread_tx.send(ControlEvent::CommandError { + command: "repeater".to_string(), + reason: format!("cross-band repeater stopped: {e}"), + }); + // Only on the error path: a clean stop is already reported by DisableRepeater, + // and emitting there too would put two `false` edges on one transition. + let _ = thread_tx.send(ControlEvent::RepeaterChanged { enabled: false }); + } + }); + runtime_state.repeater_stop = Some(stop); + runtime_state.repeater_thread = Some(thread); runtime_state.repeater_enabled = true; let _ = event_tx.send(ControlEvent::RepeaterChanged { enabled: true }); } ControlCommand::DisableRepeater => { + reap_finished_repeater(runtime_state, event_tx); if !runtime_state.repeater_enabled { let _ = event_tx.send(ControlEvent::CommandError { command: "disable_repeater".to_string(), @@ -4334,6 +4390,24 @@ mod command_apply_tests { } } + /// Build a real, runnable repeater so `EnableRepeater` has something to start. + fn test_repeater(enabled: bool) -> openpulse_repeater::CrossBandRepeater { + let mk = || { + let mut e = ModemEngine::new(Box::new(openpulse_audio::LoopbackBackend::new())); + let _ = e.register_plugin(Box::new(bpsk_plugin::BpskPlugin::new())); + e + }; + openpulse_repeater::CrossBandRepeater::new( + Box::new(openpulse_radio::NoOpPtt::new()), + mk(), + mk(), + openpulse_repeater::RepeaterConfig { + enabled, + ..Default::default() + }, + ) + } + #[tokio::test] async fn apply_repeater_enable_disable_emits_state_changes() { let mut engine = test_engine(); @@ -4341,6 +4415,9 @@ mod command_apply_tests { let (tx, mut rx) = broadcast::channel::(16); let ev_tx = Arc::new(tx); let mut runtime_state = RuntimeControlState::default(); + // #1298: this test used to run with `repeater: None` and assert that enabling SUCCEEDED — + // it was pinning the defect. A daemon with nothing to run must not report a running repeater. + runtime_state.repeater = Some(test_repeater(true)); apply_command_to_engine( &ControlCommand::EnableRepeater, @@ -4373,6 +4450,121 @@ mod command_apply_tests { } } + /// THE #1298 GATE (enable half): with nothing to run, enabling must FAIL and say so. + /// + /// The old arm logged a `warn!`, then set `repeater_enabled = true` and emitted + /// `RepeaterChanged { enabled: true }` regardless — so a daemon with no repeater reported one as + /// running, and no command sequence could get back to a truthful state. + #[tokio::test] + async fn enabling_a_repeater_that_does_not_exist_fails_instead_of_claiming_success() { + let mut engine = test_engine(); + let active_mode: SharedMode = Arc::new(Mutex::new("BPSK250".to_string())); + let (tx, mut rx) = broadcast::channel::(16); + let ev_tx = Arc::new(tx); + let mut runtime_state = RuntimeControlState::default(); + assert!(runtime_state.repeater.is_none(), "premise: nothing to run"); + + apply_command_to_engine( + &ControlCommand::EnableRepeater, + &mut engine, + &active_mode, + &ev_tx, + None, + &mut runtime_state, + ) + .await; + + assert!( + !runtime_state.repeater_enabled, + "the daemon reports a repeater as enabled while none exists" + ); + match rx.recv().await.expect("expected an event") { + ControlEvent::CommandError { command, reason } => { + assert_eq!(command, "enable_repeater"); + assert!( + reason.contains("no repeater is available"), + "unhelpful reason: {reason}" + ); + } + other => { + panic!("expected CommandError, got {other:?} — a RepeaterChanged here is the bug") + } + } + } + + /// THE #1298 GATE (reap half): a thread that has exited must not leave the repeater "enabled". + /// + /// The thread OWNS the `CrossBandRepeater`, so its exit means the repeater is gone. Nothing + /// observed that: `EnableRepeater` answered "already enabled" forever after. + #[tokio::test] + async fn a_repeater_thread_that_exited_is_reaped_rather_than_reported_enabled() { + let mut engine = test_engine(); + let active_mode: SharedMode = Arc::new(Mutex::new("BPSK250".to_string())); + let (tx, mut rx) = broadcast::channel::(64); + let ev_tx = Arc::new(tx); + let mut runtime_state = RuntimeControlState::default(); + // A DISABLED repeater's `run_full_duplex` returns Ok(0) at once, so the thread exits + // immediately — standing in for any exit the daemon did not ask for. + runtime_state.repeater = Some(test_repeater(false)); + + apply_command_to_engine( + &ControlCommand::EnableRepeater, + &mut engine, + &active_mode, + &ev_tx, + None, + &mut runtime_state, + ) + .await; + assert!(runtime_state.repeater_enabled, "enable succeeded"); + + // Let the thread finish. + for _ in 0..200 { + if runtime_state + .repeater_thread + .as_ref() + .is_some_and(|t| t.is_finished()) + { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + } + + // The next command must see the truth rather than "already enabled". + apply_command_to_engine( + &ControlCommand::EnableRepeater, + &mut engine, + &active_mode, + &ev_tx, + None, + &mut runtime_state, + ) + .await; + + let mut saw_disabled_edge = false; + let mut saw_already_enabled = false; + while let Ok(ev) = rx.try_recv() { + match ev { + ControlEvent::RepeaterChanged { enabled: false } => saw_disabled_edge = true, + ControlEvent::CommandError { ref reason, .. } + if reason.contains("already enabled") => + { + saw_already_enabled = true + } + _ => {} + } + } + assert!( + saw_disabled_edge, + "the dead thread was never reported: clients still believe a repeater is running" + ); + assert!( + !saw_already_enabled, + "the daemon answered 'repeater already enabled' about a thread that had exited — this \ + is the state no command sequence could escape" + ); + } + #[tokio::test] async fn apply_qsy_accept_reject_record_and_emit_decisions() { let mut engine = test_engine(); diff --git a/docs/dev/project/traceability.md b/docs/dev/project/traceability.md index 8f34a115..2ccc474f 100644 --- a/docs/dev/project/traceability.md +++ b/docs/dev/project/traceability.md @@ -9,6 +9,45 @@ and the actually-observed results per change. --- +## 2026-09-08 — A repeater that is not running was reported as running (#1298) + +- **Requirement/change:** the `EnableRepeater` thread `take()`s the `CrossBandRepeater` out of the + runtime state, so its exit means the repeater is gone — and nothing observed that. + `repeater_enabled` stayed `true`, `EnableRepeater` answered "already enabled" forever, and the + no-repeater arm logged a `warn!` and then set `repeater_enabled = true` and emitted + `RepeaterChanged { enabled: true }` anyway. + +- **Design decision (reviewed by Fable as part of #1297's design; it corrected the issue's premise).** + #1298 said "the loop exits on the first capture that does not decode, which on hardware is the + first capture", making enabled-forever the *normal* state on a rig. **That went stale when #1300 + merged** — a non-decoding capture is now `Ok(None)` and the loop continues. What kills the thread + now is a PTT fault or a transmit error. So the normal state on a rig is not a dead thread reported + as enabled; it is **a live thread that never relays and never says so above DEBUG**. Both defects + in the issue survive that correction; its severity framing did not, and the issue has been amended. + Reaping at the top of the two repeater commands rather than from a periodic tick: the thread's own + events are what notify clients promptly, and the reap only has to make the *next* command truthful + — a tick poll would need `runtime_state` on the tick path for no added signal. + +- **Implementation:** `crates/openpulse-daemon/src/lib.rs` — `reap_finished_repeater`; the + no-repeater arm returns a `CommandError`; the thread emits `CommandError` + a `false` edge on the + **error path only**, since a clean stop is already reported by `DisableRepeater` and emitting there + too would put two `false` edges on one transition. + +- **Tests:** `enabling_a_repeater_that_does_not_exist_fails_instead_of_claiming_success` and + `a_repeater_thread_that_exited_is_reaped_rather_than_reported_enabled`. And + `apply_repeater_enable_disable_emits_state_changes` **was pinning the defect**: it ran with + `RuntimeControlState::default()` (`repeater: None`) and asserted that enabling SUCCEEDED. It now + builds a real `CrossBandRepeater` so the success path is the one it tests. + +- **Test results:** 7 passed. Sabotage-verified in both directions: removing the reap calls fails the + reap gate with "the dead thread was never reported"; reinstating the claim-success arm fails the + enable gate with "the daemon reports a repeater as enabled while none exists". Full workspace gate + below. + +- **Filed alongside:** #1308 — the repeater has **no audio device configuration at all**; both its + engines use the OS default input, and no config field anywhere could name a second card. #1297's + accumulation fix cannot be exercised on a real two-rig station without it. + ## 2026-09-08 — A cap-flushed burst is not ladder evidence (#1255) - **Requirement/change:** `accumulate_routed` returned `Ok(Some(burst))` for two opposite events —