From 271802a30780e013a9f2bf49b2d7bc363d7e66d4 Mon Sep 17 00:00:00 2001 From: codex Date: Wed, 7 Oct 2026 12:55:21 +0900 Subject: [PATCH 01/28] CRW-906: record a hand-opened correction of an accepted, still-current result recordHandOpened refused every generation opened by hand for a node whose result the plan had already accepted and that is still current, so a blocking defect found in an accepted member before it landed could not be corrected or merged. The binder now admits that case when the reason the generation was opened under states a correction (needs_changes_revision, the generation-open default, or accepted_result_correction, which names this route), keeping every check it already had: the generation right after the one the acceptance stands on, the manifest prepared for the node's current slice and criteria, opened under CorrectionRequestID, bound to a dispatch turn, and the inputs as they stand now. The acceptance stays active until dag-accept --supersedes takes the new result. The route is noted in the generation's own reason column: no schema, refusal name, CLI flag or specs.json change. --- docs/relay/README.md | 2 +- docs/relay/dag-scheduler.md | 22 +- .../dagsched/baserefresh_scenario_test.go | 25 +- internal/relay/dagsched/correction.go | 4 +- .../dagsched/correction_accepted_test.go | 263 ++++++++++++++++++ internal/relay/dagsched/revalidation.go | 43 ++- internal/relay/dagsched/revalidation_test.go | 9 +- internal/relay/dagsched/withdraw.go | 2 +- internal/relay/registry/registry.go | 4 +- plugins/crw/.codex-plugin/plugin.json | 2 +- .../crw/skills/crw-run/references/relay.md | 9 +- 11 files changed, 363 insertions(+), 22 deletions(-) create mode 100644 internal/relay/dagsched/correction_accepted_test.go diff --git a/docs/relay/README.md b/docs/relay/README.md index 26fbd20fbc..b86518c5f9 100644 --- a/docs/relay/README.md +++ b/docs/relay/README.md @@ -885,7 +885,7 @@ The rule is fixed by the kind. `answer` and `stop` change nothing the child's at No refusal reason is added, and a refusal writes nothing. The command line is checked first: an unknown `--decision` or a missing required option ends with the parser's own exit 2 and a message on stderr before the writer runs, an unreadable `--note @path` is a host problem (exit 3, as for `verdict --criteria`), and `malformed_receipt` is what the writer answers to a direct call or to a blank note, turn or digest. `decision-show` reads the decisions of a relationship back, oldest first, each with the state of its delivery. -Limits that are part of the contract. The relay records the decision and its delivery; it does not read the child's thread, so `dispatched` says the message was accepted, not that the child acted. A decision that opened g+1 is recorded against a DAG plan node by `dag-correct` ([the third way to open a generation](dag-scheduler.md#three-ways-to-open-the-generation)), which refuses until the decision was dispatched into the turn the generation is bound to; until it is recorded, `dag-accept` finds no execution for g+1 and refuses `stale_generation`. A message sent to the child outside this route leaves a record only when the registered parent admits its turn ([Direct parent interventions](#direct-parent-interventions)). The crw-run skill's need-input procedure follows this route. After an answer or a stop, the relationship still reads as blocked (`dispositions-show`) until the child reports again. `registry`'s `generation-open --reason` still accepts only its own two reasons: `decision_reply` is written by this command alone. An installed relay older than this change has no `decision-reply` and answers the command as an unknown subcommand. +Limits that are part of the contract. The relay records the decision and its delivery; it does not read the child's thread, so `dispatched` says the message was accepted, not that the child acted. A decision that opened g+1 is recorded against a DAG plan node by `dag-correct` ([the third way to open a generation](dag-scheduler.md#three-ways-to-open-the-generation)), which refuses until the decision was dispatched into the turn the generation is bound to; until it is recorded, `dag-accept` finds no execution for g+1 and refuses `stale_generation`. A message sent to the child outside this route leaves a record only when the registered parent admits its turn ([Direct parent interventions](#direct-parent-interventions)). The crw-run skill's need-input procedure follows this route. After an answer or a stop, the relationship still reads as blocked (`dispositions-show`) until the child reports again. `registry`'s `generation-open --reason` still accepts only the reasons that command writes: `decision_reply` is written by this command alone. An installed relay older than this change has no `decision-reply` and answers the command as an unknown subcommand. ## The coordination summary diff --git a/docs/relay/dag-scheduler.md b/docs/relay/dag-scheduler.md index e54dd56db5..b04d9b54a5 100644 --- a/docs/relay/dag-scheduler.md +++ b/docs/relay/dag-scheduler.md @@ -154,7 +154,7 @@ The reading says why a node is stale and, in the `action` of the stale object, w | `action` | When | What is done | What is never done | | --- | --- | --- | --- | | `revalidate` | only the node's criteria changed (cause `criteria_changed`) and everything it consumed is still current: no acceptance it consumed was replaced and the manifest rebuilt as it consumed it is complete and digests to the same name | the same output is ruled again under the plan's criteria and `dag-accept` records a revalidation of the same acceptance (E-11, E-21): the criteria registered for the relationship change, the parent rules the output `verified` under the new set, and accepts it again | a new generation or a new child | -| `correct` | everything else on a node whose relationship is active: its spec, an edge into it or an input it consumed changed, or a change of criteria that also moved an input | the output is reworked by the same child, as the next generation of the same relationship: `dag-correct --prepare`, then either the `needs_changes` ruling that carries its instruction or a generation opened by hand ([below](#two-ways-to-open-the-generation)), `dag-correct`, then `dag-accept --supersedes` for the reworked result | a child made again | +| `correct` | everything else on a node whose relationship is active: its spec, an edge into it or an input it consumed changed, or a change of criteria that also moved an input | the output is reworked by the same child, as the next generation of the same relationship: `dag-correct --prepare`, then either the `needs_changes` ruling that carries its instruction or a generation opened by hand ([below](#three-ways-to-open-the-generation)), `dag-correct`, then `dag-accept --supersedes` for the reworked result | a child made again | | `hold` | the node rests on a stale predecessor (whatever its own cause: cause `predecessor_stale`, or a replaced input, or a change of criteria on a node that does), or an input is not there (an incoming edge that is not satisfied now: a predecessor nobody has accepted yet, a decision that was rejected), or the plan holds the node (paused, or the whole plan paused), or its relationship is paused, or a correction generation of it is already open | nothing on this node: the detail names what to wait for | | | `redefine` | the relationship has ended (archived, superseded or cancelled), so there is no child to send a correction to | the parent decides whether to redefine the node: a new relationship registered with `supersedes` (contract 5, ID-4), which makes a new child | | @@ -174,7 +174,7 @@ The correction goes to the same child as the next generation of the same relatio * **A ruling.** The relay's verdict writer takes another ruling on an event it already ruled `verified` in two cases ([Ruling an event that is already ruled](README.md#ruling-an-event-that-is-already-ruled)). The first is while a review is open, which the relay opens when the criteria registered for the relationship are no longer the set the head was ruled under (`delivery/ack.go`, re-review; the accepted event must still be the head of the current generation). The second is a `needs_changes` ruling before anything rests on the verified ruling: no acceptance of the event, no merged mark, no merge turn that is merging, of unknown effect or landed. Once the plan has accepted the event and no review is open, the writer refuses a second ruling with `disposition_conflict`, and the second way below is the route. The parent rules `needs_changes` with the instruction as the restoration finding, the writer opens the generation and sends the instruction, and `dag-correct` binds the manifest the finding names. A plan revision that changes the node's criteria digest, with the new set registered for the relationship, opens that review. * **A generation opened by hand.** When the head is ruled `verified` under the criteria registered now, which is the case of a node made stale because an input it consumed was replaced and nothing else, the writer refuses a second ruling on the accepted head (`disposition_conflict`, naming this route) and opens no generation. The relay has a route for that, written in the coordinator's skill (`crw-run`, "A fresh execution generation"): the coordinator opens the generation with `generation-open --reason needs_changes_revision` under the dispatch request id `dag-correct --prepare` printed, sends the instruction line to the child in the message that dispatches the re-review, and binds that turn with `generation-bind`. Then `dag-correct --manifest-digest D` binds the generation to the manifest. Only a node whose stale reading says `correct` takes this route: `dag-correct --prepare` succeeds for any node, and a generation opened for a node whose route is `revalidate` or `hold` would be refused when it is recorded. -What the second way binds is bounded, so it cannot make a rerun or bind a manifest other than the one the generation was opened for: the node's accepted result must be stale (a result that is not accepted yet is corrected by a ruling, and an accepted result that is current has no recorded correction route in this build (a generation that only merged the base is recorded by [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) instead): the writer refuses a ruling on it and this route refuses it as not stale; a node that landed was refused before); the manifest must be named with `--manifest-digest`, be the one stored for this node at its current slice and criteria, and still rest on the inputs the node's edges are satisfied by now (it is judged against the store again, the file bytes aside, as release does when it records its own intent: a predecessor accepted again after the prepare leaves another input than the one the child was told to consume, and nothing is bound); the generation must have been opened under the dispatch request id derived from that manifest (`CorrectionRequestID`: the plan, the node, the manifest and the generation), so it was opened for this manifest and no other; it must be bound to a dispatch turn, because a child reports in a bound generation only; and it must be the generation right after the one the accepted result stands on, so a correction that is already open is not skipped by opening another beside it (opening one moved the relationship past the generation that correction is accepted on: the refusal is reported and no further generation is opened, as when the plan moved the node). Each refusal is `disposition_conflict` with its reason in the detail and writes nothing. What is recorded of how the instruction reached the child is that request id (in `managed_request_id` of the execution row, which a ruling leaves empty) and the turn the generation is bound to; the answer says `opened_by` (`ruling`, `generation_open` or `decision_reply`), `dispatch_request_id` and `dispatch_turn_id`, and a repeated `dag-correct` answers the same as a replay. The generation is not recorded for a node whose route, without that generation, is anything but a correction: a change of the criteria alone is ruled again and opens no generation, and what rests on a stale predecessor or on an input that is not there waits. +What the second way binds is bounded, so it cannot make a rerun or bind a manifest other than the one the generation was opened for: the node's accepted result must be stale, or accepted and still current with a reason that states a correction ([Correcting a result that was accepted and is still current](#correcting-a-result-that-was-accepted-and-is-still-current); a result that is not accepted yet is corrected by a ruling, and a generation that only merged the base is recorded by [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) instead: the writer refuses a ruling on it and this route refuses it as not stale; a node that landed was refused before); the manifest must be named with `--manifest-digest`, be the one stored for this node at its current slice and criteria, and still rest on the inputs the node's edges are satisfied by now (it is judged against the store again, the file bytes aside, as release does when it records its own intent: a predecessor accepted again after the prepare leaves another input than the one the child was told to consume, and nothing is bound); the generation must have been opened under the dispatch request id derived from that manifest (`CorrectionRequestID`: the plan, the node, the manifest and the generation), so it was opened for this manifest and no other; it must be bound to a dispatch turn, because a child reports in a bound generation only; and it must be the generation right after the one the accepted result stands on, so a correction that is already open is not skipped by opening another beside it (opening one moved the relationship past the generation that correction is accepted on: the refusal is reported and no further generation is opened, as when the plan moved the node). Each refusal is `disposition_conflict` with its reason in the detail and writes nothing. What is recorded of how the instruction reached the child is that request id (in `managed_request_id` of the execution row, which a ruling leaves empty) and the turn the generation is bound to; the answer says `opened_by` (`ruling`, `generation_open` or `decision_reply`), `dispatch_request_id` and `dispatch_turn_id`, and a repeated `dag-correct` answers the same as a replay. The generation is not recorded for a node whose route, without that generation, is anything but a correction: a change of the criteria alone is ruled again and opens no generation, and what rests on a stale predecessor or on an input that is not there waits. What the second way does not prove: the relay does not read the child's thread, so the request id and the bound turn are the coordinator's statement of how the instruction was delivered, where a ruling's restoration note is compared with the instruction line. The child checks the manifest file against the digest and sha256 the line names and answers `blocked_needs_input` on a mismatch. The child's first receipt in the generation carries no `--supersedes-revision` (the generation has no earlier revision to supersede). A generation opened by hand is also not recoverable by dag-correct if the plan moved the node between the opening and the recording: the recording is refused (the manifest was prepared for another version of the node), preparing again names the next generation, and recording that one is refused at the gap (`generation 3 of R follows generation 2, and the last recorded execution of N is 1`). Open no further generation: report the refusal and stop, since the same holds for a ruling that opened its generation before the plan moved. @@ -192,6 +192,17 @@ The row is a `correction` execution whose `managed_request_id` is the decision r What a decision does not do to a generation: `answer` and `stop` keep it, so there is nothing to record and `dag-correct` replays the generation already recorded. A result in g+1 is accepted as any result is (`dag-accept`, with `--supersedes` when the node already had an accepted result), and the node reads accepted and not stale, because its manifest carries the plan's slice and criteria digests. The generation must be recorded before anything else opens another one: a second advancing decision, or a ruling's generation, before g+1 is recorded leaves a gap that `dag-correct` refuses for good (generation g+2 follows a generation that is not recorded), and the way out is a redefinition. A paused plan, node or relationship is refused as for every correction, and the same call is recorded after the resume. +#### Correcting a result that was accepted and is still current + +A blocking defect can be found in a result that the plan already accepted and that is still current, before it lands: a bundle review that finds one in an accepted member is the case. Nothing above the node changed, so the reading gives it no stale object and no route, the relay's verdict writer takes no second ruling on the head it ruled `verified` (`disposition_conflict`), and `dag-accept` refuses a generation that is not recorded as an execution of the node. The result is corrected by the same child as the next generation of the same relationship, opened by hand, with the same steps as the stale case and one more: the reason the generation is opened under states the correction. + +1. `dag-correct --prepare` rebuilds the node's manifest as the store holds it now and prints the instruction line, the manifest digest and the dispatch request id. +2. `generation-open --relationship --dispatch-request-id --reason accepted_result_correction` (or `--reason needs_changes_revision`, the default; both state a correction) opens the next generation. The instruction line is sent to the child in the message that dispatches the re-review, and that turn is bound with `generation-bind`. +3. `dag-correct --manifest-digest D` records the generation as a `correction` execution of the node (`opened_by` `generation_open`), under every check the stale case has: the generation right after the one the acceptance stands on, the manifest stored for the node at its current slice and criteria, the dispatch request id derived from that manifest, a bound dispatch turn, and the inputs as they stand now. The node must not have landed, and a node whose result is not accepted yet keeps the ruling route. +4. The child reports in the generation, the parent rules it and takes the result with `dag-accept --supersedes `. Until then the previous acceptance stays active, and the reading of the node whose current generation is not recorded as an execution reads `blocked:stale_head`, so nothing downstream consumes the corrected generation before it is accepted. + +What notes the route is the generation's own `reason` column: no column, table, schema, refusal name, CLI flag or `specs.json` entry is added. A generation opened under a reason that states no correction (the assignment itself, a returning tenure, a reply) is refused by the same step, naming the reason it carries, and a generation opened for a base refresh is not bound here either: [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) records that one. A generation opened for this route and never sent is withdrawn like any other ([A generation opened by hand and never sent](#a-generation-opened-by-hand-and-never-sent)). + ### Input checks When all incoming edges of a candidate are satisfied, the artifacts it would consume are read again. These file checks apply to non-code `artifact_verified` inputs (a code-pinned input consumes the accepted head instead), and a frozen copy of the manifest is checked when the receipt records its reference: every file the predecessor's receipt declared must still @@ -564,7 +575,7 @@ A parent rules a node's report `verified`, finds before `dag-accept` that the ba `crw relay dag-generation-withdraw --plan P --node N --actor A --relationship R --generation G --reason TEXT [--expect-epoch E]` (answer `dag-generation-withdraw/1`) is the counterpart of `generation-open` for the plan's node. It closes the generation and puts the relationship back on the generation before it; the commands that follow are the ordinary ones. The answer carries `relationship_id`, `withdrawn_generation`, `restored_generation`, `dispatch_request_id`, `reason` and `replayed`. -**What the relay requires, all read in one transaction.** The caller holds the plan's epoch; the relationship named with `--relationship` (`assignment-show` prints it) is an execution of the node and the caller is its parent; the plan does not hold the node (paused, cancelled, archived); the relationship is active and not superseded; `G` is 2 or more and is the generation the relationship stands on (`stale_generation` otherwise); the generation's row is an anchor still pending, with no dispatch turn and no bind time, and carries a reason `generation-open` gives (`initial_assignment` or `needs_changes_revision`: the generation of a returning tenure has none, and a withdrawal does not undo a tenure); no event of any outcome belongs to it (a report, a revision request, a decision reply); no turn was admitted to it; no `needs_changes` ruling opened it; and no execution of a node was recorded on it (`dag-correct` and `dag-adopt` record one). A generation that fails one of these was sent, used or recorded, and the refusal is `disposition_conflict` naming what was found, exactly as `dag-accept` refuses it today. A poll the daemon stored for an older turn under the generation's number is not read: it says nothing about the generation. +**What the relay requires, all read in one transaction.** The caller holds the plan's epoch; the relationship named with `--relationship` (`assignment-show` prints it) is an execution of the node and the caller is its parent; the plan does not hold the node (paused, cancelled, archived); the relationship is active and not superseded; `G` is 2 or more and is the generation the relationship stands on (`stale_generation` otherwise); the generation's row is an anchor still pending, with no dispatch turn and no bind time, and carries a reason `generation-open` gives (`initial_assignment`, `needs_changes_revision` or `accepted_result_correction`: the generation of a returning tenure has none, and a withdrawal does not undo a tenure); no event of any outcome belongs to it (a report, a revision request, a decision reply); no turn was admitted to it; no `needs_changes` ruling opened it; and no execution of a node was recorded on it (`dag-correct` and `dag-adopt` record one). A generation that fails one of these was sent, used or recorded, and the refusal is `disposition_conflict` naming what was found, exactly as `dag-accept` refuses it today. A poll the daemon stored for an older turn under the generation's number is not read: it says nothing about the generation. **What it writes.** One row of `dag_generation_withdrawals` (appended, never changed: relationship, generation, plan, node, the dispatch request id it was opened under, the reason it was opened for, the generation the relationship stands on again, the coordinator's reason, who withdrew it, the epoch and the time), the relationship's `execution_generation`, and a `generation_withdrawn` journal row. The generation's own row stays. **A number is never reused**: the next generation, whoever opens it (`generation-open`, a ruling, a decision reply, a tenure), takes one more than the highest the relationship ever held, so after a withdrawn generation 2 the next is 3. A generation that was withdrawn is final: `generation-open` repeated under its request id, `generation-bind` and the anchor binders of delivery are refused `stale_generation` (the daemon's binder skips it), so it cannot come back. The generation a correction follows is the nearest one that was not withdrawn, in the relay's currency (the predecessor a child's first report declares) and in `dag-correct`. Nothing else is written; the command does not touch a delivery: a generation-open leaves only a `stale_generation` note on older deliveries that were not yet delivered, the delivery pass reads the current generation, so an older delivery that was queued then can become eligible again under the delivery policy as it stands. The same relationship and generation again are a replay, also after the node was adopted by another relationship: the relationship is named so that a repeated call can never withdraw the generation of another relationship that has the same number. @@ -586,7 +597,7 @@ supersedes the active one and keeps the history, so a rejection after an approva A correction goes back to the same child (contract 3.2): a `needs_changes` ruling opens the next generation of the same relationship and the existing verdict writer sends the correction message in the same transaction, so a manifest cannot be placed in that generation afterwards. The protocol therefore puts the manifest into the one text the relay itself sends. `dag-correct --prepare --manifest-request @file` rebuilds and verifies the node's input manifest as the store holds it now (the base it records is read from the named target branch as a release reads it: the request names the repository and the ref, and a `sha` it states is checked against the branch tip and refused as `merge_base_mismatch` when the branch reads another commit, with nothing stored; the recorded commit is always the one the branch reads), stores it, and prints an English -instruction line that names the manifest, a copy of its canonical bytes kept under the child's first artifact root (the file the child reads, since it cannot read the relay's store; see *The frozen copy*) and the sha256 of that file; the parent submits that line as the **restoration** finding of its `needs_changes` ruling. (When no ruling can open the generation, the coordinator opens it by hand under the dispatch request id the same step prints and carries the line in its own dispatching message: [Two ways to open the generation](#two-ways-to-open-the-generation).) After the ruling, `dag-correct` (without `--prepare`) binds the generation the ruling opened to the manifest that finding names +instruction line that names the manifest, a copy of its canonical bytes kept under the child's first artifact root (the file the child reads, since it cannot read the relay's store; see *The frozen copy*) and the sha256 of that file; the parent submits that line as the **restoration** finding of its `needs_changes` ruling. (When no ruling can open the generation, the coordinator opens it by hand under the dispatch request id the same step prints and carries the line in its own dispatching message: [Three ways to open the generation](#three-ways-to-open-the-generation). A result the plan already accepted and that is still current has no ruling to open it, and its generation is opened by hand under a reason that states the correction: [Correcting a result that was accepted and is still current](#correcting-a-result-that-was-accepted-and-is-still-current).) After the ruling, `dag-correct` (without `--prepare`) binds the generation the ruling opened to the manifest that finding names (`dag_node_executions`, kind `correction`) once the manifest is stored for this node at its current slice and criteria and the restoration note carries, word for word, the instruction that manifest was prepared with (this generation, the path of the copy and its hash). No file is read when the generation is recorded: the child checks the copy against the hash in the line it was sent. The bound digest is derived from what the child was told: a digest in an ordinary finding does not bind, a restoration block that names two different manifests is refused, a ruling without the block leaves the previous manifest in force (`carried_over`), and a `--manifest-digest` that differs is refused, also when the generation is already bound. A node whose accepted head landed in every target is refused in both steps ([Handling a stale node](#handling-a-stale-node)). The previous acceptance stays active and reads `blocked:stale_head` until the parent accepts the corrected result with `--supersedes`; what consumed the old acceptance then reads stale ([Invalidation](#invalidation)). @@ -914,7 +925,8 @@ Where the code reads the contract differently, or adds to it, and why: * An unknown edit region overlaps everything; a node that edits no repository (`non_pr`) never conflicts. * The reading adds the disposition and state `stale` with the reasons `stale:slice_changed`, `stale:criteria_changed` and `stale:edge:` (contract 3.1 `stale:`), and an artifact edge, or an integrated edge a landing satisfies, from a stale result reads `blocked:stale_predecessor`; the merge judgement refuses a stale result with the existing `disposition_conflict`. No refusal reason, outcome or zone statement is added. Contract 4.2 lists the landed commit among the consumed values: the integrated predicate takes the earliest positive observation of the containment run, so a manifest names a stable landing, and a landed commit that an observation of the same run names is read as unchanged ([Invalidation](#invalidation)). * The stale reading carries a route, `action` and `action_detail` (`revalidate`, `correct`, `hold`, `redefine`), which the contract leaves to the parent. Decision D-08 is read from contract 5: a correction is a new generation of the same relationship and a node redefinition a new relationship with `supersedes`; the scheduler never registers the second. No refusal reason, reading reason, outcome or zone statement is added: a revalidation of a node that is not stale for its criteria alone, and a correction of a node that landed, are refused with `disposition_conflict`. -* Contract 3.2 and E-19 send a correction to the same child through the existing path. That path opens a generation with a `needs_changes` ruling, and the relay's verdict writer takes a second ruling on a head it ruled `verified` only while a review is open or, for `needs_changes`, before the plan accepted it, and refuses it with `disposition_conflict` otherwise. For a stale accepted node whose criteria did not change the generation is opened by hand with the relay's `generation-open` and `generation-bind` (documented for the coordinator in `crw-run`), and `dag-correct` binds it to the prepared manifest under the conditions of [Two ways to open the generation](#two-ways-to-open-the-generation). No refusal reason, zone statement or table is added: the dispatch request id is kept in the existing nullable `managed_request_id` column of `dag_node_executions`. +* Contract 3.2 and E-19 send a correction to the same child through the existing path. That path opens a generation with a `needs_changes` ruling, and the relay's verdict writer takes a second ruling on a head it ruled `verified` only while a review is open or, for `needs_changes`, before the plan accepted it, and refuses it with `disposition_conflict` otherwise. For a stale accepted node whose criteria did not change the generation is opened by hand with the relay's `generation-open` and `generation-bind` (documented for the coordinator in `crw-run`), and `dag-correct` binds it to the prepared manifest under the conditions of [Three ways to open the generation](#three-ways-to-open-the-generation). No refusal reason, zone statement or table is added: the dispatch request id is kept in the existing nullable `managed_request_id` column of `dag_node_executions`. +* A result the plan accepted and that is still current, with a blocking defect found in it before it lands, is corrected the same way and not by a new route: the generation is opened by hand under a reason that states the correction (`accepted_result_correction`, or the `needs_changes_revision` default), and `dag-correct --manifest-digest` records it under the checks the stale case has ([Correcting a result that was accepted and is still current](#correcting-a-result-that-was-accepted-and-is-still-current)). The reason the generation carries is what notes the route, so no column, table, schema, refusal reason, reading reason, CLI flag or `specs.json` entry is added; the acceptance stays active until `dag-accept --supersedes` takes the new result. The contract's 3.2 and E-19 speak of a stale result; this case is the contract's silence on a defect found after the acceptance, and the parent's answer to it is recorded here. * Contract 3.2 and E-10 read the pull request head against `dag_acceptances.head_sha`. A node whose acceptance stands on a recorded base refresh ([A base refresh of an accepted node](#a-base-refresh-of-an-accepted-node)) is read against the head that record names, because the lane's own check wants the head the newest generation's report names, which is that head; an unrecorded head, or a head no record can be made for, is E-10 as written. `dag_merge_checks.head_sha` holds the head the acceptance stood on when the check was made, which is the accepted head unless a record moved it. No refusal reason, outcome or zone statement is added. * The conflict sweep (CRW-410) and the merge-order constraint are not in the contract. Contract 7.2 says an observed conflict is the measured exception to a first in, first out merge order (D-16); the sweep, the ledger, the drift and the constraint are how this build measures that exception and says it. The constraint is advisory: the merge lane's own order and its checks are unchanged, and nothing refuses a merge for it. diff --git a/internal/relay/dagsched/baserefresh_scenario_test.go b/internal/relay/dagsched/baserefresh_scenario_test.go index fc9c085ee3..1db9dc6133 100644 --- a/internal/relay/dagsched/baserefresh_scenario_test.go +++ b/internal/relay/dagsched/baserefresh_scenario_test.go @@ -156,8 +156,8 @@ func (s *refreshScenario) slotHeld() bool { } // The state CRW-430 is about, on the code without a record of the refresh: the head the parent accepted is contained in dev, the merged mark sits on generation 2, so the node reads is_ancestor true, -// integrated false and mark_present false, holds its slot, and the hand-opened generation is refused by dag-correct because the result is current. Nothing here is a defect to fix by itself: the -// relay does not integrate a node on a generation's mark until a base refresh is recorded. +// integrated false and mark_present false, holds its slot, and dag-correct binds nothing from the hand-opened generation. Nothing here is a defect to fix by itself: the relay does not integrate a node +// on a generation's mark until a base refresh is recorded. func TestWithoutARecordTheRefreshGenerationLeavesTheNodeUnintegrated(t *testing.T) { t.Parallel() s := newRefreshScenario(t) @@ -174,8 +174,25 @@ func TestWithoutARecordTheRefreshGenerationLeavesTheNodeUnintegrated(t *testing. if n := s.read("g").node("I"); n.State == StateIntegrated || n.Disposition == DispDone && n.Reason == DoneIntegrated { t.Fatalf("I = %+v, want it not integrated", n) } + // CRW-906 admits a generation opened by hand for a correction of a current accepted result, so this one is + // refused by what it is not: it was opened for a base refresh (its request id is not the one dag-correct + // --prepare prints) and no manifest is named for it. dag-base-refresh is what records it. _, err = s.sched.RecordCorrection(context.Background(), "g", "I", "parent", "") - if refusalReason(err) != "disposition_conflict" || !strings.Contains(err.Error(), "is not stale") { - t.Fatalf("dag-correct = %v, want disposition_conflict naming that the result is not stale", err) + if refusalReason(err) != "disposition_conflict" || !strings.Contains(err.Error(), "--manifest-digest") { + t.Fatalf("dag-correct = %v, want disposition_conflict asking for the manifest", err) + } + roots, err := relationshipRoots(context.Background(), s.s.Q(context.Background()), s.rid) + if err != nil || len(roots) == 0 { + t.Fatalf("the artifact roots of the child: %v %v", roots, err) + } + prepared, err := s.sched.PrepareCorrection(context.Background(), "g", "I", "parent", ManifestInput{Base: &BaseRef{Repository: s.target(), Ref: "dev"}, RuleVersion: s.request(false).RuleVersion}, VerifyOptions{ArtifactRoots: roots}) + if err != nil { + t.Fatalf("prepare I: %v", err) + } + if _, err := s.sched.RecordCorrection(context.Background(), "g", "I", "parent", prepared.ManifestDigest); refusalReason(err) != "disposition_conflict" || !strings.Contains(err.Error(), "not opened for this manifest") { + t.Fatalf("dag-correct with a prepared manifest = %v, want the generation not opened for it", err) + } + if n := s.count("SELECT COUNT(*) FROM dag_node_executions WHERE relationship_id = ? AND execution_generation = 2", s.rid); n != 0 { + t.Fatalf("dag-correct bound %d executions of the refresh generation", n) } } diff --git a/internal/relay/dagsched/correction.go b/internal/relay/dagsched/correction.go index 6b6c8944a9..4837a08305 100644 --- a/internal/relay/dagsched/correction.go +++ b/internal/relay/dagsched/correction.go @@ -258,7 +258,9 @@ func (s *Scheduler) RecordCorrection(ctx context.Context, plan, node, actor, sup if reason.String == delivery.DecisionReply { return s.recordDecisionOpened(txCtx, tx, plan, snap, n, rel, actor, suppliedDigest, &out) } - if reason.String != "needs_changes_revision" { + // the generation's own reason says how it was opened: a decision reply has its own binder below, and a correction was opened either by a needs_changes ruling or by hand. A correction of an accepted + // result that is still current has no ruling to open it (the writer refuses a second ruling on the accepted head), so it carries the reason the coordinator gave it (revalidation.go). + if !correctionOpenReason(reason.String) { return refuse(contract.RefusalDispositionConflict, "generation %d of %s was not opened by a needs_changes ruling or a decision reply (%q)", rel.Generation, rel.ID, reason.String) } // the ruling on the previous generation's head must have opened this generation diff --git a/internal/relay/dagsched/correction_accepted_test.go b/internal/relay/dagsched/correction_accepted_test.go new file mode 100644 index 0000000000..dbe3e2c0e8 --- /dev/null +++ b/internal/relay/dagsched/correction_accepted_test.go @@ -0,0 +1,263 @@ +package dagsched + +import ( + "context" + "database/sql" + "strings" + "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/relay/registry" +) + +// CRW-906: the correction route of a result that was accepted and is still current. A bundle review found +// a blocking defect in an accepted member before it landed; the parent opened the next generation by hand +// and the child fixed it, but nothing could record that generation (the accepted result is not stale, so +// the hand-opened route refused it) and dag-accept refused the unrecorded generation, so the corrected +// head could not be accepted or merged. recordHandOpened now admits that generation when the reason it was +// opened under says a correction (needs_changes_revision, the generation-open default, or +// accepted_result_correction, which names the route) and every check it already had still holds: the +// generation right after the one the acceptance stands on, the manifest prepared for the node's current +// slice and criteria, opened under CorrectionRequestID, bound to a dispatch turn, and the inputs as they +// stand now. The acceptance stays active until dag-accept --supersedes takes the new result, the route is +// noted in the generation's own reason column, and no schema, refusal name, CLI flag or specs.json entry +// is added. + +// acOpenByHand is the coordinator's act with the relay's own commands, under a named reason: +// generation-open under the request id dag-correct --prepare printed, then generation-bind to the turn that +// carried the instruction line to the child. +func acOpenByHand(t *testing.T, k *releaseKit, relationship, request, reason string, generation int64, bind bool) { + t.Helper() + reg := ®istry.Registry{Store: k.s} + if _, err := reg.OpenGeneration(context.Background(), relationship, request, reason, sql.NullString{}); err != nil { + t.Fatalf("generation-open --reason %s: %v", reason, err) + } + if bind { + if _, err := reg.BindAnchor(context.Background(), relationship, generation, "turn-dispatch-"+relationship, "dispatch_receipt"); err != nil { + t.Fatalf("generation-bind: %v", err) + } + } +} + +// acExecution is the execution row of a node at one generation, as text, or "" when there is none: the +// kind, the manifest it is bound to and the request id it was opened under. +func acExecution(k *releaseKit, node string, generation int64) string { + k.t.Helper() + return rvRows(k, "SELECT kind, manifest_digest, COALESCE(managed_request_id, '') FROM dag_node_executions WHERE node_id = ? AND execution_generation = ?", node, generation) +} + +// acReason is the reason the generation row carries: what the record notes of the route without a schema +// change. +func acReason(k *releaseKit, relationship string, generation int64) string { + k.t.Helper() + var reason sql.NullString + if err := k.s.DB.QueryRow("SELECT reason FROM generations WHERE relationship_id = ? AND execution_generation = ?", relationship, generation).Scan(&reason); err != nil { + k.t.Fatal(err) + } + return reason.String +} + +// acPointer is the generation the relationship stands on. +func acPointer(k *releaseKit, relationship string) int64 { + k.t.Helper() + var generation int64 + if err := k.s.DB.QueryRow("SELECT execution_generation FROM relationships WHERE relationship_id = ?", relationship).Scan(&generation); err != nil { + k.t.Fatal(err) + } + return generation +} + +// Criterion c1 and the first case of c2: an accepted, current and not landed node whose generation was +// opened by hand for a correction is recorded as a correction (refused before this change), and the result +// the child reports in that generation replaces the acceptance through dag-accept --supersedes. The child +// is the same one, the acceptance stays active until then, and the siblings keep their records. +func TestAnAcceptedCurrentResultIsCorrectedByAHandOpenedGeneration(t *testing.T) { + t.Parallel() + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + siblings := map[string]string{"R": rvRecords(k, "sr", "R"), "A": rvRecords(k, "sr", "A"), "C": rvRecords(k, "sr", "C")} + fleet := k.rvFleet() + + // the premise: B was accepted and nothing above it changed, so its result is current and the relay's + // verdict writer has no review to open a second ruling on its head with + if n := k.read("sr").node("B"); n.Disposition != DispDone || n.Reason != DoneAccepted { + t.Fatalf("B = %+v, want it accepted and current", n) + } + prepared := k.rvPrepare("sr", "B") + if prepared.DispatchRequestID != CorrectionRequestID("sr", "B", prepared.ManifestDigest, 2) { + t.Fatalf("prepare = %+v", prepared) + } + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + + res, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if err != nil || res.Replayed || res.Generation != 2 || res.RelationshipID != rid || res.OpenedBy != OpenedByGenerationOpen || + res.ManifestDigest != prepared.ManifestDigest || res.DispatchRequestID != prepared.DispatchRequestID || res.DispatchTurnID != "turn-dispatch-"+rid { + t.Fatalf("dag-correct for the accepted current result = %v %+v", err, res) + } + if got := acExecution(k, "B", 2); got != "correction|"+prepared.ManifestDigest+"|"+prepared.DispatchRequestID { + t.Fatalf("the execution row = %q", got) + } + if got := acReason(k, rid, 2); got != "needs_changes_revision" { + t.Fatalf("the generation's reason = %q, want the one it was opened under", got) + } + // the acceptance stays active until the corrected result replaces it + var active string + if err := k.s.DB.QueryRow("SELECT acceptance_id FROM dag_acceptances WHERE plan_id = 'sr' AND node_id = 'B' AND state = 'active'").Scan(&active); err != nil || active != accepted["B"].AcceptanceID { + t.Fatalf("the active acceptance = %q (%v), want %q", active, err, accepted["B"].AcceptanceID) + } + // recording again is a replay that says how the generation was opened + if again, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); err != nil || !again.Replayed || again.OpenedBy != OpenedByGenerationOpen { + t.Fatalf("replay = %v %+v", err, again) + } + + // the child reports in the bound generation and the parent accepts the corrected result in place of the first one + k.rvReportGeneration(rid, "B", 2, releaseCriteriaDigest()) + second, err := k.accept("sr", "B", AcceptInput{Supersedes: accepted["B"].AcceptanceID}) + if err != nil || second.SupersededID != accepted["B"].AcceptanceID || second.Generation != 2 || second.AcceptanceID == accepted["B"].AcceptanceID { + t.Fatalf("dag-accept --supersedes of the corrected result = %v %+v", err, second) + } + if n := k.read("sr").node("B"); n.Disposition != DispDone || n.Reason != DoneAccepted { + t.Fatalf("B after the correction was accepted = %+v", n) + } + // the correction went to the child B already had and touched no other node + if got := k.count("SELECT COUNT(DISTINCT relationship_id) FROM dag_node_executions WHERE plan_id = 'sr' AND node_id = 'B'"); got != 1 { + t.Fatalf("the correction made %d relationships for B", got) + } + if got := k.rvFleet(); !strings.HasPrefix(got, strings.Split(fleet, " generations=")[0]) { + t.Fatalf("the correction made a child:\nbefore %s\nafter %s", fleet, got) + } + for node, before := range siblings { + if now := rvRecords(k, "sr", node); now != before { + t.Fatalf("the records of the unrelated node %s changed:\nbefore %s\nafter %s", node, before, now) + } + } +} + +// Criterion c1: the second reason the route admits names it. A generation opened with +// accepted_result_correction is a generation-open reason like any other, dag-correct records it as a +// correction, and the generation's own reason column is what notes the route (no schema change). +func TestTheReasonThatNamesTheRouteIsOpenableAndRecorded(t *testing.T) { + t.Parallel() + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "accepted_result_correction", 2, true) + res, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if err != nil || res.Replayed || res.OpenedBy != OpenedByGenerationOpen || res.ManifestDigest != prepared.ManifestDigest { + t.Fatalf("dag-correct under accepted_result_correction = %v %+v", err, res) + } + if got := acReason(k, rid, 2); got != "accepted_result_correction" { + t.Fatalf("the generation's reason = %q, want accepted_result_correction", got) + } + // the reason is one literal in two packages: dagsched declares the constant and the registry's closed + // list is what generation-open validates against, and the registry cannot import dagsched. This pins the + // two to each other, so a rename in one place cannot leave the other accepting a value nothing records. + for _, accepted := range []string{AcceptedResultCorrection, "needs_changes_revision", "initial_assignment"} { + if _, err := (®istry.Registry{Store: k.s}).OpenGeneration(context.Background(), "rel-none", "request-"+accepted, accepted, sql.NullString{}); refusalReason(err) == "unknown_generation" { + t.Fatalf("the registry refuses the reason %q that generation-open must accept", accepted) + } + } + + // a generation opened for this route and never sent is withdrawn like every other generation opened by + // hand: the withdrawal's own rule is about generation-open reasons, and this is one + k2, accepted2 := rvSettledSharedRoot(t) + rid2 := accepted2["C"].RelationshipID + acOpenByHand(t, k2, rid2, "correction-"+rid2, "accepted_result_correction", 2, false) + out, err := k2.sched.WithdrawGeneration(context.Background(), "sr", "C", "parent", WithdrawInput{Relationship: rid2, Generation: 2, Reason: "the correction is not needed"}) + if err != nil || out.Replayed || out.RestoredGeneration != 1 || out.Generation != 2 { + t.Fatalf("dag-generation-withdraw = %v %+v", err, out) + } + if got := acPointer(k2, rid2); got != 1 { + t.Fatalf("the relationship stands on generation %d after the withdrawal, want 1", got) + } +} + +// The second case of c2: every check the hand-opened route already had still refuses, on the same accepted +// current node, and a refusal writes no execution row. +func TestAHandOpenedGenerationOfAnAcceptedCurrentResultIsStillRefused(t *testing.T) { + t.Parallel() + bound := func(k *releaseKit, generation int64) string { return acExecution(k, "B", generation) } + t.Run("an open reason that states no correction", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "initial_assignment", 2, true) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); refusalReason(err) != "disposition_conflict" { + t.Fatalf("correction = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), `"initial_assignment"`) { + // the assignment itself is not a correction of anything: the recording refuses the generation by the reason it was opened under + t.Fatalf("the refusal does not name the reason the generation was opened under: %v", err) + } else if got := bound(k, 2); got != "" { + t.Fatalf("the refused correction bound %q", got) + } + }) + t.Run("a node that landed", func(t *testing.T) { + k, a := rvMerged(t) + acOpenByHand(t, k.releaseKit, a.Acceptance.RelationshipID, "correction-"+a.Acceptance.RelationshipID, "needs_changes_revision", 2, true) + if _, err := k.sched.RecordCorrection(context.Background(), "g", "I", "parent", dig("a manifest of the landed node")); refusalReason(err) != "disposition_conflict" { + t.Fatalf("correction = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), "landed") { + t.Fatalf("the refusal does not say why: %v", err) + } + }) + t.Run("a generation that is not the one after the acceptance's", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + first := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, first.DispatchRequestID, "needs_changes_revision", 2, true) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", first.ManifestDigest); err != nil { + t.Fatalf("the first correction: %v", err) + } + next := k.rvPrepare("sr", "B") + if !strings.Contains(next.Instruction, "generation 3") { + t.Fatalf("the second prepare = %+v", next) + } + acOpenByHand(t, k, rid, next.DispatchRequestID, "needs_changes_revision", 3, true) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", next.ManifestDigest); refusalReason(err) != "disposition_conflict" { + t.Fatalf("a second correction beside the open one = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), "already open") { + t.Fatalf("the refusal does not say why: %v", err) + } else if got := bound(k, 3); got != "" { + t.Fatalf("the refused correction bound %q", got) + } + }) + t.Run("a manifest prepared for another version of the node", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.invRevise("sr", "B", "sr-r2", invTitle("B, as the revision specifies it")) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); refusalReason(err) != "disposition_conflict" { + t.Fatalf("correction = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), "open no further generation") { + t.Fatalf("the refusal does not say what to do: %v", err) + } else if got := bound(k, 2); got != "" { + t.Fatalf("the refused correction bound %q", got) + } + }) + t.Run("a generation nobody bound", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, false) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); refusalReason(err) != "disposition_conflict" { + t.Fatalf("correction = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), "generation-bind") { + t.Fatalf("the refusal does not name the way on: %v", err) + } else if got := bound(k, 2); got != "" { + t.Fatalf("the refused correction bound %q", got) + } + }) + t.Run("no manifest named", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", ""); refusalReason(err) != "disposition_conflict" { + t.Fatalf("correction = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), "--manifest-digest") { + t.Fatalf("the refusal does not ask for the manifest: %v", err) + } else if got := bound(k, 2); got != "" { + t.Fatalf("the refused correction bound %q", got) + } + }) +} diff --git a/internal/relay/dagsched/revalidation.go b/internal/relay/dagsched/revalidation.go index 10c7311767..9704a7f379 100644 --- a/internal/relay/dagsched/revalidation.go +++ b/internal/relay/dagsched/revalidation.go @@ -51,6 +51,26 @@ const ( OpenedByDecisionReply = "decision_reply" ) +// AcceptedResultCorrection is the reason a coordinator gives generation-open when it corrects a result that was accepted and is still current (recordHandOpened): the relay's verdict writer has no review to +// open a second ruling on the accepted head with, so the generation is opened by hand and its own reason says why. needs_changes_revision, the reason generation-open writes by default, is admitted the same way; +// a generation opened for anything else (the assignment itself, a returning tenure, a reply) is not a correction of an accepted result. +const AcceptedResultCorrection = "accepted_result_correction" + +// correctionOpenReason is whether the reason a generation was opened with says a correction: the generation-open default, or the reason the coordinator gives when the accepted result is current. +func correctionOpenReason(reason string) bool { + return reason == "needs_changes_revision" || reason == AcceptedResultCorrection +} + +// correctionOpenedReason is the reason the relationship's current generation was opened under: generations.reason, which generation-open writes and a returning tenure leaves empty. Named for this route +// rather than for the column, because a sibling node of the same plan edits this package too and two top-level names that only differ by which file declares them do not conflict in git. +func (s *Scheduler) correctionOpenedReason(ctx context.Context, q store.Querier, rel relRow) (string, error) { + var reason sql.NullString + if _, err := queryOne(ctx, q, "SELECT reason FROM generations WHERE relationship_id = ? AND execution_generation = ?", []any{rel.ID, rel.Generation}, &reason); err != nil { + return "", err + } + return reason.String, nil +} + // CorrectionRequestID is the dispatch request id a correction generation opened by hand carries (generation-open --dispatch-request-id): derived from the plan, the node, the manifest and the generation, so // the generation row itself names the one manifest it was opened for and a generation opened under any other id is not bound to a manifest by dag-correct. func CorrectionRequestID(plan, node, manifestDigest string, generation int64) string { @@ -91,7 +111,8 @@ func (s *Scheduler) describeOpening(ctx context.Context, q store.Querier, plan, // an accepted head it ruled verified (disposition_conflict, naming this route) unless the criteria registered for the relationship changed (a node whose consumed input was replaced, its criteria untouched). The // route is bounded so that it cannot make a rerun or bind a manifest other than the one the generation was opened for: // -// - the node's accepted result is stale and its route, without this generation, is a correction: a result that is not accepted yet is corrected by a ruling, an accepted result that is current has no recorded correction route in this build, a change of the criteria alone is a revalidation and +// - the node's accepted result is stale and its route, without this generation, is a correction, or the result is accepted and still current and the generation's own reason states a correction +// (needs_changes_revision, the generation-open default, or accepted_result_correction): a result that is not accepted yet is corrected by a ruling, a change of the criteria alone is a revalidation and // opens no generation, and a node that landed was refused before this; // - the manifest is named (--manifest-digest) and is the one stored for this node at its current slice and criteria, as on the ruling route, and its inputs are still the ones the node's edges are satisfied // by now (VerifyManifest without the file bytes, as release checks its own intent): a predecessor accepted again after the prepare leaves another input than the one the child was told to consume; @@ -101,6 +122,11 @@ func (s *Scheduler) describeOpening(ctx context.Context, q store.Querier, plan, // What is recorded of how the instruction reached the child is that request id (dag_node_executions.managed_request_id, which a ruling leaves NULL) and the bound dispatch turn. It is the coordinator's // statement: the relay does not read the child's thread, so unlike the ruling route (which compares the restoration note with the instruction line) nothing here compares the dispatching message with the // line; the child checks the manifest file against the digest and hash the line names and answers blocked_needs_input on a mismatch. +// +// CRW-906: a result that was accepted and is still current (not stale) is corrected by hand as well, when a blocking defect is found in it before it lands and no ruling can open the generation: a bundle +// review that finds one in an accepted member is the case. The generation's own reason states the correction (needs_changes_revision, what generation-open writes by default, or accepted_result_correction, +// which names the route), the node must not have landed, and every check above is the one the stale route had. The acceptance stays active until the parent takes the new result with dag-accept --supersedes, +// and the reason of the generation row is what notes that the correction came through this route: nothing is added to the schema, no column and no refusal name is new. func (s *Scheduler) recordHandOpened(ctx context.Context, q store.Querier, plan string, snap dag.Snapshot, n dag.SnapNode, rel relRow, suppliedDigest string, out *CorrectionResult) error { before, err := store.LiveGenerationBefore(ctx, q, rel.ID, rel.Generation) if err != nil { @@ -130,8 +156,21 @@ func (s *Scheduler) recordHandOpened(ctx context.Context, q store.Querier, plan if err != nil { return err } + // CRW-906: an accepted result that is still current is corrected by hand as well, when the generation's own reason says a correction (needs_changes_revision or accepted_result_correction) and the node has + // not landed. Every check below is the one the stale route had. The reason of the generation row is what notes the route, so nothing is added to the schema and no refusal name is new. The caller refuses a + // generation whose reason states no correction before it reaches this binder (correction.go); the case below is this binder's own bound, so an accepted result is never recorded as corrected by a + // generation that was opened for something else. if st == nil && !ambiguousPrevious { - return refuse(contract.RefusalDispositionConflict, "%s, and the accepted result of %s is not stale, so it is not corrected by a generation opened by hand either: a result that is not accepted yet is corrected by a ruling, and an accepted result that is current has no recorded correction route in this build. A generation that only merged the base into the branch is not a correction: when the generation was ruled verified, dag-base-refresh records it, after proving from git that its head is the accepted head plus merges of the base", notRuled, n.NodeID) + reason, err := s.correctionOpenedReason(ctx, q, rel) + if err != nil { + return err + } + switch { + case !hasAcceptance: + return refuse(contract.RefusalDispositionConflict, "%s, and the accepted result of %s is not stale, so it is not corrected by a generation opened by hand either: a result that is not accepted yet is corrected by a ruling. A generation that only merged the base into the branch is not a correction: when the generation was ruled verified, dag-base-refresh records it, after proving from git that its head is the accepted head plus merges of the base", notRuled, n.NodeID) + case !correctionOpenReason(reason): + return refuse(contract.RefusalDispositionConflict, "%s, and the accepted result of %s is current: a generation opened by hand corrects it only when the reason it was opened under states a correction, and generation %d reads %q. Open the generation with generation-open --reason needs_changes_revision or %s, or, when it only merged the base into the branch, record it with dag-base-refresh", notRuled, n.NodeID, rel.Generation, reason, AcceptedResultCorrection) + } } // the same route a stale node has without the generation: a change of the criteria alone is ruled again (a revalidation, no generation), and what rests on a stale predecessor or an input that is not // there waits; only a result that must be reworked is corrected diff --git a/internal/relay/dagsched/revalidation_test.go b/internal/relay/dagsched/revalidation_test.go index 652d87a849..944d82cf0a 100644 --- a/internal/relay/dagsched/revalidation_test.go +++ b/internal/relay/dagsched/revalidation_test.go @@ -865,7 +865,8 @@ func TestReworkWithUnchangedCriteriaGoesThroughAGenerationOpenedByHand(t *testin } // What dag-correct will not bind for a generation opened by hand: one opened for another manifest, one nobody bound to a turn, one without a named manifest, a manifest that is not the node's, and a -// node whose result is not stale (those are corrected by a ruling). Each leaves no execution row. +// node whose result is not stale (those are corrected by a ruling). Each leaves no execution row. A result that was accepted and is still current is corrected by hand under a reason that states a +// correction (CRW-906, correction_accepted_test.go); under any other reason it is refused as well, and this is the case below. func TestAGenerationOpenedByHandIsBoundOnlyToTheManifestItWasOpenedFor(t *testing.T) { t.Parallel() bound := func(k *releaseKit) int { @@ -1009,14 +1010,14 @@ func TestAGenerationOpenedByHandIsBoundOnlyToTheManifestItWasOpenedFor(t *testin t.Fatalf("the refusal does not name the route: %v", err) } }) - t.Run("a node whose accepted result is current has no hand-opened route", func(t *testing.T) { + t.Run("a node whose accepted result is current under a reason that states no correction", func(t *testing.T) { k, accepted, _, _ := rvHandKit(t) ridB := accepted["B"].RelationshipID prepared := k.rvPrepare("sr", "B") - rvOpenByHand(t, k, ridB, prepared.DispatchRequestID, true) + acOpenByHand(t, k, ridB, prepared.DispatchRequestID, "initial_assignment", 2, true) if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); refusalReason(err) != "disposition_conflict" || k.count("SELECT COUNT(*) FROM dag_node_executions WHERE node_id = 'B' AND execution_generation = 2") != 0 { t.Fatalf("correction = %v", err) - } else if !strings.Contains(err.Error(), "not stale") { + } else if !strings.Contains(err.Error(), "not opened by a needs_changes ruling or a decision reply") { t.Fatalf("the refusal does not say why: %v", err) } }) diff --git a/internal/relay/dagsched/withdraw.go b/internal/relay/dagsched/withdraw.go index e6db0998c5..d3f8eedae6 100644 --- a/internal/relay/dagsched/withdraw.go +++ b/internal/relay/dagsched/withdraw.go @@ -112,7 +112,7 @@ func (s *Scheduler) WithdrawGeneration(ctx context.Context, plan, node, actor st return refuse(contract.RefusalUnknownGeneration, "%s has no generation %d", rel.ID, in.Generation) } // generation-open gives the generation a reason; a returning tenure's generation has none, and undoing a tenure is not what a withdrawal does (it leaves the supersession and the linkage as they are) - if opened.String != "initial_assignment" && opened.String != "needs_changes_revision" { + if opened.String != "initial_assignment" && !correctionOpenReason(opened.String) { return refuse(contract.RefusalDispositionConflict, "generation %d of %s was not opened by generation-open (its reason is %q): a returning tenure or a reply opened it, and a withdrawal does not undo that", in.Generation, rel.ID, opened.String) } if anchor != "anchor_pending" || turn.Valid || bound.Valid { diff --git a/internal/relay/registry/registry.go b/internal/relay/registry/registry.go index 4563e6e928..5df8d5ada7 100644 --- a/internal/relay/registry/registry.go +++ b/internal/relay/registry/registry.go @@ -23,7 +23,9 @@ const ( AnchorPending = "anchor_pending" ) -var reasons = []string{"initial_assignment", "needs_changes_revision"} +// reasons are the values generation-open accepts for --reason: the assignment itself, the correction of a stale result, and the correction of a result that is accepted and still current +// (dagsched's AcceptedResultCorrection, which recordHandOpened admits under the same checks). +var reasons = []string{"initial_assignment", "needs_changes_revision", "accepted_result_correction"} func isLive(status string) bool { return status == "active" || status == "paused" } diff --git a/plugins/crw/.codex-plugin/plugin.json b/plugins/crw/.codex-plugin/plugin.json index e710575e5b..7bfa5deae7 100644 --- a/plugins/crw/.codex-plugin/plugin.json +++ b/plugins/crw/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "crw", - "version": "0.4.0+2752172a3d91", + "version": "0.4.0+e1c14d5b9dc7", "description": "Linear-driven workflow skills for Codex: define an initiative, plan projects and issues, run delivery, and check the result against its canonical criteria.", "author": { "name": "thisisjun786", diff --git a/plugins/crw/skills/crw-run/references/relay.md b/plugins/crw/skills/crw-run/references/relay.md index cc74397b7c..71ece5417a 100644 --- a/plugins/crw/skills/crw-run/references/relay.md +++ b/plugins/crw/skills/crw-run/references/relay.md @@ -1326,8 +1326,13 @@ A `needs_changes` verdict opens the generation itself. Open one by hand when not --dispatch-request-id --reason needs_changes_revision \ --dispatch-turn-id -`--reason` takes `initial_assignment` or `needs_changes_revision`; anything else is refused with -`unknown_generation`. Opening SENDS nothing, because the command is store-only and a generation is +`--reason` takes `initial_assignment`, `needs_changes_revision` or `accepted_result_correction`; anything +else is refused with `unknown_generation`. The last names the route for a result the plan already accepted +and that is still current, when a blocking defect is found in it before it lands: `dag-correct` records that +generation as a correction under the checks the stale case has, and the parent takes the corrected result +with `dag-accept --supersedes` +([Correcting a result that was accepted and is still current](../../../../../docs/relay/dag-scheduler.md#correcting-a-result-that-was-accepted-and-is-still-current)). +Opening SENDS nothing, because the command is store-only and a generation is not an event the relay can deliver. The child learns about the re-review the same way it learned about the original assignment, through whatever transport dispatched it, and the turn that resume creates is the new generation's anchor. Supply that turn when opening, as above, or afterwards: From 3252c353244db52acdf8de635e9981a2da78a3a0 Mon Sep 17 00:00:00 2001 From: codex Date: Wed, 7 Oct 2026 13:10:36 +0900 Subject: [PATCH 02/28] CRW-906: name the correction route where the ruling refusal and the route table point A refusal that told the coordinator an accepted current result has no recorded correction route, and the normative route table that repeated it, are stale now that the route exists. The refusal names the steps and the reason that states the correction, and the route table points at the procedure. The ruling refusal's own test asserts the new text. --- docs/relay/README.md | 2 +- internal/relay/delivery/rulingchange.go | 2 +- internal/relay/delivery/rulingchange_test.go | 8 +++++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/relay/README.md b/docs/relay/README.md index b86518c5f9..542bc60dcd 100644 --- a/docs/relay/README.md +++ b/docs/relay/README.md @@ -840,7 +840,7 @@ When a step refuses, the refusal names what to do: | recorded `verified`, asked `unverified` or `aborted` | `disposition_conflict` | rule `needs_changes`, or stop the assignment by changing the relationship's status | | recorded `needs_changes` | `disposition_conflict` | the ruling opened a generation and is not withdrawn: rule the head of that generation when the child reports there | | recorded `unverified` or `aborted` | `disposition_conflict` | the ruling is final for the event: open a fresh execution generation (`generation-open`, then `generation-bind`) and rule what the child reports there | -| a plan accepted the verified event | `disposition_conflict` | read the node's stale reading: when its action is `correct`, `dag-correct --prepare` prints the instruction and the dispatch request id, the generation is opened by hand and `dag-correct` binds it; `revalidate` and `hold` have their own steps; when the accepted result is current, a base that moved after the acceptance is recorded by `dag-base-refresh` (a generation opened by hand that only merges the base, ruled `verified`; the relay proves from git that its head is the accepted head plus merges of the base), and for any other current result this build records no correction route, so report it and open no generation that `dag-correct` will refuse | +| a plan accepted the verified event | `disposition_conflict` | read the node's stale reading: when its action is `correct`, `dag-correct --prepare` prints the instruction and the dispatch request id, the generation is opened by hand and `dag-correct` binds it; `revalidate` and `hold` have their own steps; when the accepted result is current and a defect was found in it before it landed, the same steps correct it, with the generation opened under a reason that states the correction (`accepted_result_correction`, or the `needs_changes_revision` default), after which `dag-accept --supersedes` takes the corrected result ([the scheduler](dag-scheduler.md#correcting-a-result-that-was-accepted-and-is-still-current)); when only the base moved, `dag-base-refresh` records it (a generation opened by hand that only merges the base, ruled `verified`; the relay proves from git that its head is the accepted head plus merges of the base) | | the work is marked merged, or a turn landed it | `disposition_conflict` | a merged result is corrected by new work, not by a second ruling | | a merge turn is merging or of unknown effect | `disposition_conflict` | resolve the turn (`merge-turn-resolve` reads the branch), then rule again | | the event is not the head | `stale_generation` or `superseded_revision` | rule the head the assignment shows | diff --git a/internal/relay/delivery/rulingchange.go b/internal/relay/delivery/rulingchange.go index 5484c314b2..ee8145e629 100644 --- a/internal/relay/delivery/rulingchange.go +++ b/internal/relay/delivery/rulingchange.go @@ -63,7 +63,7 @@ func builtOn(eventID, turn string, rest registry.Rest) error { prefix := fmt.Sprintf("%s is already ruled verified (verdict turn %s), and a ruling of needs_changes cannot replace it: ", strconv.Quote(eventID), strconv.Quote(turn)) switch rest.Kind { case registry.RestsAccepted: - return refuse(DispositionConflict, "%splan %s accepted it as acceptance %s (node %s), and an accepted result is no longer open to a second ruling. Read the node's stale reading: when its action is correct, dag-correct --prepare prints the instruction and the dispatch request id, the generation is opened by hand and dag-correct binds it; when the action is revalidate or hold, that action's own step applies; when the accepted result is current, a base that moved after the acceptance has its own route, dag-base-refresh: the child merges the base into the branch in a generation opened by hand and changes nothing else, the parent rules that generation verified and records it, and the relay proves from git that its head is the accepted head plus merges of the base (crw-run's 'A base refresh the child made after the acceptance'); for every other current result there is no recorded correction route in this build, so report it on the coordination record and do not open a generation dag-correct will refuse", + return refuse(DispositionConflict, "%splan %s accepted it as acceptance %s (node %s), and an accepted result is no longer open to a second ruling. Read the node's stale reading: when its action is correct, dag-correct --prepare prints the instruction and the dispatch request id, the generation is opened by hand and dag-correct binds it; when the action is revalidate or hold, that action's own step applies; when the accepted result is current and a defect was found in it before it landed, the same steps correct it: dag-correct --prepare prints the instruction and the dispatch request id, the generation is opened under a reason that states the correction (accepted_result_correction, or the needs_changes_revision default), the instruction is dispatched and that turn is bound, and dag-correct --manifest-digest binds it, after which dag-accept --supersedes takes the corrected result; when the accepted result is current, a base that moved after the acceptance has its own route, dag-base-refresh: the child merges the base into the branch in a generation opened by hand and changes nothing else, the parent rules that generation verified and records it, and the relay proves from git that its head is the accepted head plus merges of the base (crw-run's 'A base refresh the child made after the acceptance'). A generation opened under a reason that states no correction is refused, naming the reason it carries", prefix, rest.Plan, rest.ID, rest.Node) case registry.RestsMarkedMerged: return refuse(DispositionConflict, "%sthe work is marked merged (at %s), and a merged result is corrected by new work, not by a second ruling", prefix, rest.At) diff --git a/internal/relay/delivery/rulingchange_test.go b/internal/relay/delivery/rulingchange_test.go index 45511e905f..e814500e1b 100644 --- a/internal/relay/delivery/rulingchange_test.go +++ b/internal/relay/delivery/rulingchange_test.go @@ -237,10 +237,12 @@ func TestRC04_acceptance_and_a_merge_mark_close_the_change(t *testing.T) { event := h.managedVerified() h.rcAccept(event) detail := h.rcRefused(event, "needs_changes", "v2", rcRestoration(), nil, DispositionConflict) - rcMentions(t, detail, "acceptance-1", "plan-1", "node-1", "dag-correct --prepare", "stale", "no recorded correction route", "generation dag-correct will refuse") + rcMentions(t, detail, "acceptance-1", "plan-1", "node-1", "dag-correct --prepare", "stale", "accepted_result_correction", "dag-correct --manifest-digest", "dag-accept --supersedes") rcMentions(t, detail, "a base that moved after the acceptance", "dag-base-refresh", "merges of the base") - if strings.Contains(detail, "a base that moved after the acceptance is such a case") { - t.Fatalf("the refusal still files the moved base under the case that has no route: %s", detail) + // CRW-906: a defect found in a current accepted result has a recorded route, so the refusal must not + // tell the coordinator that no route exists for it + if strings.Contains(detail, "no recorded correction route") { + t.Fatalf("the refusal still says a current accepted result has no correction route: %s", detail) } }) t.Run("merged", func(t *testing.T) { From d2b442d107df03eb91c622b5ce7f2d8f1d5d771d Mon Sep 17 00:00:00 2001 From: codex Date: Wed, 7 Oct 2026 05:14:17 +0000 Subject: [PATCH 03/28] CRW-906: pin that the correction reason does not loosen the stale route gate The new reason is a correction reason like the default, not a route of its own: a stale node whose route says correct records under it, and one whose route is revalidate is still refused whatever reason it carries. Both cases are pinned so the admission cannot be read as a loosening of the gate. --- .../dagsched/correction_accepted_test.go | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/internal/relay/dagsched/correction_accepted_test.go b/internal/relay/dagsched/correction_accepted_test.go index dbe3e2c0e8..cd0764d5a4 100644 --- a/internal/relay/dagsched/correction_accepted_test.go +++ b/internal/relay/dagsched/correction_accepted_test.go @@ -261,3 +261,43 @@ func TestAHandOpenedGenerationOfAnAcceptedCurrentResultIsStillRefused(t *testing } }) } + +// The reason a generation was opened under is what states the correction, and the stale route keeps its own +// gate: a stale node whose route says correct is recorded under either reason (the new value is a correction +// reason like the default, not a route of its own), and one whose route is anything else is still refused +// whatever reason it carries. This pins that the change admits one case rather than loosening the gate. +func TestTheCorrectionReasonDoesNotLoosenTheStaleRouteGate(t *testing.T) { + t.Parallel() + t.Run("a stale node whose route is correct records under the new reason", func(t *testing.T) { + k, _, ridC, prepared := rvHandKit(t) + if got := rvAction(t, k.read("sr"), "C"); got != rvCorrect { + t.Fatalf("the route of C = %q, want %q: the premise of the stale correction", got, rvCorrect) + } + acOpenByHand(t, k, ridC, prepared.DispatchRequestID, "accepted_result_correction", 2, true) + res, err := k.sched.RecordCorrection(context.Background(), "sr", "C", "parent", prepared.ManifestDigest) + if err != nil || res.Replayed || res.OpenedBy != OpenedByGenerationOpen || res.ManifestDigest != prepared.ManifestDigest { + t.Fatalf("a stale correction under the new reason = %v %+v", err, res) + } + if got := acReason(k, ridC, 2); got != "accepted_result_correction" { + t.Fatalf("the generation's reason = %q", got) + } + }) + t.Run("a stale node whose route is revalidate is refused whatever reason it carries", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + ridA := accepted["A"].RelationshipID + // only A's criteria change: its route is revalidate, which opens no generation + k.rvReregister("sr", "A", "sr-r2", ridA, dig("A's new criteria"), nil) + if got := rvAction(t, k.read("sr"), "A"); got != rvRevalidate { + t.Fatalf("the route of A = %q, want %q: the premise", got, rvRevalidate) + } + prepared := k.rvPrepare("sr", "A") + acOpenByHand(t, k, ridA, prepared.DispatchRequestID, "accepted_result_correction", 2, true) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "A", "parent", prepared.ManifestDigest); refusalReason(err) != "disposition_conflict" { + t.Fatalf("a revalidation recorded as a correction = %v, want disposition_conflict", err) + } else if !strings.Contains(err.Error(), "revalidate") { + t.Fatalf("the refusal does not name the route: %v", err) + } else if got := acExecution(k, "A", 2); got != "" { + t.Fatalf("the refused correction bound %q", got) + } + }) +} From 19899c37088abc047e604d10241fcf98731f5263 Mon Sep 17 00:00:00 2001 From: codex Date: Wed, 7 Oct 2026 18:38:17 +0900 Subject: [PATCH 04/28] CRW-906: refuse to merge an accepted result whose correction generation is open The issue's decision 2 promises that a node under correction is not merged on its old head. The bundle open and the single lane refuse a pull request head that differs from the accepted head, and that guard holds only once the child has pushed: between opening (or recording) the correction generation and that first push the pull request still shows the accepted head H, so a bundle open, a bundle verify, a bundle land and the single-lane judgement all carried H. The merge gate now reads the relationship's live execution generation against the generation its active acceptance stands on (acceptance.UnderCorrection, whose two values are one statement). While the live one is later, the node is under correction and the head the acceptance stands on is refused disposition_conflict, naming the open generation: dag-merge-judge and dag-merge-request write no row and no turn, and trainStandRefusal (open, verify and land) refuses a bundle member. A correction accepted over with dag-accept --supersedes, or withdrawn, puts the node back in the lane, and a generation recorded by dag-base-refresh moves the stand generation with the live one, so a base refresh is not a correction. No refusal name, column, schema, CLI flag or specs.json entry is added. --- docs/relay/dag-scheduler.md | 4 + internal/relay/acceptance/stand.go | 72 ++++++++ .../relay/acceptance/undercorrection_test.go | 154 +++++++++++++++++ internal/relay/dagsched/mergejudge.go | 27 +++ .../mergejudge_undercorrection_test.go | 163 ++++++++++++++++++ internal/relay/mergeturn/train.go | 41 +++-- .../mergeturn/train_undercorrection_test.go | 101 +++++++++++ 7 files changed, 546 insertions(+), 16 deletions(-) create mode 100644 internal/relay/acceptance/undercorrection_test.go create mode 100644 internal/relay/dagsched/mergejudge_undercorrection_test.go create mode 100644 internal/relay/mergeturn/train_undercorrection_test.go diff --git a/docs/relay/dag-scheduler.md b/docs/relay/dag-scheduler.md index b04d9b54a5..b2ddcc2ced 100644 --- a/docs/relay/dag-scheduler.md +++ b/docs/relay/dag-scheduler.md @@ -201,6 +201,8 @@ A blocking defect can be found in a result that the plan already accepted and th 3. `dag-correct --manifest-digest D` records the generation as a `correction` execution of the node (`opened_by` `generation_open`), under every check the stale case has: the generation right after the one the acceptance stands on, the manifest stored for the node at its current slice and criteria, the dispatch request id derived from that manifest, a bound dispatch turn, and the inputs as they stand now. The node must not have landed, and a node whose result is not accepted yet keeps the ruling route. 4. The child reports in the generation, the parent rules it and takes the result with `dag-accept --supersedes `. Until then the previous acceptance stays active, and the reading of the node whose current generation is not recorded as an execution reads `blocked:stale_head`, so nothing downstream consumes the corrected generation before it is accepted. + The accepted head is held back while the correction is open, not only after the child pushes. The merge gate reads the relationship's live execution generation against the generation its active acceptance stands on: while the live one is later, the node is **under correction** and its old head is not a merge candidate. The single-lane judgement (`dag-merge-judge`, `dag-merge-request`) is refused `disposition_conflict` naming the open generation and writes no row and no turn, and the bundle open, verify and land refuse a member the same way (see [Merge eligibility](#merge-eligibility)). Without this, a bundle or a lane turn taken between opening (or recording) the generation and the child's first push would carry and land exactly the head the correction is repairing, because until that push the pull request still shows the accepted head. The gate is about the correction being open, not about the head: `dag-accept --supersedes` of the new generation's result moves the acceptance to the stand generation and the node is judged again, a withdrawn generation puts the live generation back and the old head may ride again, and a generation recorded by [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) moves the stand generation with the live one, so a base refresh is not a correction. + What notes the route is the generation's own `reason` column: no column, table, schema, refusal name, CLI flag or `specs.json` entry is added. A generation opened under a reason that states no correction (the assignment itself, a returning tenure, a reply) is refused by the same step, naming the reason it carries, and a generation opened for a base refresh is not bound here either: [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) records that one. A generation opened for this route and never sent is withdrawn like any other ([A generation opened by hand and never sent](#a-generation-opened-by-hand-and-never-sent)). ### Input checks @@ -636,6 +638,8 @@ request (by the forge identity recorded with the acceptance) and the base branch A stale accepted result is not judged at all: a stale result never merges (contract 8.2, [Invalidation](#invalidation)). The call is refused `disposition_conflict` naming the stale reason, ahead of every rule above (an evicted head included), and writes no row of the history. The judgement is asked again inside the transaction that writes, so a plan revision that lands between the read of the pull request and the write is seen; `dag-merge-request` asks it once more in the transaction that creates the turn (after the criteria and the predecessors, so a criteria change that lands in that gap is `criteria_set_changed` and an integrated or code-pinned edge added in it, whose predecessor has not landed, is the unmet predecessor), and a revision that lands after the judgement has recorded an `eligible` row leaves that row in the history and creates no turn. A result that is current again (the plan repaired, the node accepted again) is judged as before. +A result that is **under correction** is not judged either, and this one is about the generation rather than the head: while the relationship's live execution generation is later than the generation its active acceptance stands on, a correction generation is open over the accepted result and has not been accepted over yet, so the head the acceptance stands on is the very result being repaired. The call is refused `disposition_conflict` naming the open generation and the generation the acceptance stands on, ahead of every rule in the table above (an evicted head included), and writes no row of the history; `dag-merge-request` makes the same judgement, so it creates no turn. The reading is taken with the acceptance in one statement and asked again in the transaction that writes, so a generation opened while the pull request is being read is seen. It is the guard [Correcting a result that was accepted and is still current](#correcting-a-result-that-was-accepted-and-is-still-current) rests on before the child has pushed: without it, a bundle or a lane turn taken between opening (or recording) the generation and that first push would carry and land the accepted head, which the pull request still shows. A correction accepted over with `dag-accept --supersedes` moves the acceptance to the stand generation and the node is judged as before; a withdrawn generation puts the live generation back and the old head may ride again; a generation recorded by [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) moves the stand generation with the live one, so a base refresh is not a correction. The merge train's own gate (`trainStandRefusal`) refuses a bundle member the same way at open, verify and land, naming the open generation. + A required check is read the way merge-evidence and the merge lane's own check read it: of every run only its newest attempt counts, every run of a required name must be a `success` (a skipped or neutral run is not one, so a head the judgement lets in is not refused by the lane for the same check), a required (name, integration) pair needs its own run (a name that two integrations must answer needs both), and the opaque run identities are never ordered. Failures count only when every required check has finished, so one red job seen beside a running one never uses up the retry. A second, different failure after the retry round opened evicts. A failure is told from another by the run's identity, its attempt and the time the forge last changed it (a commit status has no attempt and a check run can be reset in place), and a failure of a run that a judgement saw not failing (running or passing) since it failed is a new failure whatever its identity; another check running again says nothing about this one. Everything is computed from the newest reading of each run (a listing that still carries a failed first attempt beside the passing second one judges as the second), a failure counts once a judgement recorded it (a red job beside a running one is not counted), and a reading of a run that is older than one already judged (an earlier attempt, or an earlier completion of the same attempt, arriving late) is not judged: it is `merge_candidate_moved` and the caller reads again. So is a reading of the pull request that was in flight while another judgement of the node was recorded: it is older than the history whatever it contains, and nothing is written. Times are compared as instants (the forge's spelling does not matter). A forge that returns an older state of a run after a newer one was judged cannot always be told from a run that was reset: a reading that goes backwards without a time and with no judgement between the two (a run read as running after its completion) can count as a rerun, so a head can be evicted one failure early. Two failures that carry the same identity and the same time and that no judgement saw apart read as one failure: the node is not eligible either way, but it can take more than one retry. A pull request whose list of required checks cannot be read is not judged (`merge_evidence_malformed`, nothing written): ignorance is not "none required". A draft, closed or merged pull request is `disposition_conflict` (a merged one once its reading is classified, [The forge reading of a merged pull request](#the-forge-reading-of-a-merged-pull-request)); evidence the relay could not read completely is the host's failure; both write nothing. A row of the history that no longer digests to what was recorded (B-13) is `revision_mismatch`, and the edges built on it read `blocked:evidence_mismatch`. diff --git a/internal/relay/acceptance/stand.go b/internal/relay/acceptance/stand.go index 31c978d8f0..0ca58b9a93 100644 --- a/internal/relay/acceptance/stand.go +++ b/internal/relay/acceptance/stand.go @@ -126,6 +126,78 @@ func ActiveForRelationship(ctx context.Context, q store.Querier, relationshipID return a, true, nil } +// ActiveWithLiveGeneration is ActiveForRelationship with the relationship's own live execution +// generation taken in the same statement, so the acceptance and the generation are one reading. The +// generation moves when a generation is opened, withdrawn or accepted over, and the acceptance moves +// when one is accepted: a correction gate that read the two apart could see neither move, and would let +// a node under correction be judged on the head its old acceptance stands on (CRW-906). +// +// The acceptance is read exactly as ActiveForRelationship reads it, and the generation comes from the +// relationship the acceptance names, in the same statement. A store whose relationships row is absent +// (a fixture that seeded only the acceptance, as some tests do) answers the acceptance's own +// generation, which is the reading that says no correction is open. found is false when the +// relationship has no active acceptance; the live generation is then read on its own, because no +// correction gate turns on it there (the caller's own "no acceptance" refusal wins), and a store that +// predates the zone table has no acceptance at all. +func ActiveWithLiveGeneration(ctx context.Context, q store.Querier, relationshipID string) (active Active, generation int64, found bool, err error) { + present, err := tableExists(ctx, q, "dag_acceptances") + if err != nil || !present { + return Active{}, 0, false, err + } + row := q.QueryRowContext(ctx, "SELECT a.acceptance_id, a.event_id, a.head_sha, a.revision_hash, a.execution_generation,"+ + " COALESCE(r.execution_generation, a.execution_generation)"+ + " FROM dag_acceptances a LEFT JOIN relationships r ON r.relationship_id = a.relationship_id"+ + " WHERE a.relationship_id = ? AND a.state = 'active' ORDER BY a.accepted_at DESC, a.acceptance_id DESC LIMIT 1", relationshipID) + var head sql.NullString + if err := row.Scan(&active.AcceptanceID, &active.EventID, &head, &active.RevisionHash, &active.Generation, &generation); err != nil { + if errors.Is(err, sql.ErrNoRows) { + live, liveErr := liveGeneration(ctx, q, relationshipID) + return Active{}, live, false, liveErr + } + return Active{}, 0, false, err + } + active.HeadSHA = head.String + return active, generation, true, nil +} + +// liveGeneration is the generation a relationship stands on now, for the relationship that has no active +// acceptance. 0 means the store holds no such relationship. +func liveGeneration(ctx context.Context, q store.Querier, relationshipID string) (int64, error) { + var generation int64 + if err := q.QueryRowContext(ctx, "SELECT execution_generation FROM relationships WHERE relationship_id = ?", relationshipID).Scan(&generation); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return 0, nil + } + return 0, err + } + return generation, nil +} + +// UnderCorrection is the CRW-906 gate: whether the relationship's live execution generation is later +// than the generation its active acceptance stands on. A correction generation is then open (opened by +// hand, a ruling or a decision reply) and has not been accepted over yet, so the node is under +// correction and its old head is not a merge candidate: the defect the correction answers was found in +// the accepted result, and carrying that head to the base would land exactly what is being repaired. +// +// The stand generation counts a recorded base refresh (StandOf), so a generation that only carried the +// accepted head onto a moved base is not a correction: it moves the stand generation with it and the +// member may ride again. A withdrawn correction generation moves the live generation back to the +// generation the acceptance stands on, and the old head may ride again as well. +// +// under is false when the relationship has no active acceptance: that is the caller's own refusal, not +// this one, so the two are told apart. A store that predates the acceptance zone answers false. +func UnderCorrection(ctx context.Context, q store.Querier, relationshipID string) (under bool, live, stand int64, err error) { + active, live, found, err := ActiveWithLiveGeneration(ctx, q, relationshipID) + if err != nil || !found { + return false, live, 0, err + } + reading, err := StandOf(ctx, q, active.AcceptanceID, relationshipID, active.Generation, active.EventID, active.RevisionHash, active.HeadSHA) + if err != nil { + return false, live, 0, err + } + return live > reading.Generation, live, reading.Generation, nil +} + // RulingHead is the head a ruling fixed for an event: dag_verified_heads.head_sha for that event // (CRW-742), or the acceptance's own head when the event has no row, with source "acceptance". func RulingHead(ctx context.Context, q store.Querier, eventID, acceptanceHead string) (head, source string, err error) { diff --git a/internal/relay/acceptance/undercorrection_test.go b/internal/relay/acceptance/undercorrection_test.go new file mode 100644 index 0000000000..54aaa5312a --- /dev/null +++ b/internal/relay/acceptance/undercorrection_test.go @@ -0,0 +1,154 @@ +package acceptance + +import ( + "context" + "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" +) + +// CRW-906 generation 2: the correction gate both the merge train and the single-lane judgement read. A +// relationship whose live execution generation is later than the generation its active acceptance stands +// on has a correction open over an accepted result that has not been accepted over yet, and the accepted +// head is not a merge candidate. These tests pin the reading on temporary stores, including the two +// cases that must NOT read as a correction: a recorded base refresh (which moves the stand generation +// with the live one) and a withdrawn generation (which moves the live generation back). + +// ucStore is a temporary store with one relationship and its acceptance, at the generations given. +func ucStore(t *testing.T, relationship string, acceptanceGeneration, liveGeneration int64) store.Querier { + t.Helper() + s := openStore(t) + ctx := context.Background() + if _, err := s.DB.ExecContext(ctx, "INSERT INTO relationships (relationship_id, issue_key, status, parent_task_id, parent_host_id, child_task_id, child_host_id, execution_generation, artifact_roots, allowed_recipients, created_at, updated_at)"+ + " VALUES (?, 'ISS-1', 'active', 'task-parent', 'host-p', 'task-child', 'host-c', ?, '[]', '[\"task-parent\"]', '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')", + relationship, liveGeneration); err != nil { + t.Fatal(err) + } + if _, err := s.DB.ExecContext(ctx, "INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES ('acc-1','p','n','m',?,?,'ev-1','rev-1','c','verified','head-1','owner/repo',1,'bound','t','{}','task-parent',0,'2026-10-01T00:00:00Z','active')", + relationship, acceptanceGeneration); err != nil { + t.Fatal(err) + } + q := s.Querier(ctx) + return q +} + +// TestUnderCorrectionReadsTheLiveGenerationAgainstTheStand: the gate is exactly "the live generation is +// later than the generation the acceptance stands on". +func TestUnderCorrectionReadsTheLiveGenerationAgainstTheStand(t *testing.T) { + t.Run("the acceptance stands on the live generation", func(t *testing.T) { + q := ucStore(t, "rel-1", 1, 1) + under, live, stand, err := UnderCorrection(context.Background(), q, "rel-1") + if err != nil { + t.Fatal(err) + } + if under || live != 1 || stand != 1 { + t.Fatalf("UnderCorrection = under %v, live %d, stand %d; want false, 1, 1", under, live, stand) + } + }) + t.Run("a correction generation is open", func(t *testing.T) { + q := ucStore(t, "rel-1", 1, 2) + under, live, stand, err := UnderCorrection(context.Background(), q, "rel-1") + if err != nil { + t.Fatal(err) + } + if !under || live != 2 || stand != 1 { + t.Fatalf("UnderCorrection = under %v, live %d, stand %d; want true, 2, 1", under, live, stand) + } + }) + t.Run("the acceptance moved past the correction generation", func(t *testing.T) { + // dag-accept --supersedes of the corrected result: both moved, so nothing is under correction + q := ucStore(t, "rel-1", 2, 2) + under, _, _, err := UnderCorrection(context.Background(), q, "rel-1") + if err != nil { + t.Fatal(err) + } + if under { + t.Fatal("an accepted-over correction still reads under correction") + } + }) + t.Run("the relationship has no active acceptance", func(t *testing.T) { + s := openStore(t) + ctx := context.Background() + if _, err := s.DB.ExecContext(ctx, "INSERT INTO relationships (relationship_id, issue_key, status, parent_task_id, parent_host_id, child_task_id, child_host_id, execution_generation, artifact_roots, allowed_recipients, created_at, updated_at)"+ + " VALUES ('rel-none', 'ISS-1', 'active', 'task-parent', 'host-p', 'task-child', 'host-c', 2, '[]', '[\"task-parent\"]', '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')"); err != nil { + t.Fatal(err) + } + q := s.Querier(ctx) + under, live, stand, err := UnderCorrection(ctx, q, "rel-none") + if err != nil { + t.Fatal(err) + } + // no acceptance is the caller's own refusal, not this one: the gate must not claim it + if under || stand != 0 || live != 2 { + t.Fatalf("UnderCorrection with no acceptance = under %v, live %d, stand %d; want false, 2, 0", under, live, stand) + } + }) +} + +// TestUnderCorrectionIsNotARecordedBaseRefresh: a generation that only carried the accepted head onto a +// moved base is not a correction. The refresh moves the stand generation to the live one, so the member +// may ride again on the refreshed head, exactly as dag-base-refresh intends. +func TestUnderCorrectionIsNotARecordedBaseRefresh(t *testing.T) { + q := ucStore(t, "rel-1", 1, 2) + ctx := context.Background() + head := "head-refreshed" + event, revision := "ev-refresh", "rev-refreshed" + id := RefreshDigest("acc-1", "rel-1", 2, event, revision, head, "owner/repo", "dev", "base-0", "{}", "[]") + if _, err := q.ExecContext(ctx, "INSERT INTO dag_base_refreshes (refresh_id, acceptance_id, refresh_seq, relationship_id, execution_generation, event_id, revision_hash, head_sha, base_repository, base_ref, base_tip_sha, proof_json, resolved_paths_json, recorded_by_task_id, coordinator_epoch, recorded_at)"+ + " VALUES (?,?,1,?,?,?,?,?,?,?,?,?,?,?,?,?)", + id, "acc-1", "rel-1", int64(2), event, revision, head, "owner/repo", "dev", "base-0", "{}", "[]", "task-parent", int64(0), "2026-10-01T00:00:00Z"); err != nil { + t.Fatal(err) + } + under, live, stand, err := UnderCorrection(ctx, q, "rel-1") + if err != nil { + t.Fatal(err) + } + if under || live != 2 || stand != 2 { + t.Fatalf("UnderCorrection after a base refresh = under %v, live %d, stand %d; want false, 2, 2", under, live, stand) + } +} + +// TestActiveWithLiveGenerationReadsTheAcceptanceAsBefore: the reading is the acceptance +// ActiveForRelationship answered, with the live generation taken in the same statement, and a store +// whose relationships row is absent (a fixture that seeded only the acceptance) answers the +// acceptance's own generation rather than claiming a correction. +func TestActiveWithLiveGenerationReadsTheAcceptanceAsBefore(t *testing.T) { + q := ucStore(t, "rel-1", 1, 3) + active, live, found, err := ActiveWithLiveGeneration(context.Background(), q, "rel-1") + if err != nil || !found { + t.Fatalf("ActiveWithLiveGeneration = %v %v %v", active, found, err) + } + if active.AcceptanceID != "acc-1" || active.EventID != "ev-1" || active.HeadSHA != "head-1" || active.RevisionHash != "rev-1" || active.Generation != 1 { + t.Fatalf("the acceptance read = %+v", active) + } + if live != 3 { + t.Fatalf("live generation = %d, want 3", live) + } + plain, found, err := ActiveForRelationship(context.Background(), q, "rel-1") + if err != nil || !found { + t.Fatal(err) + } + if plain != active { + t.Fatalf("the acceptance read apart from the generation = %+v, want %+v", plain, active) + } + // an absent relationship row: the acceptance's own generation stands in, so no correction is claimed + s := openStore(t) + ctx := context.Background() + if _, err := s.DB.ExecContext(ctx, "INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES ('acc-2','p','n','m','rel-orphan',4,'ev-2','rev-2','c','verified','head-2','owner/repo',1,'bound','t','{}','task-parent',0,'2026-10-01T00:00:00Z','active')"); err != nil { + t.Fatal(err) + } + oq := s.Querier(ctx) + _, live, found, err = ActiveWithLiveGeneration(ctx, oq, "rel-orphan") + if err != nil || !found { + t.Fatalf("an acceptance with no relationship row = %v %v", found, err) + } + if live != 4 { + t.Fatalf("live generation without a relationships row = %d, want the acceptance's own 4", live) + } + under, _, _, err := UnderCorrection(ctx, oq, "rel-orphan") + if err != nil || under { + t.Fatalf("UnderCorrection without a relationships row = %v %v, want false", under, err) + } +} diff --git a/internal/relay/dagsched/mergejudge.go b/internal/relay/dagsched/mergejudge.go index 630de5efb0..5e913ef8ee 100644 --- a/internal/relay/dagsched/mergejudge.go +++ b/internal/relay/dagsched/mergejudge.go @@ -7,6 +7,7 @@ import ( "strconv" "github.com/thisisjun786/codex-relay-workflow/internal/contract" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/acceptance" "github.com/thisisjun786/codex-relay-workflow/internal/relay/dag" "github.com/thisisjun786/codex-relay-workflow/internal/relay/mergeturn" "github.com/thisisjun786/codex-relay-workflow/internal/relay/registry" @@ -30,6 +31,14 @@ type JudgeResult struct { // Eligible is whether the judgement lets the pull request go to the merge lane. func (r JudgeResult) Eligible() bool { return r.Outcome == OutcomeEligible } +// refuseUnderCorrection is the CRW-906 refusal: the accepted result is being corrected in a later +// generation, so the head its acceptance stands on is not judged, not eligible and not carried to the +// merge lane. It is the existing disposition_conflict and it names the open generation, so a parent +// reading the refusal knows which generation to finish or withdraw; no refusal name is added. +func refuseUnderCorrection(node string, live, standGeneration int64) error { + return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is under correction: generation %d is open over the acceptance, which stands on generation %d, so its head is not judged and does not go to the merge lane; accept the corrected result with dag-accept --supersedes, or withdraw the generation", node, live, standGeneration) +} + const maxRounds = 2 // completedConclusions are the answers a finished check gives; anything else (empty, pending, queued, in_progress) has not finished. @@ -218,6 +227,17 @@ func (s *Scheduler) Judge(ctx context.Context, plan, node, actor string, in Judg case pr.HeadSHA != stand.Head: m.Outcome, m.Reason = OutcomeStaleHead, staleHeadReason(pr.HeadSHA, acc.HeadSHA, stand) default: + // CRW-906: this is the head the acceptance stands on, and the relationship's live generation + // is later than the generation it stands on, so the accepted result is under correction and + // the head the judgement is about is the result being repaired. Refused, and nothing is + // written: an eligible row here would let the old head ride to the base. A pull request at + // another head is stale_head above, as it was before this change, and a correction that is + // accepted over or withdrawn moves the stand generation and this gate stops firing. + if under, live, standGeneration, err := acceptance.UnderCorrection(txCtx, tx, still.RelationshipID); err != nil { + return err + } else if under { + return refuseUnderCorrection(node, live, standGeneration) + } landed, why, err := s.predecessorsLanded(txCtx, tx, plan, current, node) if err != nil { return err @@ -357,6 +377,13 @@ func (s *Scheduler) RequestMergeTurn(ctx context.Context, plan, node, actor stri if err != nil { return err } + // CRW-906: no turn is requested for a result whose correction generation opened while the + // judgement above was being made, so the lane never carries the head being repaired + if under, live, standGeneration, err := acceptance.UnderCorrection(txCtx, tx, acc.RelationshipID); err != nil { + return err + } else if under { + return refuseUnderCorrection(node, live, standGeneration) + } if landed, why, err := s.predecessorsLanded(txCtx, tx, plan, current, node); err != nil { return err } else if !landed { diff --git a/internal/relay/dagsched/mergejudge_undercorrection_test.go b/internal/relay/dagsched/mergejudge_undercorrection_test.go new file mode 100644 index 0000000000..7c964308da --- /dev/null +++ b/internal/relay/dagsched/mergejudge_undercorrection_test.go @@ -0,0 +1,163 @@ +package dagsched + +import ( + "context" + "database/sql" + "strings" + "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/relay/registry" +) + +// CRW-906 generation 2: the merge gate of an accepted result that is being corrected. The issue's +// decision 2 promises that a node under correction is not merged on its old head. The bundle open and +// the single lane refuse a pull request head that differs from the accepted head, and that guard holds +// only once the child has pushed: between opening (or recording) the correction generation and that +// first push the pull request still shows the accepted head H, so the single-lane judgement must refuse +// by the generation alone. These tests are the single-lane half of that promise, on temporary stores. + +// ucRelationship is the relationship of the kit's accepted node I. +func ucRelationship(t *testing.T, k *judgeKit) string { + t.Helper() + var rid string + if err := k.s.DB.QueryRow("SELECT relationship_id FROM dag_acceptances WHERE plan_id = 'g' AND node_id = 'I' AND state = 'active'").Scan(&rid); err != nil { + t.Fatal(err) + } + return rid +} + +// ucOpenCorrection opens a correction generation of the relationship by hand, the shape generation-open +// leaves: the generations row and the relationship's live generation moved to it. +func ucOpenCorrection(t *testing.T, k *judgeKit, rid string) { + t.Helper() + if _, err := (®istry.Registry{Store: k.s}).OpenGeneration(context.Background(), rid, "correction-"+rid, "needs_changes_revision", sql.NullString{}); err != nil { + t.Fatalf("generation-open: %v", err) + } +} + +// ucJudge is one judgement of the kit's accepted node, with the refusal it is expected to answer. +func ucJudge(t *testing.T, k *judgeKit) (JudgeResult, error) { + t.Helper() + return k.sched.Judge(context.Background(), "g", "I", "parent", JudgeInput{}) +} + +// ucPointer is the generation the relationship stands on now. +func ucPointer(t *testing.T, k *judgeKit, rid string) int64 { + t.Helper() + var generation int64 + if err := k.s.DB.QueryRow("SELECT execution_generation FROM relationships WHERE relationship_id = ?", rid).Scan(&generation); err != nil { + t.Fatal(err) + } + return generation +} + +// Criterion c1 (generation 2): while the relationship's live generation is later than the generation its +// active acceptance stands on, the accepted head is not judged at all: the judgement is refused +// disposition_conflict, names the open generation, and writes no row. A generation that is recorded as +// the node's execution (dag-correct --manifest-digest) is the same state to this gate. +func TestTheSingleLaneRefusesAnAcceptedResultUnderCorrection(t *testing.T) { + t.Parallel() + for _, recorded := range []bool{false, true} { + name := "a correction generation that is open and bound" + if recorded { + name = "a correction generation recorded as the node's execution" + } + t.Run(name, func(t *testing.T) { + k := newJudgeKit(t) + rid := ucRelationship(t, k) + ucOpenCorrection(t, k, rid) + if recorded { + // what dag-correct --manifest-digest leaves: the execution row of the new generation + k.exec("INSERT INTO dag_node_executions (plan_id, node_id, relationship_id, execution_generation, manifest_digest, kind, managed_request_id)"+ + " VALUES ('g','I',?,2,?,'correction',?)", rid, dig("the corrected manifest"), "request-"+rid) + } + res, err := ucJudge(t, k) + if err == nil { + t.Fatalf("the judgement of an accepted result under correction was accepted: %+v", res) + } + if got := refusalReason(err); got != "disposition_conflict" { + t.Fatalf("reason = %s, want disposition_conflict (%v)", got, err) + } + for _, want := range []string{"under correction", "generation 2", "generation 1"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("the refusal does not name %q: %v", want, err) + } + } + if n := k.count("SELECT COUNT(*) FROM dag_merge_checks"); n != 0 { + t.Fatalf("the refused judgement wrote %d row(s)", n) + } + if res.Eligible() { + t.Fatal("the refused judgement reads eligible") + } + }) + } +} + +// Criterion c1 (generation 2): the gate is about the correction being open, not about the head. A +// withdrawn generation puts the relationship back on the generation its acceptance stands on, and the +// old head is judged again exactly as before. +func TestTheSingleLaneJudgesAgainAfterTheCorrectionIsWithdrawn(t *testing.T) { + t.Parallel() + k := newJudgeKit(t) + rid := ucRelationship(t, k) + ucOpenCorrection(t, k, rid) + if _, err := ucJudge(t, k); err == nil { + t.Fatal("the judgement of an accepted result under correction was accepted") + } + if _, err := k.sched.WithdrawGeneration(context.Background(), "g", "I", "parent", + WithdrawInput{Relationship: rid, Generation: 2, Reason: "the correction is not needed"}); err != nil { + t.Fatalf("dag-generation-withdraw: %v", err) + } + if got := ucPointer(t, k, rid); got != 1 { + t.Fatalf("the relationship stands on generation %d after the withdrawal, want 1", got) + } + res, err := ucJudge(t, k) + if err != nil { + t.Fatalf("the judgement after the withdrawal: %v", err) + } + if !res.Eligible() { + t.Fatalf("the judgement after the withdrawal = %+v, want eligible", res) + } +} + +// Criterion c1 (generation 2): the accepted result is accepted over (the corrected head of the new +// generation replaces the acceptance), and the node is a candidate again. The gate reads the stand +// generation the acceptance moved to, so nothing about the correction is left behind. +func TestTheSingleLaneJudgesAgainAfterTheCorrectionIsAcceptedOver(t *testing.T) { + t.Parallel() + k := newJudgeKit(t) + rid := ucRelationship(t, k) + ucOpenCorrection(t, k, rid) + // dag-accept --supersedes of the corrected result: the acceptance moves to generation 2, the same + // generation the relationship now stands on, so nothing is under correction any more + k.exec("UPDATE dag_acceptances SET execution_generation = 2 WHERE relationship_id = ? AND state = 'active'", rid) + res, err := ucJudge(t, k) + if err != nil { + t.Fatalf("the judgement of the corrected result: %v", err) + } + if !res.Eligible() { + t.Fatalf("the judgement of the corrected result = %+v, want eligible", res) + } +} + +// Criterion c1 (generation 2): the single lane's other half, dag-merge-request, refuses too. It makes +// its own judgement, so no turn is requested and the lane never carries the head being repaired. +func TestRequestMergeTurnRefusesAnAcceptedResultUnderCorrection(t *testing.T) { + t.Parallel() + k := newJudgeKit(t) + rid := ucRelationship(t, k) + ucOpenCorrection(t, k, rid) + _, turn, err := k.sched.RequestMergeTurn(context.Background(), "g", "I", "parent", MergeRequestInput{Host: "host-a"}) + if err == nil { + t.Fatalf("a merge turn was requested for an accepted result under correction: %v", turn) + } + if got := refusalReason(err); got != "disposition_conflict" { + t.Fatalf("reason = %s, want disposition_conflict (%v)", got, err) + } + if !strings.Contains(err.Error(), "under correction") { + t.Fatalf("the refusal does not say why: %v", err) + } + if n := k.count("SELECT COUNT(*) FROM merge_turns"); n != 0 { + t.Fatalf("the refused request wrote %d merge turn(s)", n) + } +} diff --git a/internal/relay/mergeturn/train.go b/internal/relay/mergeturn/train.go index 38317cc976..a64da074fd 100644 --- a/internal/relay/mergeturn/train.go +++ b/internal/relay/mergeturn/train.go @@ -235,24 +235,26 @@ func (s *Service) Open(ctx context.Context, turn, actor, base string, members [] } // every member (the leader included) must stand on an active acceptance whose stand head is the // head its pull request shows and its turn holds (Blocking 1): in the single lane that tie is - // dag-accept, which the bundle path would otherwise never pass through - standFor := func(relationship, memberHead string) (acceptance.Active, string, string, string, error) { - active, found, err := acceptance.ActiveForRelationship(ctx, s.Store.Querier(ctx), relationship) + // dag-accept, which the bundle path would otherwise never pass through. The reading carries the + // correction gate too (CRW-906): a member whose accepted result is being corrected in an open + // generation is not a candidate this bundle may carry. + standFor := func(relationship, memberHead string) (acceptance.Active, string, string, string, bool, int64, int64, error) { + active, live, found, err := acceptance.ActiveWithLiveGeneration(ctx, s.Store.Querier(ctx), relationship) if err != nil { - return acceptance.Active{}, "", "", "", trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) + return acceptance.Active{}, "", "", "", false, 0, 0, trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) } if !found { - return acceptance.Active{}, "", "", "", nil + return acceptance.Active{}, "", "", "", false, live, 0, nil } stand, err := acceptance.StandOf(ctx, s.Store.Querier(ctx), active.AcceptanceID, relationship, active.Generation, active.EventID, active.RevisionHash, active.HeadSHA) if err != nil { - return acceptance.Active{}, "", "", "", trainUnreadable("what acceptance %s stands on was not read: %v", pyvalue.StrRepr(active.AcceptanceID), err) + return acceptance.Active{}, "", "", "", false, live, 0, trainUnreadable("what acceptance %s stands on was not read: %v", pyvalue.StrRepr(active.AcceptanceID), err) } rulingHead, rulingSource, err := acceptance.RulingHead(ctx, s.Store.Querier(ctx), active.EventID, active.HeadSHA) if err != nil { - return acceptance.Active{}, "", "", "", trainUnreadable("the ruling head of event %s was not read: %v", pyvalue.StrRepr(active.EventID), err) + return acceptance.Active{}, "", "", "", false, live, 0, trainUnreadable("the ruling head of event %s was not read: %v", pyvalue.StrRepr(active.EventID), err) } - return active, stand.Head, rulingHead, rulingSource, nil + return active, stand.Head, rulingHead, rulingSource, live > stand.Generation, live, stand.Generation, nil } for _, pr := range members { pull, err := forge.PullRequest(ctx, early.Repository, pr) @@ -272,11 +274,11 @@ func (s *Service) Open(ctx context.Context, turn, actor, base string, members [] if !SameCommit(pull.HeadSHA, early.CandidateHead) { return nil, trainConflict("the leader's pull request %d reads head %s and its turn holds %s; a member's head is not refreshed by the bundle", pr, pyvalue.StrRepr(pull.HeadSHA), pyvalue.StrRepr(early.CandidateHead)) } - active, standHead, rulingHead, rulingSource, err := standFor(early.RelationshipID.String, early.CandidateHead) + active, standHead, rulingHead, rulingSource, under, live, standGeneration, err := standFor(early.RelationshipID.String, early.CandidateHead) if err != nil { return nil, err } - if err := trainStandRefusal(pr, early.RelationshipID.String, early.CandidateHead, active, standHead); err != nil { + if err := trainStandRefusal(pr, early.RelationshipID.String, early.CandidateHead, active, standHead, under, live, standGeneration); err != nil { return nil, err } expectations = append(expectations, TrainMemberExpectation{TurnID: turn, PRNumber: pr, RelationshipID: early.RelationshipID.String, AcceptedHead: early.CandidateHead, RuledEventID: active.EventID, RulingHead: rulingHead, RulingHeadSource: rulingSource}) @@ -295,11 +297,11 @@ func (s *Service) Open(ctx context.Context, turn, actor, base string, members [] if !member.RelationshipID.Valid || member.RelationshipID.String == "" { return nil, trainConflict("pull request %d's turn %s records no relationship, so its landing cannot be recorded", pr, pyvalue.StrRepr(member.TurnID)) } - active, standHead, rulingHead, rulingSource, err := standFor(member.RelationshipID.String, member.CandidateHead) + active, standHead, rulingHead, rulingSource, under, live, standGeneration, err := standFor(member.RelationshipID.String, member.CandidateHead) if err != nil { return nil, err } - if err := trainStandRefusal(pr, member.RelationshipID.String, member.CandidateHead, active, standHead); err != nil { + if err := trainStandRefusal(pr, member.RelationshipID.String, member.CandidateHead, active, standHead, under, live, standGeneration); err != nil { return nil, err } expectations = append(expectations, TrainMemberExpectation{TurnID: member.TurnID, PRNumber: pr, RelationshipID: member.RelationshipID.String, AcceptedHead: member.CandidateHead, RuledEventID: active.EventID, RulingHead: rulingHead, RulingHeadSource: rulingSource}) @@ -359,14 +361,21 @@ func waitingTurnFor(turns []store.MergeTurnsRow, pr int64) (store.MergeTurnsRow, } // trainStandRefusal is Blocking 1's gate: the member must have an active acceptance whose stand head -// is the head its pull request shows and its turn holds. -func trainStandRefusal(pr int64, relationship, memberHead string, active acceptance.Active, standHead string) error { +// is the head its pull request shows and its turn holds, and no correction generation may be open over +// the accepted result (CRW-906). A defect found in an accepted result before it lands is corrected in a +// later generation; until that generation's result is accepted with dag-accept --supersedes, or the +// generation is withdrawn, the old head is not a candidate a bundle may carry or land. The refusal is +// the existing disposition_conflict and it names the open generation: no new refusal name, no column. +func trainStandRefusal(pr int64, relationship, memberHead string, active acceptance.Active, standHead string, underCorrection bool, liveGeneration, standGeneration int64) error { if active.AcceptanceID == "" { return trainConflict("pull request %d's relationship %s has no active acceptance, so the member is not a verified, accepted candidate this bundle may carry; run dag-accept on its head first", pr, pyvalue.StrRepr(relationship)) } if !SameCommit(standHead, memberHead) { return trainConflict("pull request %d stands on %s and its acceptance %s stands on %s, so the member's head is not the head the ruling covers", pr, pyvalue.StrRepr(memberHead), pyvalue.StrRepr(active.AcceptanceID), pyvalue.StrRepr(standHead)) } + if underCorrection { + return trainConflict("pull request %d's relationship %s is under correction: generation %d is open over the accepted result, which stands on generation %d, so the member is not a candidate a bundle may carry or land; accept the corrected result with dag-accept --supersedes, or withdraw the generation", pr, pyvalue.StrRepr(relationship), liveGeneration, standGeneration) + } return nil } @@ -587,7 +596,7 @@ func (s *Service) Verify(ctx context.Context, train, actor, bundlePR, head, run, func (s *Service) trainExpectations(ctx context.Context, members []store.MergeTrainMemberRow) ([]TrainMemberExpectation, error) { out := make([]TrainMemberExpectation, 0, len(members)) for _, m := range members { - active, found, err := acceptance.ActiveForRelationship(ctx, s.Store.Querier(ctx), m.RelationshipID) + active, live, found, err := acceptance.ActiveWithLiveGeneration(ctx, s.Store.Querier(ctx), m.RelationshipID) if err != nil { return nil, trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(m.RelationshipID), err) } @@ -598,7 +607,7 @@ func (s *Service) trainExpectations(ctx context.Context, members []store.MergeTr if err != nil { return nil, trainUnreadable("what acceptance %s stands on was not read: %v", pyvalue.StrRepr(active.AcceptanceID), err) } - if err := trainStandRefusal(m.PRNumber, m.RelationshipID, m.MemberHead, active, stand.Head); err != nil { + if err := trainStandRefusal(m.PRNumber, m.RelationshipID, m.MemberHead, active, stand.Head, live > stand.Generation, live, stand.Generation); err != nil { return nil, err } rulingHead, rulingSource, err := acceptance.RulingHead(ctx, s.Store.Querier(ctx), active.EventID, active.HeadSHA) diff --git a/internal/relay/mergeturn/train_undercorrection_test.go b/internal/relay/mergeturn/train_undercorrection_test.go new file mode 100644 index 0000000000..34156300f8 --- /dev/null +++ b/internal/relay/mergeturn/train_undercorrection_test.go @@ -0,0 +1,101 @@ +package mergeturn + +import ( + "fmt" + "strings" + "testing" +) + +// CRW-906 generation 2: the bundle half of the issue's decision 2. A node whose accepted result is being +// corrected is not merged on its old head. The bundle gate already refuses a member whose pull request +// head differs from the accepted head, and that guard holds only once the child has pushed: between +// opening (or recording) the correction generation and that first push the pull request still shows the +// accepted head H, so the train must refuse by the generation alone. These tests drive open, verify and +// land, each writing nothing. + +// ucCorrection opens a correction generation of a relationship by hand, the shape generation-open leaves: +// the generations row and the relationship's live execution generation moved to it. The turn fixture +// seeds an acceptance without a relationships row, so the row is written here too. +func (w *tr) ucCorrection(relationship string) { + w.t.Helper() + allowed := fmt.Sprintf("[%q]", trLeader) + w.exec("INSERT INTO relationships (relationship_id, issue_key, status, parent_task_id, parent_host_id, child_task_id, child_host_id, execution_generation, artifact_roots, allowed_recipients, created_at, updated_at)"+ + " VALUES (?, 'ISS-1', 'active', ?, 'host-a', 'child-1', 'host-c', 2, '[]', ?, '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')", + relationship, trLeader, allowed) + w.exec("INSERT INTO generations (relationship_id, execution_generation, dispatch_request_id, anchor_state, dispatch_turn_id, reason, opened_at, bound_at)"+ + " VALUES (?, 2, ?, 'bound', 'turn-dispatch-2', 'needs_changes_revision', '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')", + relationship, "correction-"+relationship) +} + +// TestTrainRefusesAMemberUnderCorrection: open, verify and land each refuse a member whose accepted +// result has an open correction generation, name it, and write nothing. +func TestTrainRefusesAMemberUnderCorrection(t *testing.T) { + t.Run("open refuses the member and writes no train", func(t *testing.T) { + w := newTr(t) + leader, members := w.threeMembers() + w.ucCorrection("rel-task-m2") + _, err := w.open(leader, trLeader, "base-0", members...) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a member under correction: %v", err) + } + for _, want := range []string{"under correction", "generation 2", "generation 1"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("the refusal does not name %q: %v", want, err) + } + } + if n := w.trainEvents(); n != 0 { + t.Fatalf("a refused open wrote %d event(s)", n) + } + }) + t.Run("verify refuses after the correction opened and writes no verified event", func(t *testing.T) { + w := newTr(t) + train := w.openedTrain() + w.pr(900, "head-bundle", TrainLaneLabel) + w.forge.runs["run-1"] = runFor("head-bundle") + w.ucCorrection("rel-task-m3") + if _, err := w.m.Verify(w.ctx, train, trLeader, "900", "head-bundle", "run-1", "/checkout", w.forge, w.proof); err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a member under correction at verify: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'verified'"); n != 0 { + t.Fatalf("a refused verify wrote %d verified event(s)", n) + } + }) + t.Run("land refuses and lands no turn", func(t *testing.T) { + w := newTr(t) + train := w.verifiedTrain() + w.ucCorrection("rel-task-m2") + w.tip.set(trRepo, trBase, "merge-1") + w.forge.commits["merge-1"] = TrainCommit{SHA: "merge-1", Parents: []string{"base-0", "head-bundle"}, Tree: "tree-bundle"} + _, err := w.m.TrainLand(w.ctx, train, trLeader, "merge-1", "", w.tip, w.forge) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a member under correction at land: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'landed'"); n != 0 { + t.Fatalf("a refused land wrote %d landed event(s)", n) + } + if n := w.count("SELECT count(*) FROM merge_turns WHERE state = 'landed'"); n != 0 { + t.Fatalf("a refused land landed %d turn(s)", n) + } + }) +} + +// TestTrainCarriesTheMemberAgainAfterTheCorrectionCloses: the gate is about the correction being open, +// not about the head. A withdrawn generation puts the relationship back on the generation the acceptance +// stands on, and the member rides again on the same head. +func TestTrainCarriesTheMemberAgainAfterTheCorrectionCloses(t *testing.T) { + w := newTr(t) + leader, members := w.threeMembers() + w.ucCorrection("rel-task-m2") + if _, err := w.open(leader, trLeader, "base-0", members...); err == nil { + t.Fatal("a member under correction was carried") + } + // the withdrawal puts the relationship back on the generation its acceptance stands on + w.exec("UPDATE relationships SET execution_generation = 1 WHERE relationship_id = 'rel-task-m2'") + answer, err := w.open(leader, trLeader, "base-0", members...) + if err != nil { + t.Fatalf("the bundle after the correction was withdrawn: %v", err) + } + if answer["train"] == nil { + t.Fatalf("the bundle after the correction was withdrawn = %v, want a train", answer) + } +} From 5921d1a355f887f447511f008e532bdc30e370dc Mon Sep 17 00:00:00 2001 From: codex Date: Wed, 7 Oct 2026 19:49:48 +0900 Subject: [PATCH 05/28] CRW-906: hold the accepted head back in the lane's own check and land, and re-read the members inside the train's writes The pre-merge evaluation of this head found two more places the issue's decision 2 promise was still open, both about when the correction state is read rather than about the gate itself. A merge turn can have been granted before the correction generation was opened, and the lane's own commands are what actually carry the head to the base: merge-turn-check moves the turn to merging and merge-turn-land records the merge, and neither read the correction state. Both now ask acceptance.UnderCorrection inside the transaction that writes, so a generation opened while the check was reading the forge is seen; the refusal is the existing disposition_conflict and names the open generation, the check writes no current row and leaves the turn holding, and the land records no landing and leaves the turn merging. The bundle's open and verify read each member's acceptance before the transaction that writes, and a generation opened in that gap left the reading stale: open rechecked only the turns and the membership, and verify only the train state. Both now re-read every member's correction state on the transaction's own snapshot, so an under-correction member is neither opened nor verified. TrainLand already rechecked inside its transaction. The two refusals and the re-read live in one new file (mergeturn/undercorrection.go), so the lane and the bundle word the same answer. No refusal name, column, schema, CLI flag or specs.json entry is added. --- internal/relay/mergeturn/check.go | 10 ++ internal/relay/mergeturn/outcome.go | 11 ++ internal/relay/mergeturn/train.go | 18 ++- .../mergeturn/train_undercorrection_test.go | 109 ++++++++++++++++++ internal/relay/mergeturn/undercorrection.go | 67 +++++++++++ 5 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 internal/relay/mergeturn/undercorrection.go diff --git a/internal/relay/mergeturn/check.go b/internal/relay/mergeturn/check.go index 8894e23dee..df9b0bce06 100644 --- a/internal/relay/mergeturn/check.go +++ b/internal/relay/mergeturn/check.go @@ -117,6 +117,16 @@ func (s *Service) Check(ctx context.Context, turn, actor, head, base string, che } else if guard != nil { refusal = guard } + // CRW-906: an accepted result whose correction generation is open is not carried to the base by + // the lane either. A turn can have been granted before the correction was opened, and this check + // is what moves it to merging, so the state is read here, inside the transaction that writes. + if refusal == nil { + if guard, e := underCorrectionRefusal(tx, s.Store.Querier(tx), row); e != nil { + return e + } else if guard != nil { + refusal = guard + } + } if refusal == nil { switch { case row.HolderTaskID != actor: diff --git a/internal/relay/mergeturn/outcome.go b/internal/relay/mergeturn/outcome.go index 9f4bc57796..cfaddea5da 100644 --- a/internal/relay/mergeturn/outcome.go +++ b/internal/relay/mergeturn/outcome.go @@ -88,6 +88,17 @@ func (s *Service) Land(ctx context.Context, turn, actor, landed, stated, evidenc } else if guard != nil { refusal = guard } + // CRW-906: a correction generation opened over the accepted result after the currency check (or + // before it) refuses the landing, on this transaction's snapshot: the merge already happened on + // the forge, but the relay records no landing of the head that is being repaired, and the turn + // stays merging for the holder to read again. + if refusal == nil { + if guard, e := underCorrectionRefusal(tx, s.Store.Querier(tx), r); e != nil { + return e + } else if guard != nil { + refusal = guard + } + } if refusal == nil { switch { case r.HolderTaskID != actor: diff --git a/internal/relay/mergeturn/train.go b/internal/relay/mergeturn/train.go index a64da074fd..e330b77710 100644 --- a/internal/relay/mergeturn/train.go +++ b/internal/relay/mergeturn/train.go @@ -322,6 +322,14 @@ func (s *Service) Open(ctx context.Context, turn, actor, base string, members [] if row.HolderTaskID != actor || row.State != Holding { return trainConflict("turn %s changed while the bundle was being read, so it is not the holding turn this open began on; call again", pyvalue.StrRepr(turn)) } + // CRW-906: every member's correction state is read again on this transaction's snapshot. The + // reads above happen before it, and a generation opened in that gap would otherwise be recorded + // as an opened train: nothing is written for a member whose accepted result is under correction. + for _, m := range expectations { + if e := trainMemberCorrectionRefusal(tx, s.Store.Querier(tx), m.PRNumber, m.RelationshipID, m.AcceptedHead); e != nil { + return e + } + } // a train id names one opening, not the candidate: a turn that abandons a train and reopens // one for the same head gets a fresh id rather than colliding with the abandoned record for suffix := 2; ; suffix++ { @@ -374,7 +382,7 @@ func trainStandRefusal(pr int64, relationship, memberHead string, active accepta return trainConflict("pull request %d stands on %s and its acceptance %s stands on %s, so the member's head is not the head the ruling covers", pr, pyvalue.StrRepr(memberHead), pyvalue.StrRepr(active.AcceptanceID), pyvalue.StrRepr(standHead)) } if underCorrection { - return trainConflict("pull request %d's relationship %s is under correction: generation %d is open over the accepted result, which stands on generation %d, so the member is not a candidate a bundle may carry or land; accept the corrected result with dag-accept --supersedes, or withdraw the generation", pr, pyvalue.StrRepr(relationship), liveGeneration, standGeneration) + return trainUnderCorrectionRefusal(pr, relationship, liveGeneration, standGeneration) } return nil } @@ -581,6 +589,14 @@ func (s *Service) Verify(ctx context.Context, train, actor, bundlePR, head, run, if !found || (current != store.MergeTrainOpened && current != store.MergeTrainVerified) { return trainConflict("train %s changed while the bundle was being read, so it is no longer a train a verify may advance; call again", pyvalue.StrRepr(train)) } + // CRW-906: each member's correction state is read again on this transaction's snapshot. The + // expectations above were read before the run and the chain were proved, and a generation opened + // in that gap would otherwise be recorded as verified: nothing is written for such a member. + for _, m := range expected { + if e := trainMemberCorrectionRefusal(tx, s.Store.Querier(tx), m.PRNumber, m.RelationshipID, m.AcceptedHead); e != nil { + return e + } + } return store.RecordMergeTrainEvent(tx, s.Store, store.MergeTrainEventRow{TrainID: train, Seq: seq, Kind: store.MergeTrainVerified, Actor: actor, DetailJSON: pythonJSON(detail), RecordedAt: at}) }) if err != nil { diff --git a/internal/relay/mergeturn/train_undercorrection_test.go b/internal/relay/mergeturn/train_undercorrection_test.go index 34156300f8..39d6bb3386 100644 --- a/internal/relay/mergeturn/train_undercorrection_test.go +++ b/internal/relay/mergeturn/train_undercorrection_test.go @@ -1,6 +1,7 @@ package mergeturn import ( + "database/sql" "fmt" "strings" "testing" @@ -99,3 +100,111 @@ func TestTrainCarriesTheMemberAgainAfterTheCorrectionCloses(t *testing.T) { t.Fatalf("the bundle after the correction was withdrawn = %v, want a train", answer) } } + +// ucLaneReport is the work report the lane's head comparison reads: the assignment the turn names has a +// report naming the head, so the check has something to compare the restated head with. +func (w *fx) ucLaneReport(relationship, head string) { + w.t.Helper() + w.exec("INSERT INTO work_reports (event_id, submission_no, relationship_id, execution_generation, revision_hash, repository, head_sha, cxc_status, cxc_reason, contract_version, summary, next_action, recorded_at)"+ + " VALUES (?, 1, ?, 1, ?, ?, ?, 'DONE', 'proved', 'v1', 'done', 'merge', '2023-11-14T22:13:20.000000+00:00')", + "ev-report-"+relationship, relationship, "rev-"+relationship, fxRepo, head) +} + +// ucLaneRelationship writes the relationship the turn names at the generation given, and (when the +// generation is 2) the correction generation opened over the accepted result. The fx fixture seeds no +// relationship row, so one is written for the turn the test holds. +func (w *fx) ucLaneRelationship(relationship string, generation int64) { + w.t.Helper() + w.exec("INSERT INTO relationships (relationship_id, issue_key, status, parent_task_id, parent_host_id, child_task_id, child_host_id, execution_generation, artifact_roots, allowed_recipients, created_at, updated_at)"+ + " VALUES (?, 'ISS-1', 'active', ?, 'host-a', 'child-1', 'host-c', ?, '[]', ?, '2023-11-14T22:13:20.000000+00:00', '2023-11-14T22:13:20.000000+00:00')", + relationship, alpha.TaskID, generation, fmt.Sprintf("[%q]", alpha.TaskID)) + if generation > 1 { + w.exec("INSERT INTO generations (relationship_id, execution_generation, dispatch_request_id, anchor_state, dispatch_turn_id, reason, opened_at, bound_at)"+ + " VALUES (?, ?, ?, 'bound', 'turn-dispatch-2', 'needs_changes_revision', '2023-11-14T22:13:20.000000+00:00', '2023-11-14T22:13:20.000000+00:00')", + relationship, generation, "correction-"+relationship) + // the acceptance the correction is opened over: the gate compares the live generation with the + // generation this acceptance stands on + w.exec("INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES (?, 'plan-x', 'node-1', ?, ?, 1, ?, ?, 'crit-1', 'verified', 'head-a', ?, 1, 'bound', 'turn-1', '{}', ?, 0, '2023-11-14T22:13:20.000000+00:00', 'active')", + "acc-"+relationship, "manifest-"+relationship, relationship, "ev-"+relationship, "rev-"+relationship, fxRepo, alpha.TaskID) + } +} + +// ucHeldOn is a held, acknowledged turn on a head whose claim records a relationship. +func (w *fx) ucHeldOn(relationship, head string) string { + w.t.Helper() + options := ClaimOptions{Relationship: sql.NullString{String: relationship, Valid: true}} + turn := w.must(w.m.Request(w.ctx, fxRepo, fxBase, fxA, alpha.TaskID, alpha.HostID, head, true, options))["turnId"].(string) + w.answer(turn, alpha.TaskID) + return turn +} + +// CRW-906 generation 2, the lane's own half: a merge turn can have been granted before the correction was +// opened, so the check that moves it to merging and the land that records the merge read the correction +// state themselves, inside the transaction that writes. Both refuse disposition_conflict naming the open +// generation, the check writes no current row and the turn stays holding, and the land records no landing +// and leaves the turn merging. +func TestTheLaneRefusesATurnWhoseAcceptedResultIsUnderCorrection(t *testing.T) { + t.Run("the check refuses and the turn stays holding", func(t *testing.T) { + w := newFx(t) + turn := w.ucHeldOn("rel-lane", "head-a") + w.ucLaneReport("rel-lane", "head-a") + w.ucLaneRelationship("rel-lane", 2) + _, err := w.check(turn, "head-a", "base-0", "") + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("check under correction: %v", err) + } + for _, want := range []string{"under correction", "generation 2", "generation 1"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("the refusal does not name %q: %v", want, err) + } + } + if got := w.must(w.m.Turn(w.ctx, turn))["state"]; got != "holding" { + t.Fatalf("the turn is %v after the refused check, want holding", got) + } + if n := w.ucCount("SELECT count(*) FROM merge_turn_checks WHERE result = 'current'"); n != 0 { + t.Fatalf("the refused check wrote %d current row(s)", n) + } + }) + t.Run("the land refuses and records no landing", func(t *testing.T) { + w := newFx(t) + turn := w.ucHeldOn("rel-lane", "head-a") + w.ucLaneReport("rel-lane", "head-a") + w.must(w.check(turn, "head-a", "base-0", "")) + w.merged("merge-1") + w.ucLaneRelationship("rel-lane", 2) + _, err := w.land(turn, "merge-1", "", "") + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("land under correction: %v", err) + } + if !strings.Contains(err.Error(), "under correction") { + t.Fatalf("the refusal does not say why: %v", err) + } + if got := w.must(w.m.Turn(w.ctx, turn))["state"]; got != "merging" { + t.Fatalf("the turn is %v after the refused land, want merging", got) + } + if n := w.ucCount("SELECT count(*) FROM merge_turns WHERE state = 'landed'"); n != 0 { + t.Fatalf("the refused land landed %d turn(s)", n) + } + }) + t.Run("a turn whose relationship is not under correction is unchanged", func(t *testing.T) { + w := newFx(t) + turn := w.ucHeldOn("rel-lane", "head-a") + // the relationship stands on the generation its acceptance stands on: no correction is open + w.ucLaneReport("rel-lane", "head-a") + w.ucLaneRelationship("rel-lane", 1) + if _, err := w.check(turn, "head-a", "base-0", ""); err != nil { + t.Fatalf("a turn with no correction open was refused: %v", err) + } + }) +} + +// ucCount is the number of rows a query answers, for the writes a refusal must not leave behind. +func (w *fx) ucCount(query string, args ...any) int64 { + w.t.Helper() + var n int64 + if err := w.s.DB.QueryRowContext(w.ctx, query, args...).Scan(&n); err != nil { + w.t.Fatal(err) + } + return n +} diff --git a/internal/relay/mergeturn/undercorrection.go b/internal/relay/mergeturn/undercorrection.go new file mode 100644 index 0000000000..cb25b2319a --- /dev/null +++ b/internal/relay/mergeturn/undercorrection.go @@ -0,0 +1,67 @@ +package mergeturn + +import ( + "context" + "strconv" + + "github.com/thisisjun786/codex-relay-workflow/internal/contract" + "github.com/thisisjun786/codex-relay-workflow/internal/pyvalue" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/acceptance" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/registry" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" +) + +// underCorrectionRefusal is CRW-906's gate on the single lane's own operations. A defect found in an +// accepted result before it lands is corrected in a later generation of the same relationship, and +// until that generation's result is accepted over (dag-accept --supersedes) or the generation is +// withdrawn, the accepted head is the result being repaired: it must not be merged. +// +// The DAG judgement and the bundle gate refuse it already, but a merge turn can have been granted +// before the correction was opened, and the lane's own check and land are what actually carry the +// head to the base. They read the relationship's live execution generation against the generation its +// active acceptance stands on here, inside the transaction that writes, so a generation opened while +// the check was reading the forge is seen and nothing is written for it. +// +// The refusal is the existing disposition_conflict and it names the open generation: no new refusal +// name, no column, no schema change. nil means the turn is not under correction (or names no +// relationship, which the lane's own rules already cover). +func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeTurnsRow) (*registry.CoordinationRefusal, error) { + if !r.RelationshipID.Valid || r.RelationshipID.String == "" { + return nil, nil + } + under, live, stand, err := acceptance.UnderCorrection(ctx, q, r.RelationshipID.String) + if err != nil { + return nil, err + } + if !under { + return nil, nil + } + return coordination(r, contract.RefusalDispositionConflict, + "the accepted result of this turn's relationship "+pyvalue.StrRepr(r.RelationshipID.String)+ + " is under correction: generation "+strconv.FormatInt(live, 10)+" is open over the acceptance, which stands on generation "+ + strconv.FormatInt(stand, 10)+", so the head this turn holds is the result being repaired and is not merged. "+ + "Accept the corrected result with dag-accept --supersedes, or withdraw the generation, and request the turn again", + r.CandidateHead, r.HolderTaskID), nil +} + +// trainUnderCorrectionRefusal is the bundle member's refusal: the member's relationship has a correction +// generation open over its accepted result, so the head the bundle would carry is the result being +// repaired. It is the existing disposition_conflict and it names the open generation. +func trainUnderCorrectionRefusal(pr int64, relationship string, liveGeneration, standGeneration int64) error { + return trainConflict("pull request %d's relationship %s is under correction: generation %d is open over the accepted result, which stands on generation %d, so the member is not a candidate a bundle may carry or land; accept the corrected result with dag-accept --supersedes, or withdraw the generation", pr, pyvalue.StrRepr(relationship), liveGeneration, standGeneration) +} + +// trainMemberCorrectionRefusal re-reads one member's correction state on the caller's querier and refuses +// the member when a correction generation is open over its accepted result (CRW-906). Open and verify +// read each member's acceptance before the transaction that writes, and a generation opened in that gap +// leaves the earlier reading stale; this is the same question asked again inside the write. +func trainMemberCorrectionRefusal(ctx context.Context, q store.Querier, pr int64, relationship, memberHead string) error { + under, live, stand, err := acceptance.UnderCorrection(ctx, q, relationship) + if err != nil { + return trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) + } + if !under { + return nil + } + return trainUnderCorrectionRefusal(pr, relationship, live, stand) +} From a547bf1a2fd4e1a14347f2752a227c7e3658bb28 Mon Sep 17 00:00:00 2001 From: crw-934 Date: Wed, 7 Oct 2026 21:42:34 +0900 Subject: [PATCH 06/28] CRW-906: let the shipped relationship contract carry the correction reason, and resolve a lane turn's relationship from its pull request Two defects found in the generation-1 head, both inside this issue's promise. The record carried a reason the contract refused. The correction of an accepted, still-current result is recorded under accepted_result_correction, and Generation.contract emits it while Relationship.ContractRecord includes every generation, so a relationship opened by the documented route serialized a generations[].reason the shipped contract/schema/relationship.json did not allow. The enum gains that value; the 6 existing relationship cases were re-judged against the revised bytes with jsonschema 4.19.2 Draft7Validator and kept their verdicts, 6 new cases cover each reason the registry accepts, the empty reason, an unknown one and a mistyped one, and the schema pins in the store tests move with it. A turn that names no relationship bypassed the correction gate. The single lane lets a claim carry a forge pull request instead, and underCorrectionRefusal returned nil when the turn omitted RelationshipID, so the accepted head could still be carried and landed while its correction was open. The relationship is now resolved from the turn's pull request through the acceptance's recorded forge identity (dag_acceptance_forge), so both routes meet the same gate; a turn whose pull request has no accepted result is left to the lane's own rules. --- contract/schema/SOURCE.md | 13 +- contract/schema/relationship.json | 1 + .../mergeturn/train_undercorrection_test.go | 38 ++++++ internal/relay/mergeturn/undercorrection.go | 33 ++++- .../store/relationship_reason_schema_test.go | 114 ++++++++++++++++++ .../relay/store/schema_conformance_test.go | 2 +- internal/relay/store/schema_validator_test.go | 4 +- .../relationship-reason-verdicts.json.gz | Bin 0 -> 807 bytes 8 files changed, 196 insertions(+), 9 deletions(-) create mode 100644 internal/relay/store/relationship_reason_schema_test.go create mode 100644 internal/relay/store/testdata/fixtures/relationship-reason-verdicts.json.gz diff --git a/contract/schema/SOURCE.md b/contract/schema/SOURCE.md index 39123babdc..713467f761 100644 --- a/contract/schema/SOURCE.md +++ b/contract/schema/SOURCE.md @@ -9,7 +9,7 @@ with this note until todo 44 removed its source; the copies here are the ones th | acknowledgement.json | 193c2a1dbdb6197f852aaa38c6b8b4e55ba804ffc66e7b2a73925365b133eb7c | | completion-receipt.json | fd6498fdca80c7e8f4d97e37ceec12935bf1afce10211826fdc232593b9fb0d5 | | delivery-attempt.json | ad856f98872952ffc2235acc12ccc5942bbd6e5cae0df1771f063dc74526d24b | -| relationship.json | c8ebaf4559fa1ac6df26d98c4214938caf78bd8c61659bce026da6a3595a4b90 | +| relationship.json | 6a994cad6400a07d3e2faaf2161a16818ac7784be651cdcb2d6c9b9439988cbd | | verification-verdict.json | 0b3f8f4b061cff2992fc60a7c1f45dec6f803116894735751c40df3a8d356af9 | Contract bundle revision c37d332e2daba95c9ef47adf00a82bc9c6a539ab62538f0ad857561e470d2989. @@ -32,3 +32,14 @@ on a `ready_for_review` receipt. Old receipts without it still validate. The 18 were re-judged against the revised bytes with jsonschema 4.19.2 Draft7Validator and kept their verdicts; 18 additional recorded cases cover the item stated, absent, on an execution-only receipt and malformed. This changes the stored JSON contract, not the SQLite schema. + +The relationship copy now allows one more `generations[].reason`: `accepted_result_correction`, +the reason a coordinator gives `generation-open` when it corrects a result that was accepted and is +still current (the relay's verdict writer refuses a second ruling on an accepted head, so the +generation is opened by hand and its reason is what notes the route). The enum previously allowed +only `initial_assignment` and `needs_changes_revision`, so a relationship opened by that route +serialized a reason the shipped contract refused. Old relationships without it still validate; a +generation with no reason (a returning tenure) still validates as null. The 6 existing relationship +cases were re-judged against the revised bytes with jsonschema 4.19.2 Draft7Validator and kept their +verdicts; 6 additional recorded cases cover each reason the registry accepts, the empty reason, and +an unknown and a mistyped one. This changes the stored JSON contract, not the SQLite schema. diff --git a/contract/schema/relationship.json b/contract/schema/relationship.json index cc6452275a..aa6764fe3a 100644 --- a/contract/schema/relationship.json +++ b/contract/schema/relationship.json @@ -95,6 +95,7 @@ "enum": [ "initial_assignment", "needs_changes_revision", + "accepted_result_correction", null ] } diff --git a/internal/relay/mergeturn/train_undercorrection_test.go b/internal/relay/mergeturn/train_undercorrection_test.go index 39d6bb3386..288c5f67ee 100644 --- a/internal/relay/mergeturn/train_undercorrection_test.go +++ b/internal/relay/mergeturn/train_undercorrection_test.go @@ -5,6 +5,9 @@ import ( "fmt" "strings" "testing" + + "github.com/thisisjun786/codex-relay-workflow/internal/contract" + "github.com/thisisjun786/codex-relay-workflow/internal/relay/store" ) // CRW-906 generation 2: the bundle half of the issue's decision 2. A node whose accepted result is being @@ -130,6 +133,41 @@ func (w *fx) ucLaneRelationship(relationship string, generation int64) { } } +// ucLaneForge is the acceptance's forge identity, written with the acceptance: the mapping from a pull +// request back to the relationship whose result it carries. +func (w *fx) ucLaneForge(relationship string, pr int64) { + w.t.Helper() + w.exec("INSERT INTO dag_acceptance_forge (acceptance_id, forge_repository, pr_number) VALUES (?, ?, ?)", "acc-"+relationship, fxRepo, pr) +} + +// CRW-906 generation 2: the lane's CLI lets a claim carry a forge pull request instead of a +// relationship, and that route must meet the same gate. The acceptance's forge identity is what maps +// the turn's pull request back to the relationship whose correction is open, so a PR-only turn is +// refused on the head the correction is repairing rather than exempted by the field it omits. +func TestTheLaneGateResolvesTheRelationshipFromThePullRequest(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + w.ucLaneForge("rel-lane", 7) + turn := store.MergeTurnsRow{TurnID: "mtn-pr-only", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", PRNumber: sql.NullInt64{Int64: 7, Valid: true}, State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a PR-only turn whose accepted result is under correction was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "under correction") || !strings.Contains(refusal.Detail, "rel-lane") { + t.Fatalf("the refusal does not name the relationship and the reason: %s", refusal.Detail) + } + // a turn whose pull request has no accepted result is left to the lane's own rules + other := turn + other.PRNumber = sql.NullInt64{Int64: 99, Valid: true} + if refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), other); err != nil || refusal != nil { + t.Fatalf("a turn whose pull request has no accepted result = %+v %v, want no refusal", refusal, err) + } +} + // ucHeldOn is a held, acknowledged turn on a head whose claim records a relationship. func (w *fx) ucHeldOn(relationship, head string) string { w.t.Helper() diff --git a/internal/relay/mergeturn/undercorrection.go b/internal/relay/mergeturn/undercorrection.go index cb25b2319a..8c210d9ec0 100644 --- a/internal/relay/mergeturn/undercorrection.go +++ b/internal/relay/mergeturn/undercorrection.go @@ -2,6 +2,8 @@ package mergeturn import ( "context" + "database/sql" + "errors" "strconv" "github.com/thisisjun786/codex-relay-workflow/internal/contract" @@ -22,14 +24,33 @@ import ( // active acceptance stands on here, inside the transaction that writes, so a generation opened while // the check was reading the forge is seen and nothing is written for it. // +// A turn that names no relationship is not exempt: the CLI lets a claim carry a forge pull request +// instead, and a node under correction would otherwise be carried on the head the correction is +// repairing by the very route that omits the relationship. The acceptance's forge identity is the +// mapping from that pull request back to the relationship (dag_acceptance_forge, written with the +// acceptance), and it is read here so both routes meet the same gate. A turn whose pull request has no +// accepted result is left to the lane's own rules. +// // The refusal is the existing disposition_conflict and it names the open generation: no new refusal -// name, no column, no schema change. nil means the turn is not under correction (or names no -// relationship, which the lane's own rules already cover). +// name, no column, no schema change. func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeTurnsRow) (*registry.CoordinationRefusal, error) { - if !r.RelationshipID.Valid || r.RelationshipID.String == "" { - return nil, nil + relationship := r.RelationshipID.String + if !r.RelationshipID.Valid || relationship == "" { + if !r.PRNumber.Valid { + return nil, nil + } + var found string + if err := q.QueryRowContext(ctx, "SELECT a.relationship_id FROM dag_acceptance_forge f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id"+ + " WHERE f.forge_repository = ? AND f.pr_number = ? AND a.state = 'active' ORDER BY a.accepted_at DESC, a.acceptance_id DESC LIMIT 1", + r.Repository, r.PRNumber.Int64).Scan(&found); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + return nil, err + } + relationship = found } - under, live, stand, err := acceptance.UnderCorrection(ctx, q, r.RelationshipID.String) + under, live, stand, err := acceptance.UnderCorrection(ctx, q, relationship) if err != nil { return nil, err } @@ -37,7 +58,7 @@ func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeT return nil, nil } return coordination(r, contract.RefusalDispositionConflict, - "the accepted result of this turn's relationship "+pyvalue.StrRepr(r.RelationshipID.String)+ + "the accepted result of this turn's relationship "+pyvalue.StrRepr(relationship)+ " is under correction: generation "+strconv.FormatInt(live, 10)+" is open over the acceptance, which stands on generation "+ strconv.FormatInt(stand, 10)+", so the head this turn holds is the result being repaired and is not merged. "+ "Accept the corrected result with dag-accept --supersedes, or withdraw the generation, and request the turn again", diff --git a/internal/relay/store/relationship_reason_schema_test.go b/internal/relay/store/relationship_reason_schema_test.go new file mode 100644 index 0000000000..2340122538 --- /dev/null +++ b/internal/relay/store/relationship_reason_schema_test.go @@ -0,0 +1,114 @@ +package store + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" +) + +// relationshipReasonCases are the cases the generations[].reason enum of the shipped relationship +// contract is judged on. CRW-906 records a result that was accepted and is still current as a +// correction, and the reason its generation carries is accepted_result_correction: the reason is +// what notes that route, so the record every schema-conforming consumer reads has to carry a value +// the contract allows. The three reasons the registry's generation-open accepts, and the empty +// reason a returning tenure leaves, are valid; anything else is refused. +func relationshipReasonCases(t *testing.T) []schemaCase { + t.Helper() + base := `{"relationshipId":"rel-0123456789abcdef","parent":{"taskId":"01parent-task","hostId":"host-a","cwd":null},` + + `"child":{"taskId":"01child-task","hostId":"host-a","cwd":null},"issueKey":"CRW-906","status":"active",` + + `"createdAt":"2026-01-01T00:00:00Z","executionGeneration":2,"generations":[` + + `{"executionGeneration":1,"dispatchRequestId":"dispatch-1","anchorState":"bound","dispatchTurnId":"turn-dispatch-1","openedAt":"2026-01-01T00:00:00Z","boundAt":"2026-01-01T00:00:00Z","reason":null},` + + `{"executionGeneration":2,"dispatchRequestId":"dispatch-2","anchorState":"bound","dispatchTurnId":"turn-dispatch-2","openedAt":"2026-01-01T00:00:00Z","boundAt":"2026-01-01T00:00:00Z","reason":"initial_assignment"}` + + `],"authorizedScope":{"artifactRoots":["/synthetic/work"],"allowedRecipients":["01parent-task"]}}` + // The reason of the generation the record's current pointer names is the one generation-open wrote. + reason := func(value any) func(map[string]any) { + return func(record map[string]any) { + generations := record["generations"].([]any) + generations[1].(map[string]any)["reason"] = value + } + } + cases := []schemaCase{} + for _, c := range []struct { + label string + value any + valid bool + }{ + {"correction of an accepted current result", "accepted_result_correction", true}, + {"correction of a stale result", "needs_changes_revision", true}, + {"the assignment itself", "initial_assignment", true}, + {"a returning tenure, which leaves the reason empty", nil, true}, + {"a reason the relay never writes", "ruled_again", false}, + {"a reason of the wrong type", 2, false}, + } { + cases = append(cases, schemaCase{Schema: "relationship", Label: c.label, Valid: c.valid, Instance: mutated(t, base, reason(c.value))}) + } + return cases +} + +func TestRelationshipReasonSchemaAcceptsEveryRecordedReason(t *testing.T) { + cases := relationshipReasonCases(t) + var fixture struct { + SchemaSHA string `json:"schemaSha256"` + Verdicts []draft7Verdict `json:"verdicts"` + } + readFixture(t, "relationship-reason-verdicts.json", &fixture) + dir := filepath.Join(repositoryRoot(t), "contract", "schema") + if problems := schemaPinProblems(dir, map[string]string{"relationship": fixture.SchemaSHA}, map[string]int{"relationship": len(cases)}); len(problems) != 0 { + t.Fatal(problems) + } + if len(fixture.Verdicts) != len(cases) { + t.Fatalf("%d recorded judgments, want %d", len(fixture.Verdicts), len(cases)) + } + for i, c := range cases { + verdict := fixture.Verdicts[i] + if verdict.Schema != c.Schema || verdict.Label != c.Label || verdict.Instance != normalInstance(t, c.Instance) || (len(verdict.Errors) == 0) != c.Valid { + t.Fatalf("%s: independent Draft 7 judgment does not match case: %+v", c.Label, verdict) + } + } + // The schema the judgments were made against is the shipped one, and it is the one the other + // verdicts of this schema were judged against: one contract, one pin. + if fixture.SchemaSHA != schemaPins["relationship"] { + t.Fatalf("the reason cases were judged against relationship.json %s, and the schema pins hold %s", fixture.SchemaSHA, schemaPins["relationship"]) + } +} + +// TestRelationshipReasonSchemaRefusesAReasonTheContractDoesNotKnow pins the shape of the guard: the +// case above is only evidence while the schema still refuses a reason outside its enum. +func TestRelationshipReasonSchemaRefusesAReasonTheContractDoesNotKnow(t *testing.T) { + var schema struct { + Properties struct { + Generations struct { + Items struct { + Properties struct { + Reason struct { + Enum []any `json:"enum"` + } `json:"reason"` + } `json:"properties"` + } `json:"items"` + } `json:"generations"` + } `json:"properties"` + } + raw, err := os.ReadFile(filepath.Join(repositoryRoot(t), "contract", "schema", "relationship.json")) + if err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(raw, &schema); err != nil { + t.Fatal(err) + } + allowed := map[any]bool{} + for _, value := range schema.Properties.Generations.Items.Properties.Reason.Enum { + allowed[value] = true + } + if allowed["ruled_again"] { + t.Fatal("the contract now allows a reason the relay never writes; a case that judges one refused proves nothing") + } + for _, reason := range []string{"initial_assignment", "needs_changes_revision", "accepted_result_correction"} { + if !allowed[reason] { + t.Errorf("generation-open accepts %q and the record carries it, but the shipped relationship contract does not allow it: %v", reason, schema.Properties.Generations.Items.Properties.Reason.Enum) + } + } + if !allowed[nil] { + t.Error("a returning tenure leaves generations.reason empty, which the contract spells null") + } +} diff --git a/internal/relay/store/schema_conformance_test.go b/internal/relay/store/schema_conformance_test.go index e48e393610..80590c5069 100644 --- a/internal/relay/store/schema_conformance_test.go +++ b/internal/relay/store/schema_conformance_test.go @@ -368,7 +368,7 @@ func TestSchemaFiles_python_packaged_contract(t *testing.T) { // These digests follow the contract bundle (SOURCE.md). The Draft 7 verdicts keep their own // pins (schemaPins, schema_validator_test.go), which move only when the cases for a changed // schema are judged again, so a contract revision that edits a schema fails both. - expected := map[string]string{"acknowledgement.json": "193c2a1dbdb6197f852aaa38c6b8b4e55ba804ffc66e7b2a73925365b133eb7c", "completion-receipt.json": "fd6498fdca80c7e8f4d97e37ceec12935bf1afce10211826fdc232593b9fb0d5", "delivery-attempt.json": "ad856f98872952ffc2235acc12ccc5942bbd6e5cae0df1771f063dc74526d24b", "relationship.json": "c8ebaf4559fa1ac6df26d98c4214938caf78bd8c61659bce026da6a3595a4b90", "verification-verdict.json": "0b3f8f4b061cff2992fc60a7c1f45dec6f803116894735751c40df3a8d356af9"} + expected := map[string]string{"acknowledgement.json": "193c2a1dbdb6197f852aaa38c6b8b4e55ba804ffc66e7b2a73925365b133eb7c", "completion-receipt.json": "fd6498fdca80c7e8f4d97e37ceec12935bf1afce10211826fdc232593b9fb0d5", "delivery-attempt.json": "ad856f98872952ffc2235acc12ccc5942bbd6e5cae0df1771f063dc74526d24b", "relationship.json": "6a994cad6400a07d3e2faaf2161a16818ac7784be651cdcb2d6c9b9439988cbd", "verification-verdict.json": "0b3f8f4b061cff2992fc60a7c1f45dec6f803116894735751c40df3a8d356af9"} for name, want := range expected { data, err := os.ReadFile(filepath.Join(schema, name)) if err != nil { diff --git a/internal/relay/store/schema_validator_test.go b/internal/relay/store/schema_validator_test.go index a3a4ef3345..fc74ee5d3b 100644 --- a/internal/relay/store/schema_validator_test.go +++ b/internal/relay/store/schema_validator_test.go @@ -37,13 +37,15 @@ var recordSchemas = []string{"relationship", "completion-receipt", "delivery-att // were re-judged unchanged, with new attribution cases in runtime-attempt-verdicts.json.gz. // Completion-receipt gained the optional independentReview item the same way: its 18 original // cases were re-judged unchanged, with the new cases in independent-review-verdicts.json.gz. +// Relationship gained the correction reason CRW-906 records the accepted-current route under: its +// 6 original cases were re-judged unchanged, with the reason cases in relationship-reason-verdicts.json.gz. // Schemas hold only internal "#/definitions" references; a "$ref" to another file would escape // the pin, and that file would need a pin too. var schemaPins = map[string]string{ "acknowledgement": "193c2a1dbdb6197f852aaa38c6b8b4e55ba804ffc66e7b2a73925365b133eb7c", "completion-receipt": "fd6498fdca80c7e8f4d97e37ceec12935bf1afce10211826fdc232593b9fb0d5", "delivery-attempt": "ad856f98872952ffc2235acc12ccc5942bbd6e5cae0df1771f063dc74526d24b", - "relationship": "c8ebaf4559fa1ac6df26d98c4214938caf78bd8c61659bce026da6a3595a4b90", + "relationship": "6a994cad6400a07d3e2faaf2161a16818ac7784be651cdcb2d6c9b9439988cbd", "verification-verdict": "0b3f8f4b061cff2992fc60a7c1f45dec6f803116894735751c40df3a8d356af9", } diff --git a/internal/relay/store/testdata/fixtures/relationship-reason-verdicts.json.gz b/internal/relay/store/testdata/fixtures/relationship-reason-verdicts.json.gz new file mode 100644 index 0000000000000000000000000000000000000000..ef83afbca60532f2343a63732a17b0f731fc9153 GIT binary patch literal 807 zcmV+?1K9i@iwFn+00002|8iw)VRUJ4ZgXg9a4m9WVRLV8Ep}ycWNBk`b1rIgZ*BnX zm``uoFciS={S;O%TtFo$O`)15O*>4}q#fv_9RQ(+^B~q-JF^`aLwxtyF-fRew6&@> zr1?UU_s{RW`2A$EyC5XeG{<|m%OSZ=qgxWCFd7XgWa*&ahyG1=g~<|NnZ#)faXO5L zKyPk_g9WD7G0o^A$x=F6j0RVu(Qrr?S=4n=*Qhc^jkXad_L-1f_1moHR#M2pFe!A- zips0fb66mEYATgN>I{fn5)cF^#lm1lXlXUV5QVzrrgddP8xRzo_iic(nMP2WTq^bx zvpXsaoLW_-sdSvn4Q6ActY9{tcG`*8I(1@FpV0eORQG*<0{}$vduyY z!M5$4@rG}NWEzwH+e4sfgg=m$uF)S*psL=9GffWaRUZi@F z^uza>1M+B`>j^m~QEhHzJzsI0wxePJV@h3DYuFlHBwJNWAyi7L3VgE*zdc?DfW)rN zF+eXsFF@~{peGPBJ*PQ{6>3YqHPZojF9M#02LY|wO5D5S9WxsFGLSBiE|BglNXG;- z4EVgA(LlN9Qx5DA&b>G$R>Ytv6?VxcXEY}qVU3zN4EnYcyf4f)5HJuh5bz8Lc+`== zyJzvPf|=(UhbJ|i99 zsm|=y-nNbE5T=QchDd?#Gu@=~_>^n|be{f7;54T{np*8=8+H~Q5_>HW+cX`EH%huA l=(fPX;K1O(;4@%w5~zEUy2;-s?S;GB{{nT{vxO%g002L2gBbt- literal 0 HcmV?d00001 From 6b66b302c8fe0d62d76a7b23dc7d10ea3eae5d78 Mon Sep 17 00:00:00 2001 From: crw-934 Date: Wed, 7 Oct 2026 22:45:42 +0900 Subject: [PATCH 07/28] CRW-906: close the two ways the correction gate could still be passed, and keep it from starting over an in-flight merge The pre-merge evaluation of d4ca75cf9 found three ways the promise of decision 2 (a node under correction is not merged on its old head) could still be broken. A lane turn that named neither --relationship nor --pr held only the head, and the gate exempted it. Both selectors are optional on merge-turn-request, so the head is the identity left: a turn whose head is an active acceptance's recorded head of its own repository resolves to that acceptance's relationship. A turn whose head no acceptance stands on is still left to the lane's own rules. The pull-request fallback compared the forge slug with a case-sensitive equality, while GitHub slugs are case-insensitive and every other repository identity comparison in the relay folds case: a turn spelling the repository differently read as no accepted result. The lookup folds case now. A correction could be recorded while a merge turn of the same relationship was already merging, of unknown effect, or landed. The relay reads the forge, not the merge, so such a turn may already have carried the accepted head to the base: recording a correction over it would name a head on its way to landing as the one being repaired, and no later refusal can undo a merge the forge already made. The correction is refused and the turn is resolved first, the same conservative reading the verdict writer makes when a ruling rests on a merge turn. A turn that only waits or holds the lane is not this case: the lane's own check and land already refuse it under correction, so it cannot progress. The documentation said the under-correction refusal precedes every judgement rule including an evicted head; the refusal is made where the head would otherwise be judged, so an already-evicted head keeps its eviction. The text now says what the code does. --- docs/relay/dag-scheduler.md | 2 +- .../dagsched/correction_accepted_test.go | 42 ++++++++++++ internal/relay/dagsched/revalidation.go | 33 ++++++++++ .../mergeturn/train_undercorrection_test.go | 27 ++++++++ internal/relay/mergeturn/undercorrection.go | 66 ++++++++++++++++--- 5 files changed, 160 insertions(+), 10 deletions(-) diff --git a/docs/relay/dag-scheduler.md b/docs/relay/dag-scheduler.md index b1581b4f71..71d23b8aa8 100644 --- a/docs/relay/dag-scheduler.md +++ b/docs/relay/dag-scheduler.md @@ -642,7 +642,7 @@ request (by the forge identity recorded with the acceptance) and the base branch A stale accepted result is not judged at all: a stale result never merges (contract 8.2, [Invalidation](#invalidation)). The call is refused `disposition_conflict` naming the stale reason, ahead of every rule above (an evicted head included), and writes no row of the history. The judgement is asked again inside the transaction that writes, so a plan revision that lands between the read of the pull request and the write is seen; `dag-merge-request` asks it once more in the transaction that creates the turn (after the criteria and the predecessors, so a criteria change that lands in that gap is `criteria_set_changed` and an integrated or code-pinned edge added in it, whose predecessor has not landed, is the unmet predecessor), and a revision that lands after the judgement has recorded an `eligible` row leaves that row in the history and creates no turn. A result that is current again (the plan repaired, the node accepted again) is judged as before. -A result that is **under correction** is not judged either, and this one is about the generation rather than the head: while the relationship's live execution generation is later than the generation its active acceptance stands on, a correction generation is open over the accepted result and has not been accepted over yet, so the head the acceptance stands on is the very result being repaired. The call is refused `disposition_conflict` naming the open generation and the generation the acceptance stands on, ahead of every rule in the table above (an evicted head included), and writes no row of the history; `dag-merge-request` makes the same judgement, so it creates no turn. The reading is taken with the acceptance in one statement and asked again in the transaction that writes, so a generation opened while the pull request is being read is seen. It is the guard [Correcting a result that was accepted and is still current](#correcting-a-result-that-was-accepted-and-is-still-current) rests on before the child has pushed: without it, a bundle or a lane turn taken between opening (or recording) the generation and that first push would carry and land the accepted head, which the pull request still shows. A correction accepted over with `dag-accept --supersedes` moves the acceptance to the stand generation and the node is judged as before; a withdrawn generation puts the live generation back and the old head may ride again; a generation recorded by [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) moves the stand generation with the live one, so a base refresh is not a correction. The merge train's own gate (`trainStandRefusal`) refuses a bundle member the same way at open, verify and land, naming the open generation. +A result that is **under correction** is not judged either, and this one is about the generation rather than the head: while the relationship's live execution generation is later than the generation its active acceptance stands on, a correction generation is open over the accepted result and has not been accepted over yet, so the head the acceptance stands on is the very result being repaired. The call is refused `disposition_conflict` naming the open generation and the generation the acceptance stands on, and writes no row of the history; `dag-merge-request` makes the same judgement, so it creates no turn. The refusal is made where the head would otherwise be judged, so a head the history already evicted keeps its eviction (that eviction is final and is restated, never lifted) and a pull request at another head is still stale_head; either way the head is not eligible and no turn is created. The reading is taken with the acceptance in one statement and asked again in the transaction that writes, so a generation opened while the pull request is being read is seen. It is the guard [Correcting a result that was accepted and is still current](#correcting-a-result-that-was-accepted-and-is-still-current) rests on before the child has pushed: without it, a bundle or a lane turn taken between opening (or recording) the generation and that first push would carry and land the accepted head, which the pull request still shows. A correction accepted over with `dag-accept --supersedes` moves the acceptance to the stand generation and the node is judged as before; a withdrawn generation puts the live generation back and the old head may ride again; a generation recorded by [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) moves the stand generation with the live one, so a base refresh is not a correction. The merge train's own gate (`trainStandRefusal`) refuses a bundle member the same way at open, verify and land, naming the open generation. A required check is read the way merge-evidence and the merge lane's own check read it: of every run only its newest attempt counts, every run of a required name must be a `success` (a skipped or neutral run is not one, so a head the judgement lets in is not refused by the lane for the same check), a required (name, integration) pair needs its own run (a name that two integrations must answer needs both), and the opaque run identities are never ordered. Failures count only when every required check has finished, so one red job seen beside a running one never uses up the retry. A second, different failure after the retry round opened evicts. A failure is told from another by the run's identity, its attempt and the time the forge last changed it (a commit status has no attempt and a check run can be reset in place), and a failure of a run that a judgement saw not failing (running or passing) since it failed is a new failure whatever its identity; another check running again says nothing about this one. Everything is computed from the newest reading of each run (a listing that still carries a failed first attempt beside the passing second one judges as the second), a failure counts once a judgement recorded it (a red job beside a running one is not counted), and a reading of a run that is older than one already judged (an earlier attempt, or an earlier completion of the same attempt, arriving late) is not judged: it is `merge_candidate_moved` and the caller reads again. So is a reading of the pull request that was in flight while another judgement of the node was recorded: it is older than the history whatever it contains, and nothing is written. Times are compared as instants (the forge's spelling does not matter). A forge that returns an older state of a run after a newer one was judged cannot always be told from a run that was reset: a reading that goes backwards without a time and with no judgement between the two (a run read as running after its completion) can count as a rerun, so a head can be evicted one failure early. Two failures that carry the same identity and the same time and that no judgement saw apart read as one failure: the node is not eligible either way, but it can take more than one retry. A pull request whose list of required checks cannot be read is not judged (`merge_evidence_malformed`, nothing written): ignorance is not "none required". A draft, closed or merged pull request is `disposition_conflict` (a merged one once its reading is classified, [The forge reading of a merged pull request](#the-forge-reading-of-a-merged-pull-request)); evidence the relay could not read diff --git a/internal/relay/dagsched/correction_accepted_test.go b/internal/relay/dagsched/correction_accepted_test.go index cd0764d5a4..e0a1843638 100644 --- a/internal/relay/dagsched/correction_accepted_test.go +++ b/internal/relay/dagsched/correction_accepted_test.go @@ -262,6 +262,48 @@ func TestAHandOpenedGenerationOfAnAcceptedCurrentResultIsStillRefused(t *testing }) } +// The reason a generation was opened under is what states the correction, and the stale route keeps its own +// Criterion c1, the safety bound of the accepted-current route: a correction is not recorded over an +// accepted head a merge turn of the same relationship is already carrying to the base. The relay reads the +// forge, not the merge, so a turn that is merging or of unknown effect may already have landed the head +// outside the relay; naming that head as the one being repaired would leave the merge unrecorded and the +// correction unmergeable. The turn is resolved first. +func TestACorrectionIsNotRecordedOverAnAcceptedHeadAlreadyOnItsWayToTheBase(t *testing.T) { + t.Parallel() + for _, state := range []string{"merging", "unknown", "landed"} { + t.Run("a merge turn that is "+state, func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, candidate_head, state, tenure, requested_at, updated_at)"+ + " VALUES ('mtn-"+state+"', 'tgt', 'owner/repo', 'dev', 'P-TEST', 'parent', 'host', ?, ?, ?, 1, 't', 't')", rid, accepted["B"].HeadSHA, state) + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a turn that is %s = %v, want disposition_conflict", state, err) + } + if !strings.Contains(err.Error(), "on its way to the base") || !strings.Contains(err.Error(), "mtn-"+state) { + t.Fatalf("the refusal does not name the turn and the reason: %v", err) + } + if got := acExecution(k, "B", 2); got != "" { + t.Fatalf("the refused correction wrote the execution %q", got) + } + }) + } + t.Run("a turn that only holds the lane does not block the correction", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + // a held turn cannot progress: the lane's own check and land refuse it under correction + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, candidate_head, state, tenure, requested_at, updated_at)"+ + " VALUES ('mtn-held', 'tgt', 'owner/repo', 'dev', 'P-TEST', 'parent', 'host', ?, ?, 'holding', 1, 't', 't')", rid, accepted["B"].HeadSHA) + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); err != nil { + t.Fatalf("a held turn blocked the correction: %v", err) + } + }) +} + // The reason a generation was opened under is what states the correction, and the stale route keeps its own // gate: a stale node whose route says correct is recorded under either reason (the new value is a correction // reason like the default, not a route of its own), and one whose route is anything else is still refused diff --git a/internal/relay/dagsched/revalidation.go b/internal/relay/dagsched/revalidation.go index 9704a7f379..462358cd14 100644 --- a/internal/relay/dagsched/revalidation.go +++ b/internal/relay/dagsched/revalidation.go @@ -171,6 +171,16 @@ func (s *Scheduler) recordHandOpened(ctx context.Context, q store.Querier, plan case !correctionOpenReason(reason): return refuse(contract.RefusalDispositionConflict, "%s, and the accepted result of %s is current: a generation opened by hand corrects it only when the reason it was opened under states a correction, and generation %d reads %q. Open the generation with generation-open --reason needs_changes_revision or %s, or, when it only merged the base into the branch, record it with dag-base-refresh", notRuled, n.NodeID, rel.Generation, reason, AcceptedResultCorrection) } + // CRW-906: a turn of this node that is merging or of unknown effect may already have carried the + // accepted head to the base on the forge, outside the relay. Recording a correction now would name + // a head that is on its way to landing as the one being repaired, and no later refusal can undo a + // merge the forge already made: the turn is resolved first (merge-turn-resolve, or merge-turn-unknown + // then merge-turn-resolve). A turn that only waits or holds the lane is not this case: the lane's own + // check and land refuse it under correction, so it cannot progress. This is the same conservative + // reading the verdict writer makes when a ruling rests on a merge turn (registry.RestsOn). + if err := s.refuseAcceptedHeadOnItsWayToTheBase(ctx, q, acc); err != nil { + return err + } } // the same route a stale node has without the generation: a change of the criteria alone is ruled again (a revalidation, no generation), and what rests on a stale predecessor or an input that is not // there waits; only a result that must be reworked is corrected @@ -464,6 +474,29 @@ func (s *Scheduler) refuseRevalidation(ctx context.Context, q store.Querier, pla return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is stale (%s) and ruling the same output again would not make it current; its route is %s: %s", n.NodeID, st.Reason(), action, detail) } +// refuseAcceptedHeadOnItsWayToTheBase is the guard CRW-906 adds to the accepted-current correction: a +// merge turn of the node's relationship that is merging or of unknown effect may already have carried the +// accepted head to the base on the forge. The relay reads the forge, not the merge itself, so the merge +// is a fact the relay may not have recorded; recording a correction over it would name a head that is on +// its way to landing as the one being repaired, and no later refusal can undo a merge the forge already +// made. The turn is resolved first (merge-turn-resolve, or merge-turn-unknown then merge-turn-resolve). +// +// A turn that only waits or holds the lane is not this case: the lane's own check and land refuse it +// under correction (mergeturn.underCorrectionRefusal), so it cannot progress to the base while the +// generation is open. The reading is the same conservative one the verdict writer makes when a ruling +// rests on a merge turn (registry.RestsOn): a turn of the assignment that is merging, unknown or landed +// counts, whatever head it names. +func (s *Scheduler) refuseAcceptedHeadOnItsWayToTheBase(ctx context.Context, q store.Querier, acc Acceptance) error { + var turn, state, head string + found, err := queryOne(ctx, q, "SELECT turn_id, state, candidate_head FROM merge_turns"+ + " WHERE relationship_id = ? AND state IN ('merging','unknown','landed') ORDER BY requested_at, turn_id LIMIT 1", []any{acc.RelationshipID}, &turn, &state, &head) + if err != nil || !found { + return err + } + return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is on its way to the base already: merge turn %s of the relationship is %s on head %s, and a correction cannot be recorded over a head the forge may already have merged. Resolve that turn first (merge-turn-resolve, or merge-turn-unknown then merge-turn-resolve) and open the generation again", + acc.NodeID, short(turn), state, short(head)) +} + // refuseLanded is the guard of a correction (contract 8.4, E-20): a node whose accepted head landed in every target it lands on is never run again, whatever changed above it and whatever the plan now // says the node is: what landed is judged from the acceptance, so a revision that changes the kind of a landed node does not lift it. The change is carried by a successor node of a new plan revision, // which is released like any other node. diff --git a/internal/relay/mergeturn/train_undercorrection_test.go b/internal/relay/mergeturn/train_undercorrection_test.go index 288c5f67ee..b735a23f1d 100644 --- a/internal/relay/mergeturn/train_undercorrection_test.go +++ b/internal/relay/mergeturn/train_undercorrection_test.go @@ -168,6 +168,33 @@ func TestTheLaneGateResolvesTheRelationshipFromThePullRequest(t *testing.T) { } } +// CRW-906 generation 2: --relationship and --pr are both optional on merge-turn-request, so a claim can +// carry neither and hold only the head. The accepted head of a node under correction is then the one +// identity left, and the acceptance that recorded it is what the head belongs to: the turn is refused on +// the head the correction is repairing rather than exempted by the two selectors it omits. +func TestTheLaneGateResolvesTheRelationshipFromTheHeadItHolds(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + turn := store.MergeTurnsRow{TurnID: "mtn-bare", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a turn naming neither selector, holding an accepted head under correction, was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "under correction") || !strings.Contains(refusal.Detail, "rel-lane") { + t.Fatalf("the refusal does not name the relationship and the reason: %s", refusal.Detail) + } + // a head no active acceptance of that repository stands on is left to the lane's own rules + other := turn + other.CandidateHead = "head-unaccepted" + if refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), other); err != nil || refusal != nil { + t.Fatalf("a turn whose head no acceptance stands on = %+v %v, want no refusal", refusal, err) + } +} + // ucHeldOn is a held, acknowledged turn on a head whose claim records a relationship. func (w *fx) ucHeldOn(relationship, head string) string { w.t.Helper() diff --git a/internal/relay/mergeturn/undercorrection.go b/internal/relay/mergeturn/undercorrection.go index 8c210d9ec0..a6310e17e5 100644 --- a/internal/relay/mergeturn/undercorrection.go +++ b/internal/relay/mergeturn/undercorrection.go @@ -28,25 +28,57 @@ import ( // instead, and a node under correction would otherwise be carried on the head the correction is // repairing by the very route that omits the relationship. The acceptance's forge identity is the // mapping from that pull request back to the relationship (dag_acceptance_forge, written with the -// acceptance), and it is read here so both routes meet the same gate. A turn whose pull request has no -// accepted result is left to the lane's own rules. +// acceptance), and it is read here so both routes meet the same gate. The forge slug is compared +// case-insensitively, as every other repository identity comparison in the relay is: GitHub slugs are +// case-insensitive, so a turn that spells the repository differently names the same pull request. +// +// A turn that names neither is not exempt either: --relationship and --pr are both optional on +// merge-turn-request, so the head alone is left, and a turn holding the accepted head of a node under +// correction would slip through the very route that omits both selectors. The active acceptance +// records its own repository and head (dag_acceptances.repository, head_sha), so a turn whose head is +// an accepted head of its repository resolves to that acceptance's relationship. A turn whose head no +// active acceptance of that repository stands on is left to the lane's own rules. // // The refusal is the existing disposition_conflict and it names the open generation: no new refusal // name, no column, no schema change. func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeTurnsRow) (*registry.CoordinationRefusal, error) { relationship := r.RelationshipID.String if !r.RelationshipID.Valid || relationship == "" { - if !r.PRNumber.Valid { + // The DAG zone arrives with the first write open (D-01), and the lane serves projects that + // have none: a store that predates the table has no accepted result for this turn, which is + // absence and not an error. + present, err := ucZoneTable(ctx, q, "dag_acceptances") + if err != nil { + return nil, err + } + if !present { return nil, nil } var found string - if err := q.QueryRowContext(ctx, "SELECT a.relationship_id FROM dag_acceptance_forge f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id"+ - " WHERE f.forge_repository = ? AND f.pr_number = ? AND a.state = 'active' ORDER BY a.accepted_at DESC, a.acceptance_id DESC LIMIT 1", - r.Repository, r.PRNumber.Int64).Scan(&found); err != nil { - if errors.Is(err, sql.ErrNoRows) { - return nil, nil + switch { + case r.PRNumber.Valid: + // the turn names the pull request: the acceptance's recorded forge identity maps it back + if err := q.QueryRowContext(ctx, "SELECT a.relationship_id FROM dag_acceptance_forge f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id"+ + " WHERE lower(f.forge_repository) = lower(?) AND f.pr_number = ? AND a.state = 'active' ORDER BY a.accepted_at DESC, a.acceptance_id DESC LIMIT 1", + r.Repository, r.PRNumber.Int64).Scan(&found); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + return nil, err } - return nil, err + case r.CandidateHead != "": + // the turn names neither selector: the head it holds is the only identity left, and the + // acceptance that recorded it is what the head belongs to + if err := q.QueryRowContext(ctx, "SELECT a.relationship_id FROM dag_acceptances a"+ + " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND a.head_sha = ? ORDER BY a.accepted_at DESC, a.acceptance_id DESC LIMIT 1", + r.Repository, r.CandidateHead).Scan(&found); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + return nil, err + } + default: + return nil, nil } relationship = found } @@ -65,6 +97,22 @@ func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeT r.CandidateHead, r.HolderTaskID), nil } +// trainUnderCorrectionRefusal is the bundle member's refusal: the member's relationship has a correction +// ucZoneTable reports whether the store holds the named DAG zone table. The zone arrives with the first +// write open (D-01), so a store that predates it holds no acceptance and the lane's turn is left to the +// lane's own rules; that is absence and not an error, the way acceptance.Stands reads dag_base_refreshes. +func ucZoneTable(ctx context.Context, q store.Querier, table string) (bool, error) { + var name string + err := q.QueryRowContext(ctx, "SELECT name FROM sqlite_master WHERE type = 'table' AND name = ?", table).Scan(&name) + if errors.Is(err, sql.ErrNoRows) { + return false, nil + } + if err != nil { + return false, err + } + return name == table, nil +} + // trainUnderCorrectionRefusal is the bundle member's refusal: the member's relationship has a correction // generation open over its accepted result, so the head the bundle would carry is the result being // repaired. It is the existing disposition_conflict and it names the open generation. From 30087f61af09f3774bdd093a340b7e90733d5395 Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Wed, 7 Oct 2026 23:32:20 +0900 Subject: [PATCH 08/28] CRW-906: resolve a refreshed head in the lane gate, and read an excluded member's correction at land The pre-merge evaluation of 6b66b302c found two more ways the promise of decision 2 (a node under correction is not merged on its old head) could still be broken. Both paths existed on dev without the gate, and both fall inside the promise this issue makes. A recorded base refresh moves the head the acceptance stands on without changing the accepted head, so a lane turn naming neither --relationship nor --pr and holding the refreshed head resolved to no acceptance. The head lookup reads dag_base_refreshes beside dag_acceptances now: the head a turn holds is the accepted head or the head of a refresh of it, and either belongs to the same acceptance. merge-train-land excluded a member whose turn left the lane from the landing, but the member's code is still in the merge commit the bundle lands, so a tree that still carried a result under correction could be recorded landed. The excluded members' correction state is read inside the same transaction now. The CRW-897 carve-out keeps holding for what it is about: a revoked acceptance or a moved head of a member that left does not refuse the rest of the bundle. --- internal/relay/mergeturn/train.go | 13 ++++ .../mergeturn/train_undercorrection_test.go | 63 +++++++++++++++++++ internal/relay/mergeturn/undercorrection.go | 21 +++++-- 3 files changed, 93 insertions(+), 4 deletions(-) diff --git a/internal/relay/mergeturn/train.go b/internal/relay/mergeturn/train.go index a278d2dc01..6ed84514c3 100644 --- a/internal/relay/mergeturn/train.go +++ b/internal/relay/mergeturn/train.go @@ -969,6 +969,13 @@ func (s *Service) TrainLand(ctx context.Context, train, actor, landed, observed survivors := make([]store.MergeTrainMemberRow, 0, len(members)) turns := make(map[string]store.MergeTurnsRow, len(members)) excluded := make([]any, 0, len(members)) + // CRW-906: the members whose turn left the lane are excluded from the landing, but their code is + // still in the merge commit this bundle lands (every member head is checked as an ancestor of it + // above), so their correction state is read too. The CRW-897 carve-out keeps holding for what it + // is about (a revoked acceptance or a moved head of a member that left does not refuse the rest + // of the bundle); this is the separate question of whether the tree being landed still contains a + // result the plan is repairing. + leftMembers := make([]store.MergeTrainMemberRow, 0, len(members)) for _, m := range members { turn, e := s.Store.MergeTurn(tx, m.TurnID) if e != nil { @@ -976,6 +983,7 @@ func (s *Service) TrainLand(ctx context.Context, train, actor, landed, observed } if turn.State != Holding && turn.State != Waiting { excluded = append(excluded, map[string]any{"seq": m.Seq, "turnId": m.TurnID, "prNumber": m.PRNumber, "relationshipId": m.RelationshipID, "state": turn.State, "closeReason": value(turn.CloseReason)}) + leftMembers = append(leftMembers, m) continue } if !SameCommit(turn.CandidateHead, m.MemberHead) { @@ -988,6 +996,11 @@ func (s *Service) TrainLand(ctx context.Context, train, actor, landed, observed // or moved after open refuses the landing with nothing written (Blocking 1). A member that // left the lane is excluded above and is not re-read, so its parent revoking the acceptance // does not refuse the rest of the bundle. + for _, m := range leftMembers { + if e := trainMemberCorrectionRefusal(tx, s.Store.Querier(tx), m.PRNumber, m.RelationshipID, m.MemberHead); e != nil { + return e + } + } expectations, e := s.trainExpectations(tx, survivors) if e != nil { return e diff --git a/internal/relay/mergeturn/train_undercorrection_test.go b/internal/relay/mergeturn/train_undercorrection_test.go index b735a23f1d..4e1aadbb6b 100644 --- a/internal/relay/mergeturn/train_undercorrection_test.go +++ b/internal/relay/mergeturn/train_undercorrection_test.go @@ -264,6 +264,69 @@ func TestTheLaneRefusesATurnWhoseAcceptedResultIsUnderCorrection(t *testing.T) { }) } +// CRW-906 generation 2: a recorded base refresh moves the head the acceptance stands on without changing +// the accepted head, so a lane turn that names neither selector and holds the refreshed head must still +// resolve to the relationship whose correction is open. The head it holds is the accepted head or the head +// of a refresh of it, and either belongs to the same acceptance. +func TestTheLaneGateResolvesARefreshedHead(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + // the acceptance stands on head-a; a base refresh moved it to head-refreshed + refresh := "dbr-" + strings.Repeat("a", 60) + w.exec("INSERT INTO dag_base_refreshes (refresh_id, acceptance_id, refresh_seq, relationship_id, execution_generation, event_id, revision_hash, head_sha, base_repository, base_ref, base_tip_sha, proof_json, resolved_paths_json, recorded_by_task_id, coordinator_epoch, recorded_at)"+ + " VALUES (?, ?, 1, ?, 1, 'ev-refresh', 'rev-refresh', 'head-refreshed', ?, ?, 'base-0', '{}', '[]', ?, 0, '2023-11-14T22:13:20.000000+00:00')", + refresh, "acc-rel-lane", "rel-lane", fxRepo, fxBase, alpha.TaskID) + turn := store.MergeTurnsRow{TurnID: "mtn-refreshed", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-refreshed", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a turn holding the refreshed head of an accepted result under correction was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "under correction") || !strings.Contains(refusal.Detail, "rel-lane") { + t.Fatalf("the refusal does not name the relationship and the reason: %s", refusal.Detail) + } +} + +// CRW-906 generation 2: a member whose turn left the lane is excluded from the landing, but its code is +// still in the merge commit the bundle lands (every member head is checked as an ancestor of it), so the +// bundle must not record a landing of a tree that still carries a result the plan is repairing. The +// CRW-897 carve-out keeps holding for what it is about: a revoked acceptance or a moved head of a member +// that left does not refuse the rest of the bundle. +func TestTrainLandRefusesAnExcludedMemberUnderCorrection(t *testing.T) { + w := newTr(t) + train := w.verifiedTrain() + rows, err := w.s.All(w.ctx, "SELECT turn_id FROM merge_train_members WHERE train_id = ? AND seq = 2", train) + if err != nil || len(rows) != 1 { + t.Fatalf("member 2: %v", err) + } + memberTurn := rows[0].Get("turn_id").(string) + // the member's parent withdraws its waiting turn, and the correction over its accepted result opens + if _, err := w.m.Withdraw(w.ctx, memberTurn, "task-m2"); err != nil { + t.Fatalf("withdrawing the member's turn: %v", err) + } + w.ucCorrection("rel-task-m2") + w.tip.set(trRepo, trBase, "merge-1") + w.forge.commits["merge-1"] = TrainCommit{SHA: "merge-1", Parents: []string{"base-0", "head-bundle"}, Tree: "tree-bundle"} + _, err = w.m.TrainLand(w.ctx, train, trLeader, "merge-1", "", w.tip, w.forge) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a land of a bundle still carrying an excluded member under correction: %v", err) + } + for _, want := range []string{"under correction", "generation 2", "generation 1"} { + if !strings.Contains(err.Error(), want) { + t.Fatalf("the refusal does not name %q: %v", want, err) + } + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'landed'"); n != 0 { + t.Fatalf("a refused land wrote %d landed event(s)", n) + } + if n := w.count("SELECT count(*) FROM merge_turns WHERE state = 'landed'"); n != 0 { + t.Fatalf("a refused land landed %d turn(s)", n) + } +} + // ucCount is the number of rows a query answers, for the writes a refusal must not leave behind. func (w *fx) ucCount(query string, args ...any) int64 { w.t.Helper() diff --git a/internal/relay/mergeturn/undercorrection.go b/internal/relay/mergeturn/undercorrection.go index a6310e17e5..ac2edf7a31 100644 --- a/internal/relay/mergeturn/undercorrection.go +++ b/internal/relay/mergeturn/undercorrection.go @@ -68,10 +68,23 @@ func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeT } case r.CandidateHead != "": // the turn names neither selector: the head it holds is the only identity left, and the - // acceptance that recorded it is what the head belongs to - if err := q.QueryRowContext(ctx, "SELECT a.relationship_id FROM dag_acceptances a"+ - " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND a.head_sha = ? ORDER BY a.accepted_at DESC, a.acceptance_id DESC LIMIT 1", - r.Repository, r.CandidateHead).Scan(&found); err != nil { + // acceptance that recorded it is what the head belongs to. A recorded base refresh moves + // the head the acceptance stands on without changing the accepted head (dag_base_refreshes + // carries the refreshed head), so both are read: the head the turn holds is the accepted + // head or the head of a refresh of it, and either belongs to the same acceptance. + query := "SELECT a.relationship_id FROM dag_acceptances a" + + " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND a.head_sha = ?" + args := []any{r.Repository, r.CandidateHead} + if present, err := ucZoneTable(ctx, q, "dag_base_refreshes"); err != nil { + return nil, err + } else if present { + query = "SELECT a.relationship_id FROM dag_acceptances a" + + " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND a.head_sha = ?" + + " UNION ALL SELECT a.relationship_id FROM dag_base_refreshes f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id" + + " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND f.head_sha = ?" + args = []any{r.Repository, r.CandidateHead, r.Repository, r.CandidateHead} + } + if err := q.QueryRowContext(ctx, query+" ORDER BY 1 LIMIT 1", args...).Scan(&found); err != nil { if errors.Is(err, sql.ErrNoRows) { return nil, nil } From e1a90722a47362d100b70760bcc4bbb62d43adf6 Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Thu, 8 Oct 2026 00:36:11 +0900 Subject: [PATCH 09/28] CRW-906: resolve the in-flight turn by every identity, ask every matching acceptance, and hold an excluded member to its stand The pre-merge evaluation of 30087f61a found three more ways the promise of decision 2 (a node under correction is not merged on its old head) could still be broken. d1: the in-flight-merge guard read only merge_turns.relationship_id, so a turn requested without --relationship escaped it. --relationship and --pr are both optional on merge-turn-request, and a successful merge-turn-check has already authorized the merge of H before the correction is recorded, so the later Land refusal cannot undo what the forge was authorized to do. The guard now resolves the turn by the relationship, the pull request or the head it holds, the same three identities the lane's own gate resolves. d2: the selector fallback asked one arbitrarily chosen acceptance (newest, or relationship order). Acceptance uniqueness is per node and output, not per repository and pull request or per repository and head, so two accepted nodes can name the same commit. Every matching relationship is read now, and the turn is refused when any of them has a correction open over the head it holds. d3: an excluded member's hold cleared as soon as its corrected result was accepted over. Exclusion changes the accounting, not the bundle tree: the verified tree still carries the head it was verified on, so once the acceptance stands on a corrected head the bundle no longer holds what the plan accepts for that member. The excluded member is now held to its stand, and a bundle that no longer carries it is refused for rebuild and re-verification. The CRW-897 carve-out keeps holding for what it is about: a revoked acceptance or a moved head of a member that left does not refuse the rest of the bundle. --- .../dagsched/correction_accepted_test.go | 43 +++++++ internal/relay/dagsched/revalidation.go | 24 +++- internal/relay/mergeturn/train.go | 2 +- .../mergeturn/train_undercorrection_test.go | 64 ++++++++++ internal/relay/mergeturn/undercorrection.go | 116 +++++++++++++----- 5 files changed, 217 insertions(+), 32 deletions(-) diff --git a/internal/relay/dagsched/correction_accepted_test.go b/internal/relay/dagsched/correction_accepted_test.go index e0a1843638..5b6eeac77c 100644 --- a/internal/relay/dagsched/correction_accepted_test.go +++ b/internal/relay/dagsched/correction_accepted_test.go @@ -263,6 +263,49 @@ func TestAHandOpenedGenerationOfAnAcceptedCurrentResultIsStillRefused(t *testing } // The reason a generation was opened under is what states the correction, and the stale route keeps its own +// Criterion c1, the safety bound of the accepted-current route: a correction is not recorded over an +// Criterion c1, generation 2, d1: the guard resolves the merge turn by every identity a claim may carry. +// --relationship and --pr are both optional on merge-turn-request, and a successful merge-turn-check has +// already authorized the merge of H before the correction is recorded. A guard that read only the +// relationship would miss that turn and admit the record, and the later Land refusal cannot undo a merge +// the forge was already authorized to make. +func TestACorrectionIsNotRecordedOverATurnThatNamesNoRelationship(t *testing.T) { + t.Parallel() + for _, identity := range []string{"pull request", "head"} { + t.Run("a merging turn identified by its "+identity, func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + // the acceptance records the repository, the pull request and the head the turn may name + k.exec("UPDATE dag_acceptances SET repository = 'owner/repo', head_sha = 'head-b', pr_number = 5 WHERE relationship_id = ?", rid) + // the turn names no relationship: the guard must resolve it from the pull request or the head + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, pr_number, candidate_head, state, tenure, requested_at, updated_at)"+ + " VALUES ('mtn-pr-only', 'tgt', 'owner/repo', 'dev', 'P-TEST', 'parent', 'host', NULL, ?, 'head-b', 'merging', 1, 't', 't')", + prNumberFor(identity)) + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a merging turn identified by its %s = %v, want disposition_conflict", identity, err) + } + if !strings.Contains(err.Error(), "on its way to the base") || !strings.Contains(err.Error(), "mtn-pr-only") { + t.Fatalf("the refusal does not name the turn and the reason: %v", err) + } + if got := acExecution(k, "B", 2); got != "" { + t.Fatalf("the refused correction wrote the execution %q", got) + } + }) + } +} + +// prNumberFor is the pull request the turn records for the case named: the acceptance's, or NULL so the +// head is the only identity left. +func prNumberFor(identity string) any { + if identity == "pull request" { + return int64(5) + } + return nil +} + // Criterion c1, the safety bound of the accepted-current route: a correction is not recorded over an // accepted head a merge turn of the same relationship is already carrying to the base. The relay reads the // forge, not the merge, so a turn that is merging or of unknown effect may already have landed the head diff --git a/internal/relay/dagsched/revalidation.go b/internal/relay/dagsched/revalidation.go index 462358cd14..baa45c7c01 100644 --- a/internal/relay/dagsched/revalidation.go +++ b/internal/relay/dagsched/revalidation.go @@ -486,10 +486,32 @@ func (s *Scheduler) refuseRevalidation(ctx context.Context, q store.Querier, pla // generation is open. The reading is the same conservative one the verdict writer makes when a ruling // rests on a merge turn (registry.RestsOn): a turn of the assignment that is merging, unknown or landed // counts, whatever head it names. +// +// The turn is resolved by every identity a claim may carry, because --relationship and --pr are both +// optional on merge-turn-request: the relationship, the pull request, or the head the turn holds. The +// lane's own gate resolves the same three (mergeturn.underCorrectionRefusal), and a guard that read only +// the relationship would miss the turn a successful merge-turn-check had just authorized. func (s *Scheduler) refuseAcceptedHeadOnItsWayToTheBase(ctx context.Context, q store.Querier, acc Acceptance) error { + heads, err := s.stoodOn(ctx, q, acc) + if err != nil { + return err + } + clauses := []string{"relationship_id = ?"} + args := []any{acc.RelationshipID} + for _, head := range heads { + if head == "" { + continue + } + clauses = append(clauses, "(lower(repository) = lower(?) AND candidate_head = ?)") + args = append(args, acc.Repository, head) + } + if acc.PRNumber > 0 { + clauses = append(clauses, "(lower(repository) = lower(?) AND pr_number = ?)") + args = append(args, acc.Repository, acc.PRNumber) + } var turn, state, head string found, err := queryOne(ctx, q, "SELECT turn_id, state, candidate_head FROM merge_turns"+ - " WHERE relationship_id = ? AND state IN ('merging','unknown','landed') ORDER BY requested_at, turn_id LIMIT 1", []any{acc.RelationshipID}, &turn, &state, &head) + " WHERE state IN ('merging','unknown','landed') AND ("+strings.Join(clauses, " OR ")+") ORDER BY requested_at, turn_id LIMIT 1", args, &turn, &state, &head) if err != nil || !found { return err } diff --git a/internal/relay/mergeturn/train.go b/internal/relay/mergeturn/train.go index 6ed84514c3..8be0d194f2 100644 --- a/internal/relay/mergeturn/train.go +++ b/internal/relay/mergeturn/train.go @@ -997,7 +997,7 @@ func (s *Service) TrainLand(ctx context.Context, train, actor, landed, observed // left the lane is excluded above and is not re-read, so its parent revoking the acceptance // does not refuse the rest of the bundle. for _, m := range leftMembers { - if e := trainMemberCorrectionRefusal(tx, s.Store.Querier(tx), m.PRNumber, m.RelationshipID, m.MemberHead); e != nil { + if e := trainExcludedMemberRefusal(tx, s.Store.Querier(tx), m.PRNumber, m.RelationshipID, m.MemberHead); e != nil { return e } } diff --git a/internal/relay/mergeturn/train_undercorrection_test.go b/internal/relay/mergeturn/train_undercorrection_test.go index 4e1aadbb6b..1487ad820f 100644 --- a/internal/relay/mergeturn/train_undercorrection_test.go +++ b/internal/relay/mergeturn/train_undercorrection_test.go @@ -327,6 +327,70 @@ func TestTrainLandRefusesAnExcludedMemberUnderCorrection(t *testing.T) { } } +// CRW-906 generation 2, d2: acceptance uniqueness is per node and output, not per repository and pull +// request or per repository and head, so two accepted nodes can name the same commit. The gate must ask +// every matching relationship, not the newest one: a turn is refused when any of them is correcting the +// very head it holds. +func TestTheLaneGateAsksEveryMatchingAcceptance(t *testing.T) { + w := newFx(t) + // rel-a-live stands on its live generation and sorts first, so a lookup that took one arbitrary + // match would take it; rel-z-correcting has a correction open over the same head + w.ucLaneRelationship("rel-a-live", 1) + w.exec("INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES ('acc-rel-a-live', 'plan-x', 'node-live', 'manifest-live', 'rel-a-live', 1, 'ev-live', 'rev-live', 'crit-1', 'verified', 'head-a', ?, 1, 'bound', 'turn-1', '{}', ?, 0, '2023-11-14T22:13:19.000000+00:00', 'active')", + fxRepo, alpha.TaskID) + w.ucLaneRelationship("rel-z-correcting", 2) + turn := store.MergeTurnsRow{TurnID: "mtn-two", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a head two nodes accept, one of them under correction, was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "rel-z-correcting") { + t.Fatalf("the refusal does not name the correcting relationship: %s", refusal.Detail) + } +} + +// CRW-906 generation 2, d3: the excluded member's hold must not clear just because the correction was +// accepted over. Exclusion changes the accounting, not the bundle tree: the verified tree still carries the +// member head it was verified on, so once the acceptance stands on a corrected head the bundle no longer +// holds what the plan accepts for that member and must be rebuilt and verified. +func TestTrainLandRefusesAnExcludedMemberWhoseAcceptanceMovedOn(t *testing.T) { + w := newTr(t) + train := w.verifiedTrain() + rows, err := w.s.All(w.ctx, "SELECT turn_id FROM merge_train_members WHERE train_id = ? AND seq = 2", train) + if err != nil || len(rows) != 1 { + t.Fatalf("member 2: %v", err) + } + memberTurn := rows[0].Get("turn_id").(string) + if _, err := w.m.Withdraw(w.ctx, memberTurn, "task-m2"); err != nil { + t.Fatalf("withdrawing the member's turn: %v", err) + } + // dag-accept --supersedes moved the acceptance onto the corrected result: the relationship and the + // acceptance stand on the same generation again, so no correction is open, and the head it stands on + // is the corrected one the verified bundle does not carry. + w.exec("UPDATE relationships SET execution_generation = 2 WHERE relationship_id = 'rel-task-m2'") + w.exec("UPDATE dag_acceptances SET execution_generation = 2, head_sha = 'head-m2-corrected' WHERE relationship_id = 'rel-task-m2' AND state = 'active'") + w.tip.set(trRepo, trBase, "merge-1") + w.forge.commits["merge-1"] = TrainCommit{SHA: "merge-1", Parents: []string{"base-0", "head-bundle"}, Tree: "tree-bundle"} + _, err = w.m.TrainLand(w.ctx, train, trLeader, "merge-1", "", w.tip, w.forge) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a land of a bundle whose excluded member's acceptance moved on: %v", err) + } + if !strings.Contains(err.Error(), "head-m2-corrected") || !strings.Contains(err.Error(), "rebuild and verify") { + t.Fatalf("the refusal does not name the stand and the way on: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'landed'"); n != 0 { + t.Fatalf("a refused land wrote %d landed event(s)", n) + } + if n := w.count("SELECT count(*) FROM merge_turns WHERE state = 'landed'"); n != 0 { + t.Fatalf("a refused land landed %d turn(s)", n) + } +} + // ucCount is the number of rows a query answers, for the writes a refusal must not leave behind. func (w *fx) ucCount(query string, args ...any) int64 { w.t.Helper() diff --git a/internal/relay/mergeturn/undercorrection.go b/internal/relay/mergeturn/undercorrection.go index ac2edf7a31..1c45f43a26 100644 --- a/internal/relay/mergeturn/undercorrection.go +++ b/internal/relay/mergeturn/undercorrection.go @@ -39,11 +39,18 @@ import ( // an accepted head of its repository resolves to that acceptance's relationship. A turn whose head no // active acceptance of that repository stands on is left to the lane's own rules. // +// Acceptance uniqueness is per node and output, not per repository and pull request or per repository +// and head, so more than one accepted node can name the same pull request or the same commit. Every +// matching relationship is asked, not the newest one: a turn is refused when ANY of them has a +// correction open, because the head it holds is then a result being repaired by at least one of them. +// // The refusal is the existing disposition_conflict and it names the open generation: no new refusal // name, no column, no schema change. func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeTurnsRow) (*registry.CoordinationRefusal, error) { - relationship := r.RelationshipID.String - if !r.RelationshipID.Valid || relationship == "" { + candidates := []string{} + if r.RelationshipID.Valid && r.RelationshipID.String != "" { + candidates = append(candidates, r.RelationshipID.String) + } else { // The DAG zone arrives with the first write open (D-01), and the lane serves projects that // have none: a store that predates the table has no accepted result for this turn, which is // absence and not an error. @@ -54,60 +61,78 @@ func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeT if !present { return nil, nil } - var found string + var query string + var args []any switch { case r.PRNumber.Valid: // the turn names the pull request: the acceptance's recorded forge identity maps it back - if err := q.QueryRowContext(ctx, "SELECT a.relationship_id FROM dag_acceptance_forge f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id"+ - " WHERE lower(f.forge_repository) = lower(?) AND f.pr_number = ? AND a.state = 'active' ORDER BY a.accepted_at DESC, a.acceptance_id DESC LIMIT 1", - r.Repository, r.PRNumber.Int64).Scan(&found); err != nil { - if errors.Is(err, sql.ErrNoRows) { - return nil, nil - } - return nil, err - } + query = "SELECT DISTINCT a.relationship_id FROM dag_acceptance_forge f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id" + + " WHERE lower(f.forge_repository) = lower(?) AND f.pr_number = ? AND a.state = 'active'" + args = []any{r.Repository, r.PRNumber.Int64} case r.CandidateHead != "": // the turn names neither selector: the head it holds is the only identity left, and the // acceptance that recorded it is what the head belongs to. A recorded base refresh moves // the head the acceptance stands on without changing the accepted head (dag_base_refreshes // carries the refreshed head), so both are read: the head the turn holds is the accepted // head or the head of a refresh of it, and either belongs to the same acceptance. - query := "SELECT a.relationship_id FROM dag_acceptances a" + + query = "SELECT DISTINCT a.relationship_id FROM dag_acceptances a" + " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND a.head_sha = ?" - args := []any{r.Repository, r.CandidateHead} + args = []any{r.Repository, r.CandidateHead} if present, err := ucZoneTable(ctx, q, "dag_base_refreshes"); err != nil { return nil, err } else if present { - query = "SELECT a.relationship_id FROM dag_acceptances a" + + query = "SELECT DISTINCT a.relationship_id FROM dag_acceptances a" + " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND a.head_sha = ?" + " UNION ALL SELECT a.relationship_id FROM dag_base_refreshes f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id" + " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND f.head_sha = ?" args = []any{r.Repository, r.CandidateHead, r.Repository, r.CandidateHead} } - if err := q.QueryRowContext(ctx, query+" ORDER BY 1 LIMIT 1", args...).Scan(&found); err != nil { - if errors.Is(err, sql.ErrNoRows) { - return nil, nil - } - return nil, err - } default: return nil, nil } - relationship = found + found, err := ucRelationships(ctx, q, query, args...) + if err != nil { + return nil, err + } + candidates = found } - under, live, stand, err := acceptance.UnderCorrection(ctx, q, relationship) + for _, relationship := range candidates { + under, live, stand, err := acceptance.UnderCorrection(ctx, q, relationship) + if err != nil { + return nil, err + } + if !under { + continue + } + return coordination(r, contract.RefusalDispositionConflict, + "the accepted result of this turn's relationship "+pyvalue.StrRepr(relationship)+ + " is under correction: generation "+strconv.FormatInt(live, 10)+" is open over the acceptance, which stands on generation "+ + strconv.FormatInt(stand, 10)+", so the head this turn holds is the result being repaired and is not merged. "+ + "Accept the corrected result with dag-accept --supersedes, or withdraw the generation, and request the turn again", + r.CandidateHead, r.HolderTaskID), nil + } + return nil, nil +} + +// ucRelationships are every relationship a resolution query names, in the query's order. The callers +// read all of them rather than the newest: acceptance uniqueness is per node and output, so more than +// one accepted node can name the same pull request or the same head, and the turn is refused when any +// of them has a correction open. +func ucRelationships(ctx context.Context, q store.Querier, query string, args ...any) ([]string, error) { + rows, err := q.QueryContext(ctx, query, args...) if err != nil { return nil, err } - if !under { - return nil, nil + defer rows.Close() + var out []string + for rows.Next() { + var relationship string + if err := rows.Scan(&relationship); err != nil { + return nil, err + } + out = append(out, relationship) } - return coordination(r, contract.RefusalDispositionConflict, - "the accepted result of this turn's relationship "+pyvalue.StrRepr(relationship)+ - " is under correction: generation "+strconv.FormatInt(live, 10)+" is open over the acceptance, which stands on generation "+ - strconv.FormatInt(stand, 10)+", so the head this turn holds is the result being repaired and is not merged. "+ - "Accept the corrected result with dag-accept --supersedes, or withdraw the generation, and request the turn again", - r.CandidateHead, r.HolderTaskID), nil + return out, rows.Err() } // trainUnderCorrectionRefusal is the bundle member's refusal: the member's relationship has a correction @@ -147,3 +172,34 @@ func trainMemberCorrectionRefusal(ctx context.Context, q store.Querier, pr int64 } return trainUnderCorrectionRefusal(pr, relationship, live, stand) } + +// trainExcludedMemberRefusal is CRW-906's guard on a member whose turn left the lane, and it answers the +// question the excluded path must still ask: is the result the bundle carries for this member still the +// result the plan stands on? +// +// The CRW-897 carve-out keeps holding for what it is about: a member whose turn was returned or withdrawn +// is not recorded landed, and neither its pull request moving nor its acceptance being revoked refuses +// the rest of the bundle. This is a different question. A correction of the member's accepted result is +// accepted over by dag-accept --supersedes, which moves the acceptance to the corrected head; the old +// bundle's tree still carries the defective head it was verified on, so landing it would land exactly the +// result the correction repaired. Once the acceptance stands on a head the bundle does not carry, the +// bundle no longer describes what the plan accepts for that member and must be rebuilt and verified. +func trainExcludedMemberRefusal(ctx context.Context, q store.Querier, pr int64, relationship, memberHead string) error { + active, found, err := acceptance.ActiveForRelationship(ctx, q, relationship) + if err != nil { + return trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) + } + if !found { + // the carve-out: a revoked acceptance does not refuse the rest of the bundle + return nil + } + stand, err := acceptance.StandOf(ctx, q, active.AcceptanceID, relationship, active.Generation, active.EventID, active.RevisionHash, active.HeadSHA) + if err != nil { + return trainUnreadable("what acceptance %s stands on was not read: %v", pyvalue.StrRepr(active.AcceptanceID), err) + } + if stand.Head != "" && !SameCommit(stand.Head, memberHead) { + return trainConflict("pull request %d's relationship %s now stands on %s and the bundle carries %s for it, so this bundle no longer holds the result the plan accepts for that member: rebuild and verify the bundle on the current result before landing it", + pr, pyvalue.StrRepr(relationship), pyvalue.StrRepr(stand.Head), pyvalue.StrRepr(memberHead)) + } + return trainMemberCorrectionRefusal(ctx, q, pr, relationship, memberHead) +} From 141c105f88cf4cabaf5774699b23725ef4fdfb2d Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Thu, 8 Oct 2026 01:35:59 +0900 Subject: [PATCH 10/28] CRW-906: keep a named relationship from hiding a shared head, and catch a live bundle or a moved stand The pre-merge evaluation of e1a90722a found three more ways the promise of decision 2 (a node under correction is not merged on its old head) could still be broken. d1: naming a relationship suppressed the other identities. Acceptance uniqueness is per node and output, so two accepted nodes can accept the same commit: a turn that names the relationship which is NOT being corrected is still carrying a head another relationship is repairing. The lane gate now resolves the relationship AND the pull-request and head identities, so a named relationship adds to the set instead of replacing it. d2: the in-flight guard read merge turns only. A verified bundle is the other way the accepted head may already be on the base: the parent merges the bundle on the forge and records it with merge-train-land afterwards, so between those two the members still hold or wait in the lane while the forge already carries the head. A live (opened or verified) bundle carrying the node's stand head now refuses the correction, and the way on is merge-train-close. A bundle that landed, was done or was abandoned carries nothing live. d3: the recheck inside the write transaction asked only whether a correction is open. A correction opened AND accepted over inside the gap makes the live and stand generations equal again, so the check now compares the head the bundle carries with the head the acceptance stands on. That covers verify and open, and the excluded-member path uses the same comparison. --- .../dagsched/correction_accepted_test.go | 44 +++++++ internal/relay/dagsched/revalidation.go | 49 +++++++- .../mergeturn/train_undercorrection_test.go | 80 +++++++++++- internal/relay/mergeturn/undercorrection.go | 115 ++++++++++++------ 4 files changed, 248 insertions(+), 40 deletions(-) diff --git a/internal/relay/dagsched/correction_accepted_test.go b/internal/relay/dagsched/correction_accepted_test.go index 5b6eeac77c..02c1b3aa17 100644 --- a/internal/relay/dagsched/correction_accepted_test.go +++ b/internal/relay/dagsched/correction_accepted_test.go @@ -264,6 +264,50 @@ func TestAHandOpenedGenerationOfAnAcceptedCurrentResultIsStillRefused(t *testing // The reason a generation was opened under is what states the correction, and the stale route keeps its own // Criterion c1, the safety bound of the accepted-current route: a correction is not recorded over an +// Criterion c1, generation 2, d2: a live bundle is the other way the accepted head may already be on the +// base. The parent merges a verified bundle on the forge and records it with merge-train-land afterwards, +// so between those two the bundle's members still hold or wait in the lane while the forge already carries +// the head. Recording a correction in that window names a head the forge may already have merged. +func TestACorrectionIsNotRecordedOverALiveBundleCarryingTheHead(t *testing.T) { + t.Parallel() + for _, kind := range []string{"opened", "verified"} { + t.Run("a bundle that is "+kind, func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("UPDATE dag_acceptances SET repository = 'owner/repo', head_sha = 'head-b' WHERE relationship_id = ?", rid) + // a live bundle carries this node's accepted head as a member + k.exec("INSERT INTO merge_trains (train_id, target_key, repository, base_ref, base_sha, leader_task_id, created_at) VALUES ('trn-" + kind + "', 'tgt', 'owner/repo', 'dev', 'base-0', 'parent', 't')") + k.exec("INSERT INTO merge_train_members (train_id, seq, turn_id, pr_number, relationship_id, member_head) VALUES ('trn-"+kind+"', 1, 'turn-1', 5, ?, 'head-b')", rid) + k.exec("INSERT INTO merge_train_events (train_id, seq, kind, actor, detail_json, recorded_at) VALUES ('trn-"+kind+"', 1, ?, 'parent', '{}', 't')", kind) + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a %s bundle carrying the head = %v, want disposition_conflict", kind, err) + } + if !strings.Contains(err.Error(), "trn-"+kind) || !strings.Contains(err.Error(), "merge-train-close") { + t.Fatalf("the refusal does not name the bundle and the way on: %v", err) + } + if got := acExecution(k, "B", 2); got != "" { + t.Fatalf("the refused correction wrote the execution %q", got) + } + }) + } + t.Run("a bundle that landed does not block the correction", func(t *testing.T) { + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + k.exec("UPDATE dag_acceptances SET repository = 'owner/repo', head_sha = 'head-b' WHERE relationship_id = ?", rid) + k.exec("INSERT INTO merge_trains (train_id, target_key, repository, base_ref, base_sha, leader_task_id, created_at) VALUES ('trn-done', 'tgt', 'owner/repo', 'dev', 'base-0', 'parent', 't')") + k.exec("INSERT INTO merge_train_members (train_id, seq, turn_id, pr_number, relationship_id, member_head) VALUES ('trn-done', 1, 'turn-1', 5, ?, 'head-b')", rid) + k.exec("INSERT INTO merge_train_events (train_id, seq, kind, actor, detail_json, recorded_at) VALUES ('trn-done', 1, 'landed', 'parent', '{}', 't')") + if _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest); err != nil { + t.Fatalf("a closed bundle blocked the correction: %v", err) + } + }) +} + // Criterion c1, generation 2, d1: the guard resolves the merge turn by every identity a claim may carry. // --relationship and --pr are both optional on merge-turn-request, and a successful merge-turn-check has // already authorized the merge of H before the correction is recorded. A guard that read only the diff --git a/internal/relay/dagsched/revalidation.go b/internal/relay/dagsched/revalidation.go index baa45c7c01..4b73240080 100644 --- a/internal/relay/dagsched/revalidation.go +++ b/internal/relay/dagsched/revalidation.go @@ -512,11 +512,54 @@ func (s *Scheduler) refuseAcceptedHeadOnItsWayToTheBase(ctx context.Context, q s var turn, state, head string found, err := queryOne(ctx, q, "SELECT turn_id, state, candidate_head FROM merge_turns"+ " WHERE state IN ('merging','unknown','landed') AND ("+strings.Join(clauses, " OR ")+") ORDER BY requested_at, turn_id LIMIT 1", args, &turn, &state, &head) - if err != nil || !found { + if err != nil { + return err + } + if found { + return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is on its way to the base already: merge turn %s of the relationship is %s on head %s, and a correction cannot be recorded over a head the forge may already have merged. Resolve that turn first (merge-turn-resolve, or merge-turn-unknown then merge-turn-resolve) and open the generation again", + acc.NodeID, short(turn), state, short(head)) + } + // A live bundle is the second way the head may already be on the base. The parent merges a verified + // bundle on the forge and records it with merge-train-land afterwards, so between those two the + // bundle's members still hold or wait in the lane while the forge already carries the head; the + // bundle's own state cannot be told from one that was never merged, and the relay reads no forge + // here. Recording a correction now would name a head the forge may already have merged, and the + // later train refusal cannot undo that. The bundle is closed first (merge-train-close), which is what + // a parent that found a defect in a bundle review does anyway. + if err := s.refuseLiveBundleCarrying(ctx, q, acc, heads); err != nil { + return err + } + return nil +} + +// refuseLiveBundleCarrying refuses the correction while an opened or verified bundle carries the node's +// stand head as a member: the bundle's merge may already be on the forge, and the relay records that +// merge only when the bundle lands. A bundle that landed, was done or was abandoned carries nothing +// live, so a parent that closed the bundle before correcting a member is not held back. +func (s *Scheduler) refuseLiveBundleCarrying(ctx context.Context, q store.Querier, acc Acceptance, heads []string) error { + present, err := tableExists(ctx, q, "merge_train_members") + if err != nil || !present { return err } - return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is on its way to the base already: merge turn %s of the relationship is %s on head %s, and a correction cannot be recorded over a head the forge may already have merged. Resolve that turn first (merge-turn-resolve, or merge-turn-unknown then merge-turn-resolve) and open the generation again", - acc.NodeID, short(turn), state, short(head)) + for _, head := range heads { + if head == "" { + continue + } + var train string + found, err := queryOne(ctx, q, "SELECT m.train_id FROM merge_train_members m"+ + " WHERE m.relationship_id = ? AND m.member_head = ?"+ + " AND (SELECT kind FROM merge_train_events e WHERE e.train_id = m.train_id ORDER BY e.seq DESC LIMIT 1) IN ('opened','verified')"+ + " ORDER BY m.train_id, m.seq LIMIT 1", []any{acc.RelationshipID, head}, &train) + if err != nil { + return err + } + if !found { + continue + } + return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is on its way to the base already: bundle %s is live and carries head %s as a member, so its merge may already be on the forge and a correction cannot be recorded over it. Close the bundle first (merge-train-close), then open the generation again", + acc.NodeID, short(train), short(head)) + } + return nil } // refuseLanded is the guard of a correction (contract 8.4, E-20): a node whose accepted head landed in every target it lands on is never run again, whatever changed above it and whatever the plan now diff --git a/internal/relay/mergeturn/train_undercorrection_test.go b/internal/relay/mergeturn/train_undercorrection_test.go index 1487ad820f..79fc8ced0c 100644 --- a/internal/relay/mergeturn/train_undercorrection_test.go +++ b/internal/relay/mergeturn/train_undercorrection_test.go @@ -1,6 +1,7 @@ package mergeturn import ( + "context" "database/sql" "fmt" "strings" @@ -160,9 +161,11 @@ func TestTheLaneGateResolvesTheRelationshipFromThePullRequest(t *testing.T) { if !strings.Contains(refusal.Detail, "under correction") || !strings.Contains(refusal.Detail, "rel-lane") { t.Fatalf("the refusal does not name the relationship and the reason: %s", refusal.Detail) } - // a turn whose pull request has no accepted result is left to the lane's own rules + // a turn whose pull request has no accepted result, and whose head no acceptance stands on, is left + // to the lane's own rules other := turn other.PRNumber = sql.NullInt64{Int64: 99, Valid: true} + other.CandidateHead = "head-unaccepted" if refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), other); err != nil || refusal != nil { t.Fatalf("a turn whose pull request has no accepted result = %+v %v, want no refusal", refusal, err) } @@ -354,6 +357,65 @@ func TestTheLaneGateAsksEveryMatchingAcceptance(t *testing.T) { } } +// CRW-906 generation 2, d3: the excluded member's hold must not clear just because the correction was +// CRW-906 generation 2, d3: the recheck inside the write transaction asks about the head the bundle +// CRW-906 generation 2, d1: naming a relationship must not suppress the other identities. Acceptance +// uniqueness is per node and output, so two nodes can accept the same commit: a turn that names the +// relationship which is NOT being corrected is still carrying a head another relationship is repairing, +// and the gate must read that one too rather than trusting the name it was given. +func TestTheLaneGateAsksTheOtherAcceptancesOfANamedRelationship(t *testing.T) { + w := newFx(t) + // rel-named is the relationship the turn names, and it is not under correction + w.ucLaneRelationship("rel-named", 1) + w.exec("INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES ('acc-rel-named', 'plan-x', 'node-named', 'manifest-named', 'rel-named', 1, 'ev-named', 'rev-named', 'crit-1', 'verified', 'head-shared', ?, 1, 'bound', 'turn-1', '{}', ?, 0, '2023-11-14T22:13:19.000000+00:00', 'active')", + fxRepo, alpha.TaskID) + // rel-correcting accepts the same head and has a correction open over it + w.ucLaneRelationship("rel-correcting", 2) + w.exec("UPDATE dag_acceptances SET head_sha = 'head-shared' WHERE relationship_id = 'rel-correcting'") + turn := store.MergeTurnsRow{TurnID: "mtn-named", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, RelationshipID: sql.NullString{String: "rel-named", Valid: true}, CandidateHead: "head-shared", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a turn naming a live relationship but holding a head another one is repairing was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "rel-correcting") { + t.Fatalf("the refusal does not name the correcting relationship: %s", refusal.Detail) + } +} + +// CRW-906 generation 2, d3: the recheck inside the write transaction asks about the head the bundle +// carries, not only whether a correction is open. A correction opened AND accepted over inside the gap +// makes the live and stand generations equal again, so a guard that only asked UnderCorrection would let +// the obsolete bundle through with a verified event for a head the plan no longer accepts. +func TestTrainVerifyRefusesAMemberWhoseAcceptedResultMovedOn(t *testing.T) { + w := newTr(t) + train := w.openedTrain() + w.pr(900, "head-bundle", TrainLaneLabel) + w.forge.runs["run-1"] = runFor("head-bundle") + // the member's accepted result is corrected and accepted over while the run and the chain are being + // proved: the live and stand generations are equal again by the time the write transaction runs, so a + // guard that only asked whether a correction is open would let the obsolete bundle through. The + // mutation runs inside the proof, which is the gap the recheck exists for. + proof := &ucProver{trProof: w.proof, during: func() { + w.exec("UPDATE relationships SET execution_generation = 2 WHERE relationship_id = 'rel-task-m2'") + w.exec("UPDATE dag_acceptances SET execution_generation = 2, head_sha = 'head-m2-corrected' WHERE relationship_id = 'rel-task-m2' AND state = 'active'") + }} + _, err := w.m.Verify(w.ctx, train, trLeader, "900", "head-bundle", "run-1", "/checkout", w.forge, proof) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a verify whose member's accepted result moved on: %v", err) + } + if !strings.Contains(err.Error(), "head-m2-corrected") { + t.Fatalf("the refusal does not name the stand the acceptance moved to: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'verified'"); n != 0 { + t.Fatalf("a refused verify wrote %d verified event(s)", n) + } +} + // CRW-906 generation 2, d3: the excluded member's hold must not clear just because the correction was // accepted over. Exclusion changes the accounting, not the bundle tree: the verified tree still carries the // member head it was verified on, so once the acceptance stands on a corrected head the bundle no longer @@ -391,6 +453,22 @@ func TestTrainLandRefusesAnExcludedMemberWhoseAcceptanceMovedOn(t *testing.T) { } } +// ucProver is the checkout stand-in with a seam: it runs during() inside the chain proof, which is the +// gap between the reads a verify makes and the transaction it writes in. That is where a member's +// accepted result can move without either read seeing it, and what the transaction recheck exists for. +type ucProver struct { + *trProof + during func() +} + +func (p *ucProver) Chain(ctx context.Context, checkout, head, base string, members []TrainMemberExpectation) (TrainChain, error) { + chain, err := p.trProof.Chain(ctx, checkout, head, base, members) + if err == nil && p.during != nil { + p.during() + } + return chain, err +} + // ucCount is the number of rows a query answers, for the writes a refusal must not leave behind. func (w *fx) ucCount(query string, args ...any) int64 { w.t.Helper() diff --git a/internal/relay/mergeturn/undercorrection.go b/internal/relay/mergeturn/undercorrection.go index 1c45f43a26..dd24aa3fe2 100644 --- a/internal/relay/mergeturn/undercorrection.go +++ b/internal/relay/mergeturn/undercorrection.go @@ -50,52 +50,76 @@ func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeT candidates := []string{} if r.RelationshipID.Valid && r.RelationshipID.String != "" { candidates = append(candidates, r.RelationshipID.String) - } else { - // The DAG zone arrives with the first write open (D-01), and the lane serves projects that - // have none: a store that predates the table has no accepted result for this turn, which is - // absence and not an error. - present, err := ucZoneTable(ctx, q, "dag_acceptances") - if err != nil { - return nil, err - } - if !present { - return nil, nil + } + // The turn's own selectors are not the only identities that matter. Acceptance uniqueness is per + // node and output, so two accepted nodes can name the same commit or the same pull request: a turn + // that names one relationship can still be carrying a head another relationship is repairing. The + // identities a claim may carry are resolved here whether or not the turn named a relationship, so a + // named relationship adds to the set rather than replacing it. + // + // The DAG zone arrives with the first write open (D-01), and the lane serves projects that have + // none: a store that predates the table has no accepted result for this turn, which is absence and + // not an error. + present, err := ucZoneTable(ctx, q, "dag_acceptances") + if err != nil { + return nil, err + } + if present { + queries := []struct { + query string + args []any + }{} + if r.PRNumber.Valid { + // the acceptance's recorded forge identity maps the pull request back to its relationship + queries = append(queries, struct { + query string + args []any + }{"SELECT DISTINCT a.relationship_id FROM dag_acceptance_forge f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id" + + " WHERE lower(f.forge_repository) = lower(?) AND f.pr_number = ? AND a.state = 'active'", + []any{r.Repository, r.PRNumber.Int64}}) } - var query string - var args []any - switch { - case r.PRNumber.Valid: - // the turn names the pull request: the acceptance's recorded forge identity maps it back - query = "SELECT DISTINCT a.relationship_id FROM dag_acceptance_forge f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id" + - " WHERE lower(f.forge_repository) = lower(?) AND f.pr_number = ? AND a.state = 'active'" - args = []any{r.Repository, r.PRNumber.Int64} - case r.CandidateHead != "": - // the turn names neither selector: the head it holds is the only identity left, and the - // acceptance that recorded it is what the head belongs to. A recorded base refresh moves - // the head the acceptance stands on without changing the accepted head (dag_base_refreshes - // carries the refreshed head), so both are read: the head the turn holds is the accepted - // head or the head of a refresh of it, and either belongs to the same acceptance. - query = "SELECT DISTINCT a.relationship_id FROM dag_acceptances a" + + if r.CandidateHead != "" { + // the head the turn holds belongs to the acceptance that recorded it. A recorded base refresh + // moves the head the acceptance stands on without changing the accepted head + // (dag_base_refreshes carries the refreshed head), so both are read. + query := "SELECT DISTINCT a.relationship_id FROM dag_acceptances a" + " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND a.head_sha = ?" - args = []any{r.Repository, r.CandidateHead} - if present, err := ucZoneTable(ctx, q, "dag_base_refreshes"); err != nil { + args := []any{r.Repository, r.CandidateHead} + if refreshed, err := ucZoneTable(ctx, q, "dag_base_refreshes"); err != nil { return nil, err - } else if present { + } else if refreshed { query = "SELECT DISTINCT a.relationship_id FROM dag_acceptances a" + " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND a.head_sha = ?" + " UNION ALL SELECT a.relationship_id FROM dag_base_refreshes f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id" + " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND f.head_sha = ?" args = []any{r.Repository, r.CandidateHead, r.Repository, r.CandidateHead} } - default: - return nil, nil + queries = append(queries, struct { + query string + args []any + }{query, args}) } - found, err := ucRelationships(ctx, q, query, args...) - if err != nil { - return nil, err + for _, one := range queries { + found, err := ucRelationships(ctx, q, one.query, one.args...) + if err != nil { + return nil, err + } + candidates = append(candidates, found...) } - candidates = found } + if len(candidates) == 0 { + return nil, nil + } + seen := map[string]bool{} + unique := candidates[:0] + for _, relationship := range candidates { + if relationship == "" || seen[relationship] { + continue + } + seen[relationship] = true + unique = append(unique, relationship) + } + candidates = unique for _, relationship := range candidates { under, live, stand, err := acceptance.UnderCorrection(ctx, q, relationship) if err != nil { @@ -163,14 +187,33 @@ func trainUnderCorrectionRefusal(pr int64, relationship string, liveGeneration, // read each member's acceptance before the transaction that writes, and a generation opened in that gap // leaves the earlier reading stale; this is the same question asked again inside the write. func trainMemberCorrectionRefusal(ctx context.Context, q store.Querier, pr int64, relationship, memberHead string) error { - under, live, stand, err := acceptance.UnderCorrection(ctx, q, relationship) + // the question is about the head the bundle carries, not only about a correction being open: a + // correction that is opened AND accepted over inside the gap moves the acceptance to the corrected + // head, which makes the live and stand generations equal again, so UnderCorrection alone would let + // the obsolete bundle through. The stand head is compared with the head the bundle froze. + active, found, err := acceptance.ActiveForRelationship(ctx, q, relationship) + if err != nil { + return trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) + } + if !found { + return trainConflict("pull request %d's relationship %s has no active acceptance, so the member is no longer a verified, accepted candidate", pr, pyvalue.StrRepr(relationship)) + } + stand, err := acceptance.StandOf(ctx, q, active.AcceptanceID, relationship, active.Generation, active.EventID, active.RevisionHash, active.HeadSHA) + if err != nil { + return trainUnreadable("what acceptance %s stands on was not read: %v", pyvalue.StrRepr(active.AcceptanceID), err) + } + if stand.Head != "" && !SameCommit(stand.Head, memberHead) { + return trainConflict("pull request %d's relationship %s stands on %s and the bundle carries %s for it, so the member's accepted result moved while the bundle was being read: rebuild and verify the bundle on the current result", + pr, pyvalue.StrRepr(relationship), pyvalue.StrRepr(stand.Head), pyvalue.StrRepr(memberHead)) + } + under, live, standGeneration, err := acceptance.UnderCorrection(ctx, q, relationship) if err != nil { return trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) } if !under { return nil } - return trainUnderCorrectionRefusal(pr, relationship, live, stand) + return trainUnderCorrectionRefusal(pr, relationship, live, standGeneration) } // trainExcludedMemberRefusal is CRW-906's guard on a member whose turn left the lane, and it answers the From bbb6cc8891f76183219680288308e2bd32f140db Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Thu, 8 Oct 2026 03:04:45 +0900 Subject: [PATCH 11/28] CRW-906: match the forge identity, ask a member's own gate, and refuse a replaced head The pre-merge evaluation of 141c105f8 found three more ways the promise of decision 2 (a node under correction is not merged on its old head) could still be broken. d1: the bundle gate read only the relationship named on each member. Two accepted nodes can name the same commit or the same pull request, so a member whose own relationship is live can still carry a head another relationship is repairing. Every member is now asked through the lane's own gate with its own identities, so the bundle applies the same rule the single lane does. d2: after dag-accept --supersedes the old head belongs to no active acceptance, so a turn held for it escaped every lookup. A turn holding a head an acceptance recorded and that a later acceptance replaced is now refused, naming the relationship. d3: the new lookups matched dag_acceptances.repository, but an acceptance written before the forge rule keeps whatever target it was accepted against, a local checkout included, while dag_acceptance_forge holds the owner/name a merge turn is requested against. Both the lane gate and the in-flight guard match on the forge identity now, falling back to the acceptance's own repository when no forge row exists. d4: the refusals told the coordinator to open the generation again, which would leave the unrecorded generation behind. They now say to record this same generation once the bundle is closed or the turn resolved, and docs/relay/dag-scheduler.md states that precondition. --- docs/relay/dag-scheduler.md | 2 + internal/relay/dagsched/revalidation.go | 4 +- internal/relay/mergeturn/train.go | 6 +- .../mergeturn/train_undercorrection_test.go | 90 ++++++++++++ internal/relay/mergeturn/undercorrection.go | 132 +++++++++++++----- 5 files changed, 195 insertions(+), 39 deletions(-) diff --git a/docs/relay/dag-scheduler.md b/docs/relay/dag-scheduler.md index 71d23b8aa8..4eab300f38 100644 --- a/docs/relay/dag-scheduler.md +++ b/docs/relay/dag-scheduler.md @@ -203,6 +203,8 @@ A blocking defect can be found in a result that the plan already accepted and th The accepted head is held back while the correction is open, not only after the child pushes. The merge gate reads the relationship's live execution generation against the generation its active acceptance stands on: while the live one is later, the node is **under correction** and its old head is not a merge candidate. The single-lane judgement (`dag-merge-judge`, `dag-merge-request`) is refused `disposition_conflict` naming the open generation and writes no row and no turn, and the bundle open, verify and land refuse a member the same way (see [Merge eligibility](#merge-eligibility)). Without this, a bundle or a lane turn taken between opening (or recording) the generation and the child's first push would carry and land exactly the head the correction is repairing, because until that push the pull request still shows the accepted head. The gate is about the correction being open, not about the head: `dag-accept --supersedes` of the new generation's result moves the acceptance to the stand generation and the node is judged again, a withdrawn generation puts the live generation back and the old head may ride again, and a generation recorded by [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) moves the stand generation with the live one, so a base refresh is not a correction. +Recording is refused while the accepted head may already be on its way to the base: a merge turn of the node is `merging`, of unknown effect or `landed` (resolve it first, `merge-turn-resolve`), or a live bundle (opened or verified) carries the node's stand head as a member (close it first, `merge-train-close`). Neither refusal is a dead end and neither opens a generation: the generation opened in step 2 is still unrecorded, so the coordinator closes the bundle or resolves the turn and then repeats step 3 with the same manifest digest. Preparing again and opening another generation would leave the unrecorded one behind, and the next correction would be refused for not following it. + What notes the route is the generation's own `reason` column: no column, table, schema, refusal name, CLI flag or `specs.json` entry is added. A generation opened under a reason that states no correction (the assignment itself, a returning tenure, a reply) is refused by the same step, naming the reason it carries, and a generation opened for a base refresh is not bound here either: [`dag-base-refresh`](#a-base-refresh-of-an-accepted-node) records that one. A generation opened for this route and never sent is withdrawn like any other ([A generation opened by hand and never sent](#a-generation-opened-by-hand-and-never-sent)). ### Input checks diff --git a/internal/relay/dagsched/revalidation.go b/internal/relay/dagsched/revalidation.go index 4b73240080..b3fd30b4b0 100644 --- a/internal/relay/dagsched/revalidation.go +++ b/internal/relay/dagsched/revalidation.go @@ -516,7 +516,7 @@ func (s *Scheduler) refuseAcceptedHeadOnItsWayToTheBase(ctx context.Context, q s return err } if found { - return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is on its way to the base already: merge turn %s of the relationship is %s on head %s, and a correction cannot be recorded over a head the forge may already have merged. Resolve that turn first (merge-turn-resolve, or merge-turn-unknown then merge-turn-resolve) and open the generation again", + return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is on its way to the base already: merge turn %s of the relationship is %s on head %s, and a correction cannot be recorded over a head the forge may already have merged. Resolve that turn first (merge-turn-resolve, or merge-turn-unknown then merge-turn-resolve), then record this same generation again with the manifest digest dag-correct --prepare already printed: it is not yet recorded, and opening another generation would skip it", acc.NodeID, short(turn), state, short(head)) } // A live bundle is the second way the head may already be on the base. The parent merges a verified @@ -556,7 +556,7 @@ func (s *Scheduler) refuseLiveBundleCarrying(ctx context.Context, q store.Querie if !found { continue } - return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is on its way to the base already: bundle %s is live and carries head %s as a member, so its merge may already be on the forge and a correction cannot be recorded over it. Close the bundle first (merge-train-close), then open the generation again", + return refuse(contract.RefusalDispositionConflict, "the accepted result of %s is on its way to the base already: bundle %s is live and carries head %s as a member, so its merge may already be on the forge and a correction cannot be recorded over it. Close the bundle first (merge-train-close), then record this same generation again with the manifest digest dag-correct --prepare already printed: it is not yet recorded, and opening another generation would skip it", acc.NodeID, short(train), short(head)) } return nil diff --git a/internal/relay/mergeturn/train.go b/internal/relay/mergeturn/train.go index 8be0d194f2..90c963794c 100644 --- a/internal/relay/mergeturn/train.go +++ b/internal/relay/mergeturn/train.go @@ -346,7 +346,7 @@ func (s *Service) Open(ctx context.Context, turn, actor, base string, members [] // reads above happen before it, and a generation opened in that gap would otherwise be recorded // as an opened train: nothing is written for a member whose accepted result is under correction. for _, m := range expectations { - if e := trainMemberCorrectionRefusal(tx, s.Store.Querier(tx), m.PRNumber, m.RelationshipID, m.AcceptedHead); e != nil { + if e := trainMemberCorrectionRefusal(tx, s.Store.Querier(tx), row.Repository, m.PRNumber, m.RelationshipID, m.AcceptedHead); e != nil { return e } } @@ -744,7 +744,7 @@ func (s *Service) Verify(ctx context.Context, train, actor, bundlePR, head, run, // expectations above were read before the run and the chain were proved, and a generation opened // in that gap would otherwise be recorded as verified: nothing is written for such a member. for _, m := range expected { - if e := trainMemberCorrectionRefusal(tx, s.Store.Querier(tx), m.PRNumber, m.RelationshipID, m.AcceptedHead); e != nil { + if e := trainMemberCorrectionRefusal(tx, s.Store.Querier(tx), row.Repository, m.PRNumber, m.RelationshipID, m.AcceptedHead); e != nil { return e } } @@ -997,7 +997,7 @@ func (s *Service) TrainLand(ctx context.Context, train, actor, landed, observed // left the lane is excluded above and is not re-read, so its parent revoking the acceptance // does not refuse the rest of the bundle. for _, m := range leftMembers { - if e := trainExcludedMemberRefusal(tx, s.Store.Querier(tx), m.PRNumber, m.RelationshipID, m.MemberHead); e != nil { + if e := trainExcludedMemberRefusal(tx, s.Store.Querier(tx), row.Repository, m.PRNumber, m.RelationshipID, m.MemberHead); e != nil { return e } } diff --git a/internal/relay/mergeturn/train_undercorrection_test.go b/internal/relay/mergeturn/train_undercorrection_test.go index 79fc8ced0c..c81a2ebdb2 100644 --- a/internal/relay/mergeturn/train_undercorrection_test.go +++ b/internal/relay/mergeturn/train_undercorrection_test.go @@ -359,6 +359,96 @@ func TestTheLaneGateAsksEveryMatchingAcceptance(t *testing.T) { // CRW-906 generation 2, d3: the excluded member's hold must not clear just because the correction was // CRW-906 generation 2, d3: the recheck inside the write transaction asks about the head the bundle +// CRW-906 generation 2, d1: naming a relationship must not suppress the other identities. Acceptance +// CRW-906 generation 2: a bundle member is refused when the head it carries is the result another +// relationship is repairing, even though the member's own relationship is live. Acceptance uniqueness is +// per node and output, so two accepted nodes can name the same commit, and the bundle must apply the same +// rule the single lane does. +func TestTrainRefusesAMemberWhoseHeadAnotherRelationshipIsRepairing(t *testing.T) { + w := newTr(t) + train := w.openedTrain() + w.pr(900, "head-bundle", TrainLaneLabel) + w.forge.runs["run-1"] = runFor("head-bundle") + w.ucOtherCorrection("rel-other", "head-m2") + _, err := w.m.Verify(w.ctx, train, trLeader, "900", "head-bundle", "run-1", "/checkout", w.forge, w.proof) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a member whose head another relationship is repairing: %v", err) + } + if !strings.Contains(err.Error(), "rel-other") { + t.Fatalf("the refusal does not name the repairing relationship: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'verified'"); n != 0 { + t.Fatalf("a refused verify wrote %d verified event(s)", n) + } +} + +// ucOtherCorrection writes another relationship's active acceptance on a head with a correction +// generation open over it: the shape the bundle gate must see through a member's own live relationship. +func (w *tr) ucOtherCorrection(relationship, head string) { + w.t.Helper() + allowed := fmt.Sprintf("[%q]", trLeader) + w.exec("INSERT INTO relationships (relationship_id, issue_key, status, parent_task_id, parent_host_id, child_task_id, child_host_id, execution_generation, artifact_roots, allowed_recipients, created_at, updated_at)"+ + " VALUES (?, 'ISS-1', 'active', ?, 'host-a', 'child-x', 'host-c', 2, '[]', ?, '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')", + relationship, trLeader, allowed) + w.exec("INSERT INTO generations (relationship_id, execution_generation, dispatch_request_id, anchor_state, dispatch_turn_id, reason, opened_at, bound_at)"+ + " VALUES (?, 2, ?, 'bound', 'turn-dispatch-x', 'needs_changes_revision', '2026-10-01T00:00:00Z', '2026-10-01T00:00:00Z')", + relationship, "correction-"+relationship) + w.exec("INSERT INTO dag_acceptances (acceptance_id, plan_id, node_id, manifest_digest, relationship_id, execution_generation, event_id, revision_hash, criteria_set_digest, verdict, head_sha, repository, pr_number, ack_tier, verdict_turn_id, rule_version_json, accepted_by_task_id, coordinator_epoch, accepted_at, state)"+ + " VALUES (?, 'plan-x', 'node-x', 'manifest-x', ?, 1, 'ev-x', 'rev-x', 'crit-x', 'verified', ?, ?, 1, 'bound', 'turn-x', '{}', ?, 0, '2026-10-01T00:00:00Z', 'active')", + "acc-"+relationship, relationship, head, trRepo, trLeader) + w.exec("INSERT INTO dag_acceptance_forge (acceptance_id, forge_repository, pr_number) VALUES (?, 'owner/repo', 1)", "acc-"+relationship) +} + +// CRW-906 generation 2: an acceptance recorded before the forge rule keeps whatever target it was +// accepted against — a local checkout included — while dag_acceptance_forge holds the owner/name a merge +// turn is actually requested against. The gate matches the forge identity, not the stored target. +func TestTheLaneGateMatchesTheForgeIdentityNotTheStoredTarget(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-local", 2) + w.ucLaneForge("rel-local", 7) + w.exec("UPDATE dag_acceptances SET repository = '/synthetic/checkout' WHERE relationship_id = 'rel-local'") + for _, turn := range []store.MergeTurnsRow{ + {TurnID: "mtn-by-pr", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", PRNumber: sql.NullInt64{Int64: 7, Valid: true}, State: Holding}, + {TurnID: "mtn-by-head", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding}, + } { + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("%s: a turn matched by the forge identity rather than the stored target was not refused: %+v", turn.TurnID, refusal) + } + if !strings.Contains(refusal.Detail, "rel-local") { + t.Fatalf("%s: the refusal does not name the relationship: %s", turn.TurnID, refusal.Detail) + } + } +} + +// CRW-906 generation 2: after dag-accept --supersedes the head an acceptance recorded belongs to no +// active acceptance any more, so a turn held for it would otherwise merge exactly the result the +// correction replaced. +func TestTheLaneGateRefusesAHeadAnAcceptanceRecordedAndThenReplaced(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + w.ucLaneForge("rel-lane", 7) + // the acceptance that recorded head-a was replaced: a later one stands on the corrected head now + w.exec("UPDATE dag_acceptances SET state = 'superseded' WHERE relationship_id = 'rel-lane'") + turn := store.MergeTurnsRow{TurnID: "mtn-replaced", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a turn holding a replaced head was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "rel-lane") || !strings.Contains(refusal.Detail, "replaced") { + t.Fatalf("the refusal does not name the relationship and what happened: %s", refusal.Detail) + } +} + // CRW-906 generation 2, d1: naming a relationship must not suppress the other identities. Acceptance // uniqueness is per node and output, so two nodes can accept the same commit: a turn that names the // relationship which is NOT being corrected is still carrying a head another relationship is repairing, diff --git a/internal/relay/mergeturn/undercorrection.go b/internal/relay/mergeturn/undercorrection.go index dd24aa3fe2..e32869a9a8 100644 --- a/internal/relay/mergeturn/undercorrection.go +++ b/internal/relay/mergeturn/undercorrection.go @@ -79,19 +79,31 @@ func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeT []any{r.Repository, r.PRNumber.Int64}}) } if r.CandidateHead != "" { - // the head the turn holds belongs to the acceptance that recorded it. A recorded base refresh + // The head the turn holds belongs to the acceptance that recorded it. A recorded base refresh // moves the head the acceptance stands on without changing the accepted head // (dag_base_refreshes carries the refreshed head), so both are read. + // + // The repository an acceptance is matched by is its FORGE identity where one was recorded + // (dag_acceptance_forge), because an acceptance written before the forge rule keeps whatever + // target it was accepted against — a local checkout included — while only the forge row names + // the owner/name a merge turn is requested against. An acceptance with no forge row is + // matched by its own repository, so a store that predates the table is unchanged. + identity := "lower(a.repository)" + if hasForge, err := ucZoneTable(ctx, q, "dag_acceptance_forge"); err != nil { + return nil, err + } else if hasForge { + identity = "lower(COALESCE((SELECT g.forge_repository FROM dag_acceptance_forge g WHERE g.acceptance_id = a.acceptance_id), a.repository))" + } query := "SELECT DISTINCT a.relationship_id FROM dag_acceptances a" + - " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND a.head_sha = ?" + " WHERE a.state = 'active' AND " + identity + " = lower(?) AND a.head_sha = ?" args := []any{r.Repository, r.CandidateHead} if refreshed, err := ucZoneTable(ctx, q, "dag_base_refreshes"); err != nil { return nil, err } else if refreshed { query = "SELECT DISTINCT a.relationship_id FROM dag_acceptances a" + - " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND a.head_sha = ?" + + " WHERE a.state = 'active' AND " + identity + " = lower(?) AND a.head_sha = ?" + " UNION ALL SELECT a.relationship_id FROM dag_base_refreshes f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id" + - " WHERE a.state = 'active' AND lower(a.repository) = lower(?) AND f.head_sha = ?" + " WHERE a.state = 'active' AND " + identity + " = lower(?) AND f.head_sha = ?" args = []any{r.Repository, r.CandidateHead, r.Repository, r.CandidateHead} } queries = append(queries, struct { @@ -107,9 +119,6 @@ func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeT candidates = append(candidates, found...) } } - if len(candidates) == 0 { - return nil, nil - } seen := map[string]bool{} unique := candidates[:0] for _, relationship := range candidates { @@ -120,6 +129,21 @@ func underCorrectionRefusal(ctx context.Context, q store.Querier, r store.MergeT unique = append(unique, relationship) } candidates = unique + // A head that some relationship accepted and then moved off is not the head the plan accepts for it + // any more. dag-accept --supersedes replaces the acceptance, so the old head no longer belongs to any + // ACTIVE acceptance and the active-only lookups above cannot see it: a turn that was held for the old + // head would otherwise proceed to merge exactly the result the correction replaced. The acceptance + // that recorded the head is read whatever its state, and a turn holding it is refused. + if r.CandidateHead != "" && len(candidates) == 0 { + if replaced, err := ucReplacedHead(ctx, q, r.Repository, r.CandidateHead); err != nil { + return nil, err + } else if replaced != "" { + return coordination(r, contract.RefusalDispositionConflict, + "the head this turn holds is no longer the accepted result of its relationship "+pyvalue.StrRepr(replaced)+ + ": an acceptance of it was replaced by a later one (dag-accept --supersedes), so the result the plan accepts has moved and this turn would merge the result that was replaced. Request the turn for the head the acceptance stands on now", + r.CandidateHead, r.HolderTaskID), nil + } + } for _, relationship := range candidates { under, live, stand, err := acceptance.UnderCorrection(ctx, q, relationship) if err != nil { @@ -159,6 +183,34 @@ func ucRelationships(ctx context.Context, q store.Querier, query string, args .. return out, rows.Err() } +// ucReplacedHead names a relationship whose acceptance recorded this head and was then replaced, so the +// head is no longer the result the plan accepts for it. "" means no acceptance ever recorded the head. +// It reads the acceptance whatever its state: a superseded or revoked one is exactly the case, because +// the active-only lookups above cannot see it. A store that predates the forge table falls back to the +// acceptance's own repository. +func ucReplacedHead(ctx context.Context, q store.Querier, repository, head string) (string, error) { + if head == "" { + return "", nil + } + identity := "lower(a.repository)" + if hasForge, err := ucZoneTable(ctx, q, "dag_acceptance_forge"); err != nil { + return "", err + } else if hasForge { + identity = "lower(COALESCE((SELECT g.forge_repository FROM dag_acceptance_forge g WHERE g.acceptance_id = a.acceptance_id), a.repository))" + } + query := "SELECT a.relationship_id FROM dag_acceptances a" + + " WHERE " + identity + " = lower(?) AND a.head_sha = ? ORDER BY a.accepted_at DESC, a.acceptance_id DESC LIMIT 1" + args := []any{repository, head} + var relationship string + if err := q.QueryRowContext(ctx, query, args...).Scan(&relationship); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return "", nil + } + return "", err + } + return relationship, nil +} + // trainUnderCorrectionRefusal is the bundle member's refusal: the member's relationship has a correction // ucZoneTable reports whether the store holds the named DAG zone table. The zone arrives with the first // write open (D-01), so a store that predates it holds no acceptance and the lane's turn is left to the @@ -183,37 +235,45 @@ func trainUnderCorrectionRefusal(pr int64, relationship string, liveGeneration, } // trainMemberCorrectionRefusal re-reads one member's correction state on the caller's querier and refuses -// the member when a correction generation is open over its accepted result (CRW-906). Open and verify -// read each member's acceptance before the transaction that writes, and a generation opened in that gap -// leaves the earlier reading stale; this is the same question asked again inside the write. -func trainMemberCorrectionRefusal(ctx context.Context, q store.Querier, pr int64, relationship, memberHead string) error { - // the question is about the head the bundle carries, not only about a correction being open: a - // correction that is opened AND accepted over inside the gap moves the acceptance to the corrected - // head, which makes the live and stand generations equal again, so UnderCorrection alone would let - // the obsolete bundle through. The stand head is compared with the head the bundle froze. - active, found, err := acceptance.ActiveForRelationship(ctx, q, relationship) - if err != nil { +// the member when the head the bundle carries is not a head the plan still accepts for it (CRW-906). Open +// and verify read each member's acceptance before the transaction that writes, and a generation opened in +// that gap leaves the earlier reading stale; this is the same question asked again inside the write. +// +// The question is asked through the lane's own gate, with the member's own identities, because a member is +// not identified by its relationship alone: acceptance uniqueness is per node and output, so another +// accepted node can name the same pull request or the same commit, and a bundle member whose relationship +// is live can still carry a head another relationship is repairing. The gate resolves every matching +// acceptance and asks each one, which is the same rule the single lane applies. +func trainMemberCorrectionRefusal(ctx context.Context, q store.Querier, repository string, pr int64, relationship, memberHead string) error { + // A member whose own relationship still holds an active acceptance is also checked against the head + // the plan accepts for it: a correction that was opened AND accepted over inside the gap moves the + // acceptance to the corrected head, which makes the live and stand generations equal again, so the + // generation comparison alone would let the obsolete bundle through. + if active, found, err := acceptance.ActiveForRelationship(ctx, q, relationship); err != nil { return trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) + } else if found { + stand, err := acceptance.StandOf(ctx, q, active.AcceptanceID, relationship, active.Generation, active.EventID, active.RevisionHash, active.HeadSHA) + if err != nil { + return trainUnreadable("what acceptance %s stands on was not read: %v", pyvalue.StrRepr(active.AcceptanceID), err) + } + if stand.Head != "" && !SameCommit(stand.Head, memberHead) { + return trainConflict("pull request %d's relationship %s stands on %s and the bundle carries %s for it, so the member's accepted result moved while the bundle was being read: rebuild and verify the bundle on the current result", + pr, pyvalue.StrRepr(relationship), pyvalue.StrRepr(stand.Head), pyvalue.StrRepr(memberHead)) + } } - if !found { - return trainConflict("pull request %d's relationship %s has no active acceptance, so the member is no longer a verified, accepted candidate", pr, pyvalue.StrRepr(relationship)) - } - stand, err := acceptance.StandOf(ctx, q, active.AcceptanceID, relationship, active.Generation, active.EventID, active.RevisionHash, active.HeadSHA) - if err != nil { - return trainUnreadable("what acceptance %s stands on was not read: %v", pyvalue.StrRepr(active.AcceptanceID), err) - } - if stand.Head != "" && !SameCommit(stand.Head, memberHead) { - return trainConflict("pull request %d's relationship %s stands on %s and the bundle carries %s for it, so the member's accepted result moved while the bundle was being read: rebuild and verify the bundle on the current result", - pr, pyvalue.StrRepr(relationship), pyvalue.StrRepr(stand.Head), pyvalue.StrRepr(memberHead)) - } - under, live, standGeneration, err := acceptance.UnderCorrection(ctx, q, relationship) + refusal, err := underCorrectionRefusal(ctx, q, store.MergeTurnsRow{ + Repository: repository, + RelationshipID: sql.NullString{String: relationship, Valid: relationship != ""}, + PRNumber: sql.NullInt64{Int64: pr, Valid: pr > 0}, + CandidateHead: memberHead, + }) if err != nil { - return trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) + return trainUnreadable("the correction state of pull request %d was not read: %v", pr, err) } - if !under { + if refusal == nil { return nil } - return trainUnderCorrectionRefusal(pr, relationship, live, standGeneration) + return trainConflict("%s", refusal.Detail) } // trainExcludedMemberRefusal is CRW-906's guard on a member whose turn left the lane, and it answers the @@ -227,7 +287,7 @@ func trainMemberCorrectionRefusal(ctx context.Context, q store.Querier, pr int64 // bundle's tree still carries the defective head it was verified on, so landing it would land exactly the // result the correction repaired. Once the acceptance stands on a head the bundle does not carry, the // bundle no longer describes what the plan accepts for that member and must be rebuilt and verified. -func trainExcludedMemberRefusal(ctx context.Context, q store.Querier, pr int64, relationship, memberHead string) error { +func trainExcludedMemberRefusal(ctx context.Context, q store.Querier, repository string, pr int64, relationship, memberHead string) error { active, found, err := acceptance.ActiveForRelationship(ctx, q, relationship) if err != nil { return trainUnreadable("the acceptance of relationship %s was not read: %v", pyvalue.StrRepr(relationship), err) @@ -244,5 +304,9 @@ func trainExcludedMemberRefusal(ctx context.Context, q store.Querier, pr int64, return trainConflict("pull request %d's relationship %s now stands on %s and the bundle carries %s for it, so this bundle no longer holds the result the plan accepts for that member: rebuild and verify the bundle on the current result before landing it", pr, pyvalue.StrRepr(relationship), pyvalue.StrRepr(stand.Head), pyvalue.StrRepr(memberHead)) } - return trainMemberCorrectionRefusal(ctx, q, pr, relationship, memberHead) + // the same question the surviving members get: the excluded member's code is still in the merge + // commit, so a correction open over the head the bundle carries for it refuses the landing, whoever + // opened it. The carve-out above still lets a revoked acceptance or a moved pull request leave the + // rest of the bundle alone. + return trainMemberCorrectionRefusal(ctx, q, repository, pr, relationship, memberHead) } From f1c1141337be33c7d1b14300a8ae363a6f33bdfd Mon Sep 17 00:00:00 2001 From: thisisjun786 <259586770+thisisjun786@users.noreply.github.com> Date: Thu, 8 Oct 2026 04:26:47 +0900 Subject: [PATCH 12/28] CRW-906: hold a correction's accepted head in every merge path, and fix the links to dag-scheduler Round 6 of the correction review. The lane's gates and the bundle's landing now resolve the correction state of every member, the refreshed heads of an acceptance survive dag-accept --supersedes, and a legacy acceptance is matched by its forge identity in the in-flight guard. The coordinator's skill references describe the hand-opened correction route for an accepted, current result. - train.go: TrainLand re-checks the surviving members' correction state inside the landing transaction. - undercorrection.go: a replaced head keeps the heads recorded by dag_base_refreshes. - revalidation.go: the in-flight guard matches the acceptance's forge identity. - correction_forge_identity_test.go: red first on the previous head, green after. - train_undercorrection_test.go: lane and landing cases for these identities. - plugins/crw/skills/crw-run references: the accepted-result correction route; plugin.json re-recorded. --- .../correction_forge_identity_test.go | 37 +++++++++ internal/relay/dagsched/revalidation.go | 14 +++- internal/relay/mergeturn/train.go | 5 ++ .../mergeturn/train_undercorrection_test.go | 77 +++++++++++++++++++ internal/relay/mergeturn/undercorrection.go | 15 +++- plugins/crw/.codex-plugin/plugin.json | 2 +- .../references/dispatch-verification.md | 4 +- .../crw-run/references/merge-readiness.md | 28 ++++--- .../skills/crw-run/references/operations.md | 4 +- 9 files changed, 167 insertions(+), 19 deletions(-) create mode 100644 internal/relay/dagsched/correction_forge_identity_test.go diff --git a/internal/relay/dagsched/correction_forge_identity_test.go b/internal/relay/dagsched/correction_forge_identity_test.go new file mode 100644 index 0000000000..f21eb5ccb4 --- /dev/null +++ b/internal/relay/dagsched/correction_forge_identity_test.go @@ -0,0 +1,37 @@ +package dagsched + +import ( + "context" + "strings" + "testing" +) + +// CRW-906 generation 2, round 6 d3: an acceptance written before the forge rule keeps the target it was +// accepted against (a local checkout here) while dag_acceptance_forge holds the owner/name a merge turn is +// requested against. The in-flight guard must match the forge identity. A pull-request turn that names no +// relationship and is already merging is otherwise missed, and the correction would be recorded over a head +// the forge may already have merged. +func TestACorrectionIsNotRecordedOverALegacyAcceptanceTurnOfItsForgeRepository(t *testing.T) { + t.Parallel() + k, accepted := rvSettledSharedRoot(t) + rid := accepted["B"].RelationshipID + prepared := k.rvPrepare("sr", "B") + acOpenByHand(t, k, rid, prepared.DispatchRequestID, "needs_changes_revision", 2, true) + // the stored target is a local checkout; the forge identity is the owner/name the turn is requested against + k.exec("UPDATE dag_acceptances SET repository = '/synthetic/checkout', head_sha = 'head-b', pr_number = 5 WHERE relationship_id = ?", rid) + k.exec("DELETE FROM dag_acceptance_forge WHERE acceptance_id = (SELECT acceptance_id FROM dag_acceptances WHERE relationship_id = ?)", rid) + k.exec("INSERT INTO dag_acceptance_forge (acceptance_id, forge_repository, pr_number) VALUES ((SELECT acceptance_id FROM dag_acceptances WHERE relationship_id = ?), 'owner/repo', 5)", rid) + // a merging turn of the forge repository for the pull request, naming neither a relationship nor the head + k.exec("INSERT INTO merge_turns (turn_id, target_key, repository, base_ref, project_key, holder_task_id, holder_host_id, relationship_id, pr_number, candidate_head, state, tenure, requested_at, updated_at)" + + " VALUES ('mtn-forge', 'tgt', 'owner/repo', 'dev', 'P-TEST', 'parent', 'host', NULL, 5, 'head-other', 'merging', 1, 't', 't')") + _, err := k.sched.RecordCorrection(context.Background(), "sr", "B", "parent", prepared.ManifestDigest) + if refusalReason(err) != "disposition_conflict" { + t.Fatalf("a correction over a merging turn of the legacy acceptance's forge repository = %v, want disposition_conflict", err) + } + if !strings.Contains(err.Error(), "mtn-forge") { + t.Fatalf("the refusal does not name the turn: %v", err) + } + if got := acExecution(k, "B", 2); got != "" { + t.Fatalf("the refused correction wrote the execution %q", got) + } +} diff --git a/internal/relay/dagsched/revalidation.go b/internal/relay/dagsched/revalidation.go index b3fd30b4b0..8510e022dc 100644 --- a/internal/relay/dagsched/revalidation.go +++ b/internal/relay/dagsched/revalidation.go @@ -498,16 +498,26 @@ func (s *Scheduler) refuseAcceptedHeadOnItsWayToTheBase(ctx context.Context, q s } clauses := []string{"relationship_id = ?"} args := []any{acc.RelationshipID} + // The turn's repository predicate is the acceptance's FORGE identity where one was recorded: an + // acceptance written before the forge rule keeps whatever target it was accepted against, a local + // checkout included, while dag_acceptance_forge names the owner/name a merge turn is requested + // against. Without this a PR-only or bare-head turn of a legacy acceptance is never found. + forge := acc.Repository + if has, err := queryOne(ctx, q, "SELECT forge_repository FROM dag_acceptance_forge WHERE acceptance_id = ?", []any{acc.AcceptanceID}, &forge); err != nil { + return err + } else if !has { + forge = acc.Repository + } for _, head := range heads { if head == "" { continue } clauses = append(clauses, "(lower(repository) = lower(?) AND candidate_head = ?)") - args = append(args, acc.Repository, head) + args = append(args, forge, head) } if acc.PRNumber > 0 { clauses = append(clauses, "(lower(repository) = lower(?) AND pr_number = ?)") - args = append(args, acc.Repository, acc.PRNumber) + args = append(args, forge, acc.PRNumber) } var turn, state, head string found, err := queryOne(ctx, q, "SELECT turn_id, state, candidate_head FROM merge_turns"+ diff --git a/internal/relay/mergeturn/train.go b/internal/relay/mergeturn/train.go index 90c963794c..cc6f8f87f3 100644 --- a/internal/relay/mergeturn/train.go +++ b/internal/relay/mergeturn/train.go @@ -996,6 +996,11 @@ func (s *Service) TrainLand(ctx context.Context, train, actor, landed, observed // or moved after open refuses the landing with nothing written (Blocking 1). A member that // left the lane is excluded above and is not re-read, so its parent revoking the acceptance // does not refuse the rest of the bundle. + for _, m := range survivors { + if e := trainMemberCorrectionRefusal(tx, s.Store.Querier(tx), row.Repository, m.PRNumber, m.RelationshipID, m.MemberHead); e != nil { + return e + } + } for _, m := range leftMembers { if e := trainExcludedMemberRefusal(tx, s.Store.Querier(tx), row.Repository, m.PRNumber, m.RelationshipID, m.MemberHead); e != nil { return e diff --git a/internal/relay/mergeturn/train_undercorrection_test.go b/internal/relay/mergeturn/train_undercorrection_test.go index c81a2ebdb2..75033ecd99 100644 --- a/internal/relay/mergeturn/train_undercorrection_test.go +++ b/internal/relay/mergeturn/train_undercorrection_test.go @@ -293,6 +293,58 @@ func TestTheLaneGateResolvesARefreshedHead(t *testing.T) { } } +// CRW-906 generation 2: a member whose turn left the lane is excluded from the landing, but its code is +// CRW-906 generation 2, round 6 d2: a recorded base refresh outlives the acceptance it was recorded for, +// so a turn held for a refreshed head must still resolve once dag-accept --supersedes replaced the +// acceptance: otherwise the obsolete result merges after the correction was accepted over. +func TestTheLaneGateRefusesAReplacedHeadThatWasOnceRefreshed(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-lane", 2) + w.ucLaneForge("rel-lane", 7) + // the acceptance stood on head-a, a base refresh moved it to head-refreshed, and then the whole + // acceptance was replaced by the corrected one + refresh := "dbr-" + strings.Repeat("b", 60) + w.exec("INSERT INTO dag_base_refreshes (refresh_id, acceptance_id, refresh_seq, relationship_id, execution_generation, event_id, revision_hash, head_sha, base_repository, base_ref, base_tip_sha, proof_json, resolved_paths_json, recorded_by_task_id, coordinator_epoch, recorded_at)"+ + " VALUES (?, ?, 1, ?, 1, 'ev-refresh', 'rev-refresh', 'head-refreshed', ?, ?, 'base-0', '{}', '[]', ?, 0, '2023-11-14T22:13:20.000000+00:00')", + refresh, "acc-rel-lane", "rel-lane", fxRepo, fxBase, alpha.TaskID) + w.exec("UPDATE dag_acceptances SET state = 'superseded' WHERE relationship_id = 'rel-lane'") + turn := store.MergeTurnsRow{TurnID: "mtn-refreshed-replaced", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-refreshed", State: Holding} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a turn holding a refreshed head whose acceptance was replaced was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "rel-lane") { + t.Fatalf("the refusal does not name the relationship: %s", refusal.Detail) + } +} + +// CRW-906 generation 2, round 6 d3: an acceptance written before the forge rule keeps whatever target it +// was accepted against (a local checkout included) while dag_acceptance_forge holds the owner/name a merge +// turn is requested against. The in-flight guard must match the forge identity too, not the stored target. +func TestTheInFlightGuardMatchesTheForgeIdentityNotTheStoredTarget(t *testing.T) { + w := newFx(t) + w.ucLaneRelationship("rel-local", 2) + w.ucLaneForge("rel-local", 7) + w.exec("UPDATE dag_acceptances SET repository = '/synthetic/checkout' WHERE relationship_id = 'rel-local'") + // a turn of the real forge repository, naming neither selector, already merging on the accepted head + turn := store.MergeTurnsRow{TurnID: "mtn-legacy", TargetKey: "tgt-x", Repository: fxRepo, BaseRef: fxBase, ProjectKey: fxA, + HolderTaskID: alpha.TaskID, CandidateHead: "head-a", State: Merging} + refusal, err := underCorrectionRefusal(w.ctx, w.s.Querier(w.ctx), turn) + if err != nil { + t.Fatal(err) + } + if refusal == nil || refusal.Reason != contract.RefusalDispositionConflict { + t.Fatalf("a legacy acceptance's forge turn was not refused: %+v", refusal) + } + if !strings.Contains(refusal.Detail, "rel-local") { + t.Fatalf("the refusal does not name the relationship: %s", refusal.Detail) + } +} + // CRW-906 generation 2: a member whose turn left the lane is excluded from the landing, but its code is // still in the merge commit the bundle lands (every member head is checked as an ancestor of it), so the // bundle must not record a landing of a tree that still carries a result the plan is repairing. The @@ -360,6 +412,31 @@ func TestTheLaneGateAsksEveryMatchingAcceptance(t *testing.T) { // CRW-906 generation 2, d3: the excluded member's hold must not clear just because the correction was // CRW-906 generation 2, d3: the recheck inside the write transaction asks about the head the bundle // CRW-906 generation 2, d1: naming a relationship must not suppress the other identities. Acceptance +// CRW-906 generation 2: a bundle member is refused when the head it carries is the result another +// CRW-906 generation 2, round 6 d1: the landing transaction must apply the same identity-based gate the +// open and verify use. A correction of another relationship that accepts the same head, opened after the +// bundle was verified, is refused at land: otherwise the landing records the exact result being repaired. +func TestTrainLandRefusesAMemberWhoseHeadAnotherRelationshipIsRepairing(t *testing.T) { + w := newTr(t) + train := w.verifiedTrain() + w.ucOtherCorrection("rel-other", "head-m2") + w.tip.set(trRepo, trBase, "merge-1") + w.forge.commits["merge-1"] = TrainCommit{SHA: "merge-1", Parents: []string{"base-0", "head-bundle"}, Tree: "tree-bundle"} + _, err := w.m.TrainLand(w.ctx, train, trLeader, "merge-1", "", w.tip, w.forge) + if err == nil || trReason(err) != "disposition_conflict" { + t.Fatalf("a land whose member's head another relationship is repairing: %v", err) + } + if !strings.Contains(err.Error(), "rel-other") { + t.Fatalf("the refusal does not name the repairing relationship: %v", err) + } + if n := w.count("SELECT count(*) FROM merge_train_events WHERE kind = 'landed'"); n != 0 { + t.Fatalf("a refused land wrote %d landed event(s)", n) + } + if n := w.count("SELECT count(*) FROM merge_turns WHERE state = 'landed'"); n != 0 { + t.Fatalf("a refused land landed %d turn(s)", n) + } +} + // CRW-906 generation 2: a bundle member is refused when the head it carries is the result another // relationship is repairing, even though the member's own relationship is live. Acceptance uniqueness is // per node and output, so two accepted nodes can name the same commit, and the bundle must apply the same diff --git a/internal/relay/mergeturn/undercorrection.go b/internal/relay/mergeturn/undercorrection.go index e32869a9a8..fb3c5257b7 100644 --- a/internal/relay/mergeturn/undercorrection.go +++ b/internal/relay/mergeturn/undercorrection.go @@ -198,9 +198,22 @@ func ucReplacedHead(ctx context.Context, q store.Querier, repository, head strin } else if hasForge { identity = "lower(COALESCE((SELECT g.forge_repository FROM dag_acceptance_forge g WHERE g.acceptance_id = a.acceptance_id), a.repository))" } + // A head the acceptance once stood on through a recorded base refresh is covered too: the refresh + // rows outlive the acceptance they were recorded for, and a turn held for a refreshed head would + // otherwise resolve to nothing once dag-accept --supersedes replaced the acceptance. query := "SELECT a.relationship_id FROM dag_acceptances a" + - " WHERE " + identity + " = lower(?) AND a.head_sha = ? ORDER BY a.accepted_at DESC, a.acceptance_id DESC LIMIT 1" + " WHERE " + identity + " = lower(?) AND a.head_sha = ?" args := []any{repository, head} + if refreshed, err := ucZoneTable(ctx, q, "dag_base_refreshes"); err != nil { + return "", err + } else if refreshed { + query = "SELECT a.relationship_id FROM dag_acceptances a" + + " WHERE " + identity + " = lower(?) AND a.head_sha = ?" + + " UNION ALL SELECT f.relationship_id FROM dag_base_refreshes f JOIN dag_acceptances a ON a.acceptance_id = f.acceptance_id" + + " WHERE " + identity + " = lower(?) AND f.head_sha = ?" + args = []any{repository, head, repository, head} + } + query = "SELECT relationship_id FROM (" + query + ") ORDER BY relationship_id LIMIT 1" var relationship string if err := q.QueryRowContext(ctx, query, args...).Scan(&relationship); err != nil { if errors.Is(err, sql.ErrNoRows) { diff --git a/plugins/crw/.codex-plugin/plugin.json b/plugins/crw/.codex-plugin/plugin.json index baf135861b..796d4328ab 100644 --- a/plugins/crw/.codex-plugin/plugin.json +++ b/plugins/crw/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "crw", - "version": "0.4.0+e7af5fae7c4a", + "version": "0.4.0+5b95fa972142", "description": "Linear-driven workflow skills for Codex: define an initiative, plan projects and issues, run delivery, and check the result against its canonical criteria.", "author": { "name": "thisisjun786", diff --git a/plugins/crw/skills/crw-run/references/dispatch-verification.md b/plugins/crw/skills/crw-run/references/dispatch-verification.md index 3e8e30d68f..7463b64675 100644 --- a/plugins/crw/skills/crw-run/references/dispatch-verification.md +++ b/plugins/crw/skills/crw-run/references/dispatch-verification.md @@ -196,14 +196,14 @@ a case whose owner has moved or been reworded before relying on it. | S25d | A correction generation, a base-refresh generation and a separated publication step are not the first work-phase of new work, so they do not open `LOOP-DOCS-FIRST-01`'s docs-only cycle, and the CXC rule text is unchanged | [Default independent execution](../../crw-plan/references/integrations.md#default-independent-execution), the Loop bullet of the [Launch packet](task-packet.md#launch-packet) and the workflow bullet of the [Restoration block](task-packet.md#restoration-block) | Default settings | added 2026-10-03; decides the cases S25c left open, from S25c's goalplan record, and no child reading the line was observed, see below | | S26 | A task that has to stop its own tests, a child or the parent, stops them by the pid it recorded when it started them or by the process group it created for them, and never by pattern or name (`pkill -f`, `killall`); a long command records its pid from the start or runs under `timeout`, and a process the task did not start is reported with its pid and working directory, not stopped | The `Processes you start:` line of the [Launch packet](task-packet.md#launch-packet) and its line in [First full assignment required fields](task-packet.md#first-full-assignment-required-fields); for the parent, [Observe and verify](../SKILL.md#observe-and-verify) and [Check CI for this candidate](merge-readiness.md#check-ci-for-this-candidate) | Preserved limits | added 2026-10-03; the mechanics were measured on this host's own processes, no child or parent reading the wording has been observed, see below | | S27 | Devin Review and the Codex review are references and not merge gates: a candidate merges when every CI job succeeds on one and the same head, the coordinator verified it by its usual procedure (it read the diff and the code and reran the tests the criteria rest on) and the local gates pass; the merge waits for no Devin or Codex status or review, neither inheriting an earlier review by patch-id nor a substitute review comment is required, and a later head with no review of its own is normal | [The three gates](merge-readiness.md#the-three-gates) | Merge gate | added 2026-10-03; basis below | -| S27b | The child waits for the one run of each reviewer within the waiting budget before it emits: a Devin status `Analyzing your changes` is waited for up to the budget and `Full review skipped: trial expired and no credits remaining` is not; Codex is waited for while its eyes reaction is on the pull request or any row of its summary comment is not `Completed`, and each of its code review and security review must be `Completed` or skipped by the bot's notice (a thumbs-up counts for both when no row contradicts it), so a Security Review row `Completed` alone is not finished; past the budget a run still going is recorded as pending, not complete, and the wait is released only where the candidate's required independent review and its finding dispositions already exist; when neither reviewer shows any signal 30 minutes after the pull request became ready or was opened, the handoff says `review unavailable (no signal)` and the child goes on, and when only one is silent the same 30 minutes apply to it and the handoff says so; a thread that arrives after the receipt is a late finding: unless the parent has dispositioned it for that head (`--late-dispositions`) it returns the candidate before the acceptance, and after the acceptance no correction can reach the child and the candidate is held ([Late review threads](merge-readiness.md#late-review-threads)) | [The one run of each reviewer, awaited before the receipt](merge-readiness.md#the-one-run-of-each-reviewer-awaited-before-the-receipt) and [Recheck, integrate, and record](merge-readiness.md#recheck-integrate-and-record) and [a late thread the parent dispositions itself](merge-readiness.md#a-late-thread-the-parent-dispositions-itself) | Merge gate | changed with CRW-808; the unlimited wait became the waiting budget with a pending record | +| S27b | The child waits for the one run of each reviewer within the waiting budget before it emits: a Devin status `Analyzing your changes` is waited for up to the budget and `Full review skipped: trial expired and no credits remaining` is not; Codex is waited for while its eyes reaction is on the pull request or any row of its summary comment is not `Completed`, and each of its code review and security review must be `Completed` or skipped by the bot's notice (a thumbs-up counts for both when no row contradicts it), so a Security Review row `Completed` alone is not finished; past the budget a run still going is recorded as pending, not complete, and the wait is released only where the candidate's required independent review and its finding dispositions already exist; when neither reviewer shows any signal 30 minutes after the pull request became ready or was opened, the handoff says `review unavailable (no signal)` and the child goes on, and when only one is silent the same 30 minutes apply to it and the handoff says so; a thread that arrives after the receipt is a late finding: unless the parent has dispositioned it for that head (`--late-dispositions`) it returns the candidate before the acceptance, and after the acceptance the correction goes through the hand-opened generation [Late review threads](merge-readiness.md#late-review-threads) describes, while a node that landed is held ([Late review threads](merge-readiness.md#late-review-threads)) | [The one run of each reviewer, awaited before the receipt](merge-readiness.md#the-one-run-of-each-reviewer-awaited-before-the-receipt) and [Recheck, integrate, and record](merge-readiness.md#recheck-integrate-and-record) and [a late thread the parent dispositions itself](merge-readiness.md#a-late-thread-the-parent-dispositions-itself) | Merge gate | changed with CRW-808; the unlimited wait became the waiting budget with a pending record | | S27c | The Devin status is read by its description and never by its state: `Completed analysis in