fix: prevent Python subprocess deadlock by removing hardcoded OTEL_LOG_LEVEL DEBUG#3392
fix: prevent Python subprocess deadlock by removing hardcoded OTEL_LOG_LEVEL DEBUG#3392renyi9044-png wants to merge 1 commit intotriggerdotdev:mainfrom
Conversation
|
|
Hi @renyi9044-png, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚩 OTEL env filtering not applied consistently across all methods
The OTEL env var filtering was applied to runScript (line 93-97) and runInline (line 158-163), but not to run (line 32) or any of the stream.* methods (stream.run at line 213, stream.runScript at line 260, stream.runInline at line 305). All of these still use ...process.env directly. If the intent is to "prevent stderr flood" from parent OTEL vars leaking into Python subprocesses, the same issue presumably exists for all execution paths. This inconsistency may be intentional (perhaps only script-based methods are affected), but it's worth confirming.
(Refers to line 32)
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Filter out parent OTEL vars to prevent stderr flood | ||
| ...Object.fromEntries( | ||
| Object.entries(process.env).filter( | ||
| ([key]) => !key.startswith("OTEL_") |
There was a problem hiding this comment.
🔴 key.startswith is not a function — should be key.startsWith (capital W)
JavaScript's String.prototype method is startsWith (camelCase with capital W), not startswith (all lowercase, which is the Python convention). Calling key.startswith("OTEL_") will throw a TypeError: key.startswith is not a function at runtime, crashing python.runScript() every time it is called. This affects line 95 in runScript and line 161 in runInline.
| ([key]) => !key.startswith("OTEL_") | |
| ([key]) => !key.startsWith("OTEL_") |
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Filter out parent OTEL vars to prevent stderr flood | ||
| ...Object.fromEntries( | ||
| Object.entries(process.env).filter( | ||
| ([key]) => !key.startswith("OTEL_") |
There was a problem hiding this comment.
🔴 key.startswith is not a function in runInline — should be key.startsWith
Same bug as in runScript: key.startswith("OTEL_") at packages/python/src/index.ts:161 uses the Python naming convention instead of JavaScript's startsWith. This will throw a TypeError at runtime, crashing python.runInline() on every invocation.
| ([key]) => !key.startswith("OTEL_") | |
| ([key]) => !key.startsWith("OTEL_") |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #3357
Problem
python.runScript()permanently deadlocks when the Python subprocess writes >64KB to stderr. This happens because:OTEL_LOG_LEVEL: "DEBUG"forces verbose OpenTelemetry logging in every Python subprocess...process.envspreadwrite()syscall → permanent hangFix
Two changes in
packages/python/src/index.ts:OTEL_LOG_LEVEL: "DEBUG"— was overriding any user settingprocess.envspread* — prevents parent environment contaminationPython subprocesses now use default OTEL log level (INFO) unless explicitly overridden via
options.env.Impact
OTEL_LOG_LEVELexplicitly viaoptions.envif neededTesting
Verified the fix removes the hardcoded DEBUG level and filters parent OTEL vars while preserving other env vars and user-provided overrides.
Note
This fixes the contributing cause identified in #3357. The root cause (eager stderr draining in tinyexec) would require a separate fix in the dependency.