Skip to content

Conversation

romanchechyotkin
Copy link

@romanchechyotkin romanchechyotkin commented Sep 21, 2025

Description

Added custom suffix for circuit breaker in order not to catch panic via existing name

Fixes #37


  • I have read and followed all requirements in CONTRIBUTING.md;
  • I used LLM/AI assistance to make this pull request;

If you have used LLM/AI assistance please provide model name and full prompt:

Model: {{model-name}}
Prompt: {{prompt}}

Summary by CodeRabbit

  • New Features
    • Added configurable suffix for circuit breaker names, enabling distinct instances per environment or deployment.
  • Tests
    • Updated bulk and integration tests to use unique environment and circuit names per test, improving isolation and reducing interference in parallel runs.
    • Enhanced single-suite tests to generate per-environment unique names.
  • Chores
    • Standardized test setup to propagate environment-specific suffixes into circuit breaker configuration for consistency across suites.

@romanchechyotkin romanchechyotkin changed the title #37 added custom suffix for circuit breaker fix: fixed bug during tests Sep 21, 2025
BulkCircuit: circuitbreaker.Config{
RequestVolumeThreshold: 101, // disable circuit breaker
Timeout: time.Hour,
CustomSuffix: cfg.Name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please verify that this will fix the issue?

I see that we have several suites (for example TestBasicIntegration with name Basic) with multiple tests inside those suites. Each test calls setup.NewTestingEnv with this CustomSuffix so we still can face panics.

Or am I missing something?

Copy link
Author

@romanchechyotkin romanchechyotkin Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, you are right, i missed this point

Copy link
Author

@romanchechyotkin romanchechyotkin Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkharms already updated test configs, check please

@romanchechyotkin romanchechyotkin changed the title fix: fixed bug during tests fix: fixed panic during tests Sep 22, 2025
Copy link

coderabbitai bot commented Sep 25, 2025

📝 Walkthrough

Walkthrough

  • Adds Config.CustomSuffix to network/circuitbreaker/circuitbreaker.go and updates New to append "-" to the circuit name when non-empty, using the modified name for lookup and creation.
  • Updates tests to ensure unique names per test:
    • tests/setup/env.go sets BulkCircuit.CustomSuffix to cfg.Name during Ingestor construction.
    • proxy/bulk/ingestor_test.go initializes BulkCircuit with a unique CustomSuffix per test.
    • tests/suites/single.go and tests/integration_tests/* append test names to s.Config.Name in setup.
    • tests/integration_tests/single_test.go appends an index to each env’s Name.
    • tests/integration_tests/replicas_test.go appends t.Name() to config.Name.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title is overly generic because it simply states “fixed panic during tests” without mentioning circuit breaker naming or custom suffix, so it does not clearly summarize the primary change addressing TOCTOU collisions. Consider updating the title to something more specific like “fix: add custom suffix to circuit breaker names to avoid test panics” to clearly reflect the main change.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues Check ✅ Passed The pull request adds a configurable suffix to circuit breaker names in both production and test code so that each test uses a unique circuit name, directly resolving the TOCTOU race described in issue #37 by preventing name collisions that caused panics.
Out of Scope Changes Check ✅ Passed All modifications across production and multiple test files introduce or propagate the custom suffix for circuit breaker naming to prevent test collisions in alignment with issue #37, and no unrelated functionality or files were altered.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (7)
network/circuitbreaker/circuitbreaker.go (1)

55-56: Clarify intent and scope of CustomSuffix

Nit: consider naming it NameSuffix and documenting it as primarily for tests to avoid metric cardinality surprises in prod if used widely.

tests/suites/single.go (1)

151-152: Avoid cumulative name growth across tests in the same suite instance

BeforeTest mutates s.Config.Name in-place. In testify, the same suite instance runs all tests; names may accumulate like “Basic-TestA-TestB-…”.

Base it on the original name captured at suite construction.

Apply:

 type Single struct {
   Base
   Env *setup.TestingEnv
+  origName string
 }
 
 func NewSingle(cfg *setup.TestingEnvConfig) *Single {
   return &Single{
-    Base: *NewBase(cfg),
+    Base:     *NewBase(cfg),
+    origName: cfg.Name,
   }
 }
 
 func (s *Single) BeforeTest(suiteName, testName string) {
   s.Base.BeforeTest(suiteName, testName)
-  s.Config.Name = fmt.Sprintf("%s-%s", s.Config.Name, testName)
+  s.Config.Name = fmt.Sprintf("%s-%s", s.origName, testName)
   s.Env = setup.NewTestingEnv(s.Config)
 }
tests/integration_tests/single_test.go (1)

545-549: Set per-subtest env name deterministically

Looks good. Minor: you can move cfg.Name = ... before t.Parallel() for clarity, but functionally fine.

proxy/bulk/ingestor_test.go (3)

87-90: Prefer t.Name() to hardcoded strings for suffixes

This avoids drift if the test name changes.

-    BulkCircuit: circuitbreaker.Config{
-      CustomSuffix: "TestProcessDocuments",
-    },
+    BulkCircuit: circuitbreaker.Config{
+      CustomSuffix: t.Name(),
+    },

493-496: Prefer b.Name() in benchmarks for consistency

Keeps suffixes aligned with the benchmark’s actual name.

-  BulkCircuit: circuitbreaker.Config{
-    CustomSuffix: "BenchProcessDocuments",
-  },
+  BulkCircuit: circuitbreaker.Config{
+    CustomSuffix: b.Name(),
+  },

573-579: Prefer t.Name() for this test too

Reduce maintenance when renaming tests.

-    BulkCircuit: circuitbreaker.Config{
-      CustomSuffix: "TestProcessDocumentType",
-    },
+    BulkCircuit: circuitbreaker.Config{
+      CustomSuffix: t.Name(),
+    },
tests/integration_tests/integration_test.go (1)

63-64: Encapsulate per-test env setup

There are 38 calls to setup.NewTestingEnv across your tests; extract the Config.Name = fmt.Sprintf("%s-%s", …) plus NewTestingEnv into a small helper (e.g. a suite method) to eliminate boilerplate.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8d4e256 and 2e4f7a7.

📒 Files selected for processing (7)
  • network/circuitbreaker/circuitbreaker.go (2 hunks)
  • proxy/bulk/ingestor_test.go (4 hunks)
  • tests/integration_tests/integration_test.go (33 hunks)
  • tests/integration_tests/replicas_test.go (1 hunks)
  • tests/integration_tests/single_test.go (1 hunks)
  • tests/setup/env.go (1 hunks)
  • tests/suites/single.go (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
proxy/bulk/ingestor_test.go (3)
network/circuitbreaker/circuitbreaker.go (1)
  • Config (27-57)
proxyapi/ingestor.go (1)
  • NewIngestor (110-168)
proxy/bulk/ingestor.go (3)
  • NewIngestor (94-117)
  • IngestorConfig (52-72)
  • MappingProvider (47-50)
tests/integration_tests/single_test.go (1)
tests/suites/single.go (1)
  • SingleEnvs (160-170)
🔇 Additional comments (3)
tests/integration_tests/replicas_test.go (1)

40-41: Good: include t.Name() in env name to avoid collisions

This aligns with the suffixing approach and prevents cross-test panics.

tests/setup/env.go (1)

351-354: All test circuitbreakers use unique CustomSuffix
tests/setup/env.go uses cfg.Name; proxy/bulk/ingestor_test.go uses distinct literal suffixes.

network/circuitbreaker/circuitbreaker.go (1)

69-71: Ensure atomic circuit creation Appending a suffix still allows two goroutines to race between GetCircuit and MustCreateCircuit. Wrap creation in an atomic path: if cep21/circuit v3 exposes a non-panicking CreateCircuit(name, cfg) → (Circuit, error), use that with a fallback to GetCircuit on “AlreadyExists”; otherwise, serialize MustCreateCircuit calls via sync/singleflight or a per-name mutex. Verify the exact API and error types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TOCTOU Bug in Circuit Breaker
2 participants