Skip to content

Let bin/jobs check validate a config for an env it isn't running in, and without a database #780

Description

@kylekeesling

What we're trying to do

Catch a malformed config/recurring.yml in CI, before it reaches production — the #722 failure mode, where a fat-fingered schedule deploys and the worker won't boot. #745 made that legible after the fact; we're trying to catch it beforehand.

bin/jobs check looks made for this ("Validates the Solid Queue configuration for the current Rails env without starting anything"), but two things stop it, and they compound.

1. check can only see the current env's section

Our recurring.yml is keyed production: — it has to be, since Procfile.dev runs bin/jobs locally and we don't want a laptop scheduling production work. CI runs as test, and config_from does:

config = config[env.to_sym] ? config[env.to_sym] : config

With no test: key it falls back to the whole file, yielding one pseudo-task keyed production with no :schedule, which if options&.has_key?(:schedule) then drops. Zero tasks validated, and:

Solid Queue configuration is valid.

Exit 0, with a broken schedule sitting in the file. So the one file whose env scoping is load-bearing is the one file check can't inspect.

Our worker configs don't hit this only because they're unkeyed, which makes the fallback above work in our favour.

env: is already a keyword on config_from, and neither processes_config nor recurring_tasks_config passes it — both inherit Rails.env. Threading a CLI option through would let a test-env run validate the production: section directly. Happy to PR just this.

(We deliberately suggest targeting one env rather than "validate all sections": for recurring files an unkeyed file's top-level keys are task names, so a section named production: is ambiguous without a new shape heuristic. Worker configs escape that via keys: [:workers, :dispatchers, :scheduler]. Targeting composes anyway — call it once per env.)

2. Even with the right section, validation needs a database

An unkeyed copy of the file sidesteps the env problem, and Configuration then sees all 37 of our tasks under RAILS_ENV=test. But validation instantiates RecurringTask AR objects, and the solid_queue tables live in a separate queue database that only dev/production declare:

$ printf 'my_task:\n  class: SomeJob\n  schedule: every 15 minutz\n' > /tmp/recurring.yml
$ RAILS_ENV=test bin/jobs check --recurring_schedule_file /tmp/recurring.yml
PG::UndefinedTable: ERROR:  relation "solid_queue_recurring_tasks" does not exist
  ... (144 lines total)

This looks like a plain bug: check already anticipates no database ten lines above, in warn_about_incorrectly_sized_database_pool:

rescue ActiveRecord::ActiveRecordError
  # No usable database connection. Skip the pool-size warning in that case.
end

ensure_valid_recurring_tasks sits just above it, unguarded.

The two together are why we couldn't use check: (1) means it looks at the wrong section, and (2) means fixing that still isn't enough in an env with no queue database.

On 1.5.1; config_from and the unguarded validation are unchanged on main. --only-recurring (#757) doesn't help — it narrows which processes get built, so both blockers still apply.

The workaround we're using

A bin/rails runner step in CI that reads the YAML and reimplements the three validations, needing neither a database nor the matching Rails.env:

tasks.each do |id, t|
  bad << "#{id}: needs either a class: or a command:" unless t["class"] || t["command"]
  bad << "#{id}: class #{t["class"].inspect} does not resolve" if t["class"] && !t["class"].safe_constantize
  begin
    bad << "#{id}: bad schedule #{t["schedule"].inspect}" unless Fugit.parse(t["schedule"], multi: :fail).instance_of?(Fugit::Cron)
  rescue ArgumentError => e
    bad << "#{id}: #{e.message}"
  end
end

It works, but it's a mirror of RecurringTask's validations maintained outside the gem, so it drifts silently when those change. We'd rather call check.

It's also evidence for the second half: it validates all 37 tasks with no database at all. We diffed its verdicts against RecurringTask#valid? across 12 task shapes — good cron, good natural-language, command-only, unparseable schedule, "multiple crons", bogus time zone, out-of-range field, empty schedule, unresolvable class, neither class nor command, nil body, missing schedule key — and got 12 agree, 0 diverge.

Suggested direction for (2)

The three validations read only pure attributes (Fugit.parse, safe_constantize, a presence check), and Configuration uses recurring_tasks for just two things: validation (recurring_tasks.select(&:invalid?)) and handing tasks to the scheduler. Only the second needs an AR object.

So: extract the validations onto a plain object including ActiveModel::Validations (RecurringTask::Definition, or whatever you'd prefer). Configuration validates those — no table required — and RecurringTask includes the same module or builds from one, leaving persisted and dynamic tasks unchanged. The validate calls move verbatim, since ActiveModel::Validations on a PORO needs no columns.

Minor, and separable: a warning when a recurring file was found and skip_recurring wasn't set but zero tasks resolved. "Configuration is valid" after validating nothing is what cost us the most time; may fit the errors/warnings work in #759.

Glad to take a shot at any of it — wanted to check direction before sending a refactor. Thanks for solid_queue.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions