Skip to content

Make factory/model the single source of truth for streaming capability #11

Description

@BumpyClock

Problem

Streaming support is declared in two places that must be kept in lockstep:

  1. On the loaded engine via SttEngine::supports_streamingcrates/core/src/transcription/engine_trait.rs:34 (default false).
  2. On the factory/model via SttEngineFactory::supports_streamingcrates/core/src/transcription/engine_trait.rs:80 (default false).

TranscriptionEngineState exposes both as well: an engine-first supports_streaming() and a lock-free, registry-resolved model_supports_streaming().

crates/core/src/transcription/engine.rs:587supports_streaming() peeks the resident engine, else falls back to the model:

/// Whether the selected engine supports streaming transcription.
pub fn supports_streaming(&self) -> bool {
    {
        let slot = self.engine.lock();
        if let Some(engine) = slot.engine.as_ref() {
            return engine.supports_streaming();
        }
    }
    self.model_supports_streaming()
}

crates/core/src/transcription/engine.rs:604model_supports_streaming() resolves the same answer from the registry without taking the engine inference lock. Its own doc comment states the equivalence: "The registry's per-model capability matches what the loaded engine reports for the same model, so the answer is equivalent."

The only production caller is the controller, and it already uses the lock-free path — crates/core/src/controller.rs:291:

let use_streaming = self.transcription.model_supports_streaming();

So TranscriptionEngineState::supports_streaming() is production-unused — only tests call it (engine.rs:968, engine.rs:976, engine.rs:979). Its resident-engine branch adds no distinct behavior over model_supports_streaming() given the documented equivalence.

The cost shows up per backend. crates/core/src/transcription/nemotron.rs sets the flag twice: the factory at nemotron.rs:70 and the engine at nemotron.rs:127 (fn supports_streaming(&self) -> bool { true }). Every new backend must remember to set both, with no compiler enforcement that they agree.

Why it matters

  • Maintainability: Two capability declarations per backend that must stay consistent, with no enforced coupling — a silent drift surface (engine says streaming, factory says batch, or vice versa).
  • Dead surface: TranscriptionEngineState::supports_streaming() is reachable only from tests; it implies a meaningful engine-vs-model distinction that the code explicitly documents as equivalent.
  • Low blast radius: the lock-free model_supports_streaming() is the one path the controller hits on every dictation start, so consolidating onto it removes the rarely-correct lock-taking variant.

Proposed change

Make the factory/model capability the single source of truth:

  1. Delete TranscriptionEngineState::supports_streaming() (engine.rs:587), or make it delegate to model_supports_streaming() if an engine-side entry point is still wanted.
  2. Keep model_supports_streaming() (engine.rs:604) and streaming_chunk_ms() (engine.rs:620) on the factory as-is.
  3. SttEngine::supports_streaming (engine_trait.rs:34) stays — it still gates the per-utterance streaming lifecycle on the trait — but it stops being a second capability contract the state layer reads for routing decisions.

Tests that must change:

  • streaming_engine_lifecycle (engine.rs:972) asserts state.supports_streaming() twice — once before warm-up (engine.rs:976, model path) and once after warm-up (engine.rs:979, loaded-engine path). Repoint both to model_supports_streaming(), or keep one delegation test if supports_streaming() is retained as a thin delegate.
  • batch_engine_does_not_support_streaming (engine.rs:960) asserts !state.supports_streaming() after warm-up (engine.rs:968) — repoint to model_supports_streaming().

Recommendation / triage

DO — clean single-source-of-truth, low risk. Caveat: the two tests asserting state.supports_streaming() after warm_up (engine.rs:979, engine.rs:968) cover the loaded-engine path; update them to model_supports_streaming() or keep one delegation test. Verify no other production caller before deleting — confirmed at time of writing that only controller.rs:291 reads streaming capability in production and it uses model_supports_streaming().

Acceptance criteria

  • TranscriptionEngineState::supports_streaming() is removed, or delegates to model_supports_streaming() (no independent resident-engine branch).
  • No production code reads streaming capability through the engine inference lock; controller.rs:291 continues to use model_supports_streaming().
  • Streaming-capability tests pass against model_supports_streaming() (or a single delegation test): streaming_engine_lifecycle, batch_engine_does_not_support_streaming.
  • Behavior unchanged: a streaming model still routes to the streaming live-preview path and a batch model still routes to batch — proven by the updated streaming_engine_lifecycle and batch_engine_does_not_support_streaming tests plus the existing streaming-session tests (streaming_session_resets_state_between_sessions, streaming_feed_resets_after_unload_reload).
  • No new backend needs to set two streaming flags for the state layer's routing decision (the SttEngine::supports_streaming trait hook may remain for the per-utterance lifecycle).

Traceability

  • clawpatch finding ID: fnd_sig-feat-service-25552574c9-ecce_85857f5057
  • Revalidate: clawpatch revalidate --finding fnd_sig-feat-service-25552574c9-ecce_85857f5057 --reasoning-effort high
  • Planning follow-up; no coupled issue key at this time.

Metadata

Metadata

Assignees

No one assigned

    Labels

    deslopifyFound by clawpatch deslopify reviewtech-debtCode quality / maintainability cleanup

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions