diff --git a/.github/workflows/dashboard-check.mjs b/.github/workflows/dashboard-check.mjs index e1bcc03..f2b67f6 100644 --- a/.github/workflows/dashboard-check.mjs +++ b/.github/workflows/dashboard-check.mjs @@ -21,7 +21,6 @@ // Usage: node dashboard-check.mjs import vm from 'node:vm'; -import { readFile } from 'node:fs/promises'; const base = process.argv[2]; if (!base) { @@ -47,15 +46,15 @@ for (const marker of ['
', '
', 'artifacts</ti } // Every inline type the server publishes needs an option inside the compose type select, or // that type can only be published through the API, the CLI or MCP. redirect shipped without -// one (T2.1.7). The list comes from server.js rather than a copy here, so a sixth type fails -// this check on the day it is added instead of shipping with no way to compose it. +// one (T2.1.7). The list is imported rather than copied here, so a sixth type fails this check +// on the day it is added instead of shipping with no way to compose it. It reads SOURCE_EXT, +// which is what server.js builds TYPES from and what the type-change cleanup keys off, so all +// three cannot drift apart (T2.1.9). const typeSelect = html.match(/<select id="type">([\s\S]*?)<\/select>/); if (!typeSelect) fail('the compose form has no <select id="type">'); -const serverSrc = await readFile(new URL('../../server.js', import.meta.url), 'utf8'); -const typesLine = serverSrc.match(/^const TYPES = \[([^\]]*)\]/m); -if (!typesLine) fail('could not read the TYPES list out of server.js'); -const types = [...typesLine[1].matchAll(/'([^']+)'/g)].map((m) => m[1]); -if (types.length < 2) fail(`parsed only ${types.length} type(s) from server.js TYPES`); +const { SOURCE_EXT } = await import(new URL('../../lib/artifact-files.js', import.meta.url)); +const types = Object.keys(SOURCE_EXT); +if (types.length < 2) fail(`parsed only ${types.length} type(s) from SOURCE_EXT`); for (const type of types) { if (!typeSelect[1].includes(`value="${type}"`)) { fail(`the compose type select has no option for ${type}`); diff --git a/.github/workflows/smoke.sh b/.github/workflows/smoke.sh index 44e03fa..334a4cb 100644 --- a/.github/workflows/smoke.sh +++ b/.github/workflows/smoke.sh @@ -478,6 +478,42 @@ curl -sf -X PUT "$BASE/api/artifacts/ci-redir-conv" -H "$AUTH" -H "$JSON" \ curl -sf -X DELETE "$BASE/api/artifacts/ci-redir-conv" -H "$AUTH" > /dev/null echo "ok: html converted to a redirect" +# T2.1.9: a type change drops the objects the old type owned. The bytes themselves are not +# reachable over HTTP (every serve path keys off meta.type, which is why they sat there +# unnoticed since md shipped), so what this proves is the other half: the cleanup never takes a +# file the new type still needs. Walk one artifact through every type and read it back after +# each hop. test/artifact-files.test.js covers which keys each direction drops, and +# test/storage-local.test.js plus test/storage-sql.test.js cover the delete itself. +convert_to() { # convert_to <type> <content> + curl -sf -X PUT "$BASE/api/artifacts/ci-conv" -H "$AUTH" -H "$JSON" \ + -d "$(node -e 'process.stdout.write(JSON.stringify({content: process.argv[1], type: process.argv[2]}))' "$2" "$1")" > /dev/null \ + || fail "converting to $1 was refused" + [ "$(list_field ci-conv type)" = "$1" ] || fail "converted artifact is not listed as $1" +} +curl -sf -X POST "$BASE/api/artifacts" -H "$AUTH" -H "$JSON" \ + -d '{"content":"<h1>step html</h1>","type":"html","slug":"ci-conv","visibility":"public"}' > /dev/null +convert_to md '# step md' +# md renders inside the frame, so the rendered body is behind ?raw=1 like every other type +curl -s "$BASE/a/ci-conv?raw=1" | grep -q '<h1>step md</h1>' || fail "md conversion lost the rendered body" +[ "$(curl -s "$BASE/a/ci-conv/source")" = '# step md' ] || fail "md conversion lost source.md" +convert_to jsx 'export default () => <p>step jsx</p>' +curl -s "$BASE/a/ci-conv?raw=1" | grep -q 'step jsx' || fail "jsx conversion lost index.html" +curl -s "$BASE/a/ci-conv/source" | grep -q 'step jsx' || fail "jsx conversion lost source.jsx" +# jsx to tsx is the direction where both types bake an index.html and only the extension moves +convert_to tsx 'export default () => <p>step tsx</p>' +curl -s "$BASE/a/ci-conv?raw=1" | grep -q 'step tsx' || fail "tsx conversion lost index.html" +curl -s "$BASE/a/ci-conv/source" | grep -q 'step tsx' || fail "tsx conversion lost source.tsx" +convert_to redirect 'https://example.com/step-redirect' +code=$(curl -s -o /dev/null -w '%{http_code}' "$BASE/a/ci-conv") +expect_code 301 "$code" "the conversion walk's redirect hop" +[ "$(curl -s "$BASE/a/ci-conv/source")" = 'https://example.com/step-redirect' ] \ + || fail "redirect conversion lost source.url" +convert_to html '<h1>step html again</h1>' +curl -s "$BASE/a/ci-conv?raw=1" | grep -q 'step html again' || fail "html conversion lost index.html" +curl -s "$BASE/a/ci-conv/source" | grep -q 'step html again' || fail "html conversion lost source.html" +curl -sf -X DELETE "$BASE/api/artifacts/ci-conv" -H "$AUTH" > /dev/null +echo "ok: every type conversion serves the type it was given" + # a copy keeps the target dupslug=$(curl -s -X POST "$BASE/api/artifacts/ci-redir/duplicate" -H "$AUTH" -H "$JSON" \ -d '{"slug":"ci-redir-copy","visibility":"public"}' | sed -n 's/.*"slug":"\([^"]*\)".*/\1/p') diff --git a/docs/mcp.md b/docs/mcp.md index cc1897c..14f1337 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -42,12 +42,12 @@ author's own HTML. Details in [Link previews](formats.md#link-previews). `FRAME_ENABLED=false` nothing is framed and `frame: true` changes nothing, so a client that gets no frame should check the server config before the artifact. -`update_artifact` rewrites the artifact rather than patching it, and two of its optional arguments -fall back to a default instead of keeping what is there. Pass `type` on every update of a `jsx`, -`tsx`, `md` or `redirect` artifact, or the artifact comes back as HTML. Pass `title` to keep the -title, or it resets to the slug. `frame`, `tags`, `project` and `visibility` do keep their current -value when omitted, and take the values documented on the matching `set_artifact_*` row. To change -one field and touch nothing else, use that row's tool instead of `update_artifact`. +`update_artifact` rewrites the artifact rather than patching it, and `type` falls back to `html` +instead of keeping what is there. Pass it on every update of a `jsx`, `tsx`, `md` or `redirect` +artifact, or the artifact comes back as HTML and the files the old type owned are deleted. `title`, +`frame`, `tags`, `project`, `visibility`, `description` and `ogImage` all keep their current value +when omitted, and take the values documented on the matching `set_artifact_*` row. To change one +field and touch nothing else, use that row's tool instead of `update_artifact`. No MCP tool for zip sites, because the payload is binary. Use the [CLI](cli.md) or the [zip endpoint](api.md#zip-sites-multi-file-static-projects). diff --git a/lib/artifact-files.js b/lib/artifact-files.js new file mode 100644 index 0000000..9075a47 --- /dev/null +++ b/lib/artifact-files.js @@ -0,0 +1,69 @@ +// Which storage objects an artifact of each type owns, and what a type change leaves behind. +// +// This lives outside server.js so a unit test can walk every ordered pair of types without a +// server or a storage backend. saveArtifact only ever wrote: a PUT converting html to a redirect +// left index.html and source.html on disk, and the reverse left source.url. Nothing served them, +// because /a/:slug and /a/:slug/source both key off meta.type, so it was at-rest bloat that grew +// with every conversion. +// +// What this does NOT do: on the git backend the deletion is a commit like any other, so the old +// bytes stay in history and `git log -p` on the remote still hands them back. A private or +// password artifact's old body is readable to anyone with read access to GIT_REMOTE_URL, exactly +// as it was before. Reclaiming that needs a history rewrite, which is not something a publish +// request gets to do. + +// The `source.<ext>` extension each type uses. /a/:slug/source reads it back. +export const SOURCE_EXT = { html: 'html', jsx: 'jsx', tsx: 'tsx', md: 'md', redirect: 'url' }; + +// html, jsx and tsx bake an index.html at publish time. md renders per request from source.md +// and a redirect answers with a header, so neither owns one. A zip site is in neither list: its +// files live under site/ and no API path converts a zip to anything (storeArtifact refuses to +// replace one with inline content, storeZipArtifact 409s on a slug that exists), so a zip +// namespace only ever goes away whole, through deleteSlug. +const BAKES_INDEX = new Set(['html', 'jsx', 'tsx']); + +// The content objects a stored artifact of this type owns. Never meta.json: the record belongs +// to the artifact, not to the type. +export function ownedKeys(slug, type) { + const keys = []; + if (BAKES_INDEX.has(type)) keys.push(`${slug}/index.html`); + // hasOwn, not a plain lookup: a hand-edited meta.type of "constructor" or "toString" would + // otherwise find a function on the prototype and build `source.function Object() {...}` as a + // key. Nothing would match it, but a delete should not be built from a value off the chain. + if (Object.hasOwn(SOURCE_EXT, type)) keys.push(`${slug}/source.${SOURCE_EXT[type]}`); + return keys; +} + +// The keys the old type owned that the new type does not. Empty when the type did not change +// and empty on a first publish, where there is no old type. +export function staleKeys(slug, oldType, newType) { + if (!oldType || oldType === newType) return []; + const kept = new Set(ownedKeys(slug, newType)); + return ownedKeys(slug, oldType).filter((key) => !kept.has(key)); +} + +// Drop what the conversion left behind. The caller runs this AFTER meta.json names the new type +// and BEFORE flush, so a crash in between leaves the old record whole rather than a listed +// artifact with no body, and git carries the deletions in the same commit as the write. +// +// It lives here rather than inline in server.js so a test can hand it a fake storage and prove +// the deletes are actually issued. No test boots server.js, so an inline loop was provably dead +// weight: neutering it to `for (const key of [])` left both suites green on all five backends. +// +// A delete that fails is logged, not thrown. The write has already landed and meta already names +// the new type, so throwing would turn a successful replace into a 500 the caller would retry. +export async function dropStaleObjects(storage, slug, oldType, newType) { + const dropped = []; + for (const key of staleKeys(slug, oldType, newType)) { + try { + await storage.delete(key); + dropped.push(key); + } catch (err) { + console.warn( + `storage: could not drop ${key} after a type change: ${err.message}. ` + + 'The artifact is fine and nothing serves that file. Nothing retries, so remove it by hand.', + ); + } + } + return dropped; +} diff --git a/server.js b/server.js index b94b1f0..d9ce445 100644 --- a/server.js +++ b/server.js @@ -31,6 +31,7 @@ import { validateCredentials, parseKeyInput, } from './lib/auth.js'; +import { SOURCE_EXT, dropStaleObjects } from './lib/artifact-files.js'; import { createConfigStore } from './lib/config.js'; import { ApiError } from './lib/errors.js'; import { artifactExpired } from './lib/expiry.js'; @@ -210,7 +211,9 @@ function artifactUnlocked(req, meta) { return unlockValid(req, meta); } -const TYPES = ['html', 'jsx', 'tsx', 'md', 'redirect']; +// Read off the same table lib/artifact-files.js cleans up from, so a sixth type cannot become +// publishable here and stay invisible to the type-change cleanup there. +const TYPES = Object.keys(SOURCE_EXT); const SLUG_RE = /^[a-z0-9][a-z0-9-]{2,63}$/; const TAG_RE = /^[a-z0-9][a-z0-9-]{0,31}$/; const MAX_TAGS = 10; @@ -218,7 +221,6 @@ const MAX_TAGS = 10; // slug — Unicode letters/digits, spaces, and - _ . — but bounded, and must // start with a letter or digit. Internal whitespace is collapsed on input. const PROJECT_RE = /^[\p{L}\p{N}][\p{L}\p{N}\p{M} ._-]{0,63}$/u; -const SOURCE_EXT = { html: 'html', jsx: 'jsx', tsx: 'tsx', md: 'md', redirect: 'url' }; // Pinned versions shared with the jsx shell. `external=react` keeps packages on // the shell's React instance — separate copies cause "Invalid hook call". @@ -736,6 +738,10 @@ async function storeArtifact(finalSlug, { content, type = 'html', title, descrip await storage.put(`${finalSlug}/meta.json`, JSON.stringify(meta, null, 2), { contentType: 'application/json', }); + // The old type's objects are unreachable now that meta names the new one, so drop them. After + // the meta write and before flush; the ordering and the swallowed failure are explained in + // lib/artifact-files.js. + await dropStaleObjects(storage, finalSlug, existing?.type, type); await storage.flush?.(); // durably commit the completed write (git); no-op elsewhere dropMdRender(finalSlug); // A non-public artifact needs the session secret resident to mint its capability token; @@ -1178,9 +1184,15 @@ function parseRange(header, size) { // status/headers are flushed and immutable, so an upstream error must ABORT the socket // (res.destroy) — never res.end(), which would pass a truncated artifact off as complete. function pipeStream(res, stream) { - stream.on('error', () => { - if (!res.headersSent) res.status(500).type('text/plain').send('internal error'); - else res.destroy(); + stream.on('error', (err) => { + if (res.headersSent) return res.destroy(); + // A read that passed the stat can still miss when the open happens: local checks the file + // and then opens it, so a write that landed in between took the object away. That is the + // same "not there" the stat catches, and the rename path has always answered it as a 404, + // so it answers 404 here too rather than the 500 a real read error gets. + const gone = err?.code === 'ENOENT' || err?.code === 'ENOTDIR'; + if (gone) return res.status(404).type('text/plain').send('not found'); + res.status(500).type('text/plain').send('internal error'); }); stream.pipe(res); } @@ -1387,6 +1399,9 @@ app.get('/a/:slug/source', async (req, res, next) => { if (target === null) return notFound(res); return res.type('text/plain; charset=utf-8').send(target); } + // meta.type comes off disk unvalidated, and a bare SOURCE_EXT lookup walks the prototype, so a + // hand-edited "constructor" built a key out of a function body. Same guard ownedKeys uses. + if (!Object.hasOwn(SOURCE_EXT, meta.type)) return notFound(res); // forceType keeps source inert: an HTML/JSX source is served as text/plain, never executed. serveObject(req, res, `${slug}/source.${SOURCE_EXT[meta.type]}`, { forceType: 'text/plain; charset=utf-8', @@ -1844,7 +1859,7 @@ function createMcpServer(scopes = SCOPES) { { title: 'Update artifact', description: - 'Rewrite an existing artifact by slug. Only type and title reset when omitted: type becomes html and title becomes the slug, so pass both on every update. Every other field the artifact has keeps its current value. Returns the share URL, tokened for private and password artifacts.', + 'Rewrite an existing artifact by slug. Only type resets when omitted: it becomes html, and changing the type deletes the files the old type owned, so pass it on every update of a jsx, tsx, md or redirect artifact. Every other field, title included, keeps its current value. Returns the share URL, tokened for private and password artifacts.', inputSchema: { slug: z.string(), content: z.string(), diff --git a/storage/git.js b/storage/git.js index 6dd73b0..4b2617f 100644 --- a/storage/git.js +++ b/storage/git.js @@ -213,6 +213,7 @@ export async function create() { put: (key, data, opts) => files.put(key, data, opts), move: (oldSlug, newSlug) => files.move(oldSlug, newSlug), copySlug: (src, dst) => files.copySlug(src, dst), + delete: (key) => files.delete(key), deleteSlug: (slug) => files.deleteSlug(slug), // Commit the completed write and push it, serialized so two operations never race the diff --git a/storage/index.js b/storage/index.js index 0b09598..ee23c59 100644 --- a/storage/index.js +++ b/storage/index.js @@ -16,6 +16,7 @@ // listMetas() -> [{ slug, buffer }] // every artifact's meta.json // move(oldSlug, newSlug) // rename a whole namespace // copySlug(srcSlug, dstSlug) // copy a namespace's content objects (NOT meta.json) +// delete(key) // remove ONE object (never a prefix); a key that is gone is not an error // deleteSlug(slug) // remove a whole namespace // flush?() // optional: durably commit a completed write (git) // } @@ -39,6 +40,10 @@ // content objects first and `<slug>/meta.json` LAST as a commit marker, because readMeta // and listMetas key off meta.json — a namespace with no meta is invisible (404), never // half-served. deleteSlug removes meta first. See server.js for where this is applied. +// +// The same ordering decides where delete(key) goes: a replace that changes the type drops the +// old type's objects AFTER meta.json names the new one, so a crash mid-conversion leaves the +// old record whole rather than a listed artifact whose body is gone. // A key/segment that fails validation. Callers map this to 404 (it only reaches a backend // via user-controlled zip sub-paths); it must never surface as a 500. @@ -73,6 +78,30 @@ const BACKENDS = { sqlite: () => import('./sqlite.js'), }; +// Every method the app calls on a store. A backend that is missing one fails the boot rather +// than the request that first needs it: the type-change cleanup swallows a failed delete on +// purpose (a write that already landed must not 500), so a backend with no delete would warn +// once per conversion forever and never fail a test. flush is left out because it is optional. +const REQUIRED = [ + 'getBuffer', + 'get', + 'head', + 'put', + 'listMetas', + 'move', + 'copySlug', + 'delete', + 'deleteSlug', +]; + +export function assertComplete(name, storage) { + const missing = REQUIRED.filter((method) => typeof storage?.[method] !== 'function'); + if (missing.length) { + throw new Error(`storage backend "${name}" is missing: ${missing.join(', ')}`); + } + return storage; +} + // Instantiate the configured backend and run its boot check (fail-fast, like the // ARTIFACTS_API_KEY check) so a misconfigured store crashes at startup, not first request. export async function createStorage() { @@ -90,7 +119,7 @@ export async function createStorage() { `storage backend "${name}" could not be loaded — is its dependency installed? (${err.message})`, ); } - const storage = await mod.create(); + const storage = assertComplete(name, await mod.create()); await storage.init?.(); return storage; } diff --git a/storage/local.js b/storage/local.js index 14cafc0..9835626 100644 --- a/storage/local.js +++ b/storage/local.js @@ -222,6 +222,13 @@ export async function createAt(root) { }); }, + // Remove one object. `force` so a key that is already gone is not an error: a conversion + // asks for the old type's files without checking, and an artifact published before that + // type owned one of them has nothing there to drop. + async delete(key) { + await fs.rm(resolveKey(key), { force: true }); + }, + async deleteSlug(slug) { // meta.json first so a crash mid-delete leaves an invisible (404) namespace, never a // live artifact with missing files. diff --git a/storage/postgres.js b/storage/postgres.js index 7bbcba3..512c983 100644 --- a/storage/postgres.js +++ b/storage/postgres.js @@ -78,6 +78,12 @@ export async function create() { ); }, + async delete(key) { + // Equality, not a range, so no collation question: this drops one object, and a row that + // is not there is a no-op. + await q('DELETE FROM artifacts WHERE key = $1', [key]); + }, + async deleteSlug(slug) { await q('DELETE FROM artifacts WHERE key COLLATE "C" >= $1 AND key COLLATE "C" < $2', [`${slug}/`, `${slug}0`]); }, diff --git a/storage/s3.js b/storage/s3.js index 7187ac3..5f00887 100644 --- a/storage/s3.js +++ b/storage/s3.js @@ -258,6 +258,12 @@ export async function create() { } }, + // Remove one object. deleteKeys already treats a 404 as done, which is what a conversion + // needs: it asks for the old type's files without checking whether each one is there. + async delete(key) { + await deleteKeys([key]); + }, + async deleteSlug(slug) { const keys = await listKeys(slug); const meta = `${slug}/meta.json`; diff --git a/storage/sqlite.js b/storage/sqlite.js index 937ef46..5270820 100644 --- a/storage/sqlite.js +++ b/storage/sqlite.js @@ -35,6 +35,7 @@ export async function create() { 'UPDATE artifacts SET key = ? || substr(key, ?) WHERE key >= ? AND key < ?', ); const deleteStmt = db.prepare('DELETE FROM artifacts WHERE key >= ? AND key < ?'); + const deleteKeyStmt = db.prepare('DELETE FROM artifacts WHERE key = ?'); const copyStmt = db.prepare( 'INSERT INTO artifacts (key, data, content_type) ' + 'SELECT ? || substr(key, ?), data, content_type FROM artifacts ' + @@ -76,6 +77,11 @@ export async function create() { copyStmt.run(dstSlug, srcSlug.length + 1, `${srcSlug}/`, `${srcSlug}0`, `${srcSlug}/meta.json`); }, + delete(key) { + // Equality, not a range: this drops one object, so a row that is not there is a no-op. + deleteKeyStmt.run(key); + }, + deleteSlug(slug) { deleteStmt.run(`${slug}/`, `${slug}0`); }, diff --git a/storage/sqlstore.js b/storage/sqlstore.js index a1fd839..45d1231 100644 --- a/storage/sqlstore.js +++ b/storage/sqlstore.js @@ -1,7 +1,7 @@ // Shared core for the SQL-backed stores (sqlite, postgres). Both keep every object as a row // in one table `artifacts(key, data, content_type)`, keyed by `<slug>/<relpath>`. A driver -// supplies the six data operations; this module wraps them with the key guard and the stream -// shape the serving layer expects. +// supplies the data operations listed below; this module wraps them with the key guard and the +// stream shape the serving layer expects. // // Note [streaming]: unlike local/s3, SQL rows are read whole — get() buffers the entire object // in memory (there is no partial-row streaming), so these backends rely on the upload size @@ -29,6 +29,7 @@ function toBuffer(data) { // listMetas() -> Promise<[{slug, buffer}]> // move(oldSlug, newSlug) -> Promise<void> // copySlug(srcSlug, dstSlug) -> Promise<void> +// delete(key) -> Promise<void> // deleteSlug(slug) -> Promise<void> // init() -> Promise<void> (create table + connectivity probe) // close?() -> Promise<void> @@ -78,6 +79,11 @@ export function makeSqlStore(driver) { await driver.copySlug(srcSlug, dstSlug); }, + async delete(key) { + assertSafeKey(key); + await driver.delete(key); + }, + async deleteSlug(slug) { assertSafeKey(slug); await driver.deleteSlug(slug); diff --git a/test/artifact-files.test.js b/test/artifact-files.test.js new file mode 100644 index 0000000..85733cd --- /dev/null +++ b/test/artifact-files.test.js @@ -0,0 +1,156 @@ +// Unit tests for lib/artifact-files.js. No server and no storage backend: the rule is a pure +// function over two type names, so a test can walk every ordered pair of types, which is what +// T2.1.9 asks for with "a case per direction". +import test from 'node:test'; +import assert from 'node:assert/strict'; + +import { dropStaleObjects, ownedKeys, staleKeys, SOURCE_EXT } from '../lib/artifact-files.js'; + +const TYPES = ['html', 'jsx', 'tsx', 'md', 'redirect']; + +// Minimal stand-in for storage/*.js: dropStaleObjects only calls delete, so recording the keys +// it asks for is the whole contract. `fails` makes that delete throw, which is the branch that +// decides whether a failed cleanup can sink a write that already landed. +function stubStorage({ fails = false } = {}) { + const asked = []; + return { + asked, + async delete(key) { + asked.push(key); + if (fails) throw new Error('backend said no'); + }, + }; +} + +test('each type owns the objects the serve path reads back', () => { + assert.deepEqual(ownedKeys('s', 'html'), ['s/index.html', 's/source.html']); + assert.deepEqual(ownedKeys('s', 'jsx'), ['s/index.html', 's/source.jsx']); + assert.deepEqual(ownedKeys('s', 'tsx'), ['s/index.html', 's/source.tsx']); + // md renders at serve time from source.md and a redirect answers with a header, so neither + // bakes an index.html. + assert.deepEqual(ownedKeys('s', 'md'), ['s/source.md']); + assert.deepEqual(ownedKeys('s', 'redirect'), ['s/source.url']); +}); + +test('a write that does not change the type drops nothing', () => { + for (const type of TYPES) { + assert.deepEqual(staleKeys('s', type, type), [], `${type} to ${type}`); + } +}); + +test('a first publish has no old type, so it drops nothing', () => { + for (const type of TYPES) { + assert.deepEqual(staleKeys('s', undefined, type), [], `new ${type}`); + } +}); + +// The invariant, walked across all twenty ordered pairs: what the conversion drops plus what +// the new type owns covers everything the old type owned, and nothing the new type still needs +// is on the drop list. +test('every direction drops exactly what the new type stops using', () => { + for (const from of TYPES) { + for (const to of TYPES) { + if (from === to) continue; + const label = `${from} to ${to}`; + const stale = staleKeys('s', from, to); + const kept = ownedKeys('s', to); + for (const key of stale) { + assert.ok(!kept.includes(key), `${label} would drop ${key}, which ${to} still serves`); + } + for (const key of ownedKeys('s', from)) { + assert.ok( + stale.includes(key) || kept.includes(key), + `${label} leaves ${key} behind with nothing serving it`, + ); + } + } + } +}); + +test('the named directions match what the item filed', () => { + // html to redirect: the baked page and the html source both stop being read. + assert.deepEqual(staleKeys('s', 'html', 'redirect'), ['s/index.html', 's/source.html']); + // redirect back to html: the target file goes, index.html is written fresh. + assert.deepEqual(staleKeys('s', 'redirect', 'html'), ['s/source.url']); + // md to html: the markdown goes, index.html appears for the first time. + assert.deepEqual(staleKeys('s', 'md', 'html'), ['s/source.md']); + // html to md: the baked page goes because md renders per request. + assert.deepEqual(staleKeys('s', 'html', 'md'), ['s/index.html', 's/source.html']); + // jsx to tsx: both bake an index.html, so only the extension changes. + assert.deepEqual(staleKeys('s', 'jsx', 'tsx'), ['s/source.jsx']); +}); + +test('meta.json survives every conversion', () => { + for (const from of TYPES) { + for (const to of TYPES) { + assert.ok( + !staleKeys('s', from, to).includes('s/meta.json'), + `${from} to ${to} would delete the record`, + ); + } + } +}); + +// A zip site is never a conversion source: storeArtifact refuses to replace one with inline +// content, and storeZipArtifact 409s on a slug that already exists. Nothing here should invent +// a delete for a type it has no extension for. +test('a type with no source extension drops nothing', () => { + assert.equal(SOURCE_EXT.zip, undefined); + assert.deepEqual(staleKeys('s', 'zip', 'html'), []); + assert.deepEqual(ownedKeys('s', 'zip'), []); +}); + +// A meta.json edited by hand can carry any string as its type. A plain SOURCE_EXT[type] lookup +// walks the prototype, so "constructor" built `s/source.function Object() { [native code] }`. +test('a type name off the prototype chain builds no key', () => { + for (const type of ['constructor', 'toString', '__proto__', 'hasOwnProperty', 'valueOf']) { + assert.deepEqual(ownedKeys('s', type), [], type); + assert.deepEqual(staleKeys('s', type, 'html'), [], `${type} to html`); + } +}); + +// The link the rest of the file does not cover: that something actually calls delete. No test +// boots server.js, so while the loop lived inline there it was provably dead weight - replacing +// it with `for (const key of [])` left every unit test and the whole smoke suite green, on all +// five backends, because a stale object is not reachable over HTTP. +test('a conversion asks the backend to drop every stale key', async () => { + const storage = stubStorage(); + const dropped = await dropStaleObjects(storage, 'conv', 'html', 'redirect'); + assert.deepEqual(storage.asked, ['conv/index.html', 'conv/source.html']); + assert.deepEqual(dropped, ['conv/index.html', 'conv/source.html']); +}); + +test('a write that does not change the type asks the backend for nothing', async () => { + const storage = stubStorage(); + assert.deepEqual(await dropStaleObjects(storage, 'conv', 'md', 'md'), []); + assert.deepEqual(await dropStaleObjects(storage, 'conv', undefined, 'md'), []); + assert.deepEqual(storage.asked, []); +}); + +test('every direction asks for exactly the keys staleKeys names', async () => { + for (const from of TYPES) { + for (const to of TYPES) { + const storage = stubStorage(); + await dropStaleObjects(storage, 'conv', from, to); + assert.deepEqual(storage.asked, staleKeys('conv', from, to), `${from} to ${to}`); + } + } +}); + +// The write has already landed and meta already names the new type, so a backend that refuses +// the cleanup must not turn a successful replace into a 500 the caller would retry. It still +// tries every key rather than stopping at the first failure. +test('a delete that throws is swallowed and does not stop the rest', async () => { + const storage = stubStorage({ fails: true }); + const dropped = await dropStaleObjects(storage, 'conv', 'html', 'redirect'); + assert.deepEqual(storage.asked, ['conv/index.html', 'conv/source.html']); + assert.deepEqual(dropped, []); +}); + +// A backend that never implemented delete raises "storage.delete is not a function", which the +// same catch swallows. createStorage refuses such a backend at boot; this pins that the write +// path survives it rather than 500ing on every conversion. +test('a backend with no delete does not sink the write', async () => { + const dropped = await dropStaleObjects({}, 'conv', 'html', 'md'); + assert.deepEqual(dropped, []); +}); diff --git a/test/storage-contract.test.js b/test/storage-contract.test.js new file mode 100644 index 0000000..8ae51a9 --- /dev/null +++ b/test/storage-contract.test.js @@ -0,0 +1,96 @@ +// Every backend has to answer the whole interface in storage/index.js, and createStorage now +// checks that at boot. The check earns its keep because the type-change cleanup swallows a failed +// delete on purpose (a write that already landed must not 500), so a backend missing the method +// would warn once per conversion forever and never fail anything. +// +// Coverage split: local and sqlite are built for real here. postgres shares its whole surface with +// sqlite through makeSqlStore, so a stub driver covers it. s3 and git need a bucket and a remote, +// so neither is built on a laptop; CI boots all five, and since createStorage runs the check on +// every boot, an incomplete s3 or git fails there rather than serving. +import test from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; + +import { assertComplete } from '../storage/index.js'; +import { createAt } from '../storage/local.js'; +import { create as createSqlite } from '../storage/sqlite.js'; +import { makeSqlStore } from '../storage/sqlstore.js'; + +const METHODS = [ + 'getBuffer', + 'get', + 'head', + 'put', + 'listMetas', + 'move', + 'copySlug', + 'delete', + 'deleteSlug', +]; + +function completeStub() { + return Object.fromEntries(METHODS.map((m) => [m, () => {}])); +} + +test('assertComplete names every method a backend is missing', () => { + assert.throws( + () => assertComplete('stub', { put() {}, get() {} }), + /storage backend "stub" is missing: getBuffer, head, listMetas, move, copySlug, delete, deleteSlug/, + ); + assert.throws(() => assertComplete('stub', null), /missing/); +}); + +// A property that is present but not callable is missing as far as the app is concerned: the +// write path would reach it and throw TypeError, which the cleanup's catch would swallow. +test('a method that is not a function counts as missing', () => { + assert.throws(() => assertComplete('stub', { ...completeStub(), delete: true }), /missing: delete/); +}); + +test('the check accepts a store that answers all of it', () => { + const stub = completeStub(); + assert.equal(assertComplete('stub', stub), stub); +}); + +// If a method is added to the interface and not to this list, the list is the thing that is +// stale, so prove the two agree rather than trusting the copy. +test('this file lists the same methods the contract requires', () => { + const short = completeStub(); + delete short.delete; + assert.throws(() => assertComplete('stub', short), /missing: delete/); + for (const method of METHODS) { + const one = completeStub(); + delete one[method]; + assert.throws(() => assertComplete('stub', one), new RegExp(`missing: ${method}`), method); + } +}); + +test('local answers the whole contract', async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'artifacts-contract-')); + assertComplete('local', await createAt(root)); +}); + +test('sqlite answers the whole contract', async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'artifacts-contract-')); + process.env.SQLITE_PATH = path.join(dir, 'artifacts.db'); + assertComplete('sqlite', await createSqlite()); +}); + +// postgres and sqlite both return makeSqlStore(driver), so the wrapper is the whole surface +// either of them exposes. A driver that answers everything proves the wrapper does too. +test('the shared SQL wrapper answers the whole contract', () => { + const driver = { + kind: 'stub', + get: () => null, + size: () => null, + put: () => {}, + listMetas: () => [], + move: () => {}, + copySlug: () => {}, + delete: () => {}, + deleteSlug: () => {}, + init: () => {}, + }; + assertComplete('postgres', makeSqlStore(driver)); +}); diff --git a/test/storage-local.test.js b/test/storage-local.test.js index 4ef94b7..baa157b 100644 --- a/test/storage-local.test.js +++ b/test/storage-local.test.js @@ -77,6 +77,33 @@ test('a put keeps the mode the object already had', async () => { assert.equal((await fs.stat(abs)).mode & 0o777, 0o600); }); +test('delete removes one object and leaves the rest of the namespace alone', async () => { + const { root, store } = await tmpStore(); + await store.put('conv/meta.json', SHORT); + await store.put('conv/index.html', '<h1>was html</h1>'); + await store.put('conv/source.html', '<h1>was html</h1>'); + await store.delete('conv/source.html'); + assert.deepEqual((await fs.readdir(path.join(root, 'conv'))).sort(), ['index.html', 'meta.json']); + assert.equal(await store.getBuffer('conv/source.html'), null); + assert.equal((await store.getBuffer('conv/meta.json')).toString('utf8'), SHORT); +}); + +// A conversion runs the same delete on every backend, and the object may already be gone (an +// artifact published before the type owned that file). A throw there would turn a write that +// already landed into a 500. +test('deleting an object that is not there is not an error', async () => { + const { store } = await tmpStore(); + await store.put('conv/meta.json', SHORT); + await store.delete('conv/source.url'); + await store.delete('never-published/source.md'); +}); + +test('delete refuses a key that escapes the namespace', async () => { + const { store } = await tmpStore(); + await assert.rejects(() => store.delete('../outside.json')); + await assert.rejects(() => store.delete('/etc/passwd')); +}); + test('a scratch file a crash left behind is swept at startup and never copied', async () => { const { root, store } = await tmpStore(); await store.put('race/meta.json', SHORT); diff --git a/test/storage-sql.test.js b/test/storage-sql.test.js new file mode 100644 index 0000000..a03e34f --- /dev/null +++ b/test/storage-sql.test.js @@ -0,0 +1,74 @@ +// Unit tests for the SQL-backed stores, driven through sqlite. node:sqlite ships with the Node +// >=22 this project already requires, so this runs with no external service. postgres.js hands +// makeSqlStore the same operations against the same one-table schema, so the shared core these +// tests drive is the same code postgres runs. postgres's own SQL is not: `DELETE FROM artifacts +// WHERE key = $1` (storage/postgres.js) only ever runs under the CI backend matrix. +import test from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; + +import { create } from '../storage/sqlite.js'; + +async function tmpStore() { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'artifacts-sqlite-')); + process.env.SQLITE_PATH = path.join(dir, 'artifacts.db'); + return create(); +} + +test('delete removes one object and leaves the rest of the namespace alone', async () => { + const store = await tmpStore(); + await store.put('conv/meta.json', '{"slug":"conv"}'); + await store.put('conv/index.html', '<h1>was html</h1>'); + await store.put('conv/source.html', '<h1>was html</h1>'); + await store.delete('conv/source.html'); + assert.equal(await store.getBuffer('conv/source.html'), null); + assert.equal(await store.head('conv/source.html'), null); + assert.equal((await store.getBuffer('conv/index.html')).toString('utf8'), '<h1>was html</h1>'); + assert.deepEqual( + (await store.listMetas()).map((m) => m.slug), + ['conv'], + ); +}); + +test('deleting an object that is not there is not an error', async () => { + const store = await tmpStore(); + await store.put('conv/meta.json', '{"slug":"conv"}'); + await store.delete('conv/source.url'); + await store.delete('never-published/source.md'); + assert.equal((await store.listMetas()).length, 1); +}); + +// The prefix operations use range predicates so a key is never read as a LIKE pattern. A +// single-key delete takes an equality match, so the same worry does not apply, but a key that +// looks like a pattern must still hit only itself. +test('delete matches one key exactly', async () => { + const store = await tmpStore(); + await store.put('conv/a%b.html', 'percent'); + await store.put('conv/a_b.html', 'underscore'); + await store.put('conv/axb.html', 'literal'); + await store.delete('conv/a_b.html'); + assert.equal(await store.getBuffer('conv/a_b.html'), null); + assert.equal((await store.getBuffer('conv/a%b.html')).toString('utf8'), 'percent'); + assert.equal((await store.getBuffer('conv/axb.html')).toString('utf8'), 'literal'); +}); + +test('delete refuses a key that escapes the namespace', async () => { + const store = await tmpStore(); + await assert.rejects(() => store.delete('../outside.json')); + await assert.rejects(() => store.delete('/etc/passwd')); +}); + +test('deleteSlug still takes the whole namespace', async () => { + const store = await tmpStore(); + await store.put('conv/meta.json', '{"slug":"conv"}'); + await store.put('conv/site/index.html', 'site'); + await store.put('conv-two/meta.json', '{"slug":"conv-two"}'); + await store.deleteSlug('conv'); + assert.equal(await store.getBuffer('conv/site/index.html'), null); + assert.deepEqual( + (await store.listMetas()).map((m) => m.slug), + ['conv-two'], + ); +});