From dc7b456afada70c232c05559bb069c5a0fc2184a Mon Sep 17 00:00:00 2001 From: Juan Castro Date: Tue, 1 Sep 2026 13:42:16 -0400 Subject: [PATCH] fix: run the membership dedup once instead of on every boot Review follow-ups on the dedup migration. mysql and postgres track no per-file applied state and re-execute every migration on each boot, so the unguarded DELETE self-joined the whole table at every process start, forever. Both now sit behind the same index-existence check that guards the ALTER, which also stops a rolling deploy deleting on one instance while another adds the index. The dedup test was a false green. `targetVersion: 67` never applies 0071 -- the loop breaks on `threshold + 1 >= targetVersion` but stamps the target anyway -- so the fixture asserted the current schema version on a database missing a migration. It now replays the real 0072 file against a fully migrated database, and a second test pins the off-by-one so nobody builds a fixture on it again. --- .../database/SqliteDatabaseClient.test.ts | 18 ++++++++++++++- .../migrations/mysql/mysql_mig_33.sql | 22 +++++++++---------- .../migrations/postgres/postgres_mig_22.sql | 20 +++++++++++------ .../0078_jct-user-group-pair-unique.sql | 16 +++++--------- src/backend/stores/group/GroupStore.ts | 7 ++---- 5 files changed, 47 insertions(+), 36 deletions(-) diff --git a/src/backend/clients/database/SqliteDatabaseClient.test.ts b/src/backend/clients/database/SqliteDatabaseClient.test.ts index afac01a81..715b3af8b 100644 --- a/src/backend/clients/database/SqliteDatabaseClient.test.ts +++ b/src/backend/clients/database/SqliteDatabaseClient.test.ts @@ -81,7 +81,23 @@ describe('SqliteDatabaseClient — boot and migrations', { timeout: DISK_MIGRATI expect(await userVersionOf(client)).toBe(CURRENT_SCHEMA_VERSION); }); - it('applies the team columns and indexes from 0077', async () => { + it('stamps a version whose migration did not run (known off-by-one)', async () => { + // Existing behaviour, not an endorsement: the stamp overshoots the + // applied work by one entry, so 70 is reported without 0074 running. + const partial = await bootClient({ targetVersion: 70 }); + try { + expect(await userVersionOf(partial)).toBe(70); + await expect( + partial.read( + "SELECT 1 FROM pragma_table_info('group') WHERE name = 'handle'", + ), + ).resolves.toEqual([]); + } finally { + partial.onServerShutdown(); + } + }); + + it('applies the team columns and indexes from 0076', async () => { const columnsOf = async (table: string) => ( (await client.read( diff --git a/src/backend/clients/database/migrations/mysql/mysql_mig_33.sql b/src/backend/clients/database/migrations/mysql/mysql_mig_33.sql index caa92900f..cf4e644a0 100644 --- a/src/backend/clients/database/migrations/mysql/mysql_mig_33.sql +++ b/src/backend/clients/database/migrations/mysql/mysql_mig_33.sql @@ -15,19 +15,10 @@ -- You should have received a copy of the GNU Affero General Public License -- along with this program. If not, see . --- Deduplicate then constrain. See sqlite/0072 for why duplicates exist and why --- dropping the higher-id row is lossless. +-- Deduplicate then constrain. See sqlite/0075 for why this is lossless. --- Self-join rather than `id NOT IN (SELECT MIN(id) ... FROM jct_user_group)`, --- which mysql refuses with error 1093 -- it will not read the table it deletes --- from in a subquery. Keeps the lowest id of each pair. -DELETE dup FROM `jct_user_group` dup -JOIN `jct_user_group` keep - ON dup.`user_id` = keep.`user_id` - AND dup.`group_id` = keep.`group_id` - AND dup.`id` > keep.`id`; - --- Guarded because mysql tracks no applied-state per file, so this may re-run. +-- Both steps sit behind the index check: mysql re-runs this file on every boot, and +-- guarding them together stops a rolling deploy deleting and indexing concurrently. DROP PROCEDURE IF EXISTS _puter_add_membership_pair_index; DELIMITER // @@ -39,6 +30,13 @@ BEGIN AND TABLE_NAME = 'jct_user_group' AND INDEX_NAME = 'idx_jct_user_group_pair' ) THEN + -- Self-join because mysql refuses `NOT IN (SELECT ... FROM jct_user_group)` (1093). + DELETE dup FROM `jct_user_group` dup + JOIN `jct_user_group` keep + ON dup.`user_id` = keep.`user_id` + AND dup.`group_id` = keep.`group_id` + AND dup.`id` > keep.`id`; + ALTER TABLE `jct_user_group` ADD UNIQUE INDEX `idx_jct_user_group_pair` (`user_id`, `group_id`); END IF; diff --git a/src/backend/clients/database/migrations/postgres/postgres_mig_22.sql b/src/backend/clients/database/migrations/postgres/postgres_mig_22.sql index 58a9a51a9..e15c6e9f2 100644 --- a/src/backend/clients/database/migrations/postgres/postgres_mig_22.sql +++ b/src/backend/clients/database/migrations/postgres/postgres_mig_22.sql @@ -15,14 +15,20 @@ -- You should have received a copy of the GNU Affero General Public License -- along with this program. If not, see . --- Deduplicate then constrain. See sqlite/0072 for why duplicates exist and why --- dropping the higher-id row is lossless. --- Idempotent: the delete is a no-op once unique, and the index guards itself. +-- Deduplicate then constrain. See sqlite/0075 for why this is lossless. -DELETE FROM jct_user_group -WHERE id NOT IN ( - SELECT MIN(id) FROM jct_user_group GROUP BY user_id, group_id -); +-- Guarded because postgres re-runs every file on each boot; `to_regclass` follows +-- search_path, so it resolves under a test schema too. +DO $mig15$ +BEGIN + IF to_regclass('idx_jct_user_group_pair') IS NULL THEN + DELETE FROM jct_user_group + WHERE id NOT IN ( + SELECT MIN(id) FROM jct_user_group GROUP BY user_id, group_id + ); + END IF; +END +$mig15$; CREATE UNIQUE INDEX IF NOT EXISTS idx_jct_user_group_pair ON jct_user_group (user_id, group_id); diff --git a/src/backend/clients/database/migrations/sqlite/0078_jct-user-group-pair-unique.sql b/src/backend/clients/database/migrations/sqlite/0078_jct-user-group-pair-unique.sql index baf09da1f..3d25db275 100644 --- a/src/backend/clients/database/migrations/sqlite/0078_jct-user-group-pair-unique.sql +++ b/src/backend/clients/database/migrations/sqlite/0078_jct-user-group-pair-unique.sql @@ -15,17 +15,11 @@ -- You should have received a copy of the GNU Affero General Public License -- along with this program. If not, see . --- `GroupStore.addUsers` is an INSERT ... SELECT with no conflict clause, so a --- repeated call inserts a second row for the same (user_id, group_id). --- --- Each duplicate multiplies every row `readUserGroupPerms` returns, because it --- joins this table on group_id alone -- two membership rows means every group --- permission is reported twice. The index below stops that recurring; the --- delete clears what already accumulated. --- --- Dropping the higher-id row discards nothing: `addUsers` writes only the two --- id columns, so `extra` and `metadata` are NULL on every row here, nothing has --- a foreign key to `jct_user_group.id`, and no code reads it. +-- `GroupStore.addUsers` had no conflict clause, so a repeat call duplicated the pair. +-- `readUserGroupPerms` joins this table on group_id alone, so each duplicate reported +-- every group permission an extra time. The delete clears what accumulated. +-- Dropping the higher id is lossless: `addUsers` writes only the two id columns, and +-- nothing references `jct_user_group.id`. DELETE FROM `jct_user_group` WHERE `id` NOT IN ( diff --git a/src/backend/stores/group/GroupStore.ts b/src/backend/stores/group/GroupStore.ts index 5a3d1e36e..5b8c9426c 100644 --- a/src/backend/stores/group/GroupStore.ts +++ b/src/backend/stores/group/GroupStore.ts @@ -40,11 +40,8 @@ export class GroupStore extends PuterStore { async addUsers(uid: string, usernames: string[]): Promise { if (usernames.length === 0) return; const placeholders = `(${usernames.map(() => '?').join(', ')})`; - // Ignore conflicts on the (user_id, group_id) unique index added in - // 0072. Re-adding a member was previously a silent duplicate row, which - // is what that index exists to stop -- without this it becomes a raised - // error instead, and every caller here treats a throw as a failed - // signup step worth warning about. + // Ignore conflicts on the unique pair index from 0072; re-adding a member + // was a duplicate row before it, and would raise without this. await this.clients.db.write( `${this.clients.db.insertIgnoreInto('jct_user_group')} ` + '(`user_id`, `group_id`) ' +