diff --git a/server/app/core/transferrer.js b/server/app/core/transferrer.js index 193fd1b0..83e8fcdc 100644 --- a/server/app/core/transferrer.js +++ b/server/app/core/transferrer.js @@ -192,6 +192,23 @@ export async function stageOut(task) { await _internal.setTaskState(task, taskState); } +/** + * Get the distinct remotehostIDs that a project has pending deferred cleanups for, without + * consuming/clearing the registry (aicshud/WHEEL#1023). A caller that needs its SSH + * connections re-established before runDeferredCleanups() can run (e.g. onCleanProject(), + * invoked after stopProject() has already unconditionally disconnected everything) uses this + * to know which remotehosts to reconnect to first. + * @param {string} projectRootDir - project's root path + * @returns {string[]} - distinct remotehostIDs referenced by this project's registered entries + */ +export function getDeferredCleanupRemotehostIDs(projectRootDir) { + const entries = deferredCleanupRegistry.get(projectRootDir) || []; + const remotehostIDs = entries.map((entry)=>{ + return entry.remotehostID; + }); + return [...new Set(remotehostIDs)]; +} + /** * Run all deferred cleanup operations for a project. * Deletes remote-symlink output files that were preserved during per-component cleanup, diff --git a/server/app/handlers/projectController.js b/server/app/handlers/projectController.js index 03e7c5db..910985a0 100644 --- a/server/app/handlers/projectController.js +++ b/server/app/handlers/projectController.js @@ -22,6 +22,7 @@ import { getProjectJson, getProjectState, setProjectState, updateProjectDescript import { createSsh, removeSsh, askPassword } from "../core/sshManager.js"; import { setJWTServerPassphrase, removeAllJWTServerPassphrase } from "../core/jwtServerPassphraseManager.js"; import { runProject, cleanProject, stopProject, releaseRuntimeResources } from "../core/projectController.js"; +import { runDeferredCleanups, getDeferredCleanupRemotehostIDs } from "../core/transferrer.js"; import { isValidOutputFilename } from "../lib/utility.js"; import { checkWritePermissions, parentDirs, eventEmitters } from "../core/global.js"; import { sendWorkflow, sendProjectJson, sendTaskStateList, sendResultsFileDir, sendComponentTree } from "./senders.js"; @@ -50,7 +51,12 @@ const _internal = { setProjectState, selectRunHandler, unlockIfFinished, - rootDispatchers: new Map() + rootDispatchers: new Map(), + remoteHost, + createSsh, + removeSsh, + runDeferredCleanups, + getDeferredCleanupRemotehostIDs }; async function updateProjectState(projectRootDir, state, force) { @@ -529,6 +535,39 @@ export async function onCleanComponent(clientID, projectRootDir, targetComponent ]); } +/** + * reconnect SSH to every remotehost this project still has pending deferred cleanups for, + * run them, then disconnect again (aicshud/WHEEL#1023). + * stopProject() (aicshud/WHEEL#1020/#1021) unconditionally disconnects every SSH connection + * on stop, but deliberately leaves deferredCleanupRegistry populated - a not-yet-executed + * downstream task may still need the preserved remote-symlink target file if the project is + * resumed. If the user cleans the project instead of resuming it, that entry never gets a + * chance to run naturally (only runProject()'s own natural-completion path calls + * runDeferredCleanups()), permanently leaking the preserved remote files. Since cleanProject + * is only ever triggered by an explicit user action (the "clean" button), it is fine for this + * to need a fresh password/passphrase prompt (via createSsh()'s askPassword callback) even + * though the project is not running. + * @param {string} clientID - socket's ID, used if createSsh needs to ask for a password + * @param {string} projectRootDir - project's root path + */ +async function reconnectAndRunDeferredCleanups(clientID, projectRootDir) { + const remotehostIDs = _internal.getDeferredCleanupRemotehostIDs(projectRootDir); + if (remotehostIDs.length === 0) { + return; + } + for (const id of remotehostIDs) { + const hostinfo = _internal.remoteHost.get(id); + if (!hostinfo) { + getLogger(projectRootDir).warn(`remotehost ${id} is no longer defined; skipping its deferred cleanup`); + continue; + } + await _internal.createSsh(projectRootDir, hostinfo.name, hostinfo, clientID, false); + } + await _internal.runDeferredCleanups(projectRootDir); + _internal.removeSsh(projectRootDir); +} +_internal.reconnectAndRunDeferredCleanups = reconnectAndRunDeferredCleanups; + async function onCleanProject(clientID, projectRootDir) { try { await askUnsavedFiles(clientID, projectRootDir); @@ -540,6 +579,7 @@ async function onCleanProject(clientID, projectRootDir) { } try { await clearProjectEdits(projectRootDir); + await _internal.reconnectAndRunDeferredCleanups(clientID, projectRootDir); await Promise.all([ cleanProject(projectRootDir), removeTempd(projectRootDir, "viewer"), diff --git a/server/test/app/core/transferrer.js b/server/test/app/core/transferrer.js index aa6e9ba6..8f710fea 100644 --- a/server/test/app/core/transferrer.js +++ b/server/test/app/core/transferrer.js @@ -9,7 +9,7 @@ const expect = chai.expect; import chaiAsPromised from "chai-as-promised"; chai.use(chaiAsPromised); import sinon from "sinon"; -import { stageIn, stageOut, runDeferredCleanups, clearDeferredCleanups, _internal } from "../../../app/core/transferrer.js"; +import { stageIn, stageOut, runDeferredCleanups, clearDeferredCleanups, getDeferredCleanupRemotehostIDs, _internal } from "../../../app/core/transferrer.js"; describe("#stageIn", ()=>{ let setTaskStateStub; let getSshHostinfoStub; @@ -454,6 +454,44 @@ describe("#runDeferredCleanups", ()=>{ }); }); +describe("[reproduction] issue aicshud/WHEEL#1023 - getDeferredCleanupRemotehostIDs must expose which remotehosts need reconnecting", ()=>{ + afterEach(()=>{ + sinon.restore(); + }); + + it("should return an empty array when no deferred cleanups are registered for the project", ()=>{ + expect(getDeferredCleanupRemotehostIDs("/proj/not-registered")).to.deep.equal([]); + }); + + it("should return the distinct remotehostIDs of all registered entries, without mutating the registry", ()=>{ + _internal.addDeferredCleanup("/proj/multi", { + remoteWorkingDir: "/remote/task1", + remotehostID: "hostX", + symlinkTargetNames: ["file1.dat"] + }); + _internal.addDeferredCleanup("/proj/multi", { + remoteWorkingDir: "/remote/task2", + remotehostID: "hostY", + symlinkTargetNames: ["file2.dat"] + }); + _internal.addDeferredCleanup("/proj/multi", { + remoteWorkingDir: "/remote/task3", + remotehostID: "hostX", + symlinkTargetNames: ["file3.dat"] + }); + + expect(getDeferredCleanupRemotehostIDs("/proj/multi").sort()).to.deep.equal(["hostX", "hostY"]); + + //must be a non-destructive peek - runDeferredCleanups must still see all 3 entries + const sshExecStub = sinon.stub().resolves(0); + sinon.stub(_internal, "getSsh").returns({ exec: sshExecStub }); + sinon.stub(_internal, "getLogger").returns({ debug: sinon.stub(), warn: sinon.stub() }); + return runDeferredCleanups("/proj/multi").then(()=>{ + expect(sshExecStub.callCount).to.equal(6); //1 file + 1 dir for each of the 3 entries + }); + }); +}); + describe("#clearDeferredCleanups", ()=>{ afterEach(()=>{ sinon.restore(); diff --git a/server/test/app/handlers/projectController.js b/server/test/app/handlers/projectController.js index b816b14a..6ec12b8f 100644 --- a/server/test/app/handlers/projectController.js +++ b/server/test/app/handlers/projectController.js @@ -500,4 +500,44 @@ describe("project Controller handler UT", function () { sinon.assert.calledOnceWithExactly(removeTransferrersStub, projectRootDir); }); }); + + describe("[reproduction] issue aicshud/WHEEL#1023 - onCleanProject must reconnect SSH before running deferred cleanups", ()=>{ + it("should reconnect ssh for every remotehost with a pending deferred cleanup, run them, then disconnect again", async ()=>{ + //stopProject() (aicshud/WHEEL#1020/#1021) leaves the deferredCleanupRegistry populated + //but has already unconditionally disconnected every SSH connection - so by the time the + //user clicks "clean", runDeferredCleanups() can not just be called directly (it would + //throw "ssh instance is not registerd for the project"). onCleanProject must reconnect + //first, using the same remotehost.json entry runDispatcher() itself would have used. + const clientID = "test-client-id"; + const hostinfoA = { id: "hostA", name: "hostA-name" }; + sinon.stub(_internal, "getDeferredCleanupRemotehostIDs").returns(["hostA"]); + sinon.stub(_internal, "remoteHost").value({ + get: sinon.stub().withArgs("hostA") + .returns(hostinfoA) + }); + const createSshStub = sinon.stub(_internal, "createSsh").resolves({}); + const runDeferredCleanupsStub = sinon.stub(_internal, "runDeferredCleanups").resolves(); + const removeSshStub = sinon.stub(_internal, "removeSsh"); + + await _internal.onCleanProject(clientID, projectRootDir); + + sinon.assert.calledOnceWithExactly(createSshStub, projectRootDir, "hostA-name", hostinfoA, clientID, false); + sinon.assert.calledOnceWithExactly(runDeferredCleanupsStub, projectRootDir); + sinon.assert.calledOnceWithExactly(removeSshStub, projectRootDir); + sinon.assert.callOrder(createSshStub, runDeferredCleanupsStub, removeSshStub); + }); + + it("should not attempt any SSH reconnection when there are no pending deferred cleanups", async ()=>{ + sinon.stub(_internal, "getDeferredCleanupRemotehostIDs").returns([]); + const createSshStub = sinon.stub(_internal, "createSsh"); + const runDeferredCleanupsStub = sinon.stub(_internal, "runDeferredCleanups"); + const removeSshStub = sinon.stub(_internal, "removeSsh"); + + await _internal.onCleanProject("test-client-id", projectRootDir); + + expect(createSshStub.called).to.be.false; + expect(runDeferredCleanupsStub.called).to.be.false; + expect(removeSshStub.called).to.be.false; + }); + }); });