From 73ad25a15a7d6e3b0877b71c26dd170dd36c18b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 16:15:56 +0000 Subject: [PATCH] Fix the start-up crash Phase C shipped, and test for it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The board would not start against any existing database, so Caddy had nothing to proxy to and answered 502. Reproduced by rebuilding the server's database from the previous schema and starting the app the way gunicorn does. The cause is one line of schema.sql: CREATE INDEX IF NOT EXISTS attachments_message ON attachments(message_id) IF NOT EXISTS guards the index NAME, not the column. On a database whose attachments table predates message_id, executescript dies with "no such column" — before any migration could add it, because init_db runs the schema first by design. Every index on a column a migration introduces belongs in the migration, after the column exists. It now lives there, outside the rebuild branch so a freshly created database gets it too. Second bug, latent and worse: the rebuild set PRAGMA foreign_keys = OFF inside the transaction, where it is documented to be a no-op. It looked applied and did nothing. Moved outside BEGIN. The test fixture had been passing for the wrong reason — it left foreign keys at SQLite's default, which is off — and now turns them on as connect() does. Both are the same mistake in different clothes: the tests exercised migrations.apply directly, so nothing ever ran init_db against a database from before the change. That test exists now. It builds the previous schema, inserts the row the server actually has, starts the app, and asserts the photo survived and a restart is a no-op. Verified by reinstating the bad line and watching it fail with the same error the server gave. 230 tests. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NizVpJ2dwzCbjCrTLCjeHn --- apps/board/migrations.py | 20 ++++++---- apps/board/schema.sql | 7 +++- apps/board/tests/test_migrations.py | 60 +++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 8 deletions(-) diff --git a/apps/board/migrations.py b/apps/board/migrations.py index 6a951c1..3fc9622 100644 --- a/apps/board/migrations.py +++ b/apps/board/migrations.py @@ -45,11 +45,7 @@ def _attachments_accept_messages(db: sqlite3.Connection) -> None: on can cascade, and switched back on after, with a check that nothing was broken in between. """ - if "message_id" in _columns(db, "attachments"): - return # a new database: schema.sql already created it this way - - db.execute("PRAGMA foreign_keys = OFF") - try: + if "message_id" not in _columns(db, "attachments"): db.execute(""" CREATE TABLE attachments_new ( id INTEGER PRIMARY KEY, @@ -83,8 +79,11 @@ def _attachments_accept_messages(db: sqlite3.Connection) -> None: broken = db.execute("PRAGMA foreign_key_check").fetchall() if broken: raise RuntimeError(f"migration left dangling references: {broken}") - finally: - db.execute("PRAGMA foreign_keys = ON") + + # Outside the branch above: a database created fresh by schema.sql has the + # column but not this index, because schema.sql cannot carry it — see the + # note there. Both paths end up with the same table and the same indexes. + db.execute("CREATE INDEX IF NOT EXISTS attachments_message ON attachments(message_id)") # (number, description, function). The number is the value written to @@ -101,6 +100,11 @@ def apply(db: sqlite3.Connection) -> list[str]: for number, description, step in sorted(STEPS): if number <= version: continue + # Outside the transaction, deliberately: "PRAGMA foreign_keys is a + # no-op within a transaction". Setting it inside BEGIN looks like it + # worked and changes nothing, which is how a table rebuild ends up + # running with enforcement still on. + db.execute("PRAGMA foreign_keys = OFF") db.execute("BEGIN IMMEDIATE") try: step(db) @@ -111,5 +115,7 @@ def apply(db: sqlite3.Connection) -> list[str]: except Exception: db.execute("ROLLBACK") raise + finally: + db.execute("PRAGMA foreign_keys = ON") done.append(f"{number}: {description}") return done diff --git a/apps/board/schema.sql b/apps/board/schema.sql index e1964ff..41d53cb 100644 --- a/apps/board/schema.sql +++ b/apps/board/schema.sql @@ -137,7 +137,12 @@ CREATE TABLE IF NOT EXISTS attachments ( CREATE INDEX IF NOT EXISTS attachments_thread ON attachments(thread_id); CREATE INDEX IF NOT EXISTS attachments_comment ON attachments(comment_id); -CREATE INDEX IF NOT EXISTS attachments_message ON attachments(message_id); +-- The index on message_id is NOT here, and that is not an oversight. +-- CREATE INDEX IF NOT EXISTS guards the index NAME, not the column: run it +-- against a database whose attachments table predates message_id and it fails +-- with "no such column", taking the whole start-up with it. Any index on a +-- column a migration introduces belongs in that migration, after the column +-- exists. See migrations.py. -- Gitea access tokens for the editor. -- diff --git a/apps/board/tests/test_migrations.py b/apps/board/tests/test_migrations.py index 5017ed9..7fbc848 100644 --- a/apps/board/tests/test_migrations.py +++ b/apps/board/tests/test_migrations.py @@ -9,6 +9,7 @@ never needed for. from __future__ import annotations import sqlite3 +from pathlib import Path import pytest @@ -42,6 +43,10 @@ def old_db(tmp_path): """A database as it existed before this migration, with real rows in it.""" db = sqlite3.connect(tmp_path / "old.db", isolation_level=None) db.row_factory = sqlite3.Row + # As connect() does in db.py. Without this the fixture runs with SQLite's + # default (off) and a migration that mishandles foreign keys passes here + # and fails on the server. + db.execute("PRAGMA foreign_keys = ON") db.executescript(OLD_SCHEMA) db.execute("INSERT INTO members (id, gitea_login) VALUES (1, 'salvador')") db.execute("INSERT INTO threads (id) VALUES (7)") @@ -126,3 +131,58 @@ def test_a_fresh_database_skips_it(app, db): done and return quietly rather than rebuilding a table it just created.""" assert "message_id" in {row[1] for row in db.execute("PRAGMA table_info(attachments)")} assert migrations.apply(db) == [] + + +# --- the whole start-up, not just the step ------------------------------- + +def test_the_app_starts_against_a_database_from_before_all_this(tmp_path): + """The test that was missing, and the reason the site went down. + + Every other test here calls `migrations.apply` directly. The failure was + one layer above it: `init_db` runs `schema.sql` *first*, and schema.sql + carried `CREATE INDEX ... ON attachments(message_id)`. IF NOT EXISTS guards + the index name, not the column — so against a real database the script died + with "no such column: message_id" before any migration could fix anything, + the app never finished starting, and Caddy answered 502. + + This builds a database with the schema as it shipped, puts a real row in + it, and starts the application the way gunicorn does. + """ + from apps.board.app import create_app + from apps.board.db import connect + + shipped = (Path(__file__).resolve().parents[3] / "apps/board/schema.sql") + old_sql = shipped.read_text(encoding="utf-8") + # Reduce it to the shape that predates this migration: no message_id + # anywhere, and the CHECK that goes with it. + old_sql = old_sql.replace(" message_id INTEGER REFERENCES messages(id),\n", "") + old_sql = old_sql.replace( + " CHECK ((thread_id IS NOT NULL) + (comment_id IS NOT NULL)\n" + " + (message_id IS NOT NULL) = 1)", + " CHECK ((thread_id IS NULL) <> (comment_id IS NULL))") + + path = str(tmp_path / "board.db") + db = connect(path) + db.executescript(old_sql) + db.execute("INSERT INTO members (gitea_login, display_name, role) " + "VALUES ('salvador', 'Salvador', 'user')") + db.execute("INSERT INTO threads (author_id, title, body_md) VALUES (1, 'Hola', 'T')") + db.execute("""INSERT INTO attachments + (thread_id, stored_name, original_name, content_type, bytes, uploaded_by) + VALUES (1, 'foto-abc123abc123.jpg', 'foto.jpg', 'image/jpeg', 2048, 1)""") + db.close() + + create_app({"SECRET_KEY": "x", "DB_PATH": path, "OWNER_LOGIN": "salvador", + "UPLOAD_DIR": str(tmp_path / "uploads"), "TESTING": True}) + + db = connect(path) + assert db.execute("PRAGMA user_version").fetchone()[0] == 1 + # The row the server actually has, still there and still whole. + kept = db.execute("SELECT stored_name, thread_id, bytes FROM attachments").fetchone() + assert (kept["stored_name"], kept["thread_id"], kept["bytes"]) == ( + "foto-abc123abc123.jpg", 1, 2048) + + # And starting again changes nothing, because a container restarts. + create_app({"SECRET_KEY": "x", "DB_PATH": path, "OWNER_LOGIN": "salvador", + "UPLOAD_DIR": str(tmp_path / "uploads"), "TESTING": True}) + db.close()