Skip to content

fix(cua-driver): report encoder exit and shutdown timeout errors - #3714

Merged
injaneity merged 12 commits into
mainfrom
investigate/3713-windows-recorder
Sep 11, 2026
Merged

fix(cua-driver): report encoder exit and shutdown timeout errors#3714
injaneity merged 12 commits into
mainfrom
investigate/3713-windows-recorder

Conversation

@injaneity

@injaneity injaneity commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Problem this solves: stop_recording hid encoder exit failures and shutdown timeouts behind the same generic “did not finalize” error.
  • What changed: return the actual failure category and encoder exit code or timeout duration to callers, and retain that cause in get_recording_state.last_error. Add public-tool regression tests and execute them in Windows CI; Linux already runs them. Remove the standalone diagnostic workflow and process-comparison scripts. The existing wait loop returns success or the specific error directly, followed by shared stderr cleanup. No outcome enum, diagnostic bookkeeping, or generic process framework remains. The production method is four lines shorter than the original main-branch implementation; most of the remaining diff is durable regression coverage.

This is an error-reporting fix, not a fix for the intermittent Windows recording failure in #3713.

Related work

Refs #3713

RFC: not required for this correction to existing error reporting. No new tool, response schema, permission, or recording lifecycle contract is introduced.

Compatibility and risk

  • User-visible, API/CLI/MCP, migration, permission, or platform impact: failed FFmpeg recordings now provide more specific error text through existing error fields. Windows and Linux FFmpeg recording paths are affected; native macOS ScreenCaptureKit is unchanged. No migration or permission changes. The three-second shutdown deadline and rejection of failed recordings remain unchanged.
  • Risk and rollback: consumers matching exact error strings may need to accept the more specific messages. Reverting restores the old generic errors; there is no data migration. Small external encoder fixtures test failure reporting without mocking recorder internals or claiming successful video production.

Validation

  • Focused tests and checks: two separate red → green cycles through public recording tools: encoder exit (ad0e78efa796c73593) and shutdown timeout (bf2e157273f6f12213). Both verify error responses, disabled recording state, no successful video path, and retained failure cause. At current head 5f088c831c5b7a41a8b52637fb729aeb18da9afd, 604 core unit tests and both unchanged integration tests pass locally; formatting and diff checks pass. The cleanup preserves the three-second deadline and strict acceptance, with the failure cause returned to callers and stderr retained in failure logs. Current-head platform CI is pending; the preceding head’s ordinary platform CI passed.
  • Manual or platform evidence: focused Windows native run and Linux native run passed at preceding head f96f8f849bbc24cab4ecd5e92d25d64cbeab5343, before the shutdown refactor. These runs do not certify the current head. Existing preflight records real fixture activity and independently decodes the resulting video. Local fault-injection tests exercise the FFmpeg adapter, not native macOS recording.
  • Known gaps or CI still required: the original intermittent finalization failure remains unresolved, and these passing runs do not establish its root cause or fix it. No full desktop matrix certification claimed. Earlier diagnostic evidence remains linked in Windows recorder intermittently reports failed FFmpeg finalization during native E2E preflight #3713 and PR history, not in the shipping tree. Current-head platform CI and affected native recertification are required before merge. The PR was already non-draft when this cleanup began; its readiness state was not changed by this update.

Contributor and release checks

  • The PR is focused and the description matches the final diff.
  • This change does not require an RFC, or the accepted RFC is linked above.
  • Tests, documentation, and platform evidence are included or the gap is explained.
  • The PR title is a Conventional Commit describing the production change.
  • External contributor authorship is preserved, or no external contribution is included.
  • If release-tracked files changed but this is intentionally non-releasing, the no-release label is applied.

Release intent: a Cua Driver patch for user-visible error reporting; the conditional no-release requirement does not apply. No external contribution is included.

Refs #3713. Compare native FFmpeg process shutdown with and without the package-manager shim; preserve all attempts as diagnostics, not certification.
Refs #3713. Preserve forty independent diagnostic observations across shim/direct launch and immediate/held stdin closure. Retain errors, exit status, deadline termination, descendant cleanup and media metadata. No product changes or certification claims.
Chocolatey HTTP 504 prevented the first diagnostic from observing any recordings. Retain the setup transcript and explicitly distinguish an officially generated shim around the verified fallback from a package-installed shim. No product behavior or recording deadline change.
Refs #3713. Diagnostic-only instrumentation captures pre-stop process status, stdin write/flush results, exit status, shutdown duration, and forced termination. Preserve existing stop timing and finalization acceptance; standalone process controls did not reproduce the native preflight failure.
Retain experimental evidence in #3713/#3714 and CI artifacts, not permanent diagnostic workflows or copied recorder implementations. Continue at the agreed public recording-tool seams.
Refs #3713. Red: an external encoder fixture exits with code 23, but public stop_recording and get_recording_state lose that cause. Exercise the real FFmpeg backend through public tools in an isolated process; no recording internals are mocked.
Red: a nonresponsive external encoder is terminated at the unchanged deadline, but public recording responses still erase the timeout cause.
Review the completed red/green slices: consolidate isolated external-encoder fixture setup, keep public assertions explicit, and derive timeout text from the unchanged deadline. Windows now executes these integration tests; Linux already runs all targets.
@injaneity injaneity changed the title test(cua-driver): diagnose Windows recorder shutdown failures fix(cua-driver): preserve recorder shutdown failure causes Sep 10, 2026
@injaneity injaneity removed the no-release Skip auto-release on merge label Sep 10, 2026
@injaneity injaneity changed the title fix(cua-driver): preserve recorder shutdown failure causes fix(cua-driver): report encoder exit and shutdown timeout errors Sep 11, 2026
@injaneity
injaneity marked this pull request as ready for review September 11, 2026 04:55
@injaneity
injaneity requested a review from f-trycua as a code owner September 11, 2026 04:55
Replace correlated finalization and forced-kill flags with explicit exit and timeout outcomes. Derive the caller result once, name stdin observations, and join stderr through one shared cleanup path. Preserve failure logging before stderr collection, the existing deadline and strict recording acceptance. Existing public recording regression tests remain unchanged.
Remove the outcome enum and investigation-only process, stdin, and timing observations. Return the specific error directly from the existing wait loop, retain stderr diagnostics, and preserve the deadline and public regression tests.
@injaneity
injaneity merged commit eca08d3 into main Sep 11, 2026
36 checks passed
@injaneity
injaneity deleted the investigate/3713-windows-recorder branch September 11, 2026 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant