stop losing the answer when immich rejects mid-upload
An upload over a few MB came back as a 400 with an empty body, no message anywhere, and nothing logged by Immich, the sidecar or officer. It was a race, not a size limit. Immich judges an asset from its first few KB and rejects immediately, then closes; both our hops were still writing the body; Node treats the leftover bytes as a protocol violation and replaces the application's answer with a bodyless `400 Bad Request` + `Connection: close`. The real message never reached the wire. Measured before the change: streamed lost the message 1/4 at 8 MB and 4/4 at 32 MB — probability rising with size, which is why small photos usually worked and a phone's video never did. Both hops needed it. Fixing only the sidecar took 32 MB from 4/4 failing to 2/4, because the platform proxy was losing it one hop up. Bounded at 512 MB, above which the body streams exactly as before. That ceiling is not a refusal and is deliberately not a 413: a file Immich ACCEPTS is read to the end and never races, so a 4 GB video is unaffected. All that is given up above the cap is the error message on a file that was going to be rejected anyway. A first attempt refused over-cap uploads outright and would have broken the working 4 GB case to improve diagnosis of the doomed one. `bufferRequestBody` is opt-in and off by default: the vault and wallet proxies must keep streaming so a passphrase or macaroon never lands in the platform's heap. Also adds the proxy error logging that made this findable at all — status and two byte counts from headers, never the bodies. `responseBytes: "unknown"` is what exposed the stripped response. Verified live at 32/256 MB (buffered) and 640 MB (streamed, passes through). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -16,7 +16,8 @@ import { registerProxiedPrefix } from './proxied-prefixes';
|
||||
// the platform process is long-lived, restarts on every deploy and is the largest attack surface in the
|
||||
// system, while the bodies passing through carry wallet unlock passphrases, macaroons and vault secrets.
|
||||
// Keeping this layer structurally incapable of seeing them is the point. Note the absence of any body
|
||||
// inspection below, and that the error path logs the target path ONLY.
|
||||
// inspection below: the two error paths log a path, a status and byte counts taken from HEADERS, and
|
||||
// nothing ever reads or buffers the streams themselves.
|
||||
//
|
||||
// Sidecars own their own contracts. This file must never grow per-app logic — if a sidecar needs
|
||||
// something extra at registration time it gets `onRegister`, not a branch in here.
|
||||
@@ -35,8 +36,30 @@ export type SidecarProxyParams = {
|
||||
* of a sidecar's routes are slow.
|
||||
*/
|
||||
timeoutSeconds?: number;
|
||||
/**
|
||||
* Read the request body fully before forwarding, instead of streaming it.
|
||||
*
|
||||
* OFF by default and it must stay that way: streaming is what keeps wallet passphrases, macaroons and
|
||||
* vault secrets out of this process's heap, and what lets a multi-gigabyte upload through at all.
|
||||
*
|
||||
* Opt in only where a sidecar's upstream answers BEFORE the body finishes arriving. When that happens
|
||||
* the response is lost — the far side closes while we are still writing, and the real answer is
|
||||
* replaced by a bodyless 400. Photos hits this because Immich judges an asset from its first few KB
|
||||
* and rejects immediately; see `readBodyCapped` in sidecar/photos/routes.ts for the measurements.
|
||||
*
|
||||
* Bounded by `MAX_BUFFERED_BODY`: a body larger than that is streamed anyway, so enabling this can
|
||||
* never put a 4 GB video in the heap. Above the cap the race is accepted, because an upload that size
|
||||
* only loses its ERROR MESSAGE — one Immich accepts is read to the end and never races at all.
|
||||
*/
|
||||
bufferRequestBody?: boolean;
|
||||
};
|
||||
|
||||
/**
|
||||
* Ceiling on `bufferRequestBody`. Kept in step with the photos sidecar's own cap so the two hops make the
|
||||
* same call on the same request; officer buffering is what gives the sidecar a content-length to decide by.
|
||||
*/
|
||||
const MAX_BUFFERED_BODY = 512 * 1024 * 1024;
|
||||
|
||||
export type SidecarProxy = {
|
||||
/** Catch-all router to mount at `prefix`. Has no routes of its own by design. */
|
||||
router: Hono<{ Variables: HonoVariables }>;
|
||||
@@ -46,7 +69,13 @@ export type SidecarProxy = {
|
||||
getWsUrl: () => string | null;
|
||||
};
|
||||
|
||||
export function createSidecarProxy({ name, prefix, onRegister, timeoutSeconds }: SidecarProxyParams): SidecarProxy {
|
||||
export function createSidecarProxy({
|
||||
name,
|
||||
prefix,
|
||||
onRegister,
|
||||
timeoutSeconds,
|
||||
bufferRequestBody,
|
||||
}: SidecarProxyParams): SidecarProxy {
|
||||
let serverPort: number | null = null;
|
||||
|
||||
// Tells bodyParser to keep its hands off this prefix's request bodies. Load-bearing for uploads, not an
|
||||
@@ -106,10 +135,23 @@ export function createSidecarProxy({ name, prefix, onRegister, timeoutSeconds }:
|
||||
// is absent from TypeScript's RequestInit, hence the widened type.
|
||||
// This is only safe because bodyParser skips proxied prefixes: a body consumed upstream would arrive
|
||||
// here as an already-locked stream.
|
||||
// Buffered only where the sidecar opted in (see `bufferRequestBody`), and only up to the cap: an
|
||||
// upstream that answers before the body lands loses its own response, and streaming is what exposes
|
||||
// that race. Anything larger streams as it always did — a 4 GB video must not land in this heap, and
|
||||
// does not need to, since a file the upstream ACCEPTS is read to the end and never races.
|
||||
const declaredLength = Number(ctx.req.header('content-length') ?? '');
|
||||
const withinBufferCap =
|
||||
Number.isFinite(declaredLength) && declaredLength > 0 && declaredLength <= MAX_BUFFERED_BODY;
|
||||
const body = hasBody
|
||||
? bufferRequestBody && withinBufferCap
|
||||
? await ctx.req.raw.arrayBuffer()
|
||||
: ctx.req.raw.body
|
||||
: undefined;
|
||||
|
||||
const init: RequestInit & { duplex?: 'half' } = {
|
||||
method,
|
||||
headers,
|
||||
body: hasBody ? ctx.req.raw.body : undefined,
|
||||
body,
|
||||
duplex: 'half',
|
||||
};
|
||||
|
||||
@@ -122,6 +164,29 @@ export function createSidecarProxy({ name, prefix, onRegister, timeoutSeconds }:
|
||||
return ctx.text(`${name} sidecar unreachable`, 502);
|
||||
}
|
||||
|
||||
// Every rejection that passes through gets one line. This is METADATA ONLY — status and two byte
|
||||
// counts read from headers, never the bytes themselves — so the no-bodies rule above still holds.
|
||||
//
|
||||
// It exists because of COMMS/UPLOAD_EMPTY_400.md: photo uploads over ~16 KB failed with a 400 carrying
|
||||
// an EMPTY body, and nothing anywhere logged it — not Immich, not the sidecar, not here. The platform
|
||||
// is the only hop that sees both sides, and it said nothing, so localising the fault took an evening of
|
||||
// curl bisection. An error with no body is the unactionable case: the client is told "no" and given
|
||||
// nothing to act on, and silence here makes it invisible to the server too.
|
||||
//
|
||||
// `requestBytes` is the field that would have cracked it: it makes a size threshold legible at a glance
|
||||
// instead of by bisection. The subpath is logged without its query string — the 502 path above logs the
|
||||
// full target, but this fires on ordinary 401s and 404s too, and query strings carry tokens.
|
||||
if (upstream.status >= 400) {
|
||||
const responseBytes = upstream.headers.get('content-length');
|
||||
console.warn(`[${name}] proxy ${method} ${subpath} -> ${upstream.status}`, {
|
||||
requestBytes: ctx.req.header('content-length') ?? 'unknown',
|
||||
responseBytes: responseBytes ?? 'unknown',
|
||||
// Called out rather than left to be inferred from `responseBytes: '0'`, because this exact shape is
|
||||
// the one that is impossible to diagnose from the client.
|
||||
...(responseBytes === '0' ? { emptyBody: true } : {}),
|
||||
});
|
||||
}
|
||||
|
||||
return new Response(upstream.body, { status: upstream.status, headers: new Headers(upstream.headers) });
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user