fix: Trim tar record padding to avoid broken-pipe failure on Podman#1684
Conversation
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
WalkthroughReplaced Temporalio solution project entries with Temporal equivalents. Added internal Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Testcontainers/Containers/TarOutputMemoryStream.cs`:
- Line 45: Replace the platform-dependent Encoding.Default used in the
TarOutputMemoryStream constructor base call with Encoding.UTF8 to ensure tar
entry names are encoded as UTF-8; locate the constructor in class
TarOutputMemoryStream and change the base(...) invocation that currently passes
Encoding.Default to pass Encoding.UTF8 instead so filenames remain consistent
and non-ASCII-safe across platforms.
In `@Testcontainers.slnx`:
- Line 69: This change includes an out-of-scope project rename: the solution
entry for Testcontainers.Temporalio (and its test project) was renamed to
Testcontainers.Temporal; either revert the rename in this branch so the bugfix
PR only contains the TarOutputMemoryStream.cs fix, or split the rename into a
separate commit/PR and update the PR description to document the rename; look
for the solution entries referencing "Testcontainers.Temporalio" and
"Testcontainers.Temporal" (and their corresponding test project names) and
either restore the original names in this PR or move those edits into a distinct
refactor commit/PR and note it in the PR description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: db28a0d0-dcdf-46f4-856f-06e4efdbc59b
📒 Files selected for processing (2)
Testcontainers.slnxsrc/Testcontainers/Containers/TarOutputMemoryStream.cs
|
By the way, I've fixed the broken reference to the Temporal projects in the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Testcontainers/Images/DockerfileArchive.cs (1)
195-197: LGTM — consistent record sizing withTarOutputMemoryStream.Aligning the Dockerfile archive to a 512 B record size is the right move: the produced archive is also streamed to the build endpoint, so it benefits from the same Podman broken-pipe mitigation.
Optional nit: this is the only place
DockerfileArchive(Images) reaches intoTarOutputMemoryStream(Containers), creating a small cross-module coupling just to share the literal1. If you’d like to keep namespaces independent, a private const local to this file (or a shared internal helper) would work equally well. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Testcontainers/Images/DockerfileArchive.cs` around lines 195 - 197, The code in DockerfileArchive is referencing TarOutputMemoryStream.TarBlockFactor from another module to set the TarOutputStream block factor; to avoid cross-module coupling, introduce a private const int (e.g., TarBlockFactor = 1) inside the DockerfileArchive class/file and use that constant when constructing TarOutputStream instead of TarOutputMemoryStream.TarBlockFactor; update the TarOutputStream(...) call site in DockerfileArchive (where TarOutputMemoryStream.TarBlockFactor is currently used) to use the new private const.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Testcontainers/Images/DockerfileArchive.cs`:
- Around line 195-197: The code in DockerfileArchive is referencing
TarOutputMemoryStream.TarBlockFactor from another module to set the
TarOutputStream block factor; to avoid cross-module coupling, introduce a
private const int (e.g., TarBlockFactor = 1) inside the DockerfileArchive
class/file and use that constant when constructing TarOutputStream instead of
TarOutputMemoryStream.TarBlockFactor; update the TarOutputStream(...) call site
in DockerfileArchive (where TarOutputMemoryStream.TarBlockFactor is currently
used) to use the new private const.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1ae0883f-0108-4999-b850-b87fc91a171d
📒 Files selected for processing (2)
src/Testcontainers/Containers/TarOutputMemoryStream.cssrc/Testcontainers/Images/DockerfileArchive.cs
e72bacc
into
testcontainers:develop
|
Congrats on figuring that out. That must have been fun! 😆 |
|
For future reference: this appears to be a 10x time regression in building a large image (my fault for not paying attention to .dockerignore) on .NET Framework. Not helped by SharpZipLib constantly calling Flush: https://github.com/icsharpcode/SharpZipLib/blob/ff2d7c30bdb2474d507f001bc555405e9f02a0bb/src/ICSharpCode.SharpZipLib/Tar/TarBuffer.cs#L580 netcore is either unaffected or not noticeably enough to measure properly |
PR #1684 lowered the SharpZipLib block factor to 1 to drop the trailing record padding that races Podman's archive reader, causing a broken pipe (HTTP 500) on resource mappings (#1683). It applied this to both archive paths, but the on-disk Dockerfile build context is written to a FileStream, where a 512 B record forces a write and flush per block and regresses image builds by up to 10x (worst on .NET Framework). Only the in-memory archive streamed to the daemon (resource mappings) needs the padding gone, and a MemoryStream pays almost nothing for the small record size (~1.3x, benchmarked). So keep block factor 1 there and revert the Dockerfile build context to SharpZipLib's default block factor. Document the asymmetry so it is not "consistency-fixed" back into a regression, and add a test asserting the streamed archive carries no padding beyond the two end-of-archive blocks. Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
@Rob-Hague If you have some time, could you please test your build with the changes made in this branch: bugfix/trim-tar-padding-keep-block-factor? |
|
Looks good. Average of two runs (with other stuff in the background). 380d3da is a proxy for 4.12 with Docker.DotNet.Enhanced 4.3.3. Basically running this code https://github.com/sshnet/SSH.NET/blob/89f378a0ab99ffc85d3618da7dcc1f3a1aa06d80/test/Renci.SshNet.IntegrationTests/TestsFixtures/InfrastructureFixture.cs#L59-L67
|
|
I'm torn on whether we should keep this workaround or drop it entirely. Looking into this issue, it seems to have been fixed: podman-container-tools/buildah#6678. @artiomchi did you try this back then with the Podman fix already? I think the best approach would be to apply the suggested changes above, mark the workaround as obsolete, and plan to remove it by the end of this year (or on a similar timeline)? |
4 participants
Problem
When running MongoDb replica-set tests (or any container that uses
WithResourceMapping) against a Podman daemon, the container start intermittently fails with:The failure is flaky: it's rare on an idle host, but becomes near-certain when other containers using the same image are already running. In the MongoDb test suite specifically, the four replica-set test classes fail while the twelve non-replica-set tests pass, because the replica-set code path is the only one that calls
WithResourceMapping(to upload a keyfile init script).Fix
Set
blockFactor: 1inTarOutputMemoryStream's base constructor call (src/Testcontainers/Containers/TarOutputMemoryStream.cs).With
blockFactor = 1the record size equals the block size (512 bytes). Each block is flushed to the underlyingMemoryStreamimmediately, andWriteFinalRecordfinds no partial record to pad. The emitted archive is exactly:— no trailing padding. After the subprocess reads the second EOF block and exits, the HTTP sender has nothing more to write. The race window is closed.
The on-the-wire format is unchanged in every other respect. The tar specification does not mandate any particular record size for archive consumers; all conformant readers (GNU tar, BSD tar, Go
archive/tar, Docker's in-process reader, Podman's reader) accept a block factor of 1 archive. This is a strictly smaller byte stream, not a malformed one, so it is safe on Docker as well.Impact
WithResourceMappingcallers benefit, not just MongoDb — any module that uploads bytes or files into a container on Podman was susceptible to the same race.Related issues
References
Root cause
This is a confirmed race condition in Podman's
containers/buildahcopier package — see containers/buildah#6573.Podman's
PUT /containers/{id}/archivehandler pipes the HTTP request body into a tar subprocess viaio.Copy. The subprocess reads the body throughtar.Reader; as soon asNext()returnsio.EOF(after the two 512-byte end-of-archive zero blocks), the subprocess exits and closes the read end of the pipe. If the HTTP sender is still writing bytes at that moment, it gets EPIPE → the 500 above.Our archive producer (
TarOutputMemoryStream, backed by SharpZipLib'sTarOutputStream) uses a block factor of 20 by default, which causes SharpZipLib to pad the finished archive up to the next 10 240-byte record boundary after the two EOF zero blocks. For a small file (like the ~100-byte keyfile init script) that produces roughly 8 KB of trailing zero padding that serves no purpose but gives Podman's race condition something to EPIPE on.The race is timing-dependent: on a busy host the write is slower relative to the read, so the subprocess wins the race more often and the error appears. The upstream Buildah fix (containers/buildah#6678, merged 2026-02-11) drains the full body before exiting the subprocess, but as of this writing it is not yet included in any tagged Buildah or Podman release.
The same failure has been observed in the testcontainers-java ecosystem: testcontainers/testcontainers-java#6640 (closed as "not planned"; workaround was test-level retry).
Summary by CodeRabbit
Bug Fixes
Chores