Repository navigation
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📥 CommitsReviewing files that changed from the base of the PR and between 6e08f2949a3ce92454ddfa1cb7c195f06e3481a2 and d2c52a0. 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughConnect and handshake deadlines use shared reporting that distinguishes timeouts from deadlines delayed beyond the timeout plus 60 seconds. The deadline clock records elapsed wall and monotonic time and tracks app foreground loss. Suspended deadlines produce warning-level Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No confirmed issue blocks merging; ordinary timeouts retain their previous behavior and text. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change separates delayed connection diagnostics from genuine timeouts while preserving socket closure and authentication checks. No expanded access or weakened authentication was identified. Remaining uncertainty concerns native suspension behavior, which was not rechecked on an iOS device. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue
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. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Extracted suspension classifier —
mobile/src/transport/socket-deadline-suspension.ts(new) keys off wall-clock elapsed: a deadline that fires more thantimeout + 60spast arm is reported assuspended-dial(warn) instead of the host-blameconnect-timeout/handshake-timeout(error), while still closing the socket. - Session refactor —
armConnectTimeout/armHandshakeTimeoutcollapse intoarmSocketDeadlineinrpc-client-socket-session.ts; thependingpredicate is equivalent to the old inline guards for both kinds, and timer assignment/clearing is preserved. - Diagnostics —
suspended-dialis added toConnectionDiagnosticCodebut intentionally left out ofisDiagnosticFailureand everydiagnoseFailurebranch, so a suspended dial is never reported as an unreachable host. - Tests — new
socket-deadline-suspension.test.tscovers both kinds, the exacttimeout + slackboundary versus+1, and that close still runs; the diagnostics test pins that a lone suspended entry yields the no-single-cause fallback.
The refactor is behavior-preserving for the on-time and mildly-late paths, and the new code is scoped to the suspension case. The documented tradeoffs (a host whose timer fires >60s late, or a forward clock jump) are acceptable for this focused fix.
<!--
Pullfrog review metadata. These findings were written against bdf7d3be80;
if commits have landed on fix-18829-suspended-dial-timeout since, treat every specific bug, file, or
line callout as POTENTIALLY STALE and re-diff before acting on it.
- Mode: Review (initial)
- Files reviewed: 5
- Commits reviewed: 1
- Base: main (449b8ca17d)
- Head: fix-18829-suspended-dial-timeout (bdf7d3be80)
- Reviewed commits:
- bdf7d3be80 — fix(mobile): do not call a suspended dial an unreachable host
- Prior pullfrog review: none
-->
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
bdf7d3b to
6e08f29
Compare
Sync update (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 9cd66170-7e1a-4ca6-8b79-be9d2fcefa52
📥 Commits
Reviewing files that changed from the base of the PR and between bdf7d3be8011136f30583f3ca1af327073eb908b and 6e08f2949a3ce92454ddfa1cb7c195f06e3481a2.
📒 Files selected for processing (5)
mobile/src/transport/rpc-client-socket-session.tsmobile/src/transport/socket-deadline-clock.test.tsmobile/src/transport/socket-deadline-clock.tsmobile/src/transport/socket-deadline-suspension.test.tsmobile/src/transport/socket-deadline-suspension.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
There was a problem hiding this comment.
Important
A transient foreground flicker now permanently marks a deadline as suspended, so an on-budget timer can be reported as suspended-dial and a genuine host failure drops out of diagnostics.
Reviewed changes
- Added a deadline clock — new
mobile/src/transport/socket-deadline-clock.tsrecords wall arm time, aperformance.now()monotonic elapsed, and a stickyleftForegroundflag driven byAppStatechange. - Extended the classifier —
describeSocketDeadlinenow combines wall elapsed, monotonic elapsed, andleftForeground, so a forward wall-clock jump stays a host timeout while a foreground loss reads as suspension. - Rewired the session —
armSocketDeadlinecaptures the clock sample at fire and clears timers throughreleaseSocketTimer(which also removes theAppStatelistener), replacing the bareclearTimeoutcalls. - Tests — new
socket-deadline-clock.test.tsplus foursocket-deadline-suspension.test.tscases cover the jump, backward jump, both-clocks-long, and listener cleanup.
One concern inline on the suspended expression. Verified separately that the new clock/listener lifecycle does not leak: each armed timer reclaims its boundClocks entry and listener at fire or clear.
｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
iOS freezes setTimeout, so a 12s connect timer can fire hours later and the diagnostics log says the endpoint is unreachable. A deadline that fires more than a minute past its budget is logged as suspension, then the socket is closed so the existing retry runs. A timer that fires on its budget still reports connect-timeout or handshake-timeout. Fixes stablyai#18829
The deadline now keeps a monotonic elapsed time and whether the app left the foreground. A jump while the app stays active remains a host timeout, and leaving the foreground is still suspension even if the wall clock jumps backward. Co-authored-by: Cursor <cursoragent@cursor.com>
6e08f29 to
d2c52a0
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Made the foreground signal corroborating —
describeSocketDeadlinenow requires a late wall or monotonic clock alongsideleftForeground, so a transient inactive/background blip no longer reclassifies an on-budget timer assuspended-dial. This addresses the prior[!IMPORTANT]finding and restores the PR body's guarantee that an on-budget/≤60s-late timer still reportsconnect-timeout/handshake-timeout. - Detected dials armed while already backgrounded —
watchForegroundLossnow callsonLeave()at arm time whenAppState.currentStateis alreadyinactive/background, so a suspension that began before the timer was armed is still captured. - Added regression coverage — new
socket-deadline-suspension.test.tsandsocket-deadline-clock.test.tscases pin that foreground loss with on-budget clocks, a transientinactive→activeflicker, and a backward wall-clock step all stay host timeouts, while both-clocks-long and already-backgrounded inputs read as suspension.
The classifier change is behavior-preserving for genuine suspensions (a frozen timer still shows a late clock, so leftForeground remains sufficient when performance.now does not advance across the freeze) and closed the earlier false positive. Clock/listener lifecycle remains clean: each armed timer reclaims its boundClocks entry and AppState listener at fire or via releaseSocketTimer.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
This branch has not been deployed
1 participant
Description
On iOS, suspending Orca Mobile freezes
setTimeout. A connect timer armed for 12 seconds can fire hours later, while the socket is still connecting, and the diagnostics log saysNo TCP/WS handshake within 12s — endpoint unreachable?about a host that was up the whole time. The handshake timer has the same shape.Focused fix
In scope: if wall-clock time from arming the timer to the callback is more than the timeout plus 60 seconds, log a suspension (
suspended-dial) and still close the socket so the existing reconnect runs. The same reading covers the connect timer and the handshake timer.Out of scope: the stale Host Orca version on the diagnostics screen, liveness probes, and any change to how long a live timeout waits.
Preserves
A timer that fires on its budget, or up to 60 seconds late, still reports
connect-timeout/handshake-timeoutwith the existing unreachable and handshake text. User-initiated reconnect is unchanged. The frozen socket is still closed.Evidence
node node_modules/vitest/vitest.mjs run --config mobile/vitest.config.ts --cache false src/transport/socket-deadline-suspension.test.ts src/diagnostics/connection-diagnostics-analysis.test.ts21 passed. The 2h44m gap from the report is classified as
App suspended for 2h44m; connection state unknown, re-dialing, and that entry is not diagnosed as an endpoint that missed its timeout. An iOS device was not rechecked.User-regression-tradeoffs
A connect or handshake timer that fires more than a minute late is no longer described as an unreachable host. A genuinely dead endpoint that answers the timer on time is still reported as unreachable.
Fixes #18829