Repository navigation
[go, java, js, rust] Alternative implementation for reconciling jobs against notifier reconnect - #1564
Conversation
Run reconnect reconciliation asynchronously with a bounded context and reject stale execution results. Keep local cancellation lifecycle-aware without blocking on shutdown locks, and clear old cancellation metadata on retry across drivers. Add deterministic regression coverage and shorten comments and changelog entries.
…lation poll fallback `JobCancel` delivers a cancellation to a running job with a `NOTIFY`, which is lost if the client's notifier isn't listening when it's sent, like while it's reconnecting. The job then keeps running until the rescuer finds it, an hour later by default. Rather than recover after reconnects, reuse a mechanism River already has. Since Yugabyte support (#1347), producers in poll-only mode run `pollForJobCancellations`, which looks up the producer's own running job IDs by primary key and cancels any with `cancel_attempted_at` set. It runs in its own goroutine, keeps running while jobs drain during shutdown, and hands cancellations to the existing `cancelCh`. Clients with a notifier now run it too, once a minute instead of every two seconds. This matches how River already treats insert notifications: producers poll for jobs every second even while listening. Cancellation was the only notification-driven path without a polling fallback. Notifications still deliver cancellations right away, and the poll only matters when one is lost. It runs on a timer rather than reacting to disconnects, because a producer can't tell that a notification was lost, so a lost cancellation arrives within a minute. It reads through the normal pool, so it works even while the listener is down. It also recovers notifications dropped for reasons other than reconnects, like a full notifier or producer buffer. It costs one primary key lookup a minute per producer with running jobs, and nothing for idle producers. Cancelling a job that's already cancelled does nothing, so finding it again is harmless. This replaces the notifier recovery hooks, execution identity in notification payloads, the reconnect reconciliation and its retries, tracking of claimed jobs whose executors aren't registered yet (the next poll finds them), and keeping the notifier running through shutdown. Local delivery for poll-only clients isn't needed because polling covers those clients on master. A check triggered by reconnects would avoid the steady query, but it needs a notifier hook in every language and misses losses that aren't reconnects. Retrying a job now clears an earlier attempt's `cancel_attempted_at` so that a retried job isn't cancelled again by the poll, by Java's check when a job starts, or by JavaScript's check after a reconnect. Ruby already did this. A poll result could still reach a newer attempt of the same job if it's retried and claimed again by the same producer within one query's round trip. A `NOTIFY` has the same race today. Rust gets the same once-a-minute check. JavaScript already rechecks running jobs after its notification stream reconnects, and Java and Ruby already poll while notifications are on, so Java and JavaScript change only to clear the marker on retry.
…nterval Go's cancellation polling test set both poll intervals to the same value, so it passed even if poll-only mode used the one-minute interval meant for clients with a notifier. Set the interval each mode shouldn't use to an hour so the test fails if it's used. Rust had no test that a client with notifications waits for its interval rather than checking on every two-second tick. Add one, using the default interval, that a cancellation request without a notification isn't acted on right away while a notified one still is.
brandur
changed the title
|
@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: f5808198e9
ℹ️ 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 with a notifier too, in case a cancel notification is lost. | ||
| subroutineWG.Add(1) | ||
| go p.pollForJobCancellations(subroutineCtx, &subroutineWG) |
There was a problem hiding this comment.
When this new notifier-backed fallback observes a cancellation for attempt N, that worker can finish and JobRetry can clear the marker and start attempt N+1 before the queued ID is consumed. Because pollForJobCancellationsOnce sends only the job ID and maybeCancelJob resolves whichever executor currently owns that ID, the delayed result can cancel N+1, defeating this change's retry guarantee. Preserve the executor/attempt identity captured before the query or recheck the durable marker before cancelling; the Rust polling path has the same ID-only ABA race.
AGENTS.md reference: AGENTS.md:L36-L40
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated: Poll now snapshots each running attempt's executor before the query and cancels only that. A retry runs under a new executor so it won't be hit.
…ID later The cancellation poll looked up running jobs by ID, then cancelled whatever attempt was running under each requested ID once the query returned. If a job finished, was retried, and had its new attempt claimed by the same client during that window, the poll could cancel the retry even though `JobRetry` had cleared its cancellation request. Rust could also hold a requested ID as a pending cancellation for up to a minute and apply it to an attempt claimed later. Snapshot each running attempt's executor (Go) or cancellation token (Rust) before the query, and cancel the snapshotted one. Cancelling an attempt that's already finished does nothing to its job. In Go this also means the poll no longer goes through the producer's main goroutine.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fa864f6e4
ℹ️ 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".
| // Receives job IDs to cancel. Written by the notifier goroutine, only read | ||
| // from the main goroutine. | ||
| cancelCh chan int64 |
There was a problem hiding this comment.
When a cancellation notification for attempt N is delayed until after N finishes, JobRetry now clears the marker and allows attempt N+1 to start, but the notifier still queues only the job ID and maybeCancelJob resolves the current executor. If the notification arrives while N+1 is running, it cancels the retried attempt despite the new retry guarantee. Fresh evidence beyond the repaired polling race is that handleControlNotification still sends only decoded.JobID; the Rust notifier retains the same ID-only behavior. Include attempt identity in the notification or recheck the durable marker before cancelling.
AGENTS.md reference: AGENTS.md:L36-L40
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Going to copy some bot output here for posterity — basically, this is a preexisting problem on master today and the net value of a fix is extremely questionable.
🤖 I'd leave this one unfixed. The race is real, but it's on master today, this PR doesn't touch it, and both suggested fixes are the kind of change that made #1361 balloon.
Is it real? Yes, but it isn't new.
- Master's handleControlNotification already passes only the job ID, and maybeCancelJob cancels whatever attempt currently runs under that ID. The diff against master leaves that code alone. The only change near it is my removal of the poll's own send into that channel. The Rust notifier, attempt tracking and the Go notifier are also unchanged.
- To hit it, attempt N's cancel notification has to arrive late enough that, in between, N finishes, someone calls JobRetry, and the same client starts N+1. Notifications normally arrive within milliseconds, and a retry is usually a person or program reacting to the cancellation. This is very unlikely in practice.
- Codex frames this as breaking "the new retry guarantee", but that guarantee is narrower. Clearing the marker on retry stops a leftover marker from cancelling the retried job (the poll, Java's check at job start, JS's reconnect check). It never promised anything about a notification still in flight at retry time.
Would the cure be worse than the disease? Yes, for both suggestions.
- Attempt identity in the notification: this changes the notification format that all five implementations send and parse, with fallbacks for older clients sending the old format. That's essentially your 5fbb860 from Reconcile active jobs against cancel_attempted_at after notifier reconnects #1361.
- Recheck the marker before cancelling: every cancel notification becomes an asynchronous database read, in Go and Rust at least. That's essentially Reconcile active jobs against cancel_attempted_at after notifier reconnects #1361's 70d5145.
So the PR stays as it is. If this race ever shows up in practice, it can be its own change.
If you want to reply on the Codex thread, here's a draft:
▎ 🤖 This race is real but predates this PR. On master, handleControlNotification already passes only the job ID to maybeCancelJob, which cancels the current executor, and the Rust notifier is unchanged here as well. Clearing the marker on retry fixes a leftover marker cancelling a retried job. It doesn't cover a notification still in flight when a job is retried, which needs that notification to be delayed past the attempt finishing, a JobRetry, and the same client claiming the next attempt. Both suggested fixes, attempt identity in the notification format across all five implementations or a database read per cancel notification, are what made #1361 grow, so we'll keep this PR to the polling fallback and handle the notification race separately if it turns out to matter.
This one's presented as an alternative to #1361. Unfortunately, between the
changes needed after review feedback and all the additional languages we need
to support, the PR got really big and undesirably complex.
I had my bot go back to the drawing board and see if could find a simpler
alternative. It seems to have -- instead of attaching notifier recovery hooks,
we augment a poll loop in the producer (
pollForJobCancellations) added forYugabyte to also keep an eye out for the case cancellation before reconnect.
I left a more detailed description of the full design in the commit messages.