refactor(logs): read every log tail through one bounded, lossy routine by jussielo-amd · Pull Request #923 · ROCm/rocm-cli

jussielo-amd · 2026-11-05T17:25:43Z

Summary

Delivers ROCMAI-879. Three places read the tail of a log their own way, and the seek-bounded routine in rocm-core that the engine adapters use served none of them: the ComfyUI log view read the whole file, decoded sixteen-bit text and stripped ANSI; the services text module streamed every line and failed on any byte that is not valid UTF-8; and the rocmd MCP service_logs tool read the whole file into one string and failed the same way. The visible symptoms were that one stray byte turned rocm services logs / rocm logs into a single <failed to read ...> placeholder line (and the MCP tool call into an error), and that a multi-gigabyte log was read in full just to show its last few lines.

All three now go through rocm_core::tail_lines_decoded, which takes a LogDecode option: PLAIN (UTF-8, what the engines and the service logs use) or CONSOLE (UTF-16 little-endian detected by a byte order mark or by the zero-byte pattern the old code used, plus ANSI stripping, what the ComfyUI view used). The read is bounded by a byte budget, the window starts on a character boundary (for UTF-16 too), a partial first line is dropped, and invalid text is decoded lossily. The log views share one LOG_VIEW_MAX_TAIL_READ budget of 4 MiB. The routine lives in rocm-core because rocmd must not depend on rocm.

The private readers (decode_log_bytes, looks_like_utf16_le, the ComfyUI strip_ansi_sequences, and the stream/whole-file bodies in the other two) are deleted. Each of the three callers keeps a thin adapter that picks a LogDecode, passes the shared budget and adds its own error label; none has a reader of its own.

What moved

Was	Now
`comfyui.rs` `read_tail_lines` body, `decode_log_bytes`, `looks_like_utf16_le`, `strip_ansi_sequences`	`rocm_core::tail_lines_decoded` with `LogDecode::CONSOLE`; `strip_ansi_sequences` is exported from `rocm-core` (the doc note in `tool_policy.rs` points there now)
`services_text.rs` `read_tail_lines` body (streams with `BufRead::lines`)	`rocm_core::tail_lines` (`LogDecode::PLAIN`); the function stays as a labelled adapter because `chat.rs`, `service_launch.rs` and `serve_summary.rs` call it
`rocmd` `mcp.rs` `read_tail_lines` body (`read_to_string`)	`rocm_core::tail_lines` (`LogDecode::PLAIN`), joined with newlines as before
`rocm_core::tail_lines`	unchanged signature, now a shorthand for `tail_lines_decoded(.., LogDecode::PLAIN)`; engine callers are untouched

Which behaviour wins, and the user-visible differences

Where the three copies disagreed, the shared routine's choices win, because it is the one that is bounded and the one that never fails a log view: a bounded read over a whole-file read, and lossy decoding over a hard error for invalid text. Every difference a user or an MCP client can see:

A log larger than 4 MiB is read from its last 4 MiB instead of whole (all three). Output differs only when the requested lines do not fit in the last 4 MiB.
Invalid eight-bit text no longer fails the view. rocm services logs <id>, rocm logs, the chat tool summaries and the managed-start failure detail showed one <failed to read ...> placeholder line, and the MCP service_logs tool returned an error; they now show the readable lines with U+FFFD in place of the bad bytes. ComfyUI logs were already lossy and are unchanged.
Service logs and the MCP tool still read UTF-8 and keep ANSI sequences; only the ComfyUI view detects UTF-16 and strips ANSI (it is the only log written through a Windows shell redirect). A UTF-16 service log used to be an error and is now shown as lossy text.
ComfyUI ANSI stripping is per line: an unterminated escape sequence ends at the end of its line. Before, it could consume the following lines up to the next letter.
Error wording for a missing log: services text says failed to read <label> <path> with the cause failed to open <path> (it was failed to open <label> <path>); ComfyUI and MCP keep their outer failed to read <path> and now carry failed to open <path> as the cause instead of the bare I/O message.
A limit of 0 returns no lines without opening the file in all three. The MCP reader used to return the whole file for 0 (its window never evicted), and ComfyUI and MCP failed on a missing file before looking at the limit. All callers clamp the limit to at least 1, so only a direct call could see this.

Verification

Ran: cargo fmt --all --check; cargo clippy -p rocm -p rocmd -p rocm-core -p e2e-cucumber --all-targets -- -D warnings (clean); cargo test -p rocm-core -p rocm -p rocmd -p e2e-cucumber --no-fail-fast (all pass; 44 net new unit tests, none lost: the test names in the five touched files were diffed against origin/main); cargo xtask manifest --check; cargo xtask check-crate-edges; cargo xtask e2e -- -n "logs-0" (logs-01 to logs-04 pass); the leak scan against origin/main (no hits on added or removed lines); cargo llvm-cov -p rocm-core --summary-only (88.38% lines overall, log_tail.rs 92.01%, against a committed floor of 82.14%; the floors were not touched and no "before" run was made). Not run: the full workspace gate, the GPU lanes and scripts/smoke_local.py; CI carries them. The Windows cross-check (cargo clippy --target x86_64-pc-windows-msvc) could not run on the Linux author host because the ring and aws-lc-sys build scripts need MSVC's lib.exe; the change adds no cfg(windows) code and its tests build paths with Path::display() and join rather than separators, so the Windows CI job is what exercises them there (the UTF-16 path, the one a Windows shell produces, is unit-tested on every platform).

Each new test was shown to fail when its target is broken, by a targeted edit that was then restored byte for byte: making the shared routine read the whole file turned 6 core tests and 4 caller tests red (large-file bound for the routine, services text, both ComfyUI cases and MCP); removing the UTF-16 alignment or the byte-order-mark exception turned the every-budget UTF-16 test red; disabling ANSI stripping turned the ComfyUI and core console-decode tests red; swapping the ComfyUI decode to PLAIN turned four ComfyUI tests red; putting the old private copies back into the services text and MCP adapters turned 5 services text and 5 MCP tests red, and the new scenario red. The bound itself is shown by a counting reader: a read of an 8 MiB source under a 64 KiB budget touches at most the budget plus one probe byte (plus the 512-byte sniff for UTF-16).

User-visible behaviour

rocm logs --search reads the CLI action logs through the changed path, so the existing scenarios logs-search-reports-match-count (logs-01) and logs-search-absent-term-no-matches (logs-02) cover that output and pass unchanged; logs-service-and-search-conflict (logs-03) does not reach the reader. The changed behaviour with no scenario was a stray invalid byte, so this adds logs-invalid-byte-keeps-matching-lines (logs-04): an action log with one invalid byte still yields all nine matching lines; it fails with the old services text reader and passes now. It runs on the mock lane every PR (no GPU, no network). There is no scenario for rocm comfyui logs (its output is unchanged apart from the 4 MiB window and the per-line ANSI edge, which are covered by unit tests through the adapter) or for the rocmd MCP service_logs tool (no existing step drives the rocmd MCP server; covered by unit tests through the adapter).

Notes for reviewers

Read the commits in order. The first commit only adds tests that pass on the unmodified code and pin each reader's behaviour (large file, UTF-16 with ANSI, invalid text, missing file, limit 0, fewer lines than the limit, no trailing newline, CRLF); the two readers that read whole files return a line far outside any window there, which is the divergence. The second commit converges the three callers on the routine, flips the pinned divergences to the new behaviour (the diff of those tests is the clearest list of what changed), adds the routine's own tests and updates docs/architecture.md. The third commit adds the scenario. The two merge commits after that only bring in current main; nothing they bring touches a file this change touches except an unrelated line of docs/architecture.md, which merged cleanly.

Two judgement calls worth a look: the UTF-16 window is rounded up to an even offset and a byte-order mark is not mistaken for the end of a previous line (both pinned by the every-budget test), and strip_ansi_sequences is now public in rocm-core because a doc note in tool_policy.rs already named it as the tool for assistant prose.

Three modules read the last lines of a log their own way: the ComfyUI log view (whole file, sixteen-bit decode, ANSI strip, lossy text), the services text module (streamed, fails on invalid text) and the rocmd MCP service_logs tool (whole file into one string, fails on invalid text), while the seek-bounded routine in rocm-core serves only the engines. Add characterisation tests that pass on the unmodified code and document the differences before they are removed: a large file (the whole-file readers return a line far outside any tail window; the shared routine does not), a sixteen-bit little-endian log with ANSI colour codes, invalid eight-bit text, a missing file, a limit of zero, fewer lines than the limit, a final line without a trailing newline, and CRLF endings. Test-only; no behaviour change. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

The ComfyUI log view, the services text module and the rocmd MCP service_logs tool each read a log tail their own way, while the seek-bounded rocm_core::tail_lines served only the engines. Converge all of them on it. rocm_core::log_tail gains tail_lines_decoded, taking a LogDecode: PLAIN (UTF-8, what the engines and the service logs use) or CONSOLE (UTF-16 little-endian detected by byte order mark or zero-byte pattern, ANSI sequences stripped; what the ComfyUI log view used). The UTF-16 path keeps the seek-boundary handling: the window starts on a character boundary and a partial first line is dropped. strip_ansi_sequences is exported for the assistant-prose note in tool_policy. A shared LOG_VIEW_MAX_TAIL_READ budget (4 MiB) is what the three log views pass. The private readers are deleted; each caller keeps a thin adapter that picks a LogDecode and its own error label. User-visible differences: - A log larger than 4 MiB is read from its last 4 MiB, not whole (all three). - Invalid UTF-8 no longer fails the view: `rocm services logs` and `rocm logs` showed one "<failed to read ...>" placeholder line and the MCP service_logs tool returned an error; both now show the readable text with U+FFFD for the bad bytes. - Service logs and the MCP tool still read UTF-8 and keep ANSI sequences; a UTF-16 service log is now shown as lossy text rather than as an error. - ComfyUI logs: ANSI stripping is per line, so an unterminated escape sequence ends at its line instead of swallowing the lines after it. - Error wording: a missing log is "failed to read <label> <path>" (cause: "failed to open <path>") in services text, was "failed to open <label> <path>"; ComfyUI and MCP name the open step in the cause. - A limit of 0 returns no lines without opening the file in all three (the MCP reader used to return the whole file; ComfyUI and MCP used to fail on a missing file first). Callers clamp the limit, so only direct calls see it. The characterisation tests from the previous commit are updated to the new behaviour, and new tests cover the bound (a counting reader shows a read touches no more than its budget), every budget from nothing to the whole file for UTF-8 and UTF-16, and each caller through the shared routine. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

`rocm logs --search` reads the CLI's action logs through the shared log-tail routine. Add scenario logs-04: an action log with one byte that is not valid text still yields every matching line. Before the log views were converged on the shared routine, that byte made the whole file collapse into a single "failed to read" placeholder line and the search found nothing. The planting step gains an option for the stray byte; the existing steps and the match-count assertion are reused. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…e-the-shared-routine Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Copilot

🟡 Changes recommended

Active issues leave UTF-8 reads unbounded during concurrent appends, misreport nonempty logs as empty, and allow trailing ANSI resets to evict the last meaningful log line.

3 open issues

What changed in this PR

Centralizes log-tail reading in rocm-core so CLI log views and the daemon’s MCP tool share bounded, lossy decoding without introducing a dependency from rocmd to rocm.

Changes:

Adds shared decoding options for plain UTF-8 and ComfyUI console logs, with a 4 MiB log-view budget.
Replaces three private readers with thin adapters while preserving engine callers’ existing API.
Adds boundary and decoding tests, an invalid-byte search scenario, and architecture documentation.

File	Description
`tests/e2e-cucumber/tests/e2e/logs_steps.rs`	Adds an invalid-byte action-log fixture.
`tests/e2e-cucumber/features/logs.feature`	Covers searching logs containing invalid text.
`docs/architecture.md`	Documents the shared reader and its callers.
`crates/rocm-core/src/log_tail.rs`	Implements decoding options and adds tail-reader tests.
`crates/rocm-core/src/lib.rs`	Exports the shared decoding API and budget.
`apps/rocmd/src/mcp.rs`	Migrates MCP service logs to the shared reader.
`apps/rocm/src/tool_policy.rs`	Updates a documentation reference to the ANSI helper.
`apps/rocm/src/services_text.rs`	Migrates service and action-log reads and adds tests.
`apps/rocm/src/comfyui.rs`	Replaces private decoding with the console adapter and adds tests.

_{Review effort: Balanced}

+    rocm_core::tail_lines_decoded(
+        path,
+        limit,
+        rocm_core::LOG_VIEW_MAX_TAIL_READ,
+        rocm_core::LogDecode::CONSOLE,


-            raw_line.pop();
-            if raw_line.last() == Some(&b'\r') {
+    } else {
+        let mut reader = BufReader::new(source);


+            if decode.strip_ansi {
+                strip_ansi_sequences(&line)


…ls-use-the-shared-routine Signed-off-by: Jussi Elo <jussi.elo@amd.com> # Conflicts: # apps/rocm/src/services_text.rs

… trailing escape fragment Two ways the shared tail read disagreed with what it advertises, both found in review. The UTF-8 branch seeked from the size taken by the earlier stat and then read to the current end of the file, so a log whose writer kept appending was consumed past the byte budget by however much it had grown. The sixteen-bit branch already capped its read with take(size - window_start); the UTF-8 branch now does the same. The new test hands the routine a source that grows on its first read and asserts the lines stop where the snapshot ended and the bytes touched stay within the window; it is red with the cap removed. A console log that ends in a colour reset after its last newline ended in an unterminated fragment made only of an escape sequence. The routine counted it as a line, so with a limit of one it evicted the real last line and returned an empty string, where the ComfyUI reader it replaced (strip ANSI from the whole text, then split) returned the line. A final unterminated fragment that strips to nothing is no longer counted when ANSI stripping is on; a terminated blank line, including one made only of an escape sequence, is still a line, and plain decoding strips nothing so it keeps the fragment. The tests cover UTF-8 and UTF-16 with and without a byte order mark, and compare the result with a copy of the old reader on inputs where the two agree. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…utput The log views read at most the last 4 MiB of a log and drop a partial first line. When the newest line is itself longer than that bound the window holds only a fragment of it, so the read returned no lines and the views printed "The saved log is empty." (rocm comfyui logs) and "Recent output: no output yet" (rocm services logs) for a file that is not empty. The byte bound stays. rocm-core gains tail_log_decoded, which returns the same lines in a LogTail that also records how many bytes at the start of the file the budget left unread, and LogTail::newest_line_exceeds_the_read_bound tells an empty tail of a log with content from an empty log (or a limit of zero). The two views use it: an empty file still says it is empty, and a log whose newest line does not fit says so and names the bound, derived from LOG_VIEW_MAX_TAIL_READ so the number cannot drift from it. The unit tests and four new scenarios (logs-05 to logs-08, one pair per view) assert each message together with the log on disk that it describes: the size and the absence of a newline for the long line, zero bytes for the empty file. The rocmd MCP service_logs tool and the chat and launch-failure readers print no claim about emptiness, so they are unchanged. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd · 2026-11-06T07:23:54Z

Superseded by #929: this pull request's commits are included in that branch (head ef5155b), and #929's description covers this change. Closing so the review happens in one place.

jussielo-amd added 5 commits November 5, 2026 17:05

Merge remote-tracking branch 'origin/main' into refactor-log-tails-us…

fe12448

…e-the-shared-routine Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Merge remote-tracking branch 'origin/main' into refactor-log-tails-us…

640e4c5

…e-the-shared-routine Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd marked this pull request as ready for review November 5, 2026 17:25

jussielo-amd requested a review from a team as a code owner November 5, 2026 17:25

jussielo-amd requested review from johnl-amd and a balanced review from Copilot November 5, 2026 17:25

Copilot started reviewing on behalf of jussielo-amd November 5, 2026 17:26

View session

Copilot AI reviewed Nov 5, 2026

View reviewed changes

Merge branch 'main' of github.com:ROCm/rocm-cli into refactor-log-tai…

e3cbccb

…ls-use-the-shared-routine Signed-off-by: Jussi Elo <jussi.elo@amd.com> # Conflicts: # apps/rocm/src/services_text.rs

jussielo-amd mentioned this pull request Nov 5, 2026

refactor(rocm): move the comfyui dispatch function out of main.rs #938

Closed

jussielo-amd added 2 commits November 6, 2026 06:04

jussielo-amd closed this Nov 6, 2026

Navigation Menu

Repository navigation

refactor(logs): read every log tail through one bounded, lossy routine - #923

refactor(logs): read every log tail through one bounded, lossy routine#923
jussielo-amd wants to merge 8 commits into
mainfrom
refactor-log-tails-use-the-shared-routine

jussielo-amd commented Nov 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

jussielo-amd commented Nov 6, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jussielo-amd commented Nov 5, 2026

Summary

What moved

Which behaviour wins, and the user-visible differences

Verification

User-visible behaviour

Notes for reviewers

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

jussielo-amd commented Nov 6, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants