errors: narrow error types accepted by HistoryListener

[muzarski]

Ref: #519

New error types and variants

RequestError (reintroduced)

Error type returned by run_request_speculative_fiber. It represents either a definite request failure - last attempt error, connection pool error, or an empty plan error.

Replaced QueryError::RequestTimeout

This error variant was not good for 2 reasons:

1. it contains stringified error message
2. it was used for both user requests timeouts, and schema agreement timeouts
   I decoupled this variant into two separate variants that provide more context.

RequestError::RequestTimeout

After refactoring the timeout errors in QueryError, I added the corresponding variant to RequestError (one introduced in this PR) as well.

HistoryListener

Narrowed error types passed to log_query_error and log_attempt_error.

log_query_error (or rather log_request_error after rename) now accepts RequestError. log_attempt_error now accepts RequestAttemptError (representing an error of a single attempt).

Replaced "query" mentions with "request"

I did the replacement in trait methods, docstrings, tests etc. I tried to break the changes into multiple commits.

Execution errors summary

Currently, in session layer we can distinguish 3 kind of errors:

• RequestAttemptError - a failure of a single attempt of request execution. It is used in public API in RetrySession::decide_should_retry and HistoryListener::log_attempt_error
• RequestError - a definite failure of request execution, after all (speculative) retries etc. In addition to RequestAttemptError it contains information about the empty plan and request timeout. It also contains ConnectionPoolError variant. This variant is not included in RequestAttemptError, because we don't want to log such error in execution history - there is a comment suggesting that (is it still true, though?). RequestError is used in public API in HistoryListener::log_request_error.
• QueryError - a top-level error returned by Session methods. It holds some additional information - e.g. about the failure of request preparation - values serialization etc.

Pre-review checklist

• I have split my patch into logically separate commits.
• All commit messages clearly explain what they change and why.
• [ ] I added relevant tests for new features and bug fixes.
• All commits compile, pass static checks and pass test.
• PR description sums up the changes and reasons why they should be introduced.
• I have provided docstrings for the public items that I want to introduce.
• [ ] I have adjusted the documentation in ./docs/source/.
• I added appropriate Fixes: annotations to PR description.

[github-actions]

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: 5e6c26b

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 4a9367c1e1671773a704670de3c6acd089dc70d0
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 4a9367c1e1671773a704670de3c6acd089dc70d0
     Cloning 4a9367c1e1671773a704670de3c6acd089dc70d0
    Building scylla v0.15.0 (current)
       Built [  22.430s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.052s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  22.462s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.049s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.107s] 107 checks: 99 pass, 8 fail, 0 warn, 0 skip

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field StructuredHistory.requests in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/history.rs:255

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/enum_missing.ron

Failed in:
  enum scylla::observability::history::QueryHistoryResult, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-4a9367c1e1671773a704670de3c6acd089dc70d0/d3c4f05ef9753d3aea6d72f5a92b05356782e7b7/scylla/src/observability/history.rs:265

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/enum_variant_added.ron

Failed in:
  variant HistoryEvent:NewRequest in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/history.rs:90
  variant HistoryEvent:RequestSuccess in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/history.rs:91
  variant HistoryEvent:RequestError in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/history.rs:92

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/enum_variant_missing.ron

Failed in:
  variant HistoryEvent::NewQuery, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-4a9367c1e1671773a704670de3c6acd089dc70d0/d3c4f05ef9753d3aea6d72f5a92b05356782e7b7/scylla/src/observability/history.rs:90
  variant HistoryEvent::QuerySuccess, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-4a9367c1e1671773a704670de3c6acd089dc70d0/d3c4f05ef9753d3aea6d72f5a92b05356782e7b7/scylla/src/observability/history.rs:91
  variant HistoryEvent::QueryError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-4a9367c1e1671773a704670de3c6acd089dc70d0/d3c4f05ef9753d3aea6d72f5a92b05356782e7b7/scylla/src/observability/history.rs:92

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/struct_missing.ron

Failed in:
  struct scylla::observability::history::QueryId, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-4a9367c1e1671773a704670de3c6acd089dc70d0/d3c4f05ef9753d3aea6d72f5a92b05356782e7b7/scylla/src/observability/history.rs:18
  struct scylla::observability::history::QueryHistory, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-4a9367c1e1671773a704670de3c6acd089dc70d0/d3c4f05ef9753d3aea6d72f5a92b05356782e7b7/scylla/src/observability/history.rs:257

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field queries of struct StructuredHistory, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-4a9367c1e1671773a704670de3c6acd089dc70d0/d3c4f05ef9753d3aea6d72f5a92b05356782e7b7/scylla/src/observability/history.rs:253

--- failure trait_method_added: pub trait method added ---

Description:
A non-sealed public trait added a new method without a default implementation, which breaks downstream implementations of the trait
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/trait_method_added.ron

Failed in:
  trait method scylla::observability::history::HistoryListener::log_request_start in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/history.rs:40
  trait method scylla::observability::history::HistoryListener::log_request_success in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/history.rs:43
  trait method scylla::observability::history::HistoryListener::log_request_error in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/history.rs:46

--- failure trait_method_missing: pub trait method removed or renamed ---

Description:
A trait method is no longer callable, and may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#major-any-change-to-trait-item-signatures
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/trait_method_missing.ron

Failed in:
  method log_query_start of trait HistoryListener, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-4a9367c1e1671773a704670de3c6acd089dc70d0/d3c4f05ef9753d3aea6d72f5a92b05356782e7b7/scylla/src/observability/history.rs:40
  method log_query_success of trait HistoryListener, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-4a9367c1e1671773a704670de3c6acd089dc70d0/d3c4f05ef9753d3aea6d72f5a92b05356782e7b7/scylla/src/observability/history.rs:43
  method log_query_error of trait HistoryListener, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-4a9367c1e1671773a704670de3c6acd089dc70d0/d3c4f05ef9753d3aea6d72f5a92b05356782e7b7/scylla/src/observability/history.rs:46

     Summary semver requires new major version: 8 major and 0 minor checks failed
    Finished [  46.247s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  10.960s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.035s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  11.242s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.034s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.112s] 107 checks: 107 pass, 0 skip
     Summary no semver update required
    Finished [  22.929s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

[muzarski]

v2:

• rebased on most recent session: internal request execution API error types refactor #1157
• renamed UserRequestError -> UserRequestAttemptError and RetriableRequestError -> UserRequestError

[muzarski]

Rebased on latest #1157 (with modules refactor)

[muzarski]

v2.1: added a regression test for the scenario where the driver would unnecessarily wait for retry_interval and do the speculative retry in case of empty plan. The test's logic is explained in the (first) commit.

[muzarski]

Rebased on main

[Lorak-mmk]

I'll review whole PR soon, but I'm posting this comment separately because it may be important. Correct me if I'm wrong, but commit "spec_exec: flatten return type of run_request_speculative_fiber" seems to misunderstand speculative execution logic and introduce a bug because of that.

As you noted, the old logic ignores the empty plan error. You changed it to return immediately, which I think is not correct.

Consider the following scenario:

• We start some speculative fibers, some of them are still executing (e.g. sent a request and waiting for an answer). They exhausted the plan (note that the plan is shared between fibers!)
• Timer goes off, we start another fiber
• This new fiber gets an empty plan, so returns immediately
• We should ignore this error, because not all existing fibers finished yet. You new logic would return immediately.

There is one more issue: let's say that in the above scenario the "empty plan" fiber was actually the last fiber (and so we should return an error now, because the execution definitely failed). The error we should return is not the "empty plan" error from this fiber, but the previous error. This is because empty plan error is just a symptom of all other executions failing.
This is what this code did in the old logic:

 return last_error.unwrap_or({
      Err(EMPTY_PLAN_ERROR)
  });

This caused us to return EMPTY_PLAN_ERROR only if last_error was None. If we finished all executions (async_tasks.is_empty() && retries_remaining == 0), and the error is None, that means no attempt returned an error, so there was no attempt at all, so the plan was empty. This is the only scenario where we want to return the empty plan error.

[Lorak-mmk]

❓ Huh, I wonder why this is here. Intuition tells me that we can do a retry (using ofc a retry policy, so potentially on another node) in such situation, in which case this would fall under the "RequestAttemptError". Why is this variant separated?

[muzarski]

This is a great question. I wanted to put it under RequestAttemptError, but then I saw the comment: https://github.com/muzarski/scylla-rust-driver/blob/main/scylla/src/client/session.rs#L2033-L2035. It says that we should not log such error in metrics. By metrics, I understand the request execution history. If I introduced the corresponding variant in RequestAttemptError, we once again would be passing a "too broad" error type to HistoryListener::log_attempt_error - variant corresponding to ConnectionPoolError would never be constructed.

[Lorak-mmk]

💭 "errors: add RequestTimeout variant to RequestError " adds a new variant to RequestError, but this variant is not constructed. I assume that future commits / PRs will construct it - for example by changing some function to return RequestError instead of QueryError

[muzarski]

It is constructed in the next commit here 4bae738#diff-8dcf6c908c350e5e0977344268a9ba9095466e548365d416b436c938a83e49d2R1980-R1982 (I hope that this link works as expected)

[Lorak-mmk]

❓ Why did you make the changes in this test? They seem to change the test quite significantly.

[muzarski]

This is a perfect example of why QueryError was "too broad" for log_attempt_error. QueryError::TimeoutError could never be passed to this method, because it's an error constructed in the higher layer of execution (https://github.com/muzarski/scylla-rust-driver/blob/main/scylla/src/client/session.rs#L1974-L1987) which is done after we log all attempt errors. Since we narrowed the error type passed to log_attempt_error, I needed to adjust the test accordingly (there is no TimeoutError variant in RequestAttemptError).

[Lorak-mmk]

❓ Here too, why this change?

[muzarski]

I'll review whole PR soon, but I'm posting this comment separately because it may be important. Correct me if I'm wrong, but commit "spec_exec: flatten return type of run_request_speculative_fiber" seems to misunderstand speculative execution logic and introduce a bug because of that.

As you noted, the old logic ignores the empty plan error. You changed it to return immediately, which I think is not correct.

Consider the following scenario:

* We start some speculative fibers, some of them are still executing (e.g. sent a request and waiting for an answer). They exhausted the plan (note that the plan is shared between fibers!)

* Timer goes off, we start another fiber

* This new fiber gets an empty plan, so returns immediately

* We should ignore this error, because not all existing fibers finished yet. You new logic would return immediately.

There is one more issue: let's say that in the above scenario the "empty plan" fiber was actually the last fiber (and so we should return an error now, because the execution definitely failed). The error we should return is not the "empty plan" error from this fiber, but the previous error. This is because empty plan error is just a symptom of all other executions failing. This is what this code did in the old logic:

 return last_error.unwrap_or({
      Err(EMPTY_PLAN_ERROR)
  });

This caused us to return EMPTY_PLAN_ERROR only if last_error was None. If we finished all executions (async_tasks.is_empty() && retries_remaining == 0), and the error is None, that means no attempt returned an error, so there was no attempt at all, so the plan was empty. This is the only scenario where we want to return the empty plan error.

You are 100% correct, this is a bug. Thanks for spotting it. After giving it a thought, I think that I should revert this change - i.e. Option<Result<...>> is a good choice for a return type of run_request_speculative_fiber.

I also wonder if we should handle scenarios such as presented in the regression test. In other words, not to unnecessarily wait for a retry if the following conditions are true:

• finished task returned None (plan was empty/exhausted)
• there are no other running fibers - for checking this, we can introduce additional variable which would store the value of currently running fibers

Do you think that it makes sense to prevent from such scenarios? Or is it such rare scenario that it's not worth to introduce additional noise to the function (new variable, additional branching etc.)

[Lorak-mmk]

You are 100% correct, this is a bug. Thanks for spotting it. After giving it a thought, I think that I should revert this change - i.e. Option<Result<...>> is a good choice for a return type of run_request_speculative_fiber.

I also wonder if we should handle scenarios such as presented in the regression test. In other words, not to unnecessarily wait for a retry if the following conditions are true:

* finished task returned `None` (plan was empty/exhausted)

* there are no other running fibers - for checking this, we can introduce additional variable which would store the value of currently running fibers

Do you think that it makes sense to prevent from such scenarios? Or is it such rare scenario that it's not worth to introduce additional noise to the function (new variable, additional branching etc.)

IIUC this would mostly help in a scenario where the plan was already exhausted, but there are still some speculative execution left to start ("retries_remaining > 0").
We could of course do this, but I'm a bit worried about the possibility of bugs, both now and in the future.
While this piece of code doesn't really look complicated, it already had a bug that I fixed in the past: #1086 ,
and now you nearly introduced another one. For me this is an indication that this piece of logic is more complicated and error-prone than it seems at first glance, so making it more complicated may backfire at us.

Maybe you have an idea on how to add this functionality while also rewriting this logic to be less dangerous?

[muzarski]

IIUC this would mostly help in a scenario where the plan was already exhausted, but there are still some speculative execution left to start ("retries_remaining > 0").

Exactly.

We could of course do this, but I'm a bit worried about the possibility of bugs, both now and in the future. While this piece of code doesn't really look complicated, it already had a bug that I fixed in the past: #1086 , and now you nearly introduced another one. For me this is an indication that this piece of logic is more complicated and error-prone than it seems at first glance, so making it more complicated may backfire at us.

I 100% agree.

Maybe you have an idea on how to add this functionality while also rewriting this logic to be less dangerous?

I can think about it, but this is probably out of scope of this PR. I'll open an issue and I believe this could even be addressed in post-1.0.0 driver.

[Lorak-mmk]

We could of course do this, but I'm a bit worried about the possibility of bugs, both now and in the future. While this piece of code doesn't really look complicated, it already had a bug that I fixed in the past: #1086 , and now you nearly introduced another one. For me this is an indication that this piece of logic is more complicated and error-prone than it seems at first glance, so making it more complicated may backfire at us.

I 100% agree.

Random thought: I wonder (not for the first time) if Rust makes us developers more "lazy" because of how many problems it eliminates automatically.
By "lazy" I mean less careful and thus more likely to not notice some bugs, than developers using other languages.

[Lorak-mmk]

One more thing: could you provide a very short overview in the description on request-error related types that exist after the PR?
By that I mean what types (RequestError? QueryError? RequestAttemptError? maybe some others I don't know?) exist and what is their purpose.

[muzarski]

v3:

• rebased on main
• dropped the first commit that would change the return type of run_request_speculative_fiber from Option<Result<_>> to Result<_>
• addressed minor issues from @Lorak-mmk comments
• updated cover letter - added execution error types summary

[Lorak-mmk]

Looks fine now. I'm not fully convinced about the division between QueryError (btw will it be renamed?), RequestError and RequestAttemptError. But that is something that may be better addressed by discussion at the office some day.

[muzarski]

Looks fine now. I'm not fully convinced about the division between QueryError (btw will it be renamed?)

Yes, I'd like to rename it. We discussed it with @wprzytula and figured out that ExecutionError would fit here.