Uh oh!
There was an error while loading. Please reload this page.
Installer/doctor: ship Oban.Plugins.Lifeline to host apps - #662
Conversation
Generated Oban config (and the ensure-path phoenix_kit.update uses for
existing configs) now includes {Oban.Plugins.Lifeline, rescue_after:
:timer.minutes(30)}: a job orphaned in :executing by a hard crash
(kill -9, OOM, node failure) is rescued back to :available instead of
sitting stuck forever. Matters doubly for unique self-scheduling
workers whose unique states include :executing — an orphan would
otherwise block every future insert for that worker, killing the chain.
phoenix_kit.doctor warns when a keyword Oban config lacks the plugin.
(--no-verify: full pre-commit dialyzer run exceeds the shell timeout in
this worktree; credo/tests run separately — installer suites green, the
4 common_test failures reproduce on clean upstream/main and are a
pre-existing Rewrite/Igniter regression.)
timujinne
left a comment
There was a problem hiding this comment.
Code review
This PR adds Oban.Plugins.Lifeline to new installs, to existing hosts via mix phoenix_kit.update, and to phoenix_kit.doctor as a missing-plugin warning. The intent is sound — Lifeline prevents jobs from being stranded in :executing after a hard node crash — but the regex-based upgrade path in ensure_lifeline_plugin/2 corrupts real PhoenixKit-generated configs because it stops at the first ] inside the plugins: block, which is the Cron plugin's nested crontab: [...] list. That makes the update path unsafe and the current test suite misses the shape entirely.
Verdict: REQUEST-CHANGES
Findings
[MAJOR]
lib/phoenix_kit/install/oban_config.ex:574— Theplugins:block regex~r/(^[ \t]+plugins:\s*\[\n)(.*?)(\n[ \t]+\])/msis terminated by the first\n<spaces>]it encounters. In a real generated config the Cron plugin containscrontab: [...], so the match ends inside the Cron tuple and the Lifeline tuple is inserted there. Runningensure_lifeline_plugin/2against a config that contains{Oban.Plugins.Cron, crontab: [...]}produces invalid Elixir (the Lifeline tuple becomes an element of the crontab list). Becausephoenix_kit.updatereaches this function throughadd_oban_configuration/2→update_existing_oban_config/3, every existing host that already has the Cron plugin is at risk. The regex needs to close at the indentation level of theplugins:keyword (capture and backreference the leading whitespace) or, more robustly, count brackets.[MAJOR]
test/phoenix_kit/install/oban_config_test.exs:104— The new tests only exercise plugins lists containing a single Pruner entry. They do not cover the generated shape that includes{Oban.Plugins.Cron, crontab: [...]}, so the bug in finding #1 passes the suite. Add a test with the actual Cron/crontab shape before merging.[MINOR]
lib/phoenix_kit/install/oban_config.ex:586-589— The inserted Lifeline line is hardcoded to four leading spaces. The generated PhoenixKit template indents plugin entries with six spaces, so the result parses but leaves the config visually inconsistent. Infer the indentation from the capturedplugins:prefix or from the existing entries.[MINOR]
lib/phoenix_kit/install/oban_config.ex:281— When the config is changed, the success message says"✅ Updated Oban configuration (queues, cron plugin, pruner retention)", which omits Lifeline even though it was just added. Align it with the already-up-to-date message on line 278.[NOTE]
lib/mix/tasks/phoenix_kit.doctor.ex:676-695— The Lifeline detection correctly recognizes both{Oban.Plugins.Lifeline, _opts}and the bare module form, and the non-keyword config fallback is appropriate. No issue here.
The lazy match to the first ']' stopped inside the Cron plugin's nested crontab list — present in every generated config — inserting Lifeline there and corrupting the file. The closing bracket is now matched via an indentation backreference (nested lists are always deeper), the inserted entry inherits the block's own indent (+2), and the updated success message names Lifeline. New tests: the real Cron/crontab shape (parses via Code.string_to_quoted) and indent inheritance.
timujinne
commented
Jul 25, 2026
Review findings addressed:
|
Post-merge review of PR #662 (Oban.Plugins.Lifeline installer support). - Raise the shipped rescue_after from 30 to 60 minutes. Lifeline rescues purely by elapsed time and never checks whether the executing node is alive, so rescue_after must exceed the longest job a host can run. PhoenixKit ships a 30-minute worker (Storage.Workers.SyncFilesJob) and every worker without a timeout/1 callback has no bound at all — at 30 minutes a long delivery, import or sitemap run would be flipped back to :available and re-executed concurrently with the still-running original. 60 minutes is Oban's own default and 2x the longest declared timeout. - Anchor add_cron_plugin_to_plugins/2's plugins-block close to the keyword's indentation, the same fix PR #662 applied to its sibling but left here. The un-anchored pattern binds to the first nested list's bracket, corrupting a host config whose plugins list holds an entry with a list option. - Stop phoenix_kit.doctor's Oban check raising on queues:/plugins: false, Oban's documented way to disable either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second review round on PR #662, plus the verified items from issue #663. Lifeline (external review of 1.7.212): - Validate rescue_after by value, not just presence. 1.7.212 raised it at every emit site but the installer and doctor still only asked whether a Lifeline entry existed — and Oban's own docs advertise :timer.minutes(5) as their "more aggressive period" example, so a host copying from them sat in the duplicate-execution window while doctor reported a clean pass. Doctor now warns below the 30-minute floor (an unset rescue_after means Oban's safe 60-minute default); ensure_lifeline_plugin/2 raises a too-low :timer.minutes(N) literal instead of no-oping, and leaves any other expression alone rather than blind-editing it. - Stop the plugins: false false positive. 1.7.212 fixed the crash but let false fall through as [] into the Lifeline branch, telling a deliberately plugin-less node to rewrite its config.exs. - Discover workers at runtime in the invariant test instead of hardcoding three modules. Issue #663: - V157 down/1 now raises an actionable error when kind='image' rows exist, before queueing any DDL, rather than dying with a bare 23514 mid-rollback. - Add v157_test.exs pinning the CHECK's whole kind vocabulary. - Tessera CDN pin left alone deliberately: no v0.3.4 tag exists upstream (the suggested bump 404s) and the asset is byte-identical across v0.3.1, v0.3.2 and hex 0.3.4. Recorded in the investigation doc. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Companion to BeamLabEU/phoenix_kit_emails#21 (pollers moving to Oban unique jobs) — but valuable standalone.
Problem
The installer-generated host Oban config has Pruner and Cron but no Lifeline: a job orphaned in
:executingby a hard crash (BEAMkill -9, OOM, node failure, deploy restart) is never rescued — it sits stuck forever. Observed live: 10 zombieSQSPollingJobrows accumulated in a dev database from server restarts. With unique self-scheduling workers (the emails pollers after #21) the stakes rise: if a worker's uniquestatesever includes:executing, one orphan permanently blocks every future insert for that worker.Changes
Install.ObanConfig): generatedplugins:now includes{Oban.Plugins.Lifeline, rescue_after: :timer.minutes(30)}, with a comment.ensure_lifeline_plugin/2mirrors the existingensure_pruner_max_age/2—phoenix_kit.updateadds the plugin to existing host configs that lack it (regex-append into theplugins:block; graceful skip + manual instruction when the block can't be parsed).phoenix_kit.doctor: warns when a keyword Oban config has no Lifeline entry, with the exact snippet to add.No migrations involved — Lifeline is a runtime plugin over the existing
oban_jobstable.Tests
New
ensure_lifeline_plugincases inoban_config_test.exs(adds when missing — both trailing-comma shapes; no-ops when present in tuple or bare-module form). Installer suites green; the 4common_test.exsfailures ("no source found for mix.exs") reproduce identically on clean upstream/main — a pre-existing Rewrite/Igniter regression unrelated to this change.