Repository navigation
fix(scheduling): settle expandable-node request correlation on non-dispatch paths - #189
Conversation
…spatch paths MC-3: submit mints a full request-correlation record set for every non-caller job, including needs-deferred dynamic-matrix nodes. Those nodes route to expansion and never dispatch to a runner, so no completion, result patch or disconnect ever settles their records. On every path where such a node concludes without dispatching, the record leaked in `inflight_requests` and `session_active_requests` with `result: None`, staying renewable and resolvable to a job expansion had already deleted from the run. Settle the record (result recorded, out of inflight + every session) exactly as the completion path would, on all non-dispatch conclusions: - run cancel (`cancel_run_inner`): use a run-wide `expandable_job_ids` set instead of the narrow caller_plans-only collection. - job cancel (`cancel_job_inner`): settle via `is_expandable_node`. - submit-time workflow-concurrency overflow cancel (`submit_run_inner`). - dependency skip / `if:` eval error (`promote_ready_jobs` Skip/Error arm). Reusable callers mint no records at submit, so retiring them is a no-op; Settle is idempotent, so overlapping paths cannot double-settle harmfully. Adds tests covering all four non-dispatch conclusions.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change retires deferred-matrix and reusable-call request correlations during cancellation, queue overflow, dependency skips, and condition failures. Cancellation now preserves recorded terminal statuses. Regression tests cover cleanup and completed caller success. ChangesExpandable Request Retirement
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change settles request-correlation records for expandable nodes that conclude without dispatching, preventing stale inflight records on cancellation and dependency-skip paths. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description clearly explains the problem, fix, affected paths, tests, and verification. However, it omits the required Protocol surface, Required gates, and Checklist sections, including an explicit YES/NO protocol confirmation and gate status. Resolution Add the missing template sections. Confirm whether the change touches the runner protocol interface, mark each required gate, and complete the checklist for tests, documentation, and changelog impact. Include concrete verification evidence for the listed commands and tests where applicable. Full details: Docstring CoverageExplanation Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1 too large.)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: ecc20cdf37
ℹ️ 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".
| // `register_expanded_jobs` keeps a deferred node's plan in | ||
| // `caller_plans` when the node lives inside a reusable callee; nested | ||
| // nodes are found here even after they left every queue. | ||
| ids.extend(run.caller_plans.keys().cloned()); |
There was a problem hiding this comment.
When a nested reusable caller has already completed while another job keeps the run active, it remains in run.caller_plans; register_expanded_jobs minted it a job-request record, while propagate_reusable_outputs only terminalizes the run's job status and leaves that request's result unset. Including every caller_plans key here causes a later run or ancestor-caller cancellation to pass this already-successful node to Settle(Cancelled), leaving the persisted request result as Cancelled while run.jobs still says Success. Filter this set to non-terminal nodes, or settle each record with its stored final job status.
Useful? React with 👍 / 👎.
… not hardcoded Cancelled Codex review (P2): a nested reusable caller that finished Success while the run stayed active remains in `run.caller_plans` with an unsettled request record (`register_expanded_jobs` mints it; `propagate_reusable_outputs` retires none). The run-wide `expandable_job_ids` sweep then passed it to `Settle(Cancelled)`, corrupting the persisted request result to Cancelled while `run.jobs` still said Success. Settle each expandable node with `node_settle_status` — its own verdict from `run.jobs`, defaulting to Cancelled for a node the run no longer records. Freshly-cancelled placeholders (already flipped to Cancelled by the status loop) still settle Cancelled; an already-terminal caller keeps its result. Also drops the now-redundant narrow `deferred_matrix` settle block in `cancel_job_inner` that the unified `is_expandable_node` path supersedes. Adds `cancel_preserves_completed_reusable_caller_result` covering the exact nested-caller scenario.
|
Addressed the Codex P2 (preserve completed caller results) in 2842849. Root cause confirmed: Fix: settle each expandable node with Added |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
A nested reusable caller can still conclude without dispatching while its request-correlation record remains inflight.
Reviewed changes This review covers the request-correlation settlement changes for expandable scheduling nodes and their regression tests.
- Cancellation settlement Run and job cancellation now identify expandable nodes across scheduler state and settle their records with the node's existing terminal result.
- Non-dispatch conclusions Submit-time workflow concurrency overflow and dependency
Skip/Errorconclusions now retire associated request claims. - Regression coverage New integration tests assert settlement for deferred-matrix placeholders and preserve completed reusable-caller results during cancellation.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using GPT Terra ｜ 𝕏
| // submit-time request correlation minted for it. Settle | ||
| // it like any other terminal conclusion; the node stays | ||
| // in the run, so Settle mirrors the completion path. | ||
| if job.deferred_matrix.is_some() || job.reusable_call.is_some() { |
There was a problem hiding this comment.
Nested reusable callers materialized by register_expanded_jobs have a request record, but a failure or overflow while acquiring their JobSet gates concludes them in the reusable Run arm without retiring it. This cleanup only covers dependency Skip/Error, so those callers remain inflight despite never dispatching.
Technical details
# Settle nested reusable callers rejected by JobSet admission
## Affected sites
- `crates/preloop-runner-server/src/runtime_scheduling.rs:1023-1059` — `caller_jobset_gates` errors and `advance_jobset_admission` errors mark a reusable caller terminal and immediately continue.
- `crates/preloop-runner-server/src/runtime_scheduling.rs:1147` — the added settlement only covers the separate dependency `Skip`/`Error` branch.
- `crates/preloop-runner-server/src/runtime_scheduling.rs:2392-2433` — `register_expanded_jobs` mints records even for a reusable caller materialized inside another reusable workflow.
## Required outcome
- Every nested reusable caller that is concluded by JobSet gate evaluation failure or admission overflow must have its request record settled and removed from inflight/session ownership with the recorded terminal status.
- Add regression coverage for a nested caller whose concurrency expression fails during admission.…spatch paths (#189) * fix(scheduling): settle expandable-node request correlation on non-dispatch paths MC-3: submit mints a full request-correlation record set for every non-caller job, including needs-deferred dynamic-matrix nodes. Those nodes route to expansion and never dispatch to a runner, so no completion, result patch or disconnect ever settles their records. On every path where such a node concludes without dispatching, the record leaked in `inflight_requests` and `session_active_requests` with `result: None`, staying renewable and resolvable to a job expansion had already deleted from the run. Settle the record (result recorded, out of inflight + every session) exactly as the completion path would, on all non-dispatch conclusions: - run cancel (`cancel_run_inner`): use a run-wide `expandable_job_ids` set instead of the narrow caller_plans-only collection. - job cancel (`cancel_job_inner`): settle via `is_expandable_node`. - submit-time workflow-concurrency overflow cancel (`submit_run_inner`). - dependency skip / `if:` eval error (`promote_ready_jobs` Skip/Error arm). Reusable callers mint no records at submit, so retiring them is a no-op; Settle is idempotent, so overlapping paths cannot double-settle harmfully. Adds tests covering all four non-dispatch conclusions. * fix(scheduling): settle expandable nodes with their concluded status, not hardcoded Cancelled Codex review (P2): a nested reusable caller that finished Success while the run stayed active remains in `run.caller_plans` with an unsettled request record (`register_expanded_jobs` mints it; `propagate_reusable_outputs` retires none). The run-wide `expandable_job_ids` sweep then passed it to `Settle(Cancelled)`, corrupting the persisted request result to Cancelled while `run.jobs` still said Success. Settle each expandable node with `node_settle_status` — its own verdict from `run.jobs`, defaulting to Cancelled for a node the run no longer records. Freshly-cancelled placeholders (already flipped to Cancelled by the status loop) still settle Cancelled; an already-terminal caller keeps its result. Also drops the now-redundant narrow `deferred_matrix` settle block in `cancel_job_inner` that the unified `is_expandable_node` path supersedes. Adds `cancel_preserves_completed_reusable_caller_result` covering the exact nested-caller scenario.
1 participant
Problem
MC-3: submit mints a full request-correlation record set for every non-caller job, including needs-deferred dynamic-matrix nodes. Those nodes route to expansion and never dispatch to a runner, so no completion, result patch, or disconnect ever settles their records. On every path where such a node concludes without dispatching, the record leaked in
inflight_requestsandsession_active_requestswithresult: None— staying renewable and resolvable to a job that expansion had already deleted from the run.Fix
Settle the record (result recorded, out of inflight + every session) exactly as the completion path would, on all non-dispatch conclusions:
cancel_run_inner): use a run-wideexpandable_job_idsset instead of the narrow caller_plans-only collection.cancel_job_inner): settle viais_expandable_node.submit_run_inner).if:eval error (promote_ready_jobsSkip/Error arm).Reusable callers mint no records at submit, so retiring them is a no-op;
Settleis idempotent, so overlapping paths cannot double-settle harmfully.id_token_grants/oidc_job_contextsare deliberately kept (mirrors completed/cancelled real jobs).Tests
Adds coverage for all four non-dispatch conclusions:
cancelled_deferred_matrix_node_settles_submit_requestscancelled_deferred_matrix_node_job_cancel_settles_requestsoverflowed_run_settles_deferred_matrix_node_requestsdependency_skipped_deferred_matrix_node_settles_requestsdependency_error_deferred_matrix_node_settles_requestsVerification
Rebased onto current
main.cargo build, all 5 MC-3 tests,cargo fmt --check, andcargo clippyclean onpreloop-runner-server.Summary by cubic
Fixes MC-3: request-correlation records for needs-deferred dynamic-matrix nodes kept leaking in
inflight_requestsandsession_active_requestswhen the node concluded without ever dispatching. These submit-time records are now settled exactly like a completed job — result recorded, removed from inflight and every session — on all non-dispatch conclusion paths.Paths settled
expandable_job_idsset from all queues, replacing the narrowcaller_plans-only collection.is_expandable_node.promote_ready_jobsSkip/Error arm settles expandable nodes withSkippedorFailure.Settlement uses each node's own concluded status from the run, so an already-terminal expandable node (like a reusable caller that finished Success while the run stayed active) keeps its verdict instead of being overwritten to Cancelled.
Settleis idempotent, and reusable callers mint no records at submit, so overlapping paths and caller retirement are no-ops;id_token_grantsandoidc_job_contextsare deliberately kept to mirror completed jobs. Adds six tests covering each non-dispatch conclusion plus completed-caller preservation.Written for commit 2842849. Summary will update on new commits.
Note
Settle expandable-node request correlations on run/job cancel and dependency skip paths
cancel_run_innerandcancel_job_innernow useexpandable_job_ids/is_expandable_nodeto detect all expandable nodes and callretire_node_requestswithnode_settle_status, preserving terminal verdicts likeSuccessfor already-completed reusable callers instead of forcingCancelled.promote_ready_jobssettles request records for expandable nodes on dependencySkip/Erroroutcomes;submit_run_innersettles expandable nodes when a run is immediately cancelled on acceptance.RequestRetirement::Settleretainsid_token_grantsandoidc_job_contextsfor settled nodes while clearing inflight and session-active claims.node_settle_statusin runtime_scheduling.rs) rather than leaving them inflight or hardcodingCancelled; verify consumers ofplan_requests,agent_job_requests, andtimeline_requestsstill resolve to settled placeholders.Macroscope summarized 2842849.
Summary by CodeRabbit
Bug Fixes
Tests