Manage subprocess and encode lifecycles explicitly - #7
Open
mjc wants to merge 13 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis change replaces direct FFmpeg process handling with managed processes, adds planned encoding output lifecycles, shares score streaming between VMAF and XPSNR, strengthens sampling and cache behavior, updates CRF search, and expands CI validation. ChangesEncoding and process pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 10, 2026
mjc
changed the title
mjc
force-pushed
the
mjc/abav1-214-stack-00-managed-process-lifecycle
branch
from
August 11, 2026 16:02
d5a1ded to
9d32cd3
Compare
mjc
marked this pull request as ready for review
August 11, 2026 16:04
mjc
force-pushed
the
mjc/abav1-214-stack-00-managed-process-lifecycle
branch
from
August 11, 2026 16:09
9d32cd3 to
733be68
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors subprocess ownership and ffmpeg-driven scoring/encoding into explicit lifecycle-managed components, replacing ad-hoc child-process management with ManagedProcess-based policies and shared stream parsing utilities. It also tightens cache key identity, temp cleanup behavior, and expands CI/testing to cover the new lifecycle contracts.
Changes:
- Introduces
ManagedProcess-based streaming for ffmpeg progress and scoring (newscore_streamhelper, updated VMAF/XPSNR runners). - Reworks encode execution into explicit phases (plan/preflight/run/report) with guarded temporary outputs and improved cleanup semantics.
- Expands tests and CI (nextest + coverage) to validate lifecycle behavior, parsing edge cases, and cleanup guarantees.
Reviewed changes
Copilot reviewed 38 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/process.rs | Replaces chunk-stream child handling with managed-process stderr events and explicit completion tracking. |
| src/score_stream.rs | Adds shared “score stream” runner to unify VMAF/XPSNR parsing and lifecycle semantics. |
| src/vmaf.rs | Migrates VMAF scoring to managed process + shared score stream; improves parsing robustness. |
| src/xpsnr.rs | Migrates XPSNR scoring to managed process + shared score stream; adjusts parsing and adds fixtures. |
| src/temporary.rs | Adds CleanupGuard, improves directory cleanup semantics, and refines temp-dir selection. |
| src/sample.rs | Splits sample destination computation and ffmpeg command building; adds retry detection and tests. |
| src/ffmpeg.rs | Moves ffmpeg spawn to ManagedProcess, adds EncodeDestination abstraction, normalizes codec suffix logic. |
| src/ffprobe.rs | Improves probe fallback behavior, parsing robustness, and adds test seams and coverage. |
| src/float.rs | Adjusts “terse float” formatting heuristics and adds property/edge-case tests. |
| src/command/encode/* | Replaces monolithic encode command with preflight/plan/run/report modules and test scaffolding. |
| src/command/auto_encode.rs | Reuses encode preflight validation (same-file, downmix/copy) and adds targeted tests. |
| src/command/sample_encode/cache.rs | Refactors cache key construction to typed identities and expands cache-key coverage tests. |
| .github/workflows/ci.yml | Switches to nextest, adds coverage job, and installs ffmpeg for tests. |
| Cargo.toml | Updates toolchain/deps (tokio-process-tools, thiserror) and adds test dependencies. |
| AGENTS.md / CLAUDE.md | Adds contributor/agent workflow guidance (bd/beads + non-interactive shell notes). |
| proptest-regressions/* | Checks in regression seeds to stabilize proptest reproductions. |
Suppressed comments (1)
src/command/sample_encode/cache.rs:164
SourceIdentityis constructed withsample(full path). After switching tosample_name, populate it fromsample.file_name()so the cache key no longer depends on the per-run temp directory component.
Self {
source: SourceIdentity {
sample,
source_input,
input_duration,
input_extension,
input_size,
full_pass,
dest_ext,
},
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 47
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.rs (1)
64-67: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftWait for managed-process shutdown before temporary cleanup
When
ctrl_cwins,drop(local)drops active command futures beforetemporary::clean(keep).await.terminate_on_droponly spawnsprocess.terminate_after(...), so cleanup can run before score-process termination completes. Encode streams usemust_complete; dropping a live process panics instead of waiting. Add a shutdown barrier before cleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 64 - 67, Update the cleanup flow in main around drop(local) and temporary::clean so managed child-process shutdown completes before temporary files are removed. Add and await a shutdown barrier for the process termination work triggered by terminate_on_drop, while preserving the existing cleanup behavior and must_complete handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 36-37: Remove the duplicate cargo-nextest installation step in the
coverage job, retaining exactly one cargo install cargo-nextest --locked
command.
- Around line 35-37: Update the cargo install steps in the CI workflow to
include explicit, known-good --version values for both cargo-llvm-cov and
cargo-nextest, while retaining --locked. Remove the duplicate cargo-nextest
installation so each tool is installed once with a pinned version.
- Line 17: Update each active actions/checkout@v6 step, including the steps near
lines 17, 33, and 59, to set persist-credentials to false. Keep the existing
checkout behavior and apply the setting consistently wherever subsequent
repository commands run.
- Line 17: Update the active actions/checkout and actions/upload-artifact
references in the CI workflow to trusted, full-length commit SHAs instead of
mutable version tags, preserving their current action versions and
configuration.
- Around line 38-43: Reorder the CI coverage steps so the `cargo llvm-cov
nextest --locked --lcov --output-path lcov.info` command and
`actions/upload-artifact@v4` upload run before the `--fail-under-lines 80`
enforcement command, ensuring `lcov.info` is generated and uploaded even when
the threshold check fails.
In `@AGENTS.md`:
- Around line 22-31: Add blank lines before and after the file-operation fenced
code block in AGENTS.md lines 22-31, and before and after the session-completion
command blocks in AGENTS.md lines 69-74 and CLAUDE.md lines 35-40, resolving the
MD031 fence-spacing findings without changing their contents.
- Around line 3-13: Remove the duplicate Beads quick-reference block in
AGENTS.md, retaining a single authoritative instruction source that includes the
complete workflow commands, including bd dolt push.
In `@CLAUDE.md`:
- Around line 53-69: Replace the placeholder content in the Build & Test,
Architecture Overview, and Conventions & Patterns sections of CLAUDE.md with
accurate Rust build/test commands and project-specific guidance; remove the
unrelated npm example commands if the sections cannot yet be completed.
In `@src/command/auto_encode.rs`:
- Line 208: Remove the module-local AUTO_ENCODE_TEST_LOCK and its lock calls,
then add #[serial] to every #[tokio::test] in the auto-encode test module.
Ensure all tests that mutate TEMPS use the shared serial_test mechanism,
including the tests covering the range through the module’s final test.
In `@src/command/crf_search.rs`:
- Around line 25-34: Update output_search_score to return an explicit error when
both candidate scores are unavailable, matching
sample_encode::Output::single_score’s NaN behavior instead of defaulting to 0.0.
Propagate this missing-score error through the CRF search flow so it cannot
produce a NoGoodCrf result for an unmeasured sample, while preserving the
existing XPSNR/VMAF priority selection.
- Around line 614-637: Update vmaf_lerp_q to explicitly handle vmaf_diff values
that are not greater than 0.0 before calculating vmaf_factor and lerp; preserve
the existing valid-range result, either by documenting that the current
saturating conversion remains safe or by computing lerp only when vmaf_diff >
0.0.
- Around line 453-466: Update the adjacent-lower-bound branch in the CRF search
flow around vmaf_lerp_q to handle lower scores that are equal to or below the
upper sample’s score without calling the assertion-based interpolation. Preserve
the existing Done(lower) path when lower_score meets min_score, and otherwise
choose a safe non-panicking outcome for non-monotonic samples. Add a regression
test covering adjacent q values that both miss min_score with the lower sample
scoring no higher than the upper sample.
- Around line 405-412: Update the non-thorough score-band logic in the search
flow around `within_non_thorough_band` so it aligns with the growth of
`higher_tolerance` for successive `crf_increment` values. Remove the hard-coded
`min_score + 0.11` cap or derive the band from `higher_tolerance`, preserving an
upper bound only if explicitly intended, and avoid triggering unnecessary search
iterations.
In `@src/command/crf_search/err.rs`:
- Around line 141-157: Update the assertion in
no_good_crf_display_includes_last_crf to compare the complete expected error
message exactly, reusing the expected formatting established by
ensure_or_no_good_crf_ok_and_no_good_crf, so truncated values such as “37” do
not satisfy the test.
In `@src/command/encode/error.rs`:
- Around line 18-28: Update PartialEq for EncodePlanError so the FfmpegArgs
variant compares reflexively by comparing its wrapped error values through their
Display output, or remove the Eq implementation if that equality cannot be
supported. Ensure every variant satisfies the Eq contract, including err == err
for FfmpegArgs.
In `@src/command/encode/lifecycle.rs`:
- Around line 113-121: Both cleanup tests assert the final output path instead
of the temporary path, making them vacuous. In src/command/encode/lifecycle.rs
lines 113-121, capture PlannedOutput::path() before the guard drops and assert
that temporary path is absent after temporary::clean_all().await; in
src/command/encode/mod.rs lines 171-195, compute the expected temporary path for
output, run temporary::clean_all().await before asserting, and verify that
temporary path is absent.
In `@src/command/encode/mod.rs`:
- Around line 197-214: Add the existing #[serial] attribute to the
run_rejects_same_input_and_output_without_overwrite test, matching the other
tests that invoke run or run_with_spawner and preserving serialized access to
the shared fixture state.
In `@src/command/encode/plan.rs`:
- Around line 58-61: Remove the dead enc_args.video_only assignment in the
preflight validation block while retaining the encode.to_ffmpeg_args call and
drop(enc_args) validation flow.
In `@src/command/encode/preflight.rs`:
- Around line 73-93: Remove the local probe and temp_input helper
implementations in the preflight tests, and import and reuse
test_support::test_probe and test_support::temp_input from
crate::command::encode::test_support. Update their call sites as needed while
preserving the existing test behavior.
In `@src/command/encode/sink.rs`:
- Around line 10-22: Update the ProgressSink implementation for
indicatif::ProgressBar so set_message, set_position, and finish invoke the
corresponding indicatif::ProgressBar methods explicitly, preventing accidental
recursive dispatch while preserving the existing arguments and behavior.
In `@src/command/encode/spawner.rs`:
- Around line 76-96: Update ThreadLocalFixtureSpawner::spawn to fail explicitly
when test_hooks::fixture() returns None instead of falling back to
FfmpegSpawner.spawn, preventing an actual ffmpeg process from being launched in
tests; preserve the fixture-backed test_ffmpeg_stream path when a fixture is
available.
In `@src/command/encode/test_support.rs`:
- Around line 56-69: Add the #[must_use] attribute to the FixtureGuard struct so
callers must retain the guard returned by FixtureGuard::set and cannot
accidentally clear the fixture immediately.
- Around line 40-48: Update the test-support setup around FIXTURE_TEST and
Command::new: move the fixture test-name constant next to
managed_process_fixture_child in the process managed module, make it public as
needed, and import that shared symbol here instead of duplicating the path.
Replace current_exe().expect(...) with error propagation that adds context while
preserving the function’s anyhow::Result flow.
In `@src/command/sample_encode.rs`:
- Around line 1310-1324: Rename the test function
estimate_encode_time_scales_and_truncates_to_seconds to
estimate_encode_time_scales_by_duration_ratio, leaving its setup, execution, and
assertions unchanged.
- Around line 506-539: Update score_json::serialize to explicitly account for
positive and negative infinity alongside NaN, either by documenting the existing
serde_json null serialization behavior or handling all non-finite scores
consistently. Preserve the current mean-function behavior that filters
non-finite scores and ensure the chosen behavior is clear and intentional.
- Around line 988-1001: Extract the production formulas currently duplicated by
sample_grid_divisor and encode_progress_ratio into private helpers, then call
those helpers from both the production paths and tests in
src/command/sample_encode.rs:988-1001. Update
attempt_percentages_finite_when_sample_size_zero at
src/command/sample_encode.rs:1135-1154 to exercise EncodeResult::log_attempt or
a shared percentage helper instead of duplicating percentage math. In
src/command/crf_search.rs:988-1001, make threshold_success_matrix invoke the
real acceptance predicate used by run, or remove the test if that integration is
not feasible.
- Around line 486-493: Update the sample-offset calculation in the surrounding
function to clamp the `samples` and `sample_idx` operands to `u32::MAX` before
multiplying with `Duration`, alongside the existing `grid_divisor` clamp. Ensure
all `Duration * u32` operations use bounded values so large inputs cannot
truncate unexpectedly or panic from overflow.
- Around line 541-554: Add #[serde(default)] to both vmaf_score and xpsnr_score
in EncodeResult so omitted score fields deserialize as None while retaining the
custom serializers. Add coverage for deserializing JSON that omits xpsnr_score
and confirm cached_encode accepts it without triggering a cache error.
In `@src/command/sample_encode/cache.rs`:
- Around line 109-207: Update the cache-key migration around CacheKeyBuilder and
encode_cache_key to introduce an explicit key namespace or format version, and
add a one-time cleanup of entries from the previous namespace so obsolete sled
rows are removed after the key-format change. Preserve current key inputs and
ensure cleanup is safe and runs only once per upgrade.
In `@src/ffmpeg.rs`:
- Around line 252-259: Update remove_arg or the sample-encode call sites for
-fps_mode and -vsync so every occurrence of each flag, along with its associated
value, is removed from enc_args.output_args before sample encoding appends its
enforced settings. Preserve the existing single-occurrence behavior for other
callers unless introducing and using a dedicated all-occurrences helper.
- Around line 197-205: Update pre_extension_name so libvpx maps to vp8,
libvpx-vp9 remains mapped to vp9, and libdav1d is removed from the AV1 mapping.
Adjust the matching tests to assert these codec-specific suffixes and the
decoder-only codec’s fallback behavior.
In `@src/process.rs`:
- Around line 229-244: Add a + Send bound to the boxed event stream field in
FfmpegOutStream so the stream remains Send and can be used with tokio::spawn.
Preserve the existing Pin, Stream item type, and event handling behavior.
In `@src/process/managed.rs`:
- Around line 173-190: Update spawn_with_options to use options.stderr_limit
instead of DEFAULT_STDERR_LIMIT.bytes() when configuring replay_last_bytes, so
the replay buffer honors the caller’s with_stderr_limit setting.
- Around line 327-336: Update managed_event_from_stream_event to return only the
event variants it can produce, excluding ProcessDone, and adjust its callers
accordingly. Remove the unreachable ProcessDone match arms at the referenced
state-machine branches, preserving handling for Chunk, Gap, Eof, and ReadError.
- Around line 75-89: Update TerminateOnDropProcess::drop to obtain the Tokio
runtime handle before calling self.0.take(). If no runtime is available, return
while preserving the process in the wrapper so its underlying drop guard still
terminates the child; only take and asynchronously terminate the process after a
handle is successfully acquired.
- Around line 386-414: Remove the zero-duration readiness probe and its inferred
OutputReplayGap emission from stderr_events; only StreamEvent::Gap converted by
managed_event_from_stream_event should produce a replay-gap marker. Start
consuming events through the existing loop and consolidate conversion/yield
handling there, eliminating the duplicated first-event match while preserving
ProcessDone, RawStderr, and stream termination behavior.
In `@src/sample.rs`:
- Around line 380-409: Gate copy_e2e_real_ffmpeg because it invokes the external
ffmpeg binary. Mark the test #[ignore] so normal test runs skip it, or
conditionally compile it behind a dedicated cargo feature enabled by the
devshell; keep the existing test behavior unchanged when explicitly run.
- Around line 336-378: Serialize every test in this module that accesses the
shared temporary registry, including copy_returns_existing_dest_without_spawning
and copy_succeeds_with_process_fixture, by applying the existing #[serial]
attribute; alternatively, replace temporary::clean_all() with cleanup limited to
paths created by the individual test.
- Around line 48-106: The duplicated command construction in src/sample.rs lines
48-106 must be consolidated: extract a start_secs helper and a single
build_copy_command(..., genpts: bool), then make copy_command,
copy_command_with_genpts, and sample_dest_path call those helpers. In
src/sample.rs lines 132-149, remove the hand-built fixture command and reuse
copy_command so copy_program and test_hooks::apply_fixture are applied
consistently.
- Around line 132-149: Remove the test-only early-return block that manually
builds a Command and writes a synthetic fixture output. Let the normal copy flow
through apply_copy_args and copy_program so the existing
test_hooks::apply_fixture wiring is exercised; if fixture output setup remains
necessary, perform it after ensure_success on that shared path.
In `@src/score_stream.rs`:
- Around line 98-113: Update the score-stream error handling around
ManagedEvent::ProcessDone and the subsequent logical_score check so the “could
not parse {name} score” error is emitted only when the process completed
successfully but no score was parsed. Track or reuse the process-failure result
from exit_ok_stderr, and suppress the parse error after a non-zero child exit
while preserving the existing exit-status error.
In `@src/temporary.rs`:
- Around line 105-130: Remove the redundant temp_dir.exists() conditional in
process_dir and call std::fs::create_dir_all(&temp_dir) unconditionally,
preserving the existing error context and return behavior.
- Around line 221-251: Update the tests
default_temp_dir_uses_input_directory_ab_kgc_11 and
explicit_temp_dir_overrides_input_directory_ab_kgc_11 to clean up their created
run directories and parent test directories at the end of each test, and remove
their corresponding entries from the global TEMPS map so later serial tests
cannot delete them unexpectedly.
- Around line 31-58: Remove the unused registered field from CleanupGuard and
simplify arm and disarm accordingly, while preserving path registration in arm
and unregistration plus path return in disarm. Do not add Drop behavior; retain
cleanup through the existing global mechanism.
In `@src/vmaf.rs`:
- Around line 66-72: The pure chunk parsers currently expose error variants that
are discarded via unreachable!; narrow try_from_chunk in src/vmaf.rs (lines
66-72) to a parse-only progress-or-score result and map it directly to
ScoreStreamParse without an unreachable arm. Apply the same return-type and
mapping change to the XPSNR parser in src/xpsnr.rs (lines 67-73), preserving
progress and completed-score behavior at both sites.
- Around line 74-90: Update try_from_chunk to use rfind_line_map so the
case-insensitive window predicate returns the matching byte index directly.
Reuse that index for score parsing and remove the subsequent find/or_else search
and lowercase allocations.
In `@src/xpsnr.rs`:
- Around line 121-128: Update the character predicate in parse_score_number to
accept only ASCII digits, while preserving the existing '-' and '.' handling and
numeric-prefix parsing behavior. Replace the Unicode-aware c.is_numeric() check
with an ASCII digit check so the computed end_idx always remains a valid UTF-8
boundary.
---
Outside diff comments:
In `@src/main.rs`:
- Around line 64-67: Update the cleanup flow in main around drop(local) and
temporary::clean so managed child-process shutdown completes before temporary
files are removed. Add and await a shutdown barrier for the process termination
work triggered by terminate_on_drop, while preserving the existing cleanup
behavior and must_complete handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6bb7c2d0-15f9-4d03-80ec-f6f699440feb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.github/workflows/ci.ymlAGENTS.mdCLAUDE.mdCargo.tomlproptest-regressions/.gitkeepproptest-regressions/command/crf_search.txtsrc/command/args.rssrc/command/args/encode.rssrc/command/args/vmaf.rssrc/command/auto_encode.rssrc/command/crf_search.rssrc/command/crf_search/err.rssrc/command/encode.rssrc/command/encode/error.rssrc/command/encode/lifecycle.rssrc/command/encode/mod.rssrc/command/encode/plan.rssrc/command/encode/preflight.rssrc/command/encode/progress.rssrc/command/encode/report.rssrc/command/encode/running.rssrc/command/encode/sink.rssrc/command/encode/spawner.rssrc/command/encode/test_support.rssrc/command/sample_encode.rssrc/command/sample_encode/cache.rssrc/command/xpsnr.rssrc/ffmpeg.rssrc/ffprobe.rssrc/float.rssrc/main.rssrc/process.rssrc/process/child.rssrc/process/managed.rssrc/sample.rssrc/score_stream.rssrc/temporary.rssrc/vmaf.rssrc/xpsnr.rs
💤 Files with no reviewable changes (2)
- src/command/encode.rs
- src/process/child.rs
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 29-48: Add a job-level permissions block to the coverage job,
granting only contents: read; include any additional permission only if required
by actions/upload-artifact. Keep the existing coverage steps unchanged.
In `@src/command/args/encode.rs`:
- Around line 1108-1136: Update the SVT-AV1 merge logic in
Encode::to_ffmpeg_args to recognize the canonical “-svtav1-params=…” form
produced by parse_enc_arg. Adjust to_ffmpeg_args_merges_enc_svtav1_params to
pass that canonical form, and ensure it is merged into the generated svtav1
parameters so only the combined option remains with tune, film-grain, and crf.
In `@src/command/crf_search.rs`:
- Around line 29-37: Update output_search_score so it returns only the metric
requested by use_xpsnr: require enc.xpsnr_score when true and enc.vmaf_score
when false, returning the existing missing-score error if unavailable instead of
falling back to the other metric. Update
output_search_score_uses_vmaf_fallback_when_xpsnr_missing to assert the missing
requested metric error.
- Around line 618-647: Replace the assertion in vmaf_lerp_q with anyhow error
validation that returns an error when sample scores violate the required
ordering or bounds, while preserving the existing interpolation behavior for
valid inputs. Add a regression test covering equal-scoring adjacent samples and
verify the search returns an error instead of aborting.
In `@src/command/encode/report.rs`:
- Around line 87-104: Add a test alongside
render_encode_summary_includes_stream_breakdown for the video-only case, using
StreamSizes with audio, subtitle, and other all zero. Assert the rendered output
still contains “Encoded” but omits the stream breakdown, such as “video:”, to
cover the has_non_video suppression branch.
In `@src/command/encode/running.rs`:
- Line 26: Rename the initial binding in the running command flow from output to
output_path, and update the associated log statement to use output_path. Keep
the later output binding for CompletedOutput unchanged.
In `@src/command/encode/spawner.rs`:
- Around line 87-95: Remove the unused `let _ = FfmpegSpawner;` statement from
the test-only fixture and update the surrounding comment so it no longer claims
to type-check the production spawner there. Preserve the `session.has_audio()`,
`session.stereo_downmix()`, and `session.audio_codec()` calls and their purpose
of keeping the accessors used in test builds.
In `@src/command/encode/test_support.rs`:
- Around line 67-74: Update temp_input to append the existing unique_suffix()
value to the generated temporary path, matching the auto-encode helper’s naming
scheme. Keep the process ID, scope, and label components intact while ensuring
concurrent callers with identical scope and label receive distinct files.
- Around line 35-42: Export the fixture environment name from the
managed-process test constants alongside MANAGED_PROCESS_FIXTURE_TEST as
MANAGED_PROCESS_FIXTURE_ENV, then update the encode test support setup to import
and use that shared constant instead of redeclaring FIXTURE_ENV locally. Ensure
managed_process_fixture_child and the child-command construction use the same
centralized value.
In `@src/command/sample_encode.rs`:
- Around line 783-790: Update the duration calculation around
sample_encode_time.mul_f64 in the enclosing method to use a fallible
multiplication/conversion path instead of allowing overflow to panic. Preserve
Duration::ZERO for non-positive sample_secs, and return Duration::MAX whenever
the positive sample_factor would exceed the representable duration.
In `@src/command/sample_encode/cache.rs`:
- Around line 89-103: Update the retry loop in open_db to replace
std::thread::yield_now() with a short sleep interval between sled::open
attempts. Preserve the existing two-second LOCK_MAX_WAIT deadline and
error/context handling.
- Around line 73-87: Update cache_result to return () rather than
anyhow::Result, since insert failures are intentionally non-fatal; replace the
eprintln! call with log::warn!, preserving the existing error message and
handling flow.
In `@src/ffmpeg.rs`:
- Around line 296-312: Update the vcodec_arg_matrix test to accept an explicit
expected CRF value as a case parameter and compare VCodecSpecific::crf(&vcodec,
crf_in) directly against it. Set the libsvtav1 case’s expected value below its
input where appropriate so the cap is exercised, while preserving expected
values for other codecs.
In `@src/process.rs`:
- Around line 388-398: Move ownership of FIXTURE_ENV, FIXTURE_TEST, and
fixture_command into a single #[cfg(test)] helper in managed.rs, exporting it
for test use. Remove the duplicated constants and fixture_command from the
current test module, then call the shared helper so the child test path is
maintained in one place.
In `@src/process/managed.rs`:
- Around line 840-900: Update
assert_score_like_stream_terminates_when_dropped_after_logical_done,
assert_terminate_on_drop_stream_terminates_when_dropped_during_stderr, and
dropping_terminate_on_drop_process_terminates_instead_of_panicking to capture
the child PID before dropping the terminate-on-drop value, then poll for process
exit with a bounded deadline instead of only sleeping. Assert that the child is
gone before the deadline. Add explicit expected panic messages to the two
related #[should_panic] tests so unrelated panics cannot satisfy them.
In `@src/sample.rs`:
- Around line 129-132: Update the destination handling in copy so temporary::add
registers dest before checking dest.exists(); retain the existing early Ok(dest)
return afterward and avoid registering the path more than once.
In `@src/vmaf.rs`:
- Around line 178-188: Replace the hardcoded FIXTURE_TEST value in src/vmaf.rs
lines 178-188 and src/xpsnr.rs lines 242-252 with
crate::process::managed::MANAGED_PROCESS_FIXTURE_TEST, reusing the exported
fixture-test symbol in both fixture_command helpers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0e4559e5-728f-492e-b408-e0494b672547
📒 Files selected for processing (28)
.github/workflows/ci.ymlsrc/command/args/encode.rssrc/command/args/vmaf.rssrc/command/auto_encode.rssrc/command/crf_search.rssrc/command/crf_search/err.rssrc/command/encode/error.rssrc/command/encode/lifecycle.rssrc/command/encode/mod.rssrc/command/encode/plan.rssrc/command/encode/preflight.rssrc/command/encode/report.rssrc/command/encode/running.rssrc/command/encode/sink.rssrc/command/encode/spawner.rssrc/command/encode/test_support.rssrc/command/sample_encode.rssrc/command/sample_encode/cache.rssrc/ffmpeg.rssrc/ffprobe.rssrc/float.rssrc/process.rssrc/process/managed.rssrc/sample.rssrc/score_stream.rssrc/temporary.rssrc/vmaf.rssrc/xpsnr.rs
Comment on lines
+29
to
+48
| coverage: | ||
| runs-on: ubuntu-latest | ||
| env: | ||
| RUST_BACKTRACE: 1 | ||
| PROPTEST_CASES: "256" | ||
| steps: | ||
| - run: rustup update stable | ||
| - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 | ||
| with: | ||
| persist-credentials: false | ||
| - run: sudo apt-get update && sudo apt-get install -y ffmpeg | ||
| - run: rustup component add llvm-tools-preview | ||
| - run: cargo install cargo-llvm-cov --locked | ||
| - run: cargo install cargo-nextest --locked | ||
| - run: cargo llvm-cov nextest --locked --lcov --output-path lcov.info | ||
| - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 | ||
| with: | ||
| name: lcov | ||
| path: lcov.info | ||
| - run: cargo llvm-cov nextest --locked --summary-only --fail-under-lines 80 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict the coverage job token.
The coverage job has no permissions block. GitHub applies the repository default token permissions. Add job-level least-privilege permissions, such as contents: read. Add another scope only if the artifact upload requires it. This prevents repository code and actions in this job from receiving unnecessary write access.
Proposed fix
coverage:
+ permissions:
+ contents: read
runs-on: ubuntu-latest📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| coverage: | |
| runs-on: ubuntu-latest | |
| env: | |
| RUST_BACKTRACE: 1 | |
| PROPTEST_CASES: "256" | |
| steps: | |
| - run: rustup update stable | |
| - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 | |
| with: | |
| persist-credentials: false | |
| - run: sudo apt-get update && sudo apt-get install -y ffmpeg | |
| - run: rustup component add llvm-tools-preview | |
| - run: cargo install cargo-llvm-cov --locked | |
| - run: cargo install cargo-nextest --locked | |
| - run: cargo llvm-cov nextest --locked --lcov --output-path lcov.info | |
| - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 | |
| with: | |
| name: lcov | |
| path: lcov.info | |
| - run: cargo llvm-cov nextest --locked --summary-only --fail-under-lines 80 | |
| coverage: | |
| permissions: | |
| contents: read | |
| runs-on: ubuntu-latest | |
| env: | |
| RUST_BACKTRACE: 1 | |
| PROPTEST_CASES: "256" | |
| steps: | |
| - run: rustup update stable | |
| - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 | |
| with: | |
| persist-credentials: false | |
| - run: sudo apt-get update && sudo apt-get install -y ffmpeg | |
| - run: rustup component add llvm-tools-preview | |
| - run: cargo install cargo-llvm-cov --locked | |
| - run: cargo install cargo-nextest --locked | |
| - run: cargo llvm-cov nextest --locked --lcov --output-path lcov.info | |
| - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 | |
| with: | |
| name: lcov | |
| path: lcov.info | |
| - run: cargo llvm-cov nextest --locked --summary-only --fail-under-lines 80 |
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 29-48: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[info] 29-29: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 29 - 48, Add a job-level permissions
block to the coverage job, granting only contents: read; include any additional
permission only if required by actions/upload-artifact. Keep the existing
coverage steps unchanged.
Source: Linters/SAST tools
Comment on lines
+1108
to
+1136
| fn to_ffmpeg_args_merges_enc_svtav1_params() { | ||
| // setup | ||
| let enc = Encode { | ||
| encoder: Encoder("libsvtav1".into()), | ||
| input: "vid.mp4".into(), | ||
| vfilter: None, | ||
| preset: None, | ||
| pix_format: None, | ||
| keyint: None, | ||
| scd: None, | ||
| svt_args: vec!["film-grain=8".into()], | ||
| enc_args: vec!["svtav1-params=tune=0".into()], | ||
| enc_input_args: vec![], | ||
| }; | ||
| // execute | ||
| let args = enc | ||
| .to_ffmpeg_args(32.0, &test_probe(60, 24.0), "mkv") | ||
| .expect("to_ffmpeg_args"); | ||
| // assert | ||
| let svt = args | ||
| .output_args | ||
| .windows(2) | ||
| .find(|w| w[0].as_str() == "-svtav1-params") | ||
| .map(|w| w[1].as_str()) | ||
| .expect("svtav1-params"); | ||
| assert!(svt.contains("tune=0")); | ||
| assert!(svt.contains("film-grain=8")); | ||
| assert!(svt.contains("crf=32")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test the canonical svtav1-params argument form and fix the merge check.
Line 1119 bypasses parse_enc_arg. Parsed --enc svtav1-params=tune=0 input becomes -svtav1-params=tune=0, as the test at lines 769-775 shows. The merge condition does not recognize that form. The custom parameter stays in a separate option and can be overridden by the generated -svtav1-params option emitted later.
Proposed fix
- if opt == "svtav1-params" {
- svtav1_params.push(arg.clone());
+ if opt.trim_start_matches('-') == "svtav1-params" {
+ svtav1_params.push(val.to_owned());
vec![]- enc_args: vec!["svtav1-params=tune=0".into()],
+ enc_args: vec![parse_enc_arg("svtav1-params=tune=0").expect("parse")],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/command/args/encode.rs` around lines 1108 - 1136, Update the SVT-AV1
merge logic in Encode::to_ffmpeg_args to recognize the canonical
“-svtav1-params=…” form produced by parse_enc_arg. Adjust
to_ffmpeg_args_merges_enc_svtav1_params to pass that canonical form, and ensure
it is merged into the generated svtav1 parameters so only the combined option
remains with tune, film-grain, and crf.
Comment on lines
+29
to
+37
| fn output_search_score(enc: &sample_encode::Output, use_xpsnr: bool) -> anyhow::Result<f32> { | ||
| let score = match use_xpsnr { | ||
| true => enc.xpsnr_score.or(enc.vmaf_score), | ||
| false => enc.vmaf_score.or(enc.xpsnr_score), | ||
| }; | ||
| let score = score.context("sample encode produced no score")?; | ||
| anyhow::ensure!(score.is_finite(), "sample encode produced no finite score"); | ||
| Ok(score) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The cross-metric fallback compares scores on different scales.
When use_xpsnr is true, min_score holds the --min-xpsnr threshold. If a sample has no XPSNR score, this function returns the VMAF score instead, and the caller compares that VMAF value against the XPSNR threshold. VMAF is a 0–100 perceptual score and XPSNR is a dB value, so the comparison is not meaningful. A VMAF of 96 would satisfy --min-xpsnr 90 and end the search at the wrong CRF.
Return an error when the requested metric is missing, instead of substituting the other metric.
🐛 Proposed fix
fn output_search_score(enc: &sample_encode::Output, use_xpsnr: bool) -> anyhow::Result<f32> {
let score = match use_xpsnr {
- true => enc.xpsnr_score.or(enc.vmaf_score),
- false => enc.vmaf_score.or(enc.xpsnr_score),
+ true => enc.xpsnr_score,
+ false => enc.vmaf_score,
};
let score = score.context("sample encode produced no score")?;The test output_search_score_uses_vmaf_fallback_when_xpsnr_missing documents the current behavior and needs updating with this change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn output_search_score(enc: &sample_encode::Output, use_xpsnr: bool) -> anyhow::Result<f32> { | |
| let score = match use_xpsnr { | |
| true => enc.xpsnr_score.or(enc.vmaf_score), | |
| false => enc.vmaf_score.or(enc.xpsnr_score), | |
| }; | |
| let score = score.context("sample encode produced no score")?; | |
| anyhow::ensure!(score.is_finite(), "sample encode produced no finite score"); | |
| Ok(score) | |
| } | |
| fn output_search_score(enc: &sample_encode::Output, use_xpsnr: bool) -> anyhow::Result<f32> { | |
| let score = match use_xpsnr { | |
| true => enc.xpsnr_score, | |
| false => enc.vmaf_score, | |
| }; | |
| let score = score.context("sample encode produced no score")?; | |
| anyhow::ensure!(score.is_finite(), "sample encode produced no finite score"); | |
| Ok(score) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/command/crf_search.rs` around lines 29 - 37, Update output_search_score
so it returns only the metric requested by use_xpsnr: require enc.xpsnr_score
when true and enc.vmaf_score when false, returning the existing missing-score
error if unavailable instead of falling back to the other metric. Update
output_search_score_uses_vmaf_fallback_when_xpsnr_missing to assert the missing
requested metric error.
Comment on lines
+618
to
647
| fn vmaf_lerp_q( | ||
| min_vmaf: f32, | ||
| worse_q: &Sample, | ||
| better_q: &Sample, | ||
| use_xpsnr: bool, | ||
| ) -> anyhow::Result<i64> { | ||
| let worse_score = output_search_score(&worse_q.enc, use_xpsnr)?; | ||
| let better_score = output_search_score(&better_q.enc, use_xpsnr)?; | ||
| assert!( | ||
| worse_q.enc.single_score() <= min_vmaf | ||
| && worse_q.enc.single_score() < better_q.enc.single_score() | ||
| && worse_q.q > better_q.q, | ||
| worse_score <= min_vmaf && worse_score < better_score && worse_q.q > better_q.q, | ||
| "invalid vmaf_lerp_crf usage: ({min_vmaf}, {worse_q:?}, {better_q:?})" | ||
| ); | ||
|
|
||
| let vmaf_diff = better_q.enc.single_score() - worse_q.enc.single_score(); | ||
| let vmaf_factor = (min_vmaf - worse_q.enc.single_score()) / vmaf_diff; | ||
| let vmaf_diff = better_score - worse_score; | ||
| anyhow::ensure!(vmaf_diff > 0.0, "sample scores are indistinguishable"); | ||
| let vmaf_factor = (min_vmaf - worse_score) / vmaf_diff; | ||
|
|
||
| let q_diff = worse_q.q - better_q.q; | ||
| let lerp = (worse_q.q as f32 - q_diff as f32 * vmaf_factor).round() as i64; | ||
| lerp.clamp(better_q.q + 1, worse_q.q - 1) | ||
| let lo = better_q.q + 1; | ||
| let hi = worse_q.q - 1; | ||
| if lo > hi { | ||
| // Target score is outside the range between the two samples. | ||
| if min_vmaf > better_score { | ||
| return Ok(better_q.q - 1); | ||
| } | ||
| return Ok(worse_q.q); | ||
| } | ||
| Ok(lerp.clamp(lo, hi)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
The assertion aborts the process when measured scores are not monotonic in q.
vmaf_lerp_q asserts worse_score <= min_vmaf && worse_score < better_score && worse_q.q > better_q.q. Sample scores are measurements, so they are not guaranteed to be ordered by q. Three call sites can violate the assertion:
- Line 432:
vmaf_lerp_q(min_score, upper, &sample, use_xpsnr)requiresscore(upper) <= min_score. A previously measureduppercan score abovemin_scoreand still remain incrf_attempts. - Line 467 and Line 470: both require
score(lower) > score(sample). Equal or inverted adjacent measurements fire the assertion.
The function already returns anyhow::Result<i64>. Return an error instead of asserting, so a non-monotonic measurement fails the search cleanly rather than aborting the process.
🐛 Proposed fix
- assert!(
- worse_score <= min_vmaf && worse_score < better_score && worse_q.q > better_q.q,
- "invalid vmaf_lerp_crf usage: ({min_vmaf}, {worse_q:?}, {better_q:?})"
- );
+ anyhow::ensure!(
+ worse_score <= min_vmaf && worse_score < better_score && worse_q.q > better_q.q,
+ "non-monotonic sample scores: ({min_vmaf}, {worse_q:?}, {better_q:?})"
+ );Add a regression test where two samples score equally and confirm the search returns an error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/command/crf_search.rs` around lines 618 - 647, Replace the assertion in
vmaf_lerp_q with anyhow error validation that returns an error when sample
scores violate the required ordering or bounds, while preserving the existing
interpolation behavior for valid inputs. Add a regression test covering
equal-scoring adjacent samples and verify the search returns an error instead of
aborting.
Comment on lines
+87
to
+104
| #[test] | ||
| fn render_encode_summary_includes_stream_breakdown() { | ||
| let metrics = EncodeMetrics::from_bytes( | ||
| 100, | ||
| 400, | ||
| Some(StreamSizes { | ||
| video: 80, | ||
| audio: 0, | ||
| subtitle: 5, | ||
| other: 0, | ||
| }), | ||
| ); | ||
| let mut buf = Vec::new(); | ||
| render_encode_summary(&metrics, &mut buf).expect("render"); | ||
| let text = String::from_utf8(buf).expect("utf8"); | ||
| assert!(text.contains("Encoded")); | ||
| assert!(text.contains("subs:")); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a case for the video-only suppression branch.
has_non_video at Line 57-59 suppresses the whole stream breakdown when audio, subtitle, and other are all zero. No test covers that branch. A regression that always prints the breakdown would pass the current suite.
♻️ Proposed additional test
#[test]
fn render_encode_summary_omits_breakdown_for_video_only() {
let metrics = EncodeMetrics::from_bytes(
100,
400,
Some(StreamSizes {
video: 100,
audio: 0,
subtitle: 0,
other: 0,
}),
);
let mut buf = Vec::new();
render_encode_summary(&metrics, &mut buf).expect("render");
let text = String::from_utf8(buf).expect("utf8");
assert!(text.contains("Encoded"));
assert!(!text.contains("video:"));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/command/encode/report.rs` around lines 87 - 104, Add a test alongside
render_encode_summary_includes_stream_breakdown for the video-only case, using
StreamSizes with audio, subtitle, and other all zero. Assert the rendered output
still contains “Encoded” but omits the stream breakdown, such as “video:”, to
cover the has_non_video suppression branch.
Comment on lines
+296
to
+312
| fn vcodec_arg_matrix( | ||
| #[case] codec: &str, | ||
| #[case] crf_arg: &str, | ||
| #[case] preset_arg: &str, | ||
| #[case] crf_in: f32, | ||
| ) { | ||
| // setup | ||
| let vcodec: Arc<str> = Arc::from(codec); | ||
|
|
||
| // execute / assert | ||
| assert_eq!(VCodecSpecific::crf_arg(&vcodec), crf_arg); | ||
| assert_eq!(VCodecSpecific::preset_arg(&vcodec), preset_arg); | ||
| assert_eq!( | ||
| VCodecSpecific::crf(&vcodec, crf_in), | ||
| crf_in.min(if codec == "libsvtav1" { 63.0 } else { crf_in }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The crf assertion in vcodec_arg_matrix cannot fail.
Line 309-311 compares VCodecSpecific::crf(&vcodec, crf_in) against crf_in.min(...). For every non-libsvtav1 case the expression reduces to crf_in.min(crf_in), which is crf_in. For the libsvtav1 case, crf_in is already 63.0, so the cap is not exercised. Any implementation that returns its argument unchanged passes this assertion.
Pass the expected value as an explicit case parameter.
♻️ Proposed change
#[rstest]
- #[case::libsvtav1("libsvtav1", "-crf", "-preset", 63.0)]
- #[case::librav1e("librav1e", "-qp", "-speed", 40.0)]
- #[case::libx264("libx264", "-crf", "-preset", 32.0)]
- #[case::hevc_vt("hevc_videotoolbox", "-q:v", "-preset", 50.0)]
+ #[case::libsvtav1("libsvtav1", "-crf", "-preset", 70.0, 63.0)]
+ #[case::librav1e("librav1e", "-qp", "-speed", 40.0, 40.0)]
+ #[case::libx264("libx264", "-crf", "-preset", 32.0, 32.0)]
+ #[case::hevc_vt("hevc_videotoolbox", "-q:v", "-preset", 50.0, 50.0)]
fn vcodec_arg_matrix(
#[case] codec: &str,
#[case] crf_arg: &str,
#[case] preset_arg: &str,
#[case] crf_in: f32,
+ #[case] crf_expected: f32,
) {
// setup
let vcodec: Arc<str> = Arc::from(codec);
// execute / assert
assert_eq!(VCodecSpecific::crf_arg(&vcodec), crf_arg);
assert_eq!(VCodecSpecific::preset_arg(&vcodec), preset_arg);
- assert_eq!(
- VCodecSpecific::crf(&vcodec, crf_in),
- crf_in.min(if codec == "libsvtav1" { 63.0 } else { crf_in })
- );
+ assert_eq!(VCodecSpecific::crf(&vcodec, crf_in), crf_expected);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ffmpeg.rs` around lines 296 - 312, Update the vcodec_arg_matrix test to
accept an explicit expected CRF value as a case parameter and compare
VCodecSpecific::crf(&vcodec, crf_in) directly against it. Set the libsvtav1
case’s expected value below its input where appropriate so the cap is exercised,
while preserving expected values for other codecs.
Comment on lines
+388
to
+398
| const FIXTURE_ENV: &str = "AB_AV1_MANAGED_PROCESS_FIXTURE"; | ||
| const FIXTURE_TEST: &str = "process::managed::tests::managed_process_fixture_child"; | ||
|
|
||
| fn fixture_command(fixture: &str) -> Command { | ||
| let mut cmd = Command::new(env::current_exe().expect("current test executable")); | ||
| cmd.arg("--exact") | ||
| .arg(FIXTURE_TEST) | ||
| .arg("--nocapture") | ||
| .env(FIXTURE_ENV, fixture); | ||
| cmd | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Share the fixture harness constants with src/process/managed.rs.
FIXTURE_ENV, FIXTURE_TEST, and fixture_command duplicate the fixture harness that src/process/managed.rs owns. FIXTURE_TEST names the child test with a hard-coded path string. If that test is renamed or moved, --exact matches no test, the child exits successfully with no output, and every test here fails with an unrelated message instead of a compile error.
Export a single #[cfg(test)] helper from src/process/managed.rs and call it here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/process.rs` around lines 388 - 398, Move ownership of FIXTURE_ENV,
FIXTURE_TEST, and fixture_command into a single #[cfg(test)] helper in
managed.rs, exporting it for test use. Remove the duplicated constants and
fixture_command from the current test module, then call the shared helper so the
child test path is maintained in one place.
Comment on lines
+840
to
+900
| async fn assert_score_like_stream_terminates_when_dropped_after_logical_done( | ||
| fixture: &str, | ||
| done_marker: &str, | ||
| ) { | ||
| let cmd = fixture_command(fixture); | ||
| let process = | ||
| ManagedProcess::spawn("score-like stderr fixture", cmd).expect("spawn shell fixture"); | ||
| assert!(process.id().is_some(), "process id"); | ||
| let mut events = Box::pin(process.terminate_on_drop().stderr_events()); | ||
| let mut parsed_logical_done = false; | ||
|
|
||
| while let Some(event) = events.next().await { | ||
| match event.expect("managed event") { | ||
| ManagedEvent::RawStderr(chunk) => { | ||
| if String::from_utf8_lossy(chunk.as_bytes()).contains(done_marker) { | ||
| parsed_logical_done = true; | ||
| break; | ||
| } | ||
| } | ||
| ManagedEvent::ReplayGap(_) => {} | ||
| ManagedEvent::ProcessDone(_) => { | ||
| panic!("test must stop polling before ManagedEvent::ProcessDone") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| assert!(parsed_logical_done, "fixture should emit a parseable score"); | ||
| drop(events); | ||
| tokio::time::sleep(Duration::from_millis(50)).await; | ||
| } | ||
|
|
||
| async fn assert_terminate_on_drop_stream_terminates_when_dropped_during_stderr( | ||
| fixture: &str, | ||
| chunk_marker: &str, | ||
| ) { | ||
| let cmd = fixture_command(fixture); | ||
| let process = | ||
| ManagedProcess::spawn("terminate-on-drop fixture", cmd).expect("spawn fixture"); | ||
| assert!(process.id().is_some(), "process id"); | ||
| let mut events = Box::pin(process.terminate_on_drop().stderr_events()); | ||
| let mut saw_chunk = false; | ||
|
|
||
| while let Some(event) = events.next().await { | ||
| match event.expect("managed event") { | ||
| ManagedEvent::RawStderr(chunk) => { | ||
| if String::from_utf8_lossy(chunk.as_bytes()).contains(chunk_marker) { | ||
| saw_chunk = true; | ||
| break; | ||
| } | ||
| } | ||
| ManagedEvent::ReplayGap(_) => {} | ||
| ManagedEvent::ProcessDone(_) => { | ||
| panic!("test must stop polling before ManagedEvent::ProcessDone") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| assert!(saw_chunk, "fixture should emit stderr before sleeping"); | ||
| drop(events); | ||
| tokio::time::sleep(Duration::from_millis(50)).await; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Assert child termination instead of only sleeping.
assert_score_like_stream_terminates_when_dropped_after_logical_done, assert_terminate_on_drop_stream_terminates_when_dropped_during_stderr, and dropping_terminate_on_drop_process_terminates_instead_of_panicking drop the value and then sleep for 50 ms. They assert nothing after the drop. If the spawned termination task never runs, the 30-second fixture child stays alive and the test still passes. This is the central guarantee of TerminateOnDropProcess, so the test should verify it.
Capture the child pid before the drop and poll until the OS process is gone, with a bounded deadline.
Also add expected = "..." to the two #[should_panic] tests at lines 981 and 1176, so an unrelated panic (for example a fixture spawn failure) cannot make them pass.
Also applies to: 992-1002
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/process/managed.rs` around lines 840 - 900, Update
assert_score_like_stream_terminates_when_dropped_after_logical_done,
assert_terminate_on_drop_stream_terminates_when_dropped_during_stderr, and
dropping_terminate_on_drop_process_terminates_instead_of_panicking to capture
the child PID before dropping the terminate-on-drop value, then poll for process
exit with a bounded deadline instead of only sleeping. Assert that the child is
gone before the deadline. Add explicit expected panic messages to the two
related #[should_panic] tests so unrelated panics cannot satisfy them.
Comment on lines
+129
to
+132
| if dest.exists() { | ||
| return Ok(dest); | ||
| } | ||
| temporary::add(&dest, TempKind::Keepable); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A pre-existing destination is not registered for cleanup.
When dest already exists from an earlier run in the same temp directory, copy returns before temporary::add. The file then stays outside the temporary registry, so temporary::clean never removes it. Register the path before the early return.
🛡️ Proposed fix
let dest = sample_dest_path(input, sample_start, floor_to_sec, frames, temp_dir)?;
+ temporary::add(&dest, TempKind::Keepable);
if dest.exists() {
return Ok(dest);
}
- temporary::add(&dest, TempKind::Keepable);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if dest.exists() { | |
| return Ok(dest); | |
| } | |
| temporary::add(&dest, TempKind::Keepable); | |
| temporary::add(&dest, TempKind::Keepable); | |
| if dest.exists() { | |
| return Ok(dest); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/sample.rs` around lines 129 - 132, Update the destination handling in
copy so temporary::add registers dest before checking dest.exists(); retain the
existing early Ok(dest) return afterward and avoid registering the path more
than once.
Comment on lines
+178
to
+188
| const FIXTURE_ENV: &str = "AB_AV1_MANAGED_PROCESS_FIXTURE"; | ||
| const FIXTURE_TEST: &str = "process::managed::tests::managed_process_fixture_child"; | ||
|
|
||
| fn fixture_command(fixture: &str) -> Command { | ||
| let mut cmd = Command::new(env::current_exe().expect("current test executable")); | ||
| cmd.arg("--exact") | ||
| .arg(FIXTURE_TEST) | ||
| .arg("--nocapture") | ||
| .env(FIXTURE_ENV, fixture); | ||
| cmd | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Two test modules hardcode the managed-process fixture test path. src/process/managed.rs exports MANAGED_PROCESS_FIXTURE_TEST, and src/sample.rs already uses it. These two modules repeat the literal instead. A rename of the fixture test still compiles here and fails only at runtime when the child process starts.
src/vmaf.rs#L178-L188: setFIXTURE_TESTtocrate::process::managed::MANAGED_PROCESS_FIXTURE_TEST.src/xpsnr.rs#L242-L252: setFIXTURE_TESTtocrate::process::managed::MANAGED_PROCESS_FIXTURE_TEST.
📍 Affects 2 files
src/vmaf.rs#L178-L188(this comment)src/xpsnr.rs#L242-L252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vmaf.rs` around lines 178 - 188, Replace the hardcoded FIXTURE_TEST value
in src/vmaf.rs lines 178-188 and src/xpsnr.rs lines 242-252 with
crate::process::managed::MANAGED_PROCESS_FIXTURE_TEST, reusing the exported
fixture-test symbol in both fixture_command helpers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack 1 of 13. This is the root PR against
mjc/ab-av1-worker:main.This replaces ad-hoc child-process ownership with managed lifecycle objects, bounded stderr collection, explicit termination, and stream completion contracts. It repairs cache, sample, CRF-search, encode-cleanup, and score-stream regressions exposed by strict process ownership, then introduces typed sample cache keys, shared score command construction, and explicit encode planning/running/output phases.
The behavioral impact is that subprocesses can no longer detach silently or report logical success before their OS process completes. Encode preflight, output ownership, spawning, progress, finalization, and cleanup are separated, and temporary outputs are registered only when an encode actually begins.
Tests added in this branch exercise managed-process collection, timeout, pause/termination, drop behavior, replayed stderr, bounded diagnostics, and streaming completion. Property and regression matrices cover CRF decisions, cache-key separation, sample planning and cleanup, ffmpeg/ffprobe edge cases, score parsing, finite progress math, and real sample copying. Encode-phase tests prove unsafe output/audio combinations fail before spawn, partial outputs clean up, completed outputs survive cleanup, and fixture encodes report progress and completion correctly.
Summary by CodeRabbit