Raj Nandan Sharma
122ca71b8e
Review follow-up on #754: deleteMonitorAlertConfigsByMonitorTag removed v2 alert rows only when the whole config died, so a shared config that survived the detach kept monitor_alerts_v2 rows pointing at the deleted monitor's tag. Verified red/green with an in-memory SQLite script: the deleted tag's v2 rows now go with the detach while the surviving monitors' rows are untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2.3 KiB
Delete paths remove child rows explicitly and never rely on FK cascades
The schema declares ON DELETE CASCADE foreign keys (alert configs → monitors, trigger/monitor junctions → configs, v2 alerts → configs), but SQLite — the default deployment — never enforces them: knex does not enable the foreign_keys pragma, and knexfile.ts does not either. Code that trusted the declared cascades (deleteMonitorAlertConfig, and DeleteMonitorCompletelyUsingTag, which skipped alert configs entirely) silently orphaned junction rows, configs, and monitor_alerts_v2 state on SQLite, while behaving correctly on Postgres and MySQL (#716).
We considered fixing the root cause instead: enabling PRAGMA foreign_keys = ON per connection via pool.afterCreate. That would make every declared cascade real and align the three databases. We rejected it because knex implements SQLite ALTER TABLE as a table rebuild, which violates FK constraints transiently — the multi-monitor-alerts migration already has to toggle the pragma off around its own rebuild — so enforcement would put every past and future migration on a tightrope, and it changes write-failure behavior for all existing SQLite installs at once (inserts that used to succeed against missing parents would start throwing).
The rule is the opposite and uniform: a delete path owns its children and removes them explicitly, in child-before-parent order. deleteMonitorAlertConfig deletes v2 alerts, trigger junctions, and monitor junctions before the config row; deleteMonitorAlertConfigsByMonitorTag detaches the monitor — deleting its per-monitor monitor_alerts_v2 rows, which outlive the detach when the config is shared — and routes zero-monitor configs through that same method; DeleteMonitorCompletelyUsingTag calls it alongside its other per-tag cleanups. The declared cascades stay in the schema — on Postgres/MySQL they make the explicit deletes idempotent no-ops, and they document intent — but no code may depend on them.
A consequence: shared alert configs (one config spanning several monitors) survive the deletion of one member; a config dies only when its last monitor is detached. Monitor deletion also strips the tag from group monitors and rebalances remaining member weights equally — deletion never produces a stale member (see CONTEXT.md, "Stale Member").