Skip to content

Commit

Permalink
Don't error on remote execution timeouts (pantsbuild#8269)
Browse files Browse the repository at this point in the history
Instead, return a FallibleExecuteProcessResult with exit code -15.

This means that timeouts are recoverable, rather than fatal to the whole run.

Fixes pantsbuild#8065

Apparently we don't timeout local executions at all, so I haven't
changed that for now. I'm sure we'll get to that at some point.
  • Loading branch information
illicitonion authored Sep 10, 2019
1 parent b667572 commit 8a2d438
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 8 deletions.
1 change: 1 addition & 0 deletions src/rust/engine/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/rust/engine/process_execution/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ fs = { path = "../fs" }
futures = "^0.1.16"
grpcio = { git = "https://github.com/pantsbuild/grpc-rs.git", rev = "4dfafe9355dc996d7d0702e7386a6fedcd9734c0", default_features = false, features = ["protobuf-codec", "secure"] }
hashing = { path = "../hashing" }
libc = "0.2.39"
log = "0.4"
protobuf = { version = "2.0.6", features = ["with-bytes"] }
sha2 = "0.8"
Expand Down
35 changes: 27 additions & 8 deletions src/rust/engine/process_execution/src/remote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use fs::{self, File, PathStat};
use futures::{future, Future, Stream};
use grpcio;
use hashing::{Digest, Fingerprint};
use libc;
use log::{debug, trace, warn};
use protobuf::{self, Message, ProtobufEnum};
use sha2::Sha256;
Expand Down Expand Up @@ -250,10 +251,22 @@ impl super::CommandRunner for CommandRunner {
let elapsed = start_time.elapsed();

if elapsed > timeout {
future::err(format!(
"Exceeded time out of {:?} with {:?} for operation {}, {}",
timeout, elapsed, operation_name, description
))
let ExecutionHistory {
mut attempts,
mut current_attempt,
} = history;
current_attempt.remote_execution = Some(elapsed);
attempts.push(current_attempt);
future::ok(future::Loop::Break(FallibleExecuteProcessResult {
stdout: Bytes::from(format!(
"Exceeded timeout of {:?} with {:?} for operation {}, {}",
timeout, elapsed, operation_name, description
)),
stderr: Bytes::new(),
exit_code: -libc::SIGTERM,
output_directory: hashing::EMPTY_DIGEST,
execution_attempts: attempts,
}))
.to_boxed()
} else {
// maybe the delay here should be the min of remaining time and the backoff period
Expand Down Expand Up @@ -1017,6 +1030,7 @@ pub mod tests {
use maplit::hashset;
use mock::execution_server::MockOperation;
use protobuf::well_known_types::Timestamp;
use spectral::numeric::OrderedAssertions;
use std::collections::{BTreeMap, BTreeSet};
use std::iter::{self, FromIterator};
use std::ops::Sub;
Expand Down Expand Up @@ -1717,10 +1731,15 @@ pub mod tests {
)
};

let error_msg = run_command_remote(mock_server.address(), execute_request)
.expect_err("Timeout did not cause failure.");
assert_contains(&error_msg, "Exceeded time out");
assert_contains(&error_msg, "echo-a-foo");
let result = run_command_remote(mock_server.address(), execute_request).unwrap();
assert_eq!(result.exit_code, -15);
let error_msg = String::from_utf8(result.stdout.to_vec()).unwrap();
assert_that(&error_msg).contains("Exceeded timeout");
assert_that(&error_msg).contains("echo-a-foo");
assert_eq!(result.execution_attempts.len(), 1);
let maybe_execution_duration = result.execution_attempts[0].remote_execution;
assert!(maybe_execution_duration.is_some());
assert_that(&maybe_execution_duration.unwrap()).is_greater_than_or_equal_to(request_timeout);
}

#[test]
Expand Down

0 comments on commit 8a2d438

Please sign in to comment.