Skip to content

Commit b2d90eb

Browse files
HixieHixie
authored
Retry on transient Skia failure. (#139182)
This works around https://g-issues.skia.org/issues/40044713 using exponential backoff. This is completely untested because I have no idea how to test it. Also I only changed one of the code paths here. I figure if we get success here then we can start propagating the change to other places in this file that generate errors, maybe factoring out the retry and error reporting logic so it's not duplicated multiple times.
1 parent d7d1572 commit b2d90eb

File tree

3 files changed

+204
-34
lines changed

3 files changed

+204
-34
lines changed

‎packages/flutter_goldens_client/lib/skia_client.dart‎

Lines changed: 64 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,16 @@ class SkiaGoldClient {
314314
_tryjobInitialized = true;
315315
}
316316

317+
static void _addIndented(StringBuffer buffer, String text) {
318+
if (text.isEmpty) {
319+
buffer.writeln(' <empty>');
320+
} else {
321+
for (final String line in text.split('\n')) {
322+
buffer.writeln(' $line');
323+
}
324+
}
325+
}
326+
317327
/// Executes the `imgtest add` command in the goldctl tool for tryjobs.
318328
///
319329
/// The `imgtest` command collects and uploads test results to the Skia Gold
@@ -324,41 +334,64 @@ class SkiaGoldClient {
324334
/// The [testName] and [goldenFile] parameters reference the current
325335
/// comparison being evaluated by the [FlutterPreSubmitFileComparator].
326336
Future<void> tryjobAdd(String testName, File goldenFile) async {
327-
final List<String> imgtestCommand = <String>[
328-
_goldctl,
329-
'imgtest', 'add',
330-
'--work-dir', workDirectory
331-
.childDirectory('temp')
332-
.path,
333-
'--test-name', cleanTestName(testName),
334-
'--png-file', goldenFile.path,
335-
..._getPixelMatchingArguments(),
336-
];
337+
Duration delay = const Duration(seconds: 5);
338+
while (true) {
339+
final io.ProcessResult result = await process.run(<String>[
340+
_goldctl,
341+
'imgtest', 'add',
342+
'--work-dir', workDirectory.childDirectory('temp').path,
343+
'--test-name', cleanTestName(testName),
344+
'--png-file', goldenFile.path,
345+
..._getPixelMatchingArguments(),
346+
]);
347+
348+
final String resultStdout = result.stdout as String;
349+
final String resultStderr = result.stderr as String;
350+
if (result.exitCode == 0 ||
351+
resultStdout.contains('Untriaged') ||
352+
resultStdout.contains('negative image')) {
353+
return; // success
354+
}
337355

338-
final io.ProcessResult result = await process.run(imgtestCommand);
356+
if (resultStdout.contains('502')) {
357+
// probably a transient error, try again
358+
// Ideally we'd use something like package:test's printOnError, but best reliability
359+
// in getting logs on CI for now we're just using print.
360+
// See also: https://github.com/flutter/flutter/issues/91285
361+
print('Transient failure from Skia Gold, retrying in ${delay.inSeconds} seconds.'); // ignore: avoid_print
362+
print(''); // ignore: avoid_print
363+
print('stdout from gold:'); // ignore: avoid_print
364+
final StringBuffer buffer = StringBuffer();
365+
_addIndented(buffer, resultStdout);
366+
print(buffer); // ignore: avoid_print
367+
await Future<void>.delayed(delay);
368+
delay *= 2;
369+
continue; // retry
370+
}
339371

340-
final String/*!*/ resultStdout = result.stdout.toString();
341-
if (result.exitCode != 0 &&
342-
!(resultStdout.contains('Untriaged') || resultStdout.contains('negative image'))) {
343-
String? resultContents;
344-
final File resultFile = workDirectory.childFile(fs.path.join(
345-
'result-state.json',
346-
));
372+
final StringBuffer buffer = StringBuffer()
373+
..write('Golden test for "$testName" failed with exit code ${result.exitCode} ')
374+
..writeln('for a reason unrelated to pixel comparison.');
375+
if (resultStdout.isNotEmpty) {
376+
buffer
377+
..writeln()
378+
..writeln('stdout from gold:');
379+
_addIndented(buffer, resultStdout);
380+
}
381+
if (resultStderr.isNotEmpty) {
382+
buffer
383+
..writeln()
384+
..writeln('stderr from gold:');
385+
_addIndented(buffer, resultStderr);
386+
}
387+
final File resultFile = workDirectory.childFile('result-state.json');
347388
if (await resultFile.exists()) {
348-
resultContents = await resultFile.readAsString();
389+
buffer
390+
..writeln()
391+
..writeln('result-state.json contents:');
392+
_addIndented(buffer, resultFile.readAsStringSync());
349393
}
350-
final StringBuffer buf = StringBuffer()
351-
..writeln('Unexpected Gold tryjobAdd failure.')
352-
..writeln('Tryjob execution for golden file test $testName failed for')
353-
..writeln('a reason unrelated to pixel comparison.')
354-
..writeln()
355-
..writeln('Debug information for Gold --------------------------------')
356-
..writeln('stdout: ${result.stdout}')
357-
..writeln('stderr: ${result.stderr}')
358-
..writeln()
359-
..writeln()
360-
..writeln('result-state.json: ${resultContents ?? 'No result file found.'}');
361-
throw SkiaException(buf.toString());
394+
throw SkiaException(buffer.toString()); // failure
362395
}
363396
}
364397

‎packages/flutter_goldens_client/pubspec.yaml‎

Lines changed: 55 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,60 @@ dependencies:
1515
path: 1.8.3 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
1616
typed_data: 1.3.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
1717

18+
dev_dependencies:
19+
flutter_test:
20+
sdk: flutter
21+
test: 1.24.9
22+
23+
_fe_analyzer_shared: 65.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
24+
analyzer: 6.3.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
25+
args: 2.4.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
26+
async: 2.11.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
27+
boolean_selector: 2.1.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
28+
characters: 1.3.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
29+
clock: 1.1.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
30+
convert: 3.1.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
31+
coverage: 1.7.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
32+
fake_async: 1.3.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
33+
frontend_server_client: 3.2.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
34+
glob: 2.1.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
35+
http_multi_server: 3.2.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
36+
http_parser: 4.0.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
37+
io: 1.0.4 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
38+
js: 0.6.7 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
39+
leak_tracker: 9.0.16 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
40+
leak_tracker_testing: 1.0.5 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
41+
logging: 1.2.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
42+
matcher: 0.12.16 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
43+
material_color_utilities: 0.8.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
44+
mime: 1.0.4 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
45+
node_preamble: 2.0.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
46+
package_config: 2.1.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
47+
pool: 1.5.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
48+
pub_semver: 2.1.4 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
49+
shelf: 1.4.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
50+
shelf_packages_handler: 3.0.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
51+
shelf_static: 1.1.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
52+
shelf_web_socket: 1.0.4 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
53+
source_map_stack_trace: 2.1.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
54+
source_maps: 0.10.12 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
55+
source_span: 1.10.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
56+
stack_trace: 1.11.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
57+
stream_channel: 2.1.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
58+
string_scanner: 1.2.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
59+
term_glyph: 1.2.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
60+
test_api: 0.6.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
61+
test_core: 0.5.9 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
62+
vector_math: 2.1.4 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
63+
vm_service: 13.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
64+
watcher: 1.1.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
65+
web: 0.4.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
66+
web_socket_channel: 2.4.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
67+
webkit_inspection_protocol: 1.2.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
68+
yaml: 3.1.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
69+
1870
dartdoc:
19-
# Exclude this package from the hosted API docs.
20-
nodoc: true
71+
# Exclude this package from the hosted API docs.
72+
nodoc: true
2173

22-
# PUBSPEC CHECKSUM: 1c2e
74+
# PUBSPEC CHECKSUM: f695

‎packages/flutter_goldens_client/test/skia_client_test.dart‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
// Copyright 2014 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'dart:async';
6+
import 'dart:convert';
7+
import 'dart:io' show HttpClient, ProcessResult;
8+
9+
import 'package:file/file.dart';
10+
import 'package:file/memory.dart';
11+
import 'package:flutter_goldens_client/skia_client.dart';
12+
import 'package:flutter_test/flutter_test.dart';
13+
import 'package:platform/platform.dart';
14+
import 'package:process/process.dart';
15+
16+
void main() {
17+
test('502 retry', () async {
18+
final List<String> log = <String>[];
19+
await runZoned(
20+
zoneSpecification: ZoneSpecification(
21+
print: (Zone self, ZoneDelegate parent, Zone zone, String line) {
22+
log.add(line);
23+
},
24+
createTimer: (Zone self, ZoneDelegate parent, Zone zone, Duration duration, void Function() f) {
25+
log.add('created timer for $duration');
26+
return parent.createTimer(zone, Duration.zero, f);
27+
},
28+
),
29+
() async {
30+
final FileSystem fs;
31+
final SkiaGoldClient skiaClient = SkiaGoldClient(
32+
fs: fs = MemoryFileSystem(),
33+
process: FakeProcessManager(log),
34+
platform: FakePlatform(
35+
environment: const <String, String>{
36+
'GOLDCTL': 'goldctl',
37+
},
38+
),
39+
httpClient: FakeHttpClient(),
40+
fs.directory('/'),
41+
);
42+
print('start'); // ignore: avoid_print
43+
await skiaClient.tryjobAdd('test', fs.file('golden'));
44+
print('end'); // ignore: avoid_print
45+
expect(log, <String>[
46+
'start',
47+
'goldctl imgtest add --work-dir /temp --test-name t --png-file golden',
48+
'Transient failure from Skia Gold, retrying in 5 seconds.',
49+
'',
50+
'stdout from gold:',
51+
' 502\n',
52+
'created timer for 0:00:05.000000',
53+
'goldctl imgtest add --work-dir /temp --test-name t --png-file golden',
54+
'end'
55+
]);
56+
},
57+
);
58+
});
59+
}
60+
61+
class FakeProcessManager extends Fake implements ProcessManager {
62+
FakeProcessManager(this.log);
63+
64+
final List<String> log;
65+
int _index = 0;
66+
67+
@override
68+
Future<ProcessResult> run(List<Object> command, {
69+
Map<String, String>? environment,
70+
bool includeParentEnvironment = true,
71+
bool runInShell = false,
72+
Encoding? stderrEncoding,
73+
Encoding? stdoutEncoding,
74+
String? workingDirectory,
75+
}) async {
76+
log.add(command.join(' '));
77+
_index += 1;
78+
switch (_index) {
79+
case 1: return ProcessResult(0, 1, '502', '');
80+
case 2: return ProcessResult(0, 0, '200', '');
81+
default: throw StateError('unexpected call to run');
82+
}
83+
}
84+
}
85+
class FakeHttpClient extends Fake implements HttpClient { }

0 commit comments

Comments
 (0)