Skip to content

Commit

Permalink
Retry on transient Skia failure. (#139182)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Hixie authored Dec 1, 2023
1 parent d7d1572 commit b2d90eb
Show file tree
Hide file tree
Showing 3 changed files with 204 additions and 34 deletions.
95 changes: 64 additions & 31 deletions packages/flutter_goldens_client/lib/skia_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,16 @@ class SkiaGoldClient {
_tryjobInitialized = true;
}

static void _addIndented(StringBuffer buffer, String text) {
if (text.isEmpty) {
buffer.writeln(' <empty>');
} else {
for (final String line in text.split('\n')) {
buffer.writeln(' $line');
}
}
}

/// Executes the `imgtest add` command in the goldctl tool for tryjobs.
///
/// The `imgtest` command collects and uploads test results to the Skia Gold
Expand All @@ -324,41 +334,64 @@ class SkiaGoldClient {
/// The [testName] and [goldenFile] parameters reference the current
/// comparison being evaluated by the [FlutterPreSubmitFileComparator].
Future<void> tryjobAdd(String testName, File goldenFile) async {
final List<String> imgtestCommand = <String>[
_goldctl,
'imgtest', 'add',
'--work-dir', workDirectory
.childDirectory('temp')
.path,
'--test-name', cleanTestName(testName),
'--png-file', goldenFile.path,
..._getPixelMatchingArguments(),
];
Duration delay = const Duration(seconds: 5);
while (true) {
final io.ProcessResult result = await process.run(<String>[
_goldctl,
'imgtest', 'add',
'--work-dir', workDirectory.childDirectory('temp').path,
'--test-name', cleanTestName(testName),
'--png-file', goldenFile.path,
..._getPixelMatchingArguments(),
]);

final String resultStdout = result.stdout as String;
final String resultStderr = result.stderr as String;
if (result.exitCode == 0 ||
resultStdout.contains('Untriaged') ||
resultStdout.contains('negative image')) {
return; // success
}

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

final String/*!*/ resultStdout = result.stdout.toString();
if (result.exitCode != 0 &&
!(resultStdout.contains('Untriaged') || resultStdout.contains('negative image'))) {
String? resultContents;
final File resultFile = workDirectory.childFile(fs.path.join(
'result-state.json',
));
final StringBuffer buffer = StringBuffer()
..write('Golden test for "$testName" failed with exit code ${result.exitCode} ')
..writeln('for a reason unrelated to pixel comparison.');
if (resultStdout.isNotEmpty) {
buffer
..writeln()
..writeln('stdout from gold:');
_addIndented(buffer, resultStdout);
}
if (resultStderr.isNotEmpty) {
buffer
..writeln()
..writeln('stderr from gold:');
_addIndented(buffer, resultStderr);
}
final File resultFile = workDirectory.childFile('result-state.json');
if (await resultFile.exists()) {
resultContents = await resultFile.readAsString();
buffer
..writeln()
..writeln('result-state.json contents:');
_addIndented(buffer, resultFile.readAsStringSync());
}
final StringBuffer buf = StringBuffer()
..writeln('Unexpected Gold tryjobAdd failure.')
..writeln('Tryjob execution for golden file test $testName failed for')
..writeln('a reason unrelated to pixel comparison.')
..writeln()
..writeln('Debug information for Gold --------------------------------')
..writeln('stdout: ${result.stdout}')
..writeln('stderr: ${result.stderr}')
..writeln()
..writeln()
..writeln('result-state.json: ${resultContents ?? 'No result file found.'}');
throw SkiaException(buf.toString());
throw SkiaException(buffer.toString()); // failure
}
}

Expand Down
58 changes: 55 additions & 3 deletions packages/flutter_goldens_client/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,60 @@ dependencies:
path: 1.8.3 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
typed_data: 1.3.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"

dev_dependencies:
flutter_test:
sdk: flutter
test: 1.24.9

_fe_analyzer_shared: 65.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
analyzer: 6.3.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
args: 2.4.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
async: 2.11.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
boolean_selector: 2.1.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
characters: 1.3.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
clock: 1.1.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
convert: 3.1.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
coverage: 1.7.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
fake_async: 1.3.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
frontend_server_client: 3.2.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
glob: 2.1.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
http_multi_server: 3.2.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
http_parser: 4.0.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
io: 1.0.4 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
js: 0.6.7 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
leak_tracker: 9.0.16 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
leak_tracker_testing: 1.0.5 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
logging: 1.2.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
matcher: 0.12.16 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
material_color_utilities: 0.8.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
mime: 1.0.4 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
node_preamble: 2.0.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
package_config: 2.1.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
pool: 1.5.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
pub_semver: 2.1.4 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
shelf: 1.4.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
shelf_packages_handler: 3.0.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
shelf_static: 1.1.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
shelf_web_socket: 1.0.4 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
source_map_stack_trace: 2.1.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
source_maps: 0.10.12 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
source_span: 1.10.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
stack_trace: 1.11.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
stream_channel: 2.1.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
string_scanner: 1.2.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
term_glyph: 1.2.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
test_api: 0.6.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
test_core: 0.5.9 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
vector_math: 2.1.4 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
vm_service: 13.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
watcher: 1.1.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
web: 0.4.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
web_socket_channel: 2.4.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
webkit_inspection_protocol: 1.2.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
yaml: 3.1.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"

dartdoc:
# Exclude this package from the hosted API docs.
nodoc: true
# Exclude this package from the hosted API docs.
nodoc: true

# PUBSPEC CHECKSUM: 1c2e
# PUBSPEC CHECKSUM: f695
85 changes: 85 additions & 0 deletions packages/flutter_goldens_client/test/skia_client_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// Copyright 2014 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:async';
import 'dart:convert';
import 'dart:io' show HttpClient, ProcessResult;

import 'package:file/file.dart';
import 'package:file/memory.dart';
import 'package:flutter_goldens_client/skia_client.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:platform/platform.dart';
import 'package:process/process.dart';

void main() {
test('502 retry', () async {
final List<String> log = <String>[];
await runZoned(
zoneSpecification: ZoneSpecification(
print: (Zone self, ZoneDelegate parent, Zone zone, String line) {
log.add(line);
},
createTimer: (Zone self, ZoneDelegate parent, Zone zone, Duration duration, void Function() f) {
log.add('created timer for $duration');
return parent.createTimer(zone, Duration.zero, f);
},
),
() async {
final FileSystem fs;
final SkiaGoldClient skiaClient = SkiaGoldClient(
fs: fs = MemoryFileSystem(),
process: FakeProcessManager(log),
platform: FakePlatform(
environment: const <String, String>{
'GOLDCTL': 'goldctl',
},
),
httpClient: FakeHttpClient(),
fs.directory('/'),
);
print('start'); // ignore: avoid_print
await skiaClient.tryjobAdd('test', fs.file('golden'));
print('end'); // ignore: avoid_print
expect(log, <String>[
'start',
'goldctl imgtest add --work-dir /temp --test-name t --png-file golden',
'Transient failure from Skia Gold, retrying in 5 seconds.',
'',
'stdout from gold:',
' 502\n',
'created timer for 0:00:05.000000',
'goldctl imgtest add --work-dir /temp --test-name t --png-file golden',
'end'
]);
},
);
});
}

class FakeProcessManager extends Fake implements ProcessManager {
FakeProcessManager(this.log);

final List<String> log;
int _index = 0;

@override
Future<ProcessResult> run(List<Object> command, {
Map<String, String>? environment,
bool includeParentEnvironment = true,
bool runInShell = false,
Encoding? stderrEncoding,
Encoding? stdoutEncoding,
String? workingDirectory,
}) async {
log.add(command.join(' '));
_index += 1;
switch (_index) {
case 1: return ProcessResult(0, 1, '502', '');
case 2: return ProcessResult(0, 0, '200', '');
default: throw StateError('unexpected call to run');
}
}
}
class FakeHttpClient extends Fake implements HttpClient { }

0 comments on commit b2d90eb

Please sign in to comment.