fix(vscode-ide-companion): improve ACP error handling to prevent silent loading hangs#2546
Open
yiliang114 wants to merge 1 commit intomainfrom
Open
fix(vscode-ide-companion): improve ACP error handling to prevent silent loading hangs#2546yiliang114 wants to merge 1 commit intomainfrom
yiliang114 wants to merge 1 commit intomainfrom
Conversation
…nt loading hangs When the CLI process crashes during initialization (e.g. due to a malformed settings.json), the extension now surfaces the error instead of hanging indefinitely on "Preparing Qwen Code...": - Collect stderr output from the child process for inclusion in error messages - Race SDK initialize() against a process-exit promise so a crash rejects immediately rather than leaving a dangling await - Add a 30-second safety-net timeout in App.tsx to clear the loading spinner when initialization stalls for any reason Closes #2382 Made-with: Cursor
yiliang114
requested review from
DennisYu07,
DragonnZhang,
LaZzyMan,
Mingholy,
gwinthis,
pomelo-nwu and
tanzhenxin
as code owners
Contributor
📋 Review SummaryThis PR addresses a critical UX issue where the VS Code extension hangs indefinitely on "Preparing Qwen Code..." when the ACP CLI process crashes during initialization. The implementation introduces a three-layer defense strategy: stderr collection, Promise.race against process exit, and a safety-net timeout. The changes are well-targeted and solve the stated problem effectively. 🔍 General Feedback
🎯 Specific Feedback🟡 High
let rejectOnExit: ((error: Error) => void) | null = null;
const processExitPromise = new Promise<never>((_resolve, reject) => {
rejectOnExit = reject;
});
// Later in exit handler:
if (rejectOnExit) {
const handler = rejectOnExit;
rejectOnExit = null; // Prevent duplicate calls
handler(new Error(...));
}
useEffect(() => {
if (isAuthenticated !== null) {
setIsLoading(false);
return;
}
const timeout = setTimeout(() => {
setIsLoading(false);
}, 30_000);
return () => clearTimeout(timeout);
}, [isAuthenticated]);This is actually already implemented correctly with the early return! However, consider adding a comment noting that the timeout is intentionally not cleared on unmount since we want it to fire even if the component unmounts during the loading state. 🟢 Medium
const buildStderrSuffix = (chunks: string[]): string => {
const stderrOutput = chunks.join('').trim();
return stderrOutput ? `\nCLI stderr: ${stderrOutput.slice(-500)}` : '';
};
this.child!.stderr?.on('data', (data: Buffer) => {
const message = data.toString();
if (message.trim()) { // Only log non-empty messages
if (
message.toLowerCase().includes('error') &&
!message.includes('Loaded cached')
) {
console.error(`[ACP qwen]:`, message);
} else {
console.log(`[ACP qwen]:`, message);
}
}
});🔵 Low
✅ Highlights
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
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
1 participant
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.
TLDR
Fix the VS Code extension silently hanging on "Preparing Qwen Code..." when the ACP CLI process crashes during initialization (e.g. due to a malformed
settings.json). The extension now surfaces the actual error instead of leaving users stuck.Dive Deeper
Root cause:
When the CLI child process crashes during initialization (for example, a syntax error in
.qwen/settings.jsoncauses MCP initialization to fail), theinitialize()SDK call never resolves or rejects. This leaves the extension'sawaithanging indefinitely, and the UI displays the loading spinner forever with no error feedback.Fix — three layers of defense:
Collect stderr output: The child process's stderr handler now accumulates all output chunks. When an error occurs, the last 500 characters of stderr are appended to the error message, giving users actionable diagnostic information.
Promise.race to prevent hanging: A
processExitPromiseis created that rejects when the child process exits. Theinitialize()call is raced against this promise, so if the CLI crashes before responding, the await rejects immediately with a descriptive error (including stderr) instead of hanging forever.Safety-net timeout in App.tsx: A 30-second timeout clears the loading spinner if initialization stalls for any reason, allowing users to see the onboarding/error UI instead of being stuck on the loading screen indefinitely.
Changed files:
src/services/acpConnection.ts— stderr collection,processExitPromise,Promise.raceforinitialize()src/webview/App.tsx— 30-second safety-net timeout for loading stateReviewer Test Plan
.qwen/settings.json(e.g. trailing comma in JSON), open the extension, and verify:npm run testto verify all unit tests passTesting Matrix
Linked issues / bugs
Closes #2382
Made with Cursor