From 39daa878811ec76c3a97ff5c3a7931404f0f05a3 Mon Sep 17 00:00:00 2001 From: Jussi Maki Date: Wed, 27 May 2026 12:07:24 +0200 Subject: [PATCH] job: Log an error if a job fails to stop With the fix to job stop ordering we hit bunch of places that relied on the wrong order. To make this easier to debug in the future log an error when we time out trying to stop a job, so we know which one is stuck. Signed-off-by: Jussi Maki --- job/job.go | 2 +- job/lifecycle.go | 6 +++++- job/lifecycle_test.go | 19 ++++++------------- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/job/job.go b/job/job.go index bbc81e4..5709616 100644 --- a/job/job.go +++ b/job/job.go @@ -95,7 +95,7 @@ func (c *registry) Stop(ctx cell.HookContext) error { return nil } c.started = false - return c.runtimeLifecycle.stop(ctx) + return c.runtimeLifecycle.stop(ctx, c.logger) } // PreStopHookMarker tells [cell.DefaultLifecycle] that this diff --git a/job/lifecycle.go b/job/lifecycle.go index b89af2b..ed63d65 100644 --- a/job/lifecycle.go +++ b/job/lifecycle.go @@ -5,6 +5,7 @@ package job import ( "context" + "log/slog" "sync" "github.com/cilium/hive/cell" @@ -56,7 +57,7 @@ func (r *jobLifecycle) remove(qj *queuedJob) { qj.next = nil } -func (r *jobLifecycle) stop(ctx cell.HookContext) error { +func (r *jobLifecycle) stop(ctx cell.HookContext, log *slog.Logger) error { // Collect jobs to stop and unlink them. We must stop them without holding the // lock as [queuedJob.Stop] will try to call [jobLifecycle.remove]. var jobsToStop []*queuedJob @@ -73,6 +74,9 @@ func (r *jobLifecycle) stop(ctx cell.HookContext) error { for _, job := range jobsToStop { job.Stop(ctx) if ctx.Err() != nil { + log.Error("Stop cancelled while waiting for job to stop", + "job", + job.job.info()) break } } diff --git a/job/lifecycle_test.go b/job/lifecycle_test.go index 0dea481..a7ff841 100644 --- a/job/lifecycle_test.go +++ b/job/lifecycle_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/require" "github.com/cilium/hive/cell" + "github.com/cilium/hive/hivetest" ) func TestJobLifecycleInsertAndRemove(t *testing.T) { @@ -30,15 +31,15 @@ func TestJobLifecycleInsertAndRemove(t *testing.T) { requireRuntimeLifecycleList(t, r, third, second, first) - removeRuntimeLifecycleJob(r, second) + r.runtimeLifecycle.remove(second) requireRuntimeLifecycleList(t, r, third, first) requireRuntimeLifecycleJobUnlinked(t, r, second) - removeRuntimeLifecycleJob(r, third) + r.runtimeLifecycle.remove(third) requireRuntimeLifecycleList(t, r, first) requireRuntimeLifecycleJobUnlinked(t, r, third) - removeRuntimeLifecycleJob(r, first) + r.runtimeLifecycle.remove(first) requireRuntimeLifecycleList(t, r) requireRuntimeLifecycleJobUnlinked(t, r, first) } @@ -86,7 +87,7 @@ func TestJobLifecycleStopStopsJobsInReverseStartOrder(t *testing.T) { insertRuntimeLifecycleJobs(r, first, second, third) waitForLifecycleJobs(t, first, second, third) - require.NoError(t, stopRuntimeLifecycle(r, context.Background())) + require.NoError(t, r.runtimeLifecycle.stop(context.Background(), hivetest.Logger(t))) requireRuntimeLifecycleList(t, r) @@ -102,7 +103,7 @@ func TestJobLifecycleStopReturnsContextError(t *testing.T) { cancel() var lifecycle jobLifecycle - assert.ErrorIs(t, lifecycle.stop(ctx), context.Canceled) + assert.ErrorIs(t, lifecycle.stop(ctx, hivetest.Logger(t)), context.Canceled) } type blockingLifecycleJob struct { @@ -180,14 +181,6 @@ func insertRuntimeLifecycleJobs(r *registry, jobs ...*queuedJob) { } } -func removeRuntimeLifecycleJob(r *registry, qj *queuedJob) { - r.runtimeLifecycle.remove(qj) -} - -func stopRuntimeLifecycle(r *registry, ctx context.Context) error { - return r.runtimeLifecycle.stop(ctx) -} - func requireRuntimeLifecycleList(t *testing.T, r *registry, want ...*queuedJob) { t.Helper()