Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/contract-check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
29 changes: 27 additions & 2 deletions contract_verifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package modular

import (
"context"
"errors"
"fmt"
"sync"
"time"
Expand All @@ -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
Expand Down Expand Up @@ -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,
})
}

Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 == "" {
Expand Down
37 changes: 34 additions & 3 deletions contract_verifier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package modular

import (
"context"
"strings"
"errors"
"testing"
"time"
)
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
}
}
22 changes: 22 additions & 0 deletions parallel_init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading