diff --git a/.devcontainer/Dockerfile b/.devcontainer/Dockerfile index d609fa43f..7a3afaf68 100644 --- a/.devcontainer/Dockerfile +++ b/.devcontainer/Dockerfile @@ -24,20 +24,10 @@ RUN apt-get update && apt-get install -qy \ unzip \ wget -# Chromium, for the Puppeteer extension. Rendering a page on the server means driving a headless -# browser, and this is the copy Puppeteer is pointed at rather than one it downloads for itself: -# the distro keeps it patched, and it exists for arm64 as well as amd64. -# -# Its own RUN, and without recommends, because Chromium recommends a desktop stack -- an X server, a -# terminal emulator, usbmuxd -- that takes the package count from 61 to 177 and that nothing headless -# ever touches. The apt lists from the install above are still around, so no second update is needed. -RUN apt-get install -qy --no-install-recommends \ - chromium \ - fonts-liberation - -# Where Puppeteer looks for a browser, and that it must not fetch a second one on install -ENV PUPPETEER_SKIP_DOWNLOAD=true -ENV PUPPETEER_EXECUTABLE_PATH=/usr/bin/chromium +# The Puppeteer extension installs its own browser, in app-init.sh: the one Chrome release it is built +# against, where a distro Chromium moves on with every update and eventually stops being one Puppeteer +# can drive. Only the headless shell, which is all the renderer uses. +ENV PUPPETEER_CHROME_SKIP_DOWNLOAD=true # avoid million NPM install messages ENV npm_config_loglevel=warn diff --git a/.devcontainer/app-init.sh b/.devcontainer/app-init.sh index 50d24f2e4..2be9d0688 100644 --- a/.devcontainer/app-init.sh +++ b/.devcontainer/app-init.sh @@ -20,10 +20,19 @@ npm install # and a plain source checkout should not have to fetch it to install the backend. # # `--no-save` leaves package.json alone, which also means a later reinstall can prune it and turn -# server-side rendering back off -- run this line again if the admin area says it is missing. The -# browser itself is in the image, so this fetches no Chromium (see PUPPETEER_* in the Dockerfile). +# server-side rendering back off -- run this block again if the admin area says it is missing. The +# version is the extension definition's, as in the production image. +# +# Then the browser, which `browsers install` fetches if the postinstall did not, and the system +# libraries it lists for itself in `deb.deps` -- the `apt-get satisfy` that `--install-deps` runs, but +# under sudo alone rather than with Puppeteer running as root. echo "Installing the Puppeteer extension..." -npm install --no-save puppeteer@25.4.0 +PUPPETEER_VERSION="$(sed -n 's/^installVersion: *//p' modules/extensions/puppeteer/definition.yml)" +npm install --no-save "puppeteer@${PUPPETEER_VERSION}" +PUPPETEER_BROWSER="$(npx puppeteer browsers install chrome-headless-shell --format '{{path}}')" +sudo apt-get update -qq +sudo DEBIAN_FRONTEND=noninteractive apt-get satisfy -qy --no-install-recommends \ + "$(paste -sd, "$(dirname "$PUPPETEER_BROWSER")/deb.deps")" cd ../frontend npm install diff --git a/CLAUDE.md b/CLAUDE.md index 03b7a1e29..c13f3002e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -84,8 +84,13 @@ path in silence. `modules/authentication/local/`. `modules/storage/*` ships `db` and `disk` — see [Storage targets](#storage-targets). `modules/analytics/*` is the odd one out: a pair of YAML files and no implementation at all — see [Analytics](#analytics). -- `tasks/simple/` — jobs run in-process by the scheduler; each exports `task()`. File name is - kebab-case, the task key is its camelCase form. +- `tasks/simple/` — jobs run in-process by the scheduler; each exports `task(payload, { signal })`. + File name is kebab-case, the task key is its camelCase form. `scheduler.taskTimeout` applies here + as it does to workers, but a promise cannot be killed: at the timeout `signal` is aborted, and a + task that works in steps checks it between them and stops (`renderPages` hands the rest of its + queue to a fresh job, so a long re-render runs in slices). One that has not settled a minute later + is abandoned — failed, left running, and no second copy of that task starts on the instance until + it ends (`executeInProcess` in `core/scheduler.ts`). - `tasks/workers/` — CPU-bound jobs run in a worker thread via `worker.ts`, which boots a minimal `WIKI` global (config + logger + lazy `ensureDb()`) and dynamically imports the task. - `base.yml` — system defaults for every config key. Do not edit as a user-facing config; it defines @@ -270,8 +275,8 @@ Match the check to the size of the change. `npm run build`, `npx oxlint` and `np seconds each and are the right check for nearly everything. **Do not stand up a throwaway instance and drive a headless browser to look at a small change.** That -means booting a backend against a scratch database, seeding it, and screenshotting through -`/usr/bin/chromium` — a good ten minutes of setup that a moved border, a colour, a spacing tweak or a +means booting a backend against a scratch database, seeding it, and screenshotting through a +headless browser — a good ten minutes of setup that a moved border, a colour, a spacing tweak or a renamed label does not earn. Read the rule you wrote, trust the build, and say what you changed. It is worth the setup for a **new** piece of UI whose markup has to meet a stylesheet written @@ -296,9 +301,12 @@ Then `CONFIG_FILE=config.test.yml node --no-experimental-webstorage backend` **f `CONFIG_FILE` is resolved against `WIKI.ROOTPATH` (`core/config.ts`), so it is a path relative to the root and not to `backend/`. It seeds itself and takes ~25s to reach listening. -**Puppeteer is not installed in any workspace, and must not be added to one for a screenshot.** Install -`puppeteer-core` into a scratch directory instead and drive the browser already on the box: -`executablePath: '/usr/bin/chromium'`, `args: ['--no-sandbox']`. It pulls ~25 packages and downloads no +**Puppeteer must not be added to a workspace's `package.json` for a screenshot.** Install +`puppeteer-core` into a scratch directory instead and drive the browser already on the box: the +headless shell the dev container's Puppeteer extension installed, whose path +`npx puppeteer browsers install chrome-headless-shell --format '{{path}}'` prints from `backend/` +(it downloads nothing when the browser is there). Launch with that as `executablePath`, +`headless: 'shell'` and `args: ['--no-sandbox']`; `puppeteer-core` pulls ~25 packages and downloads no browser of its own. **Scripting the API rather than the browser**, which is the quicker way to get a page and a history in @@ -880,6 +888,13 @@ Everything else follows from that: dispatches the new bytes to every write target, since the copy a reader is served is usually the database's. Imported content is rendered with **no script or style permission** whoever ran the import, since the file need not have been written by them. +- **An import is never written back to the target it reads from.** What it adopts is saved like any + edit and so copied to every target holding it, and `git` and `sftp` serialize every operation per + target — with the import itself being one, a copy back to the source waits on the import that is + waiting on it. `importTree` runs under `storage.importingFrom(target)`, an `AsyncLocalStorage` + scope that `putAsset`, `eachPageTarget` and `eachAssetTarget` skip that target in (counting it, + for `putAsset`, as somewhere the file is held). Anything else a module does to the wiki while + holding its own queue — git's removals in `applyIncoming` — needs the same scope. - **On import a file is a page if its extension is reserved, or if it declares an `editor`** in its front matter. A text page is front matter plus the source; a **JSON** page — a redirection today — is one JSON document with the metadata at its top level and the source under `content`. diff --git a/backend/base.yml b/backend/base.yml index cd6d8e7fa..d0a40e47b 100644 --- a/backend/base.yml +++ b/backend/base.yml @@ -45,9 +45,11 @@ defaults: maxRetries: 2 retryBackoff: 60 historyExpiration: 90000 - # How long, in seconds, a job running in a worker thread may take before the scheduler stops - # waiting for it and counts it as failed. A safety net rather than a schedule: a worker thread - # that dies mid-task never answers at all, and without a ceiling the scheduler waits forever. + # How long, in seconds, a job may take before the scheduler stops waiting for it and counts it + # as failed. A safety net rather than a schedule: a worker thread that dies mid-task never + # answers at all, and an in-process task stuck on a lock or a dead browser never returns, and + # without a ceiling the scheduler waits forever. A worker is aborted at this point; an + # in-process task is asked to stop and, if it has not a minute later, abandoned. taskTimeout: 300 # How long, in seconds, a job may sit marked as running before it is assumed that whatever was # running it is gone, and it is requeued. Deliberately generous: it has to outlast the longest diff --git a/backend/core/scheduler.ts b/backend/core/scheduler.ts index 03f301312..468cf4138 100644 --- a/backend/core/scheduler.ts +++ b/backend/core/scheduler.ts @@ -17,8 +17,18 @@ import { import { and, eq, inArray, lt, sql } from 'drizzle-orm' import type { PoolClient } from 'pg' +/** What the scheduler hands an in-process task besides its payload. */ +export interface TaskContext { + /** + * Aborted when the task has run for `scheduler.taskTimeout`. A task cannot be stopped from outside + * the way a worker thread can, so this is a request: one that does its work in steps should check + * it between them and stop, and one that ignores it is abandoned — see `executeInProcess`. + */ + signal: AbortSignal +} + /** An in-process task, loaded from `tasks/simple/`. */ -export type SimpleTask = (payload?: any) => Promise | void +export type SimpleTask = (payload: any, context: TaskContext) => Promise | void /** Fallback for `scheduler.taskTimeout`, in seconds, when nothing is configured. */ const DEFAULT_TASK_TIMEOUT = 300 @@ -35,6 +45,25 @@ const DEFAULT_STALE_JOB_TIMEOUT = 3600 */ const TASK_TIMEOUT_GRACE = 5000 +/** + * How long an in-process task that has been asked to stop gets to do so before it is abandoned. + * + * Far longer than the worker's grace, because stopping here is cooperative: a task stops at the end + * of the step it is in — the page it is rendering, the target it is syncing — rather than the moment + * the signal fires, and one of those steps may itself take half a minute. + */ +const IN_PROCESS_STOP_GRACE = 60_000 + +/** + * In-process tasks that timed out without stopping, and when they were abandoned. + * + * Such a task is still running and nothing can end it, so no second copy of it is started on this + * instance until it settles. A task hangs on something — a lock, a connection, a dead browser — and a + * second copy almost always hangs on the same thing, each one taking a slot for the length of the + * timeout; a task scheduled every minute would fill the pool within a few of them. + */ +const abandonedTasks = new Map() + /** * Sends the scheduler's cross-instance notifications, one at a time. * @@ -267,6 +296,67 @@ export default { } }, + /** + * Run a job in this process, and stop waiting for it if it does not come back. + * + * The counterpart of `executeOnWorker`, and without it one task that never settles is fatal rather + * than slow: its slot is never given back, its history row says `active` for ever, and a task that + * is scheduled keeps queuing copies that wait on the same thing. + * + * There is no thread to abort, so the two ceilings mean something different here. At the timeout the + * task's signal is aborted, which a task that works in steps answers by stopping at the end of the + * current one. If it has not settled a grace period later it is abandoned: recorded as failed like a + * worker that never answered, and left running, since nothing in JavaScript can stop a promise. + */ + async executeInProcess(job: { task: string; payload?: any }): Promise { + const abandonedAt = abandonedTasks.get(job.task) + if (abandonedAt !== undefined) { + throw new Error( + `An earlier run of ${job.task} timed out ${Math.round((Temporal.Now.instant().epochMilliseconds - abandonedAt) / 1000)}s ago and is still running, so another is not being started alongside it.` + ) + } + + const timeoutSeconds = WIKI.config.scheduler.taskTimeout ?? DEFAULT_TASK_TIMEOUT + const controller = new AbortController() + const stopTimer = setTimeout(() => { + controller.abort( + new Error(`The task did not finish within ${timeoutSeconds}s and was asked to stop.`) + ) + }, timeoutSeconds * 1000) + let giveUpTimer: NodeJS.Timeout | undefined + + const run = Promise.resolve().then(() => + this.tasks![job.task](job.payload, { signal: controller.signal }) + ) + const abandon = () => { + abandonedTasks.set(job.task, Temporal.Now.instant().epochMilliseconds) + // -> Also what keeps a late rejection from surfacing as an unhandled one + void run + .then( + () => WIKI.logger.warn(`The abandoned run of ${job.task} has finished after all.`), + (err: any) => WIKI.logger.warn(`The abandoned run of ${job.task} failed: ${err.message}`) + ) + .finally(() => abandonedTasks.delete(job.task)) + return new Error( + `The task did not finish within ${timeoutSeconds}s, nor stop within ${IN_PROCESS_STOP_GRACE / 1000}s of being asked to. It is still running and cannot be stopped from here; no other run of it will start on this instance until it ends.` + ) + } + try { + await Promise.race([ + run, + new Promise((_resolve, reject) => { + giveUpTimer = setTimeout( + () => reject(abandon()), + timeoutSeconds * 1000 + IN_PROCESS_STOP_GRACE + ) + }) + ]) + } finally { + clearTimeout(stopTimer) + clearTimeout(giveUpTimer) + } + }, + /** * Take a batch of due jobs and run them. * @@ -359,7 +449,7 @@ export default { if (job.useWorker) { await this.executeOnWorker(job) } else { - await this.tasks![job.task](job.payload) + await this.executeInProcess(job) } await WIKI.db .update(jobHistoryTable) diff --git a/backend/helpers/storageFiles.ts b/backend/helpers/storageFiles.ts index e0a763757..376bfbd59 100644 --- a/backend/helpers/storageFiles.ts +++ b/backend/helpers/storageFiles.ts @@ -400,21 +400,30 @@ export interface ImportSummary { * @param readFile How to read one, for a tree that is not on this machine — the SFTP target hands in * its own and everything else about the walk is the same */ -export async function importTree({ - target, - root, - actorId, - overwrite, - files, - readFile = (filePath) => fs.readFile(filePath) -}: { +export function importTree(options: ImportTreeOptions): Promise { + // -> What is adopted here is saved like any edit, and so copied to every target holding it. Not to + // this one: the file is already there, and a module that serializes its operations would have + // the copy wait for this import to finish while the import waits for the copy. + return WIKI.models.storage.importingFrom(options.target, () => adoptTree(options)) +} + +interface ImportTreeOptions { target: StorageTarget root: string actorId: string overwrite: boolean files?: StoredFile[] | null readFile?: (filePath: string) => Promise -}): Promise { +} + +async function adoptTree({ + target, + root, + actorId, + overwrite, + files, + readFile = (filePath) => fs.readFile(filePath) +}: ImportTreeOptions): Promise { const found = files === undefined ? await walkStored(root) : files if (!found) { return null diff --git a/backend/models/extensions.ts b/backend/models/extensions.ts index 466241232..ded1dd103 100644 --- a/backend/models/extensions.ts +++ b/backend/models/extensions.ts @@ -226,11 +226,12 @@ class Extensions { * * Puppeteer is not declared anywhere, so this is a genuine first install, and the bulk of it is the * browser. Nothing has to be arranged for that: Puppeteer's own postinstall fetches one into its - * cache, which is the ordinary case and the one an install straight onto Linux takes. A server that - * already has a browser opts out with `PUPPETEER_SKIP_DOWNLOAD` and points at it with - * `PUPPETEER_EXECUTABLE_PATH` — what the Docker image does with the Chromium it takes from the - * distro. Neither is required, and neither is set here: npm inherits this process's environment, so - * an install from the admin area sees exactly what the operator set for the server and nothing else. + * cache, which is the ordinary case, and the Docker image does the same at build time into a fixed + * `PUPPETEER_CACHE_DIR`. A server that already has a browser can opt out with + * `PUPPETEER_SKIP_DOWNLOAD` and point at it with `PUPPETEER_EXECUTABLE_PATH`, but it has to be the + * Chrome release this Puppeteer was built for, which a distro package does not stay. None of these + * is set here: npm inherits this process's environment, so an install from the admin area sees + * exactly what the operator set for the server and nothing else. * * Hence the flags: * diff --git a/backend/models/rendering.ts b/backend/models/rendering.ts index 0a861d3c6..85348d20f 100644 --- a/backend/models/rendering.ts +++ b/backend/models/rendering.ts @@ -1018,8 +1018,13 @@ class Rendering { * browser. It asks the one already going to look again before it stops, which is what stops a page * queued in the moment between the last claim and the end of the drain from waiting for the next * request to come along. + * + * **A drain runs in slices of the scheduler's task timeout.** Re-rendering a whole wiki can take far + * longer than that, which is not the drain being stuck, so `signal` is answered by stopping after + * the page in hand and handing what is still queued to a fresh job — queued once this drain has let + * go of the flag, or the new one would join it just as it was finishing. */ - async drainQueue(): Promise { + async drainQueue(signal?: AbortSignal): Promise { if (this.draining) { this.drainRequested = true return @@ -1028,11 +1033,15 @@ class Rendering { try { do { this.drainRequested = false - await this.renderQueuedPages() - } while (this.drainRequested) + await this.renderQueuedPages(signal) + } while (this.drainRequested && !signal?.aborted) } finally { this.draining = false } + if (signal?.aborted) { + // -> Whether or not anything is left: a job that finds the queue empty costs one query + await WIKI.scheduler.addJob({ task: DRAIN_TASK, maxRetries: 0 }) + } } /** True while `drainQueue` is working, so that a second call joins it instead of duplicating it. */ @@ -1053,7 +1062,7 @@ class Rendering { * out of time, which leaves a page wedged in whatever loop it was in, and the pages behind it in the * queue have done nothing to deserve that. */ - private async renderQueuedPages(): Promise { + private async renderQueuedPages(signal?: AbortSignal): Promise { // -> Asked before anything else so that the common drain — a spare job for a batch already swept — // costs one query and says nothing const waiting = await WIKI.db @@ -1072,7 +1081,7 @@ class Rendering { let renderer: PageRenderer | null = null try { - while (true) { + while (!signal?.aborted) { /* Deliberately outside the per-page catch below, and ahead of the claim: a browser that will not open is not this page's fault and will not be the next one's either. Letting that throw @@ -1176,9 +1185,14 @@ class Rendering { } const browser = await puppeteer.launch({ - headless: true, + // -> The headless shell: all a renderer needs, and the build the Docker image installs + headless: 'shell', args: ['--no-sandbox', '--disable-dev-shm-usage'] }) + const mismatch = await describeVersionMismatch(browser) + if (mismatch) { + WIKI.logger.warn(mismatch) + } try { const page = await browser.newPage() // -> A shell page whose only job is to load the frontend's renderer bundle. It is served by this @@ -1242,9 +1256,44 @@ class Rendering { try { await browser.close() } catch {} + // -> A protocol mismatch fails as whichever step trips over it first, in words that name + // neither version — `Requesting main frame too early!` from the navigation, typically + if (mismatch) { + err.message = `${err.message} - ${mismatch}` + } throw err } } } +/** + * Why the browser Puppeteer launched may not be one it can drive, or null when nothing says so. + * + * Puppeteer speaks the DevTools protocol of the one Chrome release it was built against, and a browser + * some way from it fails without saying why. Puppeteer never picks a mismatched browser by itself: the + * way to get one is `PUPPETEER_EXECUTABLE_PATH` pointing at a system Chromium that has since moved on. + * + * Major versions only, since a patch release either side is routine. The expected version is read from + * Puppeteer's internal revisions module, as its own `browsers install` command reads it, because there + * is no public API for it — so if that has moved, the check is skipped rather than failing a render. + */ +async function describeVersionMismatch(browser: any): Promise { + try { + const specifier = 'puppeteer-core/internal/revisions.js' + const { PUPPETEER_REVISIONS } = await import(specifier) + const expected: string = PUPPETEER_REVISIONS['chrome-headless-shell'] + // -> `HeadlessChrome/154.0.8037.57`, or `Chrome/…` for a full browser + const actual: string = (await browser.version()).split('/').pop() + if (!expected || !actual || expected.split('.')[0] === actual.split('.')[0]) { + return null + } + const source = process.env.PUPPETEER_EXECUTABLE_PATH + ? ` It was launched from PUPPETEER_EXECUTABLE_PATH (${process.env.PUPPETEER_EXECUTABLE_PATH}): unset that to use the browser Puppeteer installs for itself, or install the Puppeteer release built for this Chrome.` + : '' + return `Puppeteer is built for Chrome ${expected}, but the browser it launched is ${actual}, and may not be one it can drive.${source}` + } catch { + return null + } +} + export const rendering = new Rendering() diff --git a/backend/models/storage.ts b/backend/models/storage.ts index 79bab08ce..0e232a155 100644 --- a/backend/models/storage.ts +++ b/backend/models/storage.ts @@ -1,3 +1,4 @@ +import { AsyncLocalStorage } from 'node:async_hooks' import fs from 'node:fs/promises' import path from 'node:path' import { load } from 'js-yaml' @@ -25,6 +26,9 @@ export type ContentType = (typeof CONTENT_TYPES)[number] */ const DB_MODULE = 'db' +/** The target an import in progress is reading from. See `Storage.importingFrom`. */ +const importSource = new AsyncLocalStorage() + /** Which content type an asset falls into when it is not large enough to count as a large file. */ const CONTENT_TYPE_BY_KIND: Record = { image: 'images', @@ -1330,6 +1334,28 @@ class Storage { return source ? [source, ...targets.filter((t) => t.id !== source.id)] : targets } + /** + * Run an import from this target without writing what it takes in back to it. + * + * Everything an import adopts is saved the way an editor saves it, and a save is copied to every + * target holding that kind of content — the one it was read from included. That copy is worse than + * redundant: the git and SFTP modules serialize every operation on a target, and an import runs as + * one of those operations, so a write back to the same target waits for the import to finish while + * the import waits for the write. The file is already there, which is where it was read from. + * + * Carried by `AsyncLocalStorage` rather than a parameter because the save that dispatches is several + * models away from the import that caused it, through `createPage`, `updatePage`, `deletePage` and + * the asset model, none of which should have to know an import is happening. + */ + importingFrom(target: StorageTarget, work: () => Promise): Promise { + return importSource.run(target.id, work) + } + + /** Whether this target is the one an import in progress is reading from. */ + private isImportSource(target: StorageTarget): boolean { + return importSource.getStore() === target.id + } + /** * Write an asset's bytes to every target that holds its kind, and that has somewhere to put them. * @@ -1372,6 +1398,9 @@ class Storage { if (accepting.length < 1) { throw this.unstorableAssetError(ref, contentType, declining) } + // -> Counted above as somewhere the file can be held, which it is: it was read from there. It + // is only not written to again — see `importingFrom`. + const writing = accepting.filter(({ target }) => !this.isImportSource(target)) if (declining.length > 0) { // -> Not a warning on the target: it did what the site configured it to do WIKI.logger.debug( @@ -1379,7 +1408,7 @@ class Storage { ) } - for (const { target, mod } of accepting) { + for (const { target, mod } of writing) { try { await mod.putAsset(target, ref, data) } catch (err: any) { @@ -1596,6 +1625,9 @@ class Storage { run: (mod: StorageModule, target: StorageTarget) => Promise ): Promise { for (const target of await this.writeTargetsFor(ref.siteId, ref.kind, ref.fileSize)) { + if (this.isImportSource(target)) { + continue + } const mod = await this.ensureModule(target.module) if (!mod) { continue @@ -1654,6 +1686,9 @@ class Storage { run: (mod: StorageModule, target: StorageTarget) => Promise ): Promise { for (const target of await this.pageTargets(siteId)) { + if (this.isImportSource(target)) { + continue + } const mod = await this.ensureModule(target.module) if (!mod) { continue diff --git a/backend/modules/extensions/puppeteer/definition.yml b/backend/modules/extensions/puppeteer/definition.yml index e8ee6c1c6..1f12d0baa 100644 --- a/backend/modules/extensions/puppeteer/definition.yml +++ b/backend/modules/extensions/puppeteer/definition.yml @@ -3,7 +3,8 @@ title: Puppeteer description: >- Headless Chromium browser. Required to re-render a page from its source on the server: the markdown renderer lives in the browser, so the server drives one. Installing it downloads a Chromium build of - a few hundred megabytes, unless the server already provides one through PUPPETEER_EXECUTABLE_PATH. + a few hundred megabytes, unless the server already provides one through PUPPETEER_EXECUTABLE_PATH, + which has to be the Chrome release this version of Puppeteer is built for. website: 'https://pptr.dev' detect: type: module @@ -17,4 +18,8 @@ architectures: isInstallable: true # The version installed, here rather than in `package.json` for the reason above. The Docker image # reads this same line, so an image and a hand-installed instance agree on what Puppeteer is. -installVersion: 25.4.0 +# +# It also decides which browser: each release is built against one Chrome, and installs that build and +# no other. Chrome for Testing has native linux-arm64 builds only from Chrome 153, and a release pinned +# to an older one installs the x64 build on arm64 without complaint, so this must not go below 25.12.0. +installVersion: 25.12.0 diff --git a/backend/modules/storage/git/storage.ts b/backend/modules/storage/git/storage.ts index fa97fe4f7..6d57fa876 100644 --- a/backend/modules/storage/git/storage.ts +++ b/backend/modules/storage/git/storage.ts @@ -34,12 +34,13 @@ interface Repo { fingerprint: string /** Whether the remote has been contacted since this entry was made. See `ensureRemote`. */ remoteReady: boolean - /** Serializes work on this repository. See `withRepo`. */ - queue: Promise } const repos = new Map() +/** The last operation queued against each target. See `withRepo`. */ +const queues = new Map>() + /** The working copy for this target, as an absolute path. */ function repoDir(target: StorageTarget): string { return resolveRoot(target.config.localRepoPath, DEFAULT_REPO_PATH) @@ -207,8 +208,7 @@ async function prepareRepo(target: StorageTarget): Promise { git, root, fingerprint: configFingerprint(target), - remoteReady: false, - queue: Promise.resolve() + remoteReady: false } } @@ -220,19 +220,25 @@ async function prepareRepo(target: StorageTarget): Promise { * safe, and the queue is per target because two targets are two working copies. */ async function withRepo(target: StorageTarget, run: (repo: Repo) => Promise): Promise { - let repo = repos.get(target.id) - if (!repo || repo.fingerprint !== configFingerprint(target)) { - repo = await prepareRepo(target) - repos.set(target.id, repo) + const turn = async () => { + let repo = repos.get(target.id) + if (!repo || repo.fingerprint !== configFingerprint(target)) { + repo = await prepareRepo(target) + repos.set(target.id, repo) + } + return run(repo) } - const entry = repo - const result = entry.queue.then( - () => run(entry), - () => run(entry) - ) + // -> The repository is prepared inside the turn, and the queue is kept apart from it. A queue that + // lived on the prepared entry would give two operations arriving before there was one an entry + // each, and a changed setting a new entry beside whatever was still running on the old one — two + // queues over one working copy, and the index lock this exists to avoid. + const result = (queues.get(target.id) ?? Promise.resolve()).then(turn, turn) // -> The queue holds the settled outcome rather than the result, so one failed operation does not // reject every operation queued behind it - entry.queue = result.catch(() => {}) + queues.set( + target.id, + result.catch(() => {}) + ) return result } @@ -1110,7 +1116,11 @@ async function applyIncoming( let deletedAssets = 0 for (const segments of removals) { try { - const removed = await removeFromWiki(target, segments, actorId) + // -> The file is already gone from this working copy, and deleting it again would queue behind + // the sync that is running this — see `storage.importingFrom` + const removed = await WIKI.models.storage.importingFrom(target, () => + removeFromWiki(target, segments, actorId) + ) if (removed === 'page') { deletedPages++ } else if (removed === 'asset') { diff --git a/backend/tasks/simple/render-pages.ts b/backend/tasks/simple/render-pages.ts index 21ba5978b..9586a67f2 100644 --- a/backend/tasks/simple/render-pages.ts +++ b/backend/tasks/simple/render-pages.ts @@ -1,3 +1,5 @@ +import type { TaskContext } from '../../core/scheduler.ts' + /** * Render every page waiting in the render queue. * @@ -7,6 +9,6 @@ * there is one browser and it renders one page at a time. A run that finds the queue empty (a second * job for a batch this one already swept) returns without launching anything. */ -export async function task(): Promise { - await WIKI.models.rendering.drainQueue() +export async function task(_payload: unknown, { signal }: TaskContext): Promise { + await WIKI.models.rendering.drainQueue(signal) } diff --git a/backend/tasks/simple/sync-storage-targets.ts b/backend/tasks/simple/sync-storage-targets.ts index 404e56a48..504a33334 100644 --- a/backend/tasks/simple/sync-storage-targets.ts +++ b/backend/tasks/simple/sync-storage-targets.ts @@ -1,3 +1,5 @@ +import type { TaskContext } from '../../core/scheduler.ts' + /** * Sync every storage target that has a remote to keep in step with, and whose site is due. * @@ -23,7 +25,7 @@ * working copy is synced. Every instance in a high-availability set keeps its own, so they each fall * in step at their own turn rather than fighting over one repository. */ -export async function task(): Promise { +export async function task(_payload: unknown, { signal }: TaskContext): Promise { const syncable = await WIKI.models.storage.syncableTargets() if (syncable.length < 1) { return @@ -53,7 +55,14 @@ export async function task(): Promise { WIKI.logger.info(`Syncing ${targets.length} storage target(s)...`) let failed = 0 - for (const target of targets) { + for (const [index, target] of targets.entries()) { + // -> A sync cannot be interrupted halfway, but the next one need not be started. The targets left + // over sync on their next due tick. + if (signal.aborted) { + throw new Error( + `Stopped after ${index} of ${targets.length} storage target(s): ${signal.reason?.message}` + ) + } try { const message = await WIKI.models.storage.executeAction(target, 'sync', actorId) WIKI.logger.info(`Synced ${target.title} for site ${target.siteId}: ${message ?? 'done'}`) diff --git a/dev/build/Dockerfile b/dev/build/Dockerfile index a8a3b5c2a..c5808f943 100644 --- a/dev/build/Dockerfile +++ b/dev/build/Dockerfile @@ -4,7 +4,6 @@ LABEL maintainer="requarks.io" RUN DEBIAN_FRONTEND=noninteractive apt-get update && apt-get install -qy --no-install-recommends \ bash \ build-essential \ - chromium \ curl \ fonts-liberation \ git \ @@ -30,11 +29,16 @@ USER node ENV NODE_ENV=production -# The browser the Puppeteer extension drives, installed above rather than downloaded by Puppeteer: the -# distro keeps it patched, it exists for arm64 as well as amd64, and the image does not carry two copies -# of Chromium. `PUPPETEER_SKIP_DOWNLOAD` has to be set before the install below for that to hold. -ENV PUPPETEER_SKIP_DOWNLOAD=true -ENV PUPPETEER_EXECUTABLE_PATH=/usr/bin/chromium +# The browser the Puppeteer extension drives is the one Puppeteer installs for itself, not the distro's: +# Puppeteer speaks the protocol of the single Chrome release it was built against, and a distro package +# moves on with every security update while Puppeteer stays pinned, so each rebuild widened the gap until +# every render failed. Installed by Puppeteer, the pin below decides both halves of the pair. +# +# Only the headless shell, which is all a renderer needs; the full browser is skipped. +# The cache is a fixed path rather than under $HOME so that it is found whatever user the container is +# run as -- an arbitrary UID on OpenShift has no home directory of its own. +ENV PUPPETEER_CACHE_DIR=/wiki/.cache/puppeteer +ENV PUPPETEER_CHROME_SKIP_DOWNLOAD=true WORKDIR /wiki/backend RUN npm ci --omit=dev @@ -48,9 +52,37 @@ RUN npm ci --omit=dev # operator adds Puppeteer to an instance by hand: one place to bump, and an image that cannot drift from # what a hand-installed instance gets. An empty read fails the build rather than quietly installing # whatever is newest. +# +# Puppeteer's postinstall downloads the browser, and `browsers install` makes sure of it: npm may be +# configured not to run install scripts, and when the browser is already there it only prints its path. RUN PUPPETEER_VERSION="$(sed -n 's/^installVersion: *//p' modules/extensions/puppeteer/definition.yml)" && \ test -n "$PUPPETEER_VERSION" && \ - npm install --no-save "puppeteer@${PUPPETEER_VERSION}" + npm install --no-save "puppeteer@${PUPPETEER_VERSION}" && \ + npx puppeteer browsers install chrome-headless-shell + +# The system libraries that browser needs, which are no longer arriving as the dependencies of a distro +# package. Each Chrome for Testing build lists its own in `deb.deps`, per architecture, and this is the +# `apt-get satisfy` that `browsers install --install-deps` would run -- done here as root, rather than +# running Puppeteer as root, so that nothing in the cache ends up owned by root. +USER root +RUN DEPS="$(find "$PUPPETEER_CACHE_DIR" -name deb.deps)" && \ + test "$(printf '%s\n' "$DEPS" | grep -c .)" -eq 1 && \ + apt-get update && \ + DEBIAN_FRONTEND=noninteractive apt-get satisfy -qy --no-install-recommends "$(paste -sd, "$DEPS")" && \ + rm -rf /var/lib/apt/lists/* +USER node + +# Launch the browser the way the renderer does and navigate once, which is the step a mismatched or +# half-installed browser fails at. A pair that cannot render fails the image build, not every render +# on every instance running the image. +RUN node --input-type=module -e " \ + import puppeteer from 'puppeteer'; \ + const browser = await puppeteer.launch({ headless: 'shell', args: ['--no-sandbox', '--disable-dev-shm-usage'] }); \ + const page = await browser.newPage(); \ + await page.goto('data:text/html,

ok

'); \ + if ((await page.\$eval('p', (e) => e.textContent)) !== 'ok') throw new Error('The browser did not render.'); \ + console.log('Puppeteer drives ' + (await browser.version())); \ + await browser.close();" WORKDIR /wiki