diff --git a/docs/semantics.md b/docs/semantics.md index 58cb803..e1c1b94 100644 --- a/docs/semantics.md +++ b/docs/semantics.md @@ -174,6 +174,30 @@ softened by an implementation detail): (`instant_deploy` on create, `/deploy` or `/restart` on update) — API mutations land in Coolify's DB, not in running containers, so a create or update that skipped this would silently not take effect. +- **Apply acts in dependency order, not manifest order** — `database` → + `service` → `application` (#45), for creates *and* updates: a redeploy is a + redeploy, and an application restarted against a database whose own pending + change has not landed is the same failure one apply later. The order cannot be + computed — nothing in a manifest declares that `core` needs `postgres`, no + resource names another, so there is no graph to walk — so it is a fixed + kind-order (`KIND_ORDER` in `apply.ts`). The direction between the three kinds + is not in question, and three is few enough to legislate. `cast destroy` tears + down in its exact reverse: things come up in the order their dependencies + allow and go down in the reverse. Only the *acting* order changes; the diff + report still reads in manifest order (a resource is read where its author + wrote it), and nothing about clean/orphans/placement moves with it. + + What that buys, and what it does not: an application is no longer created and + deployed against databases that **do not exist**, which made a first apply's + deploy fail by construction — a full build, a red deployment, and an operator + told to ignore it. It is **not** a readiness barrier. + `DeployController@deploy_resource` (v4.1.2) *queues*: + `queue_application_deployment(...)` for an application and + `StartDatabase::dispatch($resource)` for a database (only a service starts + synchronously, `StartService::run`). So cast orders its **requests**, and + Coolify runs them on its own queues. An app whose first boot must find a + *listening* database still races it; apply guarantees the database exists and + was asked to start first, not that it is up. - Direction is one-way, manifest → Coolify, always. - **Environment guards:** `apply` refuses if a var matching that environment's `forbidden_var_patterns` is present in the resolved env **at all, regardless diff --git a/src/apply.ts b/src/apply.ts index 48a223c..d991381 100644 --- a/src/apply.ts +++ b/src/apply.ts @@ -12,6 +12,35 @@ export type Executor = { redeploy(uuid: string, kind: ResourceKind): Promise; }; +// The order apply acts in, by kind. +// +// It is a FIXED order and not a computed graph because there is no graph to +// compute: nothing in a manifest declares that `core` needs `postgres` — no +// resource names another, anywhere — so the dependency edges do not exist to be +// walked. What does exist is the direction between kinds, and it is not in +// question: applications talk to databases and services, never the reverse. +// Three kinds is few enough to legislate. +// +// Ranked as a Record on purpose: a fourth ResourceKind +// does not COMPILE until someone decides where it goes. A list + `indexOf` +// would rank an unranked kind -1 — i.e. ahead of databases — which is exactly +// the bug this ordering exists to fix (#45), reintroduced silently for the new +// kind. +const KIND_RANK: Record = { + database: 0, + service: 1, + application: 2, +}; + +// The forward order, spelled out: databases → services → applications. Derived +// from the ranks rather than written twice, so the two can never drift apart. +// `cast destroy` (#43) tears down in its exact reverse — things come up in the +// order their dependencies allow and go down in the reverse — and a follow-up +// unifies the two constants in one place. +export const KIND_ORDER: readonly ResourceKind[] = ( + Object.keys(KIND_RANK) as ResourceKind[] +).sort((a, b) => KIND_RANK[a] - KIND_RANK[b]); + export function applyHostnameOverlay( desired: Desired[], overlay: Record>, @@ -66,6 +95,11 @@ export async function applyPlan( "apply requires a full diff (session token with read:sensitive) — refusing on a structural report", ); } + // Every change is checked before the first one is acted on — that is the + // guarantee ("fails loudly, before any mutation"), and it is why this is a + // separate full scan and not a check folded into the ordered walk below. A + // fold would let the databases (which now sort first) be created before the + // application whose un-updatable drift refuses the run. for (const c of report.changes) { const blocked = c.fieldDiffs.filter( (f) => !f.updatable && c.op === "update", @@ -76,8 +110,31 @@ export async function applyPlan( ); } } + // Act in dependency order, not manifest order (#45). + // + // `desiredFromManifest` emits applications first and `computeDiff` preserves + // that, so a walk in report order creates the compose app — and deploys it, + // three lines down — before the Postgres and Redis it talks to exist at all. + // A guaranteed-red first deploy, every time. + // + // Creates AND updates, not just creates: a redeploy is a redeploy. An + // application restarted against a database whose own pending change has not + // been applied yet is the same failure, one apply later. + // + // A COPY, never a sort in place: `report.changes` is what `renderDiff` prints + // and what a fleet run reports on, and that reading order is the manifest's, + // deliberately — a resource is read where its author wrote it. Only the acting + // order changes here. Nothing about WHAT apply does (clean, orphans, + // placement, the refusals above) moves with it. + // + // Stable (ES2019 guarantees it), so within a kind the manifest's order + // survives. Within-kind order carries no meaning, but a run that reshuffles + // its own resources every time is noise in an operator's terminal. + const ordered = [...report.changes].sort( + (a, b) => KIND_RANK[a.kind] - KIND_RANK[b.kind], + ); const mutated: string[] = []; - for (const c of report.changes) { + for (const c of ordered) { const spec = desired.find((d) => d.kind === c.kind && d.name === c.name); let uuid: string; let didMutate = c.op === "create"; diff --git a/test/apply.test.ts b/test/apply.test.ts index 7c1c059..d4239ff 100644 --- a/test/apply.test.ts +++ b/test/apply.test.ts @@ -1,10 +1,11 @@ import { describe, expect, it } from "vitest"; import { type Executor, + KIND_ORDER, applyHostnameOverlay, applyPlan, } from "../src/apply.js"; -import { type Desired, computeDiff } from "../src/diff.js"; +import { type Desired, type Live, computeDiff } from "../src/diff.js"; const desired: Desired[] = [ { @@ -15,12 +16,12 @@ const desired: Desired[] = [ }, ]; -function recorder() { +function recorder(uuidFor: (name: string) => string = () => "new-uuid") { const calls: string[] = []; const exec: Executor = { createResource: async (c) => { calls.push(`create ${c.name}`); - return "new-uuid"; + return uuidFor(c.name); }, updateFields: async (uuid, _k, fields) => { calls.push(`update ${uuid} ${Object.keys(fields).join(",")}`); @@ -124,6 +125,138 @@ describe("applyPlan", () => { }); }); +// The order resolve.ts actually emits (`desiredFromManifest`: applications, +// then databases, then services) and `computeDiff` faithfully preserves. This +// is the input that used to build and deploy `core` against nothing. +const manifestOrder: Desired[] = [ + { + kind: "application", + name: "core", + fields: { build_pack: "dockercompose" }, + env: { vars: { DATABASE_URL: { value: "postgres://x", secret: true } } }, + }, + { kind: "database", name: "postgres", fields: { type: "postgresql" } }, + { kind: "database", name: "redis", fields: { type: "redis" } }, + { kind: "service", name: "metabase", fields: { type: "metabase" } }, +]; +const named = (name: string) => `${name}-uuid`; + +describe("applyPlan ordering (#45)", () => { + it("creates databases, then services, then applications — never the manifest's order", async () => { + const { calls, exec } = recorder(named); + const report = computeDiff(manifestOrder, [], "full"); + // The report itself reads in manifest order: application first. + expect(report.changes.map((c) => c.name)).toEqual([ + "core", + "postgres", + "redis", + "metabase", + ]); + const r = await applyPlan(report, manifestOrder, exec); + // …and apply ACTS in dependency order. `core` is created and deployed last, + // by which point both databases and the service exist. Within a kind the + // manifest's order survives (postgres before redis) — the sort is stable. + expect(calls).toEqual([ + "create postgres", + "redeploy postgres-uuid", + "create redis", + "redeploy redis-uuid", + "create metabase", + "redeploy metabase-uuid", + "create core", + "env core-uuid", + "redeploy core-uuid", + ]); + expect(r.mutated).toEqual(["postgres", "redis", "metabase", "core"]); + }); + + it("orders updates too, not only creates", async () => { + // The apply that adds a Redis and points an existing app at it: the + // database must be created and started before the app redeploys onto it. + const { calls, exec } = recorder(named); + const withRedis: Desired[] = [ + { + kind: "application", + name: "core", + fields: { build_pack: "dockercompose" }, + env: { + vars: { REDIS_URL: { value: "redis://redis:6379", secret: false } }, + }, + }, + { kind: "database", name: "redis", fields: { type: "redis" } }, + ]; + const live: Live[] = [ + { + kind: "application", + name: "core", + uuid: "u-core", + fields: { build_pack: "dockercompose" }, + env: {}, + }, + ]; + const r = await applyPlan( + computeDiff(withRedis, live, "full"), + withRedis, + exec, + ); + expect(calls).toEqual([ + "create redis", + "redeploy redis-uuid", + "env u-core", + "redeploy u-core", + ]); + expect(r.mutated).toEqual(["redis", "core"]); + }); + + it("still refuses non-updatable drift before ANY mutation, even one that now sorts first", async () => { + // The regression the reorder could have introduced: the database sorts + // ahead of the application, so a check folded into the ordered walk would + // create postgres and only then refuse. The refusal is a full scan first. + const { calls, exec } = recorder(named); + const live: Live[] = [ + { + kind: "application", + name: "core", + uuid: "u-core", + fields: { build_pack: "nixpacks" }, // NON_UPDATABLE drift + env: { DATABASE_URL: "postgres://x" }, + }, + ]; + await expect( + applyPlan(computeDiff(manifestOrder, live, "full"), manifestOrder, exec), + ).rejects.toThrow(/build_pack/); + expect(calls).toEqual([]); + }); + + it("does not reorder the report itself — the diff reads in manifest order", async () => { + // renderDiff and the fleet summary read `report.changes`; sorting it in + // place would silently reshuffle what the operator sees. + const { exec } = recorder(named); + const report = computeDiff(manifestOrder, [], "full"); + await applyPlan(report, manifestOrder, exec); + expect(report.changes.map((c) => c.name)).toEqual([ + "core", + "postgres", + "redis", + "metabase", + ]); + // and the rest of the report is untouched by ordering + expect(report.clean).toBe(false); + expect(report.orphans).toEqual([]); + }); + + it("exports the forward kind-order, whose reverse is the teardown order", () => { + expect(KIND_ORDER).toEqual(["database", "service", "application"]); + // `cast destroy` (#43) is the exact reverse — up in dependency order, down + // in reverse. It defines its own constant today; a follow-up unifies them. + expect([...KIND_ORDER].reverse()).toEqual([ + "application", + "service", + "database", + ]); + }); +}); + describe("applyHostnameOverlay", () => { it("replaces only domains of named apps", () => { const out = applyHostnameOverlay(desired, {