Skip to content

Conversation

@KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented Sep 25, 2025

User description

apscheduler tries to schedule jobs when the interpreter is shutting down which can cause it to crash and leave us in a bad state


PR Type

Bug fix, Enhancement


Description

  • Patch APScheduler to prevent shutdown crashes

  • No-op scheduler start and job add methods

  • Invoke patch before tracer initialization


Diagram Walkthrough

flowchart LR
  A["Import find_spec"] -- "detect apscheduler" --> B["patch_ap_scheduler()"]
  B -- "no-op start/add_job" --> C["Background/Blocking/BaseScheduler"]
  D["runctx"] -- "call" --> B
  D -- "then" --> E["Tracer initialization"]
Loading

File Walkthrough

Relevant files
Bug fix
tracing_new_process.py
Patch APScheduler and invoke before tracing                           

codeflash/tracing/tracing_new_process.py

  • Add find_spec import to detect APScheduler.
  • Introduce patch_ap_scheduler() to no-op scheduler methods.
  • Call patch_ap_scheduler() before creating Tracer.
+14/-0   

apscheduler tries to schedule jobs when the interpreter is shutting down which can cause it to crash and leave us in a bad state
@KRRT7 KRRT7 requested a review from misrasaurabh1 September 25, 2025 02:02
@github-actions github-actions bot changed the title patch apscheduler when tracing patch apscheduler when tracing Sep 25, 2025
@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The condition in patch_ap_scheduler() uses if find_spec("apscheduler") is None: and then imports apscheduler.*. This will execute only when the module is not found, causing an ImportError; likely the condition is inverted and should patch only when apscheduler is present.

def patch_ap_scheduler() -> None:
    if find_spec("apscheduler") is None:

        import apscheduler.schedulers.background as bg
        import apscheduler.schedulers.blocking as bb
        from apscheduler.schedulers import base

        bg.BackgroundScheduler.start = lambda _, *_a, **_k: None
        bb.BlockingScheduler.start = lambda _, *_a, **_k: None
        base.BaseScheduler.add_job = lambda _, *_a, **_k: None
Patch Safety

Monkey-patching start and add_job to no-ops may hide legitimate scheduler usage and produce silent failures. Consider logging a warning when intercepting these calls or gating by runtime context (e.g., only during tracing or shutdown).

bg.BackgroundScheduler.start = lambda _, *_a, **_k: None
bb.BlockingScheduler.start = lambda _, *_a, **_k: None
base.BaseScheduler.add_job = lambda _, *_a, **_k: None
Patch Timing

patch_ap_scheduler() is invoked right before creating Tracer, but after various imports may have already loaded apscheduler. If any code imported it earlier, patch may be too late. Consider calling the patch as early as possible in process start or guarding with import hooks.

patch_ap_scheduler()
tracer = Tracer(
    config=args_dict["config"],
    output=Path(args_dict["output"]),
    functions=args_dict["functions"],

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix inverted availability check

The condition is inverted: imports and monkey-patching will only run when
apscheduler is not installed, causing ImportError at runtime. Flip the check to only
patch when the module exists. Also, guard the patch in a try/except to fail safe if
import layout differs.

codeflash/tracing/tracing_new_process.py [51-61]

 def patch_ap_scheduler() -> None:
-    if find_spec("apscheduler") is None:
+    try:
+        if find_spec("apscheduler") is not None:
+            import apscheduler.schedulers.background as bg
+            import apscheduler.schedulers.blocking as bb
+            from apscheduler.schedulers import base
 
-        import apscheduler.schedulers.background as bg
-        import apscheduler.schedulers.blocking as bb
-        from apscheduler.schedulers import base
+            bg.BackgroundScheduler.start = lambda _, *_a, **_k: None
+            bb.BlockingScheduler.start = lambda _, *_a, **_k: None
+            base.BaseScheduler.add_job = lambda _, *_a, **_k: None
+    except Exception:
+        pass
 
-        bg.BackgroundScheduler.start = lambda _, *_a, **_k: None
-        bb.BlockingScheduler.start = lambda _, *_a, **_k: None
-        base.BaseScheduler.add_job = lambda _, *_a, **_k: None
-
Suggestion importance[1-10]: 9

__

Why: The condition is indeed inverted: importing apscheduler inside the branch when find_spec("apscheduler") is None will raise ImportError. Flipping the check and using a try/except prevents runtime failures and aligns with the intended behavior.

High
General
Preserve originals and limit impact

Monkey-patching core methods to no-ops can break existing scheduler instances
outside tracing. Limit the patch to instance methods via wrappers that log and
return gracefully without side effects, and ensure original methods are restorable.
This reduces unexpected global impacts.

codeflash/tracing/tracing_new_process.py [58-61]

-bg.BackgroundScheduler.start = lambda _, *_a, **_k: None
-bb.BlockingScheduler.start = lambda _, *_a, **_k: None
-base.BaseScheduler.add_job = lambda _, *_a, **_k: None
+_orig_bg_start = getattr(bg.BackgroundScheduler, "start", None)
+_orig_bb_start = getattr(bb.BlockingScheduler, "start", None)
+_orig_add_job = getattr(base.BaseScheduler, "add_job", None)
 
+def _noop_start(self, *args, **kwargs):
+    return None
+
+def _noop_add_job(self, *args, **kwargs):
+    return None
+
+if _orig_bg_start:
+    bg.BackgroundScheduler.start = _noop_start
+if _orig_bb_start:
+    bb.BlockingScheduler.start = _noop_start
+if _orig_add_job:
+    base.BaseScheduler.add_job = _noop_add_job
+
Suggestion importance[1-10]: 6

__

Why: Reducing global side effects by preserving originals and using no-op wrappers improves safety and maintainability, though it's a moderate enhancement rather than a critical fix.

Low
Gate patch behind config flag

Calling the patch unconditionally may mask issues when tracing unrelated code. Gate
the patch behind a config flag from args_dict["config"] so it only applies when
explicitly enabled. This prevents surprising behavior during normal runs.

codeflash/tracing/tracing_new_process.py [855-856]

-patch_ap_scheduler()
+if args_dict["config"].get("disable_apscheduler", True):
+    patch_ap_scheduler()
Suggestion importance[1-10]: 5

__

Why: Making the patch conditional via config can prevent unexpected behavior, but it’s optional and context-dependent; the proposed change is reasonable but not critical.

Low

misrasaurabh1
misrasaurabh1 previously approved these changes Sep 25, 2025


def patch_ap_scheduler() -> None:
if find_spec("apscheduler"):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had originally used a Try except but accidentally inverted cond when implementing find_spec

@KRRT7 KRRT7 merged commit f296a0f into main Sep 25, 2025
19 of 23 checks passed
@KRRT7 KRRT7 deleted the prevent-deadlocks-pytest branch September 25, 2025 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants