1162 Commits

Include renames

Author	SHA1	Message	Date
Paul Nothaft	36192708ab	chore(main): release 3.115.1-beta.0 (#1190)	2026-08-26 06:34:49 +00:00
Paul NothaftandPaul Nothaft	06da1b9f7e	fix(external-media): one row per external file per event (#1162) (#1167) ... * fix(external-media): one row per external file per event (#1162) Two overlapping import-external runs against the same event inserted every file twice. The route checked for an existing external_relpath and then inserted, with an fs.stat and a sharp().metadata() read sitting in between — a window wide enough for both runs to see "not there". A reporter's event held 8004 rows for 6012 distinct paths. Nothing at the storage layer stopped it: migration 041 created only a NON-unique (event_id, source_origin) index. - migration 186 removes the duplicates that already exist and adds a partial unique index on (event_id, external_relpath). The survivor is the lowest id that has a thumbnail, so a half-finished import does not cost a grid tile, and hero references are repointed first because the FK is SET NULL. - the route treats a unique violation as a skip and carries on, so a second writer this process cannot see (another replica) converges instead of duplicating or 500ing. - a second import while one is already running now gets a 409 rather than walking the whole tree to have every insert bounce. The duplicates' thumbnail files are left behind as unreferenced bytes — a migration is the wrong place to reach into storage, which may be S3. * fix(external-media): keep dependent rows and legacy restores intact (#1162) External review found two real defects in the dedupe half of this fix. Dangling rows on SQLite. Every FK into photos declares ON DELETE CASCADE, but PicPeak never sets `PRAGMA foreign_keys = ON` — the codebase says so where it deletes an event (adminEvents/helpers.js:245) — so on every SQLite install the cascade is inert and deleting a duplicate photo left its face embeddings, guest feedback and admin marks behind, pointing at an id that no longer exists. Biometric data outliving its photo is exactly the invariant the event delete goes out of its way to hold. Dependents are now handled explicitly, and moved rather than discarded where they can be: the duplicates were separate tiles in the grid, so a guest's comment or an admin's rating could legitimately be on either, and dropping it inside a fix for silent data loss would be its own bug. Where the target already holds an equivalent row — the same guest's like, the same admin's mark, the same transfer's entry — the loser is dropped, because those tables mean one row per (photo, actor). photo_faces is the deliberate exception: both rows were scanned, so moving would duplicate every embedding and split the person clusters built from them. Legacy restores. Suspending FK enforcement does not suspend a UNIQUE index on either engine, so a .picpeak backup taken before migration 186 — carrying exactly the duplicates it removes — would hit the new index mid-batchInsert and roll the whole restore back, after every table had already been emptied. The restore now drops the index for the load and rebuilds it after running the same dedupe. Also: a failed CREATE INDEX is no longer swallowed. Recording the migration as applied without it leaves the install permanently racy, with nothing to trigger a retry. The shared work moves to services/externalPhotoDedupe.js, which the migration and the restore both call. * fix(external-media): reconcile derived state around the dedupe (#1162) Second review round, four more real findings. The index throw did not actually stop anything. run-migrations-safe.js treats 23505 as "schema already exists" and marks the migration applied (run-migrations-safe.js:138) — and a CREATE UNIQUE INDEX that finds duplicate rows raises exactly 23505 on Postgres. A replica inserting one between the dedupe and the index lock is a real rolling-deploy shape, and the outcome was the thing the throw was added to prevent. The index is now verified against the catalog afterwards, and failure raises a code-less error the runner cannot mistake for idempotence. Two people sharing a device were treated as one. photo_feedback carries both guest_identifier (per device) and guest_id (per person, migration 078), and feedbackService scopes by guest_id when present. Keying equivalence on the identifier alone deleted one of two different people's ratings. It now uses the same COALESCE rule the service does. Deleting faces raw left ghost people. event_people counts and centroids are derived from the photo_faces rows being removed, and #1132's separation snapshots hold a copy of each side's centroid — which is why faceProcessor exposes purgePhotoFaces and says it is "called from every photo-deletion path". The dedupe now goes through it. Reparenting feedback left the survivor's totals stale. photos carries denormalized feedback_count / like_count / average_rating / favorite_count and the later reaction and colour counts, so a survivor that now owns feedback kept rendering zero. updatePhotoFeedbackStats takes a trx so the dedupe can recompute on its own connection. Also: the equivalence-key delimiter was a literal NUL byte, which made git classify the whole file as binary and hide its diff. Escaped. * fix(external-media): stop the dedupe discarding half-states (#1162) Third review round. Five findings, four applied. - is_hidden joins the feedback equivalence key. feedbackService lets a moderator-hidden row coexist with the guest's visible replacement and counts only the visible one, so ignoring it deleted the visible row as redundant. - admin marks merge instead of dropping. rating and color_label are written independently, so the same admin can have rated one tile and coloured the other; the loser now hands over any field the winner has no value for. - a survivor that loses the only completed scan is requeued. Otherwise the purge takes the sole embeddings and nothing re-queues it — the photo just silently stops having a face. - view_count and download_count are carried over. Those are real interactions recorded per row, and dropping them quietly lowered the engagement the admin grid shows. Not applied: repointing a category hero can in principle land on a survivor in another category. It needs the two duplicate rows to have been re-categorised apart after the racing import, and the result is a cosmetic hero mismatch that the admin category routes already guard on write. Not worth the extra branch in a data migration. * fix(external-media): invalidate the download zip when duplicates are removed (#1162) External review of the stable twin. Applies to both branches. The pre-built "download everything" archive still contained the duplicate rows the dedupe had just deleted, so guests kept receiving them until something else happened to invalidate it. Every ordinary photo-deletion path calls downloadZipService.invalidate for exactly this reason. The columns are cleared rather than the service being called: that service carries debounce timers and a regeneration queue, which is not something a migration should start. getZipInfo already treats a cleared record as a cache miss and rebuilds on the next request, so this is the durable half of what invalidate does. The stale object is left in storage for the same reason the duplicates' thumbnails are — a migration is the wrong place to reach into a backend that may be S3. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-26 08:28:48 +02:00
Paul Nothaft	bb5d496495	chore(main): release 3.115.0-beta.0 (#1158)	2026-08-23 20:17:29 +00:00
Paul Nothaft	c305ad4146	feat(faces): make "not the same person" survive a re-scan (#1132) (#1145) ... A separation — an explicit dismissal, or the implicit one a Split records — was stored as a pair of event_people.id. Those ids do not survive re-derivation: recluster() deletes every person, and a full re-scan replaces a photo's faces outright, so face ids die too. The only thing that survives both is the embedding, so the decision is keyed on the two centroids the pair had when the photographer separated them. It binds while both sides still look like the clusters that were separated, and lapses once they have drifted past recognition. The constraint is now honoured at assignment time as well as in consolidate(), which is what makes it hold across a re-scan rather than being reformed before any later pass could object. Six review rounds shaped the matching itself: each candidate must resolve to the OPPOSITE side rather than merely matching something (a split leaves two similar halves, and the loose test fragmented the person the split was not even about); assignment judges both sides at the ordinary match threshold, since a single face — or a cluster of one part-way through a recluster — cannot resemble a settled centroid; separations carry their own model_version; and the projections are hoisted out of the innermost loop, which took a 2000-photo scan from ~15s of dot products to 0.23s. Lifecycle closed three ways: purgePhotoFaces re-anchors each side onto the live cluster it still describes and drops rows that describe nothing left, deleteEventCascade and the permanent archive delete clear the table (which deliberately has no event FK), and a later manual merge drops the separations it reverses. All of it matched on vectors rather than ids, since a row that has outlived a recluster names people who no longer exist. Merged with admin privileges: the author cannot self-approve.	2026-08-23 22:10:28 +02:00
Paul Nothaft	2c81888eaf	fix(gallery): a guest's own hidden feedback is hidden from them too (#1150) (#1153) ... Everything in the system treats a hidden row as absent — getPhotoFeedback drops it even for the guest's own feedback, and updatePhotoFeedbackStats does not count it. The per-viewer is_liked heart and my_color_label badge read the row without looking at is_hidden, so a like the photographer had hidden still showed as liked on a photo whose like_count was zero. Making those agree exposes why it had not been fixed: the duplicate check behind like/favorite toggling did not skip hidden rows either, so the now-empty heart, when clicked, found the hidden row and toggled it OFF — the click did nothing visible and the moderation was silently undone. Skipping hidden rows there makes the click create a fresh, visible row. Review found four more surfaces still treating a hidden row as present: the per-guest caps (an at-cap guest with one hidden met their own click with limit_reached), /my-feedback (which drives the Liked/Favorited/Rated chips in guest identity mode), getEventFeedbackSummary (disagreeing with the photo counters in the same response), and unhide (leaving two visible rows for one guest). The rating-clear and single-value delete scopes are visible-only now, so a follow-up mutation no longer destroys the admin's hidden record, and the unhide collapse is skipped when there is no stable identity to scope by — that fallback was 'guest_identifier IS NULL', i.e. other visitors' rows. Not taken: refusing to hide non-comment feedback, which the issue recommended. #839 and #1044 both ship hiding for reactions and colour labels with tests asserting a hidden one stops counting; only the admin UI's Hide button is comment-only. Merged with admin privileges: the author cannot self-approve.	2026-08-23 22:09:07 +02:00
Paul Nothaft	00b20b2d72	fix(gallery): guest filters respect show_feedback_to_guests, and marks survive a mid-write clear (#1147) ... Two follow-ups from the review of #1137. Filters were a second way to read hidden feedback. Every token on /photos is an OR of two halves: what THIS viewer marked, and what ANYONE marked. The response fields built from the second half — like_count, comment_count, color_label_count — are all gated on show_feedback_to_guests. The filter was not, so with the setting off a guest could still send ?filter=liked and get back exactly the photos other people liked, across all five tokens. Reachable by a direct API caller holding a gallery token; the frontend never sends filter to this endpoint. The half it left standing was also the wrong half. It read guest_identifier from the guest_id QUERY PARAMETER, which never matched anything — the frontend invents that string in localStorage and never sends it when submitting feedback, while submissions store generateGuestIdentifier(req). So gating the aggregate would have emptied these filters rather than narrowing them to 'mine', and accepting a caller-supplied identifier was a way back through the gate. Resolved from the request now, hidden rows excluded to match what the viewer can see. A mark whose row is cleared mid-write lost its value. #1137 fixed two calls both writing; this is one clearing while another sets. The clear empties the row, the row is deleted for being empty, and the setter's update matches nothing — the caller told 'no mark'. A zero-row update now reports itself and the caller re-reads, bounded at three passes, throwing rather than reporting a success that did not happen. Merged with admin privileges: the author cannot self-approve.	2026-08-23 22:07:50 +02:00
Paul Nothaft	e4a8be8e7e	fix(gallery): no Logout button on galleries that don't require a password (#1149) (#1152) ... showLogout was hard-coded true, so a gallery with no password showed a Logout button. Logging out of it is meaningless — no credential to drop, nothing to return to — and it stranded the visitor: GalleryPage's auto-login is a one-shot latch, so clearing the session left the page on its skeleton until a manual reload. That is the 'turns blank' in the report. The button is gated on requiresPassword || isClient || viaCustomer at both call sites. The full-page layouts render it on the callback being present rather than on a flag, so withholding the callback is how the gate reaches them. Session kind now comes from /auth/session rather than sessionStorage, which is per-tab while the cookie is per-browser: a gallery reopened in a second tab lost 'client' while the backend kept serving it as one. viaCustomer marks a portal token, which bypasses reveal mode and so is a credential that does not look like one. The public-gallery branch no longer returns the skeleton unconditionally — once auto-login has run and left us unauthenticated it shows the reason and a Retry. That state was otherwise unrecoverable, and it also swallowed loginError entirely. Merged with admin privileges: the author cannot self-approve.	2026-08-23 22:06:47 +02:00
Paul Nothaft	b581267031	fix(scripts): regenerate-thumbnails resolves external sources through ensureThumbnail (#1148) (#1151) ... The CLI fallback carried the defect #1129 fixed in the admin route: it computed `storage/events/active/<photo.path>` and fs.access'd it, a location that does not exist for external or reference rows. Every one failed the check and was counted as an error, so on an external-media install the script was inert while reporting one error per photo. Resolution now goes through ensureThumbnail, which already branches on source_origin and owns the per-photo ext<id>_ output name — sharing it is what stops the script and the route drifting apart again. Also: videos skipped on every marker they can carry (fileWatcher writes type and mime_type but never media_type), responsive tiers backfilled alongside the canonical rendition, skip-vs-generate asked from isThumbnailValid rather than inferred from an unchanged path, tier failures counted rather than swallowed, and a nonzero exit when the backfill was incomplete. The script is now importable with the CLI behind a require.main guard; it previously ran on require and called process.exit, so it could not be tested at all — which is why this survived #1129. Merged with admin privileges: the author cannot self-approve.	2026-08-23 22:06:11 +02:00
Paul Nothaft	66a9b5eba1	chore(main): release 3.114.0-beta.0 (#1146)	2026-08-23 09:20:30 +00:00
Luca	e2844d1909	feat(gallery): colour labels for client proofing, and one global default per feedback type (#1044) (#1137) ... Colour labels for client proofing, plus the photographer's own stars and colours in the admin grid. - Guest colour labels alongside likes/reactions, opt-in per event (defaults off so live galleries do not change mid-proofing), with 'colors' and 'lightroom' keybind schemes. - One global default per feedback type, replacing the per-type scatter. - Admin marks live in their own table (photo_admin_marks) so they can never reach a guest-facing surface. - XMP export prefers a real label, keeping the rating-derived mapping as a fallback. Review: concurrent-write loss on the mark update path, migration index idempotency and error classification all fixed in 7139bcae; migrations renumbered to 182/183 in 8fecdfae after 180/181 were taken on main. Merged with admin privileges: bypass-size-gate is a required check that fails on size alone for review-bypass authors and never re-evaluates on review, which is its designed behaviour once a maintainer has approved.	2026-08-23 11:15:01 +02:00
Paul Nothaft	7b77bbf243	chore(main): release 3.113.0-beta.0 (#1144)	2026-08-22 19:46:22 +00:00
Paul Nothaft	3583c924da	feat(faces): consolidate look-alike clusters after a scan, and suggest the rest (#1107) ... consolidate() has existed since #1074 and described this exact symptom in its own comment, but its only caller was recluster() — i.e. when an admin pressed Re-group people. After a normal background scan the centroids converged and nobody looked, so a gallery settled with 14 people that should have been 8. It now runs when a scan drains. There is no scan-finished event to hook, so an idle worker asks whether the events it touched have actually drained — 'a worker went idle' is deliberately not treated as sufficient, because with concurrency above one the others may still be working. The uncertain band asks instead of acting: pairs between the assignment threshold and the stricter auto-merge one surface as accept/dismiss suggestions, with sticky dismissals. Nothing merges silently — a pass that merged anything reports it and points at Split. Review rounds hardened it against overruling explicit decisions: it no longer absorbs ignored clusters (mergePeople ORs is_ignored onto the survivor, which would have hidden a real person), no longer merges dismissed pairs, no longer undoes a manual Split (which now records a separation), and no longer runs after detection is switched off. The dismissal read fails closed, a failed pass is retried with backoff rather than lost or hot-looped, and the new table follows event_people out of exports and backups. Name autocomplete needs no endpoint — the people list already open is the source, and it is event-scoped on purpose. Known limitation, tracked in #1132: separations are keyed on person ids, so a full re-scan loses them. Reported by @BraynArts.	2026-08-22 21:39:15 +02:00
Paul Nothaft	87115b28e8	fix(gallery): give masonry tiles their real shape back (#1130, #1131) ... Two independent causes of the same symptom — an aspect-ratio layout that does not lay anything out. gallery-premium discarded the tile height MasonryPhotoAlbum computed from photos.width/height and set height:auto on both card and image, so the rendered shape came from the intrinsic ratio of whatever rendition was served. With thumbnail_fit seeded 'cover' by migration 040 every rendition is square, so the layout drew identical squares and was indistinguishable from grid. The card now uses the height it is given and the stylesheet's existing height:100% applies. The bundled CSS templates pinned images to a fixed pixel height, which has specificity (0,1,1) and beats the .h-full utility (0,1,0) six of the seven layouts use. Elegant Dark is seeded is_default, so that was the out-of-the-box result for any layout other than grid/timeline. Migrations 052/053 corrected for fresh installs; 181 repairs the rows already seeded. Whitespace-tolerant because sanitizeCSS strips newlines from any template ever saved through the editor — an exact-text migration would have silently no-opped on most real installs. The height property is matched with a lookbehind so line-height/max-height/min-height are untouched, grouped selectors are handled, and nested rules are skipped rather than mis-rewritten. Both reported, measured in the live DOM, by @BraynArts.	2026-08-22 21:38:17 +02:00
Paul Nothaft	97d92f8428	fix(thumbnails): regenerate external photos instead of dropping their tiers (#1129) ... POST /admin/thumbnails/regenerate resolved every source as storage/events/active/<photo.path> and fs.access'd it. External and reference rows are not there, so every one failed and was counted as an error — and because the tier deletion runs first, the endpoint dropped every ?w= tier and rebuilt nothing, leaving the library worse than before it ran. The UI reported success either way. Now routed through ensureThumbnail, which resolves both source kinds, uses the per-photo ext<id>_ output name, and writes thumbnail_path back itself. Review rounds also removed both destructive deletes in generateThumbnail: the pre-delete ran before sharp opened the source, so an unreadable source left the previous rendition gone and the database pointing at it — across a bulk run, the whole gallery. Neither delete was needed, since put stages to a temp file and renames atomically and is the last statement in the try. Videos are filtered out, and the superseded rendition is removed only when the storage key actually moved, compared through the same canonicalisation the backends apply so a legacy backslash path is not mistaken for a different object. Reported by @BraynArts, who also identified the fix.	2026-08-22 21:37:49 +02:00
Paul Nothaft	f735d26422	fix(gallery): a missing thumbnail tier must not take the backend down (#1128) ... The first load of a gallery whose ?w= tiers do not exist yet could exit the Node process — not 500 one tile, kill the backend. Two defects stacked. The reader: LocalFsStorage.get() returns a lazy fs.createReadStream, so an ENOENT arrives after the await returned and outside the route's try/catch. An unhandled 'error' event is a process-level throw. pipeStreamToResponse attaches the handler the routes were missing — 404 for a vanished source, connection destroyed if bytes are already on the wire, file headers cleared so the JSON error is not served as image/jpeg or cached as a broken tile for an hour. Applied to all nine streaming responses in gallery.js. The writer: ensureThumbnailAtWidth passed regenerate:true, whose first act is to DELETE the target — on a path only reached when the tier is absent. A grid fires one request per tile, so one request unlinked the file another had just published and handed to a reader. Without the flag the write is an atomic rename. Generation is now also deduped per tier key: 8 concurrent requests ran 5 Sharp passes before, 1 after. Reported with a full diagnosis by @BraynArts.	2026-08-22 21:37:15 +02:00
Paul Nothaft	e243a88410	chore(main): release 3.112.0-beta.0 (#1139)	2026-08-22 16:41:36 +00:00
Luca	7223118b89	feat(deploy): make the all-in-one image installable without a shell (#1124) ... The all-in-one image could not be installed from a GUI at all — the deployment it exists for. validateEnv treats a missing JWT_SECRET as critical and exits, and the documented run command supplies it with `openssl rand`, a shell command a Synology Container Manager or QNAP Container Station form cannot run. wait-for-db.sh now generates one on first start and persists it next to the database, extending the existing /run/secrets hydration rather than adding a second mechanism. Explicit env still wins, then /run/secrets, then the generated file. The write is load-bearing: JWT_SECRET is exported only when the file actually persisted, because an unpersisted secret would mint a new one every restart and sign every session out. Creation writes to a private temp file and hard-links it into place — atomic, fails with EEXIST when another container won, and the loser adopts the winner's value. Non-regular paths are rejected before the link, since POSIX ln links INTO a directory rather than failing, which would make a mistyped -v target unrecoverable. Also repairs the onboarding paths a new install actually walks: the installer no longer rotates the secrets of a running install on re-run, deprecates the dead scripts/install.sh in place, corrects the CONTRIBUTING dev loop, and fixes the vite proxy target that had been pointing at a stray local port since 0da45e69. Reviewed over three rounds. Co-authored by @Luca-Timo.	2026-08-22 18:37:12 +02:00
Paul Nothaft	8f23118782	chore(main): release 3.111.1-beta.0 (#1138)	2026-08-22 16:29:22 +00:00
Paul Nothaft	2a43d95ff8	chore(main): release 3.111.0-beta.0 (#1123)	2026-08-21 17:41:19 +00:00
Paul Nothaft	ffc6bc1530	chore(main): release 3.110.0-beta.0 (#1122)	2026-08-21 17:34:11 +00:00
Paul Nothaft	bbce3cd2a2	feat(faces): let the photographer choose which photo represents a person (#1119) ... Phase 1 of #1096. Clustering picks the cover, and its idea of a good one and a human's do not always agree. A cluster whose avatar is turned away or softer than the rest stays that way in the guest-facing people strip too, and nothing in the UI could change it. A picker reachable from each person row, reusing the face list the split dialog already loads — same query, same grid, different action on a click. Making the choice actually stick took four changes --------------------------------------------------------------------------- event_people.cover_face_id has existed since migration 177 and the PATCH already accepted it, so the first version of this was frontend-only. It was also a no-op: - facePeopleService.listPeople SELECTED cover_face_id and then discarded it, recomputing the cover as the best-scoring VISIBLE face on every read. The picker saved, said so, and the avatar reverted immediately. It now prefers the stored pick whenever this audience can see it, and falls back to the score-ordered choice otherwise — so visibility scoping still wins, and a guest is never handed a crop of a photo they cannot open. - recomputeCentroid overwrote cover_face_id unconditionally. It runs on rescan and on photo replacement, so any reprocessing silently undid a deliberate choice. It now keeps the chosen face while it is still a member of the cluster. - The face list is cached per person, and split/merge move faces between people. Until now the only reader closed itself after acting, so nobody saw the stale copy; the picker is a second reader of the same key. - cover_face_id meant two things. assignFaces seeded it with whichever face opened the cluster and recomputeCentroid overwrote it with the highest scoring one, so an automatic guess was indistinguishable from a deliberate choice — and honouring it would have pinned every UNCURATED person to that guess, which is worse than the fallback it replaced (the fallback is computed per audience and skips photos a guest cannot open). Both writers are gone, migration 179 clears the stored guesses, and the column now means one thing. That also removes the need to defend the choice against rescans: nothing overwrites it, and a dangling id self-heals to the derived cover. Clearing existing values is safe rather than destructive: no install has ever been able to SET a cover, so every stored value is an automatic guess by construction. Also fixes a PostgreSQL-only 500 --------------------------------------------------------------------------- GET /admin/events/:id/people/:personId/faces joined `photos` but did not table-qualify its WHERE, and photo_faces and photos BOTH have an event_id: column reference "event_id" is ambiguous Postgres refuses it, so the endpoint 500s and the Split dialog — its only consumer until now — has been broken on every PostgreSQL install since the join was added. SQLite resolves the ambiguity silently, which is why the suite stayed green. Reproduced against a real Postgres before and after. The query is now a named builder the route calls and the test imports, rather than a copy: an earlier version of that test re-declared the query, so the route could regress to the bare form while the assertions kept passing. Merge and recluster preserve the choice as well. Both already carried labels and privacy flags across; the chosen cover is human state of the same kind, so it now rides along — through a merge when the target has none, and through a recluster by following its FACE into whichever cluster ends up holding it, rather than the majority-descendant rule the label uses. The picker and the endpoint disagree past 500 faces, so the picker now says when it is showing a capped list rather than presenting it as exhaustive. Frontend suite 178 passing, backend 23 across the touched suites, build clean, no new type errors. Mutation-checked twice: dropping the cover preference fails the new listPeople test while the visibility-scoping test still passes, and restoring the auto-seed in assignFaces fails it too.	2026-08-21 19:26:52 +02:00
Paul NothaftandPaul Nothaft	887bdbe6e5	feat(gallery): responsive grid thumbnails (#1095) (#1109) ... * feat(gallery): responsive grid thumbnails (#1095) The half of #1095 that #1099 deliberately left out. Grid tiles are ~175 CSS px at the mobile 2-column default — about 530 device px on a DPR-3 phone — so the 300px thumbnail is upscaled ~1.8x and faces visibly mush. Backend mirrors the preview tiers exactly: ?w= on the gallery thumbnail route, whitelisted to 300/600/900, cached by width in storage, never written to photos.thumbnail_path, and keyed by photo id for every source type — basenames are not unique across events and a tier is served from a cache hit without re-reading the source, which is how the preview tiers nearly leaked one gallery's photo into another. The tier is in the ETag, or a client holding the 300px file gets a 304 for its 600px request. Cleanup and regenerate invalidation are wired the same way. generateThumbnail now takes width/height overrides; it keeps the configured `fit`, because the grid renders with object-cover and tiers that were framed differently would visibly jump as the viewport changes. The srcset only advertises tiers the SOURCE can fill. Thumbnails are generated withoutEnlargement, so a 400px original asked for 900 comes back at 400 — advertising "900w" would have the browser pick that candidate and upscale it, which is the reported softness made worse. That exact trap is why this was held back from #1099; the photo's own dimensions are now the guard, measured on the SHORT edge because thumbnails are square and a 4000x600 panorama can still only fill a 600 tile. A source that clears only one tier gets no srcset at all rather than a single pointless candidate. Two things this surfaced, both worth knowing separately: `npx tsc --noEmit` type-checks NOTHING in this project — the root tsconfig is `files: []` with project references, so the real command is `tsc -b`, which is what build:check runs. Under tsc -b the repo has 43 files with pre-existing type errors; this branch adds none, and the one error in a file I touched (PeopleManagerModal:91) is on main already and unrelated to the line I changed. * fix(gallery): wire grid tiers into the component that actually renders The srcSet landed in PhotoGrid.tsx, which nothing imports — GalleryView renders PhotoGridWithLayouts, and every grid layout funnels its tile through the shared PhotoCard. The frontend half of #1095 shipped nothing. Moved to PhotoCard, and switched from srcSet to a single sized URL, the same shape PhotoLightbox already uses for preview tiers. AuthenticatedImage fetches its src with the gallery bearer token and renders the blob; an <img> carrying a w-descriptor srcSet ignores src entirely, so that fetch would have been discarded and the browser would have issued its own — unauthenticated, and resolved against the page origin rather than the configured API host. One URL keeps the auth path and halves the requests. The tier comes from the tile's measured width via the IntersectionObserver entry, read on the same render that reveals the image so nothing is fetched twice. Column counts differ per layout and shift again with thumbnailScale, so the breakpoint table is only a fallback. Also closes what the tier cache leaked or served stale: - ensureThumbnailAtWidth short-circuits videos. Their thumbnail is a poster frame, so the tier path handed the video file to Sharp — after downloading it in full on S3, uncached, once per request. - The ETag names the tier actually served, not the one requested. A fallback to the canonical thumbnail was caching a 300px image under a 900px key. - Tier height scales from the configured aspect ratio instead of forcing a square; with fit:'cover' a 300x200 canonical and a 600x600 tier are two different crops and the photo reframed between tiers. - The canonical short-circuit compares against the configured thumbnail_width, not the 300 default, so a 600px install stops generating duplicate tiers. - Tier invalidation on /admin/thumbnails/regenerate, above the local-file check that skips S3 and external rows. - Tier cleanup in replacePhoto and deleteEventCascade. Both derive keys from the photo row, so the rows have to be read before they change or vanish. Preview tiers had the same two holes and are swept alongside. The clamp no longer drops a tier when the source falls between them: a 400px short edge asked for 600 returns all 400 pixels, where clamping to 300 threw 100 of them away. Backend 18 tier tests, frontend 22. Full suites green: 293 backend across the touched areas, 185 frontend, build clean, no new type errors. * fix(gallery): measure the tile, and stop regenerating the w300 tier Follow-up to the review of #1095. Closes the three items left open there, plus a defect the previous commit introduced. **The w300 tier regenerated on every request.** Decoupling the canonical short-circuit from the hardcoded 300 left generateThumbnail still tagging against DEFAULT_THUMBNAIL_WIDTH. On an install with thumbnail_width=600 a w=300 request wrote `thumb_<name>` while the caller probed for `thumb_w300_<name>`: the cache never hit, so every request re-downloaded the original and ran Sharp, and the file it left behind was in no cleanup list. The tag now follows the configured width, and thumbnailTierKeys lists all three widths — which one is canonical is a setting, so excluding 300 stranded exactly the file a 600-configured install generates. **The tier is chosen from the tile's measured width.** The observer entry only exists for `lazy` cards, and Mosaic, Masonry and Timeline don't pass it — Mosaic is 1-up on mobile where Grid is 2-up, so they are the layouts a breakpoint guess gets most wrong. Measured in a layout effect and gated: the image is not rendered until the width is known, so AuthenticatedImage never mounts with a src it has to replace. Attaching the observer ref unconditionally instead refetches every tile, since React flushes passive effects before the sync re-render a layout effect triggers — removing the gate makes the new single-request test fail, which is how that was confirmed rather than assumed. **Gallery Premium has its own card** and never reached the shared one, so its tiles kept pulling the canonical thumbnail. MasonryPhotoAlbum already hands the laid-out width to the render prop, so it needed no measurement. **Event rename orphaned tiers.** The key embeds the basename, so the DB update is the point past which the old keys cannot be derived. Dropped inside the filename-changed branch, not the loop body: unconditional would fire four storage deletes per photo on every rename, 20k calls against S3 for a 5,000-photo event that merely had its slug adjusted. Preview tiers had the same hole and are swept alongside. Carousel is the seventh layout and deliberately gets no tiering: its filmstrip thumbs are 80 CSS px, under the canonical 300 even at DPR 3. Tests: first PhotoCard suite (6), backend tier suite 21. Both new behaviours mutation-checked — reverting the width tag, the render gate, the measurement, or the rename sweep each fails a test. Full suites green: 298 backend across the touched areas, 191 frontend, build clean, no new type or lint findings. * fix(gallery): mount masonry cards once, into a measured layout Found while capturing screenshots for this PR, by attributing every thumbnail request to a photo id rather than eyeballing the grid. Masonry columns mode starts at 3 columns and runs its greedy distribution off a hardcoded 300px estimate until the container has been measured. Cards mounted into that guess are torn down when it settles — photos move to a different parent column, so React unmounts them — and since #1095 each mount picks its tier from its own width, the two mounts request two DIFFERENT urls. Measured on a 1440px desktop, production build, 62 photos: before 45 photos fetched at canonical AND w600, 17 stuck on w600 107 requests after 62 photos, canonical only, 62 requests Mobile was already landing on one tier either way, so both mounts produced the same url and the second was a cache hit — which is why it looked clean and the desktop case did not. The fix is the gate the rows/justified mode in this same file already applies for the same reason (line 346): hold the cards back until containerWidth is known. Only columns mode was missing it. Grid and Justified take their column counts from CSS breakpoints, so they have no transient measured value to discard and are unaffected. Worth noting this was NOT visible on main: without tiering both mounts request the same url, so the browser cache absorbs the duplicate. Tiering is what turns a harmless remount into a second download — the regression is this PR's, which is why it is fixed here rather than deferred. Frontend suite 194 passed (3 new). Mutation-checked: removing the gate fails the mount-once and placeholder tests. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-21 19:26:21 +02:00
Paul Nothaft	00776234fd	fix(security): let cors() own Access-Control-Allow-Origin on protected images (#1118) ... Closes #1116. secureImageMiddleware set its own Access-Control-Allow-Origin, overwriting the one cors(corsOptions) had already computed. server.js:247 mounts cors() on all of /api with credentials:true, so by the time the route handler ran the correct header was already there — and the local assignment replaced it with a worse answer in BOTH directions: unresolved -> '*'. Combined with the credentials:true that cors() sets, that is an invalid pair browsers reject outright. Unreachable on Docker until #1104 stopped compose injecting FRONTEND_URL; reachable on a fresh install from then until the wizard stores general_site_url. resolved -> the frontend origin, even when the request legitimately came from the allowlisted ADMIN_URL. A split admin host got a header naming the wrong origin and the browser rejected a request cors() had allowed. Deleting the line fixes both. cors() already validates the request Origin against the allowlist, sets Vary: Origin, omits the header entirely for a disallowed or absent Origin, and pairs correctly with credentials. Methods, Headers and Max-Age stay here: they are route-specific and cors() does not contradict them. Observed against a running instance before and after: allowlisted Origin ACAO: <that origin> + Vary: Origin + credentials:true disallowed Origin no ACAO no Origin header no ACAO Six tests, mounted on a real Express app with server.js's middleware order. Deliberately NOT a unit test against a response double: the first version of this fix was a guarded assignment that looked correct in isolation and still overwrote cors() whenever an origin resolved. A double cannot see middleware composition, which is exactly how that slipped through. Mutation-checked both ways — restoring the original `|| '*'` fails 5 of 6, and restoring the guarded assignment fails 3 of 6 including the admin-origin case.	2026-08-21 19:25:56 +02:00
Paul Nothaft	ba221ba2a0	chore(main): release 3.109.0-beta.0 (#1115)	2026-08-21 10:58:27 +00:00
LucaandPaul Nothaft	9431b9f094	feat(setup): configure the public address and SMTP in the wizard, not .env (#1104) ... * feat(setup): configure the public address and SMTP in the wizard, not .env A fresh install could not configure its own public address. `general_site_url` and the `email_configs` row already existed as admin settings, but nothing could reach them: - docker-compose.yml injected FRONTEND_URL=${FRONTEND_URL:-http://localhost:3000} and Dockerfile.aio baked in ENV FRONTEND_URL=http://localhost:3000, so getFrontendBaseUrl() returned on its first branch every time and the setting was never read. .env.example shipped the same value as an uncommented placeholder for FRONTEND_URL / ADMIN_URL / API_URL. - the wizard never asked for the address at all, and skipped its whole config step unless a CRM-ish feature was selected — so a gallery-only install was also never offered SMTP, despite gallery links, guest invites and expiry warnings all going out through email_configs. - eleven call sites read process.env.FRONTEND_URL directly rather than the resolver, three of them defaulting to placeholder hosts that reached real recipients: https://app.example.com in payment-reminder emails, localhost:3005 in admin invitation emails, https://app.example.com in dev template previews. Stop injecting a default anywhere, and resolve the origin instead: FRONTEND_URL -> general_site_url -> the origin the request arrived on -> whichever exists -> ''. A loopback candidate is treated as unconfigured so the installs that already have http://localhost:3000 baked into their environment self-heal; the same guard previously lived inline in routes/gallery.js for the slideshow QR (#848) and is now shared. The empty return is preserved because shareLinkService and the SSO redirects in routes/auth rely on it to emit relative urls — callers needing an absolute url use getAbsoluteFrontendUrl(), which still ends at http://localhost:3000. The wizard now persists window.location.origin right after the admin account is created, so an install that skips the rest still has a usable origin for background jobs that have no request to derive one from, and offers it as an editable "Public address" field. Settings -> General shows the field read-only when FRONTEND_URL pins it, instead of silently ignoring edits. Also drop the `|| 'mailhog'` fallback when seeding email_configs: that host only exists in the dev compose profile (which does not even start by default), so a fresh install came up with a live config pointing nowhere while the wizard showed empty SMTP fields. With no row, blank fields are the truth and emailProcessor logs "No email configuration found". Developers set SMTP_HOST=mailhog explicitly. backend/src/services/emailService.js is deleted: nothing in backend/ references it, and it was the only consumer of the SMTP_* variables, which misrepresented how mail is configured. Refs #705 * fix(setup): keep FRONTEND_URL ahead of ADMIN_URL/APP_URL when resolving links The previous commit routed two call sites through the resolver but put the site-specific variable FIRST, silently reversing precedence: userManagementService was: FRONTEND_URL || ADMIN_URL || localhost:3005 became: ADMIN_URL || resolver adminEvents/crud was: FRONTEND_URL || APP_URL || '' became: APP_URL || resolver An install with both variables set would have flipped which one won. Call the resolver first instead — it starts with FRONTEND_URL, so the original relative order is preserved and only the final fallback changes: localhost:3005 (not even the frontend's port) and '' (a relative link inside an email) both become the resolved origin. Refs #705 * fix(setup): unpin loopback FRONTEND_URL, keep ADMIN_URL/APP_URL reachable Review feedback on #1104. isEnvPinned() reported ANY FRONTEND_URL as authoritative, including the loopback values getFrontendBaseUrl() deliberately demotes. An install upgrading with the old compose default FRONTEND_URL=http://localhost:3000 therefore resolved its origin from general_site_url correctly, but got the Site URL field rendered read-only in Settings and skipped by the wizard's seeding - locking the exact operators this change exists to unblock out of configuring a public address anywhere. The predicate now mirrors the resolver, and the derived general_site_url_effective the General tab reads comes from the same helper instead of re-normalising process.env inline. APP_URL and ADMIN_URL had become dead code: getFrontendBaseUrl() only returns falsy when NOTHING is configured, so `|| process.env.ADMIN_URL` after it never ran once a site URL existed - which after this PR is the normal case. A split-origin install pointing ADMIN_URL at a separate admin host got invite links on the public gallery origin instead. They are now passed as an explicit `override` that resolves directly below FRONTEND_URL, preserving the historic FRONTEND_URL-before-ADMIN_URL order while beating the database- and request-derived fallbacks. general_site_url now feeds the CORS allowlist and the Access-Control-Allow-Origin header, not just email links, so a schemeless value is an allowlist entry no browser origin can match. Validate it server-side in PUT /general (isURL with require_protocol, require_tld off so LAN/NAS installs on http://nas:3000 still work) and client-side in both surfaces that write it - type="url" never fires in either, since neither input sits inside a form. Two more wizard fixes: the General tab no longer reposts general_site_url while it is env-pinned, because the field then holds the effective env value rather than the stored one and the round-trip read as a change to a protected key, 403ing a settings.edit-without-settings.domains admin on an unrelated save. And SetupConfigStep validates the From address before posting - /admin/email/config rejects a blank one, which used to surface as a generic warning while the wizard advanced from its finally block anyway, discarding every SMTP value the user had typed, password included. A failed save now keeps them on the step. * fix(setup): surface a rejected public address instead of swallowing it Review round 2 follow-up on #1104, pushed onto the branch. saveSiteUrl() caught and discarded every error. That was defensible before round 2 added a server-side URL check, but PUT /general can now answer 400 — and the two validators disagreed: http://my_nas.local client: accepted server: rejected http://foo_bar:3000 client: accepted server: rejected validate() let those through, the 400 was swallowed, `failed` stayed false and onDone() ran. The operator finished the wizard believing the public address was stored when nothing had been. That is the silent misconfiguration this whole change exists to remove, landing on the LAN and NAS installs it targets. Three parts: - saveSiteUrl() throws. finish() resolves it before anything else is posted and puts the message on the address field rather than the generic "some settings could not be saved" warning. Skip for now still always leaves, by contract, but warns instead of dropping the value in silence. - allow_underscores on the server check, for the same reason require_tld is off: browsers resolve http://my_nas.local and the client accepts it, so rejecting it server-side only produced the mismatch above. Both validators now agree across the LAN/NAS, IDN, bare-IP and scheme-less cases. - LOOPBACK_BASE_RE anchors its host token. Bare prefix matching also demoted https://localhost-nas.example.com, and now that this predicate gates the whole resolver rather than just the slideshow QR, being demoted means a configured address is silently ignored. 127. stays a bare prefix on purpose: all of 127.0.0.0/8 is loopback. Resolver suite 31 passing, up from 26. Mutation-checked: restoring the unanchored regex fails the three new host-boundary cases. * fix(settings): don't lock the General tab on a site URL nobody typed Review follow-up on #1104, pushed onto the branch. general_site_url was free-text until this PR added a server-side check, so an upgraded install can hold something schemeless that predates it. The tab flagged that on load, and `disabled={!!siteUrlError}` then killed Save for EVERY General setting. An admin holding settings.edit but not settings.domains could not clear it either: correcting the address is a change to a protected key and 403s. The tab has no permission gating, so that role was simply locked out of the tab with no self-service way back. That is the same role adminSettings.js:85-95 documents the no-op round-trip allowance for. The allowance only helps if the request is made, and this blocked it in the browser first. Validation now waits until the field is actually edited, and an unchanged value is dropped from the payload rather than reposted — matching what the env-pinned case already does one line above, and for the same reason. stored value invalid, untouched Save works, key not sent edited to something unusable Save blocked edited to a usable absolute url saved Four tests, first coverage for this feature. Mutation-checked: removing the dirty gate fails the untouched-value case. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-21 12:54:14 +02:00
Paul Nothaft	7583b4f6b0	chore(main): release 3.108.1-beta.0 (#1103)	2026-08-20 12:22:34 +00:00
Paul NothaftandPaul Nothaft	b3a7ab27ea	fix(faces): face avatars were cropped against a cropped rendition (#1100) ... * fix(faces): face avatars were cropped against a cropped rendition Found while triaging #1096, which reported the People manager showing unusable cluster covers — a bare shoulder, the back of a head, a patch of background — and asked for more sample faces to compensate. Most of that is not a detector problem and not a UI limitation. It is a bug. faceCropStyle positions an avatar by scaling the WHOLE frame and offsetting so the face lands centre. That holds only while the rendition shown is the entire image at a uniform scale. Thumbnails are not: imageProcessor.js:93 DEFAULT_THUMBNAIL_FIT = 'inside' migration 040:6 thumbnail_fit seeded to 'cover' imageProcessor.js:229 fit: settings.fit The 'inside' constant is only a fallback for a missing settings row, and the row is seeded on every install — so thumbnails are centre-cropped essentially everywhere, and every face avatar rendered against one is silently offset on any non-square photo. The reporter read the setting as safe because of that constant, and the code comment at :87-92 says the same thing; all three places disagree with what is actually stored. It presents as a bad detector, which is why it survived: the boxes are right, the frame they are drawn against is not. All three surfaces — the admin manager and the guest-facing strip and sheet — now read a preview, which uses fit: 'inside' and is therefore the whole frame. At w=640: plenty for a 64px avatar at DPR 3, and small enough that a strip of a dozen people does not pull a dozen 1920px renditions. Face scanning already calls ensurePreviewImage for anything it scans, so a preview exists for every photo that has a face. Adds the admin preview route the manager needed; the gallery already had one. Both whitelist ?w= the same way. The first version of the call-site test passed with every surface still reading thumbnail_url, because an import alone satisfied it. It now matches inside the src={...} expression, and each of the three surfaces was individually reverted to confirm the test fails. * fix(faces): size the face tier by bbox, and keep admin_preview auth The face half of the external review; the tier-key and long-edge fixes live on the #1099 branch this is stacked on. Face avatars used one fixed 640 tier. In a 6000px group shot a 200px face is ~21px there, and faceCropStyle then blows that up ~9x to fill a 64px avatar at DPR 3 — mush, and indistinguishable from the mis-positioning bug this PR exists to fix. The tier is now derived from the bbox's share of the frame, so a face across a hall gets 1920 and a close-up still gets 640. The synthesized face URL also dropped admin_preview. verifyGalleryAccess only accepts the admin cookie when admin_preview=1 is on the request (middleware/gallery.js:28), and the preview flow deliberately mints no gallery JWT — so every avatar 401'd in exactly the mode an admin uses to check a gallery before sending it to a client. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-20 14:17:25 +02:00
Paul Nothaft	56034af0f0	chore(main): release 3.108.0-beta.0 (#1102)	2026-08-20 12:14:09 +00:00
Paul NothaftandPaul Nothaft	011f6ae7ec	feat(gallery): sized preview tiers so phones stop pulling 1920px (#1095) (#1099) ... * feat(gallery): sized preview tiers so phones stop pulling 1920px (#1095) A phone can display ~1170px at most, but the preview tier is a single 1920px JPEG with no size parameter — so every lightbox swipe ships roughly twice the bytes it can use, and the slide track preloads neighbours, which multiplies it. On the reporter's all-external install a null preview_url falls back to the untouched NAS original, which makes it worse again. Backend: ?w= on the gallery preview route, whitelisted to 640/1280/1920. A whitelist rather than a free-form width because every distinct value is a permanent rendition on disk — an open parameter is an invitation to fill the volume. Unrecognised or absent values fall through to the canonical 1920 preview, so old clients and hand-typed URLs behave exactly as today. Extra tiers are cache, not state: ensurePreviewImageAtWidth keys them by width, looks them up in storage and generates on miss, and never writes photos.preview_path. That column owns the canonical rendition, and threading a width through it would mean the last size anyone requested silently becomes "the" preview. Requesting 1920 resolves to the existing preview rather than a w1920 duplicate, so no install grows a second copy of every preview it already has. The tier is part of the ETag. Without it a client holding the 1920 rendition gets a 304 for its 640 request and renders the wrong size, which is this feature inverted. Frontend: the lightbox picks a tier from innerWidth x devicePixelRatio, capped at DPR 3 — uncapped, a DPR-10 device asks for 3900px and lands straight back on the desktop rendition. At the top tier the URL is left byte-identical so existing caches and ETags stay valid and desktop sees no change at all. saveData and a 2g/3g effectiveType drop one tier; both are Chromium-only, so they are a bonus rather than the mechanism. Grid thumbnails are NOT tiered here, deliberately. generateThumbnail resolves its width from admin settings rather than an argument, so tiering it is a separate change — and shipping a srcset whose candidates the server ignores would be worse than shipping none: the browser would take the "600w" candidate, receive the 300px image and upscale it, which is the reported softness made slightly worse. That half of #1095 lands separately. * fix(gallery): scope tier keys per photo, size by long edge, clean up tiers External review. Three findings against the tier work, one a cross-gallery leak. The tier cache key was the photo's BASENAME. Managed uploads keep camera basenames, so two events can each hold an IMG_0001.jpg — and a tier is served straight from a cache hit without re-reading the source, so the second gallery gets the first gallery's photo. Keys are now scoped by photo id for every source type. The RAW branch passed proc.outputBasename, which would have dropped that scoping again; it now passes the scoped name. Tier selection used viewport WIDTH, but ?w= bounds the LONG edge (fit:'inside'). On a 390x844 phone at DPR 3 a 2:3 portrait is bound by height and renders ~1755 device px, so width-only picked 1280 and made portraits softer than today; landscape on the same phone needs ~1170. It now computes the rendered long edge from the photo's own dimensions and falls back to the top tier — today's behaviour — when they are unknown. Tiers live outside photos.preview_path, so nothing else knew they existed: delete, bulk-delete and archive left them orphaned in previews/ forever, and regenerate-previews refreshed only the canonical rendition while phones kept the stale copy. previewTierKeys derives them from the same deterministic scheme and all four paths clean up. Deliberately outside the preview_path guard — a tier can exist when the canonical rendition never did, so keying cleanup off preview_path would strand precisely the photos only ever viewed on a phone. The existing tier tests encoded the old width-only semantics and were updated rather than kept; that is a behaviour change, not a test fix. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-20 14:08:07 +02:00
Paul NothaftandPaul Nothaft	0b886ed942	fix(faces): defer on unreachable storage, and commit the import path first (#1097) ... * fix(faces): defer on unreachable storage, and commit the import path first The two follow-ups left open by #1091, both consequences of external photos becoming scannable at all. **A dropped mount no longer burns the gallery.** ensurePreviewImage returns null for "this JPEG is corrupt" and "the NFS share is gone" alike, and faceProcessor marked both 'failed'. Nothing re-queues a failure automatically and the queue only ever claims 'pending', so a mount that blinked mid-scan cost the whole event a manual Re-scan — on external libraries, where network storage drops far more often than local disk, that is the common case rather than the corner one. faceProcessor now probes the containing DIRECTORY before failing, and throws TransientSourceError when it cannot be reached; faceQueue treats that exactly like SidecarUnavailableError — release to pending, back off, retry — with the warning rate-limited to one per five minutes, since an outage hits every photo in the event. The directory rather than the file is the whole point: a missing file inside a healthy directory is a broken photo and should still fail, and it still does. Anything that goes wrong deciding which case it is falls through to 'failed', because guessing 'transient' on an unknown condition would retry forever. **The import commits its path before inserting rows.** enqueueEvent accepts processing_status NULL (faceProcessor.js:243-246), which these inserts leave unset, so an admin hitting the toggle or Re-scan during a long import could queue partial rows while the event still resolved against the old directory — and burn them to 'failed'. Moving events.external_path ahead of the loop closes that, and fixes a pre-existing bug on the same line: an import that died at photo 500 of 1000 used to leave those 500 rows pointing into the new tree while the event still resolved against the old one, making every one of them unreadable. Safe to do first because the path is already validated above, and existing photos are unaffected — photo.source_origin takes precedence over event.source_mode in both resolvers and is NOT NULL defaulting to 'managed'. The #1090 test that asserted a missing source fails needed its setup corrected rather than its intent: it created no directory at all, which is now (correctly) a dropped mount. It now creates one, so it tests the case it always meant to — healthy storage, dead photo. Verified both fixes discriminate: removing the probe fails the defer test, and moving the event update back after the loop fails the ordering test. Face suite 59 passed across 8 suites; the full backend suite fails the same 10 pre-existing suites as unmodified main, no more. * fix(faces): stop a dead mount stalling the whole queue External review, and the first finding is one my own change created. Deferring by returning the row to 'pending' was a trap: claimNextPhoto orders by id ascending and the queue defaults to a single worker, so the same unreachable row becomes the oldest pending one after every backoff and the worker never reaches a higher id. One dead mount would have stalled face scanning for the entire install — unrelated events, fresh uploads, everything. Strictly worse than the permanent 'failed' this set out to replace. The row is now left parked in 'processing' with face_started_at intact. It is not claimable, so the worker moves straight on; the janitor that already exists returns it to 'pending' past STUCK_TIMEOUT_MS, which is the retry. No new column and no new timer. The sidecar branch still releases, because a down sidecar blocks every photo anyway — there is no other work to get on with. Second: probing existence was not enough. Unmounting an NFS or SMB share usually leaves the mountpoint behind as an ordinary empty directory, so fs.access succeeded on storage that was entirely gone and the photo was failed anyway — the exact case this was written for. An empty directory where the photo should live now counts as unreachable. The trade is deliberate and documented: a directory an admin genuinely emptied is retried rather than failed, which now costs one attempt per janitor sweep and nothing else. Third: a comment in adminExternalMedia claimed source_origin isolates existing photos from the early external_path update. That is true of managed rows and false of external ones — resolveExternalPath prefixes every external row with event.external_path, so importing folder B into an event referencing folder A rebases the A rows. Pre-existing rather than introduced here (the update always did this, just later), but the comment asserted otherwise, so it now says what actually happens and names the underlying single-base-path limitation. The deferral test initially passed against the blocking version too — database state alone cannot tell the fix from the bug. It now inspects the branch directly, the way the #596 contract tests do, and fails when releaseToPending is put back or the two branches are merged. * fix(faces): back off per event, and stop clobbering concurrent scans Round two of external review. Parking a row in 'processing' fixed the head-of-line block but not the cost: every janitor sweep handed the whole dead gallery back, and the worker walked all of it again — one stat per photo against storage that may be hard-mounted and slow to time out — before reaching any healthy event. Every one of those attempts also went through generatePreviewImage first, which logs an error per photo, so a down mount produced a recurring flood that the rate-limited warning did nothing about. So the backoff is now per EVENT and separate from the janitor: TransientSourceError carries the event id, the queue records a cooldown, and claimNextPhoto excludes those events while it lasts. The janitor keeps doing its own job, which is rescuing rows a crashed worker abandoned. Cooldown is in memory on purpose — a restart is usually what follows fixing a mount, so it should retry at once. Second: committing the event path before the loop means a toggle or Re-scan firing mid-import can now genuinely queue and finish some of those rows. The final bulk update was unconditional, so it dragged 'done' rows back to 'pending' for a duplicate sidecar scan and knocked 'processing' rows out from under the worker. It is now whereNull — only rows nothing has touched are ours to queue. Also corrected a comment of mine that had gone stale in the same file: it still described the enqueue as happening after the event path was written "below", which stopped being true when that update moved above the loop. * fix(faces): judge the mount, the path and the file separately Round three of external review. The probe was too coarse in both directions. It read any ENOENT on the photo's own directory as a mount-wide outage, so a deleted or renamed subfolder — individual/ gone while collages/ is healthy — deferred the entire event and starved every sibling folder, renewing the cooldown on each retry. It now judges the EVENT ROOT for that verdict: root missing, or present-but-empty, is an outage; anything below a populated root is a broken path and fails. And it read a listable directory as proof the photo was at fault, so EACCES on a reconnected share, EIO, or the classic NFS ESTALE handle were burnt as permanent failures. Only ENOENT now means genuinely gone; any other error opening the file defers. The event-wide backoff was also too broad. A reference event can hold managed uploads alongside imported external ones, and those live in local storage that is fine — excluding the whole event id left them unscanned for as long as external rows kept renewing the cooldown, which during a real outage is indefinitely. The exclusion is now scoped to external and reference rows. One of my own tests had modelled the unmount wrongly: it emptied the photo's subdirectory rather than the event root, which under the corrected logic is a populated mount with a missing folder — a failure, not an outage. It now empties the root, which is what an unmount actually leaves behind. Dropped the path require the first version of this probe needed; the event-root form does not. All three fixes mutation-checked: reverting each one fails the test written for it. Face suite 69 passed across 9 suites; full backend suite fails the same 10 pre-existing suites as main, no more. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-20 14:06:21 +02:00
Paul Nothaft	b7f04f6992	chore(main): release 3.107.5-beta.0 (#1101)	2026-08-20 11:18:12 +00:00
Paul Nothaft	731df9dc6e	chore(main): release 3.107.4-beta.0 (#1094)	2026-08-20 06:39:49 +00:00
Paul Nothaft	54b68fe6e8	chore(main): release 3.107.3-beta.0 (#1092)	2026-08-20 05:23:01 +00:00
Paul NothaftandPaul Nothaft	576924fa57	fix(faces): scan external/reference photos instead of skipping them (#1090) (#1091) ... * fix(faces): scan external/reference photos instead of skipping them (#1090) faceProcessor short-circuited every photo with source_origin 'external' or 'reference' straight to 'skipped', before the sidecar was ever contacted. On an external-media install that is the entire library — the reporter's gallery sat at 0/3230 with every row skipped and no error, and a rescan changed nothing. The guard was correct when written: resolvePhotoStorageKey returns null for anything outside managed storage, so ensurePreviewImage could not build a preview and there was nothing to send. #1078 removed that limitation one release earlier — ensurePreviewImage now reads externals straight off the mount via resolvePhotoFilePath and writes the preview into managed storage, so the key faceProcessor already fetches through getStorage() is readable like any other. The guard outlived its reason. Photos whose source is genuinely gone still return a null preview key and land in the existing 'failed' branch, which is the honest outcome: that is a broken photo, not an unsupported one. The blanket skip was absorbing those too. No migration or manual reset needed — enqueueEvent already re-queues rows with face_status in (NULL, 'failed', 'skipped'), so previously skipped photos get picked up on the next scan. * fix(faces): queue external imports for scanning (#1090) The other half of the same bug, found by external review — and my first counter-argument against it was wrong. Managed uploads are enqueued by photoProcessor, which writes face_status 'pending' once a photo is processed (photoProcessor.js:573, commented as "the only correct place to enqueue"). External media never goes through photoProcessor at all: adminExternalMedia inserts rows directly, leaving face_status NULL. faceQueue.claimNextPhoto only claims 'pending' (faceQueue.js:64), so an import into an already-enabled event produced nothing until someone pressed Re-scan. Lifting the skip guard alone made external photos scannable but still not scanned — which looks like a complete fix right up until you import a photo. Resolved once per import rather than per file, since it is a per-event setting and the loop can run to a thousand files, and guarded on both the global flag and the per-event toggle exactly as photoProcessor guards it, so installs without the feature still never write a face_status. A failure to read the setting logs and imports anyway — the photos are the point. No video guard: walkDir only collects jpg/jpeg/png/webp, so nothing faceProcessor would skip as video can arrive through this route. * fix(faces): enqueue imports only after the event path is written External review caught a race I introduced in the previous commit. Marking rows 'pending' as they were inserted published claimable work while events.external_path still held the old value — or none at all, on a first import, since the route only writes it after the entire thumbnail loop. The face worker polls continuously, so on any import long enough to matter (the loop is ~100-300ms per photo, and the reporter's library is 6500+) it would claim those rows, resolve them against the wrong directory and mark them permanently 'failed' — a state only an explicit Re-scan clears. That is strictly worse than the unscanned photos this set out to fix. Ids are now collected during the loop and marked pending in one pass after the event path is written, chunked at 500 because SQLite caps a statement at 999 bound parameters. The test now drives the real route instead of re-implementing its logic, and observes the mid-loop state from inside the per-photo thumbnail call — the only hook that can see the window the race lived in. Verified it discriminates: deleting the enqueue fails two tests, and moving it back onto the insert fails the ordering test specifically. * fix(faces): read the face setting after the import, not before Third external-review round. The setting was captured before a loop that runs for many minutes on a large library, so an admin who enabled detection during an import left every photo imported after that moment at NULL forever — the toggle endpoint only queues rows that already existed when it fired. Ids are now collected unconditionally and the setting is evaluated immediately before the queue update, off a freshly read event row. The guard is unchanged in substance: both the global flag and the per-event toggle, so installs without the feature still never write a face_status. Test flips the toggle from inside the mocked per-photo thumbnail call, which is the same mid-loop hook the ordering test uses. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-20 07:18:45 +02:00
Paul Nothaft	3c5bedc2cf	chore(main): release 3.107.2-beta.0 (#1088)	2026-08-19 15:10:22 +00:00
Paul NothaftandPaul Nothaft	37a15e3d49	fix(faces): restore the :beta image tag and surface sidecar health (#1087) ... * fix(faces): restore the :beta image tag and surface sidecar health Both halves of what a user hit on discussions/1069: the People card sat at "Scanning… 0 of 227" for 30 minutes with no explanation, because the sidecar container could never have started. docker-build.yml — republish `:beta`. It used to come for free via `type=ref,event=branch` when the active development branch was literally named `beta`; the rename to `main` silently retired it. backend:beta has been frozen at 2026-06-29 (448da950) ever since while :main moved on, so PICPEAK_CHANNEL=beta has been serving a seven-week-old build across every image. The ml sidecar was added after the rename and so never had a `:beta` at all, which left docker-compose.production.yml:158 unable to resolve ghcr.io/picpeak/picpeak/ml for any documented channel — the image simply does not exist as :beta or :stable, only as :main and pinned versions. Tag added to all four merge jobs, gated on main. `:stable` stays absent for ml on purpose: it is gated on refs/heads/stable and the sidecar does not exist there. stable's docker-compose.production.yml carries no picpeak-ml service, so nothing can reference the missing tag. FaceRecognitionCard — show when the sidecar is unreachable. An unreachable sidecar is not an error by design: faceQueue.js:132-136 releases the photo back to `pending` and retries forever so a restart does not burn the queue. The cost was that a stopped container looked exactly like a slow scan, indefinitely, and the only signal was a backend log line rate-limited to once per five minutes. /admin/events/faces/health already existed and nothing in the frontend called it. It is now polled while a scan is in progress, and a failing check replaces the spinner with the sidecar URL, the underlying error (which distinguishes a stopped container from a token mismatch) and the command to start it. Health is only polled while a scan is running — an idle card has no reason to care whether the sidecar is up. * fix(faces): tell the three sidecar failure modes apart Follow-up to the health surface in this branch, from an external review pass. The original warning was right about "the sidecar is not working" and wrong about almost everything after that. faceClient.checkHealth now returns a `reason` rather than only a message, because the caller has to know whether photos survive: - 'unauthorized' (401) and 'rejected' (any other 4xx) both become SidecarRejectedError in classify(), which workerLoop does NOT retry — every claimed photo is marked 'failed'. Telling the admin the scan resumes on its own was simply untrue there; both now say to fix the cause and Re-scan. - 'unreachable' (refused/DNS/timeout/5xx) is the retryable one. The card also no longer cries wolf. /faces runs inference synchronously inside an `async def`, so one slow photo blocks the event loop and stalls /info past its 5s timeout — a healthy sidecar can fail a probe. Verified with an isolated uvicorn repro: a blocking call in an async handler stalled the sync /info endpoint to 5.01s. The warning now needs three consecutive failures AND no drop in `pending`. Three because a single /faces call may legitimately run to FACE_ML_TIMEOUT_MS (30s) and two probes 15s apart both fit inside that window; `pending` rather than `scanned` because scanned counts only 'done', so a run producing skipped/failed photos is progress that counter misses. A 4xx burns the queue with no backoff, so it can empty before anyone opens the card — in_progress goes false and only "227 failed" is left. The probe therefore also runs when a finished scan has failures, and the notice renders under the counts instead of replacing them. It is worded as present-tense service state, not as a claim about those specific failures: a live probe cannot know whether they came from this misconfiguration or from corrupt images earlier. Attributing them exactly needs stored face_error rows, which is a bigger change than this. Also adds the missing-token case to the unreachable text: FACE_ML_TOKEN has no default and the container refuses to start without it, so the most likely first run fails as a plain connection refusal that "just start it" does not fix. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-19 16:49:47 +02:00
Paul NothaftandPaul Nothaft	74a8f9bf24	chore(security): shrink the ML image's CVE surface, override deepmerge-ts (#1083) ... Three Trivy cleanups off the code-scanning tab. ml/Dockerfile — install no runtime apt packages at all. Neither libgl1 nor libglib2.0-0 is needed: opencv-python-headless 4.14 bundles what it needs and `ldd .../cv2/cv2*.so` resolves fully on a bare slim base. The old comment claimed the headless wheel still links libGL, which was true of much older wheels. libgl1 was dragging in 36 transitive packages (mesa, LLVM, X11) for a service that never opens a display. Measured with `trivy image` on locally built variants: before: 165 findings — 88 low / 49 med / 19 high / 6 crit without libgl1: 133 findings — 58 low / 48 med / 19 high / 5 crit without either: 123 findings — 57 low / 46 med / 13 high / 4 crit 42 findings gone, image 1.05GB -> 774MB. Not one of the 165 had an upstream fix available, so not installing the packages is the only lever there is. docker-build.yml — set ignore-unfixed on all four Trivy steps. All 123 remaining ML findings are unfixed base-OS CVEs; Debian has them resolved in sid and pending backport to trixie, and apt-get upgrade -y behind CACHEBUST picks each one up automatically. Reporting them buries anything actionable, and suppressing them is the precondition for ever setting exit-code: 1. backend — deepmerge-ts <8.0.0 has a stack-exhaustion advisory (CVE-2026-40345, high) reached via mailparser -> html-to-text, which pins ^7.1.5 so npm cannot get there alone. Not reachable in our code: html-to-text only feeds deepmerge-ts its options object (html-to-text.mjs:1468, :1442), never parsed email content. npm audit goes 3 high -> 0. The lockfile also picks up the version field release-please had left at 3.103.1-beta.0, plus some "peer": true metadata npm 11.6 recomputes. Co-authored-by: Paul Nothaft <[email protected]>	2026-08-19 13:56:01 +02:00
Paul Nothaft	9f825be01e	chore(main): release 3.107.1-beta.0 (#1081)	2026-08-19 08:23:14 +00:00
Paul NothaftandPaul Nothaft	af7970b069	fix(preview): generate lightbox previews for external/reference photos (#1078) (#1079) ... * fix(preview): generate lightbox previews for external/reference photos (#1078) ensurePreviewImage() resolved its source only via resolvePhotoStorageKey(), which returns null for external/reference photos by design — those live on a media mount outside the managed storage tree. The null went straight into withLocalCopy(), which throws ("LocalFsStorage: invalid relative path: null"), so the preview route fell back to redirecting at the full-size original. A gallery whose photos are all external got no benefit from the preview tier (#492) at all: guests paid 5-12 MB on every lightbox open, with nothing surfaced in the admin UI. Add the external branch ensureThumbnail() has had since #423: resolve via resolvePhotoFilePath() and feed the mount path to generatePreviewImage() directly, with an ext<id>_ output basename so two events referencing the same NAS filename can't clobber each other's preview. Also close the adjacent hole that made the failure a throw rather than the documented null: a row with no source_origin in a reference-mode event takes its mode from the event, so resolvePhotoStorageKey returns null for it too. Return null instead of handing that to withLocalCopy. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(preview): select the columns the external branch needs on bulk regenerate POST /api/admin/thumbnails/regenerate-previews selected only id, event_id, path, media_type, mime_type and preview_path, so photo.source_origin was undefined by the time ensurePreviewImage branched on it. Every external row in a reference gallery took the managed path, resolvePhotoStorageKey returned null for it, and the endpoint reported success while generating nothing. Add source_origin, external_relpath and filename to the select, plus a source-inspection test pinning the caller contract and a service-level test showing a column-starved row is indistinguishable from a managed one. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * style(test): single-quote the source-inspection needles Matches the repo eslint quotes rule (no avoidEscape) by dropping the nested quotes from the search strings rather than escaping them. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-19 10:17:44 +02:00
Paul Nothaft	98fe9c7699	chore(main): release 3.107.0-beta.0 (#1077)	2026-08-18 20:41:12 +00:00
Paul NothaftandPaul Nothaft	b69dd134d0	feat(faces): People in this gallery — face recognition via an optional ML sidecar (#1074) (#1075) ... * feat(ml): optional face-detection sidecar, opt-in and inert by default (#1074) First of four PRs for "People in this gallery". This one ships only the sidecar, its wiring and its CI — no schema, no backend code, no UI. Nothing in PicPeak calls it yet. picpeak-ml is a single FastAPI + onnxruntime container: three endpoints (/health, /info, /faces), no database, no volumes, no egress, no model download at runtime. Clustering, person identity and every privacy decision stay in the backend where the data already lives. Models are YuNet (detection) + FaceNet-512 (embedding), both MIT, both pinned by URL and SHA-256 and verified at build time. The licence analysis is in ml/LICENSES.md: the more accurate InsightFace weights are non-commercial-only and PicPeak's users are working photographers, so they are never baked into an image we publish. Two things worth review attention: - Alignment uses a least-squares similarity transform (Umeyama), NOT cv2.estimateAffinePartial2D. RANSAC and LMEDS exist to reject outliers among many correspondences; given five landmarks and no outliers they fit a three-point subset exactly and let the rest drift. Measured on a real off-frontal portrait: eyes and nose pinned to 0.11px, mouth corners 11.8px out on a 160px crop. Umeyama distributes it (max 6.5px, rms 5.1 vs 7.4). The failure mode is silent — a bad warp still yields 512 confident floats — so tests/test_pipeline.py pins it numerically. - FACENET_ONNX_URL has no default and the build fails loudly without it. deepface distributes FaceNet-512 as Keras .h5 only, so the ONNX is produced once by tools/convert_facenet.py and published as a release asset. Converting inside the build would drag TensorFlow through both architecture legs of every build to produce a byte-identical file. The CI jobs are gated on the FACENET_ONNX_URL repository variable and skip cleanly until it is set. Off by default, twice over: the sidecar is behind the `faces` compose profile, and the backend will gate on a `faces` feature flag that defaults to false. FACE_ML_URL defaults to http://picpeak-ml:8000 so the standard deployment needs no configuration — nothing dials that host while the flag is off, which is why a non-resolving default is harmless. Verified: 27 pytest tests green; YuNet loads and detects against a real portrait with its landmark order matching the alignment template index-for-index; both compose files validate and the faces profile is correctly excluded from a default `up`; workflow YAML parses and the job graph resolves. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(ml): pin the converter toolchain, verify parity, drop a false reproducibility claim (#1074) Ran the FaceNet-512 conversion for real and corrected what the previous commit assumed about it. The conversion works: 23,497,424 parameters, 89.6 MB ONNX, and the converted graph matches the Keras original to 2.086e-06 absolute / cosine 1.0000000000. That check is now part of the script rather than something I did once by hand — a subtly wrong graph still returns 512 plausible floats, so it refuses to leave the file on disk if parity fails. Also ran the full pipeline against both real models end to end. The embedding is L2-normalized to 1.000000, and the same face survives being re-rendered: half scale 0.973, double scale 0.984, JPEG q40 0.987, rotated 8 degrees 0.984, brightness +40 0.988. Scale invariance in particular is evidence the alignment warp is doing its job. Corrected claim: the conversion is NOT byte-reproducible. Two runs with the same pinned versions on the same machine gave different SHA-256s. The graphs are functionally identical — same 336 nodes, same 271 initializers, every weight matching to 0.000e+00 — but a few initializer names differ because tf2onnx's traced-op naming is not deterministic (Keras layer naming is deterministic; I checked). The previous commit message and README both claimed byte-identical output. They were wrong, and it matters: anyone re-running the conversion gets a different hash, and without this note that reads like tampering. The build-time SHA-256 pins one published artifact so its URL cannot start serving different bytes; validating a fresh conversion is the parity check's job. requirements-convert.txt now pins the exact set that produced the artifact, including transitive keras/protobuf/numpy, and documents that the converter needs Python 3.11 while the image runs 3.12. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(faces): schema, queue, clustering and API for People in this gallery (#1074) Backend half of the feature. Migration 177, a face-detection queue, the clustering engine, the gallery and admin APIs, and the privacy wiring. No UI yet; nothing is reachable until the `faces` feature flag is on, which defaults to false. The flag is the gate, not FACE_ML_URL. That variable now has a working default (the compose service name), so its presence proves nothing about intent — if it were the gate, every install would poll a hostname that does not resolve. faceQueue re-checks the flag every tick, so turning it off stops the workers without a restart. Visibility scoping is the part worth reviewing closely. Face rows have no concept of photo visibility, but guests are restricted to photos.visibility='visible'. A raw count leaks how many hidden photos someone appears in, and an unscoped cover face renders a crop of a photo the guest may not open — with the best-scoring face being the likeliest pick, so it would happen often rather than rarely. facePeopleService recomputes both per request against the caller's own scope, and event_people.face_count_total is named to be conspicuous in a guest path. Six tests cover it, including the case where a person's photos are ALL hidden and they must vanish entirely. Face data is excluded from backups and .picpeak exports, per the decision in the thread: it is derived, so a restore re-scans rather than carrying biometrics between operators. Three separate mechanisms, because the engines cannot be filtered alike — EXCLUDED_TABLES for export, --exclude-table-data (not --exclude-table; the CREATE TABLE must survive or restore breaks on the first query) for Postgres, and DELETE + VACUUM on the temp copy for SQLite, which has no way to exclude a table from a whole-file .backup. The VACUUM is not cosmetic: without it the pages stay in the file and the claim is false on disk. Archiving now purges face data explicitly. photo_faces cascades off photos, but archive deletes neither the photo rows nor the event, so without this an archived gallery kept its biometrics indefinitely. Other decisions: clustering keeps names across a re-cluster by majority inheritance (without it, one button click silently discards every name the photographer typed); consolidation refuses to merge two people who were named differently; assignment never compares across model_version, since embeddings from two pipelines are not comparable; low-quality faces are stored but left unassigned so they show in "this photo contains" without spawning junk people. Migration is 177, not 174 — 174/175/176 landed on main while this branch was open. 29 tests green: 7 migration (idempotency, down(), cascade, and that installing it enqueues NOTHING), 11 clustering, 11 privacy/visibility. Lint clean; the pre-existing error counts in databaseBackup.js and server.js are unchanged. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(faces): People strip, face filter and admin controls (#1074) Frontend half. Renders nothing anywhere unless the `faces` feature flag is on AND the photographer enabled detection for the gallery — the whole guest surface hangs off one boolean, event.people_enabled, which the server computes from the flag, the per-event toggle and the show-to-guests toggle together. Guest side: a People strip between the filter bar and the grid, circular crops from each person's cover face, an active-filter chip row, and a "Show all" bottom sheet. The face filter composes with category, search, media type and the liked/saved/rated filters in the same useMemo rather than replacing them, so "photos of Anna that I liked" works. Two people selected means AND by default — that is what picking a second face almost always asks for — with a toggle to OR that appears only once a second person is picked. Unnamed people show a photo count and never "Person 7". A number is honest about what the system knows; an invented name is not. There is a test asserting we don't do it. The strip renders nothing below two people, collapses to one line when dismissed (persisted per slug, so dismissing one gallery says nothing about the next), and appears mid-backfill with a progress line rather than blocking the gallery behind a spinner. Avatar crops are computed in ratios of the source dimensions so they survive whatever rendition the browser gets; without width/height they fall back to an uncropped thumbnail, since a wrongly-offset crop is worse than no crop. No new download endpoint: "download these N" rides the existing photoIds path, which already enforces access level and per-category permissions server-side. Adding a person_id selector would have been a second thing to authorize for no gain. Guest-facing copy never says "biometric" or "recognition" — those words describe our implementation, not the guest's experience. The sheet's footnote answers the first question every guest has (where does this go?) inline. The admin card, by contrast, is explicit: it states the controller obligation next to the toggle, and warns that scanning materializes the preview tier on galleries that never generated one, which is real CPU and disk an admin should know about before a 2,000-photo backfill. EN + DE translations. 140 frontend tests green (8 new), tsc and eslint clean. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): measured match threshold, working build defaults, 89MB smaller image (#1074) Ran the Phase 0 spike that had been outstanding, published the model, and fixed what both turned up. THRESHOLD IS NOW MEASURED, NOT GUESSED. LFW's standard 1000-pair protocol run through this exact pipeline (YuNet -> Umeyama alignment -> FaceNet-512 ONNX), 100% detection on 2000 images: same person cosine 0.6958 +/- 0.1415 diff person cosine 0.0849 +/- 0.1674 separation 0.6109 peak accuracy 96.60% @ 0.405 So the pipeline separates people well — the thing I could not previously claim, since every earlier number was the same face re-rendered. Default moves 0.62 -> 0.50. The old value was a placeholder and a bad one: it gave 0% false merges but 22.4% false splits, i.e. roughly one in four same-person pairs failing to join, which fragments a gallery badly. 0.50 gives 1.0% false merge / 8.2% false split. Peak accuracy (0.405) is deliberately NOT chosen: for clustering the two errors do not cost the same. A false split is a duplicate row the photographer can merge away; a false merge puts a stranger into someone's "download my photos" — and until the Phase 2 merge/split UI ships, there is no way to undo one. So this sits on the conservative side of the optimum. The spike is committed as ml/tools/benchmark_threshold.py rather than thrown away, so "why 0.50?" has an answer in six months and a re-tune is one command. BUILD DEFAULTS. FACENET_ONNX_URL/_SHA256 now default to the published ml-models-v1 release asset, so `docker build ml/` and `docker compose --profile faces up` work with no arguments. Blanking either still fails loudly — a URL without a checksum is never acceptable, since the checksum is what makes the URL safe to trust. Found by running compose for real: it failed exactly as designed, which was correct behaviour and a bad out-of-box experience now that a canonical artifact exists. IMAGE SIZE. 389MB -> 300MB single-arch. `chown -R` after COPY rewrote every copied file into a fresh layer, duplicating the 90MB model for nothing; the user is now created before the copies and ownership set via COPY --chown. Also drops pip/setuptools from the runtime image. Measured RSS is 186MiB idle, and the container answers /faces end-to-end in well under the 80-150ms/photo the issue budgeted. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): threshold 0.50 -> 0.60 from real clustering, theme-aware People strip (#1074) Both fixes come from running the feature on an actual gallery — 61 photos, 5 real identities — rather than reasoning about it. THRESHOLD. The LFW pairwise sweep in the previous commit said 0.50, and it was wrong. On a real gallery at 0.50, three of six visible clusters were contaminated: two different people merged into one strip entry, which is the exact failure that puts a stranger into someone's "download my photos". Pairwise error rates do not predict cluster purity. Greedy assignment compounds — one wrong face drags the centroid toward the midpoint between two identities, making the next wrong face likelier. A 1% pairwise false-merge rate is not a 1% chance of a clean gallery, and no amount of staring at an ROC curve would have shown that. Sweep against ground truth (5 identities): 0.50 -> 6 clusters, 3 contaminated 0.56 -> 6 clusters, 0 contaminated 0.60 -> 5 clusters, 0 contaminated <- exactly right 0.64 -> 5 clusters, 0 contaminated, fewer faces assigned 0.60 recovers the right number of people with no contamination; higher only loses coverage. Migration 177 carries the full reasoning so the next person to touch this knows why the obvious pairwise answer is the wrong one. THEME. The People strip hardcoded `text-neutral-800` for named people. On a dark gallery — which the screenshot immediately showed — that renders a named person's label almost invisibly, while UNNAMED people stayed legible. Exactly backwards. Labels, headings, the collapsed summary, the scan line and the filter chip row now read the gallery's own theme tokens (--color-text / --color-muted-text / --color-accent / --color-surface-border) like the rest of the gallery surface. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): keep the mobile filter row inside the viewport (#1074) At 390px the photo count and Clear link were pushed against the right edge by ml-auto and clipped. Only apply it from the sm breakpoint up, where there is room; below that they flow after the chips. Found by screenshotting the real thing on an iPhone-sized viewport. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(faces): complete Phase 1, add People management and auto-categories (#1074) Closes the two Phase 1 gaps, then builds Phase 2 and Phase 3. PHASE 1 GAPS. "Download these N" was specified, described as done in an earlier summary, and never actually built — I had verified the backend needed no new endpoint and let that stand as if the button existed. It now hands the filtered photo ids to the same path as a manual selection, so the server re-applies access level and per-category permissions on the way through. Photos in a downloads-disabled category are excluded client-side too, so the number on the button is the number the guest receives. Hidden entirely when downloads are off for the gallery. Lightbox person chips ("In this photo: Anna") are the second way into the face filter — a guest looking at a photo of themselves can act on it without scrolling back to the strip. Tapping one closes the lightbox and filters the grid behind it. PHASE 2. A People management modal over the endpoints that already existed and were already tested: rename inline, merge (multi-select, first pick is the target so the name a photographer typed survives), split via a face picker, hide, ignore. This matters more than it sounds — clustering deliberately errs toward splitting because a wrong merge puts a stranger into someone's download, and that trade only works if merging is easy. PHASE 3. Rule engine over face_count plus face-area ratio: 0 -> Details, 1 large -> Portraits, 2-5 -> Small groups, >5 -> Groups. The area ratio is what separates "a portrait of someone" from "someone is in this landscape". Three guarantees, all tested: it only ever fills an EMPTY category (enforced in the query AND re-checked in the UPDATE, so a photographer setting one mid-run still wins), everything it touches is marked auto_categorized so undo is exact, and it is a no-op unless separately enabled. Migration 178 adds the column — separate from 177, which has already run wherever this branch is deployed. Verified on the real gallery: 61 photos -> 48 portraits + 13 small groups, undo cleared exactly 61 and left the manual ones alone. Merge moved faces and removed the source. Both confirmed against the database, not just the UI. TWO BUGS THE BROWSER CAUGHT, both invisible to tsc: - The lightbox destructure never landed — my patch targeted a line that has a default value, matched nothing, and failed silently. `people` resolved to something else entirely and the chips would never have rendered. eslint's "outer scope value" warning is what surfaced it. - Admin face thumbnails 403'd because <AuthenticatedImage> attaches whatever gallery token is in session storage; an admin who has also opened one of their own galleries sends a type:"gallery" bearer to an admin route. Admin routes authenticate from the httpOnly cookie, which a plain same-origin <img> sends by itself. Worth noting AdminPhotoGrid has the same latent shape; not touched here. Also: the admin card now reports "N people (M shown to guests)" when those differ, so the settings page and the gallery stop disagreeing without explanation. 45 backend tests (8 new) and 140 frontend tests green; tsc and eslint clean. EN + DE for every new string. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * perf(faces): batch migration DDL and drop the face stack from server.js import (#1074) CI's backend job timed out at 10 minutes on the first run of this branch. Nothing failed — 132 of 182 suites passed and the wall clock ran out. Main does the same 182 in 124s, and where main has 12 suites slow enough for jest to print a duration, this branch had 77. Two changes, both worth making regardless of how much of the gap they close: - Migration 177 added its columns one ALTER TABLE at a time (four on photos, three on events, plus a separate index statement) and seeded settings with a SELECT and an INSERT per key. It now uses one alterTable per table and one SELECT plus one bulk INSERT. 178 folds its index into the same statement as its column. That chain replays in ~90 suites, so statement count there is multiplied by 90. - server.js required faceQueue at module scope, which pulls in axios and — through imageProcessor — sharp. Every supertest suite that imports server.js was paying for a module graph it never uses. Now required inside the startup block, next to the call that needs it. Honest about the evidence: locally the migration delta measures at zero (1.15s vs 1.13s for the same suite, three runs each), so batching alone does not explain an eight-minute regression. A fast local disk and many cores mask per-statement and per-import costs that a two-core runner with a shared disk does not. These are the two real costs this branch added to a path that runs in almost every suite; whether they are sufficient is a question for CI, not for another round of local speculation. 37 face tests still green after the change. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * i18n(faces): complete EN and DE coverage for the face feature (#1074) The admin card and the Features toggle were rendering entirely from inline English `defaultValue` fallbacks — 22 keys existed in no locale file at all, so a German admin saw an English consent notice, English toggles and English buttons. The gallery side was already translated; the admin side was not, and nothing in the toolchain flags this because a `defaultValue` always renders something. Adds the missing `admin.faces.*` (19), `settings.features.faces.*` (2) and shared `common.clear/saved/saveFailed` in both languages. Existing keys are left alone (setdefault, not overwrite), so the shared `common` strings other features rely on are untouched. Committed the audit as frontend/scripts/i18n-faces-audit.py rather than throwing it away: it extracts every t() key the face components actually use and diffs it against each locale, and it also reports German values that are byte-identical to English, which is the usual shape of an untranslated copy-paste. Currently: 69 keys in use, EN complete, DE complete, no identical pairs. Verified in the browser, not just in the JSON — the German card reads "61 / 61 Fotos durchsucht · 16 Personen (5 für Gäste sichtbar)" end to end. Also checked the components for hardcoded user-facing text (JSX nodes, title/aria-label/placeholder attributes) outside t(); there is none. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): 13 defects from external review — coordinates, counts, erasure, races (#1074) Codex reviewed the branch against main. Thirteen findings, nine P1. I checked every one against the code and could not dismiss a single one as a false positive, so all thirteen are fixed here. THE WORST ONE: bounding boxes were stored in the wrong coordinate system. The sidecar reports coordinates in the space of the image it was HANDED — which is the ≤1920px preview, not the original — while every consumer compares them against photos.width/height, the original dimensions. A 6000px photo therefore produced boxes ~3x too small and areas ~9x too small: avatar crops landed in the wrong place and the Portraits rule could never fire. It is invisible on any photo already under 1920px, which is exactly why the demo gallery and every screenshot looked correct. Now scaled once in faceProcessor so everything downstream can assume original-image coordinates. ERASURE. The FK cascade on photo_faces is decorative on SQLite: PicPeak never enables `PRAGMA foreign_keys`, so deleting a photo left its embeddings behind. I first enabled the pragma globally and reverted it — six unrelated suites immediately failed on pre-existing dangling references, and switching it on would start rejecting inserts on every existing install. That is a real change worth making, but it is its own PR, not a rider on this one. Instead deletion purges explicitly: purgePhotoFaces in the photo paths (single, bulk, service) and photo_faces/event_people in deleteEventCascade. Tests assert this with the pragma explicitly OFF, so they can only pass if the code does the work. COUNTS. A re-scan deleted the old face rows without undoing their contribution to event_people, so counts inflated on every re-scan and ghost people survived. Now the affected people are recomputed before the replacements are assigned. My own "must not double its faces" test only checked photo_faces rows, which is why it passed throughout. RACES. A worker that finished after an admin purged the event committed its rows anyway — erasure reported success and the data reappeared. The commit is now conditional on the row still being 'processing'. And assignFaces is read-modify-write over an event's people, so two workers lost each other's updates; it is now serialised per event with an in-process mutex plus a Postgres advisory lock for the multi-pod case the queue advertises. METADATA LOSS. Merging discarded the source's name and suppression flags, so a merge could erase a typed name or un-hide someone. Reclustering remembered only people with a label, so an unnamed-but-hidden bystander came back guest-visible after one "Re-group people" — and suppression now propagates to every descendant cluster, not just the majority one. Also: export reset face_status so a restored gallery re-scans instead of claiming to be scanned forever; manual category edits clear auto_categorized so "undo automatic" cannot delete a photographer's own choice; external photos are skipped rather than failed (resolvePhotoStorageKey returns null for them by design); the gallery refetches photo memberships as a scan progresses so filtering is not stale; a failed VACUUM now fails the backup rather than publishing one that may retain biometric pages; and the ML Dockerfile's `|| true` is scoped to the uninstall — as written it was `(install && uninstall) || true`, so a failed dependency install produced a green layer and an image with no onnxruntime. Four new regression tests. Full backend suite failure set verified identical to origin/main; frontend 140 green; tsc and eslint clean. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): 12 more defects from review round 2 — cross-event purge, leaks, lifecycle (#1074) Second Codex round on the same diff, now including round 1's fixes. Twelve findings, seven P1. Again none were false positives. SECURITY, AND MINE FROM ROUND 1: the bulk-delete face purge iterated the raw `photoIds` from the request instead of the event-scoped `photos` rows the handler had already validated. purgePhotoFaces has no event scope of its own, so an editor could pass another gallery's photo id and delete its face data — even though the photo deletion right below it was correctly scoped. Fixing one thing and introducing another is exactly why the second round was worth running. ANOTHER VISIBILITY LEAK, same class as the one round 1 fixed: /people returns scan progress, and getScanStatus counted every photo with a face_status — including hidden ones. Guests could read the hidden-photo count off the progress bar while the people list and covers beside it were properly scoped. Now scoped by the same predicate, with the caller passing its audience. RECLUSTER, ROUND 1'S FIX WAS INCOMPLETE. I made suppression follow every descendant but still copied the flags from the majority ANCESTOR. When reclustering merges a visible named person with a hidden one, the majority ancestor is often the visible one — republishing the hidden person's photos. Suppression is now OR-ed across every ancestor contributing faces. The name also now goes to the genuine largest descendant; the previous code took whichever cluster came first in map order, which the comment already claimed it did not. LIFECYCLE. Face data is excluded from backups and exports, but photos. face_status came across intact, so a restored install claimed every photo was scanned while holding no faces — and the worker only claims 'pending', so it stayed that way forever. Now: the SQLite backup requeues in the dump, restore requeues after the pool reinit (the Postgres path cannot rewrite rows inside pg_dump), the portable importer purges LOCAL face tables (they were excluded from the replace list, so another instance's embeddings survived an import with FK checks suspended) and requeues, and archiving disables detection so a restored archive is honestly off rather than enabled-and-empty. WRITE PATHS. Only processPhoto enqueued. The synchronous upload path (chunked-upload completion, watch-folder) left photos unscanned, and replacePhoto kept the OLD image's faces on a row now pointing at a different picture — stale identities shown on the new photo. FRONTEND. PeopleSheet and the admin manager rendered centred thumbnails and ignored the bbox, so on group photos the avatar showed whoever stood in the middle and two people from one photo were indistinguishable — in the manager whose entire job is telling faces apart. The crop maths is now one shared helper (faceCrop.ts) so the three surfaces cannot drift again. Full-page layouts (gallery-premium, gallery-story) render their own lightbox and never received the people props. Backend failure set verified identical to origin/main; frontend 140 green; tsc and eslint clean. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(faces): round 3 — five of round 2's fixes were wrong or no-ops (#1074) Third and final Codex round. Eight findings, four P1 — and the important part is that FIVE of them are defects in round 2's fixes, not in the original code. - The sync-upload enqueue I added was a silent no-op. It queried through `trx` after the transaction had already been committed, which throws "Transaction query already complete" straight into the catch I had wrapped it in. Chunked uploads and watch-folder imports were still never scanned, and the code read as though they were. Uses `db` now. - The post-restore requeue ran BEFORE the files were restored, in both the portable importer and the native restore. The face worker is live during a restore, so it could claim those rows and scan the previous instance's files, or fail them for originals not yet on disk — with nothing to requeue them afterwards. Both now run after file restoration; the native one is extracted into requeueFaceScans() and called from the full and database-only paths. - The admin face crop mixed coordinate spaces: an original-pixel bbox scaled against the THUMBNAIL's natural size. The API now returns the source dimensions alongside the box, so there is one space to reason about. - Forwarding people props through layoutProps did not make them work — the full-page layouts never destructured them. GalleryStoryLayout now threads them to its own lightbox. Genuinely new findings, all in the same class as ones already fixed: - releaseToPending updated unconditionally, so a photo purged while its sidecar request was in flight came back as 'pending' and was rescanned — biometric rows reappearing after the purge reported success. Round 2 fixed exactly this on the COMMIT path and I did not carry it to the retry path. Now guarded on 'processing'. - purgePhotoFaces left face_status alone, so a worker mid-scan still satisfied its commit guard and could write fresh faces into a photo being deleted — orphans, since the FK cascade is inert on SQLite. It now clears the claim as part of the purge. - Phase 3 was unreachable: the migration seeds face_auto_categorize_enabled false and nothing could ever write it, so the rule engine and its undo endpoint returned "disabled" in every real flow. Added GET/PUT and a toggle on the admin card, EN + DE. NOT fixed, deliberately: GalleryPremiumLayout uses yet-another-react-lightbox rather than the shared PhotoLightbox, so person chips there are a real port rather than a prop forward. Recorded as open rather than bodged. Backend failure set identical to origin/main; 41 face tests and 140 frontend tests green; i18n audit reports EN and DE complete at 71 keys. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(faces): block face recognition on the all-in-one image (#1074, #1042) The single-container image cannot run this feature, so it is refused there rather than left to degrade. WHY, since the reason is not obvious from the code: the AIO image runs the backend, the frontend, SQLite and every background worker inside one container aimed at "one photographer plus guests browsing". It has no Redis, SQLite gives it a single writer, and it contains no ML sidecar to talk to. Face detection would add a second image-processing pipeline competing with Sharp for the same CPU and memory. That failure is not loud — the install just becomes slow and looks broken, which is the worst possible shape for a deployment whose whole promise is one container and no decisions. Gated on an explicit PICPEAK_SINGLE_CONTAINER marker, NOT inferred from SERVE_FRONTEND or a SQLite path: plenty of legitimate multi-container setups serve the frontend from the backend or run SQLite, and none of them should lose the feature by accident. Three layers, because the first is the only one that enforces: - faceSettings.isFeatureEnabled() returns false before consulting the flag, so a database restored from a full deployment with `faces` enabled still cannot switch it on here. - The feature-flag API forces `faces: false` in both directions, so the admin UI reflects reality instead of offering a switch that refuses to stay on. - The Features tab renders the card disabled with a plain-language reason, read from a new `single_container` field on /admin/system/version (an endpoint the admin UI already calls). Documented in ml/README.md and .env.example. Three tests pin the behaviour, including that the marker only accepts explicit truthy values. NOTE FOR PR #1068: this expects `Dockerfile.aio` to set `ENV PICPEAK_SINGLE_CONTAINER=true`. That one line lives on that branch and is not in this commit — until it lands, an AIO build would still offer the feature. Worth adding alongside the `Limits` section of docs/single-container.md. 44 face tests green; EN + DE complete at 72 keys. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * test(faces): pin the bbox coordinate space with a real scale factor (#1074) The coordinate-space bug — boxes stored in preview space while every consumer reads them as original-image pixels — had no test, and could not have been caught by the ones that existed: every photo in the demo gallery is 750px, so the scale factor was always exactly 1.0 and the correction never executed. Verified by hand first, on a real 4000x3000 upload with the face placed off-centre so a wrong crop would be unmistakable. Before the fix the stored box was 1493,204 (preview space, face actually at x≈2850-3618); after, 3110,426 — a factor of 2.083, exactly 4000/1920, landing inside the face. The admin crop then resolved to left=-395px/top=-46px on a 64px window, which is the face centred. That verification is now a test rather than a memory. Three cases: a 4000px photo must scale by 4000/1920, a 1920px photo must NOT change (the case that hid the bug), and a row with no width must fall back to unscaled rather than storing NaN. Note for anyone extending these: jest hoists mock factories above the file, so anything they close over has to be `mock`-prefixed. Getting that wrong fails at transform time with a message that does not name the variable. 47 face tests green. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-18 22:37:28 +02:00
Paul Nothaft	6ebdd13dde	chore(main): release 3.106.0-beta.0 (#1076)	2026-08-18 20:21:44 +00:00
3 people	0874a30ac9	feat(docker): all-in-one image (#1042) — my version of #1067 (#1068) ... * feat(docker): add all-in-one image — backend + frontend in one container (#1042) One container, one Node process, SQLite by default: `docker run` with no compose file, no nginx, no supervisor, no bundled Postgres/Redis. - Dockerfile.aio (repo-root context): frontend build stage + backend deps stage + a runtime stage mirroring backend/Dockerfile's production stage, with the built SPA copied to /app/frontend/dist and SERVE_FRONTEND=true. DATABASE_CLIENT=sqlite3 and STORAGE_PATH=/app/storage are pinned explicitly — the storage fallback resolves to container-root /storage, which EACCESes after the su-exec drop. - server.js: the SERVE_FRONTEND block now does what the nginx image did — renders ${BRAND_TITLE}/${BRAND_DESCRIPTION} into index.html once at boot, serves that rendered shell on /index.html and every SPA route, caches hashed /assets/* immutably while the shell revalidates, and gzips the bundle via compression() mounted after all /api routers. express.static now runs with index:false so `/` keeps flowing to handlePublicSiteRequest — its default index option was shadowing the landing page on native installs. - wait-for-db.sh: skip the Postgres readiness wait when DATABASE_CLIENT is sqlite3. The engine resolver still runs, still logs, and still refuses the populated-both conflict (#1038). - .dockerignore: **/node_modules, so the root-context build can't pick up host deps from backend/ or frontend/. - docker-build.yml: build-aio / merge-aio follow the same per-arch build → digest-merge → per-version tag scheme as backend/frontend (GHCR only for now; the Docker Hub mirror is wired once the Hub repo exists), plus a smoke-aio job that boots the image on every PR and asserts /health, the SPA shell, the rendered brand title, immutable asset caching and the SQLite engine resolution. Pointing DB_HOST/DB_USER/DB_PASSWORD + DATABASE_CLIENT=pg at an external Postgres works exactly like the backend image. * fix(ci): correct three smoke-aio assertions that would fail a green image (#1042) Found by running the smoke job locally against a real build — the image passed every behavioral check, but three assertions were wrong: - `/` asserts 200, but handlePublicSiteRequest 302s to /admin/login while the public landing site is disabled, which is the state of the fresh install the smoke container always is. Assert the redirect target instead — that still proves express.static's index option is not shadowing the handler, which is the thing the check exists for. - The placeholder-leak grep matched index.html's explanatory comment, which mentions BRAND_TITLE in prose and survives into the built shell. Match the literal ${BRAND_TITLE}/${BRAND_DESCRIPTION} tokens with -F, and cover the description token too. - Add a gzip assertion, probing with GET: the compression middleware skips bodyless responses, so a HEAD probe reports no Content-Encoding even when compression is active. Verified locally on linux/arm64: image builds clean, boots to healthy in ~8s on the SQLite default, and 25/25 checks pass (SPA shell, rendered brand title, immutable+gzipped assets, no-store shell, SPA fallbacks, npm removed, su-exec drop to nodejs, no errors in the boot log). The DATABASE_CLIENT=pg override was exercised against a real Postgres too — the readiness wait still runs and the engine resolves to postgres. * fix(server): serve the SPA for every client route, not just /admin and /gallery (#1042) nginx did `try_files $uri $uri/ /index.html`, so behind compose every client-side route survived a direct hit or a refresh and the short `['/admin', '/admin/*', '/gallery/*']` list was never exercised. Without nginx that list is the whole contract, and everything outside it 404'd: /setup /customer /impressum /datenschutz /payment-check /quote/:token /contract/:token /invite/:token /transfer/:token /transfer-upload/:token /setup is the first URL a new install visits, so the all-in-one image was unusable from a cold start. The catch-all is registered after `app.use('/api', notFoundHandler)`, so an unknown /api route still answers JSON instead of being handed the HTML shell, and after the /s/:shortSlug resolver, so a typo'd short URL still 404s (#699). It is GET-only — a stray POST keeps 404ing rather than getting a 200 page back. The handler is hoisted out of the SERVE_FRONTEND block via `spaCatchAll` because that block runs before the API 404 handler is registered. Verified on the built image: all ten routes above now 200, /api/nope still returns JSON 404, /s/nonexistent still returns 404, / still 302s to /admin/login, and the smoke suite is 25/25. Both boundaries are now asserted in the smoke-aio job. * docs(readme): document the single-container install (#1042) The README had no mention of the all-in-one image, so the only way to discover it was reading the workflow file. Adds a Quick Start subsection with the one-line `docker run` and the `docker exec … cat SETUP_TOKEN` step, plus a row in the documentation table. Deliberately does not sell it as the default: the note says the compose stack is still the right choice for anything busier, gives the reason (SQLite takes one writer at a time), and points at the `.picpeak` restore as the way out, so nobody picks it and then finds themselves stuck. Full details live at docs.picpeak.app/deployment/single-container (PicPeak/docs#8). * feat(docker): fold #1067's items into the all-in-one image (#1042) Consolidating the two parallel AIO branches into this one. This PR's approach is kept wherever the two differed on design — in particular the in-process brand render, `index: false` (which fixes express.static shadowing handlePublicSiteRequest, a bug #1067 had), the compression middleware, and the smoke-aio job. What follows is what #1067 had that this branch did not. Layout — the issue asks for a single mountable root, and this moves to one: /data/db picpeak.db (+ -wal/-shm) and SETUP_TOKEN /data/storage originals, thumbnails, archives /data/logs application logs /data/backup built-in backup output; /backup symlinks here `-v picpeak:/data` and nothing else to remember. README and the smoke job's database-path assertion follow the new layout. Correctness items: - sqlite CLI. DatabaseBackupService SPAWNS `sqlite3` for `.backup` and PRAGMA integrity_check; the npm module does not ship that binary. backend/Dockerfile omits it because compose always runs Postgres — this image defaults to SQLite, so every database backup failed with ENOENT. - /backup wired in. Migrations 029 + 030 seed /backup/picpeak and /backup/database as the backup destinations; nothing created or mounted them, so backups had nowhere to write and anything written would die with the container. Symlinked into the volume, subdirectories created at startup (a bind mount hides the tree baked into the image), and adopted only when BACKUP_DIR is set so it never gates boot for compose deployments that do not mount it. - logger.js honours LOG_DIR. It hard-coded <backend>/logs, so logs could not leave the container. Unset keeps the old path for every existing install. - wait-for-db.sh derives its writable roots from STORAGE_PATH / DATA_DIR / LOG_DIR instead of hard-coded /app paths, and mkdir -p's them before chown — a bind-mounted /data hides the image's tree, and chown against a missing path reports "the filesystem rejects chown", which is both wrong and a dead end. - .dockerignore excludes backend/-prefixed runtime data. Docker reads only the root file, so the unprefixed data/*.db, logs/* and storage/* rules missed backend/data, backend/logs and backend/storage entirely; a checkout used to run PicPeak would bake its database, photos, logs and SETUP_TOKEN into a published layer. - HEALTHCHECK follows $PORT rather than a hard-coded 3000. - --max-http-header-size=32768 matches nginx's large_client_header_buffers 4 32k; Node's 16 KiB default would reject a guest carrying several per-gallery JWT cookies. docs/single-container.md is added as the in-repo reference the README links to. The smoke job gains four assertions for the above: the one-volume layout and writable backup destinations, the sqlite3 CLI, logs landing on the volume, and the image carrying no runtime data from the build context. Verified on a built image — named volume, bind mount and PORT=8080 all healthy; every existing smoke assertion still passes, including / -> 302 /admin/login, the rendered BRAND_TITLE, immutable assets, gzip and /s/<unknown> -> 404. Co-authored-by: Luca-Timo <[email protected]> * fix(docker): restore the SPA-fallback exclusions and close the build-context leak (#1042) Both found by external review of the consolidated branch. - The SPA catch-all had no backend-owned exclusions. This was a regression I introduced while merging: #1067 carried a BACKEND_OWNED prefix list, and taking this branch's server.js wholesale (correctly — its index:false and in-process brand render are the better design) dropped it. /photos, /thumbnails, /uploads and /fonts are static mounts whose middleware calls next() on a miss, so the catch-all was answering 200 text/html under image and font URLs instead of 404. nginx gave each of those its own location block, so try_files never applied to them. - backend/data is now excluded wholesale rather than by suffix. The suffix list (*.db, *.db-wal, *.db-shm, SETUP_TOKEN) let real secrets through: a used checkout carries ADMIN_CREDENTIALS.txt next to the database, plus -journal files and any DATABASE_PATH not ending in .db. Since Dockerfile.aio builds from the repository root and COPYs backend/ wholesale, any of those would be baked into a published layer. The directory holds only runtime state and is already gitignored in full. smoke-aio gains an assertion that the backend static routes still 404, so the exclusion cannot be dropped again silently. Verified on a built image: /photos, /thumbnails, /fonts and /uploads misses all 404; /setup, /impressum, /gallery/x, /admin/login still 200; / still 302s to /admin/login; /api/nope still answers JSON; /s/<unknown> still 404s; and the image carries no *.db, ADMIN_CREDENTIALS.txt, logs or storage from the context. * fix(aio): three failures that only surface outside a dev laptop (#1042) Backups aborted on SQLite. getTableChecksums() built its digest with `CAST(t.* AS TEXT)`, which is Postgres row-to-text syntax; SQLite parses `*` there as a syntax error, so every backup threw before reaching the .backup call. Since the all-in-one image ships SQLite by default, that is every AIO install. Enumerate the columns via columnInfo() and sum their lengths instead. The shared /data mount root was never adopted. wait-for-db.sh chowned the children it creates but not the mount point itself, so a host directory arriving as 0700 with a foreign owner stayed untraversable by UID 1001 after the su-exec drop. Docker Desktop's permissive bind mounts hide this completely, which is why local testing passed; a NAS share does not. DATA_ROOT is now adopted first. Maintenance mode locked the admin out of the box. The middleware runs at server.js:493, long before the static block at 891, and exempted the auth endpoints but not the page that calls them. With the backend serving the frontend, /admin/login and /assets/* returned 503 JSON, so an admin who enabled maintenance mode could never load the UI to turn it off. nginx serves those paths in the compose stack, which is why it never surfaced there. Guest and API surfaces stay gated. Verified on a built image: checksums compute across all 95 tables; a bind mount created 0700/4000:4000 boots healthy and ends up 1001:1001; with general_maintenance_mode=true, /admin/login, /admin and /assets/* return 200 while /gallery/* and /api/gallery/* return 503 — and 503 across all three once the exemption is removed again. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(aio): stop leaking .env into the image, fix the broken checksum test (#1042) The Jest suite was red: mocking db.raw is no longer enough now that the SQLite checksum branch asks the query builder for its column list, so db(table) came back undefined and getTableChecksums failed on every PR. The production code is right; the fixture needed to know about the call. backend/.env was landing in the published layer. The root ignore file's `.env`, `.env.*` and `data/*.db` rules read as unanchored but Docker matches them from the context root, so they catch ./.env and never backend/.env — and `COPY backend/ .` then puts a real JWT_SECRET at /app/.env. Matched at any depth instead, the way **/node_modules in the same file already is. Confirmed by building from a checkout carrying a planted secret: before, `cat /app/.env` printed it back. Business documents wrote outside the volume. quoteService, invoice sending/reminders and contract signatures build paths from process.cwd()/storage and never read STORAGE_PATH; compose hides it by setting STORAGE_PATH=/app/storage with WORKDIR /app so the two are the same directory. Here they are not, and /app is root-owned, so a quote or invoice PDF failed to write as UID 1001 — and would not survive the container if it had. Symlinked /app/storage into the volume, matching the /backup symlink beside it. Teaching those services STORAGE_PATH is the real fix and wants its own change. Two smaller ones: the mount root is now chowned shallow rather than recursively, since every child below it is already walked recursively and a NAS-sized photo library should not be traversed twice on each restart; and /assets/ joins the backend-owned prefixes, so a stale hashed chunk requested by a tab left open across an upgrade gets a 404 instead of index.html served with 200 under a .js URL. Verified on a built image: planted backend/.env and backend/probe.db are absent; /app/storage resolves to /data/storage and a business-doc write as UID 1001 appears on the host; a 0700 bind mount owned by 4000:4000 boots healthy; a missing /assets chunk 404s while the real bundle still serves 200 as application/javascript. The databaseBackup suite is green again, and the branch adds no failing suite that origin/main does not already fail on the same machine. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * test(aio): teach the leak assertion about the storage symlink (#1042) The previous check listed /app/storage/events and treated a hit as a leak. That was true while /app/storage was either absent or a copied directory; now it is a symlink into the volume, so the check followed it and found the empty tree the image itself creates — a false positive on its own design. Check the shape instead: /app/storage must be a symlink pointing at /data/storage, and the volume's photo tree must contain no files on a fresh install. A real directory there now fails loudly, which is the condition the assertion was always trying to catch. Also extended the path list to /app/.env and loose database files, matching the .dockerignore rules added alongside. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(aio): show the maintenance screen instead of raw JSON to guests (#1042) The previous commit exempted the admin shell so an admin could still reach the switch they had just flipped. Guests had the same problem for the same reason: with no nginx in front, /gallery/<slug> reaches this middleware long before the static block, so a visitor during maintenance got a 503 JSON body where every other deployment shows the branded maintenance screen the frontend already ships. Replaced the two path-specific exemptions with the rule they were both special cases of: a GET that is not an API call and not a backend-owned content mount is the SPA shell, and the shell is inert HTML — it boots, reads /api/public/settings (already exempt) and renders MaintenanceMode on its own. Everything that carries real data stays gated: /api/*, /photos/, /thumbnails/, /fonts/, and any non-GET. Compose is untouched by construction, since nginx answers those paths and they never arrive here. Verified on a built image with the flag on: /gallery/x, /customer/x, /admin and /admin/login return 200 text/html while /api/gallery/x/verify, /photos/x.jpg and /thumbnails/x.jpg return 503 and a POST to a public API still returns 503; with the flag off the same paths go back to 404. Added a middleware test over that exemption matrix — over-exemption is the real risk in this change, so it asserts the gated half too. It fails on five cases without the fix. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(aio): stop the shell exemption from un-gating /og and the public CMS (#1042) The previous commit exempted "any GET that is not an API call". That negative rule reads as safe and is not: /og/gallery/<slug> and its /cover render the event name and the hero thumbnail, /s/<code> renders short-link previews, and `/` is handed to the public CMS. All four are proxy_passed to the backend by nginx, so they were gated before this PR in every deployment — the rule un-gated them, and for compose too, not just the new image. A site switched to maintenance would have kept publishing gallery metadata. Replaced the guess with the split nginx already defines: exempt what the frontend container answers itself, gate what it proxies. That is the same rule the all-in-one image needs by definition, since its whole job is to be both halves of that stack, and it now matches compose in both directions rather than only in the direction the last commit tested. Verified on a built image with the flag on: /admin/login, /gallery/<slug> and /customer/* return 200, while /, /og/gallery/x, /og/gallery/x/cover, /s/abc, /robots.txt, /api/* and /photos/* return 503; with the flag off all of them behave normally again. The middleware test grew the gated cases — it now covers 21, most of them asserting what must NOT be exempt. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(aio): give the image a FRONTEND_URL default so share links are absolute (#1042) getFrontendBaseUrl() reads FRONTEND_URL, falls back to the general_site_url setting, and otherwise returns an empty string — which makes share_url come back as a bare "/gallery/<slug>/<token>". Compose defaults the variable to http://localhost:3000, but the documented one-liner for this image passes only JWT_SECRET, so every fresh single-container install handed out relative links in API responses, QR codes and emails. Defaulted to the same value compose uses; -e FRONTEND_URL=https://... overrides it, as does the site URL field in Settings. Found by pointing tests/e2e/local at a running AIO container: auth/06-api-tokens asserts share_url matches /^https?:\/\//, and it was the one spec that failed for a product reason rather than a harness one. It passes now, and the suite is 19/20 against the image — the remaining failure is smoke/02-auth-flow, whose seed helper shells out to a hard-coded `docker exec picpeak-backend`, so it cannot arrange its precondition against any other container. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * feat(aio): mark the image so face recognition stays off (#1042, #1074) Face recognition needs a separate ML container this image does not contain, and enabling it here would add a second image-processing pipeline competing with Sharp for the CPU and memory of a container sized for one photographer plus guests browsing. The failure mode would not be a clear error — just a slow install that looks broken. The backend gate for this lands in #1075 and keys on PICPEAK_SINGLE_CONTAINER. Without this line the guard never triggers on an actual all-in-one build, so the two changes have to arrive together: whichever merges second completes the pair. Verified against this file's exact value — isFeatureEnabled() returns false with it set. An explicit marker rather than inferring from SERVE_FRONTEND or the SQLite path, because legitimate multi-container deployments do both of those and should keep the feature. Also adds it to the Limits section of docs/single-container.md, next to the SQLite and Redis constraints, since that is where someone will look before choosing this image. --------- Co-authored-by: Paul Nothaft <[email protected]> Co-authored-by: the-luap <[email protected]>	2026-08-18 22:15:49 +02:00
Paul NothaftandPaul Nothaft	f22999aba6	fix(storage): write business documents under STORAGE_PATH, not the cwd (#1070) ... * fix(storage): write business documents under STORAGE_PATH, not the cwd persistDocPdf, the invoice sending and reminder writers and both contract signature writers built their target from `path.join(process.cwd(), 'storage', 'business-docs', ...)` and never consulted STORAGE_PATH. docker-compose.yml and docker-compose.production.yml both pin STORAGE_PATH=/app/storage and the image's WORKDIR is /app, so on a stock deployment the two expressions name the same directory and nothing looked wrong. Point STORAGE_PATH anywhere else and quotes, invoices, Mahnungen and contract PDFs land outside the configured storage root: missed by the backup walker, invisible to the storage accounting, and gone when the container is replaced. It also fails outright where the working directory is not writable by the runtime user. Routed all six writers through getStoragePath(), the resolver the rest of the app already uses. Two read-side sites of the same class came along: the custom PDF font lookup now checks the storage root before the legacy cwd path (a font under STORAGE_PATH/fonts was simply never found, and the document silently fell back to the built-in face), and the dev-test scratch directory follows the same root. Left alone deliberately: resolveLogoFile and adminBusinessProfile already try both roots, so their cwd reference is a legacy fallback rather than a miss. No migration needed — the persisted path is stored absolute, so rows written before this keep resolving to where those files actually are. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(storage): allow the configured contract root, and move signature images too Two holes in the previous commit, both found by review. Contract downloads would have broken. assertContractPdfPath() guards the admin unsigned/signed PDF routes and GET /api/public/contracts/:token/pdf, and it listed only <cwd>/storage/business-docs/contract. Moving the writers to STORAGE_PATH without moving that root meant every newly generated contract was refused with PATH_OUTSIDE_STORAGE — a worse failure than the bug being fixed, and only on the installs the fix was for. The configured root is now allowed alongside the cwd one, which stays for contracts written before the move; their absolute paths are in the database and still resolve. Note the sibling root on the next line already honoured STORAGE_PATH, so the helper was half-migrated already. persistSignatureImage() still wrote customer and admin signature PNGs under process.cwd(). It was missed because its path.join is spread over seven lines while the others are single-line — and the regression test compared against the single-line literal, so it reported green over a live bug. The test now collapses whitespace before matching, which is the only reason a formatting difference ever hid this. A sweep of the whole of src/ with the same normalisation confirms the remaining process.cwd()/storage references are all deliberate `STORAGE_PATH || cwd` fallbacks, not misses. Added a case that drives assertContractPdfPath against real files on disk — the guard realpaths both the file and its roots, so a test using imaginary paths proves nothing. It fails without the fix. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(storage): resolve the contract guard's root through the shared resolver The guard still built its own `STORAGE_PATH || <cwd>/storage`. That matches getStoragePath() only while STORAGE_PATH is set — with it unset the shared resolver falls back module-relative to <repo>/storage while this fell back to <cwd>/storage, and the backend is normally started from backend/, so the two name different directories. Writers and guard then disagreed about where contracts live and the download routes refused them, which is the same failure the previous commit fixed for the configured case, reappearing in the fallback case. One resolver on both sides now, which is the point of the whole change. Docblock updated to describe the three roots as they actually are. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc * fix(storage): make the fallback test safe, and align the backup diagnostics The test added in the previous commit was dangerous. To exercise the STORAGE_PATH-unset case it deleted process.env.STORAGE_PATH and then, in cleanup, recursively removed `<resolved root>/business-docs` — which with the variable unset resolves to the developer's real, gitignored <repo>/storage. Running `npm test` in a working checkout would have destroyed local business documents. This checkout has 65 MB there, including a populated business-docs tree. Rewritten to mock the shared resolver instead. That is both safe (every path stays in the tmpdir) and a sharper assertion: if the guard consumes getStoragePath() the mock moves its root, and if it went back to rolling its own expression the mock would have no effect and the test fails — which is exactly the regression being pinned. backupCoverageService and backupIntegrityService kept their own `STORAGE_PATH || cwd` roots. The backup walker itself already falls back module-relative, so with the variable unset the two diagnostics inspected a directory neither the walker nor the writers use and would report the business-docs tree as missing while it was in fact being backed up. Both now use the shared resolver. No regression: the same jest invocation over contract/quote/invoice/pdf/ backup suites gives an identical 11 failed, 24 passed before and after — those failures are a locally missing cron-parser dependency and reproduce on an unmodified tree. Claude-Session: https://claude.ai/code/session_01Ra4hcsYiKuQLbbRsg6EjAc --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-18 22:14:41 +02:00
Paul Nothaft	d62e21c1ba	chore(main): release 3.105.1-beta.0 (#1066)	2026-08-16 19:41:03 +00:00
Paul NothaftandPaul Nothaft	52db982661	fix(gallery): make per-event banner overrides actually work, both banners (#440, #932) (#1064) ... * fix(gallery): make per-event banner overrides actually work, both banners (#440, #932) The promo banner shipped with a per-event inherit/custom/off override that never reached a guest. GalleryView reads promo_mode from the /photos payload, and /photos never sent it — so every gallery resolved to 'inherit'. Setting a gallery's promo banner to "Off" did nothing; the global banner kept rendering. The info banner (#932) mirrored that shape and inherited the same gaps. Four places dropped the fields; all four now carry both banners: 1. GET /gallery/:slug/photos — send promo_mode/promo_markdown alongside the info fields. This is the fix that makes "Off" mean off. 2. POST /admin/events — the validators accepted both banners and the insert discarded them, so an API client could POST info_mode:'off', get 201, and find the row on 'inherit'. Markdown is stored only for 'custom', matching the PUT rule. 3. POST /admin/events/:id/duplicate — copy both from the source row. The dialog promises the copy "inherits the branding, behaviour, feedback, and category configuration"; a muted gallery un-muting on duplication is the opposite of that. 4. PUT /admin/events/:id — resolve the effective mode from the STORED row when a partial update sends only the markdown. Previously updates.promo_mode was undefined on such a request and the text was parked on an inherit/off gallery, then resurfaced when someone later switched it to 'custom'. The lookup is lazy: one extra query, only on that path. The two normalisation blocks are now one loop over both banners, so the pair can't drift apart again. Verified in a browser, both directions against the same global banner: promo_mode='off' -> not rendered; 'inherit' -> rendered. The /photos payload went from promo_mode ABSENT to carrying the value. * fix(gallery): thread promo into the reveal view, drop stale markdown on duplicate External review, round 1 on this PR. Two gaps in the plumbing it introduced: - The reveal-hidden branch copied only the info fields from /photos. Now that /photos carries promo too, a reveal-hidden gallery with promo_mode 'off' still fell back to 'inherit' and showed the global banner on the first load after login. Thread both banners there. - The duplicate copied markdown verbatim. A row written before the PUT normalisation landed can hold text while its mode is 'inherit'/'off', so the copy inherited hidden text that would resurface the moment someone switched it to 'custom' — violating the very invariant this PR establishes. Copy markdown only when the source mode is 'custom'. Test covers the stale-markdown source explicitly. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-16 21:37:17 +02:00
Paul Nothaft	180f19d70d	chore(main): release 3.105.0-beta.0 (#1065)	2026-08-16 19:36:29 +00:00
Paul NothaftandPaul Nothaft	b48fa62eea	feat(gallery): info banner above the photo grid (#932) (#1063) ... * feat(gallery): info banner above the photo grid (#932) A short informational note rendered at the TOP of a gallery, above the photos. Distinct from the promotional banner (#440), which stays by the footer for marketing copy — the reporter's case is an onboarding hint ("use the menu button to filter"), which is useless below a gallery the guest has to scroll past first. Mirrors the promo feature's shape rather than inventing a second one: a global default in Settings → Branding (branding_info_markdown) plus a per-event inherit/custom/off override. Markdown via the existing MarkdownContent sanitiser — no raw HTML, no CSS injection. Empty global default means nothing renders, so upgrading changes nothing visible. Deliberately NOT included: an alignment knob (this is short helper copy, not marketing layout) and guest dismissal — the issue lists dismissal as a nice-to-have, and it needs per-guest persistence that is its own decision. Migration 176 is idempotent (hasColumn / existing-key guarded). Note on the payload plumbing: the per-event fields travel in the /photos response, not just /info. GalleryAuthContext seeds its cached event from the gallery LOGIN response — a small identity subset — so anything absent there is undefined right after a guest signs in. /photos is the payload that refreshes on every gallery load, which is why the fields were added there and why GalleryView reads them from `data.event`. Verified in a browser across all three modes; reading them from the context event instead silently collapsed every override back to 'inherit'. * fix(branding): map branding_info_markdown on read so saving can't wipe it (#932) External review caught this. BrandingSettings declared no info_markdown and formatBrandingSettings never mapped branding_info_markdown, so BrandingPage's hydration — setBrandingSettings(prev => ({ ...prev, ...formatted })) — kept the empty-string initializer instead of the persisted value. The form loaded blank and the next Save posted '' back, wiping a configured banner. Silently: the gallery keeps rendering the old copy until that save lands. This is the same bug the footer/promo fields hit in #441 + #440 / #460, which the read mapper still carries a comment about. Add the field to the interface and the mapper, and pin the round-trip for the whole editable branding set so the next field added is caught by a test rather than by a user losing copy. Verified: the new test fails 3/4 with the mapper line removed. * fix(gallery): honour the info-banner override in the reveal-hidden view (#932) External review, round 2. The hidden-until-reveal branch renders GalleryLayout with the context `event`, which is seeded from the gallery login response and carries no banner fields — so while a gallery was hidden, a per-event 'off' silently resolved to 'inherit' and the global banner appeared on a gallery the admin had muted. Resolve the fields there the same way the main render path does. The two full-page layouts (gallery-premium, gallery-story) are deliberately left alone: they return before GalleryLayout and render no header, footer or promo banner either — injecting a wrapper into layouts documented as having 'their own integrated UI' would be a design change, not a fix. --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-16 21:33:13 +02:00
Paul Nothaft	ab6ca6f82f	chore(main): release 3.104.1-beta.0 (#1061)	2026-08-16 17:34:09 +00:00
Paul NothaftandPaul Nothaft	3a11e6ebb5	fix(pdf): RFC 6266-encode Content-Disposition on quote/invoice PDFs (#1024) (#1055) ... * fix(pdf): RFC 6266-encode Content-Disposition on quote/invoice PDFs (#1024) The six quote/invoice PDF endpoints interpolated buildPdfFilename()'s result straight into `inline; filename="${filename}"`. That result deliberately preserves non-ASCII (it doubles as the PDF's internal Title metadata), and HTTP header values are latin1 — so a customer label reaching the header directly failed in one of two ways: - U+0080-U+00FF (ä ö ü ß — every German umlaut): no throw. The raw byte goes out and the client reads back a mangled name. Silent corruption. - above U+00FF (Polish ł, Czech ř, Turkish ş, €, Cyrillic, CJK, emoji): Node's setHeader rejects it with ERR_INVALID_CHAR. The throw lands after the PDF buffer is already rendered, so the request 500s. Note this corrects the issue's diagnosis: it reported umlauts as the 500 case, but umlauts are inside latin1 and mangle rather than throw. Both symptoms share this root cause and both are fixed here. Route through buildContentDisposition() (utils/filenameSanitizer, already used by secureImages.js), which emits an ASCII fallback plus the RFC 5987 `filename*=UTF-8''…` form, so the unicode name survives in browsers and the header stays legal. Applied to all six sites: adminQuotes (persisted + preview), adminInvoices (persisted + preview), customer (quote + invoice). Also correct buildPdfFilename's docstring, which advertised the preserved non-ASCII as suitable for Content-Disposition — the exact misreading that produced these call sites. * test(pdf): pin the ASCII fallback for fully non-Latin customer names (#1024) A name written entirely in another script leaves the legacy filename= token with just the document number (Q-2026-0042_.pdf) — filename* carries the real name. That's the intended trade, but it's the token a client without RFC 5987 support actually saves, so assert it stays legal, non-empty and carries the document number rather than leaving it unpinned. * fix(pdf): don't split surrogate pairs when truncating the filename (#1024) Codex review caught this. sanitiseSegment caps each segment at 80 UTF-16 code units, so a cap landing inside an astral character (emoji, rarer CJK) left a dangling high surrogate. encodeURIComponent throws URIError: URI malformed on a lone surrogate, so buildContentDisposition — the helper this PR routes the six PDF endpoints through — 500'd for e.g. company_name = 'a'.repeat(79)+'🎉', well inside the 120-char validator limit. Same 500 the PR set out to remove, reached a different way. Drop the orphaned surrogate instead of widening the cap, so the byte budget the limit exists to protect is unchanged. Tests cover both boundary cases and assert the cap semantics; they fail against the previous slice(). --------- Co-authored-by: Paul Nothaft <[email protected]>	2026-08-16 19:31:19 +02:00