diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a0e66b32..f2bddd978 100644 --- a/CHANGELOG.md +++ b/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 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