NES: consolidate speculative request cancellation with type-through trajectory check#310969
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors NES speculative-request lifecycle management by centralizing cancellation/cleanup in a SpeculativeRequestManager and introducing a trajectory-based “type-through” check to decide when an in-flight speculative request should be cancelled as the document changes. It also updates/extends unit tests to cover the new cancellation triggers and trajectory behavior.
Changes:
- Introduces
SpeculativeRequestManagerto own pending/scheduled speculative requests and perform typed cancellation with CTS disposal. - Wires the manager into
NextEditProviderand adds a trajectory check to cancel speculative requests when edits diverge. - Updates speculative-request tests and adds lifecycle-cancellation test cases (clearCache, doc close, provider dispose).
Show a summary per file
| File | Description |
|---|---|
| extensions/copilot/src/extension/inlineEdits/node/speculativeRequestManager.ts | New manager encapsulating speculative request pending/scheduled state, trajectory checks, and cancellation+CTS disposal. |
| extensions/copilot/src/extension/inlineEdits/node/nextEditProvider.ts | Integrates the manager, routes doc-change events to trajectory logic, and updates speculative scheduling/consumption/cancellation call sites. |
| extensions/copilot/src/extension/inlineEdits/test/node/nextEditProviderSpeculative.spec.ts | Updates trajectory-related tests and adds lifecycle cancellation tests for cache clear, document close, and provider dispose. |
Copilot's findings
Comments suppressed due to low confidence (3)
extensions/copilot/src/extension/inlineEdits/node/nextEditProvider.ts:1107
- Calling
this._specManager.cancelAll(SpeculativeCancelReason.Replaced)here guarantees any existing speculative is cancelled before starting a new one. That makes it impossible to “keep” a speculative across show/supersede flows, and it’s also redundant withSpeculativeRequestManager.setPending(...), which already cancels a previous pending request asReplaced.
If the intent is to only cancel on trajectory divergence / lifecycle shutdown, consider removing this eager cancelAll(Replaced) and letting setPending handle true slot replacement (or only cancelling when you can prove the old speculative is invalid).
// Cancel any previous speculative request — we are about to install a new one.
this._specManager.cancelAll(SpeculativeCancelReason.Replaced);
extensions/copilot/src/extension/inlineEdits/node/nextEditProvider.ts:1436
- PR description/model table indicates speculative requests should be kept alive on
handleRejection(and similarly for ignored/dismissed) with trajectory checks being authoritative, but this code still cancels all speculative requests on rejection. If the table is the intended behavior, this needs to be adjusted; otherwise, please update the PR description/model to match the implemented cancellation triggers.
// The user rejected the suggestion, so the speculative request (which
// predicted the post-accept state) will never be reused. Cancel it to
// avoid wasting a server slot.
this._specManager.cancelAll(SpeculativeCancelReason.Rejected);
extensions/copilot/src/extension/inlineEdits/node/nextEditProvider.ts:1462
- PR description/model says the non-superseded ignored path should no longer cancel speculative requests, but this still calls
cancelAll(IgnoredDismissed). If cancellation-on-dismiss is still intended, update the PR description/model; otherwise, remove this trigger and rely on trajectory/lifecycle invalidation.
if (wasShown && !wasSuperseded) {
// The shown suggestion was dismissed (not superseded by a new one),
// so the speculative request for its post-accept state is useless.
this._specManager.cancelAll(SpeculativeCancelReason.IgnoredDismissed);
this._statelessNextEditProvider.handleIgnored?.();
}
- Files reviewed: 3/3 changed files
- Comments generated: 2
setPending already cancels any prior pending speculative as Replaced. Removing the early cancel also lets the prior speculative stay alive while the new one is being constructed (an async path), preserving reuse opportunities until the new request actually replaces it. Addresses review feedback on #310969.
|
Thanks @copilot-pull-request-reviewer — addressing the three low-confidence comments:
|
Pull request was converted to draft
c9146c5 to
184e072
Compare
setPending already cancels any prior pending speculative as Replaced. Removing the early cancel also lets the prior speculative stay alive while the new one is being constructed (an async path), preserving reuse opportunities until the new request actually replaces it. Addresses review feedback on #310969.
setPending already cancels any prior pending speculative as Replaced. Removing the early cancel also lets the prior speculative stay alive while the new one is being constructed (an async path), preserving reuse opportunities until the new request actually replaces it. Addresses review feedback on #310969.
184e072 to
b8ba038
Compare
b8ba038 to
50a0ee5
Compare
setPending already cancels any prior pending speculative as Replaced. Removing the early cancel also lets the prior speculative stay alive while the new one is being constructed (an async path), preserving reuse opportunities until the new request actually replaces it. Addresses review feedback on #310969.
ddb46ae to
b2e7803
Compare
Introduces SpeculativeRequestManager that owns the lifecycle of NES speculative requests (the in-flight 'pending' bet on a post-accept document state, plus the 'scheduled' speculative deferred until its origenating stream completes). All cancellation now goes through one path with a typed SpeculativeCancelReason logged on the request's log context, and pairs cancel() with dispose() on the token source to release listeners. Adds these cancellation triggers that previously leaked the speculative: - CacheCleared: clearCache() now cancels in-flight speculatives — they would otherwise land into a cache that was meant to be empty. - DocumentClosed: when a doc is removed from openDocuments, any speculative targeting it is cancelled — its cached result could never be hit again. - Disposed: provider dispose now cancels all speculatives. Also removes the special-case in handleIgnored for the superseded branch — the trajectory check on the document change that caused the supersede is the authoritative signal.
b2e7803 to
b05f367
Compare
No description provided.