fix(ci): Docker build image mirrors and concurrent output race - #2884
fix(ci): Docker build image mirrors and concurrent output race#2884Erik Osterman (Cloud Posse) (osterman) wants to merge 3 commits into
Conversation
… ECR The release Docker build job was rate-limited pulling its buildx builder and QEMU binfmt images from public.ecr.aws. Bumps cloudposse/github-action-docker-build-push to v3.1.0, which switches the buildkit image default to mirror.gcr.io, and overrides binfmt-image to the equivalent Google-mirrored tonistiigi/binfmt image. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
no-release
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR updates the Docker build workflow and changes line-prefix writer locking so each write or flush operation emits output without interleaving. ChangesDocker build configuration
Line-prefix writer synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Dependency ReviewThe following issues were found:
License Issues.github/workflows/build.yml
Scanned Files
|
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
writeLine acquired/released the shared writeMu once per line, so a single Write() call that produced multiple lines (e.g. a hook's buffered "\r"-then-"\n" progress update) could have another node's writer interleave a line in between, corrupting concurrent Terraform node output. Hold writeMu for the entire flush instead. Reproduced with `go test -race -count=200` on TestExecuteTerraformConcurrentHooksUseNodeWriters, which was flaking in CI (Acceptance Tests macos, job 92489708710); now passes 200/200. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
size/s
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/io/line_prefix_writer.go (1)
68-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a regression test for multi-line write atomicity.
The adjacent test at
pkg/io/line_prefix_writer_test.go:54-92writes one line perWrite. It cannot detect interleaving between multiple lines emitted by one call. Add a deterministic test where concurrent calls write multiple complete lines and verify that each call's lines remain contiguous.Based on the adjacent test at
pkg/io/line_prefix_writer_test.go:54-92, current coverage does not exercise this lock-scope guarantee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/io/line_prefix_writer.go` around lines 68 - 74, Add a deterministic regression test alongside the existing line-prefix writer concurrency test, exercising concurrent Write calls where each call emits multiple complete lines. Synchronize the writers so both calls overlap, then assert the shared output contains each call’s lines as one contiguous block without interleaving; target the writeMu-protected Write behavior in the line-prefix writer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yml:
- Line 61: Update the binfmt-image configuration in the release build workflow
to avoid depending solely on mirror.gcr.io availability: either add an explicit
runner cache for tonistiigi/binfmt:qemu-v7.0.0 or point binfmt-image to a
registry you control, while preserving the required image tag.
---
Nitpick comments:
In `@pkg/io/line_prefix_writer.go`:
- Around line 68-74: Add a deterministic regression test alongside the existing
line-prefix writer concurrency test, exercising concurrent Write calls where
each call emits multiple complete lines. Synchronize the writers so both calls
overlap, then assert the shared output contains each call’s lines as one
contiguous block without interleaving; target the writeMu-protected Write
behavior in the line-prefix writer.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b41a4161-717b-417e-b970-05bdb4b46f8f
📒 Files selected for processing (2)
.github/workflows/build.ymlpkg/io/line_prefix_writer.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2884 +/- ##
==========================================
+ Coverage 82.76% 82.77% +0.01%
==========================================
Files 1861 1861
Lines 180438 180480 +42
==========================================
+ Hits 149342 149397 +55
+ Misses 23309 23296 -13
Partials 7787 7787
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Erik Osterman (Cloud Posse) (osterman)
changed the title
Erik Osterman (Cloud Posse) (osterman)
added
patch
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
what
cloudposse/github-action-docker-build-pushfrom v3.0.0 to v3.1.0 in the releasedockerjob (.github/workflows/build.yml).binfmt-imageinput tomirror.gcr.io/tonistiigi/binfmt:qemu-v7.0.0.LinePrefixWriter(pkg/io/line_prefix_writer.go), so concurrent Terraform node writers can't interleave a line mid-block.why
moby/buildkit) and QEMU binfmt images frompublic.ecr.aws.mirror.gcr.io/moby/buildkit), fixing that pull. The action'sbinfmt-imageinput still defaults topublic.ecr.aws/eks-distro-build-tooling/binfmt-misceven at v3.1.0 (no upstream fix yet), so it's overridden here directly to the equivalent Google-mirroredtonistiigi/binfmtimage, which publishes the sameqemu-v7.0.0tag. Verified live: bothmirror.gcr.io/tonistiigi/binfmt:qemu-v7.0.0anddocker.io/tonistiigi/binfmt:qemu-v7.0.0resolve to the same digest and pull successfully;mirror.gcr.ioalso falls through to Docker Hub origin on any cache miss, so it's never less reliable than a direct Docker Hub pull.TestExecuteTerraformConcurrentHooksUseNodeWriters(pkg/scheduler/adapters) with a real, reproducible race:LinePrefixWriter.writeLineacquired/released the shared output mutex per line, so a singleWrite()call that produced multiple lines (e.g. a hook's buffered\r-then-\nprogress update) could have a different node's writer interleave a line in between, corrupting concurrent Terraform output. Reproduced withgo test -race -count=200before the fix (intermittent failures) and confirmed 200/200 clean after.references