Skip to content

refactor(server): let the S3 backend own its proxy read slot and route storage I/O through the backend - #1159

Merged
Deeds67 merged 3 commits into
open-noodle:mainfrom
luminoso:refactor/storage-s3-stream-lifecycle
Oct 6, 2026
Merged

Deeds67 merged 3 commits into
open-noodle:mainfrom
luminoso:refactor/storage-s3-stream-lifecycle

Conversation

@luminoso

@luminoso luminoso commented Oct 4, 2026

Copy link
Copy Markdown

Summary

The S3 proxy read slot has leaked three times, and each fix landed in utils/file.ts rather than the backend: #497 (reverted in #499), #1104 (the idle timer) and #1106 (the client gone before the stream exists). The slot was freed only if the HTTP layer destroyed the stream on every exit path, which the StorageBackend interface never said. One leak was still open: a throw in sendFile after the stream existed but before pipe left the stream undestroyed and its slot held. The S3 adapter now owns the slot from acquire to release, so sendFile only has to destroy a source it stops piping. Services also stop branching on path shape where the backend can do the work.

How the slot lifetime works

sendFile creates an AbortController that aborts when the response closes, before the handler runs, and passes its signal to the handler through an AsyncLocalStorage (getResponseSignal()). serveFromBackend forwards it as ServeOptions.signal. In proxy mode the S3 adapter rejects a read whose client already left, both before queueing for a slot and after getting one. It passes the signal to GetObject, and wraps the body in a Transform that runs the idle watchdog, joined to the body with pipeline and to the signal with addAbortSignal. The slot is released on the Transform's close, which follows end, error, consumer destroy, abort and idle alike. sendFile registers "destroy the source when the response closes" before setting any header, so a throw before pipe is covered as well: its error response closes the response.

Changes

  • S3 backend (backends/s3-storage.backend.ts): owns the slot as above. S3_STREAM_IDLE_TIMEOUT_MS and the zombied-socket explanation moved here from utils/file.ts. Adds readAll.
  • HTTP layer (utils/file.ts): the response signal and its AsyncLocalStorage. The S3 idle timer is gone. An AbortError that follows the client leaving is no longer logged or answered.
  • Interface (interfaces/storage-backend.interface.ts): ServeOptions.signal, readAll, and an optional { readable } on exists.
  • Disk backend (backends/disk-storage.backend.ts): exists, readAll and delete go through StorageRepository, the calls they replace in upstream code paths, so upstream specs that mock the repository still apply. A relative key with no media location now throws instead of resolving against the working directory.
  • Services: BaseService.backendFor(key) picks the backend for a path. Absolute paths get a disk backend over the service's own StorageRepository; relative keys go through StorageService.resolveBackendForKey. ensureLocalFile, getProbeInput, serveFromBackend, the metadata sidecar check and sidecar download, and StorageService.handleDeleteFiles use it.
  • Notification and the machine-learning repository share readAll instead of two hand-written read-into-a-buffer loops.
  • Specs: the idle timeout tests moved from file.spec.ts to s3-storage.backend.spec.ts, which asserts that active slots return to 0. The ML and metadata specs dropped their disk-versus-S3 routing pairs. The MinIO integration spec gained two tests and now uses the same pinned Chainguard image as the storage-migration e2e, since MinIO withdrew its own images.

isAbsolute sites

Site Outcome Reason
base.service ensureLocalFile Deleted Disk downloadToTemp already returns the absolute path with a no-op cleanup
base.service getProbeInput Deleted Disk getReadableUrl already returns the absolute path
metadata handleMetadataExtraction sidecar download Collapsed ensureLocalFile handles both path shapes
metadata handleSidecarCheck Collapsed backend.exists(candidate, { readable: true }) keeps upstream's R_OK check on disk
machine-learning.repository getFormData Collapsed readAll on either backend; disk reads through readFile as upstream does
storage.service handleDeleteFiles Collapsed Disk delete is upstream's storageRepository.unlink
base.service backendFor New, single dispatch The one place a service picks the backend from the path shape
metadata motion photo path choice Kept, policy Chooses where a new motion video is placed
media handleAssetMigration Kept, policy fs.rename migration does not apply to S3 keys
person handlePersonMigration Kept, policy Same as above for face thumbnails
storage-template job and moveAsset (2) Kept, policy Storage templates do not apply to S3 keys
asset trim external-library check Kept, policy Blocks trimming external-library files
metadata handleSidecarWrite Kept, local path exiftool writes to a local file
download downloadArchive Kept, local path archiver adds disk files by real path
notification album thumbnail Kept, local path The mailer attaches a disk file by path; only the S3 side changed, to readAll
metadata motion video write Kept, disk write Disk creates the file only if it is new, after ensureFolders, unlike an S3 put
asset copySidecar Kept, disk write Disk uses copyFile; collapsing would change it to a stream copy

Behaviour changes

  • The leak is fixed: a throw in sendFile between getting the stream and piping it no longer leaves the slot held.
  • A client that leaves while its proxied read is queued or in flight now aborts the GetObject and frees the slot immediately, instead of after the fetch completes.
  • When the client has already gone, an AbortError from that read is dropped silently. Previously it was logged as Unable to send file and answered with a 404 on a dead response. An AbortError while the response is still open is still logged and answered.
  • Disk delete now goes through StorageRepository.unlink, which logs and ignores a file that is already gone. Its callers, handleDeleteFiles and the storage migration's source delete, already caught that error and logged a warning, so the outcome is the same.
  • Disk streams served as stream responses (face thumbnail crops) no longer get the S3 idle timer, which never applied to them in practice.
  • Each proxied read now passes through a Transform. A stalled client can buffer up to about 128 KiB more per slot than a direct pipe did. That is a bounded cost and is what lets the idle watchdog see chunks without consuming the body.

Testing

  • Unit, pnpm test -- --run: 201 files passed, 6445 tests passed.
  • pnpm lint (zero warnings) and pnpm check: clean.
  • MinIO integration spec on podman (DOCKER_HOST=unix:///run/user/1002/podman/podman.sock TESTCONTAINERS_RYUK_DISABLED=true IMMICH_TEST_DOCKER=true npx vitest --config test/vitest.config.mjs --run src/backends/s3-storage.backend.integration.spec.ts): 14/14 passed locally. CI never runs this spec, because IMMICH_TEST_DOCKER is not set in any workflow.
  • Medium metadata, storage, asset-media, asset, download, user and person specs: all passed. The full medium suite had 36 failures in exif, library and workflow-core-plugin, which also fail on main. It also had timeouts in people-identity-rbac, memory and database-migration under load; those three pass 150/150 run on their own.
  • Mutation checks, each killed by at least one test: no release on close, no wait-time abort check, no pre-queue abort check, no addAbortSignal, no SDK abortSignal, sendFile registering the destroy-on-close after the headers, a re-arm that always fires, pipe instead of pipeline (the no-unhandled-error test), the AbortError suppression removed or ignoring whether the response closed, disk exists always requiring readability (the storage-migration test), and the relative-key guard removed.

Follow-ups

  • Replace the static backend state in StorageService and the remaining lazy imports with an injected provider. Deferred because specs touch the static state 88 times and spy on resolveBackendForKey about 50 times, so the change would mostly be spec rewrites across upstream-derived spec files. backendFor already cuts the lazy imports from 5 to 3.
  • Run s3-storage.backend.integration.spec.ts in CI with IMMICH_TEST_DOCKER=true. It is the only test that releases slots against a real HTTP body, and the storage-migration e2e already pulls the same pinned MinIO image.

@Deeds67
Deeds67 force-pushed the refactor/storage-s3-stream-lifecycle branch from 7cb56e7 to 23d1949 Compare October 6, 2026 06:51
@Deeds67 Deeds67 added the changelog:chore Chore/maintenance for changelog label Oct 6, 2026
luminoso and others added 3 commits October 6, 2026 09:21
…torage I/O through the backend

The proxy read slot used to be freed only if sendFile destroyed the stream on
every exit path, an obligation the StorageBackend interface never stated. A throw
between the res.destroyed check and pipe (setting a header) left the stream
undestroyed and its slot held.

- ServeOptions gains `signal`, aborted when the response closes. sendFile hands it
  to the handler through an AsyncLocalStorage, and serveFromBackend passes it on.
- The S3 adapter aborts GetObject on it, skips a read whose client left while it
  waited for a slot, and returns a stream that releases its slot on end, error,
  destroy, abort and idle. The idle watchdog and S3_STREAM_IDLE_TIMEOUT_MS move
  from utils/file.ts into the backend.
- sendFile keeps only the generic rule, registered before any header is set:
  destroy the source when the response closes.
- StorageBackend gains readAll; the disk adapter's exists, readAll and delete go
  through StorageRepository, the calls they replace in upstream code paths.
- BaseService.backendFor resolves the backend for a path. ensureLocalFile and
  getProbeInput lose their redundant isAbsolute branches; the sidecar check, the
  sidecar download, file deletion and ML image reads stop branching on path
  shape; notification and ML share readAll instead of hand-written buffering.
- The MinIO integration spec moves to the pinned Chainguard image, since MinIO
  withdrew its own.
…am edge cases

Review follow-up.

- StorageBackend.exists takes an optional { readable }. Disk checks existence
  by default, so the storage migration still fails loudly on a source that
  exists but cannot be read instead of skipping it as missing; the sidecar check
  asks for readable, keeping upstream's R_OK. A storage-migration test pins it.
- The S3 proxy read rejects a request whose client already left before it
  queues for a slot, not only after getting one.
- A disk backend with no media location refuses a relative key instead of
  resolving it against the working directory.
- Tests for sendFile dropping an AbortError once the client left and still
  reporting one while the response is open, and for an idle destroy never
  becoming an unhandled 'error' on a stream that is only piped.
- Comments on what supplies that error listener, on the ML repository's
  dependency on StorageService bootstrap, and on what getResponseSignal returns
  outside sendFile.
Once the S3 body has ended into the slot-holding Transform, pipeline drops
its listeners, and pipe() adds none to its source. The thumbnail endpoint
resolves its stream before sendFile, so no response signal is attached
either. A client that stops reading but keeps the socket open for the idle
window then gets an 'error' with no listener: an uncaught exception that
exits the worker. Listen for it on the Transform; the error stays on
stream.errored and the slot is still released on 'close'.
@Deeds67
Deeds67 force-pushed the refactor/storage-s3-stream-lifecycle branch from 23d1949 to b818070 Compare October 6, 2026 07:22
@Deeds67
Deeds67 merged commit f047d92 into open-noodle:main Oct 6, 2026
2 checks passed
Deeds67 added a commit that referenced this pull request Oct 6, 2026
…e storage I/O through the backend (#1159)

* refactor(server): let the S3 backend own its proxy read slot, route storage I/O through the backend

The proxy read slot used to be freed only if sendFile destroyed the stream on
every exit path, an obligation the StorageBackend interface never stated. A throw
between the res.destroyed check and pipe (setting a header) left the stream
undestroyed and its slot held.

- ServeOptions gains `signal`, aborted when the response closes. sendFile hands it
  to the handler through an AsyncLocalStorage, and serveFromBackend passes it on.
- The S3 adapter aborts GetObject on it, skips a read whose client left while it
  waited for a slot, and returns a stream that releases its slot on end, error,
  destroy, abort and idle. The idle watchdog and S3_STREAM_IDLE_TIMEOUT_MS move
  from utils/file.ts into the backend.
- sendFile keeps only the generic rule, registered before any header is set:
  destroy the source when the response closes.
- StorageBackend gains readAll; the disk adapter's exists, readAll and delete go
  through StorageRepository, the calls they replace in upstream code paths.
- BaseService.backendFor resolves the backend for a path. ensureLocalFile and
  getProbeInput lose their redundant isAbsolute branches; the sidecar check, the
  sidecar download, file deletion and ML image reads stop branching on path
  shape; notification and ML share readAll instead of hand-written buffering.
- The MinIO integration spec moves to the pinned Chainguard image, since MinIO
  withdrew its own.

* refactor(server): keep disk exists semantics per caller, pin the stream edge cases

Review follow-up.

- StorageBackend.exists takes an optional { readable }. Disk checks existence
  by default, so the storage migration still fails loudly on a source that
  exists but cannot be read instead of skipping it as missing; the sidecar check
  asks for readable, keeping upstream's R_OK. A storage-migration test pins it.
- The S3 proxy read rejects a request whose client already left before it
  queues for a slot, not only after getting one.
- A disk backend with no media location refuses a relative key instead of
  resolving it against the working directory.
- Tests for sendFile dropping an AbortError once the client left and still
  reporting one while the response is open, and for an idle destroy never
  becoming an unhandled 'error' on a stream that is only piped.
- Comments on what supplies that error listener, on the ML repository's
  dependency on StorageService bootstrap, and on what getResponseSignal returns
  outside sendFile.

* fix(server): keep an idle S3 proxy destroy from crashing the process

Once the S3 body has ended into the slot-holding Transform, pipeline drops
its listeners, and pipe() adds none to its source. The thumbnail endpoint
resolves its stream before sendFile, so no response signal is attached
either. A client that stops reading but keeps the socket open for the idle
window then gets an 'error' with no listener: an uncaught exception that
exits the worker. Listen for it on the Transform; the error stays on
stream.errored and the slot is still released on 'close'.

---------

Co-authored-by: Pierre Marais <pierremarais67@gmail.com>
Deeds67 added a commit that referenced this pull request Oct 6, 2026
Deeds67 added a commit that referenced this pull request Oct 6, 2026
…e storage I/O through the backend (#1159)

* refactor(server): let the S3 backend own its proxy read slot, route storage I/O through the backend

The proxy read slot used to be freed only if sendFile destroyed the stream on
every exit path, an obligation the StorageBackend interface never stated. A throw
between the res.destroyed check and pipe (setting a header) left the stream
undestroyed and its slot held.

- ServeOptions gains `signal`, aborted when the response closes. sendFile hands it
  to the handler through an AsyncLocalStorage, and serveFromBackend passes it on.
- The S3 adapter aborts GetObject on it, skips a read whose client left while it
  waited for a slot, and returns a stream that releases its slot on end, error,
  destroy, abort and idle. The idle watchdog and S3_STREAM_IDLE_TIMEOUT_MS move
  from utils/file.ts into the backend.
- sendFile keeps only the generic rule, registered before any header is set:
  destroy the source when the response closes.
- StorageBackend gains readAll; the disk adapter's exists, readAll and delete go
  through StorageRepository, the calls they replace in upstream code paths.
- BaseService.backendFor resolves the backend for a path. ensureLocalFile and
  getProbeInput lose their redundant isAbsolute branches; the sidecar check, the
  sidecar download, file deletion and ML image reads stop branching on path
  shape; notification and ML share readAll instead of hand-written buffering.
- The MinIO integration spec moves to the pinned Chainguard image, since MinIO
  withdrew its own.

* refactor(server): keep disk exists semantics per caller, pin the stream edge cases

Review follow-up.

- StorageBackend.exists takes an optional { readable }. Disk checks existence
  by default, so the storage migration still fails loudly on a source that
  exists but cannot be read instead of skipping it as missing; the sidecar check
  asks for readable, keeping upstream's R_OK. A storage-migration test pins it.
- The S3 proxy read rejects a request whose client already left before it
  queues for a slot, not only after getting one.
- A disk backend with no media location refuses a relative key instead of
  resolving it against the working directory.
- Tests for sendFile dropping an AbortError once the client left and still
  reporting one while the response is open, and for an idle destroy never
  becoming an unhandled 'error' on a stream that is only piped.
- Comments on what supplies that error listener, on the ML repository's
  dependency on StorageService bootstrap, and on what getResponseSignal returns
  outside sendFile.

* fix(server): keep an idle S3 proxy destroy from crashing the process

Once the S3 body has ended into the slot-holding Transform, pipeline drops
its listeners, and pipe() adds none to its source. The thumbnail endpoint
resolves its stream before sendFile, so no response signal is attached
either. A client that stops reading but keeps the socket open for the idle
window then gets an 'error' with no listener: an uncaught exception that
exits the worker. Listen for it on the Transform; the error stays on
stream.errored and the slot is still released on 'close'.

---------

Co-authored-by: Pierre Marais <pierremarais67@gmail.com>
Deeds67 added a commit that referenced this pull request Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog:chore Chore/maintenance for changelog 🗄️server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants