Skip to content

Fix flakiness in the eval integration test - #10008

Open
srawlins wants to merge 2 commits into
flutter:masterfrom
srawlins:flaky-eval
Open

srawlins wants to merge 2 commits into
flutter:masterfrom
srawlins:flaky-eval

Conversation

@srawlins

Copy link
Copy Markdown
Contributor

I looked at three different logs of devtools tests flaking out on Linux bots on LUCI. All three logs failed in eval_integration_test.dart.

  • In two cases, EvalOnDartLibrary asyncEval supports expressions that do not start with the await keyword timed out after 1 minute ("TimeoutException: Test timed out after 1 minutes").
  • In the thired case, EvalOnDartLibrary asyncEval returns the result of the future completion timed out after 1 minute.
  • On all three runs, following the timeout, the test runner attempted retries (due to retry: 3), but every retry and subsequent test failed with "evaluate: (-32000) Service connection disposed."

So here are the root causes:

  1. Garbage Collection of reader before evaluation began: In eval_on_dart_library.dart:390-490, the reader list was previously created in a standalone eval (widgetInspectorService.toId(<dynamic>[], "$readerGroup")). Because WidgetInspectorService only holds objects using WeakReference, nothing held a strong reference to <dynamic>[] in the target isolate during the network round-trip between DevTools and the target app. If garbage collection occurred in that window, widgetInspectorService.toObject in the subsequent eval returned null, throwing: "Unhandled exception: type 'Null' is not a subtype of type 'List<dynamic>' in type cast." Because this exception occurred before the try/finally block inside the closure, postEvent("future_completed", ...) was never posted, causing DevTools to hang waiting on future_completed until the test timed out after 1 minute.
  2. Pinning loop stopped prematurely: The pinning loop (while (!isDone && ++bufferTicks <= 20)) intended to pin reader until retrieved. However:
    • As soon as isDone was set to true, !isDone became false and the loop terminated immediately (providing zero buffer while postEvent traveled to DevTools and DevTools issued evalInstance). If a GC occurred during that window, toObject returned null.
    • For evaluations taking longer than 1 second (20 ticks of 50ms), ++bufferTicks <= 20 became false while the future was still pending, causing reader to be unpinned before completion.
  3. Unhandled error in target isolate: Any exception occurring during reader initialization or eval setup in the target app closure was unhandled, preventing future_completed from ever firing and causing the target isolate to crash/disconnect.
  4. Stale environment reuse on connection drop: In flutter_test_environment.dart:100-125, _needsSetup was not re-evaluated if the VM service connection dropped (!connectedState.value.connected). When a test timed out and the process disconnected, retries attempted to reuse the disposed connection, resulting in "evaluate: (-32000) Service connection disposed."

Here's the fixes:

  1. In eval_on_dart_library.dart
    • Allocated final reader = <dynamic>[]; directly inside the evaluated async function and registered it with widgetInspectorService.toId(reader, "$readerGroup") as String, eliminating the preliminary eval and ensuring reader is strongly referenced from the moment of allocation.
    • Transmitted reader_id (and any initialization error) directly in postEvent("future_completed", ...).
    • Wrapped the entire async function in an outer try/catch that reports errors back via postEvent rather than hanging DevTools.
    • Kept reader strongly pinned in the target isolate in the finally block by awaiting in a loop and accessing reader.length until evalInstance calls disposeGroup (or up to 10 seconds timeout).
  2. In flutter_test_environment.dart
    • Added !serviceConnection.serviceManager.connectedState.value.connected to setupEnvironment's re-initialization condition so that if the VM service connection is disposed or dropped, the test environment is automatically re-created for subsequent tests/retries.
  3. In eval_integration_test.dart
    • Added an explicit test, "survives garbage collection while the future is pending" that triggers full garbage collections in the target isolate via getAllocationProfile(..., gc: true) while the future is pending.
    • Removed tags: skipForCustomerTestsTag and retry: 3 now that the flakiness is resolved. 🎊 🎉

I looked at three different logs of devtools tests flaking out on Linux bots on LUCI,

All three logs failed in eval_integration_test.dart:

* Log 1 & Log 2: EvalOnDartLibrary asyncEval supports expressions that do not start with the await keyword timed out after 1 minute (TimeoutException: Test timed out after 1 minutes).
* Log 3: EvalOnDartLibrary asyncEval returns the result of the future completion timed out after 1 minute.
* Cascading failures: On all three runs, following the timeout, the test runner attempted retries (due to retry: 3), but every retry and subsequent test failed with evaluate: (-32000) Service connection disposed.

1. Garbage Collection of reader before evaluation began:
In eval_on_dart_library.dart:390-490, the reader list was previously created in a standalone eval (widgetInspectorService.toId(<dynamic>[], "$readerGroup")). Because WidgetInspectorService only holds objects using
WeakReference, nothing held a strong reference to <dynamic>[] in the target isolate during the network round-trip between DevTools and the target app. If garbage collection occurred in that window, widgetInspectorService.
toObject in the subsequent eval returned null, throwing:
  Unhandled exception: type 'Null' is not a subtype of type 'List<dynamic>' in type cast
Because this exception occurred before the try/finally block inside the closure, postEvent("future_completed", ...) was never posted, causing DevTools to hang waiting on future_completed until the test timed out after 1
minute.
2. Pinning loop stopped prematurely:
The pinning loop (while (!isDone && ++bufferTicks <= 20)) intended to pin reader until retrieved. However:
   * As soon as isDone was set to true, !isDone became false and the loop terminated immediately (providing zero buffer while postEvent traveled to DevTools and DevTools issued evalInstance). If a GC occurred during that
    window, toObject returned null.
   * For evaluations taking longer than 1 second (20 ticks of 50ms), ++bufferTicks <= 20 became false while the future was still pending, causing reader to be unpinned before completion.
3. Unhandled error in target isolate:
Any exception occurring during reader initialization or eval setup in the target app closure was unhandled, preventing future_completed from ever firing and causing the target isolate to crash/disconnect.
4. Stale environment reuse on connection drop:
In flutter_test_environment.dart:100-125, _needsSetup was not re-evaluated if the VM service connection dropped (!connectedState.value.connected). When a test timed out and the process disconnected, retries attempted to
reuse the disposed connection, resulting in evaluate: (-32000) Service connection disposed.

1. eval_on_dart_library.dart
   * Allocated `final reader = <dynamic>[];` directly inside the evaluated async function and registered it with `widgetInspectorService.toId(reader, "$readerGroup") as String`, eliminating the preliminary eval and ensuring
    reader is strongly referenced from the moment of allocation.
   * Transmitted reader_id (and any initialization error) directly in postEvent("future_completed", ...).
   * Wrapped the entire async function in an outer try/catch that reports errors back via postEvent rather than hanging DevTools.
   * Kept reader strongly pinned in the target isolate in the finally block by awaiting in a loop and accessing reader.length until evalInstance calls disposeGroup (or up to 10 seconds timeout).
2. flutter_test_environment.dart
   * Added !serviceConnection.serviceManager.connectedState.value.connected to setupEnvironment's re-initialization condition so that if the VM service connection is disposed or dropped, the test environment is
    automatically re-created for subsequent tests/retries.
3. eval_integration_test.dart
   * Added an explicit test survives garbage collection while the future is pending that triggers full garbage collections in the target isolate via getAllocationProfile(..., gc: true) while the future is pending.
   * Removed tags: skipForCustomerTestsTag and retry: 3 now that the flakiness is resolved.

• Ran dart analyze on modified files: No issues found.
• Ran dart format: All modified files formatted.
• Ran 5 consecutive test suites of packages/devtools_app/test/shared/eval_integration_test.dart: All 5 runs passed (5/5 tests passing per run).
@srawlins
srawlins requested review from a team and kenzieschmoll as code owners September 14, 2026 21:34

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors asyncEval in EvalOnDartLibrary to improve garbage collection and timeout handling by declaring the reader within the evaluated block and returning its ID via the completion event. It also adds a new integration test to verify survival during garbage collection and updates the test environment setup conditions. The review feedback highlights a potential compilation error in the target isolate due to passing a nullable readerId to toObject, and notes that integration tests should assert mainIsolate.value rather than the ValueListenable container itself.

' for (int i = 0; i < 200; i++) {'
' await Future.delayed(const Duration(milliseconds: 50));'
' try {'
' if (widgetInspectorService.toObject(readerId, "$readerGroup") == null) {'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

[MUST-FIX] Since readerId is declared as a nullable String? on line 423, passing it directly to widgetInspectorService.toObject (which expects a non-nullable String) will cause a static type error / compilation failure in the target isolate under sound null safety. Use the null-assertion operator readerId! to ensure it compiles correctly.

      '          if (widgetInspectorService.toObject(readerId!, "$readerGroup") == null) {'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

await env.setupEnvironment();
final mainIsolate =
serviceConnection.serviceManager.isolateManager.mainIsolate;
expect(mainIsolate, isNotNull);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

[CONCERN] mainIsolate is a ValueListenable<IsolateRef?>, which is a container that is never null. To assert that the main isolate has been successfully populated and is not null, you should check mainIsolate.value instead.

Suggested change
expect(mainIsolate, isNotNull);
expect(mainIsolate.value, isNotNull);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

await env.setupEnvironment();
final mainIsolate =
serviceConnection.serviceManager.isolateManager.mainIsolate;
expect(mainIsolate, isNotNull);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

[CONCERN] mainIsolate is a ValueListenable<IsolateRef?>, which is a container that is never null. To assert that the main isolate has been successfully populated and is not null, you should check mainIsolate.value instead.

Suggested change
expect(mainIsolate, isNotNull);
expect(mainIsolate.value, isNotNull);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant