Problem
FFmpegPreviewGenerator.attempt_generate_preview encodes to a private sibling file and renames it into place:
https://github.com/griptape-ai/griptape-nodes-engine/blob/main/src/griptape_nodes/retained_mode/managers/artifact_providers/video/preview_generators/ffmpeg_preview_generator.py#L243
On Windows, renaming onto a destination another handle holds open fails rather than succeeding the way it does on POSIX. Serving the previous preview for that asset is exactly such a handle, so when a regeneration finishes while a client is still reading the old preview, the rename raises, the finally discards the completed encode, and the request fails. The handle closes when the response completes, so the condition is transient: the same generation would succeed a moment later.
The user-visible result is a missing or stale video preview. In CI it shows up as an intermittent PermissionError: [WinError 5] Access is denied in test_concurrent_generation_produces_decodable_preview.
This is already solved for image previews, on an open PR
#5507 (fix/silent-broken-image-previews, unmerged) fixes the same class for the image path, and does it three ways:
- Single-flight per canonicalized source path via a new
KeyedMutex, so concurrent regenerations of one asset stop contending with each other in the first place.
- The write goes through
WriteFileRequest OVERWRITE → atomic_write_bytes, one shared implementation of write-temp-then-rename.
- The Windows failure is classified rather than raised bare.
os_manager inspects the error's own winerror and reports FileIOFailureReason.FILE_LOCKED with an artist-readable "in use by another process" message instead of PERMISSION_DENIED.
The video generator inherits none of this, because it does its own anyio.Path.replace directly instead of going through WriteFileRequest. So the generalization Collin asked about largely exists; the video path just is not on it.
Suggested direction
Route video preview writes through the same path as image previews once #5507 lands, rather than adding a second retry mechanism next to it. Then decide whether a retry is still needed at all: single-flight removes the self-inflicted contention, and what remains is a reader holding the served file, which #5507 answers by reporting FILE_LOCKED rather than by waiting.
One thing to verify on Windows before assuming #5507 covers this. It classifies on winerror in (32, 33) (ERROR_SHARING_VIOLATION, ERROR_LOCK_VIOLATION). The failure observed on the video path is WinError 5 (ERROR_ACCESS_DENIED), which is the code os.replace typically raises when the destination is open without FILE_SHARE_DELETE. If that holds, the rename case falls through to PERMISSION_DENIED and the classification misses the very failure it was written for. Not confirmed: this needs a Windows run, not a reading.
Also: atomic_write_bytes does not retry either
https://github.com/griptape-ai/griptape-nodes-engine/blob/main/src/griptape_nodes/utils/file_utils.py#L41
Worth noting rather than treating as a separate bug. With #5507's classification in place a caller at least learns the file was locked, so the question is whether any caller should wait instead of reporting. model_manager._load_status_file already hand-rolls exactly that pattern (a bounded retry over a transient Windows PermissionError, with a per-cause budget) for status-file reads, so if the answer is yes there is a third site that wants the same helper.
Related
Origin
Found while reviewing the worker PR stack. Not a worker defect and deliberately not fixed there; a fix was on #5572 and has been removed from it.
Problem
FFmpegPreviewGenerator.attempt_generate_previewencodes to a private sibling file and renames it into place:https://github.com/griptape-ai/griptape-nodes-engine/blob/main/src/griptape_nodes/retained_mode/managers/artifact_providers/video/preview_generators/ffmpeg_preview_generator.py#L243
On Windows, renaming onto a destination another handle holds open fails rather than succeeding the way it does on POSIX. Serving the previous preview for that asset is exactly such a handle, so when a regeneration finishes while a client is still reading the old preview, the rename raises, the
finallydiscards the completed encode, and the request fails. The handle closes when the response completes, so the condition is transient: the same generation would succeed a moment later.The user-visible result is a missing or stale video preview. In CI it shows up as an intermittent
PermissionError: [WinError 5] Access is deniedintest_concurrent_generation_produces_decodable_preview.This is already solved for image previews, on an open PR
#5507 (
fix/silent-broken-image-previews, unmerged) fixes the same class for the image path, and does it three ways:KeyedMutex, so concurrent regenerations of one asset stop contending with each other in the first place.WriteFileRequestOVERWRITE →atomic_write_bytes, one shared implementation of write-temp-then-rename.os_managerinspects the error's ownwinerrorand reportsFileIOFailureReason.FILE_LOCKEDwith an artist-readable "in use by another process" message instead ofPERMISSION_DENIED.The video generator inherits none of this, because it does its own
anyio.Path.replacedirectly instead of going throughWriteFileRequest. So the generalization Collin asked about largely exists; the video path just is not on it.Suggested direction
Route video preview writes through the same path as image previews once #5507 lands, rather than adding a second retry mechanism next to it. Then decide whether a retry is still needed at all: single-flight removes the self-inflicted contention, and what remains is a reader holding the served file, which #5507 answers by reporting
FILE_LOCKEDrather than by waiting.One thing to verify on Windows before assuming #5507 covers this. It classifies on
winerror in (32, 33)(ERROR_SHARING_VIOLATION,ERROR_LOCK_VIOLATION). The failure observed on the video path isWinError 5(ERROR_ACCESS_DENIED), which is the codeos.replacetypically raises when the destination is open withoutFILE_SHARE_DELETE. If that holds, the rename case falls through toPERMISSION_DENIEDand the classification misses the very failure it was written for. Not confirmed: this needs a Windows run, not a reading.Also:
atomic_write_bytesdoes not retry eitherhttps://github.com/griptape-ai/griptape-nodes-engine/blob/main/src/griptape_nodes/utils/file_utils.py#L41
Worth noting rather than treating as a separate bug. With #5507's classification in place a caller at least learns the file was locked, so the question is whether any caller should wait instead of reporting.
model_manager._load_status_filealready hand-rolls exactly that pattern (a bounded retry over a transient WindowsPermissionError, with a per-cause budget) for status-file reads, so if the answer is yes there is a third site that wants the same helper.Related
WinError 17). Different cause (cross-filesystem rename, not a held handle), same subsystem.Origin
Found while reviewing the worker PR stack. Not a worker defect and deliberately not fixed there; a fix was on #5572 and has been removed from it.