From d2f751a7ba0602814a51107103451fa838233561 Mon Sep 17 00:00:00 2001 From: Simon Keimer Date: Tue, 8 Sep 2026 10:48:22 +0200 Subject: [PATCH] fix(kiss): the TNC built a SharedPtt and never started its watchdog (#1299) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `openpulse-kiss` constructs `SharedPtt::new(ptt, DEFAULT_PTT_MAX)` and takes guards from it, but called `spawn_watchdog` nowhere — so `force_release_if_expired` had no caller in the crate and the deadline was never checked. A `SharedPtt` with no watchdog thread is the bare `Box` with extra steps. The RAII guard already covers an early return and an unwind. What it cannot reach is a transmit that BLOCKS rather than returns, which is the case the watchdog exists for — on a real rig, a stuck carrier with nothing to take it back. Spawned in the constructor as ARDOP does, not in `run_with_listener`: a caller holding `bridge()` can transmit without running the listener. Found while writing #1260's Twins section, whose first draft claimed the repeater was the last hand-rolled keying path. The census that corrected it put KISS in a third state — guarded but unwatched. #1299's other half stays open: `openpulse-cli` still keys a bare `PttController` at three sites. Implements: REQ-PTT-01 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0188ATCj6DZ9aRVQ2vSirua6 --- CLAUDE.md | 1 + crates/openpulse-kiss/src/lib.rs | 9 ++++ .../tests/ptt_keys_every_transmit.rs | 52 +++++++++++++++++++ docs/dev/project/traceability.md | 24 +++++++++ 4 files changed, 86 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index f35aae35..e92240c0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -237,6 +237,7 @@ Each requirement below is done when the linked test passes. Add new links as tes | **Every DAEMON emission keys the transmitter** — handshake (CONREQ/CONACK), both QSY lines, relay forward and the non-OTA send, which all transmitted unkeyed while five guards sat in `server.rs`. No natural positive control exists here (no `lib.rs` site keyed before), so each test keys the shared PTT directly first; the production-entry twin test disables auto-ID because with it on the test passes against the UNFIXED daemon by seeing the periodic ID | `cargo test -p openpulse-daemon --no-default-features --test ptt_keys_every_daemon_transmit` + `--test twin_daemon_bridge the_handshake_keys_the_transmitter_on_both_stations` | | 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` | +| `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` | | SAR reassembly resists poison — conflicting fragment stream doesn't block the legit one | `cargo test -p openpulse-core --lib sar::tests` (poison/wrong-total/flood) + `cargo test -p openpulse-daemon --lib poison_fragment_does_not_block_conreq_verification` | diff --git a/crates/openpulse-kiss/src/lib.rs b/crates/openpulse-kiss/src/lib.rs index 0d39369f..8f0114db 100644 --- a/crates/openpulse-kiss/src/lib.rs +++ b/crates/openpulse-kiss/src/lib.rs @@ -66,6 +66,15 @@ impl KissServer { relay_forwarder, ptt, ); + // Force-release a key that outlives DEFAULT_PTT_MAX (#1299). Without this the crate built a + // `SharedPtt` and never started its watchdog thread, so `force_release_if_expired` had no + // caller and the deadline was never checked — a `SharedPtt` with no watchdog is the bare + // `Box` with extra steps. The RAII guard already covers an early return or an unwind; what + // it cannot reach is a transmit that BLOCKS rather than returns, which is the case the + // watchdog exists for. Spawned in the constructor, as ARDOP does (`ardop/src/lib.rs:112`), + // so a bridge that can transmit always has one — not in `run_with_listener`, which a caller + // holding `bridge()` can bypass. The thread exits when the last `SharedPtt` clone drops. + let _ptt_watchdog = bridge.ptt.spawn_watchdog(None); Self { bridge, tx_data_rx: Some(tx_data_rx), diff --git a/crates/openpulse-kiss/tests/ptt_keys_every_transmit.rs b/crates/openpulse-kiss/tests/ptt_keys_every_transmit.rs index 5b6619cd..775e0e55 100644 --- a/crates/openpulse-kiss/tests/ptt_keys_every_transmit.rs +++ b/crates/openpulse-kiss/tests/ptt_keys_every_transmit.rs @@ -77,6 +77,58 @@ fn the_shared_ptt_is_wired_and_keys() { assert_eq!(spy.releases.load(Ordering::SeqCst), 1); } +/// THE #1299 GATE: the crate's `SharedPtt` has a watchdog thread, so its deadline is enforced. +/// +/// `openpulse-kiss` built a `SharedPtt` and took guards from it, but called `spawn_watchdog` +/// nowhere — so `force_release_if_expired` had no caller anywhere in the crate and the deadline was +/// never checked. A `SharedPtt` with no watchdog thread is the bare `Box` with extra steps. +/// +/// The RAII guard already covers an early return and an unwind. What it cannot reach is a transmit +/// that **blocks** rather than returns, which is exactly the case a watchdog exists for — and on a +/// real rig that is a stuck carrier with nothing to take it back. +/// +/// Drives the real constructor (`KissServer::with_ptt`), not a hand-built `SharedPtt`: the defect +/// was in the wiring, so a test that builds its own would pass against the unfixed crate. +#[test] +fn the_watchdog_force_releases_a_key_that_outlives_its_deadline() { + let spy = Spy::new(); + let engine = openpulse_modem::ModemEngine::new(Box::new( + openpulse_audio::loopback::LoopbackBackend::default(), + )); + let server = openpulse_kiss::KissServer::with_ptt( + engine, + openpulse_kiss::KissConfig { + bind_addr: "127.0.0.1".into(), + port: 0, + mode: "BPSK250".into(), + loopback: true, + }, + Default::default(), + None, + Some(Box::new(spy.clone())), + ); + let ptt = server.bridge().ptt.clone(); + // Shorten the 180 s production deadline; the watchdog ticks every 100 ms. + ptt.set_max_duration(std::time::Duration::from_millis(120)); + + // Key WITHOUT holding a guard, standing in for a transmit that blocks past the deadline: a + // dropped guard would release on its own and prove nothing about the watchdog. + ptt.key(None).expect("assert"); + assert!(spy.asserted.load(Ordering::SeqCst), "keyed"); + + std::thread::sleep(std::time::Duration::from_millis(600)); + assert!( + !spy.asserted.load(Ordering::SeqCst), + "the transmitter is STILL KEYED past its deadline — the crate builds a SharedPtt but never \ + starts its watchdog, so nothing calls force_release_if_expired (#1299)" + ); + assert_eq!( + spy.releases.load(Ordering::SeqCst), + 1, + "exactly one release: the watchdog is single-fire" + ); +} + /// **Source scan.** Every `.transmit(` in `bridge.rs` sits inside `keyed_transmit`. /// /// The pattern is `.transmit(`, NOT `engine.transmit` as the ARDOP and daemon scanners use. KISS diff --git a/docs/dev/project/traceability.md b/docs/dev/project/traceability.md index 4d629e16..fafb298c 100644 --- a/docs/dev/project/traceability.md +++ b/docs/dev/project/traceability.md @@ -9,6 +9,30 @@ and the actually-observed results per change. --- +## 2026-09-08 — `openpulse-kiss` built a `SharedPtt` and never started its watchdog (#1299, KISS half) + +- **Requirement/change:** found while writing #1260's Twins section, which first claimed the + repeater was "the last hand-rolled keying path". It was not — and the census that corrected it + showed KISS in a third state: `SharedPtt` + guard (`bridge.rs:105`, `:131`) but **no + `spawn_watchdog` anywhere in the crate**, so `force_release_if_expired` had no caller and the + 180 s deadline was never checked. +- **Design decision:** spawn it in the constructor (`KissServer::with_ptt`), as ARDOP does + (`ardop/src/lib.rs:112`), rather than in `run_with_listener` — a caller holding `bridge()` can + transmit without ever running the listener, and a bridge that can transmit must have a watchdog. + The handle is dropped deliberately: the thread exits when the last `SharedPtt` clone drops. +- **Scope:** the guard already covered an early return and an unwind. What it cannot reach — and + what this restores — is a transmit that **blocks** rather than returns. +- **Implementation:** `crates/openpulse-kiss/src/lib.rs`, one line plus its rationale. +- **Tests:** `ptt_keys_every_transmit::the_watchdog_force_releases_a_key_that_outlives_its_deadline` + — drives the real constructor (a hand-built `SharedPtt` would pass against the unfixed crate), + shortens the deadline, and keys **without** holding a guard, since a dropped guard would release + on its own and prove nothing about the watchdog. +- **Test results:** 3 passed. Sabotage-verified: removing the `spawn_watchdog` line fails it with + "the transmitter is STILL KEYED past its deadline". Full workspace gate below. +- **Not closed:** #1299's other half — `openpulse-cli` still keys a bare `PttController` at + `transmit.rs:18`, `calibrate.rs:201` and `:328`. Lower risk (foreground, attended, and + `transmit.rs` releases before its `?`), but outside the discipline every other path is now under. + ## 2026-09-08 — The noise-floor tracker was a function of the caller's chunking (#1254) - **Requirement/change:** `NoiseFloorTracker::update` discarded any buffer shorter than its