Add timeouts for commit hooks #2547

DaRacci · 2025-03-05T12:39:24Z

This Pull Request closes #2546.

It changes the following:

Adds timeouts to git hooks

I followed the checklist:

I added unittests
I ran make check without errors
I tested the overall application
I added an appropriate item to the changelog

DaRacci · 2025-03-05T12:40:35Z

Opening early as a draft.

Initial changes are working and have tests added for them just need to figure out how we are going to implement a timeout value, if thats user configuration or a default impl.

DaRacci · 2025-03-06T08:03:18Z

Was wanting to get some feedback on this before progressing further, is this the correct place to implement the timeout logic? @extrawurst

extrawurst · 2025-03-11T10:24:30Z

This would impact other users like Gitbutler. @Byron what do you think about this change?

cruessler

From an implementation standpoint, this looks good. The only thing I would add is tests for non-zero timeouts (maybe that’s something you already planned or maybe I missed something).

asyncgit/src/sync/hooks.rs

DaRacci · 2025-03-16T02:47:59Z

Also worth mentioning as its done in this PR, I've changed the test shell shebang to use /usr/bin/env sh as this is a more widely accepted and allows distributions like NixOS to run the tests.

DaRacci · 2025-03-16T02:59:20Z

I've also noticed that while running tests that since i use the non POSIX Shell (nushell) that i need to prefix the command with SHELL=bash, i was wondering if it may be worth testing if the $SHELL is a known non compliant shell and trying to look for a default POSIX shell in the path?

DaRacci · 2025-03-16T03:20:36Z

Now that the implementation is solid how do i go about with setting the timeout, how do we expose configurable value for the user and should there be a default timeout?

Byron

Thanks for letting me know, @extrawurst.

The changes seem to not apply to git2-hooks, so wouldn't affect GitButler in its current form. However, that would change if timeouts would move into git2-hooks as I seem to be proposing :).

PS: I unsubscribed as this PR is relevant to me only once git2-hooks is involved. You can always ping me to change that.

asyncgit/src/sync/hooks.rs

DaRacci · 2025-03-16T07:43:14Z

@Byron I've updated the implementation and it is now inside git2-hooks, I've added new functions with suffixed _with_timeout for each hook but the run_hooks function has a breaking change having the timeout parameter added to it.

DaRacci · 2025-03-16T07:46:19Z

The last remaining thing to do is move the tests i had written over to the git2_hooks crate, even so the current tests in asyncgit should be able to cover most cases i think.

DaRacci · 2025-03-16T07:58:03Z

I was also thinking about the initial issue where the problem was an infinite wait because the hook was waiting for input, maybe a solution could be when a hook is running a new window is shown with the stdio from the hook and also allowing stdin, not sure if that would work how im thinking though.

Byron

Thanks a lot for the quick turnaround!

The new implementation definitely addresses the previous issue, and now the timeout has an actual effect on the hook.

When looking at git2-hooks exclusively, I am not sure I saw a test for this though.

git2-hooks/src/hookspath.rs

Byron · 2025-03-16T22:50:04Z

git2-hooks/src/lib.rs

@@ -66,6 +67,10 @@ pub enum HookResult {
 		/// path of the hook that was run
 		hook: PathBuf,
 	},
+	TimedOut {


Docs are missing.
Probably the crate could use a deny(missing_docs) as well.

I looked at adding deny(missing_docs) to the crate but there are a number of errors that are outside the scope of this PR.

git2-hooks/src/hookspath.rs

DaRacci · 2025-03-19T07:57:08Z

I'll get some tests put into the git2-hooks crate and resolve these issue when I get some times on the weekend to do so, please let me know if there is anything else that should be looked into for when i get to it.

Byron · 2025-03-24T01:23:52Z

@Byron I've updated the implementation and it is now inside git2-hooks, I've added new functions with suffixed _with_timeout for each hook but the run_hooks function has a breaking change having the timeout parameter added to it.

The idea would be to leave the run_hooks() function alone, and have it call the _with_timeout() function internally with a zero-timeout. That way one trivially avoids the breaking change while providing the feature to those who are interested in it.

I was also thinking about the initial issue where the problem was an infinite wait because the hook was waiting for input, maybe a solution could be when a hook is running a new window is shown with the stdio from the hook and also allowing stdin, not sure if that would work how im thinking though.

I don't think this can be a solution, as hooking up stdin must be controlled by the caller. For now, I'd think that hooks inherit stdin from the calling process, and that should work fine even if these have no stdin, as hooks can detect this and even if not they would never be able to hang - reading from a closed file will immediately fail.

Nonetheless I agree that timeouts for hooks may be solving for 'buggy' hooks, and it might be better to fix those (or understand precisely why they maybe hang legitimately so that can be fixed on the side of git2-hooks instead).

DaRacci · 2025-03-24T06:05:58Z

The idea would be to leave the run_hooks() function alone, and have it call the _with_timeout() function internally with a zero-timeout. That way one trivially avoids the breaking change while providing the feature to those who are interested in it.

I've made this change so run_hooks is left alone functionally and there is now the with_timeout version.

I don't think this can be a solution, as hooking up stdin must be controlled by the caller. For now, I'd think that hooks inherit stdin from the calling process, and that should work fine even if these have no stdin, as hooks can detect this and even if not they would never be able to hang - reading from a closed file will immediately fail.

Nonetheless I agree that timeouts for hooks may be solving for 'buggy' hooks, and it might be better to fix those (or understand precisely why they maybe hang legitimately so that can be fixed on the side of git2-hooks instead).

I think for now its best to track this in a new issue / PR, this one is pretty complete and ready to go as it is.

extrawurst · 2025-04-14T07:07:27Z

@DaRacci please update with master

…meouts All hooks functions now have their own with_timeout variant inside git2-hooks and asyncgit. The previous implementation of using a new thread with a timeout has been ditched in favour of a child command which we can now poll and kill to prevent the hook from continueing. I've added a hard coded default 5 second timeout but this will be handled by the Options struct later

Also adds tests for this function and the usage of timeouts in git2-hooks

naseschwarz

Hi @DaRacci. Thank you for your work on this, I appreciate it. This all seems well though-out and usable.

I do have some points regarding some details of your implementation I'd like to discuss before I give the go-ahead.

naseschwarz · 2025-04-14T09:11:10Z

git2-hooks/src/lib.rs

@@ -247,7 +346,7 @@ mod tests {

 		assert_eq!(res, HookResult::NoHookFound);

-		let hook = b"#!/bin/sh
+		let hook = b"#!/usr/bin/env sh


This is only tests, so who cares, but I'm curious why you changed that.

For the POSIX shell shebang to /bin/sh is portable, /usr/bin/env isn't. The latter is only relevant for more exotic languages like python, etc, AFAIK.

Did you have trouble running these tests on your machine without that change? Doesn't even nixos provide a link from /bin/sh to bash by default?

naseschwarz · 2025-04-14T09:21:38Z

src/popups/options.rs

+			"Timeout",
+			&self.options.borrow().hook_timeout().map_or_else(
+				|| "None".to_string(),
+				|d| format!("{d:?}"),


Using fmt::Debug straight into the UI seems smelly.

naseschwarz · 2025-04-14T09:37:20Z

git2-hooks/src/lib.rs

+/// Git hook: `commit_msg`
+///
+/// See [`hooks_commit_msg`] for more details.
+pub fn hooks_commit_msg_with_timeout(


This function shares code with hooks_commit_msg. Normally, I'd not be too pedantic about that, but here I see a clear chance that this would be missed in future changes.

Maybe hooks_commit_msg should call hooks_commit_msg_with_timeout(..., None) and hooks_commit_msg_with_timeout's last parameter should be Option<Duration>? This would also offer a unified interface to any users of git2-hooks.

Same comment is valid for the other functions in this file.

naseschwarz · 2025-04-14T09:44:40Z

git2-hooks/src/hookspath.rs

+	pub fn run_hook_with_timeout(
+		&self,
+		args: &[&str],
+		timeout: Duration,


Duration::ZERO signals "no timeout" in band on a public API. Should we make this Option<Duration>?

Same for all other interfaces this is passed through.

naseschwarz · 2025-04-14T10:05:24Z

git2-hooks/src/hookspath.rs

+//     Sleep Time=(8^2)×1=64, capped by `MAX_SLEEP_MILLIS` of 50
+//          Actual Sleep: 50 milliseconds
+//          Total Sleep: 190 milliseconds


// instead of ///

(Also, this is phrased as rustdoc docs, but describes internal parameters and the function isn't even parsed as it's not part of the interface. Maybe consider moving this comment into the function?)

naseschwarz · 2025-04-14T10:32:15Z

git2-hooks/src/hookspath.rs

+	loop {
+		if is_complete()? {
+			return Ok(true);
+		}


Let's make the purpose and "good path" of this loop explicit:

Suggested change

loop {

if is_complete()? {

return Ok(true);

}

while !is_complete()? {

and end function on Ok(true) below.

naseschwarz · 2025-04-14T10:46:09Z

git2-hooks/src/hookspath.rs

+		if timer.elapsed() > timeout {
+			return Ok(false);
+		}


Between your two calls to timer.elapsed() in this function, timeout - timer.elapsed() may become negative and panic in line 259.

Reduce this to one call and remove any doubt about panicking:

Suggested change

if timer.elapsed() > timeout {

return Ok(false);

}

let remaining_time = match timeout.checked_sub(timer.elapsed()) {

Some(t) => t,

None => return Ok(false),

};

naseschwarz · 2025-04-14T10:47:24Z

git2-hooks/src/hookspath.rs

+		// Ensure we do not sleep more than the remaining time
+		let remaining_time = timeout - timer.elapsed();
+		if remaining_time < sleep_time {
+			sleep_time = remaining_time;
+		}


Suggested change

// Ensure we do not sleep more than the remaining time

let remaining_time = timeout - timer.elapsed();

if remaining_time < sleep_time {

sleep_time = remaining_time;

}

(See comments above)

naseschwarz · 2025-04-14T10:54:56Z

asyncgit/src/sync/hooks.rs

+		let file = temp_dir.path().join("test");
+		let hook = format!(
+			"#!/usr/bin/env sh
+sleep 1


I'd say a test stalling for 1 s due to some I/O is common enough to warrant setting this to a longer time. If everything is alright, this should not affect the test.

Suggested change

sleep 1

sleep 60

naseschwarz · 2025-04-14T10:58:54Z

git2-hooks/src/hookspath.rs

+		let timeout = Duration::from_millis(100);
+		let start = std::time::Instant::now();
+		let result =
+			timeout_with_quadratic_backoff(timeout, || Ok(false));
+		let elapsed = start.elapsed();
+
+		assert_eq!(result.unwrap(), false);
+		assert!(elapsed < timeout + Duration::from_millis(10));


I don't think this is stable at all. If you want to make it stable, you need to offer your own Instant mock or offer a sans-i/o implementation for quadratic back-off. Same for all other tests that tightly rely on timing.

DaRacci · 2025-04-20T04:19:13Z

Apologies, I'm in the middle of building & swapping to a new computer I'll get back to this when I'm able to.

extrawurst requested review from Byron and cruessler March 11, 2025 10:24

cruessler reviewed Mar 15, 2025

View reviewed changes

asyncgit/src/sync/hooks.rs Outdated Show resolved Hide resolved

DaRacci force-pushed the master branch from 215febe to cb53532 Compare March 16, 2025 03:22

Byron reviewed Mar 16, 2025

View reviewed changes

asyncgit/src/sync/hooks.rs Outdated Show resolved Hide resolved

asyncgit/src/sync/hooks.rs Outdated Show resolved Hide resolved

asyncgit/src/sync/hooks.rs Outdated Show resolved Hide resolved

asyncgit/src/sync/hooks.rs Outdated Show resolved Hide resolved

DaRacci marked this pull request as ready for review March 16, 2025 11:02

Byron reviewed Mar 16, 2025

View reviewed changes

DaRacci force-pushed the master branch 2 times, most recently from b945cb6 to 0474735 Compare March 23, 2025 10:17

cruessler self-requested a review March 23, 2025 12:18

DaRacci added 7 commits April 14, 2025 17:53

feat(asyncgit): add timeouts for hooks

08eaefe

fix(asyncgit/test): use correct hook path and add non zero timeout test

ae55b09

chore: fix minor spelling mistakes

f619e3e

fix(test): relax time a bit

4f8b5ba

feat: implement option for hook timeout and abort the commit

2fe5f28

refactor: implement exponentional backoff for sleep duration.

883c425

Also adds tests for this function and the usage of timeouts in git2-hooks

chore: Add entry to CHANGELOG

4d0b022

DaRacci force-pushed the master branch from ce97e42 to 4d0b022 Compare April 14, 2025 08:07

naseschwarz self-requested a review April 14, 2025 09:12

naseschwarz suggested changes Apr 14, 2025

View reviewed changes

naseschwarz added the needs_work label Apr 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add timeouts for commit hooks #2547

Add timeouts for commit hooks #2547

DaRacci commented Mar 5, 2025 •

edited

Loading

DaRacci commented Mar 5, 2025

DaRacci commented Mar 6, 2025 •

edited

Loading

extrawurst commented Mar 11, 2025

cruessler left a comment

DaRacci commented Mar 16, 2025

DaRacci commented Mar 16, 2025

DaRacci commented Mar 16, 2025

Byron left a comment •

edited

Loading

DaRacci commented Mar 16, 2025 •

edited

Loading

DaRacci commented Mar 16, 2025

DaRacci commented Mar 16, 2025

Byron left a comment

Byron Mar 16, 2025

DaRacci Mar 23, 2025 •

edited

Loading

DaRacci commented Mar 19, 2025

Byron commented Mar 24, 2025

DaRacci commented Mar 24, 2025

extrawurst commented Apr 14, 2025

naseschwarz left a comment

naseschwarz Apr 14, 2025 •

edited

Loading

naseschwarz Apr 14, 2025

naseschwarz Apr 14, 2025

naseschwarz Apr 14, 2025 •

edited

Loading

naseschwarz Apr 14, 2025

naseschwarz Apr 14, 2025

naseschwarz Apr 14, 2025

naseschwarz Apr 14, 2025

naseschwarz Apr 14, 2025

naseschwarz Apr 14, 2025

naseschwarz Apr 14, 2025 •

edited

Loading

DaRacci commented Apr 20, 2025

-		if timer.elapsed() > timeout {
-			return Ok(false);
-		}
+		let remaining_time = match timeout.checked_sub(timer.elapsed()) {
+			Some(t) => t,
+			None => return Ok(false),
+		};

Add timeouts for commit hooks #2547

Are you sure you want to change the base?

Add timeouts for commit hooks #2547

Conversation

DaRacci commented Mar 5, 2025 • edited Loading

DaRacci commented Mar 5, 2025

DaRacci commented Mar 6, 2025 • edited Loading

extrawurst commented Mar 11, 2025

cruessler left a comment

Choose a reason for hiding this comment

DaRacci commented Mar 16, 2025

DaRacci commented Mar 16, 2025

DaRacci commented Mar 16, 2025

Byron left a comment • edited Loading

Choose a reason for hiding this comment

DaRacci commented Mar 16, 2025 • edited Loading

DaRacci commented Mar 16, 2025

DaRacci commented Mar 16, 2025

Byron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaRacci Mar 23, 2025 • edited Loading

Choose a reason for hiding this comment

DaRacci commented Mar 19, 2025

Byron commented Mar 24, 2025

DaRacci commented Mar 24, 2025

extrawurst commented Apr 14, 2025

naseschwarz left a comment

Choose a reason for hiding this comment

naseschwarz Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naseschwarz Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naseschwarz Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

DaRacci commented Apr 20, 2025

DaRacci commented Mar 5, 2025 •

edited

Loading

DaRacci commented Mar 6, 2025 •

edited

Loading

Byron left a comment •

edited

Loading

DaRacci commented Mar 16, 2025 •

edited

Loading

DaRacci Mar 23, 2025 •

edited

Loading

naseschwarz Apr 14, 2025 •

edited

Loading

naseschwarz Apr 14, 2025 •

edited

Loading

naseschwarz Apr 14, 2025 •

edited

Loading