ZOOKEEPER-4689: Fix inaccessible node due to inconsistent ACL index after SNAP sync by adamyi · Pull Request #1997 · apache/zookeeper

adamyi · 2023-04-20T04:10:10Z

Please refer to JIRA for details on the bug and why I chose to fix it this way.

An additional benefit of the patch is that we now no longer miscount refcount when replaying transactions. So we don't need to wait until next time we load snapshot to remove those unused ACL cache entries.

Please refer to JIRA for more details on the bug.

sonatype-lift · 2023-04-20T04:27:20Z

+    /**
+     * checks if longval is cached (convertLong can convert it to ACLs).
+     *
+     * @param longVal


EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description.

Suggested change

* @param longVal

*

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2023-04-20T04:27:22Z

+        if (longVal == OPEN_UNSAFE_ACL_ID) {
+            return true;
+        }
+        return longKeyMap.containsKey(longVal);


THREAD_SAFETY_VIOLATION: Read/Write race. Non-private method ReferenceCountedACLCache.isLongCached(...) reads with synchronization from container this.longKeyMap via call to Map.containsKey(...). Potentially races with unsynchronized write in method ReferenceCountedACLCache.deserialize(...).
Reporting because this access may occur on a background thread.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? _{Answering this survey will not impact your Lift settings.}

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

…fter SNAP sync

adamyi · 2023-06-09T04:07:06Z

Unfortunately, there are some bugs with this patch:

Bug 1

Consider the following sequence of events:

Serialize ACL (up to 5)
Create /a1 with new ACL 6
Create /a2 with ACL 6
Delete /a1

We would create ACL 6 with a refcount of 1 at step 2 and leave it unchanged at step 3 because ACL 6 already exists. Deleting /a1 at step 4 would decrement the refcount and consequently garbage-collect the ACL entry, leaving /a2 pointing to a non-existent ACL entry.

Bug 2

Consider the following sequence of events:

ACL is up to 5
Create /a0 with ACL 6
Remove /a0
Serialize ACL (up to 5)
Create /a1 with new ACL 7
Create /a2 with new ACL 8
Create /a3 with new ACL 7

When replaying, we would create ACL 6 at step 4; 7 at step 5. When replaying step 6, we would treat ACL 7 as already created with the value from step 5. That causes /a3 to have the ACL of /a2. This is wrong.

Fixes

I'm proposing three approaches to fix these bugs. I don't feel strongly among these solutions and am happy to implement any of them. Do Zookeeper reviewers have a preference?

Make the ACL cache skip indices for nodes that were deserialized even if these entries weren't there originally, as if we created and then removed them (ACL cache is discrete due to GC). We can do this by also bumping aclIndex in ReferenceCountedACLCache.addUsage. For bug 1 scenario, we'll move /a1 and /a2 both to ACL 7, making it easier to
accurately ref-count them. Deleting /a1 will not affect /a2. For bug 2 scenario, we'll set /a1 and /a3 to point to ACL 9 and /a2 to point to ACL 10.
Directly change ACL to -2 during node deserialization if ACL cache does not exist. When creating node, we overwrite ACL if and only if it's -2. This is essentially the same as skipping these indices as we're making (ACLs originally missing) and (recreated ACLs during replay) two disjoint spaces. It might feel like a more local fix than solution 1 as the code change only involves DataTree but it's not. It still relies on the fact that -2 is not a valid ACL index in ReferenceCountedACLCache (ACLCache already uses -1 as special value).
An even more radical variation of solution 2 would be to directly delete the node during node deserialization if ACL cache does not exist. The argument is that we must re-create it during transaction replay; otherwise the node would not be accessible due to missing ACL MarshallError anyway.

adamyi · 2023-06-09T04:15:33Z

@kezhuw Do you mind taking a look or do you know who might be a more appropriate reviewer for this bug? Thanks :)

kezhuw

For your comments, I did not get what fixes#1 try to express. Both fixes#2 and fixes#3 are viable from my point of view, fixes#3 should be simple to go.

I wonder whether the alternative in jira is more appropriate:

Make aclIndex consistent upon re-deserialization (by either serializing it in snapshot or paying special attention to decrement it when removing cache)

This is easy to achieve if we retain topmost acl(a.k.a ACL aclIndex pointing to) in puring or removing.

aclIndex inconsistency itself is not a problem

Not sure, but concistency is a good.

who might be a more appropriate reviewer for this bug? Thanks :)

How about sending an email to dev mailing list ?

kezhuw · 2023-06-09T06:39:54Z

                parent.stat.setCversion(parentCVersion);
                parent.stat.setPzxid(zxid);
            }
+            Long acls = aclCache.convertAcls(acl);


Given this patch, how bug#1 could happen ?

We would create ACL 6 with a refcount of 1 at step 2 and leave it unchanged at step 3 because ACL 6 already exists. Deleting /a1 at step 4 would decrement the refcount and consequently garbage-collect the ACL entry, leaving /a2 pointing to a non-existent ACL entry.

step#3 creates a new node with existing ACL, so that ACL will get ref count incremented. step#4 will not delete ACL 6.

kezhuw · 2023-06-09T07:14:59Z

+                        // case, we'll replay node creation transaction. We
+                        // should add it to cache and reset the ACL if the
+                        // previous longval does not exist.
+                        if (!aclCache.isLongCached(child.acl)) {


Is this the cause to bug#2 in this patch ?

ztzg · 2023-08-23T14:58:04Z

Hi @adamyi, @kezhuw,

For the record, we have also hit this in production.

This is indeed a very critical issue, as the corruption can spread from member to member!

I initially preferred solution 2 from the ticket description—the one which was tentatively implemented above—but given the difficulties encountered, and @kezhuw's suggestion of never removing the ACL aclIndex is pointing to, I am also reconsidering. Are we missing something?

We would also like to add some kind of (optional) "fsck" pass which sanity checks the tree before the service starts—to prevent this and other kinds of corruption from spreading—but that can be implemented in a followup ticket.

Cc: @eolivelli, @symat, @anmolnar, in case you haven't seen this.

adamyi · 2026-07-15T16:59:02Z

Closed in favor of a newer version of the patch: #2425

ZOOKEEPER-4689: Add failing test

fb94cfb

Please refer to JIRA for more details on the bug.

sonatype-lift Bot reviewed Apr 20, 2023

View reviewed changes

ZOOKEEPER-4689: Fix inaccessible node due to inconsistent ACL index a…

7b12742

…fter SNAP sync

adamyi force-pushed the ZOOKEEPER-4689 branch from 0df8f4b to 7b12742 Compare April 20, 2023 05:29

kezhuw reviewed Jun 9, 2023

View reviewed changes

ztzg force-pushed the master branch from 1c60545 to e2070be Compare October 3, 2023 12:57

adamyi mentioned this pull request Jul 15, 2026

ZOOKEEPER-4689: Fix ACL cache recovery from fuzzy snapshots #2425

Open

adamyi closed this Jul 15, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ZOOKEEPER-4689: Fix inaccessible node due to inconsistent ACL index after SNAP sync#1997

ZOOKEEPER-4689: Fix inaccessible node due to inconsistent ACL index after SNAP sync#1997
adamyi wants to merge 2 commits into
apache:masterfrom
adamyi:ZOOKEEPER-4689

adamyi commented Apr 20, 2023

Uh oh!

sonatype-lift Bot Apr 20, 2023

Uh oh!

sonatype-lift Bot Apr 20, 2023

Uh oh!

adamyi commented Jun 9, 2023 •

edited

Loading

Uh oh!

adamyi commented Jun 9, 2023

Uh oh!

kezhuw left a comment •

edited

Loading

Uh oh!

kezhuw Jun 9, 2023

Uh oh!

kezhuw Jun 9, 2023

Uh oh!

ztzg commented Aug 23, 2023

Uh oh!

adamyi commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

Uh oh!

Conversation

adamyi commented Apr 20, 2023

Uh oh!

sonatype-lift Bot Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

sonatype-lift Bot Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

adamyi commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug 1

Bug 2

Fixes

Uh oh!

adamyi commented Jun 9, 2023

Uh oh!

kezhuw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kezhuw Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

kezhuw Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

ztzg commented Aug 23, 2023

Uh oh!

adamyi commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adamyi commented Jun 9, 2023 •

edited

Loading

kezhuw left a comment •

edited

Loading