Let a stored FOS report be longer than the notification it triggers - #1217
Merged
Conversation
MAX_REASON bounded the report at 500 characters in one place, before it branched, so the same number governed two things that want opposite answers: a push notification, which has to stay readable on a phone, and a stored diagnostic, which wants the whole of what FOS had to say. 500 characters is not a failure trace. Split into two: - MAX_REASON, 500, unchanged, and now applied at the notify() call rather than at the top -- so widening what is stored can never again widen what lands in an administrator's Slack or pushbullet message. Still counted in CHARACTERS: nothing downstream has a byte budget, and cutting a multibyte reason by bytes would silently make it a third as long for anyone not writing in ASCII. - MAX_TEXT, 8192, in BYTES, governing the taskLog row and the fosreports.log line. Bytes, because the only real limit here is the column's and that one is in bytes. taskLog.logText is TEXT -- 65535 bytes -- and sql_mode carries STRICT_TRANS_TABLES, so an oversized value fails the INSERT rather than truncating, and the report is lost rather than shortened. mb_substr counts characters, so an 8192-character cut is 24576 bytes of utf8mb3; the new _limit() uses mb_strcut, which spends the budget the column actually charges for and still never splits a character. 8192 rather than the column's full 65535 because of what this endpoint is: unauthenticated, matched to a host by MAC. Taking the whole column would multiply what one unauthenticated request can write by 130 for no diagnostic gain -- a fog.download trace with its context runs to a few KB, not 64. Generous against any real report, bounded against a caller with something else in mind. Nothing needs migrating; the column has always been this size. text and script arrive sanitized separately and are then joined, so the composed string is re-bounded -- otherwise a report carrying a script name could hand the column twice what it was promised. Verified end to end against an isolated copy of the live database: a 14400 byte report stores as 8192 bytes and pushes 500 characters; 18000 bytes of multibyte stores as 8192 bytes / 4096 characters and is still valid UTF-8; the row round-trips through the ORM byte-identical with no INSERT failure. tests/task-error-report.test.php gains the split's invariants -- MAX_TEXT larger than MAX_REASON, MAX_TEXT within the column, byte-bounding rather than character-bounding, and the three call sites that make the split real. Seven mutations, all killed. One survived the first pass because the assertion matched _logRow's own SIGNATURE as well as its call, so narrowing the argument passed clean; it now pins `self::` and the semicolon. Co-Authored-By: Claude <noreply@anthropic.com>
This was referenced Aug 19, 2026
mastacontrola
added a commit
that referenced
this pull request
Aug 19, 2026
v1.6.11 carries FOGProject/fog-plugins#21, which was merged the morning after v1.6.10 was cut and so has never reached a server. That change matters more than its own PR made it sound, because the FOS reporting work landed in between. #1206/#1211/#1217/#1223 give a failed task a stored, multi-line report of what FOS actually said, and TaskError sends the flattened opening of it as HOST_IMAGE_FAIL's Reason -- but every bundled listener on v1.6.10 ignores that key and pushes the fixed string "This host has failed to image". Confirmed live before cutting the release. A report whose stored row read fog.download: failed to restore partition 2 partclone.ntfs: /dev/sda2 is busy ERROR: win11-split part 2 checksum mismatch exit code 1 (fog.download) pushed, in its entirety: "fos-deploy-test Failed" / "This host has failed to image". So the whole point of storing the trace stopped at the server. One line, because the sha256 is fetched from the release alongside the tarball rather than pinned here; bin/fetch-plugins.sh reads this constant and verifies what it downloads against it. Verified end to end on this pin: "Fetching plugins v1.6.11" -> "Plugins at v1.6.11", and all three imagefail listeners in the fetched tree read Reason. The 1.5 line keeps its plugins in-tree and has no pin, so its equivalent is a code change: #1226. Co-authored-by: JJ Fullmer <7743340+darksidemilk@users.noreply.github.com> 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.
MAX_REASONbounded the FOS report at 500 characters in_sanitize()— before the text branched — so one number governed two things that want opposite answers:MAX_REASON500 characterstaskLog.logTextrow +fosreports.logMAX_TEXT8192 bytes500 characters is not a failure trace. Now they're separate, and
MAX_REASONis applied at thenotify()call rather than at the top — so widening what is stored can never again widen what lands on someone's phone.Why bytes for one and characters for the other
MAX_REASONstays in characters: nothing downstream of it has a byte budget, and cutting a multibyte reason by bytes would silently make it a third as long for anyone not writing in ASCII.MAX_TEXTis in bytes, because the only real limit here is the column's and that one is in bytes.logTextisTEXT— 65535 bytes — andsql_modecarriesSTRICT_TRANS_TABLES, so an oversized value fails the INSERT rather than truncating, and the report is lost rather than shortened.mb_substrcounts characters, so an 8192-character cut is 24576 bytes of utf8mb3 — three times the budget. The new_limit()usesmb_strcut, which spends the budget the column actually charges for and still never splits a character.Why 8192 and not the column's 65535
Because of what this endpoint is: unauthenticated, matched to a host by MAC (it says so in its own class docblock). Taking the whole column would multiply what one unauthenticated request can write by 130 for no diagnostic gain — a
fog.downloadtrace with its context runs to a few KB, not 64. Generous against any real report, bounded against a caller with something else in mind.Nothing needs migrating. The column has always been
TEXT; only the PHP constant was tight.textandscriptarrive sanitized separately and are then joined, so the composed string is re-bounded — otherwise a report carrying a script name could hand the column twice what it was promised.Verification
End to end against an isolated copy of the live database (podman MariaDB, shadow tree), not the live install:
That last line is a full-size report written through the real ORM and read back byte-identical — no INSERT failure, which is the thing the byte-bounding exists to prevent. The probe row was cleaned up.
tests/task-error-report.test.phpgains the split's invariants:MAX_TEXTlarger thanMAX_REASON,MAX_TEXTwithin the column, byte-bounding rather than character-bounding, and the three call sites that make the split real. Seven mutations, all killed — one survived the first pass because the assertion matched_logRow's own signature as well as its call, so narrowing the argument passed clean; it now pinsself::and the semicolon.Full suite: 71 passed, 0 failed.
Not in this change
Control characters are still collapsed to spaces for all paths, so a stored trace is one long line. That guard exists because an embedded newline lets a caller forge a second line in a chat message or a log file — which is true of the notification and
fosreports.log, and not true of a database row rendered in the log modal. Splitting that the way this splits length is a separate change, and it pulls in the modal'swhite-spaceand the grid's preview column.Ported to
dev-branchseparately.🤖 Generated with Claude Code
https://claude.ai/code/session_013mJVe4CpK3rRbi9H5GubXd