fix(auth): reload token file when expiry cannot be determined

[RapidPoseidon]

Context

Follow-up review of the new Distributed Training docs: checked whether coordinator and workers running in different timezones can break token sharing.

The SDK-to-SDK path is timezone-safe. expires_at is produced by authlib as Unix epoch seconds (int(time.time()) + expires_in) and checked against time.time() — both UTC-based regardless of local timezone. Verified empirically: a token minted under TZ=Asia/Tokyo and read under TZ=America/Los_Angeles shows exactly the same remaining validity.

But the documented DIY path has a real trap. The docs say "if you produce tokens without the SDK … add expires_at yourself" without specifying the format. If someone writes a formatted datetime string (or local wall-clock arithmetic), OAuth2Token.is_expired() returns None, and the reload check in rest.py

if token is not None and not token.is_expired(leeway=self._token_leeway):
    return  # <- None -> "not expired" -> file never re-read again

treated None as "not expired", so a worker silently never re-read the token file and rode the stale token into 401s — surfacing exactly when people hand-roll timestamps across timezones.

Changes

• rest.py (+ openapi/templates/rest.mustache, kept in sync): _reload_token_from_file_if_expired now only skips the re-read when is_expired() is explicitly False — unknown expiry re-reads the file instead of trusting the token forever.
• set_token warns when expires_at is present but not parseable as epoch seconds (numeric strings stay silent, matching authlib).
• docs/distributed_training.md: states that expires_at is timezone-independent Unix epoch seconds and shows expires_at = time.time() + expires_in for the curl path.

Verification

• Behavior test against the patched code: fresh token → kept; expired → reloaded; ISO-string / missing expires_at → now re-read (previously never); set_token warns once for ISO, silent for int and numeric string.
• uv run pyright src/rapidata/rapidata_client → 0 errors.
• uv run --group docs mkdocs build → success.

🔗 Session: https://session-a17860c3.poseidon.rapidata.internal/

🤖 Generated with Claude Code

[claude]

Review

Solid, well-scoped fix. The core idea — only treat a token as "not expired" when is_expired() is explicitly False, and otherwise fall through to re-read the file — is correct and the template/generated file (rest.mustache / rest.py) were kept in sync as required by CLAUDE.md. The docs update is clear and doesn't overstate anything.

A few things worth a look:

1. Please double-check the core premise against the pinned authlib==1.7.2 behavior.
The fix (and the set_token warning) rests on the assumption that OAuth2Token.is_expired() gracefully returns None when expires_at is a malformed value (e.g. an ISO datetime string), the same way it does when the key is simply missing. From the authlib source I'm aware of, is_expired() is roughly:

def is_expired(self, leeway=60):
    expires_at = self.get('expires_at')
    if not expires_at:
        return None
    expires_at -= leeway
    return expires_at < time.time()

That returns None when expires_at is missing/falsy, but if it's present as a non-numeric string (the ISO-datetime case called out in the PR description), expires_at -= leeway would raise TypeError rather than return None. Depending on whether OAuth2Token.__init__/the token setter on OAuth2Client also eagerly coerces expires_at via int(...) at assignment time, the failure could actually surface earlier — e.g. self.session.token = json.load(f) in _reload_token_from_file_if_expired, or self.session.token = token in set_token — as an unhandled ValueError/TypeError, rather than the "silently trusted forever" symptom described in the PR.

If that's the case:

• the "ISO-string → now re-read (previously never)" line in the verification section may only actually hold for the missing-key case, not the malformed-string case, and
• there's a separate, unfixed crash path when a bad token (from set_token or the token file) is assigned at all.

Since this is the entire justification for the change, I'd suggest pinning down the actual behavior with a real unit test against the installed authlib version (construct tokens with: missing expires_at, non-numeric string, numeric string, float) and asserting what is_expired()/set_token actually do in each case — rather than relying on manual/ephemeral verification. Apologies that I couldn't execute code in this review sandbox to confirm this myself either way — flagging so it gets a concrete test rather than taking my word for it.

2. Minor: int(token["expires_at"]) as the parse-check in set_token.
This will warn on a valid-looking epoch string with a fractional part (e.g. "1699999999.5"), since int() doesn't parse decimal strings. If float epoch values are a realistic input, float(...) would be a more accurate "is this parseable as epoch seconds" check. Minor, and may be intentional if you want to nudge users toward integer epoch specifically.

3. Test coverage.
There's no existing automated test suite covering rest.py's token/auth logic (matches the rest of the repo, which has no test directory), but given this is auth-critical and the reasoning about is_expired()'s return values is subtle (see point 1), a small unit test locking in _reload_token_from_file_if_expired's three branches (fresh, expired, unknown/None) and set_token's warning conditions would go a long way to prevent regressions here.

Nice catch overall on the None-as-falsy footgun — that's a real and easy-to-miss class of bug in token-expiry checks.