Repository navigation
fix(insight): key export scheduling by execution ARN - #923
Closed
ParidelPooya wants to merge 1 commit into
Closed
ParidelPooya wants to merge 1 commit into
ParidelPooya wants to merge 1 commit into
Conversation
The SDK creates one plugin instance per handler-module initialization, so a single Workflow Insight plugin serves every execution its environment hosts. Lambda Managed Instances makes concurrent executions in one environment routine. The export scheduler held one pending record for the whole plugin and overwrote it regardless of which execution the record belonged to. Measured on this package before the change, 20 trials per case: with N concurrent executions in on-complete mode exactly 2 terminal records were delivered and N-2 were lost (8 of 10 lost at N=10); in on-change mode 2 concurrent executions lost a terminal record in 20 of 20 trials. drain() returned successfully after exporting some other execution's record, so the loss was silent. Each execution now owns its pending record, its queue position and its drain signal, so coalescing happens only within one execution and drain(executionArn) returns only once that execution's own latest record has reached every exporter. Exporter calls stay serialized per plugin instance: at most one export at a time, and a flush is served by the same pump, so an exporter never sees flush() overlap export(). Also fixed, each found while reviewing the change above: - A synchronous throw from export(), render() or truncateRecord() escaped the fan-out before Promise.allSettled, hung the invocation to its timeout, left the scheduler unable to start another pump for any later execution, and produced an unhandled rejection. - The pump re-armed itself with a synchronous self-call, so a queue of failing fan-outs grew the stack one frame pair per execution, overflowed, and silently abandoned the tail. - A throw between closing an execution's scope and removing it left a closed scope in the map, muting that execution for the life of the environment. - Per-execution state survived a non-terminal (PENDING) invocation end, pinning the whole cached execution input until the environment died. State is now dropped at every invocation end and rebuilt from InvocationStartInfo. - An operation-change hook arriving after onInvocationEnd emitted a RUNNING record after the terminal one, reverting a finished execution for every exporter that upserts by execution ARN. - startTime trusted an un-normalized SDK value and fell back to now, so a resumed execution's durationMs covered only its last invocation. An Invalid Date now degrades one field instead of throwing out of record building. - InsightExporter.flush() had no documented contract. It now states the cadence, the exclusivity guarantee, that a flush may cover other executions' records, and how failures are handled. No public type or exported symbol changed. Record shape, emit modes, sampling, per-exporter truncation and the default exporter are unchanged.
ParidelPooya
had a problem deploying
to
ai-pr-review-runtime
September 17, 2026 00:14 — with
GitHub Actions
Failure
1 of 3 tasks
Contributor
Author
|
Superseded by #924, which carries this fix as its first commit plus the plugin-contract change that removes the reason the bug was possible. Closing in favour of one PR per language. |
This branch had an error being deployed
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
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.
Problem
The SDK creates one plugin instance per handler-module initialization (
with-durable-execution.ts,loadConfiguredPlugins), so a single Workflow Insight plugin serves every execution its environment hosts. Lambda Managed Instances makes concurrent executions in one environment routine.ExportSchedulerheld onependingrecord for the whole plugin and overwrote it regardless of which execution the record belonged to. Each record is a complete snapshot of one execution, so a newer record for the same execution supersedes the older one safely — but a record for a different execution supersedes nothing.Measured on this package before the change, 20 trials per row:
With N concurrent executions in on-complete mode exactly 2 terminal records were delivered and N-2 were lost.
drain()returned successfully after exporting some other execution's record, so the loss was silent.What changed
Each execution now owns its pending record, its queue position and its drain signal. Coalescing happens only within one execution, and
drain(executionArn)returns only once that execution's own latest record has reached every exporter. Exporter calls stay serialized per plugin instance — at most oneexport()at a time — and a flush is served by the same pump, so an exporter never seesflush()overlapexport().Six further defects, all found while reviewing that change:
export(),render()ortruncateRecord()escaped the fan-out beforePromise.allSettled, hung the invocation to its timeout, left the scheduler unable to start another pump for any later execution in that environment, and produced an unhandled rejection.InvocationStartInfo.onInvocationEndemitted a RUNNING record after the terminal one, reverting a finished execution for every exporter that upserts by execution ARN.startTimetrusted an un-normalized SDK value and fell back tonow, so a resumed execution'sdurationMscovered only its last invocation. An Invalid Date now degrades one field rather than throwing out of record building.InsightExporter.flush()also had no documented contract; it now states the cadence, the exclusivity guarantee, that a flush may cover other executions' records, and how failures are handled. The same wording is going into the Python and Java exporter interfaces.Verification
renderboth leave the invocation and the scheduler healthy; a 12,000-deep queue sweeps at constant stack depth; a post-terminal change hook emits nothing; a suspend releases the cached input;flush()never overlapsexport().npm run buildclean, suite run 10 times with no flaky runs.Known follow-ups, not in this PR
flush()still stalls every invocation waiting on that flush, and now also delays later exports. The contract requiresflush()to return promptly; bounding it would introduce partial-flush semantics.endTimeanddurationMson a record whose status is RUNNING, and Python omits both. Cross-language alignment is a separate change.