Skip to content

Fix container logs -n line truncation and four performance defects - #2023

Open
gnavadev wants to merge 3 commits into
apple:mainfrom
gnavadev:perf-audit-fixes
Open

Fix container logs -n line truncation and four performance defects#2023
gnavadev wants to merge 3 commits into
apple:mainfrom
gnavadev:perf-audit-fixes

Conversation

@gnavadev

@gnavadev gnavadev commented Jul 27, 2026

Copy link
Copy Markdown

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Motivation and Context

Fixes #2022.

Five defects, all pre-existing on main. No documented flag, output format, or API
shape changes.

1. container logs -n truncated the first line, and was O(n²).
The tail loop read backwards in 1 KiB chunks, prepending each to a Data buffer and
re-decoding and re-splitting the entire accumulated buffer every iteration. It also
stopped as soon as it held n lines, so the fragment left at a chunk boundary was
printed as though complete.

It now counts line starts only in the bytes just read, joins once at the end, and reads
until it holds more than n lines so the fragment is always discarded.

ContainerLogs.swift and MachineLogs.swift contained byte-for-byte identical 82-line
copies of this, so both carried the same bug. They are now one LogTailer helper in the
shared ContainerCommands module.

2. Globber compiled every pattern twice per call: once via try Regex(...) purely
to validate and discard, and again inside range(of:options:.regularExpression), for
every entry visited during a build-context walk. Patterns are now cached by glob
component. Validation still runs through Regex on the cache-miss path so invalid globs
keep throwing, and matching uses a cached NSRegularExpression, the same engine
range(of:options:) already used underneath.

3. container system df blocked unrelated operations. Both
ContainersService.calculateDiskUsage() and VolumesService.calculateDiskUsage() held a
single global AsyncLock across a synchronous recursive filesystem walk. Both now
snapshot state under the lock, release it, and size bundles in a nonisolated async
helper, which keeps the walk off the actor's executor where it already ran.

4. BuildFSSync used Sequence.contains(where:) against a Set instead of its O(1)
contains(_:), once per entry per directory during tar creation. The store is now a
dictionary keyed by relative path, which also removes a hand-written Hashable
conformance that hashed on only one of the struct's three fields, plus an unused field.

5. container stats sampled serially. Both samples now fan out through a task group
keyed by container ID. Failure handling is unchanged: a failed first sample drops the
container, a failed second keeps the first.

Testing

  • Tested locally
  • Added/updated tests
  • Added/updated docs

Truncation fix verified end to end. Same container, same log file:

# stock release (unfixed)
$ container logs logtest | head -1 | wc -c
    2001
$ container logs -n 3 logtest | head -1 | wc -c
    1011

# this branch
$ bin/container logs -n 3 logtest | head -1 | wc -c
    2001

Added Tests/ContainerCommandsTests/LogTailerTests.swift covering blank lines, a missing
trailing newline, lines longer than the 1 KiB read chunk, and multi-byte UTF-8 straddling
a chunk boundary. Extracting lastLines(fh:n:) from tail(...) is what made this
testable; the previous code only printed.

Full unit suite passes (576 tests, 71 suites). Item 2 is covered by the existing
GlobberTests, including the invalid-pattern cases asserting malformed globs still
throw. Items 3 to 5 have no unit coverage and were verified by manual exercise of
container system df, container stats, and container build.

No documentation update: docs/command-reference.md documents -n as "number of lines
to show from the end of the logs", which is what the fixed code now does faithfully.

Follow-up from review: blank lines are now preserved on both the -n and -f
paths. Fixing the follow path also required buffering partial lines across reads,
which fixes fragments being printed as complete lines and reads being dropped when
a chunk splits a multi-byte character.

return []
}

let lines = text.components(separatedBy: .newlines).filter { !$0.isEmpty }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is dropping empty lines intentional? It changes the original log output and differs from the behavior of the standard tail command, which preserves blank lines.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's part of the original behavior. That's the same components(separatedBy: .newlines) + filter { !$0.isEmpty } this PR deletes from ContainerLogs.swift and MachineLogs.swift

That said, you're right that it's wrong. I'll make sure to change it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Checked the docs first. -n is documented only as "Number of lines to show from the end of the logs" and -f as "Follow log output", neither mentions filtering, and there was no comment on the line. The project does document this kind of filtering when it's intended, e.g. --env-file is described as "ignores # comments and blank lines" in four places. So I've treated it as a bug.

Blank lines are now preserved on both paths, matching tail and matching what container logs without -n already did.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checked the docs first. -n is documented only as "Number of lines to show from the end of the logs" and -f as "Follow log output", neither mentions filtering, and there was no comment on the line. The project does document this kind of filtering when it's intended, e.g. --env-file is described as "ignores # comments and blank lines" in four places. So I've treated it as a bug.

Blank lines are now preserved on both paths, matching tail and matching what container logs without -n already did.

@gnavadev Understood

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.

[Bug]: container logs -n truncates the first line it prints; four additional performance defects

2 participants