diff --git a/.github/workflows/contract-check.yml b/.github/workflows/contract-check.yml index 47d394da..bc38e3c1 100644 --- a/.github/workflows/contract-check.yml +++ b/.github/workflows/contract-check.yml @@ -178,7 +178,7 @@ jobs: - name: Comment PR with contract changes if: steps.contract-diff.outputs.has_changes == 'true' - uses: actions/github-script@v8 + uses: actions/github-script@v9 with: script: | const fs = require('fs'); diff --git a/contract_verifier.go b/contract_verifier.go index 205e8428..612c0cfd 100644 --- a/contract_verifier.go +++ b/contract_verifier.go @@ -2,6 +2,7 @@ package modular import ( "context" + "errors" "fmt" "sync" "time" @@ -13,6 +14,7 @@ type ContractViolation struct { Rule string // e.g., "must-return-positive-timeout" Description string Severity string // "error" or "warning" + Err error // underlying error, if applicable } // ContractVerifier verifies that implementations of Reloadable and HealthProvider @@ -60,11 +62,16 @@ func (v *StandardContractVerifier) VerifyReloadContract(module Reloadable) []Con // 3. Reload with empty changes should be idempotent. if err := v.checkReloadIdempotent(module); err != nil { + rule := "empty-reload-must-be-idempotent" + if errors.Is(err, ErrReloadPanic) { + rule = "reload-must-not-panic" + } violations = append(violations, ContractViolation{ Contract: "reload", - Rule: "empty-reload-must-be-idempotent", + Rule: rule, Description: fmt.Sprintf("Reload() with empty changes failed: %v", err), Severity: "warning", + Err: err, }) } @@ -149,10 +156,17 @@ func (v *StandardContractVerifier) runReloadWithGuard(module Reloadable, label s // checkReloadRespectsCancel calls Reload with an already-cancelled context and // returns true if Reload returned an error (i.e., it respected the cancellation). -func (v *StandardContractVerifier) checkReloadRespectsCancel(module Reloadable) bool { +// A panic from Reload is treated as "respected cancellation" (error path). +func (v *StandardContractVerifier) checkReloadRespectsCancel(module Reloadable) (respected bool) { ctx, cancel := context.WithCancel(context.Background()) cancel() // cancel immediately + defer func() { + if r := recover(); r != nil { + // A panic counts as a non-nil error path. + respected = true + } + }() err := module.Reload(ctx, nil) return err != nil } @@ -195,6 +209,17 @@ func (v *StandardContractVerifier) VerifyHealthContract(provider HealthProvider) // Can't check fields if we timed out. return violations case res := <-ch: + if errors.Is(res.err, ErrHealthCheckPanic) { + violations = append(violations, ContractViolation{ + Contract: "health", + Rule: "health-check-must-not-panic", + Description: fmt.Sprintf("HealthCheck() panicked: %v", res.err), + Severity: "error", + Err: res.err, + }) + // Skip further checks since the provider panics. + return violations + } if res.err == nil { for _, report := range res.reports { if report.Module == "" { diff --git a/contract_verifier_test.go b/contract_verifier_test.go index 34e90c8e..5878ab03 100644 --- a/contract_verifier_test.go +++ b/contract_verifier_test.go @@ -2,7 +2,7 @@ package modular import ( "context" - "strings" + "errors" "testing" "time" ) @@ -148,8 +148,7 @@ func TestContractVerifier_ReloadPanicUsesSentinelError(t *testing.T) { found := false for _, v := range violations { - if v.Rule == "empty-reload-must-be-idempotent" && - strings.Contains(v.Description, ErrReloadPanic.Error()) { + if v.Rule == "reload-must-not-panic" && errors.Is(v.Err, ErrReloadPanic) { found = true break } @@ -207,3 +206,35 @@ func TestContractVerifier_HealthPanicIsGuarded(t *testing.T) { // not active. _ = verifier.VerifyHealthContract(&panicOnActiveHealthProvider{}) } + +func TestContractVerifier_ReloadPanicWrapsErrReloadPanic(t *testing.T) { + verifier := NewStandardContractVerifier() + violations := verifier.VerifyReloadContract(&reloadPanicReloadable{}) + + found := false + for _, v := range violations { + if v.Rule == "reload-must-not-panic" && errors.Is(v.Err, ErrReloadPanic) { + found = true + break + } + } + if !found { + t.Fatalf("expected violation with ErrReloadPanic, got: %+v", violations) + } +} + +func TestContractVerifier_HealthPanicWrapsErrHealthCheckPanic(t *testing.T) { + verifier := NewStandardContractVerifier() + violations := verifier.VerifyHealthContract(&panicOnActiveHealthProvider{}) + + found := false + for _, v := range violations { + if v.Rule == "health-check-must-not-panic" && errors.Is(v.Err, ErrHealthCheckPanic) { + found = true + break + } + } + if !found { + t.Fatalf("expected violation with ErrHealthCheckPanic, got: %+v", violations) + } +} diff --git a/parallel_init_test.go b/parallel_init_test.go index 5d8e15a6..de53540e 100644 --- a/parallel_init_test.go +++ b/parallel_init_test.go @@ -115,6 +115,28 @@ func TestWithParallelInit_RecoversModulePanic(t *testing.T) { } } +func TestWithParallelInit_ConcurrentPanicsWrapErrModuleInitializationPanic(t *testing.T) { + modA := &panicInitModule{name: "panic-a"} + modB := &panicInitModule{name: "panic-b"} + + app, err := NewApplication( + WithLogger(nopLogger{}), + WithModules(modA, modB), + WithParallelInit(), + ) + if err != nil { + t.Fatalf("NewApplication: %v", err) + } + + initErr := app.Init() + if initErr == nil { + t.Fatal("expected Init to return an error after panic, got nil") + } + if !errors.Is(initErr, ErrModuleInitializationPanic) { + t.Fatalf("expected error to wrap ErrModuleInitializationPanic, got: %v", initErr) + } +} + type simpleOrderModule struct { name string deps []string