Repository navigation
Add concurrent transaction plan leaf executor - #1995
Conversation
🦋 Changeset detectedLatest commit: ba9f75e The changes in this PR will be included in the next version bump. This PR includes changesets to release 48 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
BundleMonFiles updated (4)
Unchanged files (146)
Total files change +1020B +0.18% Final result: ✅ View report in BundleMon website ➡️ |
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
This PR adds createTransactionPlanExecutorWithConcurrentLeaves, a deliberately simple sibling to createTransactionPlanExecutor that maps a SingleTransactionPlan -> Promise<SingleTransactionPlanResult<TContext>> callback over every leaf concurrently, ignoring sequential dependencies, and rebuilds a TransactionPlanResult with the same nesting, order, and divisibility as the input plan. Intended for dependency-irrelevant operations like signing without executing (extracted from anza-xyz/kit-plugins#395).
The implementation is tidy and well-scoped — this would be an approval if I had the repo access to file one:
- The recursive traversal handles all three plan kinds with an exhaustive
kind satisfies neverfallback throwingSOLANA_ERROR__INVARIANT_VIOLATION__INVALID_TRANSACTION_PLAN_KIND, consistent with the existingtraversefunction in the same file. - Unlike
createTransactionPlanExecutor, this one supports non-divisible sequential plans and reconstructs them vianonDivisibleSequentialTransactionPlanResult— that's correct given dependencies are irrelevant here, and the first unit test covers it explicitly. - The abort signal is forwarded rather than enforced (no
getAbortablePromise, no pre-flightabortedcheck). The docs are explicit that cancellation handling is the callback's responsibility, so I read this as intentional. - TSDoc is thorough and matches the actual behavior, including the
@throwsdocumentation for callback errors. - Tests cover shape preservation, concurrent start of sequential leaves, abort-signal forwarding, and the unknown-kind invariant. Type tests cover explicit and inferred
TContextplus the callback signature and context obligation. - Changeset is present with a
minorbump, which matches precedent for new public API in this repo.
Two things flagged inline:
ConcurrentLeafTransactionPlanExecutorConfigis imported in the typetest but never used — likely a lint failure waiting to happen.- A question on error semantics: a throwing leaf callback propagates the raw error via
Promise.all, with no partial result shape (unlike theFAILED_TO_EXECUTE_TRANSACTION_PLANerror the sibling executor throws, whichpassthroughFailedTransactionPlanExecutionunderstands). This looks intentional given callbacks are expected to return failed/canceled results rather than throw, but worth confirming.
For subsequent reviewers: the main thing to sanity-check is the error-propagation contract in point 2 above, especially how the kit-plugins#395 refactor will consume this — if that call site relies on passthroughFailedTransactionPlanExecution, callbacks there must never throw.
Transaction preparation workflows such as signing need to process all transaction plan leaves without enforcing sequential send dependencies. Add a concurrent-leaf executor that preserves result nesting, order, and divisibility while keeping result context caller-defined. Aggregate independent leaf failures into the complete result tree before throwing the standard execution error, and align preflight and in-flight abort behavior with the existing transaction plan executor. Cover traversal, failure aggregation, cancellation, and type inference behavior.
f9bff3a to
ba9f75e
Compare
|
I've aligned the failure aggregation behaviour, and the abort signal handling, with |
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest changes — both points from my previous review are resolved, and the error/abort semantics have been substantially strengthened. This would be an approval if I could file one.
What changed since last review:
-
Unused import — resolved.
ConcurrentLeafTransactionPlanExecutorConfigis now exercised by the new typetest block asserting that the config type has no defaultTContext(via@ts-expect-error). -
Error semantics — resolved, and better than what I asked for. Instead of propagating raw callback errors through
Promise.all, thrown errors are now caught per-leaf and converted toFailedSingleTransactionPlanResults without affecting sibling leaves. After all leaves settle, any failed/canceled result in the tree produces aSOLANA_ERROR__INSTRUCTION_PLANS__FAILED_TO_EXECUTE_TRANSACTION_PLANviacreateFailedToExecuteTransactionPlanError, carrying the complete result tree. This meanspassthroughFailedTransactionPlanExecutionnow works uniformly across both executors — which directly resolves my concern about the kit-plugins#395 refactor inheriting surprising failure behavior. -
Abort signal is now enforced, not just forwarded. Pre-aborted signals cancel leaves without invoking callbacks; mid-flight aborts race callbacks via
getAbortablePromiseand record failed results carrying the abort reason, while preserving already-completed results. TheabortReasonlands in the error context. TSDoc was updated to match.
Verification notes:
- The failed-vs-canceled asymmetry (pre-abort →
canceled, mid-flight abort →failedwith the abort reason as the error) mirrorstraverseSinglein the sibling executor exactly, is documented in the TSDoc, and is asserted by the abort tests — I read it as intentional and consistent. - The
error as Errorcast in the leaf catch matches the identical pre-existing pattern intraverseSingle; no new concern. - Test coverage is thorough: shape preservation, concurrent starts of sequential leaves, per-leaf failure aggregation with
error.causepointing at the first tree failure, waiting for all siblings to settle before throwing (the fake-timers assertion at 99ms/100ms is a nice touch), failed and canceled callback results, both abort paths, and the invariant-violation fallback.
One tiny observation, no action needed: there's a theoretical window where the signal aborts after every leaf has settled successfully but before the final aborted check, producing a FAILED_TO_EXECUTE error wrapping a fully successful result tree. The sibling executor has an equivalent window with its canceled flag, so behavior is consistent across the two — just noting it for completeness.
For subsequent reviewers: nothing outstanding from my side. The main thing left to verify is the kit-plugins#395 refactor consuming this cleanly, which the new aggregation contract should make straightforward.
|
Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up. |
This PR adds a new helper
createTransactionPlanExecutorWithConcurrentLeavesas a sibling tocreateTransactionPlanExecutor.This creates an intentionally much simpler executor, that ignores dependencies between transactions and concurrently executes the callback (slightly simplified)
SingleTransactionPlan -> Promise<SingleTransactionPlanResult<TContext>>for every leaf. It returns aTransactionPlanResultwith the same shape as the inputTransactionPlan. It is intended for cases where dependencies are irrelevant, such as signing the transactions without executing them.See also anza-xyz/kit-plugins#395 which first added a similar more specific version of this function. That PR will be refactored to use this function.
Failures are aggregated, similar to
createTransactionPlanExecutor: if a callback throws for any leaf then this is returned as aFailedSingleTransactionPlanResultat that leaf, independently of other callbacks. UnlikecreateTransactionPlanExecutor, this does not cancel dependent leafs, as they are treated independently. If there are any failures then we throwSOLANA_ERROR__INSTRUCTION_PLANS__FAILED_TO_EXECUTE_TRANSACTION_PLANwith the plan.