milestone 20: declarative configuration for iac

src/storage/config_schema.zig

@@ -1,5 +1,5 @@
-//! The `config.db` schema, verbatim from PLAN §11.2, plus the two table orders
-//! every other storage session needs.
+//! The `config.db` schema, verbatim from PLAN §11.2, plus the table lists every
+//! other storage session needs.
 //!
 //! The DDL text is data, not code: `migrations.zig` carries it as step 1 and
 //! never edits it in place. A schema change is a *new* step with new DDL, so
@@ -89,8 +89,10 @@ pub const ddl_v1: [:0]const u8 =
     \\CREATE TABLE settings (key TEXT PRIMARY KEY, value TEXT NOT NULL);
 ;
 
-/// Child-before-parent. Used by import's wipe step; correct under
-/// `foreign_keys = ON`.
+/// Child-before-parent, and correct under `foreign_keys = ON`. The reconcile
+/// engine deletes in this order so that every declarative child of a dying
+/// parent is removed — and counted — before the parent goes, which keeps the FK
+/// cascades a safety net rather than the accountant.
 ///
 /// `upstreams`, `local_records`, `forward_zones` and `settings` have no foreign
 /// keys, so their position is free; `groups` and `blocklist_sources` must come
@@ -102,18 +104,28 @@ pub const delete_order = [_][]const u8{
     "blocklist_sources", "groups",
 };
 
-/// Every table whose emptiness defines "the database has never been configured"
-/// (S5.2). `groups` is absent because migration step 1 seeds `(1, 'default')`,
-/// so an empty database still holds one group row; `schema_version` is absent
-/// for the same reason.
-pub const content_tables = [_][]const u8{
-    "clients",       "client_prefixes", "upstreams",     "blocklist_sources",
-    "group_sources", "rules",           "local_records", "forward_zones",
-    "settings",
+/// Every table that holds configuration, in a fixed order. It includes
+/// `groups`, because a byte-stability dump has to be able to see a reconcile
+/// that renumbered a group.
+///
+/// `schema_version` is absent: it is the migration's, not the operator's.
+pub const table_names = [_][]const u8{
+    "groups",            "clients",       "client_prefixes", "upstreams",
+    "blocklist_sources", "group_sources", "rules",           "local_records",
+    "forward_zones",     "settings",
 };
 
 const testing = std.testing;
 
+test "table_names names exactly the tables delete_order does" {
+    try testing.expectEqual(delete_order.len, table_names.len);
+    for (delete_order) |name| {
+        try testing.expect(indexOf(&table_names, name) != null);
+    }
+    try testing.expect(indexOf(&table_names, "groups") != null);
+    try testing.expect(indexOf(&table_names, "schema_version") == null);
+}
+
 test "delete_order lists every referrer before the table it references" {
     // The two parents in the schema. Every child that references them must be
     // deleted first, or `foreign_keys = ON` turns import's wipe into a
@@ -130,15 +142,6 @@ test "delete_order lists every referrer before the table it references" {
     }
 }
 
-test "content_tables is delete_order without groups" {
-    try testing.expectEqual(delete_order.len - 1, content_tables.len);
-    for (content_tables) |name| {
-        try testing.expect(indexOf(&delete_order, name) != null);
-    }
-    try testing.expect(indexOf(&content_tables, "groups") == null);
-    try testing.expect(indexOf(&content_tables, "schema_version") == null);
-}
-
 fn indexOf(haystack: []const []const u8, needle: []const u8) ?usize {
     for (haystack, 0..) |item, i| {
         if (std.mem.eql(u8, item, needle)) return i;

src/storage/db.zig

@@ -57,6 +57,7 @@ pub const c = struct {
     pub extern fn sqlite3_column_bytes(stmt: *c.Stmt, col: c_int) c_int;
     pub extern fn sqlite3_last_insert_rowid(db: *Sqlite3) i64;
     pub extern fn sqlite3_changes(db: *Sqlite3) c_int;
+    pub extern fn sqlite3_total_changes(db: *Sqlite3) c_int;
 };
 
 /// Result codes, from the vendored `sqlite3.h` (3.53.4).
@@ -429,6 +430,15 @@ pub const Db = struct {
     pub fn changes(self: *Db) i64 {
         return c.sqlite3_changes(self.handle);
     }
+
+    /// Every row this connection has inserted, updated or deleted since it was
+    /// opened. Monotonic, so a caller proves "this call wrote nothing" by
+    /// reading it either side and comparing — which is stronger than comparing
+    /// content, because an UPDATE that rewrites identical values still moves
+    /// this counter.
+    pub fn totalChanges(self: *Db) i64 {
+        return c.sqlite3_total_changes(self.handle);
+    }
 };
 
 fn openHandle(filename: [:0]const u8, flags: c_int) Error!*c.Sqlite3 {

src/storage/migrations.zig

@@ -353,7 +353,7 @@ test "readVersion reads a file database through an immutable open, writing nothi
     try testing.expectEqual(@as(u32, 1), try readVersion(&database));
 }
 
-test "delete_order and content_tables name exactly the tables the schema creates" {
+test "delete_order and table_names name exactly the tables the schema creates" {
     var database = try openMigrated();
     defer database.close();
     _ = try migrate(&database);
@@ -361,7 +361,7 @@ test "delete_order and content_tables name exactly the tables the schema creates
     for (config_schema.delete_order) |name| {
         try testing.expect(try tableExists(&database, name));
     }
-    for (config_schema.content_tables) |name| {
+    for (config_schema.table_names) |name| {
         try testing.expect(try tableExists(&database, name));
     }
     // delete_order covers every table except `schema_version`.

src/storage/repositories/clients_repo.zig

@@ -3,13 +3,14 @@
 //! `listClients` returns only `hand_edited = 1` rows. A client the server
 //! materialised from live traffic is runtime state, not configuration, and must
 //! not appear in an export. `hand_edited` is the only marker of operator intent
-//! in this table, so it also decides what `import.isEmpty` counts: a database
-//! carrying nothing but materialised rows has never been configured, and a seed
-//! file must still be able to fill it. `countClients` counts **all** rows and is
-//! a test helper — it deliberately does not answer that question.
+//! in this table, so it also decides what the reconcile engine may delete: a
+//! declared row the file drops is removed, an observed row is kept whatever the
+//! file says, and declaring an observed address promotes that row in place.
+//! `countClients` counts **all** rows and is a test helper.
 //!
-//! The import path is list / insert / deleteAll / count, plus the two runtime
-//! calls `upsertSeen` and `pruneStale` that `server/clients.zig`'s tracker owns.
+//! The configuration path is list / insert / update / delete / count, plus the
+//! two runtime calls `upsertSeen` and `pruneStale` that `server/clients.zig`'s
+//! tracker owns.
 //! The REST surface is the third section: it speaks row ids and shows
 //! every client, materialised ones included.
 
@@ -126,7 +127,7 @@ pub fn deleteAllClients(database: *db.Db) db.Error!void {
 }
 
 /// Counts every row, including the materialised ones `listClients` filters out.
-/// Used by tests; `import.isEmpty` counts operator intent instead.
+/// Used by tests.
 pub fn countClients(database: *db.Db) db.Error!i64 {
     return database.queryInt("SELECT count(*) FROM clients");
 }
@@ -407,6 +408,67 @@ pub fn replaceClientPrefixes(database: *db.Db, items: []const ClientPrefixInput)
     try tx.commit();
 }
 
+// ---------------------------------------------------------------------------
+// reconcile surface (milestone 20)
+// ---------------------------------------------------------------------------
+//
+// `replaceClientPrefixes` above is the REST list resource: one atomic swap of
+// the whole table, in a transaction of its own. The reconcile engine cannot use
+// it — it runs inside a transaction already, and rewriting every row would
+// forfeit the row ids and the zero-writes property the engine exists for — so
+// it edits and removes prefixes one at a time instead.
+
+/// Writes the two columns a prefix row carries besides its identity.
+///
+/// `error.NotFound`: no prefix holds `id`. `error.Constraint`:
+/// `client_prefixes.prefix` is UNIQUE, or `group_id` names no group.
+pub fn updateClientPrefix(database: *db.Db, id: i64, item: ClientPrefixInput) db.Error!void {
+    var stmt = try database.prepare(
+        "UPDATE client_prefixes SET prefix = ?2, group_id = ?3, priority = ?4 WHERE id = ?1",
+    );
+    defer stmt.deinit();
+    try stmt.bindInt(1, id);
+    try stmt.bindText(2, item.prefix);
+    try stmt.bindInt(3, item.group_id);
+    try stmt.bindInt(4, item.priority);
+    return crud.execStrict(database, &stmt);
+}
+
+/// `error.NotFound`: no prefix holds `id`. Nothing references
+/// `client_prefixes`, so a delete cannot violate a constraint.
+pub fn deleteClientPrefix(database: *db.Db, id: i64) db.Error!void {
+    var stmt = try database.prepare("DELETE FROM client_prefixes WHERE id = ?1");
+    defer stmt.deinit();
+    try stmt.bindInt(1, id);
+    return crud.execStrict(database, &stmt);
+}
+
+/// Moves the observed clients of one group to another, and reports how many
+/// rows moved.
+///
+/// `clients.group_id` references `groups(id)` with no `ON DELETE` action
+/// (config_schema.zig:26), so a group that any client still sits in cannot be
+/// deleted. When a configuration stops declaring a group, its *declared*
+/// clients go with it, but the devices the DNS path materialised into it did
+/// not come from the configuration and must not be deleted for a decision that
+/// was never about them. They move to the default group, which is also the
+/// semantics the operator asked for: they un-declared the group, not the
+/// devices.
+///
+/// `hand_edited = 1` rows are untouched — those are configuration, and the
+/// reconcile engine has already accounted for them.
+pub fn reassignObservedClients(database: *db.Db, from_group_id: i64, to_group_id: i64) db.Error!u32 {
+    var stmt = try database.prepare(
+        "UPDATE clients SET group_id = ?2 WHERE hand_edited = 0 AND group_id = ?1",
+    );
+    defer stmt.deinit();
+    try stmt.bindInt(1, from_group_id);
+    try stmt.bindInt(2, to_group_id);
+    try stmt.exec();
+    const moved = database.changes();
+    return @intCast(@min(moved, std.math.maxInt(u32)));
+}
+
 // ---------------------------------------------------------------------------
 // tests
 // ---------------------------------------------------------------------------

src/storage/repositories/groups_repo.zig

@@ -254,6 +254,41 @@ pub fn setGroupSources(database: *db.Db, group_id: i64, source_ids: []const i64)
     try tx.commit();
 }
 
+// ---------------------------------------------------------------------------
+// reconcile surface (milestone 20)
+// ---------------------------------------------------------------------------
+//
+// `group_sources` has no row id — the pair *is* the identity — so the reconcile
+// engine matches on the pair and needs the ids the name-keyed list above
+// resolves away.
+
+pub const GroupSourcePair = struct { group_id: i64, source_id: i64 };
+
+/// Every assignment as the pair of ids it is. Nothing to free.
+pub fn listGroupSourcePairs(database: *db.Db, gpa: Allocator) db.Error!std.ArrayList(GroupSourcePair) {
+    return crud.listRows(
+        GroupSourcePair,
+        database,
+        gpa,
+        "SELECT group_id, source_id FROM group_sources ORDER BY group_id, source_id",
+        readGroupSourcePair,
+    );
+}
+
+fn readGroupSourcePair(stmt: *db.Stmt, gpa: Allocator) db.Error!GroupSourcePair {
+    _ = gpa;
+    return .{ .group_id = stmt.columnInt(0), .source_id = stmt.columnInt(1) };
+}
+
+/// Removes one assignment. `error.NotFound`: no row holds the pair.
+pub fn deleteGroupSourcePair(database: *db.Db, pair: GroupSourcePair) db.Error!void {
+    var stmt = try database.prepare("DELETE FROM group_sources WHERE group_id = ?1 AND source_id = ?2");
+    defer stmt.deinit();
+    try stmt.bindInt(1, pair.group_id);
+    try stmt.bindInt(2, pair.source_id);
+    return crud.execStrict(database, &stmt);
+}
+
 fn groupExists(database: *db.Db, id: i64) db.Error!bool {
     var stmt = try database.prepare("SELECT 1 FROM groups WHERE id = ?1");
     defer stmt.deinit();

src/storage/repositories/settings_repo.zig

@@ -88,6 +88,18 @@ pub fn putSetting(database: *db.Db, key: []const u8, value: []const u8) db.Error
     try stmt.exec();
 }
 
+/// Removes one key. Silent about a key that is not stored: the reconcile
+/// engine's sweep computes the set of keys to drop from a list it has already
+/// read, so "no such row" is not a caller error the way it is for a by-id
+/// mutation, and `execStrict` would turn a harmless race into a failed
+/// transaction.
+pub fn deleteSetting(database: *db.Db, key: []const u8) db.Error!void {
+    var stmt = try database.prepare("DELETE FROM settings WHERE key = ?1");
+    defer stmt.deinit();
+    try stmt.bindText(1, key);
+    return stmt.exec();
+}
+
 // ---------------------------------------------------------------------------
 // tests
 // ---------------------------------------------------------------------------

src/storage/storage_integration_test.zig

@@ -22,7 +22,6 @@ const build_options = @import("build_options");
 const Writer = std.Io.Writer;
 
 const cli = @import("../cli.zig");
-const bootstrap = @import("../config/bootstrap.zig");
 const config_export = @import("../config/export.zig");
 const import = @import("../config/import.zig");
 const model = @import("../config/model.zig");
@@ -127,7 +126,7 @@ fn openMigrated(f: *const Fixture, name: []const u8) !Data {
     return .{ .dir = dir, .database = database };
 }
 
-fn importInto(f: *const Fixture, data: *Data, file: []const u8, force: bool) !void {
+fn importInto(f: *const Fixture, data: *Data, file: []const u8, allow_delete: bool) !void {
     var diags: validate.Diagnostics = .init(testing.allocator);
     defer diags.deinit();
     return import.importFile(
@@ -136,7 +135,7 @@ fn importInto(f: *const Fixture, data: *Data, file: []const u8, force: bool) !vo
         &data.database,
         f.tmp.dir,
         file,
-        .{ .force = force },
+        .{ .allow_delete = allow_delete },
         &diags,
     );
 }
@@ -655,7 +654,7 @@ test "S7 case 10: a config.db stamped one version ahead is refused and left alon
 }
 
 // ---------------------------------------------------------------------------
-// case 11-19: export, import and bootstrap on real files
+// case 11-19: export and import on real files
 // ---------------------------------------------------------------------------
 
 test "S7 case 11: an exported file is mode 0600 and starts with the header comment" {
@@ -704,7 +703,7 @@ test "S7 case 12: export, import and export again are byte-identical files" {
     try testing.expectEqualStrings(a, b);
 }
 
-test "S7 case 13: import refuses a configured database unless --force is given" {
+test "S7 case 13: import refuses a diff that deletes rows unless --allow-delete is given" {
     if (!build_options.integration) return error.SkipZigTest;
 
     var f: Fixture = .init();
@@ -716,7 +715,10 @@ test "S7 case 13: import refuses a configured database unless --force is given"
     defer data.deinit();
     try importInto(&f, &data, "first.zon", false);
 
-    try testing.expectError(error.DatabaseNotEmpty, importInto(&f, &data, "second.zon", false));
+    // The two files name different upstream urls, and a url is the upstream's
+    // identity: applying the second deletes the first's row, which is what the
+    // gate exists to stop.
+    try testing.expectError(error.DestructiveImport, importInto(&f, &data, "second.zon", false));
     try testing.expectEqual(
         @as(i64, 1),
         try data.database.queryInt(
@@ -753,14 +755,15 @@ test "S7 case 14: an invalid import reports every problem and writes nothing" {
         &data.database,
         f.tmp.dir,
         "config.zon",
-        .{ .force = false },
+        .{ .allow_delete = false },
         &diags,
     )) |_| {
         return error.TestUnexpectedResult;
     } else |_| {}
 
     try testing.expectEqual(@as(usize, 2), diags.problems.items.len);
-    try testing.expect(try import.isEmpty(&data.database));
+    try testing.expectEqual(@as(i64, 0), try data.database.queryInt("SELECT count(*) FROM upstreams"));
+    try testing.expectEqual(@as(i64, 0), try data.database.queryInt("SELECT count(*) FROM settings"));
 
     // Nothing beyond the database and its sidecars was created.
     var dir = try f.tmp.dir.openDir(io, "data", .{ .iterate = true });
@@ -771,129 +774,6 @@ test "S7 case 14: an invalid import reports every problem and writes nothing" {
     }
 }
 
-test "S7 case 15: bootstrap with no configuration file leaves the database empty" {
-    if (!build_options.integration) return error.SkipZigTest;
-
-    var f: Fixture = .init();
-    defer f.deinit();
-
-    var data = try openMigrated(&f, "data");
-    defer data.deinit();
-
-    var diags: validate.Diagnostics = .init(testing.allocator);
-    defer diags.deinit();
-
-    const outcome = try bootstrap.bootstrap(
-        io,
-        testing.allocator,
-        &data.database,
-        f.tmp.dir,
-        "config.zon",
-        &diags,
-    );
-    try testing.expectEqual(bootstrap.Outcome.no_config_file, outcome);
-    try testing.expect(try import.isEmpty(&data.database));
-}
-
-test "S7 case 16: bootstrap seeds an empty database from the configuration file" {
-    if (!build_options.integration) return error.SkipZigTest;
-
-    var f: Fixture = .init();
-    defer f.deinit();
-    try f.write("config.zon", rich_config);
-
-    var data = try openMigrated(&f, "data");
-    defer data.deinit();
-
-    var diags: validate.Diagnostics = .init(testing.allocator);
-    defer diags.deinit();
-
-    const outcome = try bootstrap.bootstrap(
-        io,
-        testing.allocator,
-        &data.database,
-        f.tmp.dir,
-        "config.zon",
-        &diags,
-    );
-    try testing.expectEqual(bootstrap.Outcome.seeded, outcome);
-    try testing.expectEqual(@as(i64, 2), try data.database.queryInt("SELECT count(*) FROM groups"));
-    try testing.expectEqual(@as(i64, 2), try data.database.queryInt("SELECT count(*) FROM upstreams"));
-    try testing.expectEqual(
-        @as(i64, 1),
-        try data.database.queryInt("SELECT count(*) FROM clients WHERE ip = 'fd00::1'"),
-    );
-    try testing.expectEqual(
-        @as(i64, 5353),
-        try data.database.queryInt("SELECT CAST(value AS INTEGER) FROM settings WHERE key = 'dns.port'"),
-    );
-}
-
-test "S7 case 17: bootstrap on a configured database never reads the file" {
-    if (!build_options.integration) return error.SkipZigTest;
-
-    var f: Fixture = .init();
-    defer f.deinit();
-    try f.write("seed.zon", minimal_config);
-
-    var data = try openMigrated(&f, "data");
-    defer data.deinit();
-    try importInto(&f, &data, "seed.zon", false);
-
-    // Unparseable on purpose: the call can only succeed if the file is never
-    // opened.
-    try f.write("config.zon", broken_zon);
-
-    var diags: validate.Diagnostics = .init(testing.allocator);
-    defer diags.deinit();
-
-    const outcome = try bootstrap.bootstrap(
-        io,
-        testing.allocator,
-        &data.database,
-        f.tmp.dir,
-        "config.zon",
-        &diags,
-    );
-    try testing.expectEqual(bootstrap.Outcome.db_already_configured, outcome);
-    try testing.expectEqual(@as(usize, 0), diags.problems.items.len);
-    try testing.expectEqual(
-        @as(i64, 1),
-        try data.database.queryInt(
-            "SELECT count(*) FROM upstreams WHERE url = 'https://dns.example/dns-query'",
-        ),
-    );
-}
-
-test "S7 case 18: bootstrap with an invalid configuration file fails and writes nothing" {
-    if (!build_options.integration) return error.SkipZigTest;
-
-    var f: Fixture = .init();
-    defer f.deinit();
-    try f.write("config.zon", two_problem_config);
-
-    var data = try openMigrated(&f, "data");
-    defer data.deinit();
-
-    var diags: validate.Diagnostics = .init(testing.allocator);
-    defer diags.deinit();
-
-    if (bootstrap.bootstrap(
-        io,
-        testing.allocator,
-        &data.database,
-        f.tmp.dir,
-        "config.zon",
-        &diags,
-    )) |outcome| {
-        std.debug.print("bootstrap unexpectedly returned .{s}\n", .{@tagName(outcome)});
-        return error.TestUnexpectedResult;
-    } else |_| {}
-
-    try testing.expectEqual(@as(usize, 2), diags.problems.items.len);
-    try testing.expect(try import.isEmpty(&data.database));
-}
-
 test "S7 case 19: writeToFile replaces an existing file and restores mode 0600" {
     if (!build_options.integration) return error.SkipZigTest;
 
@@ -988,11 +868,13 @@ test "S7 case 21: runCheck passes a seeded database and reports two stored probl
         try importInto(&f, &data, "config.zon", false);
     }
     {
-        // `applyToDb` rather than an import: the validator would refuse this
+        // `apply` rather than an import: the validator would refuse this
         // configuration, and the case needs the problems to reach the database.
         var data = try openMigrated(&f, "bad");
         defer data.deinit();
-        try import.applyToDb(io, testing.allocator, &data.database, two_problem_model, 42, .{});
+        var diags: validate.Diagnostics = .init(testing.allocator);
+        defer diags.deinit();
+        try import.apply(io, testing.allocator, &data.database, two_problem_model, 42, .{}, &diags);
     }
 
     {
@@ -1045,13 +927,16 @@ test "S7 case 22: runCheck probes a real upstream and prints an OK line" {
 
     const code = cli.runCheck(
         captured.runner(),
-        .{ .paths = .{ .config = config_path }, .config_explicit = true },
+        .{ .config = config_path },
         true,
     );
     try testing.expectEqual(cli.exit_ok, code);
+    // Milestone 13 changed the probe line to the redacted `OK upstreams[i]`
+    // form; this expectation went stale unnoticed because nothing ran -Dlive
+    // between then and milestone 20.
     try testing.expect(std.mem.count(
         u8,
         captured.out.written(),
-        "OK https://cloudflare-dns.com/dns-query\n",
+        "OK upstreams[0] https://cloudflare-dns.com\n",
     ) == 1);
 }