Repository navigation
CRW-944: runtime-upgrade runs only verified bytes, always restarts, and checks the install target - #873
Merged
Conversation
runtime-upgrade had five defects the post-merge review of PR #702 found: - it hashed the release directory's own archive and then copied that same original, so a swap between the two reads let unverified bytes run. The archive and its SHA256SUMS are now copied into the run directory first and the copy is what is verified, unpacked and executed. - it skipped the service restart when the update broke the pointer. The runtime the pointer names is recorded before the stop, and start is called after it whatever the update did, from the pointer when it still resolves and otherwise from the recorded runtime. - an update failure masked a configuration change. A configuration change now outranks an update failure in the final exit code, and the record keeps both reasons. - the post-check never compared the pointer or the version with what the update produced. It now compares the pointer with the runtime the update reported and that runtime's --version with the version the archive carried. - it read the service status once. It is now re-read every 2 seconds for up to 60 seconds, waiting for running true and matchesRunning same. The open-attempt check reads the relay doctor's contents.openAttempts instead of the store directly, and refuses when the contents cannot be read. The dry-run test that required a second run in the same second to refuse now pins Env.Now in the harness, so a wall-clock second boundary cannot decide it, and a control advances the injected clock to show the next second takes its own run directory.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
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. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8743be7a7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The first external review of this pull request found five gaps in the new behaviour. Each has a test that fails on the previous head: - a pointer can resolve to a directory the service cannot be started from. The restart now tries the pointer's runtime and falls back to the runtime the pointer named before the stop when that start does not succeed, and the record names the runtime the service came back on. - a configuration file the post-check could not read is not a change: it is recorded as a post-check finding, so it no longer takes the exit code from an update failure that would otherwise report it. - every failure names its reasons, including a run that refused before the update, which previously had only the single reason field. - an update that did not land leaves the pointer on the runtime it replaced, whose version is the previous one by design. That is the rollback, not a mismatch, so the version comparison runs only against a runtime the update put in service. - the installer's exit 3 is a promotion whose claim did not settle: the runtime is in service, so the pointer and version are compared with it even though the run is reported as failed.
The independent review of the previous head found two defects in what this change adds: - the run state kept the verified archive's digest and never read it; the sums step already records it in the run record, so the field is gone. - the open-attempt guard refuses both unreadable contents and readable contents that carry no count, and only the first half had a case. The second half now has one, so a regression that read a missing count as zero fails. The duplicated installer statuses stay: the constants name the coupling in a comment, and importing the installer's own constants would add a package dependency this change does not need.
The pre-merge evaluation of the reviewed head found three in-scope defects. The restart's fallback kept the relay up when the runtime the update installed would not start, but the run still reported success: the post-check compared the pointer with the installed runtime and then read the service status from the runtime the restart actually used, without requiring the two to be the same. A pointer on the new runtime with the service running the old one is now a runtime_mismatch. The post-check treated every read failure of the configuration file as no change. A file the snapshot read and that is now gone is a change the run can state exactly, so it is config_changed; a file that still exists but cannot be read stays a post-check finding of its own. A pointer that does not name the runtime the update promoted is a mismatch even when the update itself failed, so the mismatch exit code is no longer hidden by the update failure.
The pre-merge evaluation of the first correction round found two more in-scope defects, both in the restart and post-check. The restart treated a start answer of already_running as a failed launch and fell back to the previous runtime, recording that runtime as the one the service was on even when the service was up on the runtime the update installed. A service already running holds the lock, so no other runtime can start either; already_running now names the runtime the service is on, and the post-check compares that runtime with the installed one. The post-check assumed a failed update had rolled back without checking the pointer still named the runtime the update replaced. An update that puts no runtime in service is a rollback only while the pointer names the previous runtime; a pointer left on any other runtime is a mismatch.
The pre-merge evaluation found that the pointer and version were compared only before the service wait. The wait can run for a minute, and another process on the host can move or remove the pointer while this run waits; the last status answer alone then reported success with the pointer no longer naming the installed runtime. The install-target check now runs before and after the wait, and a mismatch it finds is recorded once. A start that never leaves the service up no longer records the last runtime it tried as the runtime the service came back on.
The doctor names the store the relay service serves in serviceStore when discovery selected another directory; the check ignored it and judged the selected directory's contents, so a served store with open attempts could pass as none. The served store is now asked for its own answer, and an unreadable served store is refused rather than read as empty.
The pre-verification copy read whatever it was given, so a release directory holding something far larger than the archive could fill the state disk the live relay shares before the checksum even ran. The copy now reads at most the installer's own input limit and removes its partial file when it refuses or fails. The wait-budget test now shrinks the budget and drives a service that answers but never settles, so it spends the budget and checks the last answer is recorded; the earlier form only proved the parent context ended.
# Conflicts: # internal/manage/upgrade_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes CRW-944.
crw manage runtime-upgradereplaces the relay runtime from a release archive. The post-mergereview of the earlier upgrade change found five defects in it, plus a decision to settle. This
change closes them, and a later correction round closes four more the pre-merge evaluation found.
The final head is 6b36c52.
What changed
Only the bytes that were verified are executed. The command hashed the release directory's own
archive and then reopened that same original to copy it, so replacing the original between the two
reads changed what was unpacked and run - with the upgrade runner's privileges, in dry runs too.
The archive and its
SHA256SUMSare now copied into the run directory (0700) first, the copy'sdigest is compared with the digest the copied
SHA256SUMSnames, and the copy is what is unpacked,--versioned and passed toinstall update --from. The originals are never reopened.The service always comes back. An update that broke the pointer left the service down:
startread only the new pointer and gave up when it did not resolve. The runtime the pointer names is now
recorded before the stop, and
startis called after it whatever the update did - from the pointerwhen it still resolves, otherwise from the recorded runtime - and the record's
start_fromsayswhich one it was. A start that answers that the service is already running is not a failed launch:
the service holds the lock, so no other runtime can start either, and that answer names the runtime
the service is on rather than a reason to fall back.
A configuration change outranks an update failure.
update_failed(exit 1) used to mask thepost-check's configuration finding (exit 4). The final exit code now prefers the configuration
change, and the record's
reasonslist keeps every reason that applied, so a run that both failedto update and changed the configuration names both. A configuration file the snapshot read and that
is now gone is a change the run can state exactly; a file that still exists but cannot be read is
not a change, because the run cannot tell whether it differs.
The post-check is compared with the install target. It used to accept any non-empty
--version.It now remembers the version the unpacked
crw --versionprinted and the runtime directory theupdate's own answer reported, and refuses with exit 4
runtime_mismatchwhen the pointer does notname that directory, when the update names none, or when the pointer's
crw --versiondiffers. Anupdate that put no runtime in service is a rollback only while the pointer still names the runtime
it replaced; a pointer left on any other runtime is a mismatch. The comparison runs before and
again after the service wait, because the wait can run for a minute and another process on the host
can move the pointer while this run waits. A restart that fell back to the previous runtime while
the pointer names the installed one is that same disagreement, so it is a mismatch rather than a
success.
The service status is waited for. It was read once, so a worker that finished its policy write
a moment later was treated as a failure. After
startthe status is re-read every 2 seconds for upto 60 seconds, waiting for
runningtrue andmatchesRunning same; the last answer is recordedotherwise.
Open attempts come from the relay doctor. The step used to run its own SQL against the store.
It now asks the running runtime's own
codex-session-relay doctorand readscontents.openAttempts: 0 proceeds, more than 0 is exit 3, and contents that are unavailable orcarry no count are refused rather than read as an absence. The direct store SQL and the store import
are gone.
One test is no longer decided by the wall clock.
TestUpgradeDryRunCallsNoServiceCommandrequired the second of two dry runs in the same wall-clock second to refuse, so a second boundary
between them made it fail. The harness now injects
Env.Now, both runs fall in the second the testchooses, and a control advances the injected clock by one second and shows the second run taking its
own run directory. The product's run-directory rule is unchanged.
Acceptance criteria and evidence
The issue's criteria, each with the check that pins it:
TestUpgradeReview702SwappedArchiveIsNotRunreplaces the releasedirectory's archive once the copied archive's digest has been compared, asserts the replacement
really took, and shows the run still unpacks the archive it verified while the swapped archive's
crwnever runs. On the baseline the same replacement is what the run unpacks and runs.TestUpgradeReview702StartRunsWhenUpdateBreaksThePointerhas the fake updateremove the
currentlink and exit 1;service startis still called, from the runtime thepointer named before the stop, and
start_fromrecords it.TestUpgradeReview702ServiceAlreadyRunningIsNotAFailurepins the other side: a start that answersalready_runningon the installed runtime is the service being up there, not a failure.TestUpgradeReview702ConfigChangeWinsOverUpdateFailure- update exit 1plus a configuration change gives exit 4 and a record naming both
update_failedandconfig_changed;TestUpgradeReview702ConfigDeletionWinsOverUpdateFailurepins the deletion caseand
TestUpgradeReview702UnreadableConfigIsNotAChangethe unreadable one.TestUpgradeReview702StalePointerIsAMismatch- three sub-cases: the pointerleft on the previous runtime, an update that names no runtime, and an installed runtime reporting
another version. Each is exit 4
runtime_mismatch.TestUpgradeReview702IncompletePromotionIsCheckedcovers the installer's exit-3 promotion,
TestUpgradeReview702PointerMovedToAThirdRuntimeIsAMismatcha failed update whose pointer names neither the previous nor the installed runtime, and
TestUpgradeReview702PointerBrokenDuringTheWaitIsAMismatcha pointer removed while the run waitsfor the service.
TestUpgradeReview702FailedUpdateIsNotAVersionMismatchguards the correctrollback so it is not turned into a failure.
TestUpgradeReview702SlowWorkerIsWaitedFor- the status answersunknown,unknown,same; the run reads it three times and exits 0.TestUpgradeReview702ServiceWaitBudgetIsRecordedpins the other half: a wait that ends without theservice ever reporting itself up records the failure rather than passing.
TestUpgradeReview702OpenAttemptsFromDoctor-contents.openAttempts2is exit 3 and the doctor was asked at the runtime the pointer names;
availablefalse is refused(exit 2), and readable contents with no count are refused too.
TestUpgradeDryRunCallsNoServiceCommand.Red before, green after
The new tests were run against the baseline implementation before each fix. All failed there, and
each failure was the defect:
crwran (extractreportedv0.4.0-9999-gdeadbeef);service startcall at all;update_failedwhere the configuration change had to win;postcheck_failedafter a single status read;select count(*) from attempts where internal_state != 'settled';runtime_mismatchwhere a service already up on the installed runtime had to pass;update_failedwhere a pointer on a third runtime had to be a mismatch.The C8 flake was reproduced deterministically with a probe that waits for the wall-clock second
boundary before the second run and then applies the old assertion; it fails. A tight 80-run loop of
the baseline test passes, which is why the flake was intermittent rather than constant. The fixed
test is deterministic because the harness, not the wall clock, decides the second.
Commands and results
In the delivery worktree, on the final head:
Every Go command ran through the host's heavy-check slot, under a memory-limited scope, with the
shared build cache and a short temporary directory. The hosted checks on this head are the twelve
jobs the pull request reports.
Size
1378 insertions and 291 deletions across five files, generated data excluded (none of these files is
generated).
internal/manage/upgrade_review702_test.gois new; the other four are the two productfiles and the two existing test files the changed behaviour required. The size grew with the
correction rounds because each round had to pin the behaviour it changed; no part of it is
separately verifiable, so it stays in this one change.
Out of scope
internal/relayis untouched. No installation, service activation, configuration surface or liverelay state is changed, and the runtime directory naming rule is as it was.