mirror of
https://github.com/anomalyco/opencode.git
synced 2026-11-09 22:25:02 +03:00
fix(app,server): stop subagent form polling from stalling sends (#57684)
This commit is contained in:
committed by
GitHub
Unverified
parent
ee8221c5d1
commit
946c331fce
@@ -93,7 +93,8 @@ test("a catalog refresh cannot replace an existing session's model", async ({ pa
|
||||
await expect(page.getByRole("button", { name: "Choose model variant", exact: true })).toHaveText("high")
|
||||
await view.editor.press("Enter")
|
||||
await expect.poll(() => prompts.length).toBe(1)
|
||||
expect(switches).toEqual([{ model: { id: "alpha", providerID: "opencode", variant: "high" } }])
|
||||
// The session already runs Alpha high, so the send has no switch to make.
|
||||
expect(switches).toEqual([])
|
||||
|
||||
catalog.current = provider(beta)
|
||||
await view.push([
|
||||
@@ -115,10 +116,7 @@ test("a catalog refresh cannot replace an existing session's model", async ({ pa
|
||||
await view.editor.fill("Use the replacement I chose")
|
||||
await view.editor.press("Enter")
|
||||
await expect.poll(() => prompts.length).toBe(2)
|
||||
expect(switches).toEqual([
|
||||
{ model: { id: "alpha", providerID: "opencode", variant: "high" } },
|
||||
{ model: { id: "beta", providerID: "opencode" } },
|
||||
])
|
||||
expect(switches).toEqual([{ model: { id: "beta", providerID: "opencode" } }])
|
||||
})
|
||||
|
||||
test("an existing session's model never shows a fallback while its agent loads", async ({ page }) => {
|
||||
|
||||
@@ -227,7 +227,7 @@ test("follow-up preference controls Enter while Mod+Enter uses the alternate del
|
||||
await expect(view.input).toHaveText("")
|
||||
})
|
||||
|
||||
test("a steer shows as pending in the timeline as the composer clears, before its selection commits", async ({
|
||||
test("a steer shows as pending in the timeline as the composer clears, before the server admits it", async ({
|
||||
page,
|
||||
}) => {
|
||||
// The running execution has delivered input, so a new steer waits behind its work.
|
||||
@@ -237,7 +237,7 @@ test("a steer shows as pending in the timeline as the composer clears, before it
|
||||
)
|
||||
|
||||
const view = await openQueue(page, mock, "steer")
|
||||
const commit = await holdRoute(page, (url) => url.pathname === `/api/session/${sessionID}/model`, { method: "POST" })
|
||||
const commit = await holdRoute(page, (url) => url.pathname === `/api/session/${sessionID}/prompt`, { method: "POST" })
|
||||
|
||||
const prompt = page.locator('[data-timeline-virtual-content] [data-timeline-row="UserMessage"]', {
|
||||
hasText: "steer while the model commits",
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { OpenCodeEvent } from "@opencode/client/promise"
|
||||
import { expect, test, type Page } from "@playwright/test"
|
||||
import { SERVER, sessionHref } from "../utils/app"
|
||||
import { installSseTransport } from "../utils/sse-transport"
|
||||
@@ -209,6 +210,67 @@ test("shows a pending permission dock", async ({ page }) => {
|
||||
await expect(page.locator('[data-component="composer"]')).toBeVisible()
|
||||
})
|
||||
|
||||
test("loads a running subagent's question without polling idle subagents on every update", async ({ page }) => {
|
||||
const runningID = "ses_request_running_subagent"
|
||||
const idleIDs = Array.from({ length: 20 }, (_, index) => `ses_request_idle_${index}`)
|
||||
|
||||
const workspace = await mockWorkspace(page, {
|
||||
name: "RequestDocks",
|
||||
directory,
|
||||
sessions: [
|
||||
{ id: sessionID, title },
|
||||
{ id: runningID, title: "Running subagent", parentID: sessionID },
|
||||
...idleIDs.map((id) => ({ id, title: id, parentID: sessionID })),
|
||||
],
|
||||
seed: { tabs: [sessionID] },
|
||||
// The mock reports a Session with a pending form as running.
|
||||
forms: [
|
||||
{
|
||||
id: "frm_subagent_question",
|
||||
sessionID: runningID,
|
||||
title: "Questions",
|
||||
metadata: { kind: "question" },
|
||||
fields: [{ key: "q0", type: "string", title: "Direction", description: "Which way should the subagent go?" }],
|
||||
},
|
||||
],
|
||||
})
|
||||
|
||||
// Idle subagents whose worktrees were deleted; a failed read is retried on its next sync.
|
||||
await page.route(/\/api\/session\/ses_request_idle_\d+\/form$/, (route) =>
|
||||
route.fulfill({ status: 404, json: { message: "Location not found" } }),
|
||||
)
|
||||
|
||||
const forms: string[] = []
|
||||
page.on("request", (request) => {
|
||||
const path = new URL(request.url()).pathname
|
||||
|
||||
if (request.method() === "GET" && path.startsWith("/api/session/") && path.endsWith("/form")) forms.push(path)
|
||||
})
|
||||
|
||||
await page.goto(sessionHref(sessionID))
|
||||
await expectSessionTitle(page, title)
|
||||
await expect(
|
||||
page.locator('[data-component="dock-prompt"][data-kind="question"]').getByText("Which way should the subagent go?"),
|
||||
).toBeVisible()
|
||||
|
||||
// Every session update used to re-request the forms of the whole subagent tree.
|
||||
await workspace.push(
|
||||
[...idleIDs, sessionID].map(
|
||||
(id, index) =>
|
||||
// SAFETY: the fixture event matches the generated OpenCodeEvent shape for its type.
|
||||
({
|
||||
id: `evt_request_rename_${index}`,
|
||||
created: index + 2,
|
||||
type: "session.renamed",
|
||||
durable: { aggregateID: id, seq: 1, version: 1 },
|
||||
data: { sessionID: id, title: id === sessionID ? "Renamed request docks" : `${id} renamed` },
|
||||
}) as OpenCodeEvent,
|
||||
),
|
||||
)
|
||||
await expect(page.getByRole("heading", { name: "Renamed request docks", exact: true })).toBeVisible()
|
||||
expect(forms.toSorted()).toEqual([`/api/session/${runningID}/form`, `/api/session/${sessionID}/form`].toSorted())
|
||||
})
|
||||
|
||||
test("restores the draft caret before typing after a request dock closes", async ({ page }) => {
|
||||
const transport = await installSseTransport(page, { server: SERVER, retry: 20 })
|
||||
await mockServer(page, { forms: [] })
|
||||
|
||||
@@ -214,10 +214,10 @@ describe("Composer submission", () => {
|
||||
current: { agent: "plan", model: { id: "old", providerID: "old" } },
|
||||
calls: ["admit", "switch-agent", "switch-model", "send"],
|
||||
},
|
||||
// The model still commits: cached session state may lag behind an earlier switch.
|
||||
// An unchanged selection sends without a model request to wait on.
|
||||
{
|
||||
current: { agent: "build", model: { providerID: "provider-1", id: "model-1", variant: "balanced" } },
|
||||
calls: ["admit", "switch-model", "send"],
|
||||
calls: ["admit", "send"],
|
||||
},
|
||||
// A prompt commits a staged revert, which would delete the model switch made after its boundary. The revert
|
||||
// settles before admission, whose row it would also delete.
|
||||
|
||||
@@ -16,6 +16,9 @@ import { parseSlashCommand } from "./client-slash-command"
|
||||
|
||||
const submitting = new WeakSet<object>()
|
||||
|
||||
// The model each session was last switched to from this client, by session ID.
|
||||
const switched = new Map<string, string>()
|
||||
|
||||
type ComposerSubmission = {
|
||||
id: SessionMessage.ID
|
||||
mode: "normal" | "shell"
|
||||
@@ -500,11 +503,23 @@ async function applySelection(
|
||||
|
||||
await beforeModel?.()
|
||||
|
||||
// The server deduplicates unchanged selections; cached SSE state may still be behind an earlier switch.
|
||||
// Skip an unchanged model so a send never waits on that request. An agent switch or a revert can move the
|
||||
// model, and cached SSE state may lag this client's earlier switch, which must also match.
|
||||
const model = `${selection.model.providerID}/${selection.model.modelID}/${selection.variant ?? ""}`
|
||||
const saved = current?.model && `${current.model.providerID}/${current.model.id}/${current.model.variant ?? ""}`
|
||||
|
||||
if (
|
||||
current?.agent === selection.agent &&
|
||||
!current.revert &&
|
||||
saved === model &&
|
||||
(switched.get(session.id) ?? model) === model
|
||||
)
|
||||
return
|
||||
await session.api.switchModel({
|
||||
sessionID: session.id,
|
||||
model: { id: selection.model.modelID, providerID: selection.model.providerID, variant: selection.variant },
|
||||
})
|
||||
switched.set(session.id, model)
|
||||
} catch (error) {
|
||||
cancel?.()
|
||||
throw error
|
||||
|
||||
@@ -28,13 +28,21 @@ export function createSessionRequestModel() {
|
||||
() => undefined,
|
||||
)
|
||||
})
|
||||
createEffect(() => {
|
||||
const id = params.id
|
||||
|
||||
if (!id || serverSDK.connection.status() !== "connected") return
|
||||
void Promise.all(
|
||||
sessionTreeIDs(data.session.list(), id).map((sessionID) => data.session.form.sync(sessionID)),
|
||||
).catch(() => undefined)
|
||||
// Only a running Session can hold a pending form, and events keep loaded forms current. Comparing by value
|
||||
// keeps every session update in a large subagent tree from re-requesting forms.
|
||||
const formSessions = createMemo(
|
||||
() =>
|
||||
sessionTreeIDs(data.session.list(), params.id).filter(
|
||||
(id) => id === params.id || data.session.status(id) === "running",
|
||||
),
|
||||
[],
|
||||
{ equals: (a, b) => a.join() === b.join() },
|
||||
)
|
||||
|
||||
createEffect(() => {
|
||||
if (serverSDK.connection.status() !== "connected") return
|
||||
void Promise.all(formSessions().map((sessionID) => data.session.form.sync(sessionID))).catch(() => undefined)
|
||||
})
|
||||
|
||||
const formRequest = createMemo((): FormInfo | undefined => {
|
||||
|
||||
@@ -2,7 +2,7 @@ import { buildLocationServiceMap } from "../location-services.js"
|
||||
import { LocationServiceMap } from "../location-service-map.js"
|
||||
import { LayerNode } from "@opencode/util/effect/layer-node"
|
||||
import { makeGlobalNode } from "@opencode/util/effect/app-node"
|
||||
import { Effect, Layer } from "effect"
|
||||
import { Effect, Layer, RcMap } from "effect"
|
||||
import { Instance } from "../instance/service.js"
|
||||
|
||||
const instances = makeGlobalNode({
|
||||
@@ -13,6 +13,7 @@ const instances = makeGlobalNode({
|
||||
const locations = yield* LocationServiceMap.Service
|
||||
return Instance.Service.of({
|
||||
provide: (session) => Effect.provide(locations.get(session.location)),
|
||||
live: (session) => RcMap.has(locations.rcMap, LocationServiceMap.canonical(session.location)),
|
||||
})
|
||||
}),
|
||||
),
|
||||
|
||||
@@ -12,6 +12,8 @@ export interface Interface {
|
||||
readonly provide: (
|
||||
session: Session.Info,
|
||||
) => <A, E, R>(effect: Effect.Effect<A, E, R>) => Effect.Effect<A, E | Error, Exclude<R, Services>>
|
||||
/** Whether `provide` would reuse built or booting capabilities. Selectors without it are assumed live. */
|
||||
readonly live?: (session: Session.Info) => Effect.Effect<boolean>
|
||||
}
|
||||
|
||||
export class Service extends Context.Service<Service, Interface>()("@opencode/Instance") {}
|
||||
|
||||
@@ -8,7 +8,7 @@ import { Location } from "@opencode/schema/location"
|
||||
import type { Session } from "@opencode/schema/session"
|
||||
import { makeGlobalNode } from "@opencode/util/effect/app-node"
|
||||
import type { LayerNode } from "@opencode/util/effect/layer-node"
|
||||
import { Context, Duration, Effect, Layer, LayerMap, Scope } from "effect"
|
||||
import { Context, Duration, Effect, Layer, LayerMap, RcMap, Scope } from "effect"
|
||||
|
||||
export interface Configuration {
|
||||
readonly plugins: InstancePlugins.List
|
||||
@@ -102,7 +102,7 @@ export function layer(options: Options, replacements: () => LayerNode.Replacemen
|
||||
),
|
||||
{ idleTimeToLive: Duration.infinity },
|
||||
)
|
||||
return Instance.Service.of({ provide })
|
||||
return Instance.Service.of({ provide, live: (session) => RcMap.has(instances.rcMap, key(session)) })
|
||||
}),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -3,7 +3,7 @@ import { LocationServiceMap } from "@opencode/core/location-services"
|
||||
import { Session } from "@opencode/core/session"
|
||||
import { LocationNotFoundError, InvalidRequestError, SessionNotFoundError } from "@opencode/protocol/errors"
|
||||
import { Effect, Layer } from "effect"
|
||||
import { HttpRouter, HttpServerRequest } from "effect/http"
|
||||
import { HttpRouter, HttpServerRequest, HttpServerResponse } from "effect/http"
|
||||
import { HttpApiMiddleware } from "effect/http-api"
|
||||
import { locationErrors, requestRef, sessionInfo, type LocationServices } from "../location"
|
||||
|
||||
@@ -21,7 +21,7 @@ export const formLocationLayer = Layer.effect(
|
||||
const instances = yield* Instance.Service
|
||||
const locations = yield* LocationServiceMap.Service
|
||||
|
||||
return FormLocationMiddleware.of((effect) =>
|
||||
return FormLocationMiddleware.of((effect, options) =>
|
||||
Effect.gen(function* () {
|
||||
const route = yield* HttpRouter.RouteContext
|
||||
if (route.params.sessionID === "global") {
|
||||
@@ -33,6 +33,10 @@ export const formLocationLayer = Layer.effect(
|
||||
}
|
||||
|
||||
const session = yield* sessionInfo(sessions, route.params.sessionID)
|
||||
// Forms live in memory, so a Session without live capabilities has none pending. Listing must
|
||||
// not boot its Location: clients poll whole subagent trees, including deleted worktrees.
|
||||
if (options.endpoint.identifier === "session.form.list" && instances.live && !(yield* instances.live(session)))
|
||||
return HttpServerResponse.jsonUnsafe({ data: [] })
|
||||
return yield* effect.pipe(instances.provide(session), locationErrors)
|
||||
}),
|
||||
)
|
||||
|
||||
@@ -43,7 +43,6 @@ it.live("returns LocationNotFoundError for a missing folder and recovers once it
|
||||
"/api/integration",
|
||||
`/api/session/${session.id}/permission`,
|
||||
`/api/experimental/session/${session.id}/instructions/entries`,
|
||||
`/api/session/${session.id}/form`,
|
||||
"/api/session/global/form",
|
||||
]
|
||||
for (const endpoint of endpoints) {
|
||||
|
||||
@@ -0,0 +1,50 @@
|
||||
import { expect } from "bun:test"
|
||||
import fs from "fs/promises"
|
||||
import { Effect } from "effect"
|
||||
import { tmpdirScoped } from "../../core/test/fixture/tmpdir"
|
||||
import { it } from "../../core/test/lib/effect"
|
||||
import { ServerFetch } from "../src/fetch"
|
||||
|
||||
it.live("lists no forms for a Session whose Location is not live, and pending forms once it is", () =>
|
||||
Effect.gen(function* () {
|
||||
const handler = yield* ServerFetch.make({
|
||||
app: { version: "test" },
|
||||
database: { path: ":memory:" },
|
||||
fs: { filewatcher: false },
|
||||
models: { fetch: false },
|
||||
})
|
||||
const request = (path: string, body?: unknown) =>
|
||||
Effect.promise(async () => {
|
||||
const response = await handler(
|
||||
new Request(`http://opencode.local${path}`, {
|
||||
method: body === undefined ? "GET" : "POST",
|
||||
headers: { "content-type": "application/json" },
|
||||
body: body === undefined ? undefined : JSON.stringify(body),
|
||||
}),
|
||||
)
|
||||
return { status: response.status, body: await response.json() }
|
||||
})
|
||||
const create = (directory: string) =>
|
||||
request("/api/session", { location: { directory } }).pipe(
|
||||
Effect.map((response) => (response.body as { data: { id: string } }).data.id),
|
||||
)
|
||||
|
||||
// A subagent whose worktree was deleted must not boot or 404.
|
||||
const deleted = yield* tmpdirScoped()
|
||||
const orphan = yield* create(deleted.path)
|
||||
yield* Effect.promise(() => fs.rm(deleted.path, { recursive: true, force: true }))
|
||||
expect(yield* request(`/api/session/${orphan}/form`)).toEqual({ status: 200, body: { data: [] } })
|
||||
|
||||
const directory = yield* tmpdirScoped()
|
||||
const live = yield* create(directory.path)
|
||||
const created = yield* request(`/api/session/${live}/form`, {
|
||||
title: "Pick one",
|
||||
fields: [{ key: "answer", type: "string" }],
|
||||
})
|
||||
expect(created.status).toBe(200)
|
||||
expect(yield* request(`/api/session/${live}/form`)).toEqual({
|
||||
status: 200,
|
||||
body: { data: [(created.body as { data: unknown }).data] },
|
||||
})
|
||||
}).pipe(Effect.scoped),
|
||||
)
|
||||
Reference in New Issue
Block a user