fix(client): exit the client on an explicit detach
- commit
- 4165329ad769c8d58a6fda71176be7640ce4e575
- author
- brice · · 2026-09-23 08:36 UTC
- parent
- ce4cfbb
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
7 files · +161 -9
M internal/usecase/client/attachment_session.go +6 -1
| @@ -452,8 +452,13 @@ func (w *sessionAttachmentWorker) pumpAttached(ctx context.Context, fg Attachmen | |||
| 452 | 452 | case protocol.ErrorMsg: | |
| 453 | 453 | return AttachmentEvent{Token: token, Kind: AttachmentEventFailed, Err: &ProtocolError{Code: typed.Code, Text: typed.Text}} | |
| 454 | 454 | case protocol.Detached: | |
| 455 | - | if typed.Reason == protocol.ReasonDetachToPicker { | |
| 455 | + | switch typed.Reason { | |
| 456 | + | case protocol.ReasonDetachToPicker: | |
| 456 | 457 | return attachmentLifecycleEnd(token, AttachmentDetachToPicker) | |
| 458 | + | case protocol.ReasonDetach: | |
| 459 | + | // The daemon confirmed the user's explicit detach request: the | |
| 460 | + | // client exits cleanly instead of falling back to the picker. | |
| 461 | + | return attachmentLifecycleEnd(token, AttachmentDetachAndExit) | |
| 457 | 462 | } | |
| 458 | 463 | return AttachmentEvent{Token: token, Kind: AttachmentEventEnded} | |
| 459 | 464 | } | |
| @@ -452,8 +452,13 @@ func (w *sessionAttachmentWorker) pumpAttached(ctx context.Context, fg Attachmen | |||||
| 452 | case protocol.ErrorMsg: | 452 | case protocol.ErrorMsg: | ||
| 453 | return AttachmentEvent{Token: token, Kind: AttachmentEventFailed, Err: &ProtocolError{Code: typed.Code, Text: typed.Text}} | 453 | return AttachmentEvent{Token: token, Kind: AttachmentEventFailed, Err: &ProtocolError{Code: typed.Code, Text: typed.Text}} | ||
| 454 | case protocol.Detached: | 454 | case protocol.Detached: | ||
| 455 | - | if typed.Reason == protocol.ReasonDetachToPicker { | 455 | + | switch typed.Reason { |
| 456 | + | case protocol.ReasonDetachToPicker: | |||
| 456 | return attachmentLifecycleEnd(token, AttachmentDetachToPicker) | 457 | return attachmentLifecycleEnd(token, AttachmentDetachToPicker) | ||
| 458 | + | case protocol.ReasonDetach: | |||
| 459 | + | // The daemon confirmed the user's explicit detach request: the | |||
| 460 | + | // client exits cleanly instead of falling back to the picker. | |||
| 461 | + | return attachmentLifecycleEnd(token, AttachmentDetachAndExit) | |||
| 457 | } | 462 | } | ||
| 458 | return AttachmentEvent{Token: token, Kind: AttachmentEventEnded} | 463 | return AttachmentEvent{Token: token, Kind: AttachmentEventEnded} | ||
| 459 | } | 464 | } | ||
M internal/usecase/client/lifecycle_p54a_test.go +4 -1
| @@ -59,7 +59,10 @@ func TestP54AttachmentOutcomesReturnToPicker(t *testing.T) { | |||
| 59 | 59 | settle func(*sessionTestStream) | |
| 60 | 60 | notice LifecycleNoticeKind | |
| 61 | 61 | }{ | |
| 62 | - | {name: "session termination", settle: func(s *sessionTestStream) { s.deliver(protocol.Detached{Reason: protocol.ReasonDetach}) }, notice: LifecycleNoticeSessionEnded}, | |
| 62 | + | // ReasonDetach (the explicit user detach) is intentionally excluded here: | |
| 63 | + | // TestSupervisorDetachedReasonDeterminesExitOrPicker pins that it exits | |
| 64 | + | // the client instead of returning to the picker. | |
| 65 | + | {name: "server shutdown", settle: func(s *sessionTestStream) { s.deliver(protocol.Detached{Reason: protocol.ReasonServerShutdown}) }, notice: LifecycleNoticeSessionEnded}, | |
| 63 | 66 | {name: "session deletion", settle: func(s *sessionTestStream) { s.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) }, notice: LifecycleNoticeSessionEnded}, | |
| 64 | 67 | {name: "destination failure", settle: func(s *sessionTestStream) { | |
| 65 | 68 | s.fail(ports.BrokerStreamLost{Connection: ports.BrokerConnectionID{1}, Stream: 1, Epoch: 3, Cause: domain.RemoteFailureTransport, Err: errors.New("private diagnostic")}) | |
| @@ -59,7 +59,10 @@ func TestP54AttachmentOutcomesReturnToPicker(t *testing.T) { | |||||
| 59 | settle func(*sessionTestStream) | 59 | settle func(*sessionTestStream) | ||
| 60 | notice LifecycleNoticeKind | 60 | notice LifecycleNoticeKind | ||
| 61 | }{ | 61 | }{ | ||
| 62 | - | {name: "session termination", settle: func(s *sessionTestStream) { s.deliver(protocol.Detached{Reason: protocol.ReasonDetach}) }, notice: LifecycleNoticeSessionEnded}, | 62 | + | // ReasonDetach (the explicit user detach) is intentionally excluded here: |
| 63 | + | // TestSupervisorDetachedReasonDeterminesExitOrPicker pins that it exits | |||
| 64 | + | // the client instead of returning to the picker. | |||
| 65 | + | {name: "server shutdown", settle: func(s *sessionTestStream) { s.deliver(protocol.Detached{Reason: protocol.ReasonServerShutdown}) }, notice: LifecycleNoticeSessionEnded}, | |||
| 63 | {name: "session deletion", settle: func(s *sessionTestStream) { s.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) }, notice: LifecycleNoticeSessionEnded}, | 66 | {name: "session deletion", settle: func(s *sessionTestStream) { s.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) }, notice: LifecycleNoticeSessionEnded}, | ||
| 64 | {name: "destination failure", settle: func(s *sessionTestStream) { | 67 | {name: "destination failure", settle: func(s *sessionTestStream) { | ||
| 65 | s.fail(ports.BrokerStreamLost{Connection: ports.BrokerConnectionID{1}, Stream: 1, Epoch: 3, Cause: domain.RemoteFailureTransport, Err: errors.New("private diagnostic")}) | 68 | s.fail(ports.BrokerStreamLost{Connection: ports.BrokerConnectionID{1}, Stream: 1, Epoch: 3, Cause: domain.RemoteFailureTransport, Err: errors.New("private diagnostic")}) | ||
M internal/usecase/client/resize_presentation_test.go +4 -4
| @@ -616,7 +616,7 @@ func TestSupervisorResizeDuringAttachmentSettlementPreservesSessionOutput(t *tes | |||
| 616 | 616 | // the resized geometry follow the attachment. A run that never emitted the | |
| 617 | 617 | // invalidation cannot reach this point, and a run that consumed it without a | |
| 618 | 618 | // repaint leaves only one. | |
| 619 | - | stream.deliver(protocol.Detached{}) | |
| 619 | + | stream.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) | |
| 620 | 620 | awaitPickerState(t, harness.sup) | |
| 621 | 621 | pickerAtResize := func(sample resizeRenderSample) bool { | |
| 622 | 622 | return sample.state.Presentation == PresentPicker && sample.geometry.Size == resized.Size | |
| @@ -663,7 +663,7 @@ func TestSupervisorResizeConnectingBurstStaysCoalescedAndBuffered(t *testing.T) | |||
| 663 | 663 | require.Equal(t, 1, harness.sup.attachments.pendingPresentationSignals(), | |
| 664 | 664 | "the coalesced burst is preserved through connecting and attached") | |
| 665 | 665 | ||
| 666 | - | stream.deliver(protocol.Detached{}) | |
| 666 | + | stream.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) | |
| 667 | 667 | awaitPickerState(t, harness.sup) | |
| 668 | 668 | pickerAtLatest := func(sample resizeRenderSample) bool { | |
| 669 | 669 | return sample.state.Presentation == PresentPicker && sample.geometry.Size == burst[len(burst)-1].Size | |
| @@ -731,7 +731,7 @@ func TestSupervisorAttachedResizeKeepsGeometryWakeupAndPreservesRepaint(t *testi | |||
| 731 | 731 | require.Equal(t, baseline, harness.recorder.count(), "an attached presentation is never painted over") | |
| 732 | 732 | require.Equal(t, 1, harness.sup.attachments.pendingPresentationSignals(), "the invalidation stays buffered until the picker wait") | |
| 733 | 733 | ||
| 734 | - | stream.deliver(protocol.Detached{}) | |
| 734 | + | stream.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) | |
| 735 | 735 | awaitPickerState(t, harness.sup) | |
| 736 | 736 | require.Eventually(t, func() bool { return harness.sup.attachments.pendingPresentationSignals() == 0 }, 5*time.Second, time.Millisecond, | |
| 737 | 737 | "the preserved invalidation is consumed by the picker wait") | |
| @@ -762,7 +762,7 @@ func TestSupervisorResizeAfterDetachReachesOnlyReplacementAttachment(t *testing. | |||
| 762 | 762 | ||
| 763 | 763 | // The attachment detaches: its generation is retired before any replacement | |
| 764 | 764 | // exists. | |
| 765 | - | first.deliver(protocol.Detached{}) | |
| 765 | + | first.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) | |
| 766 | 766 | awaitPickerState(t, harness.sup) | |
| 767 | 767 | require.True(t, authorityToken(&harness.sup.attachments.authority).IsZero(), "the retired token no longer owns the foreground") | |
| 768 | 768 | ||
| @@ -616,7 +616,7 @@ func TestSupervisorResizeDuringAttachmentSettlementPreservesSessionOutput(t *tes | |||||
| 616 | // the resized geometry follow the attachment. A run that never emitted the | 616 | // the resized geometry follow the attachment. A run that never emitted the | ||
| 617 | // invalidation cannot reach this point, and a run that consumed it without a | 617 | // invalidation cannot reach this point, and a run that consumed it without a | ||
| 618 | // repaint leaves only one. | 618 | // repaint leaves only one. | ||
| 619 | - | stream.deliver(protocol.Detached{}) | 619 | + | stream.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) |
| 620 | awaitPickerState(t, harness.sup) | 620 | awaitPickerState(t, harness.sup) | ||
| 621 | pickerAtResize := func(sample resizeRenderSample) bool { | 621 | pickerAtResize := func(sample resizeRenderSample) bool { | ||
| 622 | return sample.state.Presentation == PresentPicker && sample.geometry.Size == resized.Size | 622 | return sample.state.Presentation == PresentPicker && sample.geometry.Size == resized.Size | ||
| @@ -663,7 +663,7 @@ func TestSupervisorResizeConnectingBurstStaysCoalescedAndBuffered(t *testing.T) | |||||
| 663 | require.Equal(t, 1, harness.sup.attachments.pendingPresentationSignals(), | 663 | require.Equal(t, 1, harness.sup.attachments.pendingPresentationSignals(), | ||
| 664 | "the coalesced burst is preserved through connecting and attached") | 664 | "the coalesced burst is preserved through connecting and attached") | ||
| 665 | 665 | ||||
| 666 | - | stream.deliver(protocol.Detached{}) | 666 | + | stream.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) |
| 667 | awaitPickerState(t, harness.sup) | 667 | awaitPickerState(t, harness.sup) | ||
| 668 | pickerAtLatest := func(sample resizeRenderSample) bool { | 668 | pickerAtLatest := func(sample resizeRenderSample) bool { | ||
| 669 | return sample.state.Presentation == PresentPicker && sample.geometry.Size == burst[len(burst)-1].Size | 669 | return sample.state.Presentation == PresentPicker && sample.geometry.Size == burst[len(burst)-1].Size | ||
| @@ -731,7 +731,7 @@ func TestSupervisorAttachedResizeKeepsGeometryWakeupAndPreservesRepaint(t *testi | |||||
| 731 | require.Equal(t, baseline, harness.recorder.count(), "an attached presentation is never painted over") | 731 | require.Equal(t, baseline, harness.recorder.count(), "an attached presentation is never painted over") | ||
| 732 | require.Equal(t, 1, harness.sup.attachments.pendingPresentationSignals(), "the invalidation stays buffered until the picker wait") | 732 | require.Equal(t, 1, harness.sup.attachments.pendingPresentationSignals(), "the invalidation stays buffered until the picker wait") | ||
| 733 | 733 | ||||
| 734 | - | stream.deliver(protocol.Detached{}) | 734 | + | stream.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) |
| 735 | awaitPickerState(t, harness.sup) | 735 | awaitPickerState(t, harness.sup) | ||
| 736 | require.Eventually(t, func() bool { return harness.sup.attachments.pendingPresentationSignals() == 0 }, 5*time.Second, time.Millisecond, | 736 | require.Eventually(t, func() bool { return harness.sup.attachments.pendingPresentationSignals() == 0 }, 5*time.Second, time.Millisecond, | ||
| 737 | "the preserved invalidation is consumed by the picker wait") | 737 | "the preserved invalidation is consumed by the picker wait") | ||
| @@ -762,7 +762,7 @@ func TestSupervisorResizeAfterDetachReachesOnlyReplacementAttachment(t *testing. | |||||
| 762 | 762 | ||||
| 763 | // The attachment detaches: its generation is retired before any replacement | 763 | // The attachment detaches: its generation is retired before any replacement | ||
| 764 | // exists. | 764 | // exists. | ||
| 765 | - | first.deliver(protocol.Detached{}) | 765 | + | first.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) |
| 766 | awaitPickerState(t, harness.sup) | 766 | awaitPickerState(t, harness.sup) | ||
| 767 | require.True(t, authorityToken(&harness.sup.attachments.authority).IsZero(), "the retired token no longer owns the foreground") | 767 | require.True(t, authorityToken(&harness.sup.attachments.authority).IsZero(), "the retired token no longer owns the foreground") | ||
| 768 | 768 | ||||
M internal/usecase/client/supervisor_attach_test.go +115 -1
| @@ -482,6 +482,120 @@ func awaitPickerState(t *testing.T, sup *Supervisor) { | |||
| 482 | 482 | require.Eventually(t, func() bool { return sup.State().Presentation == PresentPicker }, 5*time.Second, time.Millisecond) | |
| 483 | 483 | } | |
| 484 | 484 | ||
| 485 | + | // TestSupervisorDetachedReasonDeterminesExitOrPicker pins the client-visible | |
| 486 | + | // consequence of every protocol.Detached reason the daemon can send while | |
| 487 | + | // attached: only the explicit user detach (ReasonDetach) ends the run and | |
| 488 | + | // exits the client cleanly. Every other daemon-driven reason returns to the | |
| 489 | + | // picker with a lifecycle notice instead of leaving the user stranded with no | |
| 490 | + | // way out short of Ctrl+C. | |
| 491 | + | func TestSupervisorDetachedReasonDeterminesExitOrPicker(t *testing.T) { | |
| 492 | + | tests := []struct { | |
| 493 | + | name string | |
| 494 | + | reason uint8 | |
| 495 | + | wantExit bool | |
| 496 | + | wantNotice LifecycleNoticeKind | |
| 497 | + | }{ | |
| 498 | + | { | |
| 499 | + | name: "explicit detach exits the client", | |
| 500 | + | reason: protocol.ReasonDetach, | |
| 501 | + | wantExit: true, | |
| 502 | + | wantNotice: LifecycleNoticeDetachAndExit, | |
| 503 | + | }, | |
| 504 | + | { | |
| 505 | + | name: "detach to picker stays in the client", | |
| 506 | + | reason: protocol.ReasonDetachToPicker, | |
| 507 | + | wantExit: false, | |
| 508 | + | wantNotice: LifecycleNoticeDetachToPicker, | |
| 509 | + | }, | |
| 510 | + | { | |
| 511 | + | name: "session killed returns to the picker", | |
| 512 | + | reason: protocol.ReasonSessionKilled, | |
| 513 | + | wantExit: false, | |
| 514 | + | wantNotice: LifecycleNoticeSessionEnded, | |
| 515 | + | }, | |
| 516 | + | { | |
| 517 | + | name: "server shutdown returns to the picker", | |
| 518 | + | reason: protocol.ReasonServerShutdown, | |
| 519 | + | wantExit: false, | |
| 520 | + | wantNotice: LifecycleNoticeSessionEnded, | |
| 521 | + | }, | |
| 522 | + | { | |
| 523 | + | name: "replaced returns to the picker", | |
| 524 | + | reason: protocol.ReasonReplaced, | |
| 525 | + | wantExit: false, | |
| 526 | + | wantNotice: LifecycleNoticeSessionEnded, | |
| 527 | + | }, | |
| 528 | + | } | |
| 529 | + | for _, tt := range tests { | |
| 530 | + | t.Run(tt.name, func(t *testing.T) { | |
| 531 | + | picker := newAttachTestPicker() | |
| 532 | + | notices := make(chan LifecycleNotice, 8) | |
| 533 | + | harness := startAttachHarnessConfig(t, picker, func(cfg *SupervisorConfig) { | |
| 534 | + | cfg.NotifyLifecycle = func(notice LifecycleNotice) { notices <- notice } | |
| 535 | + | }) | |
| 536 | + | stream := attachLiveSession(t, harness, picker) | |
| 537 | + | stream.deliver(protocol.Detached{Reason: tt.reason}) | |
| 538 | + | ||
| 539 | + | if tt.wantExit { | |
| 540 | + | require.NoError(t, harness.waitRun(t)) | |
| 541 | + | } else { | |
| 542 | + | awaitPickerState(t, harness.sup) | |
| 543 | + | } | |
| 544 | + | select { | |
| 545 | + | case notice := <-notices: | |
| 546 | + | require.Equal(t, tt.wantNotice, notice.Kind) | |
| 547 | + | case <-time.After(5 * time.Second): | |
| 548 | + | t.Fatal("the expected lifecycle notice was never delivered") | |
| 549 | + | } | |
| 550 | + | }) | |
| 551 | + | } | |
| 552 | + | } | |
| 553 | + | ||
| 554 | + | // TestSupervisorPendingSwapContinuesRatherThanExiting guards the overlay | |
| 555 | + | // commit-elsewhere path (same-peer switch) against being folded into the | |
| 556 | + | // explicit-detach-and-exit outcome above: the outgoing attachment ends | |
| 557 | + | // through a local lifecycle action, never a protocol.Detached message, and a | |
| 558 | + | // pendingSwap in flight must land the supervisor on the new target instead of | |
| 559 | + | // exiting or surfacing any lifecycle notice. | |
| 560 | + | func TestSupervisorPendingSwapContinuesRatherThanExiting(t *testing.T) { | |
| 561 | + | picker := newAttachTestPicker() | |
| 562 | + | notices := make(chan LifecycleNotice, 8) | |
| 563 | + | harness := startAttachHarnessConfig(t, picker, func(cfg *SupervisorConfig) { | |
| 564 | + | cfg.NotifyLifecycle = func(notice LifecycleNotice) { notices <- notice } | |
| 565 | + | }) | |
| 566 | + | first := attachLiveSession(t, harness, picker) | |
| 567 | + | first.deliver(navigationOffer(1)) | |
| 568 | + | awaitPresentation(t, harness.sup, PresentAttachedPicker) | |
| 569 | + | ||
| 570 | + | var mu sync.Mutex | |
| 571 | + | second := newSessionTestStream() | |
| 572 | + | harness.service.setOpenStream(func(context.Context, ports.BrokerOpenStreamRequest) (ports.BrokerLogicalConnection, error) { | |
| 573 | + | return second, nil | |
| 574 | + | }) | |
| 575 | + | other := sessionTestRequest(true) | |
| 576 | + | other.Target = protocol.ExactSessionTarget{LifecycleID: domain.SessionLifecycleID{2}, SessionName: "beta"} | |
| 577 | + | picker.recordOp(pickerOp{commit: true}, "row", other) | |
| 578 | + | ||
| 579 | + | awaitSent(t, first, "Detach", isSent[protocol.Detach]) | |
| 580 | + | awaitStreamHello(t, &mu, &second) | |
| 581 | + | second.deliver(protocol.Welcome{SessionName: "beta"}) | |
| 582 | + | output := sessionTestOutput(1, "\x1b[Hbeta") | |
| 583 | + | output.Context.Route.Target = other.Target | |
| 584 | + | second.deliver(output) | |
| 585 | + | awaitAttachedState(t, harness.sup) | |
| 586 | + | ||
| 587 | + | select { | |
| 588 | + | case <-harness.runDone: | |
| 589 | + | t.Fatal("a same-peer swap must not end the supervisor run") | |
| 590 | + | default: | |
| 591 | + | } | |
| 592 | + | select { | |
| 593 | + | case notice := <-notices: | |
| 594 | + | t.Fatalf("a swap must not report a lifecycle transition: %+v", notice) | |
| 595 | + | default: | |
| 596 | + | } | |
| 597 | + | } | |
| 598 | + | ||
| 485 | 599 | // awaitStreamHello waits until the admitted stream has sent its first client | |
| 486 | 600 | // message, polling the guarded stream pointer rather than a stale snapshot. | |
| 487 | 601 | func awaitStreamHello(t *testing.T, mu *sync.Mutex, stream **sessionTestStream) { | |
| @@ -615,7 +729,7 @@ func TestSupervisorUIActionBindingUsesSingleAttachmentPump(t *testing.T) { | |||
| 615 | 729 | ||
| 616 | 730 | // Ending the stream releases before picker reacquisition. A stale request | |
| 617 | 731 | // would be admitted here if finalization forgot to retire the binding. | |
| 618 | - | admitted.deliver(protocol.Detached{}) | |
| 732 | + | admitted.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) | |
| 619 | 733 | awaitPickerState(t, harness.sup) | |
| 620 | 734 | ui.mu.Lock() | |
| 621 | 735 | require.Nil(t, ui.input, "attachment finalization must revoke the UI pump binding") | |
| @@ -482,6 +482,120 @@ func awaitPickerState(t *testing.T, sup *Supervisor) { | |||||
| 482 | require.Eventually(t, func() bool { return sup.State().Presentation == PresentPicker }, 5*time.Second, time.Millisecond) | 482 | require.Eventually(t, func() bool { return sup.State().Presentation == PresentPicker }, 5*time.Second, time.Millisecond) | ||
| 483 | } | 483 | } | ||
| 484 | 484 | ||||
| 485 | + | // TestSupervisorDetachedReasonDeterminesExitOrPicker pins the client-visible | |||
| 486 | + | // consequence of every protocol.Detached reason the daemon can send while | |||
| 487 | + | // attached: only the explicit user detach (ReasonDetach) ends the run and | |||
| 488 | + | // exits the client cleanly. Every other daemon-driven reason returns to the | |||
| 489 | + | // picker with a lifecycle notice instead of leaving the user stranded with no | |||
| 490 | + | // way out short of Ctrl+C. | |||
| 491 | + | func TestSupervisorDetachedReasonDeterminesExitOrPicker(t *testing.T) { | |||
| 492 | + | tests := []struct { | |||
| 493 | + | name string | |||
| 494 | + | reason uint8 | |||
| 495 | + | wantExit bool | |||
| 496 | + | wantNotice LifecycleNoticeKind | |||
| 497 | + | }{ | |||
| 498 | + | { | |||
| 499 | + | name: "explicit detach exits the client", | |||
| 500 | + | reason: protocol.ReasonDetach, | |||
| 501 | + | wantExit: true, | |||
| 502 | + | wantNotice: LifecycleNoticeDetachAndExit, | |||
| 503 | + | }, | |||
| 504 | + | { | |||
| 505 | + | name: "detach to picker stays in the client", | |||
| 506 | + | reason: protocol.ReasonDetachToPicker, | |||
| 507 | + | wantExit: false, | |||
| 508 | + | wantNotice: LifecycleNoticeDetachToPicker, | |||
| 509 | + | }, | |||
| 510 | + | { | |||
| 511 | + | name: "session killed returns to the picker", | |||
| 512 | + | reason: protocol.ReasonSessionKilled, | |||
| 513 | + | wantExit: false, | |||
| 514 | + | wantNotice: LifecycleNoticeSessionEnded, | |||
| 515 | + | }, | |||
| 516 | + | { | |||
| 517 | + | name: "server shutdown returns to the picker", | |||
| 518 | + | reason: protocol.ReasonServerShutdown, | |||
| 519 | + | wantExit: false, | |||
| 520 | + | wantNotice: LifecycleNoticeSessionEnded, | |||
| 521 | + | }, | |||
| 522 | + | { | |||
| 523 | + | name: "replaced returns to the picker", | |||
| 524 | + | reason: protocol.ReasonReplaced, | |||
| 525 | + | wantExit: false, | |||
| 526 | + | wantNotice: LifecycleNoticeSessionEnded, | |||
| 527 | + | }, | |||
| 528 | + | } | |||
| 529 | + | for _, tt := range tests { | |||
| 530 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 531 | + | picker := newAttachTestPicker() | |||
| 532 | + | notices := make(chan LifecycleNotice, 8) | |||
| 533 | + | harness := startAttachHarnessConfig(t, picker, func(cfg *SupervisorConfig) { | |||
| 534 | + | cfg.NotifyLifecycle = func(notice LifecycleNotice) { notices <- notice } | |||
| 535 | + | }) | |||
| 536 | + | stream := attachLiveSession(t, harness, picker) | |||
| 537 | + | stream.deliver(protocol.Detached{Reason: tt.reason}) | |||
| 538 | + | ||||
| 539 | + | if tt.wantExit { | |||
| 540 | + | require.NoError(t, harness.waitRun(t)) | |||
| 541 | + | } else { | |||
| 542 | + | awaitPickerState(t, harness.sup) | |||
| 543 | + | } | |||
| 544 | + | select { | |||
| 545 | + | case notice := <-notices: | |||
| 546 | + | require.Equal(t, tt.wantNotice, notice.Kind) | |||
| 547 | + | case <-time.After(5 * time.Second): | |||
| 548 | + | t.Fatal("the expected lifecycle notice was never delivered") | |||
| 549 | + | } | |||
| 550 | + | }) | |||
| 551 | + | } | |||
| 552 | + | } | |||
| 553 | + | ||||
| 554 | + | // TestSupervisorPendingSwapContinuesRatherThanExiting guards the overlay | |||
| 555 | + | // commit-elsewhere path (same-peer switch) against being folded into the | |||
| 556 | + | // explicit-detach-and-exit outcome above: the outgoing attachment ends | |||
| 557 | + | // through a local lifecycle action, never a protocol.Detached message, and a | |||
| 558 | + | // pendingSwap in flight must land the supervisor on the new target instead of | |||
| 559 | + | // exiting or surfacing any lifecycle notice. | |||
| 560 | + | func TestSupervisorPendingSwapContinuesRatherThanExiting(t *testing.T) { | |||
| 561 | + | picker := newAttachTestPicker() | |||
| 562 | + | notices := make(chan LifecycleNotice, 8) | |||
| 563 | + | harness := startAttachHarnessConfig(t, picker, func(cfg *SupervisorConfig) { | |||
| 564 | + | cfg.NotifyLifecycle = func(notice LifecycleNotice) { notices <- notice } | |||
| 565 | + | }) | |||
| 566 | + | first := attachLiveSession(t, harness, picker) | |||
| 567 | + | first.deliver(navigationOffer(1)) | |||
| 568 | + | awaitPresentation(t, harness.sup, PresentAttachedPicker) | |||
| 569 | + | ||||
| 570 | + | var mu sync.Mutex | |||
| 571 | + | second := newSessionTestStream() | |||
| 572 | + | harness.service.setOpenStream(func(context.Context, ports.BrokerOpenStreamRequest) (ports.BrokerLogicalConnection, error) { | |||
| 573 | + | return second, nil | |||
| 574 | + | }) | |||
| 575 | + | other := sessionTestRequest(true) | |||
| 576 | + | other.Target = protocol.ExactSessionTarget{LifecycleID: domain.SessionLifecycleID{2}, SessionName: "beta"} | |||
| 577 | + | picker.recordOp(pickerOp{commit: true}, "row", other) | |||
| 578 | + | ||||
| 579 | + | awaitSent(t, first, "Detach", isSent[protocol.Detach]) | |||
| 580 | + | awaitStreamHello(t, &mu, &second) | |||
| 581 | + | second.deliver(protocol.Welcome{SessionName: "beta"}) | |||
| 582 | + | output := sessionTestOutput(1, "\x1b[Hbeta") | |||
| 583 | + | output.Context.Route.Target = other.Target | |||
| 584 | + | second.deliver(output) | |||
| 585 | + | awaitAttachedState(t, harness.sup) | |||
| 586 | + | ||||
| 587 | + | select { | |||
| 588 | + | case <-harness.runDone: | |||
| 589 | + | t.Fatal("a same-peer swap must not end the supervisor run") | |||
| 590 | + | default: | |||
| 591 | + | } | |||
| 592 | + | select { | |||
| 593 | + | case notice := <-notices: | |||
| 594 | + | t.Fatalf("a swap must not report a lifecycle transition: %+v", notice) | |||
| 595 | + | default: | |||
| 596 | + | } | |||
| 597 | + | } | |||
| 598 | + | ||||
| 485 | // awaitStreamHello waits until the admitted stream has sent its first client | 599 | // awaitStreamHello waits until the admitted stream has sent its first client | ||
| 486 | // message, polling the guarded stream pointer rather than a stale snapshot. | 600 | // message, polling the guarded stream pointer rather than a stale snapshot. | ||
| 487 | func awaitStreamHello(t *testing.T, mu *sync.Mutex, stream **sessionTestStream) { | 601 | func awaitStreamHello(t *testing.T, mu *sync.Mutex, stream **sessionTestStream) { | ||
| @@ -615,7 +729,7 @@ func TestSupervisorUIActionBindingUsesSingleAttachmentPump(t *testing.T) { | |||||
| 615 | 729 | ||||
| 616 | // Ending the stream releases before picker reacquisition. A stale request | 730 | // Ending the stream releases before picker reacquisition. A stale request | ||
| 617 | // would be admitted here if finalization forgot to retire the binding. | 731 | // would be admitted here if finalization forgot to retire the binding. | ||
| 618 | - | admitted.deliver(protocol.Detached{}) | 732 | + | admitted.deliver(protocol.Detached{Reason: protocol.ReasonSessionKilled}) |
| 619 | awaitPickerState(t, harness.sup) | 733 | awaitPickerState(t, harness.sup) | ||
| 620 | ui.mu.Lock() | 734 | ui.mu.Lock() | ||
| 621 | require.Nil(t, ui.input, "attachment finalization must revoke the UI pump binding") | 735 | require.Nil(t, ui.input, "attachment finalization must revoke the UI pump binding") | ||
M internal/usecase/daemon/client_transport_errors.go +5 -1
| @@ -16,6 +16,10 @@ func (d *Daemon) clientGoneWithoutNotice(sess *session, ac *attachedClient, fail | |||
| 16 | 16 | } | |
| 17 | 17 | ||
| 18 | 18 | func (d *Daemon) clientGoneWithNotice(sess *session, ac *attachedClient, failed ports.ServerConnection, explicit, notice bool) { | |
| 19 | + | d.clientGoneWithNoticeReason(sess, ac, failed, explicit, notice, protocol.ReasonDetach) | |
| 20 | + | } | |
| 21 | + | ||
| 22 | + | func (d *Daemon) clientGoneWithNoticeReason(sess *session, ac *attachedClient, failed ports.ServerConnection, explicit, notice bool, reason uint8) { | |
| 19 | 23 | if sess == nil || ac == nil { | |
| 20 | 24 | return | |
| 21 | 25 | } | |
| @@ -45,7 +49,7 @@ func (d *Daemon) clientGoneWithNotice(sess *session, ac *attachedClient, failed | |||
| 45 | 49 | d.clearParkingInFlightIfAbandoned(sess, ac, parkingToken) | |
| 46 | 50 | return // displaced, or the link was rebound after the precheck | |
| 47 | 51 | } | |
| 48 | - | d.finishClientGone(sess, ac, failed, explicit, notice) | |
| 52 | + | d.finishClientGoneReason(sess, ac, failed, explicit, notice, reason) | |
| 49 | 53 | } | |
| 50 | 54 | ||
| 51 | 55 | func (d *Daemon) clientGoneForAttachment(effect *attachmentEffect, explicit bool) bool { | |
| @@ -16,6 +16,10 @@ func (d *Daemon) clientGoneWithoutNotice(sess *session, ac *attachedClient, fail | |||||
| 16 | } | 16 | } | ||
| 17 | 17 | ||||
| 18 | func (d *Daemon) clientGoneWithNotice(sess *session, ac *attachedClient, failed ports.ServerConnection, explicit, notice bool) { | 18 | func (d *Daemon) clientGoneWithNotice(sess *session, ac *attachedClient, failed ports.ServerConnection, explicit, notice bool) { | ||
| 19 | + | d.clientGoneWithNoticeReason(sess, ac, failed, explicit, notice, protocol.ReasonDetach) | |||
| 20 | + | } | |||
| 21 | + | ||||
| 22 | + | func (d *Daemon) clientGoneWithNoticeReason(sess *session, ac *attachedClient, failed ports.ServerConnection, explicit, notice bool, reason uint8) { | |||
| 19 | if sess == nil || ac == nil { | 23 | if sess == nil || ac == nil { | ||
| 20 | return | 24 | return | ||
| 21 | } | 25 | } | ||
| @@ -45,7 +49,7 @@ func (d *Daemon) clientGoneWithNotice(sess *session, ac *attachedClient, failed | |||||
| 45 | d.clearParkingInFlightIfAbandoned(sess, ac, parkingToken) | 49 | d.clearParkingInFlightIfAbandoned(sess, ac, parkingToken) | ||
| 46 | return // displaced, or the link was rebound after the precheck | 50 | return // displaced, or the link was rebound after the precheck | ||
| 47 | } | 51 | } | ||
| 48 | - | d.finishClientGone(sess, ac, failed, explicit, notice) | 52 | + | d.finishClientGoneReason(sess, ac, failed, explicit, notice, reason) |
| 49 | } | 53 | } | ||
| 50 | 54 | ||||
| 51 | func (d *Daemon) clientGoneForAttachment(effect *attachmentEffect, explicit bool) bool { | 55 | func (d *Daemon) clientGoneForAttachment(effect *attachmentEffect, explicit bool) bool { | ||
M internal/usecase/daemon/handshake.go +3 -1
| @@ -179,7 +179,9 @@ func (d *Daemon) failHandshakeAttachment(sess *session, ac *attachedClient, tr p | |||
| 179 | 179 | return | |
| 180 | 180 | } | |
| 181 | 181 | if welcomed { | |
| 182 | - | d.clientGone(sess, ac, tr, true) | |
| 182 | + | // A failed handshake after Welcome is not an explicit user detach. The | |
| 183 | + | // client must return to its picker and show the destination failure. | |
| 184 | + | d.clientGoneWithNoticeReason(sess, ac, tr, true, true, protocol.ReasonServerShutdown) | |
| 183 | 185 | } else { | |
| 184 | 186 | d.clientGoneWithoutNotice(sess, ac, tr, true) | |
| 185 | 187 | } | |
| @@ -179,7 +179,9 @@ func (d *Daemon) failHandshakeAttachment(sess *session, ac *attachedClient, tr p | |||||
| 179 | return | 179 | return | ||
| 180 | } | 180 | } | ||
| 181 | if welcomed { | 181 | if welcomed { | ||
| 182 | - | d.clientGone(sess, ac, tr, true) | 182 | + | // A failed handshake after Welcome is not an explicit user detach. The |
| 183 | + | // client must return to its picker and show the destination failure. | |||
| 184 | + | d.clientGoneWithNoticeReason(sess, ac, tr, true, true, protocol.ReasonServerShutdown) | |||
| 183 | } else { | 185 | } else { | ||
| 184 | d.clientGoneWithoutNotice(sess, ac, tr, true) | 186 | d.clientGoneWithoutNotice(sess, ac, tr, true) | ||
| 185 | } | 187 | } | ||
M internal/usecase/daemon/handshake_test.go +24 -0
| @@ -9,6 +9,7 @@ import ( | |||
| 9 | 9 | ||
| 10 | 10 | "github.com/stretchr/testify/require" | |
| 11 | 11 | ||
| 12 | + | "github.com/bnema/vev/internal/adapters/sessionwire" | |
| 12 | 13 | "github.com/bnema/vev/internal/domain" | |
| 13 | 14 | "github.com/bnema/vev/internal/protocol" | |
| 14 | 15 | "github.com/bnema/vev/internal/protocol/wire" | |
| @@ -146,6 +147,29 @@ func TestHandshakeFirstPaintCancellationDoesNotRacePaintResult(t *testing.T) { | |||
| 146 | 147 | awaitTestCompletion(t, handshakeDone, "handshake cancellation did not finish") | |
| 147 | 148 | } | |
| 148 | 149 | ||
| 150 | + | func TestHandshakeFailureAfterWelcomeDoesNotReportExplicitDetach(t *testing.T) { | |
| 151 | + | pty, release := newBlockingPTY(t) | |
| 152 | + | defer release() | |
| 153 | + | d := newTestDaemon(t, newFactory(t, pty), stubClock{}) | |
| 154 | + | tr := &closeTrackingTransport{} | |
| 155 | + | sess, ac, err := d.route(protocol.Hello{Version: protocol.Version, Intent: protocol.IntentNew, Name: "welcome-failed", Size: domain.Size{Cols: 80, Rows: 24}, TermEnv: "xterm-256color"}, tr) | |
| 156 | + | require.NoError(t, err) | |
| 157 | + | ||
| 158 | + | d.failHandshakeAttachment(sess, ac, tr, true) | |
| 159 | + | ||
| 160 | + | var detached *protocol.Detached | |
| 161 | + | for _, frame := range tr.Sends() { | |
| 162 | + | message, decodeErr := sessionwire.DecodeServerEnvelope(frame.Payload) | |
| 163 | + | require.NoError(t, decodeErr) | |
| 164 | + | if value, ok := message.(protocol.Detached); ok { | |
| 165 | + | detached = &value | |
| 166 | + | break | |
| 167 | + | } | |
| 168 | + | } | |
| 169 | + | require.NotNil(t, detached, "the client needs a lifecycle response after Welcome") | |
| 170 | + | require.Equal(t, protocol.ReasonServerShutdown, detached.Reason, "a failed destination handshake must return to the picker") | |
| 171 | + | } | |
| 172 | + | ||
| 149 | 173 | func TestHandshakeTimeoutClosesBlockedReceive(t *testing.T) { | |
| 150 | 174 | clock := &signalClock{timers: make(chan *signalTimer, 1)} | |
| 151 | 175 | d := newTestDaemon(t, nil, clock) | |
| @@ -9,6 +9,7 @@ import ( | |||||
| 9 | 9 | ||||
| 10 | "github.com/stretchr/testify/require" | 10 | "github.com/stretchr/testify/require" | ||
| 11 | 11 | ||||
| 12 | + | "github.com/bnema/vev/internal/adapters/sessionwire" | |||
| 12 | "github.com/bnema/vev/internal/domain" | 13 | "github.com/bnema/vev/internal/domain" | ||
| 13 | "github.com/bnema/vev/internal/protocol" | 14 | "github.com/bnema/vev/internal/protocol" | ||
| 14 | "github.com/bnema/vev/internal/protocol/wire" | 15 | "github.com/bnema/vev/internal/protocol/wire" | ||
| @@ -146,6 +147,29 @@ func TestHandshakeFirstPaintCancellationDoesNotRacePaintResult(t *testing.T) { | |||||
| 146 | awaitTestCompletion(t, handshakeDone, "handshake cancellation did not finish") | 147 | awaitTestCompletion(t, handshakeDone, "handshake cancellation did not finish") | ||
| 147 | } | 148 | } | ||
| 148 | 149 | ||||
| 150 | + | func TestHandshakeFailureAfterWelcomeDoesNotReportExplicitDetach(t *testing.T) { | |||
| 151 | + | pty, release := newBlockingPTY(t) | |||
| 152 | + | defer release() | |||
| 153 | + | d := newTestDaemon(t, newFactory(t, pty), stubClock{}) | |||
| 154 | + | tr := &closeTrackingTransport{} | |||
| 155 | + | sess, ac, err := d.route(protocol.Hello{Version: protocol.Version, Intent: protocol.IntentNew, Name: "welcome-failed", Size: domain.Size{Cols: 80, Rows: 24}, TermEnv: "xterm-256color"}, tr) | |||
| 156 | + | require.NoError(t, err) | |||
| 157 | + | ||||
| 158 | + | d.failHandshakeAttachment(sess, ac, tr, true) | |||
| 159 | + | ||||
| 160 | + | var detached *protocol.Detached | |||
| 161 | + | for _, frame := range tr.Sends() { | |||
| 162 | + | message, decodeErr := sessionwire.DecodeServerEnvelope(frame.Payload) | |||
| 163 | + | require.NoError(t, decodeErr) | |||
| 164 | + | if value, ok := message.(protocol.Detached); ok { | |||
| 165 | + | detached = &value | |||
| 166 | + | break | |||
| 167 | + | } | |||
| 168 | + | } | |||
| 169 | + | require.NotNil(t, detached, "the client needs a lifecycle response after Welcome") | |||
| 170 | + | require.Equal(t, protocol.ReasonServerShutdown, detached.Reason, "a failed destination handshake must return to the picker") | |||
| 171 | + | } | |||
| 172 | + | ||||
| 149 | func TestHandshakeTimeoutClosesBlockedReceive(t *testing.T) { | 173 | func TestHandshakeTimeoutClosesBlockedReceive(t *testing.T) { | ||
| 150 | clock := &signalClock{timers: make(chan *signalTimer, 1)} | 174 | clock := &signalClock{timers: make(chan *signalTimer, 1)} | ||
| 151 | d := newTestDaemon(t, nil, clock) | 175 | d := newTestDaemon(t, nil, clock) | ||