S159 — make a failed migration visible: honest exit codes + a decided boot path - #604
Merged
Conversation
…ath (S159) Three layers each swallowed a migration failure independently, so a container could boot, report success and serve traffic against a half-migrated schema: MigrationRunner recorded the error and continued, scripts/run-migrations.php always exited 0, and docker/docker-entrypoint.sh wrapped the call in `|| true`. `bin/phlix migrate` was the only path in the project that returned 1. The fix is the exit code, not the execution order. - MigrationRunner::exitCodeFor() is now the single definition of "this run failed": only `errors` decides. Notes, skipped_count and an empty `applied` never do — an "already applied" replay (duplicate column/key, which MySQL 8 raises on nearly every re-applied file because it has no IF NOT EXISTS on ADD COLUMN/ADD INDEX) stays a success. `bin/phlix migrate` and scripts/run-migrations.php both route through it so they cannot drift. - scripts/run-migrations.php exits 0/1 instead of always 0, and prints a "Migrations FAILED" summary to stderr. PHLIX_MIGRATIONS_DIR was added so the contract can be proven without touching the real migrations/ set. - Execution stays CONTINUE-AND-REPORT, deliberately: files are independent, and stopping would leave every later file un-applied AND un-recorded, turning one bad file into a permanently stalled schema. A failed file is still left out of schema_migrations, so it is retried next run. - docker/docker-entrypoint.sh: `|| true` is deleted — it was indiscriminate (it also swallowed fatals and an unreachable DB), it hid the decision, and it was redundant while the script always exited 0. The day-one intent it encoded is preserved explicitly: the container still boots on a migration failure, but now prints a greppable PHLIX-MIGRATION-FAILURE banner with the exit code. PHLIX_MIGRATIONS_STRICT=1 inverts that and refuses to start. - scripts/install.sh keeps no `|| true`, so a failed migration now aborts the install/update before the service is restarted. Documented in place, along with the stale "no tracking table" claim that the SV-4.9 ledger invalidated. Proven by execution in both directions against real MySQL: clean run 0, replay 0, steady state 0, deliberately failing migration 1 (with the later file still applied and the failing one absent from the ledger), and the same against the real migrations/ dir with one bad file planted — which is exactly what CI runs. docker-entrypoint.sh had no test at all; tests/Unit/Docker/DockerEntrypointTest now drives it through /bin/sh with stub php/supervisord across all 15 branches. Re-adding `|| true` reddens 12 of them and making fail-fast the default reddens 5, so the decision is pinned rather than incidental. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 3 medium |
| Documentation | 14 minor |
| ErrorProne | 10 medium |
| CodeStyle | 71 minor |
| Complexity | 2 medium |
🟢 Metrics 66 complexity · 0 duplication
Metric Results Complexity 66 Duplication 0
🟢 Coverage 98.21% diff coverage · +0.10% coverage variation
Metric Results Coverage variation ✅ +0.10% coverage variation (-1.00%) Diff coverage ✅ 98.21% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (2ff0a9e) 63705 41994 65.92% Head commit (0ab4f87) 63747 (+42) 42084 (+90) 66.02% (+0.10%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#604) 56 55 98.21% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #604 +/- ##
============================================
+ Coverage 66.76% 66.85% +0.09%
- Complexity 21661 21676 +15
============================================
Files 668 668
Lines 67869 67915 +46
============================================
+ Hits 45313 45406 +93
+ Misses 22556 22509 -47 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…revive the boot path F1 (blocker) — MigrationRunner classified "already applied" by grepping the exception message, which Workerman\MySQL\Connection builds as "SQL:<the whole statement> <pdo message>". Any migration whose own SQL contained 'already exists' / 'Duplicate column name' / 'Duplicate key name' / 'check that column/key exists' was reclassified as an idempotent replay: exit 0, counted as a skip, AND recorded in schema_migrations so it was never retried — defeating failure classes (b) and (c) at once and making things worse than before S159. Classification now strips everything before the LAST 'SQLSTATE[', parses the MySQL errno out of the driver segment and matches a closed list. (errorInfo is NULL and getCode() is the SQLSTATE cast to int on the rethrown PDOException, so the rendered segment is the only place the errno survives.) F2 — added 1826 (dup FK name) and 3822 (dup CHECK name): same named-object shape as 1050/1061. Deliberately NOT added: 1062 (also what ADD UNIQUE raises when existing DATA violates the constraint) and 1068 (names no object, so a replay is indistinguishable from a genuine second primary key). Both exclusions are now documented in README, both workflows and the class docblock, and pinned by tests. F3 — bin/phlix migrate printed "Migrations complete." beside exit 1. Both paths now render MigrationRunner::failureSummary() and only say "complete" on success. F4 — all three Dockerfiles named /docker-entrypoint.sh in CMD and none ever copied it there, so no shipped container had ever run the entrypoint (broken since c2127f9; invisible because docker build never executes CMD). Added the COPY to each, and a test that asserts the COPY/CMD pair plus the entrypoint's defaults against every Dockerfile's WORKDIR and supervisord.conf path. F5 — every documented deployment configures PHLIX_DATABASE_*, which no PHP reads. With F4 fixed that would have printed the failure banner on every clean boot. The entrypoint now maps PHLIX_DATABASE_* onto the DB_* names config/database.php actually reads (DB_* always wins) and exports them. F6 — PHLIX_MIGRATIONS_STRICT now also refuses to start when migrations could not run at all (no host / script missing), and the value is trimmed. F7 — the failure summary no longer asserts "The schema is HALF-MIGRATED": an unreachable DB still reports 100 files "attempted", so the message states only what it can verify and tells the operator how to tell the two cases apart. F8 — the recorded php arguments are now asserted (the entrypoint must invoke scripts/run-migrations.php), and the previously-unasserted defaults are pinned. F9 — the comment above `php … || migration_status=$?` no longer describes an `if ! cmd` the code does not use. F10 — MigrateCommand returns exitCodeFor() directly instead of re-deriving it. F11 — bin/phlix now honours PHLIX_MIGRATIONS_DIR, as README already claimed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…o the container died at supervisord
Reviving the entrypoint (previous commit, S159 review finding 4) made a second
pre-existing defect in the same file reachable. Docker runs `RUN` through
/bin/sh, which on Alpine is busybox ash and does NOT expand braces, so
`mkdir -p /var/phlix/{config,data,logs,backups}` created ONE directory with that
literal name. Verified in the built image:
drwxr-xr-x 2 nobody nobody 4096 /var/phlix/{config,data,logs,backups}
Consequence, verified by running the image: the entrypoint now resolved, printed
its banner and applied migrations cleanly, then `exec supervisord` failed with
"The directory named as part of the path /var/phlix/logs/supervisord.log does
not exist" and the container exited 2 — i.e. fixing the CMD alone would have
shipped an image that still could not boot, and no Build job would have noticed
because `docker build` never executes CMD. A compose deployment masked it by
volume-mounting those four paths.
Dockerfile.intel and Dockerfile.nvidia already write the paths out; this matches
them. After the fix the container reaches Running=true with supervisord as PID 1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
S159 review round 2. Finding 1 (HIGH) — 1826/3822 swallowed a statement that created NOTHING and then RECORDED the file, so it was never retried. Round 1 added them as "named-object collisions, the same shape as 1050/1061". That holds for 1061 (index names are TABLE-local) and is false for these two: in MySQL 8 a FOREIGN KEY name and a CHECK-constraint name are unique per SCHEMA, so the colliding object can belong to a different table and the failing statement can be a CREATE TABLE that consequently created nothing at all. Measured: four files, two of them `CREATE TABLE IF NOT EXISTS ... CONSTRAINT <name-used-elsewhere>`, exited 0 with their tables absent and their names in schema_migrations — classes (b) and (c) defeated at once, and strictly worse than master (master's substring list has neither phrase, and `CREATE TABLE IF NOT EXISTS` does not contain "already exists", so master fails loudly). The narrower "squelch only on ALTER TABLE ... ADD CONSTRAINT" alternative was rejected: an ALTER whose name collides with a constraint on another table raises 1826 too and adds nothing. The set is back to the four table-local collisions, and the two phrases the branch had added to the message fallback are gone too. The cost is documented, not hidden: a genuinely replayed ADD CONSTRAINT now exits 1; no current migration does that (the 100-file replay still exits 0 with 99 notes). Finding 2 (LOW) — the invariant the classifier rests on was forgeable. For errno 1064 MySQL echoes ~80 chars of the offending statement back inside its own message, so a `SQLSTATE[..]: ..: <errno>` decoy in that window made the last `SQLSTATE[` the migration's, not the driver's. Measured verbatim: a 1064 syntax error read as 1050 — note, exit 0, ledger row written, table never created. run() now passes the statement it just executed into the classifier, which strips `'SQL:' . $statement . ' '` exactly; there is no delimiter left to forge. lastSQL() being byte-identical to the statement was verified rather than assumed (both trim; measured 99/99 exact-prefix over the real 100-file replay in both pool modes, 0 fallbacks). A non-matching prefix falls back to the old behaviour instead of guessing. Also updated so nothing teaches the rejected errnos as safe: README, both workflow comments, the IDEMPOTENT_ERROR_CODES docblock (records WHY they were rejected, beside the existing 1062/1068 rejections), and the tests — the idempotent-errno integration test now states the set is exactly four and points at its new twin, which pins the collision SHAPE against real MySQL (a CREATE TABLE failing 1826 and one failing 3822 must exit 1 and stay out of the ledger). A reflection test pins the closed list itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
Three layers each swallowed a migration error independently, so a container could boot, report
success, and serve traffic against a half-migrated schema:
MigrationRunner::run()records a genuine error and continues with every remaining statement and file.scripts/run-migrations.phpcontained zeroexit()calls ⇒ always exited 0.docker/docker-entrypoint.sh:9wrapped the call in|| true.bin/phlix migratewas the only path that could return 1.Intent was established from history before semantics were touched
git show c2127f91:docker/docker-entrypoint.shhasset -eon line 2 and|| trueon the migrationline in the file's first version — opting one command out of
set -ein the same file is adeliberate statement. The counter-evidence is equally explicit:
scripts/install.shrunsset -euo pipefailand calls the script twice with no|| true. So the original intent wasfail-fast on the attended path, boot-anyway in the container. Both are now honoured and written down
rather than left incidental.
Also corrected: the
|| truewas redundant for the case people believed it covered. Because thescript always exited 0, it could only ever mask a fatal PHP error or an unreachable DB.
Decisions
and un-recorded, turning one bad file into a permanently stalled schema. Only the exit code changed.
|| truedeleted, replaced by an explicit branch: a greppablePHLIX-MIGRATION-FAILUREbannerplus exit code to stderr, and the container still boots by default.
PHLIX_MIGRATIONS_STRICT=1inverts it. A crash-looping media server under
restart: unless-stoppedis a total outage with noUI to explain why — the loud default was not assumed to be right.
install.shstays fail-fast, aborting before the service restart.The three failure classes are kept distinct so legitimate replays do not redden: (a) "already applied"
⇒ note, still success; (b) genuine statement error ⇒ surfaces; (c) a file that failed is left
unrecorded ⇒ retried next run.
Verified by execution (orchestrator, against real MySQL 8.0)
0100 statement(s) skipped)01— failing file absent fromschema_migrations(0rows)0The "half-migrated" wording is literally accurate: the probe's
CREATE TABLEpersisted while itsINSERTfailed.Mutation-proven by the implementer: re-adding
|| true→ 12/15 red; fail-fast default → 5/15 red;exit(0)→ 1 red; treating notes as failures → 8 red.PHPStan L9
[OK] No errors. PHPCSsrc/0 ERRORS, 8 WARNINGS, 7 FILES— identical to master(A/B'd; none of the 7 files is in this diff).
🤖 Generated with Claude Code