-
Notifications
You must be signed in to change notification settings - Fork 337
fix(sdk): restore durable workflow gate control endpoints #4064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,9 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { mkdtemp, readFile, rm } from "node:fs/promises"; | ||
| import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; | ||
| import * as os from "node:os"; | ||
| import * as path from "node:path"; | ||
| import type { ExtensionAPI } from "../../extensibility/extensions"; | ||
| import { Broker } from "../broker/broker"; | ||
| import { | ||
| createInvocationReconciliation, | ||
| createSdkSessionRuntimeExtension, | ||
|
|
@@ -263,6 +264,7 @@ describe("SessionSdkSessionRuntime", () => { | |
| } as any; | ||
| const transports: Array<{ starts: number; stops: number }> = []; | ||
| createSdkSessionRuntimeExtension(api, { | ||
| agentDir: path.join(cwd, ".gjc", "agent"), | ||
| createTransport: async ({ sessionId, stateRoot, token }) => { | ||
| const stats = { starts: 0, stops: 0 }; | ||
| const failFirstStop = transports.length === 0; | ||
|
|
@@ -281,6 +283,9 @@ describe("SessionSdkSessionRuntime", () => { | |
| sendFrame: () => {}, | ||
| start: async () => { | ||
| stats.starts += 1; | ||
| const endpoint = path.join(stateRoot, "sdk", `${sessionId}.json`); | ||
| await mkdir(path.dirname(endpoint), { recursive: true }); | ||
| await writeFile(endpoint, JSON.stringify({ sessionId, token, pid: process.pid })); | ||
| return { url: `ws://127.0.0.1:${30_000 + stats.starts}` }; | ||
| }, | ||
| stop: async () => { | ||
|
|
@@ -316,6 +321,95 @@ describe("SessionSdkSessionRuntime", () => { | |
| await rm(cwd, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
| test("keeps a local SDK-only host alive through broker failure and registers after recovery", async () => { | ||
| const cwd = await mkdtemp(path.join(os.tmpdir(), "gjc-sdk-broker-recovery-")); | ||
| const agentDir = path.join(cwd, ".gjc", "agent"); | ||
| await mkdir(path.dirname(agentDir), { recursive: true }); | ||
| await writeFile(agentDir, "blocked"); | ||
| const handlers = new Map<string, (event: unknown, ctx: any) => Promise<void> | void>(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Both newly added broker tests declare their handler context as AGENTS.md reference: AGENTS.md:L113-L113 Useful? React with 👍 / 👎. |
||
| const api = { | ||
| on(event: string, handler: (event: unknown, ctx: any) => Promise<void> | void) { | ||
| handlers.set(event, handler); | ||
| }, | ||
| } as any; | ||
| const sessionId = "broker-recovery"; | ||
| createSdkSessionRuntimeExtension(api, { | ||
| agentDir, | ||
| createTransport: async ({ stateRoot, token }) => ({ | ||
| sessionId, | ||
| stateRoot, | ||
| token, | ||
| onFrame: () => undefined, | ||
| sendFrame: () => {}, | ||
| start: async () => { | ||
| const endpoint = path.join(stateRoot, "sdk", `${sessionId}.json`); | ||
| await mkdir(path.dirname(endpoint), { recursive: true }); | ||
| await writeFile(endpoint, JSON.stringify({ sessionId, token, pid: process.pid })); | ||
| return { url: "ws://127.0.0.1:1" }; | ||
| }, | ||
| stop: async () => {}, | ||
| }), | ||
| }); | ||
| const context = extensionContext(sessionId, cwd); | ||
| let broker: Broker | undefined; | ||
| try { | ||
| await handlers.get("session_start")?.({}, context); | ||
| await rm(agentDir); | ||
| await mkdir(agentDir, { recursive: true }); | ||
| broker = new Broker({ agentDir }); | ||
| await broker.start(); | ||
| await handlers.get("turn_start")?.({}, context); | ||
| expect(await broker.handleRequest("session.get_endpoint", { sessionId, endpointGeneration: 1 })).toMatchObject( | ||
| { | ||
| ok: true, | ||
| result: { sessionId, token: expect.any(String) }, | ||
| }, | ||
| ); | ||
| await handlers.get("session_shutdown")?.({}, context); | ||
| expect(await broker.handleRequest("session.get_endpoint", { sessionId, endpointGeneration: 1 })).toMatchObject( | ||
| { | ||
| ok: false, | ||
| error: { code: "endpoint_stale", message: "session endpoint is stale" }, | ||
| }, | ||
| ); | ||
| } finally { | ||
| await broker?.stop(); | ||
| await rm(cwd, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
||
| test("rejects lifecycle-required SDK-only startup when broker registration fails", async () => { | ||
| const cwd = await mkdtemp(path.join(os.tmpdir(), "gjc-sdk-broker-required-")); | ||
| const agentDir = path.join(cwd, ".gjc", "agent"); | ||
| await mkdir(path.dirname(agentDir), { recursive: true }); | ||
| await writeFile(agentDir, "blocked"); | ||
| const handlers = new Map<string, (event: unknown, ctx: any) => Promise<void> | void>(); | ||
| const api = { | ||
| on(event: string, handler: (event: unknown, ctx: any) => Promise<void> | void) { | ||
| handlers.set(event, handler); | ||
| }, | ||
| } as any; | ||
| createSdkSessionRuntimeExtension(api, { | ||
| agentDir, | ||
| brokerRegistrationRequired: true, | ||
| createTransport: async ({ sessionId, stateRoot, token }) => ({ | ||
| sessionId, | ||
| stateRoot, | ||
| token, | ||
| onFrame: () => undefined, | ||
| sendFrame: () => {}, | ||
| start: async () => ({ url: "ws://127.0.0.1:1" }), | ||
| stop: async () => {}, | ||
| }), | ||
| }); | ||
| try { | ||
| await expect( | ||
| handlers.get("session_start")?.({}, extensionContext("broker-required", cwd)), | ||
| ).rejects.toBeDefined(); | ||
| } finally { | ||
| await rm(cwd, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
| }); | ||
| interface PreflightHooks { | ||
| onPreflightAccepted?: () => void; | ||
|
|
@@ -359,6 +453,7 @@ async function invocationHarness( | |
| sendUserMessage: hooks.sendUserMessage ?? (async () => {}), | ||
| } as unknown as ExtensionAPI; | ||
| createSdkSessionRuntimeExtension(api, { | ||
| agentDir: cwd, | ||
| createTransport: async ({ sessionId: id, stateRoot, token }) => ({ | ||
| sessionId: id, | ||
| stateRoot, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,8 @@ import { type ModelSelectorValue, normalizeModelSelectorValue } from "../../conf | |
| import { type Settings, validateSettingPatch } from "../../config/settings"; | ||
| import type { ExtensionAPI, ExtensionCommandContext, ExtensionContext } from "../../extensibility/extensions"; | ||
| import { parseThinkingLevel } from "../../thinking"; | ||
| import { ensureBroker } from "../broker/ensure"; | ||
| import { SessionIndex } from "../broker/session-index"; | ||
| import { elevationAuthorityPath, verifyElevationCapability } from "../elevation/capability"; | ||
| import { | ||
| collectAuthenticatedProfileProviders, | ||
|
|
@@ -218,6 +220,10 @@ export class SessionSdkSessionRuntime { | |
|
|
||
| /** Narrow extension-facing factory for the SDK-only session path. */ | ||
| export interface CreateSdkSessionRuntimeOptions { | ||
| /** Authoritative broker state root for this session's endpoint lifecycle. */ | ||
| agentDir: string; | ||
| /** Lifecycle-owned sessions require broker publication before they become usable. */ | ||
| brokerRegistrationRequired?: boolean; | ||
| createTransport(input: { | ||
| sessionId: string; | ||
| stateRoot: string; | ||
|
|
@@ -906,6 +912,52 @@ function containsSecretConfigKey(value: unknown, seen = new Set<object>()): bool | |
| containsSecretConfigKey(nested, seen), | ||
| ); | ||
| } | ||
| async function resolveSdkWorkflowGate( | ||
| ctx: ExtensionContext, | ||
| operation: "workflow.gate_answer" | "workflow.plan_approve", | ||
| id: string, | ||
| answer: unknown, | ||
| expectedSessionId: string | undefined, | ||
| idempotencyKey: string, | ||
| canResolve: () => boolean, | ||
| ): Promise<unknown> { | ||
| if (!canResolve()) | ||
| throw Object.assign(new Error("Workflow gate is no longer answerable."), { code: "resource_gone" }); | ||
| if (expectedSessionId !== undefined && expectedSessionId !== ctx.sessionManager.getSessionId()) | ||
| throw Object.assign(new Error("Workflow gate session does not match this endpoint."), { code: "resource_gone" }); | ||
| if (expectedSessionId === undefined) logger.warn("workflow_control_missing_expected_session_id", { operation }); | ||
| const workflowGate = ctx.workflowGate; | ||
| if ( | ||
| typeof workflowGate?.resolveGate !== "function" || | ||
| typeof workflowGate.recoverAcceptedGates !== "function" || | ||
| typeof workflowGate.lookupCompletedResolution !== "function" || | ||
| typeof workflowGate.prepareTerminalization !== "function" || | ||
| typeof workflowGate.clearPreparedTerminalization !== "function" | ||
| ) | ||
| throw Object.assign(new Error("Workflow gates are unavailable for this session."), { code: "resource_gone" }); | ||
| const response = { gate_id: id, answer, idempotency_key: idempotencyKey }; | ||
| const completed = workflowGate.lookupCompletedResolution(response); | ||
| if (completed.kind === "completed") return completed.resolution; | ||
| if (completed.kind === "accepted_incomplete") { | ||
| await workflowGate.recoverAcceptedGates(); | ||
| const recovered = workflowGate.lookupCompletedResolution(response); | ||
| if (recovered.kind === "completed") return recovered.resolution; | ||
| throw Object.assign(new Error("Workflow gate resolution outcome is uncertain."), { code: "terminal_uncertain" }); | ||
| } | ||
| if (!workflowGate.prepareTerminalization(id, "not_published")) | ||
| throw Object.assign(new Error("Workflow gate is no longer answerable."), { code: "resource_gone" }); | ||
| try { | ||
| const resolution = await workflowGate.resolveGate(response); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| if ((resolution as { status?: unknown }).status === "rejected") workflowGate.clearPreparedTerminalization(id); | ||
| return resolution; | ||
| } catch (error) { | ||
| const stillPending = workflowGate.listPendingGates?.().some(gate => gate.gate_id === id) === true; | ||
| if (stillPending) workflowGate.clearPreparedTerminalization(id); | ||
| else workflowGate.quarantineGate?.(id); | ||
|
Comment on lines
+954
to
+956
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| function createControlSurface( | ||
| ctx: ExtensionContext, | ||
| api: ExtensionAPI, | ||
|
|
@@ -916,6 +968,8 @@ function createControlSurface( | |
| configOverrides?: Map<string, unknown>, | ||
| configRevision: { current: number } = { current: 0 }, | ||
| elevationAuthorityToken?: string, | ||
| canResolveGate: () => boolean = () => true, | ||
| trackGateResolution: <T>(resolution: Promise<T>) => Promise<T> = async resolution => await resolution, | ||
| ): ControlSurface { | ||
| const surfacePolicy = | ||
| policy ?? createSdkSurfacePolicyForContext(ctx, hasSdkWorkflowGateCapability(ctx.workflowGate)); | ||
|
|
@@ -1084,9 +1138,22 @@ function createControlSurface( | |
| return await submit("prompt", undefined, options => api.sendUserMessage(text, options)); | ||
| }, | ||
| answerAsk: unavailable("ask.answer"), | ||
| answerGate: (_id, _response, _expectedSessionId, _idempotencyKey, _elevationRequestId) => | ||
| unavailable("workflow.gate_answer")(), | ||
| approvePlan: (_id, _choice, _expectedSessionId, _elevationRequestId) => unavailable("workflow.plan_approve")(), | ||
| answerGate: async (id, response, expectedSessionId, idempotencyKey) => | ||
| await trackGateResolution( | ||
| resolveSdkWorkflowGate( | ||
| ctx, | ||
| "workflow.gate_answer", | ||
| id, | ||
| response, | ||
| expectedSessionId, | ||
| idempotencyKey ?? id, | ||
| canResolveGate, | ||
| ), | ||
| ), | ||
| approvePlan: async (id, choice, expectedSessionId) => | ||
| await trackGateResolution( | ||
| resolveSdkWorkflowGate(ctx, "workflow.plan_approve", id, choice, expectedSessionId, id, canResolveGate), | ||
| ), | ||
| invokeSkill: async (name, args, clientRef) => { | ||
| if (!ctx.invokeSkill) return unavailable("skill.invoke")(); | ||
| if (args !== undefined && typeof args !== "string") | ||
|
|
@@ -1268,6 +1335,9 @@ export function createSdkSessionRuntimeExtension(api: ExtensionAPI, options: Cre | |
| cursors: CursorRegistry; | ||
| reconciliation: InvocationReconciliation; | ||
| pending: Array<{ kind: InvocationKind; correlation: InvocationCorrelation }>; | ||
| registerBroker: () => Promise<void>; | ||
| fenceGateResolutions: () => void; | ||
| waitForGateResolutionQuiescence: () => Promise<void>; | ||
| activeInvocation?: { kind: InvocationKind; correlation: InvocationCorrelation }; | ||
| disposeGate?: () => void; | ||
| } | ||
|
|
@@ -1286,9 +1356,12 @@ export function createSdkSessionRuntimeExtension(api: ExtensionAPI, options: Cre | |
| }; | ||
| api.on("agent_start", async (_event, ctx) => await emitLifecycle("agent_start", ctx)); | ||
| api.on("agent_end", async (_event, ctx) => await emitLifecycle("agent_end", ctx)); | ||
| api.on("turn_start", (_event, ctx) => | ||
| active?.runtime.emitEvent({ type: "turn_start", sessionId: ctx.sessionManager.getSessionId() }), | ||
| ); | ||
| api.on("turn_start", async (_event, ctx) => { | ||
| const current = active; | ||
| if (!current) return; | ||
| await current.registerBroker(); | ||
| current.runtime.emitEvent({ type: "turn_start", sessionId: ctx.sessionManager.getSessionId() }); | ||
|
Comment on lines
+1359
to
+1363
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When broker discovery repeatedly fails—for example, a broker child launches but never publishes discovery—every model turn now awaits Useful? React with 👍 / 👎. |
||
| }); | ||
| api.on("turn_end", (_event, ctx) => | ||
| active?.runtime.emitEvent({ type: "turn_end", sessionId: ctx.sessionManager.getSessionId() }), | ||
| ); | ||
|
|
@@ -1321,6 +1394,20 @@ export function createSdkSessionRuntimeExtension(api: ExtensionAPI, options: Cre | |
| // read the live generation/seq once the runtime exists (Q30 atomic | ||
| // capture, C9). | ||
| let eventWatermarkSource: () => { generation: number; seq: number } = () => ({ generation: 0, seq: 0 }); | ||
| let acceptingGateResolutions = true; | ||
| const inFlightGateResolutions = new Set<Promise<unknown>>(); | ||
| const trackGateResolution = <T>(resolution: Promise<T>): Promise<T> => { | ||
| const tracked = resolution.finally(() => inFlightGateResolutions.delete(tracked)); | ||
| inFlightGateResolutions.add(tracked); | ||
| return tracked; | ||
| }; | ||
| const waitForGateResolutionQuiescence = async (): Promise<void> => { | ||
| const settled = Promise.allSettled(inFlightGateResolutions); | ||
| const timeout = Bun.sleep(5_000).then(() => { | ||
| throw new Error("Timed out waiting for SDK workflow gate resolutions to settle."); | ||
| }); | ||
|
Comment on lines
+1406
to
+1408
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an SDK-only session has no in-flight gate work, as in a normal print-mode shutdown, Useful? React with 👍 / 👎. |
||
| await Promise.race([settled, timeout]); | ||
| }; | ||
| const surfaceFactory = createSdkSurfaceFactory({ | ||
| ctx, | ||
| id: sessionId, | ||
|
|
@@ -1345,6 +1432,8 @@ export function createSdkSessionRuntimeExtension(api: ExtensionAPI, options: Cre | |
| options.configOverrides, | ||
| configRevision, | ||
| elevationAuthorityToken, | ||
| () => acceptingGateResolutions, | ||
| trackGateResolution, | ||
| ); | ||
| let runtime: SessionSdkSessionRuntime; | ||
| const installProviderDefinitions = (capability: string, definitions: unknown): void => { | ||
|
|
@@ -1458,9 +1547,45 @@ export function createSdkSessionRuntimeExtension(api: ExtensionAPI, options: Cre | |
| const disposeGate = ctx.workflowGate?.onGateEmitted?.(gate => | ||
| runtime.emitEvent({ kind: "workflow_gate", payload: gate }), | ||
| ); | ||
| active = { runtime, revisions, cursors, reconciliation, pending, disposeGate }; | ||
| let brokerRegistered = false; | ||
| const registerBroker = async (): Promise<void> => { | ||
| if (brokerRegistered) return; | ||
| try { | ||
| await ensureBroker({ agentDir: options.agentDir }); | ||
| const index = await new SessionIndex(options.agentDir).open(); | ||
| const locator = { repo: path.resolve(ctx.cwd), stateRoot }; | ||
| await runtime.registerWithBroker({ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the session index opens but registration itself persistently fails—for example, because the index has a corrupt suffix— Useful? React with 👍 / 👎. |
||
| register: async input => { | ||
| const endpointMtimeMs = (await fs.stat(path.join(input.stateRoot, "sdk", `${input.sessionId}.json`))) | ||
| .mtimeMs; | ||
| await index.append({ type: "host_registered", ...input, locator, pid: process.pid, endpointMtimeMs }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an SDK-only host's workspace is deleted or its endpoint becomes unreachable, the broker can signal it only if the registration contains the host's OS process incarnation; otherwise Useful? React with 👍 / 👎.
Comment on lines
+1559
to
+1561
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a Useful? React with 👍 / 👎. |
||
| }, | ||
| unregister: async input => { | ||
| await index.append({ type: "host_unregistered", ...input, locator, pid: process.pid }); | ||
| }, | ||
| }); | ||
| brokerRegistered = true; | ||
| } catch (error) { | ||
| if (options.brokerRegistrationRequired) throw error; | ||
| logger.warn(`sdk broker registration unavailable: ${String(error)}`); | ||
| } | ||
| }; | ||
| active = { | ||
| runtime, | ||
| revisions, | ||
| cursors, | ||
| reconciliation, | ||
| pending, | ||
| registerBroker, | ||
| fenceGateResolutions: () => { | ||
| acceptingGateResolutions = false; | ||
| }, | ||
| waitForGateResolutionQuiescence, | ||
| disposeGate, | ||
| }; | ||
| try { | ||
| await runtime.start(); | ||
| await registerBroker(); | ||
| } catch (error) { | ||
| active = undefined; | ||
| disposeGate?.(); | ||
|
|
@@ -1471,7 +1596,19 @@ export function createSdkSessionRuntimeExtension(api: ExtensionAPI, options: Cre | |
| code: errorCode(cleanupError), | ||
| error: String(cleanupError), | ||
| }); | ||
| active = { runtime, revisions, cursors, reconciliation, pending, disposeGate }; | ||
| active = { | ||
| runtime, | ||
| revisions, | ||
| cursors, | ||
| reconciliation, | ||
| pending, | ||
| registerBroker, | ||
| fenceGateResolutions: () => { | ||
| acceptingGateResolutions = false; | ||
| }, | ||
| waitForGateResolutionQuiescence, | ||
| disposeGate, | ||
| }; | ||
| throw new AggregateError([error, cleanupError], "SDK runtime startup failed and cleanup failed."); | ||
| } | ||
| cursors.close(); | ||
|
|
@@ -1481,10 +1618,12 @@ export function createSdkSessionRuntimeExtension(api: ExtensionAPI, options: Cre | |
| }; | ||
| const stopActive = async (): Promise<void> => { | ||
| const current = active; | ||
| active = undefined; | ||
| if (!current) return; | ||
| current.disposeGate?.(); | ||
| current.fenceGateResolutions(); | ||
| try { | ||
| await current.waitForGateResolutionQuiescence(); | ||
|
Comment on lines
+1622
to
+1624
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
During Useful? React with 👍 / 👎. |
||
| active = undefined; | ||
| current.disposeGate?.(); | ||
| await current.runtime.stop(); | ||
| } catch (error) { | ||
| logger.error("sdk runtime stop failed", { code: errorCode(error), error: String(error) }); | ||
|
Comment on lines
1628
to
1629
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a gate resolution remains in flight for more than the new five-second bound, Useful? React with 👍 / 👎. |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changed import adds more named imports from
node:fs/promises, while the repository convention requires Node modules—includingfs/promises—to use namespace imports. Convert this toimport * as fs from "node:fs/promises"and qualify the helper calls so the new tests follow the enforced filesystem convention.AGENTS.md reference: AGENTS.md:L132-L132
Useful? React with 👍 / 👎.