test(did-git-sign): serialise the hook tests that exec what they just wrote - #54
Merged
Merged
Conversation
… wrote Four tests install a commit-msg hook and then execute it, three directly and one through git commit. Run in parallel they fail intermittently on Linux with ETXTBSY (Text file busy), and the test that fails moves between runs, which is what made this look like one flaky test rather than a shared race. The write is not the cause: write_executable_hook closes its handle before returning. The race is across threads. When one test forks a child - any Command::spawn - while another still holds its hook open for writing, the child inherits that descriptor for the moment between fork and exec, and Linux refuses to exec a file any process holds open for writing. Serialising the four removes the overlap, using the serial_test dev-dependency this module already applies to a CWD-global test. Production is unaffected: there, git execs the hook long after installation. Seen today on main (run 34704223744) and twice on the 0.4.10 release branch, where a re-run did not clear it. Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
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.
Blocks the 0.4.10 release (#53), whose
Testjob failed twice on this.The failure
init::tests::commit_msg_hook_*panics withOs { code: 26, kind: ExecutableFileBusy, message: "Text file busy" }, and the test that fails moves between runs —commit_msg_hook_adds_signoff_only_when_opted_inone run,commit_msg_hook_is_idempotentthe next. That is what made it read as one flaky test rather than a shared race. Seen onmainin run 34704223744 and twice on #53, where a re-run did not clear it.The cause
Not the write.
write_executable_hookusesstd::fs::write, which closes its handle before returning.It is a race across test threads. Four tests install a hook and then execute it — three via
Command::new(&hook), one viagit commit. When one test forks a child (anyCommand::spawn) while another still holds its hook open for writing, the child inherits that descriptor for the moment betweenforkandexec, and Linux refuses to exec a file that any process holds open for writing. Hence the intermittency, and hence which test loses being effectively random.The fix
#[serial_test::serial]on those four, with a comment explaining why so it is not stripped later as redundant.serial_testis already a dev-dependency of this crate and this module already uses it atinstall_hook_dispatcher_refuses_to_take_a_local_hooks_path_it_does_not_own, for the same class of reason (a process-global concern).Production is unaffected — there, git execs the hook long after installation, so the window does not exist.
Verification, and its limit
cargo fmt --all -- --checkclean,cargo clippy --workspace --all-targets -- -D warningssilent, five consecutive runs of CI's exactcargo test -p did-git-sign --features insecure-policy-bypassat 73/73, andcargo test --workspaceat 162/162.Worth being straight about the limit: those runs are macOS, where
ETXTBSYdoes not arise — the behaviour is Linux-specific and CI is Linux, so this PR's own CI is the real evidence, not my local runs. Serialising is the right remedy regardless, on the reasoning above.After this
Rebase #53 onto
main, get a green run on the release bump, then thev0.4.10tag.