add --hooks-async and --hooks-before-symlink flags

[tylenwells]

#937

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds flags for --hooks-async and --hooks-before-symlink
Which issue(s) this PR fixes:

Fixes #937

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

- adds --hooks-async: Whether to run the exechook and webhook commands in an async or sync way. 
- adds --hooks-before-symlink: When to run the --exechook-command.  If true, will run the exechook or webhook commands before the symlink is updated. 
            ExecHook is asynchronous, so setting this will
            not block the sync process. If you need the hook to run to completion
            before the symlink is updated, you should use this in conjunction with --hooks-async=false.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tylenwells
Once this PR has been reviewed and has the lgtm label, please assign thockin for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Welcome @tylenwells!

It looks like this is your first PR to kubernetes/git-sync 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/git-sync has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[tylenwells]

/kind feature

[tylenwells]

/remove-kind documentation

[tylenwells]

/hold

[tylenwells]

adding a hold while I continue working on this.

Still need to figure out testing + resolving build errors.

[thockin]

Thanks! I am traveling the rest of this week, but will take a look when I can.

@sdowell can also offer feedback

[thockin]

It would be nice to break these into 2 distinct commits

[thockin]

Also, if you want to reformat other code that can go in a commit before or after the others

[thockin]

First, I think it would be clearer to frame this as "--exechook-wait" and equivalent for webhook.

Then. How about factoring the send and wait logic to a function, so the same logic is called in both sync and async cases?

[tylenwells]

I think splitting this as --exechook-wait and --webhook-wait would be fine.

If I'm understanding correctly then, you're suggesting:

• separate flags for delaying symlink publishing for each of the hooks
• a single function for triggering a hook runner as well as the WaitForCompletion? (one for each of the hooks)

Then if we do it like that, we would end up with

--exechook-when
--exechook-wait
--webhook-when
--webhook-wait

That sounds fine to me.

I did just push another commit though that I'd like feedback on. I was trying to reference webhookRunner, exechookRunner, the ExechookWhen flag, and the DelaySymlink flag in the SyncRepo function but it was failing as they were out of scope.

I did some reading and found that I could make it work by passing those values through with the context, which is what I did in 29eacef.

Most of my days are spent staring at YAML vs actually coding, so feel free to let me know if I'm running off the grid here. I'm taking this as a learning experience as I go.

[tylenwells]

I've been looking at this more over the evening and trying to get a better understanding of how the async vs sync runners work.

Looks like I'll need to do some further thinking about how to go about measuring completion on async runners.

[thockin]

I meant:

--exechook-wait and --webhook-wait : wait for hook(s) to complete successfully before continuing
--hooks-when=(AfterSymLink | BeforeSymlink)

But maybe even simpler:

--hooks-async=(true | false, default true)
--hooks-before-symlink=(true | false, default false)

?

[tylenwells]

Ok, I think I'm ready for further review whenever you have the time, @thockin or @sdowell.

Tests pass, Builds fine, but wanted to get your feedback and sign-off before finally editing the man page.

Thank you!

[thockin]

Sorry to be a pain, can you squash this down to the smallest set of logically complete commits for easy review? This is a couple features smashed together, so commit-by-commit will be easiest to review.

Thanks - I am travelling and reviewing on a phone -- hard to open multiple tabs or to clone it, until I can get dedicated computer time :)

[thockin]

Any reason not to just use one channel for both, e.g. results? You would need to keep a book for oneTime but that seems cleaner than 2 identical channels to me.

[tylenwells]

Yeah, this makes sense and should be fine. I'll make the change.

[thockin]

Why not just make Send be async if results change is nil, and sync if it is active? We passed async into the New function, might as well use it. Then we don't need this method at all.

[tylenwells]

I agree with this, I will consolidate.

[thockin]

Not at a computer right now, but I think pflags --help logic already prints the default - can you double check and make sure it doesn't print twice?

[tylenwells]

--help returned as follows:

  --hooks-async                          run hooks asynchronously (defaults to true, set to false to run hooks synchronously) (default true)
  --hooks-before-symlink                 run hooks before creating the symlink (defaults to false)

looks like hooks async was duplicated, so I edited the usage string there.

[thockin]

Rather than squirreling it into ctx, how about crafting a func(hash) which you pass alongside the credentials func. That way, all the hooks logic is defined here.

It might change the order of things like hooks vs. touch file, but I think that is OK. May need to include the setRepoReady. I am on mobile so this SEEMS like a good idea but I can't go try it to be sure :)

[tylenwells]

I believe I understand and will modify to do it like this instead. Much cleaner than the ctx shenanigans.

[thockin]

Is this debug leftovers?

[tylenwells]

Yep, ty. Will clean it up.

[thockin]

Ping ICYMI

[tylenwells]

Ping ICYMI

Thanks, reviewing now.

[tylenwells]

I believe I've addressed all of the feedback there, will work on re-submitting with better commit structure.

[tylenwells]

@thockin I've implemented the suggestions you gave and condensed the PR into three distinct commits. Let me know if there is anything else that sticks out to you!

Thanks,
Tylen

[thockin]

You don't need "GIT_SYNC_HOOKS_ASYNC" - some flags have old names for back-compat, but this is net-new :)

[thockin]

s/oneTimeResult/result

[thockin]

you already checked this and returned above

[thockin]

We already log below - I don't think we need logging here execpt:

if err := wait(); err != nil {
    log.Error(err, "hook failed", "hash", hash, "name", r.hook.Name())
}

[thockin]

this is weird - unintentional, I assume?

[thockin]

args-under-test go at the end of the list

[thockin]

I am unclear on what this is actually proving? Can you explain?

[thockin]

exec and web

[thockin]

exec and web

[thockin]

linewrap to 80 please

[thockin]

also need to update the manual in README.md

[thockin]

This is running before the new hash is even checked out?

[thockin]

Looking at it more, this is a bit tricky - you might get a result on the channel that indicates that the hook failed, but it will be retried (async). For one-time that's fine, but is it OK in non-one-time mode? If you ask for synchronous hooks, and the hook fails -- do we think that's "good enough" or do we wait for a success? What if success never comes?

[thockin]

I took your diff and made some of my changes because I wasn't sure I was clear what I wanted. What do you think of the below?

A few notes:

1. I did not touch the e2e logic -- I'd like to see that fleshed out.
2. I flipped hooks-async to hooks-synchronous -- it reads better.
3. The issue around failures counting as completions still exists - I am not sure what to do with that.

Quick manual testing SEEMS to work.

diff --git a/README.md b/README.md
index e05e9a6..d716b12 100644
--- a/README.md
+++ b/README.md
@@ -346,6 +346,14 @@ OPTIONS
     -?, -h, --help
             Print help text and exit.
 
+    --hooks-synchronous, $GITSYNC_HOOKS_SYNCHRONOUS
+            Whether to run the hooks synchronously or asynchronously.
+
+    --hooks-before-symlink, $GITSYNC_HOOKS_BEFORE_SYMLINK
+            Whether to run hooks before or afterupdating the symlink. Use in
+            combination with --hooks-synchronous if you need the hook to finish
+            before the symlink is updated.
+
     --http-bind <string>, $GITSYNC_HTTP_BIND
             The bind address (including port) for git-sync's HTTP endpoint.
             The '/' URL of this endpoint is suitable for Kubernetes startup and
@@ -572,15 +580,16 @@ AUTHENTICATION
 HOOKS
 
     Webhooks and exechooks are executed asynchronously from the main git-sync
-    process.  If a --webhook-url or --exechook-command is configured, they will
-    be invoked whenever a new hash is synced, including when git-sync starts up
-    and find that the --root directory already has the correct hash.  For
-    exechook, that means the command is exec()'ed, and for webhooks that means
-    an HTTP request is sent using the method defined in --webhook-method.
-    Git-sync will retry both forms of hooks until they succeed (exit code 0 for
-    exechooks, or --webhook-success-status for webhooks).  If unsuccessful,
-    git-sync will wait --exechook-backoff or --webhook-backoff (as appropriate)
-    before re-trying the hook.  Git-sync does not ensure that hooks are invoked
+    process, unless --hooks-synchronous is specified.  If a --webhook-url or
+    --exechook-command is configured, they will be invoked whenever a new hash
+    is synced, including when git-sync starts up and find that the --root
+    directory already has the correct hash.  For exechook, that means the
+    command is exec()'ed, and for webhooks that means an HTTP request is sent
+    using the method defined in --webhook-method. Git-sync will retry both
+    forms of hooks until they succeed (exit code 0 for exechooks, or
+    --webhook-success-status for webhooks).  If unsuccessful, git-sync will
+    wait --exechook-backoff or --webhook-backoff (as appropriate) before
+    re-trying the hook.  Git-sync does not ensure that hooks are invoked
     exactly once, so hooks must be idempotent.
 
     Hooks are not guaranteed to succeed on every single hash change.  For example,
diff --git a/_test_tools/exechook_command_with_sleep.sh b/_test_tools/exechook_command_with_sleep.sh
index 3bc1b95..5eccbb2 100755
--- a/_test_tools/exechook_command_with_sleep.sh
+++ b/_test_tools/exechook_command_with_sleep.sh
@@ -19,4 +19,6 @@
 
 sleep 3
 cat file > exechook
+cat exechook
+if [[ "$(pwd)" != "$(pwd -P)" ]]; then echo "true" > delaycheck; fi
 echo "ENVKEY=$ENVKEY" > exechook-env
diff --git a/main.go b/main.go
index 95dbec7..86a73df 100644
--- a/main.go
+++ b/main.go
@@ -207,7 +207,8 @@ func main() {
 	flGroupWrite := pflag.Bool("group-write",
 		envBool(false, "GITSYNC_GROUP_WRITE", "GIT_SYNC_GROUP_WRITE"),
 		"ensure that all data (repo, worktrees, etc.) is group writable")
-	flStaleWorktreeTimeout := pflag.Duration("stale-worktree-timeout", envDuration(0, "GITSYNC_STALE_WORKTREE_TIMEOUT"),
+	flStaleWorktreeTimeout := pflag.Duration("stale-worktree-timeout",
+		envDuration(0, "GITSYNC_STALE_WORKTREE_TIMEOUT"),
 		"how long to retain non-current worktrees")
 
 	flExechookCommand := pflag.String("exechook-command",
@@ -236,6 +237,13 @@ func main() {
 		envDuration(3*time.Second, "GITSYNC_WEBHOOK_BACKOFF", "GIT_SYNC_WEBHOOK_BACKOFF"),
 		"the time to wait before retrying a failed webhook")
 
+	flHooksSynchronous := pflag.Bool("hooks-synchronous",
+		envBool(false, "GITSYNC_HOOKS_SYNCHRONOUS"),
+		"run hooks synchronously")
+	flHooksBeforeSymlink := pflag.Bool("hooks-before-symlink",
+		envBool(false, "GITSYNC_HOOKS_BEFORE_SYMLINK"),
+		"run hooks before setting the symlink")
+
 	flUsername := pflag.String("username",
 		envString("", "GITSYNC_USERNAME", "GIT_SYNC_USERNAME"),
 		"the username to use for git auth")
@@ -844,6 +852,7 @@ func main() {
 			hook.NewHookData(),
 			log,
 			*flOneTime,
+			*flOneTime || *flHooksSynchronous,
 		)
 		go webhookRunner.Run(context.Background())
 	}
@@ -868,10 +877,36 @@ func main() {
 			hook.NewHookData(),
 			log,
 			*flOneTime,
+			*flOneTime || *flHooksSynchronous,
 		)
 		go exechookRunner.Run(context.Background())
 	}
 
+	// Craft a function that calls the hooks when the repo is synced.
+	runHooks := func(hash string) {
+		if exechookRunner != nil {
+			log.V(3).Info("sending exechook")
+			exechookRunner.Send(hash)
+		}
+		if webhookRunner != nil {
+			log.V(3).Info("sending webhook")
+			webhookRunner.Send(hash)
+		}
+		if *flHooksSynchronous {
+			//FIXME: do we need to wait for success, or is a failure result good enough?
+			if exechookRunner != nil {
+				if err := exechookRunner.WaitForCompletion(); err != nil {
+					log.Error(err, "exechook failed")
+				}
+			}
+			if webhookRunner != nil {
+				if err := webhookRunner.WaitForCompletion(); err != nil {
+					log.Error(err, "webhook failed")
+				}
+			}
+		}
+	}
+
 	// Setup signal notify channel
 	sigChan := make(chan os.Signal, 1)
 	if syncSig != 0 {
@@ -912,11 +947,12 @@ func main() {
 
 	failCount := 0
 	syncCount := uint64(0)
+
 	for {
 		start := time.Now()
 		ctx, cancel := context.WithTimeout(context.Background(), *flSyncTimeout)
 
-		if changed, hash, err := git.SyncRepo(ctx, refreshCreds); err != nil {
+		if changed, hash, err := git.SyncRepo(ctx, refreshCreds, runHooks, *flHooksBeforeSymlink); err != nil {
 			failCount++
 			updateSyncMetrics(metricKeyError, start)
 			if *flMaxFailures >= 0 && failCount >= *flMaxFailures {
@@ -937,12 +973,6 @@ func main() {
 						log.V(3).Info("touched touch-file", "path", absTouchFile)
 					}
 				}
-				if webhookRunner != nil {
-					webhookRunner.Send(hash)
-				}
-				if exechookRunner != nil {
-					exechookRunner.Send(hash)
-				}
 				updateSyncMetrics(metricKeySuccess, start)
 			} else {
 				updateSyncMetrics(metricKeyNoOp, start)
@@ -1716,7 +1746,7 @@ func (git *repoSync) currentWorktree() (worktree, error) {
 // SyncRepo syncs the repository to the desired ref, publishes it via the link,
 // and tries to clean up any detritus.  This function returns whether the
 // current hash has changed and what the new hash is.
-func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Context) error) (bool, string, error) {
+func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Context) error, runHooks func(hash string), hooksBeforeSymlink bool) (bool, string, error) {
 	git.log.V(3).Info("syncing", "repo", redactURL(git.repo))
 
 	if err := refreshCreds(ctx); err != nil {
@@ -1803,7 +1833,12 @@ func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Con
 		}
 
 		// If we have a new hash, update the symlink to point to the new worktree.
-		if changed {
+		if changed || git.syncCount == 0 {
+			// Fire hooks if needed.
+			if hooksBeforeSymlink {
+				runHooks(remoteHash)
+			}
+
 			err := git.publishSymlink(newWorktree)
 			if err != nil {
 				return false, "", err
@@ -1815,6 +1850,11 @@ func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Con
 					git.log.Error(err, "can't change stale worktree mtime", "path", currentWorktree.Path())
 				}
 			}
+
+			// Fire hooks if needed.
+			if !hooksBeforeSymlink {
+				runHooks(remoteHash)
+			}
 		}
 
 		// Mark ourselves as "ready".
@@ -2537,6 +2577,14 @@ OPTIONS
     -?, -h, --help
             Print help text and exit.
 
+    --hooks-synchronous, $GITSYNC_HOOKS_SYNCHRONOUS
+            Whether to run the hooks synchronously or asynchronously.
+
+    --hooks-before-symlink, $GITSYNC_HOOKS_BEFORE_SYMLINK
+            Whether to run hooks before or afterupdating the symlink. Use in
+            combination with --hooks-synchronous if you need the hook to finish
+            before the symlink is updated.
+
     --http-bind <string>, $GITSYNC_HTTP_BIND
             The bind address (including port) for git-sync's HTTP endpoint.
             The '/' URL of this endpoint is suitable for Kubernetes startup and
@@ -2763,15 +2811,16 @@ AUTHENTICATION
 HOOKS
 
     Webhooks and exechooks are executed asynchronously from the main git-sync
-    process.  If a --webhook-url or --exechook-command is configured, they will
-    be invoked whenever a new hash is synced, including when git-sync starts up
-    and find that the --root directory already has the correct hash.  For
-    exechook, that means the command is exec()'ed, and for webhooks that means
-    an HTTP request is sent using the method defined in --webhook-method.
-    Git-sync will retry both forms of hooks until they succeed (exit code 0 for
-    exechooks, or --webhook-success-status for webhooks).  If unsuccessful,
-    git-sync will wait --exechook-backoff or --webhook-backoff (as appropriate)
-    before re-trying the hook.  Git-sync does not ensure that hooks are invoked
+    process, unless --hooks-synchronous is specified.  If a --webhook-url or
+    --exechook-command is configured, they will be invoked whenever a new hash
+    is synced, including when git-sync starts up and find that the --root
+    directory already has the correct hash.  For exechook, that means the
+    command is exec()'ed, and for webhooks that means an HTTP request is sent
+    using the method defined in --webhook-method. Git-sync will retry both
+    forms of hooks until they succeed (exit code 0 for exechooks, or
+    --webhook-success-status for webhooks).  If unsuccessful, git-sync will
+    wait --exechook-backoff or --webhook-backoff (as appropriate) before
+    re-trying the hook.  Git-sync does not ensure that hooks are invoked
     exactly once, so hooks must be idempotent.
 
     Hooks are not guaranteed to succeed on every single hash change.  For example,
diff --git a/pkg/hook/exechook.go b/pkg/hook/exechook.go
index e31d996..8aa0770 100644
--- a/pkg/hook/exechook.go
+++ b/pkg/hook/exechook.go
@@ -68,7 +68,7 @@ func (h *Exechook) Do(ctx context.Context, hash string) error {
 	env := os.Environ()
 	env = append(env, envKV("GITSYNC_HASH", hash))
 
-	h.log.V(0).Info("running exechook", "hash", hash, "command", h.command, "timeout", h.timeout)
+	h.log.V(0).Info("running exechook", "hash", hash, "command", h.command, "cwd", worktreePath, "timeout", h.timeout)
 	stdout, stderr, err := h.cmdrunner.Run(ctx, worktreePath, env, h.command, h.args...)
 	if err == nil {
 		h.log.V(1).Info("exechook succeeded", "hash", hash, "stdout", stdout, "stderr", stderr)
diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go
index 5ba5a5c..8cb42c4 100644
--- a/pkg/hook/hook.go
+++ b/pkg/hook/hook.go
@@ -89,10 +89,10 @@ func (d *hookData) send(newHash string) {
 }
 
 // NewHookRunner returns a new HookRunner.
-func NewHookRunner(hook Hook, backoff time.Duration, data *hookData, log logintf, oneTime bool) *HookRunner {
-	hr := &HookRunner{hook: hook, backoff: backoff, data: data, log: log}
-	if oneTime {
-		hr.oneTimeResult = make(chan bool, 1)
+func NewHookRunner(hook Hook, backoff time.Duration, data *hookData, log logintf, oneTime bool, waitable bool) *HookRunner {
+	hr := &HookRunner{hook: hook, backoff: backoff, data: data, log: log, oneTime: oneTime}
+	if waitable {
+		hr.result = make(chan bool, 1)
 	}
 	return hr
 }
@@ -107,9 +107,11 @@ type HookRunner struct {
 	data *hookData
 	// Logger
 	log logintf
-	// Used to send a status result when running in one-time mode.
+	// Used to send a status result when running in one-time or non-async mode.
 	// Should be initialised to a buffered channel of size 1.
-	oneTimeResult chan bool
+	result chan bool
+	// Whether to exit after one run of the hook.
+	oneTime bool
 }
 
 // Just the logr methods we need in this package.
@@ -144,27 +146,25 @@ func (r *HookRunner) Run(ctx context.Context) {
 				r.log.Error(err, "hook failed", "hash", hash, "retry", r.backoff)
 				updateHookRunCountMetric(r.hook.Name(), "error")
 				// don't want to sleep unnecessarily terminating anyways
-				r.sendOneTimeResultAndTerminate(false)
+				r.sendResult(false)
 				time.Sleep(r.backoff)
 			} else {
 				updateHookRunCountMetric(r.hook.Name(), "success")
 				lastHash = hash
-				r.sendOneTimeResultAndTerminate(true)
+				r.sendResult(true)
 				break
 			}
 		}
 	}
 }
 
-// If oneTimeResult is nil, does nothing. Otherwise, forwards the caller
-// provided success status (as a boolean) of HookRunner to receivers of
-// oneTimeResult, closes said chanel, and terminates this goroutine.
-// Using this function to write to oneTimeResult ensures it is only ever
-// written to once.
-func (r *HookRunner) sendOneTimeResultAndTerminate(completedSuccessfully bool) {
-	if r.oneTimeResult != nil {
-		r.oneTimeResult <- completedSuccessfully
-		close(r.oneTimeResult)
+func (r *HookRunner) sendResult(completedSuccessfully bool) {
+	if r.result != nil {
+		r.result <- completedSuccessfully
+	}
+	// if oneTime is true, we just exit
+	if r.oneTime {
+		close(r.result)
 		runtime.Goexit()
 	}
 }
@@ -172,17 +172,19 @@ func (r *HookRunner) sendOneTimeResultAndTerminate(completedSuccessfully bool) {
 // WaitForCompletion waits for HookRunner to send completion message to
 // calling thread and returns either true if HookRunner executed successfully
 // and some error otherwise.
-// Assumes that r.oneTimeResult is not nil, but if it is, returns an error.
+// Assumes that either r.oneTime or !r.async, otherwise returns an error.
 func (r *HookRunner) WaitForCompletion() error {
 	// Make sure function should be called
-	if r.oneTimeResult == nil {
+	if r.result == nil {
 		return fmt.Errorf("HookRunner.WaitForCompletion called on async runner")
 	}
 
-	// Perform wait on HookRunner
-	hookRunnerFinishedSuccessfully := <-r.oneTimeResult
-	if !hookRunnerFinishedSuccessfully {
-		return fmt.Errorf("hook completed with error")
+	// If channel is not nil, we wait for its result.
+	if r.result != nil {
+		hookRunnerFinishedSuccessfully := <-r.result
+		if !hookRunnerFinishedSuccessfully {
+			return fmt.Errorf("hook completed with error")
+		}
 	}
 
 	return nil
diff --git a/test_e2e.sh b/test_e2e.sh
index a65f381..56cf3af 100755
--- a/test_e2e.sh
+++ b/test_e2e.sh
@@ -96,7 +96,7 @@ function assert_file_eq() {
     if [[ $(cat "$1") == "$2" ]]; then
         return
     fi
-    fail "$1 does not contain '$2': $(cat "$1")"
+    fail "$1 does not equal '$2': $(cat "$1")"
 }
 
 function assert_file_contains() {
@@ -2406,7 +2406,7 @@ function e2e::exechook_fail_retry() {
         --exechook-command="/$EXECHOOK_COMMAND_FAIL" \
         --exechook-backoff=1s \
         &
-    sleep 3 # give it time to retry
+    sleep 4 # give it time to retry
 
     # Check that exechook was called
     assert_file_lines_ge "$RUNLOG" 2
@@ -2486,6 +2486,55 @@ function e2e::exechook_startup_after_crash() {
     assert_file_lines_eq "$RUNLOG" 1
 }
 
+##############################################
+# Test exechook-success with --hooks-async=false
+##############################################
+function e2e::exechook_success_hooks_non_async() {
+    cat /dev/null > "$RUNLOG"
+
+    GIT_SYNC \
+        --hooks-async=false \
+        --period=100ms \
+        --repo="file://$REPO" \
+        --root="$ROOT" \
+        --link="link" \
+        --exechook-command="/$EXECHOOK_COMMAND_SLEEPY" \
+        &
+    sleep 5 # give it time to run
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_exists "$ROOT/link/exechook"
+    assert_file_eq "$ROOT/link/file" "${FUNCNAME[0]}"
+    assert_file_eq "$ROOT/link/exechook" "${FUNCNAME[0]}"
+    assert_file_eq "$ROOT/link/exechook-env" "$EXECHOOK_ENVKEY=$EXECHOOK_ENVVAL"
+}
+
+##############################################
+# Test exechook-success with --hooks-async=false and --hooks-before-symlink
+##############################################
+function e2e::exechook_success_hooks_before_symlink_non_async() {
+    cat /dev/null > "$RUNLOG"
+
+    GIT_SYNC \
+        --hooks-async=false \
+        --hooks-before-symlink \
+        --period=100ms \
+        --repo="file://$REPO" \
+        --root="$ROOT" \
+        --link="link" \
+        --exechook-command="/$EXECHOOK_COMMAND_SLEEPY" \
+        --exechook-backoff=1s \
+        &
+    sleep 5 # give it time to run
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_exists "$ROOT/link/exechook"
+    assert_file_eq "$ROOT/link/file" "${FUNCNAME[0]}"
+    assert_file_eq "$ROOT/link/exechook" "${FUNCNAME[0]}"
+    assert_file_eq "$ROOT/link/exechook-env" "$EXECHOOK_ENVKEY=$EXECHOOK_ENVVAL"
+    assert_file_absent "$ROOT/link/delaycheck"
+}
+
 ##############################################
 # Test webhook success
 ##############################################