Repository navigation
fix(agent): guard refreshSession's query swap like /clear's - #88769
Merged
Merged
Conversation
refreshSession performed the same in-place SDK query swap as /clear (retire the query, rewrite session.query/input/queryOptions/ abortController, await init) but claimed none of the Session.clearing protection, so a prompt, cancel, /clear, or second refresh arriving mid-swap could race the field rewrite. Generalize Session.clearing to Session.querySwap, claimed via a withQuerySwap helper (synchronous claim before the first await, released in finally), and route both swap bodies through it. Move retireQuery and the replacement-query construction inside performRefresh's try block, with the declared-then-assigned newQuery/newAbortController pattern and a null-guard in the catch, so a failure anywhere in the swap terminates and closes the session identically to /clear. Remove the one-line performClear wrapper so both swap paths read as two hops. Generated-By: PostHog Desktop Task-Id: 91ea51d0-4095-4614-acb4-b644deec3612
|
😎 Merged successfully - details. |
Contributor
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes. |
Contributor
|
React Doctor found 1 issue in 1 file · 1 warning. 1 warning
Reviewed by React Doctor for commit |
Contributor
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 0 should fix, 1 consider. Published 1 finding (view the review). Resolved comments: 1 fixed |
HostHog preview —
|
Contributor
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
The /clear refusal reused "A conversation clear is already in progress" even though querySwap now covers both a refresh and a clear, so a /clear sent during a refresh told the user a clear was running when none was. Report the accurate state and point to the next action, matching the refresh path's symmetric refusal. Generated-By: PostHog Desktop Task-Id: 09152925-2a1a-4032-94ba-9d8ab452e5fe
Contributor
Prompt To Fix All With AI### Issue 1
products/desktop/packages/agent/src/adapters/claude/claude-agent.ts:578-586
**Prompt registration misses refresh lock**
If a refresh arrives while a prompt is awaiting slash-command refresh or the pre-prompt local-tools check, the prompt has passed this gate but has not entered `turnQueue`, so the refresh is accepted and can retire the input before the prompt dispatches. The message is then pushed into the retiring input and the queued prompt never receives a response.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(agent): correct /clear refusal wordi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a concurrency hazard in the PostHog Desktop Claude ACP adapter where refresh_session could swap the underlying SDK query without taking the same in-place swap lock used by /clear, risking races with prompts/cancel/second swaps.
Changes:
- Rename
Session.clearingtoSession.querySwapand treat both/clearandrefresh_sessionas the same “query swap” critical section. - Add a shared
withQuerySwaphelper and route both swap paths through it; refuse cancel and other swaps while a swap is pending. - Harden refresh failure handling so any swap failure (including init rejection) terminates the replacement query and marks the session as closed; expand/adjust refresh-focused tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| products/desktop/packages/agent/src/adapters/claude/types.ts | Renames the session swap lock field and updates its documentation to cover both /clear and refresh swaps. |
| products/desktop/packages/agent/src/adapters/claude/claude-agent.ts | Introduces withQuerySwap, applies it to /clear and refresh, and strengthens error handling to close out failed swaps. |
| products/desktop/packages/agent/src/adapters/claude/claude-agent.refresh.test.ts | Adds concurrency and failure-mode tests for refresh (prompt waiting, cancel ignored, second refresh refused, /clear refused, init rejection closes session). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
adamleithp
approved these changes
Aug 25, 2026
The prompt() gate on session.querySwap runs before the pre-enqueue awaits (slash-command reload, pre-prompt local-tools heal), and the swap entry points refuse only while turnQueue has an entry. So a refresh or /clear started during those awaits sees an empty queue, proceeds, and retires the input stream under the waiting prompt; on the other side of that await the turn pushes into the queue unpushed (stranded) or lands on the fresh stream after a /clear. Re-check querySwap and queryClosed right before turnQueue.push, and reject before enqueue instead. Mirrors the existing mid-swap prompt test; adds a regression covering a swap that starts while the prompt is parked on the local-tools status RPC. Generated-By: PostHog Desktop Task-Id: 91ea51d0-4095-4614-acb4-b644deec3612
Generated-By: PostHog Desktop Task-Id: 91ea51d0-4095-4614-acb4-b644deec3612
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A session refresh swaps the SDK query in place, the same way
/cleardoes, but never claimed the lock/clearuses to protect that swap. A prompt, cancel,/clear, or second refresh arriving mid-swap could race the field rewrite and lose a prompt or corrupt the session.Changes
/clear's swap lock. A prompt arriving mid-swap waits and lands on the fresh input stream, a cancel becomes a no-op, and a second refresh or a/cleargets refused.Session.clearingis renamed toSession.querySwapand both swap paths now go through a sharedwithQuerySwaphelper.How did you test this code?
New tests cover: a prompt held during a refresh and delivered to the fresh stream, a cancel ignored during a refresh, a second refresh refused, a
/clearrefused during a refresh, and the session closing when the replacement query's init rejects.Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code implemented the fix and tests in this session, then ran
/squashto collapse the work into one commit before opening this PR with/create-pr.Created with PostHog Desktop