diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bdca60d..2dd7e0e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -264,6 +264,38 @@ jobs: - name: Previews reuse the one site definition run: grep -q 'import site' deploy/preview-up.sh + # #113: a preview runs on its own copy of dev, migrated by the pull + # request's own image. Each line has a silent failure: without the copy + # the preview is back on dev's database and every migration-carrying + # pull request previews as a server error; without the migrate step the + # copy carries main's schema, which is the same failure with more steps; + # without DATABASE_URL on BOTH containers — the migrator and the app — + # one of them keeps the instance .env's value and never sees the copy. + # + # Matched on the psql flag rather than the SQL alone: both files also + # MENTION these statements, in a comment and in a by-hand instruction, + # and a check a comment can satisfy is not a check. + - name: A preview gets its own database, migrated, and is pointed at it + run: | + grep -q -- '-c "create database' deploy/preview-up.sh + grep -q -- '/app/migrate.mjs' deploy/preview-up.sh + test "$(grep -c -- '--env DATABASE_URL' deploy/preview-up.sh)" -ge 2 + # And it goes away again, both ways: when the pull request closes, and — + # because an older branch's preview-up.sh knows nothing about copies — + # whenever a copy is found with no container to own it. A copy of every + # dev account's rows, left on a disk that has already filled once + # (#119), is the cost of getting this wrong in the other direction. + - name: Closing a preview drops its database + run: grep -q -- '-c "drop database if exists' deploy/preview-down.sh + - name: A copy with no container is swept up + run: grep -q 'no \$container container to own it' deploy/preview-up.sh + # The nightly copy of the databases (#168) must not copy the copies: a + # preview clone is dev's data again, and a night with two previews open + # would be three times the size of one without — which the shrink guard + # reads as a copy gone bad, and then refuses every night after it. + - name: The nightly copy skips the preview copies + run: grep -q "not like 'platform..pr..%'" deploy/backup-db.sh + # Smoke on every push and PR (SPEC §6): the database-less project only. No # DATABASE_URL_TEST here on purpose — that is what makes this job prove the # fail-closed behaviour (401s, redirects to /login, 404s) with no database at @@ -291,8 +323,9 @@ jobs: # The log is the server's whole stdout, and the dev e-mail transport # prints every message body — verification and reset links included. The # database behind those links is a throwaway container today, but the - # artifact outlives the job by a week and SPEC §8 puts PR previews on the - # shared dev database, so the links never leave the runner intact. + # artifact outlives the job by a week and the same accounts exist on dev + # and in every preview's copy of it (#113), so the links never leave the + # runner intact. - name: Redact action links from the server log if: failure() run: | @@ -381,8 +414,9 @@ jobs: # The log is the server's whole stdout, and the dev e-mail transport # prints every message body — verification and reset links included. The # database behind those links is a throwaway container today, but the - # artifact outlives the job by a week and SPEC §8 puts PR previews on the - # shared dev database, so the links never leave the runner intact. + # artifact outlives the job by a week and the same accounts exist on dev + # and in every preview's copy of it (#113), so the links never leave the + # runner intact. - name: Redact action links from the server log if: failure() run: | diff --git a/SPEC.md b/SPEC.md index 5569a23..ef89654 100644 --- a/SPEC.md +++ b/SPEC.md @@ -305,7 +305,7 @@ export function ownerKey( | Environment | Where | Database | Deployment | | ----------- | ------------------------------------------------- | -------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------- | | Local | developer machine | remote `waw` over an SSH tunnel (per developer: `platform_`, for Dawid `platform_devski`) | — | -| PR preview | dev instance, `*.dev.architektow3d.pl` | shared dev | automatic on PR open, deleted after merge; `*.dev.architektow3d.pl` (#31) | +| PR preview | dev instance, `*.dev.architektow3d.pl` | a copy of dev, taken when the preview starts and dropped with it (#113) | automatic on PR open, deleted after merge; `*.dev.architektow3d.pl` (#31) | | Dev | OVH `waw`, d2-2 (€7), `dev.architektow3d.pl` | Postgres in a container | automatic from `main` | | Prod | OVH `eu-west-par`, b3-8 (€35), `architektow3d.pl` | Managed PostgreSQL (€59) | **manual**: an approval gate in the deploy workflow, or a `vX.Y.Z` tag | @@ -337,6 +337,27 @@ export function ownerKey( plain Docker image). The one line worth paying from day one is the managed database, and not for performance: prod holds real accounts and photos, so someone else's backups and patching is the product being bought. To settle with #24. +- **A preview runs on a copy of dev, not on dev** (#113, decided 09.09.2026): when a preview + starts, `deploy/preview-up.sh` copies dev's database into `platform_pr_` with `pg_dump`, + runs the pull request's own migrator against the copy, and points the container at it; + closing the pull request drops it. Previews never migrated, so before this every pull + request that added a column its pages read previewed as a server error — twice in five days + (#112, #170), each time exactly when the preview existed to be looked at. Not "the preview + migrates the shared database": a pull request revised after its preview ran would leave a + migration in dev's journal whose hash no longer matched the file, and the next deployment of + `main` would fail on it. What a preview therefore is: dev's data as of its start, plus this + pull request's schema. Accounts created in a preview, and the rows recording what was + uploaded there, die with it. Three things follow from the copy and are enforced rather + than hoped for: dev's `sessions` and `verifications` are emptied in the copy (signing out + on dev cannot reach a copy, so a copied session would be a credential nothing could + revoke — you sign in to a preview); a delete may only touch keys under the environment's + own prefix (`src/lib/storage.ts`), because the copy's rows name dev's objects in the + bucket everything shares; and a copy with no container is dropped by the next + `preview-up.sh`, since only closing the pull request would otherwise remove one. + What stays shared: the bucket (new writes under `pr-/`), the signing key, the instance. + **The signing key being shared is the thing production must not inherit** — one + `AUTH_SECRET` across environments plus a copy of the rows is how a session from one + becomes a session in another; #24 gives production its own. - **Every wait on the database has a deadline, and each environment sets its own** (#172): the pool answers a caller it cannot give a connection to within five seconds instead of queueing them for ever, and the server cuts off a statement that runs past ten seconds or diff --git a/deploy/backup-db.sh b/deploy/backup-db.sh index 55b0225..54c1e48 100755 --- a/deploy/backup-db.sh +++ b/deploy/backup-db.sh @@ -119,8 +119,13 @@ roles=$(psql --command "select rolname from pg_roles where rolname !~ '^pg_' and # The test databases are excluded: a test run recreates them from nothing, and # copying them would double the bytes for something nobody would ever restore. +# So are the preview copies (#113): `platform_pr_` IS a copy of dev, made +# an hour ago and dropped when the pull request closes. Copying those would +# put dev's data in the object two or three times over — and, worse, would +# make the size of a night's copy depend on how many pull requests happened to +# be open, which the shrink guard below reads as a copy going bad. # `template1` and `postgres` hold nothing of ours. -databases=$(psql --command "select datname from pg_database where datallowconn and datname like 'platform\_%' and datname not like '%\_test\_%' order by 1") +databases=$(psql --command "select datname from pg_database where datallowconn and datname like 'platform\_%' and datname not like '%\_test\_%' and datname not like 'platform\_pr\_%' order by 1") if [ -z "$databases" ]; then echo "no platform_* database to copy — is this the right cluster?" >&2 exit 1 diff --git a/deploy/preview-down.sh b/deploy/preview-down.sh index 43ea3e1..c5757cc 100644 --- a/deploy/preview-down.sh +++ b/deploy/preview-down.sh @@ -1,8 +1,8 @@ #!/usr/bin/env bash # Runs ON the instance. Removes the preview of one pull request (#31): the -# container, its route, and nothing else. Safe to run twice, and safe to run -# for a preview that was never started — closing a pull request whose preview -# failed to come up must still leave the instance clean. +# container, its route, its database, and nothing else. Safe to run twice, and +# safe to run for a preview that was never started — closing a pull request +# whose preview failed to come up must still leave the instance clean. # # Expects in the environment: PR (the number). set -euo pipefail @@ -15,6 +15,7 @@ esac NAME="pr-$PR" ROUTE="/opt/platform-lite/previews/$NAME.caddy" +CLONE="platform_pr_$PR" if docker rm --force "$NAME" >/dev/null 2>&1; then echo "container $NAME removed" @@ -32,11 +33,35 @@ else echo "no route file to remove" fi +# The preview's own copy of dev (#113). After the container, so nothing is +# still connected — and WITH (FORCE) for the case where something is anyway +# (a one-off `docker run`, a psql session left open). A copy left behind is +# not free: it is dev's data again on a disk that has already filled once +# (#119), and the next preview of this pull request would drop it unread. +# +# Not fatal if it fails: the container and the route are already gone, and +# closing a pull request must not end in a red job over a database nobody is +# using. It says so loudly instead. +if docker exec -i postgres psql -U postgres -v ON_ERROR_STOP=1 -q -d postgres \ + -c "drop database if exists \"$CLONE\" with (force)" >/dev/null 2>&1; then + echo "database $CLONE dropped" +else + echo "WARNING: database $CLONE could not be dropped — drop it by hand:" >&2 + echo " docker exec postgres psql -U postgres -c 'drop database if exists \"$CLONE\" with (force)'" >&2 +fi + # What is deliberately NOT removed: objects under the `pr-/` prefix in the # bucket. Deleting them needs the S3 credentials, which this script has no # reason to hold, and they cost fractions of a cent — the bucket's lifecycle # rule already expires staged uploads. Sweeping preview prefixes belongs with -# the account-deletion work, which needs the same object-removal path. +# the account-deletion work, which needs the same object-removal path. Since +# #113 the rows naming those objects go with the database, so what is left is +# unreferenced by anything. echo "remaining previews:" docker ps --filter 'name=^pr-[0-9]+$' --format ' {{.Names}} {{.Status}}' || true + +echo "remaining preview databases:" +docker exec -i postgres psql -U postgres -At -d postgres \ + -c "select datname from pg_database where datname like 'platform\_pr\_%' order by datname" || + echo " (could not ask PostgreSQL)" diff --git a/deploy/preview-up.sh b/deploy/preview-up.sh index 6e4fbf3..3f10a04 100644 --- a/deploy/preview-up.sh +++ b/deploy/preview-up.sh @@ -14,8 +14,8 @@ set -euo pipefail : "${GH_TOKEN:?GH_TOKEN must carry a registry pull for this run}" : "${GH_ACTOR:?GH_ACTOR must name the registry user}" -# The number reaches a container name, a hostname, a file path and an S3 key -# prefix. Anything but digits belongs in none of them. +# The number reaches a container name, a hostname, a file path, an S3 key +# prefix and now a database name. Anything but digits belongs in none of them. case "$PR" in '' | *[!0-9]*) echo "STOP: PR must be digits, got: $PR" >&2; exit 1 ;; esac @@ -26,6 +26,58 @@ NAME="pr-$PR" PREVIEWS=/opt/platform-lite/previews mkdir -p "$PREVIEWS" +# A preview needs a database of its own (#113). It used to run the pull +# request's image against DEV's database, and previews never migrate: every +# pull request adding a column its pages read previewed as a server error — +# #112 first, #170 again five days later. The preview's schema has to be the +# pull request's, and dev's has to stay `main`'s, so the only honest answer is +# a copy. +# +# The copy is taken when the preview starts and dropped with it, which also +# settles what a preview IS: dev's data as of a minute ago, plus this pull +# request's migrations. Accounts made in a preview, and the rows recording +# what was uploaded there, do not outlive it. +DB_URL=$(grep -E '^DATABASE_URL=' .env | tail -1 | cut -d= -f2- || true) +: "${DB_URL:?the instance .env has no DATABASE_URL}" +# The query string is split off before the path is touched, so a future +# ?sslmode=... survives the swap of the database name instead of becoming part +# of it. +DB_BASE=${DB_URL%%\?*} +DB_QUERY=${DB_URL#"$DB_BASE"} +SOURCE_DB=${DB_BASE##*/} +CLONE="platform_pr_$PR" +PREVIEW_DB_URL="${DB_BASE%/*}/$CLONE$DB_QUERY" +DUMP="/tmp/$CLONE.sql" + +# That name reaches a `create database` statement. +case "$SOURCE_DB" in + '' | *[!a-z0-9_]*) + echo "STOP: DATABASE_URL names no plain database: $SOURCE_DB" >&2 + exit 1 + ;; +esac + +psql_pg() { + docker exec -i postgres psql -U postgres -v ON_ERROR_STOP=1 -q -d postgres "$@" +} + +# Before anything else: a copy of dev with no container is nobody's preview. +# Every failure below this line exits with the copy already made, an older +# branch's preview-down.sh knows nothing about copies at all, and a cancelled +# CI run takes its ssh with it — so the sweep, not the teardown, is what makes +# leftovers impossible to accumulate. `docker ps -a`, not `docker ps`: a +# container the OOM killer stopped is still somebody's open preview. +for left in $(psql_pg -At -c "select datname from pg_database where datname like 'platform\_pr\_%'"); do + container="pr-${left#platform_pr_}" + [ -z "$(docker ps -a --filter "name=^$container$" --format '{{.Names}}')" ] || continue + echo "dropping $left — no $container container to own it" + psql_pg -c "drop database if exists \"$left\" with (force)" || true +done +# And the dumps they were made from, which no trap catches when the process is +# killed rather than ended (this instance has no swap; the OOM killer picks by +# size, and a dump of the whole database is a fat target). +docker exec postgres sh -c 'rm -f /tmp/platform_pr_*.sql' >/dev/null 2>&1 || true + # Measured on the instance 05.09.2026: 1 vCPU, 1.9 GB, and NO SWAP. Idle, the # app holds 66 MB, PostgreSQL 58 MB, the proxy 15 MB — so steady-state memory # is not the binding constraint. Two things are. @@ -33,7 +85,7 @@ mkdir -p "$PREVIEWS" # Without swap, running out of memory does not slow the machine down, it # invokes the OOM killer, which picks a victim by size — and the biggest # process here is as likely to be PostgreSQL as the preview that caused it. -# That is dev's database, shared by every preview. +# That is dev's database, and since #113 the copies every preview runs on. # # And there is ONE core. Avatar resizing (sharp, #12) is the CPU-heavy path in # this application, and it spikes memory with it; two uploads at once already @@ -59,38 +111,102 @@ if [ "$running" -ge "$MAX_PREVIEWS" ]; then exit 1 fi -# Every setting the preview shares with dev — the database, the signing key, -# the bucket credentials — comes from the instance's own .env. Only what must -# differ is overridden below. +# Every setting the preview shares with dev — the signing key, the bucket +# credentials — comes from the instance's own .env. Only what must differ is +# overridden below. # `|| true` for the same reason: a missing key makes grep fail the pipeline, # and the explicit check below reports that far better than `set -e` does. SITE_ADDRESS=$(grep -E '^SITE_ADDRESS=' .env | tail -1 | cut -d= -f2- || true) : "${SITE_ADDRESS:?the instance .env has no SITE_ADDRESS}" HOSTNAME_="$NAME.$SITE_ADDRESS" +# Everything this run leaves behind if it does not finish. The copy goes too +# unless the preview actually came up: a copy of every dev account's rows, +# owned by nothing, is the one leftover worth being strict about. +started=0 +leave_nothing() { + docker logout ghcr.io >/dev/null 2>&1 || true + docker exec postgres rm -f "$DUMP" >/dev/null 2>&1 || true + [ "$started" = 1 ] || + psql_pg -c "drop database if exists \"$CLONE\" with (force)" >/dev/null 2>&1 || + true +} echo "$GH_TOKEN" | docker login ghcr.io -u "$GH_ACTOR" --password-stdin -trap 'docker logout ghcr.io >/dev/null 2>&1 || true' EXIT +trap leave_nothing EXIT INT TERM HUP docker pull "$APP_IMAGE" +# The old container goes first, before its database is taken away from under +# it. Every step after this can fail; what must never happen is a container +# left running and serving against a database that is being replaced — the +# same invariant remote-deploy.sh is built around, which is why it migrates +# before it swaps. docker rm --force "$NAME" >/dev/null 2>&1 || true + +# WITH (FORCE) disconnects whatever is still on the old copy. +echo "copying $SOURCE_DB into $CLONE" +psql_pg -c "drop database if exists \"$CLONE\" with (force)" +psql_pg -c "create database \"$CLONE\"" +# Dump to a file and restore from the file, rather than `pg_dump | psql`: a +# pipeline reports only its LAST command's status, so a failed dump would pass +# for a successful restore of nothing — and the preview would look like a +# working one with an empty database. (`create database ... template` is the +# other way to copy and is refused while anything is connected to the source; +# dev's own container is.) +# +# umask 077 because the file is every dev account's e-mail address and +# password hash in plain SQL, and VERBOSITY=terse because psql prints the +# offending ROW as context when a restore fails — into a CI log this +# repository publishes. +docker exec postgres sh -c "umask 077; pg_dump -U postgres --no-owner --no-privileges -f '$DUMP' '$SOURCE_DB'" +docker exec -i postgres psql -U postgres -v ON_ERROR_STOP=1 -q \ + --set VERBOSITY=terse -d "$CLONE" -f "$DUMP" +docker exec postgres rm -f "$DUMP" + +# What must not survive the copy. A session is valid for thirty days (A2) and +# both environments sign with the same key, so a copy of dev's sessions is a +# set of credentials that signing out on dev can no longer revoke — the copy +# is frozen and nothing reaches into it. Nobody needs dev's live sessions to +# look at a preview; you sign in to it. +docker exec -i postgres psql -U postgres -v ON_ERROR_STOP=1 -q \ + --set VERBOSITY=terse -d "$CLONE" -c "truncate table sessions, verifications" + +# The pull request's own migrator, against the pull request's own database. +# `set -e` ends the preview here if it fails, rather than starting a container +# whose every page will answer 500 — which is the whole of #113. +# +# Only the one variable it reads: migrate.mjs wants DATABASE_URL and nothing +# else, and a throwaway container running an unmerged branch has no business +# holding the mail key or the bucket credentials. Exported rather than written +# as --env NAME=value, because /proc//cmdline is world-readable on this +# machine and the value carries the database password. +echo "applying this pull request's migrations to $CLONE" +export DATABASE_URL="$PREVIEW_DB_URL" +docker run --rm --network platform --memory 512m \ + --env DATABASE_URL \ + --entrypoint node "$APP_IMAGE" /app/migrate.mjs + docker run --detach --name "$NAME" \ --restart unless-stopped \ --network platform \ --memory 512m \ --env-file /opt/platform-lite/.env \ + --env DATABASE_URL \ --env "APP_URL=https://$HOSTNAME_" \ --env "S3_PREFIX=$NAME/" \ --env "APP_ENV=preview" \ --env "EMAIL_API_KEY=" \ "$APP_IMAGE" >/dev/null +started=1 -# APP_ENV=preview is load-bearing twice over. It keeps X-Robots-Tag: noindex -# on (only `production` is indexed, A7), and lib/email.ts counts `preview` -# among the environments that never send: a preview is reachable by anyone +# APP_ENV=preview is load-bearing three times over. It keeps X-Robots-Tag: +# noindex on (only `production` is indexed, A7); lib/email.ts counts `preview` +# among the environments that never send — a preview is reachable by anyone # with the link and would otherwise mail real verification messages to any -# address typed into it, from our domain, against our quota. Blanking -# EMAIL_API_KEY on top of that is belt and braces — the key has no business -# in a throwaway container even unused. +# address typed into it, from our domain, against our quota; and the R360 +# collector refuses to run in one, which matters more since #113 gave the +# preview a COPY of dev's rows naming dev's objects in the shared bucket. +# Blanking EMAIL_API_KEY on top of that is belt and braces — the key has no +# business in a throwaway container even unused. printf '%s {\n\timport site %s\n}\n' "$HOSTNAME_" "$NAME" > "$PREVIEWS/$NAME.caddy" docker exec platform-lite-caddy-1 caddy reload --config /etc/caddy/Caddyfile @@ -98,7 +214,7 @@ docker exec platform-lite-caddy-1 caddy reload --config /etc/caddy/Caddyfile for attempt in $(seq 1 60); do status=$(docker inspect --format '{{.State.Health.Status}}' "$NAME" 2>/dev/null || echo starting) if [ "$status" = "healthy" ]; then - echo "preview ready: https://$HOSTNAME_ (healthy after ${attempt}s)" + echo "preview ready: https://$HOSTNAME_ (healthy after ${attempt}s) — on its own copy of $SOURCE_DB, so accounts and uploads made here die with the preview" exit 0 fi if [ "$status" = "unhealthy" ]; then diff --git a/docs/dev-environment.md b/docs/dev-environment.md index a86fce4..350a097 100644 --- a/docs/dev-environment.md +++ b/docs/dev-environment.md @@ -170,6 +170,43 @@ The bucket and its objects live independently of the instance and survive its lo Prod restore (managed database, snapshots) is defined by task #24 and will extend this document. +## The databases on the instance + +The cluster holds more than the two databases cloud-init made: + +| Database | What it is | +| ------------------------ | -------------------------------------------------- | +| `platform_` | dev's own, and what a developer's tunnel points at | +| `platform_test_` | the integration suite's (SPEC §6) | +| `platform_pr_` | one per open pull request preview (#113) | + +A preview's database is a `pg_dump` copy of dev taken when the preview starts, +migrated by the pull request's own image, and dropped by `preview-down.sh` +when the pull request closes. Two consequences worth knowing before you look +at one: + +- **A preview shows dev's data as of its start.** Anything added to dev + afterwards is not there, and anything added in the preview — an account, a + work — exists only there and goes away with it. The objects those uploads + put in the bucket do NOT: they stay under `pr-/` with nothing naming + them (the same tail as #34 and #111). +- **You have to sign in to a preview.** Its copy is restored with `sessions` + and `verifications` emptied: a session copied out of dev would stay valid in + the copy after it was revoked on dev, and nothing could reach in to end it. +- **A preview cannot delete dev's objects**, even though its rows name them: + `src/lib/storage.ts` refuses a delete whose key is outside the environment's + own `S3_PREFIX` and logs what it refused. +- **A copy can be left behind** if a preview is removed some other way than + `preview-down.sh` — a cancelled CI run, or an older branch whose + `preview-down.sh` predates #113. The next `preview-up.sh` drops every copy + with no container of its own, so this heals on the next preview; to see or + do it by hand: + + ``` + ssh docker exec postgres psql -U postgres -Atc "select datname from pg_database where datname like 'platform\_pr\_%'" + ssh docker exec postgres psql -U postgres -c "drop database if exists platform_pr_ with (force)" + ``` + ## Decisions behind this setup - **SSH tunnel instead of an allow-listed IP** (30.08.2026) — survives home-IP rotation, diff --git a/src/db/client.ts b/src/db/client.ts index 87c08f5..a7cdc66 100644 --- a/src/db/client.ts +++ b/src/db/client.ts @@ -136,8 +136,8 @@ export function poolConfig( // and 0 means no deadline at all. This is the whole of #172 in one line. connectionTimeoutMillis: connectMs, // A connection nobody has used for half a minute is given back. Previews - // come and go all day and each holds its own pool against dev's single - // PostgreSQL; idle connections there are pure occupancy. + // come and go all day and each holds its own pool against the one + // PostgreSQL on the instance; idle connections there are pure occupancy. idleTimeoutMillis: 30_000, statement_timeout: statementMs, lock_timeout: lockMs, @@ -146,8 +146,9 @@ export function poolConfig( // that long means the caller is gone. idle_in_transaction_session_timeout: idleTxMs, // Who is holding the connection, as pg_stat_activity will show it: dev - // and every preview share one database (#113), so "one of them is - // queueing" is only actionable if the row says which. + // and every preview share one PostgreSQL — a database each since #113, + // but one instance and one core — so "something is queueing" is only + // actionable if the row says which of them. application_name: connectionLabel(use), }; } diff --git a/src/db/schema.ts b/src/db/schema.ts index 2267caa..2202587 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -568,8 +568,9 @@ export const files = pgTable( // #49: the key this row's object was ACTUALLY written under, prefix and // all. The address used to be rebuilt at read time from sha256 + ext + // the READING environment's S3_PREFIX, so one row resolved to a different - // object in every environment: a PR preview, which shares dev's database - // and bucket and differs only in that prefix, 404'd on every avatar. It + // object in every environment: a PR preview, which runs on a copy of + // dev's rows against the same bucket and differs only in that prefix, + // 404'd on every avatar. It // also meant that changing S3_PREFIX anywhere silently unhooked every // existing file from its object, with the row and the object both still // there. diff --git a/src/lib/profile.test.ts b/src/lib/profile.test.ts index 79a40b8..8d5a0b7 100644 --- a/src/lib/profile.test.ts +++ b/src/lib/profile.test.ts @@ -342,8 +342,8 @@ describe("setAvatar + getProfile (A4, G2)", () => { expect(view.avatar?.fileId).toBe(uploaded.original.fileId); }); - // #49: a PR preview shares dev's database and its bucket, and differs only - // in S3_PREFIX. Rebuilding the address at read time made the same row point + // #49: a PR preview runs on a copy of dev's rows against the same bucket, + // and differs only in S3_PREFIX. Rebuilding the address at read time made the same row point // at a different object per environment, so every avatar 404'd on a // preview. The row has to name the object the upload actually wrote. it("resolves to the object the upload wrote, from an environment with another prefix", async () => { diff --git a/src/lib/r360/collector-timer.ts b/src/lib/r360/collector-timer.ts index 474636c..0987d67 100644 --- a/src/lib/r360/collector-timer.ts +++ b/src/lib/r360/collector-timer.ts @@ -4,13 +4,17 @@ // runs is collectAllUnfinishedFrameSets; this file only says when, and // where never. // -// Never in a preview. Previews share dev's database (#113), and the -// collector deletes RECORDS, not just bytes: one scoping mistake there -// would cost dev's data. So the block is a hard allow-list on APP_ENV — -// the deployed environments that own their database — not a setting that -// could be left on: a preview, CI, a developer's machine, a missing or a -// mistyped value all run nothing. Pure, with the collection handed in, so -// the schedule is tested without a database or a clock. +// Never in a preview. Since #113 a preview has a database of its own — but +// it is a COPY of dev's, naming dev's objects in the bucket every +// environment shares, and this collector deletes RECORDS and the bytes +// behind them. A sweep there would reason about dev's objects with a copy +// of dev's rows that nothing keeps current. (lib/storage.ts confines a +// delete to the environment's own key prefix, which is the belt; this is +// the braces, and the older of the two.) So the block is a hard allow-list +// on APP_ENV — the deployed environments that own their data — not a +// setting that could be left on: a preview, CI, a developer's machine, a +// missing or a mistyped value all run nothing. Pure, with the collection +// handed in, so the schedule is tested without a database or a clock. /** Every twelve hours (#126). */ export const COLLECT_EVERY_MS = 12 * 60 * 60 * 1000; diff --git a/src/lib/storage.test.ts b/src/lib/storage.test.ts index 840c0f1..ab327f8 100644 --- a/src/lib/storage.test.ts +++ b/src/lib/storage.test.ts @@ -3,6 +3,8 @@ import { contentKey, createMemoryStorage, createS3Storage, + deletesConfinedToOurPrefix, + type FileStorage, IMMUTABLE_CACHE_CONTROL, isNotFound, isStorageConfigured, @@ -484,3 +486,62 @@ describe("getStorage environment wiring", () => { }, ); }); + +describe("deletes confined to our own prefix (#113)", () => { + function spyStorage() { + const deleted: string[][] = []; + const one: string[] = []; + const storage: FileStorage = { + ...createMemoryStorage().storage, + async deleteObjects(keys: string[]) { + deleted.push(keys); + }, + async deleteObject(key: string) { + one.push(key); + }, + }; + return { storage, deleted, one }; + } + + it("passes through what this environment wrote", async () => { + const { storage, deleted } = spyStorage(); + const confined = deletesConfinedToOurPrefix(storage, "pr-113/"); + await confined.deleteObjects(["pr-113/a/one.webp", "pr-113/a/two.webp"]); + expect(deleted).toEqual([["pr-113/a/one.webp", "pr-113/a/two.webp"]]); + }); + + it("refuses a key belonging to another environment", async () => { + // A preview runs on a COPY of dev's rows, and those rows name dev's + // objects. Deleting a work there would otherwise take dev's object with + // it and leave dev's own row pointing at nothing — permanent, and + // invisible to the environment that lost it. + const { storage, deleted, one } = spyStorage(); + const said = vi.spyOn(console, "error").mockImplementation(() => {}); + const confined = deletesConfinedToOurPrefix(storage, "pr-113/"); + await confined.deleteObjects(["devski/a/one.webp", "pr-113/a/mine.webp"]); + await confined.deleteObject("devski/a/two.webp"); + expect(deleted).toEqual([["pr-113/a/mine.webp"]]); + expect(one).toEqual([]); + expect(said.mock.calls.flat().join(" ")).toContain("devski/a/one.webp"); + said.mockRestore(); + }); + + it("calls the bucket not at all when nothing is ours", async () => { + const { storage, deleted } = spyStorage(); + const said = vi.spyOn(console, "error").mockImplementation(() => {}); + const confined = deletesConfinedToOurPrefix(storage, "pr-113/"); + await confined.deleteObjects(["devski/a/one.webp"]); + expect(deleted).toEqual([]); + said.mockRestore(); + }); + + it("leaves production alone: the bare bucket is its own", async () => { + const { storage, deleted } = spyStorage(); + // Production's prefix is blank (SPEC §4), so every key is under it — the + // wrapper must not stand between production and its own objects. + const confined = deletesConfinedToOurPrefix(storage, ""); + expect(confined).toBe(storage); + await confined.deleteObjects(["a/one.webp"]); + expect(deleted).toEqual([["a/one.webp"]]); + }); +}); diff --git a/src/lib/storage.ts b/src/lib/storage.ts index 4894b95..ebdec92 100644 --- a/src/lib/storage.ts +++ b/src/lib/storage.ts @@ -583,13 +583,54 @@ let instance: FileStorage | undefined; export function getStorage(): FileStorage { if (!instance) { - instance = createS3Storage({ - endpoint: requireEnv(S3_VARIABLES.endpoint), - region: requireEnv(S3_VARIABLES.region), - bucket: requireEnv(S3_VARIABLES.bucket), - accessKeyId: requireEnv(S3_VARIABLES.accessKeyId), - secretAccessKey: requireEnv(S3_VARIABLES.secretAccessKey), - }); + instance = deletesConfinedToOurPrefix( + createS3Storage({ + endpoint: requireEnv(S3_VARIABLES.endpoint), + region: requireEnv(S3_VARIABLES.region), + bucket: requireEnv(S3_VARIABLES.bucket), + accessKeyId: requireEnv(S3_VARIABLES.accessKeyId), + secretAccessKey: requireEnv(S3_VARIABLES.secretAccessKey), + }), + keyPrefix(), + ); } return instance; } + +/** + * An environment may delete only what it could have written (#113 review). + * + * One bucket is shared by dev, every preview and — one day — production, and + * SPEC §4 gives each its own key prefix. Nothing enforced that on the way + * OUT: a delete names the key the row records, and a preview now carries a + * COPY of dev's rows, so deleting a work there would have deleted dev's + * objects out from under dev's own rows. Permanent, silent, and invisible to + * the environment that lost them. + * + * Production's prefix is blank, which makes this a no-op there — as it should + * be: the bare bucket IS production's, and it is the environments scoped + * inside it that must stay inside. + */ +export function deletesConfinedToOurPrefix( + storage: FileStorage, + prefix: string, +): FileStorage { + if (prefix === "") return storage; + const ours = (key: string): boolean => { + if (key.startsWith(prefix)) return true; + console.error( + `[storage] refusing to delete ${key}: this environment writes under "${prefix}" (#113)`, + ); + return false; + }; + return { + ...storage, + async deleteObject(key) { + if (ours(key)) await storage.deleteObject(key); + }, + async deleteObjects(keys) { + const mine = keys.filter(ours); + if (mine.length > 0) await storage.deleteObjects(mine); + }, + }; +} diff --git a/tasks/plan.md b/tasks/plan.md index a7e2afb..4739387 100644 --- a/tasks/plan.md +++ b/tasks/plan.md @@ -78,16 +78,19 @@ in under 5 minutes (manual walkthrough); e2e green. 21. [#21](https://github.com/Devski/platform-lite/issues/21) Dockerfile (standalone) + deploy on push to the dev instance, G9 (`infra`) - ~~[#31](https://github.com/Devski/platform-lite/issues/31) PR preview deployments on the dev instance~~ — **done 05.09.2026**: `pr-.dev.architektow3d.pl`, named sites over HTTP-01 (no wildcard certificate, so no DNS plugin), shared dev database and a `pr-/` key prefix. Two at a time — the instance has one core and no swap. Previews never send e-mail. -- [#113](https://github.com/Devski/platform-lite/issues/113) PR previews: a database cloned - from dev per preview, not the shared one (`deployment`). **Raised in priority 12.09.2026** - after it cost a second review cycle. Filed 09.09.2026 when the preview of #112 answered - with a server error: it runs the pull request's image against dev's database, and previews - never migrate. #170 hit it again — every page listing works answered 500 on that preview - because the migration adding `works.position` had not run there. It is not a flake but a - rule: **any pull request carrying a migration looks broken on its own preview**, exactly - when the preview exists to be looked at. The workaround both times was to apply the - migration to dev by hand first, which is only safe while migrations stay expand-only (G6) - and someone is there to reason about it. +- ~~[#113](https://github.com/Devski/platform-lite/issues/113) PR previews: a database cloned + from dev per preview, not the shared one~~ — **done 12.09.2026**. Filed 09.09.2026 when the + preview of #112 answered with a server error: it ran the pull request's image against dev's + database, and previews never migrate. #170 hit it again — every page listing works answered + 500 on that preview because the migration adding `works.position` had not run there. It was + not a flake but a rule: **any pull request carrying a migration looked broken on its own + preview**, exactly when the preview existed to be looked at. Now `preview-up.sh` copies dev + into `platform_pr_` with `pg_dump`, runs the pull request's own migrator against the + copy, and points the container at it; `preview-down.sh` drops it. A preview is therefore + dev's data as of its start plus this pull request's schema, and what is created inside one + dies with it. The copy is taken with a dump rather than `create database ... template`, + which PostgreSQL refuses while anything is connected to the source — dev's own container + always is. - [#111](https://github.com/Devski/platform-lite/issues/111) Preview cleanup loses the race with a CI run still in flight, and the orphan blocks the two-preview cap (`bug`). - ~~[#172](https://github.com/Devski/platform-lite/issues/172) The database has no deadlines~~