Repository navigation
Add transactional completion #job_complete_tx(job) for Ruby - #1490
Conversation
521db84 to
b4e1751
Compare
b4e1751 to
792a22a
Compare
|
@bgentry Hey, can you take a look at this one? |
|
@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. |
![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: 792a22a04d
ℹ️ 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".
| completed = @driver.job_get_by_id(job.row.id) | ||
| return false unless completed && completed.state == JOB_STATE_COMPLETED |
There was a problem hiding this comment.
When job_retry runs after the transaction commits but before the original worker or its middleware returns, this lookup can observe the row as available or already running in a replacement attempt. Returning false then sends the old execution through normal finalization; if the replacement has been claimed, line 320 can complete it using the old attempt's metadata while its worker is still executing (and a delete finalizer can similarly delete it). Reconciliation must detect that the attempt changed and avoid finalizing the replacement.
Useful? React with 👍 / 👎.
792a22a to
d289477
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d289477aa2
ℹ️ 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".
| # persisted state because the caller may still roll back this transaction. | ||
| job.__completion_attempted = true | ||
| now = @time_now_utc.call | ||
| completed = @driver.job_complete(id: job.row.id, finalized_at: now, metadata: job.metadata_updates, now: now) |
There was a problem hiding this comment.
When the supplied Job belongs to an old worker whose attempt has been rescued or manually retried and the replacement is already running, this update matches the replacement because job_complete predicates only on the ID and state = 'running'. The stale worker can therefore complete the replacement and merge its own metadata into it. Include the original attempt identity (such as attempt and attempted_at) in the atomic completion predicate rather than checking identity only later during runtime finalization.
Useful? React with 👍 / 👎.
| completed = @driver.job_get_by_id(job.row.id) | ||
| return false unless completed |
There was a problem hiding this comment.
When completed-job retention is configured to zero, or application code deletes the completed row while the worker continues after committing job_complete_tx, this lookup can return nil. Returning false then treats the committed completion like a rollback: normal finalization hooks run and the promised job_completed event is omitted. Preserve the completed snapshot returned by job_complete_tx (while distinguishing calls that failed before completion) so deletion before this lookup follows the same committed-completion path already handled for deletion during the later metadata merge.
Useful? React with 👍 / 👎.
d289477 to
43efc0d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43efc0d5be
ℹ️ 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".
|
|
||
| # Runs the callback only if this transaction and its savepoints commit. | ||
| def after_commit(&block) | ||
| @db.after_commit(savepoint: true, &block) |
There was a problem hiding this comment.
When an application resolves Sequel older than 5.20, which the unchanged sequel > 0 gemspec constraint still permits, after_commit accepts this hash but ignores savepoint: true because savepoint-aware transaction hooks were only added in 5.20 (Sequel changelog). If job_complete_tx is called in a nested savepoint that rolls back while the outer transaction commits, this callback therefore retains a completion snapshot that never committed; if the running row is deleted before the worker returns, finish_transactional_completion can publish a false completion event from that snapshot. Raise the Sequel dependency floor to a version that implements the required hook semantics.
Useful? React with 👍 / 👎.
An omission on the Ruby implementation is that a transactional completion path never made it in. Here, correct the oversight by adding `#job_complete_tx`. The naming is close to Rust's `job_complete_tx` and TypeScript's `completeTx`.
43efc0d to
45203ac
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
An omission on the Ruby implementation is that a transactional
completion path never made it in.
Here, correct the oversight by adding
#job_complete_tx. The naming isclose to Rust's
job_complete_txand TypeScript'scompleteTx.