fluxa-core/docs/robustness-plan.md

# fluxa-core robustness & performance plan

Goal: move the crate toward "if it compiles, it doesn't break at runtime" and make the
hot paths (dispatch, effect completion, stream ranking) cheap enough to never show up in
a profile. This document is the result of a full review of the crate as of 2026-07-07
(master @ f00a09a) and lays out what's already good, where runtime breakage can still
hide, and a prioritized plan to close each gap.

## Current state, honestly assessed

What's already strong and should be preserved:

- `headless_engine` state is fully typed — `EngineState` contains zero `serde_json::Value`
  fields at the top level (`state.rs`), cross-module mutation goes through `pub(super)`
  setters, and generations use the `GenerationKey` enum.
- Every JNI entry point wraps its body in `catch_unwind`, and `core_invoke` does too, so
  a panic can't cross the FFI boundary and abort the Android process.
- Production code is nearly panic-free: outside test modules the only `expect` is a
  static regex in `addon_store.rs:748`, which can't fail for a fixed pattern.
- Poisoned-mutex recovery exists (`lock_engines` uses `into_inner()`), so one panicking
  dispatch can't brick every later call.
- `StatePatch::diff` sends only changed domains over the wire instead of the full state.
- Fuzz targets exist (`episode_matching`, `manifest_parse`, `percent_decode`).
- Effect delivery has dedup (`delivered_effect_ids`) and expiry (`EFFECT_EXPIRY`), so a
  lost `completeEffect` doesn't leak or double-execute.

What undermines the "compiles ⇒ works" goal, in order of real-world risk:

## P0 — fix immediately

### 1. Master has a failing test

`player_policy::tests::convert_dv81_hls_still_deferred_to_manifest_rewrite` fails.
The branch at `player_policy.rs:445` returns `hls_rpu_convert` for
HLS + P7 + `convert_dv81` + DV decoder, while the test still expects
`none`/`manifest_handled`. Either the new branch is a policy change and the test is
stale, or the branch regressed the "HLS is always the OkHttp interceptor's job" rule
that `sample_hls_stream_always_deferred_to_manifest_rewrite` documents. Decide which,
fix, and add a CI gate (see §8) so a red master can't happen silently again.

## P1 — silent failures at the boundary

### 2. `Option<String>` swallows every error

The dominant FFI pattern is `*_json(&str) -> Option<String>` (~295 functions), almost
always built on `serde_json::from_str(...).ok()?` (~135 sites). Any malformed or
schema-drifted input produces `None` → the host receives `null` with zero diagnostics.
This is the single biggest source of "compiles but breaks at runtime": a Kotlin-side
field rename doesn't crash anything, it just makes a feature quietly stop working.

Plan, incremental and wire-compatible:

- Introduce one internal error type:
  ```rust
  pub(crate) enum CoreError {
      BadInput { context: &'static str, detail: String },
      NotFound { context: &'static str },
  }
  ```
  Migrate module internals from `Option<T>` to `Result<T, CoreError>`, mechanical
  module by module (`?` still works). Keep the outer `Option<String>` shims so JNI
  signatures and the Kotlin side don't change.
- At each boundary shim, on `Err` route the detail through a host-pluggable logger
  before returning `None`/`null`. Add a `core.setLogSink` (JNI: one extern that stores
  a JVM callback; desktop: a `fn(String)` setter) so failures land in logcat / desktop
  logs instead of vanishing. This turns every future wire drift from "feature silently
  dead" into a one-line log with the exact field that failed to parse.
- `core_invoke` already has an error envelope — have its shims return the real
  `CoreError` kind/message instead of collapsing to `NotFound` via `Option`.

### 3. `EffectResultInput` accepts garbage as success

Every field is `#[serde(default)]` (`contracts.rs:318`), so a completely wrong-shaped
`completeEffect` payload deserializes into `effect_id: ""`, `status: ""` — the engine
then looks up effect `""`, finds nothing, and the completion is dropped without a trace.
Make `effect_id` required, make `status` an enum:

```rust
#[serde(rename_all = "camelCase")]
enum EffectStatus { Ok, Error, Cancelled }
```

Reject unknown statuses at parse time with a logged error (via §2's sink) instead of
treating them as an empty string that matches no arm.

## P2 — make invalid states unrepresentable

### 4. `Value` leaves inside the typed engine

`EngineState` is typed, but the payloads flowing through it are not: ~370 `Value` /
`Vec<Value>` / `Option<Value>` occurrences across `headless_engine/*.rs`. The repeat
offenders in `AppAction` are `profile: Option<Value>`, `meta: Value`,
`streams: Vec<Value>`, `item: Value`. Every `.get("...").and_then(as_str)` chain on
these is a stringly-typed runtime contract the compiler can't check.

Plan: define the three or four shared wire types once, in a new `types/wire.rs` (the
`types/` module dir already exists), with `#[serde(rename_all = "camelCase")]` and
`#[serde(default)]` on genuinely optional fields:

- `Profile` — `profile_prefs.rs` already does a typed read of the same JSON; promote
  that struct instead of writing a new one.
- `MetaItem` / `Video` — the detail/library/watchlist modules all re-derive the same
  fields (`id`, `type`, `name`, `poster`, `videos`, episode fields) from `Value`.
- `Stream` — `stream_policy.rs` already has typed stream structs; reuse them in
  `AppAction::PlayerStreamsLoaded` and friends.

Crucial constraint: any field the platform sends that Rust doesn't model must survive
round-trips, because effect payloads echo these objects back. Use
`#[serde(flatten)] extra: serde_json::Map<String, Value>` on each wire type so unknown
fields pass through unchanged. This lets migration happen field-by-field without ever
breaking a consumer that sends more than we model.

Migrate one action at a time; the JSON on the wire never changes, so this is invisible
to the platforms. Priority order by breakage frequency: `profile` (touched by ~20
actions), `meta`, `streams`, `item`.

### 5. `json!` payload literals defeat compile-time checking

The stated convention is "effect payloads are small `#[derive(Serialize)]` structs",
but `headless_engine/` still has ~80 `json!({...})` payload literals. A typo'd key in
one of those compiles fine and the platform-side effect executor gets a field it
doesn't recognize. Finish the migration: one payload struct per effect kind, named
after it (`FetchDetailStreamsPayload`), living next to the module that emits it.
Once done, add `json!` to the review checklist / grep gate for `headless_engine/`
(it stays fine in tests and in `ffi.rs` envelope construction).

### 6. Stringly-typed enums that already have a closed set

- `EffectEnvelope.kind` is `String` even though `EffectKind` exists and has an
  exhaustive test. Change the field to `EffectKind` with
  `#[derive(Serialize, Deserialize)] #[serde(rename_all = "camelCase")]` on the enum —
  serde then generates exactly the strings `as_str()` produces today, the 100-line
  hand-written `as_str`/`from_str` pair and its roundtrip test get deleted, and adding
  a variant can no longer be forgotten in one of three places. `EffectEnvelope::raw`
  has to go or become test-only; audit its callers first.
- `fallback_mode` in `player_policy.rs` is compared as `"convert_dv81"` / `"auto"` /
  `"off"` string literals throughout `dv_proxy_plan_json`. Parse once into an enum at
  the request boundary; a typo in a mode string then fails at the one parse site, not
  silently in whichever comparison spelled it wrong.
- Same treatment for `source_selection_mode`, scrobble `action_name`, and auth
  `provider`/`mode` — each is a closed set spread across string comparisons today.

## P3 — the three uncoordinated FFI surfaces

### 7. One method inventory instead of three hand-wired ones

`ffi.rs` routes ~115 method names by string; `jni.rs` hand-wires ~105 externs; nothing
guarantees the same domain function is exposed consistently or spelled the same. The
CLAUDE.md assessment that a full consolidation isn't worth the rewrite risk stands —
but a cheap middle ground removes most drift without touching `jni.rs`'s hot path:

- Declare the string-routed surface in one table macro:
  ```rust
  core_methods! {
      "streamPolicy.rankStreams" => stream_policy::rank_streams_json(json),
      "engine.dispatch" => (handle: u64, action: str) headless_engine::headless_engine_dispatch_json,
      ...
  }
  ```
  generating the `route_*` functions in `ffi.rs`. Adding a method becomes one line;
  the per-domain router boilerplate and its UnknownMethod-chaining protocol disappear.
- Generate `core_capabilities` from the same table so the platform can feature-detect
  instead of calling methods that may not exist in an older core build.
- Leave `jni.rs` as-is (it's Android's proven path), but add a unit test that walks the
  table and asserts every `core_invoke` method name a consumer repo references (kept in
  a checked-in fixture list) still routes. That converts "renamed a method string"
  from an Android-runtime discovery into a test failure here.

## P4 — cross-repo wire contract

### 8. The wire format has no executable contract

Field names in `dispatch`/`completeEffect`/effect payloads are read by Kotlin/TS in
other repos; today the only protection is care. Two mechanisms, both cheap:

- **Golden fixtures**: a `tests/wire/` directory of checked-in JSON files — one real
  `AppAction` input and one expected `DispatchResult` output per action family, plus
  one payload sample per `EffectKind`. A snapshot test deserializes, dispatches, and
  compares serialized output structurally. Any refactor that changes camelCase output
  now fails a test in *this* repo instead of breaking Android in production. When a
  change is intentional, the diff in the fixture file is the review artifact the
  platform PRs can be checked against.
- **Fuzz the real boundary**: the existing fuzz targets cover parsers, but the highest-
  value target is missing — `headless_engine_dispatch_json` + `complete_effect_json`
  with arbitrary bytes and with structure-aware mutation of valid actions. The engine
  holding global state behind `catch_unwind`-free internals (dispatch is only guarded
  at the JNI layer, not in the desktop path via direct calls) makes this the place a
  panic or logic corruption would actually hurt. Add `engine_dispatch.rs` to
  `fuzz/fuzz_targets/`.

## P5 — performance

The release profile is already right (`lto = true`, `codegen-units = 1`, opt-level 3).
Keep `panic = "unwind"` — the FFI `catch_unwind` guards depend on it; `panic = "abort"`
would turn every guarded panic into a process kill. The actual costs found:

### 9. Two full state clones per dispatch

`headless_engine_dispatch_json` (`mod.rs:88-100`) clones the entire `EngineState`
before the reducer runs, clones it again after, then `StatePatch::diff` does a deep
`PartialEq` walk of all 16 domains and clones each changed one a third time. Cost
scales with everything the user has ever loaded (full catalogs, episode lists), not
with what the action touched — on a TV device after a long session this is the
dispatch latency.

Fix without changing the wire format: replace clone-and-diff with dirty tracking.
Each domain module already owns its state behind `pub(super)` setters — the setters
are the natural single place to flip a `dirty: DomainMask` bit on the engine.
`StatePatch` then clones only domains whose bit is set, and the before-clone
disappears entirely. `PartialEq` on the domain structs stays for tests. This is the
single highest-leverage performance change in the crate; do it before micro-work.

Second-order: `snapshot_json` and dispatch serialize while holding the global engines
mutex. Move `serde_json::to_string` outside the lock (clone the patch domains inside,
serialize after release) so a slow serialization of a big home state can't block a
concurrent `completeEffect`.

### 10. Per-call regex compilation in ranking loops

`stream_policy.rs:548,594,608` build a `Regex` from the user's language preference on
every call — and these run inside per-stream ranking. `external_sync.rs:65` compiles
`tt\d+` per history item; `watchlist_plan.rs:216` compiles a static GitHub-URL regex
per call. `content_identity.rs:980` already shows the house pattern (`OnceLock`).
Statics move to `OnceLock`; the preference-derived ones get a one-entry cache keyed by
the preference string (a `Mutex<Option<(String, Regex)>>` is enough — the preference
changes once per settings edit, not per stream).

### 11. Desktop pays JSON tax it doesn't owe — RESOLVED, already fixed on the desktop side

Verified against `fluxa-desktop/src-tauri` call sites (2026-07) per the CLAUDE.md rule, and
this section's premise no longer holds. The hottest paths — `dispatchAction`,
`completeEffect`, `getSnapshot` in `src/core/engine.ts` — already go through dedicated Tauri
commands (`engine_dispatch`, `engine_complete_effect`, `engine_snapshot` in
`src-tauri/src/lib.rs`) that call `FluxaCore::headless_engine_dispatch_json` etc. directly in
Rust, not through `core_invoke`. `FluxaCore` has also grown from the "8 methods" this doc
assumed to 21 (cast/AirPlay/Chromecast/Roku helpers included), all with real desktop call
sites. The one remaining `core_invoke` Tauri command is a generic fallback for the long tail
of infrequent methods (`playbackPreparePlan`, `preferencesSchema`, etc.) — writing a
dedicated Tauri command per method there would be pure boilerplate, since Tauri IPC already
serializes args as JSON crossing the JS↔Rust boundary regardless of whether the command is
generic or dedicated; there's no double-parse to eliminate. Nothing left to do here.

### 12. Measure before and after

Add a `benches/` with criterion (dev-dependency only): dispatch on a realistic large
state (fixture from §8's golden files), `StatePatch::diff` vs dirty-tracking, stream
ranking with 500 streams. Without these, §9-§11 are guesses; with them, regressions
show up in review.

## P6 — enforcement so it stays fixed

### 13. Lint and CI gates

There is no CI in this repo and no lint policy in `Cargo.toml`. Add:

```toml
[lints.rust]
unsafe_op_in_unsafe_fn = "warn"

[lints.clippy]
unwrap_used = "warn"
expect_used = "warn"
indexing_slicing = "warn"
panic = "warn"
```

`warn` not `deny` initially — test modules use these legitimately; add
`#![cfg_attr(test, allow(clippy::unwrap_used, clippy::expect_used))]` and ratchet to
`deny` once the count is zero. CI (GitHub Actions, single workflow): `cargo test --lib`,
`cargo clippy -- -D warnings` on default features, `cargo check --no-default-features
--features wasm` (allowing its known dead-code noise per CLAUDE.md), and `cargo fmt
--check`. The wasm job is what catches "compiles on Android, broken on webOS" cfg
mistakes.

## Sequencing

| Order | Work | Sections | Size |
|---|---|---|---|
| 1 | Fix failing DV test, stand up CI | §1, §13 | small |
| 2 | Golden wire fixtures + dispatch fuzz target | §8 | small-medium |
| 3 | `CoreError` + log sink at boundaries | §2, §3 | medium, mechanical |
| 4 | Dirty-flag `StatePatch`, serialize outside lock, benches | §9, §12 | medium |
| 5 | Typed `Profile`/`MetaItem`/`Stream` wire structs | §4 | large, incremental |
| 6 | `EffectKind` on envelope, mode enums, payload structs | §5, §6 | medium |
| 7 | `core_methods!` table + capability generation | §7 | medium |
| 8 | Desktop typed fast path | §11 | small |

Steps 2 and 3 are what make step 5 safe: once golden fixtures exist and parse failures
are logged instead of swallowed, converting `Value` fields to typed structs can be done
aggressively — any drift shows up as a fixture diff or a logged parse error instead of
a silent feature death on a platform we can't see from this repo.