Add timeout and symbol length limits to demangling #540
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
NicoHinderling
force-pushed
the
add-more-limits-to-demangling
branch
from
fc5f3a8 to
e5d8c3a
Compare
|
|
||
| # 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")) |
There was a problem hiding this comment.
Can we split out the DEFAULT_MAX_SYMBOL_LENGTH into another PR? I'm skeptical we should have this since many of the problematic symbols I'm finding are under 1500, so this would both not solve the problem and remove false positives.
| ) | ||
| except subprocess.TimeoutExpired: | ||
| elapsed = time.time() - start_time | ||
| logger.warning(f"Chunk {chunk_idx} timed out after {elapsed:.1f}s") |
There was a problem hiding this comment.
Can this be an error log to start, can reduce if it becomes noisy.
| 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") |
There was a problem hiding this comment.
nit: keep the logger.exception call so we get the error stack and move chunk_idx and elapsed to extra.
NicoHinderling
force-pushed
the
add-more-limits-to-demangling
branch
from
e5d8c3a to
9d6f921
Compare
|
|
||
| # Process in chunks to avoid potential issues with large inputs | ||
| chunk_size = 5000 | ||
| chunk_size = 500 |
There was a problem hiding this comment.
If we want to experiment with the chunk size maybe we should move this to an ENV as well?
| DEFAULT_DEMANGLE_TIMEOUT = int(os.environ.get("LAUNCHPAD_DEMANGLE_TIMEOUT", "10")) | ||
|
|
||
| # Default chunk size for batching symbols | ||
| DEFAULT_CHUNK_SIZE = int(os.environ.get("LAUNCHPAD_DEMANGLE_CHUNK_SIZE", "500")) | ||
|
|
||
|
|
||
| @dataclass | ||
| class CwlDemangleResult: |
There was a problem hiding this comment.
Bug: The default demangle timeout is 10 seconds, which contradicts the 30-second timeout mentioned in the pull request description.
Severity: HIGH
Suggested Fix
Align the code with the documented intention in the pull request description. Change the default value for LAUNCHPAD_DEMANGLE_TIMEOUT from "10" to "30" in cwl_demangle.py.
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#L18-L25
Potential issue: The default timeout for the demangle process is set to 10 seconds in
the code via `DEFAULT_DEMANGLE_TIMEOUT`, but the pull request description explicitly
states the intention was to add a 30-second timeout. This discrepancy means that symbol
demangling chunks that take between 10 and 30 seconds to process will time out
prematurely. When a timeout occurs, the function returns an empty dictionary, causing
the symbols in that chunk to be categorized as "Unattributed" instead of being correctly
demangled, potentially leading to less accurate symbol analysis.
Did we get this right? 👍 / 👎 to inform future reviews.
We were seeing a build with some extremely long symbols that can cause the demangling process to hang. This is an idea for curbing the impact: