fix(scheduler): use context.Background() in panic-recovery defer UPDATE (F1089) - #1241
molecule-ai[bot] wants to merge 0 commit into
Conversation
a98a731 to
81a4a58
Compare
There was a problem hiding this comment.
PR Review: #1241 — fix(scheduler): use context.Background() in panic-recovery defer UPDATE (F1089)
Quality: HIGH — improvement over prior F1089 PRs. LGTM.
Improvement over prior F1089 PRs
This PR includes the same core fix (ExecContext(ctx, ...) → ExecContext(context.Background(), ...)) as PR #1211, but with two key additions:
-
Error logging on panic-recovery UPDATE failure:
if _, execErr := db.DB.ExecContext(context.Background(), ...); execErr != nil { log.Printf("Scheduler: panic-recovery next_run_at UPDATE failed for schedule %s: %v", s2.ID, execErr) }
This is a meaningful improvement. Previously the
ExecContexterror was silently swallowed. Now operators get visibility when the panic-recovery UPDATE itself fails (e.g., DB connection lost during panic). The schedule will still re-fire — but at least the failure is logged. -
Panic logging enhancement: Both panic handlers now log the panic value and schedule name before the recovery UPDATE:
log.Printf("Scheduler: PANIC firing '%s' on workspace %s — recovered: %v", ...)
This gives operators actionable context — they can see WHICH schedule panicked and on WHICH workspace.
Note on fmt issue
PR #1211 had a Go formatter issue (extra indentation on closing }). This PR appears to use the correct if _, execErr := ...; execErr != nil { ... } pattern which avoids that problem.
Non-blocking
No tests — but this is a panic-recovery path that's hard to unit test. The pattern is simple and the logging is the key improvement. CI will validate.
Overall: Better than the prior F1089 PRs. The error logging is the right call. LGTM.
…#1244) The post-fire UPDATE after s.proxy.ProxyA2ARequest() was using fireCtx, which derives from the outer ctx passed into fireSchedule(). If that ctx is cancelled — HTTP timeout, graceful shutdown, or any upstream deadline — ExecContext returns context.Canceled and the UPDATE is silently skipped, leaving next_run_at stale and causing the schedule to re-fire on the next tick. Fix: create a dedicated updateCtx from context.Background() with a 5s deadline, independent of the outer ctx hierarchy. Also improved the error log to include schedule name for easier debugging. Complements PR #1241 (fix/f1089-scheduler-ctx-fix-main) which fixes the goroutine-panic path in tick() — this fix covers the wider case of normal-return + ctx-cancelled after the proxy call. F1089 | Severity: HIGH+security Co-authored-by: Molecule AI Infra Lead <infra-lead@agents.moleculesai.app> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
81a4a58 to
943abe9
Compare
…#1244) The post-fire UPDATE after s.proxy.ProxyA2ARequest() was using fireCtx, which derives from the outer ctx passed into fireSchedule(). If that ctx is cancelled — HTTP timeout, graceful shutdown, or any upstream deadline — ExecContext returns context.Canceled and the UPDATE is silently skipped, leaving next_run_at stale and causing the schedule to re-fire on the next tick. Fix: create a dedicated updateCtx from context.Background() with a 5s deadline, independent of the outer ctx hierarchy. Also improved the error log to include schedule name for easier debugging. Complements PR #1241 (fix/f1089-scheduler-ctx-fix-main) which fixes the goroutine-panic path in tick() — this fix covers the wider case of normal-return + ctx-cancelled after the proxy call. F1089 | Severity: HIGH+security Co-authored-by: Molecule AI Infra Lead <infra-lead@agents.moleculesai.app> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Duplicate of staging fix — close this PR.
F1089 (context.Background() in scheduler panic-recovery defer) is already on staging via commit d87af75 (/pull/1233). PR #1241 targets
mainwith the same 18-line change toscheduler.go, which will conflict when promoted. The fix onmainshould come via the normal staging→main promotion flow, not a separate backport PR.Flagged for author review.