logs: wait for logging binary before reading log file#4785
Draft
AkihiroSuda wants to merge 5 commits intocontainerd:mainfrom
Draft
logs: wait for logging binary before reading log file#4785AkihiroSuda wants to merge 5 commits intocontainerd:mainfrom
AkihiroSuda wants to merge 5 commits intocontainerd:mainfrom
Conversation
When a container has exited, `nerdctl logs` now calls WaitForLogger() before reading the JSON log file. This ensures the logging binary (a separate process) has finished writing all log entries to disk. Previously, WaitForLogger was only called in the follow (-f) code path. For non-follow reads like `nerdctl logs --since 60s`, the log file was read immediately without waiting, causing flaky test failures when the logging binary hadn't finished processing the final container output. This fixes TestLogs/since_60s and TestLogs/until_60s which failed intermittently because the log file was empty or incomplete at read time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The logging binary had a goroutine that closed internal pipe writers immediately upon container exit (via gRPC notification). This raced with copyStream, which was still draining data from the external pipe. When the gRPC notification arrived before copyStream finished, the pipe writer was closed mid-copy, causing remaining data to be lost. Fix by making copyStream responsible for closing the pipe writers after all data has been copied. The container-exit goroutine (and its getContainerWait infrastructure) is removed — pipe closure now happens naturally when the parent process exits or the shim closes the pipe. Fixes containerd#4782 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
AkihiroSuda
force-pushed
the
fix-4782-attempt-2
branch
from
e5cdd20 to
715be7f
Compare
… output" This reverts commit 715be7f.
This reverts commit 7cd18d2.
…e reading logs Two fixes for flaky TestLogs/since_60s and TestLogs/until_60s: 1. In loggingProcessAdapter, the container-exit goroutine closed internal pipe writers immediately on container exit. This raced with copyStream goroutines still draining data from external pipes, causing output to be lost. Fix: after container exit, cancel the external readers to unblock copyStream, then wait for copyStream to finish before closing the pipe writers. This ensures all buffered data is processed. 2. In Logs(), call WaitForLogger() before reading log files when the container is not running, ensuring the logging binary has finished writing all entries. Fixes containerd#4782 Co-Authored-By: Claude Opus 4.6 <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
1 participant
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.
When a container has exited,
nerdctl logsnow calls WaitForLogger() before reading the JSON log file. This ensures the logging binary (a separate process) has finished writing all log entries to disk.Previously, WaitForLogger was only called in the follow (-f) code path. For non-follow reads like
nerdctl logs --since 60s, the log file was read immediately without waiting, causing flaky test failures when the logging binary hadn't finished processing the final container output.This fixes TestLogs/since_60s and TestLogs/until_60s which failed intermittently because the log file was empty or incomplete at read time.
Attempt to fix #4782
NOTE: used Claude Code