search

dart-lang / dart_ci

Tools used by Dart's continuous integration (CI) testing that aren't needed by Dart SDK contributors. Mirrored from dart.googlesource.com/dart_ci. Do not land pull requests on Github.
BSD 3-Clause "New" or "Revised" License
18 stars 5 forks source link

Record in database had empty configurations array, crashed UI loading of data. #73

Closed whesse closed 4 years ago

whesse commented 4 years ago

The result record https://firestore.googleapis.com/v1/projects/dart-ci/databases/(default)/documents/results/3fSeVfebnvTRuHeS0QWR had an empty array as the value of the 'configurations' field, and this crashed the UI's loading of data, because the Change.fromDocument constructor threw an exception.

I can't immediately see what could have made this change to the document - I don't see a way in which a race condition could make it fail. It looks slightly like the document might have entered the bad state when the approval was done, but that seems unlikely.

We could make the UI not crash when this happens, by checking for badly formed documents and not loading them, but we don't want to hide this issue if it happens again.

I can find results documents with an empty 'configurations' array by filtering on that field, and choosing ascending sort, in the Firebase console.

whesse commented 4 years ago

The exception is thrown at line https://dart.googlesource.com/dart_ci/+/refs/heads/master/results_feed/lib/src/model/commit.dart#171 when a result document has an empty or missing 'configurations' array, because the Configurations constructor called in 6 lines later, in line https://dart.googlesource.com/dart_ci/+/refs/heads/master/results_feed/lib/src/model/commit.dart#177 returns null when it gets null or an empty array as input. We could make the Change constructor not fail in that case, and not add a result object with an null configurations object to the loaded results, if we wanted to protect against this corruption in the future.

athomas commented 4 years ago

The problem was that updateActiveResult removed from the configurations array instead of the active_configurations array when there were more than one config active.

athomas commented 4 years ago

Deploying https://dart-review.googlesource.com/c/dart_ci/+/134301 to prevent this from happening again.