Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4578 +/- ##
==========================================
- Coverage 89.54% 89.53% -0.02%
==========================================
Files 441 443 +2
Lines 21249 21375 +126
==========================================
+ Hits 19028 19138 +110
- Misses 2221 2237 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
brandonjackson
left a comment
There was a problem hiding this comment.
Thanks. I approve, but would like to request that before you wrap up, you create a separate ticket describing the work that is needed to fully support the cancel operation for jobs that are both in the queue and running. To me this only gets us halfway.
58afbb7 to
a8e960a
Compare
So far this has been tracked here: #4574 I feel like that's what we need. (And we could prio this fairly high, if you're keen to have the "whole" solution sooner rather than later.) |
elias-ba
left a comment
There was a problem hiding this comment.
Hey @taylordowns2000, great work on this - the concurrency handling with FOR UPDATE SKIP LOCKED is really solid and the test coverage is thorough. The main concern is the authorization gap on the cancel-run handlers in both index.ex and show.ex - since Runs.get/1 is unscoped, a user could potentially cancel a run from another project. The rest of the comments are minor nits.
|
This is great, @elias-ba . I think your concerns have been addressed here:
|
Add missing {:error, _} clause to prevent CaseClauseError if
cancel_many returns an error on the single work order cancel path.
Security ReviewClaude hit the max-turns limit or encountered an error before posting findings. See the workflow run for details. |
The spec only declared {:ok, non_neg_integer()} but the implementation
can propagate {:error, any()} from cancel_available_for_work_orders.
There was a problem hiding this comment.
Hey @taylordowns2000 thanks for the changes and for this huge work. All is looking good to me 👏🏽 🎉
I pushed two small fixes:
- Added missing
{:error, _}clause in the single WO cancel handler (index.ex) - Updated
cancel_manytypespec to include{:error, any()}(was failing dialyzer)
One question (cc @stuartc): the "selected" cancel path runs synchronously in the LiveView process, while rerun always goes through Oban. I assume that's intentional since cancel is just a single UPDATE query bounded by page size - but wanted to confirm: was there a deliberate decision to keep it sync for small batches, or should we align it with the rerun pattern for consistency?
It was a specific decision to keep it sync for small batches. To my knowledge, Stu was not part of this decision. Happy for you to go wherever you want with it depending on your/Brandon's/Stu's cost:benefit analysis. |
|
Thanks for the quick response, Taylor. That makes sense to me - a bounded sync UPDATE is the right call here. Merging! 🚀 |
Description
Closes #1622 by allowing users to cancel available runs, essentially removing them from the queue (
:available -> :cancelled) and changing the state of their associate work orders from:pending -> :cancelled.Action availability by work order state:
:pending(runs:available):running:success,:failed,:crashed,:killed,:exception,:lost,:cancelled,:rejected)Validation steps
(Probably easiest to start your server with
RTM=false iex -S mix phx.serverso runs don't run.)Additional notes for the reviewer
FifoQueue.dequeueandRun.dequeuesince these functions deleted runs! They were in the code base, but (thankfully) never used. You can't delete runs.AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)