Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(composer): propagate errors #1838

Merged
merged 12 commits into from
Jan 23, 2025

Conversation

ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Nov 26, 2024

Summary

Propagate errors which occur while composer is starting up and/or running so that the will be returned by the Composer's handle.

Background

Previously, composer would only exit with an error if the collectors' or executor's status channels were closed, and then the error message did not provide detailed information about the error that occurred. Additionally, if either of the wait_for_* loops failed, the composer would not shut down gracefully. This change is meant to expose the first eyre report which causes the composer to shut down, and gracefully shut down in all circumstances.

Changes

  • Started collector and executor wait_for_ready loops concurrently, and continue with graceful shutdown even if these fail.
  • Store the first error composer encounters, and return it after graceful shutdown. If waiting for collectors or executor fails, Composer continues so that it can ascertain the underlying error from the task which caused it.

Testing

Passing all tests

Changelogs

Changelog updated

Related Issues

closes #1833

@ethanoroshiba ethanoroshiba added composer pertaining to composer code-quality labels Nov 26, 2024
@ethanoroshiba ethanoroshiba changed the base branch from main to ENG-914/composer_blackbox_tests November 27, 2024 14:24
@github-actions github-actions bot added the sequencer pertaining to the astria-sequencer crate label Nov 27, 2024
@ethanoroshiba ethanoroshiba removed the sequencer pertaining to the astria-sequencer crate label Nov 27, 2024
@ethanoroshiba ethanoroshiba force-pushed the ENG-1026/propagate_composer_errors branch from c32105a to d179b70 Compare November 27, 2024 14:54
@github-actions github-actions bot added the sequencer pertaining to the astria-sequencer crate label Nov 27, 2024
@ethanoroshiba ethanoroshiba force-pushed the ENG-1026/propagate_composer_errors branch from d179b70 to 0801687 Compare November 27, 2024 14:55
@ethanoroshiba ethanoroshiba removed the sequencer pertaining to the astria-sequencer crate label Nov 27, 2024
@ethanoroshiba ethanoroshiba marked this pull request as ready for review November 27, 2024 15:12
@ethanoroshiba ethanoroshiba requested a review from a team as a code owner November 27, 2024 15:12
@SuperFluffy
Copy link
Member

Left a bunch of comments. This does not seem to be a chore but more of a refactor.

)
.unwrap();
writeln!(&mut panic_msg, "expected cause:\n\t{expected_err_msg}").unwrap();
writeln!(&mut panic_msg, "actual cause chain:\n\t{err:?}").unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Debug log might look funky because debug logs are usually multiple lines, so the first line will have a tab before it, but the subsequent lines will not. A nice crate to do that on every line is https://crates.io/crates/indenter.

But I think the error here is the single-line astria-eyre one? Then in this case my comment likely doesn't matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially, but if for some reason the error is incorrect it could have a longer source chain. I could just remove the indentation, but I'm not sure I dislike the indenting as it is tbh. It'd look similar to how one would write paragraphs normally:

did not find expected executor error message
expected cause:
      some expected cause
actual cause chain:
      {0: asdfasdlskjdflksdjlalskdjflsalsdkjflasjdfalkdsfj, 1: asflkajsldkfjalksjdflakjsdfkasdjflka
sjdflja, 2: askdjfaksjdfkjasdkfhaksjdfhjksadjfh, 3: some underlying error}

Base automatically changed from ENG-914/composer_blackbox_tests to main January 22, 2025 14:39
@ethanoroshiba ethanoroshiba added this pull request to the merge queue Jan 23, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 23, 2025
@ethanoroshiba ethanoroshiba added this pull request to the merge queue Jan 23, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 23, 2025
@ethanoroshiba ethanoroshiba added this pull request to the merge queue Jan 23, 2025
Merged via the queue into main with commit 9553576 Jan 23, 2025
46 checks passed
@ethanoroshiba ethanoroshiba deleted the ENG-1026/propagate_composer_errors branch January 23, 2025 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-quality composer pertaining to composer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

composer: propagate component startup errors
3 participants