Repository navigation
refactor(sdk): create one plugin instance per invocation - #924
ParidelPooya wants to merge 46 commits into
Conversation
GitHub Actions
Waiting
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.
05b9d11 to
acad423
Compare
ParidelPooya
changed the title
A plugin instance used to live as long as the execution environment while serving every execution that landed on it. Under Lambda Managed Instances several executions run concurrently in one environment, so every plugin had to key its own state by execution ARN and clean that map up itself. Getting that wrong loses telemetry silently, and both bundled plugins had got it wrong. The SDK now builds one plugin per invocation and drops it when the invocation ends. `plugins` takes factories, not instances: plugins: [(info) => new MyPlugin(sharedExporter, info.executionArn)] A factory is a bare function taking `InvocationInfo` and returning a plugin. Errors it throws are contained exactly like errors thrown by a hook: the invocation proceeds without that plugin. Environment-lifetime state stays in the closure or in a factory object; per-invocation state becomes ordinary instance fields. This deletes the provider path rather than adding a parallel one. While an instance path exists a plugin cannot delete its ARN-keyed map, which is the entire point of the change. Removed: `DurableInstrumentationPluginProvider`, `DurableInstrumentationPluginType`, and `DURABLE_INSTRUMENTATION_PLUGIN_API_VERSION`. Added: `DurableInstrumentationPluginFactory`. `durableExecutionPluginProvider` is now a factory, so environment-based loading works unchanged. Workflow Insight now holds no ARN-keyed structure at all. It also drops `wrapInvocation` and drains from `onInvocationEnd`, which the SDK awaits at every exit including the config-error path that `wrapInvocation` missed. The OTel plugin resolves the tracer provider once per environment and keeps each invocation's spans in plain fields, so `resetInvocationState` is gone. BREAKING CHANGE: `plugins` accepts factories instead of plugin instances, and the provider interface is removed. Pass `(info) => new MyPlugin()` where you passed `new MyPlugin()`, and export a factory where you exported a provider. Not migrating no longer degrades quietly: a plugin instance, or the plugin class itself, in `plugins` now fails every invocation with `PluginLoadError` at handler initialization, rather than leaving the plugin silently absent.
…p failure Review findings on the per-invocation plugin contract. Both blocking items share a shape: a failure that used to be loud became silent. A JavaScript caller who does not migrate passes a plugin instance in `plugins`. The SDK called it as a factory, the call threw `TypeError`, and the containment in `createInvocationPluginRunner` discarded the entry and wrote nothing, so the plugin was absent for the life of the environment with no signal at all. The loader already validated that an environment-selected provider is callable and raised `PluginLoadError`, so the two paths disagreed. `loadConfiguredPlugins` now runs the same check over the explicit entries, naming the offending position. A factory that throws when called stays contained, because that is the documented contract; an entry that is not callable at all is a packaging mistake that would otherwise be rediscovered and swallowed on every invocation. The OTel environment is built lazily inside the per-invocation factory, so a throwing `tracerProviderFactory` now throws inside an invocation instead of at Lambda init. Containment is right, but the attempt repeated: the customer's factory ran again on every invocation for the life of the environment, and a factory that builds a span processor before throwing leaked one per invocation. Creation is now attempted at most once, the failure is logged once, and later invocations are refused with the remembered error. A working factory is still resolved exactly once. Also from the review: `replayLinks` dropped a guard that became unreachable when `initialOperationLink` stopped returning `undefined`; `describeValue` reported an array as "an object" and would have said "a undefined"; and the `flush()` contract now documents that a slow `export` multiplies by environment concurrency, with every concurrent end paying the whole product, since the pump exports every awaited record before the shared flush turn.
A caller migrating from the earlier contract is most likely to pass the class,
because `plugins: [MyPlugin]` was once close enough to correct to look right.
`typeof MyPlugin === "function"`, so the callable check added in the previous
commit does not see it. Calling it throws "Class constructor cannot be invoked
without 'new'" on every invocation, and that throw is contained the way any
plugin failure is contained, so the plugin is absent for the life of the
execution environment with nothing said about why.
I had claimed this needed a per-invocation check to detect. That was wrong: the
source text of a class begins with the `class` keyword and nothing else callable
does, so `Function.prototype.toString.call(entry)` settles it at load time. Both
plugin paths now reject it there, and the message says what to pass instead.
The keyword must be followed by whitespace, `{`, or a comment start. Without
that, a shorthand method named `class` stringifies as `class(info) {}` and one
named `classify` as `classify(info) {}`, and both would be mistaken for a class.
`toString` is called through `Function.prototype` rather than through the value,
because a function can carry its own `toString`.
Measured while writing this: a class declaration, a named or anonymous class
expression, `class{}` with no space, a subclass, and a class with a TS 5
decorator all keep the keyword, as does esbuild minifying to ES2022. Downlevelling
to ES5 does not — TypeScript and esbuild both emit `function PlainPlugin() {}` —
so a class transpiled that far is still missed. Such a function called without
`new` returns `undefined` rather than throwing, and the runner already skips a
factory that hands back nothing, so containment remains the backstop for exactly
that case. Nothing further is attempted: the heuristics that would catch it,
inspecting `prototype` descriptors or method enumerability, also reject ordinary
factory functions, and a false rejection at load time would fail an invocation
that would otherwise have worked.
e040d90 to
d61acce
Compare
ParidelPooya
changed the title
GitHub Actions
Waiting
Two findings the Python reviewers raised, checked here and both real. The first is the version set. Both plugin packages declared a peer range that admits a core without the factory contract: otel `>=2.4.0` and insight `>=2.1.0`, against a core at `2.4.0`. This branch deletes the provider contract, so npm accepts otel with core `2.4.0` and the handler then fails at initialization. `RELEASING.md:33` puts version numbers in the pull request rather than in the release workflow, so this commit sets them: core to `3.0.0` because `plugins` changed element type from instances to factories, otel to `2.0.0` because its plugin classes are no longer exported and factory builders replace them, and both peer ranges to `>=3.0.0`. Insight goes to `0.1.0-alpha.1` rather than `0.2.0-alpha.0`. Only `0.1.0-alpha.0` has ever been published, so a minor bump would claim a released `0.1.0` that does not exist. The prerelease counter is the smallest step that is greater than the published version, which the publish script requires. The testing package moves for a consequential reason rather than a chosen one. Its range was `>=1.0.1 <=2.4.0`, whose upper bound excludes the core built here, and its own test asserts the core satisfies that range. It references the plugin contract nowhere, so core `3.0.0` is compatible with it and widening the bound is correct. Its version follows, because the publish script skips a package whose version is unchanged and the widened range would never reach a consumer. The existing peer check covered the testing package only. It now scans every workspace manifest, and for a package that consumes the factory contract it requires the declared lower bound to name at least the core major built here. Which packages consume the contract is derived from their sources rather than from a list, because a list is what gets left behind. The second finding is the reentrancy defect the Python plugin had, and my earlier reading that this plugin was safe was wrong. I judged that `onOperationChange` builds and schedules in one expression with no `await` between the `closed` check and the `schedule` call, so nothing could interleave. Argument expressions evaluate before the call they feed, so `buildRecord` completes before `schedule` receives its result, and `buildRecord` calls customer code: the input and output content transforms, a per-operation result override, and any accessor on a customer-thrown error. A synchronous re-entry from that code interleaves without an `await`. Measured, with a content transform re-entering `onOperationChange` while the outer frame held the older snapshot: the exporter received `[['A','B'], ['A']]`, so an exporter that upserts by execution ARN stores the older snapshot. Measured with the same re-entry into `onInvocationEnd`: the exporter received a terminal record and then a stale RUNNING record for a finished execution, which is exactly what the `closed` flag exists to prevent and cannot, because the outer hook passed its check before the nested hook set it. Each invocation now carries a build revision. Building takes the revision, and the record is scheduled only while that revision is still current, so a superseded snapshot is dropped instead of overwriting a newer one. A record is a complete snapshot, so dropping a superseded one loses nothing. That is the same property that makes the scheduler's per-execution coalescing sound. One limit on the second finding, stated because it bounds the severity rather than the correctness. Reaching the plugin instance from a transform requires the customer to capture it from their own wrapping factory, which the new contract permits but does not require. So the reordering is measured and real, and its reachability from an unmodified handler is not established.
A reviewer asked for the registration type to be an object with a method rather than a bare callable, and the reason is future extensibility rather than taste. A function type has no member to add anything to. So adding a process-level hook later -- a flush when the execution environment shuts down, for example -- would have to change the registration type from function to object, which is a second breaking change on the same public surface. An object with one method can gain an optional second member additively, so doing this now costs one break instead of two. `DurableInstrumentationPluginFactory` is therefore an interface declaring `createPlugin(info)`, not `(info: InvocationInfo) => Plugin`. The type's name and the returned type's name are both unchanged, so no identifier changes meaning. The method is `createPlugin` rather than `newInvocation` because what it returns is still called a plugin, so the verb and the noun agree. This is the shape the Java SDK already had, as `DurableExecutionPluginFactory.createPlugin(InvocationInfo)`, so the three SDKs now describe one contract. The generic parameter is kept because it is used: the OTel builders return `DurableInstrumentationPluginFactory<InvocationOtelPlugin>` and `<ExecutionOtelPlugin>`, and without it a caller could no longer see which plugin type they get back. An interface method position is covariant in its return type the same way the function type was, so nothing weakened. The loader's shape test replaces `typeof value === "function"` with a property lookup for a callable `createPlugin`. It is a lookup rather than an own-property check, so a factory written as a class instance, which carries the method on its prototype, is accepted -- the runtime check has to accept exactly what the type accepts. A function carrying a `createPlugin` property is accepted for the same reason. The value is never called. One check became redundant and is deleted. An earlier commit in this branch added a load-time heuristic that rejected a class passed where a factory belongs, reading `Function.prototype.toString` for the `class` keyword, because `typeof MyPlugin === "function"` meant a class passed the callable test. A class carries no `createPlugin`, so the shape test rejects it directly. Deleting the heuristic also retires its documented limitation: a class transpiled to an ES5 `function` did not stringify as `class` and so escaped it, and that case is now caught like any other. Its tests move to the new rejection path rather than being removed, minus three that only pinned the regex. Migrated with it: both bundled builders, which now return the object; the three dynamic-plugin-layer provider fixtures, which exported a bare function and were genuinely broken rather than merely imprecise; the cjs and esm integration tests that consume them; 18 conformance plugin handlers, 19 factories; and the prose in two READMEs and four insight design documents. No handler's observable behaviour changed, and that was established from the diff rather than assumed. `git diff -w` over the conformance package shows only wrapper lines added or removed, so every factory body is byte-identical apart from indentation. No added line contains a backtick, so no template literal was re-indented. No `plugins:` array was edited, so registration order is preserved in both handlers that register two. The conformance and integration packages resolve `@aws/*` through a worktree-root symlink into a different clone, which is why an earlier pass could not verify them. Package-local symlinks were added under `node_modules` so they compile against this worktree; `node_modules` is gitignored, so none of that is committed.
onInvocationStart set tracingEnabled from ensureTracingEnabled at the top, then called the customer-supplied contextExtractor, and only assigned executionTraceId afterwards. An extractor that throws therefore left the instance enabled with no trace identity: the plugin runner contains the rejected hook, the invocation continues, and every later hook passes its tracingEnabled check. A cross-invocation replay or continuation span then linked against an empty trace ID. This PR removed the guard that used to catch that, on the reasoning that the link is always constructible. Its span ID is; its trace ID is not. The flag is now set last, so it means what every later hook reads it to mean: this invocation's trace identity is resolved.
The pump claims the pending flush resolvers with splice(0) before it front-loads the records other ends are waiting on. From that point they are not in flushWaiters, so nothing else can find them and no later pump serves them. The front-load await sat outside the try that releases them, and exportPending is a try/finally with no catch, so a synchronous throw from its fan-out escaped and stranded every waiter. Because onInvocationEnd is awaited before the Lambda response, those invocations hung to the function timeout. The claim and the release are now the same critical section. A flush that did not run is the same answer to a waiter as a flush that failed, which is what the per-exporter containment in flushAll already establishes. A comment claiming that exporters.map runs inside the pump's guarded region is corrected: it runs inside a finally, not a catch.
Both plugin packages declared >=3.0.0 on the core with no ceiling. That is the shape of the defect this PR already fixed once: otel declared >=2.4.0 against a core whose 2.4.0 could not load the factory contract, npm accepted it, and the handler failed at initialization. The next core major that changes the plugin contract reproduces it exactly. Both are now >=3.0.0 <4.0.0, and the monorepo guard asserts a contract-bound range rejects the next major rather than only accepting the current one. The testing package had the mirror problem: >=1.0.1 <=3.0.0 excludes core 3.0.1, so the first core patch would have put its claim out of date and its own guard red, for a package that touches the plugin contract nowhere. It is now >=1.0.1 <4.0.0, and a new case asserts every declared range admits the next core patch.
The lint job checks formatting; the local check I ran filtered to error-level diagnostics, which excludes it.
Retain core SDK 3.0.0 and the plugin-compatible peer ranges while incorporating main's SDK 2.5.0 and testing SDK 1.2.0 changes. Advance the testing package to 1.2.1 and synchronize lockfile peer metadata. Update the plugin-lifetime checkpoint mock for the new token-revocation and execution-result methods introduced on main. Validation: full build, repository lint checks, source and conformance type checks, core SDK (1346), testing SDK (1227; 3 skipped), OTel (360), Insight (108), OTel conformance handlers (35), and CJS/ESM integration.
Keep the core SDK at 3.0.0 and preserve the plugin-compatible peer ranges. Advance the testing SDK to 1.3.1 after main moved to 1.3.0, with the matching lockfile version.
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| this.invocationEnded = true; | ||
| await this.flushExternalCompletions(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Confirmed the first-flush window with a blocked provider flush and a completion 25 ms later: the execution view on terminal completion and the invocation view on both pending and terminal completion ended their enclosing spans too early.
014319d keeps those spans open through an initial serialized drain/flush, drains updates received during that flush and its microtask handoff, then ends the parents and performs the final serialized flush in both views. The three regressions now pass; all 60 focused controls and all 591 OTel tests pass, along with build/type generation and lint. Existing flush-error isolation, timestamps, IDs, sampling and deduplication are preserved.
This is a bounded improvement, not a claim that every late completion is contained. A new update arriving during the final flush or after onInvocationEnd still cannot extend an immutable enclosing span; three negative diagnostics for that later window remain reproducible. The README explicitly documents that limitation and the cost of two flush passes even without updates. This change does not introduce parentless/multiple roots or a different trace topology, and does not promise delivery after Lambda stops executing.
This comment has been minimized.
This comment has been minimized.
GitHub Actions
Failure
| this.invocationEnded = true; | ||
| await this.flushExternalCompletions(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
This is the final-flush/post-end window explicitly retained in #924 (comment) and the README. The blocked-final-flush diagnostic still reproduces three containment failures; it is not marked fixed. The first-flush improvement has 60 focused and 591 full OTel tests passing, while preserving IDs, timestamps, sampling and the current trace topology.
A final provider flush alone does not prevent a new checkpoint hook from arriving after parent spans have become immutable. Dropping those observed late completions would undo the delivery repair. This PR keeps the bounded preflush improvement and states the remaining lifecycle limitation rather than claiming complete quiescence or introducing a different root topology.
This comment has been minimized.
This comment has been minimized.
GitHub Actions
Error
| // 4. Always flush TracerProvider at invocation boundaries | ||
| if ("forceFlush" in this.tracerProvider) { | ||
| this.invocationEnded = true; | ||
| await this.flushExternalCompletions(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
This is the same documented final-flush/post-end limitation addressed in #924 (comment) . I verified that the subsequent Insight and README commits do not change either OTel implementation. The three blocked-final-flush negative cases remain reproducible and are not marked fixed.
The first-flush improvement and delivery of observed late completions are retained; replacing the final drain with only a provider flush cannot prevent a new checkpoint hook after immutable parent ends. The README explicitly limits containment in that window. This change does not drop those updates or introduce a different trace/root topology.
This comment has been minimized.
This comment has been minimized.
| // 4. Always flush TracerProvider at invocation boundaries | ||
| if ("forceFlush" in this.tracerProvider) { | ||
| this.invocationEnded = true; | ||
| await this.flushExternalCompletions(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
This is the same acknowledged final-flush/post-end boundary documented in the README and explained at #924 (comment) . I checked 3b91b1f: both production plugin files are byte-identical to the previously validated implementation; the base merge adds only the four basic clock controls. The blocked-final-flush diagnostic cases were already reproduced and deliberately retained as negative evidence, rather than a claim that late containment is fixed.
The bounded preflush drains and flushes completions before enclosing spans freeze. Updates arriving during the final flush or after the end hook remain outside that containment guarantee. Simply stopping the drain would drop those completion exports; it would not establish quiescence against later updates. The current design keeps the documented limitation and does not claim delivery after the Lambda runtime stops or introduce a different root topology.
This comment has been minimized.
This comment has been minimized.
| // 4. Always flush TracerProvider at invocation boundaries | ||
| if ("forceFlush" in this.tracerProvider) { | ||
| this.invocationEnded = true; | ||
| await this.flushExternalCompletions(); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_ksc4n5ohubqpxar25g6upvizoe
P2 — Do not drain completions after closing their parent spans. The Invocation span and, for terminal executions, Workflow span have already ended here. If onOperationChange queues a completion while this final flush is blocked, drainAndFlush exports it afterward, potentially giving the child a live end time outside its parent interval. Coordinate shutdown so completion delivery is quiescent before ending enclosing spans, then call only flushTracerProvider() afterward; defer unsynchronized completions to redelivery. Apply the same ordering to InvocationOtelPlugin and add a test that blocks the final/second forceFlush().
There was a problem hiding this comment.
This is the known final-flush/post-end containment limitation documented under “External completions and replay” and discussed in #924 (comment) . I checked c462e85: the base integration preserves the existing completion drain, change-hook handling, and final serialized flush. It adds the managed-sampler root behavior without changing that shutdown boundary.
The first drain/flush keeps enclosing spans open for completions delivered during that pass. It does not establish quiescence for an independent notification arriving during the final flush or after the end hook; immutable parent spans cannot then be extended. The blocked-final-flush cases remain negative diagnostic evidence, not a claim that this window is fixed.
Stopping the final drain and relying on redelivery would discard a received completion without a guaranteed later delivery or persisted export acknowledgement. This PR therefore retains the bounded preflush behavior and the explicit late-containment limitation, preserving timestamps, IDs, sampling, and deduplication. It does not claim delivery after the Lambda runtime stops or introduce another root topology to satisfy containment.
Codex AI reviewFound one P2 telemetry lifecycle regression. Tests do not cover a completion arriving during the final provider flush. Reviewed commit |
This branch was successfully deployed
3 participants
Handler-scoped plugin instances can mix concurrent Lambda Managed Instances executions. SDK 3.x accepts factories with
createPlugin(info)and creates one plugin instance per invocation. Operation maps, spans, and buffers belong to that invocation; providers, exporters, and serialized export scheduling remain shared by the execution environment.This implements the selected major migration: explicit and environment providers declaring the legacy v1
pluginApiVersioncontract fail withPluginLoadErrorbefore their factory or handler runs. Error messages, examples, declarations, and release guidance document migration to a fresh-instance factory. Installed-package and separate-layer tests cover rejection of published v1 providers and successful core 3.0/OTel 2.0 loading. Valid unrelated factories remain supported. The public factory signature permitsnullorundefinedto skip an invocation, matching runtime behavior; bundled OTel and Insight factories retain concrete return types. Packed-package consumer tests verify both contracts without casts.The major tree preserves the coordinated minor fixes: successful replay export suppression (#953), wrapper
UpdatedOperationIds(#954), invocation-local LMI carriers (#955), and exclusive OTel views (#956). An available empty/undefined/null runtime carrier suppresses stale process environment data, while contexts without that capability retain legacy fallback. Optional carrier getters and proxy capability checks are read once only when a factory is configured; failures produce an authoritative empty carrier without aborting the handler. The factory and invocation hooks receive the same snapshot. Raw carrier helpers accept these states under exact optional-property checking. Factory exclusivity uses namespaced symbol metadata and rejects conflicts before constructing plugins.Both OTel views observe fresh external terminal updates at startup and checkpoint changes. Normal traversal exports pending completions under the active parent, and invocation-end cleanup flushes any still-unobserved completion before ending the invocation. Per-invocation dedup preserves normal replay behavior while allowing failed-invocation redelivery; no acknowledgement or unbounded execution cache is persisted. Error events preserve supplied backend completion timestamps, and missing timestamps use one captured terminal time for both event and span end.
Physical invocation boundaries use explicit
HrTimefrom bounded wall/monotonic correlation samples. Missing logical operation/attempt boundaries use coherent millisecond precision; wall ticks are accepted only inside the measured correlation interval, while supplied backendDateobjects stay authoritative. Active deferred ancestors include the actual earliest observed child boundary, with cycle protection and no revival of ended parents. Invocation ends cover observed SDK boundaries without fixed padding or cross-invocation caches. The README documents arbitrary cross-container clock-skew limits. Shared validators and strict SDK relationships are unchanged.Conformance #960's common helper and handlers 21–24 use the factory API, with both shared workflow pins at
98b802cdb172614f98e217f7464784d47b9bb484. Polling replay links follow #964: attempts link to Workflow; resumed operations link to their initial operation and Workflow. Current main333ddd606301b5820e7488f015ca745b4a9d8412, including the MicroVM examples and integration wiring, is integrated. Insight keeps late nonterminal updates, drains changes arriving during export, and rejects late running snapshots after terminal state.Validation:
Core 3.0.0, OTel 2.0.0, and Insight 0.1.0-alpha.1 use the matching major contract. Testing 1.3.2 supports core
>=1.0.1 <4.0.0. Follow minor-before-major release order. Fresh CI and review must pass on the final published head before merge. No merge or release is performed; model-blocked propagation draft #957 is not included.Refs #950. Integration dependencies: #953, #954, #955, #956, #959, #960, #962, #964.