Skip to content

Decide what createSession does with a sandbox id that is already live #26

Description

@amondnet

createSession and resumeSession both route to session(id), and every backend's acquisition adopts a sandbox that already carries the name. So two createSession calls on one id that both succeed end up as two sessions inside one sandbox, sharing a filesystem, neither aware of the other.

Split out of #16, the way #18 was: it is a concrete defect, it does not depend on the resume decision, and #16 stays a design question.

What happens

  1. Process A holds a live session on sessionId = "x". Its container is running.
  2. Process B calls createSession({ sessionId: "x" }) — the fresh path, with no resume involved.
  3. docker/container.ts acquire() finds a container with that name and adopts it; adopt() returns immediately for status === 'running'.
  4. Both sessions run. A's edits and B's edits land in the same working tree. onSession re-seeds the workspace over A's live work.

defineAgent has no resuming call shape at all, so every session it makes takes this path.

Why #25 did not fix this

#25 (cdd91d0) fixed the destructive half of #18remove() now removes only a container the handle created — and deliberately left acquire() adopting exactly as before. That was right: removal is an ownership question answerable inside container.ts, and this one is not.

Where the fix has to go, and the gap in the way

HarnessV1SandboxProvider already separates the operations — createSession is the fresh path, resumeSession "reattach[es] to an existing sandbox previously created with the same sessionId" — and packages/core/src/sandbox/harness/provider.ts:90 collapses them onto the same session(id), with a comment saying why:

The contract cannot report whether session(id) found a sandbox or minted one, so createSession is taken as the fresh path and resumeSession as the returning one.

Correcting something I wrote on #16: I said the primitive was already there and that splitting the two paths adds no API, pointing at ContainerHandle.peek(). Half right. peek() exists — and not only on Docker: sandbox/local/root.ts:104 and sandbox/just-bash/sandbox.ts:46 each have one with the same meaning, "the thing if it is already live, without creating or starting anything". But all three are backend internals. The vendor-neutral contract exposes only:

interface SandboxProvider {
  readonly backend: string
  session: (sandboxId: string) => SandboxSession
  portEndpoint: (sandboxId: string, port: number, options?: SandboxPortEndpointOptions) => Promise<SandboxPortEndpoint>
}

SandboxSession carries exec, getProcess, listProcesses, exists(path), destroy — nothing that reports whether the sandbox is already live. harness/provider.ts holds a SandboxProvider and nothing more, so it cannot reach any backend's peek.

So this needs a contract decision after all. The mitigating fact is that all three backends already implement the same shape independently, which makes lifting it discovery rather than invention.

Options

A. createSession refuses a name that is already live. Lift peek to the contract (or add a session(id, { adopt: false }) shape), have the provider's createSession check it and throw; resumeSession becomes the only adopting path.
For: a collision is either a bug in the caller's id generation or a genuine reopen, and both are better as an error than as a silent share. Against: removes behaviour harness/provider.ts documents as a feature — "adoption is what makes a sandbox id resumable across processes" — from the path that is currently the only one anybody calls. A crash-and-restart against a container that outlived the process now fails instead of recovering.

B. createSession creates under a different name. Keep the id as the caller's label, mint a distinct sandbox name under it.
For: no call ever fails on a collision. Against: the id stops identifying a sandbox, which is what portEndpoint(sandboxId, …) and every session(id) lookup rely on. Effectively a rename of the whole addressing scheme.

C. Leave it. Document that a caller-supplied id must be unique and that reusing a live one shares a sandbox.
For: no contract change. Against: the failure is silent, and the only way to hold the rule is to know it.

My reading is A, with the recovery case moved to resumeSession where it belongs rather than deleted.

Sub-case to settle with it

A name that exists but is stopped: adopt() restarts it today, so a "fresh" session inherits the previous one's filesystem — a quieter version of the same problem. Whatever A/B/C decides should cover it; I would treat it identically.

Not blocked by #16

Whether defineAgent ever exposes resumeFrom (#16, answered "not yet") does not change any of this. createSession adopting a live sandbox is wrong with or without resume support, and implementing the provider-level split leaves resumeSession correct-but-uncalled until #16 is revisited.

References

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions