Skip to content

fix(insight): key export scheduling by execution ARN - #923

Closed
ParidelPooya wants to merge 1 commit into
mainfrom
fix/insight-per-execution-export
Closed

ParidelPooya wants to merge 1 commit into
mainfrom
fix/insight-per-execution-export

Conversation

@ParidelPooya

Copy link
Copy Markdown
Contributor

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.

ExportScheduler held one pending record 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:

concurrent executions emit mode terminal records lost per trial
2 on-complete 0 of 2
3 on-complete 1 of 3, in 20 of 20 trials
5 on-complete 3 of 5
10 on-complete 8 of 10
2 on-change 1 of 2, in 20 of 20 trials
5 on-change 4 of 5

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 one export() at a time — and a flush is served by the same pump, so an exporter never sees flush() overlap export().

Six further defects, all found while reviewing that change:

  1. 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 in that environment, and produced an unhandled rejection.
  2. 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: 12,000 queued executions overflowed at ~2,740 levels and silently abandoned the rest.
  3. 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.
  4. Per-execution state survived a non-terminal (PENDING) invocation end, pinning the whole cached execution input until the environment died — 500 of 500 inputs still reachable, versus 1 of 500 after a terminal end. State is now dropped at every invocation end and rebuilt from the next InvocationStartInfo.
  5. 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.
  6. 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 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

  • Package suite green, including new permanent tests for every item above: 10 concurrent executions each deliver their terminal record exactly once in both emit modes; a slow exporter loses nothing; a synchronously-throwing exporter and a throwing render both 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 overlaps export().
  • Terminal-record loss after the change: 0 of 40, 0 of 60, 0 of 100 and 0 of 200 at 2, 3, 5 and 10 concurrent executions, in both emit modes, 20 trials each.
  • Latency for 6 simultaneous invocation ends with a 10 ms export and a 30 ms flush: 253–265 ms and 6 flushes before the flush ordering work, 95–103 ms and 1 flush after.
  • Biome clean, npm run build clean, suite run 10 times with no flaky runs.

Known follow-ups, not in this PR

  • A hung flush() still stalls every invocation waiting on that flush, and now also delays later exports. The contract requires flush() to return promptly; bounding it would introduce partial-flush semantics.
  • On a non-terminal invocation end in on-change mode this plugin stamps the execution's endTime and durationMs on a record whose status is RUNNING, and Python omits both. Cross-language alignment is a separate change.
  • Five individual-fix pinning tests are still to be added; each fix is currently covered, but reverting two of them in isolation is caught only by throwaway probes.

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

Copy link
Copy Markdown
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

1 failed deployment
ai-pr-review-runtime — b692ce61 Deployed Sep 17, 2026 by ParidelPooya via notify / Generate notification summary #202
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.

1 participant