Skip to content

fix(cli): resolve infinite loop and output bugs in log merging#743

Merged
doringeman merged 2 commits intodocker:mainfrom
doringeman:fix-logs
Mar 9, 2026
Merged

fix(cli): resolve infinite loop and output bugs in log merging#743
doringeman merged 2 commits intodocker:mainfrom
doringeman:fix-logs

Conversation

@doringeman
Copy link
Contributor

When timestamp1 == timestamp2, neither Before check is true, so neither inner loop executes. The outer loop condition is still true, creating an infinite loop. This happens frequently in practice since the service and engine logs often emit lines at the same time.

Without the second commit, you can easily reproduce this.

$ git reset --hard HEAD~1
HEAD is now at ab173d60 test(cli): add regression test for log merge equal-timestamp deadlock

$ go test ./cmd/cli/commands/ -run TestMergedLog -v
=== RUN   TestMergedLogEqualTimestamps
    logs_test.go:23: printMergedLog hung — equal timestamp deadlock
--- FAIL: TestMergedLogEqualTimestamps (3.00s)
FAIL
FAIL	github.com/docker/model-runner/cmd/cli/commands	3.499s
FAIL

$ git reset --hard HEAD@{1}
HEAD is now at a1baa3c7 fix(cli): resolve infinite loop and output bugs in log merging

$ go test ./cmd/cli/commands/ -run TestMergedLog -v -timeout 10s
=== RUN   TestMergedLogEqualTimestamps
--- PASS: TestMergedLogEqualTimestamps (0.00s)
=== RUN   TestMergedLogInterleavedTimestamps
--- PASS: TestMergedLogInterleavedTimestamps (0.00s)
PASS
ok  	github.com/docker/model-runner/cmd/cli/commands	0.520s

printMergedLog would infinite-loop when two log files contained lines with identical timestamps, since neither inner loop condition advanced.

Signed-off-by: Dorin Geman <dorin.geman@docker.com>
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively resolves a critical infinite loop bug in the log merging logic that occurred when log entries had identical timestamps. The fix is correct and also includes a regression test to prevent this issue in the future. Additionally, the changes address an output bug where the last log line could be dropped, and refactors the code to improve testability by using io.Writer and replacing println with fmt.Fprintln. The introduction of new tests significantly improves the robustness of the log merging functionality. I've noted a minor improvement in the new test file to handle potential errors during test setup.

Signed-off-by: Dorin Geman <dorin.geman@docker.com>
@doringeman doringeman merged commit b129b24 into docker:main Mar 9, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants