Auto-resolve promises in TypeScript codegen to prevent silent RPC drops#15901
Auto-resolve promises in TypeScript codegen to prevent silent RPC drops#15901
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15901Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15901" |
There was a problem hiding this comment.
Pull request overview
This PR updates the TypeScript ATS codegen/runtime to ensure fluent RPC calls execute even when users don’t await fluent chains, addressing silent drops in publish/deploy scenarios (fixes #15899).
Changes:
- Widen generated handle-typed parameters to accept
PromiseLike<T>and auto-resolve promise-like inputs before RPC argument marshalling. - Add client-side tracking of pending fluent-call promises and flush them before
build()proceeds. - Update TypeScript codegen snapshots/tests and refresh the TypeScript AppHost playground to demonstrate “no-await” chaining.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.CodeGeneration.TypeScript.Tests/Snapshots/transport.verified.ts | Snapshot updates for isPromiseLike export + promise tracking/flush on the client. |
| tests/Aspire.Hosting.CodeGeneration.TypeScript.Tests/Snapshots/AtsGeneratedAspire.verified.ts | Snapshot updates reflecting widened types, promise resolution, and PromiseImpl client threading. |
| tests/Aspire.Hosting.CodeGeneration.TypeScript.Tests/AtsTypeScriptCodeGeneratorTests.cs | Adjust assertions to expect PromiseLike<...> in generated signatures. |
| src/Aspire.Hosting.CodeGeneration.TypeScript/Resources/transport.ts | Implements trackPromise() / flushPendingPromises() and exports isPromiseLike(). |
| src/Aspire.Hosting.CodeGeneration.TypeScript/AtsTypeScriptCodeGenerator.cs | Core generator updates: widen inputs, resolve promises before RPC, track promises, flush before build. |
| playground/TypeScriptAppHost/vite-frontend/package-lock.json | Updates lockfile metadata (engines/peer flags). |
| playground/TypeScriptAppHost/aspire.config.json | Sets sdk version/channel for the playground. |
| playground/TypeScriptAppHost/apphost.ts | Updates sample to rely on promise-friendly chaining and build-time flushing. |
Copilot's findings
Files not reviewed (1)
- playground/TypeScriptAppHost/vite-frontend/package-lock.json: Language not supported
- Files reviewed: 5/8 changed files
- Comments generated: 3
src/Aspire.Hosting.CodeGeneration.TypeScript/AtsTypeScriptCodeGenerator.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.CodeGeneration.TypeScript/AtsTypeScriptCodeGenerator.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.CodeGeneration.TypeScript/AtsTypeScriptCodeGenerator.cs
Show resolved
Hide resolved
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
3 issues found (1 deadlock bug, 1 stale snapshot, 1 potential unhandled rejection).
src/Aspire.Hosting.CodeGeneration.TypeScript/AtsTypeScriptCodeGenerator.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.CodeGeneration.TypeScript/AtsTypeScriptCodeGenerator.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.CodeGeneration.TypeScript/Resources/transport.ts
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.CodeGeneration.TypeScript/Resources/transport.ts
Outdated
Show resolved
Hide resolved
davidfowl
force-pushed
the
davidfowl/auto-resolve-promises
branch
from
45a9a68 to
00b2f0a
Compare
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
2 issues found:\n- 1 bug — flushPendingPromises() while loop deadlocks when the build promise is tracked after flush begins (transport.ts)\n- 1 test gap — E2E test validates type-checking only, not runtime flush behavior (TypeScriptCodegenValidationTests.cs)
davidfowl
force-pushed
the
davidfowl/auto-resolve-promises
branch
5 times, most recently
from
b53b0d1 to
51198db
Compare
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
1 issue found (bug). Duplicate error recording in trackPromise/flushPendingPromises — rejected promise errors are pushed into _rejectedErrors from both the trackPromise handler and the allSettled loop.
| private _rejectedErrors: unknown[] = []; | ||
| throwOnPendingRejections = true; | ||
|
|
||
| constructor(private socketPath: string) { } | ||
|
|
||
| trackPromise(promise: Promise<unknown>): void { | ||
| this._pendingPromises.add(promise); | ||
| // Remove on both resolve and reject. The reject handler swallows the | ||
| // error to prevent Node.js unhandled-rejection crashes. | ||
| // | ||
| // When throwOnPendingRejections is true (default), rejection errors are | ||
| // eagerly collected so that flushPendingPromises can re-throw them even | ||
| // if the promise settles before flush is called. | ||
| // | ||
| // Limitation: JavaScript provides no way to detect whether a rejection | ||
| // was already observed by the caller (e.g., try { await p } catch {}). | ||
| // The .then() reject handler fires regardless. This means user-caught | ||
| // rejections will also be re-thrown by build(). We accept this tradeoff | ||
| // because the common case — an un-awaited chain fails silently — should | ||
| // fail loud. The uncommon case (catch an error from an un-awaited chain, | ||
| // then continue to build) can opt out with: | ||
| // createBuilder({ throwOnPendingRejections: false }) | ||
| promise.then( | ||
| () => this._pendingPromises.delete(promise), | ||
| (err) => { | ||
| this._pendingPromises.delete(promise); | ||
| if (this.throwOnPendingRejections) { | ||
| this._rejectedErrors.push(err); | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
Bug: Duplicate error recording for promises that reject during flush
When a tracked promise rejects during the await Promise.allSettled(pending) call, its error is pushed into _rejectedErrors twice:
- By
trackPromise's rejection handler (line 769):this._rejectedErrors.push(err) - By the
allSettledloop (line 782):this._rejectedErrors.push(result.reason)
Both fire because the .then(_, onRejected) handler in trackPromise runs as a microtask when the promise settles, and allSettled independently records the same rejection in its results array. The AggregateError thrown at the end will contain duplicate entries.
Fix direction: Remove the allSettled rejection-collection loop entirely — trackPromise's rejection handler already captures errors for both pre-flush and during-flush cases. Or alternatively, stop collecting in trackPromise's handler and rely solely on allSettled:
trackPromise(promise: Promise<unknown>): void {
this._pendingPromises.add(promise);
promise.then(
() => this._pendingPromises.delete(promise),
() => this._pendingPromises.delete(promise) // just remove, don't collect
);
}There was a problem hiding this comment.
GPT 5.4 just found this as well.
davidfowl
force-pushed
the
davidfowl/auto-resolve-promises
branch
5 times, most recently
from
e30b628 to
7319b95
Compare
|
🎬 CLI E2E Test Recordings — 56 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24226007414 |
When users write un-awaited fluent chains like:
const db = builder.addPostgres('pg').addDatabase('db');
builder.addContainer('c', 'nginx').withReference(db);
await builder.build().run();
the generated TypeScript now accepts PromiseLike<T> for handle parameters
(via Awaitable<T>) and automatically resolves them before RPC calls.
build() flushes all pending un-awaited promises before proceeding.
Key design decisions in transport.ts:
- trackPromise: .then(resolve, reject) removes from set on settlement.
Reject handler swallows errors to prevent Node.js unhandled-rejection
crashes. Errors are NOT eagerly collected — only promises still pending
at flush time are observed, so user-caught rejections are not re-thrown.
- flushPendingPromises: snapshots the pending set before awaiting to
prevent deadlock. The build promise is tracked by the PromiseImpl
constructor while flush is suspended — without the snapshot, a while-loop
re-check would find and await it, causing a circular await.
- Promise.allSettled collects all errors as AggregateError.
Testing:
- 8 vitest unit tests for trackPromise/flushPendingPromises covering
deadlock prevention, error aggregation, and user-caught rejection safety
- E2E test with aspire start/stop validating runtime behavior
- 73 snapshot tests for generated code shape
Fixes #15899
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
davidfowl
force-pushed
the
davidfowl/auto-resolve-promises
branch
from
7319b95 to
e61bf76
Compare
Description
Auto-resolve promises in the TypeScript code generator to prevent un-awaited fluent chains from silently dropping RPC calls.
The Bug: In TypeScript apphosts, every fluent method returns a
*PromiseImplwrapping an async RPC call. If users don'tawaitthe chain, the RPC calls never execute beforebuild().run()completes. This causes env vars, annotations, etc. to silently vanish — particularly in publish/deploy mode. Works fine in dev mode, fails silently in publish.Three changes:
Widen parameter types — handle-type params now also accept
PromiseLike<T>, so users don't need toawaita resource before passing it as input towithReference,waitFor,publishAsStaticWebsite({ apiTarget }), etc.Auto-resolve promises before RPC — all generated internal async methods resolve any promise-like handle params before building
rpcArgs, using the existingisPromiseLike()helper. Applied consistently across builder methods, type class methods, context methods, wrapper methods, and property setters.Track & flush pending promises — mutating fluent calls register their promise with the client via
trackPromise().build()callsflushPendingPromises()before proceeding, ensuring all un-awaited chains complete.Result:
awaitor don'tawait— both work correctly. Users only needawaitwhen extracting a value (likegetEndpoint()).Fixes #15899
Checklist