Improve ACL SETUSER Thread Handling

[kevinmichaelbowersox]

Describe the bug

While analyzing thread safety/handling for another pull request, I discovered improvements for thread handling in the ACL SETUSER command implementation.

Currently, the implementation of this command performs in place modifications to the User properties and modifies commands within the CommandPermissionSet as the ACL input to the command is parsed. In place modification of these objects/properties works well in a single threaded context, however it can produce inconsistent results in a multithreaded context due to modifications to shared state. (see code described here).

        // Please Note: Omitted non-essential code / comments
        private bool NetworkAclSetUser()
        {
                // User object retrieved from ACL shared by all sessions (aka shared state).
                var user = aclAuthenticator.GetAccessControlList().GetUser(username);

                try
                {
                    // Remaining parameters are ACL operations
                    for (var i = 1; i < parseState.Count; i++)
                    {
                        var op = parseState.GetString(i);
                        ACLParser.ApplyACLOpToUser(ref user, op); // Modifications to shared state in multithreaded context.
                    }
                }
                catch (ACLException exception)
                // Rest of method...

In this scenario, the User (aka shared state) is stored in memory within a ConcurrentDictionary located within the AccessControlList. I believe the AccessControlList is shared across all instances of RespServerSession (see code reference here). If multiple clients concurrently call ACL SETUSER for the same user it can produce inconsistent results that combine input arguments from multiple invocations of ACL SETUSER.

Additionally, there is a race condition that can occur if multiple clients attempt to issue the initial ACL SETUSER command for a user due to contention on the ConcurrentDictionary in the AccessControlList.

Steps to reproduce the bug

The following unit tests can be used to recreate the issue. I have included these in an accompanying PR that addresses the issue.

        /// <summary>
        /// Tests that ACL SETUSER works in parallel without corrupting the user's ACL.
        ///
        /// Test launches multiple clients that apply two simple ACL changes to the same user many times in parallel.
        /// Validates that ACL result after each execution is one of the possible valid responses.
        ///
        /// Race conditions are not deterministic so test uses repeat.
        ///
        /// </summary>
        [TestCase(128, 2048)]
        [Repeat(2)]
        public async Task ParallelAclSetUserTest(int degreeOfParallelism, int iterationsPerSession)
        {
            string activeUserWithGetCommand = $"ACL SETUSER {TestUserA} on >{DummyPassword} +get";
            string inactiveUserWithoutGetCommand = $"ACL SETUSER {TestUserA} off >{DummyPassword} -get";

            string activeUserWithGet = $"user {TestUserA} on #{DummyPasswordHash} +get";
            string inactiveUserWithoutGet = $"user {TestUserA} off #{DummyPasswordHash} -get";
            string inactiveUserWithNoCommands = $"user {TestUserA} off #{DummyPasswordHash}";

            var c = TestUtils.GetGarnetClientSession();
            c.Connect();
            _ = await c.ExecuteAsync(activeUserWithGetCommand.Split(" "));

            // Run multiple sessions that stress AUTH
            await Parallel.ForAsync(0, degreeOfParallelism, async (t, state) =>
            {
                using var c = TestUtils.GetGarnetClientSession();
                c.Connect();

                for (uint i = 0; i < iterationsPerSession; i++)
                {
                    await Task.WhenAll(
                        c.ExecuteAsync(activeUserWithGetCommand.Split(" ")),
                        c.ExecuteAsync(inactiveUserWithoutGetCommand.Split(" ")));

                    var aclListResponse = await c.ExecuteForArrayAsync("ACL", "LIST");

                    if (!aclListResponse.Contains(activeUserWithGet) &&
                        !aclListResponse.Contains(inactiveUserWithoutGet) &&
                        !aclListResponse.Contains(inactiveUserWithNoCommands))
                    {
                        string corruptedAcl = aclListResponse.First(line => line.Contains(TestUserA));
                        throw new AssertionException($"Invalid ACL: {corruptedAcl}");
                    }
                }
            });
        }

        /// <summary>
        /// Tests that ACL SETUSER works in parallel without fatal contention on user in ACL map.
        ///
        /// Test launches multiple clients that apply the same ACL change to the same user. Creates race to become the
        /// the first client to add the user to the ACL. Throws after initial insert into ACL if threading issues exist.
        ///
        /// Race conditions are not deterministic so test uses repeat.
        ///
        /// </summary>
        [TestCase(128, 2048)]
        [Repeat(10)]
        public async Task ParallelAclSetUserAvoidsMapContentionTest(int degreeOfParallelism, int iterationsPerSession)
        {
            string command1 = $"ACL SETUSER {TestUserA} on >{DummyPassword}";

            await Parallel.ForAsync(0, degreeOfParallelism, async (t, state) =>
            {
                using var c = TestUtils.GetGarnetClientSession();
                c.Connect();

                List<Task> tasks = new();
                for (uint i = 0; i < iterationsPerSession; i++)
                {
                    // Creates race between threads contending for first insert into ACL. Throws after first ACL insert.
                    tasks.Add(c.ExecuteAsync(command1.Split(" ")));
                }

                await Task.WhenAll(tasks);
            });

            ClassicAssert.Pass();
        }
    }

Expected behavior

I would expect the following behavior from the ACL SETUSER command:

• It would not throw an exception when two threads or clients race to insert the first user. Instead, one thread/client would insert the user and the other would perform an update.
• It would maintain integrity of the user ACL, avoiding the combination of input arguments across client invocations of the command.
• It would handle contention among clients attempting to modify the same user, producing linearizable results.
• Subsequent calls to ACL LIST or in the future ACL GETUSER would return with deterministic results for the ACL operations performed via ACL SETUSER.

Screenshots

No response

Release version

I am building and testing against the source code in the repo as of February 1st, 2025.

IDE

No response

OS version

No response

Additional context

I have submitted this accompanying PR to address this issue: #989