Canonical merge of #1166 - #1167
Merged
Merged
Conversation
…kely raced it The crash-cleanup test went red once in CI naming a leftover `commitlore-demo-*` directory, passed on a re-run of the same commit, and passed on the other node leg of the same run. Nothing said why: `cleanup` caught the `rmSync` error and discarded it, so the one occurrence carried no errno, and a race, a permission and a full disk were indistinguishable from each other and from a removal that never ran. Three changes, in order of what they are worth: - The failure is reported — the directory and the reason, on stderr, never rethrown. `cleanup` also runs from a signal handler, where a throw has nowhere to go, and on the crash path it must not mask the error it is unwinding. - The demo's repository sets `gc.auto=0` and `maintenance.auto=false`, so `git commit` cannot leave background maintenance writing inside a directory that is about to be removed. That is the mechanism the occurrence is most consistent with, and a throwaway repository should not start one regardless. - `rmSync` gets `maxRetries`, which covers the errno set a concurrent writer produces. What this does not do is prove the cause. The new test injects the failure through a mocked `rmSync` because a real race is not reliably reproducible, so what it pins is the reporting: a cleanup that fails names the path and the errno. The next occurrence will say what this one could not. Limit: the evidence is one CI occurrence that passed on re-run and on the other node leg of the same run, so the cause was never observed Ruled-out: waiting for a reproduction before changing anything | a real removal race is not reliably reproducible, and the discarded errno is the reason the one occurrence could not be read at all Ruled-out: calling the retry a root fix | a retry makes a race survivable without showing that a race is what happened, and the cause is still unobserved Ruled-out: failing the command when cleanup fails | a leftover temporary directory is a leak, and a non-zero exit would make `commitlore demo` fail for something the user cannot act on Warn: the new test injects the failure through a mocked `rmSync`, so it pins the reporting and not the cause; if this goes red again the stderr line carries the errno -- read it before changing anything else Blast: local Undo: easy Certainty: tentative Unverified: whether background maintenance was the writer; the repository config change removes that mechanism and nothing observed it happening Record-Id: r-cleanupreport1163 Provenance: drafted
`build:canonical` on the merged tree, so the commit that lands matches the source it lands with. The pull request carried source only, which is what a contributor on a host that cannot run a linux/amd64 Docker build can produce (#720). Limit: this proves the bundle matches this tree; whether this tree is what a reviewer wants is what the pull request is for Blast: system Undo: easy Certainty: firm Record-Id: r-canonmerge1166 Provenance: authored Verified: artifact:verify passed against the regenerated manifest in the same job, before any credential was available to it CommitLore-Version: 2.0.0
CommitLore — record lintTrailers: clean — 3 commits in Active constraints for the paths this PR touchesLimits (322)
Ruled out (411)
Truncated: 474 lines omitted — the comment hit GitHub's 65000 character limit. Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
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
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.
The commit that will land for #1166:
mainplus that source plus a canonical rebuild, built together so all eleven required contexts run on the tree that merges rather than on one that resembles it.#1166 carries source only, which is what a contributor on a host that cannot run a
linux/amd64Docker build can produce (#720). Nothing was rebuilt by hand.Merge this with a merge commit, not a squash. This branch merged #1166 with
--no-ff, so its head commit is an ancestor here: a merge commit lands that commit onmain, and GitHub then records #1166 as merged because its head is reachable -- which is what T-1502 asks for. A squash lands new bytes instead, and #1166 stays open with nothing to point at.This body deliberately carries no closing keyword. GitHub binds one only to the number straight after it, and a pull request closed by keyword is recorded closed rather than merged -- the opposite of the line above. Reachability does the closing here.
Opened by
canonical-merge.ymlfor #719.