diff --git a/AGENTS.md b/AGENTS.md index 7fb8fc4..9f35a09 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -75,6 +75,7 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for commit format (Conventional Commits) **Quick reference:** - Valid types: `feat`, `fix`, `docs`, `test`, `chore`, `refactor`, `perf`, `ci` +- PR titles must use Conventional Commits format (`type: short summary`) - Run `make fmt && make lint && make test` before submitting **Issue Linking:** diff --git a/docs/guides/error-handling.md b/docs/guides/error-handling.md index 435fb36..90db469 100644 --- a/docs/guides/error-handling.md +++ b/docs/guides/error-handling.md @@ -272,6 +272,23 @@ if err != nil { } ``` +## Shutdown Errors + +`App.Close()` aggregates multiple close failures into a single error using +`errors.Join`. You can still test for specific errors with `errors.Is` or +`errors.As`: + +```go +if err := app.Close(); err != nil { + if errors.Is(err, errDBClose) { + log.Printf("db close failed: %v", err) + } + if errors.Is(err, errCacheClose) { + log.Printf("cache close failed: %v", err) + } +} +``` + ## Tips - Return errors, don't panic (except for truly unrecoverable situations) diff --git a/docs/guides/lifecycle.md b/docs/guides/lifecycle.md index 2a61eac..db2c907 100644 --- a/docs/guides/lifecycle.md +++ b/docs/guides/lifecycle.md @@ -135,6 +135,37 @@ fmt.Println(svc1 == svc2) // true modkit does not provide automatic cleanup hooks. You must manage cleanup manually: +### App.Close and CloseContext + +If providers implement `io.Closer`, you can shut down the app explicitly: + +```go +// Close all io.Closer providers in reverse build order. +if err := app.Close(); err != nil { + log.Printf("shutdown error: %v", err) +} +``` + +`Close()` is idempotent: once it completes successfully (even with aggregated errors), +subsequent calls return `nil` and do not re-close providers. + +For context-aware shutdown, use `CloseContext(ctx)`: + +```go +ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) +defer cancel() + +if err := app.CloseContext(ctx); err != nil { + // Returns ctx.Err() if canceled or timed out + log.Printf("shutdown error: %v", err) +} +``` + +`CloseContext` checks `ctx.Err()` before closing and before each closer. If the context +is canceled mid-close, it returns the context error and leaves the app eligible for +a later `Close()` retry. While a close is in progress, concurrent close calls are +no-ops. + ### Pattern 1: Cleanup in main() ```go diff --git a/modkit/kernel/bootstrap.go b/modkit/kernel/bootstrap.go index 6b24ff9..e58fedb 100644 --- a/modkit/kernel/bootstrap.go +++ b/modkit/kernel/bootstrap.go @@ -3,7 +3,9 @@ package kernel import ( "context" + "errors" "io" + "sync/atomic" "github.com/go-modkit/modkit/modkit/module" ) @@ -14,6 +16,8 @@ type App struct { Graph *Graph container *Container Controllers map[string]any + closed atomic.Bool + closing atomic.Bool } func controllerKey(moduleName, controllerName string) string { @@ -89,11 +93,38 @@ func (a *App) Closers() []io.Closer { // Close calls Close on all io.Closer providers in reverse build order. func (a *App) Close() error { - var firstErr error + return a.CloseContext(context.Background()) +} + +// CloseContext calls Close on all io.Closer providers in reverse build order, +// stopping early if the context is canceled. +func (a *App) CloseContext(ctx context.Context) error { + if a.closed.Load() { + return nil + } + + if err := ctx.Err(); err != nil { + return err + } + + if !a.closing.CompareAndSwap(false, true) { + return nil + } + defer a.closing.Store(false) + + var errs []error for _, closer := range a.container.closersLIFO() { - if err := closer.Close(); err != nil && firstErr == nil { - firstErr = err + if err := ctx.Err(); err != nil { + return err } + if err := closer.Close(); err != nil { + errs = append(errs, err) + } + } + if len(errs) == 0 { + a.closed.Store(true) + return nil } - return firstErr + a.closed.Store(true) + return errors.Join(errs...) } diff --git a/modkit/kernel/container_test.go b/modkit/kernel/container_test.go index e7aa5ea..84fd8f5 100644 --- a/modkit/kernel/container_test.go +++ b/modkit/kernel/container_test.go @@ -3,6 +3,7 @@ package kernel_test import ( "context" "errors" + "fmt" "reflect" "sync" "sync/atomic" @@ -34,6 +35,62 @@ func (c *erroringCloser) Close() error { return c.err } +type countingCloser struct { + counter *atomic.Int32 +} + +func (c *countingCloser) Close() error { + c.counter.Add(1) + return nil +} + +type cancelingCloser struct { + cancel func() +} + +func (c *cancelingCloser) Close() error { + c.cancel() + return nil +} + +type testCloser interface { + Close() error +} + +func newTestAppWithClosers(t *testing.T, closers ...testCloser) *kernel.App { + t.Helper() + + providers := make([]module.ProviderDef, 0, len(closers)) + tokens := make([]module.Token, 0, len(closers)) + + for i, closer := range closers { + token := module.Token(fmt.Sprintf("closer.%d", i)) + c := closer + providers = append(providers, module.ProviderDef{ + Token: token, + Build: func(_ module.Resolver) (any, error) { + return c, nil + }, + }) + tokens = append(tokens, token) + } + + modA := mod("A", nil, providers, nil, nil) + + app, err := kernel.Bootstrap(modA) + if err != nil { + t.Fatalf("Bootstrap failed: %v", err) + } + + for _, token := range tokens { + if _, err := app.Get(token); err != nil { + t.Fatalf("Get %s failed: %v", token, err) + } + } + + return app +} + func TestAppGetRejectsNotVisibleToken(t *testing.T) { modA := mod("A", nil, nil, nil, nil) @@ -533,3 +590,141 @@ func TestAppCloseContinuesAfterError(t *testing.T) { t.Fatalf("expected reverse close order with all closers, got %v", closed) } } + +func TestAppCloseAggregatesErrors(t *testing.T) { + var closed []string + errA := errors.New("close a failed") + errB := errors.New("close b failed") + + app := newTestAppWithClosers( + t, + &erroringCloser{name: "a", closed: &closed, err: errA}, + &erroringCloser{name: "b", closed: &closed, err: errB}, + ) + + err := app.Close() + if err == nil { + t.Fatalf("expected error, got nil") + } + if !errors.Is(err, errA) || !errors.Is(err, errB) { + t.Fatalf("expected aggregated error to include both errA and errB") + } +} + +func TestAppCloseIsIdempotent(t *testing.T) { + var closed []string + app := newTestAppWithClosers( + t, + &recordingCloser{name: "a", closed: &closed}, + ) + + if err := app.Close(); err != nil { + t.Fatalf("expected nil error, got %v", err) + } + if err := app.Close(); err != nil { + t.Fatalf("expected nil error on second close, got %v", err) + } + + if got := len(closed); got != 1 { + t.Fatalf("expected 1 close call, got %d", got) + } +} + +func TestAppCloseContextCanceled(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + counter := &atomic.Int32{} + app := newTestAppWithClosers( + t, + &countingCloser{counter: counter}, + ) + + err := app.CloseContext(ctx) + + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected context.Canceled, got %v", err) + } + if got := counter.Load(); got != 0 { + t.Fatalf("expected 0 closes, got %d", got) + } +} + +func TestAppCloseContextCanceledAllowsLaterClose(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + counter := &atomic.Int32{} + app := newTestAppWithClosers( + t, + &countingCloser{counter: counter}, + ) + + err := app.CloseContext(ctx) + + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected context.Canceled, got %v", err) + } + if got := counter.Load(); got != 0 { + t.Fatalf("expected 0 closes after canceled CloseContext, got %d", got) + } + + if err := app.Close(); err != nil { + t.Fatalf("expected nil error on Close after canceled CloseContext, got %v", err) + } + if got := counter.Load(); got != 1 { + t.Fatalf("expected 1 close after Close, got %d", got) + } +} + +func TestAppCloseContextCanceledAfterCloseReturnsNil(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + counter := &atomic.Int32{} + app := newTestAppWithClosers( + t, + &countingCloser{counter: counter}, + ) + + if err := app.Close(); err != nil { + t.Fatalf("expected nil error on Close, got %v", err) + } + if got := counter.Load(); got != 1 { + t.Fatalf("expected 1 close after Close, got %d", got) + } + + err := app.CloseContext(ctx) + if err != nil { + t.Fatalf("expected nil error on CloseContext after Close, got %v", err) + } + if got := counter.Load(); got != 1 { + t.Fatalf("expected no additional closes after CloseContext, got %d", got) + } +} + +func TestAppCloseContextCanceledMidCloseAllowsLaterClose(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + + counter := &atomic.Int32{} + app := newTestAppWithClosers( + t, + &countingCloser{counter: counter}, + &cancelingCloser{cancel: cancel}, + ) + + err := app.CloseContext(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected context.Canceled, got %v", err) + } + if got := counter.Load(); got != 0 { + t.Fatalf("expected 0 closes after canceled CloseContext, got %d", got) + } + + if err := app.Close(); err != nil { + t.Fatalf("expected nil error on Close after canceled CloseContext, got %v", err) + } + if got := counter.Load(); got != 1 { + t.Fatalf("expected 1 close after Close, got %d", got) + } +}