Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new Ktor HTTP client plugin implementing per-name circuit breakers with CLOSED→OPEN→HALF-OPEN state machine, configurable thresholds, request tagging/routing, failure-detection predicate, exception type for rejections, documentation, build files, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt`:
- Around line 173-176: The HALF_OPEN branch currently throws
CircuitBreakerOpenException when halfOpenAttempts >= config.halfOpenRequests
which misleadingly reports the circuit as OPEN; update this to either throw a
distinct exception (e.g., CircuitBreakerHalfOpenSaturatedException) or at
minimum change the error message to indicate the circuit is HALF_OPEN and
saturated (include name, config.resetTimeout and current
halfOpenAttempts/config.halfOpenRequests), and adjust any callers that rely on
CircuitBreakerOpenException to handle the new exception or message accordingly;
locate this logic in the block that checks CircuitState.HALF_OPEN and references
halfOpenAttempts and config.halfOpenRequests to make the change.
- Around line 351-355: The catch-all in CircuitBreaker.kt around the
proceed(request) call records every Throwable via instance.recordFailure(),
which incorrectly treats CancellationException as a service failure; update the
try/catch in the call execution (the block that calls proceed(request) inside
the CircuitBreaker plugin) to explicitly rethrow CancellationException (and
subclasses) without calling instance.recordFailure(), and only call
instance.recordFailure() for non-cancellation throwables; then add a regression
test exercising a locally-cancelled request to assert the circuit state does not
record a failure (i.e., circuit stays closed).
- Around line 329-335: The unsynchronized read of the MutableMap instances in
getInstance creates a race; move the initial lookup inside the
instancesMutex.withLock so the entire lookup-and-create path is protected.
Specifically, remove the pre-lock instances[name]?.let return check and perform
instances.getOrPut(name) { CircuitBreakerInstance(name, config.globalConfig,
timeSource) } inside instancesMutex.withLock in the getInstance function so all
access to instances is guarded by instancesMutex.
In `@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/README.md`:
- Around line 13-30: The fenced diagram in the README.md lacks a language tag
which triggers markdownlint MD040; update the fence that contains the
circuit-breaker state diagram so the opening triple-backtick includes the
language identifier text (i.e., change the fence from ``` to ```text) so the
markdown linter recognizes it as a plain text block and the lint error is
resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e5c17e8-4d22-40e4-b796-0e667c5fec4a
📒 Files selected for processing (7)
ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/README.mdktor-client/ktor-client-plugins/ktor-client-circuit-breaker/api/ktor-client-circuit-breaker.apiktor-client/ktor-client-plugins/ktor-client-circuit-breaker/api/ktor-client-circuit-breaker.klib.apiktor-client/ktor-client-plugins/ktor-client-circuit-breaker/build.gradle.ktsktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.ktktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/test/io/ktor/client/plugins/circuitbreaker/CircuitBreakerTest.ktsettings.gradle.kts
Comment on lines
+173
to
+176
| CircuitState.HALF_OPEN -> { | ||
| if (halfOpenAttempts >= config.halfOpenRequests) { | ||
| throw CircuitBreakerOpenException(name, config.resetTimeout) | ||
| } |
Contributor
There was a problem hiding this comment.
Half-open overflow is reported as OPEN.
These extra half-open probes are rejected with CircuitBreakerOpenException, but that exception says the circuit is OPEN and blocked until resetTimeout elapses. In this branch the circuit is still HALF_OPEN, and the next request may be allowed as soon as an in-flight probe finishes. Please use a distinct message/path here, or document that this exception also covers half-open saturation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt`
around lines 173 - 176, The HALF_OPEN branch currently throws
CircuitBreakerOpenException when halfOpenAttempts >= config.halfOpenRequests
which misleadingly reports the circuit as OPEN; update this to either throw a
distinct exception (e.g., CircuitBreakerHalfOpenSaturatedException) or at
minimum change the error message to indicate the circuit is HALF_OPEN and
saturated (include name, config.resetTimeout and current
halfOpenAttempts/config.halfOpenRequests), and adjust any callers that rely on
CircuitBreakerOpenException to handle the new exception or message accordingly;
locate this logic in the block that checks CircuitState.HALF_OPEN and references
halfOpenAttempts and config.halfOpenRequests to make the change.
Comment on lines
+329
to
+335
| suspend fun getInstance(name: String): CircuitBreakerInstance { | ||
| instances[name]?.let { return it } | ||
| return instancesMutex.withLock { | ||
| instances.getOrPut(name) { | ||
| CircuitBreakerInstance(name, config.globalConfig, timeSource) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "CircuitBreaker.kt" | head -20Repository: ktorio/ktor
Length of output: 183
🏁 Script executed:
cat -n ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt | sed -n '1,50p'Repository: ktorio/ktor
Length of output: 2193
🏁 Script executed:
wc -l ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.ktRepository: ktorio/ktor
Length of output: 185
🏁 Script executed:
cat -n ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt | sed -n '300,366p'Repository: ktorio/ktor
Length of output: 2439
🏁 Script executed:
cat -n ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt | sed -n '150,300p'Repository: ktorio/ktor
Length of output: 6734
🏁 Script executed:
cat -n ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt | grep -n "instances"Repository: ktorio/ktor
Length of output: 425
Remove the unsynchronized read of instances before acquiring the mutex.
Line 330 reads the instances MutableMap without holding instancesMutex, creating a race condition when multiple concurrent requests call getInstance with new circuit breaker names. All concurrent threads may read null and proceed to the mutex, though getOrPut ensures only one instance survives, the unprotected read violates thread-safety guarantees for the map. Move the entire lookup inside the mutex:
🔒 Suggested fix
suspend fun getInstance(name: String): CircuitBreakerInstance {
- instances[name]?.let { return it }
- return instancesMutex.withLock {
+ return instancesMutex.withLock {
instances.getOrPut(name) {
CircuitBreakerInstance(name, config.globalConfig, timeSource)
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| suspend fun getInstance(name: String): CircuitBreakerInstance { | |
| instances[name]?.let { return it } | |
| return instancesMutex.withLock { | |
| instances.getOrPut(name) { | |
| CircuitBreakerInstance(name, config.globalConfig, timeSource) | |
| } | |
| } | |
| suspend fun getInstance(name: String): CircuitBreakerInstance { | |
| return instancesMutex.withLock { | |
| instances.getOrPut(name) { | |
| CircuitBreakerInstance(name, config.globalConfig, timeSource) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt`
around lines 329 - 335, The unsynchronized read of the MutableMap instances in
getInstance creates a race; move the initial lookup inside the
instancesMutex.withLock so the entire lookup-and-create path is protected.
Specifically, remove the pre-lock instances[name]?.let return check and perform
instances.getOrPut(name) { CircuitBreakerInstance(name, config.globalConfig,
timeSource) } inside instancesMutex.withLock in the getInstance function so all
access to instances is guarded by instancesMutex.
Comment on lines
+351
to
+355
| val call = try { | ||
| proceed(request) | ||
| } catch (cause: Throwable) { | ||
| instance.recordFailure() | ||
| throw cause |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt | head -400 | tail -100Repository: ktorio/ktor
Length of output: 4150
🏁 Script executed:
# Get more context around the mentioned lines
sed -n '340,365p' ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.ktRepository: ktorio/ktor
Length of output: 768
🏁 Script executed:
# Search for CancellationException handling in the file
rg -n "CancellationException" ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.ktRepository: ktorio/ktor
Length of output: 37
🏁 Script executed:
# Look for the entire intercept method/block to understand full context
sed -n '320,380p' ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.ktRepository: ktorio/ktor
Length of output: 1495
🏁 Script executed:
# Check test files for cancellation-related tests
find ktor-client/ktor-client-plugins/ktor-client-circuit-breaker -name "*.kt" -path "*/test/*" | head -20Repository: ktorio/ktor
Length of output: 186
🏁 Script executed:
# Examine the test file for cancellation-related tests
cat -n ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/test/io/ktor/client/plugins/circuitbreaker/CircuitBreakerTest.ktRepository: ktorio/ktor
Length of output: 15885
🏁 Script executed:
# Search for CancellationException handling patterns in Ktor codebase
rg -A 3 "catch.*CancellationException" ktor-client/ktor-client-core --type kt | head -50Repository: ktorio/ktor
Length of output: 81
🏁 Script executed:
# Check Kotlin coroutine documentation patterns - search for how other plugins handle cancellation
rg "is CancellationException\|CancellationException ->" --type kt ktor-client | head -20Repository: ktorio/ktor
Length of output: 81
Catch and rethrow CancellationException without recording a failure.
The catch-all here records every thrown Throwable as a service failure, including CancellationException from caller or scope cancellation. This causes locally aborted requests to open the circuit even when the downstream service is healthy. Rethrow cancellation exceptions without calling recordFailure(), and add a regression test to prevent this in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt`
around lines 351 - 355, The catch-all in CircuitBreaker.kt around the
proceed(request) call records every Throwable via instance.recordFailure(),
which incorrectly treats CancellationException as a service failure; update the
try/catch in the call execution (the block that calls proceed(request) inside
the CircuitBreaker plugin) to explicitly rethrow CancellationException (and
subclasses) without calling instance.recordFailure(), and only call
instance.recordFailure() for non-cancellation throwables; then add a regression
test exercising a locally-cancelled request to assert the circuit state does not
record a failure (i.e., circuit stays closed).
Comment on lines
+13
to
+30
| ``` | ||
| failures >= threshold | ||
| ┌────────┐ ┌──────┐ | ||
| │ CLOSED ├─────────────────►│ OPEN │ | ||
| └───┬────┘ └──┬───┘ | ||
| │ │ resetTimeout elapsed | ||
| │ all trial requests │ | ||
| │ succeed ▼ | ||
| │ ┌───────────┐ | ||
| └────────────────────┤ HALF-OPEN │ | ||
| └─────┬─────┘ | ||
| │ any trial request fails | ||
| │ | ||
| ▼ | ||
| ┌──────┐ | ||
| │ OPEN │ | ||
| └──────┘ | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to the state-diagram fence.
This block triggers markdownlint MD040 because the fence has no language. Mark it as text so docs lint stays clean.
✏️ Suggested fix
-```
+```text
failures >= threshold
┌────────┐ ┌──────┐
│ CLOSED ├─────────────────►│ OPEN │
└───┬────┘ └──┬───┘
│ │ resetTimeout elapsed
│ all trial requests │
│ succeed ▼
│ ┌───────────┐
└────────────────────┤ HALF-OPEN │
└─────┬─────┘
│ any trial request fails
│
▼
┌──────┐
│ OPEN │
└──────┘</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/README.md` around
lines 13 - 30, The fenced diagram in the README.md lacks a language tag which
triggers markdownlint MD040; update the fence that contains the circuit-breaker
state diagram so the opening triple-backtick includes the language identifier
text (i.e., change the fence from ``` to ```text) so the markdown linter
recognizes it as a plain text block and the lint error is resolved.
Subhanshu20135
force-pushed
the
feature/circuit-breaker
branch
from
54fb87c to
552b182
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/README.md (1)
13-30:⚠️ Potential issue | 🟡 MinorStill missing a language on the diagram fence.
Line 13 still opens the state-diagram block without a language, so markdownlint MD040 will keep firing.
✏️ Suggested fix
-``` +```text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/README.md` around lines 13 - 30, The code block containing the ASCII state diagram in README.md is missing a language specifier; update the opening fence (the triple backticks that start the ASCII diagram block) to include a language such as text (i.e., change ``` to ```text) so markdownlint MD040 stops flagging it and the diagram renders correctly.ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt (3)
329-335:⚠️ Potential issue | 🔴 CriticalStill reading
instancesoutsideinstancesMutex.Line 330 performs an unsynchronized read from a shared
MutableMapbefore acquiringinstancesMutex. Since dynamic circuit creation writes to the same map inside the lock, this is still a real race; please move the whole lookup into the critical section.🔒 Suggested fix
suspend fun getInstance(name: String): CircuitBreakerInstance { - instances[name]?.let { return it } return instancesMutex.withLock { instances.getOrPut(name) { CircuitBreakerInstance(name, config.globalConfig, timeSource) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt` around lines 329 - 335, The current getInstance reads the shared MutableMap instances outside the instancesMutex, causing a race; move the entire lookup/creation logic into the critical section by removing the early unsynchronized instances[name] check and performing instances.getOrPut(name) inside instancesMutex.withLock so that all accesses to instances (read or write) happen under instancesMutex; keep creating the CircuitBreakerInstance(name, config.globalConfig, timeSource) as the getOrPut default.
351-355:⚠️ Potential issue | 🟠 MajorStill counting cancellations as downstream failures.
Line 353 catches every
Throwable, and Line 354 records it as a circuit failure. That still includes coroutine cancellation, so caller-aborted requests can open the breaker even when the service is healthy. Re-throwCancellationExceptionbeforerecordFailure(), add a regression test for a locally cancelled request, and tighten the README wording so it does not imply that all exceptions count.🛠️ Suggested fix
val call = try { proceed(request) } catch (cause: Throwable) { + if (cause is CancellationException) throw cause instance.recordFailure() throw cause }This checks the current catch block, confirms how
CancellationExceptionis defined in the repo, and shows whether this plugin already has cancellation-focused coverage. Expect the catch block to lack a cancellation guard and the test search to return no dedicated regression case.#!/bin/bash set -euo pipefail FILE='ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt' TEST_FILE='ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/test/io/ktor/client/plugins/circuitbreaker/CircuitBreakerTest.kt' echo "== catch block in CircuitBreaker plugin ==" sed -n '349,356p' "$FILE" echo echo "== repo definitions of CancellationException ==" rg -n --type=kt 'typealias\s+CancellationException\s*=|class\s+CancellationException\b' echo echo "== cancellation-related coverage in CircuitBreakerTest ==" rg -n --type=kt 'CancellationException|cancel' "$TEST_FILE"Based on learnings,
io.ktor.utils.io.CancellationExceptionis a typealias forkotlinx.coroutines.CancellationException, so that single type check excludes all coroutine cancellation exceptions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt` around lines 351 - 355, The catch in the CircuitBreaker around proceed(request) currently treats every Throwable as a failure; change the handler so that CancellationException is re-thrown immediately (e.g., in the catch for cause: Throwable do if (cause is CancellationException) throw cause) before calling instance.recordFailure() and re-throwing other exceptions; add a regression test in CircuitBreakerTest that cancels a locally started request and asserts the breaker does not record/open from that cancellation; and update the README wording for the circuit breaker to explicitly state that coroutine cancellations are excluded from failure counts.
173-176:⚠️ Potential issue | 🟡 MinorStill reporting HALF_OPEN saturation as
OPEN.Line 175 rejects extra half-open probes with
CircuitBreakerOpenException, but this branch is saturatedHALF_OPEN, notOPEN. That makes the exposed state and retry timing wrong for callers, because another request may be allowed as soon as a probe finishes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt` around lines 173 - 176, The code currently throws CircuitBreakerOpenException when halfOpenAttempts >= config.halfOpenRequests inside the CircuitState.HALF_OPEN branch; change this to surface the correct HALF_OPEN condition: replace the thrown CircuitBreakerOpenException(name, config.resetTimeout) with a distinct exception (e.g., CircuitBreakerHalfOpenException) that indicates the circuit is saturated in HALF_OPEN (or extend CircuitBreakerOpenException to accept a state parameter and pass CircuitState.HALF_OPEN). Update any callers/tests to expect the new exception type or check the returned state accordingly; reference CircuitState.HALF_OPEN, halfOpenAttempts, config.halfOpenRequests, and the thrown exception in your changes.
🧹 Nitpick comments (1)
ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt (1)
267-277: Add the standard feedback link to this public extension KDoc.
HttpRequestBuilder.circuitBreakeris public API, but its KDoc stops after the description. Please add the repo’s usual[Report a problem]footer so the generated docs stay consistent with other public extensions. Based on learnings, public API extension functions in this repo should include a brief description and a[Report a problem]link.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt` around lines 267 - 277, The public extension function HttpRequestBuilder.circuitBreaker is missing the repo's standard "[Report a problem]" KDoc footer; update the KDoc for circuitBreaker (the block above the function that references CircuitBreakerConfig.register and CircuitBreakerConfig.global) to append the standard Report a problem link/footer used across public APIs so generated docs match other extensions, keeping existing description and references to CircuitBreakerNameKey unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt`:
- Around line 329-335: The current getInstance reads the shared MutableMap
instances outside the instancesMutex, causing a race; move the entire
lookup/creation logic into the critical section by removing the early
unsynchronized instances[name] check and performing instances.getOrPut(name)
inside instancesMutex.withLock so that all accesses to instances (read or write)
happen under instancesMutex; keep creating the CircuitBreakerInstance(name,
config.globalConfig, timeSource) as the getOrPut default.
- Around line 351-355: The catch in the CircuitBreaker around proceed(request)
currently treats every Throwable as a failure; change the handler so that
CancellationException is re-thrown immediately (e.g., in the catch for cause:
Throwable do if (cause is CancellationException) throw cause) before calling
instance.recordFailure() and re-throwing other exceptions; add a regression test
in CircuitBreakerTest that cancels a locally started request and asserts the
breaker does not record/open from that cancellation; and update the README
wording for the circuit breaker to explicitly state that coroutine cancellations
are excluded from failure counts.
- Around line 173-176: The code currently throws CircuitBreakerOpenException
when halfOpenAttempts >= config.halfOpenRequests inside the
CircuitState.HALF_OPEN branch; change this to surface the correct HALF_OPEN
condition: replace the thrown CircuitBreakerOpenException(name,
config.resetTimeout) with a distinct exception (e.g.,
CircuitBreakerHalfOpenException) that indicates the circuit is saturated in
HALF_OPEN (or extend CircuitBreakerOpenException to accept a state parameter and
pass CircuitState.HALF_OPEN). Update any callers/tests to expect the new
exception type or check the returned state accordingly; reference
CircuitState.HALF_OPEN, halfOpenAttempts, config.halfOpenRequests, and the
thrown exception in your changes.
In `@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/README.md`:
- Around line 13-30: The code block containing the ASCII state diagram in
README.md is missing a language specifier; update the opening fence (the triple
backticks that start the ASCII diagram block) to include a language such as text
(i.e., change ``` to ```text) so markdownlint MD040 stops flagging it and the
diagram renders correctly.
---
Nitpick comments:
In
`@ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.kt`:
- Around line 267-277: The public extension function
HttpRequestBuilder.circuitBreaker is missing the repo's standard "[Report a
problem]" KDoc footer; update the KDoc for circuitBreaker (the block above the
function that references CircuitBreakerConfig.register and
CircuitBreakerConfig.global) to append the standard Report a problem link/footer
used across public APIs so generated docs match other extensions, keeping
existing description and references to CircuitBreakerNameKey unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19839744-739a-4dc8-bcd2-688a4f66c336
📒 Files selected for processing (7)
ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/README.mdktor-client/ktor-client-plugins/ktor-client-circuit-breaker/api/ktor-client-circuit-breaker.apiktor-client/ktor-client-plugins/ktor-client-circuit-breaker/api/ktor-client-circuit-breaker.klib.apiktor-client/ktor-client-plugins/ktor-client-circuit-breaker/build.gradle.ktsktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/src/io/ktor/client/plugins/circuitbreaker/CircuitBreaker.ktktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/test/io/ktor/client/plugins/circuitbreaker/CircuitBreakerTest.ktsettings.gradle.kts
✅ Files skipped from review due to trivial changes (3)
- ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/api/ktor-client-circuit-breaker.klib.api
- ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/build.gradle.kts
- ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/api/ktor-client-circuit-breaker.api
🚧 Files skipped from review as they are similar to previous changes (2)
- settings.gradle.kts
- ktor-client/ktor-client-plugins/ktor-client-circuit-breaker/common/test/io/ktor/client/plugins/circuitbreaker/CircuitBreakerTest.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR: Add Circuit Breaker client plugin
Summary
ktor-client-circuit-breakermodule implementing the Circuit Breaker pattern for the Ktor HTTP client, filling the gap alongsideHttpRequestRetryandHttpTimeoutfor microservice resilience.Mutex.Motivation
The Ktor client has retry and timeout support but no built-in circuit breaker. Without one, a single failing downstream service can exhaust connection pools and cascade failures across the call graph. This is a table-stakes pattern for microservice architectures and is present in competing frameworks (Spring Cloud Circuit Breaker, Resilience4j, Hystrix).
API
Design decisions
on(Send)hook -- Same interception point asHttpRequestRetry, giving the circuit breaker visibility into the final outcome after any inner retry logic.IllegalStateException--CircuitBreakerOpenExceptionextendsIllegalStateExceptionrather thanIOExceptionbecause a circuit breaker rejection is a deliberate policy decision, not an I/O error. This also avoids unintended retries byHttpRequestRetry's default exception predicate.TimeSourceabstraction -- An internaltimeSourceproperty defaults toTimeSource.Monotonicbut can be replaced withTestTimeSourcein tests, enabling deterministic time-based state transition tests without real delays.Mutex-- Used for state transitions instead of atomics, since the state machine has multi-field invariants that need to be updated atomically.Files changed
settings.gradle.ktsktor-client-circuit-breakermodule.../ktor-client-circuit-breaker/build.gradle.kts.../circuitbreaker/CircuitBreaker.kt.../circuitbreaker/CircuitBreakerTest.kt.../api/ktor-client-circuit-breaker.api.../api/ktor-client-circuit-breaker.klib.api.../ktor-client-circuit-breaker/README.md