Refactor enqueue RFC - #325
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7ea29d9. Configure here.
| one r: TicketRequest | r.tr_job = j and r.tr_queue = qPaused and r.tr_task = tid and r.tr_time = tnext | ||
| requestTasksAt[qPaused, tnext] = requestTasksAt[qPaused, t] + tid | ||
| all q2: Queue | q2 != qPaused implies requestTasksAt[q2, tnext] = requestTasksAt[q2, t] | ||
| } |
There was a problem hiding this comment.
Completion transitions can't release multi-queue holders
High Severity
The new enqueueWithConcurrencyMixed predicate makes states reachable where a single task holds holders on two queues (qGranted and later qPaused via grantNextRequest). However, all completion-with-release transitions (completeSuccessReleaseTicket, completeFailurePermanentReleaseTicket, etc.) take a single q and call releaseHolder (singular), whose frame condition explicitly preserves other queues' holders. The general completeSuccess path using all q | releaseHolder is unsatisfiable for multi-queue tasks because each releaseHolder call's frame condition contradicts the others. The existing releaseHolders (plural) helper handles this correctly but isn't wired into any completion transition. This leaves one holder permanently orphaned after completion, causing holdersRequireActiveTask to produce counterexamples.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7ea29d9. Configure here.
| all q: Queue | holdersAt[q, tnext] = { tid: holdersAt[q, t] | | ||
| dbQueuedAt[tid, t] != j and | ||
| dbCheckRateLimitAt[tid, t] != j and | ||
| (no r: TicketRequest | r.tr_task = tid and r.tr_job = j and r.tr_time = t) } |
There was a problem hiding this comment.
Cancel can't delete CRL tasks due to fact constraint
Medium Severity
The updated cancelJob now releases holders for tasks tracked by dbCheckRateLimitAt, but the crlStateChangesOnlyViaCrlTransitions fact (line 2005) only allows CRL state changes during enqueueWithConcurrencyAndRateLimit or dequeueDropCheckRateLimit. Since cancelJob isn't listed, the fact prevents cancel from deleting the CRL task itself. This contradicts the RFC's stated goal of deleting the CRL task during cancel and leaves a dangling DbCheckRateLimitTask after cancel whose holders have been released.
Reviewed by Cursor Bugbot for commit 7ea29d9. Configure here.
kirinrastogi
left a comment
kirinrastogi
left a comment
There was a problem hiding this comment.
did an initial pass, conceptually this is correct, just adding details since the ai seemed to add implementation details in the sections
|
|
||
| 1. **Permanent holder leak at completion** — A's holder is keyed by `first_task_id` (Uuid generated at `enqueue.rs:371`, also written as the immediate-path `RunAttempt.id`). When B queues, the scanner later promotes it: the new holder is keyed by `request_id = "{job_id}:{attempt}:{suffix}"` from `ParsedConcurrencyRequestKey::request_id()` (`keys.rs:419-422`), and the terminal `Task::RunAttempt` the scanner writes has `id = request_id` and `held_queues = [B]` only (`concurrency.rs:1330, 1335`). When the job runs and `report_attempt_outcome` (`lease.rs:286-288`) releases by `(held_queues × task_id)`, it deletes `(B, request_id)` — A's holder under `first_task_id` is in neither the `held_queues` list nor the matching `task_id` slot and is never reached. Even after the chain completes, A's slot is permanently consumed. Verified by code inspection: no cross-holder release path keys on chain-wide context (job_id, attempt_number). | ||
| 2. **Limit bypass (silent under-enforcement)** — if the chain were `[A, B, C]` and B queues, the scanner has no view of C; it writes the terminal `RunAttempt` after granting B, silently skipping C. Not a leak — a correctness violation, since C's quota is never enforced for this chain. | ||
| 3. **`Task::RequestTicket` (future-scheduled)** — same shape as bugs 1 + 2. For `start_at_ms > now_ms`, `handle_enqueue`'s third branch (`concurrency.rs:826-849`) writes only a `RequestTicket` for the first concurrency queue; `process_ticket_request_task` (`concurrency.rs:863`) on grant creates a holder under the synthetic id + a single-queue terminal `RunAttempt`, dropping the rest of the chain (limit bypass) and orphaning any earlier holders that might exist (leak — though structurally `RequestTicket` is the *first* concurrency limit, so usually there are no earlier holders to orphan). |
There was a problem hiding this comment.
There are some other issues that we uncovered:
- there is no rollback of an in memory holder for a future scheduled task, this needs to call rollback_grant: https://github.com/gadget-inc/silo/blob/main/src/concurrency.rs#L827-L850
- there are multiple tasks enqueued for a single job (RequestTicket, RunAttempt etc) but they all share task_key, the broker tombstones accidentally elide these since they share the same task_key as the start_time_ms is populated all at the same time. we don't want to change the key or semantics because we need to enforce at most 1 task running per job, but we need to update the use of task_keys so that start_time_ms is correctly populated to prevent errant tombstones.
- we want to optimize this fix to not have to scan job_info on the dequeue path, for speed (you mentioned this lower in the doc, but putting it up front is good too)
|
|
||
| ### Identity unification | ||
|
|
||
| The chain's UUIDv4 `task_id` (minted at `enqueue.rs:371` initial, `lease.rs:222` retry, `import.rs:258/634` import) is the single identity throughout: |
There was a problem hiding this comment.
this is an important callout, the GrantOutcome::Queued path would generate a new request_id instead of using the original task_id.
| } | ||
| ``` | ||
|
|
||
| `LimitEntry` already exists for `JobInfo.limits`; reuse it. Canonicalization runs once at the construction site (initial enqueue, retry, import); decoded values are already canonical and the walker does not reorder. |
There was a problem hiding this comment.
this is good, we should keep the limits array the same as the received proto, and just have the same logic to prioritize the walking of it. for reference it is currently sorted so that the order is concurrenct -> floatingConcurrency -> rateLImit (unused by us)
we should continue to make concurrencyy and floatingConcurrency requests execute in the same order they were received in, for example [C1, FC1, FC2, C2, FC3] should be granted in order of [C1, C2, FC1, FC2, FC3] regardless if it is done immediately or scanned.
on second thought, see #325 (comment) maybe we want to frontload the sorting just so resuming is easier to do.
|
|
||
| **`Task::RequestTicket`** (`task.rs:28-40`, schema `internal_storage.fbs:81-91`) gains `task_id` (renamed from `request_id`), `next_limit_index`, and `limits`. It does *not* gain `held_queues` — `start_time_ms` gates the first concurrency limit, before any holders are taken, so a `RequestTicket` mid-flight always has `held_queues = []`. (If a future change ever lets `RequestTicket` exist mid-chain, add the field then.) | ||
|
|
||
| The rename `request_id` → `task_id` is a semantic change, not an append-only field addition. The new field's slot may be wire-compatible with the old, but consumers of the accessor change. Pre-launch DB nuke covers the data side; the code change is mechanical (audit map in Critical files). |
There was a problem hiding this comment.
this is good but of note we don't need wire compatibility at this stage, can break the contents of fbs for a little longer
|
|
||
| `LimitEntry` already exists for `JobInfo.limits`; reuse it. Canonicalization runs once at the construction site (initial enqueue, retry, import); decoded values are already canonical and the walker does not reorder. | ||
|
|
||
| **`Task::RequestTicket`** (`task.rs:28-40`, schema `internal_storage.fbs:81-91`) gains `task_id` (renamed from `request_id`), `next_limit_index`, and `limits`. It does *not* gain `held_queues` — `start_time_ms` gates the first concurrency limit, before any holders are taken, so a `RequestTicket` mid-flight always has `held_queues = []`. (If a future change ever lets `RequestTicket` exist mid-chain, add the field then.) |
There was a problem hiding this comment.
I think held_queues is necessary to avoid a job_info ge on job cancellation? Maybe the field is redundant, because imho it's ok to get job_info on cancellation, storing held_queues may be a pointless optimization.
|
|
||
| ### Canonical ordering | ||
|
|
||
| `LimitWalkState.limits` is stored *already in canonical order*. The walker indexes directly: `state.limits[state.next_limit_index]`. Canonicalization runs once at construction time (initial enqueue, retry, import, rate-limit continuation re-entry). The scanner and ticket-processor decode payloads that were already canonical when written, so no re-canonicalization on resume. This addresses **review issue #6** (no `order[next_limit_index]` indirection). |
There was a problem hiding this comment.
I think this is correct and probably flawed with my other approaches. Sorting and storing on enqueue is a good (or update all the clients to enqueue in the correct order, and let them deal with accidental deadlocks themselves).
2 participants
Note
Low Risk
Low risk since this PR only adds design documentation and updates the Alloy specification; it does not change runtime code behavior.
Overview
Adds a detailed RFC (
docs/RFCs/Unify silo's enqueue + concurrency-gra.md) proposing a unified limit-walker/intent model to carry full chain state throughEnqueueTask/RequestTicket/CheckRateLimitand eliminate mixed-path holder leaks and limit bypass.Updates
specs/job_shard.alsto model the new mixed grant + pause state (enqueueWithConcurrencyMixed), treatTicketRequest(durable enqueue intent) as an active marker for holders inholdersRequireActiveTask, and tighten cancel cleanup to release holders even when a chain is represented only byCheckRateLimitor aTicketRequest.Reviewed by Cursor Bugbot for commit 7ea29d9. Bugbot is set up for automated code reviews on this repo. Configure here.