Correct lock acquisition order in the pathEntityMergeID identity to fix deadlock condition

[ianferguson]

fixes #10876

This fixes lock acquisition ordering in the pathEntityMergeID function so that it matches other code in the identity backend's locking order by locking the IdentityStore lock before creating a memdb write transaction that requires a memdb lock. This avoids the dead lock scenario encountered in #10876.

In order to confirm this works, I ran the same reproduction harness described in #10876 against a locally built version of Vault with this patch and both loops got through over 20,000 requests without issue -- without the patch I had never seen the loops get past 400-500 requests.

[ncabatoff]

Hi @ianferguson,

Thanks for the PR! This looks good to me, but since I'm not that familiar with the identity subsystem I'm going to ask for another reviewer to check it out. As a follow-up someone (doesn't need to be you) should remove the grabLock option in mergeEntity now that no one will be using it. But that can wait.

[vishalnayak]

Maybe the grabLock parameter of mergeEntity is useless anymore.

[ncabatoff]

Ok @ianferguson, I'm happy to merge this change. One small request: could you add a file named changelog/10877.txt containing the following (including the backticks)?

core/identity: Fix deadlock in entity merge endpoint.

[ianferguson]

@vishalnayak @ncabatoff thank you for the fast review!

I added the changelog in 521d329

If you'd like I can open a second PR once this one is in that refactors the grabLock parameter out of the mergeEntity function -- let me know.

Also, while I've got y'alls eyes: while doing some profiling and performance troubleshooting on Vault adjacent to this issue, I ran into challenges observing the usage of the internal semaphores that gate many of the physical backend calls. I opened this PR to add metrics, and am happy to make any change to it if y'all are open to accepting the additional telemetry it adds: #10773

[ncabatoff]

Sorry, the changelog isn't quite right, it should include backticks. Can you look at one of the other files in that directory for an example? It's hard for me to show what it should look like here because the markup is interpreted by github too. Vishal pointed out that we should have contributor docs to explain this, which we'll try to take care of soon.

If you'd like I can open a second PR once this one is in that refactors the grabLock parameter out of the mergeEntity function -- let me know.

Up to you!

Re #10773: I saw it, I want to merge it, but we're trying to complete our 1.7 work before the feature freeze so I can't take the time to review it right now. I made an exception for this PR because it's a significant bugfix, and I wanted to get it into 1.6.3.

[ianferguson]

@ncabatoff sorry about that, fixed the formatting in 9fb3ea4, hopefully got it right this time!

Up to you!

I'll probably get it open later today, and y'all can review/include or not at your leisure

we're trying to complete our 1.7 work before the feature freeze so I can't take the time to review it right now. I made an exception for this PR because it's a significant bugfix, and I wanted to get it into 1.6.3.

that makes perfect sense to me and sounds great!

[fornever847]

Koo