Merge pull request #391 from sasjs/issue-390

Issue 390

api/src/controllers/internal/Execution.ts

@@ -144,7 +144,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 = []
 

api/src/controllers/internal/spec/Execution.spec.ts

@@ -14,7 +14,7 @@ const preProgramVariables: PreProgramVars = {
   httpHeaders: []
 }
 
-describe('ExecutionController.executeProgram (SAS failure path)', () => {
+describe('ExecutionController.executeProgram', () => {
   let session: Session
 
   beforeEach(() => {
@@ -33,33 +33,85 @@ describe('ExecutionController.executeProgram (SAS failure path)', () => {
     await deleteFolder(session.path)
   })
 
-  it('throws a SessionExecutionError carrying the complete log when the session fails', async () => {
-    const logPath = path.join(session.path, 'log.log')
-    const logContent =
-      'NOTE: SAS session\nERROR: SAS session terminated. See log for details.\n'
+  // 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.
+  describe('JS/PY/R failure 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'
+        })
 
-    await createFile(logPath, logContent)
+      const controller = new ExecutionController()
 
-    jest
-      .spyOn(ProcessProgramModule, 'processProgram')
-      .mockImplementation(async () => {
-        throw new Error('ERROR: SAS session terminated. See log for details.')
+      await controller.executeProgram({
+        program: 'throw new Error("boom")',
+        preProgramVariables,
+        vars: {},
+        session,
+        runTime: RunTimeType.JS
       })
 
-    const controller = new ExecutionController()
-
-    const resultPromise = controller.executeProgram({
-      program: '%abort;',
-      preProgramVariables,
-      vars: {},
-      session,
-      runTime: RunTimeType.SAS
+      expect(session.state).toBe(SessionState.failed)
     })
 
-    await expect(resultPromise).rejects.toBeInstanceOf(SessionExecutionError)
-    await expect(resultPromise).rejects.toMatchObject({
-      log: logContent,
-      message: expect.stringContaining('SAS session terminated')
+    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)
+    })
+  })
+
+  describe('SAS failure path', () => {
+    it('throws a SessionExecutionError carrying the complete log when the session fails', async () => {
+      const logPath = path.join(session.path, 'log.log')
+      const logContent =
+        'NOTE: SAS session\nERROR: SAS session terminated. See log for details.\n'
+
+      await createFile(logPath, logContent)
+
+      jest
+        .spyOn(ProcessProgramModule, 'processProgram')
+        .mockImplementation(async () => {
+          throw new Error('ERROR: SAS session terminated. See log for details.')
+        })
+
+      const controller = new ExecutionController()
+
+      const resultPromise = controller.executeProgram({
+        program: '%abort;',
+        preProgramVariables,
+        vars: {},
+        session,
+        runTime: RunTimeType.SAS
+      })
+
+      await expect(resultPromise).rejects.toBeInstanceOf(SessionExecutionError)
+      await expect(resultPromise).rejects.toMatchObject({
+        log: logContent,
+        message: expect.stringContaining('SAS session terminated')
+      })
     })
   })
 })

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,