Suppress last-ditch download exceptions w/cleanup #10029

werkt · 2019-10-14T21:24:40Z

Create an encapsulating DownloadException to represent the aggregate
exception set of an ActionResult download. IOExceptions will be retained
through exception suppression, and the outer exception has a property to
indicate if it only represents a sequence of CacheNotFoundExceptions.

InterruptedExceptions interception is improved to cancel pending work
and wrap, through suppression, any DownloadException that also occurred
during the download. InterruptedException being thrown on the download
control thread, it does not require suppression of further interrupts,
and can represent an outer download exception. Thread interrupt status
is suppressed for cancellations, and conveyed on throw.

These exception wrapping efforts allow non-asynchronous frame
representation in stack traces, and much clearer identification of
sources within remote strategy execution which produce failures based on
remote errors.

Any DownloadException in the last-ditch output download under
handleError in RemoteSpawnRunner is added as suppressed to the
initiating exception. Other exceptions (likely local IO) present clear
immediate traces and do not require specialized treatment.

Create an encapsulating DownloadException to represent the aggregate exception set of an ActionResult download. IOExceptions will be retained through exception suppression, and the outer exception has a property to indicate if it only represents a sequence of CacheNotFoundExceptions. InterruptedExceptions interception is improved to cancel pending work and wrap, through suppression, any DownloadException that also occurred during the download. InterruptedException being thrown on the download control thread, it does not require suppression of further interrupts, and can represent an outer download exception. Thread interrupt status is suppressed for cancellations, and conveyed on throw. These exception wrapping efforts allow non-asynchronous frame representation in stack traces, and much clearer identification of sources within remote strategy execution which produce failures based on remote errors. Any DownloadException in the last-ditch output download under handleError in RemoteSpawnRunner is added as suppressed to the initiating exception. Other exceptions (likely local IO) present clear immediate traces and do not require specialized treatment.

buchgr · 2019-11-12T15:49:02Z

I am not sure how I feel about this change. In part because it seems to be treating a symptom instead of the root cause: If you see a stacktrace from Bazel then I'd consider this a bug. Would you agree?

werkt · 2019-11-12T16:13:10Z

I think that might be an oversimplification of the presentation of a stack trace - fwiw this doesn't change the current behavior of only making these traces visible with --verbose_failures, it simply includes logic that ensures that if a trace is printed as a result of a download failure with --verbose_failures enabled and without --remote_local_fallback, it contains lines that refer to the actual bazel code which invoked the requests which threw exceptions, to provide a bare minimum of identification of what it was doing, as opposed to the grpc-only frames present during client calls.

werkt · 2020-03-06T20:11:16Z

@jin ping on triage for this

To any future reviewers, this does not add stack traces where they did not exist before. This only improves the quality of existing RuntimeExceptions from skyframe or exceptions printed with --verbose_failures.

jin · 2020-03-09T17:48:50Z

Sorry, was OOO. This looks like a general developer experience improvement. @philwo wdyt?

ulfjack

It generally looks like an improvement.

src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java

ulfjack · 2020-03-09T19:08:10Z

src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java

+      add(e);
+    }
+
+    void add(IOException e) {


Why not override addSuppressed?

I wanted a filter on this, so that it couldn't contain/suppress anything but IOExceptions. That is probably unnecessary, but I felt it needed scope.

And as it turns out, addSuppressed is final

src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java

src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java

And make it an IOException

Skip unnecessary self-suppressed test

ulfjack

LGTM, just a documentation nit

ulfjack · 2020-03-13T08:54:15Z

src/main/java/com/google/devtools/build/lib/remote/DownloadException.java

+    add(e);
+  }
+
+  void add(IOException e) {


Can you add documentation that this works like addSuppressed and that we can't override addSuppressed?

ulfjack · 2020-03-27T09:00:07Z

src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionCache.java

+        IOException ioEx;
+        if (cause instanceof IOException) {
+          ioEx = new IOException(cause);
+        } else {
+          ioEx = (IOException) cause;
+        }


Suggested change

IOException ioEx;

if (cause instanceof IOException) {

ioEx = new IOException(cause);

} else {

ioEx = (IOException) cause;

}

IOException ioEx = cause instanceof IOException ? (IOException) cause : new IOException(cause);

There's a better solution to this that looks more like what's available in RemoteCache.java, i'll refactor that and replace this.

src/main/java/com/google/devtools/build/lib/remote/BulkTransferException.java

ulfjack · 2020-03-27T09:01:34Z

src/main/java/com/google/devtools/build/lib/remote/BulkTransferException.java

+   * its place to selectively filter and record whether all suppressed
+   * exceptions are CacheNotFoundExceptions
+   */
+  void add(IOException e) {


I wonder if this can ever be called in a multi-threaded context.

It cannot in any of the uses I've added here - all the aggregations happen on a single thread.

…rException.java Co-Authored-By: Ulf Adams <ulfjack@users.noreply.github.com>

All transfers use this routine to wait for future list completion with optional cancellation, which replaces waitForUploads. Download futures are strongly encouraged to present IOExceptions through getFromFuture. Removed unused getFromFuture instance method. Upload cancellations, whether from inputs or outputs, remain disabled due to bazelbuild#10846.

werkt · 2020-04-07T17:28:26Z

Can we see this through? The problem that this corrects in >1.1.0 is keeping us from upgrading to 3.0.0 or even any 2. @philwo?

jin · 2020-04-08T18:21:06Z

I can bring it in.

dws · 2020-04-08T19:16:27Z

I can bring it in.

If this could be part of 3.1 that would be excellent.

jin · 2020-04-08T19:28:23Z

If this could be part of 3.1 that would be excellent.

cc @laurentlb (release manager for 3.1 #11076)

laurentlb · 2020-04-08T19:51:33Z

Sounds good. Please leave a comment on #11076 with the commit hash.

werkt · 2020-04-14T21:13:15Z

Any movement here? We need a commit in master before we can get ourselves into the running for 3.1

jin · 2020-04-14T21:14:50Z

Yes. Phillipp is OOO so I'm requesting for another reviewer internally.

…

On Wed, Apr 15, 2020 at 5:13 AM George Gensure ***@***.***> wrote: Any movement here? We need a commit in master before we can get ourselves into the running for 3.1 — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#10029 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACU6DU7A452IZTH73LBKMDRMTGXVANCNFSM4JAUGV3Q> .

Create an encapsulating DownloadException to represent the aggregate exception set of an ActionResult download. IOExceptions will be retained through exception suppression, and the outer exception has a property to indicate if it only represents a sequence of CacheNotFoundExceptions. InterruptedExceptions interception is improved to cancel pending work and wrap, through suppression, any DownloadException that also occurred during the download. InterruptedException being thrown on the download control thread, it does not require suppression of further interrupts, and can represent an outer download exception. Thread interrupt status is suppressed for cancellations, and conveyed on throw. These exception wrapping efforts allow non-asynchronous frame representation in stack traces, and much clearer identification of sources within remote strategy execution which produce failures based on remote errors. Any DownloadException in the last-ditch output download under handleError in RemoteSpawnRunner is added as suppressed to the initiating exception. Other exceptions (likely local IO) present clear immediate traces and do not require specialized treatment. Closes #10029. PiperOrigin-RevId: 306619678

ActionResult Orphaned Output Directories must be wrapped in BulkTransferExceptions in order to be eligible for re-execution. This change prevents an unavoidable build failure introduced with bazelbuild#10029 in the event that an output directory tree specified in an action result is missing from the CAS.

ActionResult Orphaned Output Directories must be wrapped in BulkTransferExceptions in order to be eligible for re-execution. This change prevents an unavoidable build failure introduced with bazelbuild#10029 in the event that an output directory tree specified in an action result is missing from the CAS, and adds testing that would detect such a regression.

ActionResult Orphaned Output Directories must be wrapped in BulkTransferExceptions in order to be eligible for re-execution. This change prevents an unavoidable build failure introduced with #10029 in the event that an output directory tree specified in an action result is missing from the CAS, and adds testing that would detect such a regression. Closes #11140. PiperOrigin-RevId: 307027914

Create an encapsulating DownloadException to represent the aggregate exception set of an ActionResult download. IOExceptions will be retained through exception suppression, and the outer exception has a property to indicate if it only represents a sequence of CacheNotFoundExceptions. InterruptedExceptions interception is improved to cancel pending work and wrap, through suppression, any DownloadException that also occurred during the download. InterruptedException being thrown on the download control thread, it does not require suppression of further interrupts, and can represent an outer download exception. Thread interrupt status is suppressed for cancellations, and conveyed on throw. These exception wrapping efforts allow non-asynchronous frame representation in stack traces, and much clearer identification of sources within remote strategy execution which produce failures based on remote errors. Any DownloadException in the last-ditch output download under handleError in RemoteSpawnRunner is added as suppressed to the initiating exception. Other exceptions (likely local IO) present clear immediate traces and do not require specialized treatment. Closes #10029. PiperOrigin-RevId: 306619678

ActionResult Orphaned Output Directories must be wrapped in BulkTransferExceptions in order to be eligible for re-execution. This change prevents an unavoidable build failure introduced with #10029 in the event that an output directory tree specified in an action result is missing from the CAS, and adds testing that would detect such a regression. Closes #11140. PiperOrigin-RevId: 307027914

laurentlb · 2020-04-20T10:01:50Z

The fix was cherrypicked on Friday.
If you try the release candidate, let me know if there's any other blocker (we might do the release tomorrow, but can delay it if it helps).

werkt · 2020-04-20T13:14:53Z

@laurentlb we've been building since rc3 was cut and haven't found any other blockers, thanks for the cherry pick!

werkt requested review from buchgr, hlopko, ishikhman and ola-rozenfeld as code owners October 14, 2019 21:24

googlebot added the cla: yes label Oct 14, 2019

jin added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Oct 14, 2019

jin assigned buchgr Oct 14, 2019

Revert unrelated testing change

77e5c65

hlopko removed their request for review October 15, 2019 12:23

Merge branch 'master' into remote-traces

58a6103

buchgr removed their assignment Jan 9, 2020

jin removed request for ishikhman, buchgr and ola-rozenfeld March 9, 2020 17:35

jin assigned jin and philwo and unassigned jin Mar 9, 2020

jin requested a review from philwo March 9, 2020 17:35

ulfjack reviewed Mar 9, 2020

View reviewed changes

George Gensure added 4 commits March 9, 2020 16:56

Merge branch 'master' into remote-traces

b315dcb

Move DownloadException to remote package

6bb872b

And make it an IOException

Revert to previous downloadException behavior

20ad9ed

Skip unnecessary self-suppressed test

Fix import for tests of DownloadException

28b94e5

ulfjack reviewed Mar 13, 2020

View reviewed changes

Remove additional IOException wrappings

7fe3e6b

ulfjack reviewed Mar 27, 2020

View reviewed changes

werkt and others added 6 commits March 27, 2020 11:12

Update src/main/java/com/google/devtools/build/lib/remote/BulkTransfe…

98bbeb4

…rException.java Co-Authored-By: Ulf Adams <ulfjack@users.noreply.github.com>

Remove unnecessary BTE wrap

2bb742a

Route downloadOutErr IOExceptions to future

65c41a8

Merge branch 'master' into remote-traces

97d4911

correct merge error

cfb2a2d

ulfjack approved these changes Mar 30, 2020

View reviewed changes

Merge branch 'master' into remote-traces

6c516b9

bazel-io closed this in aeee3e0 Apr 15, 2020

laurentlb mentioned this pull request Apr 15, 2020

Release 3.1 - April 2020 #11076

Closed

10 tasks

werkt deleted the remote-traces branch April 15, 2020 13:48

werkt mentioned this pull request Apr 17, 2020

Guard parseActionResultMetadata with bulk wrapper #11140

Closed

werkt mentioned this pull request Apr 22, 2020

Perform Bulk Wait in InputFetcher for downloads #11197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress last-ditch download exceptions w/cleanup #10029

Suppress last-ditch download exceptions w/cleanup #10029

werkt commented Oct 14, 2019

buchgr commented Nov 12, 2019

werkt commented Nov 12, 2019

werkt commented Mar 6, 2020

jin commented Mar 9, 2020

ulfjack left a comment

ulfjack Mar 9, 2020

werkt Mar 9, 2020

werkt Mar 9, 2020

ulfjack left a comment

ulfjack Mar 13, 2020

ulfjack Mar 27, 2020

werkt Mar 27, 2020

ulfjack Mar 27, 2020

werkt Mar 27, 2020

werkt commented Apr 7, 2020 •

edited

Loading

jin commented Apr 8, 2020

dws commented Apr 8, 2020

jin commented Apr 8, 2020

laurentlb commented Apr 8, 2020

werkt commented Apr 14, 2020

jin commented Apr 14, 2020 via email

laurentlb commented Apr 20, 2020

werkt commented Apr 20, 2020

Suppress last-ditch download exceptions w/cleanup #10029

Suppress last-ditch download exceptions w/cleanup #10029

Conversation

werkt commented Oct 14, 2019

buchgr commented Nov 12, 2019

werkt commented Nov 12, 2019

werkt commented Mar 6, 2020

jin commented Mar 9, 2020

ulfjack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulfjack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

werkt commented Apr 7, 2020 • edited Loading

jin commented Apr 8, 2020

dws commented Apr 8, 2020

jin commented Apr 8, 2020

laurentlb commented Apr 8, 2020

werkt commented Apr 14, 2020

jin commented Apr 14, 2020 via email

laurentlb commented Apr 20, 2020

werkt commented Apr 20, 2020

werkt commented Apr 7, 2020 •

edited

Loading