From 6996fbf6dc2e4e0b79be8ae507a5976986b629bf Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 6 Jun 2026 11:08:02 +0000 Subject: [PATCH] fix(cdn): retry manifest fetch after transient download failure _lastUpdatedAt was advanced when lastUpdateTime.json reported a newer timestamp, even if the subsequent manifest download failed. That made later refresh checks skip the update until the CDN published again, leaving CDN-backed apps on stale definitions for the rest of the session. Only commit the remote timestamp after a manifest is successfully fetched and cached. Extract cdnShouldFetchManifest helpers for unit testing. Co-authored-by: Sharjeel Yunus --- .../definition_providers/cdn_provider.dart | 73 +++++++++++++------ modules/ensemble/test/cdn_provider_test.dart | 73 +++++++++++++++++++ 2 files changed, 123 insertions(+), 23 deletions(-) diff --git a/modules/ensemble/lib/framework/definition_providers/cdn_provider.dart b/modules/ensemble/lib/framework/definition_providers/cdn_provider.dart index 1e955bd97..f04f0b8af 100644 --- a/modules/ensemble/lib/framework/definition_providers/cdn_provider.dart +++ b/modules/ensemble/lib/framework/definition_providers/cdn_provider.dart @@ -492,8 +492,8 @@ class CdnDefinitionProvider extends DefinitionProvider { /// Sets _hasPendingUpdate flag if updates were fetched Future _refreshIfStale() async { try { - final shouldFetch = await _shouldFetchManifest(); - if (!shouldFetch) { + final freshness = await _evaluateManifestFreshness(); + if (!freshness.shouldFetch) { return; } @@ -509,6 +509,7 @@ class CdnDefinitionProvider extends DefinitionProvider { _rebuildFromRoot(root); await _refreshTranslationsAtRuntime(); _etag = newEtag ?? _etag; + _commitRemoteLastUpdatedAt(freshness.remoteLastUpdatedAt); // Save to persistent cache await _saveCachedState(jsonString); @@ -548,8 +549,8 @@ class CdnDefinitionProvider extends DefinitionProvider { } Future _loadManifest() async { - final shouldFetch = await _shouldFetchManifest(); - if (!shouldFetch) { + final freshness = await _evaluateManifestFreshness(); + if (!freshness.shouldFetch) { return; } @@ -563,47 +564,50 @@ class CdnDefinitionProvider extends DefinitionProvider { final root = _decodeManifestRoot(jsonString); _rebuildFromRoot(root); + _commitRemoteLastUpdatedAt(freshness.remoteLastUpdatedAt); // Save to persistent cache await _saveCachedState(jsonString); } - Future _shouldFetchManifest() async { + /// Reads CDN lastUpdateTime and decides whether to download a new manifest. + /// Does not mutate [_lastUpdatedAt]; commit only after a successful fetch. + Future<({bool shouldFetch, int? remoteLastUpdatedAt})> + _evaluateManifestFreshness() async { final lastUpdateUri = Uri.parse('$baseUrl/$appId/lastUpdateTime.json'); try { final resp = await http.get(lastUpdateUri); if (resp.statusCode != 200) { - return true; + return (shouldFetch: true, remoteLastUpdatedAt: null); } final jsonString = _decodePossiblyBrotli(resp); if (jsonString == null || jsonString.isEmpty) { - return true; + return (shouldFetch: true, remoteLastUpdatedAt: null); } final lastUpdateData = jsonDecode(jsonString) as Map; final num? remoteLastUpdateNum = lastUpdateData['lastUpdatedAt'] as num?; final int? remoteLastUpdate = remoteLastUpdateNum?.toInt(); - if (remoteLastUpdate == null) { - return true; - } - - if (_lastUpdatedAt == null) { - _lastUpdatedAt = remoteLastUpdate; - return true; - } - - final shouldFetch = _isIncomingNewer(remoteLastUpdate, _lastUpdatedAt); - - _lastUpdatedAt = remoteLastUpdate; - - return shouldFetch; + return ( + shouldFetch: cdnShouldFetchManifest( + localLastUpdatedAt: _lastUpdatedAt, + remoteLastUpdatedAt: remoteLastUpdate, + ), + remoteLastUpdatedAt: remoteLastUpdate, + ); } catch (e) { debugPrint( 'CdnProvider: Error checking lastUpdateTime: $e, will fetch manifest'); - return true; + return (shouldFetch: true, remoteLastUpdatedAt: null); + } + } + + void _commitRemoteLastUpdatedAt(int? remoteLastUpdatedAt) { + if (remoteLastUpdatedAt != null) { + _lastUpdatedAt = remoteLastUpdatedAt; } } @@ -855,7 +859,7 @@ class CdnDefinitionProvider extends DefinitionProvider { // -------------------------------------------------------- static bool _isIncomingNewer(int? incoming, int? current) => - incoming != null && (current == null || incoming > current); + cdnIsIncomingManifestNewer(incoming, current); static Map? _asMap(dynamic value) { if (value is Map) return value; @@ -942,6 +946,14 @@ class CdnDefinitionProvider extends DefinitionProvider { @visibleForTesting int? get lastUpdatedAtForTesting => _lastUpdatedAt; + @visibleForTesting + Future<({bool shouldFetch, int? remoteLastUpdatedAt})> + evaluateManifestFreshnessForTesting() => _evaluateManifestFreshness(); + + @visibleForTesting + void commitRemoteLastUpdatedAtForTesting(int? remoteLastUpdatedAt) => + _commitRemoteLastUpdatedAt(remoteLastUpdatedAt); + Future _refreshTranslationsAtRuntime() async { try { final context = Utils.globalAppKey.currentContext; @@ -961,3 +973,18 @@ class CdnDefinitionProvider extends DefinitionProvider { } } } + +/// Whether the CDN manifest should be fetched based on lastUpdateTime values. +@visibleForTesting +bool cdnShouldFetchManifest({ + required int? localLastUpdatedAt, + required int? remoteLastUpdatedAt, +}) { + if (remoteLastUpdatedAt == null) return true; + if (localLastUpdatedAt == null) return true; + return cdnIsIncomingManifestNewer(remoteLastUpdatedAt, localLastUpdatedAt); +} + +@visibleForTesting +bool cdnIsIncomingManifestNewer(int? incoming, int? current) => + incoming != null && (current == null || incoming > current); diff --git a/modules/ensemble/test/cdn_provider_test.dart b/modules/ensemble/test/cdn_provider_test.dart index 8178cbe19..a8a532c14 100644 --- a/modules/ensemble/test/cdn_provider_test.dart +++ b/modules/ensemble/test/cdn_provider_test.dart @@ -11,6 +11,79 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:shared_preferences/shared_preferences.dart'; void main() { + group('cdnShouldFetchManifest', () { + test('fetches when remote timestamp is unknown or local is unset', () { + expect( + cdnShouldFetchManifest( + localLastUpdatedAt: 100, + remoteLastUpdatedAt: null, + ), + isTrue, + ); + expect( + cdnShouldFetchManifest( + localLastUpdatedAt: null, + remoteLastUpdatedAt: 200, + ), + isTrue, + ); + }); + + test('fetches only when remote timestamp is newer', () { + expect( + cdnShouldFetchManifest( + localLastUpdatedAt: 100, + remoteLastUpdatedAt: 200, + ), + isTrue, + ); + expect( + cdnShouldFetchManifest( + localLastUpdatedAt: 200, + remoteLastUpdatedAt: 200, + ), + isFalse, + ); + expect( + cdnShouldFetchManifest( + localLastUpdatedAt: 300, + remoteLastUpdatedAt: 200, + ), + isFalse, + ); + }); + }); + + group('CDN lastUpdatedAt commit', () { + test('does not advance local timestamp until manifest fetch succeeds', + () async { + final provider = CdnDefinitionProvider('test-app'); + expect(provider.lastUpdatedAtForTesting, isNull); + + // Simulate freshness check seeing a newer remote timestamp without + // committing it (failed manifest download must remain retryable). + expect( + cdnShouldFetchManifest( + localLastUpdatedAt: provider.lastUpdatedAtForTesting, + remoteLastUpdatedAt: 500, + ), + isTrue, + ); + expect(provider.lastUpdatedAtForTesting, isNull); + + provider.commitRemoteLastUpdatedAtForTesting(500); + expect(provider.lastUpdatedAtForTesting, 500); + + expect( + cdnShouldFetchManifest( + localLastUpdatedAt: provider.lastUpdatedAtForTesting, + remoteLastUpdatedAt: 500, + ), + isFalse, + ); + }); + }); + group('CDN cache invalidation', () { test('resets freshness metadata when persisted manifest is invalid', () async {