Re-enable coverage upload and close remaining coverage gaps

.github/workflows/build.yml

@@ -12,6 +12,11 @@ on:
   push:
   pull_request:
 
+env:
+  # Pinned so new lint rules arrive via a dependency update PR rather than breaking unrelated builds.
+  # renovate: datasource=github-releases depName=golangci/golangci-lint
+  GOLANGCI_LINT_VERSION: v2.12.2
+
 jobs:
   test:
     strategy:
@@ -35,10 +40,12 @@ jobs:
         run: make test-race
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v9
+        with:
+          version: ${{ env.GOLANGCI_LINT_VERSION }}
       - name: Output coverage
         run: go test -v -covermode=count -coverprofile=coverage.out ./...
       - name: upload coverage
         uses: shogo82148/actions-goveralls@v1
-        if: github.ref == 'refs/heads/main' && github.event_name == 'push' && matrix.go-version == '1.26.x'
+        if: github.ref == 'refs/heads/master' && github.event_name == 'push' && matrix.go-version == '1.26.x'
         with:
           path-to-profile: coverage.out

README.md

@@ -458,7 +458,7 @@ make fuzz     # active fuzzing (FUZZTIME per target, default 30s)
 make help     # full target list
 ```
 
-`make ci` mirrors the GitHub Actions workflow: build, `go test -race -count 10`, and `golangci-lint run`. If it passes locally, CI should pass.
+`make ci` mirrors the GitHub Actions workflow: build, `go test -race -count 10`, and `golangci-lint run`. If it passes locally, CI should pass. CI pins the golangci-lint version (`GOLANGCI_LINT_VERSION` in `.github/workflows/build.yml`); use the same version locally if results differ.
 
 # [Example](https://github.com/cep21/circuit/blob/master/example/main.go)
 

circuit_test.go

@@ -786,3 +786,64 @@ func TestCloseCircuit_EmitsClosedExactlyOnce(t *testing.T) {
 		t.Errorf("Closed() called %d times, want exactly 1", got)
 	}
 }
+
+func TestNilReceivers(t *testing.T) {
+	var c *Circuit
+	if c.Name() != "" {
+		t.Error("nil circuit should have no name")
+	}
+	if s := c.Var().String(); s != "null" {
+		t.Errorf("unexpected Var for nil circuit: %s", s)
+	}
+	var h *Manager
+	if h.AllCircuits() != nil {
+		t.Error("nil manager should have no circuits")
+	}
+	if h.GetCircuit("anything") != nil {
+		t.Error("nil manager should find no circuit")
+	}
+}
+
+// A ForcedClosed circuit never transitions to open, even when its opener wants to, so clearing the override
+// leaves it closed rather than springing open from stale state.
+func TestForcedClosedNeverOpens(t *testing.T) {
+	ctx := context.Background()
+	var opened faststats.AtomicInt64
+	c := NewCircuitFromConfig("forced-closed", Config{
+		General: GeneralConfig{
+			ForcedClosed:        true,
+			ClosedToOpenFactory: func() ClosedToOpen { return alwaysOpens{} },
+		},
+		Metrics: MetricsCollectors{Circuit: []Metrics{openedCounter{&opened}}},
+	})
+	for i := 0; i < 5; i++ {
+		err := c.Execute(ctx, testhelp.AlwaysFails, nil)
+		if err == nil {
+			t.Fatal("expected the run error back")
+		}
+	}
+	if c.IsOpen() || opened.Get() != 0 {
+		t.Fatalf("forced-closed circuit opened: IsOpen=%v opened=%d", c.IsOpen(), opened.Get())
+	}
+	cfg := c.Config()
+	cfg.General.ForcedClosed = false
+	c.SetConfigThreadSafe(cfg)
+	if c.IsOpen() {
+		t.Fatal("clearing ForcedClosed must not reveal an open circuit")
+	}
+	if err := c.Execute(ctx, testhelp.AlwaysFails, nil); err == nil {
+		t.Fatal("expected the run error back")
+	}
+	if !c.IsOpen() || opened.Get() != 1 {
+		t.Fatalf("circuit should open once the override is gone: IsOpen=%v opened=%d", c.IsOpen(), opened.Get())
+	}
+}
+
+type alwaysOpens struct{ neverOpens }
+
+func (alwaysOpens) ShouldOpen(context.Context, time.Time) bool { return true }
+
+type openedCounter struct{ n *faststats.AtomicInt64 }
+
+func (o openedCounter) Opened(context.Context, time.Time) { o.n.Add(1) }
+func (o openedCounter) Closed(context.Context, time.Time) {}

closers/hystrix/opener_test.go

@@ -2,6 +2,7 @@ package hystrix
 
 import (
 	"context"
+	"encoding/json"
 	"strings"
 	"sync"
 	"testing"
@@ -96,3 +97,28 @@ func TestOpenerFactory_ConcurrentCreation(t *testing.T) {
 	}
 	wg.Wait()
 }
+
+func TestOpener_MarshalJSON_ErrPercentage(t *testing.T) {
+	ctx := context.Background()
+	now := time.Now()
+	o := OpenerFactory(ConfigureOpener{Now: func() time.Time { return now }})().(*Opener)
+	o.Success(ctx, now, time.Millisecond)
+	o.Success(ctx, now, time.Millisecond)
+	o.Success(ctx, now, time.Millisecond)
+	o.ErrFailure(ctx, now, time.Millisecond)
+	b, err := o.MarshalJSON()
+	if err != nil {
+		t.Fatal(err)
+	}
+	var out map[string]json.RawMessage
+	if err := json.Unmarshal(b, &out); err != nil {
+		t.Fatal(err)
+	}
+	var pct float64
+	if err := json.Unmarshal(out["err_%"], &pct); err != nil {
+		t.Fatalf("%s: %v", b, err)
+	}
+	if pct != 0.25 {
+		t.Fatalf("expected 25%% errors, got %v from %s", pct, b)
+	}
+}

closers_test.go

@@ -21,3 +21,104 @@ func TestNeverClose(t *testing.T) {
 	require.False(t, c.Allow(ctx, time.Now()))
 	require.False(t, c.ShouldClose(ctx, time.Now()))
 }
+
+// TestNeverOpensNeverClosesIgnoreEvents runs every RunMetrics/Metrics event through the default no-op logic and
+// checks none of them change its answer.
+func TestNeverOpensNeverClosesIgnoreEvents(t *testing.T) {
+	ctx := context.Background()
+	now := time.Now()
+	opener := neverOpensFactory()
+	closer := neverClosesFactory()
+	for _, m := range []interface {
+		RunMetrics
+		Metrics
+	}{opener, closer} {
+		m.Success(ctx, now, time.Millisecond)
+		m.ErrFailure(ctx, now, time.Millisecond)
+		m.ErrTimeout(ctx, now, time.Millisecond)
+		m.ErrBadRequest(ctx, now, time.Millisecond)
+		m.ErrInterrupt(ctx, now, time.Millisecond)
+		m.ErrConcurrencyLimitReject(ctx, now)
+		m.ErrShortCircuit(ctx, now)
+		m.Opened(ctx, now)
+		m.Closed(ctx, now)
+	}
+	require.False(t, opener.ShouldOpen(ctx, now))
+	require.False(t, opener.Prevent(ctx, now))
+	require.False(t, closer.ShouldClose(ctx, now))
+	require.False(t, closer.Allow(ctx, now))
+}
+
+type configurableOpener struct {
+	neverOpens
+	prevent               bool
+	threadSafe, notThread []Config
+}
+
+func (c *configurableOpener) Prevent(context.Context, time.Time) bool { return c.prevent }
+func (c *configurableOpener) SetConfigThreadSafe(props Config) {
+	c.threadSafe = append(c.threadSafe, props)
+}
+func (c *configurableOpener) SetConfigNotThreadSafe(props Config) {
+	c.notThread = append(c.notThread, props)
+}
+
+type configurableCloser struct {
+	neverCloses
+	threadSafe, notThread []Config
+}
+
+func (c *configurableCloser) SetConfigThreadSafe(props Config) {
+	c.threadSafe = append(c.threadSafe, props)
+}
+func (c *configurableCloser) SetConfigNotThreadSafe(props Config) {
+	c.notThread = append(c.notThread, props)
+}
+
+// Open/close logic that implements Configurable is handed the circuit's config on construction and on live updates.
+func TestConfigurableLogicReceivesConfig(t *testing.T) {
+	opener := &configurableOpener{}
+	closer := &configurableCloser{}
+	c := NewCircuitFromConfig("configurable", Config{
+		General: GeneralConfig{
+			ClosedToOpenFactory: func() ClosedToOpen { return opener },
+			OpenToClosedFactory: func() OpenToClosed { return closer },
+		},
+		Execution: ExecutionConfig{MaxConcurrentRequests: 3},
+	})
+	require.Len(t, opener.notThread, 1)
+	require.Len(t, closer.notThread, 1)
+	require.Equal(t, int64(3), opener.notThread[0].Execution.MaxConcurrentRequests)
+	liveUpdates := len(opener.threadSafe)
+
+	cfg := c.Config()
+	cfg.Execution.MaxConcurrentRequests = 9
+	c.SetConfigThreadSafe(cfg)
+	require.Len(t, opener.notThread, 1)
+	require.Len(t, opener.threadSafe, liveUpdates+1)
+	require.Len(t, closer.threadSafe, liveUpdates+1)
+	require.Equal(t, int64(9), opener.threadSafe[liveUpdates].Execution.MaxConcurrentRequests)
+	require.Equal(t, int64(9), closer.threadSafe[liveUpdates].Execution.MaxConcurrentRequests)
+}
+
+// ClosedToOpen.Prevent short-circuits a request on a closed circuit without opening it.
+func TestPreventShortCircuits(t *testing.T) {
+	opener := &configurableOpener{prevent: true}
+	c := NewCircuitFromConfig("prevent", Config{
+		General: GeneralConfig{ClosedToOpenFactory: func() ClosedToOpen { return opener }},
+	})
+	ran := false
+	err := c.Execute(context.Background(), func(context.Context) error {
+		ran = true
+		return nil
+	}, nil)
+	require.Error(t, err)
+	require.False(t, ran)
+	require.False(t, c.IsOpen())
+	var ce Error
+	require.ErrorAs(t, err, &ce)
+	require.True(t, ce.CircuitOpen())
+
+	opener.prevent = false
+	require.NoError(t, c.Execute(context.Background(), func(context.Context) error { return nil }, nil))
+}

config_test.go

@@ -82,3 +82,33 @@ func TestExecutionConfig_Merge(t *testing.T) {
 		assert.True(t, cfg.IsErrInterrupt(nil))
 	})
 }
+
+func TestGeneralConfig_MergeCustomConfig(t *testing.T) {
+	t.Run("nothing to merge leaves nil map", func(t *testing.T) {
+		cfg := GeneralConfig{}
+		cfg.merge(GeneralConfig{})
+		assert.Nil(t, cfg.CustomConfig)
+	})
+
+	t.Run("copies into nil receiver map without aliasing", func(t *testing.T) {
+		other := GeneralConfig{CustomConfig: map[interface{}]interface{}{"a": 1, "b": "two"}}
+		cfg := GeneralConfig{}
+		cfg.merge(other)
+		assert.Equal(t, map[interface{}]interface{}{"a": 1, "b": "two"}, cfg.CustomConfig)
+		cfg.CustomConfig["c"] = 3
+		_, leaked := other.CustomConfig["c"]
+		assert.False(t, leaked, "merge must copy, not alias, the other map")
+	})
+
+	t.Run("receiver keys win", func(t *testing.T) {
+		cfg := GeneralConfig{CustomConfig: map[interface{}]interface{}{"a": "mine"}}
+		cfg.merge(GeneralConfig{CustomConfig: map[interface{}]interface{}{"a": "other-a", "b": "other-b"}})
+		assert.Equal(t, map[interface{}]interface{}{"a": "mine", "b": "other-b"}, cfg.CustomConfig)
+	})
+
+	t.Run("Config.Merge carries CustomConfig", func(t *testing.T) {
+		cfg := Config{}
+		cfg.Merge(Config{General: GeneralConfig{CustomConfig: map[interface{}]interface{}{"k": "v"}}})
+		assert.Equal(t, "v", cfg.General.CustomConfig["k"])
+	})
+}

errors_test.go

@@ -28,3 +28,13 @@ func TestIsBadRequest(t *testing.T) {
 	require.True(t, IsBadRequest(wrappedErr))
 	require.False(t, IsBadRequest(fmt.Errorf("wrapped: %w", errors.New("not bad"))))
 }
+
+func TestSimpleBadRequest_CauseUnwrap(t *testing.T) {
+	inner := errors.New("inner")
+	s := SimpleBadRequest{Err: inner}
+	require.Same(t, inner, s.Cause())
+	require.Same(t, inner, s.Unwrap())
+	require.ErrorIs(t, s, inner)
+	require.ErrorIs(t, fmt.Errorf("wrapped: %w", s), inner)
+	require.Equal(t, "inner", s.Error())
+}

example/main.go

@@ -25,42 +25,59 @@ import (
 const exampleURL = "http://localhost:7979/hystrix-dashboard/monitor/monitor.html?streams=%5B%7B%22name%22%3A%22%22%2C%22stream%22%3A%22http%3A%2F%2Flocalhost%3A8123%2Fhystrix.stream%22%2C%22auth%22%3A%22%22%2C%22delay%22%3A%22%22%7D%5D"
 
 func main() {
+	interval := flag.Duration("interval", time.Millisecond*100, "Setup duration between metric ticks")
+	addr := flag.String("addr", "127.0.0.1:8123", "Address to listen on")
+	flag.Parse()
+	sock, err := net.Listen("tcp", *addr)
+	if err != nil {
+		log.Fatal(err)
+	}
+	printInstructions(sock.Addr().String())
+	handler, es := newHandler(*interval)
+	go func() {
+		mustPass(es.Start())
+	}()
+	srv := http.Server{Handler: handler, ReadHeaderTimeout: time.Second}
+	log.Fatal(srv.Serve(sock))
+}
+
+// newHandler creates the example circuits and returns a handler serving /hystrix.stream and /debug/vars, plus the
+// event stream backing /hystrix.stream.  The caller must Start and eventually Close the stream.
+func newHandler(interval time.Duration) (http.Handler, *metriceventstream.MetricEventStream) {
 	f := rolling.StatFactory{}
-	h := circuit.Manager{
+	h := &circuit.Manager{
 		DefaultCircuitProperties: []circuit.CommandPropertiesConstructor{f.CreateConfig},
 	}
-	expvar.Publish("hystrix", h.Var())
-	es := metriceventstream.MetricEventStream{
-		Manager: &h,
+	if expvar.Get("hystrix") == nil {
+		expvar.Publish("hystrix", h.Var())
 	}
-	go func() {
-		log.Fatal(es.Start())
-	}()
-	interval := flag.Duration("interval", time.Millisecond*100, "Setup duration between metric ticks")
-	flag.Parse()
-	createBackgroundCircuits(&h, *interval)
-	http.Handle("/hystrix.stream", &es)
-	sock, err := net.Listen("tcp", "127.0.0.1:8123")
-	if err != nil {
-		panic(err)
+	es := &metriceventstream.MetricEventStream{
+		Manager: h,
 	}
-	log.Println("Serving on socket :8123")
+	createBackgroundCircuits(h, interval)
+	mux := http.NewServeMux()
+	mux.Handle("/hystrix.stream", es)
+	mux.Handle("/debug/vars", expvar.Handler())
+	return mux, es
+}
+
+func printInstructions(addr string) {
+	log.Printf("Serving on socket %s\n", addr)
 	log.Println("To view the stream, execute: ")
-	log.Println("  curl http://127.0.0.1:8123/hystrix.stream")
+	log.Printf("  curl http://%s/hystrix.stream\n", addr)
 	log.Println()
 	log.Println("To view expvar metrics, visit expvar in your browser")
-	log.Println("  http://127.0.0.1:8123/debug/vars")
+	log.Printf("  http://%s/debug/vars\n", addr)
 	log.Println()
 	log.Println("To view a dashboard, follow the instructions at https://github.com/Netflix-Skunkworks/hystrix-dashboard#run-via-gradle")
 	log.Println("  git clone git@github.com:Netflix-Skunkworks/hystrix-dashboard.git")
 	log.Println("  cd hystrix-dashboard")
 	log.Println("  ./gradlew jettyRun")
 	log.Println()
-	log.Println("Then, add the stream http://127.0.0.1:8123/hystrix.stream")
+	log.Printf("Then, add the stream http://%s/hystrix.stream\n", addr)
 	log.Println()
-	log.Println("A URL directly to the page usually looks something like this")
+	log.Println("A URL directly to the page usually looks something like this (adjust the port if you changed -addr)")
 	log.Printf("   %s\n", exampleURL)
-	log.Fatal(http.Serve(sock, nil))
 }
 
 func mustFail(err error) {