[05/40] Open the database once, and compile a statement once by cerede2000 · Pull Request #446 · nxzai/NextExplorer

cerede2000 · 2026-09-26T18:50:55Z

P3-14 of phase 3 (see #373). Stacked on #433–#445.

Four connections at every start

const getDb = async () => {
  if (dbInstance) return dbInstance;
  …                      // opens, migrates, assigns
};

The check and the assignment are an await apart, and everything that starts with the
server asks at once — the session store, the settings, the trash sweep, the search index,
the favourites. Each caller that arrives before the first has finished sees "not open" and
opens another. So a start opens app.db four times, runs migrate over the same file four
times in parallel, and whichever finishes last becomes the one everybody uses.

One opening is shared now: openDb does the work, getDb hands every caller the same
promise while it is in flight and the same connection afterwards, and closeDb lets a test
take it away — which is what a suite that opens a database per case needs.

A statement compiled per row

db.prepare compiles the SQL every time it is called, and the hot paths call it per row: a
listing asking whether each of a thousand entries is a favourite compiles the same statement
a thousand times.

prepared(db, sql) keeps it, in a WeakMap keyed on the connection — so the cache goes
when the connection does, and a statement is never handed to a connection that did not
compile it. That second part is the bug a cache keyed on the SQL alone would have.

Checks

db-single-open.test.js, 5 tests. Five callers in one turn get one connection, counted
on the connection objects rather than on anything the code says about itself. A later caller
gets the one already open. After closeDb, the next caller gets a new one and it works. The
same SQL twice compiles once; the same SQL against a new connection compiles again.

Putting the per-caller opening back turns the first red.

Whole backend suite: 2 561 passed, 2 failed — the two that fail on main alone.
Formatting and lint unchanged in kind.

Where `main` was ahead

betterSqliteSessionStore.js and bootstrap.js were in this batch and are not any more.
main's versions are the newer ones: the session database opens on first use rather than
when the module is required, which is what keeps node -e "require('./backend/src/app.js')"
— the check your CI runs — working where the cache directory does not exist yet. Bringing
the fork's versions over them would have broken it.

That case now has a name in the fork's own parity manifest, so it cannot be mistaken for
work again.

The batch is reversed, so its findings move from PORT to DONE in the same commit that sends them. What is left to reverse is whatever `scripts/parity.mjs` still reports.

Two routes built a command line with values from the request in it and gave the line to a shell. `routes/usage.js` pasted a folder's path into `du -sb "…"` and `df -Pk "…"`; `routes/permissions.js` pasted an owner, a group, a mode and a path into `chown`, `chgrp`, `chmod -R` and two `id` lookups. A name is not a shell string. Anything that closes the quoting leaves the rest for `/bin/sh` to run, as the user the server runs as. The usage one needs no privilege at all: any account that can create a folder can name one, and where AUTH_ENABLED is false that is anybody who can reach the server. `execFile` takes the arguments as a list, so there is no line for a shell to read and no shell. The commands, their output and the answers are unchanged — this is deliberately the smallest change that closes it, and not the rewrite that stands in nxzai#450 and nxzai#453. An argument list is not a free pass on its own: `chown` reads a leading dash as an option, so `--reference=/etc/shadow` would have copied another file's ownership onto the target. An account or group name has to look like one. `tests/routes/no-shell.test.js` — three cases. Two of them name a folder, and an owner, with a payload that creates a file, and check the file is not there; the third asks for `--reference=/etc/shadow` and expects a refusal. All three fail against the routes as they are, the first two by running the command. The payload only ever touches the working directory, and it is removed whatever an assertion does, so a run that does execute leaves nothing behind.

Two things about OIDC that this fixes. **Which of two failures it was.** A sign-in that cannot start has one of two causes: nothing was configured, or what was configured could not be made to work. The first is answered by filling in OIDC_ISSUER and the rest; the second is answered by looking at the provider. Both answered 404 "OIDC is not configured", so an administrator whose provider was unreachable was sent to change a configuration that was already right. `configureOidc` now records what it concluded and why, and the routes read it: 404 AUTH_OIDC_NOT_CONFIGURED for the first, 503 AUTH_OIDC_PROVIDER_UNAVAILABLE for the second, and neither carries the network's own words — no ENOTFOUND, no internal host name, in the body or in the address bar. A browser asking for one of those addresses is looking at a page, not reading JSON, so it is sent back to the sign-in screen with the code beside the sentence. The screen can then say what the code means in the reader's own language. **Where the callback comes back to.** The return address was built from a fixed `baseURL`, so a deployment reached through a reverse proxy on a different name sent people back to the wrong origin. It is now resolved from the request — but only from a forwarded host behind a trusted proxy, and only when the result exactly matches an origin the operator configured. A redirect target that a request header could choose is an open redirect with extra steps. The path is still held to a relative, same-site one, as before. Also here, because it is the same files: - `claimsFromIdToken` in the middleware and `uniqueOrigins` in the routes were local copies of things `utils/idToken.js` and the new `utils/oidcRedirect.js` do; both go. - The guest-session cookie was cleared on `/api` only, at five sign-in and sign-out paths. A guest session left on `/` outlived the sign-in that should have ended it. - `ServiceUnavailableError` (503) and the two AUTH_OIDC_* codes, which nothing had yet. The account lockout that `routes/auth.js` also differs in is deliberately not here: it is a different subject and goes with releasing a locked account. ## Checks Seven test files, 107 tests, including three this fork had and `main` did not: `oidc-middleware`, `oidcOrigin` and `auth-oidc-routes`. Neutralising `getOidcAvailability` so it reports one verdict for both causes turns three of them red — the three that tell the two apart. Whole backend suite: 2 391 passed, 2 failed, the two that fail on `main` on its own (`auth.test.js` on the current password, `browse-hidden-files.test.js`). `npm run lint` reports 146 against `main`'s 143, the three being the parse error on `backend/tests/**` that 141 of `main`'s own test files already draw. Formatting clean. Frontend builds, backend loads, documentation site builds. One test assertion was rewritten rather than ported as it stood: it matched the wording `express-openid-connect` uses for a callback with no sign-in in progress, and that wording differs between 2.19 and 2.20. It now asserts the refusal. One box, one name for it ------------------------ The sign-in box takes an email address or a username, so it is neither: it is whatever was typed, and this calls it `identifier` from the screen to the route. The screen and the client were renamed and the store and the route were not, so the store passed `email` to a client expecting `identifier`. `JSON.stringify` drops a key whose value is undefined, and the request went out carrying a password and nobody to sign in. The answer was "invalid credentials", which is what a wrong password looks like — so nothing about it read as a defect. `attemptLocalLogin` already took `identifier`; the route is what had not caught up. `email` and `username` still work, for a script or an older client that sends them. The identifier was the half that failed loudly. The screen also reads `totpPending`, `oidcStatus`, `cancelTotp`, `ensureStatus` and `forgetSession` off the store, and none of them were there: `totpPending` read undefined, so the box for the code from the authenticator never appeared, and a correct password on an account with a second factor landed on a screen that looked like it had done nothing. Undefined is not an error in a template — it is a `v-if` that is false. The store is here in full, with the code step read back from the server on every start so a reload in the middle of one lands back on the code. tests/routes/sign-in-identifier.test.js signs in with each of the three names, refuses a wrong password, and reads the three frontend files to check they all use the one name — the chain is four files long and three of them have no runner here, which is how it broke silently in the middle. And what a refusal says ----------------------- A code that names the kind of refusal — FORBIDDEN, NOT_FOUND, CONFLICT, RATE_LIMIT_EXCEEDED — is translated for the reader, and the server's own sentence, which says *which* refusal, went underneath rather than being lost. A lock that arrives with a duration gets a sentence of its own rather than a placeholder in the plain one, so it can never read "{minutes}". And the handler no longer asks vue-i18n for a key before checking it has it, which was a console warning for every refusal the catalogue has no entry for, twice.

Two reports the server makes about itself at start, neither of which existed. **What releases up to 1.1.7 left in the cache.** The database and app-config.json lived in CACHE_DIR until 1.1.8, which moved them to CONFIG_DIR and left links behind; 2.0.3 removed that move from the entrypoint. So an installation that started on 1.1.7 or earlier and skipped the releases in between comes up on a new, empty app.db in CONFIG_DIR with its accounts, shares and settings sitting unread in the cache. Nothing said so: the server started, the sign-in page offered to create the first administrator, and the answer looked like a fresh installation rather than a lost one. It is a notice and nothing more — nothing is moved, nothing is deleted. It names what it found and where, and says what to do with it. **What the process is costing.** `services/performanceDiagnostics.js` samples CPU, resident memory as the cgroup sees it rather than as the host does, event-loop delay at p99, and the queues that can grow: thumbnails, folder sizes, transfers. Off unless PERFORMANCE_DIAGNOSTICS_ENABLED is set, and then it reports only the intervals that pass a threshold — a diagnostic that logs every interval by default is a diagnostic that fills a disk. PERFORMANCE_DIAGNOSTICS_LOG_EVERY_INTERVAL asks for all of them. An interval below its floor is held to the default: a sampler on a 1 ms interval costs more than whatever it was meant to diagnose, and 1 ms is what an emptied field sends. Each queue reports itself through an optional call. One that has no report is a queue this installation has nothing to say about, not a reason for the whole record to fail — a diagnostic that throws says nothing at the moment it is most wanted. ## Checks `legacy-cache-check.test.js`, 3 tests: a cache holding the old names is named, one holding the links 1.1.8 left is named differently, and an ordinary cache says nothing. Making the inspection always see an empty cache turns all three red. `performance-diagnostics.test.js`, 7 tests: silent unless asked for, says what it will watch and by which thresholds, holds an interval below its floor to the default, reports nothing of an ordinary interval, reports one that passes a threshold and says which kind it was, reports every interval when told to, and samples the machine rather than guessing. Making it start whether or not it was asked for turns the first red. Whole backend suite: 2 556 passed, 2 failed — the two that fail on `main` on its own.

`/api/features` already offers `preview.maxRenderBytes` to the screen, and the configuration had no such value, so it answered `null` and the preview rendered whatever it was given. A document large enough to render slowly renders slowly for everybody on the machine, and nobody could raise or lower the point at which it stops trying. PREVIEW_MAX_RENDER_SIZE sets it; 16 MB when it is not set, and a value below zero or unparseable is that default rather than a ceiling of nothing. And the listing: `GET /api/browse` answers with what is true at that moment — which documents somebody has open in an editor, what a folder weighs, whether a write would be refused. A GET with no cache header is cacheable by default, so a proxy or a browser was free to keep it and serve a folder as it was: a deleted file still listed, a document shown as open by somebody who closed it an hour ago. `private, no-store` says what it is. ## Checks `browse-caching.test.js` asserts both halves of the header, and taking the header away turns it red. The ceiling is read through `/api/features`, which has offered the field since the batch that brought the features route and was answering `null` for it. Whole backend suite: 2 556 passed, 2 failed — the two that fail on `main` on its own.

`getDb` checks whether a connection is already open and opens one when it is not. Every caller that arrives before the first has finished sees "not open" and opens another — and everything that starts with the server asks at once: the session store, the settings, the trash sweep, the search index, the favourites. So a start opened app.db four times, ran `migrate` over the same file four times in parallel, and whichever finished last became the one everybody used. One opening is shared now. `openDb` does the work, `getDb` hands every caller the same promise while it is in flight and the same connection afterwards, and `closeDb` lets a test take it away again — which is what a suite opening a database per case needs. And `prepared(db, sql)`: `db.prepare` compiles the SQL every time it is called, and the hot paths called it per row — a listing asking whether each of a thousand entries is a favourite compiled the same statement a thousand times. Kept in a WeakMap keyed on the connection, so the cache goes when the connection does and a statement is never handed to a connection that did not compile it. ## Checks `db-single-open.test.js`, 5 tests. Five callers in one turn get one connection — counted on the connection objects rather than on anything the code says about itself. A later caller gets the one already open. After `closeDb` the next caller gets a new, working one. The same SQL twice compiles once, and the same SQL against a new connection compiles again. Putting the per-caller opening back turns the first red. Whole backend suite: 2 561 passed, 2 failed — the two that fail on `main` on its own. Two files were in this batch and are not: `betterSqliteSessionStore.js` and `bootstrap.js`. `main`'s versions are the newer ones — it opens the session database on first use rather than when the module is required, which is what keeps `require('./backend/src/app.js')` working where the cache directory does not exist yet. Bringing the fork's versions over them would have broken that check.

A sign-in refused for too many attempts answered "invalid credentials" — the same sentence as a wrong password. Somebody locked out was told they had typed their password wrong, and typing it right did not help, and nothing said why or for how long. It says so now, with the time left, and an administrator can see which accounts are locked and lift a lock without waiting it out. The accounts screens come with it: the list says who is locked and until when, the panel for one account says whether it is and offers to unlock it, and a notice about the server's configuration can be dismissed. Two tests that were failing on main before this batch now pass, and the suite is green end to end for the first time. Neither was a product defect; both were tests that could not do what they meant to. `auth.test.js` removed app.db between apps without closing it. SQLite keeps an unlinked file alive for whoever still holds it open, so the second app in the file went on reading the first one's accounts through the old handle: `/auth/setup` answered "already configured", and a test about a wrong password failed on its first call for a reason that had nothing to do with passwords. It closes the database first — through the instance that has it open, since a fresh one has never opened anything. `closeDb` arrived with #446. `browse-hidden-files.test.js` wrote a preference for the account id `admin`, which nothing had created. A preference belongs to an account — `user_settings` says so with a foreign key — so it failed on the constraint. The account is created before the preference is written. Whole suite: 2853 passed, 0 failed.

Each batch so far carried the tests for what it changed. These are the rest: 119 files covering what was already here or what those batches added, and the eight helpers and one fixture they need — a fake 7-Zip, a soft authenticator, a legacy database builder, a half-red-half-blue HEIC. The suite goes from 2904 tests to 3935. Nine of the fork's test files were left behind on purpose, each because it asserts something this repository deliberately does differently: - the search catalogue answering for a file that is no longer on disk, where this repository stats the page of results and drops what is gone (#452); - the session store opening its database when the module is required, where this repository opens it on first use (#446); - the schema numbers, which are this repository's own and not the fork's; - the chunked maintenance pass, whose subject has not been sent. And the fork's `tests/scripts/` stay in the fork: they test its own repository tooling — a commit guard, an install script, an image pruner — not this application. `services/indexDb.js` says why it may remove a file directly: the index is built from the volume and made again whenever it is missing, so it does not go through the trash. It was the one thing in #463 that the deletion rule had not been told about. Whole suite: 3935 passed, 0 failed.

cerede2000 mentioned this pull request Sep 26, 2026

[06/40] Remember a folder's sort and view as rows, not as one value per account #447

Merged

cerede2000 force-pushed the p3-14 branch 3 times, most recently from 509e479 to 8175a85 Compare September 26, 2026 22:10

This was referenced Sep 27, 2026

[13/40] Say that an account is locked out, and let an administrator unlock it #454

Merged

[25/40] Bring the tests for everything the last twenty batches sent #466

Merged

cerede2000 force-pushed the p3-14 branch from 8175a85 to b9f1a0b Compare September 27, 2026 12:29

Benjy added 5 commits September 27, 2026 19:24

cerede2000 force-pushed the p3-14 branch from b9f1a0b to dd0bf16 Compare September 27, 2026 17:28

cerede2000 changed the title ~~Open the database once, and compile a statement once~~ [05/40] Open the database once, and compile a statement once Sep 27, 2026

vikramsoni2 merged commit 5c33bf2 into nxzai:main Sep 27, 2026
1 check passed

Navigation Menu

[05/40] Open the database once, and compile a statement once - #446

[05/40] Open the database once, and compile a statement once#446
vikramsoni2 merged 5 commits into
nxzai:mainfrom
cerede2000:p3-14

cerede2000 commented Sep 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

cerede2000 commented Sep 26, 2026

Four connections at every start

A statement compiled per row

Checks

Where main was ahead

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Where `main` was ahead