fix(routes): close invalid transitions and sync identity metadata
- commit
- d91224b736ab4099883caa4fb1c4dc7dba2b37c8
- author
- brice · · 2026-08-09 18:44 UTC
- parent
- eac59c2
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
9 files · +140 -18
M internal/ports/routes_test.go +1 -2
| @@ -63,8 +63,7 @@ func TestWelcomeRejectsMismatchedCommittedIdentity(t *testing.T) { | |||
| 63 | 63 | identity := &CommittedRouteIdentity{Target: testExactTarget()} | |
| 64 | 64 | payload := MarshalWelcome(Welcome{SessionID: "daemon-session", SessionName: "other", CommittedIdentity: identity}) | |
| 65 | 65 | ||
| 66 | - | _, err := UnmarshalWelcome(payload) | |
| 67 | - | require.ErrorIs(t, err, ErrInvalidRouteWire) | |
| 66 | + | require.Nil(t, payload) | |
| 68 | 67 | } | |
| 69 | 68 | ||
| 70 | 69 | func TestCommittedRouteIdentityCodec(t *testing.T) { | |
| @@ -63,8 +63,7 @@ func TestWelcomeRejectsMismatchedCommittedIdentity(t *testing.T) { | |||||
| 63 | identity := &CommittedRouteIdentity{Target: testExactTarget()} | 63 | identity := &CommittedRouteIdentity{Target: testExactTarget()} | ||
| 64 | payload := MarshalWelcome(Welcome{SessionID: "daemon-session", SessionName: "other", CommittedIdentity: identity}) | 64 | payload := MarshalWelcome(Welcome{SessionID: "daemon-session", SessionName: "other", CommittedIdentity: identity}) | ||
| 65 | 65 | ||||
| 66 | - | _, err := UnmarshalWelcome(payload) | 66 | + | require.Nil(t, payload) |
| 67 | - | require.ErrorIs(t, err, ErrInvalidRouteWire) | |||
| 68 | } | 67 | } | ||
| 69 | 68 | ||||
| 70 | func TestCommittedRouteIdentityCodec(t *testing.T) { | 69 | func TestCommittedRouteIdentityCodec(t *testing.T) { | ||
M internal/ports/wire.go +3 -0
| @@ -1293,6 +1293,9 @@ func UnmarshalAck(b []byte) (Ack, error) { | |||
| 1293 | 1293 | ||
| 1294 | 1294 | // MarshalWelcome encodes m into a Welcome message payload. | |
| 1295 | 1295 | func MarshalWelcome(m Welcome) []byte { | |
| 1296 | + | if m.CommittedIdentity != nil && (m.SessionName != m.CommittedIdentity.Target.SessionName || m.Ephemeral != m.CommittedIdentity.Ephemeral) { | |
| 1297 | + | return nil | |
| 1298 | + | } | |
| 1296 | 1299 | w := payloadWriter{} | |
| 1297 | 1300 | w.putString(m.SessionID) | |
| 1298 | 1301 | w.putString(m.SessionName) | |
| @@ -1293,6 +1293,9 @@ func UnmarshalAck(b []byte) (Ack, error) { | |||||
| 1293 | 1293 | ||||
| 1294 | // MarshalWelcome encodes m into a Welcome message payload. | 1294 | // MarshalWelcome encodes m into a Welcome message payload. | ||
| 1295 | func MarshalWelcome(m Welcome) []byte { | 1295 | func MarshalWelcome(m Welcome) []byte { | ||
| 1296 | + | if m.CommittedIdentity != nil && (m.SessionName != m.CommittedIdentity.Target.SessionName || m.Ephemeral != m.CommittedIdentity.Ephemeral) { | |||
| 1297 | + | return nil | |||
| 1298 | + | } | |||
| 1296 | w := payloadWriter{} | 1299 | w := payloadWriter{} | ||
| 1297 | w.putString(m.SessionID) | 1300 | w.putString(m.SessionID) | ||
| 1298 | w.putString(m.SessionName) | 1301 | w.putString(m.SessionName) | ||
M internal/ports/wire_test.go +23 -0
| @@ -430,6 +430,29 @@ func TestMarshalWelcomeRejectsInvalidCommittedIdentity(t *testing.T) { | |||
| 430 | 430 | require.Nil(t, got) | |
| 431 | 431 | } | |
| 432 | 432 | ||
| 433 | + | func TestMarshalWelcomeRejectsCommittedIdentityMetadataMismatch(t *testing.T) { | |
| 434 | + | base := Welcome{ | |
| 435 | + | SessionID: "session", | |
| 436 | + | SessionName: "work", | |
| 437 | + | Ephemeral: true, | |
| 438 | + | CommittedIdentity: &CommittedRouteIdentity{ | |
| 439 | + | Target: ExactSessionTarget{LifecycleID: domain.SessionLifecycleID{1}, SessionName: "work"}, | |
| 440 | + | Ephemeral: true, | |
| 441 | + | }, | |
| 442 | + | } | |
| 443 | + | for name, mutate := range map[string]func(*Welcome){ | |
| 444 | + | "session name": func(w *Welcome) { w.SessionName = "other" }, | |
| 445 | + | "ephemeral": func(w *Welcome) { w.Ephemeral = false }, | |
| 446 | + | } { | |
| 447 | + | t.Run(name, func(t *testing.T) { | |
| 448 | + | got := base | |
| 449 | + | mutate(&got) | |
| 450 | + | require.Nil(t, MarshalWelcome(got)) | |
| 451 | + | }) | |
| 452 | + | } | |
| 453 | + | require.NotNil(t, MarshalWelcome(base)) | |
| 454 | + | } | |
| 455 | + | ||
| 433 | 456 | func TestWelcomeGoldenAndRoundTrip(t *testing.T) { | |
| 434 | 457 | tests := []struct { | |
| 435 | 458 | name string | |
| @@ -430,6 +430,29 @@ func TestMarshalWelcomeRejectsInvalidCommittedIdentity(t *testing.T) { | |||||
| 430 | require.Nil(t, got) | 430 | require.Nil(t, got) | ||
| 431 | } | 431 | } | ||
| 432 | 432 | ||||
| 433 | + | func TestMarshalWelcomeRejectsCommittedIdentityMetadataMismatch(t *testing.T) { | |||
| 434 | + | base := Welcome{ | |||
| 435 | + | SessionID: "session", | |||
| 436 | + | SessionName: "work", | |||
| 437 | + | Ephemeral: true, | |||
| 438 | + | CommittedIdentity: &CommittedRouteIdentity{ | |||
| 439 | + | Target: ExactSessionTarget{LifecycleID: domain.SessionLifecycleID{1}, SessionName: "work"}, | |||
| 440 | + | Ephemeral: true, | |||
| 441 | + | }, | |||
| 442 | + | } | |||
| 443 | + | for name, mutate := range map[string]func(*Welcome){ | |||
| 444 | + | "session name": func(w *Welcome) { w.SessionName = "other" }, | |||
| 445 | + | "ephemeral": func(w *Welcome) { w.Ephemeral = false }, | |||
| 446 | + | } { | |||
| 447 | + | t.Run(name, func(t *testing.T) { | |||
| 448 | + | got := base | |||
| 449 | + | mutate(&got) | |||
| 450 | + | require.Nil(t, MarshalWelcome(got)) | |||
| 451 | + | }) | |||
| 452 | + | } | |||
| 453 | + | require.NotNil(t, MarshalWelcome(base)) | |||
| 454 | + | } | |||
| 455 | + | ||||
| 433 | func TestWelcomeGoldenAndRoundTrip(t *testing.T) { | 456 | func TestWelcomeGoldenAndRoundTrip(t *testing.T) { | ||
| 434 | tests := []struct { | 457 | tests := []struct { | ||
| 435 | name string | 458 | name string | ||
M internal/usecase/client/client.go +16 -13
| @@ -525,6 +525,22 @@ func (r *Runner) Run(ctx context.Context, request AttachRequest) (retErr error) | |||
| 525 | 525 | if !result.transportClosed { | |
| 526 | 526 | _ = connection.Close() | |
| 527 | 527 | } | |
| 528 | + | // Apply metadata from every processed frame before dispatching its | |
| 529 | + | // navigation action. A committed identity can arrive immediately before | |
| 530 | + | // a home/back action and must become the route request's authority first. | |
| 531 | + | if result.resumeToken != 0 { | |
| 532 | + | resumeToken = result.resumeToken | |
| 533 | + | } | |
| 534 | + | if result.sessionName != "" { | |
| 535 | + | attemptRequest.SessionName = result.sessionName | |
| 536 | + | } | |
| 537 | + | if result.committedIdentity != nil { | |
| 538 | + | identity := *result.committedIdentity | |
| 539 | + | if attemptRequest.ExactTarget != nil && *attemptRequest.ExactTarget != identity.Target { | |
| 540 | + | attemptRequest.RemoteTarget = nil | |
| 541 | + | } | |
| 542 | + | attemptRequest.ExactTarget = &identity.Target | |
| 543 | + | } | |
| 528 | 544 | if result.routeAction != nil { | |
| 529 | 545 | if r.ledger == nil { | |
| 530 | 546 | return errors.New("vev: route ledger unavailable") | |
| @@ -646,19 +662,6 @@ func (r *Runner) Run(ctx context.Context, request AttachRequest) (retErr error) | |||
| 646 | 662 | } | |
| 647 | 663 | return nil | |
| 648 | 664 | } | |
| 649 | - | if result.resumeToken != 0 { | |
| 650 | - | resumeToken = result.resumeToken | |
| 651 | - | } | |
| 652 | - | if result.sessionName != "" { | |
| 653 | - | attemptRequest.SessionName = result.sessionName | |
| 654 | - | } | |
| 655 | - | if result.committedIdentity != nil { | |
| 656 | - | identity := *result.committedIdentity | |
| 657 | - | if attemptRequest.ExactTarget != nil && *attemptRequest.ExactTarget != identity.Target { | |
| 658 | - | attemptRequest.RemoteTarget = nil | |
| 659 | - | } | |
| 660 | - | attemptRequest.ExactTarget = &identity.Target | |
| 661 | - | } | |
| 662 | 665 | if result.welcomed { | |
| 663 | 666 | homeNavigationPending = false | |
| 664 | 667 | } | |
| @@ -525,6 +525,22 @@ func (r *Runner) Run(ctx context.Context, request AttachRequest) (retErr error) | |||||
| 525 | if !result.transportClosed { | 525 | if !result.transportClosed { | ||
| 526 | _ = connection.Close() | 526 | _ = connection.Close() | ||
| 527 | } | 527 | } | ||
| 528 | + | // Apply metadata from every processed frame before dispatching its | |||
| 529 | + | // navigation action. A committed identity can arrive immediately before | |||
| 530 | + | // a home/back action and must become the route request's authority first. | |||
| 531 | + | if result.resumeToken != 0 { | |||
| 532 | + | resumeToken = result.resumeToken | |||
| 533 | + | } | |||
| 534 | + | if result.sessionName != "" { | |||
| 535 | + | attemptRequest.SessionName = result.sessionName | |||
| 536 | + | } | |||
| 537 | + | if result.committedIdentity != nil { | |||
| 538 | + | identity := *result.committedIdentity | |||
| 539 | + | if attemptRequest.ExactTarget != nil && *attemptRequest.ExactTarget != identity.Target { | |||
| 540 | + | attemptRequest.RemoteTarget = nil | |||
| 541 | + | } | |||
| 542 | + | attemptRequest.ExactTarget = &identity.Target | |||
| 543 | + | } | |||
| 528 | if result.routeAction != nil { | 544 | if result.routeAction != nil { | ||
| 529 | if r.ledger == nil { | 545 | if r.ledger == nil { | ||
| 530 | return errors.New("vev: route ledger unavailable") | 546 | return errors.New("vev: route ledger unavailable") | ||
| @@ -646,19 +662,6 @@ func (r *Runner) Run(ctx context.Context, request AttachRequest) (retErr error) | |||||
| 646 | } | 662 | } | ||
| 647 | return nil | 663 | return nil | ||
| 648 | } | 664 | } | ||
| 649 | - | if result.resumeToken != 0 { | |||
| 650 | - | resumeToken = result.resumeToken | |||
| 651 | - | } | |||
| 652 | - | if result.sessionName != "" { | |||
| 653 | - | attemptRequest.SessionName = result.sessionName | |||
| 654 | - | } | |||
| 655 | - | if result.committedIdentity != nil { | |||
| 656 | - | identity := *result.committedIdentity | |||
| 657 | - | if attemptRequest.ExactTarget != nil && *attemptRequest.ExactTarget != identity.Target { | |||
| 658 | - | attemptRequest.RemoteTarget = nil | |||
| 659 | - | } | |||
| 660 | - | attemptRequest.ExactTarget = &identity.Target | |||
| 661 | - | } | |||
| 662 | if result.welcomed { | 665 | if result.welcomed { | ||
| 663 | homeNavigationPending = false | 666 | homeNavigationPending = false | ||
| 664 | } | 667 | } | ||
M internal/usecase/client/client_test.go +13 -2
| @@ -490,6 +490,7 @@ func TestRouteNavigationPreservesRemoteHomePickerAcrossLocalReturn(t *testing.T) | |||
| 490 | 490 | localLifecycle := domain.SessionLifecycleID{1, 1, 1} | |
| 491 | 491 | remoteLifecycle := domain.SessionLifecycleID{2, 2, 2} | |
| 492 | 492 | newLifecycle := domain.SessionLifecycleID{3, 3, 3} | |
| 493 | + | remoteNewLifecycle := domain.SessionLifecycleID{4, 4, 4} | |
| 493 | 494 | welcome := func(name string, lifecycle domain.SessionLifecycleID, token uint64) ports.Frame { | |
| 494 | 495 | return frameOf(ports.MsgWelcome, ports.MarshalWelcome(ports.Welcome{ | |
| 495 | 496 | SessionID: name + "-id", | |
| @@ -526,7 +527,7 @@ func TestRouteNavigationPreservesRemoteHomePickerAcrossLocalReturn(t *testing.T) | |||
| 526 | 527 | }} | |
| 527 | 528 | local3 := &recordingTransport{recvs: []recvItem{ | |
| 528 | 529 | {f: welcome("test", localLifecycle, 11)}, | |
| 529 | - | {f: frameOf(ports.MsgDetached, ports.MarshalDetached(ports.Detached{Reason: ports.ReasonDetach}))}, | |
| 530 | + | {f: frameOf(ports.MsgNavigationAction, ports.MarshalNavigationAction(ports.NavigationBack))}, | |
| 530 | 531 | }} | |
| 531 | 532 | remote1 := &recordingTransport{recvs: []recvItem{ | |
| 532 | 533 | {f: welcome("remote-manual", remoteLifecycle, 22)}, | |
| @@ -534,10 +535,17 @@ func TestRouteNavigationPreservesRemoteHomePickerAcrossLocalReturn(t *testing.T) | |||
| 534 | 535 | }} | |
| 535 | 536 | remote2 := &recordingTransport{recvs: []recvItem{ | |
| 536 | 537 | {f: welcome("remote-manual", remoteLifecycle, 22)}, | |
| 538 | + | {f: frameOf(ports.MsgCommittedRouteIdentity, mustMarshalCommittedIdentity(ports.CommittedRouteIdentity{ | |
| 539 | + | Target: ports.ExactSessionTarget{LifecycleID: remoteNewLifecycle, SessionName: "remote-new"}, | |
| 540 | + | }))}, | |
| 537 | 541 | {f: frameOf(ports.MsgNavigationAction, ports.MarshalNavigationAction(ports.NavigationOpenHomePicker))}, | |
| 538 | 542 | }} | |
| 543 | + | remote3 := &recordingTransport{recvs: []recvItem{ | |
| 544 | + | {f: welcome("remote-new", remoteNewLifecycle, 22)}, | |
| 545 | + | {f: frameOf(ports.MsgDetached, ports.MarshalDetached(ports.Detached{Reason: ports.ReasonDetach}))}, | |
| 546 | + | }} | |
| 539 | 547 | localDialer := &sequenceDialer{trs: []ports.Transport{local1, local2, local3}} | |
| 540 | - | remoteDialer := &sequenceDialer{trs: []ports.Transport{remote1, remote2}} | |
| 548 | + | remoteDialer := &sequenceDialer{trs: []ports.Transport{remote1, remote2, remote3}} | |
| 541 | 549 | ||
| 542 | 550 | deps := testDependencies(localDialer, term, realClock{}, nil, nil) | |
| 543 | 551 | deps.AttachHandoff = func(target ports.AttachTarget) (ports.Dialer, client.AttachRequest, error) { | |
| @@ -556,6 +564,9 @@ func TestRouteNavigationPreservesRemoteHomePickerAcrossLocalReturn(t *testing.T) | |||
| 556 | 564 | require.NoError(t, err) | |
| 557 | 565 | ||
| 558 | 566 | require.Equal(t, ports.NavigationCapabilityHomePicker, remoteHello.NavigationCapabilities) | |
| 567 | + | remoteNewHello := helloFromSend(t, remote3) | |
| 568 | + | require.Equal(t, "remote-new", remoteNewHello.Name) | |
| 569 | + | require.Equal(t, &ports.ExactSessionTarget{LifecycleID: remoteNewLifecycle, SessionName: "remote-new"}, remoteNewHello.ExactTarget) | |
| 559 | 570 | } | |
| 560 | 571 | ||
| 561 | 572 | func mustMarshalCommittedIdentity(identity ports.CommittedRouteIdentity) []byte { | |
| @@ -490,6 +490,7 @@ func TestRouteNavigationPreservesRemoteHomePickerAcrossLocalReturn(t *testing.T) | |||||
| 490 | localLifecycle := domain.SessionLifecycleID{1, 1, 1} | 490 | localLifecycle := domain.SessionLifecycleID{1, 1, 1} | ||
| 491 | remoteLifecycle := domain.SessionLifecycleID{2, 2, 2} | 491 | remoteLifecycle := domain.SessionLifecycleID{2, 2, 2} | ||
| 492 | newLifecycle := domain.SessionLifecycleID{3, 3, 3} | 492 | newLifecycle := domain.SessionLifecycleID{3, 3, 3} | ||
| 493 | + | remoteNewLifecycle := domain.SessionLifecycleID{4, 4, 4} | |||
| 493 | welcome := func(name string, lifecycle domain.SessionLifecycleID, token uint64) ports.Frame { | 494 | welcome := func(name string, lifecycle domain.SessionLifecycleID, token uint64) ports.Frame { | ||
| 494 | return frameOf(ports.MsgWelcome, ports.MarshalWelcome(ports.Welcome{ | 495 | return frameOf(ports.MsgWelcome, ports.MarshalWelcome(ports.Welcome{ | ||
| 495 | SessionID: name + "-id", | 496 | SessionID: name + "-id", | ||
| @@ -526,7 +527,7 @@ func TestRouteNavigationPreservesRemoteHomePickerAcrossLocalReturn(t *testing.T) | |||||
| 526 | }} | 527 | }} | ||
| 527 | local3 := &recordingTransport{recvs: []recvItem{ | 528 | local3 := &recordingTransport{recvs: []recvItem{ | ||
| 528 | {f: welcome("test", localLifecycle, 11)}, | 529 | {f: welcome("test", localLifecycle, 11)}, | ||
| 529 | - | {f: frameOf(ports.MsgDetached, ports.MarshalDetached(ports.Detached{Reason: ports.ReasonDetach}))}, | 530 | + | {f: frameOf(ports.MsgNavigationAction, ports.MarshalNavigationAction(ports.NavigationBack))}, |
| 530 | }} | 531 | }} | ||
| 531 | remote1 := &recordingTransport{recvs: []recvItem{ | 532 | remote1 := &recordingTransport{recvs: []recvItem{ | ||
| 532 | {f: welcome("remote-manual", remoteLifecycle, 22)}, | 533 | {f: welcome("remote-manual", remoteLifecycle, 22)}, | ||
| @@ -534,10 +535,17 @@ func TestRouteNavigationPreservesRemoteHomePickerAcrossLocalReturn(t *testing.T) | |||||
| 534 | }} | 535 | }} | ||
| 535 | remote2 := &recordingTransport{recvs: []recvItem{ | 536 | remote2 := &recordingTransport{recvs: []recvItem{ | ||
| 536 | {f: welcome("remote-manual", remoteLifecycle, 22)}, | 537 | {f: welcome("remote-manual", remoteLifecycle, 22)}, | ||
| 538 | + | {f: frameOf(ports.MsgCommittedRouteIdentity, mustMarshalCommittedIdentity(ports.CommittedRouteIdentity{ | |||
| 539 | + | Target: ports.ExactSessionTarget{LifecycleID: remoteNewLifecycle, SessionName: "remote-new"}, | |||
| 540 | + | }))}, | |||
| 537 | {f: frameOf(ports.MsgNavigationAction, ports.MarshalNavigationAction(ports.NavigationOpenHomePicker))}, | 541 | {f: frameOf(ports.MsgNavigationAction, ports.MarshalNavigationAction(ports.NavigationOpenHomePicker))}, | ||
| 538 | }} | 542 | }} | ||
| 543 | + | remote3 := &recordingTransport{recvs: []recvItem{ | |||
| 544 | + | {f: welcome("remote-new", remoteNewLifecycle, 22)}, | |||
| 545 | + | {f: frameOf(ports.MsgDetached, ports.MarshalDetached(ports.Detached{Reason: ports.ReasonDetach}))}, | |||
| 546 | + | }} | |||
| 539 | localDialer := &sequenceDialer{trs: []ports.Transport{local1, local2, local3}} | 547 | localDialer := &sequenceDialer{trs: []ports.Transport{local1, local2, local3}} | ||
| 540 | - | remoteDialer := &sequenceDialer{trs: []ports.Transport{remote1, remote2}} | 548 | + | remoteDialer := &sequenceDialer{trs: []ports.Transport{remote1, remote2, remote3}} |
| 541 | 549 | ||||
| 542 | deps := testDependencies(localDialer, term, realClock{}, nil, nil) | 550 | deps := testDependencies(localDialer, term, realClock{}, nil, nil) | ||
| 543 | deps.AttachHandoff = func(target ports.AttachTarget) (ports.Dialer, client.AttachRequest, error) { | 551 | deps.AttachHandoff = func(target ports.AttachTarget) (ports.Dialer, client.AttachRequest, error) { | ||
| @@ -556,6 +564,9 @@ func TestRouteNavigationPreservesRemoteHomePickerAcrossLocalReturn(t *testing.T) | |||||
| 556 | require.NoError(t, err) | 564 | require.NoError(t, err) | ||
| 557 | 565 | ||||
| 558 | require.Equal(t, ports.NavigationCapabilityHomePicker, remoteHello.NavigationCapabilities) | 566 | require.Equal(t, ports.NavigationCapabilityHomePicker, remoteHello.NavigationCapabilities) | ||
| 567 | + | remoteNewHello := helloFromSend(t, remote3) | |||
| 568 | + | require.Equal(t, "remote-new", remoteNewHello.Name) | |||
| 569 | + | require.Equal(t, &ports.ExactSessionTarget{LifecycleID: remoteNewLifecycle, SessionName: "remote-new"}, remoteNewHello.ExactTarget) | |||
| 559 | } | 570 | } | ||
| 560 | 571 | ||||
| 561 | func mustMarshalCommittedIdentity(identity ports.CommittedRouteIdentity) []byte { | 572 | func mustMarshalCommittedIdentity(identity ports.CommittedRouteIdentity) []byte { | ||
M internal/usecase/daemon/attachment_route_identity_test.go +49 -0
| @@ -1,6 +1,7 @@ | |||
| 1 | 1 | package daemon | |
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | + | "errors" | |
| 4 | 5 | "testing" | |
| 5 | 6 | ||
| 6 | 7 | "github.com/bnema/vev/internal/domain" | |
| @@ -8,6 +9,17 @@ import ( | |||
| 8 | 9 | "github.com/stretchr/testify/require" | |
| 9 | 10 | ) | |
| 10 | 11 | ||
| 12 | + | type committedIdentityErrorTransport struct { | |
| 13 | + | closeTrackingTransport | |
| 14 | + | } | |
| 15 | + | ||
| 16 | + | func (t *committedIdentityErrorTransport) Send(frame ports.Frame) error { | |
| 17 | + | if frame.Type == ports.MsgCommittedRouteIdentity { | |
| 18 | + | return errors.New("committed identity send failed") | |
| 19 | + | } | |
| 20 | + | return t.closeTrackingTransport.Send(frame) | |
| 21 | + | } | |
| 22 | + | ||
| 11 | 23 | func TestAttachmentTransitionPublishesCommittedRouteIdentity(t *testing.T) { | |
| 12 | 24 | d, source, ac, _ := newManualSessionWithPTYs(t, nil) | |
| 13 | 25 | transport := &closeTrackingTransport{} | |
| @@ -57,3 +69,40 @@ func TestAttachmentTransitionPublishesCommittedRouteIdentity(t *testing.T) { | |||
| 57 | 69 | require.Equal(t, "target", identity.Target.SessionName) | |
| 58 | 70 | require.Equal(t, domain.IncarnationID{2}, identity.Target.LifecycleID) | |
| 59 | 71 | } | |
| 72 | + | ||
| 73 | + | func TestAttachmentTransitionTearsDownAfterCommittedIdentitySendFailure(t *testing.T) { | |
| 74 | + | d, source, ac, _ := newManualSessionWithPTYs(t, nil) | |
| 75 | + | transport := &committedIdentityErrorTransport{} | |
| 76 | + | ac.replaceTransport(transport) | |
| 77 | + | rc := d.attachCoordinator(source, nil, ac, true) | |
| 78 | + | token := source.attachmentToken(ac, transport) | |
| 79 | + | token.lease = rc.attachmentLease(ac) | |
| 80 | + | ac.publishAttachmentCapability(token) | |
| 81 | + | ac.setRouteSnapshot(ports.RecentRouteSnapshot{Generation: 1}) | |
| 82 | + | ||
| 83 | + | target := &session{ | |
| 84 | + | sessionCore: sessionCore{ | |
| 85 | + | id: "target", | |
| 86 | + | name: "target", | |
| 87 | + | incarnation: domain.IncarnationID{2}, | |
| 88 | + | }, | |
| 89 | + | ctx: source.ctx, | |
| 90 | + | cancel: func() {}, | |
| 91 | + | tabs: []*tab{newTab(nil, domain.Size{Cols: 80, Rows: 23})}, | |
| 92 | + | } | |
| 93 | + | d.mu.Lock() | |
| 94 | + | d.sessions[target.id] = target | |
| 95 | + | d.mu.Unlock() | |
| 96 | + | ||
| 97 | + | _, err := d.transitionAttachment(attachmentTransitionRequest{ | |
| 98 | + | source: source, | |
| 99 | + | target: target, | |
| 100 | + | next: ac, | |
| 101 | + | expectedTransport: transportSnapshot{transport: transport, incarnation: ac.transportSnapshot().incarnation}, | |
| 102 | + | ready: true, | |
| 103 | + | }) | |
| 104 | + | require.ErrorContains(t, err, "committed identity send failed") | |
| 105 | + | require.Empty(t, target.snapshotAttachments()) | |
| 106 | + | require.Empty(t, source.snapshotAttachments()) | |
| 107 | + | require.True(t, transport.Closed()) | |
| 108 | + | } | |
| @@ -1,6 +1,7 @@ | |||||
| 1 | package daemon | 1 | package daemon | ||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | + | "errors" | |||
| 4 | "testing" | 5 | "testing" | ||
| 5 | 6 | ||||
| 6 | "github.com/bnema/vev/internal/domain" | 7 | "github.com/bnema/vev/internal/domain" | ||
| @@ -8,6 +9,17 @@ import ( | |||||
| 8 | "github.com/stretchr/testify/require" | 9 | "github.com/stretchr/testify/require" | ||
| 9 | ) | 10 | ) | ||
| 10 | 11 | ||||
| 12 | + | type committedIdentityErrorTransport struct { | |||
| 13 | + | closeTrackingTransport | |||
| 14 | + | } | |||
| 15 | + | ||||
| 16 | + | func (t *committedIdentityErrorTransport) Send(frame ports.Frame) error { | |||
| 17 | + | if frame.Type == ports.MsgCommittedRouteIdentity { | |||
| 18 | + | return errors.New("committed identity send failed") | |||
| 19 | + | } | |||
| 20 | + | return t.closeTrackingTransport.Send(frame) | |||
| 21 | + | } | |||
| 22 | + | ||||
| 11 | func TestAttachmentTransitionPublishesCommittedRouteIdentity(t *testing.T) { | 23 | func TestAttachmentTransitionPublishesCommittedRouteIdentity(t *testing.T) { | ||
| 12 | d, source, ac, _ := newManualSessionWithPTYs(t, nil) | 24 | d, source, ac, _ := newManualSessionWithPTYs(t, nil) | ||
| 13 | transport := &closeTrackingTransport{} | 25 | transport := &closeTrackingTransport{} | ||
| @@ -57,3 +69,40 @@ func TestAttachmentTransitionPublishesCommittedRouteIdentity(t *testing.T) { | |||||
| 57 | require.Equal(t, "target", identity.Target.SessionName) | 69 | require.Equal(t, "target", identity.Target.SessionName) | ||
| 58 | require.Equal(t, domain.IncarnationID{2}, identity.Target.LifecycleID) | 70 | require.Equal(t, domain.IncarnationID{2}, identity.Target.LifecycleID) | ||
| 59 | } | 71 | } | ||
| 72 | + | ||||
| 73 | + | func TestAttachmentTransitionTearsDownAfterCommittedIdentitySendFailure(t *testing.T) { | |||
| 74 | + | d, source, ac, _ := newManualSessionWithPTYs(t, nil) | |||
| 75 | + | transport := &committedIdentityErrorTransport{} | |||
| 76 | + | ac.replaceTransport(transport) | |||
| 77 | + | rc := d.attachCoordinator(source, nil, ac, true) | |||
| 78 | + | token := source.attachmentToken(ac, transport) | |||
| 79 | + | token.lease = rc.attachmentLease(ac) | |||
| 80 | + | ac.publishAttachmentCapability(token) | |||
| 81 | + | ac.setRouteSnapshot(ports.RecentRouteSnapshot{Generation: 1}) | |||
| 82 | + | ||||
| 83 | + | target := &session{ | |||
| 84 | + | sessionCore: sessionCore{ | |||
| 85 | + | id: "target", | |||
| 86 | + | name: "target", | |||
| 87 | + | incarnation: domain.IncarnationID{2}, | |||
| 88 | + | }, | |||
| 89 | + | ctx: source.ctx, | |||
| 90 | + | cancel: func() {}, | |||
| 91 | + | tabs: []*tab{newTab(nil, domain.Size{Cols: 80, Rows: 23})}, | |||
| 92 | + | } | |||
| 93 | + | d.mu.Lock() | |||
| 94 | + | d.sessions[target.id] = target | |||
| 95 | + | d.mu.Unlock() | |||
| 96 | + | ||||
| 97 | + | _, err := d.transitionAttachment(attachmentTransitionRequest{ | |||
| 98 | + | source: source, | |||
| 99 | + | target: target, | |||
| 100 | + | next: ac, | |||
| 101 | + | expectedTransport: transportSnapshot{transport: transport, incarnation: ac.transportSnapshot().incarnation}, | |||
| 102 | + | ready: true, | |||
| 103 | + | }) | |||
| 104 | + | require.ErrorContains(t, err, "committed identity send failed") | |||
| 105 | + | require.Empty(t, target.snapshotAttachments()) | |||
| 106 | + | require.Empty(t, source.snapshotAttachments()) | |||
| 107 | + | require.True(t, transport.Closed()) | |||
| 108 | + | } | |||
M internal/usecase/daemon/attachment_transition.go +12 -1
| @@ -184,7 +184,8 @@ func (d *Daemon) transitionAttachment(req attachmentTransitionRequest) (attachme | |||
| 184 | 184 | effect.End() | |
| 185 | 185 | } | |
| 186 | 186 | if identityErr != nil { | |
| 187 | - | d.log.Warn("publishing committed route identity failed", "err", identityErr, "action", req.action) | |
| 187 | + | d.abortPublishedAttachmentTransition(result) | |
| 188 | + | return attachmentTransitionResult{}, identityErr | |
| 188 | 189 | } | |
| 189 | 190 | } | |
| 190 | 191 | if result.sourceGeometrySession != nil { | |
| @@ -193,6 +194,16 @@ func (d *Daemon) transitionAttachment(req attachmentTransitionRequest) (attachme | |||
| 193 | 194 | return result, nil | |
| 194 | 195 | } | |
| 195 | 196 | ||
| 197 | + | // abortPublishedAttachmentTransition tears down the newly published link before | |
| 198 | + | // a post-publication control failure escapes to callers. The transition result is | |
| 199 | + | // deliberately not handed back because its target membership is no longer valid. | |
| 200 | + | func (d *Daemon) abortPublishedAttachmentTransition(result attachmentTransitionResult) { | |
| 201 | + | if result.published.ac != nil && result.published.transport.transport != nil { | |
| 202 | + | d.clientGoneWithoutNotice(result.published.sess, result.published.ac, result.published.transport.transport, true) | |
| 203 | + | } | |
| 204 | + | d.deferAttachmentTransitionCleanups(result) | |
| 205 | + | } | |
| 206 | + | ||
| 196 | 207 | func (d *Daemon) deferAttachmentTransitionCleanups(result attachmentTransitionResult) { | |
| 197 | 208 | for _, cleanup := range result.cleanups { | |
| 198 | 209 | d.attachmentCleanupWg.Go(cleanup.finish) | |
| @@ -184,7 +184,8 @@ func (d *Daemon) transitionAttachment(req attachmentTransitionRequest) (attachme | |||||
| 184 | effect.End() | 184 | effect.End() | ||
| 185 | } | 185 | } | ||
| 186 | if identityErr != nil { | 186 | if identityErr != nil { | ||
| 187 | - | d.log.Warn("publishing committed route identity failed", "err", identityErr, "action", req.action) | 187 | + | d.abortPublishedAttachmentTransition(result) |
| 188 | + | return attachmentTransitionResult{}, identityErr | |||
| 188 | } | 189 | } | ||
| 189 | } | 190 | } | ||
| 190 | if result.sourceGeometrySession != nil { | 191 | if result.sourceGeometrySession != nil { | ||
| @@ -193,6 +194,16 @@ func (d *Daemon) transitionAttachment(req attachmentTransitionRequest) (attachme | |||||
| 193 | return result, nil | 194 | return result, nil | ||
| 194 | } | 195 | } | ||
| 195 | 196 | ||||
| 197 | + | // abortPublishedAttachmentTransition tears down the newly published link before | |||
| 198 | + | // a post-publication control failure escapes to callers. The transition result is | |||
| 199 | + | // deliberately not handed back because its target membership is no longer valid. | |||
| 200 | + | func (d *Daemon) abortPublishedAttachmentTransition(result attachmentTransitionResult) { | |||
| 201 | + | if result.published.ac != nil && result.published.transport.transport != nil { | |||
| 202 | + | d.clientGoneWithoutNotice(result.published.sess, result.published.ac, result.published.transport.transport, true) | |||
| 203 | + | } | |||
| 204 | + | d.deferAttachmentTransitionCleanups(result) | |||
| 205 | + | } | |||
| 206 | + | ||||
| 196 | func (d *Daemon) deferAttachmentTransitionCleanups(result attachmentTransitionResult) { | 207 | func (d *Daemon) deferAttachmentTransitionCleanups(result attachmentTransitionResult) { | ||
| 197 | for _, cleanup := range result.cleanups { | 208 | for _, cleanup := range result.cleanups { | ||
| 198 | d.attachmentCleanupWg.Go(cleanup.finish) | 209 | d.attachmentCleanupWg.Go(cleanup.finish) | ||
M internal/usecase/daemon/daemon.go +3 -0
| @@ -1403,6 +1403,9 @@ func (d *Daemon) routeWithContext(ctx context.Context, h ports.Hello, tr ports.T | |||
| 1403 | 1403 | if h.Intent == ports.IntentResume && h.ResumeToken == 0 { | |
| 1404 | 1404 | return nil, nil, &protoErr{ports.ErrNoSuchSession, "resume token is required"} | |
| 1405 | 1405 | } | |
| 1406 | + | if h.ExactTarget != nil && h.ResumeToken == 0 && h.Name != h.ExactTarget.SessionName { | |
| 1407 | + | return nil, nil, &protoErr{ports.ErrNoSuchSession, "exact session target name mismatch"} | |
| 1408 | + | } | |
| 1406 | 1409 | if h.RemoteTarget != nil && h.ResumeToken == 0 { | |
| 1407 | 1410 | return d.routeRemoteTargetWithContext(ctx, h, tr) | |
| 1408 | 1411 | } | |
| @@ -1403,6 +1403,9 @@ func (d *Daemon) routeWithContext(ctx context.Context, h ports.Hello, tr ports.T | |||||
| 1403 | if h.Intent == ports.IntentResume && h.ResumeToken == 0 { | 1403 | if h.Intent == ports.IntentResume && h.ResumeToken == 0 { | ||
| 1404 | return nil, nil, &protoErr{ports.ErrNoSuchSession, "resume token is required"} | 1404 | return nil, nil, &protoErr{ports.ErrNoSuchSession, "resume token is required"} | ||
| 1405 | } | 1405 | } | ||
| 1406 | + | if h.ExactTarget != nil && h.ResumeToken == 0 && h.Name != h.ExactTarget.SessionName { | |||
| 1407 | + | return nil, nil, &protoErr{ports.ErrNoSuchSession, "exact session target name mismatch"} | |||
| 1408 | + | } | |||
| 1406 | if h.RemoteTarget != nil && h.ResumeToken == 0 { | 1409 | if h.RemoteTarget != nil && h.ResumeToken == 0 { | ||
| 1407 | return d.routeRemoteTargetWithContext(ctx, h, tr) | 1410 | return d.routeRemoteTargetWithContext(ctx, h, tr) | ||
| 1408 | } | 1411 | } | ||
M internal/usecase/daemon/exact_route_test.go +20 -0
| @@ -31,6 +31,26 @@ func TestRouteExactSessionTargetSelectsLifecycle(t *testing.T) { | |||
| 31 | 31 | d.clientGone(sess, ac, tr, false) | |
| 32 | 32 | } | |
| 33 | 33 | ||
| 34 | + | func TestRouteExactSessionTargetRejectsNameMismatch(t *testing.T) { | |
| 35 | + | d := newTestDaemon(t, nil, stubClock{}) | |
| 36 | + | sess := addControlSession(d, "work", "tab-1", "pane-1") | |
| 37 | + | sess.ephemeral = false | |
| 38 | + | sess.incarnation = remoteLifecycleForTest() | |
| 39 | + | tr, _ := newCapturingTransport(t) | |
| 40 | + | hello := ports.Hello{ | |
| 41 | + | Version: ports.ProtocolVersion, | |
| 42 | + | Intent: ports.IntentAttach, | |
| 43 | + | Name: "other", | |
| 44 | + | Size: domain.Size{Cols: 80, Rows: 24}, | |
| 45 | + | ExactTarget: &ports.ExactSessionTarget{LifecycleID: sess.incarnation, SessionName: sess.name}, | |
| 46 | + | } | |
| 47 | + | ||
| 48 | + | _, _, err := d.routeWithContext(context.Background(), hello, tr) | |
| 49 | + | var protocol *protoErr | |
| 50 | + | require.ErrorAs(t, err, &protocol) | |
| 51 | + | require.Equal(t, ports.ErrNoSuchSession, protocol.code) | |
| 52 | + | } | |
| 53 | + | ||
| 34 | 54 | func TestLockedExactSessionTargetRejectsReplacement(t *testing.T) { | |
| 35 | 55 | d := newTestDaemon(t, nil, stubClock{}) | |
| 36 | 56 | sess := addControlSession(d, "work", "tab-1", "pane-1") | |
| @@ -31,6 +31,26 @@ func TestRouteExactSessionTargetSelectsLifecycle(t *testing.T) { | |||||
| 31 | d.clientGone(sess, ac, tr, false) | 31 | d.clientGone(sess, ac, tr, false) | ||
| 32 | } | 32 | } | ||
| 33 | 33 | ||||
| 34 | + | func TestRouteExactSessionTargetRejectsNameMismatch(t *testing.T) { | |||
| 35 | + | d := newTestDaemon(t, nil, stubClock{}) | |||
| 36 | + | sess := addControlSession(d, "work", "tab-1", "pane-1") | |||
| 37 | + | sess.ephemeral = false | |||
| 38 | + | sess.incarnation = remoteLifecycleForTest() | |||
| 39 | + | tr, _ := newCapturingTransport(t) | |||
| 40 | + | hello := ports.Hello{ | |||
| 41 | + | Version: ports.ProtocolVersion, | |||
| 42 | + | Intent: ports.IntentAttach, | |||
| 43 | + | Name: "other", | |||
| 44 | + | Size: domain.Size{Cols: 80, Rows: 24}, | |||
| 45 | + | ExactTarget: &ports.ExactSessionTarget{LifecycleID: sess.incarnation, SessionName: sess.name}, | |||
| 46 | + | } | |||
| 47 | + | ||||
| 48 | + | _, _, err := d.routeWithContext(context.Background(), hello, tr) | |||
| 49 | + | var protocol *protoErr | |||
| 50 | + | require.ErrorAs(t, err, &protocol) | |||
| 51 | + | require.Equal(t, ports.ErrNoSuchSession, protocol.code) | |||
| 52 | + | } | |||
| 53 | + | ||||
| 34 | func TestLockedExactSessionTargetRejectsReplacement(t *testing.T) { | 54 | func TestLockedExactSessionTargetRejectsReplacement(t *testing.T) { | ||
| 35 | d := newTestDaemon(t, nil, stubClock{}) | 55 | d := newTestDaemon(t, nil, stubClock{}) | ||
| 36 | sess := addControlSession(d, "work", "tab-1", "pane-1") | 56 | sess := addControlSession(d, "work", "tab-1", "pane-1") | ||