Skip to content

Commit

Permalink
Render local process infrastructure errors more verbosely. (#10368)
Browse files Browse the repository at this point in the history
### Problem

Currently we do not render a representation of the process when a process fails to start.

### Solution

When processes fail due to infrastructure or implementation errors (ie, they fail to start at all, or we fail to receive output from them or capture their outputs), we should render the relevant process information directly in the error. This should be a relatively rare case.

We do this only for the local process runner, because in the case of remote execution, infrastruture errors are much more common, and should potentially have a different treatment.

### Result

In a sandbox without `echo` defined:
```
Failed to execute: Process {
    argv: [
        "echo",
        "-n",
        "foo",
    ],
    env: {},
    <snip>
}

Error launching process: Os { code: 2, kind: NotFound, message: "No such file or directory" }
```
  • Loading branch information
stuhood committed Jul 15, 2020
1 parent 589d45d commit 1aaac7a
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 22 deletions.
47 changes: 28 additions & 19 deletions src/rust/engine/process_execution/src/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ impl super::CommandRunner for CommandRunner {
context: Context,
) -> Result<FallibleProcessResultWithPlatform, String> {
let req = self.extract_compatible_request(&req).unwrap();
let req_debug_repr = format!("{:#?}", req);
self
.run_and_capture_workdir(
req,
Expand All @@ -278,6 +279,17 @@ impl super::CommandRunner for CommandRunner {
&self.work_dir_base,
self.platform(),
)
.map_err(|msg| {
// Processes that experience no infrastructure issues should result in an "Ok" return,
// potentially with an exit code that indicates that they failed (with more information
// on stderr). Actually failing at this level indicates a failure to start or otherwise
// interact with the process, which would generally be an infrastructure or implementation
// error (something missing from the sandbox, incorrect permissions, etc).
//
// Given that this is expected to be rare, we dump the entire process definition in the
// error.
format!("Failed to execute: {}\n\n{}", req_debug_repr, msg)
})
.await
}
}
Expand Down Expand Up @@ -530,26 +542,23 @@ export {}
platform,
})
}
Err(msg) => {
if msg == "deadline has elapsed" {
let stdout = Bytes::from(format!(
"Exceeded timeout of {:?} for local process execution, {}",
req.timeout, req.description
));
let stdout_digest = store.store_file_bytes(stdout.clone(), true).await?;

Ok(FallibleProcessResultWithPlatform {
stdout_digest,
stderr_digest: hashing::EMPTY_DIGEST,
exit_code: -libc::SIGTERM,
output_directory: hashing::EMPTY_DIGEST,
execution_attempts: vec![],
platform,
})
} else {
Err(msg)
}
Err(msg) if msg == "deadline has elapsed" => {
let stdout = Bytes::from(format!(
"Exceeded timeout of {:?} for local process execution, {}",
req.timeout, req.description
));
let stdout_digest = store.store_file_bytes(stdout.clone(), true).await?;

Ok(FallibleProcessResultWithPlatform {
stdout_digest,
stderr_digest: hashing::EMPTY_DIGEST,
exit_code: -libc::SIGTERM,
output_directory: hashing::EMPTY_DIGEST,
execution_attempts: vec![],
platform,
})
}
Err(msg) => Err(msg),
}
}

Expand Down
6 changes: 4 additions & 2 deletions src/rust/engine/process_execution/src/local_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,11 @@ async fn env_is_deterministic() {

#[tokio::test]
async fn binary_not_found() {
run_command_locally(Process::new(owned_string_vec(&["echo", "-n", "foo"])))
let err_string = run_command_locally(Process::new(owned_string_vec(&["echo", "-n", "foo"])))
.await
.expect_err("Want Err");
assert!(err_string.contains("Failed to execute"));
assert!(err_string.contains("echo"));
}

#[tokio::test]
Expand Down Expand Up @@ -396,7 +398,7 @@ async fn test_directory_preservation() {
let script_metadata = std::fs::metadata(&run_script_path).unwrap();

// Ensure the script is executable.
assert_eq!(USER_EXECUTABLE_MODE, script_metadata.permissions().mode());
assert!(USER_EXECUTABLE_MODE & script_metadata.permissions().mode() != 0);

// Ensure the bash command line is provided.
let bytes_quoted_command_line = bash::escape(&bash_contents);
Expand Down
2 changes: 1 addition & 1 deletion src/rust/engine/src/nodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ impl WrappedNode for MultiPlatformExecuteProcess {
.command_runner
.run(request, execution_context)
.await
.map_err(|e| throw(&format!("Failed to execute process: {}", e)))?;
.map_err(|e| throw(&e))?;

Ok(ProcessResult(res))
} else {
Expand Down

0 comments on commit 1aaac7a

Please sign in to comment.