Treat an album update as a patch, not a replacement - #372
Merged
Conversation
lstein
added a commit
that referenced
this pull request
Aug 19, 2026
`/update_album/` treated an omitted `umap_eps` as "clear it", and `saveAlbumChanges()` never sends the key — the edit form has no Cluster Strength control, it only carries name, description, encoder, the dimension gates and the paths or boards. So tuning an album's Cluster Strength and then renaming the album threw the tuning away, while the docs this branch adds promise the album keeps the number from then on. Omitted now means "keep what is stored", the rule `index` and the InvokeAI password on the same call already follow. An explicit null still clears, because that is how `/set_umap_eps` hands an album back to the derived value, and how the bookmark manager echoes back an album that never had one. Note this line collides with PR #372, which is fixing the same omitted-means-erased bug across every field of this endpoint; whichever lands second must not restore the wipe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
added a commit
that referenced
this pull request
Aug 19, 2026
* feat: derive a Cluster Strength for albums that have not set one eps had no album-independent right answer and one fixed number for all of them. UMAP's coordinates have no fixed scale — their span grows as the point count shrinks — so the value that carves a large library into useful clusters labels a small album as entirely unclustered. Measured on a real 240-image album: 0 clusters, 100% unclustered, which reads as a broken semantic map. `umap_eps` becomes nullable, meaning "nobody has chosen one". Those albums now resolve to a value derived from their own coordinates, marked `auto` beside the Cluster Strength field; typing a number stores it and takes over, and clearing the field hands it back — previously there was no way back to a default at all. The rule (backend/cluster_eps.py) is a refinement of the median k-distance heuristic InvokeAI's image map uses. Candidates are quantiles of the k-distance distribution, and the scan takes the largest whose biggest cluster still holds under a quarter of the album. The median alone makes half the points core points by construction, which lands at ~30% unclustered on a large library — much worse than a hand-tuned eps. Walking up to the knee instead reproduced hand-tuned values on real albums (0.119 against a hand-set 0.12 on 38k images, 0.051 against 0.05 on 86k) while fixing the small ones. Also borrowed from that implementation, and worth keeping separate: * The neighbour-pair budget shrinks eps until sklearn's DBSCAN will fit in memory. It applies to every value, typed or derived, because the Cluster Strength control could always ask for a radius a six-figure album cannot afford — an out-of-memory crash, not a slow response. * The span clamp applies to derived values only. A number the user typed is theirs to keep; retuning it silently would make the control lie. Its 0.05 fraction was copied at first and had to be raised: that figure suits a median, and against a scan that deliberately climbs higher it overrode correct answers outright. Deriving costs a k-distance pass plus a handful of DBSCAN fits — 2.4s on 122k points — so it runs in a thread and is memoized beside umap.npz, keyed by a fingerprint of the coordinates so a re-index invalidates it. Verified through the real app on the 240-image album: 0 clusters/240 unclustered at the old fixed 0.07, 9 clusters/41 unclustered derived, with the second request served from the memo in 1ms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: write a derived Cluster Strength as an absent key, not an explicit null A PhotoMapAI predating the nullable umap_eps parses `umap_eps: null` into a non-nullable float field and refuses to load the config at all. The two versions share one config file, so writing a null would stop the older one from starting — an unpleasant surprise for anyone testing this branch beside their normal install, or downgrading after it. Absent and null mean the same thing to this codebase, and absent is what the older one already handles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: load an album's UMAP coordinates off the event loop `album_umap_coords` reads `Embeddings.umap_embeddings`, which is not a read at all when `umap.npz` is missing or older than `embeddings.npz`: the property refits UMAP from scratch, minutes of CPU on a large album. Any rewrite of the index leaves exactly that state behind — deleting an image, an `update_images` run, a hand-removed cache. Three call sites ran it inside the endpoint coroutine, stalling every other request for the duration, including the indexing-progress polling on the very screen the semantic map is opened from: * `/get_umap_eps` called it directly. Before this branch it was a config read, and `initializeUmapWindow` awaits it before anything else. * `/cluster_labels` passed it as an *argument* to `asyncio.to_thread`, so it was evaluated before the thread was ever spawned. This is the trap the new `album_umap_coords_async` docstring names, since the call reads as threaded. * `/umap_data` loaded it directly. That line predates this branch, but the map fetches it in parallel with `/cluster_labels`, so leaving it would have stalled the server regardless of what the other two did. Test drives all three against a deliberately stale `umap.npz` and asserts the refit happened somewhere without a running event loop — and that it happened at all, so the assertion cannot pass vacuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: keep a tuned Cluster Strength when an album is edited `/update_album/` treated an omitted `umap_eps` as "clear it", and `saveAlbumChanges()` never sends the key — the edit form has no Cluster Strength control, it only carries name, description, encoder, the dimension gates and the paths or boards. So tuning an album's Cluster Strength and then renaming the album threw the tuning away, while the docs this branch adds promise the album keeps the number from then on. Omitted now means "keep what is stored", the rule `index` and the InvokeAI password on the same call already follow. An explicit null still clears, because that is how `/set_umap_eps` hands an album back to the derived value, and how the bookmark manager echoes back an album that never had one. Note this line collides with PR #372, which is fixing the same omitted-means-erased bug across every field of this endpoint; whichever lands second must not restore the wipe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: re-resolve the Cluster Strength after the album is re-indexed The `albumIndexUpdated` listener redrew the map without asking the server what the strength now resolves to, and `fetchUmapData` sends whatever the spinner holds. New coordinates invalidate a derived value — the server keys its memo on a fingerprint of them precisely so a re-index recomputes — but with the map open that recomputed number was never requested, so the backend invalidation this branch added had no effect on a live session. The case that bites is the one the feature exists for: open the map on an album that has not been indexed yet and the spinner gets the not-indexed-yet fallback, a fixed number, under an "auto" badge. Indexing finishes, the map redraws at that fixed number, and a few-hundred-image album lands back at zero clusters — the exact failure this branch set out to fix, now wearing a badge that says the value was chosen for it. Closing and reopening the map already recovered, since `toggleUmapWindow` re-fetches on show; this makes staying open behave the same. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: serialize rebuilds of a stale UMAP cache Threading the coordinate load in the previous commit removed an accidental mutual exclusion. Both endpoints used to load coordinates on the event loop, so the two loads could not overlap: whichever ran first refit and rewrote umap.npz, and the second found it fresh and read it. Off the loop they run at the same time, and `umap_embeddings` guards a rebuild with nothing but an mtime that is not written until the fit finishes — so both see the same stale cache and both fit. Reproduced deterministically: index an album, delete one image (which rewrites embeddings.npz and leaves umap.npz older), open the semantic map. `/get_umap_eps` returns early for an album with a stored strength, so nothing warms the cache, and the map then fetches `/umap_data` and `/cluster_labels` in parallel. Two fits is not just twice the work and twice the peak memory. UMAP is constructed without a `random_state`, so the layouts differ, and the two endpoints hand back cluster ids describing different coordinates — the divergence the hover labels depend on not happening. Worse, both writers raced on one temp file, which could rename a corrupt archive into place; because freshness is judged by mtime, that archive then looks valid forever and every later request 500s until someone deletes it by hand. So: one rebuild lock per index path, with the freshness check repeated under it, and `atomic_savez` given a per-process, per-thread temp name so concurrent writers of any cache race to rename whole files instead of interleaving into one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: re-check what moved while the post-re-index resolve was in flight The re-resolve added in 47166a1 put an await between the listener's checks and the redraw they guard, and the await is exactly as long as deriving a strength — seconds to minutes on a large album. Four things can move in it: * **The album.** `refreshResolvedEps` read `state.album` when building the request but wrote whatever came back into the shared spinner. Switch albums during the derive and album A's number and its "auto" badge land beside album B's map, which is then fetched with A's strength. The album is now pinned for the round trip and the reply dropped if it lost. * **The window.** Closing the map during the derive still rendered Plotly into a hidden container, and consumed the dataChanged flag the next open needs to redraw. * **`state.dataChanged`.** It was set before the await, so any redraw finishing inside it cleared the flag and `fetchUmapData` silently no-opped — losing the one redraw this listener exists to guarantee. It is now re-armed immediately before the call. * **The spinner itself.** A pending debounced edit means the user is typing a number that is about to become a stored one; overwriting it moved the value under the cursor, re-showed "auto" for a value that was about to be the user's own, and left the map drawn at the derived number while the config stored the typed one. The re-resolve is skipped while an edit is pending, and the debounce redraws against the new coordinates anyway. The pending-edit check keys off the debounce handle, which the timer never cleared when it fired — left alone, one edit would have disabled the re-resolve for the rest of the session. A failed resolve now says so rather than redrawing silently with a stale number under a badge claiming otherwise. The tests live in their own file and import umap.js exactly once: it registers window listeners at module scope and nothing unregisters them, so the per-test re-import the sibling suite uses leaves several live listeners racing each other through the shared spinner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: drop a cached config edit whose save failed `ConfigManager` caches the parsed config and hands out the live `Album` objects inside it, and `set_umap_eps` mutates one in place before asking for the save. The cache was only invalidated after `save_config()` returned, so a failed write (read-only config dir, disk full) left the mutation sitting in memory while the caller got a 500. That was survivable while an omitted `umap_eps` meant "clear it". Now that it means "keep what is stored", the next unrelated edit reads the phantom forward and writes it out: a Cluster Strength the user was told had failed to save comes back for good when they rename the album. Invalidate in a `finally` instead, at all three call sites that share the pattern, so the next read comes from disk whatever happened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: point users at clearing the Cluster Strength field An album that carries a saved Cluster Strength never shows the auto marker, and there is no migration, so every album that predates this feature keeps whatever number the old code stamped on it — 0.1 on anything created through the UI, 0.07 on anything edited. Clearing the field is the only route to a derived value, and it was a clause at the end of a paragraph. Promoted to a tip that says what clearing does, that it is reversible, and that small albums are where the old fixed numbers actually fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
``POST /update_album/`` rebuilt the album from the request payload alone, so every field the payload left out was reset to its model default. Real callers send partial payloads — the bookmark menu sends five keys, the search dialog sends the album plus three overrides — and the worst of what that reset was ``source_type``: an InvokeAI-board album came back as a directory album with every ``invokeai_*`` field cleared. After that the album's own indexing walked InvokeAI's output directories as if they were ordinary folders, and deletions stopped routing through InvokeAI's API, leaving dangling rows in its database. Reproducing it took one bookmark and the "Move to folder → Yes, Add Folder" prompt. Now a key the payload does not carry keeps its stored value, and falsy values that real edits send (``min_image_bytes: 0``, ``use_query_optimization: false``, an emptied description) still win. Null is treated as absent, because ``create_album`` turns a None into the model *default* rather than into a cleared field — honoring it would swap an album's encoder for the host default and silently invalidate its index. The one field that genuinely needs clearing, ``invokeai_username`` (InvokeAI leaving multi-user mode), goes through a helper that keys on presence instead. Two things the patch rule would otherwise have broken, and one it exposed: * Board albums pass ``image_paths=None`` so the model re-derives them. Carrying the stored list over would suppress that derivation and pin the album to its old root the moment the user edits the root — the re-index would write under the new root while the access gate still pointed at the old one, 404ing every image. * ``min_search_score`` is left to the model to re-resolve when the encoder *family* changes. The floors differ by an order of magnitude (0.005 for SigLIP, 0.2 for CLIP), so carrying one across makes the new encoder look like it returns nothing — but swapping one CLIP model for another must not discard a score the user tuned by hand. Both the model and the router now read the rule from one ``default_min_search_score``. * An update whose ``source_type`` disagrees with the stored album is refused: the edit form branches on the stored kind and never offers to switch, so a disagreement is a partial payload being read as a replacement. A board album's directories cannot be changed or added to, and saying so beats accepting the request and quietly deriving something else — the request is refused with a 400 unless it merely echoes the album's paths (either the stored list or the one the same request derives, both of which callers legitimately send while a root edit is in flight). An ``HTTPException`` raised inside the handler is re-raised rather than wrapped, so that 400 (and the 404 for an unknown key) survives as itself. Frontend: the bookmark menu no longer offers to add a destination folder to a board album, since the backend has to refuse it, and its update payload carries only the keys it is changing. Tests: eleven new cases covering the demotion, the refused folder and source-type changes, root changes moving the derived paths, an explicit null neither clearing a tuning field nor surviving as one, the username clear, the encoder-family re-resolve and the same-family preservation, a blank index, and an unresolvable path being refused rather than crashing. Board-album tests also stopped deleting from the developer's real user data directory: ``default_board_index_path`` resolves against ``platformdirs.user_data_dir``, which nothing isolated, so a test album keyed like a real one removed the real index on cleanup. Backend 680 passed, frontend 581 passed, ruff and prettier clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A blank password means "I did not touch this" — the edit form never sees the stored one — so there was no way to say "forget it". A backend that leaves multi-user mode left a credential in config.yaml that nothing but a text editor could remove, and ``has_invokeai_password`` kept reporting it to the form. ``invokeai_password: null`` now clears it, while ``""`` and an omitted key still keep what is stored. No existing caller can send that null by accident: ``_album_public_dict`` carries ``has_invokeai_password`` and never the password itself, so the search-settings persister's round trip cannot reach it, and the bookmark menu sends three keys. The only source is a new *Forget saved password* checkbox in the edit form, offered only when there is one to forget and reset every time the form opens; a password typed in the same edit wins over it. The checkbox needs its own ``[hidden]`` rule: it lives in a ``<label>`` inside a ``.form-group``, and that selector sets ``display: block``, which as an author rule beats the UA stylesheet's ``[hidden]`` — the element would have stayed on screen for albums with no stored password. Same shape as the existing ``.video-player-*[hidden]`` rules. Clearing (or changing) a credential also invalidates the cached JWT. The token is keyed on ``(base_url, username)``, neither of which has to change when a password does, so a forgotten password would otherwise keep working from cache until the token expired — up to a day later. Tests: the three-way backend distinction (absent / blank / null), and on the frontend the payload matrix including "typed wins over the box", the row's visibility under the real CSS rule (asserting computed style, since the attribute alone reported hidden while the box was visible), and the reset on re-open. Backend 681 passed, frontend 587 passed, ruff and prettier clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebase fallout, not a behaviour change. #369 gave board albums an ``outputs/videos`` directory alongside ``outputs/images``, so "the paths this root derives" is now both. The test echoed back only the images directory, which is genuinely not what the album derives, and the guard was right to refuse it with a 400. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
force-pushed
the
lstein/fix/update-album-preserves-fields
branch
from
August 20, 2026 01:57
42be70a to
d42a031
Compare
Adversarial review of this branch. The first is a regression the patch
semantics introduced; the rest are older, but this endpoint's new contract
is what makes them contradictions.
**A null min_search_score kept a stale floor across an encoder change.**
The re-resolve was gated on `"min_search_score" not in album_data` while the
value came from `kept`, which treats a null as absent — so the two halves
disagreed. An explicit null both suppressed the re-resolve and fell through
to the stored number, leaving a CLIP floor of 0.35 on a SigLIP album whose
similarities sit around 0.05-0.15, i.e. a search that silently returns
nothing. Gating on `is None` makes both halves agree. Before this branch the
same payload re-resolved correctly, so this one is new.
**`name` was not patchable.** It was read straight out of the payload while
the docstring promised that any omitted key keeps its stored value, so
`{"key": "a", "min_search_score": 0.3}` raised a KeyError that surfaced as a
500 whose detail was the word "name".
**A missing album answered 404 or 500 depending on the payload.** A complete
one reached the failed write and got its 404; a partial one died building a
half-formed Album first. With patch semantics there is nothing to patch
either way, so the key is checked up front.
**The token cache was invalidated before the write, not after.** The cache
key is (url, username) and does not include the password, so any request
that read the album in between — an index scan, a board delete — logged in
with the old password and re-cached a token that outlived the change by up
to a day. That is precisely what the invalidation was added to prevent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…h list Two remaining findings from the adversarial review of this branch. **A stale score could be persisted back over a re-resolved one.** Changing an album's encoder family re-resolves min_search_score server-side, but the album manager never told state about it, so `state.minSearchScore` kept the old family's floor. The next nudge of any search setting then POSTed that floor back — 0.2 on a SigLIP album, whose similarities sit around 0.05-0.15, so search silently returns nothing. The stomp predates this branch; what is new is that it sticks. Every album edit used to re-resolve the score from the encoder, so a bad value was corrected on the next save; now that an update keeps the fields its payload carries, it survives. `saveAlbumChanges` reloads the active album's settings before anything can write them back. **A board album refused a path list it had just handed out.** The guard compared normalized lists, so a caller echoing the album's own directories in a different order got a 400, and so did one echoing a snapshot taken before board albums gained their `outputs/videos` directory — one path where the album now has two. Neither is asking for a change. It compares sets and accepts a subset now, which is the same tolerance the guard already extends to a caller whose snapshot straddles a root edit. A directory the album does not derive is still refused out loud. Three sibling suites mock state.js and had to learn the new export. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
enabled auto-merge (squash)
August 20, 2026 02:39
Windows-only CI failure, on both 3.10 and 3.14; Linux and macOS passed. `Album.validate_index_path` stores the posix form of the index, so the stored value is `C:/Users/...` while `str(tmp_path / ...)` in the test is `C:\Users\...`. Same file, different string. Comparing `Path` objects normalizes the separator on the platform where it matters and changes nothing anywhere else. The assertion predates the review fixes; it was hidden while the branch was failing on every platform for other reasons. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
added a commit
that referenced
this pull request
Aug 21, 2026
…g write The rebase onto master silently kept two `default_min_search_score` definitions: master's #372 introduced one in `config.py` at the same time this branch moved the logic to `encoders.py`. The module-local one shadowed the import, so every caller inside `config.py` still saw the old two-value function — new OpenCLIP albums were given 0.2 again, and the migration became a no-op that stamped configs as migrated without touching them. `config.py` now keeps only the import, and `routers/album.py` takes the function from `encoders` directly rather than through the re-export. Resolving that surfaces a real disagreement with #372 rather than a textual one: OpenAI CLIP -> OpenCLIP is a change of *band*, so a hand-tuned floor is now re-resolved across it. `test_a_hand_tuned_score_survives_a_change_ within_one_family` swapped two specs that no longer share a scale; it now swaps two OpenCLIP models, which is what it claims to test, and a new case pins the cross-band re-resolve. Also: * `atomic_write_text` creates its temp file 0600 and narrows an inherited one before writing, instead of chmod-ing after the payload is already on disk; a config written for the first time now lands 0600 rather than at the process umask. It holds an InvokeAI password. * The migration notice is printed once per album per process. The migration itself re-applies on every load until a save persists it, so it was repeating on a config that had not changed. * `_migrate_score_floors` no longer returns a value nobody reads, and its docstring no longer claims it can tell a machine-chosen 0.2 from one the user typed — nothing records that, and a user who typed 0.2 is moved off it too. The log line is what tells them. * Stale prose: the SigLIP docstring and `state.js` no longer describe one CLIP floor, `_version_tuple`'s docstring admits `Config.validate_version` rejects an unparseable stamp a few lines later anyway, and the docs quote the measured 0.24-0.30 for OpenAI CLIP rather than 0.24-0.35. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lstein
added a commit
that referenced
this pull request
Aug 21, 2026
* fix: score floors per encoder family, not one number for all CLIPs
Text search filters results below the album's ``min_search_score``, which
was resolved once at album creation as "0.005 for SigLIP, else 0.2". The
0.2 was calibrated for OpenAI CLIP, and it is wrong by roughly the width
of the match band for the encoder PhotoMapAI actually recommends.
Measured on the same 400 images with the same eight queries:
encoder mean top-1 median
openai-clip:ViT-B/32 0.267 +0.17
open-clip:ViT-L-14/dfn2b_s39b 0.151 -0.07
siglip (calibrated probability) 0.015 0.00
and again on a 38,000-image photo library indexed with the OpenCLIP
default, where a 0.2 floor returns *zero* results for five of eight
ordinary queries ("a dog", "a birthday cake", "a snowy mountain
landscape", "a plate of food", "a city street at night") and fewer than
five for two more. The matches are there; they score 0.15 to 0.26.
So the floor is now a per-backend table: 0.1 for OpenCLIP, 0.2 for
OpenAI CLIP, 0.005 for SigLIP. A single number cannot serve them — the
same 0.1 applied to OpenAI CLIP returns the entire album ranked, since
its *median* image scores 0.17 against an arbitrary query. On the 38k
library 0.1 yields 56 to 2953 hits per query, at most ~8% of the album
for a very broad one.
The floor is stored per album, so the default alone would only help
albums created after the upgrade. A one-time migration re-resolves
albums still carrying the machine-chosen 0.2 whose encoder now resolves
lower; a floor the user typed is left alone, and the migration is gated
on the config version so 0.2 stays a value they can choose afterwards.
Loading a config stays a read: the migration applies in memory and rides
along with the next save the user causes. Rewriting config.yaml on every
first start would drop its comments and any key this build does not
know, unprompted. ``atomic_write_text`` now also preserves the file's
mode and follows symlinks — it replaces rather than writes through, so a
config tightened to 0600 (it holds an InvokeAI password and a LocationIQ
key) was being widened to the umask by any album edit, and a symlinked
config was being replaced by a regular file.
Search resolves an omitted floor through the album first and the encoder
second, so an API client that omits it gets the user's tuned value
rather than a default.
Backend 680 passed, frontend 578 passed, ruff and prettier clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: resolve the score-floor table against #372, and harden the config write
The rebase onto master silently kept two `default_min_search_score`
definitions: master's #372 introduced one in `config.py` at the same time
this branch moved the logic to `encoders.py`. The module-local one shadowed
the import, so every caller inside `config.py` still saw the old two-value
function — new OpenCLIP albums were given 0.2 again, and the migration
became a no-op that stamped configs as migrated without touching them.
`config.py` now keeps only the import, and `routers/album.py` takes the
function from `encoders` directly rather than through the re-export.
Resolving that surfaces a real disagreement with #372 rather than a textual
one: OpenAI CLIP -> OpenCLIP is a change of *band*, so a hand-tuned floor
is now re-resolved across it. `test_a_hand_tuned_score_survives_a_change_
within_one_family` swapped two specs that no longer share a scale; it now
swaps two OpenCLIP models, which is what it claims to test, and a new case
pins the cross-band re-resolve.
Also:
* `atomic_write_text` creates its temp file 0600 and narrows an inherited
one before writing, instead of chmod-ing after the payload is already on
disk; a config written for the first time now lands 0600 rather than at
the process umask. It holds an InvokeAI password.
* The migration notice is printed once per album per process. The migration
itself re-applies on every load until a save persists it, so it was
repeating on a config that had not changed.
* `_migrate_score_floors` no longer returns a value nobody reads, and its
docstring no longer claims it can tell a machine-chosen 0.2 from one the
user typed — nothing records that, and a user who typed 0.2 is moved off
it too. The log line is what tells them.
* Stale prose: the SigLIP docstring and `state.js` no longer describe one
CLIP floor, `_version_tuple`'s docstring admits `Config.validate_version`
rejects an unparseable stamp a few lines later anyway, and the docs quote
the measured 0.24-0.30 for OpenAI CLIP rather than 0.24-0.35.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: the permission assertions are POSIX-only, and keep the text write path
Windows CI failed on three tests asserting 0o600. `os.chmod` there toggles
the read-only flag and nothing else, so a readable file always reports
0o666 and "private to its owner" is not a property Windows has —
`test_config_rewrites_keep_the_file_private` had been failing that way
since it was written. All three now skip on win32 rather than branching on
an expected mode; the production code runs the same chmod calls on every
platform, they simply have no user to exclude there.
Writing the payload through the raw descriptor was a hazard on the same
platform: `os.open` does not set O_BINARY, and a text-mode descriptor under
a text-mode wrapper translates each newline twice. The temp file is now
created empty and narrowed to 0600 through the descriptor, then written via
`Path.open` exactly as before — the payload still never exists at a mode
wider than the target's, and the bytes are unchanged from the original
implementation. The test follows the file's mode at open time rather than
spying on `os.fdopen`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Fixes #371.
POST /update_album/rebuilt the album from the request payload alone, so every field the payload left out was reset to its model default. Real callers send partial payloads — the bookmark menu sends five keys, the search dialog sends the album plus three overrides — and the worst thing that reset wassource_type: an InvokeAI-board album came back as a directory album with everyinvokeai_*field cleared. After that its indexing walked InvokeAI's output directories as ordinary folders, and deletions stopped routing through InvokeAI's API, leaving dangling rows in its database.Reproducing it took one bookmark: bookmark menu → Move to folder → pick a folder outside the album → "Yes, Add Folder".
What changed
An update is a patch. A key the payload does not carry keeps its stored value. Falsy values that real edits send —
min_image_bytes: 0,use_query_optimization: false, an emptied description — still win. Null is treated as absent, becausecreate_albumturns a None into the model default rather than into a cleared field; honoring it would swap an album's encoder for the host default and silently invalidate its index.invokeai_usernameis the one field that genuinely needs clearing (InvokeAI leaving multi-user mode), so it goes through a helper that keys on presence instead.Three things that needed handling beyond the plain rule:
image_paths=Noneso the model re-derives them. Carrying the stored list over would suppress that derivation and pin the album to its old root the moment the user edits the root — the re-index would write under the new root while the access gate still pointed at the old one, 404ing every image in the album.min_search_scoreis left to the model to re-resolve when the encoder family changes. The floors differ by an order of magnitude (0.005 for SigLIP, 0.2 for CLIP), so carrying one across makes the new encoder look like it returns nothing — but swapping one CLIP model for another must not discard a score the user tuned by hand. The model and the router now read that rule from onedefault_min_search_score.source_typethat disagrees with the stored album is refused. The edit form branches on the stored kind and never offers to switch, so a disagreement is a partial payload being read as a replacement.A board album's directories cannot be changed or added to, and saying so beats accepting the request and quietly deriving something else: a payload that changes them gets a 400, unless it merely echoes the album's paths — either the stored list or the one that same request derives, both of which callers legitimately send while a root edit is in flight. An
HTTPExceptionraised inside the handler is now re-raised rather than wrapped in a 500, so that 400 (and the 404 for an unknown key) survives as itself.Frontend: the bookmark menu no longer offers to add a destination folder to a board album, since the backend has to refuse it, and its update payload carries only the keys it is changing.
Clearing the InvokeAI password
The same "blank means I did not touch this" rule left the password unclearable: the edit form never sees the stored one, so there was no way to say forget it. An InvokeAI that leaves multi-user mode kept a credential in
config.yamlthat only a text editor could remove, andhas_invokeai_passwordkept reporting it to the form.invokeai_password: nullnow clears it, while""and an omitted key still keep what is stored. No existing caller can send that null by accident —_album_public_dictcarrieshas_invokeai_passwordand never the password itself, so the search-settings persister's round trip cannot reach it, and the bookmark menu sends three keys. The only source is a new Forget saved password checkbox in the edit form, offered only when there is one to forget and reset every time the form opens; a password typed in the same edit wins over it.That checkbox needed its own
[hidden]rule. It lives in a<label>inside a.form-group, and that selector'sdisplay: blockis an author rule, so it beats the UA stylesheet's[hidden] { display: none }— the box stayed on screen for albums with no stored password while the attribute reported otherwise. Same shape as the existing.video-player-*[hidden]rules.Clearing or changing a credential now also invalidates the cached JWT. The token is keyed on
(base_url, username), neither of which has to change when a password does, so a forgotten password would have kept working from cache until the token expired — up to a day later.Tests
Eleven new backend cases: the demotion itself, the refused folder and source-type changes, a root change moving the derived paths, an explicit null neither clearing a tuning field nor surviving as one, the username clear, the encoder-family re-resolve and the same-family preservation, a blank index, and an unresolvable path being refused rather than crashing. Plus the password work: the three-way backend distinction (absent / blank / null), and on the frontend the payload matrix including "typed wins over the box", the row's visibility under the real CSS rule — asserting computed style, since the attribute alone reported hidden while the box was visible — and the reset on re-open. Each was checked by reverting its production hunk and confirming it fails.
Board-album tests also stopped deleting from the developer's real user data directory:
default_board_index_pathresolves againstplatformdirs.user_data_dir, which no fixture isolated, so a test album keyed like a real one removed the real index on cleanup — the same gapisolate_video_frame_cachealready covers for the cache directory.Backend 681 passed, frontend 587 passed, ruff and prettier clean.
Notes
Found during the adversarial review of #369; the bug predates it and reproduces on
master. This branch is based onmasterand does not depend on #369.Three adversarial review rounds ran against this change and turned up fourteen defects in it, all fixed here — the board root-change breakage, the username that could no longer be cleared, the encoder-swap score reset, and the symlink-loop 500 among them.
A fourth round reviewed the password commit on top and found two more, both fixed: the Forget saved password checkbox was always visible (the CSS specificity problem above), and a cleared password kept working from the JWT cache.
🤖 Generated with Claude Code