Repository navigation
Conversation
ApprovabilityVerdict: Needs Human Approval Macroscope's review found this PR requires human approval — This focused bug fix changes shared client-runtime subscription behavior by adding snapshot fallback and race-aware stream recovery after failed resumes. Its normal path is preserved and regression tests are included, but the new stateful stream/error-handling logic is sufficiently nontrivial to merit human review. You can add or adjust custom eligibility rules. Learn more. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
kvnloo
left a comment
There was a problem hiding this comment.
Thanks for the focused recovery path and the Android before/after test; the cached-resume defect case is well covered.
One question about the linked report's failure mode: #19563 records subscribeThread ending with Interrupted / ClientAbort, whereas subscribeDynamicMapped only invokes onDefect for Die or RpcClientDefect (packages/client-runtime/src/rpc/client.ts). This PR's fallback flag is set by onDefect, and the new positive test injects Effect.die. Is the interrupted/aborted subscription in the report expected to pass through that same callback? If not, a focused interruption/reconnect reproduction would help distinguish the verified replay-defect fix from the reported stuck-hydration case before treating #19563 as fully closed.
Not requesting a broader redesign—just checking the causal link between the observed trace and this tested recovery. Credit to @hichish for the original trace and @shivamhwp for the focused fix and mobile evidence.
|
Taking over this one. I’ll reduce the tests to the useful coverage, update the implementation against current main, and verify it before publishing the changes. |
|
@kvnloo An interruption-only client cause does not invoke The #15937 already addresses unknown event types server-side. This takeover is scoped as client-side recovery for other replay defects: a failed cursor-based replay retries once from a full snapshot; a snapshot defect reports a terminal error. Ordinary client interruption is passed through, and the PR references #19563 rather than claiming its exact cause was reproduced. At rebuilt head The actual web app was also checked with the same one-shot synthetic replay failure on baseline |
Dismissing prior approval to re-evaluate e4c9b46
|
The linked-issue warning identifies an unresolved part of #19563, not a claim this PR makes. The body uses “Refs,” explicitly says the original Interrupted/ClientAbort cause is unconfirmed, and labels the live proof as an injected replay defect. It does not claim to close that issue. The retry branch deliberately passes interruption-only causes through. Retrying cancellation would also restart intentional subscription teardown. Failed resumed replay instead requests a full snapshot; a failed snapshot remains terminal. The two focused regressions and the recorded web transport probe cover that narrower behavior, including the subsequent cursor-based resume and absence of a transient error. I am declining the request to add ClientAbort recovery to this PR without a demonstrated causal path. The original issue remains open for that investigation; expanding the retry predicate based only on its server-side symptom could introduce unwanted subscriptions during teardown. |
…after a failed resume A saved thread resumes by asking the server for the events after its cursor. When that replay dies, the same cursor dies again on every reconnect and relaunch, so the thread stays on "Syncing messages...". A failed resume now retries once with the full snapshot, which moves the cursor past the bad events. The sync error is published only where the attempt is given up, so a recovered resume never flashes it and a defect on a snapshot attempt stays terminal. Refs pingdotgg#19563 Co-Authored-By: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e4c9b46 to
f5cb98a
Compare
… terminal The retry decided whether a defect broke a resume by reading the shared attempt kind when the failure reached it. switchMap can buffer that failure while a replacement session starts building its input and resets the attempt to snapshot, so the failed resume was reported as a terminal snapshot failure. The defect is now classified where it happens, before it can be buffered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dismissing prior approval to re-evaluate e9d6e85
Problem
A saved thread can remain stuck on “Syncing messages…” when replaying events from its saved cursor fails. Relaunching asks for the same failed replay again.
Change
When a resumed stream dies, retry it once using a full thread snapshot. Expected and transport failures keep their existing handling. Keep snapshot failures terminal so recovery cannot loop, and use the new snapshot cursor for later sessions. Report a replay failure only if recovery gives up, avoiding an error banner during successful snapshot recovery.
Scope and approval
This is a maintainer-requested takeover of the focused thread-sync bug fix. The shared client runtime serves web, desktop, and mobile. Refs #19563. Based on the original work by @shivamhwp.
The original report’s precise failing event is unconfirmed. #15937 already handles unknown event types on the server; this change adds client recovery for other replay defects.
Verification
After rebasing onto current main (
05b697f6), the 27 sync tests inthreads-sync.test.tsandvp run check:changed(format, lint, knip, typecheck, tests) pass. The three regression cases verify snapshot recovery, later cursor-based resumption, terminal snapshot failure, and recovery when a session replacement starts while the failed resume is still buffered. The first two fail against main'sthreads.ts; the race case fails against the earlier headf5cb98a0, which read the attempt kind after the buffer. All observed recovery states are checked for the absence of a transient error. Reintroducing transient error publication or retrying every defect fails the corresponding test.Before the rebase, the actual web app was tested against the same temporary one-shot replay failure on baseline
07d38b8dand heade4c9b469. The rebase did not changethreads.ts. The later race fix (e9d6e852) only changes when a defect is classified, so the recording still shows the same recovery path. The baseline requested cursor 18 and remained at “Syncing messages…”. The head requested cursor 18, recovered through a full snapshot, then successfully resumed from cursor 18 on a later reconnect. No error banner was observed during recovery. The temporary server probe was removed afterward; no provider turn or database write was needed.This was a synthetic replay failure, not a reproduction of the original
Interrupted/ClientAbortreport. Native clients were not run; they share the changed client runtime. The terminal double-failure path was verified by focused tests, not the recording.Before — cached thread remains syncing
After — snapshot recovery clears syncing
Recovery recording
Taken over with Claude Opus 5.5 (tests and review), Claude Fable 5.1 (implementation), and GPT-6.1 Sol (review) through T3 Code; coordinated with Codex.