review: outage bandwidth leak staging mirror - #3
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe PR adds process-wide backend degradation tracking, lock-free log throttling, degraded-mode contract request gating, maximum-interval retry initialization, and throttled transport and transfer error logging. Tests cover thresholds, recovery, concurrency, retry convergence, and suppression counts. ChangesBackend degradation controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Transport
participant BackendDegradation
participant Transfer
participant ContractRetryLoop
participant Backend
Transport->>Backend: authenticate or write
Backend-->>Transport: success or error
Transport->>BackendDegradation: record success or failure
Transfer->>BackendDegradation: check degraded state
BackendDegradation-->>Transfer: return degradation status
Transfer->>ContractRetryLoop: skip or schedule contract creation
ContractRetryLoop->>Backend: retry after configured interval
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
full-bars
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@log_throttle_test.go`:
- Around line 71-96: Update the concurrent throttling test to retain the
suppression count returned by the initial allowed call, then add it to the
suppression count from the later Allow call before asserting the total equals
callers-1. Keep the existing schedule-independent emission assertion and
allowed-result checks unchanged.
In `@transfer.go`:
- Around line 3354-3367: Update the CreateContract gate in the transfer loop to
allow one process-wide, rate-limited recovery probe while isBackendDegraded()
remains true, suppressing all other contract requests. Reuse the existing
backend degradation/success tracking mechanisms so a successful probe clears the
degraded state, and add an integration test covering OOB recovery without
reconnecting the authenticated transport.
In `@transport.go`:
- Around line 196-201: Protect noteBackendSuccess with backendFailMu so
resetting consecutiveBackendFails and lastBackendFailNano is serialized with
failure updates and cannot leave a positive failure count with a zero timestamp.
Add a concurrent success-and-failure test that exercises the transition and
asserts isBackendDegraded does not observe this invalid state.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a43d032-403d-42cf-b020-16e514de1b46
📒 Files selected for processing (7)
backend_degraded_gate_test.gobackend_degraded_test.golog_throttle.golog_throttle_test.gotransfer.gotransfer_contract_manager.gotransport.go
Comment on lines
+3354
to
+3367
| // Skip the request entirely while the backend is unreachable. Each | ||
| // CreateContract is an OOB control round-trip; with the API down | ||
| // every one of them fails, and on a provider carrying many | ||
| // sequences that is a continuous storm of requests that cannot | ||
| // succeed. The loop still waits out the retry interval, so the | ||
| // sequence resumes promptly once a successful auth or OOB | ||
| // round-trip clears the degraded state. | ||
| if !isBackendDegraded() { | ||
| self.client.ContractManager().CreateContract( | ||
| contractKey, | ||
| self.contractSeqIndex, | ||
| ByteCount(32+float32(messageByteCount+messageByteCount+self.sendBufferSettings.MinMessageByteCount)/self.sendBufferSettings.ContractFillFraction), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep one bounded recovery probe while degraded.
Lines 3354-3367 suppress every CreateContract request. A successful OOB result is the only OOB path that calls noteBackendSuccess in transfer_contract_manager.go Lines 1309-1311. If the OOB service recovers while an authenticated transport stays connected, no request observes recovery and the degraded gate remains active indefinitely.
Permit one process-wide, rate-limited contract probe while degraded. Keep all other contract requests suppressed. Add an integration test that recovers OOB service without reconnecting the transport.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@transfer.go` around lines 3354 - 3367, Update the CreateContract gate in the
transfer loop to allow one process-wide, rate-limited recovery probe while
isBackendDegraded() remains true, suppressing all other contract requests. Reuse
the existing backend degradation/success tracking mechanisms so a successful
probe clears the degraded state, and add an integration test covering OOB
recovery without reconnecting the authenticated transport.
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
full-bars
force-pushed
the
upstream/outage-bandwidth-leak
branch
from
August 5, 2026 06:19
d6f78cb to
22dedba
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
✅ Unit tests committed locally. Commit: |
…idth during control-API outages When the control API is unreachable, providers keep creating contracts, expanding client windows and retransmitting against an endpoint that cannot authorize anything. The work has nowhere to go, so it is spent bandwidth. On metered links a sustained outage can consume a large share of a monthly allowance while serving no client. Add a shared degradation signal driven by the two round-trips that actually touch the backend (platform auth and contract OOB). It requires backendDegradedFailThreshold consecutive failures with no intervening success, and the last failure must be within backendDegradedWindow, so isolated timeouts on a busy provider never trip it. Any successful round-trip clears it immediately, so recovery is not on a timer. While degraded: - skip CreateContract; every request is an OOB round-trip that cannot succeed - start contract retries at the backed-off interval instead of the fast first retry, composing with the existing nextCreateContractRetryInterval backoff - do not expand the multi-client window; each added client needs its own contract Also rate-limit the four log lines that flood under the same fault ([t]auth error, [contract]oob err, [ts]->error, [r]drop) to one per minute with a suppressed count, via a small shared logThrottle. These are emitted per sequence per retry, so during an outage they are the dominant log volume and can push out the lines needed to diagnose it. Each falls back to -v=1 so no detail is lost when the level is raised.
full-bars
force-pushed
the
upstream/outage-bandwidth-leak
branch
from
August 5, 2026 06:44
9de2399 to
ae61167
Compare
The auth sites recorded every connect() error as a backend failure, including context cancellation from this process's own teardown. Closing a multi-client window cancels many transports mid-dial at once, and that burst of canceled dials tripped the degraded threshold with fresh timestamps -- so the NEXT session started gated, skipping its first CreateContract per sequence until the first auth success cleared it. Both auth sites now make the same local-teardown carve-out the contract OOB path already makes on client.Done, pinned for both transports by a call-site anchor test. Also from review, documentation only: isBackendDegraded now describes the OOB-only-outage steady state honestly (recovery rides the recency window as a bounded probe cycle, since the gated CreateContract is the only success source once auth stops re-running); the process-wide counter names its multi-platform-url limitation and the keying upgrade path; and noteBackendSuccess loses a duplicated doc line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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
2 participants
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.
Staging mirror for CodeRabbit review. Do not merge.
1 commit, 7 files, +668/-17.
Validation on this commit:
go build,go vet,gofmtclean; 17 tests green under-race; full package test suite green at 477s.Summary by CodeRabbit
Reliability
Logging