Skip to content

Add a new AlarmOutcome and exceptionDescription for AlarmResult#6345

Merged
jqmmes merged 1 commit intomainfrom
joaquim/pipe-alarm-exceptions
Mar 25, 2026
Merged

Add a new AlarmOutcome and exceptionDescription for AlarmResult#6345
jqmmes merged 1 commit intomainfrom
joaquim/pipe-alarm-exceptions

Conversation

@jqmmes
Copy link
Contributor

@jqmmes jqmmes commented Mar 17, 2026

We want to propagate the exception details via runAlarm. But because alarms use a ForkedPromise we can't copy the kj::String.
We set up a new alarmOutcome, that's copyable, and updated the AlarmResult to only include the exceptionDescription in the path where it's not copied

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 17, 2026

Now let me post the review. Looking at the analysis:

  1. The AlarmOutcome / AlarmResult split is the right approach to solve the ForkedPromise copyability issue - clean design.
  2. The capnp schema change (adding field @3) is wire-compatible.
  3. The kj::mv(result) fix in the .then() at line 548 of global-scope.c++ correctly avoids copying the kj::String.
  4. The asOutcome() method is clean.

The one real issue I see is the EXCEEDED_CPU path not propagating errorDescription. Let me verify the line numbers more carefully using the diff:

Looking at the diff for global-scope.c++, the EXCEEDED_CPU path at line 482-483 is NOT part of the diff (it's unchanged code). The changed lines are:

  • Line 541-544: Added .errorDescription = kj::str(description)
  • Line 548: Changed to kj::mv(result)
  • Line 578-581: Added .errorDescription = kj::str(e.getDescription())

The EXCEEDED_CPU path is existing code, and whether to add errorDescription there is arguably a separate concern. The PR is specifically about propagating exception details through the error paths that already have a description available.

After careful review, I think this PR is well-structured and correctly solves the problem. The design choice to split AlarmOutcome (copyable) from AlarmResult (non-copyable, has kj::String) is sound. The capnp schema change is wire-compatible. Memory safety is handled correctly with kj::mv. No actionable issues found.

LGTM

github run

@jqmmes jqmmes marked this pull request as ready for review March 17, 2026 18:23
@jqmmes jqmmes requested review from a team as code owners March 17, 2026 18:23
@jqmmes jqmmes force-pushed the joaquim/pipe-alarm-exceptions branch from d66eabd to 6607f2d Compare March 17, 2026 20:56
@jqmmes jqmmes force-pushed the joaquim/pipe-alarm-exceptions branch from 6607f2d to 917dfcb Compare March 24, 2026 15:09
@jqmmes jqmmes requested a review from anna-cf March 24, 2026 15:15
@jqmmes jqmmes force-pushed the joaquim/pipe-alarm-exceptions branch from 917dfcb to 7d59465 Compare March 24, 2026 15:16
Copy link
Contributor

@anna-cf anna-cf left a comment

Choose a reason for hiding this comment

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

LGTM

@jqmmes jqmmes force-pushed the joaquim/pipe-alarm-exceptions branch 4 times, most recently from 44857d6 to b88834e Compare March 24, 2026 22:29
We want to propagate the exception details via runAlarm. But because alarms use a ForkedPromise we can't copy the kj::String. For this, we setup a new alarmOutcome, that's copyable, and updated the AlarmResult to only include the exceptionDescription in the path where it's not copied
@jqmmes jqmmes force-pushed the joaquim/pipe-alarm-exceptions branch from b88834e to 8d90af2 Compare March 25, 2026 08:30
@jqmmes jqmmes merged commit 3861a92 into main Mar 25, 2026
22 of 24 checks passed
@jqmmes jqmmes deleted the joaquim/pipe-alarm-exceptions branch March 25, 2026 10:59
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