Issue 121 Index List Out Of Date After Upgrading - edmozley/freeitsm GitHub Wiki

The index list is out of date (issue #121)

Reported by tjedelhauser ยท Fixed in #1703-#1706


1. What you saw

After upgrading, System โ†’ Database Verification showed a red Error row called index backfill list, with notes along these lines:

The index list is out of date vs freeitsm.sql โ€” run scripts/gen_db_verify_indexes.php and commit both files.; Index asset_physical_disks.idx_asset_physical_disks_model is in the backfill list but no longer in freeitsm.sql.

or, on an earlier version:

โ€ฆ; Index task_recurrences.ix_task_recurrences_due differs โ€” freeitsm.sql has [KEY (is_active,next_due_date)], the list has [KEY (is_active, next_due_date)].; Index tasks.ix_tasks_recurrence_id is in the backfill list but no longer in freeitsm.sql.

It came back on every run, it survived upgrades, and it told you to run a script and commit two files โ€” which is not something you can do to a copy of FreeITSM you have installed.

Nothing was wrong with your database. No data was at risk and none was lost. The two files that describe the schema, both shipped by FreeITSM, disagreed with each other.

This is the third time it has shipped: #113 first, then again in 1.6, then again in 1.9.


2. What the check is for

FreeITSM describes its schema in more than one place, deliberately:

File Job
database/freeitsm.sql builds a new install
includes/db_verify_schema.php upgrades an existing one
includes/db_verify_indexes.php a generated mirror of every index in freeitsm.sql

The mirror exists because a grown install only ever received the indexes Database Verification was explicitly told to add. An index added to the schema years later would never reach it. The mirror is the backstop: it restores every named index, idempotently.

So the mirror has to match the schema, and the check that says it does not is correct and worth having. What was wrong was who it was talking to and when it ran.


3. What was actually wrong

Fault 1 โ€” an index that existed only in the generated mirror

Commit 98ec2b9a, the disk-hiding work, added two rows to includes/db_verify_indexes.php by hand, with a comment explaining the decision:

// โš ๏ธ Kept by hand. The generator drops these two because they are no longer
// in database/freeitsm.sql, which is pre-existing drift unrelated to the
// checklists work - removing them as a side effect of it would quietly cost
// grown installs two indexes. Left in until the real question is answered:
// should freeitsm.sql get them back, or should they genuinely go?
['asset_devices', 'idx_asset_devices_asset', 'key', '(`asset_id`)'],
['asset_physical_disks', 'idx_asset_physical_disks_model', 'key', '(`model`(100))'],

The caution was reasonable and the note was honest. It was also wrong about one of the two, and it left the question open in the one file that must never be edited by hand.

  • asset_devices.idx_asset_devices_asset is in freeitsm.sql (line 2568). The generator emits it perfectly well. The hand-added row was simply a duplicate.
  • asset_physical_disks.idx_asset_physical_disks_model was never in freeitsm.sql. No matching KEY was ever added to that table.

So the mirror carried an index the schema had never heard of, and Verification said so โ€” correctly โ€” to everybody who upgraded.

Which side was wrong

This is the part that matters, and it is not obvious. The obvious fix โ€” regenerate the mirror โ€” would have been the wrong one. The generator deletes whatever freeitsm.sql does not have, so regenerating alone would have dropped the index from the mirror and quietly cost every grown install an index the code relies on:

// includes/asset_disk_visibility.php โ€” assetDiskMatchCount()
$sql = "SELECT COUNT(*) AS drives, COUNT(DISTINCT pd.asset_id) AS assets
          FROM asset_physical_disks pd
          JOIN assets a ON a.id = pd.asset_id
         WHERE pd.model <=> ? AND pd.size_bytes <=> ?" . $tenantSql;

That is what answers "also hide the other 49 drives like this, across every asset" โ€” a scan by model across every drive row in the estate, run whenever an asset screen offers the option. The index is genuinely wanted.

Which means the schema was the side in the wrong, and the situation was worse than a cosmetic error message: a grown install had the index (the backfill gave it to them) and a fresh install never did.

The KEY is now in freeitsm.sql where it belongs, and the mirror is regenerated from it.

Fault 2 โ€” a formatting difference reported as drift

From the original report:

Index task_recurrences.ix_task_recurrences_due differs โ€” freeitsm.sql has [KEY (is_active,next_due_date)], the list has [KEY (is_active, next_due_date)]

That is one index, spelled two ways, one space apart. The parser strips whitespace when it reads freeitsm.sql; a mirror generated before that normalisation was added still carried the space; and the self-check compared the two as plain strings.

freeitsm.sql still carries that space on that very line today, so this was a live trap rather than a historical one. The comparison now strips whitespace from both sides.


4. Why it kept coming back

This is the real fault, and it is not in any of the code above.

The response to #113 was to add a --check mode to the generator:

$ php scripts/gen_db_verify_indexes.php --check
OUT OF DATE: includes/db_verify_indexes.php does not match database/freeitsm.sql.
Run: php scripts/gen_db_verify_indexes.php   then commit BOTH files.
  in the list but not in freeitsm.sql: asset_physical_disks.idx_asset_physical_disks_model

That is exactly the right tool. It names the offending index and exits non-zero. It caught none of the three occurrences, because nothing was obliged to run it โ€” the only thing pointing at it was a checkbox in an internal checklist, and the repository's single CI workflow fires on a version tag, long after the mistake is baked in.

๐Ÿ”‘ A guard a human has to remember is not a guard. The tool was never the missing piece.

.github/workflows/schema-drift.yml now runs on every push and pull request:

  1. gen_db_verify_indexes.php --check โ€” the mirror matches the schema.
  2. Regenerating is a no-op. This is the step that catches a hand-edit, which is what actually happened this time โ€” a comment added to a generated file changes nothing the parser sees, so step 1 alone could miss it.
  3. freeitsm.sql and db_verify_schema.php agree about columns (the hand-maintained pair, which can disagree in either direction).
  4. The index parser's own test suite, controls included.

5. The message itself

Even with all of the above, the message could appear again one day, and it was written for the wrong reader:

The index list is out of date vs freeitsm.sql โ€” run scripts/gen_db_verify_indexes.php and commit both files.

Shown in red, on the screen an administrator opens to find out whether their database is healthy. They have no repository and nothing to commit. Three people reported it as a fault in their installation, which is the honest reading of a red error on that page.

It now says what is true:

Your database is fine and your data is unaffected - this is a fault in the FreeITSM release itself.

The list FreeITSM uses to restore missing database indexes does not match the schema it ships with, which means this installation may be missing an index. Nothing here is lost or at risk, and there is nothing for you to do beyond reporting it: please open an issue at https://github.com/edmozley/freeitsm/issues and paste the lines below. It is a one-file fix.

and it is a warning rather than an error. The loud alarm belongs in CI, where somebody can act on it.

Three things that turned up underneath

Demoting the row to warning exposed gaps that had been there since the mailbox checks introduced that status, and had never been looked at:

Missing Effect
.status-pill.warning bare text where every neighbouring status had a coloured pill
a Warnings summary card warning rows were counted internally and never displayed, so the only way to find one was to scroll
.tcard-dot.warning fell through to the base dot colour, which is green โ€” a warning card looked exactly like OK at a glance

All three are fixed, light and dark.


6. Files changed

File Change
database/freeitsm.sql KEY idx_asset_physical_disks_model (model(100)) added to asset_physical_disks
includes/db_verify_indexes.php regenerated โ€” 373 indexes, duplicate removed
includes/db_verify_index_parse.php whitespace stripped from both sides of the comparison
api/system/db_verify.php both drift rows reworded for an administrator; error โ†’ warning
system/db-verify/index.php warning pill, warning summary card, warning card dot (light + dark)
lang/en/system.php count_warnings
tests/db-verify-indexes/run.php section 5 โ€” formatting is not drift, with four controls
.github/workflows/schema-drift.yml new โ€” four checks on every push

7. How it was proved

The drift reproduces on main. Before any change, the self-check reported the error on the current repository, so 2.0.0 would have shipped it for a fourth time.

The CI catches the bug that shipped. The index was removed from freeitsm.sql again and the workflow's first step run:

OUT OF DATE: includes/db_verify_indexes.php does not match database/freeitsm.sql.
  in the list but not in freeitsm.sql: asset_physical_disks.idx_asset_physical_disks_model

exit code 1, and the parser suite went from 32 passed to 26 passed, 1 failed. The self-check reproduced the reported sentence verbatim. Then restored.

Nothing was lost in the regenerate. The old and new mirrors were compared as sets rather than as lines: 374 rows before (373 distinct), 373 after, nothing lost, nothing gained, one duplicate removed.

Formatting tolerance did not become blindness. The whitespace change ships with four controls โ€” a reordered column list, a KEY promoted to UNIQUE, an index missing from the list, an index present only in the list โ€” each of which must still be reported as drift, and is.

The screen was driven in a real browser, with the new message text extracted from api/system/db_verify.php rather than retyped, so the fixture cannot drift from what ships. 16 assertions: the Warnings card appears and counts; the card dot is not the same colour as an OK card; the detail shows a styled Warning pill; the reassuring sentence is on screen; the developer instruction is gone; the issue link is present; and clicking the row still does not ask the database to describe a table called "index backfill list" โ€” the other half of #113.

โš ๏ธ The first run of that browser check failed four assertions against selectors for a results table. The page renders a card grid. That was a bug in the test, not the product, and it is the reason the assertions above name .tcard and #detailStatus.