Repository navigation
fix(sdk): bound invocation lifetime and checkpoint admission - #1014
zhongkechen wants to merge 39 commits into
Conversation
|
Merged current main Final validation: 1,387 Core tests, 1,348 Testing tests (three existing skips), and all 640 OTel tests pass, along with builds, full Core/OTel types and repository lint. The corrected #928 candidate's 14 local lifecycle/manager assertions also pass unchanged when composed with this implementation. One unchanged external-completion case observed an extra resume during the first full rerun; its unchanged 26-test suite and a subsequent complete 640-test run both pass. No assertion was loosened for that observation. Deployed LMI deadline/recovery/overlap acceptance remains outstanding. This does not cancel arbitrary application I/O, interrupt synchronous JavaScript, or guarantee reclamation from custom transports that ignore the invocation signal. The broader #927 requirements remain open; no merge/readiness/release action was taken. |
|
The recurring extra-invocation failure was traced to a real LocalDurableTestRunner race: after receiving a terminal handler response, invocation tracking became inactive before asynchronous history recording finished. A queued callback wakeup could start another invocation in that gap. The Testing SDK now closes invocation admission before terminal response/error recording or terminal execution-state publication, and ignores queued wakeups after settlement. It still collects final notifications; normal PENDING scheduling, step retries, and existing thrown-error rejection behavior are preserved. Three deterministic regressions fail against the prior implementation for successful, failed, and throwing handlers, and pass with this fix. Full Testing SDK suites pass across the four affected PR variants (1,378 on the major factory branch; 1,351 on the minor/lifecycle/test-harness branches; three existing skips each), with types and lint passing. The original public OTel one-invocation, side-effect, metadata, and replay assertions are unchanged: all 30 focused cases pass on the rebuilt variants, and the complete carrier OTel suite passes 674 tests. This change affects local testing orchestration. Production Core lifecycle/compatibility scope, Draft states, merge holds, and deployed acceptance requirements remain unchanged. Fresh CI is running on the published follow-up heads. |
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.
| const checkFunc = () => | ||
| runInvocationCallback(context, () => | ||
| check(currentState, waitForConditionContext), | ||
| ); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_bt4sq7prdfufxispwg6q3hyul7
[P2] Fence condition-state serialization continuations
This fence ends when check settles. If the subsequent serdes.serialize remains pending past the deadline, releasing it still calls serdes.deserialize and then config.waitStrategy in the disposed invocation. The outer DurablePromise fence suppresses eventual settlement but does not prevent these callbacks; external-storage deserializers can therefore start fresh I/O after timeout and overlap a retry. Fence the serdes continuations and strategy dispatch with the owning lifecycle, leaving them suspended after closure. Add a regression that holds the post-check serializer across expiry and verifies that neither callback starts.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
Codex AI reviewOne P2 finding at Reviewed commit |
Review of
|
|
Thanks for the detailed work on this. The problem in #927 is real. I am asking to close this PR and replace it with three smaller changes. The reasons follow. Why not continue with this PR
What replaces itMost of #927 does not need SDK code. Function authors, plugin writers and transport writers can avoid it by following written rules. Two parts are SDK bugs, and no guideline covers them. Each one is a small change at its cause.
These three do not cover branches left running after an early @zhongkechen, could you close this PR? Pick up #1101 or #1102 if you want them. The tests you wrote here for those two cases are a good starting point. |
### Issue Link, if available Prepare the coordinated major release from merged #924. ### Description This PR prepares the release `sdk-3.0.0 / otel-2.0.0 / test-1.3.2`. It adds: - [Release notes](https://github.com/aws/aws-durable-execution-sdk-js/blob/release/js-core3-otel2-20261021/release-notes/sdk-3.0.0.md) with per-package changes, each cited by merge commit. They cover `main` through `9de0c7f2`. - A [2.x-to-3.0 migration guide](https://github.com/aws/aws-durable-execution-sdk-js/blob/release/js-core3-otel2-20261021/docs/migration-2.x-to-3.0.md). - A change summary under Unreleased in `CHANGELOG.md`. - The release decisions in `RELEASING.md`. - Version bumps for three packages that npm already has. This PR changes no runtime code and no publishing workflow. #### Package versions npm state checked on 2026-10-25 with `npm view`. The publisher decision comes from running `decide-dist-tag.js` against that state. | Package | Version | npm today | Publisher decision | | --- | --- | --- | --- | | `@aws/durable-execution-sdk-js` | `3.0.0` | `latest` is `2.7.1` | `latest` | | `@aws/durable-execution-sdk-js-otel` | `2.0.0` | `latest` is `1.2.1` | `latest` | | `@aws/durable-execution-sdk-js-testing` | `1.3.2` | `latest` is `1.3.1` | `latest` | | `@aws/durable-execution-sdk-js-eslint-plugin` | `1.2.0` (bumped here) | `latest` is `1.1.1` | `latest` | | `@aws/durable-execution-sdk-js-telemetry-exporter` | `0.1.0-alpha.1` (bumped here) | `latest` is `0.1.0-alpha.0` | `beta` | | `@aws/durable-execution-sdk-js-telemetry-mcp` | `0.1.0-alpha.1` (bumped here) | `latest` is `0.1.0-alpha.0` | `beta` | Core, OTel and Testing versions come from #924. #### Why three packages are bumped 1. The publisher skips a version that npm already has. 2. Telemetry Exporter and MCP `0.1.0-alpha.0` were published on 2026-10-22, outside the release workflow. npm records their `gitHead` as `6597e075`, the #924 merge commit. 3. So the 3 exporter fixes and 3 MCP fixes merged after `6597e075` need a new version. This PR uses `0.1.0-alpha.1`. It does not take the `1.0.0` bump from #1030. 4. ESLint plugin `1.1.1` was published on 2026-08-14. The new rule from `3cbb456c` and `e2f013fa` needs a new version. This PR uses the minor bump `1.2.0`. `0.0.2` was considered for the exporter and rejected. It sorts below `0.1.0-alpha.0`. So the publisher would put it on a `v0` tag, and `latest` would keep the version without the fixes. #### After publishing The manual publication left `latest` on `0.1.0-alpha.0` for both Telemetry packages. The publisher puts `0.1.0-alpha.1` on `beta` and does not move `latest`. So a maintainer must run: ```bash npm dist-tag add @aws/durable-execution-sdk-js-telemetry-exporter@0.1.0-alpha.1 latest npm dist-tag add @aws/durable-execution-sdk-js-telemetry-mcp@0.1.0-alpha.1 latest ``` The published exporter `0.1.0-alpha.0` requires Core `>=3.0.0 <4.0.0`. npm cannot satisfy that peer until Core 3 is published. #### Not published by this release Extras and MicroVM Worker stay excluded from automatic publishing. Their changes on `main` are not in this release. The Telemetry CDK stack is not in the npm package. Its fixes ship from the repository. #### Work not included | PR | Scope | State | | --- | --- | --- | | #1014 | Invocation admission, deadline and cleanup fixes | Open | | #1035 | Wrapper getter and thenable failure containment | Open. `4e4e9407` already covers a `wrap` hook that rejects on its own. The follow-up #1037 stays separate | | #928 | LMI regression harness and acceptance evidence | Open. Adds tests only | If any of these merges before the release commit is selected, update the release notes. Refresh the notes against the selected release commit before publishing the GitHub Release. ### Demo/Screenshots Not applicable. The release notes and migration guide are included for review. ### Checklist - [x] I have filled out every section of the PR template - [x] I have thoroughly tested this change ### Testing #### Unit Tests - ESLint plugin: 168 tests passed with version `1.2.0`. The plugin reads `meta.version` from `package.json`. - Telemetry MCP: `npm run build` passed. `dist/server.js` contains `0.1.0-alpha.1`. - `package-lock.json` changes only the three `version` fields. - Earlier: 102 plugin-loader and invocation-lifetime tests passed for the migration guide. #### Integration Tests The failed `integration-tests (24.x)` run on `5519e9f0` was `parent-context-in-step (cloud)`. It exceeded its 5 s Jest timeout against the cloud function. That commit changed only Markdown files, and the same job passes on `main`. This push runs it again. Earlier: the migration examples typechecked against built Core 3 / OTel 2 declarations, in ESM and CJS, with strict checking and `skipLibCheck: false`. No package publication or cloud acceptance is claimed. #### Examples - Merged `main` at `9de0c7f2` without conflicts. - Ran `decide-dist-tag.js` against live npm state for `0.0.2`, `0.1.0-alpha.1` and `0.1.0`. - Checked each release-note claim against the merge commits and source. Examples: the 3 ms cooldown constant, the `faas.max_memory` byte conversion, and `onOperationEnd` running through `fireAndForget`. - Relative links resolve. `git diff --check` passes. No prose semicolons remain. --------- Co-authored-by: Pooya Paridel <ParidelPooya@users.noreply.github.com>
The shared LocalDurableTestRunner fix landed in #1020. This PR targets
mainand includes the Core 3 / OTel 2 factory migration from #924.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue Link, if available
Related to #927. LMI acceptance coverage is in #928. This PR does not close #927.
Description
On Lambda Managed Instances, an invocation can continue after its timeout or after the handler returns. The SDK currently admits later durable work and can send checkpoints after disposal. This change gives each invocation its own deadline and admission state. Expiry interrupts it without checkpointing a business failure; normal completion closes admission while already queued checkpoints drain.
The deadline covers plugin loading and factory creation, execution-state pagination, handler work, checkpoint draining, and instrumentation waits. Native monotonic time and captured timers keep the local runner's virtual durable clock from consuming the invocation budget. Operation entry, deferred step/child/condition callbacks, plugin dispatch, awaited continuations, and durable result settlement share the owning invocation's fence. Disposal clears scheduled work; late responses cannot restart polling, publish state, or resume the closed invocation's user stack. Eager operation startup remains intact. When deadline disposal releases a checkpoint-drain waiter, both the normal-result path and the caught-error path recheck the deadline before converting the outcome. Result getters,
toJSONand custom error accessors cannot start after expiry; existing withdrawn-token/PENDING and unrecoverable-error decisions keep their priority.The SDK creates each plugin from the same invocation snapshot passed to Start, including the safe invocation-local X-Ray carrier. Factory creation and the resulting runner use the invocation's deadline. Built-in OTel instances remain separate across invocations while their provider environment is shared. An expired invocation dispatches End once to admitted plugins without waiting past the deadline; overlapping invocation B keeps its own spans live when A ends. The main branch's OTel implementation is unchanged.
Unfinished work reports RETRYING. A fully prepared terminal success or failure selected before expiry survives a later instrumentation-cleanup timeout, and remaining End hooks receive the same status, result and original error. Selection occurs only after serialization, final checkpoint/termination checks and a live deadline gate. Service-requested PENDING and acknowledged oversized results retain their existing precedence. Once an oversized result is acknowledged, deadline End includes the original handler result even if an operation-state hook is still pending; unacknowledged output is never exposed. Built-in OTel spans close before flushing.
DurableExecutionClientgains an optional third request-options argument carrying an invocation-ownedAbortSignal. The Lambda transport forwards it to individualsendcalls and checks it after lazy loading; it does not destroy a shared client. Existing one- and two-argument transports remain compatible. Handler/operation APIs, checkpoint formats and replay identities are preserved. Suspension keeps the original business stack suspended; replay preserves stored successes and failures.The SDK cannot preempt synchronous JavaScript, cancel arbitrary application I/O, undo an accepted request, or force a custom transport to honor the signal. Already-started side effects still require idempotency. Deadline export remains best effort.
Testing
Validation uses main
721217aeand the current reviewed candidate.The new public-wrapper regressions hold the terminal checkpoint of an awaited child context. Caught-error controls require no custom
messageornameaccessor after expiry, original-error identity in live FAILED End metadata, and the same FAILED wire error after a normal drain and replay. Both plugin and no-plugin modes reject on deadline without invoking the returned object'stoJSON, including after the late checkpoint responds. Positive controls drain normally, serialize once per invocation, and replay the saved child without rerunning its body. Existing acknowledged-output, service-requested PENDING, checkpoint failure and disposal controls retain their outcomes.The prior Core 2 composition of #1014 and #928 passed all 14 local lifecycle assertions and 12 deployed lifecycle cases in the closed validation PR #1044, with all 184 ledger records settled and no quarantine. That result is historical: fresh composed LMI acceptance is being prepared for this Core 3 integration. No new cloud result is claimed by these local checks.
Demo/Screenshots
Not applicable; public-wrapper, composed lifecycle and original example regressions exercise the behavior.
Checklist
The latest upstream sync includes main
721217ae, including the Extras MicroVM resume-before-delivery checkpoint fix. This review follow-up changes the Core runtime, so the composed LMI bundle and its local contracts must be rebuilt before fresh cloud acceptance. Earlier payload identity is not reused for this candidate.