Conversation
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.
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.
# Conflicts: # api/src/controllers/internal/spec/Execution.spec.ts
|
🎉 This PR is included in version 0.39.6 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
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
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.
Issue
Closes #390
Intent
Two small, independent fixes bundled on this branch:
ExecutionControllerfrom silently discarding a failed JS/PY/Rsession's state.
drive.spec.tsfrom leaking real files into the sharedsasjs_rootdirectory, which was causing spurious failures onsubsequent local test runs.
Implementation
1. JS/PY/R session state overwrite
For the JS/PY/R runtimes,
processProgramsetssession.state = failed(with
session.failureReason) itself when the spawned interpreter processexits non-zero — without throwing:
Immediately after
processProgramreturns,Execution.tsunconditionallyoverwrote that state:
So a crashed JS/PY/R session had its
failedstate silently stomped backto
completedbefore ever being inspected or cleaned up. The client-visibleHTTP error response was unaffected (that's built from the log/webout files,
not
session.state), but anything downstream inspecting the session objectitself — e.g.
scheduleSessionDestroy'sexpiresAfterMinsbranch inSession.ts— would see a crashed session mis-reported as successful.Fix (
Execution.ts:130-132): guard the assignment so afailedstateis never overwritten. No effect on SAS, which manages its own state via a
separate spawned-process lifecycle.
Tests (
Execution.spec.ts, new): covers both the failure case (statestays
failed) and the success case (state still becomescompleted), soneither direction regresses.
2.
drive.spec.tsleaking into the realsasjs_rootdrive.spec.tsalready isolates itself into a unique, timestampedtmpFolderforgetSasjsRootFolder()/getUploadsFolder(), cleaned up viaafterAll(() => deleteFolder(tmpFolder)). ButgetFilesFolder()— whatnearly every test in this file actually writes to — resolves through a
separate, unmocked function (
getSasjsDriveFolder()→process.driveLoc),so every run left real files/folders (
level1,my/path/...) behind inthe shared
api/sasjs_root/drive/files. On a subsequent run this caused:folder-listing tests seeing stale entries left by a prior run, and
file-creation tests getting
409 Conflictagainst files a previous run hadalready created.
Fix: mock
getFilesFolder()the same waygetUploadsFolder()alreadyis, so it resolves inside the same isolated
tmpFolderand gets cleaned upby the existing
afterAll.Verification: ran the suite twice in a row from a clean slate — both
runs pass, and the real
sasjs_root/drivedirectory is never touched byeither run.
Checks
npm run lint:fix).npm test).