Repository navigation
fix(web): preserve manual session titles #2102
Changes from 3 commits
fd6035a
f4a7afe
50cf65f
0a3924d
91bf820
07f039d
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 |
|---|---|---|
|
|
@@ -106,18 +106,30 @@ def get_runner_ws(ws: WebSocket) -> KimiCLIRunner: | |
| return ws.app.state.runner | ||
|
|
||
|
|
||
| def get_editable_session( | ||
| session_id: UUID, | ||
| runner: KimiCLIRunner, | ||
| ) -> JointSession: | ||
| """Get a session and verify it's not busy.""" | ||
| def get_session_or_404(session_id: UUID) -> JointSession: | ||
| """Load a session by id, raising 404 if it doesn't exist.""" | ||
| session = load_session_by_id(session_id) | ||
| if session is None: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail="Session not found", | ||
| ) | ||
| # Check if session is busy | ||
| return session | ||
|
|
||
|
|
||
| def get_editable_session( | ||
| session_id: UUID, | ||
| runner: KimiCLIRunner, | ||
| ) -> JointSession: | ||
| """Get a session and verify it's not busy. | ||
|
|
||
| Use for operations that cannot run concurrently with a live worker | ||
| (delete, fork, upload). For state-only edits (rename, archive, | ||
| generate-title) the worker merges externally-mutable fields back | ||
| via ``Session.save_state``; those endpoints can use | ||
| :func:`get_session_or_404` instead. | ||
| """ | ||
| session = get_session_or_404(session_id) | ||
| session_process = runner.get_session(session_id) | ||
| if session_process and session_process.is_busy: | ||
| raise HTTPException( | ||
|
|
@@ -586,12 +598,16 @@ async def delete_session(session_id: UUID, runner: KimiCLIRunner = Depends(get_r | |
| async def update_session( | ||
| session_id: UUID, | ||
| request: UpdateSessionRequest, | ||
| runner: KimiCLIRunner = Depends(get_runner), | ||
| ) -> Session: | ||
| """Update a session (e.g., rename title or archive/unarchive).""" | ||
| """Update a session (e.g., rename title or archive/unarchive). | ||
|
|
||
| Safe to invoke while a worker is running: only externally-mutable | ||
| fields are touched, and ``Session.save_state`` reloads them from | ||
| disk before each worker write. | ||
| """ | ||
| from kimi_cli.session_state import load_session_state, save_session_state | ||
|
|
||
| session = get_editable_session(session_id, runner) | ||
| session = get_session_or_404(session_id) | ||
|
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.
Allowing Useful? React with 👍 / 👎. |
||
| session_dir = session.kimi_cli_session.dir | ||
| state = load_session_state(session_dir) | ||
|
Comment on lines
592
to
596
|
||
|
|
||
|
|
@@ -749,14 +765,16 @@ async def fork_session_endpoint( | |
| async def generate_session_title( | ||
| session_id: UUID, | ||
| request: GenerateTitleRequest | None = None, | ||
| runner: KimiCLIRunner = Depends(get_runner), | ||
| ) -> GenerateTitleResponse: | ||
| """Generate a concise session title using AI based on the first conversation turn. | ||
|
|
||
| If request body is empty or parameters are missing, the backend will | ||
| automatically read the first turn from wire.jsonl. | ||
|
|
||
| Safe to invoke while a worker is running: the final write reloads | ||
| state from disk to merge concurrent worker changes. | ||
| """ | ||
| session = get_editable_session(session_id, runner) | ||
| session = get_session_or_404(session_id) | ||
| session_dir = session.kimi_cli_session.dir | ||
|
Comment on lines
748
to
760
|
||
|
|
||
| from kimi_cli.session_state import load_session_state, save_session_state | ||
|
|
@@ -789,6 +807,11 @@ async def generate_session_title( | |
| # If AI generation failed too many times, use fallback and mark as generated | ||
| if state.title_generate_attempts >= 3: | ||
| fresh = load_session_state(session_dir) | ||
| # Respect a title finalized by another request/user action while we | ||
| # were preparing a fallback. | ||
| if fresh.title_generated: | ||
| invalidate_sessions_cache() | ||
| return GenerateTitleResponse(title=fresh.custom_title or "Untitled") | ||
| fresh.custom_title = fallback_title | ||
| fresh.title_generated = True | ||
| save_session_state(fresh, session_dir) | ||
|
|
@@ -856,6 +879,11 @@ async def generate_session_title( | |
| # Read-modify-write: reload fresh state to avoid overwriting | ||
| # worker changes made during the LLM call | ||
| fresh = load_session_state(session_dir) | ||
| # Another request or manual rename may have finalized the title while the | ||
| # LLM call was in flight. Preserve that newer title instead of clobbering it. | ||
|
Comment on lines
863
to
+865
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.
This endpoint is now callable while a worker is busy, but it still does an unlocked read-modify-write ( Useful? React with 👍 / 👎. |
||
| if fresh.title_generated: | ||
| invalidate_sessions_cache() | ||
| return GenerateTitleResponse(title=fresh.custom_title or "Untitled") | ||
| fresh.custom_title = title | ||
| if ai_generated: | ||
| fresh.title_generated = True | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from pathlib import Path | ||
| from types import SimpleNamespace | ||
| from uuid import UUID | ||
|
|
||
| import pytest | ||
| from kaos.path import KaosPath | ||
|
|
||
| from kimi_cli.session import Session | ||
| from kimi_cli.session_state import load_session_state, save_session_state | ||
| from kimi_cli.web.api import sessions as sessions_api | ||
| from kimi_cli.web.models import GenerateTitleRequest | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def isolated_share_dir(monkeypatch, tmp_path: Path) -> Path: | ||
| share_dir = tmp_path / "share" | ||
| share_dir.mkdir() | ||
|
|
||
| def _get_share_dir() -> Path: | ||
| share_dir.mkdir(parents=True, exist_ok=True) | ||
| return share_dir | ||
|
|
||
| monkeypatch.setattr("kimi_cli.share.get_share_dir", _get_share_dir) | ||
| monkeypatch.setattr("kimi_cli.metadata.get_share_dir", _get_share_dir) | ||
| return share_dir | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def work_dir(tmp_path: Path) -> KaosPath: | ||
| path = tmp_path / "work" | ||
| path.mkdir() | ||
| return KaosPath.unsafe_from_local_path(path) | ||
|
|
||
|
|
||
| class _FakeOAuthManager: | ||
| def __init__(self, _config: object) -> None: | ||
| pass | ||
|
|
||
| async def ensure_fresh(self) -> None: | ||
| return None | ||
|
|
||
|
|
||
| class _FakeLLM: | ||
| chat_provider = object() | ||
|
|
||
|
|
||
| class _FakeMessage: | ||
| def __init__(self, text: str) -> None: | ||
| self._text = text | ||
|
|
||
| def extract_text(self) -> str: | ||
| return self._text | ||
|
|
||
|
|
||
| class _FakeResult: | ||
| def __init__(self, text: str) -> None: | ||
| self.message = _FakeMessage(text) | ||
|
|
||
|
|
||
| @pytest.mark.anyio | ||
| async def test_generate_title_preserves_concurrent_manual_title( | ||
| isolated_share_dir: Path, | ||
| work_dir: KaosPath, | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| session = await Session.create(work_dir) | ||
|
|
||
| config = SimpleNamespace( | ||
| default_model="test-model", | ||
| models={"test-model": SimpleNamespace(provider="test-provider")}, | ||
| providers={"test-provider": object()}, | ||
| ) | ||
|
|
||
| monkeypatch.setattr("kimi_cli.config.load_config", lambda: config) | ||
| monkeypatch.setattr( | ||
| "kimi_cli.llm.create_llm", | ||
| lambda provider_config, model_config, oauth=None: _FakeLLM(), | ||
| ) | ||
| monkeypatch.setattr("kimi_cli.auth.oauth.OAuthManager", _FakeOAuthManager) | ||
|
|
||
| async def fake_generate(*, chat_provider, system_prompt, tools, history): | ||
| state = load_session_state(session.dir) | ||
| state.custom_title = "Manual Title" | ||
| state.title_generated = True | ||
| save_session_state(state, session.dir) | ||
| return _FakeResult("AI Title") | ||
|
|
||
| monkeypatch.setattr("kosong.generate", fake_generate) | ||
|
|
||
| response = await sessions_api.generate_session_title( | ||
| UUID(session.id), | ||
| GenerateTitleRequest( | ||
| user_message="debug the flaky web session rename issue", | ||
| assistant_response="I'll inspect the session state writes.", | ||
| ), | ||
| ) | ||
|
|
||
| state = load_session_state(session.dir) | ||
| assert response.title == "Manual Title" | ||
| assert state.custom_title == "Manual Title" | ||
| assert state.title_generated is True | ||
| assert state.title_generate_attempts == 0 |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,4 +1,5 @@ | ||||
| import { useState, useCallback, useEffect, useRef } from "react"; | ||||
| import { toast } from "sonner"; | ||||
| import type { | ||||
| Session, | ||||
| UploadSessionFileResponse, | ||||
|
|
@@ -512,6 +513,7 @@ export function useSessions(): UseSessionsReturn { | |||
| const message = | ||||
| err instanceof Error ? err.message : "Failed to delete session"; | ||||
| setError(message); | ||||
| toast.error(message); | ||||
|
||||
| toast.error(message); |
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.
Switching this endpoint to
get_session_or_404allows renames/archive toggles while the worker is active, but the handler still does a full read-modify-write ofstate.json(load_session_statethensave_session_state) without any lock or CAS. If the running worker writes session state in between (e.g., todo/plan/approval updates viaSession.save_state), this PATCH can write an older snapshot back and silently roll back those non-title fields. This race was prevented before by the busy check, so allowing busy edits here introduces state-loss scenarios.Useful? React with 👍 / 👎.