fix: fork closure in epoch proving jobs#23390
Merged
Merged
Conversation
alexghr
requested review from
IlyasRidhuan,
MirandaWood and
jeanmon
as code owners
spalladino
approved these changes
May 19, 2026
Contributor
spalladino
left a comment
spalladino
left a comment
There was a problem hiding this comment.
LGTM. Just left a few comments on possible refactors.
Comment on lines
+345
to
+346
| // temporary stack to control fork lifetime | ||
| await using cleanup = new AsyncDisposableStack(); |
Comment on lines
+362
to
+393
| private async processCheckpoints( | ||
| parallelism: number, | ||
| processCheckpoint: (checkpoint: Checkpoint) => Promise<void>, | ||
| ): Promise<void> { | ||
| let hasError = false; | ||
| let firstError: unknown; | ||
|
|
||
| await asyncPool(Math.max(parallelism, 1), this.checkpoints, async checkpoint => { | ||
| if (hasError || this.abortController.signal.aborted) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| this.checkState(); | ||
| await processCheckpoint(checkpoint); | ||
| } catch (err) { | ||
| if (!hasError) { | ||
| hasError = true; | ||
| firstError = err; | ||
| this.failProcessing(); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| if (hasError) { | ||
| throw firstError; | ||
| } | ||
|
|
||
| if (this.abortController.signal.aborted) { | ||
| this.checkState(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Sounds like this is something we could bake into the asyncPool helper directly?
Contributor
Author
There was a problem hiding this comment.
I was wondering the same thing, but I didn't because the pool was forked from some repo.
Comment on lines
+504
to
531
| return await execWithSignal( | ||
| () => processFn(), | ||
| processingSignal, | ||
| signal => | ||
| signal.reason?.name === 'TimeoutError' ? new PublicProcessorTimeoutError() : new PublicProcessorAbortError(), | ||
| ); | ||
| } | ||
|
|
||
| private getProcessingSignal(tx: Tx, deadline: Date | undefined, signal: AbortSignal | undefined) { | ||
| if (!deadline) { | ||
| return signal; | ||
| } | ||
|
|
||
| const timeout = +deadline - this.dateProvider.now(); | ||
| if (timeout <= 0) { | ||
| throw new PublicProcessorTimeoutError(); | ||
| } | ||
|
|
||
| const txHash = tx.getTxHash(); | ||
| this.log.debug(`Processing tx ${txHash.toString()} within ${timeout}ms`, { | ||
| deadline: deadline.toISOString(), | ||
| now: new Date(this.dateProvider.now()).toISOString(), | ||
| txHash, | ||
| }); | ||
|
|
||
| return await executeTimeout( | ||
| () => processFn(), | ||
| timeout, | ||
| () => new PublicProcessorTimeoutError(), | ||
| ); | ||
| const timeoutSignal = AbortSignal.timeout(timeout); | ||
| return signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal; | ||
| } |
Contributor
There was a problem hiding this comment.
Seems like this is calling for an overload of executeTimeout or execWithSignal that has both timeout and signal?
Contributor
Author
There was a problem hiding this comment.
Honestly, I'd remove executeTimeout in favour of execWithSignal now that we can use AbortSignal.timeout in recent node versions.
Collaborator
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Handle fork lifetime correctly around checkpoints that might be cancelled part way through processing.