Skip to content

Conversation

@SirTyson
Copy link
Contributor

@SirTyson SirTyson commented Dec 5, 2025

Description

Currently, if a background thread throws, we don't properly go through graceful shutdown. This gracefully handles background exceptions.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@SirTyson SirTyson requested review from Copilot and marta-lokhova and removed request for Copilot December 5, 2025 22:19
{
JITTER_INJECT_DELAY();
timer.Update(isSlow.checkElapsedTime());
try
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m relucant to take a one-size-fits-all approach here. The system has a lot of nuanced failure modes, and in many cases catching an exception and shutting down gracefully isn’t actually the right thing to do. Sometimes the exception is a symptom of prior state corruption, and in those cases we really need to evaluate the specific scenario to decide whether we need to do something else, like a forced shutdown or an abort.

This change is specifically in the context of the new invariant, correct? If so, could we scope it to invariants only, so we don’t change failure behavior in other subsystems? Btw, currently strict invariants abort the program. With this change, it looks like we go into graceful shutdown, which doesn't seem right. I think we should probably abort for snapshot-based invariants as well (which would help spot problematic watchers).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for simplicity, I went ahead and just handled the snapshot invariant case. This is the only invariant that actually runs on the background, so it seemed to make sense just to special case the failure mode for this specific invariant.

@SirTyson SirTyson force-pushed the background-exception-handling branch from 47a7b06 to 1c8ce60 Compare December 11, 2025 15:49
Copilot AI review requested due to automatic review settings December 11, 2025 15:49
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds exception handling to the runStateSnapshotInvariant method to ensure background thread exceptions are caught and result in graceful shutdown via printErrorAndAbort, rather than leaving the exception unhandled.

Key changes:

  • Wraps invariant snapshot checking in a try-catch block to handle exceptions in background thread execution
  • Adds the GlobalChecks.h include to access printErrorAndAbort function

@SirTyson SirTyson force-pushed the background-exception-handling branch from 1c8ce60 to 351aa86 Compare December 12, 2025 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants