fix: recover shared-session viewer initial joins#12140
Conversation
Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds a recoverable initial shared-session viewer join lifecycle: transient pre-ack transport failures now retry with bounded backoff, failed initial joins transition to an in-pane error state, retryable failures can be retried in the same pane, and established-session reconnect behavior remains separate.
Concerns
- No blocking correctness, security, or approved-spec-drift concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
@seemeroland There's a small overlap in the diff between this and what you're doing in #12111 but this addresses an issue with the viewers joining the session share as opposed to initializing the session share. |
| initial_load_mode: SharedSessionInitialLoadMode, | ||
|
|
||
| stage: Stage, | ||
| initial_join_attempt_id: u64, |
There was a problem hiding this comment.
I think we can move this state within Stage::BeforeJoined since they're strongly coupled. Would also simplify some of the checks checking against that stage
There was a problem hiding this comment.
Good call! Fixed this.
| if can_retry_failed_initial_join { | ||
| Self::update_current_network(¤t_network, ctx, |network, ctx| { | ||
| model.lock().set_write_to_pty_events_for_shared_session_tx( | ||
| network.write_to_pty_events_tx(), |
There was a problem hiding this comment.
Do we actually need to store the write_to_pty_events_tx on the network to reset it here on retry? I thought this would be the same terminal manager which would have already set the same write_to_pty_events_tx before creating the network
There was a problem hiding this comment.
I was aiming for symetry with the receiver here but you're right that the network doesn't actually do anything with the sender at the moment outside of relying it to he terminal manager. I think it's probably fine to split the two up.
Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
Description
Linked Issue
Demo
Testing
./script/format --checkcargo check --manifest-path Cargo.toml -p warp --testscargo test --manifest-path Cargo.toml -p warp initial_join -- --nocapturecargo test --manifest-path Cargo.toml -p warp failed_viewer_join -- --nocapturecargo clippy --manifest-path Cargo.toml --workspace --exclude warp_completer --all-targets --tests -- -D warningscargo clippy --manifest-path Cargo.toml -p warp_completer --all-targets --tests -- -D warnings./script/presubmit; its full-workspacecargo test --no-runphase was terminated bySIGKILLin the sandbox after formatting and Clippy completed successfully.Agent Mode
CHANGELOG-NONE
Co-Authored-By: Oz oz-agent@warp.dev
Conversation: https://staging.warp.dev/conversation/a5926e17-47ce-4726-a86b-aa2da38db2bc
Run: https://oz.staging.warp.dev/runs/019e8e03-950a-7cc7-9bde-6662dcf1932d
Plans:
This PR was generated with Oz.