Update fork to upstream solid_queue v1.4.0 - #2
Conversation
- Updated Appraisals to include a new entry for Rails 8.1 with the appropriate railties version. - Modified GitHub Actions workflow to include the new Rails 8.1 gemfile in the CI process. - Created a new gemfile for Rails 8.1 to manage dependencies. This ensures compatibility with the latest Rails version and enhances CI testing.
Testing against Rails 8.1
Remove deprecated loading of `statistics.rake` from Rakefile
This reverts commit 5aa05ae.
Process#register and #degister for the supervisor to start up properly. RecurringSchedule#reload_tasks resolves all the records immediately to avoid deferred resolution outside the executor block. Supervisor#handle_claimed_jobs_by wraps its code in the executor. A second attempt at rails#655 without causing the issues from rails#670
Default logging levels changed in rails/rails@308594b5
…rails#661) * print warning on startup if path to configuration file does not exist * change `assert_predicate` to `assert` Co-authored-by: Rosa Gutierrez <rosa.ge@gmail.com> --------- Co-authored-by: Rosa Gutierrez <rosa.ge@gmail.com>
Generally in the README, it uses "we" and makes statements like "we recommend". Changing the instances of "I'd" to "we" seems to better match the rest of the document and feels like better phrasing for a project README. [skip ci]
…onsistency Improve Consistency of README
This PR notes that `SKIP LOCKED` is supported on MariaDB 10.6 and newer, and that one may need to set `config.solid_queue.use_skip_locked = false` for MariaDB versions < 10.6. I ran into this when setting up `solid_queue` on Slackware64 15.0, which is shipping MariaDB 10.5 series still.
For some reason this stopped working in CI and we don't really need it. I'm not sure why we did it in this way instead of using `mocha` ^_^U Rails still requires it, so let's make sure it's loaded in the test_helper.
And minitest 6.x requires Ruby 3.2+
…on_commit` See rails/rails#55788 This setting has gone through different stages, and we must account for them: - Rails 7.1: Method not available (no deferred enqueue) - Rails 7.2: :default (defers to adapter's method, which returns true in Solid Queue's case) - Rails 8.0-8.1: false (no deferred enqueue, the setting was deprecated) - Rails 8.2: true (deferred enqueue enabled by default)
Remove extra blank lines, column-aligned hashes, and inline comments on test assertions. Revert unrelated Gemfile.lock platform addition. Filter :static from schedule_task options since dynamic tasks are always non-static by definition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dynamic tasks require explicit `dynamic_tasks: true` in the scheduler config. Without it, the scheduler behaves as before — no extra DB queries, no polling for dynamic changes. Default polling interval changed from 1s to 5s. Rename schedule_task/unschedule_task to schedule_recurring_task/ unschedule_recurring_task so the API clearly communicates these are recurring tasks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tasks_enabled Have from_configuration read static from options instead of hardcoding it. Callers that create static tasks (YAML config) pass static: true via reverse_merge. schedule_recurring_task passes static: false directly. Rename the config key from dynamic_tasks to dynamic_tasks_enabled for clarity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep the longer sleep interval when dynamic tasks are disabled. Add tests for `dynamic_tasks_enabled` opt-in in configuration and for verifying dynamic tasks are ignored when not enabled.
We don't need to track dynamic task changes, we can just reload the metadata in every case. If it hasn't changed, Rails won't issue any new update to the process record. Also, we can just use `dynamic_tasks` everywhere, as an empty AR relation if dynamic tasks are disabled, avoiding extra queries but keeping the code simpler.
We need the dynamic task keys there and were doing a new query every time. We only need to do a query when explicitly reloading them.
We want to set that value overriding whatever we get in options, so it should be merged into options, not the other way around. Co-Authored-By: Vladyslav Davydenko <vladyslav@hey.com>
Without `#to_a`, `select_candidates` returns an `ActiveRecord::Relation`. Consequently, `executeions.none?` inside the `lock_candidates` method triggers an unintended query: ``` SELECT 1 AS one FROM `solid_queue_ready_executions` LIMIT 1 FOR UPDATE SKIP LOCKED; ```
…asks Dynamic scheduled tasks
mike-leone-wealthbox
left a comment
mike-leone-wealthbox
left a comment
There was a problem hiding this comment.
Not sure if we should really be reviewing this line by line, but given that this isn't any code we're writing ourselves and an upstream update, I'm approving it.
Looks like we won't need this fork after SQ 2.0!
nathanstpierre-wealthbox
left a comment
nathanstpierre-wealthbox
left a comment
There was a problem hiding this comment.
Threw claude at it, and I think it mostly makes sense. Here's that for posterity:
Here's my analysis of the PR. It's a big upstream jump (v1.2.2 → v1.4.0, 63 files changed) but the PR author did a trial rebase with zero conflicts. That said, there
are a few things worth verifying when batch-rc-2 gets rebased:
Things to Watch
- Supervisor kind name changed — potential query breakage
The supervisor kind field changed from "Supervisor" to "Supervisor(fork)" / "Supervisor(async)". Your custom ProcessCrashRecovery
(app/lib/solid_queue/process_crash_recovery.rb) interacts with SolidQueue::Process records. If it queries by kind, it'll need updating. Same for any admin panels or
monitoring that filter by supervisor kind.
- Supervisor class hierarchy refactored
Supervisor was split into ForkSupervisor and AsyncSupervisor subclasses. The @forks hash is now @process_instances, and methods like reap_and_replace_terminated_forks
are now check_and_replace_terminated_processes. If the batch branch patched or extended Supervisor internals, those customizations may need reworking even if the
rebase itself doesn't conflict textually.
- FailedExecution recording changed
create_or_find_by! was replaced with create! inside a nested transaction + rescue ActiveRecord::RecordNotUnique. Your ProcessCrashRecovery auto-retries failed jobs —
make sure the retry flow still works correctly with the new error-recording path, since the before_create callback became before_save.
- Semaphore now acquires a row lock on wait
Semaphore.find_by → Semaphore.lock.find_by. This is a correctness fix for a race condition in concurrency controls. Since crm-web uses ProcessQueue::ConcurrencyKey and
limits_concurrency, this is a good fix but worth a quick sanity check under load — the added FOR UPDATE could increase lock contention on high-throughput queues.
- ReadyExecution.select_candidates forces .to_a
This prevents an unintended FOR UPDATE on the full query chain. It's a fix, but changes query materialization timing. If the batch branch modified this query path,
watch for interaction.
- Recurring task schedule validation is stricter
Fugit.parse(schedule) → Fugit.parse(schedule, multi: :fail). Schedules that silently generated multiple crons will now raise. Your four recurring tasks in
config/recurring.yml use every hour at minute 12, every 5 minutes, etc. — these are all single-cron expressions, so you should be fine.
- use_index optimizer hints (MySQL-specific)
The blocked execution release query now includes a MySQL index hint via optimizer_hints. Since crm-web uses PostgreSQL, this is a no-op (PG ignores MySQL hint
comments). No concern here.
What Looks Safe
- Async mode: New feature, opt-in only. crm-web defaults to fork mode via ${SOLID_QUEUE_SUPERVISOR_MODE:-fork} — no behavioral change.
- Dynamic recurring tasks: Also opt-in via dynamic_tasks_enabled: true. Not enabled by default.
- Puma plugin refactoring: crm-web runs solid_queue as separate processes (Procfile), not via the Puma plugin. No impact.
- enqueue_after_transaction_commit skip logic: crm-web sets :never on SolidQueueApplicationJob, so the changed test skip conditions don't affect you.
- Nil-safe heartbeat (process&.heartbeat): Strictly more robust.
- Guard against nil pid in Puma plugin stop: Now checks return unless solid_queue_pid before Process.waitpid. Good defensive fix.
Recommendation
Items 1 and 2 are the most likely to bite you after the rebase onto batch-rc-2. I'd specifically:
- Grep batch-rc-2 for any references to "Supervisor" as a kind string or any direct patches to Supervisor's internal methods (@forks,
reap_and_replace_terminated_forks, handle_claimed_jobs_by, etc.) - Run the ProcessCrashRecovery tests after the rebase
- Do a production smoke test of concurrency-controlled jobs to validate the semaphore locking change under real load
Overall the upstream changes are solid bug fixes and good architectural improvements. Nothing that should destabilize your fork as long as the rebase touchpoints above
are checked.
20 participants
Linear: SRE-138
Summary
Brings our fork up to date with rails/solid_queue v1.4.0. We were pinned at v1.2.2 (90 commits behind).
Notable upstream changes (v1.2.3 → v1.4.0):
FOR UPDATEquery executionWhy now: Our
batch-rc-2branch (used by crm-web) is based on v1.2.2. Updating main first lets us rebase the batch work onto v1.4.0. Trial rebase showed zero conflicts.Next steps after merge:
batch-rc-2onto updated mainbatch-rc-2🤖 Generated with Claude Code