trx-rs/docs/Improvement-Areas.md

Improvement Areas

A comprehensive audit of the trx-rs codebase covering code quality, architecture, security, testing, and performance. Each item includes the affected location and a suggested fix.

Last updated: 2026-03-28

---

Resolved

The following items have been fixed across PRs #58, #59, and #60:

Quick Wins (all complete)

• ✅ Session cleanup timer — 5-minute periodic cleanup_expired() task
• ✅ DecodeHistory<T> type alias — replaces 9 repeated Arc<Mutex<VecDeque<...>>> patterns
• ✅ mode_to_string() allocation — returns Cow<'static, str> (zero-alloc for known modes)
• ✅ FTx dedup — HashSet<u16> for O(1) lookups
• ✅ Unbounded channels — VChanAudioCmd channels bounded at 256
• ✅ JSON serialization — #[serde(flatten)] wrapper replaces string-level splice
• ✅ AtomicUsize counter — estimated_total_count() avoids 9 mutex acquisitions
• ✅ Cookie security warning — startup warning when cookie_secure is false
• ✅ Spectrum encoding — pre-allocated output string replaces format! overhead
• ✅ pub(crate) state data — ReadyStateData/TransmittingStateData fields restricted with constructors + getters
• ✅ Lock ordering docs — module-level documentation in <vchan.rs> establishing rigs → sessions → audio_cmd

Critical (P0)

• ✅ Plugin loading validation — rejects world-writable files on Unix; TRX_PLUGINS_DISABLED env var
• ✅ Audio pipeline mutex panics — all .expect() on history mutexes and .unwrap() on audio ring buffers replaced with .unwrap_or_else(|e| e.into_inner()) poison recovery
• ✅ vchan lock panics — ~25 .unwrap() on RwLock/Mutex replaced with poison recovery

High (P1)

• ✅ RigCat trait split — 13 SDR-specific methods extracted into RigSdr extension trait; RigCat retains core CAT ops + as_sdr()/as_sdr_ref(); SoapySdrRig implements both; FT-817/FT-450D/DummyRig unchanged
• ✅ Decoder history contention — AtomicUsize total counter maintained by record/prune/clear

Medium (P2)

• ✅ Silent state machine failures — debug-level tracing for rejected transitions
• ✅ User input in logs — raw JSON truncated to 128 chars
• ✅ Rate limiting — per-IP LoginRateLimiter (10 attempts/60s) on /auth/login
• ✅ Lock-holding serialization — clone data out under lock, serialize after release
• ✅ Overly-public API — state data fields pub(crate) with controlled accessors
• ✅ Cookie security flag — startup warning for non-TLS deployments
• ✅ Lock ordering — documented in <vchan.rs> module header

---

Remaining Issues

Critical (P0)

Plugin signing and cross-platform validation

Location: src/trx-app/src/<plugins.rs>

Current protections: file permission checks (Unix), TRX_PLUGINS_DISABLED env var, loaded plugins logged at startup.

Still missing:

• No SHA-256 checksum verification — an attacker who passes the permission check can still load a tampered library
• No per-plugin permission scoping (all plugins get full context access)
• Windows has no file permission validation

Suggestions:

• SHA-256 checksum manifest (plugins.toml) verified before Library::new
• Config option to allowlist specific plugin filenames
• On Windows, verify file owner via GetSecurityInfo or equivalent

---

High Priority (P1)

Synchronous locks in async contexts

Location: src/trx-client/trx-frontend/trx-frontend-http/src/<background_decode.rs>, src/trx-client/trx-frontend/trx-frontend-http/src/<vchan.rs>

std::sync::RwLock is used inside async tasks. Current code is safe (no locks held across await points), but not idiomatic. Migrating to tokio::sync::RwLock would prevent future regressions.

Large functions in audio pipeline

Locations:

• src/trx-server/src/<audio.rs> — run_capture() (~200 lines), run_playback() (~217 lines)

These contain nested loops, device re-enumeration logic, and stream error handling that should be extracted into focused helper functions.

---

Medium Priority (P2)

Configuration duplication

Location: src/trx-server/src/<config.rs> (1512 lines), src/trx-client/src/<config.rs> (1181 lines)

14 config structs each, many mirrored between server and client. Extract shared definitions (GeneralConfig, RigConfig, defaults) into trx-app.

---

Low Priority (P3)

Missing tests for critical paths

Serial backends (FT-817, FT-450D), plugin loading/discovery, and the audio pipeline (Opus encode/decode) have no or minimal test coverage.

Core crates (trx-core, trx-server, trx-client, trx-app) have limited [dev-dependencies] and use only inline #[test] functions. Adding test utilities (mock serial ports, test fixtures) would improve coverage.

Plugin system lacks versioning and lifecycle

Location: src/trx-app/src/<plugins.rs>

No plugin API version, capability manifest, or unload/reload semantics. Old plugins break silently on API changes.

Fix: Add a version field to the registration struct and reject incompatible plugins at load time.

---

New Findings (2026-03-28 Deep Review)

High Priority (P1)

Rig task command batching uses LIFO order

Location: src/trx-server/src/<rig_task.rs> — batch.pop()

Pending commands are accumulated into a Vec and processed with pop(), which reverses arrival order. If a client sends SetFreq(14.074) then SetMode(USB), the mode change executes before the frequency change. This can cause unexpected transient state on hardware that validates mode against frequency (e.g. FT-817 rejects CW below 1.8 MHz).

Fix: Replace pop() with drain(..) or iterate in forward order.

FrontendRuntimeContext god-struct (~50 fields)

Location: src/trx-client/trx-frontend/src/<lib.rs>

Mixes audio channels, decode histories, auth config, UI settings, rig routing, virtual channel management, and branding info into a single struct passed through Arc. Every frontend receives all 50 fields even if it only needs a subset. Changes to any field group force recompilation of all frontends.

Suggested decomposition:

FrontendRuntimeContext
  ├── AudioContext          (audio_rx, audio_tx, audio_info, decode_rx)
  ├── DecodeHistoryContext  (ais, vdes, aprs, hf_aprs, cw, ft8, ft4, ft2, wspr)
  ├── HttpAuthConfig        (enabled, passphrases, session_ttl, cookie settings)
  ├── HttpUiConfig          (map_zoom, spectrum settings, history retention)
  ├── RigRoutingContext     (active_rig_id, remote_rigs, rig_states, rig_spectrums)
  ├── OwnerInfo             (callsign, website_url, website_name, ais_vessel_url)
  └── VChanContext          (vchan_audio, vchan_audio_cmd, vchan_destroyed)

Decoder history queues have no capacity bounds

Location: src/trx-server/src/<audio.rs> — DecoderHistories

History queues (VecDeque) grow unbounded until the 24h retention period expires. Under high traffic (e.g. busy AIS channel near a port), a single queue could accumulate millions of entries and consume gigabytes of memory.

Fix: Add per-decoder max capacity (e.g. 10,000 entries). Evict oldest entries when capacity is reached, independent of time-based pruning.

ExponentialBackoff has no jitter

Location: src/trx-core/src/rig/controller/<policies.rs>

Multiple rigs or reconnecting clients using the same backoff parameters will retry at identical times (thundering herd). This is especially problematic when a server restarts and all clients reconnect simultaneously.

Fix: Add randomized jitter (e.g. ±25% of the computed delay) to the ExponentialBackoff::delay() method.

No crash recovery for rig tasks

Location: src/trx-server/src/<main.rs>

If a rig task panics (e.g. due to an unexpected backend error), the task simply disappears. The listener continues routing commands to the dead rig's channel, where they silently timeout. No automatic restart or health monitoring exists.

Fix: Wrap rig tasks in a supervisor loop that detects task completion/panic and restarts with backoff. Emit a RigMachineState::Error on the watch channel so clients see the failure.

---

Medium Priority (P2)

SoapySdrRig constructor takes 20+ parameters

Location: src/trx-server/trx-backend/trx-backend-soapysdr/src/<lib.rs> — new_with_config()

The constructor accepts 20+ positional parameters with no builder pattern, making call sites fragile and hard to read. Adding a new parameter requires updating all callers.

Fix: Introduce a SoapySdrConfig builder struct with sensible defaults.

Dual command enums with mechanical 1:1 mapping

Location: src/trx-protocol/src/<mapping.rs> (675 lines), src/trx-protocol/src/<types.rs>, src/trx-core/src/rig/<command.rs>

ClientCommand and RigCommand are near-identical 40+ variant enums with purely mechanical mapping in mapping.rs. Adding a new command requires editing 4 files (command.rs, types.rs, mapping.rs in both directions, codec.rs). mapping.rs contains an unreachable!() for GetRigs that would panic if the listener logic changes.

Fix: Consider a macro that generates both enums and the mapping from a single definition. Alternatively, collapse to a single enum with serde annotations.

Lock poisoning recovery hides panics

Location: src/trx-server/src/<audio.rs> — DecoderHistories

All mutex acquisitions use .unwrap_or_else(|e| e.into_inner()) which silently recovers from poisoned mutexes. While this prevents cascading panics, it hides the original panic and may operate on inconsistent data.

Fix: Log a warning when recovering from a poisoned lock, and consider whether the recovered data is actually safe to use. For history queues, clearing the queue on poison recovery may be safer than continuing with partial data.

Configuration duplication between server and client

Location: src/trx-server/src/<config.rs> (1,512 lines), src/trx-client/src/<config.rs> (1,181 lines)

14 config structs each, many mirrored between server and client (GeneralConfig, rig model definitions, defaults). Shared config definitions should live in trx-app.

Hardcoded timeouts and retention periods

Locations: Multiple files

Constant	Value	Location
COMMAND_EXEC_TIMEOUT	10s	rig_task.rs
POLL_REFRESH_TIMEOUT	8s	rig_task.rs
IO_TIMEOUT	10s	listener.rs
REQUEST_TIMEOUT	12s	listener.rs
History retention	24h	audio.rs
FT-817 read timeout	800ms	trx-backend-ft817
RIG_TASK_CHANNEL_BUFFER	32	main.rs

None are configurable. Making these part of the TOML config would help deployments with slow serial links or high-latency networks.

---

Low Priority (P3)

FT-817 VFO state inference is fragile

Location: src/trx-server/trx-backend/trx-backend-ft817/src/<lib.rs>

VFO state starts as Unknown and is inferred by matching frequencies against cached values. When VFO A and VFO B are set to the same frequency, inference fails and the rig may report the wrong VFO.

Fix: Some FT-817 variants support extended status bytes that indicate active VFO directly. Detect firmware version and use the direct query when available.

VDES decoder has incomplete FEC

Location: src/decoders/trx-vdes/src/<lib.rs>

The VDES decoder implements burst detection and pi/4-QPSK demodulation but the Turbo FEC (1/2 rate) decoder and link-layer (M.2092-1) parser are not complete. Decoded output is limited to raw symbols.

No forward compatibility in protocol

Location: src/trx-protocol/src/<codec.rs>

Unknown commands cause parse errors. There is no protocol version field in the envelope, making it impossible for older clients to gracefully degrade when connecting to newer servers.

Fix: Add an optional protocol_version field to ClientEnvelope. Unknown commands should return an error response rather than a parse failure.

Command handler boilerplate

Location: src/trx-core/src/rig/controller/<handlers.rs>

11 RigCommandHandler implementations follow identical patterns across 500+ lines (validate state → call executor method → return result). A declarative macro could reduce this to ~100 lines.

Missing tests for critical paths

Serial backends (FT-817, FT-450D), plugin loading/discovery, and the audio pipeline (Opus encode/decode) have no or minimal test coverage. rig_task.rs (1,315 lines) and audio.rs (3,977 lines) — the two largest server modules — have no integration tests.