Repository navigation
refactor(api)!: retire tonic, serve the whole daemon API over Connect (CORE-68) - #523
Merged
Merged
Conversation
Greptile SummaryThe PR retires tonic from the daemon and CLI API paths in favor of a unified Connect implementation.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| app/arcbox-api/src/connect/machine.rs | Migrates machine unary and streaming handlers to Connect, including bidirectional interactive execution; the previously reported stream-error concern is resolved. |
| app/arcbox-api/src/connect/mod.rs | Defines the unified Connect service composition and shared runtime support for migrated daemon APIs. |
| app/arcbox-cli/src/connect.rs | Provides the CLI’s shared Connect transport and replaces the removed tonic-specific client plumbing. |
| app/arcbox-cli/src/commands/machine.rs | Moves machine commands, including split bidirectional interactive execution, onto generated Connect clients. |
| app/arcbox-daemon/src/control_plane.rs | Serves the unified Connect router and updates protocol coverage for the consolidated daemon API. |
| rpc/arcbox-connect/build.rs | Consolidates package-level message and service generation with the required proto layout configuration. |
| Cargo.lock | Updates Connect and buffa dependencies and removes obsolete tonic-related CLI dependencies. |
Sequence Diagram
sequenceDiagram
participant CLI as abctl
participant Client as Connect client
participant Daemon as Unified Connect router
participant Handler as API handler
participant Runtime as ArcBox runtime
CLI->>Client: Invoke daemon command
Client->>Daemon: Connect, gRPC, or gRPC-Web request
Daemon->>Handler: Dispatch generated service method
Handler->>Runtime: Delegate using prost types
Runtime-->>Handler: Result or stream
Handler-->>Daemon: Encode wire response
Daemon-->>Client: Protocol response
Client-->>CLI: Command result
Reviews (9): Last reviewed commit: "fix(api,daemon): fail the exec pump towa..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e59227151
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
The new registration test asserts a macOS-only service unconditionally, so it fails on Linux. Because CI does not run on this stacked PR, nothing catches it until the bases land and this retargets to master — which is exactly when the ubuntu-latest job runs the workspace tests for the first time.
Reviewed changes
The daemon's last seven services move off tonic onto the existing connectrpc router, so one handler set now answers Connect, gRPC, and gRPC-Web.
app/arcbox-api/src/connect/gainsicon,kubernetes,machine,macos,migration,stats,system;Status::*becomesConnectError::*throughout andSharedRuntime/run_macos_blockingmove here.app/arcbox-api/src/grpc/andsrc/migration.rsare deleted along with theStatus-flavoured readiness trait and Migration's duplicated copy of it.app/arcbox-daemon: the two-stackcompose()is gone,into_app()serves the Connect router directly,tonicdrops to a dev dependency, and a new test asserts every migrated path is actually routed.rpc/arcbox-connect/build.rscompiles the ninearcbox.v1protos with.file_per_package(true);rpc/arcbox-connect/src/lib.rsre-exports them asv1.app/arcbox-api/tests/icon_test.rsis rewritten against a new inherentIconServiceImpl::resolve().
⚠️ Nothing in the tree can catch a bidirectional ExecSession regression
The PR description names ExecSession as the one RPC exercising every stream shape, and it is the only migrated method where the framework has to do something genuinely harder than before: deliver inbound and outbound frames concurrently.
The handler is shaped correctly for that — it spawns a task draining InboundStream and returns the output stream immediately, mirroring the old tonic body. What I could not confirm is that connectrpc 0.8.1 actually plumbs both directions concurrently; the crate is not in this environment's registry cache, so this is unverified rather than suspected-broken.
The reason it matters here is that the existing coverage cannot falsify it. tests/e2e/tests/machine.rs:306 calls drop(tx) before issuing the RPC, so the entire input stream is buffered and closed before the response stream is opened — that exercises client-streaming-then-server-streaming, not interleaving. A handler that only produced output after its inbound stream completed would still pass.
The path that does interleave is app/arcbox-cli/src/commands/machine.rs:568-642 — abctl machine ssh <name> with no command holds msg_tx alive in a stdin/SIGWINCH pump while polling the response stream. If concurrency is broken there, it hangs at runtime with nothing failing at compile time or in the test suite.
Has an interactive session been driven end-to-end against the new stack? If yes, saying so in the PR description is enough and this is closed. If not, it is worth ten minutes before merge given the rest of the change is mechanical.
For contrast, the server-streaming and error-code halves are genuinely covered: tests/e2e/tests/stats_watch.rs drives StatsService/Watch with a real tonic client, and control_plane.rs:269-273 asserts an UNAVAILABLE ConnectError reaches a tonic client as Code::Unavailable. Those I have no concerns about.
ℹ️ Nitpicks
app/arcbox-api/src/lib.rs:12-18still re-exportskubernetes_service_server,machine_service_server,migration_service_server,stats_service_server(andmacos_service_server). Nothing implements those traits any more. The_clienthalves are still used by the CLI and e2e tests, so only the_serverones are dead. The crate doc atlib.rs:1-6also still describes the crate as "gRPC service implementations".app/arcbox-api/tests/icon_test.rsnow calls the inherentresolve()rather than going throughget_image_icon, which leaves the Connect method itself untested. Given these tests already hit live registries the tradeoff may well be deliberate — worth a line in the test module saying so, otherwise the next reader assumes coverage that isn't there.- The PR description says "
tonicdrops to a dev dependency". That holds forarcbox-daemon, butarcbox-apikeeps it as a regular dependency (Cargo.toml:22) becauseerror.rs:77-104deliberately routesApiError → ConnectErrorthroughtonic::Statusto keep one mapping table. That is a reasonable call and pre-dates this PR; flagging only because the description reads as though the crate is clean. - On "the JSON contract is unchanged": that is right for the CLI's
--jsonoutput, which serialises prost types locally with serde and never touches buffa. It is worth being precise that it does not extend to the new Connect JSON wire surface forarcbox.v1— buffa's canonical proto3 JSON encodesint64/uint64as quoted strings, where the serde rendering of the same messages emits bare numbers (VirtioDebugInfocounters,MachineStats.uptime,SystemStats.mem_total, and similar). No consumer exists today, so nothing breaks; it only matters if someone later writes a Connect JSON client usingabctl --jsonoutput as the schema. I could not verify buffa's behaviour directly here, so treat this as a question rather than a finding.
Claude Opus ｜ 𝕏
Contributor
There was a problem hiding this comment.
ℹ️ One minor suggestion inline. Nothing blocking in this delta.
Reviewed changes — 8e59227..b24b68b is one commit, and it is CLI-side only: abctl k8s moves off the tonic KubernetesServiceClient onto the generated Connect client, with a new shared app/arcbox-cli/src/connect.rs holding the transport constructor and a response→prost adapter. kubernetes_client() goes from async fn -> Result<..> to a sync infallible fn.
No daemon or arcbox-api file changed, so my four open threads still stand exactly as written — I checked them against the working tree rather than the diff and left them open instead of repeating them here.
Verified compatible — checked, no concern
UnaryExt::prostis exact, not approximate. The daemon puts the prost encoding on the wire —connect/bridge.rs::wire_responseisPreEncoded::from_bytes_unchecked(msg.encode_to_vec())— andClientConfig::new(uri)defaults to the protobuf codec (proto()/json()are explicit overrides). Soprost()re-decodes the identical bytes the server encoded. That is what makes "the JSON contract is unchanged" true for these six commands, and it is a stronger guarantee than a field-by-field bridge would give.- Neither
.expect()inconnect.rscan fire. Worth stating because"http://localhost"would panic if either call site took anhttp::uri::Authority— bothHttp2Connection::lazy_unix's second parameter andClientConfig::newtake a fullUri, which parses fine. The local variable being namedauthorityis what makes this read alarming. - The generated client's shape matches the call sites. Constructor is
new(transport, config)in that order; methods take&self, which is whylet client = kubernetes_client();needs nomut; unary methods returnResult<UnaryResponse<OwnedView<V>>, _>, which is what theUnaryExtimpl is written against. mod connect;inmain.rsalone is correct.arcbox-cli/src/lib.rsdeclares onlyrootfs_builder/templates/terminal, socommandsandconnectare both bin-only modules ofabctlandcrate::connectresolves; the second bin (arcbox) is a separate placeholder file and is unaffected.- Error propagation through
execute_*/refresh_if_enabled/execute_enableis unchanged — only the message text differs (see inline).
ℹ️ Notes
- The prost→buffa request direction is still entirely unexercised. All five Kubernetes requests are empty messages (
agent.proto:340,355,366,375,392), so every call site here ispb::XRequest::default()andconnect.rsonly ever needed a response-side helper. The nine modules that follow this template all send populated requests —machine.rsalone carries names, specs, and exec input — and there is no counterpart toUnaryExt::prostfor building them. Since this commit establishes the pattern, it is worth deciding now whether that belongs inconnect.rstoo, rather than discovering it nine times. - Nothing in the test tree constructs a Connect client.
connect::daemonandUnaryExt::prosthave no coverage, andabctl k8sis absent from the e2e scenarios, so the first end-to-end Connect client call is a manual step. I am not asking for a test ofdaemon()itself — it is three lines of construction — but note that the registration test added on the daemon side covers routing and does not exercise the client at all, so the two halves of this migration have no meeting point in CI.
Claude Opus ｜ 𝕏
Contributor
|
Run failed. View the logs →
|
Contributor
|
Run failed. View the logs →
|
Contributor
|
Run failed. View the logs →
|
Contributor
|
Run failed. View the logs →
|
AprilNEA
added a commit
that referenced
this pull request
Aug 1, 2026
…ertion Review findings on #523: a post-Init frame that fails decoding now ends the input stream (EOF sentinel, clean session end) instead of silently dropping a frame out of an interactive session — matching the posture the filesystem upload already takes. The registration test's MacosService entry gets the same cfg gate as the registration it asserts, so the suite passes on Linux. Three comments that still described the tonic composition catch up, and build.rs's expect message stops naming only the sandbox protos.
Contributor
|
Run failed. View the logs →
|
AprilNEA
force-pushed
the
feat/sandbox-connect-rpc
branch
from
August 1, 2026 12:49
c33c17e to
ab905a0
Compare
…CORE-68) First slice of retiring tonic. Generates buffa types and connectrpc service traits for the whole `arcbox.v1` package, then moves one service — Icon, which reaches nothing but dimicon — off tonic to prove the pattern end to end. Its handler body is unchanged; only the trait it implements and the router it registers on differ. Message generation is all-or-nothing here because the nine remaining protos share one `arcbox.v1` package, so this commit necessarily brings up the types for every service. The services themselves still move one at a time, which is what the dual-stack composition is for. Two prerequisites the codegen needs, both recorded at the call site: - `file_per_package` — those protos declare `arcbox.v1` while sitting at the proto root rather than in `arcbox/v1/` (the PACKAGE_DIRECTORY_MATCH violation `buf lint` already reports). Without it the generator emits a module per file and intra-package references do not resolve. - `common.proto` must be listed explicitly: it defines this package's own hand-rolled `Timestamp` and `Mount`, which are not the well-known types. Registration is what decides which stack serves a path — tonic matches first, so a service left on both would silently keep answering over tonic alone. A test asserts the route directly rather than driving Icon, which would reach the network.
Both are removed from the tonic `Routes` in the same change that adds them to the Connect router — registration is what decides which stack serves a path, and tonic matches first, so a service left on both would keep answering over tonic alone. Stats keeps its broadcast-to-stream bridge unchanged; System keeps serving `GetVirtioDebug` from `early_runtime`, which is the whole point of a diagnostics RPC that has to answer while the runtime is still empty. System needs state the plain router helper cannot reach, so it is passed in explicitly via `router_with_system`. `arcbox.v1` has its own `Empty` message rather than the well-known type, so those handlers take `pb::Empty`. Icon's lookup moves to an inherent `resolve`, with the RPC method a thin wrapper. Its tests exercise that instead of the method: what they check is registry resolution, and going through the trait would only add a request envelope to construct. The registration test now covers all three migrated services.
Both delegate straight to the runtime, so the handlers only change how the response is wrapped for the wire. Migration also loses a duplicated readiness helper: it carried its own copy of the `ready()` extension because the gRPC one is `pub(super)`. It now shares the Connect module's, so there is one definition of what "runtime not ready" means across the surface. That leaves Machine and Macos on tonic.
The largest of the daemon services, and the one that exercises every stream shape: server-streaming Exec and Events, and bidirectional ExecSession. The bidi handler takes an InboundStream exactly as the sandbox WriteFile handler does, so the PTY bridge to the agent is unchanged — only how the first Init frame and each stdin frame are decoded. Handler bodies keep working in prost types throughout; the crossing stays in `bridge`. Macos is now the only service left on tonic, which is why the tonic `Routes` can be empty on non-macOS hosts.
Moves the last service, Macos, onto Connect and removes what that made dead. `run_macos_blocking` comes along: Virtualization.framework futures are `!Send` and handlers must be `Send` — that was true under tonic and is equally true under connectrpc — so the helper survives, answering `ConnectError` instead of `Status`. With no tonic services left, the empty `Routes` and the two-stack `compose` go too; the daemon serves the Connect router directly. tonic drops to a dev dependency, where the generated client is now purely an independent implementation keeping the gRPC claim honest. `app/arcbox-api/src/grpc/` is deleted. Its only remaining contents were the `SharedRuntime` alias, which moves next to the services that use it, and a `Status`-flavoured readiness trait with no callers. Router construction and the conversion to an HTTP service are split so the registration test can still read the routing table — an `axum::Router` no longer exposes its method paths, and a service silently dropped during this migration would fail nowhere else.
Adds the CLI's transport helper and migrates the first module, Kubernetes, as the pattern for the rest. Connections are lazy: the socket is dialled on the first call rather than when the client is built, so subcommands that never reach the daemon pay nothing and a connection failure surfaces at the call that needed it, with that call's context. Responses cross back to prost through `UnaryExt::prost`, decoding the same wire bytes the zero-copy view already holds. That is what keeps every command's formatting and `--json` output untouched — the migration is confined to client construction and the call expression, not to how any result is rendered. A half-migrated CLI is a working state, not a broken one: the daemon serves both formats at one endpoint, so a module still on a tonic client talks to exactly the same handlers.
…CORE-68) Adds the request-side crossing to match the response one, so each command keeps building requests from the prost types its argument parsing already produces — enum discriminants included — and keeps rendering results from the prost types it already formats. The migration stays confined to client construction and the call expression. The daemon-liveness probe connects eagerly rather than lazily: it asks whether a daemon is actually there, so the h2c handshake has to happen at that moment rather than on a later call. One call stays on tonic, with a client kept solely for it: the interactive `machine exec -it` session. `connectrpc::client::BidiStream` takes `&mut self` for both `send` and `message` and exposes no split, so a bidi stream cannot be driven from both directions at once — which is exactly what a PTY needs. This is an upstream gap, not a shape that can be worked around here; recorded at the call site. Top, macos, agent, and sandbox are untouched and still on tonic clients. That is a working state, not a broken one — the daemon serves both formats at one endpoint, so those modules reach identical handlers.
Ports top, macos, agent, and sandbox. Every CLI call now speaks Connect except one, and `x-machine` moves from a per-request wrapper to a default header on the sandbox client config — a call that forgot the wrapper used to route to the default silently rather than fail. The sandbox file upload drops its channel and spawned sender: Connect's client-streaming call pulls from an iterator with backpressure, so the chunks are produced lazily at the call instead. Memory is unchanged, since the file was already read whole before chunking. The best-effort resize and stdin pumps run inside spawned tasks that return `()`, so they cannot use `?`. A conversion failure there can only mean the two generated representations disagree — a build fault — so they end the pump rather than pretend to continue. `machine exec -it` remains the sole tonic caller, blocked on `BidiStream` exposing no split. That is what keeps `tonic` in the CLI's dependency tree; nothing else does.
Pin the three connectrpc crates to upstream rev c5c1a6fa, ahead of the 0.9.0 release. Two client-side changes on main unblock the rest of the CLI migration: - BidiStream::into_split (#228) — independently owned send/recv halves, the missing piece that kept interactive exec on a tonic client. - Client-streaming calls take an async Stream (#227); the sandbox cp upload wraps its ready chunks in stream_iter, the documented one-line migration. buffa moves to 0.9.1 with it: main's codegen emits 0.9 size arithmetic, so the two must move together. Swap back to registry versions when connectrpc 0.9.0 ships.
…ream (CORE-68) The last tonic call in the CLI moves to Connect. exec -it splits the bidi stream into owned halves (BidiStream::into_split, upstream #228): the pumps still feed one mpsc channel, a forwarder task owns the send half, the receive half stays in the main task. That retires UnixConnector and the legacy tonic client, so tonic, arcbox-grpc, hyper-util, and tokio-stream leave the CLI's manifest. arcbox-protocol's tonic dependency had zero uses (every match was the substring in "monotonic") and goes too — abctl's dependency tree is now tonic-free, direct and transitive. A new control-plane test drives the exact wire shape: a split client opens ExecSession over the real socket, the sender task writes Init while the receiver awaits, and the handler's unavailable verdict comes back through the receive half. rpc/AGENTS.md catches up with the post-tonic serving path.
…ertion Review findings on #523: a post-Init frame that fails decoding now ends the input stream (EOF sentinel, clean session end) instead of silently dropping a frame out of an interactive session — matching the posture the filesystem upload already takes. The registration test's MacosService entry gets the same cfg gate as the registration it asserts, so the suite passes on Linux. Three comments that still described the tonic composition catch up, and build.rs's expect message stops naming only the sandbox protos.
AprilNEA
force-pushed
the
refactor/connect-control-plane
branch
from
August 1, 2026 12:53
14c8760 to
725189b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #521 (CORE-53), which is stacked on #519. Merge in that order.
The daemon no longer serves anything over tonic, and the CLI no longer speaks anything over tonic:
tonicis absent fromabctl's dependency tree entirely, direct and transitive. All daemon services —machine,stats,system,kubernetes,migration,icon,macos, and the sandbox four — answer Connect, gRPC, and gRPC-Web from one set of handlers, and every CLI command reaches them through Connect clients.Why this was cheap
CORE-53 built the composition that made it incremental: services could move one at a time with no big-bang switch. Two things that would have made it expensive turned out not to apply:
tonic-health, no request-metadata manipulation. The impls are thin runtime delegation.--jsonoutput. pbjson emits canonical camelCase and so does buffa (rename = "ipAddress"), so those consumers see the same bytes.What moved, and what it cost
Message generation for
arcbox.v1is one step, not seven: those nine protos share a package and codegen emits one module per package. Only the service impls migrate individually.Two prerequisites the generator needs, both recorded at the call site:
file_per_package— those protos declarearcbox.v1while sitting at the proto root rather than inarcbox/v1/(thePACKAGE_DIRECTORY_MATCHviolationbuf lintalready reports). Without it the generator emits a module per file and intra-package references fail to resolve — 209 errors.common.protomust be compiled explicitly: it defines this package's own hand-rolledTimestampandMount, which are not the well-known types.arcbox.v1likewise has its ownEmpty.Handler bodies are unchanged throughout — they keep working in prost types, and
bridgecrosses at the boundary.Machineexercises every stream shape including bidirectionalExecSession, whose PTY bridge to the agent is untouched; only how the firstInitframe and each stdin frame are decoded differs.run_macos_blockingsurvives the move. Virtualization.framework futures are!Sendand handlers must beSend— true under tonic, equally true under connectrpc — so it just answersConnectErrornow.The interactive exec session (the last tonic call)
abctl machine exec -itneeds a bidi stream driven from both directions at once. connectrpc 0.8.1'sBidiStreamtakes&mut selffor bothsendandmessagewith no split, which kept that one call on a private tonic client. Upstream has since landed exactly the missing piece —BidiStream::into_split, independently owned send/recv halves, merged 2026-07-20 but not yet released.So this PR pins the three connectrpc crates to upstream rev
c5c1a6fa(ahead of the 0.9.0 release; swap back to registry versions when it ships). buffa moves to 0.9.1 with it — main's codegen emits 0.9'su64size arithmetic, so the two must move together. The rev also carries #227 (client-streaming calls take an asyncStream; the sandboxcpupload wraps its ready chunks instream_iter, the documented one-line migration) and buffa 0.9's element-memory decode budget, an amplification defence that now applies to received messages.The session keeps its shape: the init/resize/stdin pumps still feed one mpsc channel; a forwarder task owns the send half and drains that channel onto the wire; the receive half stays in the main task. Dropping the receive half cancels the RPC exactly as dropping the whole stream did.
What that made dead
app/arcbox-api/src/grpc/is deleted (see earlier commits).Routesand the two-stackcomposeare gone; the daemon serves the Connect router directly.UnixConnector,legacy_machine_client, and itstonic,arcbox-grpc,hyper-util,tokio-streamdependencies are gone.arcbox-protocolcarried atonicworkspace dependency with zero uses (everyrghit was the substring in "monotonic"); removed. That is what empties the CLI's transitive tree.tonic/tonic-reflectionremain as dev-dependencies only: its tests (and e2e) drive the gRPC format through real tonic clients as the standing proof that the format still answers.Still open
bridge.rsstays. CORE-68 listed its deletion as acceptance, which was wrong: handlers work in prost internally and cross only at the boundary, so removing the bridge means pushing buffa down intoarcbox-core. That is a separate decision about the internal representation, not a loose end of this change. The issue has been corrected.Validated
cargo clippy --workspace --exclude arcbox-agent -- -D warnings— clean (the CI-exact invocation).cargo fmt --all --check— clean.cargo test -p arcbox-daemon control_plane— the three-format test, reflection, registration, and a new split-bidi test: a Connect client opensExecSessionover the real Unix socket,into_splits it, a sender task writes the Init frame while the receiver awaits — the handler consumes Init and answersunavailablethrough the receive half. That is the wire shape behindabctl machine exec -it, proven against the daemon's real router.cargo test --workspace(minusarcbox-agentas CI does,arcbox-e2ewhich needs hardware, andarcbox-fleet-agentwhose one environment-dependent failure predates this branch) — all green.cargo tree -p arcbox-cli -i tonic→ "did not match any packages".