From b2d90eb6d3c960e77ea1406252e0635cf2bd3ec2 Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Fri, 1 Dec 2023 15:22:22 -0800 Subject: [PATCH] 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. --- .../lib/skia_client.dart | 99 ++++++++++++------- packages/flutter_goldens_client/pubspec.yaml | 60 ++++++++++- .../test/skia_client_test.dart | 85 ++++++++++++++++ 3 files changed, 207 insertions(+), 37 deletions(-) create mode 100644 packages/flutter_goldens_client/test/skia_client_test.dart diff --git a/packages/flutter_goldens_client/lib/skia_client.dart b/packages/flutter_goldens_client/lib/skia_client.dart index e89a3068a9c..8e1748270d0 100644 --- a/packages/flutter_goldens_client/lib/skia_client.dart +++ b/packages/flutter_goldens_client/lib/skia_client.dart @@ -314,6 +314,16 @@ class SkiaGoldClient { _tryjobInitialized = true; } + static void _addIndented(StringBuffer buffer, String text) { + if (text.isEmpty) { + buffer.writeln(' '); + } 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 @@ -324,41 +334,64 @@ class SkiaGoldClient { /// The [testName] and [goldenFile] parameters reference the current /// comparison being evaluated by the [FlutterPreSubmitFileComparator]. Future tryjobAdd(String testName, File goldenFile) async { - final List imgtestCommand = [ - _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([ + _goldctl, + 'imgtest', 'add', + '--work-dir', workDirectory.childDirectory('temp').path, + '--test-name', cleanTestName(testName), + '--png-file', goldenFile.path, + ..._getPixelMatchingArguments(), + ]); - final io.ProcessResult result = await process.run(imgtestCommand); - - 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', - )); - if (await resultFile.exists()) { - resultContents = await resultFile.readAsString(); + 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 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()); + + 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.delayed(delay); + delay *= 2; + continue; // retry + } + + 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()) { + buffer + ..writeln() + ..writeln('result-state.json contents:'); + _addIndented(buffer, resultFile.readAsStringSync()); + } + throw SkiaException(buffer.toString()); // failure } } diff --git a/packages/flutter_goldens_client/pubspec.yaml b/packages/flutter_goldens_client/pubspec.yaml index 8aa3f3c7f6b..366b2ede2e3 100644 --- a/packages/flutter_goldens_client/pubspec.yaml +++ b/packages/flutter_goldens_client/pubspec.yaml @@ -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" -dartdoc: - # Exclude this package from the hosted API docs. - nodoc: true +dev_dependencies: + flutter_test: + sdk: flutter + test: 1.24.9 -# PUBSPEC CHECKSUM: 1c2e \ No newline at end of file + _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 + +# PUBSPEC CHECKSUM: f695 diff --git a/packages/flutter_goldens_client/test/skia_client_test.dart b/packages/flutter_goldens_client/test/skia_client_test.dart new file mode 100644 index 00000000000..3e993b1cd6e --- /dev/null +++ b/packages/flutter_goldens_client/test/skia_client_test.dart @@ -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 log = []; + 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 { + '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, [ + '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 log; + int _index = 0; + + @override + Future run(List command, { + Map? 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 { }