Fix orphaned users when registration partially fails - #5915
Conversation
Signup: post-user-insert + session-insert + CREATE MATERIALIZED VIEW
all ran un-transacted. Any failure between them (DB blip, view
collision from a prior failed attempt, container kill, max_connections,
etc.) left the users and session_ids rows orphaned and the materialized
view missing. Re-registering the same email then 500'd on the
view-create collision. Logged-in users hit 500 on /feed/subscriptions
because the view lookup referenced a non-existent relation.
Same pattern in account-delete (Users.delete + SessionIDs.delete +
DROP MATERIALIZED VIEW).
Fix: add an optional `conn` parameter (DB::Database | DB::Connection |
DB::Transaction, default PG_DB) to Users.insert/delete and all four
SessionIDs overloads, then wrap the create/delete call sites in
PG_DB.transaction { |tx| ... }. Default arg keeps every existing
call site working unchanged.
Verified on a fresh local stack:
- registration of a clean user: user + session + view all created
- registration with a pre-existing view: 500, zero rows in users and
session_ids (rollback worked), pre-existing view untouched
Closes iv-org#2509
📝 WalkthroughWalkthroughThe change adds optional connection parameters to session and user database methods. Login account creation and account deletion now run related database operations inside single transactions. ChangesTransactional database operations
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LoginRoute
participant AccountRoute
participant PGDBTransaction
participant UsersDatabase
participant SessionsDatabase
participant SubscriptionMaterializedView
LoginRoute->>PGDBTransaction: start transaction
PGDBTransaction->>UsersDatabase: insert user using conn
PGDBTransaction->>SessionsDatabase: insert session using conn
PGDBTransaction->>SubscriptionMaterializedView: create view using conn
AccountRoute->>PGDBTransaction: start transaction
PGDBTransaction->>UsersDatabase: delete user using conn
PGDBTransaction->>SessionsDatabase: delete session using conn
PGDBTransaction->>SubscriptionMaterializedView: drop view using conn
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/invidious/database/sessions.cr`:
- Around line 10-49: Remove DB::Transaction from the conn type unions in the
insert and delete methods in src/invidious/database/sessions.cr (lines 10-49)
and the corresponding methods in src/invidious/database/users.cr (lines 10-35),
leaving DB::Database | DB::Connection; transaction callers should continue
passing tx.connection.
In `@src/invidious/routes/login.cr`:
- Around line 125-131: Move the `PREFS` assignment before the
`PG_DB.transaction` block so `Invidious::Database::Users.insert` persists
preferences atomically with the user, session, and materialized view. Remove the
post-transaction `Users.update_preferences(user)` call, leaving only cookie
expiration after the commit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc149369-d55c-46f1-ba69-3413b9b15324
📒 Files selected for processing (4)
src/invidious/database/sessions.crsrc/invidious/database/users.crsrc/invidious/routes/account.crsrc/invidious/routes/login.cr
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
iv-org/invidious(manual)iv-org/invidious-companion(manual)iv-org/mocks(manual)iv-org/documentation(manual)
| def insert(sid : String, email : String, handle_conflicts : Bool = false, *, conn : DB::Database | DB::Connection | DB::Transaction = PG_DB) | ||
| request = <<-SQL | ||
| INSERT INTO session_ids | ||
| VALUES ($1, $2, now()) | ||
| SQL | ||
|
|
||
| request += " ON CONFLICT (id) DO NOTHING" if handle_conflicts | ||
|
|
||
| PG_DB.exec(request, sid, email) | ||
| conn.exec(request, sid, email) | ||
| end | ||
|
|
||
| # ------------------- | ||
| # Delete | ||
| # ------------------- | ||
|
|
||
| def delete(*, sid : String) | ||
| def delete(*, sid : String, conn : DB::Database | DB::Connection | DB::Transaction = PG_DB) | ||
| request = <<-SQL | ||
| DELETE FROM session_ids * | ||
| WHERE id = $1 | ||
| SQL | ||
|
|
||
| PG_DB.exec(request, sid) | ||
| conn.exec(request, sid) | ||
| end | ||
|
|
||
| def delete(*, email : String) | ||
| def delete(*, email : String, conn : DB::Database | DB::Connection | DB::Transaction = PG_DB) | ||
| request = <<-SQL | ||
| DELETE FROM session_ids * | ||
| WHERE email = $1 | ||
| SQL | ||
|
|
||
| PG_DB.exec(request, email) | ||
| conn.exec(request, email) | ||
| end | ||
|
|
||
| def delete(*, sid : String, email : String) | ||
| def delete(*, sid : String, email : String, conn : DB::Database | DB::Connection | DB::Transaction = PG_DB) | ||
| request = <<-SQL | ||
| DELETE FROM session_ids * | ||
| WHERE id = $1 AND email = $2 | ||
| SQL | ||
|
|
||
| PG_DB.exec(request, sid, email) | ||
| conn.exec(request, sid, email) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI '^(shard\.yml|shard\.lock)$' . -0 |
xargs -0 -r rg -n -C2 'crystal-db|^dependencies:|^ db:'
rg -n -C2 'DB::Database \| DB::Connection \| DB::Transaction|conn\.exec|conn: tx\.connection' \
src/invidious/database src/invidious/routesRepository: iv-org/invidious
Length of output: 16615
🏁 Script executed (no clone):
set -euo pipefail
for ref in v0.13.1 0.13.1; do
url="https://raw.githubusercontent.com/crystal-lang/crystal-db/${ref}/src/db/transaction.cr"
echo "=== ${url} ==="
curl -fsSL "$url" | sed -n '1,220p'
done
Length of output: 4075
🏁 Script executed:
set -euo pipefail
printf '%s\n' '=== database method signatures and transaction call sites ==='
rg -n -C3 'def (insert|delete)|DB::Transaction|tx\.connection|Users\.(insert|delete)|SessionIDs\.(insert|delete)' \
src/invidious/database/sessions.cr \
src/invidious/database/users.cr \
src/invidious/routes
printf '%s\n' '=== local crystal-db sources, if present ==='
fd -HI 'transaction\.cr|database\.cr' . | head -80Repository: iv-org/invidious
Length of output: 7109
Remove DB::Transaction from the conn unions.
DB::Transaction exposes connection, not exec. The conn.exec calls in src/invidious/database/sessions.cr and src/invidious/database/users.cr therefore fail union method resolution. Use DB::Database | DB::Connection; transaction callers already pass tx.connection.
📍 Affects 2 files
src/invidious/database/sessions.cr#L10-L49(this comment)src/invidious/database/users.cr#L10-L35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/invidious/database/sessions.cr` around lines 10 - 49, Remove
DB::Transaction from the conn type unions in the insert and delete methods in
src/invidious/database/sessions.cr (lines 10-49) and the corresponding methods
in src/invidious/database/users.cr (lines 10-35), leaving DB::Database |
DB::Connection; transaction callers should continue passing tx.connection.
| PG_DB.transaction do |tx| | ||
| Invidious::Database::Users.insert(user, conn: tx.connection) | ||
| Invidious::Database::SessionIDs.insert(sid, email, conn: tx.connection) | ||
|
|
||
| view_name = "subscriptions_#{sha256(user.email)}" | ||
| PG_DB.exec("CREATE MATERIALIZED VIEW #{view_name} AS #{MATERIALIZED_VIEW_SQL.call(user.email)}") | ||
| view_name = "subscriptions_#{sha256(user.email)}" | ||
| tx.connection.exec("CREATE MATERIALIZED VIEW #{view_name} AS #{MATERIALIZED_VIEW_SQL.call(user.email)}") | ||
| end |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include the preference write in the registration transaction.
If Invidious::Database::Users.update_preferences(user) raises at Line 141, this block has already committed the user, session, and materialized view. The route then fails after partial registration persistence.
Set the PREFS preferences before Line 125 so Users.insert persists them in this transaction. Keep only cookie expiration after the commit.
Proposed fix
+ prefs_cookie = env.request.cookies["PREFS"]?
+ user.preferences = env.get("preferences").as(Preferences) if prefs_cookie
+
PG_DB.transaction do |tx|
Invidious::Database::Users.insert(user, conn: tx.connection)
Invidious::Database::SessionIDs.insert(sid, email, conn: tx.connection)
view_name = "subscriptions_#{sha256(user.email)}"
tx.connection.exec("CREATE MATERIALIZED VIEW #{view_name} AS #{MATERIALIZED_VIEW_SQL.call(user.email)}")
end
- if env.request.cookies["PREFS"]?
- user.preferences = env.get("preferences").as(Preferences)
- Invidious::Database::Users.update_preferences(user)
-
- cookie = env.request.cookies["PREFS"]
+ if prefs_cookie
+ cookie = prefs_cookie
cookie.expires = Time.utc(1990, 1, 1)
env.response.cookies << cookie
end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/invidious/routes/login.cr` around lines 125 - 131, Move the `PREFS`
assignment before the `PG_DB.transaction` block so
`Invidious::Database::Users.insert` persists preferences atomically with the
user, session, and materialized view. Remove the post-transaction
`Users.update_preferences(user)` call, leaving only cookie expiration after the
commit.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| PG_DB.transaction do |tx| | ||
| Invidious::Database::Users.delete(user, conn: tx.connection) | ||
| Invidious::Database::SessionIDs.delete(email: user.email, conn: tx.connection) | ||
| tx.connection.exec("DROP MATERIALIZED VIEW #{view_name}") |
There was a problem hiding this comment.
When an existing user’s subscription materialized view is already absent, this unconditional DROP MATERIALIZED VIEW raises within the surrounding transaction. PostgreSQL then rolls back the preceding user and session deletes, so the account-deletion request fails and the account remains active. Make the cleanup idempotent with DROP MATERIALIZED VIEW IF EXISTS #{view_name}.
There was a problem hiding this comment.
Pull request overview
This PR improves database consistency during user registration and account deletion by wrapping multi-step writes (user row, session row, and subscriptions materialized view DDL) in a single Postgres transaction, preventing partial-failure corruption/orphaned records.
Changes:
- Wrap user registration writes in
PG_DB.transaction, ensuringusers,session_ids, and the subscriptions materialized view are created atomically. - Wrap account deletion writes in
PG_DB.transaction, ensuringusers,session_ids, and the subscriptions materialized view are dropped atomically. - Extend
Users.insert/Users.deleteandSessionIDs.insert/SessionIDs.deleteoverloads to accept an optionalconnparameter so callers can reuse a transactional connection.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/invidious/routes/login.cr | Wraps registration DB writes + view creation in a single transaction and threads the transactional connection through. |
| src/invidious/routes/account.cr | Wraps account deletion DB deletes + view drop in a single transaction and threads the transactional connection through. |
| src/invidious/database/users.cr | Adds optional conn parameter so user insert/delete can run on a provided connection/transaction. |
| src/invidious/database/sessions.cr | Adds optional conn parameter across session insert/delete overloads to support transactional call sites. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| PG_DB.transaction do |tx| | ||
| Invidious::Database::Users.delete(user, conn: tx.connection) | ||
| Invidious::Database::SessionIDs.delete(email: user.email, conn: tx.connection) | ||
| tx.connection.exec("DROP MATERIALIZED VIEW #{view_name}") |
|
Hello @McoreD. Thanks for the contribution. I used a lot sharex in the past btw, thanks for creating it. Could you please modify your PR description and fill our PR template: https://github.com/iv-org/invidious/blob/master/.github/PULL_REQUEST_TEMPLATE.md Also, did you do any testing/verification yourself? Since you didn't fill up the template I assume an AI made the PR, did you supervise it? |
|
Hi @unixfox, I did supervise it and the PR was made as a result of my user experience while setting it up. I will comply with the PR requirements now, sorry, wasn't aware. |
|
@unixfox Thanks — glad ShareX has been useful. PR body now follows the template, with the AI Policy disclosure filled in (partial AI use, models + tools listed, human-supervised). Yes, supervised and tested on my side. I rebuilt the modified image against the upstream
Leaving the PR in draft until you've had a chance to look it over. |
McoreD
changed the title
Checklist
AI Disclosure
Model(s) used (and thinking/reasoning level if relevant):
Mixture-of-agents orchestration on the Hermes Agent runtime; primary contributing model was MiniMax-M3 (also routed through Grok-4.5 via xAI OAuth for cross-checks).
Tool(s) used:
Hermes Agent with the
local-git,docker,localhost-mgr, andghintegrations; local Postgres + Docker Compose for the verification environment; the officialdocker/Dockerfile(84codes/crystal:1.20.3-alpinebuilder) to compile the modified binary.How was AI used?
The AI agent authored the 4-file Crystal patch, built the
invidious:fix-transactionimage, ran the happy-path and forced-orphan-view regression against the local stack, and drafted this PR. The Human (McoreD) directed scope, owns the fork and PR account, reviewed the diff and the running service, and takes responsibility for the change per the AI Policy. The Human supervised a fresh failure-path recheck just before posting this update: see the Verification section for the numbers.Pull request description
Summary
Wrap the user create and delete flows in a single Postgres transaction so a partial failure cannot leave the database in a corrupted state.
Root cause
In
src/invidious/routes/login.cr(around lines 125-129), the signup path executed three independent writes:None of the three were inside a
PG_DB.transaction { ... }block, so each was its own atomic commit. If the third statement failed for any reason — view name colliding with a leftover from a prior failed attempt, a postgres connection blip, container OOM-kill between statements, etc. — theusersrow and thesession_idsrow were committed while the view was missing. The account was then broken in two ways: pages that read the view (e.g./feed/subscriptions) returned 500, and re-registering the same email 500'd on the view-create collision.The symmetric bug existed in
src/invidious/routes/account.cr(around lines 127-129):Users.delete+SessionIDs.delete+DROP MATERIALIZED VIEWwere also un-transacted.This is the same defect reported in #2509 (closed 2021 as "miss-configuration"; reporter's reproducer was the literal
relation "subscriptions_... already exists (PQ::PQError)stack from these lines).Fix
Add an optional
conn : DB::Database | DB::Connection | DB::Transaction = PG_DBparameter toUsers.insert,Users.delete, and all fourSessionIDsoverloads. The default keeps every existing call site working unchanged.Wrap the three statements at each call site in
PG_DB.transaction { |tx| ... }and passtx.connectionthrough:The
DB::Database | DB::Connection | DB::Transactionunion is required becausePG_DBis aDB::Database(parent type in crystal-db) whiletx.connectionyields aDB::Connection/DB::Transaction. Including all three covers every legitimate call site.Reproduction
docker compose up -d).<view>issubscriptions_<sha256_of_email>:/loginwith that email and a fresh password.usersandsession_idsfor that email.Before this fix: 500, but
usersandsession_idseach contain a row for the email, and the view that was supposed to be created is still missing.After this fix: 500, both tables empty for that email, the pre-existing view untouched.
Verification
Built
invidious:fix-transactionfrom this branch using the officialdocker/Dockerfileand ran both paths on the locally supervised stack (localhostmgr status invidious-> running, port 3030, failures 0):/feed/subscriptions,usersandsession_idseach contain one row for the email, matching materialized view present./login, observedHTTP 500 POST /login, then queried the database:Files changed
src/invidious/database/sessions.cr— addedconnparameter toinsertand threedeleteoverloads.src/invidious/database/users.cr— addedconnparameter toinsertanddelete.src/invidious/routes/login.cr— wrapped the create block inPG_DB.transaction.src/invidious/routes/account.cr— wrapped the delete block inPG_DB.transaction.23 insertions, 19 deletions. No other call sites modified — the default
connargument preserves backward compatibility.Closes #2509.