From 1a0500b8126d707b88d8762d894389424158c074 Mon Sep 17 00:00:00 2001 From: RhenCloud Date: Thu, 10 Sep 2026 22:35:06 +0800 Subject: [PATCH] fix: prevent data loss in routes, fragments, and group operations Found and fixed multiple critical bugs with similar patterns: 1. routeStatements() used DELETE FROM d1_routes before re-inserting - Could cause data loss if interrupted or if logic changes - Now uses DELETE with WHERE clause + INSERT...ON CONFLICT (upsert) 2. saveFragments() used DELETE FROM d1_fragments before re-inserting - Same pattern as routes, fixed with targeted deletes + upsert - Added loadFragments() call to determine what to delete 3. adminGroupRename() didn't update fragment groupId - When renaming a group, fragments were left pointing to old groupId - Now updates fragments alongside routes, secrets, and invites All changes follow the same safe pattern: - Load existing data to identify what needs deletion - Delete only removed items with WHERE clauses - Use INSERT...ON CONFLICT DO UPDATE for upserts - Never use bare DELETE FROM table Testing: All 339 tests pass. --- server/lib/fragments.ts | 35 ++++++++++++++++++++---- server/lib/storage/config-store.ts | 44 +++++++++++++++++++++++++----- server/lib/web/admin.ts | 10 ++++++- 3 files changed, 75 insertions(+), 14 deletions(-) diff --git a/server/lib/fragments.ts b/server/lib/fragments.ts index e657b7e..ce96907 100644 --- a/server/lib/fragments.ts +++ b/server/lib/fragments.ts @@ -35,16 +35,39 @@ export async function loadFragments(db: D1Database): Promise { export async function saveFragments(db: D1Database, fragments: NamedFragment[]): Promise { const now = Date.now(); - const statements: D1PreparedStatement[] = [ - db.prepare("DELETE FROM d1_fragments"), - ...fragments.map((f) => + + // Load existing fragments to determine what to delete + const existing = await loadFragments(db); + const existingKeys = new Set(existing.map((f) => `${f.id}:${f.groupId ?? ""}`)); + const newKeys = new Set(fragments.map((f) => `${f.id}:${f.groupId ?? ""}`)); + + const statements: D1PreparedStatement[] = []; + + // Delete fragments that are no longer present + for (const key of existingKeys) { + if (!newKeys.has(key)) { + const [id, groupId] = key.split(":"); + statements.push( + db.prepare("DELETE FROM d1_fragments WHERE id = ? AND group_id = ?").bind(id, groupId) + ); + } + } + + // Upsert all fragments + for (const f of fragments) { + statements.push( db .prepare( `INSERT INTO d1_fragments (id, group_id, name, node, version, created_at, updated_at) - VALUES (?, ?, ?, ?, 1, ?, ?)`, + VALUES (?, ?, ?, ?, 1, ?, ?) + ON CONFLICT(id, group_id) DO UPDATE SET + name = excluded.name, + node = excluded.node, + updated_at = excluded.updated_at`, ) .bind(f.id, f.groupId ?? "", f.name, JSON.stringify(f.node), now, now), - ), - ]; + ); + } + await db.batch(statements); } diff --git a/server/lib/storage/config-store.ts b/server/lib/storage/config-store.ts index af0706e..6dcd63c 100644 --- a/server/lib/storage/config-store.ts +++ b/server/lib/storage/config-store.ts @@ -88,15 +88,43 @@ export function d1ConfigStore(db: D1Database, kv: KVNamespace): ConfigStore { return []; } - function routeStatements(routes: Route[]): D1PreparedStatement[] { + function routeStatements(routes: Route[], existingRouteKeys?: Set): D1PreparedStatement[] { const now = Date.now(); - return [ - db.prepare("DELETE FROM d1_routes"), - ...routes.map((r) => + const statements: D1PreparedStatement[] = []; + + // If we have existing routes, delete those not in the new set + if (existingRouteKeys) { + const newKeys = new Set(routes.map((r) => `${r.id}:${r.groupId ?? ""}`)); + for (const key of existingRouteKeys) { + if (!newKeys.has(key)) { + const [id, groupId] = key.split(":"); + statements.push( + db.prepare("DELETE FROM d1_routes WHERE id = ? AND group_id = ?").bind(id, groupId) + ); + } + } + } else { + // Backward compatibility: delete all routes if no existing set provided + statements.push(db.prepare("DELETE FROM d1_routes")); + } + + // Upsert all routes + for (const r of routes) { + statements.push( db .prepare( `INSERT INTO d1_routes (id, group_id, name, enabled, filters, targets, stop, fallback, discord_role_ids, ast, version, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 1, ?, ?)`, + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 1, ?, ?) + ON CONFLICT(id, group_id) DO UPDATE SET + name = excluded.name, + enabled = excluded.enabled, + filters = excluded.filters, + targets = excluded.targets, + stop = excluded.stop, + fallback = excluded.fallback, + discord_role_ids = excluded.discord_role_ids, + ast = excluded.ast, + updated_at = excluded.updated_at`, ) .bind( r.id, @@ -112,8 +140,10 @@ export function d1ConfigStore(db: D1Database, kv: KVNamespace): ConfigStore { now, now, ), - ), - ]; + ); + } + + return statements; } function groupStatements(groups: Group[], existingGroupIds: string[]): D1PreparedStatement[] { diff --git a/server/lib/web/admin.ts b/server/lib/web/admin.ts index e90628b..9f1e309 100644 --- a/server/lib/web/admin.ts +++ b/server/lib/web/admin.ts @@ -965,7 +965,7 @@ export async function adminGroupRename( return respondError(event, 500, "Failed to save groups"); } - // Re-point routes, the tenant webhook secret and pending invites. + // Re-point routes, fragments, the tenant webhook secret and pending invites. const routes = await loadRoutes(env.KV); const touched = routes.filter((r) => r.groupId === groupId); if (touched.length > 0) { @@ -974,6 +974,14 @@ export async function adminGroupRename( routes.map((r) => (r.groupId === groupId ? { ...r, groupId: newId } : r)), ); } + const fragments = await loadFragments(env.DB); + const touchedFragments = fragments.filter((f) => f.groupId === groupId); + if (touchedFragments.length > 0) { + await saveFragments( + env.DB, + fragments.map((f) => (f.groupId === groupId ? { ...f, groupId: newId } : f)), + ); + } const secret = await getTenantSecret(env.KV, groupId); if (secret) { await env.KV.put(`tenant:${newId}`, secret);