Repository navigation
refactor(plugin): create one plugin instance per invocation - #737
Open
ParidelPooya wants to merge 38 commits into
Open
ParidelPooya wants to merge 38 commits into
ParidelPooya wants to merge 38 commits into
Conversation
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.
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
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.
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.
ParidelPooya
force-pushed
the
refactor/per-invocation-plugin-instances
branch
from
September 17, 2026 20:28
7bfddca to
67eabd0
Compare
This comment has been minimized.
This comment has been minimized.
yaythomas
requested changes
Sep 18, 2026
Contributor
There was a problem hiding this comment.
Thank you for the thoroughness here. The Barrier-based isolation test, the measured loss tables, and the BaseException reasoning for worker threads are all correct, and they are exactly the evidence a concurrency fix needs. One blocker in the log filter, one design proposal at the factory type, two items for the breaking-change note, and some nits.
Size. Commit 1 (the Insight scheduler fix) is independently correct and independently urgent, and it does not depend on the contract change.
This comment has been minimized.
This comment has been minimized.
This was referenced Oct 2, 2026
Preserve Core 3 and OTel 2 invocation factories while adopting upstream handler-worker lifecycle, registration validation, Telemetry naming, and Testing SDK output validation.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
Capture each drain target without extending earlier waiters. Preserve coalescing, worker recovery, and best-effort serialized exporter calls.
Run customer transforms outside invocation locks, reject stale records, and retain closing ownership through duplicate and reentrant End calls.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
Contributor
Codex AI reviewNo actionable findings in the PR changes. Residual test risk: tests were inspected but not executed under the read-only review constraints. Reviewed commit |
zhongkechen
requested a deployment
to
ai-pr-review-runtime
October 20, 2026 18:34 — with
GitHub Actions
In progress
This branch is being deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overlapping invocations on Lambda Managed Instances can overwrite shared plugin state and lose telemetry records. This PR moves Core 3.0 and OTel 2.0 to per-invocation plugin factories and makes the shared Telemetry exporter scheduler preserve each invocation's export and flush obligations. Supersedes #734.
Plugin contract and migration
pluginsaccepts objects withcreate_plugin(info). The host resolves the factory list once and creates a fresh executor and plugin instances for each invocation. Environment entry points name factory objects, and repeated registrations of the same factory are deduplicated by identity. The removed v1 provider contract, plugin instances and bare callables are rejected at registration.The bundled factories are
InvocationOtelPluginFactory,ExecutionOtelPluginFactory, and the factory returned bydurable_execution_telemetry. OTel and Telemetry packages require Core>=3.0.0,<4. The renamed Python Telemetry exporter retains version0.0.1.Ordinary exceptions from factory calls and validation of their returned objects are contained so healthy plugins and handler code can continue. Thread-control exceptions retain their existing propagation and exception-group handling.
Factory registration preserves upstream view exclusion across explicit and environment configuration, including inherited opt-in groups and supported factory class shapes. Rejected configurations notify opted-in factories once so constructor-owned resources can be released, including when another explicit entry is invalid.
Invocation isolation and lifecycle
Start, the handler and End run in an invocation-owned worker context. Successful Start bindings reach handler code and SDK-managed callbacks; failed Start bindings are rolled back without changing the host context. Completed durable work still replays from checkpoints. End reaches every started plugin, and an End failure does not replace the handler's original failure. Service-requested PENDING remains authoritative after checkpoint-token withdrawal.
OTel state belongs to each plugin instance. The shared log filter resolves a live invocation claim for the emitting thread or task. Logs outside an active invocation retain their original fields, including constructor-time logs with an ambient span. Fallback root export and stable trace identity from upstream are preserved.
Breaking changes: migrate registrations to factory objects and upgrade Core and OTel together. Instrumented handlers inherit host context bindings, which can change ContextVar visibility and the parent of a customer span created at handler entry. Installed-wheel tests cover the coordinated pair and explicit rejection of released OTel 1.x registrations.
Telemetry export and concurrency
One shared worker serializes exporter calls. Pending records coalesce only within the same execution, and a drain retains its own execution's record obligation. Worker recovery, reentrant drain handling and deferred release of displaced records preserve progress across exporter failures and customer finalizers.
Customer content transforms and result overrides run outside the plugin lock. Immutable snapshots and revision checks prevent a stale callback result from overwriting a newer or terminal record. Terminal admission closes before finalization, and obsolete customer objects are released outside both plugin and scheduler locks.
Each drain also captures a finite snapshot of globally scheduled work. A queued or in-flight flush may be shared when it covers that snapshot; otherwise a later flush is queued. Earlier targets stay fixed while new records arrive. A coalesced replacement discharges the original queue slot's obligation, and reschedules after the worker takes that slot do not extend an earlier drain.
Exporter failures remain logged and isolated. Drain coverage describes work offered to exporters and the covering flush attempt; it does not certify delivery to the destination.
Validation
Local validation covers the exporter changes at
e10b56dc(tree8e1ba4a8) and the factory-result containment fix ate447bc50(tree7b5b2c71):e447bc50passed every GitHub check and workflow, including Python 3.11–3.14 builds and cloud tests.The latest upstream integration at
214c309c(tree62c3a0ef) includes main2b408f44and its chained-invoke trace propagation. The three import conflicts are resolved, the incoming propagation tests use invocation factories and fresh instances on replay, and the compatibility notes match Core 3 / OTel 2. Independent review found no merge-specific issues.Validation of this integration on Python 3.13:
.github/scripts/tests/test_lintcommit.py::test_subject_uppercase: the test expects lowercase-only subjects, while the linter accepts uppercase. The linter and test are byte-identical on main, the prior PR head, and this merge; isolated script tests reproduce the same failure. All tests selected by the CI script-test workflow pass in these runs.No package release or manual AWS deployment was performed. Current-head GitHub checks and independent approval remain release gates.