fix(health): report degraded while a configured job is not scheduled, and finish shutdown (#781)

Closes #780. Follows from the #773 discussion: the daemon should not
refuse to start on a rejected job, but it should stop claiming to be
fine.

## The check could not see the failure it existed to report

```go
func (hc *HealthChecker) checkScheduler() {
    check := HealthCheck{Name: "scheduler", Status: HealthStatusHealthy,
                         Message: "Scheduler is operational"}

    // In a real implementation, this would check the actual scheduler
    // For now, we'll assume it's healthy if the service is running
```

A daemon with a mistyped schedule — a job that never fires — served a
green `/health`, which is the probe `docs/INTEGRATION_PATTERNS.md` tells
operators to point a container healthcheck at. After #777 the rejection
is on stderr at startup, and nothing a monitoring system polls reflected
it. Log lines get read once someone already suspects a problem.

## Where the state lives, and why there

The scheduler records the jobs it refuses, keyed by name with the
reason.

Recording it in the scheduler rather than in the config loader is what
makes this cover **jobs added long after startup** from Docker labels
(`--docker-poll-interval`), not only those read from the INI at boot —
every path that adds a job goes through `AddJob`. A name that later
registers clears its own entry, as does removing the job, so a corrected
config reloaded at runtime recovers **without a restart** instead of
staying degraded until someone notices.

## Degraded, not unhealthy

Per the comment at `health.go:24`, only `unhealthy` makes `/ready`
answer 503. Taking a daemon out of rotation because one job of twenty
has a typo would trade a silent failure for a louder one. `degraded`
appears in the body of `/health` and `/healthz`, where an operator or an
alert rule can act on it, while the jobs that do work keep running.

## Verified against a running daemon

One good job and one unschedulable one:

```json
{ "status": "degraded",
  "checks": { "scheduler": {
      "status": "degraded",
      "message": "1 configured job(s) are not scheduled and will not run: broken" } } }
```

`/ready` still answers **200**. With both jobs valid it reports
`healthy` with the original message.

A checker constructed without a scheduler reports `degraded` rather than
`healthy` — claiming health it has not established is the habit that
made the old stub useless. That widens `NewHealthChecker` by one
parameter; the callers in tests pass `nil`.

## Test plan

- [x] `go test ./...` — green
- [x] `go test -race -tags=e2e ./e2e/...` — green
- [x] `golangci-lint run` incl. `--build-tags="e2e unix"` — 0 issues
- [x] `lefthook run pre-push` — exit 0
- [x] Both directions checked against the real binary and its HTTP
endpoints, including that `/ready` stays 200
- [x] Recovery covered: a refused job that later registers clears the
complaint

---

## Follow-up from review

Addressing the review found two further defects, both in the same family
as the one above — a mechanism that looked like it worked because
nothing checked whether it did.

### The health checker's loop could not be stopped

`runPeriodicChecks` ran `for range ticker.C` with no exit. Harmless for
the daemon's single instance, a leaked goroutine per case in tests.
`Stop()` ends it, and the daemon calls it via a shutdown hook.

### Shutdown ended the process before its hooks had run

Wiring that hook up showed it never ran. The daemon ended the process on
`ShutdownChan`, which is closed when shutdown **starts**:

```go
<-c.shutdownManager.ShutdownChan()
// Give some time for graceful shutdown to complete   <- it does not
c.closeDone()
```

So only the first priority group ever executed. The web server has a
hook registered to stop it gracefully, and that hook was killed
mid-flight along with any request in progress. Measured on a release
build, `SIGTERM` reached the end of shutdown in **0 of 5 runs**; with
the fix, **5 of 5**.

The e2e test that covers shutdown asserted on the substring `"graceful
shutdown"` — which also matches `"Starting graceful shutdown"`, logged
*before* the first hook. That is why it stayed green. It now requires
the completion line.

One caveat worth stating plainly: the e2e harness builds with `-race`,
and the instrumentation reliably flips this race (5 of 5 runs complete
even on the broken code). So the strengthened e2e assertion would
**not** have caught this regression by itself. The deterministic guard
is `TestShutdownDoneClosesOnlyAfterEveryHookRan` in `core`.

### `/health` reported a version of `1.0.0` for every build

Hardcoded at the call site, so the endpoint could not be used to tell
which ofelia was answering. It now reports the build's version, or `dev`
when there are no ldflags.

## Verification of the follow-up

```
$ ofelia daemon --config=… (log-level=debug, web enabled), then SIGTERM
Executing shutdown hook: scheduler (priority: 10)      ... completed successfully
Executing shutdown hook: http-server (priority: 20)    ... completed successfully
Executing shutdown hook: health-checker (priority: 30) ... completed successfully
Graceful shutdown completed successfully
```

Before the fix, that log ended after the `scheduler` hook.

Author: CybotTM

cli/daemon.go

@@ -163,7 +163,25 @@ func (c *DaemonCommand) boot() (err error) {
 	if c.dockerHandler != nil {
 		dockerProvider = c.dockerHandler.GetDockerProvider()
 	}
-	c.healthChecker = web.NewHealthChecker(dockerProvider, "1.0.0")
+	// The version was hardcoded to "1.0.0", so /health reported the same
+	// number for every build ever shipped and could not be used to tell which
+	// ofelia was answering. Dev builds have no ldflags and say so.
+	reportedVersion := Version
+	if reportedVersion == "" {
+		reportedVersion = "dev"
+	}
+	c.healthChecker = web.NewHealthChecker(dockerProvider, c.scheduler, reportedVersion)
+
+	// Stop the checker's periodic loop on shutdown, after the server that
+	// serves its result has gone (priority 20).
+	c.shutdownManager.RegisterHook(core.ShutdownHook{
+		Name:     "health-checker",
+		Priority: 30,
+		Hook: func(context.Context) error {
+			c.healthChecker.Stop()
+			return nil
+		},
+	})
 
 	// Create graceful scheduler with shutdown support
 	gracefulScheduler := core.NewGracefulScheduler(c.scheduler, c.shutdownManager)
@@ -264,11 +282,14 @@ func (c *DaemonCommand) start() error {
 	// Start listening for shutdown signals
 	c.shutdownManager.ListenForShutdown()
 
-	// Set up a goroutine to close done channel when shutdown completes
+	// Set up a goroutine to close done channel when shutdown completes.
+	//
+	// This waited on ShutdownChan, which is closed when shutdown *starts*, so
+	// the process exited while the hooks were still running: only the first
+	// priority group (the scheduler) ever ran, and the web server was never
+	// stopped gracefully despite the hook registered for it.
 	go func() {
-		<-c.shutdownManager.ShutdownChan()
-		// Give some time for graceful shutdown to complete
-		// The shutdown manager handles the actual shutdown process
+		<-c.shutdownManager.Done()
 		c.closeDone()
 	}()
 

core/scheduler.go

@@ -58,9 +58,15 @@ type Scheduler struct {
 	retryExecutor     *RetryExecutor
 	jobsByName        map[string]Job
 	disabledNames     map[string]struct{}
-	metricsRecorder   MetricsRecorder
-	clock             Clock
-	onJobComplete     func(jobName string, success bool)
+	// unschedulable records jobs the scheduler refused, keyed by name with the
+	// reason as the value. A refused job never runs, and the only earlier trace
+	// was a log line; keeping it here lets the health report say so, and covers
+	// jobs added long after startup from Docker labels as well as those from
+	// the config.
+	unschedulable   map[string]string
+	metricsRecorder MetricsRecorder
+	clock           Clock
+	onJobComplete   func(jobName string, success bool)
 }
 
 // concurrencySemaphore holds a swappable semaphore channel used by the
@@ -342,6 +348,7 @@ func (s *Scheduler) AddJob(j Job) error {
 // on demand via RunJob() which delegates to go-cron's TriggerEntryByName().
 func (s *Scheduler) AddJobWithTags(j Job, tags ...string) error {
 	if j.GetSchedule() == "" {
+		s.recordUnschedulable(j.GetName(), ErrEmptySchedule)
 		return ErrEmptySchedule
 	}
 
@@ -365,12 +372,17 @@ func (s *Scheduler) AddJobWithTags(j Job, tags ...string) error {
 			"Failed to register job %q - %q - %q",
 			j.GetName(), j.GetCommand(), j.GetSchedule(),
 		))
+		s.recordUnschedulable(j.GetName(), err)
 		return fmt.Errorf("add cron job: %w", err)
 	}
 	j.SetCronJobID(uint64(id))
 	s.mu.Lock()
 	s.Jobs = append(s.Jobs, j)
 	s.jobsByName[j.GetName()] = j
+	// A name that registers now is no longer unschedulable: a corrected config
+	// reloaded at runtime has to clear the old complaint, or the health report
+	// would stay degraded until a restart.
+	delete(s.unschedulable, j.GetName())
 	s.mu.Unlock()
 
 	if IsTriggeredSchedule(j.GetSchedule()) {
@@ -408,6 +420,9 @@ func (s *Scheduler) RemoveJob(j Job) error {
 		}
 	}
 	delete(s.jobsByName, j.GetName())
+	// A job removed from the config is no longer expected to run, so a past
+	// refusal stops being a complaint about the current state.
+	delete(s.unschedulable, j.GetName())
 	delete(s.disabledNames, j.GetName())
 	s.Removed = append(s.Removed, j)
 	s.mu.Unlock()
@@ -635,6 +650,34 @@ func (s *Scheduler) GetActiveJobs() []Job {
 	return jobs
 }
 
+// recordUnschedulable notes that a job was refused, so the health report can
+// say a configured job is not running rather than leaving it to whoever reads
+// the startup log.
+func (s *Scheduler) recordUnschedulable(name string, reason error) {
+	if name == "" {
+		return
+	}
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	if s.unschedulable == nil {
+		s.unschedulable = make(map[string]string)
+	}
+	s.unschedulable[name] = reason.Error()
+}
+
+// GetUnschedulableJobs returns a copy of the jobs the scheduler refused,
+// keyed by job name with the reason as the value. Empty means every job that
+// was offered is registered.
+func (s *Scheduler) GetUnschedulableJobs() map[string]string {
+	s.mu.RLock()
+	defer s.mu.RUnlock()
+	out := make(map[string]string, len(s.unschedulable))
+	for name, reason := range s.unschedulable {
+		out[name] = reason
+	}
+	return out
+}
+
 // getJob finds a job in the provided slice by name.
 func getJob(jobs []Job, name string) (Job, int) {
 	for i, j := range jobs {

core/scheduler_unschedulable_test.go

@@ -0,0 +1,68 @@
+// Copyright (c) 2025-2026 Netresearch DTT GmbH
+// SPDX-License-Identifier: MIT
+
+package core
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// A refused job is remembered so /health can name it. That memory has to end
+// when the job does, or an operator who fixed the problem by deleting the job
+// would be left with a permanently degraded daemon and no job to blame it on.
+
+func TestSchedulerRecordsRefusedJob(t *testing.T) {
+	t.Parallel()
+
+	job := &TestJob{}
+	job.Name = "broken"
+	job.Schedule = "not-a-schedule"
+
+	sc := NewScheduler(newDiscardLogger())
+	require.Error(t, sc.AddJob(job))
+
+	refused := sc.GetUnschedulableJobs()
+	require.Contains(t, refused, "broken")
+	assert.NotEmpty(t, refused["broken"], "the recorded reason is what makes the report actionable")
+}
+
+// TestSchedulerRemoveJobClearsUnschedulable covers the removal half: dropping
+// the job from the config has to drop the complaint with it.
+func TestSchedulerRemoveJobClearsUnschedulable(t *testing.T) {
+	t.Parallel()
+
+	job := &TestJob{}
+	job.Name = "broken"
+	job.Schedule = "not-a-schedule"
+
+	sc := NewScheduler(newDiscardLogger())
+	require.Error(t, sc.AddJob(job))
+	require.Contains(t, sc.GetUnschedulableJobs(), "broken")
+
+	require.NoError(t, sc.RemoveJob(job))
+
+	assert.NotContains(t, sc.GetUnschedulableJobs(), "broken",
+		"a job removed from the config still held /health degraded")
+}
+
+// TestSchedulerGetUnschedulableJobsIsACopy pins that callers cannot reach into
+// the scheduler's state through the returned map: the health checker reads it
+// on every check, and a shared map would be a data race as well as a way to
+// silently erase a refusal.
+func TestSchedulerGetUnschedulableJobsIsACopy(t *testing.T) {
+	t.Parallel()
+
+	job := &TestJob{}
+	job.Name = "broken"
+	job.Schedule = "not-a-schedule"
+
+	sc := NewScheduler(newDiscardLogger())
+	require.Error(t, sc.AddJob(job))
+
+	delete(sc.GetUnschedulableJobs(), "broken")
+
+	assert.Contains(t, sc.GetUnschedulableJobs(), "broken")
+}

core/shutdown.go

@@ -22,6 +22,7 @@ type ShutdownManager struct {
 	hooks          []ShutdownHook
 	mu             sync.Mutex
 	shutdownChan   chan struct{}
+	doneChan       chan struct{}
 	isShuttingDown bool
 	logger         *slog.Logger
 }
@@ -43,6 +44,7 @@ func NewShutdownManager(logger *slog.Logger, timeout time.Duration) *ShutdownMan
 		timeout:      timeout,
 		hooks:        make([]ShutdownHook, 0),
 		shutdownChan: make(chan struct{}),
+		doneChan:     make(chan struct{}),
 		logger:       logger,
 	}
 }
@@ -89,6 +91,10 @@ func (sm *ShutdownManager) Shutdown() error {
 	sm.isShuttingDown = true
 	sm.mu.Unlock()
 
+	// Only one caller ever gets past the guard above, so this closes once and
+	// covers every return path below, including the timeout.
+	defer close(sm.doneChan)
+
 	sm.logger.Info(fmt.Sprintf("Starting graceful shutdown (timeout: %v)", sm.timeout))
 
 	// Create context with timeout
@@ -181,6 +187,15 @@ func (sm *ShutdownManager) ShutdownChan() <-chan struct{} {
 	return sm.shutdownChan
 }
 
+// Done returns a channel that's closed once every hook has run.
+//
+// ShutdownChan only says shutdown *began* — it is closed before the first hook
+// executes. Anything that ends the process must wait on this instead, or the
+// later priority groups are killed mid-flight and never run at all.
+func (sm *ShutdownManager) Done() <-chan struct{} {
+	return sm.doneChan
+}
+
 // IsShuttingDown returns true if shutdown is in progress
 func (sm *ShutdownManager) IsShuttingDown() bool {
 	sm.mu.Lock()

core/shutdown_done_test.go

@@ -0,0 +1,87 @@
+// Copyright (c) 2025-2026 Netresearch DTT GmbH
+// SPDX-License-Identifier: MIT
+
+package core
+
+import (
+	"context"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// The daemon ends the process when it sees shutdown finish. It used to watch
+// ShutdownChan, which is closed *before* the first hook runs, so everything
+// after the first priority group was killed mid-flight — the web server was
+// never stopped gracefully even though a hook was registered to do it. Done is
+// the signal that actually means finished, and these pin that meaning.
+
+func TestShutdownDoneClosesOnlyAfterEveryHookRan(t *testing.T) {
+	t.Parallel()
+
+	sm := NewShutdownManager(newDiscardLogger(), 5*time.Second)
+
+	var mu sync.Mutex
+	var ran []string
+	for _, h := range []struct {
+		name     string
+		priority int
+	}{{"first", 10}, {"second", 20}} {
+		sm.RegisterHook(ShutdownHook{
+			Name:     h.name,
+			Priority: h.priority,
+			Hook: func(context.Context) error {
+				mu.Lock()
+				defer mu.Unlock()
+				ran = append(ran, h.name)
+				return nil
+			},
+		})
+	}
+
+	select {
+	case <-sm.Done():
+		t.Fatal("Done was closed before shutdown had even started")
+	default:
+	}
+
+	require.NoError(t, sm.Shutdown())
+
+	select {
+	case <-sm.Done():
+	default:
+		t.Fatal("Done was not closed once Shutdown returned")
+	}
+
+	mu.Lock()
+	defer mu.Unlock()
+	assert.Equal(t, []string{"first", "second"}, ran,
+		"every priority group has to run, in order, before Done closes")
+}
+
+// TestShutdownDoneClosesWhenAHookTimesOut pins the other half: a hook that
+// overruns the deadline must not leave whoever waits on Done blocked forever.
+func TestShutdownDoneClosesWhenAHookTimesOut(t *testing.T) {
+	t.Parallel()
+
+	sm := NewShutdownManager(newDiscardLogger(), 50*time.Millisecond)
+	sm.RegisterHook(ShutdownHook{
+		Name:     "overruns",
+		Priority: 10,
+		Hook: func(ctx context.Context) error {
+			<-ctx.Done()
+			return ctx.Err()
+		},
+	})
+
+	require.Error(t, sm.Shutdown())
+
+	select {
+	case <-sm.Done():
+	case <-time.After(2 * time.Second):
+		t.Fatal("Done stayed open after a hook overran the shutdown timeout")
+	}
+}

e2e/graceful_shutdown_test.go

@@ -29,7 +29,16 @@ func TestE2E_GracefulShutdown_SIGTERM(t *testing.T) {
 `
 
 	configPath := writeConfig(t, configBody)
-	daemon := startDaemon(t, configPath)
+	// The web server is enabled so the shutdown exercises more than one hook
+	// priority group, which is what the completion assertion below is about.
+	//
+	// It is worth knowing what this test can and cannot catch: exiting without
+	// waiting for the hooks is a race, and a release build loses it every time
+	// (measured 0/5 completions), but this harness builds with -race, whose
+	// instrumentation reliably flips the outcome (5/5). So the assertion below
+	// would not have failed on the broken code here. The deterministic guard
+	// for that is TestShutdownDoneClosesOnlyAfterEveryHookRan in core.
+	daemon := startDaemon(t, configPath, "--enable-web", "--web-address="+reserveLoopbackAddr(t))
 	defer daemon.shutdown(t, 10*time.Second) // safety net in case signal is lost
 
 	// Let the scheduler tick at least once so there is actual work to
@@ -64,10 +73,16 @@ func TestE2E_GracefulShutdown_SIGTERM(t *testing.T) {
 
 	// Verify the shutdown banner is emitted — proves the signal reached
 	// ShutdownManager rather than the process being killed by a harness.
+	//
+	// The second needle used to be "graceful shutdown", which also matches
+	// "Starting graceful shutdown" — logged *before* the first hook runs. So
+	// this passed while the daemon exited mid-shutdown and every hook group
+	// after the first was killed in flight. Only the completion line requires
+	// that all of them actually finished.
 	out := daemon.stdout.String()
 	for _, needle := range []string{
 		"Received shutdown signal",
-		"graceful shutdown",
+		"Graceful shutdown completed successfully",
 	} {
 		if !strings.Contains(out, needle) {
 			t.Errorf("expected shutdown log to mention %q, got:\nstdout=%s",

web/auth_integration_test.go

@@ -106,7 +106,7 @@ func TestServerWithAuthEnabled(t *testing.T) {
 
 	t.Run("health_endpoints_bypass_auth", func(t *testing.T) {
 		endpoints := []string{"/health", "/healthz", "/ready", "/live"}
-		hc := webpkg.NewHealthChecker(nil, "test")
+		hc := webpkg.NewHealthChecker(nil, nil, "test")
 		srv.RegisterHealthEndpoints(hc)
 
 		for _, ep := range endpoints {