Fix small bugs with exec - #25
Open
aron-cf wants to merge 6 commits into
Open
Conversation
commit: |
The Worker-facing client accepts a stable id and a per-call timeout, but the shell stub declared neither on its wire options and passed only cwd, encoding, and backend to the host shell. A Worker calling exec with an id or a timeout therefore ran without them: the runner minted its own id, and the command was bounded by the backend default instead of the requested value. Declare both fields on WorkspaceExecOptions and forward them in both encoding branches, so the remote surface behaves the same as the in-process one.
The exec span on the durable-object side stays open until the handle stub reports how it was consumed, and only result(), stream(), and disposal reported. A Worker that signalled a command and walked away left the span open and the child's bracket unresolved for the life of the session. kill() now settles the span too. Settling is separate from claiming the handle: the signal doesn't drain anything, so a caller can still collect the output of the killed run through result(). The first terminal path to arrive describes the span's outcome, which for a bare kill means no exit code and no sync counts.
A command outlives the request that started it, but only a caller inside the durable object could say so. WorkspaceShell.get reattaches by exec id; the shell stub exposed no such method, so a Worker that lost its handle stub — a new request, a dropped connection — lost the run with it. The rebuilt handle didn't even carry an id to reattach with. The stub gains get(id, options), forwarding encoding, resume, and the backend selector to the host shell, and the client gains the matching overloads. Reattach runs no push/pull bracket and so opens no span, so the handle stub it returns is built with a consumer nobody reads and a span that has already closed. The client now addresses every run by an id it knows: the caller's own, or a minted UUID when the caller doesn't care. That id lands on the rebuilt handle as `id`, matching the host ExecHandle, so a Worker can hold onto it and reattach later.
A run in the container streams to one subscriber at a time, and the slot only frees when that subscriber cancels. The handle keeps two readers on the event stream: the caller's, and a second one watching for the exit event so kill() can promise the child has gone. They come from a tee, and a tee holds its source until both branches cancel — so dropping a handle left the container still streaming to a reader nobody would read, and a later get() on the same run was refused for the rest of the run. Awaiting the caller's own cancel() never returned for the same reason. The caller's branch now takes the watcher with it: cancelling the handle abandons the watch, both branches go, and the wire stream cancels through to the container. The documented flow — start a long command in one request, reattach in a later one — works as long as the earlier handle was dropped, which the stub's disposal does. Both tests stand in for the container's subscriber bookkeeping, which the existing fakes don't model: one exec-then-reattach at the shell level, one across the stub to cover disposal.
Killing a command mid-stream reported the wrong outcome. The exec span closes on the first terminal path, and kill() counted itself as one even when a result() or stream() was already reading the handle: the span recorded exit code -1 and zero sync counts while the reader went on to observe the real exit code off the wire. A reader in flight now owns the outcome. kill() only closes the span when nothing claimed the handle, and it no longer waits on a span that closes whenever the caller finishes reading. Two smaller things the same review turned up. The shell stub now rejects a spawn that came back under an id other than the one asked for: the client addresses runs by an id it mints, so a backend substituting its own would hand callers an id that get() can't reattach with. And the reattach path's disposal gives the subscription back rather than stopping the command — it didn't start it — which the comment now says.
Workspace clients returned by getWorkspace() omit the Think compatibility surface even when the owner enables useThink. Remote Think consumers must otherwise duplicate Computer's filesystem translations or avoid the common client API. Preserve the owner option on Workspace and its remote stub, then install the shared compatibility methods while constructing local and remote clients. Type the common filesystem surface so consumers can use it without an unsafe filesystem cast. Clients whose owner leaves useThink disabled remain unchanged. Remote stubs are also disposed if client initialization fails.
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
2 participants
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.
A Worker talking to a workspace through RPC had a thinner shell than a caller inside the durable object, in three ways.
The client advertised
idandtimeoutMsonshell.exec, but the stub declared neither and forwarded onlycwd,encoding, and the backend selector. A Worker asking for a stable id or a per-command timeout got neither: the runner minted its own id and the command ran under the backend default.kill()signalled the child and stopped there. The exec span on the durable-object side stays open until the handle reports how it was consumed, and onlyresult(),stream(), and disposal reported. A Worker that killed a command and walked away left the span open for the life of the session.Nothing on the remote side could reattach to a run.
WorkspaceShell.getalready does this in-process, but the stub exposed noget, and the handle the client rebuilt carried no id to reattach with. A Worker that lost its handle — a new request, a dropped connection — lost the run.The first two are small: the options travel, and
kill()settles the span. Settling is now separate from claiming the handle, so a caller can still collect the output of a run it killed:Reattach is the larger piece. The stub gains
get(id, options)passingencoding,resume, and the backend selector through to the host shell, and the client gains the matching overloads. Reattach joins a run already in flight, so it runs no push/pull bracket and opens no span of its own.That leaves the id. Rather than an extra round trip to ask the runner what it minted, the client now addresses every run by an id it already knows — the caller's own, or a fresh identifier when the caller doesn't care — and puts it on the handle, matching the in-process
ExecHandle:Each fix has a regression test written against it first: the options arriving at the runner and the replay cursor in
stub.test.ts, the closed span and the still-usableresult()inobserve-integration.test.ts, and the reattach round trip and handle id inclient.test.ts. The package suite and the workspace typecheck both pass. The Worker-side section of the package README picks up the reattach flow.