fix(previews): atomic writes, single-flight regeneration, and diagnosable failures for image previews

[SavagePencil]

Fixes the silent broken-image-preview failure reported in griptape-ai/internal#240.

What changes for users

Scenario	Before	After
Open a workflow after upgrading past a preview-schema bump (e.g. v0.99.0's 0.1.0→0.2.0, which invalidated every preview on disk)	Each editor component regenerates the same .webp concurrently, truncating it in place while the static server streams it to the browser; the browser caches the torn body under a same-second ?t= URL and the image is permanently broken, with every failure logged at DEBUG	Exactly one regeneration per source; the browser only ever sees the prior preview or the complete new one
A preview can't be generated	The original file is silently served; the editor shows an unexplained full-size image or empty frame	The response carries preview_failure_reason so the editor can tell the artist why. Engine-side: real failures (provider error, failed write) log at WARNING; a vanished source — routine, unactionable, once per component — stays at DEBUG, carried by a structured source_file_missing field rather than message matching
Workspace synced/copied between machines, drives, or cloud storage (mtime drift)	Preview branded stale forever by exact-float mtime equality — regenerates on every request, or hard-fails under DO_NOT_GENERATE, with a valid .webp sitting on disk	Size compared exactly, mtime within a 2s tolerance; valid previews keep serving
Source file edited while its preview renders — including a same-size in-place rewrite (re-rendered frame, cp over the file)	Preview depicts the old content but is recorded as fresh	The post-generation verify compares size and mtime exactly (both stats are same-filesystem, seconds apart — the sync-drift tolerance deliberately does not apply here) and retries once; a still-changing source stops after the retry
Disk fills mid-overwrite (small workstation disk, shared storage near capacity)	In-place truncate destroys the old file first, then the write fails: the user's file is gone	Clean DISK_FULL failure with an artist-readable message; the previous file is untouched
A 0600 secrets file (e.g. a packaged .env) is rewritten	Written in place at its own mode	Same guarantee, atomically: the scratch copy is created at a 0600 floor and aligned to the destination's exact mode before any content is written — the payload is never on disk at a looser mode
Preview metadata portability	source_macro_path recorded this machine's resolved absolute path (/Users/jason/...)	Records the macro template ({inputs}/images/...)

Root cause

A stale preview arrives as several concurrent editor requests (one per component rendering the artifact). With no single-flight guard, each judged the preview stale and regenerated it; the PIL generators wrote via WriteFileRequest(OVERWRITE), an in-place open(mode="w") truncate+write that a concurrent browser GET could stream mid-rewrite. The 1-second ?t= cache-buster made same-second URLs identical strings, so the browser cached the torn body — and the editor's <img> has no onError, making one bad load permanent. #5445 fixed this exact race for video (and documented it) but left the image generators on the torn path; the same commit's schema bump then invalidated every preview at once, maximizing the collision window. Preview failures have logged at DEBUG since #4443.

Changes

• Atomic OVERWRITE writes — every WriteFileRequest OVERWRITE goes through a private sibling scratch file + fsync + rename (atomic_write_bytes). The scratch is named .gtn-write-partial-<uuid><ext>.partial: dot-prefixed (hidden from artists), self-identifying if a power loss strands one, and ending in a fixed .partial so it can never match an extension glob over the directory (pathlib.glob matches dotfiles; pollers glob *.json in directories this writes into). Mode handling: existing destinations keep their exact permissions with no looser-mode exposure window; new files get the kernel-applied umask default. Also: symlink write-through, text-mode newline translation, best-effort parent-dir fsync, Windows rename-contention → FILE_LOCKED via winerror, ENOSPC → DISK_FULL. portalocker is unchanged for append and exclusive-create; OVERWRITE's position outside the lock protocol is documented on the request.
• Single-flight regeneration — new KeyedMutex (per-key threading.Lock, non-blocking acquire + async sleep polling) serializes preview lookup+regeneration per canonicalized source path. asyncio.Lock is unusable here: sync handle_request drives async handlers on a transient event loop per call from arbitrary threads. Polling (rather than parking to_thread workers) avoids deadlocking the bounded executor the lock holder itself needs.
• Staleness tolerance — file_utils.mtimes_match (2s window) replaces exact float mtime equality for the recorded-metadata staleness check; size still compared exactly. GetFileInfo takes one stat() so size/mtime can't straddle a concurrent replace.
• Mid-generation verify + retry — generation re-stats the source afterward with an exact compare (the tolerance exists for recorded-metadata drift, not two same-machine stats; a tolerant compare here made same-size rewrites invisible) and retries once; a source still changing after the retry stops, and its preview may briefly show older content until it settles.
• ffmpeg promote retry — the video preview's rename onto its destination retries a transiently denied rename (Windows sharing violation from a competing promote or an active reader) instead of failing; a persistent denial still raises.
• Diagnosability — CreateStaticFileDownloadUrlFromPathResultSuccess.preview_failure_reason (additive, human-readable, presence-only contract) gives the editor something to show; fallback logging is WARNING for real failures and DEBUG for the routine missing-source case (structured source_file_missing on the failure payload); ?t= is millisecond-resolution so same-second URLs can't share a cached bad response.
• Macro threading — the original {inputs}/… template flows through the download-URL path into preview metadata instead of a machine-specific absolute path.

No editor dependency: all changes are engine-internal or additive; current editors ignore the new fields.

Verification

• make check clean; full unit suite passes (the one test_config_project_layer_dropout failure pre-exists on main).
• ~35 new tests: staleness boundaries, single-flight (N concurrent → one regeneration), cross-loop/cross-thread mutex exclusion and cancellation, atomic replace/mode/symlink/newline/ENOSPC/glob-immunity/permission-window behavior, mid-generation retry (append, same-size rewrite, still-changing, vanished source), preview_failure_reason + log-level split, macro preservation, ffmpeg promote retry. Patch coverage 100%.
• Repro harness staging the reporter's actual artifacts: pre-fix, 6 concurrent requests → 6 regenerations per round; post-fix → exactly 1, zero torn reads (also at concurrency 12), metadata heals, macro recorded.
• Three internal review-loop iterations plus three rounds of automated PR review; every finding fixed and replied to on its thread.

Follow-ups (separate repos)

• Editor (griptape-vsl-gui): <img> onError/retry, surface preview_failure_reason, request dedupe — issue to be filed.
• Standard library: same macro-preservation fix in select_from_grid.py (branch ready, ships separately).
• Known rough edge (pre-existing, out of scope): cloud-URL Path() round-trip in the degenerate-macro fallback.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[griptapeops]

⏳ GriptapeOps is working… on @SavagePencil's request

[griptapeops]

⏳ GriptapeOps is working… on @SavagePencil's request

• Read PR description + linked issues/PRs
• Read the diff with surrounding context
• Judge against the review rubric
• Post review + commit status

Review posted at 94d1374.

[griptapeops]

Aimed at the right place: the atomicity fix sits on the write primitive and the single-flight sits on the lookup, not downstream of either.

1 correctness finding, 3 advisory:

• artifact_manager.py:581 — the mid-generation verify inherits the 2s mtime tolerance, so a same-size rewrite reads as unchanged
• static_files_manager.py:249 — advisory: WARNING on every fallback re-adds the noise #4443 removed
• file_utils.py:84 and :39 — advisory: unhidden temp siblings; import-time umask mutation
• os_manager.py:3192 — advisory: diff-relative comments

[SavagePencil]

@griptapeops re-review

[griptapeops]

✅ GriptapeOps finished @SavagePencil's request in 32m 12s

• Find prior review cycle + open threads
• Check out new head, judge each of the 5 open findings against current code
• Review commits since 94d1374
• Post review + commit status

Review posted at 30d6ef0.

[griptapeops]

All 5 findings from the last round are fixed and resolved; the mtime fix is the right one and its test genuinely exercises the mtime arm (reverting to mtimes_match locally fails it).

The new scratch-file naming introduced 2 correctness findings, both open:

• file_utils.py:81 — keeping path.suffix makes the scratch file match an extension glob over the destination directory; a mid-write poll sees a duplicate model-download row
• file_utils.py:83 — the scratch file holds the full new content at 0o666 & ~umask before the chmod tightens it, so a 0600 destination's replacement is briefly world-readable

Also still open: test_file_utils.py:654 on the LLMisms thread.

[SavagePencil]

@griptape-ops re-review

[SavagePencil]

@griptapeops re-review

[griptapeops]

✅ GriptapeOps finished @SavagePencil's request in 9m 46s

Re-review (cycle 8):

• Read prior reviews and open threads
• Check out new head (7dadbbee), verify the 3 open findings against current code
• Review the commit since 30d6ef0c
• Reply on threads + resolve
• Post review + commit status

Review posted at 7dadbbe.

[griptapeops]

All 3 open findings fixed and resolved, and 7dadbbe is clean against the rubric. Verified atomic_write_bytes and its tests against the current code: the scratch name can no longer match an extension glob, and the mode observed mid-write now equals the destination's own in every case I staged — strict 0600, looser 0664, tight umask, setgid, and brand-new (identical to open(mode="w")). Also re-checked the paths the earlier stat() could have disturbed: read-only destination, directory destination, symlink write-through, and no stranded scratch on failure.

make check clean on the changed files; tests/unit/utils/test_file_utils.py 49 passed. The two test_git_utils failures in my run are my clone's tokenized remote and reproduce on main.

[collindutter]

I'm a little weary of the complexity this PR adds. Breaking down the root cause a bit:

A stale preview arrives as several concurrent editor requests (one per component rendering the artifact).

With no single-flight guard, each judged the preview stale and regenerated it; the PIL generators wrote via WriteFileRequest(OVERWRITE), an in-place open(mode="w") truncate+write that a concurrent browser GET could stream mid-rewrite.

So the first request would start writing the preview file. Meanwhile, a second request would generate a preview for a partially written preview file.

Am I understanding this correctly?

If so: seems like we could cut out this whole class of problem if we make the preview's file name the cache key. i.e.

  def preview_key(source: Path, gen: str, params: dict, schema: str) -> str:
      st = source.stat()
      return blake2b(
          f"{source}|{st.st_size}|{st.st_mtime_ns}|{gen}|{canonical_json(params)}|{schema}".encode(),
          digest_size=8,
      ).hexdigest()

  # cat.png → cat.png-a3f29b41.webp

Then, resolving looks something like:

  def resolve_preview_url(source: Path) -> str:
      key = preview_key(source, *current_settings())
      return f"/previews/{rel(source)}/{source.name}-{key}.webp"

What do you think?

[SavagePencil]

Close on the mechanics, one correction: the second request doesn't read a partial preview — both requests regenerate from the source, and the tear happens because each writes the .webp in place while the browser is streaming that same file from the static server. The reader at risk is outside the engine entirely, which is why nothing engine-side ever noticed.

On the proposal: I think immutable cache entries are the right north star, and I'd genuinely like to get there. Three things the sketch needs solved before it replaces what's here, though:

1. It still needs the atomic write and single-flight underneath. Every consumer of one source computes the same new key, so N concurrent requests on a stale source all write the same new filename — a browser fetching request A's URL while request B is mid-write to that name is the same torn read on a new name. Content-addressing makes old URLs immortal; it doesn't make new-file creation safe.
2. st_mtime_ns in the key makes the cache machine-local. The previews dir lives inside the project, so the moment a workspace syncs, every mtime shifts, every key changes, every collaborator regenerates everything — and the old hash-named files orphan on every machine. That's the GC story this design owes before it ships (today's overwrite-in-place layout is self-cleaning by construction).
3. Path-in-key means no dedupe either — the same bytes at two paths (or via a symlink) get independent previews. Fine, but worth being explicit that this is a location+stat cache, not a file cache; a true content hash would dedupe but costs reading the full source (or caching content-hash-by-stat, which reinvents the sidecar).

There's a cheaper cut of your insight that dodges all three: keep the stable filename, put the key in the URL — a deterministic stat-derived ?v=<key> instead of the mint-time ?t=. Same content → same URL → browser cache hits (better than today, where every mint busts); changed content → new URL; combined with this PR's atomic rename, the file is never torn and the URL is never stale. No renamed files, no GC, no template migration.

Proposal: land this PR as the bleeding-stopper (Jason's bug is live in 0.100), and I'll file the content-addressed design as a follow-up issue capturing your sketch plus the GC/sync/dedupe questions — with ?v= as the incremental step we could take immediately. Worth saying: about half this PR is orthogonal to the naming question and needed under either design (atomic OVERWRITE as a general primitive, preview_failure_reason/log levels, macro portability, DISK_FULL mapping).

[cjkindel]

Do we want to expand this to include videos as well (is there a media-generic implementation possible?): #5579

[SavagePencil]

@cjkindel Good timing — most of #5579 is already in this PR, and the rest is a clean extraction:

• Single-flight is media-generic already: the KeyedMutex wraps GetPreviewForArtifactRequest itself, keyed on the canonicalized source path, before any provider dispatch — video regenerations of one asset stop contending exactly like images.
• The specific failure Windows: a finished video preview is discarded when its rename target is still held open #5579 describes is fixed here: this PR adds a bounded retry (3 attempts, 50ms apart) to the ffmpeg promote, because two promotes racing one destination — or a reader serving the old preview, your case — deny the rename only transiently on Windows. The intermittent WinError 5 in test_concurrent_generation_produces_decodable_preview was reproduced on this PR's CI and is what prompted the fix; there's now a cross-platform test forcing two denials before success. A persistent denial (long-lived reader) still raises, which then surfaces via preview_failure_reason instead of vanishing.
• What's genuinely left for a media-generic implementation: ffmpeg can't route its encode through WriteFileRequest (the subprocess writes the file itself), but its hand-rolled scratch-name + promote could share a helper with atomic_write_bytes' rename/retry semantics — one implementation of "promote a scratch file onto a served destination." I'd scope Windows: a finished video preview is discarded when its rename target is still held open #5579 to that extraction; if the retry alone resolves the CI flake and the user-visible symptom, closing it as fixed-by-this-PR is also defensible.

Related: #5607 now tracks the larger immutable-preview-cache design from @collindutter's thread, which would subsume promote collisions entirely for the serve case (old URLs keep serving old bytes; nothing rewrites a served name).

[collindutter]

Couple code quality comments, will defer to you. As mentioned in my other comment, I have some concerns on the low-level complexity this adds around our file writing operations. I'm trusting that you've proven through testing that this complexity is warranted to solve the root issue.

[collindutter]

An arbitrary sleep feels a bit fragile. At the very least we can extract this retry logic to a dedicated module? Don't want to repeat this throughout the engine.

[SavagePencil]

Agreed on both counts — done in #5616: the retry moved to a shared tenacity-based promote_scratch_file/_async in file_utils (tenacity was already the idiom in http_utils, so this extends it rather than adding a second pattern), and both the ffmpeg promote and atomic_write_bytes now go through it. The PR body carries a full audit of every retry-shaped site in src/ — the only remaining hand-rolled loop is _load_status_file, left deliberately because its per-cause budgets (#5471) would need a custom stateful stop bigger than the loop.

[collindutter]

See above comment, seems like we're re-implementing retry logic quite a bit. Can we extract and/or use something like tenacity?

[SavagePencil]

Agreed on both counts — done in #5616: the retry moved to a shared tenacity-based promote_scratch_file/_async in file_utils (tenacity was already the idiom in http_utils, so this extends it rather than adding a second pattern), and both the ffmpeg promote and atomic_write_bytes now go through it. The PR body carries a full audit of every retry-shaped site in src/ — the only remaining hand-rolled loop is _load_status_file, left deliberately because its per-cause budgets (#5471) would need a custom stateful stop bigger than the loop.