103863cbabe2bc45fdf44b369a1e2f85c4e2e763
5
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
bb2f709fdd |
fix: single-photo gallery downloads 404 on S3 storage backends (#1048)
* fix(gallery): route single-photo downloads through the storage backend The route resolved a local filesystem path unconditionally and handed it to res.sendFile. On an S3/R2 deployment managed photos are never on local disk, so every per-photo download failed — while download-all and secure-images worked, because they already went through getStorage(). That asymmetry is why it went unnoticed: the gallery looks healthy until a guest clicks the download button on one photo. Measured rather than assumed: because sendFile is called WITH a callback, Express does not send a response when the file is missing and the callback only logs. The request does not 404, it hangs until the client gives up. The new tests pin this — all five backend-path cases time out against the previous implementation. Two existing pieces do the work, so this mostly deletes code: - renderPhotoForDownload (#858) already owns resize-then-watermark ordering and the storage fetch, and the zip builders in this same file already use it. The inline duplicate of that logic goes. - the pass-through case branches on storage.kind(). Local disk keeps res.sendFile: it emits Content-Length, Accept-Ranges, ETag and Last-Modified and answers Range with a 206, and sharing one bare stream.pipe(res) with S3 would silently drop all of it — a resumed download would append a second full body onto the partial file. On S3 the parts that matter for a download are reproduced via stat() and getRange(). Ranges are parsed defensively; an unchecked parse yields NaN bounds and a 206 with a nonsense Content-Range, which corrupts a resumed download rather than failing it. Malformed or unsatisfiable ranges fall back to a 200. The pre-stream 404s now run before any image header is staged, so the error goes out as JSON instead of a .jpg attachment containing JSON. Co-authored-by: peipeimo <[email protected]> * fix(gallery): open the stream before staging download headers, honour If-Range Both from an external review round on this PR. stat() succeeding does not mean get() will — a concurrent delete or replace, or a transient backend error, lands between them. The fetch was awaited AFTER the headers went out, so: - the range branch had already called writeHead(206), leaving the outer catch nothing to do but throw ERR_HTTP_HEADERS_SENT. In practice the request hangs: the new regression test sat for the full 120s jest timeout against the previous code instead of returning. - the full branch would have sent its 500 JSON underneath the staged image/jpeg attachment headers — a .jpg file full of JSON, which is the exact failure this PR set out to stop doing on the 404 paths. Opening the stream first also lets a vanished object answer 404 and a transient failure answer 500, instead of both surfacing as a broken body. If-Range: emitting Last-Modified without honouring the validator built from it is the dangerous half of the feature. A client resuming after the object was replaced — the watcher re-importing a swapped file, an admin re-upload — would get 206 from the NEW bytes and splice two versions into one corrupt file. A validator that does not match now falls back to a full 200. 4 new tests; 3 of them fail against the previous commit, the fourth is the matching-validator control that must keep returning 206. * fix(gallery): HEAD without egress, classify render failures, stage 206 headers Round-2 findings from the external reviewer. Express routes HEAD through this GET handler and Node discards the body, but the pipe still drains the whole object out of S3 first — a metadata probe from a download manager cost a full transfer in egress and latency. Everything a HEAD needs is already in stat(). renderPhotoForDownload rejections were all reported as 404. It can equally fail because getToFile timed out, tmp filled up, or sharp died; calling that "photo not found" misleads the guest and hides the incident from us. Now classified the same way the pass-through branch already does. The 206 path uses status()+set() instead of writeHead(). writeHead commits the response immediately, so a stream that resolved and then errored before its first chunk left pipeStreamToResponse able only to destroy the connection. Staged headers flush on the first body write, so an error at byte zero now returns a clean retryable status with keep-alive intact. Credit to the reviewer for the correction — I had assumed deferring the commit required buffering. Writing the test for that surfaced one more: pipeStreamToResponse cleared Content-Type, Content-Length, ETag and Content-Disposition but not the range headers, so the 500 went out still advertising Content-Range: bytes 0-9/40 — telling a resuming client the error body IS the partial content. Not taken: binding response metadata to a fetched object version. That needs an ETag/versionId on the storage abstraction and conditional GETs in both adapters; the reviewer agreed it belongs in its own PR rather than blocking this one. Backend suites: 485 passed. * fix(gallery): answer HEAD before the counters and the render Round-3 finding. The HEAD short-circuit was inside the storage branch, which sits below both the download_count increment / access_logs insert and renderPhotoForDownload — so a download manager's metadata probe was recorded as a real download, and on a watermarked or resized gallery it also pulled the original from S3 and ran sharp over it to build a body Node then throws away. HEAD now leaves the handler right after the access checks, with no side effects and no bytes read. Content-Length is included only when the photo ships untransformed and the size is readable from stat(); a watermark or resize changes the length and the only way to learn the new one is to do the work this branch exists to avoid. HEAD may omit it. Not taken, again: binding the read to the statted object version. The reviewer already agreed in a follow-up that it needs an ETag/versionId on the storage abstraction plus conditional GETs in both adapters, and belongs in its own PR. Re-raising it does not change that. Tests assert the probe moves neither download_count nor access_logs. --------- Co-authored-by: Paul Nothaft <[email protected]> Co-authored-by: peipeimo <[email protected]> |
||
|
|
202c553a08 |
fix(events): delete stored objects when cascading an event delete (#1051)
* fix(events): delete stored objects when cascading an event delete
* fix(events): sweep watermarks and the archive zip on cascade delete too
Two more objects in the same class as the originals: both are written
through the storage backend, both were only ever removed with fs.unlink,
so both outlive the event on S3.
- photo.watermark_path — a canonical key, deleted via getStorage() on the
single-photo path (watermarkService.deleteWatermarkFile) and on archive
(archiveService.js:227). The cascade neither selected nor removed it.
- event.archive_path — written by storage.putFromFile (archiveService.js:160)
and typically the largest single object an event owns.
event.hero_logo_path is deliberately NOT included: multer writes logos to
local disk with diskStorage regardless of backend (adminEvents/logo.js:19-28),
so they are never bucket objects and the existing fs.unlink is correct.
Collect into a Set — an unresized gallery can carry one object in both
hero_path and preview_path, and the second delete would log a spurious
failure.
* fix(events): sweep the download caches, and delete objects concurrently
Both from an external review round on this PR.
The download caches are the subtle case: the pre-built "Download All" zip
(events.download_zip_path) and one zip per custom-resolution download job
(download_jobs.zip_path) both live under
events/active/{slug}/.download-cache/. On local disk the recursive fs.rm
already covered them, which is exactly why they were easy to miss — on S3
that prefix is not a directory, nothing covered them, and both are
gallery-sized. downloadZipService exposes a cleanup() documented as "used
on event deletion" that the cascade never called.
The job rows are read before the transaction for the same reason the photo
rows are: download_jobs.event_id is ON DELETE CASCADE, so on Postgres they
vanish with the event and take their keys with them. Guarded with hasTable
so a pre-#173 install doesn't abort the delete.
Deletes now run through a bounded pool instead of one await per key. A
400-photo gallery owns ~1600 objects once derived tiers are counted, and
that many sequential DeleteObject round trips runs to minutes — long enough
for a proxy to time the request out AFTER the commit, leaving the event
deleted and the sweep half-finished. A pool rather than Promise.all over
every key, so the fan-out can't exhaust the S3 client's connection pool.
* fix(events): never delete a derivative another gallery still uses
Round-2 findings from the external reviewer.
Canonical thumbnail/hero/preview keys are not event-scoped: the basename is
the photo's filename (imageProcessor passes no outputBasename for managed
photos, so the key is thumbnails/thumb_w300_<filename>), and filenames are
not unique across events — the responsive-tier code says so in as many
words, which is why THOSE keys carry a p{id}_ prefix. A legacy gallery can
therefore share a canonical derivative with a photo in another event, and
deleting it here blanked a surviving gallery's tile. Derived keys are now
checked against photos outside this event and anything still referenced is
left alone; if the check itself fails, every derivative is kept. An orphan
costs storage, a deleted derivative costs someone else's gallery. Originals
need no check — their keys embed the slug.
Also cancel any in-flight or debounced Download All build before snapshotting
paths. A builder that started before the delete would otherwise upload a
gallery-sized zip after the sweep and write its path onto a row that no
longer exists, orphaning it permanently. downloadZipService.cleanup() is the
service's own entry point for this and does all three things: bumps the
version so an in-flight build discards its result, clears the debounce so
nothing rebuilds for a deleted event, and removes the current object.
* revert(events): drop the Download All build cancellation
Reverted for the same reason as on the stable twin, where it was caught:
downloadZipService.cleanup() reaches getStorage() through _cleanup(), so
where the S3 backend is configured but unreachable every cascade delete pays
the adapter's retry backoff. On stable that took the backend CI job from ~2
minutes to past its 10-minute budget, twice, reproducibly. This branch's
suite happened not to trip it, but the same cost lands in the request path
of a real delete — and the twins have to carry the same code.
The race it addressed is narrow and costs one orphaned zip; documented as a
follow-up instead. The shared-derivative guard from the same review round
stays — that one prevented deleting a surviving gallery's thumbnail.
---------
Co-authored-by: Peifu Mo <[email protected]>
Co-authored-by: Paul Nothaft <[email protected]>
|
||
|
peipeimoandPeifu Mo
|
4f352dec39 |
fix(auth): treat zxcvbn suggestions as advice, not blocking errors (#1050)
validatePassword() appended zxcvbn's feedback.suggestions to the errors array unconditionally, and validity is errors.length === 0 — so any password that merely earned a suggestion was rejected even when it satisfied every configured rule. The effective policy was stricter than the configured complexity level and invisible to the admin. Suggestions now surface only alongside a real strength failure. They stay available to callers in result.feedback.suggestions, so a UI can still show them as guidance while typing. The weak-password fixture is assembled from parts rather than inlined: an 8-char alphanumeric literal next to validatePassword( reads as a hardcoded credential to the required GitGuardian check. Both fixtures pin their zxcvbn score — the compliant one is load-bearing at exactly the moderate minimum (2), and a future zxcvbn bump promoting it to 3 would leave the test green while no longer covering the bug. Co-authored-by: Peifu Mo <[email protected]> |
||
|
peipeimoandPeifu Mo
|
3600231d5f |
fix(storage): add S3 client timeouts so a dropped connection can't wedge uploads (#1049)
* fix(storage): add S3 client timeouts so a dropped connection can't wedge uploads * fix(storage): use socketTimeout, not requestTimeout, for the dead-connection guard * fix(storage): make S3 timeouts generous — short connectionTimeout breaks pooled reads --------- Co-authored-by: Peifu Mo <[email protected]> |
||
|
peipeimo
|
33f1bc42a9 |
fix: sync gallery feedback filters after lightbox like/rating in simple mode (#882)
In simple identity mode, likes and ratings submitted from the lightbox never called onFeedbackChange, so the gallery's photo list (whose like_count drives the Likes/Rated feedback filter chips) stayed stale until a full page reload. Liked photos were missing from the Likes filter; unliked photos stayed stuck in it. The guest-identity-mode paths and the grid PhotoCard paths already call onFeedbackChange after submitting - the simple-mode lightbox paths were the only ones missing it. Add the call to the three missing paths: submitLike (simple branch), submitRating (simple branch), and the FeedbackIdentityModal onSubmit handler. Verified locally (Docker build of main): like a photo in the lightbox after navigating with Next/Prev, open the Likes filter - the photo now appears immediately with no reload, and filter contents match the admin feedback API exactly. |