fix(video): align the CLI and server video-frames fallback (per-clip lead sentence, temp file mode, blocking probes, request cloning, cancellable decode) #1766

Summary

PR #1759 (issue #1322) landed the video-frames fallback: a clip is decoded, evenly subsampled to --video-max-frames, PNG-encoded, and spliced back into the request as ordered image_url parts for a checkpoint that has a vision tower and no native temporal path. Its review and security passes reported five MEDIUM/LOW items that were deliberately left out of that PR as pre-existing or out of that issue's scope; they are listed in the "Reported and not fixed" paragraph of the #1759 body and in TECHNICAL_REPORTS/1759-video-frames-fallback-20260910.en.md. All five sit on src/multimodal/video.rs, src/server/chat_request.rs, src/server/media.rs and src/commands/generate.rs, so they are one issue and one PR. Every citation below was re-verified against e3ca8afc.

Current behavior

1. One lead sentence for all clips on the CLI, one per clip on the server. expand_cli_videos_to_frames sums the frame count across every --video flag at src/commands/generate.rs:942, builds a single lead_text from that sum at src/commands/generate.rs:957-964, and run_generate_once prepends it once at src/commands/generate.rs:2529. The server does the opposite: apply_video_frame_expansion pushes one video_frames_lead_text(frames.len()) per clip at that clip's own position (src/server/chat_request.rs:568-578, sentence at src/server/chat_request.rs:367). Two clips through the CLI therefore reach the model as one undivided run of frames announced with a summed count, and the model has no way to tell where the first clip ends and the second begins. The same two clips through the server arrive correctly separated, so the two fronts answer the same question differently.

2. CLI frame PNGs are world-readable in a shared directory. expand_cli_videos_to_frames writes every frame into std::env::temp_dir() (src/commands/generate.rs:918) with a plain std::fs::write at src/commands/generate.rs:944-953, which creates mode 0644 minus umask. This matches the pre-existing write_video_temp_file at src/server/media.rs:996-1015 and was reported as inherited rather than introduced. Any local user can read the decoded frames of a private clip for the life of the run.

3. Two blocking calls run on a Tokio worker. expand_video_parts_to_frames calls video_dir_allowlist_from_env() at src/server/chat_request.rs:392 and ffmpeg_available() at src/server/chat_request.rs:457, both from async context with no spawn_blocking. ffmpeg_available() (src/multimodal/video.rs:889-909) spawns ffmpeg -version and ffprobe -version; it is memoized behind a OnceLock, so only the first caller pays, but that first caller is a Tokio worker on the first video request of the process. video_dir_allowlist_from_env() (src/server/media.rs:585-608) is not memoized at all: it re-reads the env var and std::fs::canonicalizes each entry on every request, at src/server/chat_request.rs:392 and again at src/server/media.rs:544 on the native-video path.

4. The prompt-inspection route clones the whole request. render_chat_prompt takes &ChatCompletionRequest (src/server/routes/prompt_inspection.rs:95-98) and clones it wholesale at src/server/routes/prompt_inspection.rs:119 whenever the body carries a clip, duplicating every caller-supplied base64 image payload and every tool definition alongside the messages it is about to rewrite.

5. The decode is not cancellable. The ffmpeg decode plus PNG encode runs in tokio::task::spawn_blocking at src/server/chat_request.rs:484-495. A dropped JoinHandle does not cancel a blocking task, so when the client disconnects the handler future is dropped while the decode keeps running to --video-max-frames, once per clip, for a response nobody will read.

Expected behavior

1. The CLI splices one lead sentence per clip, naming that clip's own frame count, immediately ahead of that clip's frames, matching the server.
2. CLI frame PNGs are created mode 0600 inside a per-run directory created mode 0700, and both the files and the directory are removed when the run ends.
3. No request-path await point is blocked by a process spawn or a filesystem canonicalization for the video fallback.
4. Rendering a prompt for a body carrying a clip does not duplicate the caller's image payloads.
5. A client disconnect stops the fallback before the next clip is decoded and before a queued decode starts.

Implementation plan

1. Per-clip lead sentence on the CLI. Change CliVideoFrames (src/commands/generate.rs:870-878) so lead_text: String becomes an ordered per-clip structure: carry Vec<(String, Vec<PathBuf>)> of (sentence, that clip's frame paths) instead of one sentence plus one flat frame_paths. In the loop at src/commands/generate.rs:922-955 build one format!("Here is a video as a sequence of {} frames in chronological order.", encoded.len()) per clip from encoded.len(), not from the running total_frames. At the call site (src/commands/generate.rs:2521-2534) prepend the sentences in clip order ahead of user_prompt, one per clip, and extend args.generation.image in the same order, so the Nth sentence still precedes the Nth clip's frames once load_cli_prompt emits one image content part per frame. Reuse the server's wording verbatim; better still, export video_frames_lead_text (src/server/chat_request.rs:367) and call it from both fronts so the sentence cannot drift, adding the // Used by: comment the repository requires for a shared function.

2. Private directory, 0600 frames. Create temp_dir().join(format!("mlxcel-video-frames-{}", Uuid::new_v4())) once per expand_cli_videos_to_frames call with std::fs::DirBuilder::new().mode(0o700) under #[cfg(unix)] and a plain create_dir elsewhere, replacing the bare std::env::temp_dir() at src/commands/generate.rs:918. Write each PNG through OpenOptions::new().write(true).create_new(true).mode(0o600) (std::os::unix::fs::OpenOptionsExt) under #[cfg(unix)], falling back to std::fs::write elsewhere, in place of the write at src/commands/generate.rs:946. TempFile (src/multimodal/video.rs:491-519) only unlinks a file, so add a sibling directory guard beside it that calls remove_dir_all on drop, and hold it in CliVideoFrames::_guards so it outlives the vision-tower read exactly as the file guards do. Use std, not the tempfile crate: tempfile = "3" is a dev-dependency only (Cargo.toml:343-345) and promoting it to [dependencies] for this is not worth the release surface. Leave write_video_temp_file (src/server/media.rs:996-1015) alone: it is the server's native-video path and a separate change.

3. Move the blocking work to startup. Call ffmpeg_available() once during server startup so the OnceLock at src/multimodal/video.rs:891 is already warm when the first request arrives, and keep the request-path call at src/server/chat_request.rs:457 as the cheap memoized read it then is. For the allowlist, add a resolved Arc<Vec<PathBuf>> field on AppState next to media_support (src/server/state.rs:466, builder at src/server/state.rs:734, defaults at src/server/state.rs:647 and :706), populate it during startup where warn_on_insecure_video_allowlist already reads the env var (src/server/startup.rs:1089), and have the three routes pass it to the existing expand_video_parts_to_frames_with_allowlist (src/server/chat_request.rs:403) and extract_chat_video_paths_with_allowlist (src/server/media.rs:547). Do not put a OnceLock inside video_dir_allowlist_from_env() itself: the _with_allowlist variants exist because MLXCEL_VIDEO_DIR_ALLOWLIST is process-global and the tests inject a controlled allowlist rather than hold the crate env lock across awaits, and memoizing the getter would break them.

4. Take the request by value. All three callers of render_chat_prompt already own the value they pass: src/server/routes/prompt_inspection.rs:204 and :234 pass a locally parsed ChatCompletionRequest, and :273 passes translated.chat_request, which translated owns. Change the parameter to mut request: ChatCompletionRequest, expand in place, and delete the clone and the expanded local at src/server/routes/prompt_inspection.rs:117-130. Do not introduce a Cow or a borrowed view type; the ownership already exists at every call site.

5. Thread a cancellation token. Use tokio_util::sync::CancellationToken (tokio-util is already a direct dependency at Cargo.toml:229, and src/distributed/heartbeat.rs:48 already uses it), not the Arc<AtomicBool> alias at src/server/streaming.rs:139, which belongs to the SSE sender and does not exist yet at this seam: the expansion runs at src/server/routes/chat.rs:368, before the SSE channel is created. In each wiring route (src/server/routes/chat.rs:368, src/server/routes/responses.rs:111, src/server/routes/prompt_inspection.rs:120) create a token, hold its drop_guard() in the handler future so a client disconnect cancels it when the future is dropped, and pass the token into expand_video_parts_to_frames*. Check it in three places: once before the clip loop, once per clip immediately before resolve_video_url, and at entry to the spawn_blocking closure (src/server/chat_request.rs:484-495), which matters because that closure can sit queued behind other blocking work. Return a distinct Err variant the routes drop rather than render as a 400. Out of scope, and stated so an implementer does not attempt it: killing an ffmpeg child that is already mid-decode. extract_frames_single_pass (src/multimodal/video.rs:1729) has three callers in that file (:1279, :1442, :1636), so threading a kill handle through it is a shared-function change belonging to its own issue. What this buys is a bound: at most one clip's decode is wasted instead of the whole body's.

Validation

Existing scopes, all of which this change touches:

cargo test --profile test-fast --features metal,accelerate --lib multimodal::video
cargo test --profile test-fast --features metal,accelerate --lib server::chat_request
cargo test --profile test-fast --features metal,accelerate --lib server::startup
cargo test --profile test-fast --features metal,accelerate --lib server::media
cargo test --profile test-fast --features metal,accelerate --bin mlxcel commands::generate

commands::generate_tests lives in the binary crate, not the lib, so --lib commands::generate matches zero tests and the --bin mlxcel scope is the correct one (#1759 report section 5.6).

New tests required:

• Item 1, in src/commands/generate_tests.rs, alongside cli_video_fallback_appends_frame_images_and_clears_video: two clips produce two lead sentences, each naming its own clip's frame count rather than the sum, and each sentence precedes its own clip's frames in the rendered prompt.
• Item 2, in src/commands/generate_tests.rs under #[cfg(unix)]: the per-run frame directory is mode 0700 and every PNG in it is mode 0600, and both the files and the directory are gone once the guards drop.
• Item 5, in src/server/chat_request_tests.rs: a token cancelled before the call returns the cancellation error without invoking ffmpeg, and a token cancelled after the first clip stops before the second is resolved.

Real run on models/gemma-3-4b-it-4bit with ffmpeg at /opt/homebrew/bin, using two short synthetic clips. Generate them with overlay, which produces visible motion:

ffmpeg -f lavfi -i "color=c=white:s=448x448:d=4:r=25" -f lavfi -i "color=c=red:s=128x128:d=4:r=25" -filter_complex "[0:v][1:v]overlay=x='(W-w)*t/4':y=(H-h)/2" -c:v libx264 -pix_fmt yuv420p clip_a.mp4
ffmpeg -f lavfi -i "color=c=white:s=448x448:d=4:r=25" -f lavfi -i "color=c=blue:s=128x128:d=4:r=25" -filter_complex "[0:v][1:v]overlay=x=(W-w)/2:y='(H-h)*t/4'" -c:v libx264 -pix_fmt yuv420p clip_b.mp4

Do not build the fixture with drawbox and a t-dependent x: it renders four seconds of blank white, and the run then looks like a successful validation because the log line, the frame count and the token growth are all correct while every model simply describes a blank frame (#1759 report section 5). Check the first and last frame by eye before trusting any number.

Then confirm on both fronts, at --video-max-frames 4, greedy decoding, the same user text:

• CLI: ./target/release/mlxcel generate -m models/gemma-3-4b-it-4bit --video clip_a.mp4 --video clip_b.mp4 --video-max-frames 4 --profile -p "..." emits two lead sentences, each naming 4 frames, and --profile reports the Prompt tokens: line (src/lib/mlxcel-core/src/generate.rs:1331).
• Server: ./target/release/mlxcel-server -m models/gemma-3-4b-it-4bit --video-max-frames 4 with MLXCEL_VIDEO_DIR_ALLOWLIST set to the clips' directory, posting both clips in one /v1/chat/completions body, logs the fallback line twice and returns HTTP 200.
• The CLI's Prompt tokens: and the server's usage.prompt_tokens are identical for that body, which is the check that the two fronts now splice the same sentences at the same positions. They differ today, by the token cost of one lead sentence, because the CLI emits one sentence where the server emits two.
• POST /v1/chat/completions/input_tokens on the same body returns that same count, which exercises item 4's now-uncloned call path.

Acceptance criteria

• The CLI emits one lead sentence per --video clip, naming that clip's own frame count, spliced ahead of that clip's frames.
• CLI frame PNGs are mode 0600 inside a mode 0700 per-run directory, and both are removed when the run ends.
• ffmpeg_available() and the video directory allowlist are resolved at startup; no request-path call spawns a process or canonicalizes a path for the fallback.
• render_chat_prompt takes the request by value; the whole-request clone at src/server/routes/prompt_inspection.rs:119 is gone.
• A cancellation token reaches expand_video_parts_to_frames* from all three wiring routes and is checked before the clip loop, per clip, and at spawn_blocking entry.
• New tests for items 1, 2 and 5 exist and pass, and the existing multimodal::video, server::chat_request, server::startup, server::media and commands::generate scopes stay green.
• The real run above shows two lead sentences on both fronts and identical prompt-token counts between the CLI and the server for the same two clips.
• cargo test --workspace --profile test-fast --features metal,accelerate passes.
• cargo clippy --workspace --all-targets -- -D warnings and cargo fmt --check are clean.

Dependencies

Follow-up of #1322 (PR #1759).