fix(hooks): clear repository git environment for tests - #1741
Conversation
Signed-off-by: Brian McNaboe <bmcnaboe@gmail.com>
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
The gate waited 90 x 30s = 45 min for CodeQL to finish on the PR head. That is shorter than CodeQL actually takes on this repository, so the gate has been failing runs that had not failed. Measured on PR DeusData#1426, head 7b72652: the CodeQL SAST workflow completed with conclusion=success at 17:46:05, having started at 15:41:44 -- 124 minutes. The gate step ran 16:52:58 to 17:38:44 and reported "BLOCKED: CodeQL timeout" 7 minutes and 21 seconds before the scan it was waiting for succeeded. Two open contributor pull requests are red from exactly this: DeusData#1426 and DeusData#1769, both with CodeQL completed=success on their head and every other check green. Three further PRs (DeusData#1703, DeusData#1741, DeusData#1742) are also red on codeql-gate alone, but from a different cause: the CodeQL run on their head is completed=cancelled, so the gate saw a non-success conclusion and correctly exited 1 without waiting. This change does not help those and is not intended to; they need a fresh scan, most likely having been superseded by concurrency cancel-in-progress in codeql.yml. 300 x 30s = 150 min covers the measured 124 min with margin. The job already declares timeout-minutes: 240, so the wait still cannot outlive its own job. No trigger, permission or gating change: codeql-gate blocks exactly what it blocked before, and a genuine CodeQL failure still exits 1 immediately rather than waiting out the budget. Only the absence of a verdict waits longer. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
DeusData
added
maintainer-notes
|
Thank you for the poisoned-environment reproduction and for limiting the change to the hook test subprocess. The contribution queue is quite full, so review may take a little time. We have the PR routed and will come back with grounded feedback as soon as possible. |
|
Approved. Ten lines, and three separate things in them are the right choice rather than the easy one. The subshell makes the scoping structural. Wrapping the unsets in
And the process substitution matters more than it looks. The obvious spelling — git rev-parse --local-env-vars | while IFS= read -r v; do unset "$v"; done— puts the loop in a further subshell, so every The incidental The diagnosis is the good part though. "The inherited relative index path works for a normal Your CI is not your faultAll five reds are one cascade. That budget was raised from 45 to 150 minutes in #1960, which merged yesterday. A re-run should come back clean. I will update this branch and re-run before merging — Sorry it sat two weeks for a ten-line fix that unblocks a workflow the project itself recommends. |
When a suite leader has already exited, taskkill /T cannot walk its tree, so run-test-wave.py proves cleanup by asking Windows whether anything is still parented to the dead PID. That probe was timed with --kill-grace, which the parallel gate passes as 1s. A cold powershell.exe + CIM start routinely costs more than that on a runner, so TimeoutExpired -- a fact about interpreter start-up, not about the process tree -- was read as "assume the worst" and failed an already-clean shard with exit 2. Give the probe its own stable-state budget (15s) plus one retry, keeping the fail-closed semantics: an exhausted or failed probe still counts as "cannot prove absence". The refusal now names which of the two happened, an unfinished probe or a counted set of live descendants, instead of reporting both as a leaked tree. The contract test pins this structurally -- no sleeps, no timing thresholds: the probe must be bounded by the module constant rather than by the kill-grace argument, must retry a timed-out probe, must never read an unusable probe as absence, and must report the two failures distinctly. Its Windows leader-exit case now waits on the scheduler's refusal rather than on a deadline shorter than the new budget. Refs #1741 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
The CLANG64 2/2 red on this PR was the harness, not your change: the parallel scheduler's Windows descendant probe was timed by --kill-grace (1 s), so a cold PowerShell start read as a leaked process tree. That is fixed on main in 202c8e4 (#2055). This branch only needs an update from main to pick it up, which I'll do when the runner queue has room — nothing for you to do here. |
2 participants
What does this PR do?
Clears Git's repository-local environment variables only for the pre-commit hook's test subprocess. This keeps staged-index context available to diff-scoped linting while preventing
GIT_INDEX_FILE=.git/indexfrom leaking into temporary linked-worktree fixtures.Root cause
Git exports repository-local variables to hooks. The inherited relative index path works for a normal
.gitdirectory but fails whengit worktree addchecks out into a linked worktree where.gitis a file.Checklist
git commit -s)make -f Makefile.cbm test)make -f Makefile.cbm lint-ci)Validation
GIT_INDEX_FILE=.git/index ... pipelinebefore: 249 passed, 1 failedscripts/test.sh: 7,555 passed, 0 failed, 8 skippedFixes #1740