fix: Properly synchronize slog sender termination #11052
+119
−128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deploying agoric-sdk with
|
| Latest commit: |
80be157
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dfd80380.agoric-sdk.pages.dev |
| Branch Preview URL: | https://gibson-10925-followup.agoric-sdk.pages.dev |
mhofman
reviewed
Feb 28, 2025
| * @returns {F} the wrapped method | ||
| */ | ||
| wrap(method, impl) { | ||
| const wrappedMethods = /** @type {typeof methods} */ ( |
There was a problem hiding this comment.
Why not Methods instead of typeof methods?
There was a problem hiding this comment.
To allow for the increased specificity.
Comment on lines
+89
to
+92
| const waitForDrain = await written; | ||
| if (waitForDrain) { | ||
| await new Promise(resolve => stream.once('drain', resolve)); | ||
| } |
There was a problem hiding this comment.
Yeah that's a little more respectful of the drain signal.
Comment on lines
110
to
111
| // @ts-expect-error calling a possibly missing method | ||
| stream.close?.(); | ||
| if (stream.end) { | ||
| await new Promise(resolve => stream.end(() => resolve(undefined))); | ||
| } |
There was a problem hiding this comment.
close is only present on the fs WriteStream, whereas end also exists on the stdout stream, which we don't want to end. Is there any reason to change this one?
There was a problem hiding this comment.
I tried that in an early iteration and close wasn't final enough, though I don't remember the details. I'm updating to exempt process.stdout from this handling.
|
@mhofman back to you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refs: #10925
Description
This was prompted by #10925 (comment) (calling out missing
leveldata for kernel console slog entries), but more fundamentally fixes the interaction between cosmic-swingset, the kernel, and the telemetry system (and performs a few cleanups along the way).Security Considerations
Security posture should not be affected.
Scaling Considerations
None.
Documentation Considerations
n/a
Testing Considerations
Manual confirmation per #10776:
slog excerpt
Upgrade Considerations
This fixes a late-discovered flaw in #10925 and would ideally be cherry-picked into agoric-upgrade-19, but if it doesn't make it in then we'll just be missing the kernel console
leveldata until agoric-upgrade-20 (which is still acceptable).Release verification should look for slog entries like the above (type "console", source "kernel", level "log"/"warn"/"error"/etc.).