Skip to content

fix(cdn): retry manifest fetch after transient download failure #2262

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
cursor wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(cdn): retry manifest fetch after transient download failure #2262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 50 additions & 23 deletions modules/ensemble/lib/framework/definition_providers/cdn_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,8 +492,8 @@ class CdnDefinitionProvider extends DefinitionProvider {
/// Sets _hasPendingUpdate flag if updates were fetched
Future<void> _refreshIfStale() async {
try {
final shouldFetch = await _shouldFetchManifest();
if (!shouldFetch) {
final freshness = await _evaluateManifestFreshness();
if (!freshness.shouldFetch) {
return;
}

Expand All @@ -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);
Expand Down Expand Up @@ -548,8 +549,8 @@ class CdnDefinitionProvider extends DefinitionProvider {
}

Future<void> _loadManifest() async {
final shouldFetch = await _shouldFetchManifest();
if (!shouldFetch) {
final freshness = await _evaluateManifestFreshness();
if (!freshness.shouldFetch) {
return;
}

Expand All @@ -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<bool> _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<String, dynamic>;
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;
}
}

Expand Down Expand Up @@ -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<String, dynamic>? _asMap(dynamic value) {
if (value is Map<String, dynamic>) return value;
Expand Down Expand Up @@ -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<void> _refreshTranslationsAtRuntime() async {
try {
final context = Utils.globalAppKey.currentContext;
Expand All @@ -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);
73 changes: 73 additions & 0 deletions modules/ensemble/test/cdn_provider_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading