Skip to content

fix(utils): stabilize segmented video reversal - #668

Open
rusetiq wants to merge 8 commits into
jeertmans:mainfrom
rusetiq:fix/segmented-video-reversal
Open

rusetiq wants to merge 8 commits into
jeertmans:mainfrom
rusetiq:fix/segmented-video-reversal

Conversation

@rusetiq

@rusetiq rusetiq commented Aug 20, 2026 •

Copy link
Copy Markdown

Problem and fix

Addresses #562 and #569.

Segmented reversal can stall when a default fork pool inherits a lock held by a renderer or logging thread. Reversal workers now use the spawn context without changing the application's multiprocessing default. Generated segment paths are sorted before reversal and concatenation, and the extra dot in segment filenames is removed.

Reproduction and coverage

  • tests/data/reversal_deadlock.py provides a small three-second ReversalDeadlock slide. It forces the application's default to fork and synchronizes a background thread holding the renderer's real Rich console lock at fork time. PyAV logging in inherited workers then blocks on that lock.
  • The utility regression uses the same setup with one and two workers, enforces a 30-second subprocess timeout, and kills the entire worker process group on failure.
  • Both reversal regressions verify all six decoded frames against the reversed source, including frame count and brightness. The frame-order patch is scoped to the reversal call.
  • The controlled reproducer demonstrates a concrete inherited-lock failure; it does not establish that every stall in the linked issues has the same cause. Reproduction commands and the direct-call __main__ guard requirement are documented in the contributor internals guide.

Validation

  • Unmodified main (df0f1f1): both the utility case and slide example stalled and were terminated after 10 seconds.
  • Updated branch: the slide completed all three segments and produced 45 forward and 45 reversed frames.
  • Full pytest run: 230 passed, 1 skipped; 13 tests failed because the sandbox blocked network/OpenGL access. The failed cases passed on an unsandboxed retry (36 passed, including additional tests selected by pytest's failure cache). No failures remain.
  • Full pre-commit suite passed, including Ruff, ty, codespell, and repository checks.

Merged current main and resolved the changelog, converter, and utility-test conflicts while preserving upstream fixes. The Windows opener uses a ty-specific suppression, and the required TOML formatter's whitespace changes are included.

@github-actions github-actions Bot added waiting review Waiting review (or response) by a maintainer waiting triage This issue or PR needs to be labeled accordingly. labels Aug 20, 2026
@codacy-production

codacy-production Bot commented Aug 20, 2026 •

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 4 high · 1 medium · 3 minor

Alerts:
⚠ 8 issues (≤ 0 issues of at least minor severity)

Results:
8 new issues

Category Results
Documentation 2 minor
ErrorProne 1 high
Security 1 medium
1 minor
3 high

View in Codacy

🟢 Metrics 20 complexity · 0 duplication

Metric Results
Complexity 20
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@rusetiq

rusetiq commented Aug 20, 2026

Copy link
Copy Markdown
Author

pre-commit.ci run

@rusetiq

rusetiq commented Aug 20, 2026

Copy link
Copy Markdown
Author

pre-commit.ci run

@codecov

codecov Bot commented Aug 21, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.10%. Comparing base (8a84306) to head (2030a56).
⚠️ Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
manim_slides/convert.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #668   +/-   ##
=======================================
  Coverage   79.10%   79.10%           
=======================================
  Files          23       23           
  Lines        2178     2178           
=======================================
  Hits         1723     1723           
  Misses        455      455           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jeertmans jeertmans added bug Something isn't working lib Related to the library (a.k.a. module) python Pull requests that update Python code and removed waiting triage This issue or PR needs to be labeled accordingly. waiting review Waiting review (or response) by a maintainer labels Aug 21, 2026

@jeertmans jeertmans left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rusetiq, thanks for your contribution, this looks like a very nice fix to an old problem!

Don't mind the pre-commit.ci app, it cannot run ty since it requires internet access :-/

Just run it locally and make sure that it passes the checks.

I'll review you PR soon, I just need some time to find a way to reproduce the bug.

Comment thread tests/test_utils.py Outdated
def reversed_iterdir(path: Path) -> Iterator[Path]:
return iter(reversed(list(original_iterdir(path))))

monkeypatch.setattr(Path, "iterdir", reversed_iterdir)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the context manager and only call the function that should be affected inside the scope :-)

Comment thread manim_slides/convert.py Outdated
subprocess.call(("open", str(file)))
elif system == "Windows":
os.startfile(str(file)) # type: ignore[attr-defined]
os.startfile(str(file)) # nosec B606 # ty: ignore[unresolved-attribute]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What tool did you use to report a security issue? Because this error should not be raised by the linters used for this project.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i used codacy’s public pr api for that as an external pr check before pushing, ill revert the unrelated stuff

@rusetiq

rusetiq commented Aug 21, 2026

Copy link
Copy Markdown
Author

Thanks! I removed the Codacy-specific accommodations in 2030a56 and verified the project checks locally. The full pre-commit suite passes (including ty), and the focused utility tests pass 3/3. I'll disregard the hosted pre-commit.ci dependency-download failures as suggested.

@jeertmans

Copy link
Copy Markdown
Owner

Unfortunately, I cannot find a simple way to reproduce the deadlock issue. I know it depends on factors we cannot control, but do you have an example slides script that would also always stall?

@rusetiq

rusetiq commented Oct 5, 2026

Copy link
Copy Markdown
Author

Hi @jeertmans, sorry for the long delay getting back to you, and thank you for taking the time to investigate this!

I still owe you a reliable reproducer for the deadlock. The regression test in this PR covers the segment ordering fix, so it doesn't demonstrate the stall itself. I understand why a reproducible slides example is needed to validate the multiprocessing change.

Thanks again for your patience, and apologies for leaving your question unanswered for so long.

@rusetiq

rusetiq commented Oct 5, 2026

Copy link
Copy Markdown
Author

Hi @jeertmans, I've now added a reproducible case and pushed the update.

The three-second slide is in tests/data/reversal_deadlock.py:

uv run manim render tests/data/reversal_deadlock.py ReversalDeadlock -ql --disable_caching

On a platform with fork, it synchronizes a background thread holding the renderer's real Rich console lock when the pool forks. The inherited worker then blocks when PyAV logs through that console. This is a controlled reproduction of the inherited-lock failure, rather than a claim that every stall in #562/#569 has the same cause.

Both the utility case and this slide stalled on unmodified main; I terminated their process groups after 10 seconds. With the PR's explicit spawn context, the slide completes all three segments and produces all 45 reversed frames. The application's default multiprocessing context remains unchanged.

The automated regression covers one and two workers, checks every output frame, and has a 30-second timeout with worker cleanup. Run it with:

uv run pytest tests/test_utils.py -k inherited_renderer_lock

I also scoped the frame-order monkeypatch to the reversal call as requested and resolved the conflicts with current main. All pre-commit checks pass. The full test run had 230 passes and one skip; the 13 network/OpenGL failures under the sandbox all passed on an unsandboxed retry.

Thanks again for your patience!

@jeertmans jeertmans added waiting review Waiting review (or response) by a maintainer and removed waiting author Waiting author response labels Oct 11, 2026
@jeertmans

Copy link
Copy Markdown
Owner

hi @rusetiq, thanks for your reproducer! I'll review this when I can, hopefully by the end of next week

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lib Related to the library (a.k.a. module) python Pull requests that update Python code waiting review Waiting review (or response) by a maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants