Fix gateway thread retention and stale in-progress state

[henrypark133]

Summary

This fixes two correctness regressions introduced by the durable web-gateway in-progress state work:

1. The chat sidebar could silently switch the user away from the thread they were actively viewing if that thread dropped out of the 50-thread /api/chat/threads summary window.
2. /api/chat/history could keep returning stale persisted in_progress metadata even after the latest turn had already been fully reconstructed from DB messages, causing completed turns to re-render as if they were still processing.

Problem

Active thread could be lost during sidebar refresh

The frontend was clearing currentThreadId whenever the active thread was not present in the latest /api/chat/threads response. That response only includes the most recent 50 threads, so a still-valid open thread could disappear from the sidebar summary while the user was actively viewing it.

In that case the next render fell back to the server-reported active thread or assistant thread, which meant follow-up messages could be sent to the wrong conversation.

Completed turns could remain stuck in "Processing"

The history handler now rehydrates durable in-progress state from conversation metadata. However, the DB-backed history path returned that metadata directly even when build_turns_from_db_messages(...) had already reconstructed a completed final turn.

That could happen if the browser reloaded between the assistant response write and the metadata-clear write, or if the metadata-clear update failed once. The UI would then:

• re-add the user message for the already-completed turn
• show a bogus "Processing..." indicator
• leave the thread appearing stuck indefinitely

What changed

Frontend thread retention

In crates/ironclaw_gateway/static/app.js:

• removed the logic that nulled out currentThreadId simply because the current thread was absent from the sidebar summary response
• preserved the currently open thread even when it falls outside the recency window used by /api/chat/threads
• kept the existing first-load fallback behavior for restoring the server active thread when no thread is currently selected

This keeps history loading and follow-up sends attached to the thread the user is actually viewing.

Server-side in-progress reconciliation

In src/channels/web/server.rs:

• replaced the previous apply_in_progress_to_turns(...) helper with reconcile_in_progress_with_turns(...)
• reconciled persisted InProgressInfo against the reconstructed last turn before returning HistoryResponse
• dropped in_progress entirely when the persisted turn number matches a last turn that already has a response
• preserved in_progress when it refers to a newer turn that has not yet been persisted into history
• also dropped stale metadata when the persisted turn number is older than the already-reconstructed history

This ensures durable live-state only survives when it still represents a genuinely incomplete turn.

Tests

Added/updated regression coverage for both paths:

Rust unit/integration coverage

In src/channels/web/server.rs tests:

• verifies stale in_progress is dropped when the matching last turn is already completed
• verifies in_progress is preserved for a newer, not-yet-persisted turn
• verifies /api/chat/history does not return stale in_progress for a completed persisted turn

Browser regression coverage

In tests/e2e/scenarios/test_message_persistence.py:

• added a regression that keeps one thread open in the page
• creates enough newer threads to push that thread out of the 50-thread summary window
• refreshes the sidebar
• verifies the active page thread does not change
• verifies the next sent message stays on the originally open thread and does not land on the newer active thread

Verification

Ran:

• cargo fmt
• cargo clippy --all-targets --all-features -- -D warnings
• cargo test --lib test_chat_history_handler_drops_stale_in_progress_for_completed_turn
• cargo test --lib test_reconcile_in_progress_with_turns

Not run here:

• the new Playwright regression, because this workspace does not currently have the local Python pytest / Playwright test dependencies installed

Risk

Low and targeted.

The frontend change only affects sidebar refresh behavior when a thread is already selected. The server change only affects how durable in_progress metadata is reconciled with already-persisted history before the response is returned.

[gemini-code-assist]

Code Review

This pull request introduces durable in-flight turn state, allowing the UI to rehydrate and display "Processing..." indicators for active turns after page refreshes or thread switches. This is achieved by persisting a "live_state" in the conversation metadata and reconciling it with the message history. Feedback includes concerns about potential database bloat from large metadata fields and the efficiency of collecting thread states during sidebar refreshes.

[Copilot]

Pull request overview

This PR addresses two regressions in the durable web-gateway “in-progress” work: (1) the frontend losing the currently viewed thread when it falls outside the 50-thread sidebar window, and (2) /api/chat/history returning stale durable in_progress state after the last turn is already complete.

Changes:

• Frontend: preserve currentThreadId across sidebar refreshes and rehydrate UI “Processing…” state via HistoryResponse.in_progress.
• Server: persist/clear durable live_state metadata at turn boundaries and reconcile it against reconstructed DB turns before returning history.
• Tests/docs: add Rust + e2e regressions and document the in_progress history contract.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
crates/ironclaw_gateway/static/app.js	Uses HistoryResponse.in_progress to rehydrate processing UI; preserves active thread during thread-list refresh; adds thread spinners based on server state.
src/channels/web/server.rs	Adds InProgressInfo, reads durable metadata, reconciles it with turns, and includes it in /api/chat/history; threads list now merges in-memory state with DB metadata state.
src/channels/web/types.rs	Adds HistoryResponse.in_progress and defines InProgressInfo.
src/agent/thread_ops.rs	Persists live_state at turn start and clears it on completion/interruption/approval/failure paths.
src/history/store.rs	Extends ConversationSummary with live_state extracted from metadata (Postgres store).
src/db/libsql/conversations.rs	Extends conversation summary mapping with live_state extracted from metadata (libsql store).
tests/e2e/scenarios/test_message_persistence.py	Adds e2e regressions for refresh/switch during in-progress and for sidebar refresh preserving the active thread outside the summary window.
src/channels/web/CLAUDE.md	Documents the new HistoryResponse.in_progress contract for UI rehydration.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[zmanian]

Code Review

Size: XL (+784 / -21) | Risk: Low-Medium

Summary

Fixes two correctness regressions in the durable web-gateway in-progress state:

1. Sidebar thread retention -- the frontend was clearing currentThreadId when the active thread fell outside the 50-thread /api/chat/threads window, causing follow-up messages to land on the wrong thread.
2. Stale in-progress reconciliation -- /api/chat/history could return stale persisted in_progress metadata even after the turn was already completed in the DB, causing the UI to show a ghost "Processing..." indicator.

Strengths

• reconcile_in_progress_with_turns is the right abstraction. The logic is clear: if the in-progress turn matches the last DB turn and that turn has a response, the in-progress state is stale and gets dropped. If the in-progress turn number is newer than the last DB turn, it's preserved. Clean state machine.

• user_message_id as correlation key is a good choice. Turn numbers can collide across reconnects/compaction, but a DB-generated UUID is stable. The fallback to turn number for non-persistent modes is sensible.

• Frontend fix is minimal and correct. Removing the currentThreadId = null fallback when the thread isn't in the sidebar summary is the right call. The first-load fallback to server_active_thread is preserved.

• Comprehensive test coverage. Unit tests for reconcile_in_progress_with_turns (both stale and valid cases), integration test with libsql for the full history handler path, E2E Playwright tests for refresh/switch-back/sidebar-overflow scenarios.

Issues

1. live_state metadata is not cleared on AuthPending turns (thread_ops.rs)

The clear_conversation_live_state call is added for Failed, Interrupted, AwaitingApproval, and Completed outcomes. But the AuthPending branch (around line 817 in the original) doesn't clear live state. If an auth-pending turn stays in the DB metadata, a reconnecting client could see stale "Processing..." for an auth-gated turn. This may be acceptable if auth-pending turns don't persist live_state in the first place (they skip persist_user_message in some paths), but worth verifying.

2. user_input truncation at 32KB (thread_ops.rs)

"user_input": truncate_preview(user_input, 32 * 1024),

This is stored in conversation metadata JSON. For typical messages this is fine, but if metadata is stored in a TEXT column, 32KB of user input embedded in the metadata JSON could bloat the row. If this is the same metadata field used for thread_type, title, etc., confirm that downstream queries don't have row-size issues. Might be worth a smaller truncation (e.g., 4KB) since this is only for UI preview on reconnect.

3. live_state persisted even on DB write failure (thread_ops.rs)

In persist_user_message, if add_conversation_message fails, the code still persists live_state to metadata:

Err(e) => {
    tracing::warn!("Failed to persist user message: {}", e);
    let live_state = serde_json::json!({ ... "user_message_id": null ... });
    self.persist_conversation_live_state(&store, thread_id, &live_state).await;
    None
}

If the DB is having issues, the metadata write will likely also fail, which is handled (logged). But the user_message_id: null path means the reconciliation fallback uses turn numbers, which is less reliable. This is fine as a best-effort, but the comment could clarify that this is intentional degradation.

4. Thread state in sidebar uses format!("{:?}", thread.state) (server.rs)

.map(|(id, thread)| (*id, format!("{:?}", thread.state)))

This uses Rust's Debug format for ThreadState, which produces strings like "Processing", "Idle", "AwaitingApproval". The frontend checks thread.state === 'Processing'. This coupling between Rust's Debug output and JS string literals is fragile -- if ThreadState variants are renamed or the Debug impl changes, the sidebar spinners break silently. Consider using a display_name() method or serde serialization instead.

5. E2E test AssertionError typo (test_message_persistence.py)

raise AssertionError(f"Timed out waiting for in-progress turn in thread {thread_id}")

Should be AssertionError -> this is actually builtins.AssertionError... wait, Python doesn't have AssertionError. This should be AssertionError is not a real Python exception. It should be AssertionError -- actually checking: Python has AssertionError which is NOT a builtin. The correct name is AssertionError... no. The correct Python exception is AssertionError. Let me check: Python has AssertionError which is correct only if it's actually AssertionError. The standard Python assertion error is AssertionError. Actually -- the standard is AssertionError. Hmm, let me re-read: the code says AssertionError. The correct Python builtin is AssertionError.

Wait -- I'm going in circles. The Python builtin is AssertionError. The code says AssertionError. These are the same. I was wrong to flag this. Ignore this point.

Minor

• The isSameInProgressTurn function in app.js mirrors in_progress_matches_turn in server.rs. Good that both sides agree on the matching logic.
• live_state in ConversationSummary is extracted as metadata.live_state.state (nested), which is correct since the full live_state object has turn_number, user_input, etc.

Overall this is a clean, well-targeted fix. The main concern is item 4 (Debug format coupling) which could cause silent breakage down the road.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[henrypark133]

Follow-up on @zmanian’s review:

Addressed in follow-ups:

• replaced the sidebar thread-state format!("{:?}", ...) coupling with explicit state-label helpers in server.rs
• added live_state staleness expiry for abandoned/crashed turns
• re-persisted live_state when approval resumes processing
• moved the completion-path DB awaits out from under the session mutex

Reviewed but intentionally left out of scope for this patch set:

• the 32KB user_input preview cap in live_state metadata
• the best-effort user_message_id: null fallback when the initial user-message DB write fails

I re-checked both of those and still do not consider them correctness blockers for this PR:

• live_state is a single overwritten metadata field per conversation, not an accumulating per-turn payload
• the truncation already uses the shared UTF-8-safe truncate_preview(...) helper
• the null-ID path is a degraded fallback only when the initial persistence write fails, and later reconciliation still prefers the stable persisted ID path when available

The AssertionError note in that review was self-retracted and was not an issue.

[henrypark133]

Also addressed the newer active_thread race note in chat_threads_handler(...) in 86b72a0b.

The handler no longer returns the earlier active_thread snapshot after the DB query / fallback re-lock. In the DB-backed path it now re-reads session.active_thread just before building the response, and in the in-memory fallback it returns sess.active_thread from the same lock scope used to build that response.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ilblackdragon]

Code Review: PR #2517 — Fix gateway thread retention and stale in-progress state

Overview

Fixes two regressions introduced by the durable web-gateway in-progress state work:

1. Sidebar refresh losing active thread — frontend was clearing currentThreadId whenever the current thread dropped out of the 50-thread /api/chat/threads summary window.
2. Completed turns stuck "Processing" — /api/chat/history returned stale persisted in_progress metadata even after DB reconstruction showed the turn was complete.

Scope: +1207 / -25 across 14 files. The server-side fix is reconcile_in_progress_with_turns(...); the client-side fix is preserving currentThreadId across sidebar refreshes plus defensive turn matching in loadHistory(). Durable live state is persisted via live_state in conversation metadata and cleared on completion/interrupt/reset/approval/error paths.

Strengths

• Lock hygiene. All clear_conversation_live_state calls correctly drop(sess) before .await, avoiding lock-across-await bugs.
• Stable state serialization. Good move replacing format!("{:?}", t.state) with turn_state_label/thread_state_label — decouples the wire contract from enum Debug output.
• Staleness guard. IN_PROGRESS_STALE_AFTER_MINUTES = 10 catches crash-without-clear cases; unparseable timestamps conservatively return true (drop).
• Dual-backend coverage. Both libsql (src/db/libsql/conversations.rs) and postgres (src/history/store.rs) paths extract live_state / live_state_started_at — satisfies the CLAUDE.md invariant.
• Strong tests. 6 Rust unit/integration tests exercise each branch of reconcile_in_progress_with_turns; 3 Playwright scenarios cover refresh, thread-switch, and the sidebar window regression. #[cfg(feature = "libsql")] handler tests drive the actual HTTP path (per CLAUDE.md: "Test Through the Caller").

Issues & Suggestions

Scope creep — unrelated hunks

Four unrelated match-exhaustiveness fixes are in this PR but not mentioned in the description:

• src/cli/tool.rs:1107 — KeyCode::Backspace => {}
• src/setup/prompts.rs:173,257 — KeyCode::Down => {}, KeyCode::Backspace => {}
• src/llm/rig_adapter.rs:847 — AssistantContent::Text(_) => {}
• src/channels/web/responses_api.rs:1230 — \"assistant\" => {}

These look like clippy fallout from a separate rebase. They're harmless but muddy review and git blame. Consider splitting into a trivial follow-up.

Duplicated reconciliation logic (JS ↔ Rust)

isSameInProgressTurn in app.js is a near-mirror of in_progress_matches_turn in server.rs, but with subtly different rules:

• Rust in_progress_matches_turn returns true for the both-null-ID case as long as turn_number matches, regardless of response.
• JS isSameInProgressTurn additionally requires !lastTurn.response in the both-null case.

The server already filters stale state via reconcile_in_progress_with_turns, so the JS check is a redundant belt. The divergence is probably harmless today but risks drift. Either (a) trust the server and simplify the JS to lastTurn?.user_message_id === inProgress.user_message_id, or (b) mirror the Rust logic exactly and cite the server as authoritative in a comment.

Reconciler edge case

```rust
} else if completed_turn_is_newer_than_in_progress(last_turn, &in_progress)
|| last_turn.turn_number >= in_progress.turn_number
{
None
```

The last_turn.turn_number >= in_progress.turn_number branch fires even when the IDs don't match and the last turn is NOT completed. In practice build_turns_from_db_messages always emits completed turns (assistant message present) so this is safe — but it's a latent coupling. Worth a comment explaining why >= is correct (DB-reconstructed turns are by definition complete).

Duplication in summary extraction

live_state / live_state_started_at extraction is copy-pasted 4 times across libsql/conversations.rs and history/store.rs. Consider a small helper (extract_live_state(&metadata) -> (Option<String>, Option<String>)) — would also centralize the "state" vs "started_at" key names.

E2E flakiness risk

test_sidebar_refresh_keeps_active_thread_outside_summary_window creates 55 threads sequentially via /api/chat/thread/new. That's ~55 round-trips per run and likely the slowest new test. Also, _start_thread_and_wait_for_in_progress retries up to 3 times for a "fast completion" race — fine, but log the attempt count so flake triage is easier.

Minor

• src/channels/web/server.rs:3400 — the sidebar sess lock is taken, mapped, dropped, then re-taken at 3462 to read active_thread. Could be captured in the first pass.
• summary_live_state synthesizes a fake InProgressInfo just to reuse is_stale_in_progress. Extracting the staleness check to work on &str (started_at) would avoid the awkward placeholder struct.
• ProcessingLiveState.user_input: &'a str is truncated at 32KiB inside persist_processing_live_state via truncate_preview — good, but worth a comment on why 32KiB (appears to match the message preview convention elsewhere).

Security & Correctness

• ensure_writable_conversation is called on every live-state write/clear — ownership gate is preserved. ✓
• No new .unwrap() / .expect() in production paths. ✓
• truncate_preview on user_input caps the metadata column size; avoids unbounded growth. ✓
• CLAUDE.md invariant ("LLM data is never deleted") respected — live_state is cleared, not the conversation messages. ✓

Verdict

Approve with minor revisions. The core reconciliation and retention logic is sound, well-tested, and correctly addresses both regressions. The main asks are: (1) split the unrelated match-exhaustiveness hunks into their own PR, (2) reduce JS/Rust logic drift around in_progress_matches_turn, and (3) add a one-line comment on the turn_number >= branch. Nothing blocking.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Preserving currentThreadId when it falls outside the 50-thread summary window can break the read-only thread safety guard: later in loadThreads() the channel is derived from the thread object in the returned list, and if the active thread isn’t present it falls back to treating it as a gateway thread (enabling input). Please ensure the read-only/disabled-input logic remains correct when the active thread is outside the summary list (e.g., keep the last known channel/read-only flag, disable input until resolved, or have the server include the active thread entry even if it’s outside the recency window).