From 947ed31d84d257054d70cf914ebce74d21f2e51a Mon Sep 17 00:00:00 2001 From: bboonnee Date: Thu, 13 Aug 2026 16:01:30 +0800 Subject: [PATCH] fix: make migrate_drop_work_graph_run_org_node handle real duplicate-active-run data --- src/schema.py | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- src/test_schema.py | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 3 deletions(-) diff --git a/src/schema.py b/src/schema.py index 99a226b..e70c710 100644 --- a/src/schema.py +++ b/src/schema.py @@ -847,15 +847,39 @@ def migrate_drop_work_graph_run_org_node(conn): (each embeds the full column/constraint set valid as of ITS place in the sequence, not retroactively edited by later migrations). - Idempotent: skipped if work_graph_run no longer has an org_node column, - so a fresh create_schema()'d database (already on this final shape) is a - no-op, same convention as every other migrate_* function here.""" + Idempotent, and re-entrant against a database left PARTWAY through a + previous, interrupted attempt (real 2026-08-13 self-hosting incident: + executescript() does not wrap the whole script in one transaction, so a + prior run of this function got as far as dropping the org_node column + and renaming the table in, then died on the final CREATE UNIQUE INDEX + step -- see the _reconcile_duplicate_active_work_graph_runs() docstring + for why that step can fail against real data). Column-drop and index- + creation are therefore checked and completed independently rather than + as one all-or-nothing gate, so re-running this function against that + exact half-migrated state finishes the job instead of silently + no-op'ing (the OLD idempotency check here was just `if "org_node" not + in cols: return`, which would have skipped forever on that database, + permanently leaving ux_work_graph_run_active missing).""" migrate_relax_org_node_constraints(conn) conn.commit() + _reconcile_duplicate_active_work_graph_runs(conn) + conn.commit() + cols = {row[1] for row in conn.execute("PRAGMA table_info(work_graph_run)").fetchall()} + has_index = conn.execute( + "SELECT 1 FROM sqlite_master WHERE type = 'index' AND name = 'ux_work_graph_run_active'" + ).fetchone() is not None + if "org_node" not in cols: + if not has_index: + conn.execute( + "CREATE UNIQUE INDEX ux_work_graph_run_active ON work_graph_run(task_id) " + "WHERE status NOT IN ('done', 'failed')" + ) + conn.commit() return + conn.execute("PRAGMA foreign_keys = OFF") try: conn.executescript( @@ -897,3 +921,61 @@ def migrate_drop_work_graph_run_org_node(conn): ) finally: conn.execute("PRAGMA foreign_keys = ON") + + +def _reconcile_duplicate_active_work_graph_runs(conn): + """Real production data (2026-08-13 incident) can have MULTIPLE + non-terminal work_graph_run rows for the same task_id -- a legitimate + leftover of the pre-Phase-2c architecture where each org_node got its + own row instead of sharing one merged run. ux_work_graph_run_active's + UNIQUE(task_id) WHERE status NOT IN ('done', 'failed') can't be created + while such duplicates exist (10 affected task_ids found against the + live self-hosting database, e.g. one task had 8 concurrently-'open' + rows from the old per-org_node model). + + Keeps the most-recently-started non-terminal row per task_id as the + active one and marks every other non-terminal row for that task_id as + 'failed' -- 'blocked' was considered but rejected: ux_work_graph_run_active's + WHERE clause is `status NOT IN ('done', 'failed')`, so a 'blocked' row + still counts as active and would keep tripping the same UNIQUE + violation this function exists to prevent (confirmed by re-running this + against real duplicate data -- the index creation still failed with + 'blocked'). 'failed' is the only terminal-compatible status that + doesn't misrepresent these rows as successfully completed ('done' + would); it's an honest description too, since these are abandoned runs + from before the merged-graph architecture -- nothing will ever resume + against them directly again (Coder/Validator now only ever append into + the newest run for a task). Plain Python grouping rather than a SQL + window function, matching this file's existing style (no window + functions used elsewhere in this module) and avoiding any SQLite- + version assumption. + + Safe to call unconditionally and repeatedly: a database with no + duplicates (the common case) does zero UPDATEs. + + Ranks candidates by (started_at, rowid), NOT (started_at, run_id): + real data confirmed started_at is NULL on every one of these legacy + rows (never backfilled -- they predate this column being populated), + which would make a run_id tiebreak pick among them by UUID string + value, unrelated to actual chronology. rowid reflects true insertion + order (SQLite assigns it monotonically increasing unless the table is + declared WITHOUT ROWID, which this one isn't), so it's the only + honest proxy available for "most recently created" when started_at + can't answer that.""" + rows = conn.execute( + "SELECT rowid, run_id, task_id, started_at FROM work_graph_run " + "WHERE status NOT IN ('done', 'failed')" + ).fetchall() + by_task = {} + for rowid, run_id, task_id, started_at in rows: + by_task.setdefault(task_id, []).append((started_at or "", rowid, run_id)) + + to_block = [] + for task_id, entries in by_task.items(): + if len(entries) <= 1: + continue + entries.sort() + to_block.extend(run_id for _, _, run_id in entries[:-1]) + + for run_id in to_block: + conn.execute("UPDATE work_graph_run SET status = 'failed' WHERE run_id = ?", (run_id,)) diff --git a/src/test_schema.py b/src/test_schema.py index e94cafb..e46b28c 100644 --- a/src/test_schema.py +++ b/src/test_schema.py @@ -915,6 +915,90 @@ class MigrateDropWorkGraphRunOrgNodeTest(unittest.TestCase): finally: fresh.close() + def test_duplicate_non_terminal_rows_per_task_reconciled_before_index(self): + # Real 2026-08-13 incident: pre-Phase-2c databases can have several + # non-terminal work_graph_run rows for the same task_id (one per + # org_node, before runs were merged). The narrowed unique index + # can't be created until only one non-terminal row per task_id + # remains -- this must not raise. + self.conn.execute( + "INSERT INTO work_graph_run (run_id, task_id, org_node, graph_manifest, status, started_at) " + "VALUES ('run-2', 'task-1', 'coder', '{}', 'running', '2026-08-01T00:00:00')" + ) + self.conn.execute( + "INSERT INTO work_graph_run (run_id, task_id, org_node, graph_manifest, status, started_at) " + "VALUES ('run-3', 'task-1', 'validator', '{}', 'planning', '2026-08-02T00:00:00')" + ) + self.conn.commit() + + schema.migrate_drop_work_graph_run_org_node(self.conn) + + rows = self.conn.execute( + "SELECT run_id, status FROM work_graph_run WHERE task_id = 'task-1' AND run_id != 'run-1'" + ).fetchall() + statuses = dict(rows) + # run-3 started latest among the non-terminal duplicates -> stays active. + self.assertEqual(statuses["run-3"], "planning") + self.assertEqual(statuses["run-2"], "failed") + # the narrowed index must have actually been created and be enforced. + with self.assertRaises(sqlite3.IntegrityError): + self.conn.execute( + "INSERT INTO work_graph_run (run_id, task_id, graph_manifest, status) " + "VALUES ('run-4', 'task-1', '{}', 'running')" + ) + + def test_duplicate_reconciliation_uses_insertion_order_not_uuid_when_started_at_null(self): + # Real 2026-08-13 incident: every legacy duplicate row had + # started_at = NULL. A run_id-string tiebreak would pick among + # them by UUID value, unrelated to which one was actually created + # last -- e.g. 'aaaa...' sorts after 'zzzz...' alphabetically but + # was inserted first. rowid (SQLite's true insertion-order + # counter) must be used instead, so the row inserted LAST (run-2, + # despite its run_id sorting before run-3's alphabetically) is the + # one kept active. + self.conn.execute( + "INSERT INTO work_graph_run (run_id, task_id, org_node, graph_manifest, status) " + "VALUES ('aaa-run-3', 'task-1', 'validator', '{}', 'planning')" + ) + self.conn.execute( + "INSERT INTO work_graph_run (run_id, task_id, org_node, graph_manifest, status) " + "VALUES ('zzz-run-2', 'task-1', 'coder', '{}', 'running')" + ) + self.conn.commit() + + schema.migrate_drop_work_graph_run_org_node(self.conn) + + statuses = dict( + self.conn.execute( + "SELECT run_id, status FROM work_graph_run WHERE run_id IN ('aaa-run-3', 'zzz-run-2')" + ).fetchall() + ) + self.assertEqual(statuses["aaa-run-3"], "failed") + self.assertEqual(statuses["zzz-run-2"], "running") + + def test_reentrant_after_partial_migration_missing_index_only(self): + # Real 2026-08-13 incident: executescript() commits each DDL + # statement individually, so a process crash between ALTER TABLE + # ... RENAME and CREATE UNIQUE INDEX leaves a database with the + # org_node column already gone but ux_work_graph_run_active never + # created. Re-running the migration against that exact state must + # finish the job, not silently no-op because "org_node" is already + # absent. + schema.migrate_drop_work_graph_run_org_node(self.conn) + self.conn.execute("DROP INDEX ux_work_graph_run_active") + self.conn.commit() + has_index = self.conn.execute( + "SELECT 1 FROM sqlite_master WHERE type = 'index' AND name = 'ux_work_graph_run_active'" + ).fetchone() + self.assertIsNone(has_index) # sanity: simulated partial state matches the real incident + + schema.migrate_drop_work_graph_run_org_node(self.conn) + + has_index = self.conn.execute( + "SELECT 1 FROM sqlite_master WHERE type = 'index' AND name = 'ux_work_graph_run_active'" + ).fetchone() + self.assertIsNotNone(has_index) + class MigrateAddWorkGraphLoopLimitConfigTest(unittest.TestCase): """2026-08-08: real upgrade path for an already-populated database that -- libgit2 0.21.4