test

Signed-off-by: Timur Tuktamyshev <timur.tuktamyshev@flant.com>

Author: timmilesdw

pkg/module_manager/models/modules/basic.go

@@ -246,7 +246,7 @@ func (bm *BasicModule) RegisterHooks(logger *log.Logger) ([]*hooks.ModuleHook, e
 		return nil, nil
 	}
 
-	hks, err := bm.searchModuleHooks()
+	hks, hasReadiness, err := bm.searchModuleHooks()
 	if err != nil {
 		return nil, fmt.Errorf("search module hooks failed: %w", err)
 	}
@@ -267,33 +267,27 @@ func (bm *BasicModule) RegisterHooks(logger *log.Logger) ([]*hooks.ModuleHook, e
 	}
 
 	bm.hooks.registered = true
+	bm.hasReadiness = hasReadiness
 
 	return hks, nil
 }
 
-func (bm *BasicModule) searchModuleHooks() ([]*hooks.ModuleHook, error) {
+func (bm *BasicModule) searchModuleHooks() ([]*hooks.ModuleHook, bool, error) {
 	shellHooks, err := bm.searchModuleShellHooks()
 	if err != nil {
-		return nil, fmt.Errorf("search module shell hooks: %w", err)
+		return nil, false, fmt.Errorf("search module shell hooks: %w", err)
 	}
 
 	goHooks := bm.searchModuleGoHooks()
 
-	batchHooks, err := bm.searchModuleBatchHooks()
+	batchHooks, hasReadiness, err := bm.searchModuleBatchHooks()
 	if err != nil {
-		return nil, fmt.Errorf("search module batch hooks: %w", err)
-	}
-
-	// TEMPORARY: Simulate error after hasReadiness is set to reproduce the bug.
-	// Set SIMULATE_READINESS_BUG=<module_name> to trigger error for specific module.
-	// After first error, hasReadiness stays true, causing "multiple readiness hooks found" on retry.
-	if simulateModule := os.Getenv("SIMULATE_READINESS_BUG"); simulateModule != "" && bm.Name == simulateModule && bm.hasReadiness {
-		return nil, fmt.Errorf("SIMULATED ERROR: testing hasReadiness bug for module %s", bm.Name)
+		return nil, false, fmt.Errorf("search module batch hooks: %w", err)
 	}
 
 	if len(shellHooks)+len(batchHooks) > 0 {
 		if err := bm.AssembleEnvironmentForModule(environmentmanager.ShellHookEnvironment); err != nil {
-			return nil, fmt.Errorf("Assemble %q module's environment: %w", bm.GetName(), err)
+			return nil, false, fmt.Errorf("Assemble %q module's environment: %w", bm.GetName(), err)
 		}
 	}
 
@@ -318,7 +312,7 @@ func (bm *BasicModule) searchModuleHooks() ([]*hooks.ModuleHook, error) {
 		return mHooks[i].GetPath() < mHooks[j].GetPath()
 	})
 
-	return mHooks, nil
+	return mHooks, hasReadiness, nil
 }
 
 func (bm *BasicModule) searchModuleShellHooks() ([]*kind.ShellHook, error) {
@@ -380,25 +374,22 @@ func (bm *BasicModule) searchModuleShellHooks() ([]*kind.ShellHook, error) {
 	return hks, nil
 }
 
-func (bm *BasicModule) searchModuleBatchHooks() ([]*kind.BatchHook, error) {
-	// FIX DISABLED FOR TESTING:
-	// Reset hasReadiness to handle retries after errors.
-	// If a previous search set hasReadiness=true but failed later (e.g., in AssembleEnvironmentForModule),
-	// hooks.registered remains false but hasReadiness stays true, causing "multiple readiness hooks found"
-	// error on retry.
-	// bm.hasReadiness = false
-
+// searchModuleBatchHooks searches for batch hooks and returns them along with hasReadiness flag.
+// This function has no side effects - it doesn't modify bm.hasReadiness directly.
+// The caller (RegisterHooks) is responsible for setting bm.hasReadiness after successful registration.
+func (bm *BasicModule) searchModuleBatchHooks() ([]*kind.BatchHook, bool, error) {
 	hooksDir := filepath.Join(bm.Path, "hooks")
 	if _, err := os.Stat(hooksDir); os.IsNotExist(err) {
-		return nil, nil
+		return nil, false, nil
 	}
 
 	hooksRelativePaths, err := RecursiveGetBatchHookExecutablePaths(bm.safeName(), hooksDir, bm.logger, hooksExcludedDir...)
 	if err != nil {
-		return nil, err
+		return nil, false, err
 	}
 
 	hks := make([]*kind.BatchHook, 0)
+	hasReadiness := false
 
 	// sort hooks by path
 	sort.Strings(hooksRelativePaths)
@@ -407,20 +398,20 @@ func (bm *BasicModule) searchModuleBatchHooks() ([]*kind.BatchHook, error) {
 	for _, hookPath := range hooksRelativePaths {
 		hookName, err := normalizeHookPath(filepath.Dir(bm.Path), hookPath)
 		if err != nil {
-			return nil, fmt.Errorf("could not get hook name: %w", err)
+			return nil, false, fmt.Errorf("could not get hook name: %w", err)
 		}
 
 		sdkcfgs, err := kind.GetBatchHookConfig(bm.safeName(), hookPath)
 		if err != nil {
-			return nil, fmt.Errorf("getting sdk config for '%s': %w", hookName, err)
+			return nil, false, fmt.Errorf("getting sdk config for '%s': %w", hookName, err)
 		}
 
 		if sdkcfgs.Readiness != nil {
-			if bm.hasReadiness {
-				return nil, fmt.Errorf("multiple readiness hooks found in %s", hookPath)
+			if hasReadiness {
+				return nil, false, fmt.Errorf("multiple readiness hooks found in %s", hookPath)
 			}
 
-			bm.hasReadiness = true
+			hasReadiness = true
 
 			// add readiness hook
 			nestedHookName := fmt.Sprintf("%s-readiness", hookName)
@@ -436,7 +427,7 @@ func (bm *BasicModule) searchModuleBatchHooks() ([]*kind.BatchHook, error) {
 		}
 	}
 
-	return hks, nil
+	return hks, hasReadiness, nil
 }
 
 func RecursiveGetBatchHookExecutablePaths(moduleName, dir string, logger *log.Logger, excludedDirs ...string) ([]string, error) {

pkg/module_manager/models/modules/basic_test.go

@@ -307,63 +307,26 @@ exit 0
 	require.Equal(t, "Kubernetes version is too low", *erGetter)
 }
 
-// TestHasReadinessResetOnRetry tests that hasReadiness is reset when searching for batch hooks.
-// This prevents "multiple readiness hooks found" error when registration is retried after
-// an error that occurred after hasReadiness was set (e.g., in AssembleEnvironmentForModule).
-func TestHasReadinessResetOnRetry(t *testing.T) {
+// TestHasReadinessNotSetOnFailedRegistration tests that hasReadiness is only set
+// after successful hook registration. This prevents stale hasReadiness state when
+// registration is retried after an error (e.g., in AssembleEnvironmentForModule).
+func TestHasReadinessNotSetOnFailedRegistration(t *testing.T) {
 	tmpModuleDir := t.TempDir()
 
-	bm, err := NewBasicModule("test-readiness-reset", tmpModuleDir, 1, utils.Values{}, nil, nil)
+	bm, err := NewBasicModule("test-readiness-registration", tmpModuleDir, 1, utils.Values{}, nil, nil)
 	require.NoError(t, err)
 
 	logger := log.NewLogger()
 	bm.WithLogger(logger)
 	bm.WithDependencies(stubDeps(logger))
 
-	// Simulate a state where hasReadiness was set to true from a previous failed attempt
+	// Simulate stale state from a previous failed registration attempt
 	bm.hasReadiness = true
 
-	// searchModuleBatchHooks should reset hasReadiness before searching for hooks
-	// Even though hooks.registered is false, hasReadiness was left as true
-	// Without the fix, this would cause issues on retry
+	// RegisterHooks should overwrite hasReadiness based on actual search results
 	_, err = bm.RegisterHooks(logger)
-	require.NoError(t, err, "RegisterHooks should not fail even if hasReadiness was true from previous attempt")
-
-	// After successful registration, hasReadiness should be false (no hooks found in empty dir)
-	require.False(t, bm.hasReadiness, "hasReadiness should be false after registering empty module")
-}
-
-// TestSearchModuleBatchHooksResetsReadiness verifies that searchModuleBatchHooks
-// resets hasReadiness at the start, preventing false "multiple readiness hooks found"
-// errors when retrying after a previous failed attempt.
-//
-// To reproduce the original bug WITHOUT the fix:
-// 1. Comment out "bm.hasReadiness = false" in searchModuleBatchHooks()
-// 2. Run this test - it will fail with "multiple readiness hooks found" error
-func TestSearchModuleBatchHooksResetsReadiness(t *testing.T) {
-	tmpModuleDir := t.TempDir()
-
-	bm, err := NewBasicModule("test-readiness-reset-batch", tmpModuleDir, 1, utils.Values{}, nil, nil)
 	require.NoError(t, err)
 
-	logger := log.NewLogger()
-	bm.WithLogger(logger)
-
-	// Simulate state after a failed registration attempt:
-	// - hasReadiness was set to true during searchModuleBatchHooks
-	// - but hooks.registered stayed false because error occurred later
-	bm.hasReadiness = true
-	// hooks.registered is false by default
-
-	// Call searchModuleBatchHooks directly (via RegisterHooks -> searchModuleHooks)
-	// Without the fix, this would return "multiple readiness hooks found" error
-	// because hasReadiness is already true from the "previous" attempt
-	batchHooks, err := bm.searchModuleBatchHooks()
-	require.NoError(t, err, "searchModuleBatchHooks should reset hasReadiness and not fail")
-
-	// No hooks in empty dir
-	require.Empty(t, batchHooks)
-
-	// hasReadiness should be false (no readiness hooks found in empty dir)
-	require.False(t, bm.hasReadiness, "hasReadiness should be false after searching empty hooks dir")
+	// hasReadiness should be false (no hooks in empty module)
+	require.False(t, bm.hasReadiness, "hasReadiness should reflect actual hooks found")
 }

pkg/module_manager/models/modules/global.go

@@ -586,9 +586,6 @@ func (gm *GlobalModule) searchGlobalShellHooks(hooksDir string) ([]*kind.ShellHo
 
 // searchGlobalHooks recursively find all executables in hooksDir. Absent hooksDir is not an error.
 func (gm *GlobalModule) searchGlobalBatchHooks(hooksDir string) ([]*kind.BatchHook, error) {
-	// Reset hasReadiness to handle retries after errors.
-	gm.hasReadiness = false
-
 	if _, err := os.Stat(hooksDir); os.IsNotExist(err) {
 		return nil, nil
 	}