diff --git a/.changeset/thin-heads-train.md b/.changeset/thin-heads-train.md new file mode 100644 index 00000000000..f105e2e2d0c --- /dev/null +++ b/.changeset/thin-heads-train.md @@ -0,0 +1,6 @@ +--- +"@kilocode/cli": patch +"kilo-code": patch +--- + +fixes session cleanup race conditions diff --git a/cli/src/commands/__tests__/session.test.ts b/cli/src/commands/__tests__/session.test.ts index b007859fa8f..b577a1c818c 100644 --- a/cli/src/commands/__tests__/session.test.ts +++ b/cli/src/commands/__tests__/session.test.ts @@ -687,7 +687,7 @@ describe("sessionCommand", () => { await sessionCommand.handler(mockContext) expect(SessionManager.init).toHaveBeenCalled() - expect(mockSessionManager.renameSession).toHaveBeenCalledWith("My New Session Name") + expect(mockSessionManager.renameSession).toHaveBeenCalledWith("test-session-123", "My New Session Name") expect(mockContext.addMessage).toHaveBeenCalledTimes(1) const message = (mockContext.addMessage as ReturnType).mock.calls[0][0] expect(message.type).toBe("system") @@ -701,7 +701,7 @@ describe("sessionCommand", () => { await sessionCommand.handler(mockContext) - expect(mockSessionManager.renameSession).toHaveBeenCalledWith("SingleWord") + expect(mockSessionManager.renameSession).toHaveBeenCalledWith("test-session-123", "SingleWord") const message = (mockContext.addMessage as ReturnType).mock.calls[0][0] expect(message.type).toBe("system") expect(message.content).toContain("SingleWord") @@ -720,7 +720,7 @@ describe("sessionCommand", () => { }) it("should handle rename error when no active session", async () => { - mockSessionManager.renameSession = vi.fn().mockRejectedValue(new Error("No active session")) + mockSessionManager.sessionId = null mockContext.args = ["rename", "New", "Name"] await sessionCommand.handler(mockContext) @@ -750,10 +750,11 @@ describe("sessionCommand", () => { await sessionCommand.handler(mockContext) - expect(mockSessionManager.renameSession).toHaveBeenCalledWith("New Name") + expect(mockSessionManager.renameSession).toHaveBeenCalledWith("test-session-123", "New Name") }) it("should handle backend error gracefully", async () => { + mockSessionManager.sessionId = "test-session-123" mockSessionManager.renameSession = vi.fn().mockRejectedValue(new Error("Network error")) mockContext.args = ["rename", "New", "Name"] diff --git a/cli/src/commands/session.ts b/cli/src/commands/session.ts index a46b21932f4..fc9487027b7 100644 --- a/cli/src/commands/session.ts +++ b/cli/src/commands/session.ts @@ -313,7 +313,11 @@ async function renameSession(context: CommandContext, newName: string): Promise< } try { - await sessionService.renameSession(newName) + if (!sessionService.sessionId) { + throw new Error("No active session to rename") + } + + await sessionService.renameSession(sessionService.sessionId, newName) addMessage({ ...generateMessage(), diff --git a/src/shared/kilocode/cli-sessions/core/SessionManager.ts b/src/shared/kilocode/cli-sessions/core/SessionManager.ts index 0310a5cb6dc..f9cc37f3b3c 100644 --- a/src/shared/kilocode/cli-sessions/core/SessionManager.ts +++ b/src/shared/kilocode/cli-sessions/core/SessionManager.ts @@ -313,12 +313,11 @@ export class SessionManager { }) } - async renameSession(newTitle: string) { + async renameSession(sessionId: string, newTitle: string) { if (!this.sessionClient) { throw new Error("SessionManager used before initialization") } - const sessionId = this.sessionId if (!sessionId) { throw new Error("No active session") } @@ -406,6 +405,7 @@ export class SessionManager { this.logger?.debug("Destroying SessionManager", "SessionManager", { sessionId: this.sessionId, isSyncing: this.isSyncing, + currentTaskId: this.currentTaskId, }) if (this.timer) { @@ -423,7 +423,9 @@ export class SessionManager { this.paths = { ...defaultPaths } this.sessionId = null + this.currentTaskId = null this.sessionTitle = null + this.sessionGitUrl = null this.isSyncing = false this.logger?.debug("SessionManager flushed", "SessionManager") @@ -455,6 +457,8 @@ export class SessionManager { } this.isSyncing = true + // capture the sessionId at the start of the sync + let capturedSessionId = this.sessionId try { if (!this.platform || !this.sessionClient || !this.sessionPersistenceManager) { @@ -488,28 +492,29 @@ export class SessionManager { }) } - if (!this.sessionId && this.currentTaskId) { + if (!capturedSessionId && this.currentTaskId) { const existingSessionId = this.sessionPersistenceManager.getSessionForTask(this.currentTaskId) if (existingSessionId) { this.sessionId = existingSessionId + capturedSessionId = existingSessionId } } - if (this.sessionId) { + if (capturedSessionId) { const gitUrlChanged = gitInfo?.repoUrl && gitInfo.repoUrl !== this.sessionGitUrl if (gitUrlChanged) { - this.logger?.debug("Updating existing session", "SessionManager", { sessionId: this.sessionId }) - await this.sessionClient.update({ - session_id: this.sessionId, + session_id: capturedSessionId, ...basePayload, }) this.sessionGitUrl = gitInfo?.repoUrl || null - this.logger?.debug("Session updated successfully", "SessionManager", { sessionId: this.sessionId }) + this.logger?.debug("Session updated successfully", "SessionManager", { + sessionId: capturedSessionId, + }) } } else { this.logger?.debug("Creating new session", "SessionManager") @@ -528,21 +533,23 @@ export class SessionManager { }) this.sessionId = session.session_id + capturedSessionId = session.session_id + this.sessionGitUrl = gitInfo?.repoUrl || null - this.logger?.info("Session created successfully", "SessionManager", { sessionId: this.sessionId }) + this.logger?.info("Session created successfully", "SessionManager", { sessionId: capturedSessionId }) - this.sessionPersistenceManager.setLastSession(this.sessionId) + this.sessionPersistenceManager.setLastSession(capturedSessionId) this.onSessionCreated?.({ timestamp: Date.now(), event: "session_created", - sessionId: this.sessionId, + sessionId: capturedSessionId, }) } if (this.currentTaskId) { - this.sessionPersistenceManager.setSessionForTask(this.currentTaskId, this.sessionId) + this.sessionPersistenceManager.setSessionForTask(this.currentTaskId, capturedSessionId) } const blobUploads: Array> = [] @@ -550,7 +557,11 @@ export class SessionManager { if (rawPayload.apiConversationHistoryPath && this.hasBlobChanged("apiConversationHistory")) { blobUploads.push( this.sessionClient - .uploadBlob(this.sessionId, "api_conversation_history", rawPayload.apiConversationHistoryPath) + .uploadBlob( + capturedSessionId, + "api_conversation_history", + rawPayload.apiConversationHistoryPath, + ) .then(() => { this.markBlobSynced("apiConversationHistory") this.logger?.debug("Uploaded api_conversation_history blob", "SessionManager") @@ -566,7 +577,7 @@ export class SessionManager { if (rawPayload.taskMetadataPath && this.hasBlobChanged("taskMetadata")) { blobUploads.push( this.sessionClient - .uploadBlob(this.sessionId, "task_metadata", rawPayload.taskMetadataPath) + .uploadBlob(capturedSessionId, "task_metadata", rawPayload.taskMetadataPath) .then(() => { this.markBlobSynced("taskMetadata") this.logger?.debug("Uploaded task_metadata blob", "SessionManager") @@ -582,7 +593,7 @@ export class SessionManager { if (rawPayload.uiMessagesPath && this.hasBlobChanged("uiMessages")) { blobUploads.push( this.sessionClient - .uploadBlob(this.sessionId, "ui_messages", rawPayload.uiMessagesPath) + .uploadBlob(capturedSessionId, "ui_messages", rawPayload.uiMessagesPath) .then(() => { this.markBlobSynced("uiMessages") this.logger?.debug("Uploaded ui_messages blob", "SessionManager") @@ -610,7 +621,7 @@ export class SessionManager { if (this.hasBlobChanged("gitState")) { blobUploads.push( this.sessionClient - .uploadBlob(this.sessionId, "git_state", gitStateData) + .uploadBlob(capturedSessionId, "git_state", gitStateData) .then(() => { this.markBlobSynced("gitState") this.logger?.debug("Uploaded git_state blob", "SessionManager") @@ -630,8 +641,8 @@ export class SessionManager { if (!this.sessionTitle && rawPayload.uiMessagesPath) { this.generateTitle(rawPayload.uiMessagesPath as ClineMessage[]) .then((generatedTitle) => { - if (generatedTitle) { - return this.renameSession(generatedTitle) + if (capturedSessionId && generatedTitle) { + return this.renameSession(capturedSessionId, generatedTitle) } return null @@ -645,7 +656,7 @@ export class SessionManager { } catch (error) { this.logger?.error("Failed to sync session", "SessionManager", { error: error instanceof Error ? error.message : String(error), - sessionId: this.sessionId, + sessionId: capturedSessionId, hasApiHistory: !!this.paths.apiConversationHistoryPath, hasUiMessages: !!this.paths.uiMessagesPath, hasTaskMetadata: !!this.paths.taskMetadataPath, diff --git a/src/shared/kilocode/cli-sessions/core/__tests__/SessionManager.test.ts b/src/shared/kilocode/cli-sessions/core/__tests__/SessionManager.test.ts index d086ee87b2e..e556e47048b 100644 --- a/src/shared/kilocode/cli-sessions/core/__tests__/SessionManager.test.ts +++ b/src/shared/kilocode/cli-sessions/core/__tests__/SessionManager.test.ts @@ -434,26 +434,23 @@ describe("SessionManager", () => { describe("renameSession", () => { it("should throw error when no active session", async () => { - sessionManager.sessionId = null - - await expect(sessionManager.renameSession("New Title")).rejects.toThrow("No active session") + await expect(sessionManager.renameSession("", "New Title")).rejects.toThrow("No active session") }) it("should throw error when title is empty", async () => { - sessionManager.sessionId = "session-123" - - await expect(sessionManager.renameSession(" ")).rejects.toThrow("Session title cannot be empty") + await expect(sessionManager.renameSession("session-123", " ")).rejects.toThrow( + "Session title cannot be empty", + ) }) it("should rename the session", async () => { - sessionManager.sessionId = "session-123" vi.mocked(sessionManager.sessionClient!.update).mockResolvedValue({ session_id: "session-123", title: "New Title", updated_at: new Date().toISOString(), }) - await sessionManager.renameSession("New Title") + await sessionManager.renameSession("session-123", "New Title") expect(sessionManager.sessionClient!.update).toHaveBeenCalledWith({ session_id: "session-123", @@ -463,14 +460,13 @@ describe("SessionManager", () => { }) it("should trim the title", async () => { - sessionManager.sessionId = "session-123" vi.mocked(sessionManager.sessionClient!.update).mockResolvedValue({ session_id: "session-123", title: "Trimmed Title", updated_at: new Date().toISOString(), }) - await sessionManager.renameSession(" Trimmed Title ") + await sessionManager.renameSession("session-123", " Trimmed Title ") expect(sessionManager.sessionClient!.update).toHaveBeenCalledWith({ session_id: "session-123", @@ -636,6 +632,32 @@ describe("SessionManager", () => { expect(sessionManager["isSyncing"]).toBe(false) }) + + it("should clear currentTaskId to prevent session ID clobbering across tasks", async () => { + sessionManager.setPath("task-A", "apiConversationHistoryPath", "/path/to/taskA/api.json") + sessionManager.sessionId = "session-A" + vi.mocked(sessionManager.sessionPersistenceManager!.getSessionForTask).mockReturnValue("session-A") + vi.mocked(sessionManager.sessionClient!.create).mockResolvedValue({ + session_id: "session-B", + title: "Task B", + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + }) + + await sessionManager.destroy() + + expect(sessionManager["currentTaskId"]).toBeNull() + expect(sessionManager.sessionId).toBeNull() + + sessionManager.setPath("task-B", "apiConversationHistoryPath", "/path/to/taskB/api.json") + + vi.mocked(sessionManager.sessionPersistenceManager!.getSessionForTask).mockReturnValue(undefined) + + await sessionManager["syncSession"]() + + expect(sessionManager.sessionId).toBe("session-B") + expect(sessionManager.sessionId).not.toBe("session-A") + }) }) describe("getFirstMessageText", () => {