fix: agent hang from leaked external-edit lock#326067
Draft
stevenh wants to merge 5 commits into
Draft
Conversation
Author
|
@microsoft-github-policy-service agree company="Rocket Science" |
1 similar comment
Author
|
@microsoft-github-policy-service agree company="Rocket Science" |
Contributor
There was a problem hiding this comment.
Pull request overview
Bounds external-edit acknowledgments to prevent write-permission hangs.
Changes:
- Adds cancellation and timeout handling.
- Bounds edit finalization waits.
- Adds regression tests for stalled acknowledgments.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
permissionHelpers.ts |
Forwards cancellation tokens. |
externalEditTracker.ts |
Adds bounded acknowledgment handling. |
externalEditTracker.spec.ts |
Tests timeout and cancellation paths. |
Author
|
While this currently prevents the infinite hang, it doesn't address the underlying cause, using it as a base to identify that. |
stevenh
changed the title
stevenh
changed the title
Granting a write permission was gated on an unbounded, un-cancellable wait for VS Code core to invoke the externalEdit proceed callback. When core dropped that acknowledgment, the write handler awaited forever, respondToPermission was never called, and the agent turn wedged mid-prompt with no user action (microsoft#320292). ExternalEditTracker.trackEdit now releases the permission gate on whichever happens first: core acknowledges, the request is cancelled, or a safety timeout elapses. completeEdit is likewise bounded so request finalization cannot block on a stalled acknowledgment. The write-permission handler now forwards its cancellation token into trackEdit so Stop unblocks a pending write. Both the Copilot CLI and Claude agents share this tracker. Fixes microsoft#320292
…ycle The cancellation listener was disposed together with the timeout the moment the permission gate settled (core acknowledged or the safety timeout elapsed). A request cancelled after that point but before completeEdit was then a no-op: the ongoing-edit map entry was never deleted and the deferred the externalEdit proceed callback awaits was never completed, so a cancelled tool that emits no completion event left both the callback and the map entry pending indefinitely. Only the timeout is now tied to the permission gate and cleared on settle. The cancellation listener lives for the whole edit lifecycle and is disposed either when it fires or by completeEdit, so cancellation always releases the edit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…trackEdit Add handler-level tests covering the auto-approval and confirmed write-permission paths, asserting that handleWritePermission forwards the request cancellation token (and tool call id / stream) into editTracker.trackEdit. The existing handler tests passed no tool call or stream and never asserted forwarded arguments, so dropping the token from a call site would have left the suite green. These tests fail if the token is not forwarded, guarding the fix for the write-permission hang. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Write permissions for agent tools are gated on VS Code core acknowledging the externalEdit; when that ack never arrives the turn stalls until the safety timeout fires (see microsoft#320292). Add bad-path diagnostics along the ack path so a future occurrence reveals the cause instead of only surfacing as a silent timeout: - ExternalEditTracker: log an error (with edit key, file count, waited ms) when the start ack times out, and a warning when completion is never confirmed. Inject an optional ILogger from the Copilot CLI and Claude sessions. - ChatEditingSession.startExternalEdits: add a watchdog that warns which phase (readiness gate, streaming-edit lock, or snapshotting) is stuck if the method does not resolve before the extension-side timeout. - mainThreadChatAgents2: warn when an externalEdits chunk is dropped (missing editing session/response), distinguishing a missing ack from a stalled one. These only emit on the failure path, so they add no happy-path noise while providing the breadcrumb that made this class of hang diagnosable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A single tool call in the Copilot CLI path calls trackEdit once per file with the same tool call id, but ExternalEditTracker keyed _ongoingEdits by edit key alone. The second file overwrote the first file's map entry, stranding its deferred: the first file's core externalEdit proceed callback never returned, so stopExternalEdits was never sent and core's per-resource streaming-edit lock leaked. Every subsequent write to that same file then hung forever in startExternalEdits (acquiring-locks), masked only by the 10s acknowledgment timeout (see microsoft#320292). Store a list of entries per edit key so each tracked file keeps its own deferred and acknowledgment. completeEdit now completes and awaits every file under the key, releasing all locks, and cancellation removes only the affected file's entry. Add a regression test proving a single edit key that tracks two files releases both edits on completeEdit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
stevenh
force-pushed
the
agents/fix-permission-issues-analysis
branch
from
e2b6e99 to
3148e15
Compare
Comment on lines
24
to
35
| /** | ||
| * Tracks ongoing external edit operations for agent tools. | ||
| * Manages the lifecycle of external edits by coordinating with VS Code's | ||
| * externalEdit API to ensure proper tracking and attribution of file changes. | ||
| */ | ||
| interface IOngoingEdit { | ||
| complete: () => void; | ||
| onDidComplete: Thenable<string>; | ||
| dispose: () => void; | ||
| } | ||
|
|
||
| export class ExternalEditTracker { |
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.
A single agent tool call that writes multiple files tracked all of them under one edit key, so later files overwrote earlier ones. The overwritten edit never acknowledged, VS Code core leaked that file's streaming-edit lock, and every later write to the same file hung the agent turn with no user action.
ExternalEditTracker now tracks each file separately per edit key and completes them all, so no lock is leaked. Waiting for a core acknowledgment is also bounded by cancellation and a safety timeout so a dropped ack can never wedge a turn, and bad-path diagnostics were added to make any future stall visible. Shared by the Copilot CLI and Claude agents.
Fixes #320292