Skip to content

tests/load: load timeout is reported as an assertion failure #5913

Description

@JonathanOppenheimer

@claude:

Summary

When --load-timeout expires, LoadGenerator.Run cancels the context that workers are actively using to issue and await transactions. The in-flight transaction on each worker then fails its require.NoError assertion, so a normal, intended end-of-run is logged as an assertion failure. There is currently no way to tell that log line apart from a real load-test failure.

Mechanism

  1. LoadGenerator.Run wraps the parent context in context.WithTimeout(ctx, loadTimeout) and hands that context to every worker.
  2. Each worker goroutine only checks ctx.Done() between iterations, so the timeout can (and normally does) fire in the middle of an iteration.
  3. execTestWithRecovery derives the test context from that same canceled context and installs it as the default context parent.
  4. The tests pass it straight into the wallet — e.g. TransferTest.Run and executeContractTx both call wallet.SendTx(tc.GetDefaultContextParent(), tx).
  5. Wallet.awaitTx returns ctx.Err() on cancellation.
  6. require.NoError then calls SimpleTestContext.Errorf, which logs at ERROR with a full testify error trace, followed by FailNow.

Net effect: every timed load run ends with up to one bogus ERROR + error trace per worker (context deadline exceeded), and errgroup still returns nil, so the run "passes" while the logs say it failed.

Note on ordering: on master, SimpleTestContext.Recover does not call recover() directly, so the FailNow panic isn't stopped at all. #5872 fixes that. This issue is about what remains once the panic is correctly recovered: the spurious error log and the failure semantics of a clean timeout.

Proposed fix

Separate "stop generating load" from "cancel the transaction in flight". Concretely, in LoadGenerator.Run:

  • Use the loadTimeout context (or a plain done channel) only for the worker loop's ctx.Done() guard.
  • Derive each iteration's context in execTestWithRecovery from the parent (un-timed) context plus testTimeout, so a worker that has already started a transaction finishes or fails it on its own merits.

Workers then drain naturally: no assertion fires, and Run can overrun loadTimeout by at most testTimeout (currently 1 minute), which seems like an acceptable trade for a trustworthy exit.

If bounding total runtime strictly matters more, the alternative is to keep cancelling the in-flight context and suppress the report when ctx.Err() != nil — but that has to happen before Errorf logs, so it means constructing the per-iteration context with NewTestContextWithArgs and a handler/logger that drops cancellation-caused failures. That's more machinery and it risks masking genuine timeouts.

Acceptance criteria

  • A load run with --load-timeout set logs no ERROR lines attributable to the timeout itself.
  • A genuine transaction failure (e.g. errTxExecutionFailed, an RPC error) is still reported as an assertion failure.
  • Runtime stays bounded (documented overrun of at most testTimeout).

Context

Raised in review of #5872: #5872 (comment)

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions