Don't block fsnotify watcher during stack application

[twz123]

Description

Decouple the stack application from the fsnotify watcher loop. This prevents the loop from stalling due to a long-running application and reduces the risk of the operating system buffer getting overflowed. Trigger applications via a separate channel with a buffer size of one item. This naturally implements debouncing.

Fixes:

• Panic in applier unit tests: DATA RACE / send on closed channel #6337

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

How Has This Been Tested?

• Manual test
• Auto test added

Checklist

• My code follows the style guidelines of this project
• My commit messages are signed-off
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

[github-actions]

The PR is marked as stale since no activity has been recorded in 30 days

[jaitaiwan]

One small nit, but the rest LGTM

[jaitaiwan]

This part seems like unnecessary complication; s.log is available inside s.runWatcher so why not just do the defer and log the error inside s.runWatcher and just go s.runWatcher?

[twz123]

Good question. Maybe I felt that the function which created the watcher should close it, instead of delegating the ownership to runWatcher. Anyhow, I've pushed the defers down the line. PTAL!

[jaitaiwan]

I don’t think err = is needed now

[twz123]

It is, if we want to forward any errors that might occur when running the watcher to the caller. But: we have to close the trigger channel after assigning to err here. Therefore, at least that defer must be pulled one level up from runWatcher.

[jaitaiwan]

So I did a bit of pontificating, I think this should still pass the unit test but will make it a bit clearer what's happening with errors:

// Run executes the initial apply and watches the stack for updates.
func (s *StackApplier) Run(ctx context.Context) error {
	if ctx.Err() != nil {
		return nil // The context is already done.
	}

	ctx, cancel := context.WithCancel(ctx)
	defer cancel()

	watcher, err := fsnotify.NewWatcher()
	if err != nil {
		return fmt.Errorf("failed to create watcher: %w", err)
	}

	errCh := make(chan error)
	go s.runWatcher(watcher, errCh, ctx)

	if addErr := watcher.Add(s.path); addErr != nil {
		return fmt.Errorf("failed to watch %q: %w", s.path, addErr)
	}

	for err := range errCh {
		return err
	}

	return nil
}

func (s *StackApplier) runWatcher(watcher *fsnotify.Watcher, errCh chan error, ctx context.Context) (err error) {
	defer func() { errCh <- errors.Join(err, watcher.Close()) }()

	const timeout = 1 * time.Second // debounce events for one second
	timer := time.NewTimer(timeout)
	defer timer.Stop()

	for {
		select {
		case err := <-watcher.Errors:
			return fmt.Errorf("while watching stack: %w", err)

		case event := <-watcher.Events:
			// Only consider events on manifest files
			if match, _ := filepath.Match(manifestFilePattern, filepath.Base(event.Name)); !match {
				continue
			}
			timer.Reset(timeout)

		case <-timer.C:
			s.apply(ctx)

		case <-ctx.Done():
			return nil
		}
	}
}

[jaitaiwan]

Not sure if the errCh would need to be buffered and have it closed in the defer of the runWatcher function though.

[twz123]

The main purpose of this PR is to decouple the stack application from the watcher loop, to ensure that long-running stack applications do not create backpressure into the kernel inotify event queue. While I agree your proposed change is more straight forward, it's quite important to not block the event consumption.

[jaitaiwan]

That's a good point, I went a bit far in my simplification. I think if you used the channel approach but kept the triggers that would make the non-obvious coupling between when to close the triggers and the err = being populated by the time the program returns.

[jaitaiwan]

LGTM