"Developer: Reload Window" causes issues for long running tasks

[markw65]

Does this issue occur when all extensions are disabled?: Yes

• VS Code Version: 1.77.3
• OS Version: MacOS 13.3.1

Steps to Reproduce:

Run the following commands

mkdir test
cd test
cat <<"EOF" > package.json
{
    "scripts": {
        "watch": "while true; do sleep 1; echo \"$$ $(date)\"; done"
    }
}
EOF
code .

Trust the workspace, then start the script from the "NPM Scripts" panel. The script runs, and the name of the script appears to the right of the output panel, together with a spinner showing that it's running.

Now run the script again. A popup window appears saying that it's already running, and do you want to terminate or restart it (just close it without doing either).

This is all expected behavior.

Now (with the task running) run Cmd-Shift-P "Developer: Reload Window". The window reloads, and the script continues to run (you can tell it's the same script because it's printing its pid, and it doesn't change). The name is still in the right hand pane, but now the spinner has gone, as if vscode no longer knows that it's running.

Now run the script again, and although it's already running, it starts a second copy. This is the bug. It's especially painful for "runOnOpen" tasks, because every time you reload the window (eg because an extension needs updating) they all continue to run AND a new copy of each one starts.

I thought there might be another bug in the recently added AbstractTaskService._inProgressTasks (since that was supposed to fix exactly this issue - #159640), and took a look at that. There does appear to be a pretty serious bug in that code:

vscode/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts

Lines 1835 to 1858 in e9c1114

	if (this._inProgressTasks.has(qualifiedLabel)) {
	this._logService.info('Prevented duplicate task from running', qualifiedLabel);
	return { exitCode: 0 };
	}
	this._inProgressTasks.add(qualifiedLabel);
	if (await this._saveBeforeRun()) {
	await this._configurationService.reloadConfiguration();
	await this._updateWorkspaceTasks();
	const taskFolder = task.getWorkspaceFolder();
	const taskIdentifier = task.configurationProperties.identifier;
	const taskType = CustomTask.is(task) ? task.customizes()?.type : (ContributedTask.is(task) ? task.type : undefined);
	// Since we save before running tasks, the task may have changed as part of the save.
	// However, if the TaskRunSource is not User, then we shouldn't try to fetch the task again
	// since this can cause a new'd task to get overwritten with a provided task.
	taskToRun = ((taskFolder && taskIdentifier && (runSource === TaskRunSource.User))
	? await this.getTask(taskFolder, taskIdentifier, false, taskType) : task) ?? task;
	}
	await ProblemMatcherRegistry.onReady();
	const executeResult = runSource === TaskRunSource.Reconnect ? this._getTaskSystem().reconnect(taskToRun, resolver) : this._getTaskSystem().run(taskToRun, resolver);
	if (executeResult) {
	this._inProgressTasks.delete(qualifiedLabel);
	return this._handleExecuteResult(executeResult, runSource);
	}
	this._inProgressTasks.delete(qualifiedLabel);

It adds qualifiedLabel to the _inProgressTasks at line 1839, then starts the task at line 1853, and then removes qualifiedLabel at line 1855 without waiting for the task to finish. So in practice the list is almost always empty, and doesn't actually do anything. I think the delete at line 1855 should actually be in a .finally clause attached to the _handleExecuteResult(...) on line 1856, so that it doesn't run until the task completes.

But fixing that doesn't actually fix this issue, because the set isn't preserved (or reconstructed) across reloads. In addition it breaks the above behavior where trying to run an already running task asks what you want to do with it (because now that inProgressTasks is actually working, it aborts the attempt to re-run the task before it gets to the point where that dialog is presented). It also breaks tasks that are explicitly marked as allowing more than one instance.

So since it isn't currently doing anything, and fixing it breaks things, I would suggest removing all the associated code (@meganrogge)

I'm not sure what the actual fix should be though. There seems to be code to try to reconnect tasks to terminals:

vscode/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts

Lines 340 to 341 in e9c1114

	if (this._terminalService.getReconnectedTerminals('Task')?.length) {
	this._attemptTaskReconnection();

but it doesn't seem to work (I've stepped through the code with my example, and it goes into _attemptTaskReconnection but doesn't actually find any tasks to reconnect). So maybe the bug is there. Or maybe the bug is that the tasks shouldn't be preserved across reload (many are not; a terminal that isn't running anything will be killed by reload, for example).

But I've reached the limits of what I can debug, unless anyone has suggestions for where to look.

[meganrogge]

thanks for looking into this. the reason it's not in a finally is this was meant to (and did) fix an issue that was super hard to reproduce where duplicate tasks would get started.

the only tasks that get reconnected to are those with isBackground: true and have problem matchers.

It's especially painful for "runOnOpen" tasks, because every time you reload the window
you can disable this with task.reconnection: false.

[meganrogge]

task reconnection works for me
https://user-images.githubusercontent.com/29464607/232880261-079e986a-98eb-462f-ba12-5d8ca8767f9a.mov

[markw65]

the reason it's not in a finally is this was meant to (and did) fix an issue

Ok. So there's a race that happens between deciding to launch the task and actually launching it? In that case, the solution seems kind of misleading.

Shouldn't it be something like _launchingTasks rather than _inProgressTasks? but also, instead of

 if (executeResult) { 
 	this._inProgressTasks.delete(qualifiedLabel); 
 	return this._handleExecuteResult(executeResult, runSource); 
 } 
 this._inProgressTasks.delete(qualifiedLabel); 

why not just do

 this._inProgressTasks.delete(qualifiedLabel); 
 if (executeResult) { 
 	return this._handleExecuteResult(executeResult, runSource); 
 } 

And why do you need the delete in

vscode/src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts

Line 1918 in e9c1114

this._inProgressTasks.delete(task.getQualifiedLabel());

since it was already deleted right after it launched?

But all this is incidental; I was hoping this code could prevent the bug I reported, but clearly it can't.

[markw65]

the only tasks that get reconnected to are those with isBackground: true and have problem matchers

Ok... but the problem is that my task is left running across the reload, but it doesn't get reconnected (apparently intentionally), so attempting to start it again succeeds, even though it's supposed to only allow one instance.

task reconnection works for me
https://user-images.githubusercontent.com/29464607/232880261-079e986a-98eb-462f-ba12-5d8ca8767f9a.mov

I'm not sure what that .mov is supposed to be showing me; I can't see anything relating to tasks being reconnected (but I'm not sure what Im looking for). But anyway, I'm not claiming it's broken; just that it doesn't reconnect my task (or similar ones). And if, as you say, thats intentional, then the bug is elsewhere. But as far as I can tell, either my task shouldn't be left running, or it should be reconnected.

[markw65]

Another data point.

After reloading, the task is clearly running, but Cmd-Shift-P: "Tasks: Show Running Tasks" tells me "No running tasks"

[markw65]

you can disable this with task.reconnection: false

So when I set this to false, my task is killed on window reload, which does mitigate the problem (in fact, for my use case, I think this does solve the problem - although I don't know yet whether it introduces new ones).

But that still leaves us with the issue when task.reconnection is true. The task is left running, but not (properly) reconnected - so although it's there, and its output is going to.a terminal, the abstractTaskService, and terminalTaskSystem aren't aware of it (so that "Show Running Tasks" says there are none, and attempting to start a second instance succeeds).

[meganrogge]

I won't be looking into this any time soon just fyi. So if it bothers you, feel free to keep digging. Otherwise, just keep the setting disabled.

[patroza]

I run into this problem all the time too, which is very frustrating. I have to reload often due to SSH reconnection etc, this reattach issue together with ssh agent forwarding on mac os breaking, are really detrimental to the work flow.

Using task.reconnection: false works around some of the issue, but exposes a new one; one of my processes doesn't get killed and blocks the new one from starting. That process appears to be a nodemon process, perhaps related to #87321

[meganrogge]

Yeah @patroza that is unrelated to this feature as you mentioned.

[markw65]

@meganrogge as noted in the pull request, this doesn't fix everything, but it makes my workflow (with several auto-run tasks) much cleaner. Those tasks will be properly reconnected on reload, rather than an additional set being kicked off on each reload.

For tasks that don't auto-run on reload, the only change is that the first attempt to run a new instance of that task will simply attach to the existing process; and subsequent attempts will fail the same way they do before the reload.

Other than that, there should be no change in behavior, so I think this should be pretty safe.

I spent some time trying to figure out how to actually reconnect all the running tasks, rather than just marking them for reconnection, but I kept running into issues where abstractTaskService.ts needed to know about the internals of terminalTaskSystem.ts (IReconnectionTaskData in particular). Or I needed to extend the ITaskSystem api - which I'm reluctant to do without guidance.

I also tried a completely different approach - namely, including non-background tasks in _persistentTasks. That almost worked, but the problem was that task.getRecentlyUsedKey() gave a different result when the task is launched manually vs when it's run from _reconnectTasks (when run manually, its a ContributedTask, but when run from _reconnectTasks its a ConfiguringTask, which gets resolved to a ConfiguredTask). As a result, the tasks did get reconnected properly, the various commands (eg workbench.action.tasks.showTasks) know they're there and do the right thing; but now trying to run them manually uses a different key, and still starts a new copy...

[meganrogge]

@markw65 thanks for looking into this. I commented on your PR

[meganrogge]

To verify, run a non background task. Reload the window. Verify that you can run that task again