Repository navigation
refactor(plugin)!: create one plugin instance per invocation - #721
Merged
Merged
Conversation
One plugin instance serves every execution its environment hosts, and 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. Measured before the change, 20 trials per case: 2 concurrent executions lost a terminal record in 20 of 20 trials, 10 concurrent lost 5 to 8 per trial, and with 5 concurrent executions and a 200 ms exporter 2 of 5 records were exported while drain() returned successfully anyway. Pending records are now keyed by execution ARN in insertion order, each execution has its own completion signal, and drain(executionArn) waits for that signal rather than for the whole queue. Coalescing happens only within one execution. One pump is unchanged, so no exporter sees two records exported at once. flush() is now served by that same pump, between records, so an exporter never sees flush() overlap export() — including an export belonging to a different execution in the same environment. Requests queued together share one flush, and the pump exports the records an invocation is waiting on before spending a flush fan-out, which keeps a burst of simultaneous invocation ends from paying N times for one flush (measured: 8 ends with a 60 ms flush, 514 ms and 8 flushes before, 60 to 138 ms and 1 to 2 flushes after). Also fixed, each found while reviewing the change above: - Orphan detection inferred "nothing outstanding" from the pending map alone, which is also true of a record already taken and being exported, so an exiting pump could complete another execution's drain signal mid-export and that invocation returned before its record was delivered. - drainAll() gave up as soon as an already-seen execution queued new work, silently weakening every assertion made after it returned. - The drain failure path left a record queued for an unrelated execution's pump to export later, out of order and after the invocation had returned. - flush() and drain() called from the pump thread parked forever, because the only thread able to serve the wait was the one waiting. Both now report an IllegalStateException through the failure handler and return. - A null execution ARN threw NullPointerException out of onInvocationEnd into the SDK, which the hook's own javadoc forbids. - 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 changed: ExportScheduler stays package-private and ExecutionState stays a private nested class. Record fields, emit modes, sampling, truncation, per-record exporter fan-out and the default exporter are unchanged.
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. `withPlugins` takes factories, not instances: .withPlugins(info -> new MyPlugin(sharedExporter, info.durableExecutionArn())) `DurableExecutionPluginFactory` is a functional interface, so a lambda or a method reference works directly. Environment-lifetime state stays in the enclosing object; per-invocation state becomes plain 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. `DurableExecutionPluginProvider` now extends the factory and keeps only `getName()`; `getApiVersion()`, `getPluginType()`, and the no-arg `createPlugin()` are removed. A provider is therefore itself the per-invocation factory, so `ServiceLoader` discovery works unchanged. Workflow Insight now holds no ARN-keyed structure at all: `WorkflowInsight` becomes a thin factory over the new `InsightPlugin` and `InsightSettings`, and `ExecutionState` is deleted. The OTel plugin resolves its tracer provider once per environment in the new `OtelPluginEnvironment` and makes every per-invocation field `final`, assigned in the constructor. Constructor assignment is what makes dropping `volatile` safe: `PluginRunner` publishes the plugin list with a volatile write before firing `onInvocationStart`, so a field written in that hook is not guaranteed visible to a checkpoint or operation thread that already existed, whereas a field written in the constructor is. BREAKING CHANGE: `withPlugins` accepts `DurableExecutionPluginFactory` instead of `DurableExecutionPlugin`, and the provider interface loses `getApiVersion()`, `getPluginType()`, and the no-arg `createPlugin()`. Pass `info -> new MyPlugin()` where you passed `new MyPlugin()`, and implement `createPlugin(InvocationInfo)` in a provider.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
ParidelPooya
changed the base branch from
fix/insight-per-execution-export
to
main
September 17, 2026 17:14
2 tasks done
Review findings, both blocking, plus one test-determinism fix. With two or more exporters configured, `forEachExporterSettled` submits one task per exporter and the pump blocks waiting for them, so an exporter callback runs on a thread the pump cannot outlive. The re-entry guard only recognized the pump thread itself. An exporter that called `flush()` or `drain()` from a fan-out worker therefore waited for the pump while the pump waited for it, and a two-exporter test deadlocked both calls past a five-second deadline. The one-exporter regression passed because with a single exporter the callback runs directly on the pump thread, which the guard did recognize. Fan-out workers are now marked through a per-scheduler `ThreadLocal` and refused the same way the pump thread is: reported as `IllegalStateException` through the failure handler, returning without touching state, so the record stays in the invocation's slot and the waiting pump exports it as soon as the fan-out settles. The mark is per scheduler rather than static so one scheduler's worker is not refused by another, and the previous mark is restored rather than cleared so the rejected-execution fallback, where the fan-out thread is the pump, cannot clear a mark an enclosing frame needs. Second, `PluginRunner` caught `Exception` but not `LinkageError`. This matters because of the migration this change forces: a provider compiled against the previous interface throws `AbstractMethodError`, and one with a missing optional dependency throws `NoClassDefFoundError`. Both are `Error`, so both escaped invocation startup even though the factory contract promises a factory failure cannot disrupt the execution — end to end, a factory throwing `NoClassDefFoundError` failed the whole execution. Both `createPlugins` and the hook dispatch path now catch `Exception | LinkageError`. The catch stays narrow deliberately: `OutOfMemoryError` and `StackOverflowError` still propagate, and tests assert that. `releasePlugins` needs no equivalent change, because it drops references without calling into plugin code. `DynamicPluginLoader.getProviderName` still lets a `LinkageError` escape without the wrapping message, which is a follow-up rather than a defect: that path is fatal either way and only the message quality differs. Third, `StateCleanupLifecycleTest` polled weak references after `System.gc()`, which the JVM may ignore, so a correct implementation could fail it. It now asserts scheduler ownership and queue state directly, plus a reflective sweep proving no scheduler field reaches a plugin instance, and keeps the weak-reference count as a diagnostic that cannot fail. The sweep replaces what the weak-reference check was genuinely catching: an empty queue alone would not notice a registry reintroduced under a new field name. Verified by mutation — adding an uncleaned plugin registry to the scheduler fails all three assertions. Two suspend tests now emit on change, because under the default ON_COMPLETE mode a PENDING invocation queues nothing and the retention assertions passed trivially.
This comment has been minimized.
This comment has been minimized.
Review accepted shipping the factory-only plugin contract as a new major rather than keeping deprecated bridges, on the condition that the required changes are documented. This is that document. It covers the three paths a consumer can be on. Direct registration: `withPlugins(new MyPlugin())` becomes `withPlugins(info -> new MyPlugin())`, with worked examples for environment-lifetime state held outside the lambda and for per-invocation state that used to sit in an ARN-keyed map. Service providers: the before and after of a provider, now that the interface keeps only `getName()` and inherits `createPlugin(InvocationInfo)`. And `PluginRunner`, which was never customer API and should not be reached for at all. Two things the guide states that are easy to get wrong. A provider JAR that is not rebuilt still loads and is still selected, because its class file references nothing 3.x removed. Its inherited `createPlugin(InvocationInfo)` then throws `AbstractMethodError`, which the SDK now contains and logs, so the execution succeeds with no instrumentation at all. A deployment can therefore lose its telemetry while every invocation reports success, and the only signal is a per-invocation warning. That is the failure mode an operator most needs to know about, so it gets its own section along with three ways to confirm a provider actually loaded. A plugin instance does not survive suspension. A resume is a new invocation with a new instance, so state that must be stable across an execution has to be derived from `InvocationInfo` on each invocation rather than accumulated. Every "after" snippet was compiled against this branch rather than written from memory, which caught two errors in the drafts. Compiling the "before" snippets confirmed something worth stating in the guide: `DurableExecutionPlugin` declares only default methods, so it is not a functional interface and an instance cannot be silently coerced into a factory. Every direct registration site fails to compile until it is updated, rather than compiling and failing later. Linked from README.md and AGENTS.md alongside the 1.x to 2.x guide. Packaging and ordering details are cross-referenced to docs/advanced/configuration.md rather than duplicated.
The plugin contract says a factory or hook failure is logged, skipped, and never disrupts the execution. The catch was a list of types, so that promise held only while the list was complete. It was not complete. Four shapes reach the boundary and none is an Exception. A provider JAR compiled against an earlier interface throws AbstractMethodError. A provider whose optional dependency is absent throws NoClassDefFoundError. A plugin running with assertions enabled throws AssertionError. A plugin that loads its own exporter back ends through ServiceLoader throws ServiceConfigurationError. Only the first two are LinkageError, so the previous catch let the other two escape and fail an execution the plugin was only observing. Measured before the change: AssertionError and ServiceConfigurationError both escaped onInvocationStart, from the factory path and the hook path. The boundary now catches Throwable and rethrows only the fatal cases, so the promise stops depending on a list. Fatal means VirtualMachineError and nothing else. It is the JVM reporting it can no longer run correctly, which is not a plugin defect, and the process cannot be assumed able to continue. Naming the supertype covers OutOfMemoryError, StackOverflowError, InternalError and UnknownError, and keeps the rule stable if the JVM adds another. ThreadDeath is deliberately absent, on measurement rather than taste. The JVM delivers it only through Thread.stop(), which throws UnsupportedOperationException on JDK 20 and later, so it cannot arrive from the JVM on a current runtime. It is also deprecated for removal since JDK 20, so naming it would add a removal warning to every compile of this class and need a suppression that would then mask genuine removal warnings here. A ThreadDeath that plugin code throws itself is a plugin defect and is contained like any other. One deliberate behaviour change beyond containment. Throwing an InterruptedException clears the throwing thread's interrupt status, and the previous catch of Exception already contained it, so containment discarded the cancellation request silently. The threads that run factories and hooks are SDK threads that carry SDK work after the plugin returns. So the status is now restored before returning, immediately rather than after the dispatch loop, which keeps the flag true for every later read on that thread. A remaining plugin whose blocking call then fails fast is contained by this same rule, so every plugin is still called. One correction to an earlier claim in this branch. A previous round reported that DynamicPluginLoader.getProviderName lets a LinkageError escape without the wrapping message. That is wrong. getProviderName runs inside indexProviders' try block, whose LinkageError catch already wraps it, and with the version-compatibility message rather than the name-specific one. Widening getProviderName would replace the better diagnostic with the worse one, so it stays as it is. The real residual gap is narrower: only an AssertionError from getName() escapes bare, on a fail-fast startup path where startup fails either way and only the message differs.
This comment has been minimized.
This comment has been minimized.
ParidelPooya
added a commit
to aws/aws-durable-execution-sdk-python
that referenced
this pull request
Sep 18, 2026
Two reviewers found two orderings where the log filter attributed a record to the wrong execution. Both went through the same fallback, which resolved a record to the only open invocation when the emitting thread carried no live claim. The first ordering is a stale claim. Invocation A claims thread T. A ends, and unbind_invocation cannot reset T's ContextVar, because only the thread that set a ContextVar can reset it. B starts and is the only open invocation. A thread the customer started under A still carries A's claim, so the claim is stale, the fallback runs, and A's record is stamped with B's trace. Measured: the record carried B's trace and span exactly. The second ordering is a missing claim. A is open. B emits a log on its own wrapper thread before B reaches on_invocation_start, so B's thread has no claim and the registry holds only A. Measured: B's record carried A's trace and span exactly. Both outcomes are the one the design chose against, which is that an unattributed record is better than a record attributed to another customer's execution. So the fallback is deleted and a live claim is now required. One reviewer proposed keeping the fallback for threads that never held a claim, which fixes the first ordering and leaves the second. Requiring a claim fixes both. Deleting it is affordable because the threads that run customer code all carry a claim. The core SDK copies the invocation's context into the thread running the handler body, and the operation hooks claim the map and parallel branch threads before the branch body runs. Measured over one execution driving a step, a parallel, a map and a customer-started thread: every record from user code carried a claim. Two threads now go unstamped whatever the invocation count. The SDK's background checkpointing thread is submitted without the invocation's context deliberately. A thread customer code starts itself does not inherit the starting thread's context. The README states both rather than claiming every record is stamped. A second defect made the registry untrustworthy, so it is fixed here too. on_invocation_end ended the spans and exported the workflow span before releasing the invocation scope, with no finally. Both calls run customer tracer and span-processor code, which can raise. The plugin executor contains that exception, so the invocation survives, but the release was skipped: the plugin stayed registered as open for the life of the environment and its OTel context stayed attached. Measured with a span processor whose on_end raises: the plugin remained in the open registry for both plugins, and for ExecutionOtelPlugin a later record on the still-claimed thread was stamped with the finished invocation. The release and the flush now run in a finally, in both plugins. force_flush is contained there as well, because an exception escaping a finally would replace the exception that ended the invocation and hide its cause. Two documentation corrections from the same review. The factory docs presented a plugin class as the example factory, which encourages __init__(self, info) to do setup work, and CONTRIBUTING.md:246 asks for light constructors. A class is still a valid factory, because calling a class constructs an instance, so only the emphasis changes: the recommended forms are now a lambda or a classmethod, with class-as- factory noted as permitted. And the PluginHost docstring cited two cross-SDK symbols as if they were on the default branches. Checked against those repositories: the JS function there is createPluginRunner, and createInvocationPluginRunner exists only in aws/aws-durable- execution-sdk-js#924; the Java PluginRunner does exist on main, but takes plugin instances, and aws/aws-durable-execution-sdk-java#721 is what makes it per-invocation. The docstring now says that.
A reviewer found this defect in the Python plugin, and a probe has since confirmed it in the JS plugin. Both are fixed. This makes Java the third, and it is a latent defect here rather than a live one. Three properties produce it. Customer code runs while a record is being built, on the hook thread, before anything is scheduled: the input and output content transforms, an operation's result transform, and any Jackson serializer registered for a customer type. The scheduler's per-invocation slot takes whichever record is handed over last and compares no ages, so there is no sequence number, revision or timestamp anywhere in this module. And customer code can call back into a hook of the same instance, so a build can be overtaken by a newer build that starts and finishes inside it. The first and third properties hold here. The second does not, as shipped: the SDK serialises change hooks for one execution, because CheckpointManager.checkpointBatch wraps the plugin dispatch in synchronized (pollingFutures) and batches for one execution are chained sequentially through thenRunAsync. So the defect is not reachable today. Nothing pins that. Neither CheckpointManagerTest nor ApiRequestDelayedBatcherTest asserts that change hooks are serialised per execution, so the plugin is correct only because of an SDK property no test defends. A probe that forced the re-entry produced both Python outcomes: with the pump held, the newer two-operation snapshot was coalesced away and only the older one-operation snapshot was exported; with the pump running immediately, the exporter saw the newer snapshot and then the older one. Each invocation now counts the record builds it has started. A build takes the next value before it begins, and the scheduler queues the record only while that value is still the newest, so an overtaken build's record is dropped. Nothing is lost, because a record is a complete snapshot of one execution and the record that superseded it carries everything the older one carried. That is the same property that already makes the slot's coalescing sound. The counter is an AtomicLong rather than a volatile long. Incrementing a volatile long is a read-modify-write, so two concurrent builds could take the same value and each would conclude its own record is the newest. That is the case the check exists for, so a racy counter would guard nothing. The check itself is read inside the critical section that queues the record, which is the monitor `closed` is already read under, so the two checks and the hand-off are one section. The revision is taken before the build and never after. A value read after the build would already belong to the build that customer code started from inside the transform, so the older record would pass the check and overwrite the newer one. The final record takes no revision and is not checked. Customer code running inside its build can start a newer RUNNING build, which would make a revision taken there stale, and a checked hand-off would then drop the final record and leave a RUNNING snapshot as the execution's last exported state. Exempting it cannot let a stale record win, because closeAndSchedule sets `closed` in the same critical section that queues the final record and every RUNNING record handed over afterwards is rejected. So the two orderings are separate and neither subsumes the other. `closed` orders RUNNING records against the final record. The revision orders RUNNING records against each other. A boolean cannot say which of two RUNNING builds is newer, and the revision cannot reject a RUNNING record that follows the final one. Four tests. Two pin the outcomes, one per pump timing, and both fail when the revision term is removed. Two pin the failure mode a revision check can introduce, and both fail when the final record is checked as well.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Comment on lines
+376
to
+378
| } catch (Throwable failure) { | ||
| // Match CompletableFuture.handle: transfer failures unchanged for the invocation caller to rethrow. | ||
| result.completeExceptionally(failure); |
Contributor
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_ulzyrefhqtr3lzgodlol3ljz4j
[P1] Re-throw finalizer fatals outside CompletableFuture callbacks. On normal async executions this runs on a reusable handler or checkpoint worker. If result serialization or onInvocationEnd throws VirtualMachineError/ThreadDeath, only finalized fails; the worker returns normally and may be reused after a VM/thread-termination fatal. Run finalization on the invocation caller or through an observed executor runnable that rethrows fatal outside CompletableFuture containment, and add an async uncaught-worker regression test.
| Runnable task = () -> runSafely(() -> runAsExporterFanOut(() -> action.accept(exporter))); | ||
| try { | ||
| settled.add(CompletableFuture.runAsync(task, executor)); | ||
| settledExporters.add(CompletableFuture.runAsync(task, executor)); |
Contributor
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_u2i752a4wlmds2ulcs3kf477u7
[P1] Do not contain fatal exporter callbacks in CompletableFuture.runAsync. With multiple exporters, runSafely rethrows VirtualMachineError/ThreadDeath, but CompletableFuture catches it and only fails the task. The signal kills the pump while the actual fan-out worker survives and returns to the shared pool, defeating an injected ThreadDeath. Use an observed runnable that completes a separate future and then rethrows the fatal from the executor task itself; add multi-exporter uncaught-worker coverage.
Contributor
Codex AI reviewTwo P1 paths still contain fatal errors on reusable worker threads, defeating the PR’s fatal-propagation guarantees. Reviewed commit |
4 of 8 tasks
zhongkechen
added a commit
to aws/aws-durable-execution-sdk-python
that referenced
this pull request
Oct 22, 2026
* fix(insight): key export scheduling by execution ARN One plugin instance serves every execution its environment hosts, and 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 before the change, 20 trials per case: 2 concurrent executions lost a terminal record in 16 of 20 trials, 10 concurrent lost 8.5 per trial, and with 5 concurrent executions and a 200 ms exporter only 1 of 5 terminal records was exported while every drain() still returned without error. Pending records are now keyed by execution ARN with a per-execution lane, so coalescing happens only within one execution and drain(execution_arn) returns only once that execution's own record has reached every exporter and a flush covering it has completed. One worker thread and one export at a time are unchanged, so an exporter never sees concurrent calls. Also fixed, each found while reviewing the change above: - A BaseException from a customer exporter (asyncio.CancelledError inherits from it, so an exporter touching asyncio can raise it without writing raise) killed the worker between consuming a record and publishing its bookkeeping. The worker slot stayed occupied by a dying thread, no replacement started, and the parked drain hung the invocation thread permanently. - A drain could request a second flush while one was already in flight, so a flush ran after drain() had returned, calling an exporter after the invocation went back to Lambda. - The per-execution lock was not reentrant while a displaced record was released under it, so a record whose finalizer re-entered a hook for the same execution self-deadlocked the invocation thread. - The closed gate was a check-then-act: customer code running under the reentrant lock could complete on_invocation_end on the same thread, after which the outer frame still scheduled its RUNNING record behind the terminal one. - A flush happened only when a record was emitted, where JS and Java flush once per sampled-in invocation end. A buffering exporter now sees the same rhythm in all three languages. - InsightExporter.flush had no docstring. It now states the cadence, the exclusivity guarantee, that a flush may cover other executions' records, what happens when an exporter omits the method, and how failures are handled. No public API changed. Record fields, emit modes, sampling, truncation and the default exporter are unchanged; the emitted record surface is byte-identical to before the change. * refactor(plugin): create one plugin instance per invocation 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: @durable_execution(plugins=[lambda info: MyPlugin(shared_exporter)]) A factory is any callable taking `InvocationStartInfo` and returning a plugin. Environment-lifetime state stays in a callable class or a closure; per-invocation state becomes ordinary instance attributes. This deletes the provider path rather than adding a parallel one. While an instance path exists a plugin cannot delete its ARN-keyed dict, which is the entire point of the change. Removed: `DurableInstrumentationPluginProvider` and `DURABLE_INSTRUMENTATION_PLUGIN_API_VERSION`. Added: `DurableInstrumentationPluginFactory`. Entry points in the `aws_durable_execution.plugins` group now name a callable, so environment-based loading works unchanged; `load_configured_plugins` dedups by factory identity instead of by plugin type. Workflow Insight now holds no ARN-keyed structure at all. Its `RLock` and its pre-hand-off re-check both stay: hooks dispatch on the producing thread, so races within one invocation remain, and `schedule()` releases displaced records while holding the lock, which can re-enter through a customer finalizer. The OTel plugin keeps each invocation's spans in plain attributes, so the state-reset machinery is gone. `install_log_filter` now rebinds the installed filter to the current invocation's plugin. Without that, a logging handler outlives the plugin that installed it, keeps asking a discarded instance for a span context, and log correlation stops after the first invocation while the dead instance stays reachable for the life of the environment. BREAKING CHANGE: `plugins` accepts factories instead of plugin instances, and the provider class is removed. Where you passed `MyPlugin()`, pass a factory: `lambda info: MyPlugin()`. A plugin class is itself a valid factory when its constructor takes the info argument, because calling a class in Python constructs an instance. Entry points in the `aws_durable_execution.plugins` group must now name a callable rather than a provider object. * fix(plugin): give each invocation its own plugin session Review findings on the per-invocation plugin contract. The first one meant the contract change did not actually deliver what it promised under concurrency. `durable_execution` built one `PluginExecutor` at decoration time and every invocation shared it, while the executor stored that invocation's plugin instances, invocation metadata and operations provider on itself. So the SDK created one plugin per invocation and then parked it in a shared mutable slot. Measured with two invocations held inside their user function at once, invocation A's operation hook was delivered to B's plugin and A's plugin never received its own operation or end hook; in a second scenario A's scope exit cleared the shared state while B was live and B lost `on_invocation_end` entirely. A new handler-lifetime `PluginHost` now holds nothing but the resolved factory list and hands out a fresh `PluginExecutor` per invocation, which is the shape JS and Java already had. The executor is reachable only from the frames of the invocation that owns it, and a second `run()` on one executor raises, so reuse fails loudly instead of as silent crosstalk. Its thread pool is per invocation too: sharing one would let an ending invocation shut down a pool a concurrent invocation was still submitting to. The OTel log filter had the same class of bug. It held one mutable plugin reference, so with two invocations open the one that started last won and every other invocation's records carried its trace. Reading the active span from the OTel context, as the Java plugin does, is not sufficient here: the invocation span is never attached to the context, and the SDK runs the handler body on a pool thread that does not inherit the context of the thread the plugin bound on. The filter is now stateless and resolves per record, preferring the invocation that claimed the emitting thread through a `ContextVar` and falling back to the single open invocation when exactly one is open. With several open and an unclaimed thread it leaves the record unstamped, because an unattributed record is a smaller defect than one attributed to another customer's execution. Installation is serialized so concurrent first- time callers cannot stack filters. Third, the Insight scheduler used `0` for both "no flush in flight" and "a flush in flight covering zero exports", which are different facts. An invocation that emitted no record could not tell that its flush was already running, so it requested a second one that then ran after both invocations had returned — customer exporter code past the invocation boundary, which the flush contract forbids. Absence is now `None` and coverage an integer. * fix(otel): correlate top-level logs under concurrency A log record emitted at the top of a handler got no OTel correlation at all when two invocations were open, which contradicts the documented behaviour of the default filter. The chain: `on_invocation_start` runs on the Lambda invocation thread, so the plugin's `ContextVar` claim lands there, but the SDK then submits the handler body to a worker thread, and Python does not propagate a `ContextVar` into a thread started that way. No hook runs on that worker before the handler's first statement -- `on_user_function_start` is dispatched for durable operations only -- so the claim never reached it. The filter fell back to "the single open invocation", which is unavailable when two are open, and left both records unstamped. The SDK now carries the invocation thread's context into the worker that runs the handler body, with `contextvars.copy_context().run`. The copy is taken at submit time, after `on_invocation_start` has run on the same thread, so the claim is inside it. This is the correct layer: the SDK creates that thread to run user code, and any contextvar-based instrumentation is otherwise invisible there. It also matches `asyncio.to_thread`, which runs its callable in a copy of the caller's context. The background checkpointing thread is deliberately left starting from an empty context. It runs SDK checkpointing rather than user code, and it dispatches operation-end hooks, so copying the context there would make the invocation thread's ambient OTel context current on a thread that starts and ends spans. Sharing one copy between the two submissions is impossible anyway, because a `Context` cannot be entered twice concurrently. Propagation had one side effect worth closing rather than documenting. `get_current_span_context` prefers a same-trace span that is current over the plugin's own `Invocation` span, so once the ambient context reached the handler thread, a top-level record named the ambient span instead. Under X-Ray active tracing with the ADOT layer that ambient span is the parent of `Invocation`, so records moved one level up the tree. The plugin now records the span that enclosed the invocation and excludes that one span from the preference, which restores the previous behaviour. Anything that becomes current later is inside the invocation -- an attempt span, a child context span, or a span the handler starts itself -- and still wins, because it is more specific. One case remains uncorrelated by design: a record emitted on a thread that carries no claim. That covers a thread customer code starts itself and the SDK's checkpointing thread. The README states this rather than claiming every record is stamped. BREAKING CHANGE: the handler body now runs in a copy of the invocation thread's context rather than in an empty one, so two behaviours change for every customer, not only for plugin authors. A span started at the top of the handler body now has the ambient span as its parent instead of being a root span; under X-Ray active tracing with the ADOT layer that ambient span is the Lambda invocation span, so existing traces gain a level after upgrading. And a ContextVar set before the decorator is now visible inside the handler body. Both follow from the propagation, which matches what asyncio.to_thread does with the caller's context. * fix(insight): contain exporter BaseException; bump to 3.0.0 Four review findings. The first is the most serious defect in this branch so far, because it exhausts the environment rather than only stalling it. An exporter whose flush() raises a BaseException on every call used to hang the invocation without bound. A drain is released only by a flush that completed. The flush raised, so nothing advanced, and the worker died. A drain replaces a dead worker, so the drain started a replacement, which ran the same failing flush and died the same way. Measured over three seconds on one such exporter: 4842 flush attempts, 3682 live threads, and the drain never returned. So the earlier fix that let a replacement start converted a permanent park into a thread storm. The same finally block also advanced the completion bookkeeping when the first exporter aborted the fan-out with a BaseException. Measured with a failing exporter followed by a healthy one: the drain returned, the healthy exporter received nothing, and coverage read as delivered. The snapshot was already out of the pending slot, so the loss was permanent. Both exporter call sites now contain BaseException, so the containment that already held for Exception holds for every shape an exporter can raise. That is safe here specifically because none of the three signalling cases can reach these sites as a signal: the interpreter raises KeyboardInterrupt only in the main thread, threading discards a SystemExit raised in a worker thread, and nothing cancels the export worker because nothing outside the scheduler knows it exists. A BaseException seen there was raised by the exporter, so it reports a defective exporter rather than instructing this thread to stop. asyncio.CancelledError is the reachable case, since an exporter that touches asyncio raises it without writing `raise`. Consecutive worker deaths are now bounded at three, as a backstop for the scheduler's own code rather than for exporters. At the bound the scheduler latches asynchronous export off, wakes every parked waiter and drops what is queued, with a warning. Releasing the waiter is the priority because the waiter is an invocation thread inside on_invocation_end: parking it turns an instrumentation defect into a stalled customer execution, while dropping records loses instrumentation data only. Any completed export or flush resets the count, so a worker making progress never approaches it. Second, the decorated handler advertised a phantom argument. functools.wraps was applied to a three-argument internal function, so inspect.signature() on the handler reported a required plugin_executor parameter and signature.bind(event, context) raised TypeError. A signature-aware runtime or test harness can reject such a handler. The user function's metadata is now copied at the head of the wrapper chain, so the handler reports (event, context) again. Third, explicit plugins entries were never validated. The entry-point path rejects a non-callable target, but explicit entries were copied through, so plugins=[MyPlugin()] passed configuration and then raised TypeError inside _create_plugins on every invocation, which logged it and continued without that plugin. A breaking migration therefore produced repeated telemetry loss instead of one configuration failure. Explicit entries are now validated at load time and name the offending position. A plugin class is still accepted, because calling a class in Python constructs an instance, so plugins=[MyPlugin] is a valid factory. Fourth, both plugin packages allowed a core version that cannot work. They declared aws-durable-execution-sdk-python>=2.0.0 while requiring the factory contract, so pip accepted a resolution that fails at handler initialization. RELEASING.md makes version numbers this repository's responsibility, so this commit sets them: core 2.0.0 to 3.0.0 for the removed provider contract, otel 1.0.0 to 2.0.0 for the replaced plugin constructors, and insight 0.0.1 to 0.1.0 rather than 0.0.2 so a consumer pinning ~=0.0.1 cannot silently accept a breaking release. Both plugin bounds move to >=3.0.0, the OTel layer's sdk-version pin follows, and the two conformance packages' exact pins follow. Two new metadata tests assert that the declared bound tracks the core major this repository builds and that the layer pin matches the core version, so the next bump cannot leave one of them behind. * fix(otel): stamp a log record only from a live claim Two reviewers found two orderings where the log filter attributed a record to the wrong execution. Both went through the same fallback, which resolved a record to the only open invocation when the emitting thread carried no live claim. The first ordering is a stale claim. Invocation A claims thread T. A ends, and unbind_invocation cannot reset T's ContextVar, because only the thread that set a ContextVar can reset it. B starts and is the only open invocation. A thread the customer started under A still carries A's claim, so the claim is stale, the fallback runs, and A's record is stamped with B's trace. Measured: the record carried B's trace and span exactly. The second ordering is a missing claim. A is open. B emits a log on its own wrapper thread before B reaches on_invocation_start, so B's thread has no claim and the registry holds only A. Measured: B's record carried A's trace and span exactly. Both outcomes are the one the design chose against, which is that an unattributed record is better than a record attributed to another customer's execution. So the fallback is deleted and a live claim is now required. One reviewer proposed keeping the fallback for threads that never held a claim, which fixes the first ordering and leaves the second. Requiring a claim fixes both. Deleting it is affordable because the threads that run customer code all carry a claim. The core SDK copies the invocation's context into the thread running the handler body, and the operation hooks claim the map and parallel branch threads before the branch body runs. Measured over one execution driving a step, a parallel, a map and a customer-started thread: every record from user code carried a claim. Two threads now go unstamped whatever the invocation count. The SDK's background checkpointing thread is submitted without the invocation's context deliberately. A thread customer code starts itself does not inherit the starting thread's context. The README states both rather than claiming every record is stamped. A second defect made the registry untrustworthy, so it is fixed here too. on_invocation_end ended the spans and exported the workflow span before releasing the invocation scope, with no finally. Both calls run customer tracer and span-processor code, which can raise. The plugin executor contains that exception, so the invocation survives, but the release was skipped: the plugin stayed registered as open for the life of the environment and its OTel context stayed attached. Measured with a span processor whose on_end raises: the plugin remained in the open registry for both plugins, and for ExecutionOtelPlugin a later record on the still-claimed thread was stamped with the finished invocation. The release and the flush now run in a finally, in both plugins. force_flush is contained there as well, because an exception escaping a finally would replace the exception that ended the invocation and hide its cause. Two documentation corrections from the same review. The factory docs presented a plugin class as the example factory, which encourages __init__(self, info) to do setup work, and CONTRIBUTING.md:246 asks for light constructors. A class is still a valid factory, because calling a class constructs an instance, so only the emphasis changes: the recommended forms are now a lambda or a classmethod, with class-as- factory noted as permitted. And the PluginHost docstring cited two cross-SDK symbols as if they were on the default branches. Checked against those repositories: the JS function there is createPluginRunner, and createInvocationPluginRunner exists only in aws/aws-durable- execution-sdk-js#924; the Java PluginRunner does exist on main, but takes plugin instances, and aws/aws-durable-execution-sdk-java#721 is what makes it per-invocation. The docstring now says that. * refactor(plugin): make the factory an object with create_plugin 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 callable type alias 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 callable 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 a Protocol declaring `create_plugin(info)`, not `Callable[[InvocationStartInfo], ...]`. The type's name and the returned type's name are both unchanged, so no identifier changes meaning. The method is `create_plugin` rather than `new_invocation` 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 protocol is deliberately not `@runtime_checkable`, for three reasons. An `isinstance` check against a runtime-checkable protocol tests only that the member name exists, so it accepts an object whose `create_plugin` is a string. The SDK also has to name what an invalid entry actually was, which a boolean cannot supply. And such a check requires every declared member, which would make the optional second member above non-additive for any caller who wrote one. `_is_plugin_factory` tests for a callable `create_plugin` instead, structurally, so a factory need not import the protocol. `info` is positional-only on the protocol method. A protocol parameter that is positional-or-keyword is part of the structural contract, so a factory naming it `invocation` would otherwise be a type error. One consequence reverses guidance added earlier in this branch. A plugin class used to be a valid factory, because calling a class constructs an instance, and the docs said so. A class carries no `create_plugin`, so `plugins=[MyPlugin]` now fails at handler initialization, and the test that pinned the old behaviour is replaced by one pinning the new. A class that declares `create_plugin` itself, as a classmethod, is still valid, because the requirement is the member and not the kind of object. The reversal is an improvement for a reason the earlier round raised separately: `CONTRIBUTING.md:246` asks for light constructors, and class-as-factory encouraged `__init__(self, info)` to do setup work. A factory object puts that work in the factory's own constructor, where it can fail before any invocation depends on it. Migrated with it: both bundled plugins' factory classes, whose `__call__` becomes `create_plugin`; 23 conformance plugin handlers, each gaining a small factory class beside its plugin; two examples; one testing-package e2e test that previously aborted collection of that whole suite; and the prose in four READMEs. No handler's observable behaviour changed, and that was measured rather than assumed. The pre-migration handlers were extracted against the pre- migration SDK, every handler was driven through the local runner in both trees, and the emitted records were compared after dropping the execution ARN and wall-clock fields: 23 handlers, 89 records, 23 identical streams, 0 differing. Registration order is preserved in both handlers that register two plugins. * fix(insight): drop a superseded record; lock wrapper installs Two findings that had been accepted and deferred. Both are real, and the first one left Python as the only SDK without a guard the other two carry, which is backwards for a finding that originated here. Record building runs customer callbacks while the per-execution lock is held: the content transforms and an operation's result transform. A callback can re-enter `on_operation_change` on the same thread, build a newer record and schedule it, after which the outer frame resumes and schedules its older snapshot. Measured with a transform that re-enters while the outer frame holds an empty operation map: with export deferred the exporter saw `[[]]`, so the newer `['s1']` snapshot was coalesced away in the pending slot; with export immediate it saw `[['s1'], []]`, so the stale snapshot arrived after the newer one. Each invocation now counts the record builds it has started. A build takes the next value before it begins and the hand-off queues the record only while that value is still the newest, so an overtaken build's record is dropped. Nothing is lost, because a record is a complete snapshot of one execution and the record that superseded it carries everything the older one carried. That is the same property that makes per-execution coalescing sound. The counter is a plain `int` rather than an atomic, which differs from the Java port deliberately. Every increment and every read happens while `self._lock` is held, because `_emit` is only ever called from inside a `with self._lock` block. So the read-modify-write cannot interleave. Java needs an `AtomicLong` because two of its builds can genuinely run at once; in Python a second thread entering a hook blocks on that lock, so the only builds in flight together are nested frames on one thread that already hold it. The revalidation sits beside the existing closed-gate re-check, inside the same lock hold that queues the record, so no build can start, finish and queue between the check and the queue. The closing record takes no revision and is exempt from the check. One honest note: in Python that exemption is not reachable through a hook, because `on_invocation_end` sets the closed gate before the closing build and all three hooks check the gate before reaching `_emit`, so nothing can advance the counter during that build. The exemption is kept for parity with the JS and Java ports and so the closing record's survival does not depend on that gating holding for every future hook. It is pinned by mutation rather than by a reachable scenario: dropping the exemption fails exactly the test that asserts the closing record still arrives. Second, the OTel wrapper installs were a check-then-set with no lock, and the tracer they reach into is shared. `TracerProvider.get_tracer` caches by instrumentation scope, so two plugins built for two concurrent first invocations receive the same `Tracer` object; `get_tracer(name) is get_tracer(name)` is True. Two installs could therefore each wrap the provider's original, and the loser would keep a wrapper the tracer no longer holds, so its deterministic overrides are ignored and span ids become random, which breaks cross-invocation stitching. The window is a few bytecodes, so 400 trials at the default switch interval produced nothing. With the switch interval lowered to make the interpreter preempt inside that existing window, 448 of 2000 trials produced rival id generators, 191 of 2000 produced rival samplers, and 11 of 200 left a plugin holding an id generator or sampling delegate the tracer no longer held, for each factory class. After the fix all four measurements are zero. Both installs now hold a module-level lock across the read, the check, the construction and the assignment, and each returns whatever the tracer holds, so a plugin that loses the race uses the winner's wrapper and the winner's delegate. This follows the precedent already in this package: the log filter's install is guarded the same way for the same reason. * fix(plugin): reject a factory that returns a non-plugin Two review findings, one behavioural and one documentation. The load-time shape check establishes only that a factory has a callable `create_plugin`. What that call returns is knowable only when it is called, and `_create_plugins` rejected `None` but accepted every other object. So a factory returning `object()` was registered, and it then failed every hook inside `_dispatch_plugin`. The consequence is one logged error per hook per invocation for the life of the function, while that plugin provides no telemetry at all. The return is now checked against `DurableInstrumentationPlugin` and an invalid one is logged once and skipped, which is the same containment the `None` case already had. Every plugin in this repository already subclasses that base class, including all 25 in the conformance handlers, so the check rejects nothing that worked before. Second, the Insight README described a flush cadence the plugin no longer has. It said an invocation that emits no record neither starts nor flushes the worker. Every sampled-in invocation end now drains and flushes, whether or not it emitted a record, which is the cadence the JS and Java plugins have and which the tests already assert. Only a sampled-out execution neither exports nor flushes. The note now says that. * style(sdk): format the factory docstring example * fix(insight): drain outside the hook's lock hold A hook runs customer code while holding the plugin's lock: the input and output transforms, a result override, and __del__ on an object a displaced record carried. The lock is reentrant so that code re-entering a hook on the same thread does not self-deadlock, so a re-entrant on_invocation_end ran its drain inside the outer frame's hold. A drain waits for the export worker, and an exporter that re-enters a hook blocks that worker on the very lock the waiting thread holds, so neither side proceeds and the invocation hangs until Lambda times it out. Hook frames are now counted per thread, shared by every instance because customer code can call a hook on another execution's plugin. A drain is requested rather than performed, and the outermost frame runs it once every lock hold on the thread is released. The unnested case is unchanged. drain() called on the export worker thread is now refused and reported rather than waiting for work only that thread can do, which is the guard Java already had in refuseWaitThatWouldBlockThePump. * fix(otel): hold the log-filter claim weakly unbind_invocation resets the ContextVar claim only on the thread that ends the invocation, because a ContextVar cannot be reset from another thread. A pool thread that is never used again therefore kept a strong reference to a finished invocation's plugin, its spans and its context tokens for the life of the execution environment, and kept a plugin whose end hook never ran reachable despite _open_invocations being weak. The claim is now a weakref. A collected referent resolves to nothing, which is the answer the liveness check already gives for a finished invocation. While an invocation is open the SDK holds its plugin, which is what keeps the referent alive for every record the filter resolves. * fix(sdk): reject a factory class at registration plugins=[MyFactory] -- the factory class rather than an instance of it -- passed the registration check, because create_plugin read off the class is a plain function and therefore callable. The per-invocation call supplies only the info, Python binds it to self, and the resulting TypeError is contained like any other factory failure: instrumentation is silently absent for the lifetime of the function. Registration now binds one positional argument to the member's signature, which no factory code runs. A bound method, a classmethod, a staticmethod and a __call__ on an instance all bind; an instance method read off the class does not. A callable whose signature cannot be read is accepted on the member alone, because a missing description of a factory is not evidence of a broken one. * fix(plugin): fire the end hook on every exit The invocation wrapper fired on_invocation_end on the success path and on except Exception, so an exit by BaseException skipped it. A handler that surfaces an asyncio.CancelledError -- user code awaiting a cancelled task -- leaves that way, and so does a KeyboardInterrupt or SystemExit. The invocation then ended without the one hook a plugin has to finish on: Insight never drained, so the records it held for that execution were dropped, and OTel never ended the spans it had opened. Teardown still ran, so nothing failed visibly. Every exit now fires the hook and re-raises the exception unchanged. Plugin-code containment widens with it. The factory and hook boundaries caught Exception, so a factory or hook raising CancelledError failed an execution it was only observing and stopped the remaining plugins from running. Both now contain every BaseException except KeyboardInterrupt, SystemExit and GeneratorExit, which are instructions to the calling thread rather than reports of a plugin defect. CancelledError is not among them: nothing cancels the invocation thread or the single-worker plugin pool, so one arriving from plugin code came from the plugin's own asyncio use. ErrorObject.from_exception is annotated BaseException, which is what its own helper already accepted. * fix(plugin): send one end notification per invocation The success dispatch of on_invocation_end sat inside the try that reports a handler failure, so an end hook that raised was caught as though the handler had failed and the hook ran a second time with a RETRY outcome. Later plugins were told the wrong thing about an invocation that succeeded, and an exporter exported twice. A hook can raise: _dispatch_plugin re-raises the three exceptions that instruct the calling thread to stop, and from_dict can reject an output the handler built. The output is parsed inside the try and the success hook is dispatched after it, so each invocation produces exactly one end notification. Registration also rejects more factory-class shapes. A signature bind accepts create_plugin(self, info=None) and create_plugin(self, *args), because the probe binds to self and what remains is satisfied, so plugins=[MyFactory] still passed and then failed on every invocation where the error is swallowed. For a class the kind of the member now decides: a classmethod carries __self__, a staticmethod is identified through its descriptor, and an attribute holding a callable object takes no implicit first argument. Anything else read off a class takes self and is rejected. inspect.getattr_static walks the MRO without running a descriptor, so this still runs no factory code. * fix(plugin): pair the end hook with the start hook A start hook raising one of the three exceptions that instruct the calling thread to stop propagates out of the dispatch loop, so plugins later in the list never receive their start hook. The invocation-end hook that the propagating exception then triggers reached them anyway, leaving a plugin to tear down state it had never been told to build. Hooks after the start hook are now dispatched to the plugins that received the start hook, which makes the pairing an invariant rather than a coincidence. A plugin counts as started before its hook is dispatched, because a hook that begins and then fails may already have allocated what its end hook releases. A refused drain also requests a flush now. An exporter that re-enters a plugin hook can queue a record from the export worker, and drain() refuses the wait that hook makes. The worker exits its loop once nothing is pending and no flush is requested, so the record reached the exporters and the worker stopped, leaving a buffering exporter holding an execution's terminal telemetry when Lambda froze the environment. The request is made without waiting, so the worker stays unblocked, and its barrier covers the record just queued. * fix(plugin): finish the end dispatch, flush only new work Two follow-ups to the previous commit, both from its own review. An end hook raising one of the three control exceptions aborted the dispatch loop, so plugins after it lost the hook that is their only chance to finish: Insight would not drain and OTel would leave spans unended. Every plugin the loop reaches has already started, so the first such exception is now held and re-raised once every plugin has been called. No other hook defers, because stopping a start-hook loop early leaves later plugins with nothing to clean up: the pairing rule withholds their end hook too. The flush a refused drain requests is now conditional on a pending record. An exporter whose flush() re-enters a plugin hook arrives at the refused drain from inside a flush, and an unconditional request asked for the next one, which re-entered and requested again for as long as the environment lived. A pending record is what distinguishes the record the re-entering hook queued from that loop. * fix(plugin): split exception groups, drain before the build A BaseExceptionGroup is neither case the containment boundary tested. A group carrying a KeyboardInterrupt is not an instance of one, so the tuple handler naming the three did not match it and the broad handler contained the interrupt inside it. Plugin code produces such a group without asking: an asyncio.TaskGroup whose task is interrupted raises one. Both boundaries now partition what plugin code raised. The control leaves are returned to the caller and re-raised; what remains is logged as a contained plugin failure. A group of ordinary failures is therefore contained whole, and a mixed group is logged and then propagates only its control part. The end-hook dispatch catches BaseException rather than naming the three, because everything reaching it has already been partitioned. Insight registers its invocation-end drain on entering the hook rather than after building the record. _emit runs customer code, and a content transform raising a BaseException escapes _apply_data_content, which contains only Exception. That failure left the hook with the drain unrequested, so records this execution had already scheduled stayed in a buffering exporter when the environment froze. * fix(plugin): dispatch on one thread, keep the real failure Two findings from a fresh review pass. The failure exit fired the end hook inside its except block, so an exception out of the hook left that block before the handler's own failure was re-raised. The caller saw the plugin's exception and the real failure survived only as __context__. The end-hook dispatch does raise: it re-raises the control exceptions it holds through the fan-out. Instrumentation does not decide what an execution failed with, so the hook's exception is now contained and logged there and the original is re-raised unchanged. execute_plugins also had a sync parameter no caller ever passed. Its asynchronous branch submitted the hook to a per-invocation thread pool, which skipped the end-hook fan-out rule and swallowed a control exception in a Future nobody reads -- a way to opt out of the invariants this PR establishes, on a path with no production caller. The parameter and the pool are removed: hooks are dispatched on the calling thread, which is what lets a plugin set thread-affine state the SDK's own logging reads, and the gate that used to test the pool now tests the factory list. The pool created no thread either way, because ThreadPoolExecutor spawns lazily on first submit. * docs(insight): correct two rationales, note drain latency The log filter's bind_invocation took a process-global lock on every operation-start and user-function-start hook although the work is idempotent after the first call on a thread. It now returns early when the thread already carries this provider's claim. Two comments described an ordering the SDK prevents. Insight's lock said a checkpoint-path operation-change "genuinely races" the invocation-end hook; the SDK joins the checkpoint thread and the branch pools before that hook is dispatched. The lock stays -- it is what makes reentrancy from customer code inside a build safe, and a guard resting on the SDK's join ordering is one refactor from being wrong -- but the comment now says which of the two it is. The Insight README documents what the invocation-end drain costs under concurrency: it waits for every record any execution had pending, and one worker serializes every export and flush, so ends are released together at the slowest. Measured with a 30 ms exporter: ~72 ms at one execution, ~1.8 s each at 48. * fix(deps): bound the core dependency above Both plugin packages required aws-durable-execution-sdk-python>=3.0.0 with no ceiling. The lower bound exists because their entry points resolve to factories, which core 2.x cannot call: pip accepts the resolution and the handler fails at initialization. Without a ceiling the next core major that changes the plugin contract reproduces that exactly. Both are now >=3.0.0,<4, and each package's metadata test asserts the specifier rejects the next core major as well as accepting the current one. A second case asserts it still admits the next core patch, because a <= ceiling looks equivalent and excludes it. * fix(insight): flush a record already taken for export The condition on the refused drain's flush request was pending-only, and the worker takes a record out of the pending map before it calls the exporter. A hook re-entered from inside export() that reached an invocation end emitting no record therefore found nothing pending, asked for no flush, and left the snapshot it had just been handed in a buffering exporter until the environment froze -- the loss the request exists to prevent, arriving by the other door. The condition now separates the two re-entry paths, because they need opposite answers: a flush already in flight with nothing newly pending is the one case that asks for nothing, which is what stops an exporter whose flush() re-enters from flushing for as long as the environment lives. The plugin contract also said a plugin may hold "per-execution" state in its attributes. Per invocation is narrower, and the difference is the replay model: an execution spans as many invocations as it waits, retries or resumes, and the instance is dropped when each returns. The README and the factory protocol now say per-invocation and name the two homes for anything that has to outlive it -- the operation-map snapshot the invocation hooks carry, or the factory. * fix(insight): release displaced records outside every lock `_emit` hands a record to the scheduler while holding this instance's lock, and that hand-off displaces the record already pending for the execution. Releasing the displaced one can run a customer `__del__`, because the record is a snapshot of customer data, and a finalizer that reaches another execution's plugin blocks on that instance's lock. Two threads doing that to each other at once hang both invocations, and the invocation end is awaited before the response. The reentrant lock does not help: it covers re-entry on the same instance, which is why customer code inside a build can call this execution's hooks safely, and says nothing about a second instance. `schedule()` now returns what it displaced and the hook frame releases it once the outermost hook returns, with no lock held. Releases run before the deferred drains, so a finalizer that schedules a record is covered by the drain that follows it. * test(insight): make the finalizer probe deterministic The new test used the capturing exporter, which retains every record it is given. If the export worker exported the first record before it was displaced, the probe stayed reachable from the exporter and no finalizer ran -- so the test decided on a race it did not control, and lost it in CI while winning locally. It now uses an exporter that keeps nothing, and collects before asserting. * fix(plugin): keep factory diagnostics out of customer code Naming a failing factory for the log read the factory itself, so a factory whose __getattr__ or __getattribute__ raises made the containment raise: a create_plugin failure that should have been logged and skipped failed the invocation instead. The name now comes from the factory's type, which no instance attribute hook can intercept, and the whole lookup is wrapped, because a name is never worth failing a hook for. The scheduler's disabled path also handed the newly built record straight to the caller's local, so it was released when _emit returned -- still inside the plugin's lock, which is the finalizer hazard the previous commit closed for displaced records. It is now returned for the hook frame to release. * fix(plugin): name the invalid return type safely too The invalid-return message read type(plugin).__qualname__ directly, which a metaclass attribute hook can intercept, in the same containment path the factory name was just moved out of. Both now go through one helper that reads the type and cannot raise. * fix(plugin): name the rejected class, not its metaclass The registration diagnostic named type(value), which is the metaclass when the entry is a class. Both likely migration mistakes are classes: plugins=[MyPlugin] is the previous major's shape, and plugins=[MyPluginFactory] is this major's shape with the parentheses left off. Both reported builtins.type, which identifies neither. A class and a function are now named by their own qualified name, and the kind is stated -- "the class", "the function", "an instance of" -- because a factory class and an instance of it share one name and passing the class is itself a rejected shape. * fix(insight): make the plugin factory type public workflow_insight() declared _WorkflowInsightFactory as its return type. That name is private and absent from __all__, so a py.typed consumer could not annotate the value it holds without importing a private symbol. The sibling OTel package, changed in the same release, exports InvocationOtelPluginFactory and ExecutionOtelPluginFactory. The class is now WorkflowInsightPluginFactory and is exported from the package root. The _ExportState mixin also set five non-underscored fields, and the mixin lands on WorkflowInsightPlugin, which the package exports. Those fields belong to the export scheduler and are guarded by its lock, so a public name advertised scheduler bookkeeping as part of the plugin's API. They are now underscore-prefixed; _ExportScheduler is declared in the same module, so it still reads them directly. * fix(plugin): notify rejected explicit factories Validate explicit entries inside the registration rejection cleanup boundary. A valid opted-in factory still receives its rejection callback when another explicit entry is invalid, regardless of list order. * fix(telemetry): cover finite drain entry snapshots Capture each drain target without extending earlier waiters. Preserve coalescing, worker recovery, and best-effort serialized exporter calls. * fix(telemetry): build snapshots outside hook locks Run customer transforms outside invocation locks, reject stale records, and retain closing ownership through duplicate and reentrant End calls. * fix(plugin): contain factory return validation Validate returned proxy objects inside the existing factory containment boundary so a raising __class__ descriptor cannot abort healthy plugins or handler execution. Preserve accepted proxies and thread-control errors. * fix(plugin): clean up after factory inspection fails Notify rejected factories when ordinary validation exceptions escape attribute or signature inspection. Preserve the original exception and thread-control propagation. Cover explicit registration order, discovered factories, resource release, and descriptor and signature failures. * refactor(plugin): retain failed start context Keep Start, handler work, cleanup and End in the invocation worker Context after ordinary hook failures. Remove startup snapshots and per-hook Context.run dispatch while preserving started-plugin pairing, End fan-out and thread-control propagation. Document the Core 3 context change and cover failed bindings, token ownership, branch propagation, replay and caller isolation. * fix(plugin): complete registration cleanup Retain loaded entry-point candidates for rejection cleanup before validating their factory interface. Notify each object once across explicit registrations and environment aliases. Inspect actual factory types without invoking __class__ descriptors, and contain metadata lookup alongside registration callbacks so cleanup preserves the first failure and continues to remaining factories. Cover real entry-point loading, constructor-owned file resources, invalid signatures, raising descriptors and both explicit list orders. --------- Co-authored-by: Frank Chen <65260095+zhongkechen@users.noreply.github.com>
zhongkechen
added a commit
to aws/aws-durable-execution-sdk-python
that referenced
this pull request
Oct 22, 2026
* fix(insight): key export scheduling by execution ARN One plugin instance serves every execution its environment hosts, and 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 before the change, 20 trials per case: 2 concurrent executions lost a terminal record in 16 of 20 trials, 10 concurrent lost 8.5 per trial, and with 5 concurrent executions and a 200 ms exporter only 1 of 5 terminal records was exported while every drain() still returned without error. Pending records are now keyed by execution ARN with a per-execution lane, so coalescing happens only within one execution and drain(execution_arn) returns only once that execution's own record has reached every exporter and a flush covering it has completed. One worker thread and one export at a time are unchanged, so an exporter never sees concurrent calls. Also fixed, each found while reviewing the change above: - A BaseException from a customer exporter (asyncio.CancelledError inherits from it, so an exporter touching asyncio can raise it without writing raise) killed the worker between consuming a record and publishing its bookkeeping. The worker slot stayed occupied by a dying thread, no replacement started, and the parked drain hung the invocation thread permanently. - A drain could request a second flush while one was already in flight, so a flush ran after drain() had returned, calling an exporter after the invocation went back to Lambda. - The per-execution lock was not reentrant while a displaced record was released under it, so a record whose finalizer re-entered a hook for the same execution self-deadlocked the invocation thread. - The closed gate was a check-then-act: customer code running under the reentrant lock could complete on_invocation_end on the same thread, after which the outer frame still scheduled its RUNNING record behind the terminal one. - A flush happened only when a record was emitted, where JS and Java flush once per sampled-in invocation end. A buffering exporter now sees the same rhythm in all three languages. - InsightExporter.flush had no docstring. It now states the cadence, the exclusivity guarantee, that a flush may cover other executions' records, what happens when an exporter omits the method, and how failures are handled. No public API changed. Record fields, emit modes, sampling, truncation and the default exporter are unchanged; the emitted record surface is byte-identical to before the change. * refactor(plugin): create one plugin instance per invocation 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: @durable_execution(plugins=[lambda info: MyPlugin(shared_exporter)]) A factory is any callable taking `InvocationStartInfo` and returning a plugin. Environment-lifetime state stays in a callable class or a closure; per-invocation state becomes ordinary instance attributes. This deletes the provider path rather than adding a parallel one. While an instance path exists a plugin cannot delete its ARN-keyed dict, which is the entire point of the change. Removed: `DurableInstrumentationPluginProvider` and `DURABLE_INSTRUMENTATION_PLUGIN_API_VERSION`. Added: `DurableInstrumentationPluginFactory`. Entry points in the `aws_durable_execution.plugins` group now name a callable, so environment-based loading works unchanged; `load_configured_plugins` dedups by factory identity instead of by plugin type. Workflow Insight now holds no ARN-keyed structure at all. Its `RLock` and its pre-hand-off re-check both stay: hooks dispatch on the producing thread, so races within one invocation remain, and `schedule()` releases displaced records while holding the lock, which can re-enter through a customer finalizer. The OTel plugin keeps each invocation's spans in plain attributes, so the state-reset machinery is gone. `install_log_filter` now rebinds the installed filter to the current invocation's plugin. Without that, a logging handler outlives the plugin that installed it, keeps asking a discarded instance for a span context, and log correlation stops after the first invocation while the dead instance stays reachable for the life of the environment. BREAKING CHANGE: `plugins` accepts factories instead of plugin instances, and the provider class is removed. Where you passed `MyPlugin()`, pass a factory: `lambda info: MyPlugin()`. A plugin class is itself a valid factory when its constructor takes the info argument, because calling a class in Python constructs an instance. Entry points in the `aws_durable_execution.plugins` group must now name a callable rather than a provider object. * fix(plugin): give each invocation its own plugin session Review findings on the per-invocation plugin contract. The first one meant the contract change did not actually deliver what it promised under concurrency. `durable_execution` built one `PluginExecutor` at decoration time and every invocation shared it, while the executor stored that invocation's plugin instances, invocation metadata and operations provider on itself. So the SDK created one plugin per invocation and then parked it in a shared mutable slot. Measured with two invocations held inside their user function at once, invocation A's operation hook was delivered to B's plugin and A's plugin never received its own operation or end hook; in a second scenario A's scope exit cleared the shared state while B was live and B lost `on_invocation_end` entirely. A new handler-lifetime `PluginHost` now holds nothing but the resolved factory list and hands out a fresh `PluginExecutor` per invocation, which is the shape JS and Java already had. The executor is reachable only from the frames of the invocation that owns it, and a second `run()` on one executor raises, so reuse fails loudly instead of as silent crosstalk. Its thread pool is per invocation too: sharing one would let an ending invocation shut down a pool a concurrent invocation was still submitting to. The OTel log filter had the same class of bug. It held one mutable plugin reference, so with two invocations open the one that started last won and every other invocation's records carried its trace. Reading the active span from the OTel context, as the Java plugin does, is not sufficient here: the invocation span is never attached to the context, and the SDK runs the handler body on a pool thread that does not inherit the context of the thread the plugin bound on. The filter is now stateless and resolves per record, preferring the invocation that claimed the emitting thread through a `ContextVar` and falling back to the single open invocation when exactly one is open. With several open and an unclaimed thread it leaves the record unstamped, because an unattributed record is a smaller defect than one attributed to another customer's execution. Installation is serialized so concurrent first- time callers cannot stack filters. Third, the Insight scheduler used `0` for both "no flush in flight" and "a flush in flight covering zero exports", which are different facts. An invocation that emitted no record could not tell that its flush was already running, so it requested a second one that then ran after both invocations had returned — customer exporter code past the invocation boundary, which the flush contract forbids. Absence is now `None` and coverage an integer. * fix(otel): correlate top-level logs under concurrency A log record emitted at the top of a handler got no OTel correlation at all when two invocations were open, which contradicts the documented behaviour of the default filter. The chain: `on_invocation_start` runs on the Lambda invocation thread, so the plugin's `ContextVar` claim lands there, but the SDK then submits the handler body to a worker thread, and Python does not propagate a `ContextVar` into a thread started that way. No hook runs on that worker before the handler's first statement -- `on_user_function_start` is dispatched for durable operations only -- so the claim never reached it. The filter fell back to "the single open invocation", which is unavailable when two are open, and left both records unstamped. The SDK now carries the invocation thread's context into the worker that runs the handler body, with `contextvars.copy_context().run`. The copy is taken at submit time, after `on_invocation_start` has run on the same thread, so the claim is inside it. This is the correct layer: the SDK creates that thread to run user code, and any contextvar-based instrumentation is otherwise invisible there. It also matches `asyncio.to_thread`, which runs its callable in a copy of the caller's context. The background checkpointing thread is deliberately left starting from an empty context. It runs SDK checkpointing rather than user code, and it dispatches operation-end hooks, so copying the context there would make the invocation thread's ambient OTel context current on a thread that starts and ends spans. Sharing one copy between the two submissions is impossible anyway, because a `Context` cannot be entered twice concurrently. Propagation had one side effect worth closing rather than documenting. `get_current_span_context` prefers a same-trace span that is current over the plugin's own `Invocation` span, so once the ambient context reached the handler thread, a top-level record named the ambient span instead. Under X-Ray active tracing with the ADOT layer that ambient span is the parent of `Invocation`, so records moved one level up the tree. The plugin now records the span that enclosed the invocation and excludes that one span from the preference, which restores the previous behaviour. Anything that becomes current later is inside the invocation -- an attempt span, a child context span, or a span the handler starts itself -- and still wins, because it is more specific. One case remains uncorrelated by design: a record emitted on a thread that carries no claim. That covers a thread customer code starts itself and the SDK's checkpointing thread. The README states this rather than claiming every record is stamped. BREAKING CHANGE: the handler body now runs in a copy of the invocation thread's context rather than in an empty one, so two behaviours change for every customer, not only for plugin authors. A span started at the top of the handler body now has the ambient span as its parent instead of being a root span; under X-Ray active tracing with the ADOT layer that ambient span is the Lambda invocation span, so existing traces gain a level after upgrading. And a ContextVar set before the decorator is now visible inside the handler body. Both follow from the propagation, which matches what asyncio.to_thread does with the caller's context. * fix(insight): contain exporter BaseException; bump to 3.0.0 Four review findings. The first is the most serious defect in this branch so far, because it exhausts the environment rather than only stalling it. An exporter whose flush() raises a BaseException on every call used to hang the invocation without bound. A drain is released only by a flush that completed. The flush raised, so nothing advanced, and the worker died. A drain replaces a dead worker, so the drain started a replacement, which ran the same failing flush and died the same way. Measured over three seconds on one such exporter: 4842 flush attempts, 3682 live threads, and the drain never returned. So the earlier fix that let a replacement start converted a permanent park into a thread storm. The same finally block also advanced the completion bookkeeping when the first exporter aborted the fan-out with a BaseException. Measured with a failing exporter followed by a healthy one: the drain returned, the healthy exporter received nothing, and coverage read as delivered. The snapshot was already out of the pending slot, so the loss was permanent. Both exporter call sites now contain BaseException, so the containment that already held for Exception holds for every shape an exporter can raise. That is safe here specifically because none of the three signalling cases can reach these sites as a signal: the interpreter raises KeyboardInterrupt only in the main thread, threading discards a SystemExit raised in a worker thread, and nothing cancels the export worker because nothing outside the scheduler knows it exists. A BaseException seen there was raised by the exporter, so it reports a defective exporter rather than instructing this thread to stop. asyncio.CancelledError is the reachable case, since an exporter that touches asyncio raises it without writing `raise`. Consecutive worker deaths are now bounded at three, as a backstop for the scheduler's own code rather than for exporters. At the bound the scheduler latches asynchronous export off, wakes every parked waiter and drops what is queued, with a warning. Releasing the waiter is the priority because the waiter is an invocation thread inside on_invocation_end: parking it turns an instrumentation defect into a stalled customer execution, while dropping records loses instrumentation data only. Any completed export or flush resets the count, so a worker making progress never approaches it. Second, the decorated handler advertised a phantom argument. functools.wraps was applied to a three-argument internal function, so inspect.signature() on the handler reported a required plugin_executor parameter and signature.bind(event, context) raised TypeError. A signature-aware runtime or test harness can reject such a handler. The user function's metadata is now copied at the head of the wrapper chain, so the handler reports (event, context) again. Third, explicit plugins entries were never validated. The entry-point path rejects a non-callable target, but explicit entries were copied through, so plugins=[MyPlugin()] passed configuration and then raised TypeError inside _create_plugins on every invocation, which logged it and continued without that plugin. A breaking migration therefore produced repeated telemetry loss instead of one configuration failure. Explicit entries are now validated at load time and name the offending position. A plugin class is still accepted, because calling a class in Python constructs an instance, so plugins=[MyPlugin] is a valid factory. Fourth, both plugin packages allowed a core version that cannot work. They declared aws-durable-execution-sdk-python>=2.0.0 while requiring the factory contract, so pip accepted a resolution that fails at handler initialization. RELEASING.md makes version numbers this repository's responsibility, so this commit sets them: core 2.0.0 to 3.0.0 for the removed provider contract, otel 1.0.0 to 2.0.0 for the replaced plugin constructors, and insight 0.0.1 to 0.1.0 rather than 0.0.2 so a consumer pinning ~=0.0.1 cannot silently accept a breaking release. Both plugin bounds move to >=3.0.0, the OTel layer's sdk-version pin follows, and the two conformance packages' exact pins follow. Two new metadata tests assert that the declared bound tracks the core major this repository builds and that the layer pin matches the core version, so the next bump cannot leave one of them behind. * fix(otel): stamp a log record only from a live claim Two reviewers found two orderings where the log filter attributed a record to the wrong execution. Both went through the same fallback, which resolved a record to the only open invocation when the emitting thread carried no live claim. The first ordering is a stale claim. Invocation A claims thread T. A ends, and unbind_invocation cannot reset T's ContextVar, because only the thread that set a ContextVar can reset it. B starts and is the only open invocation. A thread the customer started under A still carries A's claim, so the claim is stale, the fallback runs, and A's record is stamped with B's trace. Measured: the record carried B's trace and span exactly. The second ordering is a missing claim. A is open. B emits a log on its own wrapper thread before B reaches on_invocation_start, so B's thread has no claim and the registry holds only A. Measured: B's record carried A's trace and span exactly. Both outcomes are the one the design chose against, which is that an unattributed record is better than a record attributed to another customer's execution. So the fallback is deleted and a live claim is now required. One reviewer proposed keeping the fallback for threads that never held a claim, which fixes the first ordering and leaves the second. Requiring a claim fixes both. Deleting it is affordable because the threads that run customer code all carry a claim. The core SDK copies the invocation's context into the thread running the handler body, and the operation hooks claim the map and parallel branch threads before the branch body runs. Measured over one execution driving a step, a parallel, a map and a customer-started thread: every record from user code carried a claim. Two threads now go unstamped whatever the invocation count. The SDK's background checkpointing thread is submitted without the invocation's context deliberately. A thread customer code starts itself does not inherit the starting thread's context. The README states both rather than claiming every record is stamped. A second defect made the registry untrustworthy, so it is fixed here too. on_invocation_end ended the spans and exported the workflow span before releasing the invocation scope, with no finally. Both calls run customer tracer and span-processor code, which can raise. The plugin executor contains that exception, so the invocation survives, but the release was skipped: the plugin stayed registered as open for the life of the environment and its OTel context stayed attached. Measured with a span processor whose on_end raises: the plugin remained in the open registry for both plugins, and for ExecutionOtelPlugin a later record on the still-claimed thread was stamped with the finished invocation. The release and the flush now run in a finally, in both plugins. force_flush is contained there as well, because an exception escaping a finally would replace the exception that ended the invocation and hide its cause. Two documentation corrections from the same review. The factory docs presented a plugin class as the example factory, which encourages __init__(self, info) to do setup work, and CONTRIBUTING.md:246 asks for light constructors. A class is still a valid factory, because calling a class constructs an instance, so only the emphasis changes: the recommended forms are now a lambda or a classmethod, with class-as- factory noted as permitted. And the PluginHost docstring cited two cross-SDK symbols as if they were on the default branches. Checked against those repositories: the JS function there is createPluginRunner, and createInvocationPluginRunner exists only in aws/aws-durable- execution-sdk-js#924; the Java PluginRunner does exist on main, but takes plugin instances, and aws/aws-durable-execution-sdk-java#721 is what makes it per-invocation. The docstring now says that. * refactor(plugin): make the factory an object with create_plugin 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 callable type alias 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 callable 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 a Protocol declaring `create_plugin(info)`, not `Callable[[InvocationStartInfo], ...]`. The type's name and the returned type's name are both unchanged, so no identifier changes meaning. The method is `create_plugin` rather than `new_invocation` 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 protocol is deliberately not `@runtime_checkable`, for three reasons. An `isinstance` check against a runtime-checkable protocol tests only that the member name exists, so it accepts an object whose `create_plugin` is a string. The SDK also has to name what an invalid entry actually was, which a boolean cannot supply. And such a check requires every declared member, which would make the optional second member above non-additive for any caller who wrote one. `_is_plugin_factory` tests for a callable `create_plugin` instead, structurally, so a factory need not import the protocol. `info` is positional-only on the protocol method. A protocol parameter that is positional-or-keyword is part of the structural contract, so a factory naming it `invocation` would otherwise be a type error. One consequence reverses guidance added earlier in this branch. A plugin class used to be a valid factory, because calling a class constructs an instance, and the docs said so. A class carries no `create_plugin`, so `plugins=[MyPlugin]` now fails at handler initialization, and the test that pinned the old behaviour is replaced by one pinning the new. A class that declares `create_plugin` itself, as a classmethod, is still valid, because the requirement is the member and not the kind of object. The reversal is an improvement for a reason the earlier round raised separately: `CONTRIBUTING.md:246` asks for light constructors, and class-as-factory encouraged `__init__(self, info)` to do setup work. A factory object puts that work in the factory's own constructor, where it can fail before any invocation depends on it. Migrated with it: both bundled plugins' factory classes, whose `__call__` becomes `create_plugin`; 23 conformance plugin handlers, each gaining a small factory class beside its plugin; two examples; one testing-package e2e test that previously aborted collection of that whole suite; and the prose in four READMEs. No handler's observable behaviour changed, and that was measured rather than assumed. The pre-migration handlers were extracted against the pre- migration SDK, every handler was driven through the local runner in both trees, and the emitted records were compared after dropping the execution ARN and wall-clock fields: 23 handlers, 89 records, 23 identical streams, 0 differing. Registration order is preserved in both handlers that register two plugins. * fix(insight): drop a superseded record; lock wrapper installs Two findings that had been accepted and deferred. Both are real, and the first one left Python as the only SDK without a guard the other two carry, which is backwards for a finding that originated here. Record building runs customer callbacks while the per-execution lock is held: the content transforms and an operation's result transform. A callback can re-enter `on_operation_change` on the same thread, build a newer record and schedule it, after which the outer frame resumes and schedules its older snapshot. Measured with a transform that re-enters while the outer frame holds an empty operation map: with export deferred the exporter saw `[[]]`, so the newer `['s1']` snapshot was coalesced away in the pending slot; with export immediate it saw `[['s1'], []]`, so the stale snapshot arrived after the newer one. Each invocation now counts the record builds it has started. A build takes the next value before it begins and the hand-off queues the record only while that value is still the newest, so an overtaken build's record is dropped. Nothing is lost, because a record is a complete snapshot of one execution and the record that superseded it carries everything the older one carried. That is the same property that makes per-execution coalescing sound. The counter is a plain `int` rather than an atomic, which differs from the Java port deliberately. Every increment and every read happens while `self._lock` is held, because `_emit` is only ever called from inside a `with self._lock` block. So the read-modify-write cannot interleave. Java needs an `AtomicLong` because two of its builds can genuinely run at once; in Python a second thread entering a hook blocks on that lock, so the only builds in flight together are nested frames on one thread that already hold it. The revalidation sits beside the existing closed-gate re-check, inside the same lock hold that queues the record, so no build can start, finish and queue between the check and the queue. The closing record takes no revision and is exempt from the check. One honest note: in Python that exemption is not reachable through a hook, because `on_invocation_end` sets the closed gate before the closing build and all three hooks check the gate before reaching `_emit`, so nothing can advance the counter during that build. The exemption is kept for parity with the JS and Java ports and so the closing record's survival does not depend on that gating holding for every future hook. It is pinned by mutation rather than by a reachable scenario: dropping the exemption fails exactly the test that asserts the closing record still arrives. Second, the OTel wrapper installs were a check-then-set with no lock, and the tracer they reach into is shared. `TracerProvider.get_tracer` caches by instrumentation scope, so two plugins built for two concurrent first invocations receive the same `Tracer` object; `get_tracer(name) is get_tracer(name)` is True. Two installs could therefore each wrap the provider's original, and the loser would keep a wrapper the tracer no longer holds, so its deterministic overrides are ignored and span ids become random, which breaks cross-invocation stitching. The window is a few bytecodes, so 400 trials at the default switch interval produced nothing. With the switch interval lowered to make the interpreter preempt inside that existing window, 448 of 2000 trials produced rival id generators, 191 of 2000 produced rival samplers, and 11 of 200 left a plugin holding an id generator or sampling delegate the tracer no longer held, for each factory class. After the fix all four measurements are zero. Both installs now hold a module-level lock across the read, the check, the construction and the assignment, and each returns whatever the tracer holds, so a plugin that loses the race uses the winner's wrapper and the winner's delegate. This follows the precedent already in this package: the log filter's install is guarded the same way for the same reason. * fix(plugin): reject a factory that returns a non-plugin Two review findings, one behavioural and one documentation. The load-time shape check establishes only that a factory has a callable `create_plugin`. What that call returns is knowable only when it is called, and `_create_plugins` rejected `None` but accepted every other object. So a factory returning `object()` was registered, and it then failed every hook inside `_dispatch_plugin`. The consequence is one logged error per hook per invocation for the life of the function, while that plugin provides no telemetry at all. The return is now checked against `DurableInstrumentationPlugin` and an invalid one is logged once and skipped, which is the same containment the `None` case already had. Every plugin in this repository already subclasses that base class, including all 25 in the conformance handlers, so the check rejects nothing that worked before. Second, the Insight README described a flush cadence the plugin no longer has. It said an invocation that emits no record neither starts nor flushes the worker. Every sampled-in invocation end now drains and flushes, whether or not it emitted a record, which is the cadence the JS and Java plugins have and which the tests already assert. Only a sampled-out execution neither exports nor flushes. The note now says that. * style(sdk): format the factory docstring example * fix(insight): drain outside the hook's lock hold A hook runs customer code while holding the plugin's lock: the input and output transforms, a result override, and __del__ on an object a displaced record carried. The lock is reentrant so that code re-entering a hook on the same thread does not self-deadlock, so a re-entrant on_invocation_end ran its drain inside the outer frame's hold. A drain waits for the export worker, and an exporter that re-enters a hook blocks that worker on the very lock the waiting thread holds, so neither side proceeds and the invocation hangs until Lambda times it out. Hook frames are now counted per thread, shared by every instance because customer code can call a hook on another execution's plugin. A drain is requested rather than performed, and the outermost frame runs it once every lock hold on the thread is released. The unnested case is unchanged. drain() called on the export worker thread is now refused and reported rather than waiting for work only that thread can do, which is the guard Java already had in refuseWaitThatWouldBlockThePump. * fix(otel): hold the log-filter claim weakly unbind_invocation resets the ContextVar claim only on the thread that ends the invocation, because a ContextVar cannot be reset from another thread. A pool thread that is never used again therefore kept a strong reference to a finished invocation's plugin, its spans and its context tokens for the life of the execution environment, and kept a plugin whose end hook never ran reachable despite _open_invocations being weak. The claim is now a weakref. A collected referent resolves to nothing, which is the answer the liveness check already gives for a finished invocation. While an invocation is open the SDK holds its plugin, which is what keeps the referent alive for every record the filter resolves. * fix(sdk): reject a factory class at registration plugins=[MyFactory] -- the factory class rather than an instance of it -- passed the registration check, because create_plugin read off the class is a plain function and therefore callable. The per-invocation call supplies only the info, Python binds it to self, and the resulting TypeError is contained like any other factory failure: instrumentation is silently absent for the lifetime of the function. Registration now binds one positional argument to the member's signature, which no factory code runs. A bound method, a classmethod, a staticmethod and a __call__ on an instance all bind; an instance method read off the class does not. A callable whose signature cannot be read is accepted on the member alone, because a missing description of a factory is not evidence of a broken one. * fix(plugin): fire the end hook on every exit The invocation wrapper fired on_invocation_end on the success path and on except Exception, so an exit by BaseException skipped it. A handler that surfaces an asyncio.CancelledError -- user code awaiting a cancelled task -- leaves that way, and so does a KeyboardInterrupt or SystemExit. The invocation then ended without the one hook a plugin has to finish on: Insight never drained, so the records it held for that execution were dropped, and OTel never ended the spans it had opened. Teardown still ran, so nothing failed visibly. Every exit now fires the hook and re-raises the exception unchanged. Plugin-code containment widens with it. The factory and hook boundaries caught Exception, so a factory or hook raising CancelledError failed an execution it was only observing and stopped the remaining plugins from running. Both now contain every BaseException except KeyboardInterrupt, SystemExit and GeneratorExit, which are instructions to the calling thread rather than reports of a plugin defect. CancelledError is not among them: nothing cancels the invocation thread or the single-worker plugin pool, so one arriving from plugin code came from the plugin's own asyncio use. ErrorObject.from_exception is annotated BaseException, which is what its own helper already accepted. * fix(plugin): send one end notification per invocation The success dispatch of on_invocation_end sat inside the try that reports a handler failure, so an end hook that raised was caught as though the handler had failed and the hook ran a second time with a RETRY outcome. Later plugins were told the wrong thing about an invocation that succeeded, and an exporter exported twice. A hook can raise: _dispatch_plugin re-raises the three exceptions that instruct the calling thread to stop, and from_dict can reject an output the handler built. The output is parsed inside the try and the success hook is dispatched after it, so each invocation produces exactly one end notification. Registration also rejects more factory-class shapes. A signature bind accepts create_plugin(self, info=None) and create_plugin(self, *args), because the probe binds to self and what remains is satisfied, so plugins=[MyFactory] still passed and then failed on every invocation where the error is swallowed. For a class the kind of the member now decides: a classmethod carries __self__, a staticmethod is identified through its descriptor, and an attribute holding a callable object takes no implicit first argument. Anything else read off a class takes self and is rejected. inspect.getattr_static walks the MRO without running a descriptor, so this still runs no factory code. * fix(plugin): pair the end hook with the start hook A start hook raising one of the three exceptions that instruct the calling thread to stop propagates out of the dispatch loop, so plugins later in the list never receive their start hook. The invocation-end hook that the propagating exception then triggers reached them anyway, leaving a plugin to tear down state it had never been told to build. Hooks after the start hook are now dispatched to the plugins that received the start hook, which makes the pairing an invariant rather than a coincidence. A plugin counts as started before its hook is dispatched, because a hook that begins and then fails may already have allocated what its end hook releases. A refused drain also requests a flush now. An exporter that re-enters a plugin hook can queue a record from the export worker, and drain() refuses the wait that hook makes. The worker exits its loop once nothing is pending and no flush is requested, so the record reached the exporters and the worker stopped, leaving a buffering exporter holding an execution's terminal telemetry when Lambda froze the environment. The request is made without waiting, so the worker stays unblocked, and its barrier covers the record just queued. * fix(plugin): finish the end dispatch, flush only new work Two follow-ups to the previous commit, both from its own review. An end hook raising one of the three control exceptions aborted the dispatch loop, so plugins after it lost the hook that is their only chance to finish: Insight would not drain and OTel would leave spans unended. Every plugin the loop reaches has already started, so the first such exception is now held and re-raised once every plugin has been called. No other hook defers, because stopping a start-hook loop early leaves later plugins with nothing to clean up: the pairing rule withholds their end hook too. The flush a refused drain requests is now conditional on a pending record. An exporter whose flush() re-enters a plugin hook arrives at the refused drain from inside a flush, and an unconditional request asked for the next one, which re-entered and requested again for as long as the environment lived. A pending record is what distinguishes the record the re-entering hook queued from that loop. * fix(plugin): split exception groups, drain before the build A BaseExceptionGroup is neither case the containment boundary tested. A group carrying a KeyboardInterrupt is not an instance of one, so the tuple handler naming the three did not match it and the broad handler contained the interrupt inside it. Plugin code produces such a group without asking: an asyncio.TaskGroup whose task is interrupted raises one. Both boundaries now partition what plugin code raised. The control leaves are returned to the caller and re-raised; what remains is logged as a contained plugin failure. A group of ordinary failures is therefore contained whole, and a mixed group is logged and then propagates only its control part. The end-hook dispatch catches BaseException rather than naming the three, because everything reaching it has already been partitioned. Insight registers its invocation-end drain on entering the hook rather than after building the record. _emit runs customer code, and a content transform raising a BaseException escapes _apply_data_content, which contains only Exception. That failure left the hook with the drain unrequested, so records this execution had already scheduled stayed in a buffering exporter when the environment froze. * fix(plugin): dispatch on one thread, keep the real failure Two findings from a fresh review pass. The failure exit fired the end hook inside its except block, so an exception out of the hook left that block before the handler's own failure was re-raised. The caller saw the plugin's exception and the real failure survived only as __context__. The end-hook dispatch does raise: it re-raises the control exceptions it holds through the fan-out. Instrumentation does not decide what an execution failed with, so the hook's exception is now contained and logged there and the original is re-raised unchanged. execute_plugins also had a sync parameter no caller ever passed. Its asynchronous branch submitted the hook to a per-invocation thread pool, which skipped the end-hook fan-out rule and swallowed a control exception in a Future nobody reads -- a way to opt out of the invariants this PR establishes, on a path with no production caller. The parameter and the pool are removed: hooks are dispatched on the calling thread, which is what lets a plugin set thread-affine state the SDK's own logging reads, and the gate that used to test the pool now tests the factory list. The pool created no thread either way, because ThreadPoolExecutor spawns lazily on first submit. * docs(insight): correct two rationales, note drain latency The log filter's bind_invocation took a process-global lock on every operation-start and user-function-start hook although the work is idempotent after the first call on a thread. It now returns early when the thread already carries this provider's claim. Two comments described an ordering the SDK prevents. Insight's lock said a checkpoint-path operation-change "genuinely races" the invocation-end hook; the SDK joins the checkpoint thread and the branch pools before that hook is dispatched. The lock stays -- it is what makes reentrancy from customer code inside a build safe, and a guard resting on the SDK's join ordering is one refactor from being wrong -- but the comment now says which of the two it is. The Insight README documents what the invocation-end drain costs under concurrency: it waits for every record any execution had pending, and one worker serializes every export and flush, so ends are released together at the slowest. Measured with a 30 ms exporter: ~72 ms at one execution, ~1.8 s each at 48. * fix(deps): bound the core dependency above Both plugin packages required aws-durable-execution-sdk-python>=3.0.0 with no ceiling. The lower bound exists because their entry points resolve to factories, which core 2.x cannot call: pip accepts the resolution and the handler fails at initialization. Without a ceiling the next core major that changes the plugin contract reproduces that exactly. Both are now >=3.0.0,<4, and each package's metadata test asserts the specifier rejects the next core major as well as accepting the current one. A second case asserts it still admits the next core patch, because a <= ceiling looks equivalent and excludes it. * fix(insight): flush a record already taken for export The condition on the refused drain's flush request was pending-only, and the worker takes a record out of the pending map before it calls the exporter. A hook re-entered from inside export() that reached an invocation end emitting no record therefore found nothing pending, asked for no flush, and left the snapshot it had just been handed in a buffering exporter until the environment froze -- the loss the request exists to prevent, arriving by the other door. The condition now separates the two re-entry paths, because they need opposite answers: a flush already in flight with nothing newly pending is the one case that asks for nothing, which is what stops an exporter whose flush() re-enters from flushing for as long as the environment lives. The plugin contract also said a plugin may hold "per-execution" state in its attributes. Per invocation is narrower, and the difference is the replay model: an execution spans as many invocations as it waits, retries or resumes, and the instance is dropped when each returns. The README and the factory protocol now say per-invocation and name the two homes for anything that has to outlive it -- the operation-map snapshot the invocation hooks carry, or the factory. * fix(insight): release displaced records outside every lock `_emit` hands a record to the scheduler while holding this instance's lock, and that hand-off displaces the record already pending for the execution. Releasing the displaced one can run a customer `__del__`, because the record is a snapshot of customer data, and a finalizer that reaches another execution's plugin blocks on that instance's lock. Two threads doing that to each other at once hang both invocations, and the invocation end is awaited before the response. The reentrant lock does not help: it covers re-entry on the same instance, which is why customer code inside a build can call this execution's hooks safely, and says nothing about a second instance. `schedule()` now returns what it displaced and the hook frame releases it once the outermost hook returns, with no lock held. Releases run before the deferred drains, so a finalizer that schedules a record is covered by the drain that follows it. * test(insight): make the finalizer probe deterministic The new test used the capturing exporter, which retains every record it is given. If the export worker exported the first record before it was displaced, the probe stayed reachable from the exporter and no finalizer ran -- so the test decided on a race it did not control, and lost it in CI while winning locally. It now uses an exporter that keeps nothing, and collects before asserting. * fix(plugin): keep factory diagnostics out of customer code Naming a failing factory for the log read the factory itself, so a factory whose __getattr__ or __getattribute__ raises made the containment raise: a create_plugin failure that should have been logged and skipped failed the invocation instead. The name now comes from the factory's type, which no instance attribute hook can intercept, and the whole lookup is wrapped, because a name is never worth failing a hook for. The scheduler's disabled path also handed the newly built record straight to the caller's local, so it was released when _emit returned -- still inside the plugin's lock, which is the finalizer hazard the previous commit closed for displaced records. It is now returned for the hook frame to release. * fix(plugin): name the invalid return type safely too The invalid-return message read type(plugin).__qualname__ directly, which a metaclass attribute hook can intercept, in the same containment path the factory name was just moved out of. Both now go through one helper that reads the type and cannot raise. * fix(plugin): name the rejected class, not its metaclass The registration diagnostic named type(value), which is the metaclass when the entry is a class. Both likely migration mistakes are classes: plugins=[MyPlugin] is the previous major's shape, and plugins=[MyPluginFactory] is this major's shape with the parentheses left off. Both reported builtins.type, which identifies neither. A class and a function are now named by their own qualified name, and the kind is stated -- "the class", "the function", "an instance of" -- because a factory class and an instance of it share one name and passing the class is itself a rejected shape. * fix(insight): make the plugin factory type public workflow_insight() declared _WorkflowInsightFactory as its return type. That name is private and absent from __all__, so a py.typed consumer could not annotate the value it holds without importing a private symbol. The sibling OTel package, changed in the same release, exports InvocationOtelPluginFactory and ExecutionOtelPluginFactory. The class is now WorkflowInsightPluginFactory and is exported from the package root. The _ExportState mixin also set five non-underscored fields, and the mixin lands on WorkflowInsightPlugin, which the package exports. Those fields belong to the export scheduler and are guarded by its lock, so a public name advertised scheduler bookkeeping as part of the plugin's API. They are now underscore-prefixed; _ExportScheduler is declared in the same module, so it still reads them directly. * fix(plugin): notify rejected explicit factories Validate explicit entries inside the registration rejection cleanup boundary. A valid opted-in factory still receives its rejection callback when another explicit entry is invalid, regardless of list order. * fix(telemetry): cover finite drain entry snapshots Capture each drain target without extending earlier waiters. Preserve coalescing, worker recovery, and best-effort serialized exporter calls. * fix(telemetry): build snapshots outside hook locks Run customer transforms outside invocation locks, reject stale records, and retain closing ownership through duplicate and reentrant End calls. * fix(plugin): contain factory return validation Validate returned proxy objects inside the existing factory containment boundary so a raising __class__ descriptor cannot abort healthy plugins or handler execution. Preserve accepted proxies and thread-control errors. * fix(plugin): clean up after factory inspection fails Notify rejected factories when ordinary validation exceptions escape attribute or signature inspection. Preserve the original exception and thread-control propagation. Cover explicit registration order, discovered factories, resource release, and descriptor and signature failures. * refactor(plugin): retain failed start context Keep Start, handler work, cleanup and End in the invocation worker Context after ordinary hook failures. Remove startup snapshots and per-hook Context.run dispatch while preserving started-plugin pairing, End fan-out and thread-control propagation. Document the Core 3 context change and cover failed bindings, token ownership, branch propagation, replay and caller isolation. * fix(plugin): complete registration cleanup Retain loaded entry-point candidates for rejection cleanup before validating their factory interface. Notify each object once across explicit registrations and environment aliases. Inspect actual factory types without invoking __class__ descriptors, and contain metadata lookup alongside registration callbacks so cleanup preserves the first failure and continues to remaining factories. Cover real entry-point loading, constructor-owned file resources, invalid signatures, raising descriptors and both explicit list orders. --------- Co-authored-by: Frank Chen <65260095+zhongkechen@users.noreply.github.com>
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.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache2.0 license.
Issue Link, if available
Addresses #761 and #763. Retains the fixes from #765, #766, #767, #771 and the conformance handlers from #769, plus the read-only AI intake correction from #772.
Description
Plugin instances previously shared invocation state across overlapping Lambda Managed Instances. Register factories and create one plugin instance per invocation, while keeping provider/resource ownership in the reusable environment. Factories and start hooks run in registration order; even fatal partial startup retains already-created instances for finalization. This is the explicitly breaking Java3.0 boundary: all nine reactor POMs and packaged SDK artifacts use
3.0.0-SNAPSHOT. Rebuild custom providers/layers against the factory API; the minor PRs remain independently backward compatible.Nonfatal plugin failures, including incompatible optional APIs, remain isolated. Invocation-end dispatch gives each remaining plugin its one finalization opportunity after an earlier end hook fails fatally, then rethrows the first fatal; registration order and the shared end snapshot are preserved. The first end-hook fatal is reported before later hooks run, so a blocked exporter cannot let queued user work continue; an earlier invocation fatal in the snapshot takes precedence. Plugin-owned
VirtualMachineError/ThreadDeathcauses retain their identity across invocation, operation and checkpoint callbacks. An invocation-local signal prevents operation retry and user-error serialization after such a failure, immediately fails delayed checkpoint and polling futures, cancels flush timers, rejects later requests, and propagates through the invocation boundary. Backend admission and suspension respect that signal while the root worker finishes its own scope cleanup. The batcher checks that signal before and after its backend call, so a queued START skipped after another hook fails cannot be reported as successful or authorize an at-most-once step body. At-least-once steps may execute again on replay if their result was not checkpointed; at-most-once attempts await START acknowledgment. Async operation and checkpoint workers rethrow their reported plugin fatal outside CompletableFuture containment while settling the sequencing future. New operation dispatch and queued user handlers reject an already-recorded fatal before user code runs; direct executors propagate at the operation call so subsequent handler code cannot execute. If a later user-function start hook fails fatally, successfully started hooks receive failure cleanup in reverse order on their owner thread before the fatal is reported to invocation finalization. Cleanup failures do not replace the original fatal. Rejected executor submissions settle their published future and cancel only their unaccepted registration, so shutdown cannot wait forever. Aggregate operation-change hooks receive the overlaid checkpoint snapshot before terminal waiters are published, so a resumed handler cannot advance past an observed fatal checkpoint hook. Operation registration, storage publication and notification retain their existing atomic locks. The same fatal guard applies to step, child-context and wait-for-condition handling. Legacy user-body failures retain their prior semantics. Ordinary application ExecutionException values keep their own type, message, cause chain, and end-hook identity; deep completion/future unwrapping is used only to recognize fatal causes.Root handler context uses explicit
@HandlerScopedmetadata naming a JDK Function opener. Bundled openers call private SDK code, preserving the minor correction's avoidance of coincidentally named application methods. Scopes close on their owner thread in reverse order. Worker and finalization scopes restore their previous MDC, including input or later factory/start failures before the normal handler logger scope opens; later startup plugins still observe the invocation trace. The no-plugin path retains its existing behavior. Optional cleanup waiting remains bounded to500ms, after reserving5000ms per configured plugin plus1000ms for shutdown/response. This is best effort; existing callbacks and resource draining remain potentially unbounded. Finalization atomically selects one caller/end-hook outcome. Known plugin fatals finalize once as RETRYING and retain their original caller identity. A scope fatal first observed after that boundary still escapes its actual owner thread; its duplicate shutdown copy cannot rewrite an end snapshot already delivered. Other shutdown errors remain visible, and cleanup wrappers are retained through try-with-resources to avoid self-suppression.End dispatch installs its completion observer before waiting, preserving legacy completion-thread behavior. One ordered finalizer handles races between owner cleanup and the existing bounded caller handoff without adding a wait or public API.
Invocation-local trace headers are captured on the runtime thread and stored in
InvocationInfo.xRayTraceId(). The same object reachescreatePlugin(info)andonInvocationStart(info); bundled and custom extractors receiveextract(info)before span creation. An actual accessor returning null/empty, or failing nonfatally, yields authoritative empty context. An unavailable accessor or the neutral Lambda Context 1.4 default leaves null and preserves legacy fallback. The no-plugin path does not read the optional carrier. Old constructors and compiled accessors remain usable; the eight-component record carries the same accepted source-pattern, reflection and value-semantics boundary as #765.Factory metadata enforces one bundled OTel view before construction, configuration copying avoids rediscovery, and synthetic roots retain deterministic identity/first-invocation export and consumed sampling overrides. Handlers21–24 use the public factory API and shared conformance revision98b802cdb172614f98e217f7464784d47b9bb484. Model-blocked outbound propagation draft #768 remains separate.
Insight retains fail-open handling for ordinary plugin failures, while propagating VirtualMachineError and ThreadDeath through JSON transforms, lazy metadata access, async export, draining and flushing. Queued work and waiting callers observe the recorded fatal even if another exporter is blocked; already-running arbitrary exporter code is not forcibly interrupted.
Cloud E2E and core conformance workflows use
queue: maxwith their existing shared concurrency groups andcancel-in-progress: false. This preserves existing waiting runs when this branch enqueues and keeps deployments to the shared CloudFormation stacks serial. Other branches using the older default single-pending policy can still cancel the group's pending runs; runs cancelled before starting are requeued until those branches adopt the shared queue policy.Demo/Screenshots
Not applicable (SDK lifecycle and tracing).
Checklist
Testing
mvn -o -B clean verify: 2,595 reported tests, 2,562 passed and 33 skipped (cloud-only and Java-version-specific cases), zero failures/errors.extract(info)implementations run before span creation. They reproduce eight failures when the field is present but the prior dispatch path is retained. Corretto 25 passes all three constructor/record-pattern compatibility cases using Java 21 bytecode and source fixtures.git diff --check, Javadocs, and the configured local package checks pass. Current-head GitHub CI and AI review are tracked separately while the published revision runs.Minor compatibility evidence remains in its owning PRs: old/current core/plugin layer combinations, precompiled legacy subclasses/default interfaces, and their documented source-compatibility boundaries. PR #765 separately records the accepted InvocationInfo record-component exception. This major intentionally requires factory migration; old view subclasses are rejected by its already-final view classes, and migration controls assert that explicit boundary.
CI retains pending runs with
queue: maxandcancel-in-progress: false. OpenTelemetry callers share theotel-conformance-testsgroup, so one caller runs at a time; existing test matrices, deployment resources, workflow pins, and test requirements remain unchanged.