mirror of
https://github.com/calesthio/OpenMontage.git
synced 2026-08-05 15:20:40 +08:00
fix(video_compose): surface Remotion failures and add render timeout passthrough
The high-level Remotion render path hid the useful failure reason. run_command runs with check=True + capture_output, so a non-zero exit raised CalledProcessError whose str() is only 'returned non-zero exit status 1' — the actual Remotion diagnostics in stderr were dropped. Catch CalledProcessError and surface the stderr/stdout tail, and TimeoutExpired with an actionable hint. Also add a creator-facing remotion_timeout_ms input, passed through as Remotion's --timeout (headless-browser setup + delayRender). Slow browser startup on restricted networks previously failed opaquely at the default 30s with no way to raise it. The subprocess timeout is widened to match so run_command does not kill Remotion before its own timeout fires. Closes #217
This commit is contained in:
95
tests/tools/test_remotion_diagnostics.py
Normal file
95
tests/tools/test_remotion_diagnostics.py
Normal file
@@ -0,0 +1,95 @@
|
||||
"""Tests for Remotion render debuggability in video_compose (issue #217).
|
||||
|
||||
Two creator-facing gaps:
|
||||
1. A failed `npx remotion render` surfaced only "returned non-zero exit
|
||||
status 1"; the useful Remotion diagnostics in stderr were dropped.
|
||||
2. There was no pass-through for Remotion's `--timeout`, so a slow headless
|
||||
browser setup failed opaquely with no way to raise the limit.
|
||||
"""
|
||||
|
||||
import subprocess
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
PROJECT_ROOT = Path(__file__).resolve().parent.parent.parent
|
||||
sys.path.insert(0, str(PROJECT_ROOT))
|
||||
|
||||
from tools.video.video_compose import VideoCompose # noqa: E402
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def tool(monkeypatch):
|
||||
monkeypatch.setattr("shutil.which", lambda _: "/usr/bin/npx")
|
||||
return VideoCompose()
|
||||
|
||||
|
||||
def test_render_failure_surfaces_remotion_stderr_tail(tool, tmp_path, monkeypatch):
|
||||
stderr = "some npm noise\nError: Delayed render timed out\nRemotion actual cause here"
|
||||
|
||||
def fake_run_command(cmd, *a, **k):
|
||||
raise subprocess.CalledProcessError(returncode=1, cmd=cmd, output="", stderr=stderr)
|
||||
|
||||
monkeypatch.setattr(tool, "run_command", fake_run_command)
|
||||
result = tool._remotion_render(
|
||||
{"composition_data": {"cuts": []}, "output_path": str(tmp_path / "out.mp4")}
|
||||
)
|
||||
|
||||
assert result.success is False
|
||||
assert "exit 1" in result.error
|
||||
assert "Remotion actual cause here" in result.error
|
||||
|
||||
|
||||
def test_timeout_expired_gives_actionable_message(tool, tmp_path, monkeypatch):
|
||||
def fake_run_command(cmd, *a, **k):
|
||||
raise subprocess.TimeoutExpired(cmd=cmd, timeout=600)
|
||||
|
||||
monkeypatch.setattr(tool, "run_command", fake_run_command)
|
||||
result = tool._remotion_render(
|
||||
{"composition_data": {"cuts": []}, "output_path": str(tmp_path / "out.mp4")}
|
||||
)
|
||||
|
||||
assert result.success is False
|
||||
assert "timed out" in result.error.lower()
|
||||
assert "remotion_timeout_ms" in result.error
|
||||
|
||||
|
||||
def test_remotion_timeout_ms_is_passed_through(tool, tmp_path, monkeypatch):
|
||||
seen = {}
|
||||
|
||||
def fake_run_command(cmd, *a, **k):
|
||||
seen["cmd"] = cmd
|
||||
seen["timeout"] = k.get("timeout")
|
||||
return None # output file intentionally absent
|
||||
|
||||
monkeypatch.setattr(tool, "run_command", fake_run_command)
|
||||
tool._remotion_render(
|
||||
{
|
||||
"composition_data": {"cuts": []},
|
||||
"output_path": str(tmp_path / "out.mp4"),
|
||||
"remotion_timeout_ms": 120000,
|
||||
}
|
||||
)
|
||||
|
||||
assert "--timeout=120000" in seen["cmd"]
|
||||
# subprocess timeout widened past the 120s render budget so run_command
|
||||
# does not kill Remotion before its own timeout fires.
|
||||
assert seen["timeout"] >= 180
|
||||
|
||||
|
||||
def test_no_timeout_flag_when_not_requested(tool, tmp_path, monkeypatch):
|
||||
seen = {}
|
||||
|
||||
def fake_run_command(cmd, *a, **k):
|
||||
seen["cmd"] = cmd
|
||||
seen["timeout"] = k.get("timeout")
|
||||
return None
|
||||
|
||||
monkeypatch.setattr(tool, "run_command", fake_run_command)
|
||||
tool._remotion_render(
|
||||
{"composition_data": {"cuts": []}, "output_path": str(tmp_path / "out.mp4")}
|
||||
)
|
||||
|
||||
assert not any(str(c).startswith("--timeout") for c in seen["cmd"])
|
||||
assert seen["timeout"] == 600
|
||||
@@ -187,6 +187,15 @@ class VideoCompose(BaseTool):
|
||||
"codec": {"type": "string", "default": "libx264"},
|
||||
"crf": {"type": "integer", "default": 23},
|
||||
"preset": {"type": "string", "default": "medium"},
|
||||
"remotion_timeout_ms": {
|
||||
"type": "integer",
|
||||
"description": (
|
||||
"Remotion render timeout in milliseconds, passed through as "
|
||||
"`--timeout` (governs headless-browser setup and delayRender). "
|
||||
"Raise this when the browser is slow to start (e.g. restricted "
|
||||
"networks). The subprocess timeout is widened to match."
|
||||
),
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
@@ -1738,12 +1747,45 @@ class VideoCompose(BaseTool):
|
||||
except (ImportError, ValueError):
|
||||
pass
|
||||
|
||||
# Optional creator-facing render timeout. Remotion's `--timeout` (ms)
|
||||
# governs headless-browser setup and delayRender(); on slow machines or
|
||||
# restricted networks the default 30s browser setup times out with an
|
||||
# opaque failure. Pass it through and give the subprocess enough headroom
|
||||
# so run_command() does not kill Remotion before its own timeout fires.
|
||||
remotion_timeout_ms = inputs.get("remotion_timeout_ms")
|
||||
subprocess_timeout = 600
|
||||
if remotion_timeout_ms:
|
||||
try:
|
||||
ms = int(remotion_timeout_ms)
|
||||
cmd.append(f"--timeout={ms}")
|
||||
subprocess_timeout = max(subprocess_timeout, ms // 1000 + 60)
|
||||
except (TypeError, ValueError):
|
||||
pass
|
||||
|
||||
try:
|
||||
# Invoke from inside the composer dir so npx can resolve the
|
||||
# local remotion binary via node_modules/.bin. Without this,
|
||||
# Windows npx cannot locate the CLI and returns "could not
|
||||
# determine executable to run".
|
||||
self.run_command(cmd, timeout=600, cwd=composer_dir)
|
||||
self.run_command(cmd, timeout=subprocess_timeout, cwd=composer_dir)
|
||||
except subprocess.CalledProcessError as e:
|
||||
# run_command uses check=True + capture_output, so the useful
|
||||
# Remotion diagnostics live in stderr/stdout — surface the tail
|
||||
# instead of the bare "returned non-zero exit status 1".
|
||||
detail = (e.stderr or e.stdout or "").strip()
|
||||
tail = "\n".join(detail.splitlines()[-25:]) if detail else "(no output captured)"
|
||||
return ToolResult(
|
||||
success=False,
|
||||
error=f"Remotion render failed (exit {e.returncode}):\n{tail}",
|
||||
)
|
||||
except subprocess.TimeoutExpired as e:
|
||||
return ToolResult(
|
||||
success=False,
|
||||
error=(
|
||||
f"Remotion render timed out after {e.timeout}s. If the headless "
|
||||
"browser is slow to start, raise remotion_timeout_ms (ms)."
|
||||
),
|
||||
)
|
||||
except Exception as e:
|
||||
return ToolResult(success=False, error=f"Remotion render failed: {e}")
|
||||
finally:
|
||||
|
||||
Reference in New Issue
Block a user