Skip to content

Commit 1b72b0b

Browse files
moebiusband73claude
andcommitted
Fix critical/severe issues in init, startup and shutdown
- auth: do not abort the server when authentication is disabled. auth.Init is now always called; with disable-authentication it sets up an ephemeral session store (SESSION_KEY not required) and registers no authenticators, so the unconditional auth.GetAuthInstance() callers (server init, api.New()) always get a valid instance. - main: run the graceful-shutdown sequence on the startup-error path. runServer derives a cancelable context and, on a server-start failure, cancels it and waits so the metricstore final checkpoint / WAL rotation, archiver flush and taskmanager shutdown actually run before exit. - server: log the :80 HTTP->HTTPS redirect listener error instead of dropping it. - archiver: guard Shutdown against being called when Start never ran (avoids close(nil) panic / blocking on a nil workerDone). - nats API: stop worker goroutines on shutdown via a stop channel + idempotent Shutdown(); workers and subscription callbacks select on stop and the channels are never closed, so no send-on-closed-channel can occur. Wired into Server.Shutdown after the NATS client is closed. - metricstore: make Shutdown idempotent (nil shutdownFunc, early return) and release shutdownFuncMu before the checkpoint write. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 3c179f9caa8f
1 parent 56ae1de commit 1b72b0b

6 files changed

Lines changed: 119 additions & 32 deletions

File tree

cmd/cc-backend/main.go

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -172,14 +172,20 @@ func handleUserCommands() error {
172172
return fmt.Errorf("--add-user and --del-user can only be used if authentication is enabled")
173173
}
174174

175-
if !config.Keys.DisableAuthentication {
176-
if cfg := ccconf.GetPackageConfig("auth"); cfg != nil {
177-
auth.Init(&cfg)
178-
} else {
179-
cclog.Warn("Authentication disabled due to missing configuration")
180-
auth.Init(nil)
175+
// Always initialize the auth subsystem so the HTTP server and REST API have a
176+
// valid (non-nil) auth instance, even when authentication is disabled. With
177+
// authentication disabled, Init only sets up an ephemeral session store and
178+
// registers no authenticators (see auth.Init).
179+
if cfg := ccconf.GetPackageConfig("auth"); cfg != nil {
180+
auth.Init(&cfg)
181+
} else {
182+
if !config.Keys.DisableAuthentication {
183+
cclog.Warn("Authentication enabled but no auth configuration found")
181184
}
185+
auth.Init(nil)
186+
}
182187

188+
if !config.Keys.DisableAuthentication {
183189
// Check for default security keys
184190
checkDefaultSecurityKeys()
185191

@@ -337,6 +343,12 @@ func initSubsystems() error {
337343
}
338344

339345
func runServer(ctx context.Context) error {
346+
// Derive a cancelable context so the startup-error path below can trigger the
347+
// same graceful-shutdown sequence as a signal (via the signal handler that
348+
// waits on ctx.Done()).
349+
ctx, cancel := context.WithCancel(ctx)
350+
defer cancel()
351+
340352
var wg sync.WaitGroup
341353

342354
// Initialize metric store if configuration is provided
@@ -438,26 +450,32 @@ func runServer(ctx context.Context) error {
438450
// Wait for either:
439451
// 1. An error from server startup
440452
// 2. Completion of all goroutines (normal shutdown or crash)
453+
var runErr error
441454
select {
442-
case err := <-errChan:
455+
case runErr = <-errChan:
443456
// errChan will be closed when waitDone is closed, which happens
444457
// when all goroutines complete (either from normal shutdown or error)
445-
if err != nil {
446-
return err
447-
}
448458
case <-time.After(100 * time.Millisecond):
449459
// Give the server 100ms to start and report any immediate startup errors
450460
// After that, just wait for normal shutdown completion
451461
select {
452-
case err := <-errChan:
453-
if err != nil {
454-
return err
455-
}
462+
case runErr = <-errChan:
456463
case <-waitDone:
457464
// Normal shutdown completed
458465
}
459466
}
460467

468+
if runErr != nil {
469+
// A subsystem failed (e.g. the HTTP server could not bind). Trigger the
470+
// graceful-shutdown path for the subsystems that were already started
471+
// (metricstore checkpoint, archiver flush, taskmanager) by cancelling the
472+
// context the signal handler waits on, then wait for it to finish so we
473+
// don't exit before the final checkpoint is written.
474+
cancel()
475+
<-waitDone
476+
return runErr
477+
}
478+
461479
cclog.Print("Graceful shutdown completed!")
462480
return nil
463481
}

cmd/cc-backend/server.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -415,7 +415,9 @@ func (s *Server) Start(ctx context.Context) error {
415415

416416
if !strings.HasSuffix(config.Keys.Addr, ":80") && config.Keys.RedirectHTTPTo != "" {
417417
go func() {
418-
http.ListenAndServe(":80", http.RedirectHandler(config.Keys.RedirectHTTPTo, http.StatusMovedPermanently))
418+
if err := http.ListenAndServe(":80", http.RedirectHandler(config.Keys.RedirectHTTPTo, http.StatusMovedPermanently)); err != nil {
419+
cclog.Errorf("HTTP-to-HTTPS redirect listener on :80 failed: %v", err)
420+
}
419421
}()
420422
}
421423

@@ -460,6 +462,11 @@ func (s *Server) Shutdown(ctx context.Context) {
460462
if nc != nil {
461463
nc.Close()
462464
}
465+
// Stop the NATS API worker goroutines after the client is closed (no more
466+
// subscription callbacks can enqueue once the connection is down).
467+
if s.natsAPIHandle != nil {
468+
s.natsAPIHandle.Shutdown()
469+
}
463470
cclog.Infof("Shutdown: NATS closed (%v)", time.Since(natsStart))
464471

465472
httpStart := time.Now()

internal/api/nats.go

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,12 @@ type NatsAPI struct {
7878
jobCh chan natsMessage
7979
// nodeCh receives node state messages for processing by worker goroutines.
8080
nodeCh chan natsMessage
81+
// stop signals worker goroutines and subscription callbacks to stop.
82+
// Closing it (via Shutdown) makes workers exit and callbacks drop further
83+
// messages instead of blocking; the channels are never closed so in-flight
84+
// callbacks can never send on a closed channel.
85+
stop chan struct{}
86+
stopOnce sync.Once
8187
}
8288

8389
// NewNatsAPI creates a new NatsAPI instance with channel-based worker pools.
@@ -99,6 +105,7 @@ func NewNatsAPI() *NatsAPI {
99105
JobRepository: repository.GetJobRepository(),
100106
jobCh: make(chan natsMessage, jobConc),
101107
nodeCh: make(chan natsMessage, nodeConc),
108+
stop: make(chan struct{}),
102109
}
103110

104111
// Start worker goroutines
@@ -112,17 +119,36 @@ func NewNatsAPI() *NatsAPI {
112119
return api
113120
}
114121

122+
// Shutdown stops the worker goroutines and tells subscription callbacks to stop
123+
// enqueueing. It is safe to call multiple times. Callers must ensure the NATS
124+
// client is closed first so no new callbacks are invoked.
125+
func (api *NatsAPI) Shutdown() {
126+
api.stopOnce.Do(func() {
127+
close(api.stop)
128+
})
129+
}
130+
115131
// jobWorker processes job event messages from the job channel.
116132
func (api *NatsAPI) jobWorker() {
117-
for msg := range api.jobCh {
118-
api.handleJobEvent(msg.subject, msg.data)
133+
for {
134+
select {
135+
case <-api.stop:
136+
return
137+
case msg := <-api.jobCh:
138+
api.handleJobEvent(msg.subject, msg.data)
139+
}
119140
}
120141
}
121142

122143
// nodeWorker processes node state messages from the node channel.
123144
func (api *NatsAPI) nodeWorker() {
124-
for msg := range api.nodeCh {
125-
api.handleNodeState(msg.subject, msg.data)
145+
for {
146+
select {
147+
case <-api.stop:
148+
return
149+
case msg := <-api.nodeCh:
150+
api.handleNodeState(msg.subject, msg.data)
151+
}
126152
}
127153
}
128154

@@ -140,13 +166,19 @@ func (api *NatsAPI) StartSubscriptions() error {
140166
s := config.Keys.APISubjects
141167

142168
if err := client.Subscribe(s.SubjectJobEvent, func(subject string, data []byte) {
143-
api.jobCh <- natsMessage{subject: subject, data: data}
169+
select {
170+
case api.jobCh <- natsMessage{subject: subject, data: data}:
171+
case <-api.stop:
172+
}
144173
}); err != nil {
145174
return err
146175
}
147176

148177
if err := client.Subscribe(s.SubjectNodeState, func(subject string, data []byte) {
149-
api.nodeCh <- natsMessage{subject: subject, data: data}
178+
select {
179+
case api.nodeCh <- natsMessage{subject: subject, data: data}:
180+
case <-api.stop:
181+
}
150182
}); err != nil {
151183
return err
152184
}

internal/archiver/archiveWorker.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,13 @@ func TriggerArchiving(job *schema.Job) {
222222
func Shutdown(timeout time.Duration) error {
223223
cclog.Info("Initiating archiver shutdown...")
224224

225+
// Guard against Shutdown being called when Start was never run: closing a nil
226+
// channel and receiving from a nil workerDone would panic/block forever.
227+
if archiveChannel == nil {
228+
cclog.Warn("Archiver shutdown called but archiver was never started")
229+
return nil
230+
}
231+
225232
// Close channel to signal no more jobs will be accepted
226233
close(archiveChannel)
227234

internal/auth/auth.go

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ package auth
99
import (
1010
"bytes"
1111
"context"
12+
"crypto/rand"
1213
"database/sql"
1314
"encoding/base64"
1415
"encoding/json"
@@ -187,20 +188,37 @@ func Init(authCfg *json.RawMessage) {
187188

188189
sessKey := os.Getenv("SESSION_KEY")
189190
if sessKey == "" {
190-
cclog.Fatal("environment variable 'SESSION_KEY' not set: refusing to start with an ephemeral session key. " +
191-
"Set SESSION_KEY in .env (base64-encoded 32 random bytes); a random key would invalidate all sessions on every restart " +
192-
"and prevent sessions from validating across replicas.")
193-
}
194-
keyBytes, err := base64.StdEncoding.DecodeString(sessKey)
195-
if err != nil {
196-
cclog.Fatal("Error while initializing authentication -> decoding session key failed")
191+
if !config.Keys.DisableAuthentication {
192+
cclog.Fatal("environment variable 'SESSION_KEY' not set: refusing to start with an ephemeral session key. " +
193+
"Set SESSION_KEY in .env (base64-encoded 32 random bytes); a random key would invalidate all sessions on every restart " +
194+
"and prevent sessions from validating across replicas.")
195+
}
196+
// Authentication is disabled: no user sessions are issued, so an
197+
// ephemeral random key is sufficient and SESSION_KEY is not required.
198+
ephemeralKey := make([]byte, 32)
199+
if _, err := rand.Read(ephemeralKey); err != nil {
200+
cclog.Fatalf("Error while initializing authentication -> generating ephemeral session key failed: %v", err)
201+
}
202+
authInstance.sessionStore = sessions.NewCookieStore(ephemeralKey)
203+
} else {
204+
keyBytes, err := base64.StdEncoding.DecodeString(sessKey)
205+
if err != nil {
206+
cclog.Fatal("Error while initializing authentication -> decoding session key failed")
207+
}
208+
authInstance.sessionStore = sessions.NewCookieStore(keyBytes)
197209
}
198-
authInstance.sessionStore = sessions.NewCookieStore(keyBytes)
199210

200211
if d, err := time.ParseDuration(config.Keys.SessionMaxAge); err == nil {
201212
authInstance.SessionMaxAge = d
202213
}
203214

215+
// When authentication is disabled no authenticators are required; the
216+
// session store created above is enough for the server to run with a
217+
// valid (non-nil) auth instance.
218+
if config.Keys.DisableAuthentication {
219+
return
220+
}
221+
204222
if authCfg == nil {
205223
return
206224
}

pkg/metricstore/metricstore.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -275,10 +275,15 @@ func Shutdown() {
275275
totalStart := time.Now()
276276

277277
shutdownFuncMu.Lock()
278-
defer shutdownFuncMu.Unlock()
279-
if shutdownFunc != nil {
280-
shutdownFunc()
278+
if shutdownFunc == nil {
279+
// Already shut down (or never initialized): nothing to do. This keeps
280+
// Shutdown idempotent so it is safe to call from more than one path.
281+
shutdownFuncMu.Unlock()
282+
return
281283
}
284+
shutdownFunc()
285+
shutdownFunc = nil
286+
shutdownFuncMu.Unlock()
282287
cclog.Infof("[METRICSTORE]> Background workers cancelled (%v)", time.Since(totalStart))
283288

284289
if Keys.Checkpoints.FileFormat == "wal" {

0 commit comments

Comments
 (0)