Skip to content

Treat missing thread snapshots as terminal - #4235

Open
Quicksaver wants to merge 76 commits into
pingdotgg:mainfrom
Quicksaver:fix/thread-not-found-subscription-loop
Open

Treat missing thread snapshots as terminal#4235
Quicksaver wants to merge 76 commits into
pingdotgg:mainfrom
Quicksaver:fix/thread-not-found-subscription-loop

Conversation

@Quicksaver

@Quicksaver Quicksaver commented Jul 21, 2026

Copy link
Copy Markdown

Summary

Treat authoritative missing-thread snapshot responses as terminal client state instead of falling through to a repeatedly failing WebSocket subscription, including after session replacement or application-foreground resubscription. Transient HTTP failures still retain the existing socket-snapshot fallback.

Route web detail synchronization through one canonical resolver so known local drafts wait until the server shell observes thread creation. The merged thread hook detects drafts automatically or honors an explicit shell-wait request, while direct detail and status consumers map their readiness through the same resolver. Draft workspace-mode changes preserve lookup through the reserved thread ref, maintaining the legitimate pre-creation flow without letting a missing-resource response delete the draft.

What Changed

  • Preserve decoded HTTP thread_not_found errors through the thread snapshot loader while mapping transient failures to socket fallback.
  • Clear persisted thread detail, mark the client state deleted, emit structured diagnostics, and terminate before opening or retrying the WebSocket subscription for an authoritative miss or later resubscription signal. Cache removal is serialized with persistence so a queued or in-flight save cannot resurrect deleted data.
  • Represent a missing WebSocket thread snapshot with a dedicated OrchestrationThreadNotFoundError in the subscribeThread RPC error union instead of using a control-flow discriminator on the generic snapshot error.
  • Make resolveThreadDetailRef the canonical web subscription gate, with automatic draft-store detection and explicit shell-wait support in useThread.
  • Route direct detail and status consumers, including the server-thread route, through readiness-aware hooks backed by the same resolver.
  • Preserve pre-shell draft discovery by reserved thread ref when switching between current-checkout and new-worktree modes.
  • Add direct HTTP-loader, thread synchronization, resolver, and web hook lifecycle regression coverage.
  • Document the branch-owned synchronization contract in BRANCH_DETAILS.md.

Why

While investigating renderer memory growth from roughly 300 MB after launch to nearly 2 GB after extended use and idle time, stale thread details were observed being prewarmed after their server resources no longer existed. Each HTTP lookup returned authoritative thread_not_found, but the client treated that as a transient miss, opened a WebSocket subscription that also failed, and retried the cycle every 250 ms. Multiple stale subscriptions made this a concrete source of sustained allocation, retained state, and background CPU pressure, though it should not be assumed to explain every byte of renderer memory growth by itself.

The terminal handling stops that loop, evicts the stale persisted detail, prevents pending persistence work from restoring it, and prevents later session or foreground events from reopening the socket. Because a brand-new local draft also legitimately has no server thread before its first send, the canonical web resolver now makes automatic and explicit draft callers wait for the shell's creation signal so the fix does not turn that expected pre-creation 404 into a deleted draft.

Validation

  • Focused tests passed across HTTP snapshot loading, thread synchronization, web resolver/hook lifecycle behavior, draft identity across workspace-mode changes, and the server missing-thread RPC path.
  • Contracts and client-runtime typechecks passed, with one unrelated existing suggestion in client-runtime src/relay/discovery.ts.
  • Targeted formatting and lint passed for the changed source files included in focused validation.
  • Integrated Playwright verification confirmed that a local draft issued no pre-creation detail request, promoted on first send, then requested only its created server thread and completed the response.

Proof

No additional proof artifacts are required; the focused automated coverage and integrated browser flow exercise the affected lifecycle directly.


Note

Medium Risk
Changes core thread sync, persistence, and subscription lifecycle; incorrect not-found vs draft gating could mark live threads deleted or leave stale retries.

Overview
Authoritative thread not found responses now end thread-detail sync instead of falling through to a WebSocket path that retried every 250ms. HTTP thread_not_found stays in the error channel (transient HTTP failures still use socket fallback). The server exposes OrchestrationThreadNotFoundError on subscribeThread when a snapshot is missing. The client marks the thread deleted, clears persisted detail under a lock so queued saves cannot resurrect it, and stops resubscribing on foreground wakeups or session replacement.

On the web, resolveThreadDetailRef gates detail subscriptions: local drafts and explicit waitForShell wait for a server shell before subscribing, so expected pre-creation 404s do not delete drafts. The chat route uses readiness-aware detail/status hooks backed by the same resolver.

Reviewed by Cursor Bugbot for commit 5b79dd2. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Treat missing thread snapshots as terminal by propagating not-found errors to subscriptions

  • Introduces OrchestrationThreadNotFoundError in orchestration.ts and adds it to the subscribeThread RPC error union so authoritative not-found signals are distinguishable from transient failures.
  • ThreadSnapshotLoader.load in threadSnapshotHttp.ts now fails the effect with EnvironmentResourceNotFoundError on HTTP 404 thread-not-found instead of returning Option.none; other failures still yield Option.none for socket fallback.
  • The makeEnvironmentThreadState machine in threads.ts marks a thread deleted and terminates its subscription on authoritative not-found from either HTTP or WebSocket, removes the cache entry under a persistence lock, and skips resubscription on foreground wakeups or session replacement.
  • The subscribeThread WebSocket handler in ws.ts now returns OrchestrationThreadNotFoundError instead of OrchestrationGetSnapshotError when a snapshot is missing.
  • Draft thread subscriptions in entities.ts are deferred until a server shell exists, preventing premature not-found signals for threads still being created.
  • Behavioral Change: threads that receive an authoritative not-found response are permanently marked deleted and will not resubscribe, replacing the previous behavior of retrying or surfacing a generic error.

Macroscope summarized 5b79dd2.

- Preserve authoritative thread-not-found snapshot failures
- Mark missing threads deleted before opening socket subscriptions
- Cover terminal missing-thread handling with sync tests
- Document thread detail subscription reliability
- Delay draft detail loading until the server shell appears
- Cover draft and normal thread subscription decisions
- Document pre-creation draft synchronization behavior
- Verify decoded 404 and transient HTTP loader behavior
- Cover draft detail subscription activation through the hook
- Add structured metadata for terminal missing-thread cleanup
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9900c10f-c564-41a4-90d8-3b8a13e31ab2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 21, 2026

@macroscopeapp macroscopeapp Bot left a comment

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.

Effect service review: one convention violation found. The new HTTP thread_not_found handling uses Effect.catchTag, but the conventions require Effect.catchTags for statically known tagged failures — including when handling a single tag.

Posted via Macroscope — Effect Service Conventions

Comment thread packages/client-runtime/src/state/threads.ts Outdated
@macroscopeapp

macroscopeapp Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This PR changes how thread subscriptions handle missing resources, converting infinite retry loops to terminal states. It modifies core subscription lifecycle behavior across multiple packages with new error types, persistence locking, and state machine guards. The scope and runtime impact warrant human review.

You can customize Macroscope's approvability policy. Learn more.

Comment thread apps/web/src/state/entities.ts
Comment thread packages/client-runtime/src/state/threads.ts Outdated
- Gate direct route detail reads until local drafts have a shell
- Interrupt subscribe input creation once thread state is deleted
- Cover foreground resubscription and direct draft consumers
…d-subscription-loop

# Conflicts:
#	apps/web/src/routes/_chat.$environmentId.$threadId.tsx
#	apps/web/src/state/entities.ts

@cursor cursor Bot left a comment

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.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 2d25d20. Configure here.

Comment thread apps/web/src/routes/_chat.$environmentId.$threadId.tsx
Comment thread packages/client-runtime/src/state/threads.ts

@macroscopeapp macroscopeapp Bot left a comment

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.

One finding: the new reason discriminator on the shared OrchestrationGetSnapshotError drives caller control flow and should be a distinct error class. The previously flagged catchTag usage in packages/client-runtime/src/state/threads.ts is now Effect.catchTags — thanks.

Posted via Macroscope — Effect Service Conventions

Comment thread packages/contracts/src/orchestration.ts Outdated
- Serialize snapshot persistence with cache removal
- Model missing thread subscriptions with a dedicated RPC error

@macroscopeapp macroscopeapp Bot left a comment

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.

Effect service conventions review of the changed TypeScript. One finding: the new OrchestrationThreadNotFoundError does not derive a message from its attributes, dropping the caller-visible message the previous OrchestrationGetSnapshotError sent over the WS RPC error channel. Everything else (the catchTags handling in threads.ts, the Semaphore-serialized persistence, the separate error tag, subpath namespace imports, and the Layer.effect/Context.Service shape of ThreadSnapshotLoader) matches the conventions.

Posted via Macroscope — Effect Service Conventions

Comment thread packages/contracts/src/orchestration.ts Outdated
T3 Verification and others added 22 commits August 3, 2026 15:30
…d-subscription-loop

# Conflicts:
#	packages/client-runtime/src/state/threadSnapshotHttp.ts
#	packages/client-runtime/src/state/threads.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant