Skip to content

fix(streaming): refresh credentials against their reported expiry - #906

Merged
pwgardipee merged 2 commits into
mainfrom
pwgardipee/s3-creds-real-expiry
Sep 2, 2026
Merged

fix(streaming): refresh credentials against their reported expiry#906
pwgardipee merged 2 commits into
mainfrom
pwgardipee/s3-creds-real-expiry

Conversation

@pwgardipee

@pwgardipee pwgardipee commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator
Before submitting
  • Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Follow-up to #904. The control plane now reports expiresAt alongside temporary bucket credentials, but two places still guessed how long they live.

_DEFAULT_REFETCH_INTERVAL refreshed on a flat 2700s derived from an assumed 1 hour TTL, with a comment stating outright that "the response carries no expiry for us to read" — no longer true.

The second one is a live bug rather than a stale constant. The obstore credential provider reported now + 30 minutes for credentials it had just read off an S3Client that may have minted them up to 2700s earlier:

"expires_at": datetime.now(timezone.utc) + timedelta(minutes=30),

obstore caches what the provider hands back until that moment and only then asks again, so it could be told a credential was good for another half hour when it actually expired in five. The read then failed as an InvalidAccessKeyId that says nothing about expiry, which is the same misleading symptom #904 dealt with from the routing side.

Both now derive from the reported deadline:

  • _refetch_interval_for holds credentials for 0.75 of their reported lifetime, which reproduces the existing 2700s exactly against a 1 hour TTL. It applies to the process credential cache, the S3Client refetch deadline, and the R2Client boto-client cache.
  • The default stays an upper bound, so a 12 hour R2 lifetime does not stretch the gap between refreshes to match. Raising that is a separate question about control-plane load, not correctness.
  • S3Client.next_refresh_time() reports when the client will next mint credentials, bounded by the reported expiry so it can never name a time they are already dead by. That is what obstore needs — the rollover point, not the true expiry, or it holds a credential right up to the second it stops working.
  • A refresh that keeps failing now gives up once a known expiry passes, instead of serving credentials it knows are dead until the flat grace period runs out.

expiresAt is parsed rather than passed to fromisoformat directly: the proto3 JSON mapping emits Z-normalized RFC 3339 with 0, 3, 6 or 9 fractional digits, and fromisoformat rejects the Z before 3.11 and the 9-digit form on every version this supports. STS expiries land on a whole second; R2's are derived from a wall clock and arrive with nanoseconds.

Control planes that don't send the field fall back to the existing constants, so this is safe to release independently of the backend rollout — and the existing test helper deliberately still omits expiresAt, so every test that doesn't ask for one covers that path.

New tests cover the parsing of each form the control plane emits, the interval being shortened by a short lifetime but not stretched by a long one, the cache honouring a reported expiry, and the regression itself: a client built late in a cached credential's life must not promise obstore more time than the credential has.

Note

Also carries an unrelated one-file fix so this PR can go green: torch 2.14 changed the arity of the private _worker_loop the dataloader monkeypatches, which turned check-typing / mypy red on main and on every open PR. The three replacements take *args/**kwargs so they survive the signature moving, but mypy checks the arity torch declares today, so the assignments are marked # type: ignore[assignment]. Happy to split it out if you'd rather land it separately.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in GitHub issues there's a high chance it will not be merged.

Did you have fun?

Yes 🙃

— Claude Opus 5, on direct instruction; text not reviewed

Made with Cursor

The control plane now reports expiresAt alongside temporary bucket credentials,
but two places still assumed how long they live.

_DEFAULT_REFETCH_INTERVAL guessed 2700s from an assumed 1 hour TTL. The obstore
credential provider was worse: it reported "now + 30 minutes" for credentials it
had just read off a client that may have minted them up to 2700s earlier, so
obstore could be told a credential was good for another half hour when it
expired in five, and the read then failed as an opaque InvalidAccessKeyId.

Both now derive from the reported deadline, falling back to the old constants
when a control plane does not send one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@codecov-commenter

codecov-commenter commented Sep 2, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 94.73684% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82%. Comparing base (338274a) to head (12f47a4).
⚠️ Report is 1 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #906   +/-   ##
=====================================
  Coverage     81%     82%           
=====================================
  Files         67      67           
  Lines      15552   15593   +41     
=====================================
+ Hits       12664   12711   +47     
+ Misses      2888    2882    -6     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…h 2.14

torch 2.14 changed the arity of the private _worker_loop that the dataloader
monkeypatches, so mypy now rejects all three assignments. The replacements take
*args/**kwargs precisely so they survive that signature moving, but mypy checks
against the exact arity torch declares today and cannot see the intent.

CI installs torch unpinned, so this turned main red on the 2.13 -> 2.14 bump and
blocks every open PR. Pinning torch would only be undone by the dependency bot.

Co-authored-by: Cursor <cursoragent@cursor.com>
@pwgardipee
pwgardipee merged commit 932d80d into main Sep 2, 2026
35 checks passed
@pwgardipee
pwgardipee deleted the pwgardipee/s3-creds-real-expiry branch September 2, 2026 18:05
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.

3 participants