Skip to content

Remove libkrun patches, buffer tarball writes for next unstable - #923

Merged
twitchyliquid64 merged 3 commits into
mainfrom
tom/session-sop
Jul 22, 2026
Merged

Remove libkrun patches, buffer tarball writes for next unstable#923
twitchyliquid64 merged 3 commits into
mainfrom
tom/session-sop

Conversation

@twitchyliquid64

@twitchyliquid64 twitchyliquid64 commented Jul 22, 2026

Copy link
Copy Markdown
Member
  • Removes the libkrun patches which are confirmed not to fix the issue
  • Buffer tarball writes to 4k so they arent a bunch of tiny writes

Conbined with #922 this should be a good next test

Note

Remove libkrun vendor patches and buffer tar writes in workspace upload

  • Removes two vsock-related patch files from vendor/libkrun/patches.
  • Simplifies stream_tar_zstd in file_upload.rs to return Result<()> instead of a compressed-byte count; removes the CountingWriter wrapper and throughput logging.
  • Introduces a BufWriter before the underlying writer in the upload path to buffer tar writes.
  • Updates client.rs to drop byte-count reporting from the daemon unpack failure error message.
  • Behavioral Change: upload errors no longer include transferred-byte counts or elapsed-time details; callers no longer receive a compressed-byte count on success.

Changes since #923 opened

  • Modified patches_hash computation in the 'Resolve libkrun pin' step to conditionally check for the existence of vendor/libkrun/patches directory before running find and shasum commands [0833018]

Macroscope summarized 50c10d2.

Summary by CodeRabbit

  • Bug Fixes
    • Improved workspace upload failures with clearer, more focused unpack error details.
    • Fixed virtio-vsock credit updates so the receiving side reliably wakes when new credits are requested.
    • Improved vsock data reception by filling available buffers to reduce fragmentation.
    • Preserved and surfaced already-received data during connection pauses, closes, or recoverable errors.
  • Chores
    • Made the macOS setup workflow compute patch hashing more robustly when no patches directory is present.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The 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.

Changes

Workspace upload streaming

Layer / File(s) Summary
Buffered tar.zstd upload flow
crates/minimal/src/file_upload.rs, crates/minimal/src/client.rs
stream_tar_zstd now returns unit success, buffers and finalizes writes, and workspace unpack errors omit uploaded byte counts.

Vsock credit signaling

Layer / File(s) Summary
Credit-request queue signaling
vendor/libkrun/patches/0001-vsock-signal-the-used-queue-when-requesting-credit.patch
TsiStreamProxy and UnixProxy mark credit-request updates for used-queue signaling.

Libkrun patch hashing

Layer / File(s) Summary
Conditional patch hash generation
.github/actions/setup-libkrun-macos/action.yml
Patch files are hashed only when the vendor patch directory exists.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

  • gominimal/minimal#902 — Updates the same macOS setup action to hash libkrun patch files for cache-key correctness.

Possibly related PRs

Suggested reviewers: norrietaylor

Poem

A rabbit buffers tar on a bright little stream,
While credit queues wake from a snoozy dream.
Errors grow plain, and patches hash right,
The upload path hops cleanly tonight.
Hop, hop—what a tidy delight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers the changes, but it omits the required Summary, Testing, and Checklist sections from the template. Rewrite the PR body using the template headings, add testing commands/output, and complete the checklist items.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title matches the main change set: removing libkrun patches and buffering tar writes.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@jtnkminimal jtnkminimal left a comment

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.

Stampy.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (3)
crates/minimal/src/file_upload.rs (3)

64-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider naming the buffer-size constant.

4 * 1024 is an inline magic number right next to the named PIPE_BUF constant; extracting a matching const 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 value

Explicit flush() before shutdown() is redundant.

AsyncWrite::shutdown/BufWriter::poll_shutdown already 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 | 🔵 Trivial

Verify 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 e2e and/or just 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 confirm just e2e/just test-vm were 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

📥 Commits

Reviewing files that changed from the base of the PR and between ff79c8c and 50c10d2.

📒 Files selected for processing (4)
  • crates/minimal/src/client.rs
  • crates/minimal/src/file_upload.rs
  • vendor/libkrun/patches/0001-vsock-signal-the-used-queue-when-requesting-credit.patch
  • vendor/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

@twitchyliquid64
twitchyliquid64 enabled auto-merge (squash) July 22, 2026 22:30

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 50c10d2 and 0833018.

📒 Files selected for processing (1)
  • .github/actions/setup-libkrun-macos/action.yml

Comment on lines +33 to +37
# 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)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ 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.

Suggested change
# 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.

@twitchyliquid64
twitchyliquid64 merged commit c01513b into main Jul 22, 2026
29 checks passed
@twitchyliquid64
twitchyliquid64 deleted the tom/session-sop branch July 22, 2026 22:50
norrietaylor added a commit that referenced this pull request Jul 22, 2026
… 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
norrietaylor added a commit that referenced this pull request Jul 22, 2026
… 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
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.

2 participants