From 4858245372fa19929b6e59c28148f5d004239c9d Mon Sep 17 00:00:00 2001 From: YuryShkoda Date: Tue, 14 Jul 2026 11:29:30 +0300 Subject: [PATCH 1/2] fix(api): stop overwriting a failed JS/PY/R session back to completed For JS/PY/R, processProgram sets session.state = failed (with failureReason) itself when the interpreter process exits non-zero, without throwing. ExecutionController.executeProgram then unconditionally set session.state = completed right after processProgram returned, silently overwriting that - so anything downstream inspecting session.state (e.g. scheduleSessionDestroy's expiresAfterMins branch) would see a crashed session mis-reported as successful. Guard the assignment so a failed state is never overwritten. No effect on SAS, which sets state independently via its own spawned process lifecycle. Added Execution.spec.ts covering both the failure case (state stays failed) and the success case (state still becomes completed), to guard against regressing in either direction. --- api/src/controllers/internal/Execution.ts | 11 ++- .../internal/spec/Execution.spec.ts | 84 +++++++++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 api/src/controllers/internal/spec/Execution.spec.ts diff --git a/api/src/controllers/internal/Execution.ts b/api/src/controllers/internal/Execution.ts index c1ff197..d256275 100644 --- a/api/src/controllers/internal/Execution.ts +++ b/api/src/controllers/internal/Execution.ts @@ -120,7 +120,16 @@ export class ExecutionController { : '' // it should be deleted by scheduleSessionDestroy - session.state = SessionState.completed + // + // Guarded: for JS/PY/R, processProgram sets state to `failed` itself + // (without throwing) when the interpreter process exits non-zero - if + // we unconditionally set `completed` here we'd silently overwrite that, + // and anything downstream inspecting session.state (e.g. + // scheduleSessionDestroy's expiresAfterMins branch in Session.ts) would + // see a crashed session mis-reported as successful. + if ((session.state as SessionState) !== SessionState.failed) { + session.state = SessionState.completed + } const resultParts = [] diff --git a/api/src/controllers/internal/spec/Execution.spec.ts b/api/src/controllers/internal/spec/Execution.spec.ts new file mode 100644 index 0000000..ec91cfe --- /dev/null +++ b/api/src/controllers/internal/spec/Execution.spec.ts @@ -0,0 +1,84 @@ +import path from 'path' +import os from 'os' +import { deleteFolder, generateTimestamp } from '@sasjs/utils' +import * as ProcessProgramModule from '../processProgram' +import { ExecutionController } from '../Execution' +import { Session, SessionState, PreProgramVars } from '../../../types' +import { RunTimeType } from '../../../utils' + +// Regression coverage: for JS/PY/R, processProgram sets session.state to +// `failed` itself (without throwing) on a non-zero interpreter exit. +// ExecutionController.executeProgram used to unconditionally overwrite +// that back to `completed` right after processProgram returned, silently +// losing the failure for anything downstream that inspects session.state. + +const preProgramVariables: PreProgramVars = { + username: 'testUser', + userId: 1, + displayName: 'Test User', + serverUrl: 'http://localhost:5000', + httpHeaders: [] +} + +describe('ExecutionController.executeProgram (JS/PY/R failure path)', () => { + let session: Session + + beforeEach(() => { + const sessionId = `test-session-${generateTimestamp()}` + session = { + id: sessionId, + state: SessionState.pending, + creationTimeStamp: '0', + deathTimeStamp: '0', + path: path.join(os.tmpdir(), sessionId) + } + }) + + afterEach(async () => { + jest.restoreAllMocks() + await deleteFolder(session.path) + }) + + it('does not overwrite a failed session state back to completed', async () => { + // mirrors processProgram's real JS/PY/R branch on a non-zero exit: + // it sets state/failureReason itself and resolves - it does not throw + jest + .spyOn(ProcessProgramModule, 'processProgram') + .mockImplementation(async () => { + session.state = SessionState.failed + session.failureReason = 'Error: process exited with code 1' + }) + + const controller = new ExecutionController() + + await controller.executeProgram({ + program: 'throw new Error("boom")', + preProgramVariables, + vars: {}, + session, + runTime: RunTimeType.JS + }) + + expect(session.state).toBe(SessionState.failed) + }) + + it('still marks a genuinely successful session as completed', async () => { + jest + .spyOn(ProcessProgramModule, 'processProgram') + .mockImplementation(async () => { + session.state = SessionState.completed + }) + + const controller = new ExecutionController() + + await controller.executeProgram({ + program: 'console.log("hello")', + preProgramVariables, + vars: {}, + session, + runTime: RunTimeType.JS + }) + + expect(session.state).toBe(SessionState.completed) + }) +}) From dec51914911c10b66b43ebe95f77b0af63e2d03e Mon Sep 17 00:00:00 2001 From: YuryShkoda Date: Tue, 14 Jul 2026 11:32:01 +0300 Subject: [PATCH 2/2] fix(api): isolate drive.spec.ts's files folder from the real drive drive.spec.ts already isolates itself into a unique, timestamped tmpFolder for getSasjsRootFolder()/getUploadsFolder(), cleaned up via afterAll(() => deleteFolder(tmpFolder)). But getFilesFolder() - what nearly every test in this file actually writes to - resolves through a separate, unmocked function (getSasjsDriveFolder()/process.driveLoc), so every run left real files/folders (e.g. 'level1', 'my/path/...') behind in the shared api/sasjs_root/drive/files, causing later runs to fail: folder-listing tests saw stale entries, and file-creation tests got 409 Conflict against files a previous run already created. Mock getFilesFolder() the same way getUploadsFolder() already is, so it resolves inside the same isolated tmpFolder and gets cleaned up by the existing afterAll. Verified by running the suite twice in a row from a clean slate: both runs pass, and the real sasjs_root/drive is never touched. --- api/src/routes/api/spec/drive.spec.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/api/src/routes/api/spec/drive.spec.ts b/api/src/routes/api/spec/drive.spec.ts index c4c28ce..c6bfd9d 100644 --- a/api/src/routes/api/spec/drive.spec.ts +++ b/api/src/routes/api/spec/drive.spec.ts @@ -28,6 +28,15 @@ jest .spyOn(fileUtilModules, 'getUploadsFolder') .mockImplementation(() => path.join(tmpFolder, 'uploads')) +// getFilesFolder() resolves via getSasjsDriveFolder()/process.driveLoc, a +// separate root from getSasjsRootFolder()/process.sasjsRoot above - without +// this, every test in this file that creates content under the drive (e.g. +// 'level1', 'my/path/...') writes into the real, shared sasjs_root/drive +// instead of this run's isolated tmpFolder, leaking state into later runs. +jest + .spyOn(fileUtilModules, 'getFilesFolder') + .mockImplementation(() => path.join(tmpFolder, 'drive', 'files')) + import appPromise from '../../../app' import { UserController,