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" |
src/Aspire.Hosting.CodeGeneration.TypeScript/Resources/transport.ts
Outdated
Show resolved
Hide resolved
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).
| // Flush pending promises before build to ensure all un-awaited chains complete | ||
| if (string.Equals(capability.MethodName, "build", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| WriteLine(" await this._client.flushPendingPromises();"); |
There was a problem hiding this comment.
Deadlock: flushPendingPromises() is inside _buildInternal, not the public build() wrapper
The earlier review thread identified this deadlock and the fix was described, but it was not applied. flushPendingPromises() is still emitted inside _buildInternal():
build()calls_buildInternal(), which suspends atawait flushPendingPromises()- The resulting Promise is passed to
new PromiseImpl(buildPromise, client) - The constructor calls
client.trackPromise(buildPromise)— the build promise is now in_pendingPromises flushPendingPromises()re-checks the set in itswhileloop, finds the build promise, andawaits it- The build promise is waiting for
flushPendingPromises()to return → circular await → hangs forever
The fix described in the review (async closure in the public build() wrapper) needs to be applied — move the flush before _buildInternal() is called, e.g.:
build(): DistributedApplicationPromise {
const flushAndBuild = async () => {
await this._client.flushPendingPromises();
return this._buildInternal();
};
return new DistributedApplicationPromiseImpl(flushAndBuild(), this._client);
}| /// <code> | ||
| /// /** @internal */ | ||
| /// private async _withEnvironmentInternal(name: string, value: string): Promise<RedisResource> { | ||
| /// await this._client.flushPendingPromises(); // only emitted for build() |
There was a problem hiding this comment.
Stale snapshot files
The snapshot TwoPassScanningGeneratedAspire.verified.ts does not contain flushPendingPromises anywhere. The generated _buildInternal() at its line 4048 is missing the flush call. This means the snapshots were accepted before the flush code was added (or the test models don't include a build capability with that exact MethodName). Either way, the snapshots and the code generator are out of sync — running dotnet test will likely fail with a snapshot mismatch once the deadlock fix is applied to the correct location.
|
|
||
| trackPromise(promise: Promise<unknown>): void { | ||
| this._pendingPromises.add(promise); | ||
| promise.finally(() => this._pendingPromises.delete(promise)); |
There was a problem hiding this comment.
Potential unhandled promise rejection
The return value of promise.finally(...) is discarded. If the tracked promise rejects, .finally() re-throws that rejection into a new derived promise that nobody observes. In Node.js (since v15), unhandled rejections crash the process by default (--unhandled-rejections=throw).
flushPendingPromises() observes the original promise via Promise.all, but the derived promise from .finally() is a separate chain.
Consider suppressing the secondary rejection:
trackPromise(promise: Promise<unknown>): void {
this._pendingPromises.add(promise);
promise.finally(() => this._pendingPromises.delete(promise)).catch(() => {});
}Or use void promise.then(...) with both paths to avoid creating an unobserved chain.
| while (this._pendingPromises.size > 0) { | ||
| await Promise.all(this._pendingPromises); | ||
| } |
There was a problem hiding this comment.
Is this loop so new promises added while adding existing promises are awaited?
Add a quick comment to explain. Also say awaiting pending promoses removes them from the collection
| promise.finally(() => this._pendingPromises.delete(promise)); | ||
| } | ||
|
|
||
| async flushPendingPromises(): Promise<void> { |
There was a problem hiding this comment.
How much logging do we do in TS client code? Should we log if there are pending promises? It seems like having them is a possible mistake that user/agent should clean up.
Three changes to make un-awaited fluent chains work correctly: 1. Widen parameter types to accept PromiseLike<T> for handle-type inputs 2. Auto-resolve promise-like params before RPC calls in all internal methods 3. Track pending mutating promises and flush them in build() This ensures that both awaited and un-awaited fluent chains produce correct results, preventing env vars and annotations from being silently dropped in publish/deploy mode. Fixes #15899 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove unnecessary awaits on Redis and Vite frontend chains to demonstrate that build() now flushes pending promises automatically. Await is only needed when the resolved resource is used elsewhere. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Revert playground/TypeScriptAppHost/aspire.config.json changes - Use promise.finally() instead of .then(cleanup, cleanup) for cleaner promise tracking Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add GenerateResolveAndBuildArgs unified helper for resolve + arg building - Add GeneratePromiseResolutionForParam for property setters - Fix GenerateEntryPointFunction: add promise resolution (async IIFE for fluent path) - Use shared helpers in context/wrapper methods instead of inline logic - Target let/const: only use let for handle-type optional params Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix union type splitting: use structural AtsTypeRef approach instead of string splitting to avoid breaking PromiseLike<A | B> generics - Fix potential deadlock: move flushPendingPromises to public build() wrapper (before PromiseImpl tracking) instead of inside _buildInternal - Fix promise resolution predicate: rename IsHandleOrHandleUnionType to IsWidenedHandleType to match exactly which types are widened with PromiseLike - Fix var→const in playground apphost.ts Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add Awaitable<T> = T | PromiseLike<T> to base.ts. Generated signatures now use Awaitable<RedisResource> instead of the verbose RedisResource | PromiseLike<RedisResource> inline expansion. For unions: string | Awaitable<RedisResource | PostgresResource> instead of duplicating the full union inside PromiseLike<>. Also fixes union type construction to use structural AtsTypeRef approach instead of string splitting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Awaitable<T> type is used in generated method signatures but was only imported internally. Consumers importing types from aspire.js need it resolved for tsc --noEmit to pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e<T> IsWidenedHandleType now checks _wrapperClassNames to match the exact widening logic in MapInputTypeToTypeScript. This excludes special types like ReferenceExpression that are handle-category but not widened, preventing 'Type unknown is not assignable to ReferenceExpression' tsc errors in generated code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
RunWithMissingAwaitShowsHelpfulError → RunWithMissingAwaitSucceedsWithAutoResolve Now that withReference accepts PromiseLike<T>, un-awaited chains work correctly and the app starts successfully instead of showing an error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The e2e test was timing out because aspire run needs PostgreSQL/DCP which isn't available in this CI environment. Use tsc --noEmit instead to validate that un-awaited chains (withReference on PromiseLike<T>) compile without type errors — which is the actual behavior being tested. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…mments
- Move flushPendingPromises() from _buildInternal to public build() wrapper
using async closure pattern to prevent deadlock (JamesNK)
- Apply same fix in GenerateTypeClassMethod for type class build() paths
- Add .catch(() => {}) to promise.finally() in trackPromise to prevent
unhandled rejection crashes in Node.js (JamesNK)
- Add console.warn when flushing pending promises to surface implicit flushes
- Add explanatory comment on the while loop in flushPendingPromises (JamesNK)
- Update doc comment to reflect flush location change
- Update snapshots (TwoPassScanning, transport)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
davidfowl
force-pushed
the
davidfowl/auto-resolve-promises
branch
from
45a9a68 to
00b2f0a
Compare
|
🎬 CLI E2E Test Recordings — 56 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24004528901 |
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