fix(video): enforce one absolute deadline for streaming frame decode - #9393
Merged
lstein merged 3 commits intoAug 9, 2026
Merged
Conversation
`iter_video_frames` waited up to `timeout` for a decoder/stream slot and then handed `_iter_video_frames_unbounded` a fresh full `timeout` for the first frame, so a call could take nearly 2x the bound its caller thought it was enforcing (upload probing and the video nodes both size their budget on that value). The capacity wait is now charged against the same deadline as the first frame — the remaining-time-after-acquire pattern `_run_worker` already uses. Frames after the first still get a full `timeout` each: past the first frame the budget is a decoder-inactivity bound, not a queueing one, and shrinking it would kill legitimate long decodes that had queued for a slot. Deferred non-blocker from PR invoke-ai#9163. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
requested review from
JPPhoto,
Pfannkuchensack,
blessedcoolant and
dunkeroni
as code owners
July 28, 2026 00:51
JPPhoto
approved these changes
Aug 9, 2026
JPPhoto
left a comment
Collaborator
There was a problem hiding this comment.
LGTM! Other findings/issues that may be covered in related PRs (I haven't yet gotten to them) follow. If you don't have PRs for these, I suggest making them:
-
invokeai/app/util/video_thumbnails.py:296,369: Relative remainder is converted into a new deadline after worker startup, so slow startup can exceed the promised single timeout. Test: delay worker startup after near-expired capacity wait and assert total time stays within the original deadline. -
tests/app/util/test_video_thumbnails.py:308-319: Unsynchronizedsleepcan release the slot before the consumer starts, causing a flaky false failure. Test: synchronize at semaphore acquisition before starting the hold timer.
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.
Summary
Follow-on to #9163 (deferred non-merge-blocker).
iter_video_framescharged itstimeouttwice:timeoutto acquire the stream slot and a decoder slot, then_iter_video_frames_unboundeda fresh fulltimeoutfor the first frame.So a caller that waited 29 s for capacity and then hit a hung decoder blocked for ~59 s — twice the bound it believed it was enforcing. Both callers size real budgets on that value: upload probing (which runs on a request worker) and the video nodes.
_run_workeralready had the right shape for the one-shot path — take a deadline up front, and pass the remaining time to the worker after acquiring the slot. This applies the same pattern to the streaming path.Why the first frame only
timeoutin the streaming decoder is a decoder-inactivity bound, not a total wall-clock budget: it is restarted after every yielded frame so a long video isn't killed for being long. Shrinking it for all frames (the naive "passremainingthrough") would mean a decode that queued 29 s for a slot then got 1 s per frame — spurious failures on perfectly good videos.So
_iter_video_frames_unboundedgains an optionalfirst_frame_timeoutthat overrides the budget for the first frame only. Post-first-frame behavior is unchanged.Testing
New regression test
test_capacity_wait_and_first_frame_share_one_deadline: holds the single stream slot for 1.6 s of a 2.0 s budget, then releases it against a worker that never produces a frame, and asserts the total stays under one budget plus jitter. Verified it fails onmain(~3.6 s, the oldhold + timeout) and passes with the fix.pytest tests/app/util/test_video_thumbnails.py— 54 passed.🤖 Generated with Claude Code