Add CoPlan::DebouncedJob, a reusable event-coalescing primitive - #208
Merged
Conversation
Downstream hosts wanting to coalesce a burst of comment_created events into one delayed notification (e.g. a Slack digest) keep hand-rolling the same debounce logic, and getting the same three bugs wrong: using call time instead of the triggering event's own timestamp as the batch boundary, a racy read-then-write claim that lets two callers both enqueue, and cache TTLs shorter than the job's own retry backoff that quietly drop part of a retried batch. CoPlan::DebouncedJob is a mixin any host ActiveJob can include to get this once, correctly: an atomic write-if-absent claim, a boundary that's required to be the caller-supplied event timestamp, and a state TTL sized off the window plus a configurable retry horizon rather than the window alone. The including job implements perform_batch(key:, batch_start:) instead of perform, and triggers a batch with MyJob.debounce(key:, event_at:). Not wired into Comment/comment_created or any host callback yet — this is the standalone primitive, tested against each of the three failure modes above plus a 25-thread concurrent-claim test.
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: ad6900e23d
ℹ️ 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".
Two P1/P2 issues from Codex review on #208: - An event landing after perform_batch has already queried but before release_debounce runs was silently dropped: the held claim makes debounce a no-op, and the completed query can't retroactively include it. Adds a narrower `running?` flag (distinct from the claim, which also spans the wait) so debounce can tell "still waiting, will be picked up naturally" apart from "already querying, might be missed", and marks the event dirty in the latter case. perform now rearms the same claim for a follow-up run instead of releasing when something came in dirty, keeping the earliest such event so multiple mid-run arrivals don't shadow each other. release_debounce deliberately leaves a lingering dirty marker alone (rather than clearing it) so a later claim can still fold it in as a fallback. - If perform_later raised (queue adapter error) or returned a job that wasn't actually enqueued (a halted enqueue callback), debounce still left the claim in place, so every event for that key would silently no-op until debounce_state_ttl expired with no job ever scheduled. schedule_run now releases the claim in both cases and re-raises on the exception path rather than swallowing it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CoPlan::DebouncedJob, a mixin any host ActiveJob can include to coalesce a burst of same-key events into a single delayed job, instead of hand-rolling it per host.event_at:is required and validated — the boundary can never silently become call time.Rails.cache.write(..., unless_exist: true); its return value is the only input to who wins.window + retry_horizon(both configurable, separately), with a loud warning + widest-window fallback if state is ever lost anyway.Comment/comment_createdor any host callback yet — this PR is just the standalone, tested primitive.Test plan
bundle exec rspec spec/jobs/coplan/debounced_job_spec.rb— 18 examples, 0 failures, including a 25-thread concurrent-claim test and mutation-style coverage of each of the three failure modesCI=1eager loading, confirming the newapp/jobs/concernsautoload path resolves)bundle exec rubocopon new files — no offenses