From c6b4d36ba3a790d464c87e9c16970ee037c3c26e Mon Sep 17 00:00:00 2001 From: ruv Date: Tue, 21 Jul 2026 16:00:28 -0700 Subject: [PATCH] docs(adr-186): propose training progress API fix (refs #1233) --- docs/adr/ADR-186-training-progress-api.md | 470 ++++++++++++++++++++++ 1 file changed, 470 insertions(+) create mode 100644 docs/adr/ADR-186-training-progress-api.md diff --git a/docs/adr/ADR-186-training-progress-api.md b/docs/adr/ADR-186-training-progress-api.md new file mode 100644 index 00000000..878dda20 --- /dev/null +++ b/docs/adr/ADR-186-training-progress-api.md @@ -0,0 +1,470 @@ +# ADR-186: Training progress API — wire the orphaned in-server trainer to `/ws/train/progress` + +| Field | Value | +|-------|-------| +| **Status** | Proposed | +| **Date** | 2026-07-21 | +| **Deciders** | ruv | +| **Codename** | **TRAIN-RECONNECT** — connecting a trainer that was written, committed, and then never plugged in | +| **Relates to** | [ADR-051](ADR-051-sensing-server-decomposition.md) (main.rs decomposition into ~14 modules), [ADR-151](ADR-151-per-room-calibration.md) (`train-room` specialist bank), [ADR-152](ADR-152-wifi-pose-sota-2026.md) (MAE recipe / geometry conditioning), [ADR-166](ADR-166-quality-engineering-security-hardening.md) (WS auth + god-object decomposition) | +| **Tracking issue** | [#1233](https://github.com/ruvnet/wifi-densepose/issues/1233) — "Training does not start – /ws/train/progress returns 404 and no model is generated" (open) | + +--- + +## 1. Context + +### 1.1 The reported gap + +A user starting training from the web dashboard hits +`ws://localhost:3000/ws/train/progress`, which **404s**, and the backend never +produces a trained `.rvf` model or any further log output beyond a single +"Training started" line. Issue #1233 is open, and the repo owner's own comment on +it states: + +> The `/ws/train/progress` WebSocket endpoint is not yet exposed in the stable +> server — the training pipeline (room-calibration specialists, MAE pretraining) +> runs via the CLI (`wifi-densepose train-room`) rather than through the +> HTTP/WebSocket API, which is why the Docker image returns 404 for that path. + +So the dashboard has a **"Start Training" button that silently no-ops**: it POSTs a +config, receives a `success: true` response, and then nothing happens — no error is +surfaced, no model is produced, no progress stream exists. A button that appears to +work but does nothing is the definition of slop, and this ADR exists to close that +gap honestly. + +### 1.2 What the live server actually does today (evidence) + +The stable server mounts **stub** training handlers. The POST handler flips a string +flag, logs one line, and returns success — it starts no job: + +```rust +// v2/crates/wifi-densepose-sensing-server/src/main.rs:4986–5006 +async fn train_start( + State(state): State, + Json(body): Json, +) -> Json { + let mut s = state.write().await; + if s.training_status == "running" { /* ... */ } + s.training_status = "running".to_string(); + s.training_config = Some(body.clone()); + info!("Training started with config: {}", body); // ← the one log line the issue reports + Json(serde_json::json!({ + "success": true, + "status": "running", + "message": "Training pipeline started. Use GET /api/v1/train/status to monitor.", + })) +} +``` + +These three stubs — and **nothing else training-related** — are wired into the live +router: + +```rust +// v2/crates/wifi-densepose-sensing-server/src/main.rs:8068–8071 +// Training endpoints +.route("/api/v1/train/status", get(train_status)) +.route("/api/v1/train/start", post(train_start)) +.route("/api/v1/train/stop", post(train_stop)) +``` + +There is **no `/ws/train/progress` route in the live app** — hence the 404 that +issue #1233 reports. The stub state fields backing them are just: + +```rust +// v2/crates/wifi-densepose-sensing-server/src/main.rs:1125–1127 +training_status: String, // "idle" | "running" | ... +training_config: Option, +``` + +### 1.3 The surprising finding: a real trainer already exists, orphaned + +The gap is **not** that training was never built for the server. A complete +in-server training pipeline **already exists in the tree** at +`v2/crates/wifi-densepose-sensing-server/src/training_api.rs` (1,860 lines). Its own +module doc describes what it does (`training_api.rs:1–25`): + +- Loads recorded CSI from `.csi.jsonl` files, extracts signal features (subcarrier + variance, temporal gradients, Goertzel frequency-domain power). +- Trains a regularised linear model via batch gradient descent. +- Exports a calibrated `.rvf` model container via `RvfBuilder` on completion. +- **"No PyTorch / `tch` dependency is required. All linear algebra is implemented + inline using standard Rust math."** (`training_api.rs:11–13`) + +It runs training on a **background tokio task** and streams progress over a +`tokio::sync::broadcast` channel to a real WebSocket handler: + +- `start_training` spawns the job: `tokio::spawn(async move { ... })` + (`training_api.rs:1564`, spawn at `:1610`). +- `ws_train_progress_handler` subscribes to `training_progress_tx` and forwards + `{"type":"progress", "data": …}` frames (`training_api.rs:1778–1836`). +- A `routes()` factory wires the whole surface, **including the missing route**: + +```rust +// v2/crates/wifi-densepose-sensing-server/src/training_api.rs:1841–1849 +pub fn routes() -> Router { + Router::new() + .route("/api/v1/train/start", post(start_training)) + .route("/api/v1/train/stop", post(stop_training)) + .route("/api/v1/train/status", get(training_status)) + .route("/api/v1/train/pretrain", post(start_pretrain)) + .route("/api/v1/train/lora", post(start_lora_training)) + .route("/ws/train/progress", get(ws_train_progress_handler)) +} +``` + +**This module is dead code.** There is no `mod training_api;` declaration anywhere +in the crate — a repo-wide search for `training_api` returns only a doc-comment +mention in `path_safety.rs:9`. Because Rust never sees the file without a `mod` +declaration, `training_api.rs` is **not compiled into the binary at all**, and +`training_api::routes()` is never merged into the app. It was written, committed +(last touched by commit `9b07dff29`), and then orphaned. + +### 1.4 Why it would not even compile if naively wired in + +The orphan was written against a **different state shape than the one that shipped**. +`training_api.rs` expects its parent to expose an `AppStateInner` carrying a training +sub-state and a broadcast sender: + +```rust +// v2/crates/wifi-densepose-sensing-server/src/training_api.rs:249 +pub type AppState = Arc>; +// handlers read s.training_state.status, s.training_state.task_handle, +// s.training_progress_tx (e.g. training_api.rs:1588, :1610, :1788) +``` + +But the **real** `AppStateInner` (`main.rs:1024`, aliased `SharedState` at +`main.rs:1249`) has none of those fields — only the `training_status: String` / +`training_config` stubs from §1.2. `training_state: TrainingState` is defined +locally in `training_api.rs:232`, and `training_progress_tx` exists nowhere on the +live state. So adding `mod training_api;` today produces a compile error: the module +references `AppStateInner` fields that do not exist. Wiring it in requires +**reconciling the state struct first**, not merely uncommenting a route. + +### 1.5 The working path today + +The path that actually trains a model is the CLI, exactly as the maintainer's +comment says: + +- `wifi-densepose train-room` → `room.rs:241` `train_room(...)`, the ADR-151 + Stage-2–5 per-room specialist-bank trainer (`enroll → train-room → room-watch`). +- The heavier `wifi-densepose-train` crate exposes epoch-level metrics + (`trainer.rs:43` `pub epoch: usize`, `trainer.rs:64` `best_epoch`) that a progress + stream could surface directly — the data a WebSocket needs already exists in the + training loop. + +### 1.6 What this ADR is *not* + +- Not a rewrite of the trainer. The pipeline in `training_api.rs` already exists; + this ADR reconnects and hardens it. +- Not a move of GPU/`tch`-backed training into the Axum server. The in-server + trainer is deliberately `tch`-free (§1.3). Heavy MAE/LoRA training stays in the + CLI / `wifi-densepose-train` crate; the server streams progress for the light, + pure-Rust specialist trainer and (optionally) proxies status for CLI-launched runs. +- Not a change to the `train-room` CLI contract (ADR-151). The CLI remains the + authoritative path for offline / batch training. + +--- + +## 2. Current state — evidence + +| Artifact | Value | Source | +|---|---|---| +| Live POST handler | `train_start` — flips a flag, logs, returns `success:true`, starts no job | `main.rs:4986–5006` | +| The "Training started" log line from the issue | `info!("Training started with config: {}", body)` | `main.rs:5000` | +| Live training routes | `train/status`, `train/start`, `train/stop` (stubs only) | `main.rs:8068–8071` | +| `/ws/train/progress` in live app | **Absent** → 404 | (no route in `main.rs` router) | +| Live training state fields | `training_status: String`, `training_config: Option` | `main.rs:1125–1127` | +| Real in-server trainer | 1,860-line implemented pipeline, `tch`-free, exports `.rvf` | `training_api.rs:1–25` | +| Real WS progress handler | subscribes to broadcast, streams `progress` frames | `training_api.rs:1778–1836` | +| Real route factory (has the missing route) | `routes()` incl. `/ws/train/progress` | `training_api.rs:1841–1849` | +| Background job spawn | `tokio::spawn` of the training task | `training_api.rs:1564`, spawn `:1610` | +| `mod training_api;` declaration | **None in the crate** (only a doc mention) | `path_safety.rs:9` | +| State-shape mismatch | expects `super::AppStateInner.{training_state, training_progress_tx}` | `training_api.rs:249`, `:232` | +| Real `AppStateInner` / `SharedState` | has neither field | `main.rs:1024`, `:1249` | +| Working training path | CLI `train-room` (ADR-151 specialist bank) | `room.rs:241` | +| Epoch metrics available to stream | `TrainMetrics.epoch`, `best_epoch` | `train/src/trainer.rs:43`, `:64` | + +--- + +## 3. Gap analysis + +| Capability | Desired | Today | Gap severity | +|---|---|---|---| +| `/ws/train/progress` resolves | 101 Switching Protocols, streams epoch/loss/eta | 404 (route absent) | **Critical** — the reported bug | +| "Start Training" produces a model | background job trains and writes `.rvf` | flag flip + one log line, no job, no model | **Critical** | +| Error surfaced to the user | button reflects real state / disabled with reason | silent no-op, `success:true` | **Critical** (slop) | +| In-server trainer compiled | part of the crate, unit-tested | orphaned; not compiled (no `mod`) | **High** | +| State supports progress streaming | `training_state` + `training_progress_tx` on `AppStateInner` | absent — orphan won't compile as-is | **High** | +| WS auth on the training surface | `/ws/train/progress` under bearer gate (ADR-166 §Sprint-1) | n/a (route absent) | **High** | +| `dataset_ids` path safety | validated before file open | `path_safety.rs` exists but unreached by live routes | **Medium** | +| Server ↔ CLI parity | shared/consistent training semantics | two divergent trainers (stub vs CLI vs orphan) | **Medium** | + +--- + +## 4. Decision + +**Chosen path: wire the existing in-server trainer into the live server** — reconcile +the state struct, declare the module, merge `training_api::routes()`, delete the +stub handlers, and expose a real `/ws/train/progress` that streams epoch/loss/eta +events from the already-implemented background job. + +This is called **TRAIN-RECONNECT**. + +### 4.1 Why this path, and not "make the button honestly say CLI-only" + +The task framing offered two honest options. Investigation decided it: + +| Consideration | Evidence | Implication | +|---|---|---| +| Is server-side training genuinely GPU/`tch`-bound (→ keep CLI-only)? | The in-server trainer is explicitly **`tch`-free**, pure Rust, exports `.rvf` (`training_api.rs:11–13`) | The "too heavy for Axum" argument is contradicted by the code | +| Does a real streaming implementation already exist? | Full pipeline + broadcast + WS handler + `routes()` present (`training_api.rs:1564,1778,1841`) | The impressive-sounding option is also the *least* new code — it already exists | +| Why does it 404 then? | No `mod training_api;`; state-shape mismatch (`:249` vs `main.rs:1024`) | The fix is reconnection + reconciliation, not new invention | + +Because the honest, code-supported reality is "a working trainer was written and left +unplugged," the right decision is to plug it in — this is not choosing the flashier +option over the code; it *is* what the code says. + +**However**, path B is retained as a **mandatory fallback guarantee** (Phase P5): if, +for a given build/deployment, server-side training is disabled (e.g. behind a +feature flag, or on the lightweight appliance image where recordings aren't +available), the dashboard button MUST be disabled with a tooltip pointing at +`wifi-densepose train-room` — never a silent `success:true` no-op again. The slop is +eliminated in both the enabled and disabled configurations. + +### 4.2 Scope boundary — light trainer streams, heavy trainer proxies + +- The **pure-Rust specialist trainer** (`training_api.rs`, ADR-151 flavour) runs + in-process and streams live epoch/loss/eta over `/ws/train/progress`. +- **Heavy MAE/LoRA training** (`wifi-densepose-train`, `tch`/GPU) stays CLI-launched. + The server does not host it; at most `/api/v1/train/status` reports on a + CLI-launched run if one registers itself. Streaming heavy training is out of scope + for this ADR (noted as an open question, §8). + +--- + +## 5. Detailed design + +### 5.1 Reconcile `AppStateInner` + +Replace the two stub fields (`main.rs:1125–1127`) with the sub-state the trainer +expects, so `training_api.rs` compiles against `super::AppStateInner`: + +```rust +// main.rs — inside AppStateInner (replacing training_status / training_config) +training_state: training_api::TrainingState, // status, epoch, best_pck, task_handle +training_progress_tx: tokio::sync::broadcast::Sender, // progress fan-out +``` + +`train_status` consumers that read `s.training_status` / `s.training_config` are +updated to read `s.training_state.status`. The broadcast sender is created at state +init (`main.rs:7826` region, where the stubs are seeded today). + +### 5.2 Declare and merge the module + +- Add `mod training_api;` to `main.rs` (or `pub mod` in `lib.rs` if the router is + assembled there). +- Delete the stub handlers `train_start` / `train_stop` / `train_status` + (`main.rs:4977–5023`) and their three route mounts (`main.rs:8069–8071`). +- Merge the real router **after** `.with_state(state.clone())`, the same pattern the + RuField surface already uses (`main.rs:8104–8111`): + +```rust +// main.rs router assembly +.merge(training_api::routes()) +``` + +so that `/api/v1/train/*` and `/ws/train/progress` resolve against the shared state. + +### 5.3 Auth and safety (ADR-166 alignment) + +- `/api/v1/train/*` sits under the existing opt-in bearer gate (`main.rs:8095–8102`, + `RUVIEW_API_TOKEN`). `/ws/train/progress` follows the same policy decision made for + `/ws/sensing` — document explicitly whether the training WS is gated (recommended: + gated when a token is set, since training reads/writes recordings and models). +- `dataset_ids` from `StartTrainingRequest` (`training_api.rs:126–130`) are resolved + through `path_safety` before any file open — `path_safety.rs:9` already anticipates + `{dataset_id}.csi.jsonl` under `RECORDINGS_DIR`; wire it in the load path. +- Single-job concurrency guard: `start_training` already rejects a second run while + `training_state.status.active` (`training_api.rs:1571`) — keep it. + +### 5.4 Progress event schema (already emitted) + +The WS handler already frames messages as `{"type":"status"|"progress", "data": …}` +(`training_api.rs:1796–1815`). Confirm the `data` payload carries at minimum +`epoch`, `total_epochs`, `loss`, `best_pck`, and an `eta_seconds`; these map onto the +`TrainMetrics`/`TrainingStatus` fields already populated by the loop +(`training_api.rs:1251`, `train/src/trainer.rs:43,64`). + +### 5.5 Dashboard honesty (both configurations) + +- **Enabled build:** button POSTs `/api/v1/train/start`, then opens + `/ws/train/progress`; the UI renders live epoch/loss/eta and a terminal + success/failure with the output `.rvf` path. +- **Disabled build:** `/api/v1/train/start` returns a structured + `{"enabled": false, "reason": "...", "cli": "wifi-densepose train-room"}` and the + button renders disabled with a tooltip — no silent `success:true`. + +--- + +## 6. Phase ledger + +``` +P0 ──► P1 ──► P2 ──► P3 ──► P4 ──► P5 ──► P6 +repro state wire stream auth+ dash tests+ ++audit recon router job safety honesty witness +``` + +### P0 — Reproduce & audit (evidence lock) +- [ ] Repro the 404: run the server, `websocat ws://localhost:3000/ws/train/progress`, + capture the 404 (baseline for the fix). +- [ ] Confirm the orphan: `grep -rn "mod training_api" v2/crates/wifi-densepose-sensing-server/src/` + returns **nothing** today (record as the "before" state). +- [ ] Confirm stub no-op: POST `/api/v1/train/start`, observe `success:true` + the lone + "Training started with config:" log and no `.rvf` written. + +### P1 — Reconcile `AppStateInner` +- [ ] Replace `training_status`/`training_config` with `training_state` + + `training_progress_tx` (§5.1). +- [ ] Update state init and any readers of the old fields. +- [ ] Add `mod training_api;` so the module compiles against the real state. + +### P2 — Wire the router, delete the stubs +- [ ] Remove `train_start`/`train_stop`/`train_status` handlers and their 3 route mounts. +- [ ] `.merge(training_api::routes())` after `.with_state(...)`. +- [ ] `/api/v1/train/*` and `/ws/train/progress` resolve (no 404). + +### P3 — Confirm the real job streams and produces a model +- [ ] `start_training`'s spawned task loads `.csi.jsonl`, runs the gradient-descent + loop, and writes a `.rvf` under `data/models`. +- [ ] Progress frames carry `epoch`, `total_epochs`, `loss`, `best_pck`, `eta_seconds`. +- [ ] Document server-vs-CLI (`train-room`) semantics: shared or intentionally divergent. + +### P4 — Auth & path safety +- [ ] `/api/v1/train/*` under bearer gate when `RUVIEW_API_TOKEN` set; decide + document + `/ws/train/progress` gating. +- [ ] `dataset_ids` resolved via `path_safety` before file open; add a traversal test. +- [ ] Single-job guard verified (second start rejected with a clear error). + +### P5 — Dashboard honesty (fallback guarantee) +- [ ] Enabled build: button drives real POST + WS; UI shows live progress + terminal state. +- [ ] Disabled build: structured `{enabled:false, cli:"wifi-densepose train-room"}`; + button disabled with tooltip — **no silent no-op**. + +### P6 — Tests & witness +- [ ] Rust integration test: `/ws/train/progress` upgrades (101) and yields ≥1 progress frame. +- [ ] Regression test: POST start → poll status → `.rvf` exists. +- [ ] Update CHANGELOG + README/CLAUDE if the route table or scope changed. + +*(No item above is complete; this ADR is Proposed.)* + +--- + +## 7. Acceptance criteria (concrete verification) + +All must pass before ADR-186 is Accepted: + +- [ ] **Orphan is reconnected:** + `grep -rn "mod training_api" v2/crates/wifi-densepose-sensing-server/src/` + returns a hit, and + `cargo build -p wifi-densepose-sensing-server` **compiles** (proves the state + reconciliation in §5.1 is correct — the module cannot compile against the + current `AppStateInner`). +- [ ] **Route no longer 404s (HTTP upgrade):** + ```bash + curl -i -N \ + -H "Connection: Upgrade" -H "Upgrade: websocket" \ + -H "Sec-WebSocket-Version: 13" -H "Sec-WebSocket-Key: dGhlIHNhbXBsZQ==" \ + http://localhost:3000/ws/train/progress + ``` + returns `101 Switching Protocols`, **not** `404`. +- [ ] **Progress actually streams:** + ```bash + # terminal A + websocat ws://localhost:3000/ws/train/progress + # terminal B + curl -s -X POST http://localhost:3000/api/v1/train/start \ + -H 'Content-Type: application/json' \ + -d '{"dataset_ids":["room-a"],"config":{"epochs":10}}' + ``` + Terminal A receives at least one `{"type":"progress","data":{"epoch":…,"loss":…}}` + frame within 10 s. +- [ ] **A model is produced:** after the run completes, a fresh `.rvf` exists under + `data/models/` (`ls -t data/models/*.rvf | head -1` is newer than the run start). +- [ ] **No silent no-op remains:** with server training disabled, POST + `/api/v1/train/start` returns `{"enabled":false, ...,"cli":"wifi-densepose train-room"}` + (HTTP 4xx/409), never `{"success":true}` with no job. +- [ ] **Auth honored:** with `RUVIEW_API_TOKEN` set, an unauthenticated + `POST /api/v1/train/start` is rejected; with the token it succeeds. +- [ ] **Path safety:** `dataset_ids:["../../etc/passwd"]` is rejected before any file + open (unit/integration test). +- [ ] **Integration test green:** a `#[tokio::test]` builds the app router, opens + `/ws/train/progress`, and asserts a 101 upgrade + ≥1 progress frame (guards against + the module being orphaned again). +- [ ] **Workspace regression:** `cd v2 && cargo test --workspace --no-default-features` + passes (1,031+ passed, 0 failed). + +--- + +## 8. Consequences + +### Positive +- Closes issue #1233: the dashboard button either trains-and-streams or honestly says + "use the CLI" — the silent no-op is gone in every configuration. +- Reclaims 1,860 lines of already-written, already-committed trainer that were dead + (uncompiled) code, and adds a test that keeps them wired. +- `/ws/train/progress` gives the UI real epoch/loss/eta, matching the maintainer's + stated intent. +- Forces the state-shape reconciliation that the orphan implied but never landed, + removing a latent "two competing training designs" trap in `AppStateInner`. + +### Negative +- Editing `AppStateInner` (`main.rs:1024`) and the router (`main.rs:8068`) touches the + large `main.rs`; merge-conflict risk with concurrent work on the same file (the + ADR-166 decomposition is relevant here). +- Adds a live training code path to the server's attack surface — mitigated by the + bearer gate and `path_safety`, but it must be reviewed (network/hardware boundary, + per the pre-merge security checklist). +- Server and CLI now have two trainers that must be kept semantically consistent, or + their divergence explicitly documented. + +### Neutral +- Heavy MAE/LoRA/`tch` training remains CLI-only; the server streams only the + light pure-Rust specialist trainer. Streaming heavy runs is deferred. +- The progress event schema (`epoch/loss/best_pck/eta`) is already emitted by the + orphan; no new schema is invented, only confirmed and documented. + +--- + +## 9. Open questions + +1. **WS auth policy for `/ws/train/progress`:** gate it whenever `RUVIEW_API_TOKEN` + is set (like `/api/v1/*`), or leave it open like `/ws/sensing`? *Tentative: gate + it — training reads recordings and writes models.* +2. **Server ↔ CLI trainer parity:** should the in-server trainer and + `wifi-densepose train-room` (ADR-151) share one code path, or remain deliberately + separate (server = quick UI-driven specialist fit; CLI = full bank + geometry + conditioning)? *Tentative: keep separate, document the split, share feature + extraction where cheap.* +3. **Heavy-training progress:** can a CLI-launched `wifi-densepose-train` (`tch`) + run register itself so `/api/v1/train/status` and the WS can report on it without + hosting it in-process? *Tentative: out of scope here; a follow-on ADR.* +4. **Feature-flagging server training:** should in-server training be behind a Cargo + feature (off on the lightweight appliance image), making the P5 disabled-button + path the default there? *Tentative: yes — flag it; default the UI to the honest + disabled state on images without recordings.* + +--- + +## 10. References + +- **Issue #1233**: https://github.com/ruvnet/wifi-densepose/issues/1233 — the reported bug. +- **Live stubs**: `v2/crates/wifi-densepose-sensing-server/src/main.rs:4977–5023` (handlers), + `:8068–8071` (routes), `:1125–1127` (state fields), `:1024`/`:1249` (`AppStateInner`/`SharedState`). +- **Orphaned trainer**: `v2/crates/wifi-densepose-sensing-server/src/training_api.rs` — + module doc `:1–25`, `TrainingState` `:232`, `AppState` alias `:249`, `start_training` `:1564` + (spawn `:1610`), WS handler `:1778–1836`, `routes()` `:1841–1849`. +- **Not-a-module proof**: repo-wide `training_api` only in `path_safety.rs:9` (doc comment). +- **CLI working path**: `v2/crates/wifi-densepose-cli/src/room.rs:241` `train_room` (ADR-151). +- **Epoch metrics**: `v2/crates/wifi-densepose-train/src/trainer.rs:43`, `:64`. +- **ADR-166**: WebSocket authentication + `main.rs` decomposition (security context for this change). +- **ADR-151**: per-room calibration / `train-room` specialist bank.