Make sure Stop hooks are executed when Start times out on a long-runn…

…ing Start hook (#1061)

Currently, none of Stop hooks are run if app.Start fails with a timeout,
and app's lifecycle
is stuck on waiting for a long-running Start hook to finish executing.
While we cannot run Stop
hook for the long-running Start hook that caused the timeout, we can
still run rest of the Stop
hooks for the Start hooks that finished running.

In this PR, this is done by just easing up the requirement for running
Stop hooks to include
lifecycle's `starting` state. Ideally the app's lifecycle state should
transition into
`incompleteStart` when a long-running Start hook causes the context to
timeout and does
not respect the context expiration. However, this requires a bigger
refactor so that all
lifecycle hook invocations are made asynchronous.

Fixes #1035

Co-authored-by: Sung Yoon Whang <sungyoon@uber.com>

app_test.go

@@ -1229,6 +1229,45 @@ func TestAppStart(t *testing.T) {
 		assert.NotContains(t, err.Error(), "timed out while executing hook OnStart")
 	})
 
+	t.Run("CtxTimeoutDuringStartStillRunsStopHooks", func(t *testing.T) {
+		t.Parallel()
+
+		var ran bool
+		mockClock := clock.NewMock()
+		app := New(
+			WithClock(mockClock),
+			Invoke(func(lc Lifecycle) {
+				lc.Append(Hook{
+					OnStart: func(ctx context.Context) error {
+						return nil
+					},
+					OnStop: func(ctx context.Context) error {
+						ran = true
+						return nil
+					},
+				})
+				lc.Append(Hook{
+					OnStart: func(ctx context.Context) error {
+						mockClock.Add(5 * time.Second)
+						return ctx.Err()
+					},
+					OnStop: func(ctx context.Context) error {
+						assert.Fail(t, "This Stop hook should not be called")
+						return nil
+					},
+				})
+			}),
+		)
+
+		startCtx, cancelStart := mockClock.WithTimeout(context.Background(), time.Second)
+		defer cancelStart()
+		err := app.Start(startCtx)
+		require.Error(t, err)
+		assert.ErrorContains(t, err, "context deadline exceeded")
+		require.NoError(t, app.Stop(context.Background()))
+		assert.True(t, ran, "Stop hook for the Start hook that finished running should have been called")
+	})
+
 	t.Run("Rollback", func(t *testing.T) {
 		t.Parallel()
 

internal/lifecycle/lifecycle.go

@@ -265,7 +265,7 @@ func (l *Lifecycle) Stop(ctx context.Context) error {
 	}
 
 	l.mu.Lock()
-	if l.state != started && l.state != incompleteStart {
+	if l.state != started && l.state != incompleteStart && l.state != starting {
 		defer l.mu.Unlock()
 		return nil
 	}