Repository navigation
Fix orphaned child rows and keep FK enforcement on after migrations (#107) - #108
Conversation
…107) The v19 migration re-enabled PRAGMA foreign_keys inside an open transaction, where SQLite ignores it, so the writer connection ran with cascades disabled for the rest of the process. Combined with delete paths that relied on the cascade, child rows could outlive their parents. - schema: commit before toggling foreign_keys in v19; re-enable after the final commit; v22 migration deletes orphaned rows in every child table - indexer: one explicit delete helper for the empty-session and stale paths (covers shells/cron too); session row uses ON CONFLICT DO UPDATE so re-indexing no longer cascade-deletes children; health check reports orphan counts - tests: upgrade paths keep FK on, delete paths work with FK off, repair migration, health metric, re-index preserves child ids v20/v21 are reserved for PR #106; this must land after it.
JayantDevkar
left a comment
JayantDevkar
left a comment
There was a problem hiding this comment.
Inline context for reviewers, one note per change area.
| } | ||
| if "background_shells" in existing: | ||
| # PRAGMA foreign_keys is a no-op inside a transaction, so commit first. | ||
| conn.commit() |
There was a problem hiding this comment.
PRAGMA foreign_keys is ignored while a transaction is open. On main, a DB upgraded from v10 reaches here with v10's DELETEs uncommitted, so the OFF never applied and the DROP TABLE below cascade-wiped shell_polls; covered by test_upgrade_from_v10_keeps_shell_polls_and_foreign_keys_on.
|
|
||
| SCHEMA_VERSION = 19 | ||
| # v20/v21 are taken by PR #106 (session analytics accuracy); v22 is the orphan repair. | ||
| SCHEMA_VERSION = 22 |
| ?, ?, ?, ?, | ||
| datetime('now') | ||
| ) | ||
| ON CONFLICT(uuid) DO UPDATE SET {_SESSION_UPSERT_SET} |
There was a problem hiding this comment.
INSERT OR REPLACE deletes the row before inserting, and with FK enforcement back on that cascades to cron_jobs and cron_state_snapshots and re-ids them on every re-index. needs_shell_cron_reindex is left out of the SET list so the flag survives a re-index.
| def _count_orphan_rows(conn: sqlite3.Connection) -> int: | ||
| """FK violations, plus orphaned shells on DBs where the v19 rebuild dropped that FK.""" | ||
| count = conn.execute("SELECT COUNT(*) FROM pragma_foreign_key_check").fetchone()[0] | ||
| if not conn.execute("PRAGMA foreign_key_list(background_shells)").fetchall(): |
There was a problem hiding this comment.
The v19 rebuild defined background_shells without its FK to sessions, so pragma_foreign_key_check cannot see orphaned shells on any upgraded DB (1,266 of 1,940 rows locally). Fresh DBs do have the FK, hence the guard to avoid counting them twice.
| "cron_jobs", | ||
| "cron_state_snapshots", | ||
| ) | ||
| _DELETE_CHUNK = 500 |
There was a problem hiding this comment.
500 keeps each statement under the 999 bound-parameter limit of older SQLite builds; the stale path can hand over thousands of uuids at once.
1 participant
Summary
Fixes #107. 14,172 of 18,688
subagent_toolsrows on a local DB pointed at invocations that no longer exist, and 1,266 of 1,940background_shellsrows pointed at missing sessions. Two defects allowed it: the v19 migration re-enabledPRAGMA foreign_keysinside an open transaction, where SQLite ignores it, so the writer connection ran with cascades off for the rest of the process; and the empty-session delete path relied on that cascade. Counts on the dashboard were unaffected because every aggregate query joinssubagent_invocations.Changes
schema.py: commit before togglingforeign_keysin v19, so the OFF applies and the ON applies. Also fixes a second case found by the review, not caused by this PR: on DBs upgraded from v10 or lower an earlier migration's open transaction made the OFF a no-op and the v19 table drop cascade-wipedshell_polls.schema.py:ensure_schemare-enablesforeign_keysafter its final commit, and a v22 migration deletes orphaned rows from every child table.indexer.py: one_delete_session_rowshelper for the empty-session and stale paths, covering shells, polls, cron jobs and snapshots. The sessions write is nowON CONFLICT DO UPDATE; with FK back on,INSERT OR REPLACEwould cascade-delete and re-id child rows on every re-index.indexer.py: the health metrics include an orphan count, with an explicit shells check for upgraded DBs where the v19 rebuild dropped that FK.Component
Type
Testing
cd api && pytest): 1863 passed, 6 skipped, run with HOME isolated so the real DB was not touchedtests/test_referential_integrity.py; each failed before its production changeVACUUM INTOcopy of the real DB:Related Issues
Fixes #107. Depends on #106: main is at v19 and #106 adds v20/v21, so this is numbered v22 and must merge after #106. A DB stamped 22 before #106 lands would never run its v21 column add.