bazel l1 [fork 5/5 re-land]: MinIO edge — retry cap, read deadline, per-backend breaker - #36
Merged
Conversation
…nd breaker (#31) * s3proxy: cap minio client retries at 3 attempts minio-go's library default is MaxRetry = 10 attempts with exponential backoff, and bazel-remote configures no retry policy of its own on the S3 path — against a sick MinIO backend that turned every failing queued PUT into ~62s of retention (worker pinned, FD held) before the failure surfaced. Cap the constructed client at 3 attempts so a blip is still ridden out but a genuine failure surfaces in seconds. Applies to every backend (multi-backend map mode and single-backend mode both construct clients through newBackend). Co-authored-by: Cursor <cursoragent@cursor.com> * s3proxy: bound miss fall-through reads with a 5s deadline We measured reads against a wedged MinIO node pinned for ~220s, each retaining a file descriptor and a pooled connection the whole time: nothing bounded how long a fall-through Get or a Contains probe could wait on a sick backend. Give every read-path MinIO call a 5s context deadline — a healthy LAN backend answers in milliseconds; one that cannot answer in 5s should fail the call, and the disk cache layer treats that like any other backend error. The deadline context also governs the streamed GET body: the timer is released when the caller closes the body (cancelReadCloser), so a transfer stalled mid-stream is reclaimed too instead of holding its connection open indefinitely. The deadline is a package-level var only so tests can shrink it, mirroring uploadTimeout. Co-authored-by: Cursor <cursoragent@cursor.com> * s3proxy: per-backend circuit breaker on the MinIO edge Even with capped retries and a read deadline, every request against a known-sick MinIO backend still pays the full failure price (and holds an FD while doing so). Put a circuit breaker on every MinIO call, one breaker per backend so in multi-backend map mode a sick shard trips only its own breaker and cannot blind healthy shards. The breaker is hand-rolled (cache/s3proxy/breaker.go: a mutex, a counter, a timestamp — no goroutines, no timers), modeled on the FA agent's failureBreaker precedent rather than pulling in a library: this module is embedded in the FA agent, and an extra module in the fork's go.mod ripples into the agent's module graph. Policy: 5 consecutive failures open the breaker — consecutive-only, no failure-ratio window, since the 5s read deadline already bounds what a brownout costs per request and consecutive-only matches the agent precedent. Open lasts 15s; then half-open admits exactly ONE probe: success closes and resets, failure re-opens for another fixed 15s (no exponential backoff — a cache edge wants fast re-probe, and a wasted probe costs one request, not a job). Read path (miss fall-through Get, Contains): when the breaker is open the proxy answers with the existing miss convention immediately — a miss is the honest answer the disk cache layer can act on; an error would surface to the client. A genuine not-found counts as breaker success (a miss means MinIO answered), a read-deadline expiry counts as failure, and a parent-context cancellation (the client went away) is counted neither way. Write path (queued upload workers): PUTs are breaker-wrapped with no extra deadline (large CAS blobs may legitimately be slow; the existing 10m uploadTimeout still reclaims workers). When open, items fail fast without dialing MinIO and are classified in bazel_remote_s3_upload_outcomes_total and the OperationObserver as status="error", reason="breaker_open". A precondition failure (already_exists) counts as breaker success. New metrics: bazel_remote_s3_breaker_state{backend} (0=closed, 1=half-open, 2=open) and bazel_remote_s3_breaker_transitions_total{backend,to}; every transition also logs a line through the proxy's errorLogger. Co-authored-by: Cursor <cursoragent@cursor.com> * s3proxy: split read deadline into connect legs + configurable overall bound Review (fork 5/5, Piotr): the single 5s deadline conflated two things — failing fast on a dead backend, and bounding a healthy backend's body stream. A large CAS fall-through read could be guillotined mid-stream on a perfectly healthy MinIO; raising the one knob to 60s would instead have re-introduced the 60-220s miss stalls the drills measured. Now: connection establishment (dial, TLS, response headers) is bounded by a constant 10s connectTimeout at the transport layer — that is where the black-hole fail-fast lives — while the overall per-call deadline (which the streamed body rides) defaults to 5m, mirroring uploadTimeout's large-blob reasoning, and is configurable via s3_proxy.read_timeout / WithReadDeadline. Co-authored-by: Cursor <cursoragent@cursor.com> * s3proxy: breaker learns from the body, uploads never probe, key-named metrics Post-approval review feedback on the MinIO-edge piece (Piotr + bugbot): - Streamed GETs now defer their breaker outcome to the body's terminal state (bodyOutcomeReadCloser). Recording at time-to-first-byte meant the dominant brownout mode -- connections open fine, bytes don't flow -- could never trip the breaker: every read recorded a header success that reset the streak, then degraded to a miss at the deadline, so the fleet paid a full deadline per read indefinitely. Clean EOF is a success, a mid-stream error (including our own deadline) with the caller still interested is a failure, caller-side abandonment counts neither way. The half-open probe slot is held until the probe's body terminates. - Write-through uploads use ExecuteNoProbe: a PutObject bounded only by the 10m uploadTimeout must not become the half-open recovery probe, or a recovered MinIO could keep serving artificial misses for the whole upload. Uploads fail fast unless the breaker is closed; probing is the read path's job (bounded in seconds, arrives constantly). - The breaker's "backend" metric label is now the backends-map key (endpoint fallback) instead of endpoint/bucket, matching every other backend-labeled series so they join on dashboards. Bucket is per-request state since the multi-backend piece, not backend identity. Co-authored-by: Cursor <cursoragent@cursor.com> * s3proxy: close the body when a casblob extract fails On the CAS/v2 path Get returned casblob.ExtractLogicalSize(rc) directly: on failure that yields a nil reader, so the caller never closes the bodyOutcomeReadCloser — leaking the deadline timer and, when the call was the half-open probe, wedging probeInFlight forever (a >=16-byte header with a non-positive size errors without any Read ever seeing EOF, so no outcome fires). Close both releases the timer and files the body outcome: an un-expired early close counts neither way, because a malformed stored object says nothing about backend health (bugbot, PR #31). Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
shreyas-blacksmith
marked this pull request as ready for review
August 4, 2026 15:36
Contributor
Author
|
This was approved in #31 but needs to be re-merged. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 87e6779. Configure here.
| // Straggler; the open window from the trip stands. | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Stragglers break half-open probe
High Severity
record clears probeInFlight and applies half-open transitions for every outcome while the breaker is half-open, without tying that outcome to the call allow admitted as the probe. Deferred body reports and long ExecuteNoProbe uploads admitted earlier can therefore open a second probe, reopen after a healthy probe, or discard a successful probe once state is open again.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 87e6779. Configure here.
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.
Summary
Clean re-land of piece-5 (#31) onto
patchset. Supersedes #35, which conflicted because #30/#31 merged seven seconds apart and GitHub squash-landed #31 onto the piece-4 branch instead ofpatchset.bcca7bc) onto currentpatchset(158f885).patchsettip, so the apply is clean (no content rewrite).c3fcb04/ wrong-landbcca7bcexactly.Test plan
go test ./cache/s3proxy/ ./config/ ./ -count=1Made with Cursor
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled. (Staging)Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.