From 1210ae4445c3419a9e28b5caab3dbb3189ac81d4 Mon Sep 17 00:00:00 2001 From: claude-hdb Date: Thu, 16 Jul 2026 18:25:34 +0000 Subject: [PATCH] fix(draft): read backup schedules and emit backup blocks (#75) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --emit-draft still told every reader that backup schedules "are not exposed by Coolify's API" — the exact pre-#51 claim that issue disproved: GET /databases/{uuid}/backups is a route, and diff/apply have read it on every run since. The draft path was never brought along, so it warned instead of reading, and a rebuild from a draft came up with no backups. Now the draft loop makes the same supplementary per-database GET (databaseBackupSchedules) for every DRAFTED database and databaseSpec emits a real backup: { frequency, retention } block for the one shape the manifest can express — a single, enabled schedule. Ungated on purpose: fetchLive's opts.backups gate exists because the read-side sweeps never look at the answer, and the draft is the sweep that does. The read stays sequential (like the existing per-resource env GETs) and a failed read degrades to an UNCAPTURED entry per resource rather than aborting the whole-instance sweep — a draft's reader is a human, not an apply about to write. UNCAPTURED keeps only what the route genuinely cannot answer: - the S3 target: save_s3 now rides on LiveBackup, and a schedule that saves to S3 gets a per-database entry saying the target reads back only as s3_storage_id, an int nothing maps to a storage UUID - a DISABLED schedule (declaring the block would make apply re-enable it) - several schedules where a manifest declares one - an unreadable route (reported, never read as "no backups") The stale NO_API_COVERAGE "backup schedules" row becomes "a backup schedule's S3 target", and semantics.md's draft section now tells the truth about what is captured. Closes #75 Co-Authored-By: Claude Fable 5 --- docs/semantics.md | 17 ++++--- src/cli.ts | 14 +++++- src/coolify.ts | 17 +++++++ src/draft.ts | 98 +++++++++++++++++++++++++++++++++------ test/coolify.test.ts | 28 ++++++++++- test/draft-cli.test.ts | 45 +++++++++++++++++- test/draft.test.ts | 102 +++++++++++++++++++++++++++++++++++++++++ 7 files changed, 297 insertions(+), 24 deletions(-) diff --git a/docs/semantics.md b/docs/semantics.md index e5b0c87..300006a 100644 --- a/docs/semantics.md +++ b/docs/semantics.md @@ -1058,15 +1058,18 @@ 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 (settable/readable via the API and carried by `diff`/`apply` as `service_domains` -since #72, but the same as backups below — `inventory --emit-draft` does not yet +since #72, but `inventory --emit-draft` does not yet make the per-service `GET /services/{uuid}`, so a drafted service has none until you declare them), Basic Auth / custom Traefik labels, -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 +build and deploy command overrides, what a backup schedule cannot fully say +(the schedule itself **is captured** since #75 — the draft reads +`GET /databases/{uuid}/backups`, the route `diff`/`apply` have used since #51, +and a single enabled schedule becomes a real `backup: { frequency, retention }` +block; what stays uncaptured is reported per database instead of guessed: the S3 +*target*, which reads back only as an unmappable `s3_storage_id` int, a DISABLED +schedule — the manifest cannot say "disabled", and declaring the block would make +the first `apply` re-enable it — a database carrying *several* schedules where a +manifest declares one, and a read that failed outright), 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 diff --git a/src/cli.ts b/src/cli.ts index a929cfc..93dd05b 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1952,7 +1952,19 @@ async function main(): Promise { for (const r of resources) { // Databases hold no manifest-templated env of their own — their URL is // what the APPS reference, and that name is generated, not captured. - if (r.kind === "database") continue; + // What a database DOES hold is a backup schedule, on its own route + // (#51) — and a blueprint that omits backups is the quietest possible + // loss, so the draft pays the one supplementary GET per DRAFTED + // database that diff/apply pay per compared one. Ungated on purpose: + // fetchLive's `opts.backups` gate exists because the read-side sweeps + // never look at the answer, and the draft is the sweep that does. A + // failed read lands on `undefined` and becomes an UNCAPTURED entry + // (see draftBackup) — reported, never aborting the whole-instance + // sweep the way diff/apply refuse a single-project plan. + if (r.kind === "database") { + r.backups = await client.databaseBackupSchedules(r.uuid); + continue; + } r.env = flattenEnv( await fetchEnv(client, { kind: r.kind, uuid: r.uuid }), ); diff --git a/src/coolify.ts b/src/coolify.ts index 7375ee0..182f66e 100644 --- a/src/coolify.ts +++ b/src/coolify.ts @@ -31,6 +31,13 @@ export type LiveBackup = { frequency: string; retention: number; enabled: boolean; + // Whether the schedule saves to S3 (`save_s3`). Carried for the DRAFT's + // UNCAPTURED report (#75): the schedule itself is now capturable, but its + // TARGET reads back only as `s3_storage_id` — an int no endpoint maps to a + // storage UUID — so "saves to S3" is readable and "saves to WHICH S3" is not, + // and a draft has to say so. diff/apply ignore it (apply asserts save_s3 + + // the environment's s3_destination on every write). + saveS3: boolean; }; // The result of trying to read a database's schedules. The two absences are @@ -56,6 +63,15 @@ function readEnabled(raw: Record): boolean { return !(v === false || v === 0 || v === "0"); } +// `save_s3` is a tinyint with no cast, exactly like `enabled`, so accept 1/0 +// too. Unlike `enabled`, an ABSENT value reads as FALSE: the only consumer is +// the draft's "this backup lands in S3" report, and that claim must never be +// made off a field the row did not carry. +function readSaveS3(raw: Record): boolean { + const v = raw.save_s3; + return v === true || v === 1 || v === "1"; +} + // 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 @@ -114,6 +130,7 @@ export function parseBackupSchedules(raw: unknown): BackupRead { frequency, retention, enabled: readEnabled(row), + saveS3: readSaveS3(row), }); } return schedules; diff --git a/src/draft.ts b/src/draft.ts index d548974..93bd28b 100644 --- a/src/draft.ts +++ b/src/draft.ts @@ -2,6 +2,7 @@ import { existsSync, mkdirSync, readdirSync, writeFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { stringify } from "yaml"; import { GENERATED_PLACEHOLDER } from "./capture.js"; +import type { BackupRead } from "./coolify.js"; import { isProviderGeneratedEnvName, isReservedEnvName, @@ -149,6 +150,14 @@ export type DraftResource = { // Live env vars. Empty for databases (their URL is what the apps reference, // and that name is generated, not captured). env: Record; + // Databases only: the backup schedules read off GET /databases/{uuid}/backups + // — the same supplementary per-database GET diff/apply have made on every run + // since #51, made here by the CLI's draft loop for every DRAFTED database. + // BackupRead's two absences stay distinct (see coolify.ts): `[]` is a clean + // "no schedule" (nothing to draft, nothing to report), `undefined` is "could + // not read" — which the draft REPORTS in UNCAPTURED.md rather than aborting a + // whole-instance sweep the way diff/apply refuse a single-project plan. + backups?: BackupRead; }; export type DraftProject = { @@ -557,17 +566,80 @@ function databaseSpec( `image "${image}" — no version could be read from its tag, so none was written and \`apply\` would create this database on Coolify's default image.`, ); } - // Coolify DOES expose a database's backup schedule — GET /databases/{uuid}/backups - // answers, and `diff`/`apply` read and write it (#51). What the DRAFT path cannot - // yet do is CAPTURE it: `inventory --emit-draft` does not read that route, so a - // `backup:` block is not recovered here. Until it is taught to, a rebuild from - // this draft still comes up with NO BACKUPS — the quietest possible loss, and the - // one you discover at the worst moment — unless the block is declared by hand. - flag( - "backup", - "backup schedules are NOT in this draft — `inventory --emit-draft` does not yet read `GET /databases/{uuid}/backups` (which `diff` and `apply` do, #51). If this database is backed up, a rebuild from here would not be until you declare it. Read the schedule from a `cast diff` or the Coolify UI (Backups tab) and set `backup: { frequency, retention }` yourself.", - ); - return { type, ...(version ? { version } : {}) }; + // The backup schedule IS captured (#75): the CLI's draft loop reads + // GET /databases/{uuid}/backups — the route diff/apply have read on every run + // since #51 — and the one shape the manifest can express (a single, enabled + // schedule) becomes a real `backup:` block. Everything the route genuinely + // cannot answer stays a per-resource UNCAPTURED entry: reported, never + // guessed, and never silently dropped. + const backup = draftBackup(r, flag); + return { + type, + ...(version ? { version } : {}), + ...(backup ? { backup } : {}), + }; +} + +// The drafted `backup:` block, or the reason there isn't one. The same four +// answers attachBackup (cli.ts) reads for a diff — but where diff refuses or +// skips a comparison, a draft REPORTS: its reader is a human adopting a box, +// not an `apply` about to write one, and a sweep that aborted on one database +// would trade a whole blueprint for one row. +// +// unreadable -> UNCAPTURED, no block. If this database is backed up, the +// draft cannot say so — and a rebuild from it would not be. +// no schedule -> nothing at all. Read cleanly, absence IS the answer, and a +// manifest with no `backup:` block expresses it exactly. +// one enabled -> a real block, `{ frequency, retention }` — the same +// projection the desired side builds (resolve.ts), so the +// drafted manifest diffs clean the moment it is applied. Plus +// an UNCAPTURED entry for the S3 TARGET when the schedule +// saves to S3: the route returns it only as `s3_storage_id`, +// an int no endpoint maps to a storage UUID (#72), so the +// block cannot carry WHICH bucket. +// one disabled -> UNCAPTURED, no block. The manifest cannot express a +// disabled schedule (`backup:` asks for backups, and every +// cast write asserts enabled: true), so emitting the block +// would make the first `apply` re-enable a schedule someone +// turned off on purpose. +// several -> UNCAPTURED, no block. A manifest declares ONE schedule; +// picking one to write down would be a coin toss dressed up +// as a blueprint. +function draftBackup( + r: DraftResource, + flag: (setting: string, detail: string) => void, +): { frequency: string; retention: number } | undefined { + const read = r.backups; + if (read === undefined) { + flag( + "backup", + "`GET /databases/{uuid}/backups` was unreachable or returned a shape cast does not recognize, so the schedule is NOT in this draft. If this database is backed up, a rebuild from here would not be until you declare it — read the schedule off a `cast diff` or the Coolify UI (Backups tab) and set `backup: { frequency, retention }` yourself.", + ); + return undefined; + } + if (read.length === 0) return undefined; + if (read.length > 1) { + flag( + "backup", + `Coolify holds ${read.length} backup schedules for this database and a manifest declares ONE, so none was drafted. Decide which schedule the manifest should carry and declare its \`backup: { frequency, retention }\` yourself.`, + ); + return undefined; + } + const schedule = read[0]; + if (!schedule.enabled) { + flag( + "backup", + `a backup schedule exists (frequency "${schedule.frequency}", retention ${schedule.retention}) but it is DISABLED — it backs nothing up, and the manifest cannot say "disabled": declaring \`backup:\` asks for backups, and the first \`apply\` would re-enable it. It is NOT in this draft; decide whether it was turned off on purpose before you declare it.`, + ); + return undefined; + } + if (schedule.saveS3) { + flag( + "backup S3 target", + "this schedule saves to S3, and the draft cannot say WHERE: Coolify returns the target only as `s3_storage_id`, an int no endpoint maps to a storage UUID (the destination_id problem again, #21/#72). The drafted `backup:` block carries frequency and retention; `apply` points the schedule at the environment's own `s3_destination` — verify that is the bucket you meant.", + ); + } + return { frequency: schedule.frequency, retention: schedule.retention }; } function serviceSpec( @@ -775,8 +847,8 @@ const NO_API_COVERAGE: Array<[string, string]> = [ "and its neighbours on a resource's Settings tab: no API coverage in 4.1.2, and not returned by the endpoints cast reads.", ], [ - "backup schedules", - "the API exposes them (`GET /databases/{uuid}/backups`, which `diff`/`apply` use — #51), but `inventory --emit-draft` does not yet read that route, so no `backup:` block is captured. A rebuild from this draft has NO backups until you declare them.", + "a backup schedule's S3 target", + "the schedule itself IS captured (`GET /databases/{uuid}/backups`, the route `diff`/`apply` have used since #51 — a single enabled schedule becomes a real `backup:` block above), but its target reads back only as `s3_storage_id`, an int no endpoint maps to a storage UUID. `apply` points every schedule it writes at the environment's own `s3_destination`; whether that is the bucket the source box used must be verified by hand.", ], [ "database kinds cast does not model", diff --git a/test/coolify.test.ts b/test/coolify.test.ts index 552459a..a6734ce 100644 --- a/test/coolify.test.ts +++ b/test/coolify.test.ts @@ -85,7 +85,13 @@ describe("CoolifyClient", () => { 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 }, + { + uuid: "sched-1", + frequency: "0 3 * * *", + retention: 7, + enabled: true, + saveS3: true, + }, ]); }); // The dangerous 404. fetchLive reads a 404 as "no live environment" three @@ -106,7 +112,13 @@ describe("CoolifyClient", () => { describe("parseBackupSchedules", () => { it("reads a well-formed collection", () => { expect(parseBackupSchedules([row()])).toEqual([ - { uuid: "sched-1", frequency: "0 3 * * *", retention: 7, enabled: true }, + { + uuid: "sched-1", + frequency: "0 3 * * *", + retention: 7, + enabled: true, + saveS3: true, + }, ]); }); it("reads an empty list as a trustworthy 'no schedule', not as unknown", () => { @@ -160,6 +172,18 @@ describe("parseBackupSchedules", () => { parseBackupSchedules([row({ enabled: undefined })])?.[0].enabled, ).toBe(true); }); + it("reads save_s3 however Coolify spells it — and absent as false", () => { + // Same tinyint-with-no-cast story as `enabled`, opposite default: "this + // backup lands in S3" must never be claimed off a field the row lacks. + expect(parseBackupSchedules([row({ save_s3: 1 })])?.[0].saveS3).toBe(true); + expect(parseBackupSchedules([row({ save_s3: 0 })])?.[0].saveS3).toBe(false); + expect(parseBackupSchedules([row({ save_s3: false })])?.[0].saveS3).toBe( + false, + ); + expect( + parseBackupSchedules([row({ save_s3: undefined })])?.[0].saveS3, + ).toBe(false); + }); it("accepts an integer retention however it is serialized", () => { expect( parseBackupSchedules([ diff --git a/test/draft-cli.test.ts b/test/draft-cli.test.ts index 7a762ca..b4eac66 100644 --- a/test/draft-cli.test.ts +++ b/test/draft-cli.test.ts @@ -14,6 +14,7 @@ import { join } from "node:path"; import { afterEach, beforeAll, describe, expect, it } from "vitest"; import { loadBindings } from "../src/bindings.js"; import { GENERATED_PLACEHOLDER } from "../src/capture.js"; +import { loadManifest } from "../src/manifest.js"; import { decryptSecrets } from "../src/secrets.js"; // `cast inventory --emit-draft` against a stub shaped like the box that made it @@ -157,6 +158,22 @@ async function stubCoolify(opts: { ambiguous?: boolean } = {}): Promise { ], }); + // The draft's supplementary per-database GET (#75) — the same route + // diff/apply read (#51). One enabled schedule, saving to S3: the block is + // draftable, the S3 TARGET (s3_storage_id, an unmappable int) is not. + if (path === "/databases/d1/backups") + return json([ + { + uuid: "sched-1", + enabled: true, + save_s3: true, + frequency: "0 3 * * *", + database_backup_retention_amount_locally: 7, + s3_storage_id: 2, + executions: [], + }, + ]); + const envs = path.match(/^\/[a-z]+\/([a-z0-9]+)\/envs$/); if (envs) return json(ENVS[envs[1]] ?? []); res.writeHead(404); @@ -358,7 +375,10 @@ describe("cast inventory --emit-draft (#27)", () => { expect(md).toContain("destination_id 3"); expect(md).toContain("legacy-analytics"); // a MySQL cast cannot model expect(md).toContain("custom_labels"); // Basic Auth / Traefik labels - expect(md).toContain("backup"); // API exposes it, draft doesn't capture it yet (#51) + // The schedule itself is CAPTURED now (#75); what stays uncaptured is the + // S3 target, which reads back only as an unmappable s3_storage_id int. + expect(md).toContain("backup S3 target"); + expect(md).not.toContain("does not yet read"); // the stale pre-#51 claim expect(md).toContain("legacy.flag"); // not a name a template can hold // And the standing sections, emitted on every run whatever was found: @@ -372,6 +392,29 @@ describe("cast inventory --emit-draft (#27)", () => { expect(r.output).toContain("UNCAPTURED.md"); }); + it("reads the backup schedule and emits a real backup block (#75)", async () => { + const f = fixture((await stubCoolify()).url); + const r = await run([ + "--env", + "prod", + "--state", + f.state, + "--emit-draft", + f.out, + "--recipient", + RECIPIENT, + ]); + expect(r.code).toBe(0); + // The drafted manifest carries the live schedule, in the exact shape the + // desired side declares — so it diffs clean the moment it is applied. + const manifest = loadManifest( + join(f.out, "incubator", ".infra", "manifest.yaml"), + ); + expect( + manifest.environments.prod.databases?.["Incubator Database v2"]?.backup, + ).toEqual({ frequency: "0 3 * * *", retention: 7 }); + }); + it("writes the projects: registry — the list of what exists", async () => { const f = fixture((await stubCoolify()).url); await run([ diff --git a/test/draft.test.ts b/test/draft.test.ts index 09452ee..c5c4b2d 100644 --- a/test/draft.test.ts +++ b/test/draft.test.ts @@ -450,6 +450,108 @@ describe("planDraft — the emitted shape", () => { }); }); +describe("backup schedules — read and drafted, not hand-waved (#75)", () => { + type Backups = DraftProject["resources"][number]["backups"]; + const withDb = (backups: Backups): DraftProject => + project({ + resources: [ + ...project().resources, + { + kind: "database", + name: "Incubator Database v2", + uuid: "d1", + raw: { + database_type: "standalone-postgresql", + image: "postgres:16-alpine", + }, + env: {}, + backups, + }, + ], + }); + const schedule = (over: Partial[number]> = {}) => ({ + uuid: "sched-1", + frequency: "0 3 * * *", + retention: 7, + enabled: true, + saveS3: false, + ...over, + }); + const loadedDb = (plan: ReturnType) => { + const manifest = plan.files.find((f) => f.path.endsWith("manifest.yaml")); + const dir = mkdtempSync(join(tmpdir(), "cast-draft-")); + const path = join(dir, "manifest.yaml"); + writeFileSync(path, manifest?.content ?? ""); + return loadManifest(path).environments.prod.databases?.[ + "Incubator Database v2" + ]; + }; + const backupItems = (plan: ReturnType) => + plan.uncaptured.filter((u) => u.setting.startsWith("backup")); + + it("emits a real backup block for a single enabled schedule — and nothing uncaptured", () => { + const plan = planDraft([withDb([schedule()])], ctx); + expect(loadedDb(plan)?.backup).toEqual({ + frequency: "0 3 * * *", + retention: 7, + }); + expect(backupItems(plan)).toEqual([]); + // The stale pre-#51 claim is gone from every artifact. + expect(JSON.stringify(plan.files)).not.toContain("does not yet read"); + }); + + it("reports the S3 target it cannot map when the schedule saves to S3", () => { + const plan = planDraft([withDb([schedule({ saveS3: true })])], ctx); + // The block is still drafted — frequency/retention ARE readable. + expect(loadedDb(plan)?.backup).toEqual({ + frequency: "0 3 * * *", + retention: 7, + }); + const items = backupItems(plan); + expect(items).toHaveLength(1); + expect(items[0].setting).toBe("backup S3 target"); + expect(items[0].detail).toContain("s3_storage_id"); + }); + + it("emits nothing for a clean 'no schedule' read — absence IS the answer", () => { + const plan = planDraft([withDb([])], ctx); + expect(loadedDb(plan)?.backup).toBeUndefined(); + expect(backupItems(plan)).toEqual([]); + }); + + it("does NOT draft a disabled schedule — apply would re-enable it", () => { + const plan = planDraft( + [withDb([schedule({ enabled: false, saveS3: true })])], + ctx, + ); + expect(loadedDb(plan)?.backup).toBeUndefined(); + const items = backupItems(plan); + expect(items).toHaveLength(1); + expect(items[0].detail).toContain("DISABLED"); + expect(items[0].detail).toContain('"0 3 * * *"'); + }); + + it("will not pick between several schedules", () => { + const plan = planDraft( + [withDb([schedule(), schedule({ uuid: "sched-2", frequency: "daily" })])], + ctx, + ); + expect(loadedDb(plan)?.backup).toBeUndefined(); + const items = backupItems(plan); + expect(items).toHaveLength(1); + expect(items[0].detail).toContain("2 backup schedules"); + }); + + it("reports an unreadable route rather than aborting or claiming 'no backups'", () => { + const plan = planDraft([withDb(undefined)], ctx); + expect(loadedDb(plan)?.backup).toBeUndefined(); + const items = backupItems(plan); + expect(items).toHaveLength(1); + expect(items[0].detail).toContain("unreachable"); + expect(items[0].resource).toBe("Incubator Database v2"); + }); +}); + describe("the emit refusals — adoption is one-way", () => { it("refuses a target directory that is not empty", () => { const dir = mkdtempSync(join(tmpdir(), "cast-draft-")); -- 2.45.2