diff --git a/crates/buzz-admin/tests/postgres_user_inventory.rs b/crates/buzz-admin/tests/postgres_user_inventory.rs index 2c53cb053db..d697f12d693 100644 --- a/crates/buzz-admin/tests/postgres_user_inventory.rs +++ b/crates/buzz-admin/tests/postgres_user_inventory.rs @@ -71,7 +71,7 @@ async fn cluster_global_inventory_cli_uses_only_existing_membership_tables() { for target in [community, participant_community] { for _ in 0..2 { let channel = Uuid::new_v4(); - sqlx::query("INSERT INTO channels (community_id, id, name, created_by) VALUES ($1, $2, 'inventory-test', decode($3, 'hex'))") + sqlx::query("INSERT INTO channels (community_id, id, name, created_by) VALUES ($1, $2, 'inventory-test-' || $2::uuid::text, decode($3, 'hex'))") .bind(target).bind(channel).bind(pubkey).execute(&mut *tx).await.unwrap(); sqlx::query("INSERT INTO channel_members (community_id, channel_id, pubkey) VALUES ($1, $2, decode($3, 'hex'))") .bind(target).bind(channel).bind(pubkey).execute(&mut *tx).await.unwrap(); diff --git a/crates/buzz-db/src/error.rs b/crates/buzz-db/src/error.rs index 7e795dbcc7e..8cf397d1a7c 100644 --- a/crates/buzz-db/src/error.rs +++ b/crates/buzz-db/src/error.rs @@ -26,7 +26,7 @@ pub enum DbError { ChannelNotFound(uuid::Uuid), /// Another listed open channel in the community already has this name - /// (see `channel::ensure_listed_channel_name_unique`). + /// (the `idx_channels_listed_name` index, migration 0083). #[error("an open channel named \"{0}\" already exists")] ChannelNameTaken(String), diff --git a/crates/buzz-db/src/runtime/migration.rs b/crates/buzz-db/src/runtime/migration.rs index 5bd47588b6f..924140b1f5d 100644 --- a/crates/buzz-db/src/runtime/migration.rs +++ b/crates/buzz-db/src/runtime/migration.rs @@ -778,7 +778,7 @@ mod postgres_tests { let mut migrations: Vec<_> = MIGRATOR.iter().collect(); migrations.sort_by_key(|migration| migration.version); - assert_eq!(migrations.len(), 82); + assert_eq!(migrations.len(), 83); for (index, migration) in migrations.iter().enumerate() { assert_eq!(migration.version, (index + 1) as i64); } @@ -786,6 +786,18 @@ mod postgres_tests { .sql .as_str() .contains("ADD COLUMN auto_join BOOLEAN NOT NULL DEFAULT false")); + // Listed channel names (0083): a bounded contract index that refuses + // to build over unrepaired duplicates. The desired schema carries the + // same plain index. + assert_eq!(migrations[82].version, 83); + let listed_names = strip_sql_comments(migrations[82].sql.as_str()); + assert!(listed_names.contains("SET LOCAL lock_timeout = '1s';")); + assert!(listed_names.contains("SET LOCAL statement_timeout = '5s';")); + assert!(listed_names.contains("CREATE UNIQUE INDEX idx_channels_listed_name")); + assert!(!listed_names.contains("CONCURRENTLY")); + assert!(crate::test_support::desired_state_schema_sql().contains( + "CREATE UNIQUE INDEX idx_channels_listed_name ON channels (community_id, lower(name))" + )); assert!(migrations[70] .sql .as_str() @@ -2924,6 +2936,21 @@ mod postgres_tests { "column parity mismatch for channels: schema.sql desired state has drifted \ from the migrations; update schema/tables/public/channels.sql to match" ); + // The listed-name index (0083) is compared on its own for that reason. + let listed_name = |shapes: Vec<(String, String, String)>| { + shapes + .into_iter() + .filter(|(name, _, _)| name == "idx_channels_listed_name") + .collect::>() + }; + let desired_listed_name = listed_name(index_shapes(&desired, "channels").await); + assert_eq!(desired_listed_name.len(), 1, "desired state has the index"); + assert_eq!( + desired_listed_name, + listed_name(index_shapes(&migrated, "channels").await), + "idx_channels_listed_name parity mismatch: update \ + schema/tables/public/channels.sql to match migration 0083" + ); assert_eq!( events_key_defs(&desired).await, events_key_defs(&migrated).await, diff --git a/crates/buzz-db/src/runtime/tests/listed_channel_name_postgres_tests.rs b/crates/buzz-db/src/runtime/tests/listed_channel_name_postgres_tests.rs index cfc3a7a0b63..8ebd4a49680 100644 --- a/crates/buzz-db/src/runtime/tests/listed_channel_name_postgres_tests.rs +++ b/crates/buzz-db/src/runtime/tests/listed_channel_name_postgres_tests.rs @@ -2,9 +2,18 @@ //! renames that leave no two listed channels in a community with one name, //! ignoring case. It keeps the name on the oldest unarchived channel and picks //! a new name that no other listed channel has. +//! +//! Migration 0083 then adds the unique index. It refuses to run over +//! duplicates that were not renamed. Otherwise it builds the index itself on a +//! populated table under its lock budget, accepts a valid concurrent prebuild +//! without replacing it, and rejects an invalid one. use super::*; +/// The last migration before 0083. Duplicates can exist, and the plan has +/// work to do, only before the index does. +const BEFORE_INDEX: i64 = 82; + const PLAN_SQL: &str = include_str!("../../../../../scripts/maintenance/plan_duplicate_listed_channel_renames.sql"); @@ -64,7 +73,7 @@ fn renamed(id: Uuid, name: &str) -> (Uuid, String) { /// The plan for `host`: (channel, current name, new name), as the rename /// script reads it. -pub(super) async fn rename_plan(pool: &PgPool, host: &str) -> Vec<(Uuid, String, String)> { +async fn rename_plan(pool: &PgPool, host: &str) -> Vec<(Uuid, String, String)> { rename_plan_with_starters(pool, host, &[]).await } @@ -92,7 +101,7 @@ async fn rename_plan_with_starters( /// Applies the plan the way the relay applies each rename the script sends, /// and returns how many channels it renamed. -pub(super) async fn rename_duplicates(pool: &PgPool, host: &str) -> usize { +async fn rename_duplicates(pool: &PgPool, host: &str) -> usize { let plan = rename_plan(pool, host).await; for (channel, _, new_name) in &plan { sqlx::query("UPDATE channels SET name = $2 WHERE id = $1") @@ -108,7 +117,8 @@ pub(super) async fn rename_duplicates(pool: &PgPool, host: &str) -> usize { #[tokio::test] #[ignore = "requires Postgres"] async fn plan_keeps_each_listed_name_on_its_oldest_unarchived_channel() { - let (pool, _scratch) = ScratchDb::migrated(&admin_url().await, "listed_name_plan").await; + let (pool, _scratch) = + ScratchDb::migrated_through(&admin_url().await, "listed_name_plan", BEFORE_INDEX).await; let seed = Seed::new(&pool).await; let [kept, archived, newer, private, deleted] = [(); 5].map(|_| Uuid::new_v4()); seed.open(&pool, kept, "square-ios", 30).await; @@ -187,7 +197,8 @@ async fn plan_keeps_each_listed_name_on_its_oldest_unarchived_channel() { #[ignore = "requires Postgres"] async fn plan_frees_a_starter_name_that_only_archived_channels_hold() { let (pool, _scratch) = - ScratchDb::migrated(&admin_url().await, "listed_name_plan_starter").await; + ScratchDb::migrated_through(&admin_url().await, "listed_name_plan_starter", BEFORE_INDEX) + .await; let seed = Seed::new(&pool).await; let [general, welcome_old, welcome_new, random] = [(); 4].map(|_| Uuid::new_v4()); seed.open(&pool, general, "General", 30).await; @@ -299,7 +310,9 @@ async fn plan_frees_a_starter_name_that_only_archived_channels_hold() { #[tokio::test] #[ignore = "requires Postgres"] async fn plan_picks_a_free_name_when_the_short_one_is_taken() { - let (pool, _scratch) = ScratchDb::migrated(&admin_url().await, "listed_name_plan_taken").await; + let (pool, _scratch) = + ScratchDb::migrated_through(&admin_url().await, "listed_name_plan_taken", BEFORE_INDEX) + .await; let seed = Seed::new(&pool).await; // The longest name the column holds, so the new name must truncate it. let long = "a".repeat(255); @@ -381,7 +394,8 @@ async fn run_rename_script( #[tokio::test] #[ignore = "requires Postgres"] async fn script_renames_each_planned_channel_once_whatever_its_name() { - let (pool, scratch) = ScratchDb::migrated(&admin_url().await, "listed_name_script").await; + let (pool, scratch) = + ScratchDb::migrated_through(&admin_url().await, "listed_name_script", BEFORE_INDEX).await; let seed = Seed::new(&pool).await; let [kept, duplicate, other] = [(); 3].map(|_| Uuid::new_v4()); let tricky = format!("-ops\x1e{other}\x1fowned\x1f\n\t\"$(x)"); @@ -428,7 +442,9 @@ async fn script_renames_each_planned_channel_once_whatever_its_name() { #[tokio::test] #[ignore = "requires Postgres"] async fn script_stops_at_the_first_auth_refusal() { - let (pool, scratch) = ScratchDb::migrated(&admin_url().await, "listed_name_script_auth").await; + let (pool, scratch) = + ScratchDb::migrated_through(&admin_url().await, "listed_name_script_auth", BEFORE_INDEX) + .await; let seed = Seed::new(&pool).await; for (group, age) in [("first", 30), ("first", 10), ("second", 30), ("second", 10)] { seed.open(&pool, Uuid::new_v4(), group, age).await; @@ -449,7 +465,9 @@ async fn script_stops_at_the_first_auth_refusal() { #[tokio::test] #[ignore = "requires Postgres"] async fn script_stops_for_a_community_the_relay_does_not_serve() { - let (pool, scratch) = ScratchDb::migrated(&admin_url().await, "listed_name_script_state").await; + let (pool, scratch) = + ScratchDb::migrated_through(&admin_url().await, "listed_name_script_state", BEFORE_INDEX) + .await; let archived = Seed::new(&pool).await; let deleting = Seed::new(&pool).await; for seed in [&archived, &deleting] { @@ -479,3 +497,142 @@ async fn script_stops_for_a_community_the_relay_does_not_serve() { assert!(stderr.contains(message), "{host}: {stderr}"); } } + +async fn ledger_version(pool: &PgPool) -> i64 { + sqlx::query_scalar("SELECT max(version) FROM _sqlx_migrations") + .fetch_one(pool) + .await + .expect("read migration ledger") +} + +async fn index_oid(pool: &PgPool) -> Option { + sqlx::query_scalar("SELECT to_regclass('public.idx_channels_listed_name')::oid::bigint") + .fetch_one(pool) + .await + .expect("look up index") +} + +#[tokio::test] +#[ignore = "requires Postgres"] +async fn migration_schema_listed_name_index_needs_the_renames_then_builds_inline() { + let (pool, _scratch) = + ScratchDb::migrated_through(&admin_url().await, "listed_name_index", BEFORE_INDEX).await; + let seed = Seed::new(&pool).await; + let (kept, newer) = (Uuid::new_v4(), Uuid::new_v4()); + seed.open(&pool, kept, "square-ios", 30).await; + seed.open(&pool, newer, "Square-iOS", 10).await; + let before = seed.names(&pool).await; + + let error = migration::run_migrations(&pool).await.unwrap_err(); + assert!( + error.to_string().contains("duplicate names remain"), + "must name the duplicate rows, not merely fail: {error}" + ); + assert_eq!(ledger_version(&pool).await, BEFORE_INDEX); + assert_eq!(index_oid(&pool).await, None); + assert_eq!(seed.names(&pool).await, before, "no data changes"); + + assert_eq!(rename_duplicates(&pool, &seed.host).await, 1); + assert!(rename_plan(&pool, &seed.host).await.is_empty()); + + // A channel writer that holds its lock past the 1s lock budget makes the + // inline build fail closed: nothing is recorded and startup can retry. + let mut writer = pool.begin().await.unwrap(); + sqlx::query("LOCK TABLE channels IN ROW EXCLUSIVE MODE") + .execute(&mut *writer) + .await + .unwrap(); + let error = migration::run_migrations(&pool).await.unwrap_err(); + assert!( + error.to_string().contains("lock timeout"), + "the build must not queue behind a writer: {error}" + ); + writer.rollback().await.unwrap(); + assert_eq!(ledger_version(&pool).await, BEFORE_INDEX); + assert_eq!(index_oid(&pool).await, None); + + // With no writer in the way, the populated table migrates with no + // operator step: the migration builds the index itself. + migration::run_migrations(&pool) + .await + .expect("migrate a populated table after the renames"); + assert_eq!(ledger_version(&pool).await, BEFORE_INDEX + 1); + let mut expected = vec![ + (kept, "square-ios".to_owned()), + renamed(newer, "Square-iOS"), + ]; + expected.sort(); + assert_eq!(seed.names(&pool).await, expected); + + // The database now refuses what an older relay could still write. + let duplicate = sqlx::query( + "INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) \ + VALUES ($1, $2, 'SQUARE-IOS', 'forum', 'open', $3)", + ) + .bind(Uuid::new_v4()) + .bind(seed.community) + .bind(&seed.created_by) + .execute(&pool) + .await + .unwrap_err(); + assert_eq!( + duplicate + .as_database_error() + .and_then(|e| e.constraint()) + .map(str::to_owned), + Some("idx_channels_listed_name".to_owned()) + ); +} + +#[tokio::test] +#[ignore = "requires Postgres"] +async fn migration_schema_listed_name_index_validates_a_prebuild() { + let (pool, _scratch) = + ScratchDb::migrated_through(&admin_url().await, "listed_name_prebuild", BEFORE_INDEX).await; + for remnant in [ + // A different predicate: archived channels left out. + "CREATE UNIQUE INDEX idx_channels_listed_name ON channels (community_id, lower(name)) \ + WHERE visibility = 'open' AND NOT unlisted AND archived_at IS NULL", + // A failed concurrent build. Simulated only in this disposable + // superuser database. + "CREATE UNIQUE INDEX idx_channels_listed_name ON channels (community_id, lower(name)) \ + WHERE visibility = 'open' AND NOT unlisted \ + AND channel_type IN ('stream', 'forum') AND deleted_at IS NULL; \ + UPDATE pg_index SET indisvalid = false \ + WHERE indexrelid = 'idx_channels_listed_name'::regclass", + ] { + sqlx::raw_sql(sqlx::AssertSqlSafe(remnant)) + .execute(&pool) + .await + .unwrap(); + let error = migration::run_migrations(&pool).await.unwrap_err(); + assert!( + error + .to_string() + .contains("is invalid or has a different definition"), + "must reject the catalog shape: {error}" + ); + assert_eq!(ledger_version(&pool).await, BEFORE_INDEX); + sqlx::query("DROP INDEX CONCURRENTLY idx_channels_listed_name") + .execute(&pool) + .await + .unwrap(); + } + + crate::test_support::prebuild_listed_channel_name_index(&pool).await; + let oid = index_oid(&pool).await; + // A channel writer holds its lock through the migration. Over a prebuild, + // 0083 must not request the ShareLock a CREATE INDEX takes, even with + // IF NOT EXISTS: that waits behind writers and queues new ones. + let mut writer = pool.begin().await.unwrap(); + sqlx::query("LOCK TABLE channels IN ROW EXCLUSIVE MODE") + .execute(&mut *writer) + .await + .unwrap(); + migration::run_migrations(&pool) + .await + .expect("accept the prebuild without waiting for channel writers"); + writer.rollback().await.unwrap(); + assert_eq!(ledger_version(&pool).await, BEFORE_INDEX + 1); + assert_eq!(index_oid(&pool).await, oid, "the prebuild is not replaced"); +} diff --git a/crates/buzz-db/src/store/channel.rs b/crates/buzz-db/src/store/channel.rs index 21cf59e46f4..a27698b3d23 100644 --- a/crates/buzz-db/src/store/channel.rs +++ b/crates/buzz-db/src/store/channel.rs @@ -161,7 +161,6 @@ pub async fn create_channel( .execute(tx.conn()) .await .map_err(|e| listed_name_taken(e, Some(name)))?; - ensure_listed_channel_name_unique(&mut tx, id).await?; sqlx::query( r#" @@ -405,7 +404,6 @@ async fn insert_channel_rows( let was_created = rows_affected > 0; if was_created { - ensure_listed_channel_name_unique(tx, channel_id).await?; // Bootstrap the creator as owner. sqlx::query( r#" @@ -454,69 +452,22 @@ async fn insert_channel_rows( Ok(was_created) } -/// SQL that is true for a channel the channel directory lists: an open, -/// listed stream or forum that is not deleted. An archived channel is still -/// listed, so it keeps its name. Within a community, no two of these share a -/// name, ignoring case ([`ensure_listed_channel_name_unique`]). +/// The unique index (migration 0083) that keeps two channels the channel +/// directory lists from sharing a name in a community, ignoring case. It +/// covers open, not unlisted streams and forums that are not deleted; an +/// archived channel keeps its name. Private channels, DMs and the other +/// types may repeat names: nobody browses them side by side, and refusing a +/// name that a private channel holds would tell anyone that the channel +/// exists. A write that claims a taken name waits for a concurrent writer of +/// that name to finish, then fails if it committed. +const LISTED_NAME_INDEX: &str = "idx_channels_listed_name"; + +/// SQL that is true for a channel the channel directory lists: the predicate +/// of [`LISTED_NAME_INDEX`]. An archived channel is still listed. pub(crate) const LISTED_CHANNEL_SQL: &str = "visibility = 'open' AND NOT unlisted AND channel_type IN ('stream', 'forum') \ AND deleted_at IS NULL"; -/// Refuses, with [`DbError::ChannelNameTaken`], a write on `tx` that left -/// `channel_id` listed ([`LISTED_CHANNEL_SQL`]) under a name another listed -/// channel in the community already has, ignoring case. Private channels, -/// DMs and the other types may repeat names: nobody browses them side by -/// side, and refusing a name that a private channel holds would tell anyone -/// that the channel exists. -/// -/// Call it after the write that creates the channel or changes its name, -/// visibility or unlisted flag. It first takes a transaction advisory lock -/// on the name, so two writers that claim one name take turns and the second -/// sees the first's committed row. A writer takes this lock last, after the -/// locks of [`lock_channel_for_settings_write`], and holds at most one. -async fn ensure_listed_channel_name_unique( - tx: &mut crate::AdmittedTx, - channel_id: Uuid, -) -> Result<()> { - let community_id = tx.community(); - let name: Option = sqlx::query_scalar(sqlx::AssertSqlSafe(format!( - "SELECT name FROM channels WHERE community_id = $1 AND id = $2 AND {LISTED_CHANNEL_SQL}" - ))) - .bind(community_id.as_uuid()) - .bind(channel_id) - .fetch_optional(tx.conn()) - .await?; - let Some(name) = name else { - return Ok(()); - }; - sqlx::query("SELECT pg_advisory_xact_lock(hashtextextended($1 || lower($2), 0))") - .bind(format!("buzz_channel_name:{}:", community_id.as_uuid())) - .bind(&name) - .execute(tx.conn()) - .await?; - let taken: bool = sqlx::query_scalar(sqlx::AssertSqlSafe(format!( - "SELECT EXISTS (SELECT 1 FROM channels WHERE community_id = $1 AND id <> $2 \ - AND lower(name) = lower($3) AND {LISTED_CHANNEL_SQL})" - ))) - .bind(community_id.as_uuid()) - .bind(channel_id) - .bind(&name) - .fetch_one(tx.conn()) - .await?; - if taken { - return Err(DbError::ChannelNameTaken(name)); - } - Ok(()) -} - -/// The unique index on `(community_id, lower(name))` over -/// [`LISTED_CHANNEL_SQL`] channels that migration 0081 adds. It refuses a -/// duplicate at the write itself, before -/// [`ensure_listed_channel_name_unique`] runs, so this release maps the -/// refusal too: it can run beside that index during the rollout of 0081 and -/// after a rollback. -const LISTED_NAME_INDEX: &str = "idx_channels_listed_name"; - /// Reports a channel write that [`LISTED_NAME_INDEX`] refused as /// [`DbError::ChannelNameTaken`] for `name`, the name the channel would have /// had. @@ -1302,9 +1253,6 @@ async fn update_channel_on( if result.rows_affected() == 0 { return Err(DbError::ChannelNotFound(channel_id)); } - if updates.name.is_some() || updates.visibility.is_some() || updates.unlisted.is_some() { - ensure_listed_channel_name_unique(tx, channel_id).await?; - } if policy_changed { sqlx::query( "UPDATE communities SET artifact_policy_modified_at=clock_timestamp() WHERE id=$1", diff --git a/crates/buzz-db/src/store/user_deletion/discovery_postgres_tests.rs b/crates/buzz-db/src/store/user_deletion/discovery_postgres_tests.rs index 00b9c9df9eb..503b2b07c69 100644 --- a/crates/buzz-db/src/store/user_deletion/discovery_postgres_tests.rs +++ b/crates/buzz-db/src/store/user_deletion/discovery_postgres_tests.rs @@ -102,7 +102,7 @@ async fn channel_memberships_discover_participants_deduplicate_and_preserve_owne // Reuse channel IDs across communities to exercise community-scoped identity. // Two hidden channel memberships still count as one participating community. for channel in [Uuid::from_u128(1), Uuid::from_u128(2)] { - sqlx::query("INSERT INTO channels (community_id, id, name, created_by) VALUES ($1, $2, 'inventory-test', decode($3, 'hex'))") + sqlx::query("INSERT INTO channels (community_id, id, name, created_by) VALUES ($1, $2, 'inventory-test-' || $2::uuid::text, decode($3, 'hex'))") .bind(community).bind(channel).bind(&key) .execute(&pool).await.unwrap(); sqlx::query("INSERT INTO channel_members (community_id, channel_id, pubkey, removed_at, hidden_at) VALUES ($1, $2, decode($3, 'hex'), CASE WHEN $4 THEN now() END, now())") diff --git a/crates/buzz-db/src/test_support.rs b/crates/buzz-db/src/test_support.rs index fcf99792840..f1243156805 100644 --- a/crates/buzz-db/src/test_support.rs +++ b/crates/buzz-db/src/test_support.rs @@ -459,6 +459,22 @@ pub(crate) async fn named_pool(application_name: &str) -> sqlx::PgPool { .expect("connect named test pool") } +/// Prebuild `idx_channels_listed_name` concurrently, as an operator of a +/// deployment that migrates out of band does before migration 0083 +/// (docs/channel-name-uniqueness-deployment.md). Run it with no open +/// transaction in the database: the build waits for every one. +pub(crate) async fn prebuild_listed_channel_name_index(pool: &sqlx::PgPool) { + sqlx::query( + "CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS idx_channels_listed_name \ + ON channels (community_id, lower(name)) \ + WHERE visibility = 'open' AND NOT unlisted \ + AND channel_type IN ('stream', 'forum') AND deleted_at IS NULL", + ) + .execute(pool) + .await + .expect("prebuild idx_channels_listed_name"); +} + /// Wait until the session named `application_name` in this test's database is /// blocked on a lock. pub(crate) async fn wait_for_lock_wait(pool: &sqlx::PgPool, application_name: &str) { diff --git a/crates/buzz-relay/src/api/admin/mod.rs b/crates/buzz-relay/src/api/admin/mod.rs index 2728aa73511..85bdbf56d03 100644 --- a/crates/buzz-relay/src/api/admin/mod.rs +++ b/crates/buzz-relay/src/api/admin/mod.rs @@ -5500,7 +5500,7 @@ mod postgres_tests { // Kick needs a channel-scoped report. sqlx::query( "WITH ch AS (INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) \ - SELECT gen_random_uuid(), community_id, 'staff-guard', 'stream', 'open', $2 \ + SELECT gen_random_uuid(), community_id, 'staff-guard-' || $1::uuid::text, 'stream', 'open', $2 \ FROM moderation_reports WHERE id = $1 RETURNING id) \ UPDATE moderation_reports SET channel_id = (SELECT id FROM ch) WHERE id = $1", ) @@ -7272,7 +7272,7 @@ mod postgres_tests { let channel_id = uuid::Uuid::new_v4(); sqlx::query( r#"INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) - VALUES ($1, $2, 'event-report-ch', 'stream', 'open', $3)"#, + VALUES ($1, $2, 'event-report-ch-' || $1::uuid::text, 'stream', 'open', $3)"#, ) .bind(channel_id) .bind(community_id) @@ -12184,7 +12184,7 @@ mod postgres_tests { // Retarget to the staff-authored event, channel-scoped so kick is valid. sqlx::query( "WITH ch AS (INSERT INTO channels (id, community_id, name, channel_type, visibility, created_by) \ - VALUES (gen_random_uuid(), $2, 'staff-guard', 'stream', 'open', $3) RETURNING id) \ + VALUES (gen_random_uuid(), $2, 'staff-guard-' || $1::uuid::text, 'stream', 'open', $3) RETURNING id) \ UPDATE moderation_reports SET channel_id = (SELECT id FROM ch), target_kind = 'event', \ target_pubkey = NULL, target_event_id = $4 WHERE id = $1", ) diff --git a/docs/channel-name-uniqueness-deployment.md b/docs/channel-name-uniqueness-deployment.md new file mode 100644 index 00000000000..17e61ffcd5d --- /dev/null +++ b/docs/channel-name-uniqueness-deployment.md @@ -0,0 +1,205 @@ +# Listed channel name deployment + +Two listed channels in a community must not share a name, ignoring case. A +listed channel is an open, not unlisted stream or forum that is not deleted. +An archived channel still holds its name until it is deleted. Private +channels, DMs, workflow and system channels may repeat names. + +The rule ships in two releases: + +1. **Writer release: the first relay release that contains + [#8782](https://github.com/block/buzz/pull/8782).** The relay refuses a + create or rename that would give a listed channel a name another listed + channel has, with an `invalid:` reply. It + also reports a write that the contract index refuses with the same reply, + so it can run beside that index. It lets a relay operator rename a listed + channel, archived or not, with a name-only kind:9002, and ships the rename + script `scripts/maintenance/rename_duplicate_listed_channel_names.sh`. +2. **Contract release.** Migration `0083_channels_listed_name_unique.sql` adds + the unique index that makes the database enforce the rule: + + ```sql + CREATE UNIQUE INDEX idx_channels_listed_name + ON public.channels (community_id, lower(name)) + WHERE visibility = 'open' AND NOT unlisted + AND channel_type IN ('stream', 'forum') AND deleted_at IS NULL; + ``` + + From this release the index, not a relay check, enforces the rule. + +The writer release is a required stop. Do not upgrade past it to the contract +release until every step below is complete. + +A deployment that plans schema changes from the desired state in `schema/` +(see [database.md](database.md)) follows the same order: steps 1 to 4 come +first. Its tool must not build `idx_channels_listed_name` before the renames, +and the contract release must not deploy until the index is valid, because +that release no longer checks names in the relay. + +## Procedure + +1. Deploy the writer release, and wait until no older relay pod serves. Check + the image of every pod: a database with no duplicates does not prove that + older relays are gone, and an older relay can still create a duplicate. +2. Rename the existing duplicates in each community. Relays keep serving. + The script reads a plan from the database (a read-only query) and sends + each rename to the relay as a kind:9002, signed by a relay operator. The + relay checks the name and publishes the new kind:39000 as for any rename, + so clients show the new name. + + ```sh + DATABASE_URL=postgres://... BUZZ_PRIVATE_KEY= \ + scripts/maintenance/rename_duplicate_listed_channel_names.sh + ``` + + The key must belong to a relay operator (`RELAY_OPERATOR_PUBKEYS`, or an + `operator` row in `relay_operators`), not a moderator. It must also be a + member of each community: `POST /events` checks community membership + before the operator role, and answers 403 `relay_membership_required` to a + key that is not a member. Add it first: + + ```sh + RELAY_URL=wss:// buzz-admin add-member --pubkey + ``` + + The script stops at the first auth refusal and says so. `BUZZ_RELAY_URL` + defaults to `wss://`. The script needs `psql` and + `python3`. Run it with `--dry-run` first to see + the plan. The host must match `communities.host` exactly; the script stops + for a host that no community has. + + In each group of channels that share a name, the oldest unarchived channel + keeps the name (when all are archived, the clients' starter channel, below, + if it is one of them, else the oldest archived one). Each + other channel becomes `-`, or + `-` when another listed channel already has that name. The script + prints each rename, then reads the plan again and exits 1 if any rename + remains: for example, the relay refused a name that a channel took + meanwhile. Run it again. + + The script also renames every channel named `general` or + `welcome-everyone` when all the listed channels with that name are + archived. Released desktop and mobile apps create those starter channels + when no open, unarchived stream has the name, and the relay refuses that + create while an archived channel holds the name. The apps create each + starter at a fixed id, a UUIDv5 of the relay's HTTP origin and the name. + The script computes those ids with `python3` from `BUZZ_RELAY_URL` (so it + must be the URL clients use), or from `BUZZ_STARTER_ORIGIN` when set (for + a desktop build with its own `BUZZ_RELAY_HTTP`), and prints them; check + them against a client before the real run. When any channel row has one + of those ids, whatever its name or state, the relay refuses the create + there, so the script renames nothing for that starter name, and an + archived starter at its id keeps the name over its namesakes. Those + clients still cannot set up that starter; that needs a relay-side fix, + planned separately. + + If a rename fails on every run because another channel holds its new name, + rename that channel by hand with + `buzz channels update --channel= --name=`. + + The relay applies no rename in an archived community, and the script + stops for one. Unarchive it first, then add the operator key as a member + (`add-member` also finds only an unarchived community), run the script, + and archive it again. Its channels show to members while it is unarchived: + + ```sh + buzz-admin communities unarchive --host \ + --owner-pubkey --operator-id --reason "rename duplicate channels" + RELAY_URL=wss:// buzz-admin add-member --pubkey + # run the rename script for + buzz-admin communities archive --host \ + --owner-pubkey --operator-id --reason "rename duplicate channels" + ``` + + The relay applies no rename in a community that is being deleted either, + and the script stops for one. Its duplicates block migration 0083 until its + deletion completes. If the deletion is blocked, fix the recorded failure + and resume it as + [operator-community-deletion.md](operator-community-deletion.md) + describes; do not deploy the contract release until it completes. +3. Confirm that no duplicate remains in any community: + + ```sql + SELECT co.host, co.archived_at, co.deletion_state, lower(ch.name), count(*) + FROM public.channels ch + JOIN public.communities co ON co.id = ch.community_id + WHERE ch.visibility = 'open' AND NOT ch.unlisted + AND ch.channel_type IN ('stream', 'forum') AND ch.deleted_at IS NULL + GROUP BY co.host, co.archived_at, co.deletion_state, lower(ch.name) + HAVING count(*) > 1; + ``` + + Expect zero rows. Each row names a host to run the script for: an archived + community, or one being deleted, needs the steps in step 2. +4. Only for a deployment that migrates out of band (auto-migration off) and + whose `channels` table is too large for the migration's budgets: build the + index concurrently first. Otherwise skip this step. Migration 0083 builds + the index itself with a plain `CREATE UNIQUE INDEX`, under a 1s + `lock_timeout` and a 5s `statement_timeout`, which takes milliseconds on a + table of a few thousand channels. If the build cannot get its lock or + finish within those budgets, the migration fails, records nothing, and the + relay retries at its next start. A plain build blocks channel writes while + it runs; a concurrent one does not. `CREATE INDEX CONCURRENTLY` cannot run + inside a transaction block: + + ```sql + CREATE UNIQUE INDEX CONCURRENTLY idx_channels_listed_name + ON public.channels (community_id, lower(name)) + WHERE visibility = 'open' AND NOT unlisted + AND channel_type IN ('stream', 'forum') AND deleted_at IS NULL; + ``` + + Then verify that PostgreSQL considers the index valid, ready, live, and an + exact match for the definition that migration 0083 expects. Set the + `search_path` first: without `public` on it, PostgreSQL prints the enum casts + as `public.channel_visibility` and the text no longer matches. + + ```sql + SET search_path = public, pg_catalog; + SELECT i.indisvalid, i.indisready, i.indislive, + pg_get_indexdef(i.indexrelid) AS definition + FROM pg_index AS i + JOIN pg_class AS c ON c.oid = i.indexrelid + JOIN pg_namespace AS n ON n.oid = c.relnamespace + WHERE n.nspname = 'public' AND c.relname = 'idx_channels_listed_name'; + ``` + + Expect all three flags to be `true`, and this definition: + + ```text + CREATE UNIQUE INDEX idx_channels_listed_name ON public.channels USING btree (community_id, lower((name)::text)) WHERE ((visibility = 'open'::channel_visibility) AND (NOT unlisted) AND (channel_type = ANY (ARRAY['stream'::channel_type, 'forum'::channel_type])) AND (deleted_at IS NULL)) + ``` + + The writer release works with the index in place. +5. Apply migration 0083. It refuses to run while duplicates remain. With no + index, it builds one; over a prebuilt index it skips its own `CREATE`. + Then it checks the index and records the migration. + - **Auto-migration on:** deploy the contract release. The relay applies + 0083 at startup. It does not start while a duplicate remains, so finish + steps 2 and 3 first. + - **Auto-migration off:** run `buzz-admin migrate` with the target + `DATABASE_URL`, confirm that it records 0083, then deploy the contract + release. Nothing at startup checks the index, and the contract release + no longer checks names in the relay. Do not run the file with `psql`: that + does not record the migration ledger, and outside a transaction both + `SET LOCAL` timeouts do nothing. + +## Recovery + +A concurrent build fails if a duplicate remains or appears while it runs, and +leaves an invalid index. Migration 0083 rejects an invalid, unready, non-live, +or differently defined index. Remove only that index, outside a transaction, +run the rename script again, repeat step 3, and build again (step 4 or 5): + +```sql +DROP INDEX CONCURRENTLY IF EXISTS public.idx_channels_listed_name; +``` + +## Rollback + +Roll back from the contract release to the writer release, not further: an +older relay cannot report the index's refusal as `invalid:`. Keep the index. +Run the writer release with auto-migration off (`BUZZ_AUTO_MIGRATE=false`, or +`migrate.autoMigrate=false` in the Helm chart). With it on, the writer release +refuses a database that has migration 0083, which it does not know +(`VersionMissing`). diff --git a/migrations/0083_channels_listed_name_unique.sql b/migrations/0083_channels_listed_name_unique.sql new file mode 100644 index 00000000000..1452c89e878 --- /dev/null +++ b/migrations/0083_channels_listed_name_unique.sql @@ -0,0 +1,54 @@ +-- Contract: two listed channels in a community never share a name, ignoring +-- case. A listed channel is an open, not unlisted stream or forum that is not +-- deleted; an archived one still counts. The previous release checks the rule +-- in the relay on create and rename; from this release the index enforces it, +-- and the relay reports a refused write as "invalid: an open channel named +-- ... already exists" (`LISTED_NAME_INDEX` in crates/buzz-db/src/store/channel.rs). +-- +-- Before deploying, every serving relay must enforce the rule and +-- scripts/maintenance/rename_duplicate_listed_channel_names.sh must have +-- renamed the existing duplicates. See docs/channel-name-uniqueness-deployment.md. +-- +-- The migration builds the index itself, as 0049 does, under the lock and +-- statement budgets below (docs/database.md Rule 6). A deployment too large +-- for them migrates out of band and builds the index concurrently first; the +-- migration then only verifies it. +SET LOCAL lock_timeout = '1s'; +SET LOCAL statement_timeout = '5s'; +-- pg_get_indexdef qualifies the enum casts below (`public.channel_visibility`) +-- when `public` is not on the search_path; pin it so the exact compare holds. +SET LOCAL search_path = public, pg_catalog; +-- IF NOT EXISTS still requests a writer-conflicting ShareLock before checking +-- whether the index exists. Bypass CREATE entirely for a prebuilt index, then +-- validate it below; a failed concurrent build must never count as success. +DO $$ +BEGIN + IF to_regclass('public.idx_channels_listed_name') IS NULL THEN + IF EXISTS ( + SELECT 1 FROM public.channels + WHERE visibility = 'open' AND NOT unlisted + AND channel_type IN ('stream', 'forum') AND deleted_at IS NULL + GROUP BY community_id, lower(name) + HAVING count(*) > 1 + ) THEN + RAISE EXCEPTION 'Migration 0083 stopped: listed channels with duplicate names remain. Rename them with scripts/maintenance/rename_duplicate_listed_channel_names.sh (docs/channel-name-uniqueness-deployment.md, steps 2 and 3), then migrate again.'; + END IF; + + CREATE UNIQUE INDEX idx_channels_listed_name + ON public.channels (community_id, lower(name)) + WHERE visibility = 'open' AND NOT unlisted + AND channel_type IN ('stream', 'forum') AND deleted_at IS NULL; + END IF; + + IF NOT EXISTS ( + SELECT 1 FROM pg_index i + JOIN pg_class c ON c.oid = i.indexrelid + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'public' AND c.relname = 'idx_channels_listed_name' + AND i.indisvalid AND i.indisready AND i.indislive + AND pg_get_indexdef(i.indexrelid) = + 'CREATE UNIQUE INDEX idx_channels_listed_name ON public.channels USING btree (community_id, lower((name)::text)) WHERE ((visibility = ''open''::channel_visibility) AND (NOT unlisted) AND (channel_type = ANY (ARRAY[''stream''::channel_type, ''forum''::channel_type])) AND (deleted_at IS NULL))' + ) THEN + RAISE EXCEPTION 'Migration 0083 stopped: idx_channels_listed_name is invalid or has a different definition. Drop it and build it again (docs/channel-name-uniqueness-deployment.md, Recovery), then migrate again.'; + END IF; +END $$; diff --git a/schema/tables/public/channels.sql b/schema/tables/public/channels.sql index 859bd3d3d3d..9162b502a64 100644 --- a/schema/tables/public/channels.sql +++ b/schema/tables/public/channels.sql @@ -62,3 +62,11 @@ CREATE INDEX idx_channels_ttl_expiry ON channels (ttl_deadline) -- Not UNIQUE — the same channel id may exist under more than one community. CREATE INDEX idx_channels_id_live ON channels (id) INCLUDE (community_id) WHERE deleted_at IS NULL; +-- Two listed channels in a community never share a name, ignoring case. A +-- listed channel is an open, not unlisted stream or forum that is not deleted; +-- an archived one still counts. Created by migration 0083; the relay maps a +-- refused write to ChannelNameTaken (LISTED_NAME_INDEX in +-- crates/buzz-db/src/store/channel.rs). +CREATE UNIQUE INDEX idx_channels_listed_name ON channels (community_id, lower(name)) + WHERE visibility = 'open' AND NOT unlisted + AND channel_type IN ('stream', 'forum') AND deleted_at IS NULL; diff --git a/scripts/maintenance/plan_duplicate_listed_channel_renames.sql b/scripts/maintenance/plan_duplicate_listed_channel_renames.sql index 4ef4a567dc2..e87939912e1 100644 --- a/scripts/maintenance/plan_duplicate_listed_channel_renames.sql +++ b/scripts/maintenance/plan_duplicate_listed_channel_renames.sql @@ -5,7 +5,7 @@ -- that no other listed channel there has, ignoring case. A listed channel is -- an open, not unlisted stream or forum that is not deleted; an archived one -- still counts. This is the rule the relay enforces on create and rename --- (`LISTED_CHANNEL_SQL` in crates/buzz-db/src/store/channel.rs). +-- (the predicate of `idx_channels_listed_name`, migration 0083). -- -- In each group of channels that share a name, one channel keeps it: the -- oldest unarchived one, or when all are archived, the clients' starter