Tell a subscribing client whether the thread's turn is still running

[marmeladema]

A thread's first turn is started over HTTP, before the browser has a
socket for that thread, so the composer locks on nothing more than the
knowledge that a start request was sent. Every other lock is released by
something the socket delivers, but this one has no such release when the
turn finishes before the socket attaches: the TurnCompleted was broadcast
to a thread nobody was subscribed to, and a turn that is over leaves no
LiveTurnSnapshot behind. Against a fast harness that is the common case,
not the rare one, and it left the composer disabled — Stop showing, Send
gone — with the agent's reply already on screen, until the thread was
re-opened from the sidebar.

The subscribe snapshot now states turn liveness outright. ThreadState
carries active_turn, answered from the active-turn gate rather than the
live buffer: the gate is held from before the start request returns until
the turn ends, so it also covers the window where the harness has
accepted a turn but not yet streamed anything and the buffer is still
empty. A false releases the optimistic lock; anything else leaves it
alone, so a live turn still holds the composer.

Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01DEjHUhgZUTTwPtYKjA7YN2

Summary by CodeRabbit

• Bug Fixes

  • Improved composer locking and unlocking during initial message processing.
  • Prevented stale locked states when a response completes before a live connection is established.
  • Ensured thread subscriptions accurately reflect turns already completed or still in progress.

• Tests

  • Added coverage for reconnecting, completed initial turns, and sending follow-up messages successfully.

• Documentation

  • Clarified turn-status and subscription behavior in the product specification.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

Active turn synchronization

Layer / File(s)	Summary
Protocol and server active-turn snapshots crates/giskard-proto/src/lib.rs, crates/giskard-server/src/routes.rs	ThreadState now includes live active_turn status. Subscriptions register clients before creating snapshots, and broadcasts include the same status.
Client resynchronization behavior crates/giskard-server/static/app.js, specs/giskard-specification.md	The client uses active_turn to release the first-turn composer lock only after the server reports an idle thread.
Subscription race regression coverage crates/giskard-server/tests/e2e_smoke.rs, crates/giskard-server/tests/ui.rs, tests/e2e/tests/draft-composer.spec.ts	Tests cover completed turns, accepted but unstreamed turns, updated extraction markers, and composer recovery after late socket attachment.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant WebSocketSubscription
  participant TurnRegistry
  participant Composer
  Client->>WebSocketSubscription: subscribe to thread
  WebSocketSubscription->>TurnRegistry: register client and read active_turn
  TurnRegistry-->>WebSocketSubscription: live turn status
  WebSocketSubscription-->>Client: ThreadState with active_turn
  Client->>Composer: render state and release lock if idle

Loading

Possibly related PRs

• marmeladema/Giskard#119: Related WebSocket thread-subscription synchronization and turn-lifecycle test helpers.
• marmeladema/Giskard#126: Related WebSocket subscription and client state synchronization changes.

Suggested reviewers: claude

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: exposing whether a thread turn remains active to subscribing clients.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/fix-stuck-active-turn

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/giskard-server/src/routes.rs (1)

3391-3430: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Clean up failed WebSocket subscriptions.

If subscription bootstrap fails after state.hub.subscribe(...), call state.hub.unsubscribe(thread_id, client_id).await before returning the error. handle_ws sends the error and continues; it only removes subscriptions when the socket closes. Add focused failure-path coverage.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/giskard-server/src/routes.rs` around lines 3391 - 3430, Update the
WebSocket subscription bootstrap surrounding state.hub.subscribe and the
subsequent recompute_aggregates, serialization, and ThreadState send operations
so any failure after subscribing first awaits state.hub.unsubscribe(thread_id,
client_id), then propagates the original error. Preserve normal successful
subscriptions and add focused coverage verifying cleanup when bootstrap fails.

Sources: Coding guidelines, Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/giskard-server/src/routes.rs`:
- Around line 3391-3430: Update the WebSocket subscription bootstrap surrounding
state.hub.subscribe and the subsequent recompute_aggregates, serialization, and
ThreadState send operations so any failure after subscribing first awaits
state.hub.unsubscribe(thread_id, client_id), then propagates the original error.
Preserve normal successful subscriptions and add focused coverage verifying
cleanup when bootstrap fails.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 24256c94-eb49-42af-b13c-1544261dc6f1

📥 Commits

Reviewing files that changed from the base of the PR and between 2ea6df9 and d22e00b.

📒 Files selected for processing (7)

• crates/giskard-proto/src/lib.rs
• crates/giskard-server/src/routes.rs
• crates/giskard-server/static/app.js
• crates/giskard-server/tests/e2e_smoke.rs
• crates/giskard-server/tests/ui.rs
• specs/giskard-specification.md
• tests/e2e/tests/draft-composer.spec.ts

[marmeladema]

Skipping the "clean up failed WebSocket subscriptions" finding, with reasoning.

Nothing accumulates: the browser sends subscribe only from ws.onopen, once per socket, and never retries it on error, so a client can hold at most one entry — and hub.disconnect clears it when the socket closes. There is no unbounded growth to reclaim.

What is left is a choice of failure semantics, and unsubscribing makes the user-visible outcome worse. After a bootstrap failure handle_ws logs, sends the error, and keeps the socket open, and the browser has no path back to subscribe on a live socket (reconnectIfNeeded only fires once the socket is closed). Retaining the subscription means live events still reach the client and the next authoritative ThreadState broadcast runs resetTranscriptForAuthoritativeSnapshot, which recovers the view. Dropping it guarantees a permanently silent thread view until the user re-opens the thread.

It is also pre-existing rather than introduced here: hub.subscribe has always preceded the fallible recompute_aggregates, and this PR only adds a comment and one field to that block. The comment is there because the ordering is now load-bearing — active_turn is only safe to act on because a turn ending after the subscribe registration still broadcasts its TurnCompleted to this client.

The real improvement is probably to close the socket on a failed subscribe so the client reconnects and retries, which is a separate change with its own failure-path tests rather than something to fold into this one.

---

Generated by Claude Code