diff --git a/internal/web/sessions.go b/internal/web/sessions.go index 234c496..de84f80 100644 --- a/internal/web/sessions.go +++ b/internal/web/sessions.go @@ -4,7 +4,6 @@ import ( "encoding/json" "io" "net/http" - "time" "github.com/cloudwego/eino/schema" "github.com/cnjack/jcode/internal/config" @@ -213,10 +212,17 @@ func (s *Server) handleDeleteSession(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "session id is required"}) return } - // Tear down the live engine for this task (if any) so its run is cancelled and - // resources reclaimed. The active foreground engine is left in place — but its - // recorder is reset to a fresh session so post-delete writes don't land in the - // now-unlinked file (silent data loss). + // Refuse while the agent is mid-run. Cancelling + deleting a live task races + // the recorder (file/index resurrection) and is a bad UX for an intentional + // stop — the user should stop the run first, then delete. + if eng := s.resolveEngine(id); eng != nil && eng.running.Load() { + writeJSON(w, http.StatusConflict, map[string]string{"error": "agent is currently running"}) + return + } + // Tear down the live engine for this task (if any) so leftover cancel state + // is cleared and resources reclaimed. The active foreground engine is left + // in place — but its recorder is reset so post-delete writes don't land in + // the now-unlinked file (silent data loss). if eng := s.resolveEngine(id); eng != nil { eng.emu.Lock() cancel := eng.runCancel @@ -227,12 +233,7 @@ func (s *Server) handleDeleteSession(w http.ResponseWriter, r *http.Request) { if eng != s.activeEngine() { s.deleteEngine(id) } else { - // Active task: wait for the cancelled run to drain so its final - // RecordAssistant/usage writes land before we close + reset the recorder - // (a post-close write would re-create and truncate the file). - for i := 0; i < 200 && eng.running.Load(); i++ { - time.Sleep(5 * time.Millisecond) - } + // Not running (guarded above): safe to close the recorder now. eng.emu.Lock() if eng.recorder != nil && eng.recorder.UUID() == id { eng.recorder.Close() diff --git a/internal/web/tasks_test.go b/internal/web/tasks_test.go index 004c68c..133ac44 100644 --- a/internal/web/tasks_test.go +++ b/internal/web/tasks_test.go @@ -196,6 +196,34 @@ func TestUpdateTaskMetaResponseShape(t *testing.T) { } } +// Running tasks must not be deleted: cancel+delete races the recorder and is +// the wrong product behaviour (stop first, then delete). +func TestDeleteSessionRejectsRunning(t *testing.T) { + seedIndex(t, map[string][]session.SessionMeta{ + "/work/p": {{UUID: "run-1", Project: "/work/p"}}, + }) + eng := &Engine{taskID: "run-1", pwd: "/work/p"} + eng.running.Store(true) + s := &Server{Engine: eng, tasks: map[string]*Engine{"run-1": eng}} + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodDelete, "/api/sessions/run-1", nil) + req.SetPathValue("id", "run-1") + s.handleDeleteSession(rec, req) + if rec.Code != http.StatusConflict { + t.Fatalf("delete while running: code=%d body=%q, want 409", rec.Code, rec.Body.String()) + } + all, err := session.ListAllSessions() + if err != nil { + t.Fatal(err) + } + if len(all["/work/p"]) != 1 || all["/work/p"][0].UUID != "run-1" { + t.Fatalf("running session must remain indexed, got %+v", all["/work/p"]) + } + if !eng.running.Load() { + t.Fatal("delete must not clear the running flag") + } +} + // Regression: deleting a task that belongs to a project OTHER than the active // one (s.pwd) must still remove it from the index — the sidebar tree can delete // across projects. Previously this silently no-op'd, leaving a ghost task. diff --git a/web/src/app/store.ts b/web/src/app/store.ts index 2597b0c..b82ca1f 100644 --- a/web/src/app/store.ts +++ b/web/src/app/store.ts @@ -497,6 +497,14 @@ const sessionSlice = createSlice({ const { uuid: _uuid, ...rest } = a.payload Object.assign(t, rest) }, + /** Remove a deleted session from BOTH lists, inside the reducer (operates + * on the latest state — a component-side filter over render-scope copies + * would clobber any list update that landed during the delete round-trip, + * and unlike setTasks/setSessions this never re-adds anything). */ + removeSession(s, a: { payload: string }) { + s.sessions = s.sessions.filter((x) => x.uuid !== a.payload) + s.tasks = s.tasks.filter((x) => x.uuid !== a.payload) + }, /** Insert or merge a session for the active-project fallback list. */ upsertSession(s, a: { payload: SessionItem }) { const i = s.sessions.findIndex((x) => x.uuid === a.payload.uuid) diff --git a/web/src/components/Sidebar.tsx b/web/src/components/Sidebar.tsx index 2d231fe..9eb8954 100644 --- a/web/src/components/Sidebar.tsx +++ b/web/src/components/Sidebar.tsx @@ -92,6 +92,11 @@ export function Sidebar() { const tasks = useAppSelector((s) => s.session.tasks) const projectTimes = useAppSelector((s) => s.session.projectTimes) const currentSessionId = useAppSelector((s) => s.session.currentSessionId) + // Latest currentSessionId readable from async handlers (deleteItem) after an + // await, when the render-scope value is stale: if the user navigated away + // while the DELETE was in flight, the delete must not yank the foreground. + const currentSessionRef = useRef(currentSessionId) + currentSessionRef.current = currentSessionId const activePath = useAppSelector((s) => s.session.projectPath) const activeView = useAppSelector((s) => s.ui.activeView) @@ -375,20 +380,40 @@ export function Sidebar() { } async function deleteItem(row: SessionRow) { + // Running tasks must not be deleted — backend also returns 409. Stop first. + if (row.running) return const wasActive = row.uuid === currentSessionId + if (wasActive) { + // Drop queued type-ahead for this session BEFORE the delete round-trip: + // a late agent_done (from a prior cancel/stop) can arrive before the + // DELETE response and would otherwise drain the queue back into the + // deleted session — resurrecting its file + index entry on disk. + dispatch(chatActions.dropSessionQueue(row.uuid)) + } try { await api.deleteSession(row.uuid) } catch { return } - dispatch(sessionActions.setSessions(sessions.filter((s) => s.uuid !== row.uuid))) - dispatch(sessionActions.setTasks(tasks.filter((t) => t.uuid !== row.uuid))) - dispatch(chatActions.dropSessionQueue(row.uuid)) - if (wasActive) { - dispatch(chatActions.clearChat()) - dispatch(sessionActions.setCurrentSession('')) - dispatch(chatActions.setRunning(false)) + // Filter inside the reducer: the render-scope sessions/tasks copies are + // stale after the await (a WS update or refresh may have landed since), + // and a whole-list setTasks would clobber it. + dispatch(sessionActions.removeSession(row.uuid)) + if (!wasActive) { + dispatch(chatActions.dropSessionQueue(row.uuid)) + return } + // The user may have navigated to another session while the DELETE was in + // flight (their click's loadSession wins) — only take over the foreground + // if we're still on the deleted session. + if (currentSessionRef.current !== row.uuid) return + // Land on the welcome page with a FRESH session. Merely clearing the UI + // would strand the backend on the deleted task's engine: the next + // message would run on that stale engine, whose events are stamped with + // the deleted task id and dropped by the WS filter (a conversation that + // appears dead). startNewChat provisions a consistent new engine/task — + // and reclaims the stale one (its recorder was reset on delete). + await dispatch(startNewChat()) } function openContext(e: React.MouseEvent, row: SessionRow) { @@ -457,6 +482,8 @@ export function Sidebar() { icon={TrashIcon} label={t('sidebar.actions.delete')} danger + disabled={row.running} + title={row.running ? t('sidebar.actions.deleteWhileRunning') : undefined} onClick={() => { void deleteItem(row); closeCtx() }} /> @@ -856,19 +883,34 @@ function CtxItem({ label, onClick, danger, + disabled, + title, }: { icon: React.ComponentType<{ className?: string }> label: string onClick: () => void danger?: boolean + disabled?: boolean + title?: string }) { return (