computerd: clean up RPC interfaces - #108
Merged
Merged
Conversation
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
aron-cf
force-pushed
the
update-computerd-endpoint
branch
from
August 17, 2026 17:19
813d07e to
d0f04be
Compare
馃 Changeset detectedLatest commit: 16eacf8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
aron-cf
force-pushed
the
update-computerd-endpoint
branch
2 times, most recently
from
August 18, 2026 10:24
f529cd3 to
89e24fe
Compare
A caret range on a 0.x version admits only patch releases, so ^0.8.0 held the workspace at 0.8.x and installing could not move it. Both declarations, in packages/rpc and packages/computer, now ask for ^0.10.0. Of the releases in between, 0.9.1 fixed nodeHttpBatchRpcResponse leaving the connection open and failing with ERR_HTTP_HEADERS_SENT on a request that is not a POST, and 0.10.0 added receiver-side limits on message size, nesting depth, and bigint length. The MessagePort wire format changed in 0.9.0; nothing here opens one. @cloudflare/sandbox pins capnweb at 0.8.0, so examples/think-compare-runtimes, which imports both it and @cloudflare/computer, resolves two copies where it previously shared one. Each drives its own session and no stub crosses between them.
Setting UPSTREAM_URL made the daemon dial the given address as a client, pull once before it started listening, and then run a sync tick every 250 milliseconds. It was the only path where the daemon drove its own sync loop. It predates POST /connect, which solved the same problem for hosts that cannot reach into the container, and no deployment sets it: the container backend passes PORT and MOUNT_POINT and nothing else. Sync is driven from the host across the capnweb session. Two rough edges go with it. An address that was wrong or not yet listening made the process exit before the HTTP server bound, so /health never answered and the operator got "WebSocket connection failed" and no further detail. And once a healthy peer dropped the socket nothing redialed, so the tick failed every 250 milliseconds while /health kept answering ok, leaving a dead sync loop behind a healthy probe and a log that grew without bound. The upstream option on createNodeVirtualFileSystem goes too, along with the polling loop and the stopSync handle it returned. Of the calls to that function, all but one passed no argument; the exception was this path. What is left has no opinion about syncing at all.
POST /connect used to accept a base URL and build two paths from it: the base plus /health for the readiness poll, and the base plus /ws for the dial. Both belong to the host, so the daemon was deciding routes it does not serve, and renaming either one meant releasing a new binary alongside the host change. The request now names all three parts as base, health, and api. All are required: defaulting health to /health and api to /ws would leave the paths compiled in after all. A base must carry an http or https scheme, and each path must be absolute, so toHttpUrl is gone and toWebSocketUrl keeps normalizing the dial to ws or wss. Both paths are rejected if they parse as an address of their own or begin with a double slash. The daemon dials whatever it is handed, and either form would resolve somewhere other than under the given base.
The daemon mounted one RPC object on two carriers: an HTTP batch handler on POST /api and a websocket upgrade on /ws. It now serves the websocket alone, on /api. The batch route goes because it could not carry the interface. Every call that reads data back returns a ReadableStream, and delivering one means the server calls write() on a stub the client exposed and waits for an acknowledgement. A batch session cannot complete a call from server to client, so fetchChanges, fetchObjects, exec, and getExec all fail once there is anything to deliver, while watermarks and hasObjects succeed. An endpoint that answers the cheap calls and fails the rest is worse than one that is absent. A request that reaches /api without an Upgrade header now gets 400 and a sentence saying a websocket is required. Handshakes that do try to upgrade are answered by the ws package, except an unsupported Sec-WebSocket-Version: it answers 400 where the specification calls for 426, so that case is handled before handing over and reports the versions the server speaks. Samplers that only want revision numbers have GET /__computerd/watermarks, which reads through the same watermarks() the wire serves so the two cannot drift.
The daemon served capnweb's HTTP batch transport through this wrapper and no longer does, leaving it without a caller. Removing it also drops the package's last use of capnweb's batch surface.
The container backend sent only the egress base and left the daemon to append /health and /ws. The daemon now expects the request to name both paths, so send them. EGRESS_HEALTH_PATH and EGRESS_API_PATH sit next to DEFAULT_EGRESS_HOST because they describe the same endpoint: the routes WorkspaceProxy answers on the far side of the egress interceptor. Keeping them beside the host name means a rename touches one place.
The daemon now serves its capnweb session on /api, and the host end of the same connection follows, so both sides of the wire use one name. WorkspaceProxy answers /api, the container backend routes /api upgrades into the in-flight connect(), and the path travels to the daemon in the /connect request rather than being assembled there. The tokenized callback under /__workspace_connect keeps its existing spelling. Nothing in this repository builds those URLs, so renaming that segment would change an inbound contract without moving anything; both forms normalize to /api before reaching the durable object. handleFetch answered 426 for a request with no Upgrade header. That is a malformed handshake, which is a 400. 426 belongs to the narrower case of an unsupported Sec-WebSocket-Version, which is what the daemon now answers for it, so both ends agree on what each status means.
The daemon and the host both serve the capnweb session on /api now, so the durable objects that route the container's outbound upgrade follow. Each example matched /ws in its fetch handler; the container example forwards every request to the backend and needed no change beyond its comments.
The daemon serves its session on /api. The FUSE flush check and the stub soak both dial it directly, so both move with it.
The soak booted two daemons and pointed UPSTREAM_URL at the first, then measured how fast it caught up. The second daemon was never the subject: what the script measured was a daemon driving its own sync loop, and driving a loop needs a peer to drive against. That mode is gone, and no deployment used it. A workspace pairs one host with one container, so the script now boots one daemon and takes the host's part. It holds the authoritative store, writes into it at the configured rate, and ticks pushOnce and pullOnce across the capnweb session. The table reports the host revision beside the daemon revision, and the gap between them is the convergence lag. Resident memory still gets sampled every row, which is the other reason this script exists. Revisions now come from GET /__computerd/watermarks rather than an RPC session per sample, and SOAK_TICK_MS sets how often the tick runs.
Bring the written surface in line with the daemon. It serves one endpoint, /api, carrying one transport, and the HTTP batch alternative is gone: every call that returns data returns a ReadableStream, and delivering one needs a call from server to client that a batch session cannot complete. UPSTREAM_URL is removed from the environment tables and from the prose that described a daemon-driven sync loop. The lifecycle document claimed the Cloudflare backend wired that variable automatically, which was never true; the carrier is POST /connect and the durable object drives sync across it. That sentence is the one a reader lands on when working out how a container recovers after a restart, so it pointed at the wrong mechanism. The /connect description now shows the request naming a base and both paths, records the readiness probe reading base plus health, and covers the new watermarks endpoint and the handshake statuses.
The tokenized connect callback matched /__workspace_connect/<id>/ws while everything else moved to /api, leaving the proxy answering two spellings for one endpoint. It now matches /api, so both the plain and tokenized forms read the same and normalize to the same path before reaching the durable object. Nothing in this repository builds those URLs, so this changes an inbound shape that only external callers can be using. A caller still requesting the /ws form gets a 404 and needs the new segment.
The wire moved to /api. Two comments still named /ws as the path the composite server is served on and the URL a client dials.
Two places still introduced the wire as a WebSocket "with an HTTP batch alternative" while the paragraphs below them explained why there is no such alternative. Say it once, in the opening, and give the reason: capnweb's batch transport cannot deliver a stream returned from a call, and every read on this interface returns one.
The endpoint moved to /api. This script embeds its client in a heredoc rather than a .mjs file, so it was missed when the other harnesses were updated.
The check wrote through FUSE to /workspace/x.txt and then read /x.txt back from the receiver store, which cannot resolve: the virtual filesystem keys entries by their absolute in-container path, so the mount point is part of the key. Both ends now use one constant. It also logged pullOnce's return as if it were a count. pullOnce resolves to an ApplyResult, so the line printed "[object Object] entries" and the guard comparing it against 0 could never fire. Destructure applied.
The last section of exec-tests exercises Workspace.runtime through the high-level facade, and it had never run. It imported TestBackend and Workspace from per-file dist paths, which the bundler does not emit and never has, so the script died on a module resolution error after the seven wire-level checks passed. Those symbols live on the package's main entry, which cannot be imported under plain node: it re-exports WorkspaceProxy, extending WorkerEntrypoint from cloudflare:workers, and that specifier only resolves inside workerd. A node --import hook now registers a resolve hook for the specifier and answers with stub classes, which is enough to satisfy `class X extends Y` at module evaluation time. Nothing in the stub is ever constructed. The alternative was adding build entries for each file the script wants, which would duplicate bundled code in a published package to suit a harness. Also correct the soak entry in the script list, which still described the two-container topology.
The carrier entry stated that no HTTP alternative exists and then explained why. A contract document is better describing what the wire is than cataloguing what it is not, and the claim reads as though it were about capnweb, which does ship an HTTP batch transport.
The write loop passed writeSeq++ for the file index and writeSeq for the payload seed. Arguments evaluate left to right, so the increment landed before the seed was read: every file carried the bytes generated for the next index, and seed zero was never used. Nothing in the script asserts on content, so the soak still measured what it was built to measure. The pairing is wrong for anyone who later checks the bytes against the name.
The endpoint rename, the /connect payload change, and the removal of UPSTREAM_URL landed as a single entry. They are three separate things a consumer may care about, and a reader scanning the changelog for the one that affects them should not have to unpick a paragraph covering all three.
The route read sync revisions through rpc.sync.watermarks() but sat at /__computerd/watermarks, alongside runtime info and process memory. Those report on the daemon; this reports the workspace, and it returns exactly what a session would. It is now /api/watermarks, which draws the line where it belongs: /api is the workspace surface, including anything that reads through it, and /__computerd is daemon introspection. Only the exact /api path upgrades, so a handshake aimed at the subpath still answers 404, and the shared secret covers the new path the same way it covered the old one.
aron-cf
force-pushed
the
update-computerd-endpoint
branch
from
August 18, 2026 13:39
be297d4 to
16eacf8
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.
The daemon in the container served its RPC on two paths:
/wsfor a websocket, and/apifor a transport that sends one HTTP request per call.This PR makes the following changes:
/api, carrying a WebSocket, at both ends of the connection.UPSTREAM_URLand the sync loop have been removed to simplify computerd.GET /api/watermarksreports sync revisions over plain HTTP for use in integration tests./apiwithout an upgrade header gets a400. One asking for a websocket version the server does not speak gets a426and the versions it does.capnweblibrary is bumped from 0.8.0 to 0.10.0.Example /connect request:
sequenceDiagram participant Host as Durable object participant Daemon as Daemon in container Host->>Daemon: POST /connect with base and both paths Daemon->>Host: poll base + health until it answers Daemon->>Host: open websocket at base + api Host->>Daemon: drive syncing over that connectionImportant
This breaks hosts that route the upgrade themselves. A durable object matching
/wsin its ownfetch()handler has to match/api. I don't expect there to be any of these.New tests cover the connect request refusing an incomplete body, the handshake statuses, and the revisions endpoint, plus one that points the daemon at deliberately unusual paths to prove it dials what it is told. Running the harnesses surfaced three faults in them, unrelated to this work and fixed alongside it. The daemon reference, the lifecycle notes, both readme files and the debugging notes now describe a single endpoint, and a claim that the container backend set
UPSTREAM_URLautomatically is corrected, since it never did.