fix(scheduler): alert on routine failure + run routines with sys.executable#124
Open
mt-alarcon wants to merge 1 commit into
Open
Conversation
…utable Two defects that compound into silent, long-lived outages of scheduled work. **1. Routine failures were invisible.** `run_adw` reported them only via `print()` to buffered stdout. Since the scheduler's output is normally redirected to a log file, nothing reached disk until the buffer filled — so a routine failing every day looked exactly like a routine that never ran. We lost a daily routine for 25 days to this before noticing. Failures now append to `ADWs/logs/routine-failures.jsonl` (timestamp, routine name, and the tail of the child's stderr, so the cause is diagnosable without reproducing) and send a Telegram notification via the existing `ADWs/runner.send_telegram`. The disk record is written first and always; the notification is best-effort and wrapped so it can never take the scheduler down. All three failure paths are covered: missing script, non-zero exit, and timeout. The status `print()`s also gain `flush=True` so the log stops lying. **2. `uv run python` silently pruned dependencies.** Routines were executed via `uv run`, which re-syncs the venv against `uv.lock` before every run and removes any package not declared in `pyproject.toml`. A routine whose dependency was installed ad-hoc therefore deleted that dependency while starting up, then failed — on every single run, unrecoverably, since reinstalling by hand only survived until the next tick. Routines now run under `sys.executable`, the same interpreter as the scheduler, which has no such side effect. Declaring dependencies in `pyproject.toml` remains the correct practice; this only stops the scheduler from actively punishing the ones that aren't. Verified by executing each failure path against this branch and confirming both the JSONL record and the Telegram delivery. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reviewer's GuideThis PR changes the scheduler to run routines using the same Python interpreter as the scheduler and adds durable alerting for routine failures via a JSONL log and optional Telegram notifications, ensuring failures are visible and do not silently break scheduled work. Sequence diagram for routine execution and failure alerting in schedulersequenceDiagram
participant Scheduler
participant run_adw
participant Subprocess as subprocess.run
participant Alert as _alert_failure
participant File as FAILURE_LOG
participant Telegram as send_telegram
Scheduler->>run_adw: run_adw(name, script, args)
run_adw->>run_adw: script_path.exists()
alt script not found
run_adw->>Scheduler: print(..., flush=True)
run_adw->>Alert: _alert_failure(name, "script not found: script")
else script exists
run_adw->>Subprocess: subprocess.run([PYTHON, script_path, args], timeout=900)
alt returncode == 0
run_adw->>Scheduler: print("✓ name", flush=True)
else returncode != 0
run_adw->>Scheduler: print("✗ name", flush=True)
run_adw->>Alert: _alert_failure(name, "exit code + stderr tail")
end
end
alt subprocess.TimeoutExpired
run_adw->>Scheduler: print("timeout (15min)", flush=True)
run_adw->>Alert: _alert_failure(name, "timed out after 15min")
else other Exception
run_adw->>Scheduler: print("error: e", flush=True)
run_adw->>Alert: _alert_failure(name, "scheduler exception: e")
end
Alert->>File: open(FAILURE_LOG, "a")
Alert->>File: write(json.dumps({ts, routine, detail}))
Alert->>Scheduler: print("WARN: could not write", flush=True)
Alert->>Telegram: send_telegram("Routine failed — name\ndetail\n(ts)")
Alert->>Scheduler: print("WARN: Telegram alert not sent", flush=True)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider avoiding repeated
sys.path.insertand dynamic imports inside_alert_failure; importingsend_telegramonce at module level (behind a try/except) or caching it after first use would reduce path churn and make resolution more predictable. - The
subprocess.runcall still relies onshell=Truewith a manually constructed command where onlysys.executableis quoted; usingshell=Falsewith an argument list would simplify handling of spaces in paths/args and avoid subtle quoting/injection edge cases.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider avoiding repeated `sys.path.insert` and dynamic imports inside `_alert_failure`; importing `send_telegram` once at module level (behind a try/except) or caching it after first use would reduce path churn and make resolution more predictable.
- The `subprocess.run` call still relies on `shell=True` with a manually constructed command where only `sys.executable` is quoted; using `shell=False` with an argument list would simplify handling of spaces in paths/args and avoid subtle quoting/injection edge cases.
## Individual Comments
### Comment 1
<location path="scheduler.py" line_range="92-101" />
<code_context>
+ Telegram notification is best-effort and can never take the scheduler down.
+ """
+ ts = datetime.now().isoformat(timespec="seconds")
+ try:
+ FAILURE_LOG.parent.mkdir(parents=True, exist_ok=True)
+ with open(FAILURE_LOG, "a") as f:
+ f.write(json.dumps({"ts": ts, "routine": name, "detail": detail},
+ ensure_ascii=False) + "\n")
+ except Exception as e:
+ print(f" WARN: could not write {FAILURE_LOG}: {e}", flush=True)
+
+ try:
+ sys.path.insert(0, str(WORKSPACE / "ADWs"))
+ from runner import send_telegram
+ send_telegram(f"\U0001F6A8 Routine failed — {name}\n{detail}\n({ts})")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Repeatedly inserting into `sys.path` inside `_alert_failure` can cause subtle global side effects.
Since `_alert_failure` may be invoked frequently, each call inserts a new entry at the front of `sys.path`, changing import resolution order and potentially growing `sys.path` indefinitely, which is risky in a long-lived or multi-threaded scheduler. Consider moving this `sys.path` modification to module import time, or guarding it so the path is only inserted if it’s not already present.
Suggested implementation:
```python
try:
adws_path = str(WORKSPACE / "ADWs")
if adws_path not in sys.path:
sys.path.insert(0, adws_path)
from runner import send_telegram
send_telegram(f"\U0001F6A8 Routine failed — {name}\n{detail}\n({ts})")
except Exception as e:
print(f" WARN: Telegram alert not sent: {e}", flush=True)
```
If `_alert_failure` is called from hot paths and Telegram sending is expected to be available for the lifetime of the scheduler, you may also want to:
1. Move the `adws_path` insertion and `send_telegram` import to module import time (top-level of `scheduler.py`) to avoid repeated imports and path checks.
2. Wrap that top-level import in a `try/except` and fall back to a no-op `send_telegram` to keep `_alert_failure` logic simple.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+92
to
+101
| try: | ||
| FAILURE_LOG.parent.mkdir(parents=True, exist_ok=True) | ||
| with open(FAILURE_LOG, "a") as f: | ||
| f.write(json.dumps({"ts": ts, "routine": name, "detail": detail}, | ||
| ensure_ascii=False) + "\n") | ||
| except Exception as e: | ||
| print(f" WARN: could not write {FAILURE_LOG}: {e}", flush=True) | ||
|
|
||
| try: | ||
| sys.path.insert(0, str(WORKSPACE / "ADWs")) |
There was a problem hiding this comment.
suggestion (bug_risk): Repeatedly inserting into sys.path inside _alert_failure can cause subtle global side effects.
Since _alert_failure may be invoked frequently, each call inserts a new entry at the front of sys.path, changing import resolution order and potentially growing sys.path indefinitely, which is risky in a long-lived or multi-threaded scheduler. Consider moving this sys.path modification to module import time, or guarding it so the path is only inserted if it’s not already present.
Suggested implementation:
try:
adws_path = str(WORKSPACE / "ADWs")
if adws_path not in sys.path:
sys.path.insert(0, adws_path)
from runner import send_telegram
send_telegram(f"\U0001F6A8 Routine failed — {name}\n{detail}\n({ts})")
except Exception as e:
print(f" WARN: Telegram alert not sent: {e}", flush=True)If _alert_failure is called from hot paths and Telegram sending is expected to be available for the lifetime of the scheduler, you may also want to:
- Move the
adws_pathinsertion andsend_telegramimport to module import time (top-level ofscheduler.py) to avoid repeated imports and path checks. - Wrap that top-level import in a
try/exceptand fall back to a no-opsend_telegramto keep_alert_failurelogic simple.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two defects in
scheduler.pythat compound into silent, long-lived outages of scheduled work. We hit both at once and lost a daily routine for 25 days without noticing.1. Routine failures were invisible
run_adwreported failures only throughprint()to buffered stdout. The scheduler's output is normally redirected to a log file, so nothing reached disk until the buffer filled. A routine failing every single day was indistinguishable from a routine that never ran — and the log file sat empty either way.2.
uv run pythonsilently pruned dependenciesRoutines were executed via
uv run, which re-syncs the venv againstuv.lockbefore every run and removes any package not declared inpyproject.toml.The consequence is nastier than it first looks: a routine whose dependency was installed ad-hoc would delete that dependency while starting up, and then fail because of it. Every run. Reinstalling by hand only survived until the next tick, which makes the problem look intermittent or haunted rather than deterministic.
Fix
Failures are now recorded and announced. They append to
ADWs/logs/routine-failures.jsonlwith timestamp, routine name, and the tail of the child's stderr — enough to diagnose without reproducing — and send a Telegram notification through the existingADWs/runner.send_telegram.Ordering is deliberate: the disk record is written first and always; the notification is best-effort and wrapped so a missing token or a dead bot can never take the scheduler down. All three failure paths are covered — missing script, non-zero exit, timeout. The status
print()s also gainflush=True.Routines run under
sys.executable, the same interpreter as the scheduler. No sync, no pruning. Declaring dependencies inpyproject.tomlis still the correct practice; this just stops the scheduler from actively punishing the ones that aren't.Verification
Each failure path was executed against this branch — not merely compiled:
Telegram delivery confirmed separately with credentials loaded (the
⚠ Telegram not configuredline above is the guard working as intended in a bare shell).Notes for reviewers
ADWs/logs/, already gitignored._alert_failureimportssend_telegramlazily and insidetry— no new hard dependency, and installs without Telegram configured are unaffected beyond one warning line.shlex.quoteonPYTHONbecausesys.executablecan contain spaces and the command is built forshell=True.uv runalso had the side effect of installing missing declared deps before each run. Environments that leaned on that will now needuv syncat deploy time — which start-services.sh style bootstraps already do.🤖 Generated with Claude Code
Summary by Sourcery
Improve scheduler reliability by recording routine failures and running routines with the same Python interpreter as the scheduler.
Bug Fixes:
uv run.Enhancements:
sys.executableto align with the scheduler’s runtime environment.