Skip to content

test: time out a job that cannot finish first instead of racing a 1ms timeout - #63

Closed
Project516 wants to merge 1 commit into
masterfrom
fix/flaky-timeout-tests
Closed

Project516 wants to merge 1 commit into
masterfrom
fix/flaky-timeout-tests

Conversation

@Project516

Copy link
Copy Markdown
Owner

What

The timeout tests (tests/ffmpeg-core.test.js, tests/ffmpeg.test.js, tests/ffmpeg-node.test.mjs) now run a long lavfi job (testsrc for an hour, to -f null) with a 50ms timeout instead of a 1s transcode with a 1ms timeout.

Why

The old test raced the job. The st core is cooperative, so the main fiber that polls is_timeout() only runs when the other fibers block, and a one-second clip can finish before it does. node-tests failed once on the #41 merge to master with expected +0 to equal 1 and passed on every earlier run. release.yml runs the whole CI workflow as its gate, so a flaky test blocks publishing. A job that cannot finish first makes the result deterministic on both cores.

Testing

CI on this PR.

@coderabbitai

coderabbitai Bot commented Oct 4, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 1 minute.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 103131b7-90a8-48c5-bb4a-758998f353a3
📥 Commits

Reviewing files that changed from the base of the PR and between ee187d3 and d54c9d8.

📒 Files selected for processing (3)
  • tests/ffmpeg-core.test.js
  • tests/ffmpeg-node.test.mjs
  • tests/ffmpeg.test.js
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@project516-review-bot project516-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Replaces the 1ms-timeout / 1s-clip timeout tests in three test files with a 50ms timeout against an hour-long lavfi testsrc job writing to null, so the job cannot finish before the timeout fires on either the st or mt core. The old 1ms timeout raced the cooperative st scheduler, which is why the node test flaked once on the #41 merge.


review-bot, model qwen/qwen3.8-27b:free, verdict approve

@Project516

Copy link
Copy Markdown
Owner Author

Closing. A long running job with a short timeout is deterministic on st, but on the mt core the exec() after it fails with memory access out of bounds ([ffmpeg-core][mt] setLogger() should handle logs, run 37241597951). That is a separate mt problem with timing out a long job, not a test fix. The original race is st only, so the replacement skips that one test on st with the reason, and keeps it on mt.

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