fix(localdb): refuse AUTOINCREMENT keys at RegisterReplicated (LocalDb-005)

Each node assigns its own AUTOINCREMENT keys, so two different rows inserted on the two nodes share a pk_json and LWW silently keeps one on both nodes. RegisterReplicated now refuses such a table (detected from SQLite column metadata, before any trigger is installed). A plain INTEGER PRIMARY KEY is still accepted (application-assigned ids are legitimate); the requirement that replicated keys be unique across both nodes is documented in the RegisterReplicated remarks and README Limitations. No ScadaBridge or OtOpcUa replicated table is AUTOINCREMENT. (scadaproj#46) Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
2026-11-09 15:09:45 -05:00
parent 76e9c355b4
commit fd46999946
5 changed files with 142 additions and 6 deletions
@@ -38,11 +38,22 @@ public interface ILocalDb
    /// schema digest, and installs the three AFTER triggers that capture every change into the
    /// oplog + row-version tables within the same transaction as the consumer's write. Idempotent
    /// (re-registering regenerates the triggers). Throws <see cref="LocalDbRegistrationException"/>
-    /// if the name is unsafe, the table is missing, it has no primary key, or it is the parent of a
-    /// foreign key whose <c>ON DELETE</c> action mutates the child (<c>CASCADE</c> / <c>SET NULL</c> /
-    /// <c>SET DEFAULT</c>) — see the remarks for why that last one cannot be replicated.
+    /// if the name is unsafe, the table is missing, it has no primary key, its primary key is
+    /// <c>AUTOINCREMENT</c> (0.3.6), or it is the parent of a foreign key whose <c>ON DELETE</c> action
+    /// mutates the child (<c>CASCADE</c> / <c>SET NULL</c> / <c>SET DEFAULT</c>) — see the remarks for why
+    /// the last two cannot be replicated.
    /// </summary>
    /// <remarks>
+    /// <para>
+    /// <b>Replicated primary keys must be unique across BOTH nodes.</b> Last-writer-wins keys on the
+    /// primary key, so two different rows that two nodes insert under the same key are read as two
+    /// versions of ONE row and one of them is silently lost on both nodes. A key each node assigns on
+    /// its own therefore cannot replicate: an <c>AUTOINCREMENT</c> key is refused outright, and a plain
+    /// <c>INTEGER PRIMARY KEY</c> (a rowid alias) is accepted only because the application may assign
+    /// it from a node-scoped range — left NULL on insert it loses rows the same way. Prefer a TEXT GUID
+    /// minted by the writer.
+    /// </para>
+    /// <para>
    /// <b>Replicated tables may not be the parent of a cascading foreign key.</b> The apply path
    /// deletes rows for remote tombstones (and, before 0.3.6, wrote remote rows with
    /// <c>INSERT OR REPLACE</c>, which deleted the conflicting row first); with
@@ -52,6 +63,7 @@ public interface ILocalDb
    /// runs at registration and inspects the whole database's foreign keys; SQLite cannot add a
    /// foreign key to an existing table, so a later-created child table is caught on the next
    /// registration (i.e. the next startup), not at the moment of the DDL.
+    /// </para>
    /// <para>
    /// <b>Secondary <c>UNIQUE</c> keys must be unique across BOTH nodes.</b> Since 0.3.6 a remote row
    /// is upserted on its primary key only; if it collides with a different local row on another
@@ -199,6 +199,7 @@ internal sealed class SqliteLocalDb : ILocalDb, IDisposable
                throw new LocalDbRegistrationException(
                    $"Cannot register table '{tableName}': replication requires an explicit primary key.");

+            EnsureNoAutoincrementKey(tableName);
            EnsureNoInboundActionOnDelete(tableName);

            var allColumns = columns.Select(c => (c.Name, c.Type)).ToList();
@@ -295,6 +296,46 @@ internal sealed class SqliteLocalDb : ILocalDb, IDisposable
        }
    }

+    /// <summary>
+    /// Refuses to register a table whose primary key is declared <c>AUTOINCREMENT</c> (LocalDb-005).
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// <b>Why.</b> LWW keys on <c>pk_json</c>. <c>AUTOINCREMENT</c> exists to have SQLite assign the
+    /// key, and each node assigns its own 1, 2, 3 … — so two DIFFERENT rows inserted on the two nodes
+    /// (both nodes of a pair logging during a split, say) carry the same <c>pk_json</c>, LWW reads them
+    /// as two versions of one row, and one row is lost on BOTH nodes with no dead-letter and no metric.
+    /// That is never what a replicated table wants, so it is refused rather than warned about.
+    /// </para>
+    /// <para>
+    /// <b>A plain <c>INTEGER PRIMARY KEY</c> is still allowed</b>, because the application may assign
+    /// it itself from a globally unique or node-scoped range. Left NULL on insert it has exactly the
+    /// same hazard (it is the same rowid alias, minus the never-reuse guarantee); the remarks on
+    /// <see cref="ILocalDb.RegisterReplicated(string)"/> say so, and the safe choice is a TEXT GUID.
+    /// </para>
+    /// <para>
+    /// Detection reads SQLite's own column metadata (<c>sqlite3_table_column_metadata</c>, surfaced as
+    /// <c>IsAutoIncrement</c> in the reader's schema table) rather than parsing the <c>CREATE</c> text,
+    /// so a column named or defaulted to the word is not mistaken for the keyword.
+    /// </para>
+    /// </remarks>
+    private void EnsureNoAutoincrementKey(string tableName)
+    {
+        using var cmd = _master.CreateCommand();
+        cmd.CommandText = $"SELECT * FROM {TriggerSqlGenerator.Ident(tableName)} LIMIT 0";
+        using var reader = cmd.ExecuteReader();
+        var schema = reader.GetSchemaTable();
+        foreach (System.Data.DataRow row in schema.Rows)
+        {
+            if (row["IsAutoIncrement"] is true)
+                throw new LocalDbRegistrationException(
+                    $"Cannot register table '{tableName}': its primary key '{row["BaseColumnName"]}' is declared " +
+                    "AUTOINCREMENT. Each node would assign the same keys to different rows, and last-writer-wins " +
+                    "would silently keep only one of them on both nodes. Use a globally unique key instead " +
+                    "(a TEXT GUID, or an INTEGER the application assigns from a node-scoped range).");
+        }
+    }
+
    /// <summary>
    /// Refuses to register a table that is the PARENT of a foreign key whose <c>ON DELETE</c> action
    /// mutates the child (<c>CASCADE</c>, <c>SET NULL</c>, <c>SET DEFAULT</c>).