diff --git a/extensions/cli/docs/storage-sync.md b/extensions/cli/docs/storage-sync.md index fd42e674c17..2bd83f87325 100644 --- a/extensions/cli/docs/storage-sync.md +++ b/extensions/cli/docs/storage-sync.md @@ -40,11 +40,20 @@ This document captures the responsibilities for both the CLI and backend compone - Resolve `storageId` into the desired S3 prefix (e.g., `sessions///`). - Generate two short-lived pre-signed `PUT` URLs: one for `session.json`, one for `diff.txt`. - Return both URLs and their keys in the response payload described above. -- **Expiration**: The initial implementation can hand out URLs with a generous TTL (e.g., 60 minutes). Later we will add CLI-side refresh logic before expiry. +- **Expiration**: URLs are issued with a 60-minute TTL. The CLI automatically refreshes them before expiry (see URL Refresh Strategy below). + +## URL Refresh Strategy + +Pre-signed URLs are automatically refreshed using a dual-strategy approach: + +1. **Proactive Refresh**: URLs are refreshed at the 50-minute mark (10 minutes before expiry) to prevent disruption +2. **Reactive Refresh**: If a 403 Forbidden error is detected (indicating expired URLs), an immediate refresh is triggered +3. **Error Handling**: Upload errors are logged but non-fatal; the service continues running with automatic recovery + +This ensures continuous operation during devbox suspension, network interruptions, and clock drift. ## Open Questions & Future Enhancements -- **URL refresh**: When the presigned URLs expire we currently have no refresh cycle. We'd need either a renewal endpoint or to re-call the existing endpoint on failure. - **Upload cadence**: The 30-second interval is hard-coded for now. Consider making it configurable in both CLI and backend policies. - **Error telemetry**: Decide if repeated upload failures should trip analytics or circuit breakers. - **Diff source**: `diff.txt` currently mirrors the `/diff` endpoint response. Confirm backend expectations for format and size limits. diff --git a/extensions/cli/src/services/StorageSyncService.test.ts b/extensions/cli/src/services/StorageSyncService.test.ts index 1e67a6c2e91..ce8a358a368 100644 --- a/extensions/cli/src/services/StorageSyncService.test.ts +++ b/extensions/cli/src/services/StorageSyncService.test.ts @@ -276,4 +276,87 @@ describe("StorageSyncService", () => { service.stop(); }); + + it("refreshes URLs on 403 error", async () => { + const syncSessionHistory = vi.fn(); + const getCompleteStateSnapshot = vi.fn().mockReturnValue({ test: "data" }); + + // Mock initial presign success + fetchMock.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + session: { putUrl: "https://upload/session-old", key: "session.json" }, + diff: { putUrl: "https://upload/diff-old", key: "diff.txt" }, + }), + }); + + // Mock 403 upload failure + fetchMock.mockResolvedValueOnce({ + ok: false, + status: 403, + statusText: "Forbidden", + text: async () => "Request has expired", + }); + + // Mock refresh presign success + fetchMock.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + session: { putUrl: "https://upload/session-new", key: "session.json" }, + diff: { putUrl: "https://upload/diff-new", key: "diff.txt" }, + }), + }); + + gitDiffMock.mockResolvedValue({ diff: "test", repoFound: true }); + + const result = await service.startFromOptions({ + storageOption: "session-123", + accessToken: "token", + syncSessionHistory, + getCompleteStateSnapshot, + isActive: () => true, + }); + + expect(result).toBe(true); + + // Wait for async refresh + await new Promise((resolve) => setTimeout(resolve, 100)); + + expect(fetchMock).toHaveBeenCalledTimes(3); // presign + failed upload + refresh + expect(service.getState().isEnabled).toBe(true); + + service.stop(); + }); + + it("prevents concurrent refresh requests", async () => { + const syncSessionHistory = vi.fn(); + const getCompleteStateSnapshot = vi.fn().mockReturnValue({}); + + fetchMock.mockResolvedValue({ + ok: true, + json: async () => ({ + session: { putUrl: "https://upload/session", key: "session.json" }, + diff: { putUrl: "https://upload/diff", key: "diff.txt" }, + }), + }); + + await service.start({ + storageId: "session-123", + accessToken: "token", + syncSessionHistory, + getCompleteStateSnapshot, + }); + + const refreshMethod = (service as any).refreshStorageTargets.bind(service); + const results = await Promise.all([ + refreshMethod(), + refreshMethod(), + refreshMethod(), + ]); + + const successCount = results.filter((r) => r === true).length; + expect(successCount).toBe(1); // Only first succeeds + + service.stop(); + }); }); diff --git a/extensions/cli/src/services/StorageSyncService.ts b/extensions/cli/src/services/StorageSyncService.ts index 52f0be59d45..18e8950cb37 100644 --- a/extensions/cli/src/services/StorageSyncService.ts +++ b/extensions/cli/src/services/StorageSyncService.ts @@ -45,6 +45,9 @@ export class StorageSyncService { private missingRepoLogged = false; private targets: StorageTargets | null = null; private options: StorageSyncStartOptions | null = null; + private targetsExpiresAt: number | null = null; + private refreshTimer: NodeJS.Timeout | null = null; + private isRefreshing = false; async initialize(): Promise { this.stop(); @@ -112,6 +115,7 @@ export class StorageSyncService { } this.targets = targets; + this.targetsExpiresAt = Date.now() + 60 * 60 * 1000; // Track 60-minute expiration this.options = options; this.stopped = false; this.uploadInFlight = false; @@ -130,10 +134,17 @@ export class StorageSyncService { ), ); + // Schedule proactive URL refresh at 50-minute mark + this.scheduleProactiveRefresh(); + await this.performUpload(); this.intervalHandle = setInterval(() => { - void this.performUpload(); + this.performUpload().catch((error) => { + logger.warn( + `Storage sync upload failed in interval: ${formatError(error)}`, + ); + }); }, intervalMs); return true; @@ -178,6 +189,7 @@ export class StorageSyncService { stop(): void { this.stopped = true; this.targets = null; + this.targetsExpiresAt = null; this.options = null; this.uploadInFlight = false; @@ -186,6 +198,12 @@ export class StorageSyncService { this.intervalHandle = null; } + // Clear refresh timer + if (this.refreshTimer) { + clearTimeout(this.refreshTimer); + this.refreshTimer = null; + } + if (this.state.isEnabled) { this.setState({ isEnabled: false, @@ -245,6 +263,59 @@ export class StorageSyncService { } } + private async refreshStorageTargets(): Promise { + if (this.isRefreshing || !this.options) { + return false; + } + + this.isRefreshing = true; + logger.info("Refreshing storage pre-signed URLs..."); + + try { + const newTargets = await this.requestStorageTargets( + this.options.storageId, + this.options.accessToken, + ); + + if (!newTargets) { + logger.warn( + "Failed to refresh storage URLs - continuing with existing URLs", + ); + return false; + } + + this.targets = newTargets; + this.targetsExpiresAt = Date.now() + 60 * 60 * 1000; // 60 minutes from now + logger.info("Storage URLs refreshed successfully"); + + // Schedule next proactive refresh at 50-minute mark + this.scheduleProactiveRefresh(); + + return true; + } catch (error) { + logger.warn(`Storage URL refresh error: ${formatError(error)}`); + return false; + } finally { + this.isRefreshing = false; + } + } + + private scheduleProactiveRefresh(): void { + // Clear any existing refresh timer + if (this.refreshTimer) { + clearTimeout(this.refreshTimer); + this.refreshTimer = null; + } + + // Schedule refresh at 50-minute mark (10 minutes before expiry) + const refreshIn = 50 * 60 * 1000; // 50 minutes + this.refreshTimer = setTimeout(() => { + this.refreshStorageTargets().catch((error) => { + logger.warn(`Proactive URL refresh failed: ${formatError(error)}`); + }); + }, refreshIn); + } + private async uploadToPresignedUrl( url: string, body: string, @@ -271,6 +342,16 @@ export class StorageSyncService { if (!response.ok) { const statusText = `${response.status} ${response.statusText}`.trim(); const responseBody = await response.text(); + + // Detect expired URL (403 Forbidden) and trigger refresh + if (response.status === 403) { + logger.warn("Storage URL expired (403), triggering refresh..."); + // Trigger async refresh without blocking the current upload error + this.refreshStorageTargets().catch((error) => { + logger.debug(`Background URL refresh failed: ${formatError(error)}`); + }); + } + throw new Error(`Storage upload failed (${statusText}): ${responseBody}`); } }