Bugfix: Resolve race condition with FIFO cleanup and child process termination

[robsyme]

Problem Description

Users encountered a race condition where STAR would delete FIFO files while child processes (running samtools view via --readFilesCommand) were still writing to them. This manifested as the process leaving unclosed file descriptors. In lsof output, we see:

sh  1427  root  7r  unknown  /path/_STARtmp/tmp.fifo.read1 (deleted) (stat: No such file or directory)

The child process holds a file descriptor to a deleted FIFO, indicating improper synchronization between process termination and file cleanup.

Root Cause Analysis

Primary Issue: Incorrect Cleanup Ordering

The main problem was in STAR.cpp - the cleanup sequence was backwards:

// BROKEN ORDER - deletes files while processes still running
if (P.outTmpKeep == "None") {
    sysRemoveDir(P.outFileTmp);      // Deletes FIFOs first
};
P.closeReadsFiles();                 // Cleans up processes after files gone

Secondary Issue: Inadequate Process Synchronization

In Parameters_closeReadsFiles.cpp, the process cleanup was too aggressive:

// OLD CODE - no synchronization
kill(readFilesCommandPID[imate], SIGKILL);  // Forceful kill, returns immediately
// No waiting - function returns while process may still be dying

The Race Condition Window

// OLD CODE - Race condition
kill(readFilesCommandPID[imate], SIGKILL);  // Process is dying...
// Function returns immediately, no synchronization
// Meanwhile: sysRemoveDir() deletes FIFOs while process still holds FDs

Timeline:

• T0: STAR finishes processing, enters cleanup
• T1: sysRemoveDir(P.outFileTmp) deletes FIFO files while samtools view still running
• T2: P.closeReadsFiles() sends SIGKILL to processes writing to already-deleted FIFOs
• T3: Child processes hold file descriptors to deleted files during termination

Solution

Fix 1: Correct Cleanup Ordering (STAR.cpp)

// FIXED ORDER - processes first, then files
P.closeReadsFiles();                 //  Clean up processes FIRST
if (P.outTmpKeep == "None") {
    sysRemoveDir(P.outFileTmp);      //  THEN delete files
};

Fix 2: Proper Process Synchronization (Parameters_closeReadsFiles.cpp)

// NEW CODE - synchronized cleanup
kill(readFilesCommandPID[imate], SIGTERM);      // Graceful shutdown request
int status;
waitpid(readFilesCommandPID[imate], &status, 0); // Wait for actual termination

Why This Fixes The Race Condition

1. Ordering Fix (Primary): Directory removal only happens after all processes are confirmed terminated
2. SIGTERM vs SIGKILL: Allows graceful shutdown instead of forced termination
3. waitpid() Synchronization: Guarantees child processes have fully released file descriptors before cleanup
4. Status Checking: Provides visibility into samtools failures for better debugging

Impact

• Eliminates race condition: No more deleted FIFOs with active file descriptors
• Better error reporting: Users can see if samtools view failed on their BAM files
• Maintains functionality: No change to STAR's core behavior or performance
• Defensive programming: Handles edge cases gracefully

The combination of correct ordering and proper synchronization ensures that sysRemoveDir() only executes after all child processes have properly released their file descriptors, eliminating the race condition entirely.

[robsyme]

@felixschlesinger - this is my best guess at the race condition bug.

If you have an opportunity - would it be possible to compile STAR from this branch and try it in your container?

[felixschlesinger]

Thanks @robsyme, we'll try this.
Semi-related but just to have all the information here as well, I think the other set of samtools processes started here needs to be cleaned up as well: https://github.com/alexdobin/STAR/blob/master/source/Parameters_readSAMheader.cpp#L32

AFAIK relying on closing the fifo is not sufficient, because the samtools command might only start writing to it after main STAR already closed it (race condition).