Report a replacement and its record as two outcomes, and re-read the swap gate on resume (CRW-93) - #63
Conversation
The staging claim is written last, after the selection is committed and the owned pointer is placed and read back, so a host already reaches the new runtime by the time writing it can fail. Both places that settle it let that failure escape. In cmd_install it reached the handler for failures this command does not model and was reported as an update failure: applied false, a null pointer for a link that had in fact been swapped, and a destination that "cannot be retried until the selection moves" -- when rerunning the same command is exactly the repair. In _finish_promotion it escaped the command altogether, through a finally with no except, leaving no result and no exit status at all for a resume that had already written the pointer. _settle_claim now answers instead of raising, and both callers report the record beside the replacement rather than in place of it. "promoted" stays the replacement, "claimSettled" is the record of it, and "recoveryRequires" -- the same key a refusal reports it under -- says what the next run must do. A third exit status, EXIT_INCOMPLETE, carries the same distinction for a caller that reads only the status. That advice is checked against behaviour rather than wording. A promotion that could not settle leaves STAGING over a selection and a pointer that both name the environment, which staging.decide() already reads as RESUME, so the next run writes the record and rebuilds nothing; the regression test runs it and asserts on what happened. The release rule the meta-check enforces gains the second promoted exit for the reason the first one is exempt: past the promotion the directory is not a leftover, it is the installation a host reaches.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
Codex review, PR #63: when write_claim replaces the claim and then raises while releasing its lock, the bytes are already on disk and the result still reported claimSettled false, prescribed a repair for bookkeeping that was already correct, and exited 3. That is the defect this branch exists to fix, one layer down: a cleanup failure standing in for the write failure. So the record gets the same split the result got, rather than a branch beside it. _settle_claim now answers with two outcomes decided by two readings: "settled" is the record landing, read back from the claim and never inferred from the exception, and "released" is the call finishing. What failed afterwards is named as itself -- a release that fails leaves the lock file it could not unlink, and that file is not cosmetic, because the next claim write at this path waits on it and then refuses until it is gone or older than STALE_LOCK_SECONDS. It is read on the filesystem, reported in residualPaths, and carried into the recovery sentence the way _install_failed already carries its own residue. The readback also decides the advice, because the next run's behaviour differs. A readable claim that never settled is repaired by rerunning: staging.decide() answers RESUME. A claim that cannot be read is not one this command may act on, so it answers KEEP and rerunning alone repairs nothing; that case says to make the claim readable first and says plainly that the environment is in service and must not be deleted. Settlement fails closed on an unusable reading. Both new cases are red at 7641e40 with AssertionError: the landed-record case reports settled false beside a readback saying COMPLETE, and the unreadable case is given the advice that only fits the case a rerun does repair.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ns here Devin review, PR #63, two findings on the same contract. The recovery advice prescribed a rerun without naming what has to be true for it to work. If whatever stopped the claim write is still there, the next run reads RESUME, reaches _settle_claim again and returns the same answer, so the operator is told to repeat something that cannot yet succeed. The sentence now names the write failure as the thing to clear first, points at 'detail' for what it was, and says plainly that rerunning before then reaches the same failure and returns the same result. It also says what makes that harmless: the rerun rebuilds nothing and removes nothing. test_rerunning_while_the_write_still_fails_loses_nothing pins that: the same status, claimSettled still false, the selection unchanged, the pointer still naming the candidate, the build still there and the predecessor still there. The second is the contract EXIT_INCOMPLETE introduces, and a wrapper author reads the constant rather than the result. Non-zero here means the opposite of what it means everywhere else in this command: the candidate was promoted, it is selected, the pointer names it, and a host is running out of it. A caller treating every non-zero install status as "nothing happened, clean it up" would delete the runtime in service, so the constant now says so, and says the release path is reached only by EXIT_REFUSED while these two statuses keep the environment deliberately. The operator-procedure paragraph for status 3 is not written here. docs/runtime-install.md is held by CRW-100 PR #52, so the proposed text goes to the coordinator for placement and that review thread stays open until it does.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
Codex review, PR #63. hostrecord.Busy is an OSError, so catching the broad type alone read a COMPETING writer's live lock file as this run's leftover: it went into residualPaths and recoveryRequires told the operator to deal with it. Locked excludes writers by O_EXCL on that filename, so acting on that advice admits a second writer into a read-modify-write that is still running. The result was advising the accident. Busy is now caught before the broad arm and answered as what it is. Nothing was written, the lock belongs to somebody else, residualPaths is empty because this run left nothing, and the next action is to wait for the run that holds it and rerun -- with the reason stated, that taking the lock away would let a second writer in. The claim is still read back, because the run this one lost the lock to may be the run that settled this very promotion. The other arm stops prescribing removal too. Whether a lock file present after some other failure outlived its taker or belongs to a live writer cannot be established from here, so it is reported rather than condemned, with the fact that settles it either way: Locked clears a lock older than STALE_LOCK_SECONDS by itself, so nothing needs a hand at all. test_a_competing_writers_lock_is_never_called_this_runs_residue is red at 2c29b04 with AssertionError -- the live lock listed in residualPaths -- and it contrasts the two cases directly, so the contended answer cannot drift back into the one written for a lock this run really could have stranded.
|
@codex review |
|
@codex review |
…ing a retry Codex review, PR #63, two findings on the same exit. _finish_promotion can return EXIT_INCOMPLETE too, and its result carried 'applied' and 'claimSettled' but never 'promoted'. A consumer using that marker to tell "the runtime is in service, leave it alone" from "nothing happened, clean it up" found nothing on the resume path -- the exact misreading the constant warns about, on the one exit that does not report it. It reports it now, the same way the ordinary promotion does. The recovery advice was also composed from a selection this run committed rather than one it read. Settling deliberately runs outside the promotion lock, so an install queued on that lock can promote its own environment between this run releasing it and the advice being written. "Rerun and it will finish the bookkeeping" is then false in the expensive direction: with the selection moved, staging.decide() reads this directory as an abandoned staging and answers RECLAIM, so following the advice removes and rebuilds it. _settle_claim now re-reads the host record and derives the advice from what it says at that moment, in the three answers the rest of this module uses. Still selected keeps the existing advice. Moved on says plainly not to rerun here, and why. A record that could not be read says to read it first, because whether a rerun records or rebuilds is exactly what it decides. Both are red at 97f7497 with AssertionError. The second compares the two cases with the temporary paths normalised out -- compared raw they differ by path alone, which is a test that passes for the wrong reason, and it did until the normalisation went in.
|
@codex review |
Devin review, PR #63. The previous commit read the selection alone and called anything it did not name abandoned. staging.decide() takes two inputs, not one: 'selected' authorises finishing a promotion, and 'protected' keeps a directory something may still reach. A promotion writes the selection before it moves the pointer, so one that dies between them leaves the record naming somewhere else while the host still reaches this environment through the pointer -- and that directory is KEPT, not reclaimed. The advice promised a reclaim the next run would never perform. _settle_claim now reads the same pair, through protected_environment, and the branches follow staging.decide()'s own order: an unreadable claim is answered before the state is consulted at all, and only then do selected and protected choose between finishing the bookkeeping, leaving the directory alone, and removing and rebuilding it. An unreadable record leaves both None and says to read the record first rather than guessing which of the three it is. test_a_directory_the_pointer_still_names_is_not_called_abandoned drives the real shape -- the selection moves, the pointer does not -- and asserts the rerun answers KEEP, then contrasts the advice against the genuinely abandoned case. Red at c7ac9e0 with AssertionError.
|
@codex review |
Codex review, PR #63. hostrecord.load() fills an ABSENT record with an EMPTY one, and ABSENT is usable -- so a record that disappeared between the promotion and the claim write read back as a record selecting nothing, which is exactly what a record superseded by another run's promotion looks like. This command then reported a lost host record as "another run moved the selection on" and told the operator nothing needed doing. They are different events. Being superseded means a later promotion succeeded and this staging is genuinely past. A record that is gone means the authority for what this host selected was lost, so the promotion this run made cannot be confirmed or finished by anything until it is restored, and the next run keeps the directory rather than repairing it. The state of the reading is consulted now rather than only its usability: 'ok' selects the branch that reads a selection at all, and ABSENT gets its own answer naming restoration as the action, with the warning not to remove the environment because the owned pointer may still reach it. test_a_host_record_that_vanished_is_not_reported_as_a_selection_that_moved contrasts it against the record that merely moved on, with temporary paths normalised out. Red at 7927b5e with AssertionError.
|
@codex review |
…nt service Review, PR #63. The previous commit made inService a three-valued answer, and that was wrong in the one direction this cell may not be wrong in. inService is exported as "do not release this destination". A snapshot that could not be taken does not establish an environment is out of service -- the promotion placed and read back its pointer before any of this ran -- and JSON null is falsey in most things that will read it, so an unreadable safety check became permission to clean up. It is false only where the readings say so now, which is the shape protected_environment already uses and for the same reason: the cost of keeping a directory is a report, the cost of releasing a live one is the accident. And the documented status table promised more than a status can. A competing install can supersede this environment between the promotion and the result, so "status 3 means in service" contradicted a result correctly reporting inService false. The rows now say what the RUN did -- its change to the host landed, its record did not -- and the text says plainly that the status does not promise what is true when you read it, that a cleanup decision keys on inService and never on the status alone. Red at 5f9e8a4 with AssertionError: inService null for a snapshot that could not be taken.
|
@codex review |
Review, PR #63. staging.decide() answers KEEP for every COMPLETE claim and says why: it is a runtime that was promoted once, and a process started from it may still be running out of it. inService was left to the selection and the pointer alone, so a successful update whose environment a competing install superseded before the snapshot reported inService false -- and the contract this branch documents points a wrapper at exactly that cell for its cleanup decision. A compliant wrapper would have deleted what this command refuses to delete. A settled claim now keeps the destination on its own, which is the same rule staging.decide() already applies, asserted here against decide() itself rather than restated: the test settles the claim, supersedes the environment, and requires both that decide() answers KEEP and that the result agrees. Red at 3ee9454 with AssertionError: settled true, inService false.
|
@codex review |
…s rule Review, PR #63. Three findings arrived in a row about inService and they were one defect wearing three hats: a settled claim, an unreadable claim, a snapshot nobody could take. Each time the keep rule was restated here, each time the restatement disagreed with staging.decide() -- which is what actually removes directories -- and the documented contract points a wrapper at this cell, so every disagreement was a wrapper deleting something this command refuses to. Patching the fourth instance would only move the disagreement. So the guard is decide()'s own answer now. REMOVES is the declared set of decisions that remove anything and inService is its complement, asked with the liveness a LATER run would see, because that is whose behaviour it describes. The two cannot drift apart again, and where they still disagree the disagreement is in decide() where it can be argued about -- which is the once-promoted STAGING case reported separately and deliberately left open. The snapshot handler also caught OSError alone. hostrecord.shape() checks that 'selected' is a table, not what its values are, so a record this command ACCEPTS can carry a truthy non-path that protected_environment hands to Path(), raising TypeError; it escaped into the generic failure path and reported exit 1 for a promotion whose selection, pointer and claim had all landed. It catches reading.READ_FAILURES now, which is the set reading already declares for exactly this. The fixture for that one also has to supersede the selection, and that is not decoration: protected_environment folds the entries with any(), which short-circuits on the first match, so a selection still naming this environment never reaches the bad value. Only a superseded one does. Both red at 0396e97 with AssertionError.
…cement Review, PR #63. The exit table described 0 and 3 as a runtime changed by this run, but _finish_promotion returns them for legacy adoption too -- where the caller emits promoted false, because the runtime was already selected and only its bookkeeping was adopted. The table and the "the candidate was promoted" sentence both overstated that path. The column is what this RUN changed now, and a paragraph names the three shapes that reach it: promoting a candidate replaces the selected runtime, finishing an interrupted promotion places a pointer a dead run never wrote, and adopting an installation older than claims replaces nothing at all. All three land on 0 or 3 by the same rule -- whether the claim settled -- and 'promoted' is what distinguishes them.
|
@codex review |
…and stop the exit-3 paragraph guaranteeing service Review, PR #63. Both are the same shape again: an arm asserting state it never consulted. The unreadable-claim advice told the operator to repair or remove the claim and rerun, while asserting the environment is in service. decide() does keep such a directory either way -- a claim it cannot read is not one it may act on -- but what follows from the prescribed repair depends on the selection. Rewritten as STAGING over a selection that has moved on, the next run reads an abandoned staging and rebuilds; removed, the directory is populated and claimless and reads as somebody else's from then on. That arm branches on the selection now and says to leave it alone when nothing selects this environment any more. And the exit-3 paragraph still guaranteed the environment is the one the record selects and the pointer names, which the paragraph after it contradicts: a competing promotion during the claim write produces exit 3 with selects false, pointerNames false and inService false. The guarantee is gone; the status says the run's change landed, inService says whether the destination must be kept, and the text points a cleanup decision at the second. Red at 9e954af with AssertionError.
|
@codex review |
… asks Review, PR #63. Three findings, all in the arms added last commit. 'selected is not True' folded None into False, so a selection that could not be established was told what a positively superseded one is told: leave the unreadable claim alone. That is wrong if this environment is in fact still selected, where the claim has to be repaired before a rerun can finish anything. Unknown gets its own answer now, naming both repairs and which reading decides between them. And the arms promising a bookkeeping-only rerun asked recordSelectsIt, which protected_environment folds with any(). One component inside this environment and the others elsewhere reads as selected -- rightly, for keeping the directory -- while _finish_promotion requires EVERY configured component through _names_environment and refuses. So those arms ask _names_environment now, and the partly-selected state gets an answer of its own saying a resume refuses and what actually needs deciding. The recovery table in the documentation is split the same three ways, because it had one row where the code now has three. Both new cases are red at be4ac76 with AssertionError.
# Conflicts: # scripts/runtime_install.py
CRW-93 Half A. The path that takes over an interrupted promotion moved the owned pointer without asking OPS-4.4 at all. What it takes over is DURABLE: a selection on disk and a claim beside it, sitting there for however long it took somebody to notice. All three gate cells read state outside this process and all three move while that state sits still. A supervisor can be started. Attempts open and close continuously. The store's schema is whatever the selected runtime has since migrated it to. So none of them may be carried across the interruption, which CRW-92 settled for the daemon in its strongest form: a prior ALLOWED cannot cross a process boundary. There was nothing to carry either, and that is the sharper statement of the defect. The interrupted run died before recording any verdict, so the durable state holds no gate reading of any kind -- this path was not reusing a stale ALLOWED, it was moving a host's runtime having never asked. The candidate's own declared schema is the single input that cannot have changed, being derived from bytes already built, but it is only ever read as half of a comparison against a store that can, so even that buys no reuse. No condition is reusable. Asked where something is replaced, which is not every caller. A resume finds a link naming the predecessor and moves a host from it to this environment: that is a swap. An installation older than claims has no link at all, and writing the first one changes which PATH reaches a runtime the record already selects rather than which runtime is reached -- which is what its own result has always said. The test is the link rather than the caller: a link that already names this environment, or no link, replaces nothing. A verdict that is not ALLOWED refuses by name, carrying the verdict and the cells that blocked or could not answer, because "the gate said no" sends an operator to read this command's source while the cell sends them to the daemon, the attempts or the store. It refuses BEFORE the ownership entry, which is this call's first write, so nothing is written and nothing is removed: the selection is left as found, the pointer still names what it named, and the destination is retriable as it stands. ResumeGateTests is red at 40243ec with AssertionError on the defect itself -- the pointer moved from the predecessor to the candidate under a blocked gate -- across all three blocking cells and the unreadable one. The retriable claim is proved by rerunning with the cell readable and requiring the promotion to finish, not by asserting a flag. The adoption case is asserted to stay ungated. PointerOwnershipLifetimeTests calls _finish_promotion directly and now stubs the gate: that case is about the ownership binding rule, and asked for real against a relay that was never built every cell answers UNESTABLISHED, so the refusal would come from the gate rather than the guard under test.
The resume section stated the limit that "neither re-runs the gate conditions", which Half A makes false. It is corrected, and the behaviour it now describes is written out beside the narrower link question that section already covers: why no cell may be carried across an interruption, that there was no verdict to carry in the first place, that the gate is asked where something is replaced rather than for every caller of that path, and what a non-ALLOWED verdict leaves behind. Placed at 788 and 803, in the section that already discusses finishing an interrupted promotion, clear of CRW-101's landed text.
CI cancelled tests (3.13) on its timeout, inside the case that holds the promotion lock across a settle. Not an assertion: the suite grew past 1200 cases when #58 and #64 landed, and several cases here wait out SETTLE_SNAPSHOT_TIMEOUT_SECONDS for real -- five seconds each of wall clock spent proving nothing, because what they assert is the ANSWER on the busy path and never how long it took to give up. The fixture patches the timeout to 0.2s. The busy path is still exercised exactly as before; only the waiting is gone. SettledRecordTests and ResumeGateTests together drop from 10.7s to 2.8s.
CRW-93, both halves. Do not merge without the coordinator: two review threads are open by design (below).
Half B — the record of a replacement is not the replacement
The staging claim is written last, after the selection is committed and the owned pointer is
placed and read back, so by the time writing it can fail the registered command already
resolves into the new environment. Both places that settle it let that failure escape as though
the replacement had failed: in
cmd_installit reached the handler for failures the command doesnot model and reported
applied: falsewith a null pointer and a destination that "cannot beretried", and in
_finish_promotionit escaped through afinallywith noexceptand produced noresult at all.
_settle_claimanswers instead of raising.promotedis the replacement,claimSettledthe record,inServicewhether the destination must be kept, andrecoveryRequireswhat to do next. Writinga claim is itself two steps, so the claim answer carries
settled(read back, never inferred fromthe exception) and
released(whether the call finished), with what failed afterwards named asitself.
EXIT_INCOMPLETE = 3carries the distinction for a status-only caller.The advice is derived, not composed: from the claim as it reads back and a selection snapshot
taken under the promotion lock. It distinguishes a rerun that records, a directory to leave
alone, a superseded staging not to rerun against, a split selection a resume would refuse, an
unreadable claim, a vanished host record, a competing writer holding the lock, and a snapshot
that could not be taken.
inServiceisdecision not in staging.REMOVES— asked ofstaging.decide()itself, so the documented cleanup guard and the code that deletes thingscannot drift apart.
Half A — a resume re-reads the swap gate
The path that takes over an interrupted promotion moved the owned pointer without asking
OPS-4.4 at all. What it takes over is durable — a selection on disk and a claim beside it —
while all three gate cells read state outside the process that moves in the meantime: a
supervisor can be started, attempts open and close, the store's schema is whatever the selected
runtime has since migrated it to.
No condition is reusable, and there was nothing to reuse. The interrupted run died before
recording any verdict, so the durable state holds no gate reading: this path was not carrying a
stale
ALLOWED, it was moving a host's runtime having never asked. CRW-92 settled the daemon inits strongest form — a prior
ALLOWEDcannot cross a process boundary. The candidate's declaredschema is the one input that cannot have changed, being derived from bytes already built, but it
is read only as half of a comparison against a store that can.
The gate is asked where something is replaced, which is not every caller of that path. A
resume finds a link naming the predecessor and moves a host from it to this environment. An
installation older than claims has no link at all, and writing the first one changes which path
reaches a runtime the record already selects rather than which runtime is reached — so it stays
ungated, which is what its own result has always said. The test is the link, not the caller.
A verdict that is not
ALLOWEDrefuses by name, carrying the verdict and the cells that blockedor could not answer, and refuses before the ownership entry — this call's first write — so
nothing is written and nothing removed.
Verification
22b221bRan 6 tests ... FAILED (failures=4), allAssertionError, zero errors40243ecEvery review fix is additionally red at the commit its finding was raised against —
7641e40,2c29b04,97f7497,c7ac9e0,7927b5e,9544ac8,2498763,5f9e8a4,3ee9454,0396e97,be4ac76— withAssertionErroronly. Red-at-parent runsare taken in a separate baseline worktree with only the test file copied in, so a harness
failure cannot be mistaken for a defect.
At head
e02922cd4d51bd4d586fd5080fdb6479b2997ea1, after mergingorigin/dev(which broughtCRW-100 #52 and CRW-101 #62):
python3.14 / 3.13 / 3.10 -m unittest discover -s scripts/ci/testsRan 1020 tests OK(skipped 2, 2, 84)validate.py/plugin.py/contracts.py/git diff --checkThe merge conflicted once, in
protected_environment: both lanes independently added apointer_pathparameter. Resolved to the landed contract, with my caller's reason folded intoits docstring.
PointerOwnershipLifetimeTestsnow stubs the gate where it calls_finish_promotiondirectly, because that case is about the ownership binding rule and anunstubbed gate answers
UNESTABLISHEDagainst a relay that was never built.Not established here: this is source behaviour under injected failure in temporary destinations.
It says nothing about an installed runtime, a live App Server, or any real host.
Open by design
Two threads — one from each reviewer, independently — report that a superseded environment keeps
a
STAGINGclaim, so a later ordinary install of the same combination reachesstaging.decide()with nothing selecting or pointing at it and answers
RECLAIM, deleting a directory a processmay still be running from. The
COMPLETEbranch preserves environments for exactly that reason.It is real and it pre-dates this branch. Closing it means teaching
decide()to tell "neverpromoted" from "promoted once and superseded" for a
STAGINGclaim, which it cannot todaybecause nothing in a staging claim records that a promotion completed. That is a shared
answer-set change with its own red test and review, so it is reported for a follow-up issue
rather than widened into here. This PR makes the reporting honest about it instead:
inServicesays
falsefor that state, becausedecide()really would reclaim.Linear: CRW-93