Skip to content

fix(sessions): handle turn_complete event ordering for local sessions#2197

Merged
k11kirky merged 1 commit into
mainfrom
posthog-code/reapply-this-fix
May 18, 2026
Merged

fix(sessions): handle turn_complete event ordering for local sessions#2197
k11kirky merged 1 commit into
mainfrom
posthog-code/reapply-this-fix

Conversation

@k11kirky
Copy link
Copy Markdown
Contributor

@k11kirky k11kirky commented May 18, 2026

TL;DR

This PR reapplies a critical fix for local session event ordering where turn_complete events arrive before JSON-RPC responses, causing race conditions. The fix ensures cloud and local sessions handle turn completion differently based on their canonical signal sources.

closes #2182 to fix codex message queuing

Problem

Local sessions with Codex agents experience a race condition where the turn_complete event arrives before the corresponding JSON-RPC response. The previous implementation cleared currentPromptId immediately on turn_complete, which interfered with the ID-match guard that relies on this value being present when the JSON-RPC response arrives. This caused prompts to hang or fail to complete properly.

Cloud sessions use turn_complete as their canonical turn-done signal, while local sessions use the JSON-RPC response. These require different handling to prevent race conditions.

Changes

  • Type safety improvement: Updated dequeueMessagesAsText mock return type annotation from implicit to explicit (): string | null => null for clarity
  • Event handling logic: Renamed isCloudTurnCompleteEvent() to isTurnCompleteEvent() to better reflect its universal applicability
  • Conditional turn completion: Modified turn_complete event handling to only clear session state (isPromptPending, promptStartedAt, currentPromptId) for cloud sessions, preventing the race condition in local sessions
  • Test coverage: Added comprehensive test case "drains queued messages when turn_complete arrives before the JSON-RPC response (local Codex regression)" to verify proper ordering and message queue drainage for local sessions

How did you test this?

The changes include a new integration test that reproduces the race condition scenario:

  1. Creates a local session with a queued message
  2. Sends a turn_complete event before the JSON-RPC response
  3. Verifies that currentPromptId is preserved until the JSON-RPC response arrives
  4. Confirms the subsequent prompt call is made with the correct session ID

Created with PostHog Code

Generated-By: PostHog Code
Task-Id: abebbeb8-aae5-4d41-94be-f2e7e878e3a2
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 18, 2026

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
apps/code/src/renderer/features/sessions/service/service.test.ts:3099
The test only asserts `currentPromptId` is preserved after `turn_complete` for local sessions, but the fix also skips clearing `isPromptPending` and `promptStartedAt`. If a future change incorrectly clears those two fields on `turn_complete` for local sessions, this test would not catch it. Adding assertions for all three fields would make the regression spec fully self-describing.

```suggestion
      expect(session?.currentPromptId).toBe(42);
      expect(session?.isPromptPending).toBe(true);
      expect(session?.promptStartedAt).not.toBeNull();
```

Reviews (1): Last reviewed commit: "fix(sessions): handle turn_complete even..." | Re-trigger Greptile

},
});

expect(session?.currentPromptId).toBe(42);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The test only asserts currentPromptId is preserved after turn_complete for local sessions, but the fix also skips clearing isPromptPending and promptStartedAt. If a future change incorrectly clears those two fields on turn_complete for local sessions, this test would not catch it. Adding assertions for all three fields would make the regression spec fully self-describing.

Suggested change
expect(session?.currentPromptId).toBe(42);
expect(session?.currentPromptId).toBe(42);
expect(session?.isPromptPending).toBe(true);
expect(session?.promptStartedAt).not.toBeNull();
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/sessions/service/service.test.ts
Line: 3099

Comment:
The test only asserts `currentPromptId` is preserved after `turn_complete` for local sessions, but the fix also skips clearing `isPromptPending` and `promptStartedAt`. If a future change incorrectly clears those two fields on `turn_complete` for local sessions, this test would not catch it. Adding assertions for all three fields would make the regression spec fully self-describing.

```suggestion
      expect(session?.currentPromptId).toBe(42);
      expect(session?.isPromptPending).toBe(true);
      expect(session?.promptStartedAt).not.toBeNull();
```

How can I resolve this? If you propose a fix, please make it concise.

@k11kirky k11kirky enabled auto-merge (squash) May 18, 2026 17:10
@k11kirky k11kirky merged commit 82e07fe into main May 18, 2026
15 checks passed
@k11kirky k11kirky deleted the posthog-code/reapply-this-fix branch May 18, 2026 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Codex queued messages not working

2 participants