fix(threads): stop missing-thread subscription retries - #12711
Quicksaver wants to merge 1 commit into
Conversation
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This is a cross-package runtime change that alters WebSocket/HTTP error contracts, subscription retry behavior, cache deletion ordering, and draft readiness across existing paths. An unresolved medium-severity concurrency finding also concerns an old scope deleting a successor's cached thread after an ownership handoff. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds negotiated thread-not-found errors, preserves authoritative HTTP not-found failures, terminates missing-thread state, serializes cache deletion, and delays web subscriptions for local drafts until their server shells exist. ChangesThread synchronization reliability
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ThreadState
participant ThreadSnapshotLoader
participant Server
participant CacheStore
ThreadState->>ThreadSnapshotLoader: load thread snapshot
ThreadSnapshotLoader->>Server: request snapshot
Server-->>ThreadSnapshotLoader: thread_not_found failure
ThreadSnapshotLoader-->>ThreadState: terminal not-found error
ThreadState->>CacheStore: remove cached thread
ThreadState-->>Server: interrupt subscription
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No confirmed issue currently prevents merging. Draft subscriptions do not start before successful server-thread creation; the cache-removal race remains unverified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new protocol preserves older-client behavior and does not show a new authorization bypass. A narrow remount race may still discard locally cached thread detail; no server-side deletion is indicated. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🤖 Co-authored by GPT-6 in Codex via T3 Code
533af0c to
d1cf1f9
Compare
|
One small repo-hygiene note: The root Would it make sense to drop this file from the PR? The code and tests already capture the compatibility seams it describes. |
|
@coderabbitai The retained ownership concern in your updated review is the same concern already assessed and resolved here. I rechecked the current head, d1cf1f9. All 70 tests in The generic docstring coverage warning also remains unchanged. It identifies no missing explanation, and the repository documentation policy asks us to document non-obvious constraints rather than add per-function narration. No documentation change is needed.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf 'HEAD: '; git rev-parse --short HEAD
printf '\nFiles:\n'; fd -a '^(threads\.ts|AGENTS\.md)$' | head -60
printf '\nThread symbols:\n'; ast-grep outline packages/client-runtime/src/state/threads.ts --items all --match 'setDeletedLocked|remember|persistenceLock|persist|save|resume' | head -160
printf '\nRelevant source lines:\n'; rg -n -C 7 'setDeletedLocked|persistenceLock|remember\(|saveThread\(|removeThread\(|resum(e|ed)' packages/client-runtime/src/state/threads.ts | head -340
printf '\nPolicy matches:\n'; rg -n -C 4 'implementation plans|research notes|PR summaries|non-obvious|docstring|document' AGENTS.md | head -160Length of output: 20223 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf 'State initialization, handoff and persistence:\n'
sed -n '185,335p' packages/client-runtime/src/state/threads.ts
printf '\nDeletion and stream gate:\n'
sed -n '390,475p' packages/client-runtime/src/state/threads.ts
printf '\nRegression test locations:\n'
rg -n -C 5 'pending finalizer|disposed successor|mounted successor|removeThread|persistenceLock' packages/client-runtime/src/state/threads-atoms.test.ts | tail -180
printf '\nRelated review text (if accessible):\n'
gh api repos/pingdotgg/t3code/issues/comments/5748829793 --jq '.body' 2>/dev/null | head -130 || true
gh api repos/pingdotgg/t3code/pulls/comments/4056962476 --jq '.body' 2>/dev/null | head -100 || trueLength of output: 19006
The generic docstring warning was also not actionable under the repository’s documentation policy. No code or documentation change is needed for either finding. I did not rerun the 70 tests you reported. Thanks for the careful correction. ✏️ Learnings added
You are interacting with an AI system. |
This branch has not been deployed
2 participants
Summary
Deleted threads could trigger a subscription retry every 250 ms. The client treated an authoritative HTTP
thread_not_foundresponse as temporary, fell back to WebSocket, and retried when that snapshot failed too.Missing-thread responses now clear cached detail, mark the thread deleted, and stop subscription retries, including after reconnecting or foregrounding the app. Other snapshot failures retain their fallback and retry behavior. Local web drafts wait for their server thread to exist before subscribing, so an expected pre-creation 404 cannot delete a valid draft.
Interactive demo - try it without building and installing
What changed
Validation
245 focused tests passed; scoped typechecks and lint completed.
git diff --check upstream/main...HEADpassed.Summary by CodeRabbit
Bug Fixes
Compatibility