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.
This commit is contained in:
Juan Castro
2026-09-03 14:33:46 -04:00
parent c1c1588939
commit dc7b456afa
5 changed files with 47 additions and 36 deletions
@@ -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(
@@ -15,19 +15,10 @@
-- You should have received a copy of the GNU Affero General Public License
-- along with this program. If not, see <https://www.gnu.org/licenses/>.
-- 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;
@@ -15,14 +15,20 @@
-- You should have received a copy of the GNU Affero General Public License
-- along with this program. If not, see <https://www.gnu.org/licenses/>.
-- 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);
@@ -15,17 +15,11 @@
-- You should have received a copy of the GNU Affero General Public License
-- along with this program. If not, see <https://www.gnu.org/licenses/>.
-- `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 (
+2 -5
View File
@@ -40,11 +40,8 @@ export class GroupStore extends PuterStore {
async addUsers(uid: string, usernames: string[]): Promise<void> {
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`) ' +