Modules: Execute mutations in sequence (clean implementation)

[scr-oath]

Summary

This PR implements sequential mutation execution to ensure mutations are applied in the order hooks are listed in configuration, not in the order they complete. This makes mutation behavior predictable and allows dependent mutations to work correctly.

This is a clean-room implementation built from first principles that achieves the same goal as PR #4279 while proactively addressing all critical issues identified during code review.

Problem Statement

Current behavior: Hooks execute in parallel and mutations are applied in an unpredictable order based on which hook completes first (race condition).

Desired behavior: Mutations should be applied in the order hooks are listed in config, providing predictable sequencing for dependent mutations.

Solution Approach

Uses the same core strategy as PR #4279:

1. Pre-allocate a slice []hookResponse[P] with size = len(group.Hooks)
2. Each goroutine writes to its designated index (matches config order)
3. After WaitGroup completes, responses are in deterministic order
4. Mutations applied sequentially in that predictable order

Key Improvements Over Original Implementation

1. Proper Goroutine Cancellation ✅

• Context created at parent level, not in child goroutine
• Parent can cancel child goroutines on timeout/rejection
• Fixes goroutine leak where child continued executing after timeout

2. Simplified Timeout Handling ✅

• Uses ctx.Done() instead of time.After()
• Eliminates timer leak - no separate timer to manage
• Simpler: One timeout mechanism (context) instead of two

3. Zero-Value Bug Fix ✅

• Always initializes response pointer, even when canceled
• Fixes bug where rejected hooks left uninitialized responses
• Adds zero-value check in handleHookResponses to skip canceled hooks

4. Race-Free Parameter Passing ✅

• Payload passed as goroutine parameter, not captured by closure
• Prevents data race on newPayload variable
• Explicit, clear ownership of data

5. Comprehensive Documentation ✅

• Function-level documentation explaining resource cleanup
• Inline comments explaining non-obvious decisions
• Clear documentation of cancellation behavior

Test Coverage

✅ Sequential Mutation Order Test

Added TestSequentialMutationExecution that proves mutations apply in config order using channel-based synchronization (no flaky time.Sleep):

Test Design:

• 3 hooks configured in order: hook-1, hook-2, hook-3
• Each hook appends to User.CustomData (building on previous values)
• Channels force completion order: hook-2 → hook-3 → hook-1 (out of config!)
• Final result proves ordering

The Proof:

If mutations apply in CONFIG ORDER (1→2→3):
  Result: "123" ✓

If mutations apply in COMPLETION ORDER (2→3→1):
  Result: "231" ✗

Test asserts: CustomData == "123"

Test uses only concurrency constructs (channels, WaitGroups), no time-based testing.

✅ All Existing Tests Pass

• All 40+ existing tests pass with race detector enabled
• Zero race conditions detected
• Backward compatible with existing behavior

Comparison with Related PRs

vs. PR #4279 (Original)

• ✅ Same core strategy (pre-allocated slice + indexed writes)
• ✅ Fixes all 5 critical bugs identified in code review
• ✅ Production-ready as-is (no follow-up fixes needed)

Detailed comparison available here: https://github.com/prebid/prebid-server/blob/28a5cd15d06e1f10cb8e89a8295a0b128ab2ce95/IMPLEMENTATION_COMPARISON.md

vs. PR #4585 (Original + Improvements Plan)

• ✅ Delivers same end result but as cohesive implementation
• ✅ Simpler approach: Uses ctx.Done() instead of timer management
• ✅ Better documented from the start
• ✅ Easier to review: Single coherent implementation vs. incremental fixes

Related PRs

• PR Modules: Execute mutations in the order they are listed #4279: Original implementation (has critical bugs, needs fixes)
• PR Revert "Modules: Execute mutations in the order they are listed" #4529: Reverted PR Modules: Execute mutations in the order they are listed #4279 due to issues
• PR Execute mutations in the sequence they are listed (Re-apply #4279) #4585: PR Modules: Execute mutations in the order they are listed #4279 + improvements plan (competing approach) - CLOSED

Files Changed

• hooks/hookexecution/execution.go - Core implementation (~80 lines changed)
• hooks/hookexecution/mocks_test.go - Added mockSequencedHook for testing
• hooks/hookexecution/executor_test.go - Added sequential mutation test

Code Review Notes

This implementation incorporates valuable feedback during development:

• User insight: "Why manage a separate timer when context already has timeout?"
• Result: Simplified to use ctx.Done() instead of time.NewTimer

The implementation was built as a clean-room exercise to verify the approach works and compare design decisions.

---

🤖 Generated with Claude Code

Co-Authored-By: Claude [email protected]