fix(daemon): bound concurrent teardown waits
- commit
- f00a00d64a748305734e339f888246c1492a4130
- author
- brice · · 2026-07-24 16:33 UTC
- parent
- be1bb36
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +193 -26
M internal/usecase/daemon/session.go +91 -6
| @@ -41,10 +41,16 @@ type session struct { | |||
| 41 | 41 | // resolve focus against one state and mutate a later one. Lock order: | |
| 42 | 42 | // dispatchMu strictly before d.mu, sess.mu, or any tab mu. | |
| 43 | 43 | dispatchMu sync.Mutex | |
| 44 | - | // teardownMu is the outermost per-session lifecycle lock. It serializes | |
| 45 | - | // competing pane-exit, control, and daemon-shutdown teardown paths before | |
| 46 | - | // they acquire daemon, session, snapshot, or pane locks. | |
| 44 | + | // teardownMu guards teardown ownership and is the outermost per-session | |
| 45 | + | // lifecycle lock: never acquire it while holding daemon, session, snapshot, | |
| 46 | + | // tab, or pane locks. The owner performs teardown without holding teardownMu; | |
| 47 | + | // duplicate daemon-shutdown callers can therefore bound their ownership wait | |
| 48 | + | // with Serve's shared snapshot deadline. | |
| 47 | 49 | teardownMu sync.Mutex | |
| 50 | + | teardownActive bool | |
| 51 | + | teardownDone chan struct{} | |
| 52 | + | teardownWaiters uint | |
| 53 | + | teardownChanged chan struct{} | |
| 48 | 54 | mu sync.Mutex // guards tabs, active, client, clipFiles, and clipboard queue state | |
| 49 | 55 | themeMu sync.Mutex | |
| 50 | 56 | tabs []*tab | |
| @@ -907,6 +913,82 @@ func (d *Daemon) killSession(sess *session, reason uint8, purge bool) error { | |||
| 907 | 913 | return d.killSessionWithSnapshotDeadline(sess, reason, purge, nil) | |
| 908 | 914 | } | |
| 909 | 915 | ||
| 916 | + | func (s *session) teardownChangeLocked() chan struct{} { | |
| 917 | + | if s.teardownChanged == nil { | |
| 918 | + | s.teardownChanged = make(chan struct{}) | |
| 919 | + | } | |
| 920 | + | return s.teardownChanged | |
| 921 | + | } | |
| 922 | + | ||
| 923 | + | func (s *session) signalTeardownChangedLocked() { | |
| 924 | + | close(s.teardownChangeLocked()) | |
| 925 | + | s.teardownChanged = make(chan struct{}) | |
| 926 | + | } | |
| 927 | + | ||
| 928 | + | // beginTeardown waits for ownership without holding teardownMu across teardown. | |
| 929 | + | // A false result means Serve's shared shutdown budget expired while another | |
| 930 | + | // lifecycle path owned the session. Ordinary callers wait and retry ownership, | |
| 931 | + | // preserving failed-teardown and stopped-purge retry semantics. | |
| 932 | + | func (s *session) beginTeardown(deadline *snapshotShutdownDeadline) bool { | |
| 933 | + | for { | |
| 934 | + | if deadline != nil { | |
| 935 | + | select { | |
| 936 | + | case <-deadline.Done(): | |
| 937 | + | return false | |
| 938 | + | default: | |
| 939 | + | } | |
| 940 | + | } | |
| 941 | + | ||
| 942 | + | s.teardownMu.Lock() | |
| 943 | + | if !s.teardownActive { | |
| 944 | + | if deadline != nil { | |
| 945 | + | select { | |
| 946 | + | case <-deadline.Done(): | |
| 947 | + | s.teardownMu.Unlock() | |
| 948 | + | return false | |
| 949 | + | default: | |
| 950 | + | } | |
| 951 | + | } | |
| 952 | + | s.teardownActive = true | |
| 953 | + | s.teardownDone = make(chan struct{}) | |
| 954 | + | s.signalTeardownChangedLocked() | |
| 955 | + | s.teardownMu.Unlock() | |
| 956 | + | return true | |
| 957 | + | } | |
| 958 | + | done := s.teardownDone | |
| 959 | + | s.teardownWaiters++ | |
| 960 | + | s.signalTeardownChangedLocked() | |
| 961 | + | s.teardownMu.Unlock() | |
| 962 | + | ||
| 963 | + | timedOut := false | |
| 964 | + | if deadline == nil { | |
| 965 | + | <-done | |
| 966 | + | } else { | |
| 967 | + | select { | |
| 968 | + | case <-done: | |
| 969 | + | case <-deadline.Done(): | |
| 970 | + | timedOut = true | |
| 971 | + | } | |
| 972 | + | } | |
| 973 | + | s.teardownMu.Lock() | |
| 974 | + | s.teardownWaiters-- | |
| 975 | + | s.signalTeardownChangedLocked() | |
| 976 | + | s.teardownMu.Unlock() | |
| 977 | + | if timedOut { | |
| 978 | + | return false | |
| 979 | + | } | |
| 980 | + | } | |
| 981 | + | } | |
| 982 | + | ||
| 983 | + | func (s *session) finishTeardown() { | |
| 984 | + | s.teardownMu.Lock() | |
| 985 | + | s.teardownActive = false | |
| 986 | + | close(s.teardownDone) | |
| 987 | + | s.teardownDone = nil | |
| 988 | + | s.signalTeardownChangedLocked() | |
| 989 | + | s.teardownMu.Unlock() | |
| 990 | + | } | |
| 991 | + | ||
| 910 | 992 | // killSessionWithSnapshotDeadline shares Serve's shutdown budget with its | |
| 911 | 993 | // terminal checkpoint and coordinator join. A timed-out repository call keeps | |
| 912 | 994 | // only immutable capture state; it never observes closed worker channels or | |
| @@ -918,9 +1000,12 @@ func (d *Daemon) killSessionWithSnapshotDeadline(sess *session, reason uint8, pu | |||
| 918 | 1000 | // Pane EOF, explicit close, and daemon shutdown can all converge on the same | |
| 919 | 1001 | // session. Only one owner may create the terminal mutation and checkpoint; | |
| 920 | 1002 | // later callers observe the completed registry transition instead of | |
| 921 | - | // manufacturing a newer generation that the owner will quarantine. | |
| 922 | - | sess.teardownMu.Lock() | |
| 923 | - | defer sess.teardownMu.Unlock() | |
| 1003 | + | // manufacturing a newer generation that the owner will quarantine. A | |
| 1004 | + | // shutdown duplicate abandons this wait when the shared budget expires. | |
| 1005 | + | if !sess.beginTeardown(deadline) { | |
| 1006 | + | return nil | |
| 1007 | + | } | |
| 1008 | + | defer sess.finishTeardown() | |
| 924 | 1009 | ||
| 925 | 1010 | d.mu.Lock() | |
| 926 | 1011 | current := d.sessions[sess.id] | |
| @@ -41,10 +41,16 @@ type session struct { | |||||
| 41 | // resolve focus against one state and mutate a later one. Lock order: | 41 | // resolve focus against one state and mutate a later one. Lock order: | ||
| 42 | // dispatchMu strictly before d.mu, sess.mu, or any tab mu. | 42 | // dispatchMu strictly before d.mu, sess.mu, or any tab mu. | ||
| 43 | dispatchMu sync.Mutex | 43 | dispatchMu sync.Mutex | ||
| 44 | - | // teardownMu is the outermost per-session lifecycle lock. It serializes | 44 | + | // teardownMu guards teardown ownership and is the outermost per-session |
| 45 | - | // competing pane-exit, control, and daemon-shutdown teardown paths before | 45 | + | // lifecycle lock: never acquire it while holding daemon, session, snapshot, |
| 46 | - | // they acquire daemon, session, snapshot, or pane locks. | 46 | + | // tab, or pane locks. The owner performs teardown without holding teardownMu; |
| 47 | + | // duplicate daemon-shutdown callers can therefore bound their ownership wait | |||
| 48 | + | // with Serve's shared snapshot deadline. | |||
| 47 | teardownMu sync.Mutex | 49 | teardownMu sync.Mutex | ||
| 50 | + | teardownActive bool | |||
| 51 | + | teardownDone chan struct{} | |||
| 52 | + | teardownWaiters uint | |||
| 53 | + | teardownChanged chan struct{} | |||
| 48 | mu sync.Mutex // guards tabs, active, client, clipFiles, and clipboard queue state | 54 | mu sync.Mutex // guards tabs, active, client, clipFiles, and clipboard queue state | ||
| 49 | themeMu sync.Mutex | 55 | themeMu sync.Mutex | ||
| 50 | tabs []*tab | 56 | tabs []*tab | ||
| @@ -907,6 +913,82 @@ func (d *Daemon) killSession(sess *session, reason uint8, purge bool) error { | |||||
| 907 | return d.killSessionWithSnapshotDeadline(sess, reason, purge, nil) | 913 | return d.killSessionWithSnapshotDeadline(sess, reason, purge, nil) | ||
| 908 | } | 914 | } | ||
| 909 | 915 | ||||
| 916 | + | func (s *session) teardownChangeLocked() chan struct{} { | |||
| 917 | + | if s.teardownChanged == nil { | |||
| 918 | + | s.teardownChanged = make(chan struct{}) | |||
| 919 | + | } | |||
| 920 | + | return s.teardownChanged | |||
| 921 | + | } | |||
| 922 | + | ||||
| 923 | + | func (s *session) signalTeardownChangedLocked() { | |||
| 924 | + | close(s.teardownChangeLocked()) | |||
| 925 | + | s.teardownChanged = make(chan struct{}) | |||
| 926 | + | } | |||
| 927 | + | ||||
| 928 | + | // beginTeardown waits for ownership without holding teardownMu across teardown. | |||
| 929 | + | // A false result means Serve's shared shutdown budget expired while another | |||
| 930 | + | // lifecycle path owned the session. Ordinary callers wait and retry ownership, | |||
| 931 | + | // preserving failed-teardown and stopped-purge retry semantics. | |||
| 932 | + | func (s *session) beginTeardown(deadline *snapshotShutdownDeadline) bool { | |||
| 933 | + | for { | |||
| 934 | + | if deadline != nil { | |||
| 935 | + | select { | |||
| 936 | + | case <-deadline.Done(): | |||
| 937 | + | return false | |||
| 938 | + | default: | |||
| 939 | + | } | |||
| 940 | + | } | |||
| 941 | + | ||||
| 942 | + | s.teardownMu.Lock() | |||
| 943 | + | if !s.teardownActive { | |||
| 944 | + | if deadline != nil { | |||
| 945 | + | select { | |||
| 946 | + | case <-deadline.Done(): | |||
| 947 | + | s.teardownMu.Unlock() | |||
| 948 | + | return false | |||
| 949 | + | default: | |||
| 950 | + | } | |||
| 951 | + | } | |||
| 952 | + | s.teardownActive = true | |||
| 953 | + | s.teardownDone = make(chan struct{}) | |||
| 954 | + | s.signalTeardownChangedLocked() | |||
| 955 | + | s.teardownMu.Unlock() | |||
| 956 | + | return true | |||
| 957 | + | } | |||
| 958 | + | done := s.teardownDone | |||
| 959 | + | s.teardownWaiters++ | |||
| 960 | + | s.signalTeardownChangedLocked() | |||
| 961 | + | s.teardownMu.Unlock() | |||
| 962 | + | ||||
| 963 | + | timedOut := false | |||
| 964 | + | if deadline == nil { | |||
| 965 | + | <-done | |||
| 966 | + | } else { | |||
| 967 | + | select { | |||
| 968 | + | case <-done: | |||
| 969 | + | case <-deadline.Done(): | |||
| 970 | + | timedOut = true | |||
| 971 | + | } | |||
| 972 | + | } | |||
| 973 | + | s.teardownMu.Lock() | |||
| 974 | + | s.teardownWaiters-- | |||
| 975 | + | s.signalTeardownChangedLocked() | |||
| 976 | + | s.teardownMu.Unlock() | |||
| 977 | + | if timedOut { | |||
| 978 | + | return false | |||
| 979 | + | } | |||
| 980 | + | } | |||
| 981 | + | } | |||
| 982 | + | ||||
| 983 | + | func (s *session) finishTeardown() { | |||
| 984 | + | s.teardownMu.Lock() | |||
| 985 | + | s.teardownActive = false | |||
| 986 | + | close(s.teardownDone) | |||
| 987 | + | s.teardownDone = nil | |||
| 988 | + | s.signalTeardownChangedLocked() | |||
| 989 | + | s.teardownMu.Unlock() | |||
| 990 | + | } | |||
| 991 | + | ||||
| 910 | // killSessionWithSnapshotDeadline shares Serve's shutdown budget with its | 992 | // killSessionWithSnapshotDeadline shares Serve's shutdown budget with its | ||
| 911 | // terminal checkpoint and coordinator join. A timed-out repository call keeps | 993 | // terminal checkpoint and coordinator join. A timed-out repository call keeps | ||
| 912 | // only immutable capture state; it never observes closed worker channels or | 994 | // only immutable capture state; it never observes closed worker channels or | ||
| @@ -918,9 +1000,12 @@ func (d *Daemon) killSessionWithSnapshotDeadline(sess *session, reason uint8, pu | |||||
| 918 | // Pane EOF, explicit close, and daemon shutdown can all converge on the same | 1000 | // Pane EOF, explicit close, and daemon shutdown can all converge on the same | ||
| 919 | // session. Only one owner may create the terminal mutation and checkpoint; | 1001 | // session. Only one owner may create the terminal mutation and checkpoint; | ||
| 920 | // later callers observe the completed registry transition instead of | 1002 | // later callers observe the completed registry transition instead of | ||
| 921 | - | // manufacturing a newer generation that the owner will quarantine. | 1003 | + | // manufacturing a newer generation that the owner will quarantine. A |
| 922 | - | sess.teardownMu.Lock() | 1004 | + | // shutdown duplicate abandons this wait when the shared budget expires. |
| 923 | - | defer sess.teardownMu.Unlock() | 1005 | + | if !sess.beginTeardown(deadline) { |
| 1006 | + | return nil | |||
| 1007 | + | } | |||
| 1008 | + | defer sess.finishTeardown() | |||
| 924 | 1009 | ||||
| 925 | d.mu.Lock() | 1010 | d.mu.Lock() | ||
| 926 | current := d.sessions[sess.id] | 1011 | current := d.sessions[sess.id] | ||
M internal/usecase/daemon/snapshot_scheduler_test.go +102 -20
| @@ -4,7 +4,6 @@ import ( | |||
| 4 | 4 | "context" | |
| 5 | 5 | "errors" | |
| 6 | 6 | "io" | |
| 7 | - | "runtime" | |
| 8 | 7 | "sync" | |
| 9 | 8 | "sync/atomic" | |
| 10 | 9 | "testing" | |
| @@ -443,6 +442,85 @@ func TestForcedSnapshotShutdownTimeoutRetainsRetryableStateAndNotice(t *testing. | |||
| 443 | 442 | } | |
| 444 | 443 | } | |
| 445 | 444 | ||
| 445 | + | func TestShutdownDeadlineBoundsWaitForExistingTeardownOwner(t *testing.T) { | |
| 446 | + | clock := newSnapshotDeadlineClock() | |
| 447 | + | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), clock) | |
| 448 | + | repository := portsmocks.NewMockSnapshotRepository(t) | |
| 449 | + | WithSnapshotRepository(repository, nil)(d) | |
| 450 | + | startSnapshotEncodeWorker(t, d) | |
| 451 | + | ||
| 452 | + | publicationStarted := make(chan struct{}) | |
| 453 | + | releasePublication := make(chan struct{}) | |
| 454 | + | var releaseOnce sync.Once | |
| 455 | + | release := func() { releaseOnce.Do(func() { close(releasePublication) }) } | |
| 456 | + | t.Cleanup(release) | |
| 457 | + | repository.EXPECT().Publish(mock.Anything, mock.Anything).RunAndReturn(func(_ context.Context, _ ports.SnapshotPublication) error { | |
| 458 | + | close(publicationStarted) | |
| 459 | + | <-releasePublication | |
| 460 | + | return nil | |
| 461 | + | }).Once() | |
| 462 | + | ||
| 463 | + | sess := newSnapshotTestSession(t, "work", false, "/work") | |
| 464 | + | pty, ok := sess.tabs[0].panes["pane-1"].pty.(*portsmocks.MockPTY) | |
| 465 | + | require.True(t, ok) | |
| 466 | + | pty.EXPECT().Close().Return(nil).Maybe() | |
| 467 | + | d.sessions[sess.id] = sess | |
| 468 | + | ||
| 469 | + | ordinaryDone := make(chan error, 1) | |
| 470 | + | go func() { ordinaryDone <- d.killSession(sess, ports.ReasonSessionKilled, false) }() | |
| 471 | + | select { | |
| 472 | + | case <-publicationStarted: | |
| 473 | + | case <-time.After(testWaitTimeout): | |
| 474 | + | t.Fatal("ordinary teardown did not become owner") | |
| 475 | + | } | |
| 476 | + | ||
| 477 | + | // The ordinary teardown owns its own final-snapshot deadline. Leave it | |
| 478 | + | // unfired so it remains distinct from Serve's shared shutdown budget. | |
| 479 | + | _ = clock.nextTimer(t) | |
| 480 | + | deadline := newSnapshotShutdownDeadline(clock) | |
| 481 | + | defer deadline.stop() | |
| 482 | + | shutdownDeadlineTimer := clock.nextTimer(t) | |
| 483 | + | shutdownDone := make(chan error, 1) | |
| 484 | + | go func() { | |
| 485 | + | shutdownDone <- d.killSessionWithSnapshotDeadline(sess, ports.ReasonServerShutdown, false, deadline) | |
| 486 | + | }() | |
| 487 | + | awaitTeardownWaiters(t, sess, 1) | |
| 488 | + | shutdownDeadlineTimer.fire() | |
| 489 | + | ||
| 490 | + | select { | |
| 491 | + | case err := <-shutdownDone: | |
| 492 | + | require.NoError(t, err) | |
| 493 | + | case <-time.After(testWaitTimeout): | |
| 494 | + | t.Fatal("daemon shutdown waited beyond its shared deadline for the ordinary teardown owner") | |
| 495 | + | } | |
| 496 | + | ||
| 497 | + | release() | |
| 498 | + | select { | |
| 499 | + | case err := <-ordinaryDone: | |
| 500 | + | require.NoError(t, err) | |
| 501 | + | case <-time.After(testWaitTimeout): | |
| 502 | + | t.Fatal("ordinary teardown did not finish after its publication was released") | |
| 503 | + | } | |
| 504 | + | } | |
| 505 | + | ||
| 506 | + | func awaitTeardownWaiters(t *testing.T, sess *session, want uint) { | |
| 507 | + | t.Helper() | |
| 508 | + | for { | |
| 509 | + | sess.teardownMu.Lock() | |
| 510 | + | waiters := sess.teardownWaiters | |
| 511 | + | changed := sess.teardownChangeLocked() | |
| 512 | + | sess.teardownMu.Unlock() | |
| 513 | + | if waiters == want { | |
| 514 | + | return | |
| 515 | + | } | |
| 516 | + | select { | |
| 517 | + | case <-changed: | |
| 518 | + | case <-time.After(testWaitTimeout): | |
| 519 | + | t.Fatalf("teardown ownership waiters = %d, want %d", waiters, want) | |
| 520 | + | } | |
| 521 | + | } | |
| 522 | + | } | |
| 523 | + | ||
| 446 | 524 | func TestConcurrentSessionTeardownPublishesOneFinalSnapshot(t *testing.T) { | |
| 447 | 525 | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{}) | |
| 448 | 526 | repository := portsmocks.NewMockSnapshotRepository(t) | |
| @@ -466,34 +544,28 @@ func TestConcurrentSessionTeardownPublishesOneFinalSnapshot(t *testing.T) { | |||
| 466 | 544 | d.sessions[sess.id] = sess | |
| 467 | 545 | ||
| 468 | 546 | first := make(chan error, 1) | |
| 469 | - | go func() { first <- d.killSession(sess, ports.ReasonServerShutdown, false) }() | |
| 547 | + | go func() { first <- d.killSession(sess, ports.ReasonSessionKilled, false) }() | |
| 470 | 548 | select { | |
| 471 | 549 | case <-publicationStarted: | |
| 472 | 550 | case <-time.After(testWaitTimeout): | |
| 473 | - | t.Fatal("first terminal snapshot did not reach repository") | |
| 551 | + | t.Fatal("ordinary teardown did not publish its terminal snapshot") | |
| 474 | 552 | } | |
| 475 | 553 | ||
| 554 | + | sess.teardownMu.Lock() | |
| 555 | + | require.True(t, sess.teardownActive) | |
| 556 | + | require.NotNil(t, sess.teardownDone) | |
| 557 | + | sess.teardownMu.Unlock() | |
| 558 | + | ||
| 476 | 559 | const duplicateCallers = 8 | |
| 477 | - | started := make(chan struct{}, duplicateCallers) | |
| 478 | 560 | duplicates := make(chan error, duplicateCallers) | |
| 479 | 561 | for range duplicateCallers { | |
| 480 | 562 | go func() { | |
| 481 | - | started <- struct{}{} | |
| 482 | - | duplicates <- d.killSession(sess, ports.ReasonServerShutdown, false) | |
| 563 | + | duplicates <- d.killSession(sess, ports.ReasonSessionKilled, false) | |
| 483 | 564 | }() | |
| 484 | 565 | } | |
| 485 | - | for range duplicateCallers { | |
| 486 | - | <-started | |
| 487 | - | } | |
| 488 | - | for range 1000 { | |
| 489 | - | runtime.Gosched() | |
| 490 | - | sess.snapshotMu.Lock() | |
| 491 | - | generation := sess.snapshotGeneration | |
| 492 | - | sess.snapshotMu.Unlock() | |
| 493 | - | if generation > 1 { | |
| 494 | - | break | |
| 495 | - | } | |
| 496 | - | } | |
| 566 | + | // Synchronize on the ownership state itself: every duplicate is waiting | |
| 567 | + | // behind the distinct ordinary teardown before its publication is released. | |
| 568 | + | awaitTeardownWaiters(t, sess, duplicateCallers) | |
| 497 | 569 | ||
| 498 | 570 | sess.snapshotMu.Lock() | |
| 499 | 571 | generation := sess.snapshotGeneration | |
| @@ -501,9 +573,19 @@ func TestConcurrentSessionTeardownPublishesOneFinalSnapshot(t *testing.T) { | |||
| 501 | 573 | require.Equal(t, uint64(1), generation, "duplicate teardown callers must not create synthetic terminal generations") | |
| 502 | 574 | ||
| 503 | 575 | releaseOnce.Do(func() { close(releasePublication) }) | |
| 504 | - | require.NoError(t, <-first) | |
| 576 | + | select { | |
| 577 | + | case err := <-first: | |
| 578 | + | require.NoError(t, err) | |
| 579 | + | case <-time.After(testWaitTimeout): | |
| 580 | + | t.Fatal("ordinary teardown did not finish") | |
| 581 | + | } | |
| 505 | 582 | for range duplicateCallers { | |
| 506 | - | require.NoError(t, <-duplicates) | |
| 583 | + | select { | |
| 584 | + | case err := <-duplicates: | |
| 585 | + | require.NoError(t, err) | |
| 586 | + | case <-time.After(testWaitTimeout): | |
| 587 | + | t.Fatal("duplicate teardown did not observe the completed registry transition") | |
| 588 | + | } | |
| 507 | 589 | } | |
| 508 | 590 | sess.snapshotMu.Lock() | |
| 509 | 591 | generation = sess.snapshotGeneration | |
| @@ -4,7 +4,6 @@ import ( | |||||
| 4 | "context" | 4 | "context" | ||
| 5 | "errors" | 5 | "errors" | ||
| 6 | "io" | 6 | "io" | ||
| 7 | - | "runtime" | |||
| 8 | "sync" | 7 | "sync" | ||
| 9 | "sync/atomic" | 8 | "sync/atomic" | ||
| 10 | "testing" | 9 | "testing" | ||
| @@ -443,6 +442,85 @@ func TestForcedSnapshotShutdownTimeoutRetainsRetryableStateAndNotice(t *testing. | |||||
| 443 | } | 442 | } | ||
| 444 | } | 443 | } | ||
| 445 | 444 | ||||
| 445 | + | func TestShutdownDeadlineBoundsWaitForExistingTeardownOwner(t *testing.T) { | |||
| 446 | + | clock := newSnapshotDeadlineClock() | |||
| 447 | + | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), clock) | |||
| 448 | + | repository := portsmocks.NewMockSnapshotRepository(t) | |||
| 449 | + | WithSnapshotRepository(repository, nil)(d) | |||
| 450 | + | startSnapshotEncodeWorker(t, d) | |||
| 451 | + | ||||
| 452 | + | publicationStarted := make(chan struct{}) | |||
| 453 | + | releasePublication := make(chan struct{}) | |||
| 454 | + | var releaseOnce sync.Once | |||
| 455 | + | release := func() { releaseOnce.Do(func() { close(releasePublication) }) } | |||
| 456 | + | t.Cleanup(release) | |||
| 457 | + | repository.EXPECT().Publish(mock.Anything, mock.Anything).RunAndReturn(func(_ context.Context, _ ports.SnapshotPublication) error { | |||
| 458 | + | close(publicationStarted) | |||
| 459 | + | <-releasePublication | |||
| 460 | + | return nil | |||
| 461 | + | }).Once() | |||
| 462 | + | ||||
| 463 | + | sess := newSnapshotTestSession(t, "work", false, "/work") | |||
| 464 | + | pty, ok := sess.tabs[0].panes["pane-1"].pty.(*portsmocks.MockPTY) | |||
| 465 | + | require.True(t, ok) | |||
| 466 | + | pty.EXPECT().Close().Return(nil).Maybe() | |||
| 467 | + | d.sessions[sess.id] = sess | |||
| 468 | + | ||||
| 469 | + | ordinaryDone := make(chan error, 1) | |||
| 470 | + | go func() { ordinaryDone <- d.killSession(sess, ports.ReasonSessionKilled, false) }() | |||
| 471 | + | select { | |||
| 472 | + | case <-publicationStarted: | |||
| 473 | + | case <-time.After(testWaitTimeout): | |||
| 474 | + | t.Fatal("ordinary teardown did not become owner") | |||
| 475 | + | } | |||
| 476 | + | ||||
| 477 | + | // The ordinary teardown owns its own final-snapshot deadline. Leave it | |||
| 478 | + | // unfired so it remains distinct from Serve's shared shutdown budget. | |||
| 479 | + | _ = clock.nextTimer(t) | |||
| 480 | + | deadline := newSnapshotShutdownDeadline(clock) | |||
| 481 | + | defer deadline.stop() | |||
| 482 | + | shutdownDeadlineTimer := clock.nextTimer(t) | |||
| 483 | + | shutdownDone := make(chan error, 1) | |||
| 484 | + | go func() { | |||
| 485 | + | shutdownDone <- d.killSessionWithSnapshotDeadline(sess, ports.ReasonServerShutdown, false, deadline) | |||
| 486 | + | }() | |||
| 487 | + | awaitTeardownWaiters(t, sess, 1) | |||
| 488 | + | shutdownDeadlineTimer.fire() | |||
| 489 | + | ||||
| 490 | + | select { | |||
| 491 | + | case err := <-shutdownDone: | |||
| 492 | + | require.NoError(t, err) | |||
| 493 | + | case <-time.After(testWaitTimeout): | |||
| 494 | + | t.Fatal("daemon shutdown waited beyond its shared deadline for the ordinary teardown owner") | |||
| 495 | + | } | |||
| 496 | + | ||||
| 497 | + | release() | |||
| 498 | + | select { | |||
| 499 | + | case err := <-ordinaryDone: | |||
| 500 | + | require.NoError(t, err) | |||
| 501 | + | case <-time.After(testWaitTimeout): | |||
| 502 | + | t.Fatal("ordinary teardown did not finish after its publication was released") | |||
| 503 | + | } | |||
| 504 | + | } | |||
| 505 | + | ||||
| 506 | + | func awaitTeardownWaiters(t *testing.T, sess *session, want uint) { | |||
| 507 | + | t.Helper() | |||
| 508 | + | for { | |||
| 509 | + | sess.teardownMu.Lock() | |||
| 510 | + | waiters := sess.teardownWaiters | |||
| 511 | + | changed := sess.teardownChangeLocked() | |||
| 512 | + | sess.teardownMu.Unlock() | |||
| 513 | + | if waiters == want { | |||
| 514 | + | return | |||
| 515 | + | } | |||
| 516 | + | select { | |||
| 517 | + | case <-changed: | |||
| 518 | + | case <-time.After(testWaitTimeout): | |||
| 519 | + | t.Fatalf("teardown ownership waiters = %d, want %d", waiters, want) | |||
| 520 | + | } | |||
| 521 | + | } | |||
| 522 | + | } | |||
| 523 | + | ||||
| 446 | func TestConcurrentSessionTeardownPublishesOneFinalSnapshot(t *testing.T) { | 524 | func TestConcurrentSessionTeardownPublishesOneFinalSnapshot(t *testing.T) { | ||
| 447 | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{}) | 525 | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{}) | ||
| 448 | repository := portsmocks.NewMockSnapshotRepository(t) | 526 | repository := portsmocks.NewMockSnapshotRepository(t) | ||
| @@ -466,34 +544,28 @@ func TestConcurrentSessionTeardownPublishesOneFinalSnapshot(t *testing.T) { | |||||
| 466 | d.sessions[sess.id] = sess | 544 | d.sessions[sess.id] = sess | ||
| 467 | 545 | ||||
| 468 | first := make(chan error, 1) | 546 | first := make(chan error, 1) | ||
| 469 | - | go func() { first <- d.killSession(sess, ports.ReasonServerShutdown, false) }() | 547 | + | go func() { first <- d.killSession(sess, ports.ReasonSessionKilled, false) }() |
| 470 | select { | 548 | select { | ||
| 471 | case <-publicationStarted: | 549 | case <-publicationStarted: | ||
| 472 | case <-time.After(testWaitTimeout): | 550 | case <-time.After(testWaitTimeout): | ||
| 473 | - | t.Fatal("first terminal snapshot did not reach repository") | 551 | + | t.Fatal("ordinary teardown did not publish its terminal snapshot") |
| 474 | } | 552 | } | ||
| 475 | 553 | ||||
| 554 | + | sess.teardownMu.Lock() | |||
| 555 | + | require.True(t, sess.teardownActive) | |||
| 556 | + | require.NotNil(t, sess.teardownDone) | |||
| 557 | + | sess.teardownMu.Unlock() | |||
| 558 | + | ||||
| 476 | const duplicateCallers = 8 | 559 | const duplicateCallers = 8 | ||
| 477 | - | started := make(chan struct{}, duplicateCallers) | |||
| 478 | duplicates := make(chan error, duplicateCallers) | 560 | duplicates := make(chan error, duplicateCallers) | ||
| 479 | for range duplicateCallers { | 561 | for range duplicateCallers { | ||
| 480 | go func() { | 562 | go func() { | ||
| 481 | - | started <- struct{}{} | 563 | + | duplicates <- d.killSession(sess, ports.ReasonSessionKilled, false) |
| 482 | - | duplicates <- d.killSession(sess, ports.ReasonServerShutdown, false) | |||
| 483 | }() | 564 | }() | ||
| 484 | } | 565 | } | ||
| 485 | - | for range duplicateCallers { | 566 | + | // Synchronize on the ownership state itself: every duplicate is waiting |
| 486 | - | <-started | 567 | + | // behind the distinct ordinary teardown before its publication is released. |
| 487 | - | } | 568 | + | awaitTeardownWaiters(t, sess, duplicateCallers) |
| 488 | - | for range 1000 { | |||
| 489 | - | runtime.Gosched() | |||
| 490 | - | sess.snapshotMu.Lock() | |||
| 491 | - | generation := sess.snapshotGeneration | |||
| 492 | - | sess.snapshotMu.Unlock() | |||
| 493 | - | if generation > 1 { | |||
| 494 | - | break | |||
| 495 | - | } | |||
| 496 | - | } | |||
| 497 | 569 | ||||
| 498 | sess.snapshotMu.Lock() | 570 | sess.snapshotMu.Lock() | ||
| 499 | generation := sess.snapshotGeneration | 571 | generation := sess.snapshotGeneration | ||
| @@ -501,9 +573,19 @@ func TestConcurrentSessionTeardownPublishesOneFinalSnapshot(t *testing.T) { | |||||
| 501 | require.Equal(t, uint64(1), generation, "duplicate teardown callers must not create synthetic terminal generations") | 573 | require.Equal(t, uint64(1), generation, "duplicate teardown callers must not create synthetic terminal generations") | ||
| 502 | 574 | ||||
| 503 | releaseOnce.Do(func() { close(releasePublication) }) | 575 | releaseOnce.Do(func() { close(releasePublication) }) | ||
| 504 | - | require.NoError(t, <-first) | 576 | + | select { |
| 577 | + | case err := <-first: | |||
| 578 | + | require.NoError(t, err) | |||
| 579 | + | case <-time.After(testWaitTimeout): | |||
| 580 | + | t.Fatal("ordinary teardown did not finish") | |||
| 581 | + | } | |||
| 505 | for range duplicateCallers { | 582 | for range duplicateCallers { | ||
| 506 | - | require.NoError(t, <-duplicates) | 583 | + | select { |
| 584 | + | case err := <-duplicates: | |||
| 585 | + | require.NoError(t, err) | |||
| 586 | + | case <-time.After(testWaitTimeout): | |||
| 587 | + | t.Fatal("duplicate teardown did not observe the completed registry transition") | |||
| 588 | + | } | |||
| 507 | } | 589 | } | ||
| 508 | sess.snapshotMu.Lock() | 590 | sess.snapshotMu.Lock() | ||
| 509 | generation = sess.snapshotGeneration | 591 | generation = sess.snapshotGeneration | ||