Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 50 additions & 16 deletions packages/acp/src/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import {
createChannel,
createScope,
ensure,
Err,
Ok,
Expand Down Expand Up @@ -2756,24 +2757,57 @@ function* useAcpxProviderState(
// the reader's terminal while offering no way to reach the owner it
// was waiting for. It refuses instead, and the coordinator is what
// refuses it.
yield* authority.perform(request, {
prepare: () =>
withSessionRoute(context, () =>
prepareLaunch(invocation, agentName, callerCwd, request.instructions, placement),
),
detach: (prepared) => detachSession(invocation, prepared, agentCommandOf(placement)),
exit: (prepared) => runNativeUi(invocation, prepared, agentCommandOf(placement)),
//
// The launch runs in a scope of its own so that this owner can bring
// it down deliberately and watch how that goes. A cancelled launch —
// the reader closing a terminal grid is one — unwinds past every
// statement after it, so a decision written down here would never be
// reached; written as this scope's cleanup, it is reached on every
// path there is.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// path there is.

const [running, stop] = createScope(yield* useScope());
let stopped = false;

yield* ensure(function* () {
// Registered after the scope exists, so it runs before the scope
// is destroyed on its own: the launch comes down here, and
// `destroy()` carries the outcome of its teardown. A child that
// could not be proven stopped, or a cleanup that failed, throws
// out of it — and is not quiescence, and is still a failure.
try {
yield* until(stop());
stopped = true;
} finally {
// Everything this owner started has to be finished with the
// session, and that is two facts rather than one: the native
// child and its cleanup settled, and this provider holds no
// handle for the session — a detach that failed, or a session
// prepared and never handed over, leaves one. Either one
// missing leaves the session owned rather than looking
// finished, which is what the next owner is told to recover
// deliberately.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// deliberately.

if (stopped && !holding(placement.sessionKey)) {
ownership.quiesced();
}
}
});

// Only here, and only once this provider is holding nothing. By the
// time `perform` returns the native child has exited and been reaped,
// so what is left to check is the ACP handle: a handoff that released
// it quiesces, and one that could not — a detach that failed, a
// session prepared but never handed over — leaves the session owned
// rather than looking finished.
if (!holding(placement.sessionKey)) {
ownership.quiesced();
}
yield* running.run(() =>
authority.perform(request, {
prepare: () =>
withSessionRoute(context, () =>
prepareLaunch(
invocation,
agentName,
callerCwd,
request.instructions,
placement,
),
),
detach: (prepared) =>
detachSession(invocation, prepared, agentCommandOf(placement)),
exit: (prepared) => runNativeUi(invocation, prepared, agentCommandOf(placement)),
}),
);
},
);
} catch (error) {
Expand Down
75 changes: 71 additions & 4 deletions packages/acp/tests/native-launch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@ import type {
PreparedLaunchRecord,
Session,
} from "@executablemd/core";
import { flushOutput, installControlledLauncher, reserveTerminal } from "@executablemd/runtime";
import {
flushOutput,
installControlledLauncher,
NativeLauncher,
reserveTerminal,
} from "@executablemd/runtime";
import type { AgentSessionCoordinator, NativeLaunchRequest } from "@executablemd/runtime";
import { createAcpxProvider } from "../src/provider.ts";
import type { AcpxProviderDependencies } from "../src/provider.ts";
Expand Down Expand Up @@ -206,6 +211,14 @@ interface ProviderOptions {
withSessionRoute?: AcpxProviderDependencies["withSessionRoute"];
/** Blocks the native child until this resolves. */
hold?: Operation<void>;
/**
* Make the launch's own teardown fail, in place of a child that cannot be
* proven stopped.
*
* Composed in front of the launcher rather than replacing it, so what fails
* is the cleanup of a launch that was otherwise ordinary.
*/
cleanupFails?: string;
onLaunch?: () => void;
exitCode?: number;
/**
Expand Down Expand Up @@ -328,6 +341,20 @@ function* installLaunchStack(
outcome: () => ({ exitCode: options.exitCode ?? 0 }),
});

if (options.cleanupFails !== undefined) {
const reason = options.cleanupFails;
yield* NativeLauncher.around({
*launch([request, spawned], next) {
// Registered inside the launch, so it unwinds with it — and refuses to
// say the child is gone.
yield* ensure(function* () {
throw new Error(reason);
});
return yield* next(request, spawned);
},
});
}

const factory = createAcpxProvider({
createRuntime: harness.create,
sessionStore: options.store ?? makeStore(),
Expand Down Expand Up @@ -2518,11 +2545,51 @@ describe("Tier CX — cancellation before ownership ends", () => {
),
),
).toBe(false);
const released = trace.ownership.events.indexOf("released-active");
const released = trace.ownership.events.indexOf("released-idle");
expect(trace.ownership.events.indexOf("cancelling") < released).toBe(true);
// A launch that stopped on the way never proved the session stopped, so it
// stays owned rather than looking finished.
// An orderly stop that finished is a stop. The child was proven gone, its
// cleanup settled, and this provider held no handle for the session — so
// nothing this owner started can still act on it, which is exactly what
// quiescence acknowledges. Withholding it here would leave a recovery
// tombstone for a cancellation that had already proved everything a normal
// return proves.
expect(trace.ownership.events).toContain("quiesced");
expect(trace.ownership.events).not.toContain("released-active");
});

it("CX2: a cancellation whose cleanup could not finish stays owned", function* () {
const harness = createFakeRuntime();
const trace = newTrace();
const hold = withResolvers<void>();
const started = withResolvers<void>();
let halting = "";

yield* scoped(function* () {
yield* installLaunchStack(harness, trace, {
routeStore: createMemorySessionRouteStore(),
cleanupFails: "the native child could not be proven stopped",
hold: (function* () {
started.resolve();
yield* hold.operation;
})(),
});

const launching = yield* spawn(() => Agent.operations.launch(launchRequest(INSTRUCTIONS)));
yield* started.operation;
try {
yield* launching.halt();
} catch (error) {
halting = error instanceof Error ? error.message : String(error);
}
});

// The teardown failed, and said so rather than passing quietly.
expect(halting).toContain("could not be proven stopped");
// So nothing was acknowledged: a cancellation is not evidence on its own,
// and neither is the lease coming back. The session stays owned, and the
// next owner is told to recover it deliberately.
expect(trace.ownership.events).not.toContain("quiesced");
expect(trace.ownership.events).toContain("released-active");
});
});

Expand Down
23 changes: 18 additions & 5 deletions packages/core/src/expand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ import { durableGrid, openTerminalGrid, toRequest } from "./terminal/grid.ts";
import type { PaneWork } from "./terminal/grid.ts";
import { recordGridLayout } from "./terminal/journal.ts";
import { usePaneTerminal } from "./terminal/pane.ts";
import { usePaneNativeLauncher } from "./terminal/pane-launcher.ts";
import {
asBindingViolation,
asExpressionViolation,
Expand Down Expand Up @@ -2236,13 +2237,28 @@ function paneWork(pane: TerminalPane, title: string, site: GridSite): PaneWork {
// in its content has no loop to exit and says so.
yield* ActiveLoop.set(undefined);
yield* usePaneTerminal(claim);
const shown: Segment[] = [];
// What this pane has rendered and not yet shown. A native UI is about
// to draw over the pane, so the same rule the root flush follows holds
// here: everything the pane has said reaches the reader first.
const flushPane = function* (): Operation<void> {
const pending = renderSegments(shown);
shown.length = 0;
if (pending.length > 0) {
yield* composite.display(pane.ordinal, pending);
}
};
// A `<Session.Launch>` written in this pane finds this launcher simply
// by being here: it reserves and flushes this pane instead of competing
// for the run's one foreground lease, and the child it starts is what
// makes this pane ready.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant comment — restates what the code does.

Suggested change
// makes this pane ready.

yield* usePaneNativeLauncher(claim, flushPane);
const siteEnv = yield* env;
// Starts from what the grid site can see and keeps its own writes: a
// binding this pane makes is visible to later work in this pane and to
// nothing else.
yield* provideEnv(derivedEnvironment(siteEnv, { ...(siteEnv?.values ?? {}) }));

const shown: Segment[] = [];
yield* expandSegmentsWithin(
pane.element.children,
site.parentMeta,
Expand All @@ -2266,10 +2282,7 @@ function paneWork(pane: TerminalPane, title: string, site: GridSite): PaneWork {
// one outside the grid.
undefined,
);
const text = renderSegments(shown);
if (text.length > 0) {
yield* composite.display(pane.ordinal, text);
}
yield* flushPane();
});
},
};
Expand Down
73 changes: 73 additions & 0 deletions packages/core/src/terminal/pane-launcher.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/**
* How a native UI reaches a pane's terminal instead of the run's
* (architecture.md §Terminal authority, spec §Terminal-grid composition).
*
* `<Session.Launch>` written at the root takes the one foreground-terminal
* lease, and every other launch waits for it. Written inside a pane it must
* not: panes stay interactive at the same time, which is the whole reason a
* grid exists. So core installs this in the pane's own scope, and the launch
* finds it simply by being there.
*
* Nothing about the launch changes. It is handed no pane prop, token,
* identifier or mode; its request, its result and its retained phases are the
* ones a root launch would have. What changes is which terminal answers
* `reserve` and `flush`, and that is a composition fact rather than something
* the document or the provider can see.
*
* The claim is the authority, and it is closed over rather than passed on. A
* pane claim buys one interactive terminal at one ordinal — it says nothing
* about which Agent session that pane may own, which stays the session
* coordinator's to answer.
*/

import { resource } from "effection";
import type { Operation } from "effection";
import { NativeLauncher } from "@executablemd/runtime";

import type { TerminalPaneClaim } from "./authority.ts";

/**
* Install one pane's native launcher for the scope that runs that pane's work.
*
* `flush` is how this pane catches the reader up. A pane's rendered text
* belongs to the pane, so it goes where the pane's text goes rather than to the
* root's streams — which the native UI is not drawing over.
*/
export function* usePaneNativeLauncher(
claim: TerminalPaneClaim,
flush: () => Operation<void>,
): Operation<void> {
yield* NativeLauncher.around({
/**
* This pane, for as long as the launch holds it.
*
* Deliberately not delegated: delegating would ask for the root lease,
* which the grid itself is already holding, and two panes would contend
* over a terminal neither of them is using. The claim refuses a second live
* launch on *this* pane and does not contend with any other, which is
* exactly the exclusivity a pane has.
*
* It is released when the launch's scope ends, so the pane is free only
* after the launcher has finished with the child it started.
*/
reserve() {
return resource<void>(function* (provide) {
yield* claim.admit(function* () {
yield* provide();
});
});
},
*flush() {
yield* flush();
},
*launch([request, spawned], next) {
// The exact request, untouched, to whichever host launcher is installed.
// What this adds is a listener: the pane is ready when the runtime says
// the child started, and at no earlier moment.
return yield* next(request, () => {
claim.ready();
spawned();
});
},
});
}
Loading
Loading