Remove libkrun patches, buffer tarball writes for next unstable - #923
Conversation
📝 WalkthroughWalkthroughThe PR removes compressed-byte accounting from tar.zstd uploads, buffers and finalizes streamed writes, simplifies unpack-failure errors, signals vsock credit requests, and makes macOS libkrun patch hashing conditional. ChangesWorkspace upload streaming
Vsock credit signaling
Libkrun patch hashing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/minimal/src/file_upload.rs (3)
64-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider naming the buffer-size constant.
4 * 1024is an inline magic number right next to the namedPIPE_BUFconstant; extracting a matchingconst UPLOAD_BUF: usize = 4 * 1024;would keep the two buffer sizes equally self-documenting.🤖 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 `@crates/minimal/src/file_upload.rs` at line 64, In the buffer setup near the existing PIPE_BUF constant, introduce a named UPLOAD_BUF constant with the current 4 * 1024 value and update BufWriter::with_capacity to use UPLOAD_BUF instead of the inline expression.
64-64: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueExplicit
flush()beforeshutdown()is redundant.
AsyncWrite::shutdown/BufWriter::poll_shutdownalready flush any buffered data before shutting down the inner writer, so callers don't need to call flush first — Invocation of a shutdown implies an invocation of flush, and once this method returns Ready it implies that a flush successfully happened before the shutdown happened, so callers don't need to call flush before calling shutdown. Not a bug, just an unnecessary extra step/error context.♻️ Optional simplification
- w.flush().await.context("flushing upload stream")?; - w.shutdown().await.context("shutting down upload stream")?; + w.shutdown().await.context("shutting down upload stream")?;Also applies to: 68-69
🤖 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 `@crates/minimal/src/file_upload.rs` at line 64, Remove the explicit flush call before shutdown in the file upload writer flow using BufWriter, including the corresponding handling on the additionally referenced lines. Rely on AsyncWrite::shutdown to flush buffered data before closing, while preserving the existing shutdown/error propagation behavior.
48-77: 🧹 Nitpick | 🔵 TrivialVerify integration/e2e coverage for this daemon-path change.
This changes the wire behavior of the workspace-upload path to
minimald(buffering, explicit flush/shutdown) and removes the unit tests that previously exercised byte-count/mid-stream-failure behavior. As per coding guidelines, "When changing VM or daemon paths, run the relevant integration coverage:just e2eand/orjust test-vm" and "Do not rely only on unit tests for VM/networking behavior; preserve and run the applicable integration and root-integration harnesses." Please confirmjust e2e/just test-vmwere run against this change (noted as intended to be tested alongside PR#922).🤖 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 `@crates/minimal/src/file_upload.rs` around lines 48 - 77, Add or preserve integration coverage for the daemon workspace-upload path affected by stream_tar_zstd, including buffering, flush/shutdown, and mid-stream failure behavior. Run the applicable harnesses, specifically just e2e and/or just test-vm, against this change and record the results; do not rely solely on unit tests.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@crates/minimal/src/file_upload.rs`:
- Line 64: In the buffer setup near the existing PIPE_BUF constant, introduce a
named UPLOAD_BUF constant with the current 4 * 1024 value and update
BufWriter::with_capacity to use UPLOAD_BUF instead of the inline expression.
- Line 64: Remove the explicit flush call before shutdown in the file upload
writer flow using BufWriter, including the corresponding handling on the
additionally referenced lines. Rely on AsyncWrite::shutdown to flush buffered
data before closing, while preserving the existing shutdown/error propagation
behavior.
- Around line 48-77: Add or preserve integration coverage for the daemon
workspace-upload path affected by stream_tar_zstd, including buffering,
flush/shutdown, and mid-stream failure behavior. Run the applicable harnesses,
specifically just e2e and/or just test-vm, against this change and record the
results; do not rely solely on unit tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b77d9dcb-cc7d-456e-8d4e-5c882898bb7f
📒 Files selected for processing (4)
crates/minimal/src/client.rscrates/minimal/src/file_upload.rsvendor/libkrun/patches/0001-vsock-signal-the-used-queue-when-requesting-credit.patchvendor/libkrun/patches/0002-vsock-fill-the-rx-descriptor-instead-of-one-recv-per-packet.patch
💤 Files with no reviewable changes (2)
- vendor/libkrun/patches/0002-vsock-fill-the-rx-descriptor-instead-of-one-recv-per-packet.patch
- vendor/libkrun/patches/0001-vsock-signal-the-used-queue-when-requesting-credit.patch
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/actions/setup-libkrun-macos/action.yml:
- Around line 33-37: Update the patches_hash computation to use an explicit
directory check: when vendor/libkrun/patches is absent, hash empty input;
otherwise run find and shasum without || true so any hashing failure propagates
and fails the step. Preserve the existing sorting and final SHA-256 aggregation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3eb711ee-37a8-49e8-a66f-2ddbc8a5baba
📒 Files selected for processing (1)
.github/actions/setup-libkrun-macos/action.yml
| # still deterministic. The dir may be ABSENT (git drops empty dirs once | ||
| # the last patch is removed); guard with `-d` so a missing directory | ||
| # yields empty input rather than a non-zero `find` that pipefail turns | ||
| # into a step failure. | ||
| patches_hash="$( { [ -d vendor/libkrun/patches ] && find vendor/libkrun/patches -name '*.patch' -type f -exec shasum -a 256 {} + || true; } | sort | shasum -a 256 | cut -d' ' -f1)" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not suppress hashing failures.
The -d guard already handles a missing patch directory, but || true also converts real find/shasum failures into the valid empty-input hash. That can collide with the no-patches cache key and reuse a libkrun build whose patches were not successfully hashed.
Use an explicit if/else for the absent-directory case and let hashing errors fail the step.
Proposed fix
- patches_hash="$( { [ -d vendor/libkrun/patches ] && find vendor/libkrun/patches -name '*.patch' -type f -exec shasum -a 256 {} + || true; } | sort | shasum -a 256 | cut -d' ' -f1)"
+ if [ -d vendor/libkrun/patches ]; then
+ patches_hash="$(find vendor/libkrun/patches -name '*.patch' -type f -exec shasum -a 256 {} + | sort | shasum -a 256 | cut -d' ' -f1)"
+ else
+ patches_hash="$(printf '' | shasum -a 256 | cut -d' ' -f1)"
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # still deterministic. The dir may be ABSENT (git drops empty dirs once | |
| # the last patch is removed); guard with `-d` so a missing directory | |
| # yields empty input rather than a non-zero `find` that pipefail turns | |
| # into a step failure. | |
| patches_hash="$( { [ -d vendor/libkrun/patches ] && find vendor/libkrun/patches -name '*.patch' -type f -exec shasum -a 256 {} + || true; } | sort | shasum -a 256 | cut -d' ' -f1)" | |
| # still deterministic. The dir may be ABSENT (git drops empty dirs once | |
| # the last patch is removed); guard with `-d` so a missing directory | |
| # yields empty input rather than a non-zero `find` that pipefail turns | |
| # into a step failure. | |
| if [ -d vendor/libkrun/patches ]; then | |
| patches_hash="$(find vendor/libkrun/patches -name '*.patch' -type f -exec shasum -a 256 {} + | sort | shasum -a 256 | cut -d' ' -f1)" | |
| else | |
| patches_hash="$(printf '' | shasum -a 256 | cut -d' ' -f1)" | |
| fi |
🤖 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 @.github/actions/setup-libkrun-macos/action.yml around lines 33 - 37, Update
the patches_hash computation to use an explicit directory check: when
vendor/libkrun/patches is absent, hash empty input; otherwise run find and
shasum without || true so any hashing failure propagates and fails the step.
Preserve the existing sorting and final SHA-256 aggregation.
… packets The unix-vsock proxy issues one recv() per RX descriptor and emits whatever that single read found buffered. Against a bulk sender that hands the stream over in small chunks, the muxer drains the socket faster than the writer fills it, so descriptors carry a fraction of their capacity and the stream fragments into sub-KiB packets while the peer's window is wide open. Linux charges every queued packet SKB_TRUESIZE(0) against buf_alloc regardless of payload -- 576 bytes on arm64 -- so a stream whose mean packet falls below that exhausts the receiver's queue budget long before its byte budget. At a 256 KiB window the queue caps at 455 packets, and since 6.12.92 the receiver resets the connection (ENOBUFS) on the 456th rather than dropping it. That is the failure behind bulk host->guest uploads. Fill the descriptor rather than emit one recv()'s worth, and -- because draining alone does not help when the writer is slower than we are across wakeups -- wait briefly on EAGAIN while still below a packet worth carrying. poll() returns as soon as more data lands, so a fast writer pays nothing, a slow one costs at most 4 ms, and data is always delivered rather than held. Instrumented on both the guest rejection path and libkrun's emit path: 86% of sub-576-byte packets came from the fill loop exiting on EAGAIN with descriptor space and credit both free, not from the credit clamp (14%, tail only) and never from the descriptor size (constant 3732 B). A/B with libkrun the only variable: 0 of 10 uploads completed without this, 16 of 16 with it, and the instrumented kernel logged no rejections. This is a single self-contained patch against the pinned libkrun commit, numbered 0001 because #923 removed the earlier fill-loop patches from the tree. It subsumes their fill-loop mechanics; it does not re-carry the separate signal-used-queue credit-request fix, which is worth its own PR. Refs: #869
… packets (#921) The unix-vsock proxy issues one recv() per RX descriptor and emits whatever that single read found buffered. Against a bulk sender that hands the stream over in small chunks, the muxer drains the socket faster than the writer fills it, so descriptors carry a fraction of their capacity and the stream fragments into sub-KiB packets while the peer's window is wide open. Linux charges every queued packet SKB_TRUESIZE(0) against buf_alloc regardless of payload -- 576 bytes on arm64 -- so a stream whose mean packet falls below that exhausts the receiver's queue budget long before its byte budget. At a 256 KiB window the queue caps at 455 packets, and since 6.12.92 the receiver resets the connection (ENOBUFS) on the 456th rather than dropping it. That is the failure behind bulk host->guest uploads. Fill the descriptor rather than emit one recv()'s worth, and -- because draining alone does not help when the writer is slower than we are across wakeups -- wait briefly on EAGAIN while still below a packet worth carrying. poll() returns as soon as more data lands, so a fast writer pays nothing, a slow one costs at most 4 ms, and data is always delivered rather than held. Instrumented on both the guest rejection path and libkrun's emit path: 86% of sub-576-byte packets came from the fill loop exiting on EAGAIN with descriptor space and credit both free, not from the credit clamp (14%, tail only) and never from the descriptor size (constant 3732 B). A/B with libkrun the only variable: 0 of 10 uploads completed without this, 16 of 16 with it, and the instrumented kernel logged no rejections. This is a single self-contained patch against the pinned libkrun commit, numbered 0001 because #923 removed the earlier fill-loop patches from the tree. It subsumes their fill-loop mechanics; it does not re-carry the separate signal-used-queue credit-request fix, which is worth its own PR. Refs: #869
Conbined with #922 this should be a good next test
Note
Remove libkrun vendor patches and buffer tar writes in workspace upload
stream_tar_zstdin file_upload.rs to returnResult<()>instead of a compressed-byte count; removes theCountingWriterwrapper and throughput logging.BufWriterbefore the underlying writer in the upload path to buffer tar writes.Changes since #923 opened
patches_hashcomputation in the 'Resolve libkrun pin' step to conditionally check for the existence ofvendor/libkrun/patchesdirectory before runningfindandshasumcommands [0833018]Macroscope summarized 50c10d2.
Summary by CodeRabbit