Studio: queue media generation behind model teardown - #8866
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c90dcd4cca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while self._teardown_waiters and not cancel.is_set(): | ||
| # Cancellation uses its independent lock and cannot notify this condition | ||
| # while a load owns _lock, so wake periodically only while actually queued. | ||
| self._teardown_drained.wait(timeout = 0.1) |
There was a problem hiding this comment.
When a generation wins _generate_lock after a replacement has reserved teardown, it releases that lock and waits on this condition. The replacement can then acquire both _generate_lock and _lock and hold _lock throughout model construction. If the user calls cancel_generate() in that ordering, the independent cancellation lock sets the event, but Condition.wait() must reacquire _lock before returning, so the request remains blocked until the potentially minutes-long load completes; the 100 ms timeout does not make cancellation responsive. Use cancellation waiting that does not require reacquiring the load-held state lock, or avoid holding that lock during construction.
Useful? React with 👍 / 👎.
| if self._state is None and self._teardown_waiters == 0: | ||
| raise RuntimeError(VIDEO_NOT_LOADED_MSG) | ||
| state = self._state | ||
| if state is not None: |
There was a problem hiding this comment.
When a standard replacement has torn down the old pipeline but not committed the new one, _state is None while _teardown_waiters is nonzero, so this conditional skips _resolve_keyframes, _resolve_references, and _resolve_flow_shifts. Consequently malformed image data, unsupported references, or invalid flow shifts receive a successful /video/generate response instead of the route's synchronous 400/422; the daemon worker only reports a failed job later through polling. Queuing behind replacement should not bypass the input-validation contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0a1a3cc6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self._state is None and self._teardown_waiters == 0: | ||
| raise RuntimeError(VIDEO_NOT_LOADED_MSG) |
There was a problem hiding this comment.
When an unload supersedes a standard replacement after that replacement has torn down the old state, _loading is cleared but the stale load can retain a teardown reservation until its lengthy construction reaches the next token check. During that interval _state is None and _teardown_waiters is nonzero, so /video/generate is accepted with no incoming model, waits behind the stale worker, and eventually fails through polling with VIDEO_NOT_LOADED_MSG instead of returning the synchronous 409. Treat the waiter as a queueable replacement only when _loading still describes an active load.
Useful? React with 👍 / 👎.
| if _defer_shape_validation or ( | ||
| _validated_state is not None and state is not _validated_state | ||
| ): | ||
| validate_video_request_shape( | ||
| fam, width = width, height = height, num_frames = num_frames |
There was a problem hiding this comment.
If begin_generate() validates against the currently loaded family and a replacement reserves teardown before the worker acquires the generation slot, the job now intentionally survives and runs on the replacement. The new _validated_state mismatch branch is fresh evidence that this ordering is handled, but it repeats only shape validation; keyframes, references, and flow shifts are first checked against the old family and are checked against the replacement only later inside the daemon worker. Inputs unsupported by the replacement therefore receive a successful generate response and fail asynchronously rather than preserving the route's synchronous 400/422 contract, so the full conditioning validation must be repeated when the state identity changes.
Useful? React with 👍 / 👎.
| with self._generation_cancel_lock: | ||
| cancels = set(self._queued_generate_cancels) | ||
| if self._active_generate_cancel is not None: | ||
| cancels.add(self._active_generate_cancel) | ||
| if not cancels: | ||
| return False | ||
| cancel.set() | ||
| for cancel in cancels: | ||
| cancel.set() |
There was a problem hiding this comment.
When two image requests reach either generation endpoint concurrently, the second waits on _generate_lock solely because the first is denoising; neither route has a busy-request guard. _generation_slot() now adds that second request to _queued_generate_cancels before it knows whether any teardown exists, and cancel_generate() sets every event in that set. Pressing Stop for the active request therefore also makes unrelated serialized requests fail with the cancellation sentinel, whereas the previous implementation signalled only the active generation. Track teardown/load waiters separately from ordinary generation-lock waiters, or restrict cancellation fan-out to requests actually queued by lifecycle teardown.
Useful? React with 👍 / 👎.
for more information, see https://pre-commit.ci
1 participant
Summary
Fixes #8309.
Studio image and video generation could race with a model replacement or unload. Because Python lock acquisition is not FIFO, a newly queued generation could acquire the generation lock after teardown had been reserved but before teardown completed.
This caused generation requests to incorrectly report that they were cancelled or, for video replacements, observe the temporary unloaded state while the replacement model was still being constructed.
Changes
Regression coverage
Added deterministic threaded tests covering:
Validation
git diff --checkpassed.