From d8d5cf83979c4cd8718f2465796ec074acfc5da3 Mon Sep 17 00:00:00 2001 From: claude-hdb Date: Tue, 14 Jul 2026 22:37:01 +0000 Subject: [PATCH] feat: diff and apply a database's backup schedule (#51) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backup schedules were write-only, filed under "known limitations" on the claim that "live Coolify state doesn't expose it back". The parenthesis was load-bearing and false: a schedule is not on the database's own GET, but it was never meant to be — it has its own route, GET /databases/{uuid}/backups, which cast had been POSTing to all along and had simply never read. The cost was exact. A database created before its `backup:` block was declared never got one (apply set the schedule only inside the create branch); a schedule deleted in the UI was invisible; and the `--full` diff that gates a production cutover passed with an unbacked-up production database. Shape settled from the source rather than the vendored spec, which documents the body as "Content is very complex. Will be implemented later.": DatabasesController@database_backup_details_uuid (v4.1.2) returns a raw Eloquent collection — a JSON array of ScheduledDatabaseBackup rows, columns per $fillable (uuid, enabled, frequency, database_backup_retention_amount_locally). `frequency` round-trips verbatim: the controller validates it and stores $request->only(...) unchanged, with no mutator on the model. The "diffing it would flag spurious drift" fear was a guess about a read nobody had performed. - `backup` becomes a diffed field like any other (resolve.ts), replacing the side channel that carried it around the diff. - The live side reads the route (coolify.ts, fetchLive), and apply sets the schedule on UPDATE as well as create — POST or PATCH, decided by a read. - A disabled schedule is a row that backs nothing up: neither clean nor absent. cast diffs it and re-enables it. Degrades honestly, since no live box was probed: an unreachable or unrecognized response can only ever produce "declared, NOT compared — verify in the Coolify UI", never invented drift and never a clean bill on an unread database. On the write side the same failure raises rather than guessing — POSTing blind would duplicate a schedule that may already exist. --- docs/semantics.md | 106 ++++++++++++++-- src/cli.ts | 206 ++++++++++++++++++++++++++----- src/coolify.ts | 121 +++++++++++++++++++ src/diff.ts | 46 +++++++ src/resolve.ts | 58 ++++++--- test/backup-cli.test.ts | 254 +++++++++++++++++++++++++++++++++++++++ test/coolify.test.ts | 109 ++++++++++++++++- test/diff.test.ts | 133 ++++++++++++++++++++ test/live-lookup.test.ts | 109 ++++++++++++++++- test/resolve.test.ts | 53 +++++--- test/wire.test.ts | 222 +++++++++++++++++++++++++++++++++- 11 files changed, 1337 insertions(+), 80 deletions(-) create mode 100644 test/backup-cli.test.ts diff --git a/docs/semantics.md b/docs/semantics.md index c399192..d5787b3 100644 --- a/docs/semantics.md +++ b/docs/semantics.md @@ -506,6 +506,80 @@ Coolify says so in the same response (*"can cause routing conflicts and unpredic behavior"*). Nothing in cast can send that flag, and no retry may ever set it — if it is ever wanted, it is an explicit operator act in the UI, not a tool's decision. +## Backup schedules + +A manifest database's `backup` block (`frequency`, `retention`) is a **diffed +field like any other**: compared on every run, written on create *and* on +update. Declaring `backup:` on a database that already exists starts backing it +up — which is what every reader of that manifest already assumed it did. + +It did not always work that way, and the reason is worth keeping: the schedule +used to be write-only, applied inside `apply`'s create branch and then never +looked at again, because *"live Coolify state doesn't expose it back"*. That was +false. A schedule is not on the database's own `GET` — but it was never supposed +to be. It has **its own route**, which cast had been POSTing to all along and had +simply never read: + +``` +GET /databases/{uuid}/backups ← list a database's schedules +POST /databases/{uuid}/backups ← create +PATCH /databases/{uuid}/backups/{scheduled_backup_uuid} ← update +``` + +The cost of not looking was exact: a database created before its `backup:` block +was declared never got one, a schedule deleted in the UI was invisible, and the +`--full` diff that gates a production cutover passed with an unbacked-up +production database (#51). + +**What the route returns.** The vendored OpenAPI documents the body as *"Content +is very complex. Will be implemented later."*, so the shape comes from the source: +`DatabasesController@database_backup_details_uuid` (v4.1.2) returns +`ScheduledDatabaseBackup::…->with('executions')->where('database_id', …)->get()` +straight to `response()->json()` — a **JSON array of raw Eloquent rows** (no API +resource, no `removeSensitiveData`), whose columns are the model's `$fillable`: +`uuid`, `enabled`, `save_s3`, `frequency`, +`database_backup_retention_amount_locally`, … plus an eager-loaded `executions` +array cast ignores. `retention` is +`database_backup_retention_amount_locally` — the same field cast has always sent +on create. + +**`frequency` round-trips verbatim**, which is what makes it diffable at all: the +controller *validates* it (`validate_cron_expression`, which only returns a bool) +and then stores `$request->only($backupConfigFields)` unchanged, with no mutator +on the model. `"0 3 * * *"` reads back as `"0 3 * * *"`; the preset words +(`daily`, `weekly`, …) read back as themselves. The old "diffing it would flag +spurious drift every run" fear was a guess about a read nobody had performed. + +**A disabled schedule is not a backup.** A row with `enabled: false` exists but +backs nothing up, so it is neither clean (it diffs against a declared block) nor +absent (`apply` PATCHes it, rather than adding a second schedule). Every cast +write asserts `enabled: true`. + +**What is still NOT compared**, and says so on screen when it applies: + +- **An unreadable answer.** If the route is unreachable, or answers a shape cast + does not recognize, cast reports `backup schedule for database + declared, NOT compared — verify in the Coolify UI` and treats it as neither + drift nor clean. An absence of evidence is not evidence of drift: cast will not + invent a change it cannot see, nor certify a database it could not read. On the + write side the same read failure **raises** rather than guessing — POSTing blind + would duplicate a schedule that may already exist, and skipping is the silent + no-op this whole section exists to kill. +- **More than one schedule.** A manifest declares one; a database carrying + several is outside that vocabulary, and choosing one to compare against would + be a coin toss reported as a fact. Reported, not compared, not written. +- **The S3 target.** Coolify returns the storage as `s3_storage_id` (an int) and + takes it as a UUID, the same unmappable pair as `destination_id` (see + Placement). cast **asserts** `save_s3: true` + the environment's + `s3_destination` on every write and can never verify it afterwards. +- **An undeclared schedule.** A live schedule on a database whose manifest says + nothing about backups is left alone, unremarked — `apply` never removes. + +**A backup change redeploys the database.** `apply` redeploys any resource it +mutates, and a schedule change is a mutation of the database, so changing +`frequency` restarts the container. Consistent with every other field, and worth +knowing before you edit a schedule on a live production database. + ## Instance selection **The Coolify a command talks to is an explicit, named value** — not a property @@ -791,8 +865,12 @@ and rebuild a *different box*. Per resource, it names what was seen and could no be written: `destination_id` (which Docker network — no destinations API in 4.1.2 to resolve it to the UUID `destination_uuid:` wants, #21), service hostnames (no flat `domains` on a Coolify 4.1.2 service), Basic Auth / custom Traefik labels, -build and deploy command overrides, backup schedules (not exposed on a database's -GET — **a rebuild has no backups until you declare them**), database kinds cast +build and deploy command overrides, backup schedules (**a rebuild has no backups +until you declare them** — *not* because they cannot be read, which is what this +line used to say and #51 disproved, but because `inventory --emit-draft` has not +yet been taught to read them: `GET /databases/{uuid}/backups` answers, and `diff` +and `apply` now use it. Until the draft path does too, a blueprint still omits +them and still says so), database kinds cast does not model (MySQL, MariaDB, MongoDB, KeyDB, Dragonfly, ClickHouse — named, never silently dropped), env var names a cast template cannot express, names **reserved by the platform** (`SOURCE_COMMIT`, `COOLIFY_*` — suppressed, never @@ -946,15 +1024,23 @@ one used — verified against a live private clone. **Known limitations, not defects:** -- **Backup schedules are create-time only.** A manifest database's `backup` - block (`frequency`, `retention`) is applied only when the database is - first created; it is deliberately kept out of the diffed `fields` (live - Coolify state doesn't expose it back, so diffing it would flag spurious - drift every run — breaking idempotency). Changing a schedule on an - existing database is a runbook act, done by hand in the Coolify UI. +- **~~Backup schedules are create-time only.~~** **Corrected (#51).** This entry + used to claim that a database's `backup` block was create-time only and kept + out of the diffed `fields` because *"live Coolify state doesn't expose it + back"*. The parenthesis was load-bearing and it was false — `GET + /databases/{uuid}/backups` is a route, and cast had been POSTing to it all + along without ever reading it. Backup schedules are now compared on every run + and written on create *and* update; see **Backup schedules** above for what + is still not compared (an unreadable answer, several schedules, the S3 + target) and how each says so out loud. Kept here, struck through, because + this entry is *why nobody looked*: a limitation filed as a defect gets fixed, + and a defect filed as a limitation does not. - **A service's `domains` cannot be applied via the API in Coolify 4.1.2, - and is deliberately kept out of the diffed `fields` for the same - idempotency reason as backup schedules above.** The `/services` + and is deliberately kept out of the diffed `fields` for idempotency.** (This + used to cite backup schedules as its precedent; it can't any more — that + reasoning was disproved above. This one was re-checked and holds: Coolify + 4.1.2 exposes no flat `domains` on a service, on any route. If that is ever + disproved the same way, `domains` belongs in `fields` too.) The `/services` create/update payload takes a structured per-container `urls` list, not the manifest's flat `domains: string[]`, and the manifest has no per-container name to build that list correctly from — so cast diff --git a/src/cli.ts b/src/cli.ts index 92eb827..70124e8 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -384,10 +384,63 @@ export type LiveLookup = environment: string; }; +// Attach the live backup schedule to a database, or the reason there isn't one +// to attach. Split out from fetchLive so the "cast could not read this" paths — +// the ones that must never lie in either direction — are all visible together. +// +// The four answers, and why each is what it is: +// +// unreadable -> backupNotCompared. Says nothing, claims nothing, prints. +// no schedule -> no `backup` in fields. Read cleanly: a declared backup is +// then REAL drift, and apply creates the schedule. This is +// the case the old side-channel design could never see, and +// the reason a rebuilt database silently had no backups. +// one schedule -> compared, like any other field. +// >1 schedule -> backupNotCompared. cast's manifest declares ONE schedule; +// a database carrying several is outside that vocabulary, +// and picking one to compare against would be a coin toss +// reported as a fact. +// +// A DISABLED schedule is deliberately NOT treated as "no schedule": the row +// exists (so apply must PATCH it, not POST a second one) but it backs nothing +// up (so it must not read as clean). Carrying `enabled: false` into the compared +// value gets both — it diffs against a desired block that implies enabled, and +// the update path re-enables it. +export async function attachBackup( + client: CoolifyClient, + db: Live, +): Promise { + const read = await client.databaseBackupSchedules(db.uuid); + if (read === undefined) { + db.backupNotCompared = + "GET /databases/{uuid}/backups was unreachable or returned a shape cast does not recognize"; + return; + } + if (read.length > 1) { + db.backupNotCompared = `Coolify holds ${read.length} schedules for this database; a manifest declares one`; + return; + } + const schedule = read[0]; + if (!schedule) return; // read cleanly: no schedule. Absence IS the answer. + db.fields.backup = { + // Same key order as the desired side (resolve.ts) — computeDiff compares + // by JSON.stringify. `enabled` rides along only when false, so the ordinary + // healthy case is a two-key object on both sides and compares equal. + frequency: schedule.frequency, + retention: schedule.retention, + ...(schedule.enabled ? {} : { enabled: false }), + }; +} + export async function fetchLive( client: CoolifyClient, projectName: string, envName: string, + // Backups cost one extra GET per database, so only the callers that actually + // compare them ask for them: `diff` and `apply`. The read-side sweeps + // (inventory, capture, smoke) walk every project on a box and would pay it on + // every database for an answer they never look at. + opts: { backups?: boolean } = {}, ): Promise { const projects = (await client.get("/projects")) as Array<{ uuid: string; @@ -454,15 +507,18 @@ export async function fetchLive( destinationId: typeof i.destination_id === "number" ? i.destination_id : undefined, })); - return { - found: true, - live: [ - ...map("application", env.applications), - ...map("database", env.postgresqls), - ...map("database", env.redis), - ...map("service", env.services), - ], - }; + const live = [ + ...map("application", env.applications), + ...map("database", env.postgresqls), + ...map("database", env.redis), + ...map("service", env.services), + ]; + if (opts.backups) { + for (const db of live.filter((l) => l.kind === "database")) { + await attachBackup(client, db); + } + } + return { found: true, live }; } // Why `diff` refuses instead of reporting an empty live side: see LiveLookup. @@ -910,7 +966,7 @@ async function runProject( ); } const secrets = decryptSecrets(store, keyFileFor(ctx.envName)); - let { desired, resolvedEnvs, backupSchedules } = desiredFromManifest( + let { desired, resolvedEnvs } = desiredFromManifest( checkout, ctx.envName, secrets, @@ -930,7 +986,10 @@ async function runProject( parseYaml(readFileSync(ctx.hostnameOverlay, "utf8")), ); } - const lookup = await fetchLive(ctx.client, projectName, coolifyEnv); + // `backups: true` — this is the one path that compares them (see fetchLive). + const lookup = await fetchLive(ctx.client, projectName, coolifyEnv, { + backups: true, + }); // apply and diff take opposite (and both correct) positions on absence: // apply is *allowed* to be the thing that brings a project into existence, // so [] is a legitimate starting point. diff is only ever a claim about @@ -1012,7 +1071,6 @@ async function runProject( bindingEnv: ctx.envName, destinationUuid: projectBinding?.destination_uuid, s3DestinationUuid: ctx.binding.s3_destination, - backupSchedules, visibleUuids, }); const { mutated } = await applyPlan(report, desired, exec); @@ -2422,13 +2480,36 @@ export function defaultDatabaseImage(type: string, version: string): string { return `${repo}:${version}-alpine`; } +// The desired backup schedule, narrowed out of the untyped `fields` bag that +// both createResource and updateFields are handed. Anything that is not a +// complete, well-typed schedule reads as "none declared" — the manifest schema +// (manifest.ts) already requires both keys, so a partial object here would mean +// a bug upstream, and writing half a schedule is worse than writing none. +export function desiredBackup( + fields: Record, +): { frequency: string; retention: number } | undefined { + const b = fields.backup as + | { frequency?: unknown; retention?: unknown } + | undefined; + if (!b || typeof b !== "object") return undefined; + if (typeof b.frequency !== "string" || typeof b.retention !== "number") + return undefined; + return { frequency: b.frequency, retention: b.retention }; +} + export function databaseApiFields( fields: Record, ): Record { // /databases/postgresql and /databases/redis accept no `type` param (the // endpoint path already encodes it) and no `version` param at all — only // `image`, a literal Docker image string. - const { type, version, ...rest } = fields; + // + // `backup` is stripped because it is not a column on the database at all: it + // is a row on a different route (/databases/{uuid}/backups), written by + // writeBackupSchedule. It rides in `fields` so that it can be DIFFED like + // any other field; it must never reach the database's own create/update body, + // which rejects unknown fields. + const { type, version, backup: _backup, ...rest } = fields; return { ...rest, ...(typeof version === "string" @@ -2796,7 +2877,6 @@ export function buildExecutor( // hosts two projects, is the right answer. destinationUuid?: string; s3DestinationUuid?: string; // raw UUID from environments.yaml — no storage API exists to resolve names - backupSchedules: Record; // The live resources of THIS project + environment, by uuid — everything cast // can see. Read by exactly one thing: the domain-conflict message, which has to // say whether the resource holding the domain is inside the applied project or @@ -2834,6 +2914,69 @@ export function buildExecutor( ctx.projectName, ctx.envName, ); + // The body of a backup-schedule write, shared by the create and update paths + // so the two cannot drift apart — the create path having been the only one + // for so long is precisely how the update path came to not exist. + // + // `save_s3` + `s3_storage_uuid` are asserted on every write, not compared: + // Coolify returns the storage as `s3_storage_id` (an int) and takes it as a + // uuid, the same unmappable pair as destination_id (see Placement), so cast + // can state its intent here but can never verify it afterwards. Declaring + // `backup:` in a manifest means "backed up, to the environment's S3" — that + // is what this writes, on create and on update alike. + const backupBody = ( + label: string, + schedule: { frequency: string; retention: number }, + ): Record => { + if (!ctx.s3DestinationUuid) { + throw new Error( + `database ${label} declares a backup schedule but environments.yaml has no s3_destination UUID for this environment`, + ); + } + return { + frequency: schedule.frequency, + database_backup_retention_amount_locally: schedule.retention, + save_s3: true, + s3_storage_uuid: ctx.s3DestinationUuid, + // Asserted on every write. A schedule row that exists with enabled=false + // backs nothing up, and a manifest that declares `backup:` is asking for + // backups, not for a disabled row that looks like backups. + enabled: true, + }; + }; + // Make a database's live schedule match the manifest — the half of this that + // did not exist. `apply` used to write a schedule ONLY inside the create + // branch, so adding `backup:` to an already-live database produced a clean + // run and zero backups; that is the defect (#51). + // + // POST creates, PATCH updates, and which one is right depends on a read — so + // this reads first. When the read fails it RAISES rather than guessing: the + // alternatives are POSTing (which duplicates the schedule if one was in fact + // there) or skipping (which is the silent no-op being fixed). An apply that + // promised to set a backup schedule and could not must say so and stop. + const reconcileBackupSchedule = async ( + dbUuid: string, + schedule: { frequency: string; retention: number }, + ): Promise => { + const body = backupBody(dbUuid, schedule); + const existing = await client.databaseBackupSchedules(dbUuid); + if (existing === undefined) { + throw new Error( + `database ${dbUuid}: cannot set the declared backup schedule — GET /databases/${dbUuid}/backups was unreachable or returned an unrecognized shape, so cast cannot tell whether a schedule already exists (creating one blindly would risk a duplicate). Set it in the Coolify UI, or re-run when the API is reachable.`, + ); + } + if (existing.length > 1) { + throw new Error( + `database ${dbUuid}: Coolify holds ${existing.length} backup schedules for this database and a manifest declares one — cast will not guess which to update. Resolve in the Coolify UI (runbook act).`, + ); + } + const current = existing[0]; + if (current) { + await client.patch(`/databases/${dbUuid}/backups/${current.uuid}`, body); + return; + } + await client.post(`/databases/${dbUuid}/backups`, body); + }; // The two instance-wide constraints a create can die on, both of them invisible // from cast's project-scoped view, both of them arriving at the FIRST create — // after apply has already made the project and the environment. One wrapper, and @@ -2926,19 +3069,15 @@ export function buildExecutor( ...databaseApiFields(fields), }, )) as { uuid: string }; - const schedule = ctx.backupSchedules[change.name]; + // A database that was created a moment ago provably has no schedule, + // so this POSTs rather than going through reconcileBackupSchedule — + // no read to do, and no read that could fail and abort a create. + const schedule = desiredBackup(fields); if (schedule) { - if (!ctx.s3DestinationUuid) { - throw new Error( - `database ${change.name} declares a backup schedule but environments.yaml has no s3_destination UUID for this environment`, - ); - } - await client.post(`/databases/${res.uuid}/backups`, { - frequency: schedule.frequency, - database_backup_retention_amount_locally: schedule.retention, - save_s3: true, - s3_storage_uuid: ctx.s3DestinationUuid, - }); + await client.post( + `/databases/${res.uuid}/backups`, + backupBody(change.name, schedule), + ); } return res.uuid; } @@ -2966,7 +3105,20 @@ export function buildExecutor( : kind === "service" ? serviceApiFields(fields) : databaseApiFields(fields); - await client.patch(`/${base}/${uuid}`, apiFields); + // A backup-only drift strips to an empty body (databaseApiFields drops + // `backup`, which belongs to another route) — and PATCHing a database + // with `{}` is a write that says nothing. Skip it; the schedule below is + // the actual change. + if (Object.keys(apiFields).length > 0) { + await client.patch(`/${base}/${uuid}`, apiFields); + } + // The declared schedule is applied on UPDATE, not only on create. This is + // what makes adding `backup:` to an existing database do what every reader + // of that manifest already assumes it does (#51). + if (kind === "database") { + const schedule = desiredBackup(fields); + if (schedule) await reconcileBackupSchedule(uuid, schedule); + } }, async syncEnv(uuid, kind, env) { // The reserved-name rule at the wire (reserved.ts). Nothing can reach here diff --git a/src/coolify.ts b/src/coolify.ts index 9016445..c667b39 100644 --- a/src/coolify.ts +++ b/src/coolify.ts @@ -20,6 +20,105 @@ export class HttpError extends Error { } } +// A database's scheduled backup, as cast is able to read it back. +// +// `retention` is Coolify's `database_backup_retention_amount_locally` — the +// same field cast has always POSTed on create. `enabled` is carried because a +// DISABLED schedule is a row that exists and backs nothing up: reporting that +// database as backed-up is the one lie this whole path exists to prevent. +export type LiveBackup = { + uuid: string; + frequency: string; + retention: number; + enabled: boolean; +}; + +// The result of trying to read a database's schedules. The two absences are +// NOT the same fact and must never collapse into one another (the LiveLookup +// lesson, one level down): +// +// [] — read cleanly, this database has NO schedule. Trustworthy, and +// therefore real drift if the manifest declares one. +// undefined — NOT READ: transport error, or a body cast does not recognize. +// Says nothing. Must never become "no backups" (which would +// invent drift, and make apply POST a duplicate schedule) nor +// "backed up" (which would pass a cutover on an unbacked-up db). +export type BackupRead = LiveBackup[] | undefined; + +// Coolify's int columns arrive as ints, but a tinyint `enabled` has no cast on +// ScheduledDatabaseBackup (v4.1.2 casts() covers only the two float storage +// fields), so it can serialize as 1/0 rather than true/false. Accept both; only +// an explicit falsey value disables. An ABSENT `enabled` is read as enabled — +// Coolify's own create path defaults it to true (DatabasesController, v4.1.2). +function readEnabled(raw: Record): boolean { + const v = raw.enabled; + if (v === undefined || v === null) return true; + return !(v === false || v === 0 || v === "0"); +} + +// Strict on purpose: a value cast cannot read EXACTLY is not coerced into a +// guess, it collapses the whole read to `undefined` (= "not compared", said out +// loud). Silence about a backup is the failure being fixed here; a wrong number +// about one would be worse than the silence. +function readInt(v: unknown): number | undefined { + if (typeof v === "number" && Number.isInteger(v)) return v; + if (typeof v === "string" && /^\d+$/.test(v)) return Number(v); + return undefined; +} + +// Parse GET /databases/{uuid}/backups. +// +// The vendored OpenAPI documents this body as "Content is very complex. Will be +// implemented later." — so the shape here comes from the source instead: +// DatabasesController@database_backup_details_uuid (v4.1.2) ends with +// +// $backupConfig = ScheduledDatabaseBackup::ownedByCurrentTeamAPI($teamId) +// ->with('executions')->where('database_id', $database->id)->get(); +// return response()->json($backupConfig); +// +// i.e. a raw Eloquent collection — a JSON ARRAY of ScheduledDatabaseBackup rows +// (no API resource, no removeSensitiveData), whose columns are the model's +// $fillable: uuid, enabled, save_s3, frequency, +// database_backup_retention_amount_locally, ... plus an eager-loaded +// `executions` array cast ignores. +// +// `frequency` round-trips VERBATIM: the controller validates it +// (validate_cron_expression, which only returns a bool) and then stores +// $request->only($backupConfigFields) unchanged — there is no mutator on the +// model. So "0 3 * * *" reads back as "0 3 * * *", and the preset words +// (daily, weekly, ...) read back as themselves. That is what makes this field +// diffable at all, and it is the fact the old "spurious drift" fear assumed +// away without checking. +export function parseBackupSchedules(raw: unknown): BackupRead { + // Not an array = not the documented collection. Unknown answer, not "none". + if (!Array.isArray(raw)) return undefined; + const schedules: LiveBackup[] = []; + for (const item of raw) { + if (typeof item !== "object" || item === null) return undefined; + const row = item as Record; + const uuid = row.uuid; + const frequency = row.frequency; + const retention = readInt(row.database_backup_retention_amount_locally); + // One unreadable row makes the whole read unreadable. A partial list would + // be indistinguishable from a complete one to every caller downstream, and + // the caller most worth protecting is the one asking "is this backed up?". + if ( + typeof uuid !== "string" || + typeof frequency !== "string" || + retention === undefined + ) { + return undefined; + } + schedules.push({ + uuid, + frequency, + retention, + enabled: readEnabled(row), + }); + } + return schedules; +} + export class CoolifyClient { constructor( private readonly baseUrl: string, @@ -282,6 +381,28 @@ export class CoolifyClient { ); } + // The SAME route as databaseBackups above, PARSED for the diff/apply half of + // the story (#51). destroy reads the raw body because it decides shape-meaning + // itself (readBackupState); diff/apply need a settled `frequency`/`retention` + // to compare and write, so this parses on top of the one HTTP call rather than + // duplicating it — one place fetches, two callers read it their own way. + // + // `undefined` means "could not read", which is a DIFFERENT fact from "has + // none" (see BackupRead). Every failure lands on `undefined`, INCLUDING a 404: + // it is tempting to read 404 as "no backups" (fetchLive does exactly that for a + // missing environment), but here a 404 is Coolify saying *the database* was not + // found, never "the database has no schedules" — the handler returns a plain + // `[]` for that, with a 200. Reading 404 as "none" would let a mistyped uuid + // report an unbacked-up database as clean, and let apply POST a second schedule + // onto a database that already had one. + async databaseBackupSchedules(uuid: string): Promise { + try { + return parseBackupSchedules(await this.databaseBackups(uuid)); + } catch { + return undefined; + } + } + async deploy(uuid: string): Promise { await this.post(`/deploy?uuid=${encodeURIComponent(uuid)}`); } diff --git a/src/diff.ts b/src/diff.ts index 1c8f51c..82e1bdd 100644 --- a/src/diff.ts +++ b/src/diff.ts @@ -24,6 +24,18 @@ export type Live = { // Putting it in `fields` would diff a UUID against an int and report drift // that can never be resolved. See Placement. destinationId?: number; + // Set ONLY when this database declares a `backup` block that cast could not + // read back (GET /databases/{uuid}/backups was unreachable, or answered a + // shape cast does not recognize — see BackupRead). The string is the reason, + // printed verbatim. + // + // Presence of this means: DO NOT COMPARE `backup` for this resource. Leaving + // `backup` merely absent from `fields` would NOT be equivalent — it would + // diff desired-against-nothing and report confident drift on a database that + // may well be perfectly backed up. An unreadable answer must produce neither + // drift nor a clean bill; it produces a line on the report. computeDiff is + // where that is enforced. + backupNotCompared?: string; }; export type FieldDiff = { field: string; @@ -94,6 +106,11 @@ export type DiffReport = { // structural mode reads no env vars at all, so it can find none — and says so. reserved: ReservedVar[]; placement: Placement; + // Databases whose declared `backup` block cast could not verify this run. + // NOT drift (nothing was read, so nothing can be claimed) and so NOT counted + // against `clean` — but printed on every run that has any, because the whole + // point is that the assumption goes on screen at the moment it is made. + backupsNotCompared: { name: string; reason: string }[]; clean: boolean; }; @@ -210,6 +227,7 @@ export function computeDiff( opts: { declaredDestination?: string } = {}, ): DiffReport { const changes: Change[] = []; + const backupsNotCompared: { name: string; reason: string }[] = []; for (const d of desired) { const l = live.find((x) => x.kind === d.kind && x.name === d.name); if (!l) { @@ -233,7 +251,17 @@ export function computeDiff( }); continue; } + // The unreadable-backup escape hatch. `backup` is dropped from the + // comparison entirely — not diffed against `undefined`, which is what + // "just leave it out of live.fields" would silently mean, and which would + // report drift cast has no evidence for and let apply write a schedule it + // never checked for. See Live.backupNotCompared. + if (l.backupNotCompared && "backup" in d.fields) { + backupsNotCompared.push({ name: d.name, reason: l.backupNotCompared }); + } + const skipBackup = l.backupNotCompared !== undefined; const fieldDiffs: FieldDiff[] = Object.entries(d.fields) + .filter(([field]) => !(skipBackup && field === "backup")) .filter(([field, value]) => !eq(value, l.fields[field])) .map(([field, value]) => ({ field, @@ -265,6 +293,7 @@ export function computeDiff( orphans, reserved, placement, + backupsNotCompared, // A split project is drift, and drift is not clean — the same disposition // as an orphan: reported, counted, and NOT repaired (apply moves nothing // between networks; see renderDiff). @@ -273,6 +302,12 @@ export function computeDiff( // it is a live defect. The box it sits on is deploying green and reporting // the wrong commit, and a `diff` that answered "clean" over it would be the // last chance anyone had to notice. + // + // `backupsNotCompared` is deliberately NOT in this sum, and it is the one + // exception to the rule the line above states: it is an absence of evidence, + // not evidence of drift, and a run that failed because a read failed would be + // a run operators learn to force past. It gets a line instead — an + // unmissable one — rather than a non-zero exit. clean: changes.length === 0 && orphans.length === 0 && @@ -348,6 +383,17 @@ export function renderDiff(report: DiffReport): string { " and `apply` never deletes — so this one is yours to remove, by hand, in the UI.", ); } + // The honest fallback, on the same principle as the placement line below: cast + // read for the schedule and did not understand the answer, so it says so here + // rather than dropping the field and letting a clean report imply a backed-up + // database. A `backup:` block that produced no line above and no line here IS + // compared, and IS clean. + for (const b of report.backupsNotCompared) { + lines.push( + `backup schedule for database ${b.name} declared, NOT compared — verify in the Coolify UI`, + ` (${b.reason})`, + ); + } const { placement } = report; if (placement.split) { lines.push( diff --git a/src/resolve.ts b/src/resolve.ts index cb0cc91..497b4bd 100644 --- a/src/resolve.ts +++ b/src/resolve.ts @@ -341,7 +341,6 @@ export function desiredFromManifest( ): { desired: Desired[]; resolvedEnvs: Record; - backupSchedules: Record; } { const manifest = loadManifest(join(checkoutDir, ".infra", "manifest.yaml")); const envSpec = manifest.environments[envName]; @@ -352,10 +351,6 @@ export function desiredFromManifest( } const desired: Desired[] = []; const resolvedEnvs: Record = {}; - const backupSchedules: Record< - string, - { frequency: string; retention: number } - > = {}; const reserved: ReservedHit[] = []; const resolveEnvFile = ( name: string, @@ -428,13 +423,33 @@ export function desiredFromManifest( desired.push({ kind: "database", name, - fields: { type: db.type, ...(db.version ? { version: db.version } : {}) }, + fields: { + type: db.type, + ...(db.version ? { version: db.version } : {}), + // `backup` is a DIFFED FIELD, like any other. + // + // It used to be routed around `fields` into a side channel, on the + // stated grounds that "live Coolify state doesn't expose it back" — so + // diffing it would flag spurious drift forever. That premise was false: + // it is not on the database's own GET, but GET /databases/{uuid}/backups + // is a route (cast has always POSTed to it), and frequency/retention + // round-trip verbatim through it. The side channel is what made a + // `backup:` block added to an EXISTING database do nothing, silently, + // and made `diff --full` pass on a production database with no backups. + // + // Key order matters: computeDiff compares by JSON.stringify, and the + // live side (fetchLive in cli.ts) builds this same object in this same + // order. Do not reorder one without the other. + ...(db.backup + ? { + backup: { + frequency: db.backup.frequency, + retention: db.backup.retention, + }, + } + : {}), + }, }); - if (db.backup) - backupSchedules[name] = { - frequency: db.backup.frequency, - retention: db.backup.retention, - }; } for (const [name, svc] of Object.entries(envSpec.services ?? {})) { if (svc.domains && svc.domains.length > 0) { @@ -450,12 +465,19 @@ export function desiredFromManifest( desired.push({ kind: "service", name, - // domains dropped from fields, same as database `backup` above: the - // live side (projectLiveFields in cli.ts) can't read service domains - // and the write side (serviceApiFields) drops them, so keeping - // domains in fields makes every domain-bearing service diff as a - // perpetual update. Hostnames stay a manual Coolify UI act (warned - // above). + // domains dropped from fields: the live side (projectLiveFields in + // cli.ts) can't read service domains and the write side + // (serviceApiFields) drops them, so keeping domains in fields makes + // every domain-bearing service diff as a perpetual update. Hostnames + // stay a manual Coolify UI act (warned above). + // + // This USED to cite database `backup` as its precedent. It no longer + // can: `backup` was dropped on the same reasoning and the reasoning + // turned out to be false there (a read route existed, unlooked-for — + // see the databases loop above). The difference is that this one was + // re-checked: Coolify 4.1.2 genuinely exposes no flat `domains` on a + // service, on any route. If that is ever disproved the same way, this + // belongs in `fields` too. fields: { type: svc.type }, env: resolveEnvFile(name, svc.env_template), }); @@ -464,5 +486,5 @@ export function desiredFromManifest( // resolved env that carries a reserved name is not desired state, it is a // suppression of the platform's own value dressed up as one. See reserved.ts. assertNoReservedEnvNames(reserved); - return { desired, resolvedEnvs, backupSchedules }; + return { desired, resolvedEnvs }; } diff --git a/test/backup-cli.test.ts b/test/backup-cli.test.ts new file mode 100644 index 0000000..c730b96 --- /dev/null +++ b/test/backup-cli.test.ts @@ -0,0 +1,254 @@ +import { execFileSync, spawn } from "node:child_process"; +import { mkdirSync, mkdtempSync, writeFileSync } from "node:fs"; +import { createServer } from "node:http"; +import type { AddressInfo } from "node:net"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeAll, describe, expect, it } from "vitest"; + +// Backup schedules, end to end: manifest -> `cast diff` -> what it prints and +// what it exits with. The unit tests prove each half (coolify.ts parses the +// route's body; computeDiff compares it; the executor writes it) — this proves +// they are wired to each other through the real binary, which is the half a +// type checker cannot see. +// +// The case that matters most is the third one. Before #51, a live database with +// NO backup schedule and a manifest that declared one produced a CLEAN diff: +// the field was never read, so the drift did not exist. A `--full` diff gating a +// production cutover passed on an unbacked-up production database. + +let recipient: string; +let keyFile: string; + +beforeAll(() => { + const dir = mkdtempSync(join(tmpdir(), "cast-age-")); + keyFile = join(dir, "age.key"); + execFileSync("age-keygen", ["-o", keyFile], { stdio: "pipe" }); + recipient = execFileSync("age-keygen", ["-y", keyFile], { + encoding: "utf8", + }).trim(); +}); + +type Stub = { url: string; close: () => Promise }; +const stubs: Stub[] = []; + +// `backups` is the knob: what GET /databases/db-1/backups answers. The live +// payload is otherwise a byte-for-byte match for the manifest below, so anything +// the diff reports is the backup schedule and nothing else. +// +// an array -> Coolify's real answer shape (raw ScheduledDatabaseBackup rows) +// "boom" -> a 500, i.e. cast asked and could not be answered +async function stubCoolify(backups: unknown[] | "boom"): Promise { + const server = createServer((req, res) => { + const path = (req.url ?? "").replace("/api/v1", ""); + const json = (body: unknown) => { + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify(body)); + }; + if (path === "/teams/current") return json({ id: 0, name: "Root Team" }); + if (path === "/projects") return json([{ uuid: "p1", name: "incubator" }]); + if (path === "/projects/p1/staging") + return json({ + postgresqls: [ + { + name: "postgres", + uuid: "db-1", + database_type: "standalone-postgresql", + image: "postgres:17-alpine", + destination_id: 1, + }, + ], + }); + if (path === "/databases/db-1/backups") { + if (backups === "boom") { + res.writeHead(500); + return res.end("boom"); + } + return json(backups); + } + res.writeHead(404); + res.end("{}"); + }); + await new Promise((r) => { + server.listen(0, "127.0.0.1", r); + }); + const stub: Stub = { + url: `http://127.0.0.1:${(server.address() as AddressInfo).port}`, + close: () => + new Promise((r) => { + server.close(() => r()); + }), + }; + stubs.push(stub); + return stub; +} + +afterEach(async () => { + await Promise.all(stubs.splice(0).map((s) => s.close())); +}); + +// A row as Coolify serializes one (DatabasesController@database_backup_details_uuid +// returns raw Eloquent rows, v4.1.2). +const row = (over: Record = {}) => ({ + uuid: "sched-1", + enabled: true, + save_s3: true, + frequency: "0 3 * * *", + database_backup_retention_amount_locally: 7, + ...over, +}); + +const MANIFEST = `project: incubator +environments: + staging: + applications: {} + databases: + postgres: + type: postgresql + version: "17" + backup: { frequency: "0 3 * * *", retention: 7 } +`; + +function fixture(url: string) { + const checkout = mkdtempSync(join(tmpdir(), "cast-co-")); + mkdirSync(join(checkout, ".infra", "env"), { recursive: true }); + writeFileSync(join(checkout, ".infra", "manifest.yaml"), MANIFEST); + + const state = mkdtempSync(join(tmpdir(), "cast-state-")); + mkdirSync(join(state, "secrets")); + writeFileSync( + join(state, ".coolify.env"), + `COOLIFY_BASE_URL="${url}"\nCOOLIFY_ACCESS_TOKEN="t"\n`, + ); + execFileSync("age", ["-r", recipient, "-o", "incubator.staging.env.age"], { + input: "\n", + cwd: join(state, "secrets"), + stdio: ["pipe", "pipe", "pipe"], + }); + writeFileSync( + join(state, "environments.yaml"), + [ + "environments:", + " staging:", + " server: shared-box", + " team: { id: 0, name: Root Team }", + " s3_destination: s3-abc", + "github_apps:", + " incubator: hdb-coolify", + "", + ].join("\n"), + ); + return { checkout, state }; +} + +function run(f: { + checkout: string; + state: string; +}): Promise<{ code: number; output: string }> { + return new Promise((resolve) => { + const child = spawn( + "node", + [ + "dist/cli.js", + "diff", + "heavy-duty/incubator", + "--env", + "staging", + "--path", + f.checkout, + "--state", + f.state, + ], + { + stdio: ["pipe", "pipe", "pipe"], + env: { ...process.env, CAST_AGE_KEY_FILE_STAGING: keyFile }, + }, + ); + let output = ""; + child.stdout.on("data", (d) => { + output += String(d); + }); + child.stderr.on("data", (d) => { + output += String(d); + }); + child.on("close", (code) => resolve({ code: code ?? 0, output })); + }); +} + +describe("cast diff — backup schedules (#51)", () => { + it("is clean when the live schedule matches the manifest", async () => { + const r = await run(fixture((await stubCoolify([row()])).url)); + expect(r.code).toBe(0); + expect(r.output).toContain("clean"); + // Compared, so it says nothing. Silence here is now EARNED, which is the + // whole difference: before #51 it was silence about a read never made. + expect(r.output).not.toMatch(/NOT compared/); + }); + + // The defect, end to end. + it("reports drift when the live database has no schedule at all", async () => { + const r = await run(fixture((await stubCoolify([])).url)); + expect(r.code).toBe(1); // this exact run used to exit 0 + expect(r.output).toContain("update database postgres"); + expect(r.output).toMatch(/backup:/); + expect(r.output).toContain('"frequency":"0 3 * * *"'); + }); + + it("reports drift when the live schedule differs", async () => { + const r = await run( + fixture( + ( + await stubCoolify([ + row({ + frequency: "0 9 * * *", + database_backup_retention_amount_locally: 2, + }), + ]) + ).url, + ), + ); + expect(r.code).toBe(1); + expect(r.output).toMatch(/backup:/); + }); + + it("reports drift when the schedule exists but is switched off", async () => { + const r = await run( + fixture((await stubCoolify([row({ enabled: false })])).url), + ); + // A disabled schedule backs nothing up. It must not read as clean. + expect(r.code).toBe(1); + expect(r.output).toMatch(/backup:/); + }); + + // The honest fallback. cast asked, could not be answered, and says so — on a + // run it still calls clean, because an absence of evidence is not evidence of + // drift. What it must never do is stay quiet and let the clean line imply a + // backed-up database. + it("says 'declared, NOT compared' out loud when the read fails", async () => { + const r = await run(fixture((await stubCoolify("boom")).url)); + expect(r.output).toContain( + "backup schedule for database postgres declared, NOT compared — verify in the Coolify UI", + ); + // Not invented drift: cast read nothing, so it claims nothing. + expect(r.output).not.toContain("update database postgres"); + expect(r.code).toBe(0); + }); + + // The same failure to read, but from a body cast does not recognize rather + // than a transport error — including the literal placeholder the vendored + // OpenAPI documents for this route. + it("says 'NOT compared' on a body it cannot recognize, rather than guessing", async () => { + const r = await run( + fixture( + ( + await stubCoolify([ + "Content is very complex. Will be implemented later.", + ]) + ).url, + ), + ); + expect(r.output).toMatch(/NOT compared/); + expect(r.output).not.toContain("update database postgres"); + expect(r.code).toBe(0); + }); +}); diff --git a/test/coolify.test.ts b/test/coolify.test.ts index 852ba43..552459a 100644 --- a/test/coolify.test.ts +++ b/test/coolify.test.ts @@ -1,5 +1,23 @@ import { describe, expect, it, vi } from "vitest"; -import { CoolifyClient } from "../src/coolify.js"; +import { CoolifyClient, parseBackupSchedules } from "../src/coolify.js"; + +// A row as Coolify actually serializes one: a raw ScheduledDatabaseBackup +// Eloquent model (DatabasesController@database_backup_details_uuid, v4.1.2), +// so every column is present and `executions` is eager-loaded alongside. +const row = (over: Record = {}) => ({ + id: 3, + uuid: "sched-1", + team_id: 1, + enabled: true, + save_s3: true, + frequency: "0 3 * * *", + database_backup_retention_amount_locally: 7, + database_id: 9, + database_type: "App\\Models\\StandalonePostgresql", + s3_storage_id: 2, + executions: [], + ...over, +}); function mockFetch(routes: Record) { return vi.fn(async (url: string | URL, init?: RequestInit) => { @@ -60,4 +78,93 @@ describe("CoolifyClient", () => { const c = new CoolifyClient("https://coolify.test", "tok", fetchImpl); await expect(c.version()).resolves.toBe("4.1.2"); }); + it("reads a database's backup schedules", async () => { + const c = new CoolifyClient( + "https://coolify.test", + "tok", + mockFetch({ "GET /api/v1/databases/db-1/backups": [row()] }), + ); + await expect(c.databaseBackupSchedules("db-1")).resolves.toEqual([ + { uuid: "sched-1", frequency: "0 3 * * *", retention: 7, enabled: true }, + ]); + }); + // The dangerous 404. fetchLive reads a 404 as "no live environment" three + // routes up, and that instinct is WRONG here: Coolify answers a database with + // no schedules with 200 [], never 404 — a 404 means the database wasn't found. + // Reading it as "no backups" would report an unbacked-up database as clean and + // make apply POST a duplicate schedule onto one that already had one. + it("does not turn a failed read into 'this database has no backups'", async () => { + const c = new CoolifyClient("https://coolify.test", "tok", mockFetch({})); + await expect(c.databaseBackupSchedules("db-1")).resolves.toBeUndefined(); + }); +}); + +// The shape cast cannot afford to be wrong about. Every unreadable answer must +// land on `undefined` ("cast cannot say"), and ONLY a genuinely empty list may +// land on `[]` ("there are none") — the two mean opposite things to every +// caller downstream, and to the operator staring at a cutover. +describe("parseBackupSchedules", () => { + it("reads a well-formed collection", () => { + expect(parseBackupSchedules([row()])).toEqual([ + { uuid: "sched-1", frequency: "0 3 * * *", retention: 7, enabled: true }, + ]); + }); + it("reads an empty list as a trustworthy 'no schedule', not as unknown", () => { + expect(parseBackupSchedules([])).toEqual([]); + }); + it("reads the spec's own placeholder body as unknown, not as 'no schedule'", () => { + // What the vendored OpenAPI literally documents for this route. If Coolify + // ever really answered this, cast must not read it as "no backups". + expect( + parseBackupSchedules( + "Content is very complex. Will be implemented later.", + ), + ).toBeUndefined(); + }); + it.each([ + ["a non-array object", { data: [] }], + ["null", null], + ["a row that is not an object", ["nope"]], + ["a row with no frequency", [row({ frequency: undefined })]], + ["a row with a non-string frequency", [row({ frequency: 3 })]], + [ + "a row with a null retention", + [row({ database_backup_retention_amount_locally: null })], + ], + [ + "a row with a non-numeric retention", + [row({ database_backup_retention_amount_locally: "many" })], + ], + ])("reads %s as unknown", (_label, body) => { + expect(parseBackupSchedules(body)).toBeUndefined(); + }); + it("collapses the WHOLE read when any one row is unreadable", () => { + // A partial list is indistinguishable from a complete one downstream, and + // the caller worth protecting is the one asking "is this backed up?". + expect( + parseBackupSchedules([row(), row({ uuid: 7, frequency: null })]), + ).toBeUndefined(); + }); + it("reads a disabled schedule as disabled, however Coolify spells it", () => { + // `enabled` has no cast on the model (v4.1.2 casts() covers only the two + // float storage fields), so a tinyint column can serialize as 1/0. + expect(parseBackupSchedules([row({ enabled: 0 })])?.[0].enabled).toBe( + false, + ); + expect(parseBackupSchedules([row({ enabled: false })])?.[0].enabled).toBe( + false, + ); + expect(parseBackupSchedules([row({ enabled: 1 })])?.[0].enabled).toBe(true); + // Absent reads as enabled: Coolify's create path defaults it to true. + expect( + parseBackupSchedules([row({ enabled: undefined })])?.[0].enabled, + ).toBe(true); + }); + it("accepts an integer retention however it is serialized", () => { + expect( + parseBackupSchedules([ + row({ database_backup_retention_amount_locally: "7" }), + ])?.[0].retention, + ).toBe(7); + }); }); diff --git a/test/diff.test.ts b/test/diff.test.ts index 5179cd9..87b66a9 100644 --- a/test/diff.test.ts +++ b/test/diff.test.ts @@ -413,3 +413,136 @@ describe("renderDiff", () => { expect(out).toMatch(/env vars not compared \(structural mode/); }); }); + +// A database's backup schedule is a field like any other — the whole point of +// #51. These pin the four answers a live read can give, and above all pin the +// two that must never be confused: "there is no schedule" (drift, fixable) and +// "cast could not read the schedule" (not drift, not clean, said out loud). +describe("backup schedules", () => { + const wantBackup = { + kind: "database" as const, + name: "postgres", + fields: { + type: "postgresql", + backup: { frequency: "0 3 * * *", retention: 7 }, + }, + }; + const liveDb = (fields: Record, extra = {}) => ({ + kind: "database" as const, + name: "postgres", + uuid: "db-1", + fields: { type: "postgresql", ...fields }, + ...extra, + }); + + it("is clean when the live schedule matches", () => { + const r = computeDiff( + [wantBackup], + [liveDb({ backup: { frequency: "0 3 * * *", retention: 7 } })], + "full", + ); + expect(r.clean).toBe(true); + expect(r.backupsNotCompared).toEqual([]); + }); + + it("reports drift when the schedule differs", () => { + const r = computeDiff( + [wantBackup], + [liveDb({ backup: { frequency: "0 5 * * *", retention: 3 } })], + "full", + ); + expect(r.changes[0].fieldDiffs).toEqual([ + { + field: "backup", + desired: { frequency: "0 3 * * *", retention: 7 }, + live: { frequency: "0 5 * * *", retention: 3 }, + updatable: true, + }, + ]); + expect(r.clean).toBe(false); + }); + + // The defect in one test: a live database with NO schedule, declared in the + // manifest, used to be invisible. It is drift, and apply can fix it. + it("reports drift when the database has no schedule at all", () => { + const r = computeDiff([wantBackup], [liveDb({})], "full"); + expect(r.clean).toBe(false); + expect(r.changes[0].fieldDiffs[0]).toMatchObject({ + field: "backup", + live: undefined, + updatable: true, + }); + }); + + // A schedule row that exists but is switched off backs nothing up. It must + // not read as clean, and it must not read as absent (apply PATCHes it rather + // than POSTing a second one). + it("reports drift when the schedule exists but is disabled", () => { + const r = computeDiff( + [wantBackup], + [ + liveDb({ + backup: { frequency: "0 3 * * *", retention: 7, enabled: false }, + }), + ], + "full", + ); + expect(r.clean).toBe(false); + expect(r.changes[0].fieldDiffs[0].field).toBe("backup"); + }); + + // The shape-mismatch path — the one that must not lie in EITHER direction. + it("invents no drift when the live schedule could not be read", () => { + const r = computeDiff( + [wantBackup], + [liveDb({}, { backupNotCompared: "unrecognized shape" })], + "full", + ); + // Not drift: cast read nothing, so it may claim nothing. In particular it + // must NOT diff the declared block against `undefined` and report a + // confident change on a database that may be perfectly backed up. + expect(r.changes).toEqual([]); + // And not silence either. + expect(r.backupsNotCompared).toEqual([ + { name: "postgres", reason: "unrecognized shape" }, + ]); + }); + + it("says so on screen, on a run that is otherwise clean", () => { + const out = renderDiff( + computeDiff( + [wantBackup], + [liveDb({}, { backupNotCompared: "unrecognized shape" })], + "full", + ), + ); + expect(out).toContain( + "backup schedule for database postgres declared, NOT compared — verify in the Coolify UI", + ); + expect(out).toContain("(unrecognized shape)"); + // Reported, but not counted as drift — an absence of evidence is not + // evidence of drift, and a run that fails on it is a run operators learn + // to force past. + expect(out).toMatch(/^clean$/m); + }); + + it("says nothing about backups when none is declared", () => { + // An undeclared schedule is uncompared, not deleted: a live schedule on a + // database whose manifest is silent is left alone, and unremarked. + const out = renderDiff( + computeDiff( + [ + { + kind: "database" as const, + name: "postgres", + fields: { type: "postgresql" }, + }, + ], + [liveDb({}, { backupNotCompared: "unrecognized shape" })], + "full", + ), + ); + expect(out).not.toContain("backup"); + expect(out).toMatch(/^clean$/m); + }); +}); diff --git a/test/live-lookup.test.ts b/test/live-lookup.test.ts index a7808f2..2bbb931 100644 --- a/test/live-lookup.test.ts +++ b/test/live-lookup.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from "vitest"; -import { fetchLive, renderAbsentTarget } from "../src/cli.js"; +import { attachBackup, fetchLive, renderAbsentTarget } from "../src/cli.js"; import { CoolifyClient } from "../src/coolify.js"; // A Coolify that answers GET /projects with `projects`, and @@ -134,3 +134,110 @@ describe("renderAbsentTarget", () => { expect(msg).toMatch(/--env/); }); }); + +// The read half of #51: a database's backup schedule is on its own route, so +// the live side has to go and get it. These pin the mapping from what that +// route answers onto what the diff is allowed to conclude. +describe("attachBackup", () => { + const db = () => ({ + kind: "database" as const, + name: "postgres", + uuid: "db-1", + fields: { type: "postgresql" }, + }); + // A Coolify whose GET /databases/db-1/backups answers with `body` (or a + // status, to exercise the unreachable path). + const client = (body: unknown, status = 200) => + new CoolifyClient( + "https://coolify.test", + "tok", + vi.fn( + async () => + new Response(status === 200 ? JSON.stringify(body) : "boom", { + status, + }), + ) as unknown as typeof fetch, + ); + + it("reads a schedule onto the live fields, in the desired key order", async () => { + const l = db(); + await attachBackup( + client([ + { + uuid: "s1", + frequency: "0 3 * * *", + database_backup_retention_amount_locally: 7, + enabled: true, + }, + ]), + l, + ); + // Key order matters: computeDiff compares by JSON.stringify, against the + // object resolve.ts builds. Same keys, same order, or every run drifts. + expect(JSON.stringify(l.fields.backup)).toBe( + JSON.stringify({ frequency: "0 3 * * *", retention: 7 }), + ); + expect(l.backupNotCompared).toBeUndefined(); + }); + + it("leaves fields.backup absent when the database genuinely has none", async () => { + const l = db(); + await attachBackup(client([]), l); + // Absence IS the answer here, and a trustworthy one: a declared backup is + // then real drift, and apply creates the schedule. + expect("backup" in l.fields).toBe(false); + expect(l.backupNotCompared).toBeUndefined(); + }); + + it("carries a disabled schedule through as disabled", async () => { + const l = db(); + await attachBackup( + client([ + { + uuid: "s1", + frequency: "0 3 * * *", + database_backup_retention_amount_locally: 7, + enabled: false, + }, + ]), + l, + ); + // The row exists (so apply PATCHes rather than POSTing a second one) but it + // backs nothing up (so it must not compare equal to a declared block). + expect(l.fields.backup).toEqual({ + frequency: "0 3 * * *", + retention: 7, + enabled: false, + }); + }); + + it("marks the read not-compared when Coolify cannot be read", async () => { + const l = db(); + await attachBackup(client(null, 500), l); + expect("backup" in l.fields).toBe(false); + expect(l.backupNotCompared).toMatch(/unreachable|does not recognize/); + }); + + it("marks the read not-compared when a database carries several schedules", async () => { + const l = db(); + await attachBackup( + client([ + { + uuid: "s1", + frequency: "0 3 * * *", + database_backup_retention_amount_locally: 7, + }, + { + uuid: "s2", + frequency: "0 9 * * *", + database_backup_retention_amount_locally: 2, + }, + ]), + l, + ); + // A manifest declares one schedule. Picking one of two to compare against + // would be a coin toss reported as a fact. + expect(l.backupNotCompared).toMatch(/2 schedules/); + expect("backup" in l.fields).toBe(false); + }); +}); diff --git a/test/resolve.test.ts b/test/resolve.test.ts index 5830efd..98f4361 100644 --- a/test/resolve.test.ts +++ b/test/resolve.test.ts @@ -137,13 +137,9 @@ environments: join(dir, ".infra", "env", "core-api.staging.env.template"), "PORT=3000\nMG=${MG}\n", ); - const { desired, resolvedEnvs, backupSchedules } = desiredFromManifest( - dir, - "staging", - { - MG: "secret-v", - }, - ); + const { desired, resolvedEnvs } = desiredFromManifest(dir, "staging", { + MG: "secret-v", + }); expect(desired).toHaveLength(1); expect(desired[0]).toMatchObject({ kind: "application", @@ -162,9 +158,13 @@ environments: value: "secret-v", secret: true, }); - expect(backupSchedules).toEqual({}); }); - it("routes a database backup block into backupSchedules, not fields", () => { + // The reverse of what this file used to assert. `backup` was deliberately + // routed AROUND `fields` into a side channel, because live Coolify was + // believed not to expose a schedule back; it does (GET + // /databases/{uuid}/backups), and the side channel is what made a `backup:` + // block added to an existing database silently do nothing (#51). + it("puts a database backup block in fields, so it is diffed like any other", () => { const dir = mkdtempSync(join(tmpdir(), "infra-co-")); mkdirSync(join(dir, ".infra"), { recursive: true }); writeFileSync( @@ -180,16 +180,35 @@ environments: backup: { frequency: "0 3 * * *", retention: 7 } `, ); - const { desired, backupSchedules } = desiredFromManifest( - dir, - "staging", - {}, - ); - expect(desired[0].fields).toEqual({ type: "postgresql", version: "17" }); - expect(backupSchedules).toEqual({ - postgres: { frequency: "0 3 * * *", retention: 7 }, + const { desired } = desiredFromManifest(dir, "staging", {}); + expect(desired[0].fields).toEqual({ + type: "postgresql", + version: "17", + backup: { frequency: "0 3 * * *", retention: 7 }, }); }); + it("leaves `backup` out of fields entirely when none is declared", () => { + const dir = mkdtempSync(join(tmpdir(), "infra-co-")); + mkdirSync(join(dir, ".infra"), { recursive: true }); + writeFileSync( + join(dir, ".infra", "manifest.yaml"), + `project: widget +environments: + staging: + applications: {} + databases: + postgres: + type: postgresql + version: "17" +`, + ); + const { desired } = desiredFromManifest(dir, "staging", {}); + expect(desired[0].fields).toEqual({ type: "postgresql", version: "17" }); + // Undeclared means uncompared, NOT "delete whatever is there": a live + // schedule on a database whose manifest says nothing about backups is left + // alone, like every other thing apply never removes. + expect("backup" in desired[0].fields).toBe(false); + }); it("warns when a service declares domains (unhonorable by apply on Coolify 4.1.2)", () => { const dir = mkdtempSync(join(tmpdir(), "infra-co-")); mkdirSync(join(dir, ".infra"), { recursive: true }); diff --git a/test/wire.test.ts b/test/wire.test.ts index 92ad045..a78292c 100644 --- a/test/wire.test.ts +++ b/test/wire.test.ts @@ -198,7 +198,6 @@ describe("buildExecutor createResource (application, dockercompose)", () => { serverName: "prod-box", orgRepo: "acme/widget", bindingEnv: "prod", - backupSchedules: {}, }); const uuid = await exec.createResource({ kind: "application", @@ -255,7 +254,6 @@ describe("buildExecutor createResource (application, dockercompose)", () => { serverName: "prod-box", orgRepo: "acme/widget", bindingEnv: "prod", - backupSchedules: {}, }); await exec.createResource({ kind: "application", @@ -361,7 +359,6 @@ describe("buildExecutor createResource (destination placement)", () => { orgRepo: "acme/widget", bindingEnv: "prod", destinationUuid: "dest-abc", - backupSchedules: {}, }); await exec.createResource(change); expect(bodies[path]?.destination_uuid).toBe("dest-abc"); @@ -390,7 +387,6 @@ describe("buildExecutor createResource (destination placement)", () => { serverName: "prod-box", orgRepo: "acme/widget", bindingEnv: "prod", - backupSchedules: {}, }); await exec.createResource(change); expect(bodies[path]).not.toHaveProperty("destination_uuid"); @@ -562,7 +558,6 @@ describe("buildExecutor createResource (environment reconcile)", () => { serverName: "prod-box", orgRepo: "acme/widget", bindingEnv: "prod", - backupSchedules: {}, }, ); } @@ -796,7 +791,6 @@ describe("buildExecutor createResource (multi-destination 400, #41)", () => { serverName: "prod-box", orgRepo: "heavy-duty/incubator", bindingEnv: "prod", - backupSchedules: {}, }); const kinds = [ @@ -893,3 +887,219 @@ describe("databaseVersionFromImage / defaultDatabaseImage", () => { expect(databaseVersionFromImage(image)).toBe("17"); }); }); + +// The write half of #51. Backup schedules live on their own route +// (/databases/{uuid}/backups), so `apply` has to read that route to know +// whether to POST or PATCH — and used to do neither outside the create branch. +describe("buildExecutor backup schedules", () => { + type Call = { method: string; path: string; body?: unknown }; + + // Records every request so a test can assert not just what was written, but + // what was NOT — a schedule silently skipped is the whole defect. + function recorder(handler: (path: string, init?: RequestInit) => Response): { + calls: Call[]; + fetchImpl: typeof fetch; + } { + const calls: Call[] = []; + const fetchImpl = vi.fn(async (url: string | URL, init?: RequestInit) => { + const path = new URL(String(url)).pathname; + calls.push({ + method: init?.method ?? "GET", + path, + body: init?.body ? JSON.parse(String(init.body)) : undefined, + }); + return handler(path, init); + }) as unknown as typeof fetch; + return { calls, fetchImpl }; + } + + const ctx = { + projectName: "widget", + envName: "prod", + serverUuid: "srv-1", + githubAppUuid: "gh-1", + serverName: "prod-box", + orgRepo: "acme/widget", + bindingEnv: "prod", + s3DestinationUuid: "s3-1", + }; + + const schedule = { frequency: "0 3 * * *", retention: 7 }; + const liveRow = { + uuid: "sched-1", + frequency: "0 5 * * *", + database_backup_retention_amount_locally: 3, + enabled: true, + }; + + // THE FIX. A database that exists and has no schedule gets one on update — + // before this, `apply` wrote a schedule only inside the create branch, so + // adding `backup:` to a live database was a clean run and zero backups. + it("CREATES the schedule on update when the database has none", async () => { + const { calls, fetchImpl } = recorder((path, init) => { + if (path === "/api/v1/databases/db-1/backups" && init?.method === "POST") + return new Response(JSON.stringify({ uuid: "sched-new" }), { + status: 201, + }); + if (path === "/api/v1/databases/db-1/backups") + return new Response(JSON.stringify([]), { status: 200 }); // read: none + return new Response("{}", { status: 200 }); + }); + const exec = buildExecutor( + new CoolifyClient("https://coolify.test", "tok", fetchImpl), + ctx, + ); + await exec.updateFields("db-1", "database", { backup: schedule }); + const post = calls.find((c) => c.method === "POST"); + expect(post?.path).toBe("/api/v1/databases/db-1/backups"); + expect(post?.body).toEqual({ + frequency: "0 3 * * *", + database_backup_retention_amount_locally: 7, + save_s3: true, + s3_storage_uuid: "s3-1", + enabled: true, + }); + }); + + it("PATCHES the existing schedule rather than adding a second one", async () => { + const { calls, fetchImpl } = recorder((path, init) => { + if (path === "/api/v1/databases/db-1/backups" && init?.method === "GET") + return new Response(JSON.stringify([liveRow]), { status: 200 }); + return new Response("{}", { status: 200 }); + }); + const exec = buildExecutor( + new CoolifyClient("https://coolify.test", "tok", fetchImpl), + ctx, + ); + await exec.updateFields("db-1", "database", { backup: schedule }); + expect(calls.filter((c) => c.method === "POST")).toEqual([]); + const patch = calls.find((c) => c.method === "PATCH"); + expect(patch?.path).toBe("/api/v1/databases/db-1/backups/sched-1"); + expect(patch?.body).toMatchObject({ + frequency: "0 3 * * *", + database_backup_retention_amount_locally: 7, + // Re-enabled: declaring `backup:` asks for backups, not for a disabled + // row that looks like backups. + enabled: true, + }); + }); + + it("does not PATCH the database itself for a backup-only change", async () => { + const { calls, fetchImpl } = recorder((path, init) => { + if (path === "/api/v1/databases/db-1/backups" && init?.method === "GET") + return new Response(JSON.stringify([liveRow]), { status: 200 }); + return new Response("{}", { status: 200 }); + }); + const exec = buildExecutor( + new CoolifyClient("https://coolify.test", "tok", fetchImpl), + ctx, + ); + await exec.updateFields("db-1", "database", { backup: schedule }); + // `backup` is not a column on the database — it must never reach the + // database's own update body, and an empty body is not worth a write. + expect(calls.some((c) => c.path === "/api/v1/databases/db-1")).toBe(false); + }); + + // Degrade honestly: an apply that promised a backup schedule and cannot tell + // whether one already exists must STOP, not guess. POSTing blind would + // duplicate an existing schedule; skipping is the silent no-op being fixed. + it("refuses to guess when the schedule read fails", async () => { + const { calls, fetchImpl } = recorder((path, init) => { + if (path === "/api/v1/databases/db-1/backups" && init?.method === "GET") + return new Response("gateway timeout", { status: 504 }); + return new Response("{}", { status: 200 }); + }); + const exec = buildExecutor( + new CoolifyClient("https://coolify.test", "tok", fetchImpl), + ctx, + ); + await expect( + exec.updateFields("db-1", "database", { backup: schedule }), + ).rejects.toThrow(/cannot set the declared backup schedule/); + expect(calls.filter((c) => c.method === "POST")).toEqual([]); + }); + + it("refuses to guess which of several schedules the manifest meant", async () => { + const { fetchImpl } = recorder((path, init) => { + if (path === "/api/v1/databases/db-1/backups" && init?.method === "GET") + return new Response( + JSON.stringify([liveRow, { ...liveRow, uuid: "sched-2" }]), + { status: 200 }, + ); + return new Response("{}", { status: 200 }); + }); + const exec = buildExecutor( + new CoolifyClient("https://coolify.test", "tok", fetchImpl), + ctx, + ); + await expect( + exec.updateFields("db-1", "database", { backup: schedule }), + ).rejects.toThrow(/holds 2 backup schedules/); + }); + + it("leaves an undeclared schedule alone (apply never removes)", async () => { + const { calls, fetchImpl } = recorder( + () => new Response("{}", { status: 200 }), + ); + const exec = buildExecutor( + new CoolifyClient("https://coolify.test", "tok", fetchImpl), + ctx, + ); + await exec.updateFields("db-1", "database", { version: "17" }); + expect(calls.some((c) => c.path.includes("/backups"))).toBe(false); + }); + + it("still POSTs the schedule on create, without a read", async () => { + const { calls, fetchImpl } = recorder((path) => { + if (path === "/api/v1/projects") + return new Response( + JSON.stringify([{ uuid: "proj-1", name: "widget" }]), + { status: 200 }, + ); + if (path === "/api/v1/projects/proj-1/environments") + return new Response(JSON.stringify([{ name: "prod" }]), { + status: 200, + }); + if (path === "/api/v1/databases/postgresql") + return new Response(JSON.stringify({ uuid: "db-9" }), { status: 201 }); + return new Response(JSON.stringify({ uuid: "sched-9" }), { status: 201 }); + }); + const exec = buildExecutor( + new CoolifyClient("https://coolify.test", "tok", fetchImpl), + ctx, + ); + await exec.createResource({ + kind: "database", + name: "postgres", + op: "create", + fieldDiffs: [ + { field: "type", desired: "postgresql", updatable: false }, + { field: "backup", desired: schedule, updatable: true }, + ], + envDiffs: [], + }); + // A database created a moment ago provably has no schedule: POST straight + // out, with no read that could fail and abort the create. + expect( + calls.some((c) => c.method === "GET" && c.path.includes("/backups")), + ).toBe(false); + const post = calls.find((c) => c.path === "/api/v1/databases/db-9/backups"); + expect(post?.body).toMatchObject({ frequency: "0 3 * * *", save_s3: true }); + // And `backup` never reaches the database's own create body. + const create = calls.find((c) => c.path === "/api/v1/databases/postgresql"); + expect(create?.body).not.toHaveProperty("backup"); + }); + + it("refuses a declared schedule with no s3_destination configured", async () => { + const { fetchImpl } = recorder( + () => new Response(JSON.stringify([]), { status: 200 }), + ); + const exec = buildExecutor( + new CoolifyClient("https://coolify.test", "tok", fetchImpl), + { ...ctx, s3DestinationUuid: undefined }, + ); + await expect( + exec.updateFields("db-1", "database", { backup: schedule }), + ).rejects.toThrow(/no s3_destination UUID/); + }); +}); -- 2.45.2