feat(KlaviyoCore): AuthTokenManager proactive refresh scheduling (MAGE-625)#588
Conversation
…E-625) Adds proactive refresh to AuthTokenManager so personalized in-app forms have a fresh JWT ready before the cached one expires, without forcing form-display callers to pay the provider round-trip. - Refresh target: iat + 0.9 * (exp - iat), clamped to [now + 5s, exp - 30s] - Wall-clock-aware sleep loop self-corrects after backgrounding - Foreground transition handler triages 3 cases: expired cache -> clear + eager fetch; missed refresh -> fire immediately; still valid -> no-op - Lifecycle observer bridges Combine -> AsyncSequence via Publisher.values on iOS 15+ with an AsyncStream + sink fallback for iOS 13/14 - Refresh shares the existing dedup slot so concurrent demand callers collapse onto a single provider invocation - Date and AppLifeCycleEvents injected via init (defaults to environment) for deterministic tests - Refresh failures log at OSLog warning (not error) since the cache remains valid until exp - leeway and retry follows naturally
📝 WalkthroughWalkthroughThis PR introduces scheduled proactive token refresh to ChangesProactive Token Refresh with Lifecycle Awareness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Tests/KlaviyoCoreTests/Auth/AuthTokenManagerRefreshTests.swift (1)
128-129: ⚡ Quick winExtract the duplicated lifecycle subject/
AppLifeCycleEventssetup into a shared helper.This two-line construction is repeated verbatim at Lines 156-157 and 199-200. A small helper keeps the lifecycle wiring in one place.
♻️ Suggested helper
private func makeLifecycle() -> (PassthroughSubject<LifeCycleEvents, Never>, AppLifeCycleEvents) { let subject = PassthroughSubject<LifeCycleEvents, Never>() let lifecycle = AppLifeCycleEvents(lifeCycleEvents: { subject.eraseToAnyPublisher() }) return (subject, lifecycle) }Then at each call site:
- let lifecycleSubject = PassthroughSubject<LifeCycleEvents, Never>() - let lifecycle = AppLifeCycleEvents(lifeCycleEvents: { lifecycleSubject.eraseToAnyPublisher() }) + let (lifecycleSubject, lifecycle) = makeLifecycle()As per coding guidelines: "When writing tests, be DRY and extract common setup, teardown, and assertion helpers into shared test utilities and fixtures."
🤖 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 `@Tests/KlaviyoCoreTests/Auth/AuthTokenManagerRefreshTests.swift` around lines 128 - 129, The two-line lifecycle setup using PassthroughSubject<LifeCycleEvents, Never>() and AppLifeCycleEvents(lifeCycleEvents:) is duplicated; extract it into a shared helper (e.g., makeLifecycle()) that returns the subject and the AppLifeCycleEvents instance, then replace the repeated constructions at the test sites with calls to makeLifecycle() and use the returned (subject, lifecycle) tuple; reference the PassthroughSubject, AppLifeCycleEvents, and new makeLifecycle() helper when making the change.Sources/KlaviyoCore/Auth/AuthTokenManager.swift (1)
261-267: ⚡ Quick winExtract the refresh-formula constants.
0.9(lifetime fraction) and5(lower-bound lead seconds) are behavioral tunables embedded as literals. Promote them to named static properties so they sit alongside the existingFetchModebudgets as a single source of truth.♻️ Proposed constants
+ /// Fraction of a token's lifetime to elapse before refreshing (fire at 90%). + private static let refreshLifetimeFraction = 0.9 + /// Minimum lead time before firing a refresh; guards against tight loops on + /// short-lived tokens or forward clock jumps. + private static let minRefreshLeadInterval: TimeInterval = 5static func refreshTarget(for token: ValidatedToken, currentDate: Date) -> Date { let total = token.expiresAt.timeIntervalSince(token.issuedAt) - let ideal = token.issuedAt.addingTimeInterval(0.9 * total) + let ideal = token.issuedAt.addingTimeInterval(refreshLifetimeFraction * total) let upperBound = token.expiresAt.addingTimeInterval(-JWTParser.defaultLeeway) - let lowerBound = currentDate.addingTimeInterval(5) + let lowerBound = currentDate.addingTimeInterval(minRefreshLeadInterval) return max(lowerBound, min(ideal, upperBound)) }As per coding guidelines: "Avoid magic strings/numbers, preferring constants, enums and static properties in Swift."
🤖 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 `@Sources/KlaviyoCore/Auth/AuthTokenManager.swift` around lines 261 - 267, The refreshTarget function embeds magic numbers (0.9 and 5); extract them into named static properties on the same type that holds FetchMode budgets (e.g., add static let refreshLifetimeFraction: Double = 0.9 and static let refreshLowerBoundLeadSeconds: TimeInterval = 5.0 to AuthTokenManager), then update refreshTarget(for:currentDate:) to use AuthTokenManager.refreshLifetimeFraction and AuthTokenManager.refreshLowerBoundLeadSeconds instead of the literals (keep the math using token.issuedAt, token.expiresAt and JWTParser.defaultLeeway as-is).
🤖 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 `@Tests/KlaviyoCoreTests/Auth/AuthTokenManagerRefreshTests.swift`:
- Around line 19-34: The test function name
refreshTargetLandsAt90PercentOfTokenLifetime is misleading because the assertion
actually checks the upper clamp (exp - leeway == 1170); rename the test to
reflect clamp behavior (e.g.
refreshTargetClampsToExpMinusLeewayWhenIdealExceedsUpperBound or, if that exact
name already exists, refreshTargetClampsToExpMinusLeeway_ScenarioIdealPastUpper)
and keep the body using AuthTokenManager.refreshTarget(for: token, currentDate:
issued) and the Date assertion as-is so the name accurately describes the
behavior being verified.
- Around line 180-221: The test's short-lived token and sleep are too tight for
slow CI; in foregroundWithExpiredCachedTokenClearsCacheAndRefetches update the
shortLivedToken expiration to give ~4s validation window (e.g. expiresAt:
nowSeconds + 34 instead of +31) and increase the post-swap sleep to ~5s (replace
Task.sleep 2s → 5s) so the cached token reliably becomes stale before
lifecycleSubject.send(.foregrounded); locate changes in the test method
foregroundWithExpiredCachedTokenClearsCacheAndRefetches and the
TokenBox/tokens.set usage.
---
Nitpick comments:
In `@Sources/KlaviyoCore/Auth/AuthTokenManager.swift`:
- Around line 261-267: The refreshTarget function embeds magic numbers (0.9 and
5); extract them into named static properties on the same type that holds
FetchMode budgets (e.g., add static let refreshLifetimeFraction: Double = 0.9
and static let refreshLowerBoundLeadSeconds: TimeInterval = 5.0 to
AuthTokenManager), then update refreshTarget(for:currentDate:) to use
AuthTokenManager.refreshLifetimeFraction and
AuthTokenManager.refreshLowerBoundLeadSeconds instead of the literals (keep the
math using token.issuedAt, token.expiresAt and JWTParser.defaultLeeway as-is).
In `@Tests/KlaviyoCoreTests/Auth/AuthTokenManagerRefreshTests.swift`:
- Around line 128-129: The two-line lifecycle setup using
PassthroughSubject<LifeCycleEvents, Never>() and
AppLifeCycleEvents(lifeCycleEvents:) is duplicated; extract it into a shared
helper (e.g., makeLifecycle()) that returns the subject and the
AppLifeCycleEvents instance, then replace the repeated constructions at the test
sites with calls to makeLifecycle() and use the returned (subject, lifecycle)
tuple; reference the PassthroughSubject, AppLifeCycleEvents, and new
makeLifecycle() helper when making the change.
🪄 Autofix (Beta)
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: Repository UI
Review profile: CHILL
Plan: Enterprise
Run ID: 57c565d8-4340-4a1d-b0b9-1e54628fc455
📒 Files selected for processing (5)
Sources/KlaviyoCore/Auth/AuthTokenManager.swiftSources/KlaviyoCore/Auth/OnceResolver.swiftTests/KlaviyoCoreTests/Auth/AuthTestHelpers.swiftTests/KlaviyoCoreTests/Auth/AuthTokenManagerRefreshTests.swiftTests/KlaviyoCoreTests/Auth/AuthTokenManagerTests.swift
💤 Files with no reviewable changes (1)
- Tests/KlaviyoCoreTests/Auth/AuthTokenManagerTests.swift
| @Test | ||
| func refreshTargetLandsAt90PercentOfTokenLifetime() { | ||
| // issued=1000, expires=1200 → 200s lifetime → ideal target at 1180. | ||
| // Lifetime is large enough that the upper clamp (exp-30=1170) and the | ||
| // lower clamp (now+5=1005) are both satisfied by the ideal point. | ||
| // But wait: ideal=1180 > upper=1170, so the upper clamp wins → 1170. | ||
| let issued = Date(timeIntervalSince1970: 1000) | ||
| let expires = Date(timeIntervalSince1970: 1200) | ||
| let token = ValidatedToken(rawToken: "ignored", expiresAt: expires, issuedAt: issued) | ||
|
|
||
| let target = AuthTokenManager.refreshTarget(for: token, currentDate: issued) | ||
|
|
||
| // The 0.9-of-lifetime point lands past `exp - leeway`, so the upper | ||
| // clamp takes effect. | ||
| #expect(target == Date(timeIntervalSince1970: 1170)) | ||
| } |
There was a problem hiding this comment.
Test name doesn't match what it asserts; overlaps with refreshTargetClampsToExpMinusLeewayWhenIdealExceedsUpperBound.
With issued=1000, exp=1200, the ideal 90% point is 1180, which exceeds exp - leeway = 1170, so the upper clamp wins and the assertion checks 1170 — not the 90% landing. As written this duplicates the upper-clamp coverage in refreshTargetClampsToExpMinusLeewayWhenIdealExceedsUpperBound (Line 51) while refreshTargetUsesIdealPointWhenInsideClamps (Line 36) already covers the true 90% case. Rename to reflect the clamp behavior to avoid implying uncovered 90% verification.
✏️ Suggested rename
- `@Test`
- func refreshTargetLandsAt90PercentOfTokenLifetime() {
+ `@Test`
+ func refreshTargetClampsToExpMinusLeewayWhenLifetimeIsModest() {🤖 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 `@Tests/KlaviyoCoreTests/Auth/AuthTokenManagerRefreshTests.swift` around lines
19 - 34, The test function name refreshTargetLandsAt90PercentOfTokenLifetime is
misleading because the assertion actually checks the upper clamp (exp - leeway
== 1170); rename the test to reflect clamp behavior (e.g.
refreshTargetClampsToExpMinusLeewayWhenIdealExceedsUpperBound or, if that exact
name already exists, refreshTargetClampsToExpMinusLeeway_ScenarioIdealPastUpper)
and keep the body using AuthTokenManager.refreshTarget(for: token, currentDate:
issued) and the Date assertion as-is so the name accurately describes the
behavior being verified.
| @Test | ||
| func foregroundWithExpiredCachedTokenClearsCacheAndRefetches() async throws { | ||
| // Token lifetime is just barely past JWTParser's 30s leeway, so it | ||
| // validates at acquisition but becomes "expired" (per | ||
| // `isCachedTokenValid`) within a couple of real seconds. | ||
| // | ||
| // iat=now-60, exp=now+31 → validates if real wall time < exp-30 = now+1. | ||
| // After sleeping ~2s, the cached token is stale. Sending .foregrounded | ||
| // then drives the "expired cached token" branch of | ||
| // handleForegroundTransition, which clears the cache and triggers an | ||
| // eager fetch via the swapped TokenBox. | ||
| let nowSeconds = Date().timeIntervalSince1970 | ||
| let shortLivedToken = try makeJWT( | ||
| issuedAt: nowSeconds - 60, | ||
| expiresAt: nowSeconds + 31, | ||
| extraClaims: ["sub": "expiring"] | ||
| ) | ||
| let freshToken = try makeJWT(extraClaims: ["sub": "fresh"]) | ||
|
|
||
| let lifecycleSubject = PassthroughSubject<LifeCycleEvents, Never>() | ||
| let lifecycle = AppLifeCycleEvents(lifeCycleEvents: { lifecycleSubject.eraseToAnyPublisher() }) | ||
| let manager = AuthTokenManager(lifeCycle: lifecycle) | ||
| let counter = CallCounter() | ||
| let tokens = TokenBox(shortLivedToken) | ||
|
|
||
| await manager.registerProvider { | ||
| await counter.increment() | ||
| return await tokens.value | ||
| } | ||
| try await counter.waitFor(atLeast: 1) | ||
|
|
||
| // Swap to a long-lived token and wait until the cached token has | ||
| // crossed the staleness threshold. | ||
| await tokens.set(freshToken) | ||
| try await Task.sleep(nanoseconds: UInt64(2 * 1_000_000_000)) | ||
|
|
||
| lifecycleSubject.send(.foregrounded) | ||
| try await counter.waitFor(atLeast: 2) | ||
|
|
||
| let resolved = try await manager.currentToken(mode: .proactive) | ||
| #expect(resolved == freshToken) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'func isCachedTokenValid' --type=swift
rg -nP 'static let defaultLeeway' --type=swiftRepository: klaviyo/klaviyo-swift-sdk
Length of output: 921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "class CallCounter|struct CallCounter|func waitFor\\(" --type=swift
rg -n "waitFor\\(atLeast:" Tests/KlaviyoCoreTests/Auth/AuthTokenManagerRefreshTests.swift
sed -n '1,260p' Tests/KlaviyoCoreTests/Auth/AuthTokenManagerRefreshTests.swiftRepository: klaviyo/klaviyo-swift-sdk
Length of output: 11176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' Tests/KlaviyoCoreTests/Auth/AuthTestHelpers.swiftRepository: klaviyo/klaviyo-swift-sdk
Length of output: 3386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the foreground transition handler and the specific branches mentioned in the test comment.
rg -n "handleForegroundTransition|foregrounded|expired cached|missed-refresh|missed refresh|clear.*cache|TokenBox|isCachedTokenValid" Sources/KlaviyoCore/Auth/AuthTokenManager.swift
# Pull the relevant surrounding code.
rg -n "handleForegroundTransition" Sources/KlaviyoCore/Auth/AuthTokenManager.swift -n -C5
# Inspect JWT parsing/validation to see how `defaultLeeway` is used during acquisition.
rg -n "defaultLeeway|leeway|isValid|validate|parse" Sources/KlaviyoCore/Auth/JWTParser.swift Sources/KlaviyoCore/Auth/AuthTokenManager.swift
# Show the JWTParser validation implementation.
sed -n '1,220p' Sources/KlaviyoCore/Auth/JWTParser.swiftRepository: klaviyo/klaviyo-swift-sdk
Length of output: 12828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '130,290p' Sources/KlaviyoCore/Auth/AuthTokenManager.swift
sed -n '360,520p' Sources/KlaviyoCore/Auth/AuthTokenManager.swiftRepository: klaviyo/klaviyo-swift-sdk
Length of output: 12571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "func registerProvider" -S Sources/KlaviyoCore/Auth/AuthTokenManager.swift
rg -n "enum FetchMode|struct FetchMode|FetchMode\\.proactive|case \\w+\\s*\\w*proactive" -S Sources/KlaviyoCore/Auth/AuthTokenManager.swift Sources/KlaviyoCore/Auth -g'*.swift'
# Inspect the relevant sections.
sed -n '1,140p' Sources/KlaviyoCore/Auth/AuthTokenManager.swift
sed -n '140,260p' Sources/KlaviyoCore/Auth/AuthTokenManager.swiftRepository: klaviyo/klaviyo-swift-sdk
Length of output: 12297
Narrow cached-token validity window can deadlock this test on slow CI.
isCachedTokenValid and JWTParser.parseAndValidate both use JWTParser.defaultLeeway = 30, so expiresAt = now + 31 becomes stale once currentTime >= exp - 30 → about now + 1. If the eager warm-cache fetch triggered by registerProvider doesn’t validate before then, cachedToken/refreshAtWallClock never get set, .foregrounded hits neither the “expired cached token” nor “missed-refresh” branches, and CallCounter.waitFor(atLeast:) has no timeout—so waitFor(atLeast: 2) can block indefinitely.
🛡️ Suggested adjustment
let shortLivedToken = try makeJWT(
issuedAt: nowSeconds - 60,
- expiresAt: nowSeconds + 31,
+ expiresAt: nowSeconds + 34,
extraClaims: ["sub": "expiring"]
) await tokens.set(freshToken)
- try await Task.sleep(nanoseconds: UInt64(2 * 1_000_000_000))
+ try await Task.sleep(nanoseconds: UInt64(5 * 1_000_000_000))A ~4s validation window with a ~5s staleness sleep keeps the token stale at .foregrounded while tolerating CI/startup latency.
🤖 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 `@Tests/KlaviyoCoreTests/Auth/AuthTokenManagerRefreshTests.swift` around lines
180 - 221, The test's short-lived token and sleep are too tight for slow CI; in
foregroundWithExpiredCachedTokenClearsCacheAndRefetches update the
shortLivedToken expiration to give ~4s validation window (e.g. expiresAt:
nowSeconds + 34 instead of +31) and increase the post-swap sleep to ~5s (replace
Task.sleep 2s → 5s) so the cached token reliably becomes stale before
lifecycleSubject.send(.foregrounded); locate changes in the test method
foregroundWithExpiredCachedTokenClearsCacheAndRefetches and the
TokenBox/tokens.set usage.
| /// `LifecycleObserver` in `KlaviyoForms`). The wrapping `AsyncStream` keeps | ||
| /// the consumer call site homogeneous, and when the SDK eventually bumps | ||
| /// its iOS minimum to 15 the `else` branch is a clean delete. | ||
| private func makeLifecycleStream() -> AsyncStream<LifeCycleEvents> { |
There was a problem hiding this comment.
Gentle pushback on the dual-path approach here. The iOS 15+ publisher.values path is idiomatic, but when wrapped inside AsyncStream { continuation in } it requires a nested Task + onTermination cancellation — which ends up being roughly the same complexity as the sink fallback. There's no meaningful performance difference for a low-frequency lifecycle stream. Since iOS 13 support isn't going away soon and the two paths have to be maintained in parallel, it might be simpler to just use the sink path everywhere and delete the #available branch. Am I missing something that makes the 15+ path worth keeping?
evan-masseau
left a comment
evan-masseau
left a comment
There was a problem hiding this comment.
LGTM, left one question on the lifecycle stream
2 participants
Description
Adds proactive refresh to
AuthTokenManagerso personalized in-app forms have a fresh JWT ready before the cached one expires, without forcing form-display callers to pay the provider round-trip. Stacks on MAGE-624 (dedup + timeouts) — targeting that branch until 624 merges torel/*.Due Diligence
Release/Versioning Considerations
PatchContains internal changes or backwards-compatible bug fixes.MinorContains changes to the public API.MajorContains breaking changes.No public API changes —
AuthTokenManagerispackage. The newinitparameters (lifeCycle,currentDate) default to the existingenvironmentclosures, soAuthTokenManager.sharedand all current call sites are unaffected.Changelog / Code Overview
Refresh scheduling
iat + 0.9 * (exp - iat), clamped to[now + 5s, exp - JWTParser.defaultLeeway]. The upper clamp matches the same skew windowJWTParseruses for the expiry check, so refresh fires before the cache itself would be considered stale. The lower clamp prevents tight refresh loops when a token is acquired close to its own expiration.AuthTokenManager.refreshTarget(for:currentDate:)so tests can verify the formula and clamps directly without driving the full schedule-and-sleep lifecycle.Wall-clock-aware sleep loop
refreshAtWallClock: Date?stores the absolute target; the loop re-readscurrentDate()on every wakeup rather than trustingTask.sleep's elapsed duration. Self-corrects automatically after backgrounding.Foreground transition handler
Triages three cases on each
.foregroundedevent:Lifecycle observer
Bridges
AppLifeCycleEvents's Combine publisher into anAsyncStream:Publisher.values(cleanest path).AsyncStream + sinkfallback, mirroring the pattern established byLifecycleObserverin KlaviyoForms.AsyncStreamkeeps the consumer call site homogeneous; when the SDK eventually bumps its iOS minimum to 15 theelsebranch is a clean delete.Dedup sharing
performScheduledRefresh()routes through the sameinFlightslot that user-drivencurrentToken(mode:)callers use, so concurrent demand + the scheduled refresh collapse onto a single provider invocation.Logging
All new OSLog lines gated
@available(iOS 14.0, *). Refresh failures log atwarning, noterror— the cached token remains valid untilexp - leeway, and a foreground transition or user fetch will retry naturally.Out of scope
ConnectivityMonitoringand network-failure retry → MAGE-683clearTokenState()and profile-reset interaction → MAGE-626Scaffolding note
MAGE-623 intentionally deferred the refresh-related actor properties (
refreshTask,refreshAtWallClock,lifecycleObserverTask,lifeCycleinjection) to this ticket so the design decisions land alongside the implementation that uses them.Test Plan
Automated — 9 new tests in
AuthTokenManagerRefreshTests.swift, all passing alongside the 38 existing auth/JWT tests.Pure-function tests (no real time):
refreshTargetLandsAt90PercentOfTokenLifetime— happy-path 90%-of-lifetime calculation.refreshTargetUsesIdealPointWhenInsideClamps— neither clamp fires.refreshTargetClampsToExpMinusLeewayWhenIdealExceedsUpperBound— upper clamp.refreshTargetClampsToNowPlusFiveWhenIdealAlreadyPast— lower clamp.Integration tests (real time):
refreshFiresAtScheduledTimeAndChainsNextSchedule— scheduled refresh fires, replaces cache, chains next schedule.foregroundWithStillValidTokenIsNoOp— no extra provider invocation when cache is healthy.nonForegroundLifecycleEventsAreIgnored— backgrounded/terminated/reachabilityChanged don't trigger the handler.foregroundWithExpiredCachedTokenClearsCacheAndRefetches— case A of the foreground handler.registerProviderCancelsPriorScheduledRefresh— provider swap cancels the pending refresh task.Manual smoke (recommended for reviewer)
Console.appon theKlaviyo.authsubsystem; verifyrefresh scheduled→refresh succeededcadence at ~54s.foreground transition (case=missed-refresh)or(case=expired-cached-token)followed by a fresh fetch.Related Issues/Tickets
ab/MAGE-624/authtokenmanager-dedup-and-timeouts)Summary by CodeRabbit
Bug Fixes
Chores