fix: idempotent migrations (ADD COLUMN IF NOT EXISTS), stale doc, safety note
This commit is contained in:
parent
7d36ec5e1f
commit
c47faf0af9
5 changed files with 80 additions and 26 deletions
|
|
@ -30,14 +30,15 @@ CREATE INDEX IF NOT EXISTS idx_approvals_pending
|
||||||
/// all known migrations. New columns go here as v5, v6, …
|
/// all known migrations. New columns go here as v5, v6, …
|
||||||
const MIGRATIONS: &[&str] = &[
|
const MIGRATIONS: &[&str] = &[
|
||||||
// v1: `kind` (pre-Phase-8 dbs): legacy rows default to `apply_commit`.
|
// v1: `kind` (pre-Phase-8 dbs): legacy rows default to `apply_commit`.
|
||||||
"ALTER TABLE approvals ADD COLUMN kind TEXT NOT NULL DEFAULT 'apply_commit'",
|
"ALTER TABLE approvals ADD COLUMN IF NOT EXISTS \
|
||||||
|
kind TEXT NOT NULL DEFAULT 'apply_commit'",
|
||||||
// v2: `description`: manager-supplied note on the dashboard card.
|
// v2: `description`: manager-supplied note on the dashboard card.
|
||||||
"ALTER TABLE approvals ADD COLUMN description TEXT",
|
"ALTER TABLE approvals ADD COLUMN IF NOT EXISTS description TEXT",
|
||||||
// v3: `fetched_sha`: canonical sha hive-c0re resolved at submit time.
|
// v3: `fetched_sha`: canonical sha hive-c0re resolved at submit time.
|
||||||
"ALTER TABLE approvals ADD COLUMN fetched_sha TEXT",
|
"ALTER TABLE approvals ADD COLUMN IF NOT EXISTS fetched_sha TEXT",
|
||||||
// v4: `submitter`: authenticated agent that submitted the approval.
|
// v4: `submitter`: authenticated agent that submitted the approval.
|
||||||
// Legacy rows are NULL → callers fall back to the root agent.
|
// Legacy rows are NULL → callers fall back to the root agent.
|
||||||
"ALTER TABLE approvals ADD COLUMN submitter TEXT",
|
"ALTER TABLE approvals ADD COLUMN IF NOT EXISTS submitter TEXT",
|
||||||
];
|
];
|
||||||
|
|
||||||
pub struct Approvals {
|
pub struct Approvals {
|
||||||
|
|
|
||||||
|
|
@ -162,23 +162,34 @@ pub struct Broker {
|
||||||
/// `messages` — the last column added before versioning.
|
/// `messages` — the last column added before versioning.
|
||||||
const BROKER_MIGRATIONS: &[&str] = &[
|
const BROKER_MIGRATIONS: &[&str] = &[
|
||||||
// v1: acked_at on messages, with backfill so existing delivered rows
|
// v1: acked_at on messages, with backfill so existing delivered rows
|
||||||
// are not phantom-requeued on the next open. The multi-statement
|
// are not phantom-requeued on the next open.
|
||||||
// batch runs atomically (execute_batch wraps in a transaction).
|
//
|
||||||
"ALTER TABLE messages ADD COLUMN acked_at INTEGER;\
|
// IF NOT EXISTS: safe to run on a partially-migrated legacy DB where
|
||||||
UPDATE messages SET acked_at = delivered_at WHERE delivered_at IS NOT NULL;",
|
// acked_at already exists but priority (the legacy marker) does not.
|
||||||
|
// The BEGIN/COMMIT block makes ALTER + UPDATE atomic — if the UPDATE
|
||||||
|
// fails the version counter is not bumped and the whole step retries.
|
||||||
|
"BEGIN;\
|
||||||
|
ALTER TABLE messages ADD COLUMN IF NOT EXISTS acked_at INTEGER;\
|
||||||
|
UPDATE messages SET acked_at = delivered_at \
|
||||||
|
WHERE delivered_at IS NOT NULL AND acked_at IS NULL;\
|
||||||
|
COMMIT;",
|
||||||
// v2: in_reply_to for thread-parent tracking. NULL = root of a thread.
|
// v2: in_reply_to for thread-parent tracking. NULL = root of a thread.
|
||||||
"ALTER TABLE messages ADD COLUMN in_reply_to INTEGER",
|
"ALTER TABLE messages ADD COLUMN IF NOT EXISTS in_reply_to INTEGER",
|
||||||
// v3: priority for operator-message fast-path. Rebuild the delivery
|
// v3: priority for operator-message fast-path. Rebuild the delivery
|
||||||
// index to include priority as a secondary sort key.
|
// index to include priority as a secondary sort key.
|
||||||
"ALTER TABLE messages ADD COLUMN priority INTEGER NOT NULL DEFAULT 0;\
|
"BEGIN;\
|
||||||
|
ALTER TABLE messages ADD COLUMN IF NOT EXISTS \
|
||||||
|
priority INTEGER NOT NULL DEFAULT 0;\
|
||||||
DROP INDEX IF EXISTS idx_messages_undelivered;\
|
DROP INDEX IF EXISTS idx_messages_undelivered;\
|
||||||
CREATE INDEX idx_messages_undelivered \
|
CREATE INDEX IF NOT EXISTS idx_messages_undelivered \
|
||||||
ON messages (recipient, priority DESC, id) WHERE delivered_at IS NULL",
|
ON messages (recipient, priority DESC, id) WHERE delivered_at IS NULL;\
|
||||||
|
COMMIT;",
|
||||||
// v4: attempt_count on reminders for the MAX_REMINDER_ATTEMPTS cap.
|
// v4: attempt_count on reminders for the MAX_REMINDER_ATTEMPTS cap.
|
||||||
"ALTER TABLE reminders ADD COLUMN attempt_count INTEGER NOT NULL DEFAULT 0",
|
"ALTER TABLE reminders ADD COLUMN IF NOT EXISTS \
|
||||||
|
attempt_count INTEGER NOT NULL DEFAULT 0",
|
||||||
// v5: last_error on reminders — last delivery failure surfaced on the
|
// v5: last_error on reminders — last delivery failure surfaced on the
|
||||||
// dashboard so a stuck reminder is visible without digging in logs.
|
// dashboard so a stuck reminder is visible without digging in logs.
|
||||||
"ALTER TABLE reminders ADD COLUMN last_error TEXT",
|
"ALTER TABLE reminders ADD COLUMN IF NOT EXISTS last_error TEXT",
|
||||||
];
|
];
|
||||||
|
|
||||||
impl Broker {
|
impl Broker {
|
||||||
|
|
@ -1134,11 +1145,6 @@ impl Broker {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Idempotent messages-table migrations. Adds `acked_at` and
|
|
||||||
/// back-fills it for every already-delivered row, so the
|
|
||||||
/// pre-migration sessions count as "fully handled" and won't be
|
|
||||||
/// resurfaced by the first `requeue_inflight` after upgrade.
|
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
|
|
|
||||||
|
|
@ -80,6 +80,11 @@ pub fn apply_versioned_migrations(
|
||||||
// old try-and-ignore path — skip them. If it doesn't exist, start
|
// old try-and-ignore path — skip them. If it doesn't exist, start
|
||||||
// at 0 (fresh install or partial state).
|
// at 0 (fresh install or partial state).
|
||||||
let (legacy_table, legacy_col) = legacy_column;
|
let (legacy_table, legacy_col) = legacy_column;
|
||||||
|
// SAFETY: `legacy_table` is always a static table-name literal
|
||||||
|
// supplied by each store's `open` (e.g. `"messages"`). Dynamic
|
||||||
|
// or untrusted table names must not be passed here — if that
|
||||||
|
// ever changes, switch to `pragma_table_info(?1)` with two
|
||||||
|
// bound parameters.
|
||||||
let is_legacy: bool = conn
|
let is_legacy: bool = conn
|
||||||
.prepare(&format!(
|
.prepare(&format!(
|
||||||
"SELECT 1 FROM pragma_table_info('{legacy_table}') WHERE name = ?1"
|
"SELECT 1 FROM pragma_table_info('{legacy_table}') WHERE name = ?1"
|
||||||
|
|
@ -143,9 +148,9 @@ mod tests {
|
||||||
|
|
||||||
const TEST_SCHEMA: &str = "CREATE TABLE IF NOT EXISTS things (id INTEGER PRIMARY KEY)";
|
const TEST_SCHEMA: &str = "CREATE TABLE IF NOT EXISTS things (id INTEGER PRIMARY KEY)";
|
||||||
const TEST_MIGRATIONS: &[&str] = &[
|
const TEST_MIGRATIONS: &[&str] = &[
|
||||||
"ALTER TABLE things ADD COLUMN alpha TEXT",
|
"ALTER TABLE things ADD COLUMN IF NOT EXISTS alpha TEXT",
|
||||||
"ALTER TABLE things ADD COLUMN beta INTEGER NOT NULL DEFAULT 0",
|
"ALTER TABLE things ADD COLUMN IF NOT EXISTS beta INTEGER NOT NULL DEFAULT 0",
|
||||||
"ALTER TABLE things ADD COLUMN gamma TEXT",
|
"ALTER TABLE things ADD COLUMN IF NOT EXISTS gamma TEXT",
|
||||||
];
|
];
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|
@ -289,4 +294,44 @@ mod tests {
|
||||||
assert_eq!(va, 1);
|
assert_eq!(va, 1);
|
||||||
assert_eq!(vb, 1);
|
assert_eq!(vb, 1);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// A partially-migrated legacy database: some columns from the old
|
||||||
|
/// try-and-ignore path exist, but the legacy marker column (used to
|
||||||
|
/// fast-forward fresh schema_versions entries to `len`) is absent.
|
||||||
|
/// With idempotent `ADD COLUMN IF NOT EXISTS` migrations this must
|
||||||
|
/// succeed — the already-present columns are skipped and only the
|
||||||
|
/// missing ones are added.
|
||||||
|
#[test]
|
||||||
|
fn partial_legacy_completes_without_error() {
|
||||||
|
let conn = open_tmp();
|
||||||
|
// Simulate a DB that has `alpha` (v1) but not `beta` (v2) or
|
||||||
|
// `gamma` (v3). No schema_versions row exists yet, and `gamma`
|
||||||
|
// (the legacy marker) is absent → the function cannot fast-forward
|
||||||
|
// and must start from v0 with idempotent migrations.
|
||||||
|
conn.execute_batch(
|
||||||
|
"CREATE TABLE IF NOT EXISTS things (id INTEGER PRIMARY KEY, alpha TEXT)",
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
apply_versioned_migrations(&conn, "things", ("things", "gamma"), TEST_MIGRATIONS).unwrap();
|
||||||
|
// All three columns must now exist.
|
||||||
|
for col in ["alpha", "beta", "gamma"] {
|
||||||
|
let exists: bool = conn
|
||||||
|
.prepare(&format!(
|
||||||
|
"SELECT 1 FROM pragma_table_info('things') WHERE name = '{col}'"
|
||||||
|
))
|
||||||
|
.unwrap()
|
||||||
|
.exists([])
|
||||||
|
.unwrap();
|
||||||
|
assert!(exists, "column {col} missing after partial-legacy migration");
|
||||||
|
}
|
||||||
|
// Version must be at the latest.
|
||||||
|
let v: i64 = conn
|
||||||
|
.query_row(
|
||||||
|
"SELECT version FROM schema_versions WHERE store = 'things'",
|
||||||
|
[],
|
||||||
|
|r| r.get(0),
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(v, 3, "must reach latest version after completing partial legacy");
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -39,13 +39,14 @@ CREATE INDEX IF NOT EXISTS idx_operator_questions_pending
|
||||||
/// past all known migrations.
|
/// past all known migrations.
|
||||||
const MIGRATIONS: &[&str] = &[
|
const MIGRATIONS: &[&str] = &[
|
||||||
// v1: `multi` — checkbox-style multi-option questions.
|
// v1: `multi` — checkbox-style multi-option questions.
|
||||||
"ALTER TABLE operator_questions ADD COLUMN multi INTEGER NOT NULL DEFAULT 0",
|
"ALTER TABLE operator_questions ADD COLUMN IF NOT EXISTS \
|
||||||
|
multi INTEGER NOT NULL DEFAULT 0",
|
||||||
// v2: `deadline_at` — optional TTL after which the watchdog auto-resolves.
|
// v2: `deadline_at` — optional TTL after which the watchdog auto-resolves.
|
||||||
"ALTER TABLE operator_questions ADD COLUMN deadline_at INTEGER",
|
"ALTER TABLE operator_questions ADD COLUMN IF NOT EXISTS deadline_at INTEGER",
|
||||||
// v3: `target` — recipient of the question. NULL = operator (back-compat
|
// v3: `target` — recipient of the question. NULL = operator (back-compat
|
||||||
// default); non-null = peer-to-peer question. Dashboard's `pending()`
|
// default); non-null = peer-to-peer question. Dashboard's `pending()`
|
||||||
// filters on `target IS NULL` so peer questions never leak to the operator.
|
// filters on `target IS NULL` so peer questions never leak to the operator.
|
||||||
"ALTER TABLE operator_questions ADD COLUMN target TEXT",
|
"ALTER TABLE operator_questions ADD COLUMN IF NOT EXISTS target TEXT",
|
||||||
];
|
];
|
||||||
|
|
||||||
#[derive(Debug, Clone, Serialize)]
|
#[derive(Debug, Clone, Serialize)]
|
||||||
|
|
|
||||||
|
|
@ -195,7 +195,8 @@ impl ScheduledPrompts {
|
||||||
("scheduled_prompts", "paused_at_unix"),
|
("scheduled_prompts", "paused_at_unix"),
|
||||||
&[
|
&[
|
||||||
// v1: add paused_at_unix for per-schedule pause support.
|
// v1: add paused_at_unix for per-schedule pause support.
|
||||||
"ALTER TABLE scheduled_prompts ADD COLUMN paused_at_unix INTEGER",
|
"ALTER TABLE scheduled_prompts \
|
||||||
|
ADD COLUMN IF NOT EXISTS paused_at_unix INTEGER",
|
||||||
],
|
],
|
||||||
)?;
|
)?;
|
||||||
// Migration: recreate the due-rows index to also exclude paused
|
// Migration: recreate the due-rows index to also exclude paused
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue