[PR #431] [MERGED] fix(ublk-daemon): roll back device handoffs the client never receives #433

New issue

Closed

opened 2026-10-29 14:23:22 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:23:22 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/431
Author: @Alex-Wengg
Created: 10/23/2026
Status: ✅ Merged
Merged: 10/27/2026
Merged by: @yingdi-shan

Base: main ← Head: fix/ublk-daemon-undelivered-handoff

📝 Commits (1)

48960c6 fix(ublk-daemon): roll back device handoffs the client never receives

📊 Changes

1 file changed (+121 additions, -2 deletions)

View changed files

📝 storage/ublk-daemon/src/server.rs (+121 -2)

📄 Description

What

Roll back a device the ublk daemon hands to a client when the client never receives the response.

handle_connection now treats the three responses that hand over a device (DeviceAcquired, DeviceCreated, OverlaybdRuntimeDeviceCreated) as a handoff:

If writing the response fails, the client is gone and the handoff is undelivered.
If the write succeeds, the daemon waits for the client to close the connection. A client closes right after reading its one response. On Linux, closing a Unix stream socket with unread data makes the peer's next read fail with ECONNRESET, so a reset means the client gave up after the response was written but before reading it.

An undelivered handoff is undone the way the client would have released it: pooled devices go through handle_release_overlaybd (one acquisition, so one shared reference), and raw devices go through handle_delete.

Why

When an acquire outlives the client's RPC timeout, the daemon records the acquisition, fails to write the response (Broken pipe), and returns. The device or shared reference stays active, and the response was the client's only record of it, so nothing ever releases it. Repeated timeouts accumulate leaked devices and references.

Closes #392

Scope and non-goals

Covers both windows named in the issue: the failed send, and the response written but dropped unread.
No protocol or client change. Existing clients already close the connection after reading the response.
The runtime directory of an undelivered OverlaybdRuntimeDeviceCreated is left in place. It lives under the sandbox work dir, which the agent cleans up when sandbox creation fails, and the client chose that path, so removing it late from the daemon could race a retry.
Not covered: a client that reads the response and then loses it (for example, its caller's future is cancelled after acquire_overlaybd returns). That is the caller's ownership problem.

Design and behavior changes

Rollback happens only on a definite signal (write error, or ECONNRESET on the follow-up read). Any other outcome counts as delivered, because releasing a device the client still uses is worse than leaking one.
For handoff responses, the connection task now stays parked until the client closes the socket instead of returning right after the write. Connection tasks are detached (tokio::spawn), and clients drop the stream as soon as call_inner returns, so this is normally immediate.
Concurrent valid users of a shared device keep their references: the rollback releases exactly the one reference this request added.

Compatibility and operations

Public API or generated protocol: N/A, wire format unchanged.
Configuration or defaults: N/A.
Snapshot manifest, artifact layout, or storage format: N/A.
Upgrade and rollback: daemon-only change; works with current clients.
Host requirements, permissions, ports, or dependencies: relies on Linux AF_UNIX reset semantics, which the daemon (ublk) already requires.

Validation

make fmt
make clippy
make test-unit
Relevant Rust integration tests
make -C services test (required when services/ changes)
Generated clients/server regenerated with the documented make target
Documentation updated
Benchmarks or performance comparison completed

Commands and results (Ubuntu 24.04 on WSL2, kernel 6.18.33.1, Rust 1.99.1):

$ make fmt
(clean)

$ cargo clippy -p uvm-ublk-daemon --all-targets --all-features -- -D warnings
(clean)

$ cargo test -p uvm-ublk-daemon --no-fail-fast
src/lib.rs                 53 passed; 0 failed
tests/ublk_daemon_test.rs  37 passed; 0 failed
(same results as main at 9f76bef, apart from the 4 new tests)

$ cargo test -p uvm-ublk-daemon --lib device_handoff
device_handoff_read_by_client_is_delivered          ok
device_handoff_to_disconnected_client_is_undelivered ok
device_handoff_dropped_unread_is_undelivered         ok
device_handoff_dropped_mid_read_is_undelivered       ok

The new tests run deliver_device_handoff over a real UnixStream::pair(), so they exercise the kernel's reset behavior rather than a mock. With the ECONNRESET check disabled, dropped_unread and dropped_mid_read fail, confirming the tests catch the written-but-unread window.

Skipped checks and reasons:

make clippy (whole workspace) and make test-unit: ran the crate-scoped equivalents above; the rest of the workspace is untouched.
Rollback against real ublk devices: my WSL kernel is built without CONFIG_BLK_DEV_UBLK, so the handle_release_overlaybd / handle_delete rollback calls were not exercised on a live device. They are the existing release paths, called with the dev_id the same request just handed out. The issue's barrier-based regression test (abandon a shared acquire while another client holds the device) would need a ublk host; happy to add it if you'd like it in this PR.

Risks and reviewer notes

The key behavior is deliver_device_handoff in storage/ublk-daemon/src/server.rs. It relies on Linux AF_UNIX semantics: unix_release_sock sets ECONNRESET on the peer when the closing socket still has unread data, and stream reads report that error before EOF.
A false "undelivered" would release a device a client is using, so only a write error or ECONNRESET counts. Any other read outcome (EOF, data, other errors) is treated as delivered.
Handoff connections now stay open until the client closes. All in-tree clients go through UblkDaemonClient::call_inner, which drops the stream right after reading the response. A custom client that kept the connection open would park one detached task until it closes, with no effect on other requests.
If shutdown runs while a rollback is pending, the release can report the device as not found because stop_all_devices already took it. That is logged and harmless.

Checklist

The PR contains one coherent change and no unrelated formatting or refactoring.
New behavior is covered by tests, or I explained why testing is impractical.
Logs and examples contain no credentials, tokens, or private registry information.
I did not manually edit generated code without updating its source and regenerating it.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/kvcache-ai/AgentENV/pull/431 **Author:** [@Alex-Wengg](https://github.com/Alex-Wengg) **Created:** 10/23/2026 **Status:** ✅ Merged **Merged:** 10/27/2026 **Merged by:** [@yingdi-shan](https://github.com/yingdi-shan) **Base:** `main` ← **Head:** `fix/ublk-daemon-undelivered-handoff` --- ### 📝 Commits (1) - [`48960c6`](https://github.com/kvcache-ai/AgentENV/commit/48960c67c6ebc8b8f4daa268fe426b511e73f02d) fix(ublk-daemon): roll back device handoffs the client never receives ### 📊 Changes **1 file changed** (+121 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `storage/ublk-daemon/src/server.rs` (+121 -2) </details> ### 📄 Description ## What Roll back a device the ublk daemon hands to a client when the client never receives the response. `handle_connection` now treats the three responses that hand over a device (`DeviceAcquired`, `DeviceCreated`, `OverlaybdRuntimeDeviceCreated`) as a handoff: 1. If writing the response fails, the client is gone and the handoff is undelivered. 2. If the write succeeds, the daemon waits for the client to close the connection. A client closes right after reading its one response. On Linux, closing a Unix stream socket with unread data makes the peer's next read fail with `ECONNRESET`, so a reset means the client gave up after the response was written but before reading it. An undelivered handoff is undone the way the client would have released it: pooled devices go through `handle_release_overlaybd` (one acquisition, so one shared reference), and raw devices go through `handle_delete`. ## Why When an acquire outlives the client's RPC timeout, the daemon records the acquisition, fails to write the response (`Broken pipe`), and returns. The device or shared reference stays active, and the response was the client's only record of it, so nothing ever releases it. Repeated timeouts accumulate leaked devices and references. ## Related issue Closes #392 ## Scope and non-goals - Covers both windows named in the issue: the failed send, and the response written but dropped unread. - No protocol or client change. Existing clients already close the connection after reading the response. - The runtime directory of an undelivered `OverlaybdRuntimeDeviceCreated` is left in place. It lives under the sandbox work dir, which the agent cleans up when sandbox creation fails, and the client chose that path, so removing it late from the daemon could race a retry. - Not covered: a client that reads the response and then loses it (for example, its caller's future is cancelled after `acquire_overlaybd` returns). That is the caller's ownership problem. ## Design and behavior changes - Rollback happens only on a definite signal (write error, or `ECONNRESET` on the follow-up read). Any other outcome counts as delivered, because releasing a device the client still uses is worse than leaking one. - For handoff responses, the connection task now stays parked until the client closes the socket instead of returning right after the write. Connection tasks are detached (`tokio::spawn`), and clients drop the stream as soon as `call_inner` returns, so this is normally immediate. - Concurrent valid users of a shared device keep their references: the rollback releases exactly the one reference this request added. ## Compatibility and operations - Public API or generated protocol: N/A, wire format unchanged. - Configuration or defaults: N/A. - Snapshot manifest, artifact layout, or storage format: N/A. - Upgrade and rollback: daemon-only change; works with current clients. - Host requirements, permissions, ports, or dependencies: relies on Linux `AF_UNIX` reset semantics, which the daemon (ublk) already requires. ## Validation - [x] `make fmt` - [ ] `make clippy` - [ ] `make test-unit` - [x] Relevant Rust integration tests - [ ] `make -C services test` (required when `services/` changes) - [ ] Generated clients/server regenerated with the documented `make` target - [ ] Documentation updated - [ ] Benchmarks or performance comparison completed Commands and results (Ubuntu 24.04 on WSL2, kernel 6.18.33.1, Rust 1.99.1): ```text $ make fmt (clean) $ cargo clippy -p uvm-ublk-daemon --all-targets --all-features -- -D warnings (clean) $ cargo test -p uvm-ublk-daemon --no-fail-fast src/lib.rs 53 passed; 0 failed tests/ublk_daemon_test.rs 37 passed; 0 failed (same results as main at 9f76bef, apart from the 4 new tests) $ cargo test -p uvm-ublk-daemon --lib device_handoff device_handoff_read_by_client_is_delivered ok device_handoff_to_disconnected_client_is_undelivered ok device_handoff_dropped_unread_is_undelivered ok device_handoff_dropped_mid_read_is_undelivered ok ``` The new tests run `deliver_device_handoff` over a real `UnixStream::pair()`, so they exercise the kernel's reset behavior rather than a mock. With the `ECONNRESET` check disabled, `dropped_unread` and `dropped_mid_read` fail, confirming the tests catch the written-but-unread window. Skipped checks and reasons: - `make clippy` (whole workspace) and `make test-unit`: ran the crate-scoped equivalents above; the rest of the workspace is untouched. - Rollback against real ublk devices: my WSL kernel is built without `CONFIG_BLK_DEV_UBLK`, so the `handle_release_overlaybd` / `handle_delete` rollback calls were not exercised on a live device. They are the existing release paths, called with the `dev_id` the same request just handed out. The issue's barrier-based regression test (abandon a shared acquire while another client holds the device) would need a ublk host; happy to add it if you'd like it in this PR. ## Risks and reviewer notes - The key behavior is `deliver_device_handoff` in `storage/ublk-daemon/src/server.rs`. It relies on Linux `AF_UNIX` semantics: `unix_release_sock` sets `ECONNRESET` on the peer when the closing socket still has unread data, and stream reads report that error before EOF. - A false "undelivered" would release a device a client is using, so only a write error or `ECONNRESET` counts. Any other read outcome (EOF, data, other errors) is treated as delivered. - Handoff connections now stay open until the client closes. All in-tree clients go through `UblkDaemonClient::call_inner`, which drops the stream right after reading the response. A custom client that kept the connection open would park one detached task until it closes, with no effect on other requests. - If shutdown runs while a rollback is pending, the release can report the device as not found because `stop_all_devices` already took it. That is logged and harmless. ## Checklist - [x] The PR contains one coherent change and no unrelated formatting or refactoring. - [x] New behavior is covered by tests, or I explained why testing is impractical. - [x] Logs and examples contain no credentials, tokens, or private registry information. - [x] I did not manually edit generated code without updating its source and regenerating it. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-29 14:23:22 +00:00