* fix(gallery): route single-photo downloads through the storage backend
Stable twin of #1048. 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.
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 current implementation.
- watermark branch: materialize a tmp local copy via withLocalCopy in S3
mode and hand applyWatermark the copy's PATH, so its path-keyed cache
still applies. Same pattern the zip builders in this file already use.
- pass-through branch: local disk keeps res.sendFile, which emits
Content-Length, Accept-Ranges, ETag and Last-Modified and answers Range
with a 206. Sharing one bare stream.pipe(res) with S3 would silently drop
all of it, and a resumed download would append a second full body onto
the partial file. On S3 the parts that matter are reproduced via stat()
and getRange().
- external/reference photos keep the local-path fallback unchanged —
resolvePhotoStorageKey returns null for them.
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.
Written against stable's shape rather than cherry-picked — main's version
delegates to renderPhotoForDownload (#858), which does not exist here.
Co-authored-by: Peifu Mo <peipeimo@users.noreply.github.com>
* fix(gallery): open the stream before staging download headers, honour If-Range
Both from an external review round on #1048.
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) and the outer catch could only throw ERR_HTTP_HEADERS_SENT
(in practice the request hangs), while the full branch would have sent its
500 JSON underneath the staged image/jpeg attachment headers.
Opening the stream first also lets a vanished object answer 404 and a
transient failure answer 500.
If-Range: emitting Last-Modified without honouring the validator built from
it is the dangerous half. A client resuming after the object was replaced
would get 206 from the NEW bytes and splice two versions into one corrupt
file. A non-matching validator now falls back to a full 200.
* fix(gallery): HEAD without egress, classify render failures, stage 206 headers
Round-2 findings from the external reviewer on #1048, ported.
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
cost a full transfer in egress and latency. Everything a HEAD needs is
already in stat().
The watermark branch reported every failure 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.
The 206 path uses status()+set() instead of writeHead(), which commits
immediately and left a stream erroring at byte zero with no outcome but a
destroyed connection. Staged headers flush on first write, so that case now
returns a clean retryable status.
pipeStreamToResponse also cleared Content-Type, Content-Length, ETag and
Content-Disposition but not the range headers, so the 500 went out still
advertising Content-Range — telling a resuming client the error body IS the
partial content.
* fix(gallery): answer HEAD before the counters
Round-3 finding on #1048, ported. The HEAD short-circuit was inside the
storage branch, below both the download_count increment / access_logs insert
and the watermark path — so a download manager's metadata probe counted as a
real download, and on a watermarked gallery it also pulled the original from
S3 and ran sharp over it to build a body Node then discards.
HEAD now leaves right after the access checks. Content-Length is included
only when the photo ships untransformed and the size is readable from stat();
a watermark changes the length and the only way to learn it is to do the work
this branch exists to avoid.
Uses stable's inline watermark resolution — resolveWatermarkSettings comes
from downloadRendition (#858), which does not exist on this branch.
---------
Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local>
Co-authored-by: Peifu Mo <peipeimo@users.noreply.github.com>
Paul Nothaft
5470fbe406