fix(db): survive a schema left without term_schedule (#275, #285)

CHANGELOG.md

@@ -77,6 +77,32 @@ ones are marked like "v1.0.0-fork".
 
 ### Fixed
 
+* **An incomplete schema no longer takes the review page and the vocabulary
+  list down with it** (#275, #285). The FSRS queue reads each term's due date
+  from `term_schedule`, and a table named in SQL cannot be guarded the way a
+  missing row can: on an install where that migration failed, MySQL rejected
+  the whole statement and both the review page and the entire term list
+  answered 500. They now fall back to the schedule a term's status implies —
+  which on such an install is the same answer, since no term there has ever
+  been graded — and start using the real schedule once the table is back.
+
+* **A failed migration gets retried again** (#285). A migration was only
+  reconsidered when an upgrade brought new files along, which on a fresh
+  install could never happen: the first run records every migration, so there
+  is never anything new afterwards and a migration that failed stayed failed at
+  one attempt, until some later release happened to add a file. Failures are
+  now retried on the next requests instead, up to three attempts, and an
+  upgrade that does bring new migrations restores that budget — so a migration
+  that failed on a prerequisite a later one repairs still gets its chance. This
+  is what left installs without `term_schedule` in the first place.
+
+* **Expected migration failures stopped being logged as failures** (#285). A
+  healthy fresh install wrote ~178 `Migration failed:` lines while every one of
+  its migrations succeeded: legacy migrations rename tables that a fresh
+  install never had, and those statements fail by design. The log said so only
+  after the fact, in wording identical to a real failure, which is what buried
+  the eight genuine errors of #275 among them. Expected failures now log as
+  skipped, and `Migration failed:` means it.
 * **A text that parses into nothing now says so** (#278). When a language's
   *Word Characters* setting does not match the script of its texts, parsing
   does not fail — it succeeds and produces nothing. The text saves, opens and

src/Modules/Review/Infrastructure/MySqlTermScheduleRepository.php

@@ -30,6 +30,10 @@ final class MySqlTermScheduleRepository implements TermScheduleRepositoryInterfa
 
     public function find(int $wordId): ?MemoryState
     {
+        if (!ScheduleSql::hasScheduleTable()) {
+            return null;
+        }
+
         $params = [$wordId];
         $scope = $this->appendUserScope($params);
 
@@ -85,6 +89,14 @@ public function findOrSeed(int $wordId): ?MemoryState
 
     public function saveReview(int $wordId, SchedulingResult $result, Rating $rating, int $stateBefore): void
     {
+        // Nowhere to write on a schema where the phase-2a migration did not
+        // run. Dropping the write rather than failing keeps the review session
+        // going: the term's status still moves, and the schedule starts being
+        // kept once the schema is repaired.
+        if (!ScheduleSql::hasScheduleTable()) {
+            return;
+        }
+
         // Ownership is checked once here rather than trusted from the caller,
         // so neither write below can touch a foreign term.
         if (!$this->ownsWord($wordId)) {
@@ -137,6 +149,10 @@ public function saveReview(int $wordId, SchedulingResult $result, Rating $rating
 
     public function countDue(?int $languageId = null): int
     {
+        if (!ScheduleSql::hasScheduleTable()) {
+            return 0;
+        }
+
         $params = [];
         $sql = 'SELECT COUNT(*) AS value
                 FROM term_schedule
@@ -175,7 +191,7 @@ private function ownsWord(int $wordId): bool
      */
     public function findMany(array $wordIds): array
     {
-        if ($wordIds === []) {
+        if ($wordIds === [] || !ScheduleSql::hasScheduleTable()) {
             return [];
         }
 
@@ -214,7 +230,7 @@ public function findMany(array $wordIds): array
      */
     public function historyFor(array $wordIds): array
     {
-        if ($wordIds === []) {
+        if ($wordIds === [] || !ScheduleSql::hasScheduleTable()) {
             return [];
         }
 

src/Modules/Review/Infrastructure/ScheduleSql.php

@@ -5,6 +5,7 @@
 namespace Lwt\Modules\Review\Infrastructure;
 
 use Lwt\Modules\Review\Domain\Scheduling\LegacyStatusSeed;
+use Lwt\Shared\Infrastructure\Database\Connection;
 
 /**
  * SQL for reading a term's due date from its FSRS schedule.
@@ -33,6 +34,11 @@
  */
 final class ScheduleSql
 {
+    /**
+     * @var bool|null Forced answer for the schema probe, null to look it up
+     */
+    private static ?bool $hasScheduleTable = null;
+
     /**
      * When a term next falls due, whether or not it has been graded.
      *
@@ -49,12 +55,54 @@ public static function effectiveDue(): string
             $cases .= ' WHEN ' . $status . ' THEN ' . (int) round($stability);
         }
 
+        $seeded = 'DATE_ADD(WoStatusChanged, INTERVAL CASE WoStatus' . $cases . ' END DAY)';
+
+        // An install where 20260805_200000_add_fsrs_scheduling.sql did not run
+        // has no term_schedule, and naming it here would fail the statement at
+        // prepare time — taking the review page and the whole vocabulary list
+        // to a 500 rather than degrading (issue #275 is such an install). Every
+        // term on that schema is ungraded by definition, so the seed expression
+        // alone is not a fallback but the exact same answer.
+        if (!self::hasScheduleTable()) {
+            return '(' . $seeded . ')';
+        }
+
         return '(SELECT COALESCE('
             . '(SELECT ts.TsDue FROM term_schedule ts WHERE ts.TsWoID = WoID),'
-            . ' DATE_ADD(WoStatusChanged, INTERVAL CASE WoStatus' . $cases . ' END DAY)'
+            . ' ' . $seeded
             . '))';
     }
 
+    /**
+     * Whether this schema carries the FSRS scheduling table.
+     *
+     * @return bool True when `term_schedule` can be named in a query
+     */
+    public static function hasScheduleTable(): bool
+    {
+        if (self::$hasScheduleTable !== null) {
+            return self::$hasScheduleTable;
+        }
+
+        // Deliberately not cached here as well: Connection memoises the lookup
+        // for the request and clears it when a migration run creates the table,
+        // so a second cache would only be one that could go stale.
+        return Connection::tableExists('term_schedule');
+    }
+
+    /**
+     * Override the schema probe — for tests, and to reset it between them.
+     *
+     * @param bool|null $exists True or false to force an answer, null to
+     *                          look it up again on the next call
+     *
+     * @return void
+     */
+    public static function setHasScheduleTable(?bool $exists): void
+    {
+        self::$hasScheduleTable = $exists;
+    }
+
     /**
      * Whether a term is due now.
      *

src/Shared/Infrastructure/Database/Connection.php

@@ -37,6 +37,11 @@ class Connection
      */
     private static ?\mysqli $instance = null;
 
+    /**
+     * @var array<string, bool> Memoised tableExists() answers, for this request
+     */
+    private static array $tableExists = [];
+
     /**
      * Get the database connection instance.
      *
@@ -291,6 +296,52 @@ public static function escapeString(string $value): string
     public static function reset(): void
     {
         self::$instance = null;
+        self::$tableExists = [];
+    }
+
+    /**
+     * Whether a table is present in the current database.
+     *
+     * For code that has to keep working against a schema where a migration did
+     * not run. A table named in raw SQL cannot be guarded by COALESCE or a LEFT
+     * JOIN: MySQL resolves names at prepare time, so a missing table fails the
+     * whole statement before any row is read. Callers ask this first and emit
+     * different SQL when the table is absent.
+     *
+     * The answer is memoised for the request. Migrations run during bootstrap,
+     * ahead of routing, and clear the cache when they finish, so a table
+     * created during the request is still seen by the code that reads it.
+     *
+     * @param string $table Table name, as it appears in the SQL
+     *
+     * @return bool True when the table exists
+     */
+    public static function tableExists(string $table): bool
+    {
+        if (isset(self::$tableExists[$table])) {
+            return self::$tableExists[$table];
+        }
+
+        /** @var int|string|null $found */
+        $found = self::preparedFetchValue(
+            'SELECT COUNT(*) AS value FROM information_schema.TABLES
+             WHERE TABLE_SCHEMA = DATABASE() AND TABLE_NAME = ?',
+            [$table]
+        );
+
+        self::$tableExists[$table] = ((int) $found) > 0;
+
+        return self::$tableExists[$table];
+    }
+
+    /**
+     * Forget memoised table lookups, after work that may have created tables.
+     *
+     * @return void
+     */
+    public static function forgetTableCache(): void
+    {
+        self::$tableExists = [];
     }
 
     /**

src/Shared/Infrastructure/Database/Migrations.php

@@ -47,8 +47,10 @@ class Migrations
     /**
      * How many times a failed migration is retried before being given up on.
      *
-     * Retries only happen when new migrations appear (see update()), so this
-     * counts upgrades, not requests.
+     * Counts requests: a failed migration is retried on the next request, and
+     * the one after, then left alone so a broken statement is not re-run on
+     * every page load. An upgrade that brings new migration files restores the
+     * budget — see {@see restoreRetryBudget()}.
      */
     public const MAX_ATTEMPTS = 3;
 
@@ -615,11 +617,35 @@ public static function getRecordedMigrations(): array
         return self::fetchMigrationNames("SELECT filename FROM _migrations");
     }
 
+    /**
+     * Give every failed migration its attempts back.
+     *
+     * Called when an upgrade brings new migration files along. A migration
+     * usually fails on a prerequisite rather than on itself, and the new files
+     * may be exactly what repairs it, so an exhausted attempt counter should
+     * not be what keeps it from ever running again.
+     *
+     * @return void
+     */
+    public static function restoreRetryBudget(): void
+    {
+        try {
+            Connection::execute(
+                "UPDATE _migrations SET attempts = 0
+                 WHERE status = '" . self::STATUS_FAILED . "'"
+            );
+        } catch (\RuntimeException $e) {
+            // Nothing to restore on an install too old to have the table yet
+            error_log('Could not restore migration retry budget: ' . $e->getMessage());
+        }
+    }
+
     /**
      * Get migrations that failed and are still worth retrying.
      *
      * A migration is retried until MAX_ATTEMPTS is reached; past that it stays
-     * on record as failed so an administrator can investigate.
+     * on record as failed so an administrator can investigate — until an
+     * upgrade restores the budget, see {@see restoreRetryBudget()}.
      *
      * @return array<string> List of failed migration filenames
      */
@@ -715,12 +741,20 @@ private static function runMigrationFile(string $filepath, string $filename): ?s
             try {
                 Connection::execute($sql_query);
             } catch (\RuntimeException $e) {
-                // Log per-statement failure but continue with remaining
-                // statements. This handles fresh installs where baseline
-                // creates modern tables and legacy migrations reference
-                // old table names that no longer exist.
+                // Continue with the remaining statements either way: a fresh
+                // install runs legacy migrations against table names that
+                // baseline.sql never created, and those fail by design.
+                //
+                // Classify before logging, and say which kind this was. Logging
+                // both under one wording buried the eight real failures of
+                // issue #275 among ~170 expected ones, in a log where they read
+                // identically (#285).
+                if (self::isHarmlessFailure($e)) {
+                    error_log("Migration statement skipped (expected): $filename - " . $e->getMessage());
+                    continue;
+                }
                 error_log("Migration failed: $filename - " . $e->getMessage());
-                if ($firstError === null && !self::isHarmlessFailure($e)) {
+                if ($firstError === null) {
                     $firstError = $e->getMessage();
                 }
             }
@@ -932,19 +966,30 @@ public static function update(): void
         $allMigrations = self::getMigrationFiles();
         $newMigrations = array_diff($allMigrations, self::getRecordedMigrations());
 
-        // A migration that failed before gets another chance whenever an
-        // upgrade brings new migrations along: the reason it failed is often a
-        // missing prerequisite that a later migration repairs. Retrying only on
-        // upgrades (and never more than MAX_ATTEMPTS times) keeps ordinary
-        // requests from re-running broken SQL over and over.
-        $retryMigrations = [];
+        // A migration that failed before gets another chance, bounded by
+        // MAX_ATTEMPTS so ordinary requests cannot re-run broken SQL forever.
+        //
+        // This used to be gated on `count($newMigrations) > 0`, which made the
+        // retry unreachable exactly where it was needed most: a fresh install
+        // records every migration on its first run, so the second run has no
+        // new ones, the gate never opens, and a migration that failed stays
+        // failed at one attempt until some later release happens to add a file
+        // (#285). An install left without `term_schedule` that way is the one
+        // in #275.
+        //
+        // The gate's own intent survives below: a migration often fails on a
+        // prerequisite that a later migration repairs, so an upgrade that
+        // brings new files restores the attempt budget of everything that
+        // failed, and the retry starts over rather than staying exhausted.
         if (count($newMigrations) > 0) {
-            $retryMigrations = array_intersect(
-                self::getRetryableMigrations(),
-                $allMigrations
-            );
+            self::restoreRetryBudget();
         }
 
+        $retryMigrations = array_intersect(
+            self::getRetryableMigrations(),
+            $allMigrations
+        );
+
         $pendingMigrations = array_merge($newMigrations, $retryMigrations);
         sort($pendingMigrations);
 
@@ -1056,6 +1101,10 @@ public static function update(): void
                 self::restoreForeignKeys($foreignKeys);
                 self::reconcileForeignKeys();
                 Connection::execute("SET FOREIGN_KEY_CHECKS = 1");
+
+                // The run may have created a table that code later in this same
+                // request probes for before naming it in SQL.
+                Connection::forgetTableCache();
             }
         }