Repository navigation
feat(continuous-ops): repository checkout service: one door for a commit's files, with a mirror cache (XWING-2440) - #132
Conversation
…es, with a mirror cache (XWING-2440)
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
…s mirror Claim stays core's atomic path when the next queued order needs no steering; when it waits for its repository's holder, the runner reaches past it through core's start takeover, so a mirror is fetched once per node. NEED(core: mirror affinity) - the hold lasts only while the holder's order is leased.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smith-xyz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
… core pin at core #311's head ba4f335 # Conflicts: # pyproject.toml # uv.lock
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
PR Summary by QodoKeep repository orders with the runner holding their lease
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
View medium (2)🟠 **Medium** 1. A planned order can bypass admission
|
![qodo-for-traust-security[bot]](https://avatars.githubusercontent.com/u/330043060?s=60&v=4){kind=small}
| open_orders = [ | ||
| order | ||
| for order in every_item(lambda p: ops.list(OrderFilter(states=OPEN_ORDER_STATES), p)) | ||
| if order.asset in assets | ||
| ] |
There was a problem hiding this comment.
🟠 Medium
1. Orders can bypass another pack's holder 🐞 Bug ≡ Correctness
_lease filters open orders to the current pack's runnable assets before building the set of held repositories. When one pack has a leased order for a repository and another pack has its next queued order, the second runner cannot see the lease and can claim work for that repository.
Agent Prompt
## Issue description
Repository holders on assets outside the current pack disappear from the affinity check.
## Fix Focus Areas
- src/traust_engine/ops/runner.py[151-169]
## Recommended Fix
Build the held-repository set from all leased open orders, then separately restrict queued candidates to runnable assets. Add a test with disjoint packs sharing a repository.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } | ||
| if queued[0].repository_id not in held: | ||
| return ops.claim(self._worker, self._lease_for, assets) |
There was a problem hiding this comment.
🟠 Medium
2. Concurrent runners can split a repository 🐞 Bug ≡ Correctness
_lease checks its listed snapshot for a holder, then calls ops.claim without restricting that claim to an unheld repository. If two runners read the queue before either claims, the first can claim one order and the second can claim another order for the same repository because the claim API filters by asset rather than repository.
Agent Prompt
## Issue description
The affinity decision and claim are separate operations, allowing simultaneous claimers to take different orders for one repository.
## Fix Focus Areas
- src/traust_engine/ops/runner.py[149-179]
## Recommended Fix
Use a core claim operation that atomically excludes repositories held by another worker, or a repository-level lease acquired with the order. Cover two simultaneous claimers with multiple queued orders for one repository.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| try: | ||
| return self._services.ops.start(request, ENGINE, self._worker, self._lease_for) |
There was a problem hiding this comment.
🟠 Medium
3. A planned order can bypass admission 🐞 Bug ≡ Correctness
_take passes a reconstructed OrderRequest to start rather than the ID and queued state of the order selected by _lease. If a planned order for that pair appears after the busy snapshot, start can take the pair's open order regardless of state, allowing the newly planned order through the fallback path.
Agent Prompt
## Issue description
Fallback selection checks state in a snapshot, but `start` selects by request rather than atomically leasing the selected queued order.
## Fix Focus Areas
- src/traust_engine/ops/runner.py[172-196]
## Recommended Fix
Use a core operation that atomically leases the selected order ID only while it remains queued, without selecting another order for the pair. Test a planned order introduced between listing candidates and taking one.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…st-engine into feat/repository-checkout-service-one-door-for-a-commi
chubirka
left a comment
chubirka
left a comment
There was a problem hiding this comment.
Code review: changes required
Reviewed commit ed5d46dcd453990e00337389a0904f6ecdd337dc. Found 1 actionable issue(s); details are attached as inline review comments.
Merge hold: keep this PR in draft with auto-merge disabled until the findings have been addressed and the affected behavior has been re-reviewed. Do not mark ready solely because unrelated CI passes.
| try: | ||
| return self._services.ops.start(request, ENGINE, self._worker, self._lease_for) |
There was a problem hiding this comment.
[P2] Atomically lease the selected order instead of starting its pair
When the first queued repository is held, _lease snapshots later queued orders and _take calls ops.start using only the asset/repository pair. If another runner claims and completes that selected order after the snapshot but before start, there is no open order left: core start creates a fresh run instead of refusing the stale claim. This runner then executes the same work again without a new controller admission/budget decision; it can also take over a newly planned order for the pair. Lease the selected order ID conditionally on its still-queued state rather than using the create-or-take-over start API.
Evidence: HarnessCheckout-runner-repro.py used real pinned OpsService/SQLite and deterministically interleaved a winning claim+complete between _lease listing and _take start. The originally queued order 1f3fa2d4-6df7-4631-8219-09c7e98a0af7 finished succeeded; the late runner then created and executed a different order 7769a3fb-dcb7-4498-a87b-42c7565a389f for the same repo/asset, requested_by machine:traust-engine. Core operations.py:280-283 creates a new order when find_open returns None.
Verification: Deterministic real-service stale-snapshot reproduction passed, demonstrating an extra command and a different run ID. tests/test_ops_runner.py: 30 passed; current concurrency tests cover a still-leased holder, not completion before _take.
There was a problem hiding this comment.
Agreed: the busy-pair snapshot prevents a pre-existing planned mate from being taken over, but it does not close the interleaving you demonstrated. After the selected order completes, start may create a new order without admission. I have not marked this fixed. The right fix is an atomic claim of the selected queued order ID in core (or removal of this fallback); I will hold #132 for that change and its deterministic race regression. The f37d779 update only fixes the separate cross-pack held-repository issue and advances the core pin.
There was a problem hiding this comment.
Progress on this reviewed defect: opened traust-core#329 (draft, head 141c49b8) with an atomic queued-ID claim across service/repository/REST, tested on SQLite and PostgreSQL. In #132’s worktree, local commit 2aa5dfa replaces start(pair) with claim(order_id=selected.id); a forced interleaving (other worker completes the selected order, then a planned mate appears) failed before and now passes, with 156 engine tests against locally installed core#329. I have NOT pushed the engine commit: its published core pin is still #327, which lacks this API. Once reviewed core#327/#329 converge, I will pin the combined commit, push, rerun CI and request your re-review. The separate concurrent repository-affinity race remains open.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Qodo finding on traust-security#132: the held set counted only orders of assets this runner makes, so a repository leased through another pack's order looked unheld and a second node could split it. Every open order counts now. Re-pinned core to 282ebbe (core#327: git transport auth, checkout origin).
|
Review disposition (head f37d779): Qodo’s cross-pack repository hold was real and fixed; the regression test fails before the fix and passes after it (155 tests pass). The planned-mate admission case is guarded and covered by |
|
Correction to my earlier Qodo disposition: I incorrectly dismissed “A planned order can bypass admission” as a false positive. The existing busy-pair test only covers a planned mate before the snapshot; Qodo explicitly describes one appearing after it. Michele independently reproduced the same non-atomic |
Plan:
plans/traust-standardization/0-continuous-ops/in-progress/23-shaun-repo-checkout-service.mdStarted by shaun. Draft until ready for review.
Pins core#312's head
09ad226(contains #311, #318); merge after the core PRs.Development moved under this PR (session-runner #131, which pins core at
acf67b8); this branch merges development and re-pins to core #312's head (09ad226), which contains #311's checkout line, #318 and #312's preflight; the suite is green on this pin.What
Admission affinity (the plan's engine item): a repository's orders stay with the runner that holds its mirror, so a mirror is fetched once per node. All in
ops/runner.py's claim path.Runner._leasekeeps core's atomicclaimwhenever the next queued order needs no steering: the uncontented path is unchanged (same statement, and the expired-lease sweepclaimdid internally still runs, viaops.requeue_expired()).start(the queued-takeover pathOpsServicealready defines). The held repository's orders wait for their holder; the runner takes other work instead. A racing lease surfaces asConflictError, and the runner tries the next candidate.startleases a pair's open order whatever its state, so a pair that still has a planned (unadmitted) or already-leased order is never taken over: the budget gate is never jumped.NEED(core: mirror affinity): the hold lasts only while the holder's order is leased. Nothing in the fixed data model records which runner holds a repository's mirror between orders, and the claim API filters by asset, not repository. So the affinity ends when the holder's order finishes — a repository's next order can then move to another node and fetch the mirror again — and reaching past a held order is list-then-takeover, not one atomic claim. Core options: a repository-to-holder record that outlives an order (a checkout lease), or a claim filter by repository.Queries (checklist 8): the claim path adds one paged read of open orders (
run_orderbystate, keyset-paged — the same shape the admission pass runs per tick on planned orders) beside core's own claim statement; no per-row loops, no new indexes. A claim-once runner runs it once per order.Reviewer checks (from the plan)
Quality checklist
Legacy files
Each file this plan replaces: reuse (core already does it), retire (the model made it obsolete) or port.
None: the engine's
ops/code has no legacy checkout path — the mirror itself is core's (core #311).Changed tests
tests/test_ops_shadow.py::test_outcome[refused answer]JobResult.refused("x")JobResult.refused(OrderReason.NOT_FOUND)ba4f335) madeJobResult.reasona closedOrderReasonenum, so the placeholder"x"no longer constructs; the row still checks a refused answer round-trips throughoutcometests/test_ops_runner.py::_runnerWORKERworker: WorkerId = WORKERparameterNew tests (
tests/test_ops_runner.py, table-driven, two runners, local SQLite undertmp_path, no remotes): the affinity table (nobody holds it / the holder takes its repository's next order / a runner without the mirror does not steal it), every order of a held repository waits, freeing the hold (the holder's order completed / its lease expired), orders wait while their holder runs mid-command, and a pair still waiting for admission is not taken over.Cross-repo coordination
Track:
development. Use these explicit related-repository targets rather than assuming the connected repository'smainis the matching overhaul baseline.Review targets
Integration and rollout
Adopt Core #311 before relying on the checkout service. Engine #133 carries a different Core adoption; reconcile the final Core pin after both upstream changes are available.
Branch URLs are compatibility baselines; PR URLs are proposed companion code. Neither proves that a manifest/lockfile pin already contains the changes or that the target is deployed. Honor the prerequisites above, reconcile compatible dependency pins before rollout, and do not dismiss a real compatibility gap merely because another PR is planned.