Skip to content

Commit 2ed756c

Browse files
authored
fix(session): restore busy route handling and add regression coverage (#20125)
1 parent 054f4be commit 2ed756c

6 files changed

Lines changed: 187 additions & 6 deletions

File tree

packages/opencode/src/server/middleware.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { Provider } from "../provider/provider"
22
import { NamedError } from "@opencode-ai/util/error"
33
import { NotFoundError } from "../storage/db"
4+
import { Session } from "../session"
45
import type { ContentfulStatusCode } from "hono/utils/http-status"
56
import type { ErrorHandler } from "hono"
67
import { HTTPException } from "hono/http-exception"
@@ -20,6 +21,9 @@ export function errorHandler(log: Log.Logger): ErrorHandler {
2021
else status = 500
2122
return c.json(err.toObject(), { status })
2223
}
24+
if (err instanceof Session.BusyError) {
25+
return c.json(new NamedError.Unknown({ message: err.message }).toObject(), { status: 400 })
26+
}
2327
if (err instanceof HTTPException) return err.getResponse()
2428
const message = err instanceof Error && err.stack ? err.stack : err.toString()
2529
return c.json(new NamedError.Unknown({ message }).toObject(), {

packages/opencode/src/session/index.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -849,7 +849,8 @@ export namespace Session {
849849
export const children = fn(SessionID.zod, (id) => runPromise((svc) => svc.children(id)))
850850
export const remove = fn(SessionID.zod, (id) => runPromise((svc) => svc.remove(id)))
851851
export async function updateMessage<T extends MessageV2.Info>(msg: T): Promise<T> {
852-
return runPromise((svc) => svc.updateMessage(MessageV2.Info.parse(msg) as T))
852+
MessageV2.Info.parse(msg)
853+
return runPromise((svc) => svc.updateMessage(msg))
853854
}
854855

855856
export const removeMessage = fn(z.object({ sessionID: SessionID.zod, messageID: MessageID.zod }), (input) =>
@@ -862,7 +863,8 @@ export namespace Session {
862863
)
863864

864865
export async function updatePart<T extends MessageV2.Part>(part: T): Promise<T> {
865-
return runPromise((svc) => svc.updatePart(MessageV2.Part.parse(part) as T))
866+
MessageV2.Part.parse(part)
867+
return runPromise((svc) => svc.updatePart(part))
866868
}
867869

868870
export const updatePartDelta = fn(

packages/opencode/src/session/revert.ts

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,20 +92,17 @@ export namespace SessionRevert {
9292
const sessionID = session.id
9393
const msgs = await Session.messages({ sessionID })
9494
const messageID = session.revert.messageID
95-
const preserve = [] as MessageV2.WithParts[]
9695
const remove = [] as MessageV2.WithParts[]
9796
let target: MessageV2.WithParts | undefined
9897
for (const msg of msgs) {
9998
if (msg.info.id < messageID) {
100-
preserve.push(msg)
10199
continue
102100
}
103101
if (msg.info.id > messageID) {
104102
remove.push(msg)
105103
continue
106104
}
107105
if (session.revert.partID) {
108-
preserve.push(msg)
109106
target = msg
110107
continue
111108
}
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
import { afterEach, describe, expect, mock, spyOn, test } from "bun:test"
2+
import { Instance } from "../../src/project/instance"
3+
import { Server } from "../../src/server/server"
4+
import { Session } from "../../src/session"
5+
import { ModelID, ProviderID } from "../../src/provider/schema"
6+
import { MessageID, PartID, type SessionID } from "../../src/session/schema"
7+
import { SessionPrompt } from "../../src/session/prompt"
8+
import { Log } from "../../src/util/log"
9+
import { tmpdir } from "../fixture/fixture"
10+
11+
Log.init({ print: false })
12+
13+
afterEach(async () => {
14+
mock.restore()
15+
await Instance.disposeAll()
16+
})
17+
18+
async function user(sessionID: SessionID, text: string) {
19+
const msg = await Session.updateMessage({
20+
id: MessageID.ascending(),
21+
role: "user",
22+
sessionID,
23+
agent: "build",
24+
model: { providerID: ProviderID.make("test"), modelID: ModelID.make("test") },
25+
time: { created: Date.now() },
26+
})
27+
await Session.updatePart({
28+
id: PartID.ascending(),
29+
sessionID,
30+
messageID: msg.id,
31+
type: "text",
32+
text,
33+
})
34+
return msg
35+
}
36+
37+
describe("session action routes", () => {
38+
test("abort route calls SessionPrompt.cancel", async () => {
39+
await using tmp = await tmpdir({ git: true })
40+
await Instance.provide({
41+
directory: tmp.path,
42+
fn: async () => {
43+
const session = await Session.create({})
44+
const cancel = spyOn(SessionPrompt, "cancel").mockResolvedValue()
45+
const app = Server.Default()
46+
47+
const res = await app.request(`/session/${session.id}/abort`, {
48+
method: "POST",
49+
})
50+
51+
expect(res.status).toBe(200)
52+
expect(await res.json()).toBe(true)
53+
expect(cancel).toHaveBeenCalledWith(session.id)
54+
55+
await Session.remove(session.id)
56+
},
57+
})
58+
})
59+
60+
test("delete message route returns 400 when session is busy", async () => {
61+
await using tmp = await tmpdir({ git: true })
62+
await Instance.provide({
63+
directory: tmp.path,
64+
fn: async () => {
65+
const session = await Session.create({})
66+
const msg = await user(session.id, "hello")
67+
const busy = spyOn(SessionPrompt, "assertNotBusy").mockRejectedValue(new Session.BusyError(session.id))
68+
const remove = spyOn(Session, "removeMessage").mockResolvedValue(msg.id)
69+
const app = Server.Default()
70+
71+
const res = await app.request(`/session/${session.id}/message/${msg.id}`, {
72+
method: "DELETE",
73+
})
74+
75+
expect(res.status).toBe(400)
76+
expect(busy).toHaveBeenCalledWith(session.id)
77+
expect(remove).not.toHaveBeenCalled()
78+
79+
await Session.remove(session.id)
80+
},
81+
})
82+
})
83+
})

packages/opencode/test/session/compaction.test.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,36 @@ describe("session.compaction.prune", () => {
509509
})
510510

511511
describe("session.compaction.process", () => {
512+
test("throws when parent is not a user message", async () => {
513+
await using tmp = await tmpdir()
514+
await Instance.provide({
515+
directory: tmp.path,
516+
fn: async () => {
517+
const session = await Session.create({})
518+
const msg = await user(session.id, "hello")
519+
const reply = await assistant(session.id, msg.id, tmp.path)
520+
const rt = runtime("continue")
521+
try {
522+
const msgs = await Session.messages({ sessionID: session.id })
523+
await expect(
524+
rt.runPromise(
525+
SessionCompaction.Service.use((svc) =>
526+
svc.process({
527+
parentID: reply.id,
528+
messages: msgs,
529+
sessionID: session.id,
530+
auto: false,
531+
}),
532+
),
533+
),
534+
).rejects.toThrow(`Compaction parent must be a user message: ${reply.id}`)
535+
} finally {
536+
await rt.dispose()
537+
}
538+
},
539+
})
540+
})
541+
512542
test("publishes compacted event on continue", async () => {
513543
await using tmp = await tmpdir()
514544
await Instance.provide({

packages/opencode/test/session/prompt-effect.test.ts

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import { NodeFileSystem } from "@effect/platform-node"
2-
import { expect } from "bun:test"
2+
import { expect, spyOn } from "bun:test"
33
import { Cause, Effect, Exit, Fiber, Layer, ServiceMap } from "effect"
44
import * as Stream from "effect/Stream"
5+
import z from "zod"
56
import type { Agent } from "../../src/agent/agent"
67
import { Agent as AgentSvc } from "../../src/agent/agent"
78
import { Bus } from "../../src/bus"
@@ -25,6 +26,7 @@ import { MessageID, PartID, SessionID } from "../../src/session/schema"
2526
import { SessionStatus } from "../../src/session/status"
2627
import { Shell } from "../../src/shell/shell"
2728
import { Snapshot } from "../../src/snapshot"
29+
import { TaskTool } from "../../src/tool/task"
2830
import { ToolRegistry } from "../../src/tool/registry"
2931
import { Truncate } from "../../src/tool/truncate"
3032
import { Log } from "../../src/util/log"
@@ -630,6 +632,69 @@ it.effect(
630632
30_000,
631633
)
632634

635+
it.effect(
636+
"cancel finalizes subtask tool state",
637+
() =>
638+
provideTmpdirInstance(
639+
(dir) =>
640+
Effect.gen(function* () {
641+
const ready = defer<void>()
642+
const aborted = defer<void>()
643+
const init = spyOn(TaskTool, "init").mockImplementation(async () => ({
644+
description: "task",
645+
parameters: z.object({
646+
description: z.string(),
647+
prompt: z.string(),
648+
subagent_type: z.string(),
649+
task_id: z.string().optional(),
650+
command: z.string().optional(),
651+
}),
652+
execute: async (_args, ctx) => {
653+
ready.resolve()
654+
ctx.abort.addEventListener("abort", () => aborted.resolve(), { once: true })
655+
await new Promise<void>(() => {})
656+
return {
657+
title: "",
658+
metadata: {
659+
sessionId: SessionID.make("task"),
660+
model: ref,
661+
},
662+
output: "",
663+
}
664+
},
665+
}))
666+
yield* Effect.addFinalizer(() => Effect.sync(() => init.mockRestore()))
667+
668+
const { prompt, chat } = yield* boot()
669+
const msg = yield* user(chat.id, "hello")
670+
yield* addSubtask(chat.id, msg.id)
671+
672+
const fiber = yield* prompt.loop({ sessionID: chat.id }).pipe(Effect.forkChild)
673+
yield* Effect.promise(() => ready.promise)
674+
yield* prompt.cancel(chat.id)
675+
yield* Effect.promise(() => aborted.promise)
676+
677+
const exit = yield* Fiber.await(fiber)
678+
expect(Exit.isSuccess(exit)).toBe(true)
679+
680+
const msgs = yield* Effect.promise(() => MessageV2.filterCompacted(MessageV2.stream(chat.id)))
681+
const taskMsg = msgs.find((item) => item.info.role === "assistant" && item.info.agent === "general")
682+
expect(taskMsg?.info.role).toBe("assistant")
683+
if (!taskMsg || taskMsg.info.role !== "assistant") return
684+
685+
const tool = toolPart(taskMsg.parts)
686+
expect(tool?.type).toBe("tool")
687+
if (!tool) return
688+
689+
expect(tool.state.status).not.toBe("running")
690+
expect(taskMsg.info.time.completed).toBeDefined()
691+
expect(taskMsg.info.finish).toBeDefined()
692+
}),
693+
{ git: true, config: cfg },
694+
),
695+
30_000,
696+
)
697+
633698
it.effect(
634699
"cancel with queued callers resolves all cleanly",
635700
() =>

0 commit comments

Comments
 (0)