From 8eecf84f09dc0d7127d75a97631a4a77c95327f2 Mon Sep 17 00:00:00 2001 From: HugoFara Date: Wed, 26 Aug 2026 11:23:41 +0200 Subject: [PATCH] fix(db): survive a schema left without term_schedule (#275, #285) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An install where 20260805_200000_add_fsrs_scheduling.sql failed has no term_schedule, and phase 2b reads every term's due date from it. COALESCE guards a missing row, not a missing table: MySQL resolves names at prepare time, so the whole statement is rejected and both the review page and the entire vocabulary list answer 500 — worse than 3.4.2, where the same broken schema was still usable. #275 is such an install. Guard the one place that names the table. ScheduleSql::effectiveDue() emits the status-seed expression alone when term_schedule is absent, which is not a degraded answer but the same one: every term on that schema is ungraded by definition. MySqlTermScheduleRepository does the same, dropping its writes rather than failing, so a review session keeps going and the term's status still moves; the schedule starts being kept once the schema is repaired. Connection::tableExists() memoises the probe for the request and Migrations clears it after a run, so a table created during bootstrap is seen by the code that reads it later in the same request. Then stop leaving installs in that state. A failed migration was only reconsidered when an upgrade brought new files along, which on a fresh install can never happen: the first run records all of them, so there is never anything new afterwards and the migration stays failed at one attempt until some later release happens to add a file. MAX_ATTEMPTS was meant to be the bound and never got the chance to count. Retry on ordinary requests instead, and keep the gate's real intent by restoring the attempt budget when new migrations do arrive — a migration usually fails on a prerequisite that a later one repairs, and an exhausted counter should not be what stops it running again. Log the classification too. runMigrationFile() logged before deciding whether a failure mattered, so a healthy fresh install wrote ~178 "Migration failed:" lines with all 53 migrations applied, and the eight real errors of #275 read exactly like the ~170 expected ones. Verified on a 3.4.2 database with term_schedule dropped: review page, vocabulary list, texts and reader all 200 with no schedule table and the retry budget spent; with one attempt left and no new migration files, the next ordinary request re-ran the migration, recreated both tables and restored the foreign keys. --- CHANGELOG.md | 27 ++++++ .../MySqlTermScheduleRepository.php | 20 ++++- .../Review/Infrastructure/ScheduleSql.php | 50 ++++++++++- .../Infrastructure/Database/Connection.php | 51 +++++++++++ .../Infrastructure/Database/Migrations.php | 85 +++++++++++++++---- .../backend/Core/Database/MigrationsTest.php | 49 +++++++++++ .../Review/Infrastructure/ScheduleSqlTest.php | 65 ++++++++++++++ 7 files changed, 326 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c944083a5..b48d60e95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,33 @@ 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. + * **Adding a term failed outright on a large vocabulary** (#277). Opening the term editor read every term of the language into memory to look for similar ones — affordable for a vocabulary built by hand, fatal for one seeded from a diff --git a/src/Modules/Review/Infrastructure/MySqlTermScheduleRepository.php b/src/Modules/Review/Infrastructure/MySqlTermScheduleRepository.php index a14e23a72..9251fd1f5 100644 --- a/src/Modules/Review/Infrastructure/MySqlTermScheduleRepository.php +++ b/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 []; } diff --git a/src/Modules/Review/Infrastructure/ScheduleSql.php b/src/Modules/Review/Infrastructure/ScheduleSql.php index a3c148d69..d3927ee93 100644 --- a/src/Modules/Review/Infrastructure/ScheduleSql.php +++ b/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. * diff --git a/src/Shared/Infrastructure/Database/Connection.php b/src/Shared/Infrastructure/Database/Connection.php index 3c1bc976f..685cf47f9 100644 --- a/src/Shared/Infrastructure/Database/Connection.php +++ b/src/Shared/Infrastructure/Database/Connection.php @@ -37,6 +37,11 @@ class Connection */ private static ?\mysqli $instance = null; + /** + * @var array 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 = []; } /** diff --git a/src/Shared/Infrastructure/Database/Migrations.php b/src/Shared/Infrastructure/Database/Migrations.php index 8fdf84830..fd88cc39f 100644 --- a/src/Shared/Infrastructure/Database/Migrations.php +++ b/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 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(); } } diff --git a/tests/backend/Core/Database/MigrationsTest.php b/tests/backend/Core/Database/MigrationsTest.php index 2d73938ff..7f372916e 100644 --- a/tests/backend/Core/Database/MigrationsTest.php +++ b/tests/backend/Core/Database/MigrationsTest.php @@ -585,6 +585,55 @@ public function testRetryStopsAfterMaxAttempts(): void Connection::preparedExecute("DELETE FROM _migrations WHERE filename = ?", [$testFilename]); } + public function testAnUpgradeRestoresTheRetryBudget(): void + { + if (!self::$dbConnected) { + $this->markTestSkipped('Database connection required'); + } + + Migrations::upgradeMigrationsTable(); + $testFilename = 'test_budget_' . time() . '.sql'; + + for ($i = 0; $i < Migrations::MAX_ATTEMPTS; $i++) { + Migrations::recordMigration($testFilename, '', Migrations::STATUS_FAILED, 'boom'); + } + $this->assertNotContains($testFilename, Migrations::getRetryableMigrations()); + + // A migration usually fails on a prerequisite rather than on itself, so + // new migration files are a reason to try an exhausted one again. + Migrations::restoreRetryBudget(); + + $this->assertContains( + $testFilename, + Migrations::getRetryableMigrations(), + 'An upgrade should let an exhausted migration run again' + ); + + Connection::preparedExecute("DELETE FROM _migrations WHERE filename = ?", [$testFilename]); + } + + public function testRestoringTheBudgetLeavesAppliedMigrationsAlone(): void + { + if (!self::$dbConnected) { + $this->markTestSkipped('Database connection required'); + } + + Migrations::upgradeMigrationsTable(); + $testFilename = 'test_budget_applied_' . time() . '.sql'; + + Migrations::recordMigration($testFilename, 'abc', Migrations::STATUS_APPLIED); + Migrations::restoreRetryBudget(); + + $this->assertNotContains( + $testFilename, + Migrations::getRetryableMigrations(), + 'An applied migration must never be queued for a retry' + ); + $this->assertContains($testFilename, Migrations::getAppliedMigrations()); + + Connection::preparedExecute("DELETE FROM _migrations WHERE filename = ?", [$testFilename]); + } + public function testRecordMigrationPromotesFailureToApplied(): void { if (!self::$dbConnected) { diff --git a/tests/backend/Modules/Review/Infrastructure/ScheduleSqlTest.php b/tests/backend/Modules/Review/Infrastructure/ScheduleSqlTest.php index c468ff6de..cc96ae7e7 100644 --- a/tests/backend/Modules/Review/Infrastructure/ScheduleSqlTest.php +++ b/tests/backend/Modules/Review/Infrastructure/ScheduleSqlTest.php @@ -20,6 +20,22 @@ #[CoversClass(ScheduleSql::class)] class ScheduleSqlTest extends TestCase { + /** + * The expression asks the schema whether `term_schedule` is there. Force + * the answer so these stay unit tests and never reach for a connection. + */ + protected function setUp(): void + { + parent::setUp(); + ScheduleSql::setHasScheduleTable(true); + } + + protected function tearDown(): void + { + ScheduleSql::setHasScheduleTable(null); + parent::tearDown(); + } + public function testFallsBackToTheStatusSeedForAnUngradedTerm(): void { $sql = ScheduleSql::effectiveDue(); @@ -65,6 +81,55 @@ public function testTomorrowPredicateLooksOneDayAhead(): void $this->assertStringStartsWith(ScheduleSql::effectiveDue(), ScheduleSql::isDueTomorrow()); } + public function testSchemaWithoutTheScheduleTableNeverNamesIt(): void + { + // An install where the phase-2a migration did not run (#275). Naming a + // missing table fails the statement at prepare time, which took the + // review page and the vocabulary list to a 500 rather than degrading. + ScheduleSql::setHasScheduleTable(false); + + $sql = ScheduleSql::effectiveDue(); + + $this->assertStringNotContainsString('term_schedule', $sql); + $this->assertStringContainsString('WoStatusChanged', $sql); + } + + public function testDegradedExpressionKeepsEverySeedInterval(): void + { + // Every term on such a schema is ungraded by definition, so the seed + // expression alone is not an approximation — it is the same answer. + ScheduleSql::setHasScheduleTable(false); + + $sql = ScheduleSql::effectiveDue(); + + foreach (LegacyStatusSeed::stabilityByStatus() as $status => $stability) { + $this->assertStringContainsString( + 'WHEN ' . $status . ' THEN ' . (int) round($stability), + $sql, + "Status $status lost its seed interval without term_schedule" + ); + } + } + + public function testDegradedExpressionIsStillSelfContained(): void + { + ScheduleSql::setHasScheduleTable(false); + + $sql = ScheduleSql::effectiveDue(); + + $this->assertStringStartsWith('(', $sql); + $this->assertStringEndsWith(')', $sql); + $this->assertSame(substr_count($sql, '('), substr_count($sql, ')')); + } + + public function testPredicatesFollowTheDegradedExpression(): void + { + ScheduleSql::setHasScheduleTable(false); + + $this->assertSame(ScheduleSql::effectiveDue() . ' <= NOW()', ScheduleSql::isDue()); + $this->assertStringStartsWith(ScheduleSql::effectiveDue(), ScheduleSql::isDueTomorrow()); + } + public function testExpressionIsSelfContained(): void { // Wrapped in parentheses so it can be dropped into a WHERE, an ORDER BY