From 2ebfe659b271593b54e37fe44653a0feb0ea55d5 Mon Sep 17 00:00:00 2001 From: Thomas Camlong Date: Fri, 14 Aug 2026 11:16:55 +0200 Subject: [PATCH] revert(perf): drop the dockerode and docker-modem pnpm patches Both patches came out of this work and both are removed at the maintainer's request. The finding was real - ssh2's WASM heap costs +14.0 MiB RSS on require, and @grpc/grpc-js another 14.6 MiB across 61 files, neither of which a socket-based Docker install ever uses - but patching a dependency means every future dockerode or docker-modem bump fails until someone regenerates the patch, and that recurring cost is not worth roughly 25 MiB. The finding stays on record in SERVER-MEMORY.md because the fix belongs upstream: both packages require a module at file scope while using it in exactly one already-guarded branch, so a PR to each would fix it for every consumer with no patch to maintain here. The two remaining patches, @types/node-unifi and trpc-to-openapi, predate this work and are untouched. allowUnusedPatches was already set on dev and is left alone. Lockfile regenerated: patchedDependencies now lists only those two. Also sharpens the Cache Components entry. The SqliteError was simply an unmigrated build database, which is fixable - but feeding the build a database would be the wrong fix, because serverSetting is per-installation data on the user's volume and a shell prerendered against the build machine's database would bake one install's settings into every other install's first paint. The error was Next.js correctly reporting a request-time read sitting in the shell path. --- docs/perf/NEXT-STEPS.md | 2 +- docs/perf/README.md | 7 ++++++- docs/perf/REJECTED.md | 21 +++++++++++++++------ docs/perf/SERVER-MEMORY.md | 18 +++++++++++++++--- patches/docker-modem@5.0.7.patch | 27 --------------------------- patches/dockerode@5.0.1.patch | 25 ------------------------- pnpm-lock.yaml | 12 +++++------- pnpm-workspace.yaml | 2 -- 8 files changed, 42 insertions(+), 72 deletions(-) delete mode 100644 patches/docker-modem@5.0.7.patch delete mode 100644 patches/dockerode@5.0.1.patch diff --git a/docs/perf/NEXT-STEPS.md b/docs/perf/NEXT-STEPS.md index 21485c3b7..c1ce3eebd 100644 --- a/docs/perf/NEXT-STEPS.md +++ b/docs/perf/NEXT-STEPS.md @@ -94,7 +94,7 @@ lines-of-code to performance ratio. On the evidence, that set is: | Umami: stop fetching 100k raw events per refresh | 1 file | −26% peak, −93% transient buffers, largest allocation 46.4 → 2.4 MiB | | `serverExternalPackages` (mysql2/pg + 8 more) | 1 block | −21 MiB idle | | `content-visibility` on media-release cards | 4 lines | −59% image requests, −41% layout objects | -| ssh2 WASM heap (docker-modem patch) | 1 patch | −15 to −18 MiB external at every stage | +| ~~ssh2 WASM heap (docker-modem patch)~~ | ~~1 patch~~ | **dropped** — the gain was real (−15 to −18 MiB external) but patching a dependency breaks on every `dockerode`/`docker-modem` bump. Worth raising upstream instead: both packages require a module at file scope while using it in one already-guarded branch | | Reuse the certificate agent instead of one per request | 1 file | 5.11 → 1.69 ms/request, 60 → 2 TLS handshakes over 60 requests | | Stream proxied images instead of buffering | 2 files | −36% peak RSS on 30 concurrent 2 MiB images (116 → 74 MiB) | diff --git a/docs/perf/README.md b/docs/perf/README.md index 48dd2c0e2..1a99341d6 100644 --- a/docs/perf/README.md +++ b/docs/perf/README.md @@ -37,9 +37,14 @@ No before/after range overlaps at any stage. The cost is **+18% CPU** (solid, no | `preloadEntriesOnStart: false` | **1 line** | −52% anon at idle, 310 fewer files even at full load | | Umami: stop fetching 100k raw events per refresh | 1 file | −26% peak, −93% transient buffers | | `serverExternalPackages` (mysql2/pg + 8 more) | 1 block | −21 MiB idle | -| ssh2's 16 MiB WASM heap (docker-modem patch) | 1 patch | −15 to −18 MiB external | +| Reuse the certificate agent instead of one per request | 1 file | 5.11 → 1.69 ms/request, 60 → 2 TLS handshakes | +| Stream proxied images instead of buffering them | 2 files | −36% peak RSS under 30 concurrent 2 MiB images | | `content-visibility` on media-release cards | 4 lines | −59% image requests, −41% layout objects | +The ssh2 / `@grpc/grpc-js` finding (−15 to −18 MiB external) is deliberately **not** in this table: it +was implemented as two pnpm patches and then reverted, because patching a dependency breaks on every +upstream bump. The finding is recorded in [SERVER-MEMORY.md](./SERVER-MEMORY.md) and belongs upstream. + ## What is left, and why it is hard One lever has real headroom and it pays twice. Roughly **41 MiB of retained bundle source** and diff --git a/docs/perf/REJECTED.md b/docs/perf/REJECTED.md index 522ae7ad5..06d6722c3 100644 --- a/docs/perf/REJECTED.md +++ b/docs/perf/REJECTED.md @@ -31,12 +31,21 @@ const [session, user, serverSettings, colorScheme] = await Promise.all([ ]); ``` -For a self-hosted app whose database is created at runtime on the user's volume, there is nothing to -query during a build. Fixing it means deferring those reads into `` — but all four feed -providers that wrap `children` (`AuthProvider session=`, `SettingsProvider user=`), so deferring them -puts essentially the whole app inside the boundary and leaves a static shell of little more than -``. The build would report `◐` while shipping an empty shell, which is precisely the -failure the migration guide warns about. +The immediate cause is mundane: no migration had been run, so the build had no tables. That is fixable +— run the migration first and the build gets a database. + +**But making the error go away that way would be the wrong fix, and the error is the useful signal.** +If a build needs the database in order to prerender a route, the route's content is being decided at +build time rather than per request. For Homarr that is actively wrong: `serverSetting` is +per-installation data on the user's own volume, so a shell prerendered against the build machine's +database would bake one install's settings into every other install's first paint. The error is Next.js +correctly reporting that a request-time read sits in the shell path. + +The honest fix is therefore to defer those reads into ``, not to feed the build a database. +And that is where it stops being worth it: all four values feed providers that wrap `children` +(`AuthProvider session=`, `SettingsProvider user=`), so deferring them puts essentially the whole app +inside the boundary and leaves a static shell of little more than ``. The build would +report `◐` while shipping an empty shell — precisely the failure the migration guide warns about. **And the prize is small.** Measured against a running container, warm, five repeats: diff --git a/docs/perf/SERVER-MEMORY.md b/docs/perf/SERVER-MEMORY.md index 08b771d84..88b63d53d 100644 --- a/docs/perf/SERVER-MEMORY.md +++ b/docs/perf/SERVER-MEMORY.md @@ -456,9 +456,21 @@ Measured directly inside the image — requiring ssh2 and nothing else: heapUsed +2.6 MiB external +0.9 MiB rss +14.0 MiB ``` -Fixed with a pnpm patch moving that one require inside the branch that uses it. SSH-based -Docker hosts still work — `require` is cached, so they pay it once — and socket-based installs, -which is everyone else, never load it. +This was fixed with a pnpm patch moving that one require inside the branch that uses it, and the +patch has since been **removed** — see below. + +> **Reverted, by decision.** Two pnpm patches came out of this work: `docker-modem@5.0.7` (the ssh2 +> require above) and `dockerode@5.0.1` (the same shape, hiding `@grpc/grpc-js` — 14.6 MiB across 61 +> files — behind `dialWithSession`). Both were dropped at the maintainer's request: patching a +> dependency means every future `dockerode` or `docker-modem` bump fails until someone regenerates the +> patch, and that recurring cost was judged not worth roughly 25 MiB of RSS. +> +> The finding itself stands and is worth keeping on record, because the fix belongs **upstream**: both +> packages require a module at file scope while using it in exactly one already-guarded branch. A PR to +> `docker-modem` and `dockerode` would fix it for every consumer, with no patch to maintain here. +> +> The two patches that remain — `@types/node-unifi` and `trpc-to-openapi` — predate this work and are +> untouched. ### The same question asked of every dependency diff --git a/patches/docker-modem@5.0.7.patch b/patches/docker-modem@5.0.7.patch deleted file mode 100644 index e325c7582..000000000 --- a/patches/docker-modem@5.0.7.patch +++ /dev/null @@ -1,27 +0,0 @@ -diff --git a/lib/modem.js b/lib/modem.js -index b45a798d63647d55e0193fd7e82c2e36785d9b5f..d2dbf42969fc0ebf39b30c1a27373c33c2d20333 100644 ---- a/lib/modem.js -+++ b/lib/modem.js -@@ -3,7 +3,6 @@ var querystring = require('querystring'), - fs = require('fs'), - path = require('path'), - url = require('url'), -- ssh = require('./ssh'), - HttpDuplex = require('./http_duplex'), - debug = require('debug')('modem'), - utils = require('./utils'), -@@ -263,8 +262,13 @@ Modem.prototype.buildRequest = function (options, context, data, callback) { - var connectionTimeoutTimer; - var finished = false; - -+ // Loaded here rather than at module scope: './ssh' pulls in ssh2, whose crypto module -+ // instantiates a ~16 MiB WebAssembly memory for ChaCha20-Poly1305 as soon as it is -+ // required. Measured at +14 MiB RSS for a require alone, paid by every Homarr install -+ // even though Docker is reached over a socket and this branch never runs. require() is -+ // cached, so SSH connections pay it once and behave exactly as before. - var opts = self.protocol === 'ssh' ? Object.assign(options, { -- agent: ssh(Object.assign({}, self.sshOptions, { -+ agent: require('./ssh')(Object.assign({}, self.sshOptions, { - 'host': self.host, - 'port': self.port, - 'username': self.username, diff --git a/patches/dockerode@5.0.1.patch b/patches/dockerode@5.0.1.patch deleted file mode 100644 index 353b63fe6..000000000 --- a/patches/dockerode@5.0.1.patch +++ /dev/null @@ -1,25 +0,0 @@ -diff --git a/lib/docker.js b/lib/docker.js -index 4951f8db867b1b12b75af79be2ebae6165534960..b333e65e99a9ef420826c0b1a908d4923dffc2f6 100644 ---- a/lib/docker.js -+++ b/lib/docker.js -@@ -12,7 +12,6 @@ var EventEmitter = require('events').EventEmitter, - Node = require('./node'), - Exec = require('./exec'), - util = require('./util'), -- withSession = require('./session'), - extend = util.extend; - - var Docker = function(opts) { -@@ -301,6 +300,12 @@ Docker.prototype.buildImage = function(file, opts, callback) { - - function dialWithSession(callback) { - if (opts?.version === "2") { -+ // Loaded here rather than at module scope: './session' pulls in @grpc/grpc-js and -+ // @grpc/proto-loader (112 files), which cost +6.0 MiB RSS on top of dockerode itself. -+ // Only the BuildKit image-build path reaches this branch, so listing or starting -+ // containers paid for a gRPC stack it never used. require() is cached, so builds behave -+ // exactly as before. -+ var withSession = require('./session'); - withSession(self, optsf.authconfig,(err, sessionId, done) => { - if (err) { - return callback(err); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c8b8e46ee..0f31e95af 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -585,8 +585,6 @@ overrides: patchedDependencies: '@types/node-unifi': 5e6ae51e2a17a7f9729bfa30b0eb3d0842a5810ac6db47603ab4a6efa1ed84c5 - docker-modem@5.0.7: 4a1c50dc8b726af921a34a77c4942a768bed229123ab116baa06c4e62ad6815c - dockerode@5.0.1: 0f6e47505aa4a9aa6fabe6a42b070ca984c787d197bcac3aa6367961f2f7eac3 trpc-to-openapi: 2ca3c16af0fcca0c736697ad4fe553a14f794524fa9ce0d5c3e8ee4aea76090c importers: @@ -1782,7 +1780,7 @@ importers: version: link:../core dockerode: specifier: 'catalog:' - version: 5.0.1(patch_hash=0f6e47505aa4a9aa6fabe6a42b070ca984c787d197bcac3aa6367961f2f7eac3)(supports-color@10.2.2) + version: 5.0.1(supports-color@10.2.2) devDependencies: '@homarr/tsconfig': specifier: workspace:^0.1.0 @@ -23648,7 +23646,7 @@ snapshots: dependencies: yaml: 2.9.0 - docker-modem@5.0.7(patch_hash=4a1c50dc8b726af921a34a77c4942a768bed229123ab116baa06c4e62ad6815c)(supports-color@10.2.2): + docker-modem@5.0.7(supports-color@10.2.2): dependencies: debug: 4.4.3(supports-color@10.2.2) readable-stream: 3.6.2 @@ -23657,12 +23655,12 @@ snapshots: transitivePeerDependencies: - supports-color - dockerode@5.0.1(patch_hash=0f6e47505aa4a9aa6fabe6a42b070ca984c787d197bcac3aa6367961f2f7eac3)(supports-color@10.2.2): + dockerode@5.0.1(supports-color@10.2.2): dependencies: '@balena/dockerignore': 1.0.2 '@grpc/grpc-js': 1.12.5 '@grpc/proto-loader': 0.7.13 - docker-modem: 5.0.7(patch_hash=4a1c50dc8b726af921a34a77c4942a768bed229123ab116baa06c4e62ad6815c)(supports-color@10.2.2) + docker-modem: 5.0.7(supports-color@10.2.2) protobufjs: 7.4.0 tar-fs: 2.1.4 transitivePeerDependencies: @@ -29220,7 +29218,7 @@ snapshots: byline: 5.0.0 debug: 4.4.3(supports-color@10.2.2) docker-compose: 1.4.2 - dockerode: 5.0.1(patch_hash=0f6e47505aa4a9aa6fabe6a42b070ca984c787d197bcac3aa6367961f2f7eac3)(supports-color@10.2.2) + dockerode: 5.0.1(supports-color@10.2.2) get-port: 5.1.1 proper-lockfile: 4.1.2 properties-reader: 3.0.1(supports-color@10.2.2) diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index 995b68e33..d6e554ed6 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -233,8 +233,6 @@ overrides: "vite@>=5.0.0 <=5.4.18": ">=8.1.4" patchedDependencies: "@types/node-unifi": "patches/@types__node-unifi.patch" - docker-modem@5.0.7: patches/docker-modem@5.0.7.patch - dockerode@5.0.1: patches/dockerode@5.0.1.patch "trpc-to-openapi": "patches/trpc-to-openapi.patch" allowUnusedPatches: true