T10.3 fix - the drop migration was deleting every project membership

Found by actually seeding the pre-migration schema and rolling forward, rather
than by checking that the column disappeared. The column disappeared correctly;
project_members came back empty.

batch_alter_table emulates ALTER on SQLite by rebuilding the table - create a
new one, copy the rows, DROP the original, rename. server/alembic/env.py:22
imports the engine from server/db.py, which registers a connect listener
setting PRAGMA foreign_keys=ON, so that DROP TABLE cascaded through
project_members.user_id (ondelete="CASCADE") and took every membership row with
it. No error, nothing in the log, and the users table looked perfect
afterwards.

Production would have escaped it - Postgres does a real ALTER TABLE DROP COLUMN
and touches nothing else - so this was a local-dev and test-fixture data loss,
which is worse in one specific way: the tests CLAUDE.md requires run against a
throwaway SQLite database, so the suite would have been validating behaviour
against silently emptied membership tables.

Wrapping the batch in PRAGMA foreign_keys=OFF is not the fix: that pragma is a
no-op inside a transaction and alembic runs migrations in one. The rebuild is
simply unnecessary - SQLite has had native ALTER TABLE DROP COLUMN since 3.35
(2021), this runtime has 3.42, and Postgres has always had it. Plain
op.drop_column touches one table and cascades nowhere.

The reasoning is written into the migration's docstring as a DO NOT, because
batch_alter_table is the reflexive thing to reach for when a migration has to
work on SQLite and the failure is invisible.

Re-verified with memberships in the fixture:

  3/3 users survive, roles intact (admin still admin)
  2/2 project_members survive
  downgrade -1 -> column back, nullable; upgrade -> gone again

Also closed BL-026: notify.send_now removed. Nothing referenced it and its
docstring described itself entirely in terms of password resets. send_email,
which it wrapped, is untouched and still used by the outbox.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-08-21 15:36:44 -05:00
parent ab7bce9f5b
commit 9f91e9e26b
3 changed files with 31 additions and 34 deletions

View File

@@ -555,7 +555,7 @@ deliberately deferred.
- **Suggested wave or follow-up:** next housekeeping pass, with the check - **Suggested wave or follow-up:** next housekeeping pass, with the check
widened so it cannot recur. widened so it cannot recur.
### BL-026 — `notify.send_now` is orphaned once D13 removes password reset ### BL-026 — CLOSED 2026-08-21 (removed; nothing referenced it)
- **Found during:** `T10.3` (D13), stripping the password code paths - **Found during:** `T10.3` (D13), stripping the password code paths
- **Where:** `server/notify.py`, `send_now()` - **Where:** `server/notify.py`, `send_now()`
@@ -563,15 +563,14 @@ deliberately deferred.
only caller was `forgot_password`, because a reset link must not sit in a queue. only caller was `forgot_password`, because a reset link must not sit in a queue.
`T10.3` deleted that endpoint, so the function now has no callers anywhere in `T10.3` deleted that endpoint, so the function now has no callers anywhere in
`server/` or `tests/` — verified by grep, not assumed. `server/` or `tests/` — verified by grep, not assumed.
- **Why not now:** deleting it is a drive-by CLAUDE.md forbids, and it is not - **Resolution:** deleted. Raised as a judgement call between "remove it" and "keep
obviously dead weight: an immediate, unqueued send is the right primitive for any it as the documented immediate-send path"; answered on Aug 21 — remove it. Nothing
future time-sensitive mail, and the queue is the wrong shape for that. Deciding in `server/` or `tests/` referenced it, and its docstring explained itself entirely
between "delete it" and "keep it as the documented immediate-send path" is a in terms of password resets, which no longer exist. Keeping an unused sender that
judgement call that deserves its own entry rather than being settled inside an bypasses the outbox is a liability, not an asset: the next person to need immediate
auth task. mail should write it against the requirement they actually have.
- **Suggested wave or follow-up:** next housekeeping pass. If kept, its docstring - **Note:** `send_email` (the raw SMTP call it wrapped) is untouched and still used by
needs rewriting — it currently explains itself in terms of password resets, which the outbox.
no longer exist.
### BL-027 — Okta exists on this estate; OIDC is a live alternative to the LDAPS bind ### BL-027 — Okta exists on this estate; OIDC is a live alternative to the LDAPS bind

View File

@@ -15,6 +15,23 @@ The column is recreated NULLABLE on downgrade, deliberately. The baseline schema
declared it NOT NULL, but there are no values to put back, so a NOT NULL column declared it NOT NULL, but there are no values to put back, so a NOT NULL column
with no server default would refuse to add itself on any table that has rows. with no server default would refuse to add itself on any table that has rows.
DO NOT WRAP THIS IN batch_alter_table. An earlier version of this migration did,
and it silently deleted every row of `project_members` on SQLite.
Why: alembic's batch mode emulates ALTER on SQLite by rebuilding the table —
create a new one, copy the rows, DROP the original, rename. `server/alembic/env.py`
imports the engine from `server/db.py`, which registers a `connect` listener setting
`PRAGMA foreign_keys=ON`, so that DROP TABLE cascades through
`project_members.user_id`, which is declared `ondelete="CASCADE"`. Every project
membership in the database goes with it, with no error and nothing in the log.
Turning the pragma off around the batch is not a fix either: `PRAGMA foreign_keys`
is a no-op inside a transaction, and alembic runs migrations in one.
The real answer is that the rebuild is unnecessary. SQLite gained native
ALTER TABLE ... DROP COLUMN in 3.35 (2021); this runtime has 3.42 and Postgres has
always had it. A plain drop_column touches one table and cascades nowhere.
Revision ID: b7e4f1a20c93 Revision ID: b7e4f1a20c93
Revises: a1b8c6d4e2f9 Revises: a1b8c6d4e2f9
Create Date: 2026-08-21 Create Date: 2026-08-21
@@ -29,14 +46,11 @@ depends_on = None
def upgrade(): def upgrade():
# batch_alter_table for SQLite's benefit: it has no DROP COLUMN before 3.35, # Plain, un-batched, on both engines. See the docstring: batching this destroys
# so alembic rebuilds the table. Postgres drops it directly. Local dev runs on # project_members on SQLite.
# SQLite and production on Postgres, so this has to work on both. op.drop_column('users', 'password_hash')
with op.batch_alter_table('users') as batch:
batch.drop_column('password_hash')
def downgrade(): def downgrade():
with op.batch_alter_table('users') as batch: op.add_column('users', sa.Column('password_hash', sa.String(length=200),
batch.add_column(sa.Column('password_hash', sa.String(length=200), nullable=True))
nullable=True))

View File

@@ -121,22 +121,6 @@ def send_email(s: dict, to_addr: str, subject: str, body: str) -> None:
srv.send_message(msg) srv.send_message(msg)
def send_now(db: Session, to_addr: str, subject: str, body: str) -> bool:
"""Send one email immediately, outside the outbox. Used for password resets —
a reset link must never sit in a queue, and it must not be persisted in the
notifications table where an admin could read it and take over the account.
Returns True if it went out."""
s = get_settings(db)
if not (s.get("email_enabled") and smtp_ready(s) and to_addr):
return False
try:
send_email(s, to_addr, subject, body)
return True
except Exception as e: # noqa: BLE001 — never surface SMTP detail to the caller
log.warning("password-reset email to %s failed: %s", to_addr, e)
return False
def enqueue(db: Session, *, user: "models.User", kind: str, subject: str, body: str, def enqueue(db: Session, *, user: "models.User", kind: str, subject: str, body: str,
link: str = "", wp_id: Optional[str] = None, project_id: Optional[str] = None) -> "models.Notification": link: str = "", wp_id: Optional[str] = None, project_id: Optional[str] = None) -> "models.Notification":
"""Record a notification. Marked 'pending' only if email is enabled + SMTP ready + """Record a notification. Marked 'pending' only if email is enabled + SMTP ready +