Repository navigation
feat(db): enforce unique listed channel names with an index - #8787
Conversation
241cacc to
27493b4
Compare
f1f446a to
9d97eb2
Compare
This comment has been minimized.
This comment has been minimized.
6e0ee8b to
8b9f791
Compare
This comment has been minimized.
This comment has been minimized.
salman1993
left a comment
There was a problem hiding this comment.
🤖 Approved on Salman's behalf by his AI agent.
Follow-up review of 8b9f7917a44784df75b22139c34765963b3a233d: no remaining findings or blockers from this review.
This head includes #8782's archived-rename fix and the Windows test guard. The index, migration and deployment document are unchanged from my earlier review. I inspected the current CI logs: all 1,291 PostgreSQL-lane tests passed on the PR merge ref containing this head, including both listed-name migration tests, the script test, and the archived operator-rename regression. Windows and unit checks also pass. Independently reran the 16 group-state relay tests on the exact upstream writer head; all passed. No separate local relay/app run was performed for this contract head.
Minimalism 9/10, elegance 9/10, correctness 9/10. This supersedes my earlier hold recommendation. Deploy only after the writer release is fully rolled out, duplicate cleanup validates to zero rows, and the unique index has been prebuilt concurrently and verified; apply 0079 before the contract relay serves traffic.
8b9f791 to
a2dcffc
Compare
This comment has been minimized.
This comment has been minimized.
9ee09a6 to
9f7b3b2
Compare
a2dcffc to
603ccad
Compare
|
🤖 Agent run of the full rollout on a local relay, with relay membership required and a fresh database. Writer release: #8782 at 0df245e. Contract release: this PR at 603ccad.
|
0df245e to
65d0c0c
Compare
196f398 to
e49678f
Compare
…8782) ## Summary Two open channels in one community could have the same name, so the channel list showed two `#square-ios` rows and people could not tell them apart. Now the relay refuses to create, rename or open up a channel when that would give it the same name as another channel in the channel list, ignoring case. The client gets `invalid: an open channel named "…" already exists`. The rule follows Slack: an archived channel keeps its name until it is deleted. Private channels, DMs and unlisted channels may still repeat names. So a refusal never reveals that a private channel exists. The PR also adds a script that renames the duplicates that exist today. It sends each rename through the relay, so relays keep serving while it runs. The stacked PR #8787 then adds a database index that enforces the rule. This PR is the release that must be fully deployed before that index. ## Details **Which channels the rule covers.** A *listed* channel: open, not unlisted, a stream or forum, not deleted. Archived channels are included. The check runs on create, rename, private → open and unlisted → listed. **Relay check** (`crates/buzz-db/src/store/channel.rs`, `crates/buzz-relay/src/handlers/ingest.rs`): - The check runs after the write, in the same transaction, under a per-name lock. So two concurrent writers cannot both commit the same name. - A refused create or edit (kind:9007 or kind:9002) gets `OK false "invalid: an open channel named … already exists"`. Nothing from it is stored. - The relay gives the same answer when the database index from #8787 refuses the write. That covers the rollout, and a rollback from that release. - A channel create with no `h` tag (the new channel's id) is now refused before it is stored. Before, such a create ran after the event was stored, so a refusal (for example, a taken name) still answered "accepted". All first-party clients send `h`. **Operator rename** (`crates/buzz-relay/src/handlers/side_effects.rs`). The rename script needs to rename channels it does not own, archived ones included. So a relay operator may now send a channel edit that changes only the name of a listed channel. Everything else still needs the channel's owner or admin. - "Operator" means `RELAY_OPERATOR_PUBKEYS`, the owner fallback, or an `operator` row in `relay_operators`. A moderator is not an operator. - The rename also applies to an archived listed channel, whatever the operator's role in that channel, and the channel stays archived. Other edits of an archived channel are still refused. - Private channels, unlisted channels and DMs still need the channel's owner or admin. The relay decides "listed" under the channel row lock, so a channel made private during the rename is not renamed. - The rename takes the normal edit path: the name check, the stored event, the channel metadata event and fan-out. **Rename script** (`scripts/maintenance/rename_duplicate_listed_channel_names.sh`, with the read-only plan `plan_duplicate_listed_channel_renames.sql`): - In each group of listed channels that share a name, the oldest unarchived channel keeps the name. Each other channel becomes `<name>-<first 8 chars of id>`, or `<name>-<id>` when that is also taken. When all the channels in a group are archived, the oldest keeps the name. - **Starter channels.** Released desktop and mobile apps create `#general` and `#welcome-everyone` when a community has no open, unarchived stream with that name. Under the new rule, the relay refuses that create while an archived channel has the name, and the apps' setup fails. So the script also renames `general` and `welcome-everyone` when every listed channel with that name is archived. That frees the name for the apps. - The apps create each starter at a fixed id, derived from the relay's address. If a channel already has that id, the relay refuses the create there whatever its name, so a rename cannot help. For that name the script renames nothing, and an archived starter at its id keeps the name. The script computes the ids from `BUZZ_RELAY_URL` (or `BUZZ_STARTER_ORIGIN` when the apps use a different address) and prints them, so the operator can check them against a client. A relay-side fix for those clients is planned separately. - The operator key goes in `BUZZ_PRIVATE_KEY`. It must be a relay operator and a member of the community. The script stops at the first auth refusal and says so. - The script stops before the plan for an unknown host, an archived community and a community being deleted, and says what to do. - A failed rename (the new name is now taken, or the relay can't be reached) is reported, and the script goes on. At the end it reads the plan again and exits 1 if any rename remains. `--dry-run` prints the plan. - The script replaces the earlier direct-SQL repair and `buzz-admin reconcile-channels --metadata`, which needed every relay pod stopped. **Duplicates that exist today** (read on 2026-10-30): `ml-platform-public` (`d41a31f8…`, `7be55f5a…`) and `square-ios` (`79f8e9d4…` archived, `fe246d3e…` live). ### Rollout The full procedure, with exact SQL and recovery steps, is in `docs/channel-name-uniqueness-deployment.md` in #8787. 1. **Deploy this PR.** Wait until no older relay pod serves; check the image of every pod. An older relay can still create a duplicate, and it refuses the operator rename. 2. **Rename the existing duplicates in each community**, with relays serving. The key must be an `operator`, not a moderator, and a member of the community (`RELAY_URL=wss://<host> buzz-admin add-member --pubkey <npub>`). ```sh DATABASE_URL=postgres://... BUZZ_PRIVATE_KEY=<operator key> \ scripts/maintenance/rename_duplicate_listed_channel_names.sh --dry-run <community host> # review the plan, then run it again without --dry-run ``` For the duplicates known today, the newer `ml-platform-public` and the archived `square-ios` (`79f8e9d4…`) get the `-<id8>` suffix. 3. **Confirm that no duplicate remains** in any community (query in the doc). 4. **Build the index, then deploy #8787 Rollback from #8787 goes to this release only, keeping the index, with auto-migration off. ### Related issue None found. ### Testing - **Live local relay.** The script ran against a local relay built at 9ee09a6, with relay membership required and six seeded channels (`ops`, `OPS`, an archived `Ops`, a private `ops`, `dev`, `DEV`): - `--dry-run` listed the three renames. - With the operator key not yet a member, the script stopped at the first refusal, named the membership precondition and renamed nothing. - After `buzz-admin add-member`, it renamed all three and exited 0. The archived channel stayed archived, the private `ops` kept its name, and a second run renamed nothing. - With the community archived, the script stopped with the unarchive message. - Rerun at 0df245e as part of the full rollout in #8787: the relay refused a new duplicate over the wire, and the script renamed the seeded duplicates with the relay serving. - **Relay tests** (`group_state_postgres_tests.rs`): a taken name is refused as `invalid:` and nothing is stored; a create with no `h` tag is refused before it is stored; an operator can rename only a listed channel, only the name, archived or not, and a moderator cannot; an operator rename is refused if the channel stops being listed before it is saved. - **Store tests** (`channel_name_postgres_tests.rs`): create, rename, private → open and unlisted → listed are refused for a taken name; private and unlisted names may repeat; an archived channel keeps its name and a deleted one frees it; of two concurrent creates with one name, the second is refused once the first commits. - **Plan and script tests** (`listed_channel_name_postgres_tests.rs`) run the shipped SQL and shell script: which channel keeps the name, the starter cases, names at the 255-character limit, names with control characters or a leading `-`, and the auth and community stops. The starter-id computation matches the mobile app's test vectors. - The new guards were each removed once to confirm that a test fails. - The full local Postgres test runs (buzz-db, buzz-relay, buzz-admin) pass, except 6 tests that also fail locally on main and pass in CI. ### Database review checklist - [x] Phase: writer release (deploy), plus a maintenance script. The stacked PR adds the contract index; this release is its required stop. - [x] No trigger, function, procedure, rule or foreign key. - [x] No migration. The script writes nothing to the database itself. It reads a plan, and the relay applies each rename as a normal channel edit. `docs/database.md` Rule 3 asks for maintenance scripts in their own PR. The script is here because it depends on the operator rename in this PR. - [x] Contract: not in this PR. - [x] Rule 4 locations: no schema change. - [x] Deployed relay: the schema is unchanged, so the deployed relay keeps working. An older relay refuses the operator rename, so run the script only after this release is fully deployed. This release also works beside the contract index (tests above). - [x] Timeouts: no index build or bulk write here. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Logan Johnson <loganj@squareup.com> Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
The base branch was changed.
a865378 to
2cbfa03
Compare
|
🤖 Rebased onto main |
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
salman1993
left a comment
There was a problem hiding this comment.
🤖 Reviewed and approved on Salman's behalf by sol, his AI agent.
Re-reviewed 2cbfa03db7ba83d913e7740819c2c5e191b51ea7 against 83d0bd32034e4684836b10d0854e107c410c1e39. No blocking findings. Minimalism 9/10, elegance 9/10, correctness 9/10.
All four rebased commits have unchanged patches relative to a8653787f (git range-diff). Checked migration 0081, the desired-state index/parity, channel write/error mapping, tests, and rollout instructions against the repository database rules. The index preserves the listed-channel predicate and community boundary; the migration rejects missing/invalid prebuilds and avoids CREATE over a valid prebuild.
Validation: inspected the current PostgreSQL CI logs on merge da53e9b containing this head and base: all 1,380 tests passed, including the migration guards, writer-lock bypass, schema parity, name collisions/concurrency, and relay refusals. All 59 executed checks passed; 32 checks were skipped. Local diff whitespace check passed. No local build or live relay run in this re-review.
Code approval only: retain the merge/deployment hold until production runs #8782 on every serving pod, existing duplicates are renamed and validated to zero, and the unique index is prebuilt concurrently and verified per docs/channel-name-uniqueness-deployment.md. Production readiness was not verified by this review.
|
🤖 Moved back to draft to hold the merge until the rollout preconditions in docs/channel-name-uniqueness-deployment.md are met in every environment: prod runs #8782, prod duplicates are renamed, and the index is prebuilt. |
Migration 0079 adds a unique partial index on (community_id, lower(name)) for listed channels: open, not unlisted streams and forums that are not deleted. It is the contract step for the relay check that refuses duplicate names on create and rename. The database now refuses a duplicate that an older relay, a raw SQL write or a missed code path could still produce. The index refuses a duplicate at the write itself, and a write that claims the name of a concurrent uncommitted one waits for it. So the relay's post-write check and its per-name advisory lock go. The store already maps the index violation to ChannelNameTaken, so the client still gets "invalid: an open channel named ... already exists". Test fixtures that inserted two open channels with one name now use distinct names, and tests that migrate a database with channels through 0079 prebuild the index first, as an operator does. The migration bounds its locks and run time. It refuses to start while duplicates remain, naming the repair script. On a populated channels table it refuses to build the index under a write-blocking lock and asks for a concurrent prebuild (docs/database.md Rule 6). It skips CREATE for a prebuild and then requires a valid, ready, live index with the exact expected definition. The desired-state schema has the same index, and the parity test compares the two. docs/channel-name-uniqueness-deployment.md gives the rollout order, the prebuild, the recovery and the rollback. The writer release is a required stop: deploy it, run scripts/maintenance/rename_duplicate_listed_channel_names.sql and prebuild the index there, before upgrading to this one. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Logan Johnson <loganj@squareup.com>
The rollout doc now says the operator key must be a member of each community, how to rename duplicates in an archived community (unarchive, rename, archive again), and that a community being deleted must finish its deletion first. Step 3 prints the host and state of each community with duplicates. Step 5 names `buzz-admin migrate` for out-of-band deploys, warns against applying the file with psql, and says that a desired-state deploy follows steps 1 to 4 first. A test now holds a channel writer's lock while migration 0079 runs over a prebuilt index, so a CREATE INDEX IF NOT EXISTS there fails the test. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Logan Johnson <loganj@squareup.com>
pg_get_indexdef qualifies the enum casts (`public.channel_visibility`) when `public` is not visible on the session search_path, so the exact definition compare in 0081, and the operator's verify query in the rollout doc, would report a valid prebuilt index as wrong. Pin the search_path in the migration and in the doc's verify query. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Logan Johnson <loganj@squareup.com>
sqlx appends the server's source line (" at line 3897") to every
Postgres error, so a message that ended in a doc path read as a line in
that doc. Each refusal now names the migration, says what to do and
which doc step covers it, and ends in a sentence.
Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Logan Johnson <loganj@squareup.com>
Main took 0081 and 0082, so the migration is now 0083. With BUZZ_AUTO_MIGRATE on, a migration that refuses a populated table crash-loops every relay with data (#8802, docs/database.md Rule 6). The migration no longer refuses a channels table with rows. As 0049 does, it skips CREATE when to_regclass finds a prebuilt index, else builds the index with a plain CREATE UNIQUE INDEX under the 1s lock and 5s statement budgets, then verifies its definition and flags. A busy or very large table fails on those budgets, records nothing, and the relay retries at its next start. It still refuses to run while listed duplicates remain: the index cannot exist over them, and the renames must go through the relay. Tests that migrate a populated database no longer prebuild the index. The listed-name migration test now checks that a held channel writer makes the inline build fail on its lock budget, and that a populated table then migrates with no operator step. The rollout doc makes the concurrent prebuild an out-of-band option only. Co-authored-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Logan Johnson <loganj@squareup.com>
2cbfa03 to
108737d
Compare
|
🤖 Changed to follow the rule from #8802 that a migration must never refuse a populated table (tornquist asked for this in review routing). Head
Pushed with |
TheSentinel454
left a comment
There was a problem hiding this comment.
🤖 LGTM. Migration 0083 checks for duplicates and validates the index (exact def, valid/ready/live) under tight lock/statement timeouts. Every production channel write path maps the index violation to listed_name_taken. Duplicate listed names are now cleared in every environment we run.
- tested-image is sha-744f394 (block/buzz 744f3946f, after the #8802 fix), with tested-digest sha256:b67a32b7…; SKILL.md section 2 reads both from the metadata, so a pin move is one edit - installs and upgrades use it; compose.md step 6, the Compose upgrade and kubernetes.md step 6 check the pulled image's digest (Kubernetes not tested) - pick-tag.py runs only when the user asks for the newest, and says on stderr whether it's the pin - before an upgrade, Compose and Kubernetes check that no two open channels share a name (migration 0083, block/buzz#8787; not tested); on Kubernetes, a helm upgrade that times out means stop and ask, never helm rollback - starter channels: invite links and Add directly join them, add-member doesn't (#8594) - troubleshooting rows for a digest mismatch and migration 0083's refusal - the four smoke scripts default to the pin; smoke-local.sh, smoke.sh and smoke-vps.sh run the digest check and the upgrade's name check verbatim Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ernetes upgrades ask about duplicate names too The restore flow's new Check first step came after Clear the way, which deletes the volumes, so a backup with a missing or empty file was caught only after the data was gone. The backup's files and address are now checked first, as operations.md says (Before changing anything); the empty-volume check stays right before the restore block, as 83cc0aa had it. The skill's Kubernetes upgrade checks for duplicate public channel names before helm upgrade (migration 0083, block/buzz#8787), so its What happens list gets the same ask as Compose's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…2904) ## Summary A brand-new community has no channels, so its owner lands on a blank Messages page ("Select a channel to read it.") after onboarding. Until the relay creates a landing channel (block/buzz#9108), this PR has the owner's client do it, and nothing more. `useFirstChannel` (`src/bundled/channels/first-channel.ts`), called once from `ChannelWorkspace`, acts only when **all** of these hold: - the session is live (not cached), the channel list is `ready`, and the viewer has joined no stream or forum channel (Solo chats and DMs don't count); - the community roster says the viewer is the community **owner** (admins and members never act); - `channels.anyChannel()` (new: `{kinds:[39000], authors:[relay], "#t":["stream","forum"], limit:20}`, Solo-labelled streams skipped) finds **no** stream or forum, open, private or archived. DMs, Solo chats (the Bestie tab opens one first) and system channels don't count. A failed check does nothing and retries next launch. A full batch of 20 Solo chats counts as not empty: it creates nothing and settles the marker, so clients stop checking. It then creates one open `general` through the existing `channelCreation.create` and flags it as a starter channel with `auto-join-on`, so the relay (block/buzz#8594) joins new members to it. Messages opens it through its existing first-channel fallback (#1624). **Not spamming general channels:** - The relay's unique open channel names (block/buzz#8787) refuse a second `general` in the same community. - A settled view-state marker per community on each device means that device never checks again once the community has a channel. - At most one attempt per community per app run. A failure (for example, the name is already taken) waits for the next launch instead of retrying on every render. - It never joins anyone, adds members or posts a message. - Relay load: the community roster (`13534`, `limit: 1`) is read only for a viewer with no channel to open, so ordinary clients make no new reads. The channel check is a single read capped at 20 events, never a directory scan. Non-goals: welcome messages, landing-channel designation, server-side provisioning (block/buzz#9108 / #2623). ## Testing - `first-channel.test.tsx` (new, jsdom + RTL `renderHook`): - the owner of an empty community gets exactly one open `general` plus `auto-join-on`, and a later mount, even in a new run, doesn't read or create again; - a community where the relay already holds a channel settles without creating, and a failed check creates and settles nothing; - `anyChannel` sends exactly the capped `#t` filter, ignores Solo chats, counts an archived stream, and treats a full batch of Solo chats as not empty (store test); - admin, member and missing roles do nothing; - a joined channel, a loading list or a cached session do nothing and don't read the roster; - a refused create isn't retried in the same run. - Fail-then-pass: loosening the owner check fails 3 cases. - Full `vitest run` locally: 1022 files / 19379 tests pass; typecheck and `biome check` clean. - `vitest run src/bundled/channels src/bundled/moderation`: all pass, including the existing starter-channel `ChannelsPage` cases. `tsc --noEmit` and the pre-push hook suite pass. **Deferred:** - No agent run against a live relay yet. - Human test: create a community in onboarding, open Messages, and #general should appear and open. Reload, and nothing else gets created. A second member joins and lands in #general. - Browser suite left to CI. --------- Signed-off-by: Bradley Axen <baxen@squareup.com> Co-authored-by: mbot <3754f8729004d95654c46dbab3129e4ab9ef05cc2534e2a3fbfc155983bd637b@buzz.block.builderlab.xyz>
Summary
Stacked on #8782 (merged), which makes the relay refuse a second open channel with a name that another channel in the community already has. That check lives in relay code, so an older relay, a direct database write or a missed code path can still create a duplicate.
This PR adds the same rule to the database: a unique index on channel name, ignoring case, for the channels that show in the channel list. With the index in place, a duplicate fails at the write, whatever wrote it. The relay's own name check from #8782 is no longer needed, so this PR removes it. Clients still get the same refusal,
invalid: an open channel named "…" already exists.The migration builds the index itself, also on a populated
channelstable, so an auto-migrating relay upgrades with no operator index step (the rule from #8802). It still refuses to run while duplicate names remain, because the index cannot exist over them.docs/channel-name-uniqueness-deployment.mdgives the full rollout.Details
The index (migration
0083_channels_listed_name_unique.sql):It covers open, not unlisted streams and forums that are not deleted, archived ones included. Private channels, DMs and unlisted channels may still repeat names.
The migration builds the index inline, as
0049does. Whento_regclassfinds no index, it runs a plainCREATE UNIQUE INDEXunderlock_timeout1s andstatement_timeout5s. On a busy or very large table it fails on those budgets, records nothing, and the relay retries at its next start. When the index already exists (a concurrent prebuild), it skipsCREATEso it never requests the write-blocking lock. In both cases it then checks that the index is valid, ready and has the exact expected definition.One refusal stays: duplicate names. The migration refuses to run while listed duplicates remain, and its error names the rename script. The renames must go through the relay so clients get the new kind:39000, and a migration must not change rows (
docs/database.mdRule 3). This is the "fails closed until its repair has run" case inmigrations/AGENTS.md, so the writer release from fix(relay): refuse a second listed open channel with the same name #8782 and the rename are a required stop.Relay. A write that takes the name of a concurrent, uncommitted write waits for it, and fails if that write commits. So the post-write check and the per-name lock from fix(relay): refuse a second listed open channel with the same name #8782 are removed. The store already maps the index violation to the same "name taken" error.
Desired-state schema.
schema/tables/public/channels.sqlhas the same index, and the schema parity test compares its live shape between the desired-state schema and the migrations.Rollout (
docs/channel-name-uniqueness-deployment.md), in order:psqlandpython3, and the operator key must be a member of each community. The doc covers archived communities and communities being deleted.CREATE UNIQUE INDEX CONCURRENTLYand verify it.buzz-admin migratefirst, never the file throughpsql.Rollback goes to the fix(relay): refuse a second listed open channel with the same name #8782 release only. Keep the index, and keep auto-migration off.
Test fixtures. Tests that inserted two open channels with one name now use distinct names. Tests that migrate a populated database through 0083 no longer prebuild the index.
Related issue
None found. Follows #8782.
Testing
listed_channel_name_postgres_tests.rs) start from a database migrated through 0082:channels.invalid:.108737dc0, one fresh database per test as the nextest wrapper does: 38 of 40 buzz-db migration-lane tests pass (both listed-name tests, the migration parity test, the thread-window and invite-upgrade tests that migrate populated databases through 0083). The 2event_follow_upcontract tests fail locally the same way on main. All 49 buzz-db channel and channel-member store tests pass.Database review checklist
migrations/,schema/tables/public/channels.sqland the parity test. A live catalog comparison covers the index.invalid:beside the index (its tests pass with the index present). A relay older than that still writes, but gets a database error, notinvalid:, for a duplicate. That is why fix(relay): refuse a second listed open channel with the same name #8782 is a required stop.lock_timeout1s,statement_timeout5s. The migration builds the index inline (plainCREATE UNIQUE INDEX, skipped whento_regclassfinds a prebuilt one). Only out-of-band deployments too large for the budgets prebuild it withCREATE UNIQUE INDEX CONCURRENTLY(doc step 4).🤖 Generated with Claude Code