Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions src/launchpad/utils/apple/cwl_demangle.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import shutil
import subprocess
import tempfile
import time
import uuid

from dataclasses import dataclass
Expand All @@ -13,6 +14,13 @@

logger = get_logger(__name__)

# Default timeout for cwl-demangle subprocess (in seconds)
DEFAULT_DEMANGLE_TIMEOUT = int(os.environ.get("LAUNCHPAD_DEMANGLE_TIMEOUT", "30"))

# Maximum symbol length to send to cwl-demangle (in characters)
# Symbols longer than this are skipped and used in mangled form
DEFAULT_MAX_SYMBOL_LENGTH = int(os.environ.get("LAUNCHPAD_MAX_DEMANGLE_SYMBOL_LENGTH", "1500"))


@dataclass
class CwlDemangleResult:
Expand Down Expand Up @@ -149,6 +157,25 @@ def _demangle_chunk_worker(
if not chunk:
return {}

start_time = time.time()

# Filter out extremely long symbols that cause cwl-demangle to hang
filtered_chunk = [s for s in chunk if len(s) <= DEFAULT_MAX_SYMBOL_LENGTH]
skipped_count = len(chunk) - len(filtered_chunk)

if skipped_count > 0:
logger.warning(
f"Chunk {chunk_idx}: Skipping {skipped_count} symbols longer than {DEFAULT_MAX_SYMBOL_LENGTH} chars "
f"(will use mangled form for these)"
)

# Use filtered chunk for processing
chunk = filtered_chunk

if not chunk:
logger.warning(f"Chunk {chunk_idx}: All symbols filtered out")
return {}

binary_path = shutil.which("cwl-demangle")
if binary_path is None:
logger.error("cwl-demangle binary not found in PATH")
Expand Down Expand Up @@ -178,9 +205,16 @@ def _demangle_chunk_worker(
command_parts.append("--continue-on-error")

try:
result = subprocess.run(command_parts, capture_output=True, text=True, check=True)
result = subprocess.run(
command_parts, capture_output=True, text=True, check=True, timeout=DEFAULT_DEMANGLE_TIMEOUT
)
except subprocess.TimeoutExpired:
elapsed = time.time() - start_time
logger.warning(f"Chunk {chunk_idx} timed out after {elapsed:.1f}s")
return {}
Comment on lines +208 to +214
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The subprocess.run call does not explicitly terminate the child process when a TimeoutExpired exception is caught, which could lead to orphan processes.
Severity: HIGH

Suggested Fix

In the except subprocess.TimeoutExpired block, add logic to explicitly terminate the child process. You can access the process via the .child attribute of the TimeoutExpired exception object and call its .kill() method to ensure it is properly cleaned up.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/launchpad/utils/apple/cwl_demangle.py#L208-L214

Potential issue: When the `subprocess.run` call in `cwl_demangle.py` times out, the
`TimeoutExpired` exception is caught, but the child `cwl-demangle` process may not be
terminated. This could lead to an accumulation of zombie or orphan processes, consuming
system resources. While the behavior of `subprocess.run` on timeout can vary between
Python versions, explicitly terminating the process is the safest approach to ensure
proper resource cleanup, a pattern seen elsewhere in the codebase.

Did we get this right? 👍 / 👎 to inform future reviews.

except subprocess.CalledProcessError:
logger.exception(f"cwl-demangle failed for chunk {chunk_idx}")
elapsed = time.time() - start_time
logger.error(f"Chunk {chunk_idx} failed after {elapsed:.1f}s")
return {}

batch_result = json.loads(result.stdout)
Expand Down
40 changes: 40 additions & 0 deletions tests/integration/test_cwl_demangle.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,46 @@ def test_environment_variable_disables_parallel(self):
demangler = CwlDemangler()
assert demangler.use_parallel is False

def test_timeout_configuration(self):
"""Test LAUNCHPAD_DEMANGLE_TIMEOUT env var configures timeout."""
demangler = CwlDemangler()

# Test with custom timeout
with mock.patch.dict(os.environ, {"LAUNCHPAD_DEMANGLE_TIMEOUT": "30"}):
# Generate a few symbols to trigger sequential processing
symbols = self._generate_symbols(100)
for symbol in symbols:
demangler.add_name(symbol)

result = demangler.demangle_all()
# Should succeed with custom timeout
assert len(result) == 100

def test_max_symbol_length_filtering(self):
"""Test LAUNCHPAD_MAX_DEMANGLE_SYMBOL_LENGTH env var filters long symbols."""
demangler = CwlDemangler()

# Generate normal symbols
normal_symbols = self._generate_symbols(10)

# Create an artificially long symbol (>1500 chars)
very_long_symbol = "_$s" + "A" * 2000 + "V"

for symbol in normal_symbols:
demangler.add_name(symbol)
demangler.add_name(very_long_symbol)

# Test with default filtering (1500 chars)
result = demangler.demangle_all()

# Normal symbols should be demangled, long symbol should be skipped
assert len(result) == 10 # Only normal symbols
assert very_long_symbol not in result

# Verify normal symbols were processed
for symbol in normal_symbols:
assert symbol in result

def _generate_symbols(self, count: int) -> list[str]:
"""Generate valid Swift mangled symbols."""
letters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
Expand Down
Loading