test(daemon): cover competing shutdown paths

commit: 98b1868b4ad192c329f238c93ab6ff8f90786fad
author: brice · 3mo ago · 2026-07-24 16:41 UTC
parent: f00a00d
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

1 file · +51 -35

M internal/usecase/daemon/snapshot_scheduler_test.go +51 -35


@@ -442,64 +442,80 @@ func TestForcedSnapshotShutdownTimeoutRetainsRetryableStateAndNotice(t *testing.
442	442		}
443	443		}
444	444
445		-	func TestShutdownDeadlineBoundsWaitForExistingTeardownOwner(t *testing.T) {
446		-	clock := newSnapshotDeadlineClock()
447		-	d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), clock)
	445	+	func TestServeShutdownDeadlineBoundsPaneExitTeardownOwner(t *testing.T) {
	446	+	pty, releasePTY := newBlockingPTY(t)
	447	+	clock := newServeShutdownClock()
	448	+	d := newTestDaemon(t, newFactory(t, pty), clock)
448	449		repository := portsmocks.NewMockSnapshotRepository(t)
449	450		WithSnapshotRepository(repository, nil)(d)
450		-	startSnapshotEncodeWorker(t, d)
	451	+	repository.EXPECT().List(mock.Anything).Return(nil, nil).Maybe()
451	452
452	453		publicationStarted := make(chan struct{})
453	454		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 {
	455	+	var releasePublicationOnce sync.Once
	456	+	releaseSnapshot := func() { releasePublicationOnce.Do(func() { close(releasePublication) }) }
	457	+	t.Cleanup(releaseSnapshot)
	458	+	repository.EXPECT().Publish(mock.Anything, mock.Anything).RunAndReturn(func(_ context.Context, publication ports.SnapshotPublication) error {
	459	+	require.Equal(t, uint64(1), publication.Generation)
458	460		close(publicationStarted)
459	461		<-releasePublication
460	462		return nil
461	463		}).Once()
462	464
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
	465	+	tr, sends, _ := newConn(t, mustHello(ports.IntentNew, "work", domain.Size{Cols: 80, Rows: 24}))
	466	+	listener := serveSnapshotListener(t, tr)
	467	+	ctx, cancel := context.WithCancel(context.Background())
	468	+	defer cancel()
	469	+	served := make(chan error, 1)
	470	+	go func() { served <- d.Serve(ctx, listener) }()
	471	+
	472	+	awaitFrame(t, sends, ports.MsgWelcome)
	473	+	sess := firstSession(d)
	474	+	require.NotNil(t, sess)
468	475
469		-	ordinaryDone := make(chan error, 1)
470		-	go func() { ordinaryDone <- d.killSession(sess, ports.ReasonSessionKilled, false) }()
	476	+	// Releasing the real pane reader drives EOF through ptyReader, reapPane, and
	477	+	// closePane. That ordinary lifecycle path must own teardown before Serve's
	478	+	// context cancellation enters its competing shutdownAll path.
	479	+	releasePTY()
471	480		select {
472	481		case <-publicationStarted:
473	482		case <-time.After(testWaitTimeout):
474		-	t.Fatal("ordinary teardown did not become owner")
	483	+	t.Fatal("pane EOF teardown did not publish its terminal snapshot")
475	484		}
	485	+	// The pane-exit owner has an independent final-snapshot timer. Keep it
	486	+	// unfired so Serve must use, and remain bounded by, its shared deadline.
	487	+	_ = clock.nextFinalTimer(t)
476	488
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		-	}()
	489	+	cancel()
	490	+	shutdownDeadlineTimer := clock.nextFinalTimer(t)
487	491		awaitTeardownWaiters(t, sess, 1)
488	492		shutdownDeadlineTimer.fire()
	493	+	awaitTeardownWaiters(t, sess, 0)
489	494
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		-	}
	495	+	sess.snapshotMu.Lock()
	496	+	generation := sess.snapshotGeneration
	497	+	sess.snapshotMu.Unlock()
	498	+	require.Equal(t, uint64(1), generation, "Serve shutdown must not create a competing terminal generation")
	499	+	require.Equal(t, int64(snapshotFinalFlushTimeout), clock.finalBudget.Load(), "Serve must spend only its shared snapshot deadline")
496	500
497		-	release()
	501	+	// Let the ordinary owner commit removal, then join the complete Serve and
	502	+	// snapshot-worker lifecycles so no reader, teardown, or worker is leaked.
	503	+	releaseSnapshot()
498	504		select {
499		-	case err := <-ordinaryDone:
	505	+	case err := <-served:
500	506		require.NoError(t, err)
501	507		case <-time.After(testWaitTimeout):
502		-	t.Fatal("ordinary teardown did not finish after its publication was released")
	508	+	t.Fatal("Serve did not return after the pane-exit owner completed")
	509	+	}
	510	+	d.snapshotWorkerMu.Lock()
	511	+	workerDone := d.snapshotWorkerDone
	512	+	d.snapshotWorkerMu.Unlock()
	513	+	if workerDone != nil {
	514	+	select {
	515	+	case <-workerDone:
	516	+	case <-time.After(testWaitTimeout):
	517	+	t.Fatal("snapshot worker leaked after Serve returned")
	518	+	}
503	519		}
504	520		}
505	521

 	}
 }
-func TestShutdownDeadlineBoundsWaitForExistingTeardownOwner(t *testing.T) {
+func TestServeShutdownDeadlineBoundsPaneExitTeardownOwner(t *testing.T) {
-	clock := newSnapshotDeadlineClock()
+	pty, releasePTY := newBlockingPTY(t)
-	d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), clock)
+	clock := newServeShutdownClock()
+	d := newTestDaemon(t, newFactory(t, pty), clock)
 	repository := portsmocks.NewMockSnapshotRepository(t)
 	WithSnapshotRepository(repository, nil)(d)
-	startSnapshotEncodeWorker(t, d)
+	repository.EXPECT().List(mock.Anything).Return(nil, nil).Maybe()
 	publicationStarted := make(chan struct{})
 	releasePublication := make(chan struct{})
-	var releaseOnce sync.Once
+	var releasePublicationOnce sync.Once
-	release := func() { releaseOnce.Do(func() { close(releasePublication) }) }
+	releaseSnapshot := func() { releasePublicationOnce.Do(func() { close(releasePublication) }) }
-	t.Cleanup(release)
+	t.Cleanup(releaseSnapshot)
-	repository.EXPECT().Publish(mock.Anything, mock.Anything).RunAndReturn(func(_ context.Context, _ ports.SnapshotPublication) error {
+	repository.EXPECT().Publish(mock.Anything, mock.Anything).RunAndReturn(func(_ context.Context, publication ports.SnapshotPublication) error {
+		require.Equal(t, uint64(1), publication.Generation)
 		close(publicationStarted)
 		<-releasePublication
 		return nil
 	}).Once()
-	sess := newSnapshotTestSession(t, "work", false, "/work")
+	tr, sends, _ := newConn(t, mustHello(ports.IntentNew, "work", domain.Size{Cols: 80, Rows: 24}))
-	pty, ok := sess.tabs[0].panes["pane-1"].pty.(*portsmocks.MockPTY)
+	listener := serveSnapshotListener(t, tr)
-	require.True(t, ok)
+	ctx, cancel := context.WithCancel(context.Background())
-	pty.EXPECT().Close().Return(nil).Maybe()
+	defer cancel()
-	d.sessions[sess.id] = sess
+	served := make(chan error, 1)
+	go func() { served <- d.Serve(ctx, listener) }()
+	awaitFrame(t, sends, ports.MsgWelcome)
+	sess := firstSession(d)
+	require.NotNil(t, sess)
-	ordinaryDone := make(chan error, 1)
+	// Releasing the real pane reader drives EOF through ptyReader, reapPane, and
-	go func() { ordinaryDone <- d.killSession(sess, ports.ReasonSessionKilled, false) }()
+	// closePane. That ordinary lifecycle path must own teardown before Serve's
+	// context cancellation enters its competing shutdownAll path.
+	releasePTY()
 	select {
 	case <-publicationStarted:
 	case <-time.After(testWaitTimeout):
-		t.Fatal("ordinary teardown did not become owner")
+		t.Fatal("pane EOF teardown did not publish its terminal snapshot")
 	}
+	// The pane-exit owner has an independent final-snapshot timer. Keep it
+	// unfired so Serve must use, and remain bounded by, its shared deadline.
+	_ = clock.nextFinalTimer(t)
-	// The ordinary teardown owns its own final-snapshot deadline. Leave it
+	cancel()
-	// unfired so it remains distinct from Serve's shared shutdown budget.
+	shutdownDeadlineTimer := clock.nextFinalTimer(t)
-	_ = clock.nextTimer(t)
-	deadline := newSnapshotShutdownDeadline(clock)
-	defer deadline.stop()
-	shutdownDeadlineTimer := clock.nextTimer(t)
-	shutdownDone := make(chan error, 1)
-	go func() {
-		shutdownDone <- d.killSessionWithSnapshotDeadline(sess, ports.ReasonServerShutdown, false, deadline)
-	}()
 	awaitTeardownWaiters(t, sess, 1)
 	shutdownDeadlineTimer.fire()
+	awaitTeardownWaiters(t, sess, 0)
-	select {
+	sess.snapshotMu.Lock()
-	case err := <-shutdownDone:
+	generation := sess.snapshotGeneration
-		require.NoError(t, err)
+	sess.snapshotMu.Unlock()
-	case <-time.After(testWaitTimeout):
+	require.Equal(t, uint64(1), generation, "Serve shutdown must not create a competing terminal generation")
-		t.Fatal("daemon shutdown waited beyond its shared deadline for the ordinary teardown owner")
+	require.Equal(t, int64(snapshotFinalFlushTimeout), clock.finalBudget.Load(), "Serve must spend only its shared snapshot deadline")
-	}
-	release()
+	// Let the ordinary owner commit removal, then join the complete Serve and
+	// snapshot-worker lifecycles so no reader, teardown, or worker is leaked.
+	releaseSnapshot()
 	select {
-	case err := <-ordinaryDone:
+	case err := <-served:
 		require.NoError(t, err)
 	case <-time.After(testWaitTimeout):
-		t.Fatal("ordinary teardown did not finish after its publication was released")
+		t.Fatal("Serve did not return after the pane-exit owner completed")
+	}
+	d.snapshotWorkerMu.Lock()
+	workerDone := d.snapshotWorkerDone
+	d.snapshotWorkerMu.Unlock()
+	if workerDone != nil {
+		select {
+		case <-workerDone:
+		case <-time.After(testWaitTimeout):
+			t.Fatal("snapshot worker leaked after Serve returned")
+		}
 	}
 }