From 3fa2a31167f700c15e362c9a1908a166b7c11a71 Mon Sep 17 00:00:00 2001 From: "Zonily Jame (@kuyazee)" Date: Thu, 13 Aug 2026 11:21:22 +0800 Subject: [PATCH 1/2] fix(T2.1.9): a type change drops the objects the old type owned saveArtifact only ever wrote, so 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 every serve path keys off meta.type, but they stayed at rest and on the git backend they stayed in commit history after the artifact was deleted. Adds delete(key) to the storage interface on all five backends: local removes the file with force so a missing key is not an error, git delegates to local, s3 reuses the delete helper that already treats 404 as done, and the two SQL stores take an equality match rather than the range predicate the prefix operations need. lib/artifact-files.js owns which objects each type owns and what a conversion drops, so a unit test can walk all twenty ordered pairs without a server. server.js runs the deletes after meta.json lands and before flush: 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. A delete that fails is logged, not thrown, so a cleanup problem cannot turn a replace that already landed into a 500 the caller would retry. Red first: five conversions on one slug left source.html, source.jsx, source.md, source.tsx and source.url side by side; each hop now leaves exactly what the new type owns. --- .github/workflows/smoke.sh | 38 ++++++++++++++++ lib/artifact-files.js | 34 +++++++++++++++ server.js | 16 ++++++- storage/git.js | 1 + storage/index.js | 5 +++ storage/local.js | 7 +++ storage/postgres.js | 6 +++ storage/s3.js | 6 +++ storage/sqlite.js | 6 +++ storage/sqlstore.js | 10 ++++- test/artifact-files.test.js | 87 +++++++++++++++++++++++++++++++++++++ test/storage-local.test.js | 27 ++++++++++++ test/storage-sql.test.js | 73 +++++++++++++++++++++++++++++++ 13 files changed, 313 insertions(+), 3 deletions(-) create mode 100644 lib/artifact-files.js create mode 100644 test/artifact-files.test.js create mode 100644 test/storage-sql.test.js diff --git a/.github/workflows/smoke.sh b/.github/workflows/smoke.sh index 44e03fa..be83184 100644 --- a/.github/workflows/smoke.sh +++ b/.github/workflows/smoke.sh @@ -478,6 +478,44 @@ 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 + 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":"

step html

","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 '

step md

' || 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 () =>

step jsx

' +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 () =>

step tsx

' +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" "converted artifact serves the redirect" +[ "$(curl -s "$BASE/a/ci-conv/source")" = 'https://example.com/step-redirect' ] \ + || fail "redirect conversion lost source.url" +convert_to html '

step html again

' +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" +# md renders per request, so an md that became html must not still be handed back as markdown +if curl -s "$BASE/a/ci-conv?raw=1" | grep -q 'step md'; then fail "html artifact still serves the old markdown"; fi +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/lib/artifact-files.js b/lib/artifact-files.js new file mode 100644 index 0000000..a4c2332 --- /dev/null +++ b/lib/artifact-files.js @@ -0,0 +1,34 @@ +// 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, and on +// the git backend it was bloat that stayed in commit history after the artifact was deleted. + +// The extension `source.` gets per type. /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`); + if (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)); +} diff --git a/server.js b/server.js index b94b1f0..0374a1d 100644 --- a/server.js +++ b/server.js @@ -31,6 +31,7 @@ import { validateCredentials, parseKeyInput, } from './lib/auth.js'; +import { SOURCE_EXT, staleKeys } from './lib/artifact-files.js'; import { createConfigStore } from './lib/config.js'; import { ApiError } from './lib/errors.js'; import { artifactExpired } from './lib/expiry.js'; @@ -218,7 +219,7 @@ 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' }; +// SOURCE_EXT and the type-change cleanup rule live in lib/artifact-files.js, imported above. // 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 +737,19 @@ 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, never before: a crash in between then leaves the old record whole rather + // than a listed artifact whose body is gone. Before flush, so git carries the deletions in the + // same commit as the write. + for (const key of staleKeys(finalSlug, existing?.type, type)) { + // Bloat, not correctness. The write has already landed, so a cleanup that fails must not + // turn a successful replace into a 500 the caller would retry. + try { + await storage.delete(key); + } catch (err) { + console.warn(`storage: could not drop ${key} after a type change: ${err.message}`); + } + } 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; 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..c9a9721 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; 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 `/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. 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 `/`. 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 // copySlug(srcSlug, dstSlug) -> Promise +// delete(key) -> Promise // deleteSlug(slug) -> Promise // init() -> Promise (create table + connectivity probe) // close?() -> Promise @@ -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..bdb281b --- /dev/null +++ b/test/artifact-files.test.js @@ -0,0 +1,87 @@ +// 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 { ownedKeys, staleKeys, SOURCE_EXT } from '../lib/artifact-files.js'; + +const TYPES = ['html', 'jsx', 'tsx', 'md', 'redirect']; + +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'), []); +}); 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', '

was html

'); + await store.put('conv/source.html', '

was html

'); + 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..7697824 --- /dev/null +++ b/test/storage-sql.test.js @@ -0,0 +1,73 @@ +// 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 six operations against the same one-table schema, so proving the shared +// core here is as close as a laptop gets to covering both. +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', '

was html

'); + await store.put('conv/source.html', '

was html

'); + 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'), '

was html

'); + 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'], + ); +}); From 0a90379dc2d56a2fcf17434ed42f3ae27e27c483 Mon Sep 17 00:00:00 2001 From: "Zonily Jame (@kuyazee)" Date: Thu, 13 Aug 2026 11:46:04 +0800 Subject: [PATCH 2/2] fix(T2.1.9): prove the cleanup runs, and fail a backend that cannot do it The four review lenses on 3fa2a31. Two gaps were real and both are closed here. The cleanup had no test proving anything called it. Replacing the loop body with `for (const key of [])` left all 96 unit tests and the whole smoke suite green on every backend, because a stale object is not reachable over HTTP and no test boots server.js. The loop moved into `dropStaleObjects` in lib/artifact-files.js, where a fake storage can record the keys it asks for. The same neuter now fails 3 tests. A backend with no `delete` raised "storage.delete is not a function" straight into the catch that exists so a failed cleanup cannot 500 a write that already landed, so it would have warned once per conversion forever and failed nothing. `createStorage` now checks every method the app calls and refuses the boot, which is how the backend already treats a store it cannot reach. test/storage-contract.test.js drives local and sqlite for real and the shared SQL wrapper for postgres; s3 and git are covered by the boot check under the CI matrix. Smaller findings from the same pass: - pipeStream answered 500 when a read opened after the object was deleted. local stats then opens, so a conversion landing in between produced a hard 500 on a plain GET. It answers 404 now, which the comment above serveObject already promised and which the rename path has always given during its own window. - ownedKeys and the /a/:slug/source route both looked SOURCE_EXT up bare, so a hand-edited meta.type of "constructor" built a key out of a function body. Both use Object.hasOwn now. - TYPES is read off SOURCE_EXT, so a sixth type cannot be publishable and invisible to the cleanup. dashboard-check.mjs imports the same table instead of grepping server.js for the array. - The update_artifact description said title resets when omitted, which T2.1.7 made false, and neither it nor docs/mcp.md said a type change now deletes files. - The warn on a failed delete says what it means for the artifact and that nothing retries, matching the other operator lines in this repo. - Dropped a smoke assertion that could not fire, and the duplicate ok-line label. The commit message on 3fa2a31 says the old bytes "stayed in commit history"; that reads as if this fixes it. It does not. A deletion on the git backend is another commit, so the bytes stay in history and `git log -p` on the remote still hands them back. lib/artifact-files.js says so now. --- .github/workflows/dashboard-check.mjs | 15 ++--- .github/workflows/smoke.sh | 4 +- docs/mcp.md | 12 ++-- lib/artifact-files.js | 43 ++++++++++-- server.js | 39 +++++------ storage/index.js | 28 +++++++- test/artifact-files.test.js | 71 +++++++++++++++++++- test/storage-contract.test.js | 96 +++++++++++++++++++++++++++ test/storage-sql.test.js | 5 +- 9 files changed, 268 insertions(+), 45 deletions(-) create mode 100644 test/storage-contract.test.js 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 be83184..334a4cb 100644 --- a/.github/workflows/smoke.sh +++ b/.github/workflows/smoke.sh @@ -505,14 +505,12 @@ curl -s "$BASE/a/ci-conv?raw=1" | grep -q 'step tsx' || fail "tsx conversion los 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" "converted artifact serves the redirect" +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" -# md renders per request, so an md that became html must not still be handed back as markdown -if curl -s "$BASE/a/ci-conv?raw=1" | grep -q 'step md'; then fail "html artifact still serves the old markdown"; fi curl -sf -X DELETE "$BASE/api/artifacts/ci-conv" -H "$AUTH" > /dev/null echo "ok: every type conversion serves the type it was given" 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 index a4c2332..9075a47 100644 --- a/lib/artifact-files.js +++ b/lib/artifact-files.js @@ -3,10 +3,16 @@ // 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, and on -// the git backend it was bloat that stayed in commit history after the artifact was deleted. +// 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 extension `source.<ext>` gets per type. /a/:slug/source reads it back. +// 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 @@ -21,7 +27,10 @@ const BAKES_INDEX = new Set(['html', 'jsx', 'tsx']); export function ownedKeys(slug, type) { const keys = []; if (BAKES_INDEX.has(type)) keys.push(`${slug}/index.html`); - if (SOURCE_EXT[type]) keys.push(`${slug}/source.${SOURCE_EXT[type]}`); + // 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; } @@ -32,3 +41,29 @@ export function staleKeys(slug, oldType, newType) { 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 0374a1d..d9ce445 100644 --- a/server.js +++ b/server.js @@ -31,7 +31,7 @@ import { validateCredentials, parseKeyInput, } from './lib/auth.js'; -import { SOURCE_EXT, staleKeys } from './lib/artifact-files.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'; @@ -211,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; @@ -219,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; -// SOURCE_EXT and the type-change cleanup rule live in lib/artifact-files.js, imported above. // Pinned versions shared with the jsx shell. `external=react` keeps packages on // the shell's React instance — separate copies cause "Invalid hook call". @@ -738,18 +739,9 @@ async function storeArtifact(finalSlug, { content, type = 'html', title, descrip contentType: 'application/json', }); // The old type's objects are unreachable now that meta names the new one, so drop them. After - // the meta write, never before: a crash in between then leaves the old record whole rather - // than a listed artifact whose body is gone. Before flush, so git carries the deletions in the - // same commit as the write. - for (const key of staleKeys(finalSlug, existing?.type, type)) { - // Bloat, not correctness. The write has already landed, so a cleanup that fails must not - // turn a successful replace into a 500 the caller would retry. - try { - await storage.delete(key); - } catch (err) { - console.warn(`storage: could not drop ${key} after a type change: ${err.message}`); - } - } + // 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; @@ -1192,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); } @@ -1401,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', @@ -1858,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/index.js b/storage/index.js index c9a9721..ee23c59 100644 --- a/storage/index.js +++ b/storage/index.js @@ -16,7 +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; a key that is gone is not an error +// 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) // } @@ -78,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() { @@ -95,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/test/artifact-files.test.js b/test/artifact-files.test.js index bdb281b..85733cd 100644 --- a/test/artifact-files.test.js +++ b/test/artifact-files.test.js @@ -4,10 +4,24 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { ownedKeys, staleKeys, SOURCE_EXT } from '../lib/artifact-files.js'; +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']); @@ -85,3 +99,58 @@ test('a type with no source extension drops nothing', () => { 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-sql.test.js b/test/storage-sql.test.js index 7697824..a03e34f 100644 --- a/test/storage-sql.test.js +++ b/test/storage-sql.test.js @@ -1,7 +1,8 @@ // 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 six operations against the same one-table schema, so proving the shared -// core here is as close as a laptop gets to covering both. +// 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';