From 9f91e9e26b5fedc30805cea266cd64a98920a15e Mon Sep 17 00:00:00 2001 From: Cody Schaefer Date: Fri, 21 Aug 2026 15:36:44 -0500 Subject: [PATCH] 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) --- docs/waves/backlog.md | 19 ++++++------ .../b7e4f1a20c93_drop_user_password_hash.py | 30 ++++++++++++++----- server/notify.py | 16 ---------- 3 files changed, 31 insertions(+), 34 deletions(-) diff --git a/docs/waves/backlog.md b/docs/waves/backlog.md index c932fe5..2e1aaba 100644 --- a/docs/waves/backlog.md +++ b/docs/waves/backlog.md @@ -555,7 +555,7 @@ deliberately deferred. - **Suggested wave or follow-up:** next housekeeping pass, with the check 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 - **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. `T10.3` deleted that endpoint, so the function now has no callers anywhere in `server/` or `tests/` — verified by grep, not assumed. -- **Why not now:** deleting it is a drive-by CLAUDE.md forbids, and it is not - obviously dead weight: an immediate, unqueued send is the right primitive for any - future time-sensitive mail, and the queue is the wrong shape for that. Deciding - between "delete it" and "keep it as the documented immediate-send path" is a - judgement call that deserves its own entry rather than being settled inside an - auth task. -- **Suggested wave or follow-up:** next housekeeping pass. If kept, its docstring - needs rewriting — it currently explains itself in terms of password resets, which - no longer exist. +- **Resolution:** deleted. Raised as a judgement call between "remove it" and "keep + it as the documented immediate-send path"; answered on Aug 21 — remove it. Nothing + in `server/` or `tests/` referenced it, and its docstring explained itself entirely + in terms of password resets, which no longer exist. Keeping an unused sender that + bypasses the outbox is a liability, not an asset: the next person to need immediate + mail should write it against the requirement they actually have. +- **Note:** `send_email` (the raw SMTP call it wrapped) is untouched and still used by + the outbox. ### BL-027 — Okta exists on this estate; OIDC is a live alternative to the LDAPS bind diff --git a/server/alembic/versions/b7e4f1a20c93_drop_user_password_hash.py b/server/alembic/versions/b7e4f1a20c93_drop_user_password_hash.py index 5515670..9013823 100644 --- a/server/alembic/versions/b7e4f1a20c93_drop_user_password_hash.py +++ b/server/alembic/versions/b7e4f1a20c93_drop_user_password_hash.py @@ -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 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 Revises: a1b8c6d4e2f9 Create Date: 2026-08-21 @@ -29,14 +46,11 @@ depends_on = None def upgrade(): - # batch_alter_table for SQLite's benefit: it has no DROP COLUMN before 3.35, - # so alembic rebuilds the table. Postgres drops it directly. Local dev runs on - # SQLite and production on Postgres, so this has to work on both. - with op.batch_alter_table('users') as batch: - batch.drop_column('password_hash') + # Plain, un-batched, on both engines. See the docstring: batching this destroys + # project_members on SQLite. + op.drop_column('users', 'password_hash') def downgrade(): - with op.batch_alter_table('users') as batch: - batch.add_column(sa.Column('password_hash', sa.String(length=200), - nullable=True)) + op.add_column('users', sa.Column('password_hash', sa.String(length=200), + nullable=True)) diff --git a/server/notify.py b/server/notify.py index 8baadab..4be476f 100644 --- a/server/notify.py +++ b/server/notify.py @@ -121,22 +121,6 @@ def send_email(s: dict, to_addr: str, subject: str, body: str) -> None: 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, 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 +