OCI: build the client on the first call, and retry closed connections and the token fetch by milos-lk · Pull Request #68 · livekit/storage

milos-lk · 2026-11-10T15:16:18Z

OCI Object Storage with workload identity loses requests in ways the SDK does not recover from. This changes when the client is built and what is retried.

Issue

With workload identity, the OCI SDK fetches a token from the cluster's token service as a client is constructed: one request, no retry, no context. When that service briefly answers 429, NewOCI fails and the caller has no client for the rest of its life, although the service recovers within seconds.
A client that is never used, such as one for a fallback store, fetches a token all the same, which adds load to the service doing the refusing.
A request whose connection the server closes before answering fails after one attempt. The SDK compares the error to io.EOF by equality, and Go's HTTP client hands it back wrapped in a url.Error.

Design walkthrough with diagrams: https://claude.ai/artifact/739SRiGGzDXemFM2whLLVR

What changes

Setup: NewOCI makes no request. With workload identity the SDK fetches a token as its client is built, so the first call builds it, bounded by that call's context. Calls waiting on the build share one, and DeleteObjects checks once for all its paths. A build that fails is not kept: a later call tries again. A namespace the config leaves out is looked up by the first call too.
Token fetch: the SDK makes it once, at build and on renewal. It is now repeated up to 5 times with jittered backoff (under 4s in total). A 403 is not retried.
Requests: a connection the server closed before answering (EOF, unexpected EOF, server closed idle connection) is added to what the retry policy in effect retries. That policy is the one configured through OCI_SDK_DEFAULT_RETRY_ENABLED or common.GlobalRetry, or else the SDK's default; its attempts and backoff stay.
PresignedURL is sent again only when the server answered with a failure. A request with no answer may have made its grant, and a second one would leave the first with a URI nobody has.

Worth knowing

Errors move: with workload identity, or with no namespace configured, a setup failure fails the first call, not NewOCI. A client with API key credentials is still built in NewOCI, which needs no request, so a bad key fails there.
A call released by its context leaves the build running for the next call. The SDK's token request takes no context, so it cannot be cancelled.
The SDK's default policy wraps an inner one and runs that inner one whenever no eventual-consistency window is open, so cases added to the outer policy alone are never consulted. They are added to both.
The policy is per client, so it covers every call but PresignedURL. A request that ended in EOF may have been received, so a retried IfAbsent write can report ErrExists, as it already can after a timeout. Likewise a multipart commit that landed can answer its retry with a 404, so UploadObject reports a failure for an object that exists, and the caller uploads again.
The 403 is found by its text, as the SDK returns the token response as a formatted string.

Tests

TestOCIBuildsClientOnFirstCall: a build that fails makes no storage request and the next call succeeds; a call is released at its deadline while the token fetch is held open; calls that leave, and a 40-path delete, cost one build.
TestOCIKeepsConfiguredRetries and TestOCIPresignedURLRetriesOnlyAnswered: retries switched off make one request, a global policy keeps its attempts, and a grant request with no answer is not sent again.
TestOCILooksUpNamespaceOnFirstCall: NewOCI makes no request; the first PutObject looks the namespace up, then uploads.
TestOCIRetriesTokenFetch and TestOCIRetriesClosedConnection: the SDK alone fails on one throttled fetch or one closed connection; with this change the client is built and the upload succeeds.
Run in CI only so far.

🤖 Generated with Claude Code

…token fetch A request whose connection the server closes before answering fails with a url.Error wrapping io.EOF. The OCI SDK's retry predicate compares the error to io.EOF by equality, so the wrapped one gets a single attempt and the upload is lost. The client's policy now adds EOF, unexpected EOF and a server-closed idle connection to the SDK's predicate. It is built on the default without eventual consistency, as the eventual-consistency default replaces a custom predicate with its own. With workload identity the SDK fetches its token with one request, inside the client constructor and again on renewal, so a throttled fetch fails NewOCI or a request mid-run. The provider now repeats the fetch up to five times with jittered backoff, except on a 403. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

With workload identity the SDK fetches a token as its client is built. In NewOCI that fetch has no context to bound it, a throttled fetch leaves the caller with no client for good, and a client that is never used fetches a token all the same. NewOCI now makes no request. The first call builds the client, under a mutex and released when its context ends, and a build that fails is not kept, so the next call tries again. A namespace the config leaves out is looked up by the first call as well. A client with API key credentials is still built in NewOCI, which needs no request, so a bad key fails there. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

milos-lk · 2026-11-10T23:09:17Z

maybe some sort of decorator could be nice here - checking.
EDIT:
not worth it - introduces a lot of bloat for no tangible gains - leaving as is.

biglittlebigben

LGTM. Building the client lazily is race-free, the token wrapper keeps the SDK's Refreshable check working, and the client-level retry policy covers every call. Three minor, non-blocking comments inline.

🤖 Generated with Claude Code

biglittlebigben · 2026-11-11T00:07:38Z

@@ -453,6 +614,9 @@ func (s *ociStorage) ListObjects(ctx context.Context, prefix string) ([]string,

 func (s *ociStorage) DeleteObjects(ctx context.Context, storagePaths []string) error {
 	return deleteEach(ctx, storagePaths, func(ctx context.Context, p string) error {


Minor: a failed build isn't kept, and every call that finds the client unbuilt pays up to 5 token fetches. Here that happens once per path, and the deletes run concurrently, so during a sustained 429 a 100-path delete can make up to 500 fetches, where NewOCI used to fail once. Could DeleteObjects call ready once before deleteEach? A short cache of the last build failure would also limit the load from other callers during an outage.

🤖 Generated with Claude Code

Done at the head of #68. DeleteObjects now checks the client once before the per-path loop, and when the client can't be built it names every path in the *DeleteError, as the S3 backend does for a failed batch. Calls waiting on a build also share one now, so a caller that gives up leaves nothing queued behind it.

I left the failure cache out. With one shared build a client makes at most one attempt at a time, and a cache would delay recovery by its lifetime. Happy to add it if token-service load from retries shows up in practice.

biglittlebigben · 2026-11-11T00:07:38Z

+// ociShouldRetry is the SDK's predicate plus a connection the server closed
+// under the request, which the SDK matches only when nothing wraps the error.
+// Such a request may have been received, as with a timeout the SDK retries.
+func ociShouldRetry(r common.OCIOperationResponse) bool {


Minor: this also retries requests that aren't idempotent. If CommitMultipartUpload lands but the server closes the connection before answering, the retry gets a 404 for the finished upload, and UploadObject reports a failure for an object that exists. Timeouts already have this risk, but this widens it. Worth a sentence next to the existing IfAbsent note in the description, or a check for the object when a commit retry fails with 404?

🤖 Generated with Claude Code

Agreed. The description now says, next to the IfAbsent note, that a multipart commit that landed can answer its retry with a 404, so UploadObject reports a failure for an object that exists.

I didn't add the object check. After a 404 it can't tell a commit that landed from one that failed while an older object sits at the same path, and reporting success in the second case would hide a lost write. A false failure costs a re-upload, so I kept that side of the trade.

biglittlebigben · 2026-11-11T00:07:38Z

+		if err == nil || attempt == ociTokenAttempts || !ociTokenRetryable(err) || time.Since(start) > ociTokenRetryWindow {
+			return v, err
+		}
+		time.Sleep(backoff/2 + rand.N(backoff/2+1))


Minor: these retries happen while the SDK signs a request and don't see its context, so a cancelled call can still spend up to about 4 s here. The 403 is also matched by its text. Both are noted in the description and fine as they are; a comment here would help whoever next updates the SDK.

🤖 Generated with Claude Code

Added a comment on retryOCIToken: the SDK asks for a token with no context, so the fetch of a call that has ended can run on for under 4s. The 403 text match is already explained on ociTokenRetryable.

…t once Every call that found the client unbuilt started a build of its own, queued behind the others, so calls that had given up left attempts running: a build per call, each with up to five token requests. The calls waiting now share one build, and DeleteObjects checks the client once for all its paths, not once per path. The client's retry policy replaced the one the SDK installs from OCI_SDK_DEFAULT_RETRY_ENABLED or common.GlobalRetry. The closed connection cases are now added to the policy in effect, on both of the layers the SDK's default has, so its attempts and backoff stay and retries switched off stay off. A pre-authenticated request is not idempotent, and its URI comes back only in its answer. PresignedURL is now sent again only when the server answered with a failure: a request with no answer may have made its grant, and a second one would leave the first with a URI nobody has. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

The closed connection cases were added to whichever policy was in effect, a one-attempt policy included. The SDK asks a policy whether to retry before it counts attempts, and with one attempt it never marks the request body as rewindable. So with retries switched off, an upload whose connection closed came back as "Unable to perform Retry on this request body type", an error that does not unwrap to the one behind it. A policy of one attempt is now returned unchanged. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>

milos-lk and others added 2 commits November 10, 2026 16:16

milos-lk changed the title ~~Retry OCI requests on a closed connection, and the workload identity token fetch~~ OCI: build the client on the first call, and retry closed connections and the token fetch Nov 10, 2026

milos-lk marked this pull request as ready for review November 10, 2026 23:02

milos-lk requested a review from a team November 10, 2026 23:02

github-actions Bot added the slack-ts:cs-devs:1794351753.946829 label Nov 10, 2026

biglittlebigben approved these changes Nov 11, 2026

View reviewed changes

milos-lk and others added 2 commits November 11, 2026 10:59

milos-lk merged commit 7f40e75 into main Nov 11, 2026
10 checks passed

milos-lk deleted the milos/oci-retry-gaps branch November 11, 2026 12:56

milos-lk mentioned this pull request Nov 11, 2026

Bump livekit/storage for OCI client setup and retry fixes livekit/egress#1532

Merged

Navigation Menu

Repository navigation

OCI: build the client on the first call, and retry closed connections and the token fetch - #68

OCI: build the client on the first call, and retry closed connections and the token fetch#68
milos-lk merged 4 commits into
mainfrom
milos/oci-retry-gaps

milos-lk commented Nov 10, 2026 •

edited

Loading

Uh oh!

milos-lk commented Nov 10, 2026 •

edited

Loading

Uh oh!

biglittlebigben left a comment

Uh oh!

biglittlebigben Nov 11, 2026

Uh oh!

milos-lk Nov 11, 2026

Uh oh!

biglittlebigben Nov 11, 2026

Uh oh!

milos-lk Nov 11, 2026

Uh oh!

biglittlebigben Nov 11, 2026

Uh oh!

milos-lk Nov 11, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -453,6 +614,9 @@ func (s *ociStorage) ListObjects(ctx context.Context, prefix string) ([]string,

		func (s *ociStorage) DeleteObjects(ctx context.Context, storagePaths []string) error {
		return deleteEach(ctx, storagePaths, func(ctx context.Context, p string) error {

Conversation

milos-lk commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milos-lk commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

biglittlebigben left a comment

Choose a reason for hiding this comment

Uh oh!

biglittlebigben Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

milos-lk Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

biglittlebigben Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

milos-lk Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

biglittlebigben Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

milos-lk Nov 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

milos-lk commented Nov 10, 2026 •

edited

Loading

milos-lk commented Nov 10, 2026 •

edited

Loading