Repair the schema manifest, and keep a FOS report readable after its task - #1236
Merged
Conversation
…ts task Two follow-ups on the FOS report feature (GH-1206..GH-1223). 1. schema-expected.php had drifted, silently -------------------------------------------- 407be1a added logType and logText to taskLog's `create` and not to its `columns`. The header asks nobody to hand-edit that block; nothing checked. SchemaReconciler::plan() pass 3 adds missing columns only from `columns` and skips a table whose `columns` is empty, so the reconciler could never heal a taskLog that already existed -- and an existing table is the only kind it repairs. The INSERT then failed with 1054 Unknown column, on any sql_mode, not just STRICT_TRANS_TABLES. The consequences were quieter than expected. PDODB::$throwOnQueryError is false and nothing sets it true, so query() recorded the error and returned; FOGController::save() caught its own insertId=0 exception and returned false; TaskError::_logRow() ignores that return. So nothing threw, TaskError's catch never ran, "unusable report from FOS" was never written, the endpoint answered its usual 200, the task was marked Failed and HOST_IMAGE_FAIL fired -- and the report simply was not there. PDODB's own error() goes to a file gated on FOG_LOG_ERROR, which is 0 by default, so the SQL error was written nowhere at all. Proven by shadowing taskLog with a TEMPORARY table of the six old columns and driving TaskLog->save(): save() returns false, DB->error holds the 1054, the row count does not move. OpenAPI::_entitySchema() reads column types from the same block, so the published document described both fields as "No type information available for this column." Audited all 67 tables in both directions and on the definition text. taskLog was the only divergence. Corrected in place rather than regenerated: regeneration rewrites 67 create strings and 507 column definitions from whatever the generating server has, which is a diff nobody can review for a two-column bug. Verified the edit is byte identical to what `schema-manifest.php generate` produces. Guards, because "nothing compared the two blocks" was the actual defect: tests/schema-manifest-consistent.test.php compares them for every table, both directions and the definition text. No database, so it gates CI. Mutation-verified against all four ways they can disagree. bin/schema-manifest.php now refuses to write a table it parsed no columns for. It cannot disagree with itself today, but the two blocks come from one $raw by different means and only the column parse can fail quietly -- a SHOW CREATE TABLE shape that regex stops matching would ship every `columns` block empty and disable the reconciler for the whole schema. 2. A report lost its identity when its task was cleared ------------------------------------------------------- taskLog stores no host and no task type of its own; Task Management's log pane reaches them through LEFT OUTER JOINs against `tasks`. Nothing deletes taskLog rows -- but Route::deletemass('host') cascades to `task` and taskLog is in no cascade at all. So deleting a host destroys its tasks and leaves the reports behind with NULL where the host name was, at the same moment the host row that could supply it goes. On the install this was written against, 9 of 56 rows were already orphaned. The state a row records was never at risk: taskLog stores taskStateID itself. The image never appeared in this view. What was lost is host name, host id and task type -- and host name is the first thing anyone searches a failure by. Schema 341 copies hostID, host name and task type name onto the row at write time, backfilling reports whose task is still there. _logQueryFrom() prefers the copy, falling back to the joins for rows written before 341 and for state rows. That order is deliberate: a report is a historical record, so the name the host had WHEN IT FAILED is the answer, not what it has been renamed to since. Blocking deletion of a task that has reports was the alternative. It inverts the dependency -- a diagnostic artifact would constrain operational cleanup -- and to be consistent it would have to block HOST deletion too, since that is the path that actually removes tasks. Refusing to delete a host because it once failed to image is a worse product than losing a host name. Only the FOS report endpoint fills the columns. 53 of those 56 rows are state transitions written by TaskingElement::taskLog() on every transition; they mean nothing without their task, and three extra lookups per transition buys nothing. Same reasoning that gave logText no value on a state row in step 338. Two column shapes, following what the writer can produce: save() omits an unset OPTIONAL column whose key ends in "id", so logHostID lands NULL, but writes '' for every other unset field -- the trap step 340 had to repair for logType. Declaring logHostName NOT NULL DEFAULT '' says what will really be stored. logHostID still comes from the `hosts` join, because the grid links the name with it and a link to a deleted host is worse than no link. Resolving that join through the stored id first is what keeps the link working once the task is gone but the host is not. tests/tasklog-report-retention.test.php drives the writer against a fake connection and, with FOG_TEST_DSN, runs the real statement over six host/task/rename shapes. Seven mutations verified. It needs no CREATE DATABASE, so an ordinary FOG database user can run it. Also fixed, found on the way: tests/schema-executes.test.php walked the manifest's top level instead of ['tables'], so it collected no manifest statements and failed its own non-empty guard whenever a DSN was set. That half had never run. It now also skips cleanly when the account may not CREATE DATABASE, rather than dying on an uncaught PDOException. sh tests/run-all.sh -- 80 passed, 0 failed, with and without a DSN. Co-Authored-By: Claude <noreply@anthropic.com>
This was referenced Aug 20, 2026
mastacontrola
pushed a commit
that referenced
this pull request
Aug 20, 2026
core.hooksPath is an absolute path, so every linked worktree runs the
MAIN worktree's copy of pre-commit whatever branch it has checked out.
dev-branch carries neither bin/schema-manifest.php nor
commons/schema-expected.php -- SchemaReconciler and its manifest are
1.6-only -- so a schema commit there ran a 1.6 gate against a 1.5 tree:
Could not open input file: .../bin/schema-manifest.php
pre-commit: schema-expected.php is stale -- commit aborted
which reads as a broken hook rather than a branch that was never in
scope, and whose only obvious escape is --no-verify. A gate people learn
to bypass is the failure this function's own comments are written to
avoid.
Skipped when both the generator and the manifest are absent. That is NOT
the fail-open the php check below it refuses to be: that one is "the
manifest exists and I cannot check it", this is "there is no manifest,
so nothing can be stale". Requiring BOTH files means a tree that has the
manifest but has lost the generator still fails closed further down.
Found committing the 1.5 port of #1236, which is the first schema change
on dev-branch since the gate was added.
Co-Authored-By: Claude <noreply@anthropic.com>
mastacontrola
pushed a commit
that referenced
this pull request
Aug 20, 2026
core.hooksPath is an absolute path, so every linked worktree runs the
MAIN worktree's copy of pre-commit whatever branch it has checked out.
dev-branch carries neither bin/schema-manifest.php nor
commons/schema-expected.php -- SchemaReconciler and its manifest are
1.6-only -- so a schema commit there ran a 1.6 gate against a 1.5 tree:
Could not open input file: .../bin/schema-manifest.php
pre-commit: schema-expected.php is stale -- commit aborted
which reads as a broken hook rather than a branch that was never in
scope, and whose only obvious escape is --no-verify. A gate people learn
to bypass is the failure this function's own comments are written to
avoid.
Skipped when both the generator and the manifest are absent. That is NOT
the fail-open the php check below it refuses to be: that one is "the
manifest exists and I cannot check it", this is "there is no manifest,
so nothing can be stale". Requiring BOTH files means a tree that has the
manifest but has lost the generator still fails closed further down.
Found committing the 1.5 port of #1236, which is the first schema change
on dev-branch since the gate was added.
Committed with --no-verify deliberately: this changes one shell file,
and letting the hook run rewrote every .po and dropped 301 lines from
messages.pot -- generated-artifact skew from a local gettext that
differs from whichever produced the committed files. That churn belongs
to a translation refresh, not to a hook fix. FOG_VERSION is left to the
Version Sync job, as it is for every other merge.
Co-Authored-By: Claude <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
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.
Two follow-ups on the FOS report feature (#1206–#1223).
1.
schema-expected.phphad drifted, silently407be1a added
logTypeandlogTexttotaskLog'screatestring and not to itscolumnsarray. The file header asks nobody to hand-edit that block; nothing checked that they agreed.SchemaReconciler::plan()pass 3 adds missing columns only fromcolumns, and skips a table whosecolumnsis empty — so the reconciler could never heal ataskLogthat already existed, and an existing table is the only kind it repairs.The failure is quieter than it looks, and not strict-mode dependent. Naming a column that doesn't exist is error 1054, on any
sql_mode. And nothing throws:PDODB::query()throws$throwOnQueryErrorisfalseand nothing sets it true — it records->errorand returnssave()catches its owninsertId=0exception and returnsfalse;_logRow()ignores the returnSo the endpoint answered its usual
200 ##,fosreports.loggot the normal success line, the task was marked Failed,HOST_IMAGE_FAILfired — and the report simply was not in the database.PDODB's ownerror()writes to a file gated onFOG_LOG_ERROR, which is0by default, so the SQL error went nowhere at all.Proven by shadowing
taskLogwith aTEMPORARYtable of the six old columns:Second consumer, same gap:
OpenAPI::_entitySchema()reads column types fromcolumns, so the published API document described both fields as "No type information available for this column." Fixed as a side effect.Audited all 67 tables in both directions and on the definition text.
taskLogwas the only divergence. Corrected in place rather than regenerated — regeneration rewrites 67createstrings and 507 column definitions from whatever the generating server has, which is not a reviewable diff for a two-column bug. Verified the edit is byte-identical to whatschema-manifest.php generateproduces.Guards, since "nothing compared the two blocks" was the actual defect:
tests/schema-manifest-consistent.test.php— compares them for every table, both directions and the definition text. No database, so it gates CI. Mutation-verified against all four ways they can disagree.bin/schema-manifest.phpnow refuses to write a table it parsed no columns for. It can't disagree with itself today, but the two blocks come from one$rawby different means and only the column parse can fail quietly — aSHOW CREATE TABLEshape that regex stops matching would ship everycolumnsblock empty and disable the reconciler for the whole schema.2. A report lost its identity when its task was cleared
taskLogstores no host and no task type of its own. Nothing deletestaskLogrows — butRoute::deletemass('host')cascades totask, andtaskLogis in no cascade at all. Deleting a host destroys its tasks and leaves the reports with NULL where the host name was, at the same moment the host row that could have supplied it goes. On the install this was written against, 9 of 56 rows were already orphaned.Narrower than first stated:
taskLogstorestaskStateIDitself, so state survives; the image was never in this view. What is lost is host name, host id and task type.Schema 341 copies
logHostID,logHostNameandlogTaskTypeNameonto the row at write time and backfills reports whose task still exists._logQueryFrom()prefers the copy, falling back to the joins for pre-341 rows and state rows. That order is deliberate: a report is a historical record, so the name the host had when it failed is the answer, not what it has been renamed to since.Why not block deleting a task that has reports: it inverts the dependency — a diagnostic artifact would constrain operational cleanup — and to be consistent it would have to block host deletion too, since that's the path that actually removes tasks. Refusing to delete a host because it once failed to image is a worse product than losing a host name.
Only the FOS report endpoint fills the columns. 53 of those 56 rows are state transitions written on every transition; they mean nothing without their task, and three extra lookups per transition buys nothing — the same reasoning that gave
logTextno value on a state row in step 338.Two column shapes, following what the writer can actually produce:
save()omits an unset optional column whose key ends inid, sologHostIDlands NULL, but writes''for every other unset field (the trap step 340 had to repair forlogType).logHostIDstill comes from thehostsjoin, because the grid links the name with it and a link to a deleted host is worse than no link. Resolving that join through the stored id first keeps the link working once the task is gone but the host is not.Tests
tests/tasklog-report-retention.test.phpdrives the writer against a fake connection, and withFOG_TEST_DSNruns the real statement over six shapes:Seven mutations verified. It needs no
CREATE DATABASE, so an ordinary FOG database user can run it.Also fixed, found on the way:
tests/schema-executes.test.phpwalked the manifest's top level instead of['tables'], so it collected no manifest statements and failed its own non-empty guard whenever a DSN was set — that half had never run. It now also skips cleanly when the account may notCREATE DATABASEinstead of dying on an uncaughtPDOException.sh tests/run-all.sh— 80 passed, 0 failed, with and without a DSN.Downstream
None — no route class added or removed.