Repository navigation
Conversation
Plan for keycast#440. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
People who posted through a Divine app before the cutoff with a key they created or brought themselves have no Keycast signup record, so the chit endpoint answered false for them. Add og_diviner_grants, a tenant-scoped list of granted pubkeys with a closed evidence vocabulary, and let is_og_diviner accept a grant in the same statement. The grant lookup sits beside the users lookup rather than inside it: most of this cohort has no users row, and a branch inside that join would never match them. With an empty table every answer is unchanged. Refs #440. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
The roster for keycast#440 is built privately from relay data and loaded once per environment. Parsing rejects the whole file if any non-blank line is not a 64-character lowercase hex pubkey, naming line numbers but never contents. Loading is one INSERT ... ON CONFLICT DO NOTHING, so a re-run grants nothing new and reports the rows as already granted. A dry run counts without writing, and an unknown tenant fails either way. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
A cargo example, following migrate-vine-users, that reads a roster file and loads it through OgDivinerGrantRepository. It is a dry run unless --apply is passed, requires the tenant and evidence label explicitly, and prints counts and line numbers only: never a pubkey, a line of the file, or its path. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
Nothing caught numbering bad lines by non-blank entry rather than by line in the file, which would send an operator to the wrong line. Also correct the repository doc: load runs two statements, one after the other, not one. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
Follows the support-admin seed section: the migration ships an empty table, the roster stays private, and the loader is run dry first and then with --apply over a connection that bypasses the transaction-mode pooler. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
Only the loader needs the pubkey list, so nothing outside keycast_core can print it. Also bring the eligibility test's summary up to date with the grant coverage it now holds. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
Account deletion never touched og_diviner_grants, so a deleted grantee stayed eligible. Most grantees have no Keycast account, so the grant is removed before the ownership check: the service deletion path, run by the account-deletion coordinator on the person's own vanish request, returns already_absent for them and still commits. The delete is scoped to the calling tenant, so another tenant's grant for the same pubkey survives. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
Nothing caught a dry run that counted grants from every tenant, which would report pubkeys granted elsewhere as already granted here and under-predict the real load. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
is_err() also passed on unrelated failures such as a missing tenant's foreign key. Assert SQLSTATE 23514 so the test proves the table's own CHECK constraints did the rejecting. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
A repeated flag let the last value win, so --tenant-id 2 --tenant-id 1 silently loaded into tenant 1. The loader now refuses any flag given twice. Its report also prints the tenant's domain and name beside the id, so the operator can confirm the tenant on the dry run before applying. The example's argument parsing is now tested, and core/Cargo.toml opts the example into `cargo test` so CI runs those tests. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
The loader prints load errors. It stays quiet about pubkeys only because RepositoryError keeps a database error's message and drops its detail, where Postgres puts the failing row. Pin that with a real CHECK violation and say why at the conversion. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
Say which connection the loader needs (the migration job's direct one), and have the operator confirm the tenant and match the dry run's unique count against the roster builder's before applying. Explain that the endpoint's day-long Cache-Control can keep showing false after a load, so the spot-check bypasses caches. Add a transactional removal for a bad load, and say which deletions remove a grant on their own and which do not. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
The deletion comments and runbook described the coordinator's call as following a "vanish" and told operators a grantee could keep their row until it was removed by hand. The coordinator actually calls service deletion for every account deleted in the Divine app, including people Keycast never held an account for, so deletion in the app is what removes the grant and no account holder needs a manual step. The removal SQL is now framed only as an operator undo for a bad load, and the plan records the deletion requirement added after review. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
A load would grant someone who had already deleted their account, and nothing would ever take the grant away: the deletion coordinator does not call Keycast again for a completed deletion, and a replayed request id is answered from the stored record. The load now skips any roster pubkey with a service_account_deletions row in the same tenant, in the dry run and the apply alike, and revokes a grant such a pubkey already holds. The report counts them as "account deleted" (and "grants revoked"), so unique pubkeys still equal newly + already + excluded. Checking the records is not enough on its own. A deletion deletes the grant and records itself in one transaction, the load reads records and inserts grants in another, and under READ COMMITTED neither sees the other's uncommitted rows, so a deletion running alongside a load could miss the grant while the load missed the deletion. A sweep after the insert narrows that window but does not close it: a deletion whose delete ran before the insert committed but which commits after the sweep still escapes both. So both sides take one tenant-scoped advisory lock: account deletion shared, so deletions never wait on each other, and the load exclusive, for its single transaction. A load then waits for deletions in flight and sees their records, and a later deletion waits for the load and sees its grant. One lock rather than one per pubkey, so a large roster cannot exhaust the lock table, and the load stays one transaction with one created_at. The apply also refuses to commit if its counts ever fail to add up to the roster, instead of wrapping around in a release build. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
delete_account's step list said the grant is removed even when no account row exists. On that path delete_account returns NotFound without committing, so the removal rolls back. Say it only where it holds: delete_account_in_tx, whose caller can commit an AlreadyAbsent outcome, as service deletion does when it records already_absent. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
The runbook pointed operators at "the migration job's direct connection", but production's migrate job reaches Cloud SQL through the Auth Proxy with IAM auth, so there was no string to copy, and putting one inline leaves it in shell history. It also implied an old roster could be re-run safely and that in-app deletion covers every deletion, and its undo SQL used a hand-typed time window and a count check that legitimately fails once grantees delete their accounts. Say to build the roster immediately before loading, since Keycast's deletion records can be compacted after 30 days; give the proxy and export steps; show headers on the spot-check; match the undo on the exact created_at and check it against the SELECT's count; and state plainly that a deletion made only from another Nostr app never reaches Keycast, which is accepted. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
Moves the tenant grant lock into groups/service.rs next to lock_group_account and takes it in delete_account_in_tx right after that lock, so lock order is explicit at both sites: deletion takes group-account then the grant lock; the load takes only the grant lock, so there is no cycle. One key per tenant rather than per pubkey, because a lock per roster pubkey held in one transaction could exhaust Postgres's server-wide lock table. Adds the reverse-order test, a deletion arriving while a load holds the lock, through the real deletion path. Tradeoff: while a load holds the exclusive lock (a few seconds at most), account deletions in that tenant wait for it. The dry run takes no lock. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
Tasks 1-3 shipped earlier on this branch but were still unchecked. Task 5 records the review-driven change that keeps a load from granting anyone whose account deletion Keycast has recorded, and the tenant grant lock that keeps a load and a deletion from missing each other. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
Compaction deleted a deletion record under only its per-request lock, so it could land between a load's insert, sweep and count. The load then reported that pubkey as already granted and could skip revoking a grant whose deletion was recorded when it started. Compaction now takes the tenant's grant lock shared, after its deletion lock; the load takes only the grant lock, so there is no cycle. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
…keys Keycast's deletion records miss compacted deletions and accounts deleted through DELETE /api/user/account, which writes no record. Say so in the runbook and in the deletion lock comment, and list every table the loader's database identity needs.
It purged a key another loader test grants and counts. #[serial] only orders tests within one process, so a process-per-test runner could race them.
A commit can land on the server and still return an error if the connection drops before the reply. Point the operator at the dry run, which shows what landed.
While a load holds or waits for the tenant's grant lock, every account deletion there waits too, and nothing limited that wait: a deletion that never finished, or an operator's stalled session, could block them indefinitely. The load now sets transaction-local lock, statement and idle-in-transaction timeouts, and gives up writing nothing. It also pins READ COMMITTED, since a REPEATABLE READ default would take its snapshot before the lock wait and miss deletions committed during it, and runs the insert, revoke and count as one statement, so the roster is sent once and the counts share one snapshot. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
Every database seeds tenant 1 as login.divine.video, so the tenant line cannot tell production from staging or a local database. The loader now prints the database, user and server address before loading, and names its session so an operator can find and end a stuck load. The runbook gives the proxy an explicit port, the IAM connection string shape, and what to do if a load stalls. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
The tenant lock kept a one-time grant load and account deletion from missing each other, but it put every account deletion behind that load, and needed timeouts and an isolation pin to keep that wait bounded. The failure it prevented is small: a grant left on an account deleted in the seconds a load is writing. The runbook now has the operator re-run the dry run straight after the load and apply again if it reports anything to revoke; the existing revoke step removes those grants. The dry run now joins against the same "deleted" set the load uses. Its per-row EXISTS was inlined into each count and scanned the deletion records once per roster row per count, which took minutes at full size. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
…n records No test failed if the load's revoke lost its tenant filter, which would delete other tenants' grants, and nothing covered a pubkey with two deletion records. The runbook's post-load re-check now uses the same roster file, adds a next-day check, and says it cannot see a self-deletion that raced the load. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
The runbook's post-load re-check re-applied the roster. Re-applying also re-grants anyone whose grant was removed since by a deletion that left no record, such as a self-deletion through DELETE /api/user/account, so the check could put a badge back on a deleted account for good. It is replaced by a cleanup statement that only removes grants whose account has a recorded deletion, run straight after the load and again the next day. The two newest loader tests get their own key prefix, so a test runner that runs tests in separate processes cannot have them purge each other's rows. Claude-Session: https://claude.ai/code/session_013BpfRgy8mDVdoVjg7ms53Y
|
@dcadenas once you've reviewed this and you're happy with it, the rest needs your prod access. the full steps are in docs/DEPLOYMENT.md under "OG Diviner grants", short version:
if you'd rather do any of this differently, tell me here and we'll adjust. |
dcadenas
left a comment
dcadenas
left a comment
There was a problem hiding this comment.
The tenant-scoped grant path covers early app posters without a Keycast signup while preserving the existing eligibility rule.
Reviewed cbb39a2 with no blocking findings and no branch changes.
Checks
- Formatting and workspace all-target, all-feature Clippy passed.
bun run test:deploy-preflightpassed.bun run test:cipassed against isolated local Postgres and Redis: 773 nextest passes with 22 skips, followed by overlapping integration suites with zero failures and seven existing ignored tests.- The 12 grant-loader tests passed, including tenant isolation, deletion exclusion, revocation and dry-run counts.
- Current-head CI passed Rust tests, Web Tests and the semantic title check. Image publication and deployment were skipped for the PR.
- Local web typechecking, web unit tests, browser checks and E2E were not run. There are no frontend changes.
Accepted limits and handback
The one-time load does not lock account deletion.
The documented revoke-only cleanup addresses recorded deletions; unrecorded self-deletions and deletions made only through other apps remain explicit limits.
@mbradley the code review is complete without a push or behavior changes.
Production roster generation, loading and spot checks have not been performed; those remain separate operational work, not conditions on this code approval.
Merge remains with Daniel.
Agent-Model: openai/gpt-6.1-sol
Agent-Effort: medium
Agent-Harness: opencode/unknown
Agent-Context: fresh
Initiated-By: @dcadenas
|
I approved the code here, but I'd like to hold the merge and keep this data out of Keycast. TL;DR
Proposal
The roster work is the same as in this PR. Wrong owner: why this belongs with the profileEvery pubkey that posts is in Funnelcake. Only custodial accounts are in Keycast. A test for what Keycast should store: would this fact exist for someone who never used Keycast?
This PR shows the mismatch in its own code:
Profile reads compete with signing: the shared poolKeycast is sized for signing bursts. Signing leans on that pool hardest when it is busiest. Measured on production:
Its load follows profile views, not logins or signing. Caching it in Keycast would duplicate FunnelcakeThe two domains answer very differently today:
Caching Funnelcake already has all of that, and already serves badges: NIP-58 badges at Notes for the roster query
Anyone the query misses can get a manual exception: the badge is cosmetic and the cohort is closed. |
Holding the merge until we agree where this data should live. Details: #443 (comment)
rabble
left a comment
rabble
left a comment
There was a problem hiding this comment.
Advisory review of current feedback and next action at cbb39a2.
The outstanding blocker is the ownership decision recorded in the maintainer comment, not a failing check or an unanswered inline code-fix request. The prior approval was deliberately dismissed with the reason “Holding the merge until we agree where this data should live.” There are no review threads or inline comments to resolve.
Change made: added a prominent hold notice to the PR description, preserving the existing description below it. It now distinguishes the existing implementation/rollout proposal from authorization to merge, deploy, generate a production roster or load it. The source branch and head are unchanged; no code or migration was pushed.
Verified: current-head CI ran and passed Rust tests, clippy, formatting and Web Tests; image publication/deployment were skipped. The semantic check rerun after the description edit passed. Local cargo fmt --all -- --check and cargo clippy --workspace --all-targets --all-features -- -D warnings -A deprecated also passed in an isolated checkout. No local database integration suite or production operation was run. GitHub's effective main rules require one approving review; the approval is currently dismissed. Another approval must not be used to bypass the explicit hold.
Next action: Rabble, dcadenas and mbradley need to settle whether eligibility moves to Funnelcake's private profile/read path with a mobile migration and a compatibility period for the Keycast endpoint. That proposal changes service ownership and client behavior, so I have asked for that specific decision in the active session rather than implementing it implicitly. Until agreed, keep this PR unmerged and do not run its production loading procedure.
This is an advisory feedback/next-action review, not a renewed whole-diff code approval. The ownership proposal remains unresolved; no approval or request-changes verdict is being substituted for that decision.
Agent-Model: openai/gpt-6[exact-checkpoint-unknown]
Agent-Effort: unknown
Agent-Harness: codex/unknown
Agent-Context: shared
Initiated-By: @rabble
|
Prepared the Funnelcake migration proposal in #448 (documentation commit 1f894be). #443 remains held. Its source head is still @dcadenas review routed to you on #448. @mbradley please review its legacy-rule parity and older-client compatibility contract. The proposed direction preserves the existing account predicate, moves the private cohort/boolean serving to Funnelcake, and makes mobile/cache/deletion migration explicit. Two activation decisions need evidence: whether retained relay arrival data can establish the historical poster cohort, and how to account for past direct Keycast deletions that a future outbox cannot recover retroactively. The proposal also distinguishes pending vanish filters from durable deletion history and records the cache limit of existing mobile binaries. No production data was accessed for this work. Local formatting and strict workspace Clippy passed. Local workspace tests reached two secret-pool timing failures, reproduced serially; measured cost-10 hashes took 9.79s/9.61s against a 1s pool wait on the contended host. The extra standalone build was stopped for disk pressure. These gaps are recorded in #448; its current-head CI passed on |
Relates to #440
What this solves
The OG Diviner badge goes to people who joined Divine before invite codes were removed. Since #416, the only evidence Keycast checks is its own signup records. People who used the Divine app before the cutoff with a Nostr key they created or brought themselves never had a Keycast signup, so they get
eligible: falseeven when they have been posting since before launch. A user in that position wrote in, and Rabble agreed we should cover this group (#440).Why this approach
The group is closed, since nobody new can have posted before the cutoff, so it is found once from production relay data and loaded once, using the criteria in #440. Keycast keeps the list in a new per-tenant table and checks it as one more way to qualify. The list itself is private and never committed, in keeping with #416's aim of keeping the cohort out of public view. The loader prints counts only, never pubkeys.
What it deliberately does not change
DELETE /api/user/accountroute leaves no deletion record. So whoever builds the list must drop accounts that have vanished on the relay (NIP-62), and the post-load cleanup cannot catch such a deletion if it races the load.Migration:
20261023120000_add_og_diviner_grantsadds one empty table. Nothing else in the schema changes.Deploying
Ship the code. Loading the list is a separate, one-time production step, described in docs/DEPLOYMENT.md under "OG Diviner grants": build the list right before loading, dry run, check the database line and the counts, apply, then run the cleanup.
How it was verified