Repository navigation
Bring Ruby into the main repo - #1463
Conversation
a37d4df to
35ca222
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| - Expanded checks against Go-generated conformance fixtures to cover cron schedules, snooze counters, all shared metadata keys, and queue-control and leadership notification emission through both SQL drivers. | ||
| - Added `CRON_TZ=` / `TZ=` prefixes, `?` wildcards, and Go-style `@every` durations to `PeriodicCron`. | ||
|
|
||
| ### Fixed | ||
|
|
||
| - Snoozing tolerates nonnumeric metadata counters and follows Go's JSON integer conversion instead of failing on booleans or collections. | ||
| - Periodic jobs now carry `periodic: true` and, when named, `river:periodic_job_id`, while preserving constructor options and application metadata. | ||
| - Queue pause, resume, metadata changes, and leader resignation now broadcast notifications for clients in other languages. Notifications commit and roll back with the corresponding database change. | ||
| - Cron schedules include both occurrences of repeated daylight-saving times when given a local reference time. |
There was a problem hiding this comment.
I'm seeing that these new entries are all missing PR numbers.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35ca222faa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| max_attempts: insert_opts.max_attempts || args_insert_opts.max_attempts || MAX_ATTEMPTS_DEFAULT, | ||
| metadata: (args_insert_opts.metadata || {}).merge(insert_opts.metadata || {}), | ||
| priority: insert_opts.priority || args_insert_opts.priority || PRIORITY_DEFAULT, | ||
| queue: (insert_opts.queue || args_insert_opts.queue || QUEUE_DEFAULT).to_s, |
There was a problem hiding this comment.
When an insertion supplies a queue such as "billing queue", this line persists it because the database schema only enforces queue length. The same name is rejected by Config and queue_add via QUEUE_NAME_REGEX, so no Ruby worker can configure that queue and the successfully inserted job is stranded. Apply the configured-queue name validation to insertion queues before calling the driver.
Useful? React with 👍 / 👎.
| if unique_opts.by_period && unique_opts.by_period != 0 | ||
| lower_period_bound = truncate_time(insert_params.scheduled_at || @time_now_utc.call, unique_opts.by_period).utc | ||
|
|
||
| unique_key += "&period=#{lower_period_bound.strftime("%FT%TZ")}" |
There was a problem hiding this comment.
Any nonzero by_period reaches this calculation without validation, so values such as 0.5 are accepted even though the resulting boundary is formatted only to whole seconds and distinct sub-second periods therefore generate the same uniqueness key. Negative periods are accepted as well, while the Go client explicitly rejects every nonzero period below one second in insert_opts.go. Reject these values before generating the key to preserve the advertised cross-language uniqueness semantics.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3eb6c4d30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if now >= next_schedule | ||
| @driver.job_schedule(now: now) | ||
| run_periodic(now) | ||
| @config.maintenance_services.each { |service| service.run(@client, @driver, now) } |
There was a problem hiding this comment.
When a configured maintenance service raises on every invocation, this exception exits the loop before job_rescue_stuck and job_delete_finalized; the method-level rescue then retries while the same client continues reacquiring leadership. A broken optional service can therefore indefinitely prevent stuck jobs from being rescued and finalized jobs from being cleaned, while also preventing a healthy client from taking over. Isolate service failures or run the core rescue and cleanup tasks independently.
Useful? React with 👍 / 👎.
| args: args, | ||
| encoded_args: args_json, | ||
| kind: args.kind.to_s, | ||
| max_attempts: insert_opts.max_attempts || args_insert_opts.max_attempts || MAX_ATTEMPTS_DEFAULT, |
There was a problem hiding this comment.
When max_attempts is 0 or negative, Ruby treats the integer as truthy and passes it directly to the driver. PostgreSQL rejects it, but the current SQLite schema has no positive-value constraint, so SQLite persists a job with an invalid attempt limit and the first failed attempt is immediately considered exhausted. Validate that the effective value is positive before constructing the insert parameters so behavior remains consistent across drivers.
Useful? React with 👍 / 👎.
| when :metadata | ||
| runtime_json(value) |
There was a problem hiding this comment.
When job_update is called with metadata: nil, this encodes the JSON literal null rather than rejecting it or replacing it with an object. The database update commits successfully because JSON null is not SQL NULL, but the subsequent row decode requires metadata to be a hash and raises JobRowDecodeError, leaving the persisted job unreadable to administrative calls and workers. Validate metadata as an object before issuing the update.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ea568ae37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return :cancelled if job_get_cancelled_ids([id]).include?(id) | ||
|
|
||
| job_set_state_if_running(id: id, finalized_at: finalized_at, metadata: metadata, now: now, state: "completed") |
There was a problem hiding this comment.
When job_cancel commits after this cancellation probe but before job_set_state_if_running, the completion update still changes the now-marked running job to completed. This loses a cancellation that already won the database race and leaves cancel_attempted_at on a completed row; combine the marker check with the state transition so a committed cancellation is finalized as cancelled.
Useful? React with 👍 / 👎.
| when :attempt, :max_attempts | ||
| Integer(value).to_s |
There was a problem hiding this comment.
When Client#job_update receives max_attempts: 0 or a negative value, this conversion accepts it. PostgreSQL rejects the update through its positive-value constraint, but the bundled SQLite schema only makes this column non-null, so SQLite persists the invalid limit and the next failed attempt is immediately exhausted. Validate positive values here, as the insertion path already does, to keep both drivers consistent.
Useful? React with 👍 / 👎.
| completed_at = Time.now.utc | ||
| stats = JobStatistics.new(0, started_at - job.scheduled_at, completed_at - started_at) |
There was a problem hiding this comment.
Every job lifecycle event hard-codes complete_duration to zero, regardless of how long the database finalization takes, while run_duration includes that finalization interval. Metrics consumers therefore cannot observe completion latency and receive inflated execution timing; capture the worker-finished timestamp separately from the post-finalization publication timestamp.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed3cfddda5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @driver.job_delete_if_running(row.id) | ||
| return [:deleted, nil] |
There was a problem hiding this comment.
Although job_complete now checks the cancellation marker atomically, the :delete finalization branch still does not: when job_cancel commits after the probe on line 286 but before this call, the row remains in running state with cancel_attempted_at, so job_delete_if_running deletes it and the runtime reports :deleted. This irreversibly loses a cancellation that already won the database race; make the delete predicate reject cancellation-marked rows and route that result through normal cancellation finalization.
Useful? React with 👍 / 👎.
| def queue_update(name, metadata:, now: Time.now.utc) | ||
| transaction do | ||
| row = runtime_query_rows(<<~SQL).first | ||
| UPDATE river_queue SET metadata = #{runtime_json(metadata)}, updated_at = #{runtime_time(now)} |
There was a problem hiding this comment.
When queue_update receives metadata: nil, an array, or another non-Hash JSON value, this statement persists it because both schemas accept any non-SQL-NULL JSON value. On SQLite the subsequent decode returns a Queue whose metadata is not the declared Hash, leaving that invalid value stored for future readers; PostgreSQL adapters may instead fail while decoding and roll back, making behavior driver-dependent. Reject non-Hash metadata before issuing the update, as the job-update path already does.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15890985a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| state = state.to_s | ||
| retrying = [River::JOB_STATE_AVAILABLE, River::JOB_STATE_RETRYABLE, River::JOB_STATE_SCHEDULED].include?(state) | ||
| cancel_path = retrying ? runtime_cancel_attempted : "false" |
There was a problem hiding this comment.
When job_cancel commits after a last-attempt worker error but before this state update, the requested state is discarded, so retrying is false and cancel_path is hard-coded to false. The update then overwrites the still-running, cancellation-marked row as discarded, leaving cancel_attempted_at on a discarded job and reporting a failure instead of cancellation. The newly guarded completion and deletion paths do not cover this failure-finalization race; discarded transitions must check the marker atomically as well.
Useful? React with 👍 / 👎.
| finalized_at: final ? now : nil, | ||
| metadata: {"river:rescue_count" => rescue_count + 1}, | ||
| now: now, | ||
| scheduled_at: final ? nil : retry_policy.next_retry(job, error, now: now), |
There was a problem hiding this comment.
If a configured retry policy raises or returns an invalid value for a non-final stuck job, this unguarded callback aborts and rolls back the entire rescue batch. The maintenance loop then retries the same stuck row before reaching finalized-job and notification cleanup, so one persistently broken policy can indefinitely block recovery of every job in the batch and all later cleanup, despite ordinary attempt finalization falling back to the default policy. Apply the same per-job fallback when calculating rescue times.
Useful? React with 👍 / 👎.
| queues = @mutex.synchronize { @queue_configs.dup } | ||
| queues.each { |name, queue_config| start_producer(name, queue_config) } | ||
| start_maintenance unless @queue_configs.empty? |
There was a problem hiding this comment.
When a client is started with periodic_jobs but queues: {}, this condition skips the only maintenance thread, so even run_on_start jobs are silently never constructed or inserted; later bundle additions merely wake a nonexistent thread. Config accepts this combination and periodic production does not require the same process to consume a queue, so maintenance should start when periodic jobs are present, or the configuration should be rejected explicitly.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b418e4238a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| max_attempts = insert_opts.max_attempts || args_insert_opts.max_attempts || MAX_ATTEMPTS_DEFAULT | ||
| raise ArgumentError, "max_attempts must be greater than zero" unless max_attempts > 0 |
There was a problem hiding this comment.
When max_attempts exceeds 32,767, this validation accepts it even though PostgreSQL defines the column as smallint; PostgreSQL therefore raises an out-of-range error while SQLite's unbounded integer column persists the same job. This makes documented positive values behave differently across drivers, so normalize to the signed-smallint maximum (as the Go drivers do) or reject larger values before insertion; the update path should enforce the same bound.
Useful? React with 👍 / 👎.
| queues = (name == "*") ? @driver.queue_list : [@driver.queue_get(name)].compact | ||
| queues.each do |queue| | ||
| @runtime.publish_queue(EVENT_QUEUE_PAUSED, queue) |
There was a problem hiding this comment.
When more than 100 queues exist, pausing "*" updates every row but this unparameterized queue_list call uses the driver's default limit of 100, so local subscribers never receive pause events for the remaining queues; the identical resume path has the same truncation. Fetch all affected queues or publish a wildcard event so subscription state matches the database operation.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44f7d64977
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| scheduled_at = insert_opts.scheduled_at || args_insert_opts.scheduled_at | ||
| now = @time_now_utc.call | ||
| state = (insert_opts.state || args_insert_opts.state || ((scheduled_at && scheduled_at > now) ? JOB_STATE_SCHEDULED : JOB_STATE_AVAILABLE)).to_s #: jobStateAll # rubocop:disable Layout/LeadingCommentSpace |
There was a problem hiding this comment.
When a caller or extension supplies state: :running, this inserts a row with attempt = 0 and attempted_at = NULL. Producers only claim available jobs, while stuck-job rescue requires attempted_at < horizon, so this successfully persisted job can never execute or be rescued. Restrict explicit insertion states to valid initial states such as available, scheduled, and pending.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 602289d38c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| last_fetch = 0.0 | ||
| poll_interval = queue_config.resolved_fetch_poll_interval(@config) | ||
| loop do | ||
| break if queue_stopping?(queue) |
There was a problem hiding this comment.
When stop or queue_remove is requested while a job is running, this check terminates the producer before its next check_remote_cancellations call—the only code path that interrupts workers for externally persisted cancellation markers. A cancellation committed during the subsequent drain therefore cannot interrupt the attempt, so these calls wait for natural completion or a timeout and can block indefinitely when the worker timeout is disabled. Keep cancellation observation alive until the queue's active attempts have drained.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5179fd44bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if error | ||
| finish_failed(row, job, error, timing) |
There was a problem hiding this comment.
When a batch extension reports an error through __finish_claimed_job, this call passes no worker to finish_failed, so retry_allowed? sees nil and always permits a retry. A registered worker whose retry?(job, error) returns false—including the Active Job worker—therefore has the externally executed job moved to retryable instead of immediately discarded. Resolve and pass the registered worker here as the normal execute path does.
Useful? React with 👍 / 👎.
| def finish_claimed(row, error = nil) | ||
| timing = JobTiming.new(Time.now.utc, monotonic_now, row.scheduled_at) |
There was a problem hiding this comment.
When an extension claims a job, executes it for a significant interval, and only then calls __finish_claimed_job, initializing timing with the current time makes queue_wait_duration include that entire execution interval and records a failed attempt's timestamp at finalization. The claimed row already has attempted_at, which represents the execution start required by the JobStatistics contract; use it for wall-clock timing while retaining the intentionally zero monotonic run duration.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Thanks! Going to pull this, cut a release for its new source location (and for the many fixes now included), then make a note of the move in the old repo. |
This brings the code from our secondary Ruby Git repo into the main one: https://github.com/riverqueue/riverqueue-ruby I've had Codex do a pass to bring it into line with CI, testing, and tooling conventions established by Ruby and TypeScript, then also check that we're in maximum compliance with the conformance test suite. The idea with this one is that after pulling Ruby in here we'll archive the original.
2 participants
This brings the code from our secondary Ruby Git repo into the main one:
https://github.com/riverqueue/riverqueue-ruby
I've had Codex do a pass to bring it into line with CI, testing, and
tooling conventions established by Ruby and TypeScript, then also check
that we're in maximum compliance with the conformance test suite.
The idea with this one is that after pulling Ruby in here we'll archive the
original.