Skip to content

Prevent Any Storage Errors from Clobbering aZKS Entry#432

Merged
dillongeorge merged 2 commits intofacebook:mainfrom
dillongeorge:prevent-azks-clobbering
Apr 5, 2024
Merged

Prevent Any Storage Errors from Clobbering aZKS Entry#432
dillongeorge merged 2 commits intofacebook:mainfrom
dillongeorge:prevent-azks-clobbering

Conversation

@dillongeorge
Copy link
Contributor

No description provided.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 4, 2024
@dillongeorge dillongeorge force-pushed the prevent-azks-clobbering branch 2 times, most recently from 127ea25 to 9b9da58 Compare April 4, 2024 20:02
@dillongeorge
Copy link
Contributor Author

Current

Prior to this patch, AKD would incorrectly assume that any error during initialization of
a Directory should result in an aZKS being created and stored in the underlying database
implementation. The problem here is that this behavior happened on every error, even those which
do not definitively indicate that the read for an aZKS passed but a record was not found.

For example, if a connection issue occurs when connecting to the storage layer, the current implementation
could wrongly overwrite an existing aZKS. In such a scenario, the original read for the aZKS fails
during Directory initialization, but a subsequent write succeeds. This can result in clobbered records
and unexpected behavior.

Proposed/New

With this change, we update the initialization logic for the Directory type to only create an aZKS if
the returned Err indicates that the read was successful, but an aZKS was not found. In all other instances,
we propagate the Err back to the caller instead of swallowing it and wrongly creating an aZKS.

@dillongeorge dillongeorge force-pushed the prevent-azks-clobbering branch from 9b9da58 to 210af9d Compare April 4, 2024 20:07
@dillongeorge dillongeorge marked this pull request as ready for review April 4, 2024 20:22
@dillongeorge dillongeorge force-pushed the prevent-azks-clobbering branch from 210af9d to dfc8e77 Compare April 4, 2024 21:24
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.64%. Comparing base (0094631) to head (a1848e6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #432      +/-   ##
==========================================
+ Coverage   88.57%   88.64%   +0.07%     
==========================================
  Files          39       39              
  Lines        9077     9109      +32     
==========================================
+ Hits         8040     8075      +35     
+ Misses       1037     1034       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dillongeorge dillongeorge force-pushed the prevent-azks-clobbering branch 5 times, most recently from 62b975d to bc0ad74 Compare April 4, 2024 23:06
@dillongeorge dillongeorge force-pushed the prevent-azks-clobbering branch from bc0ad74 to a1848e6 Compare April 4, 2024 23:40
Dillon George added 2 commits April 4, 2024 17:00
**Current**
--
Prior to this patch, AKD would incorrectly assume that any error during initialization of
a `Directory` should result in an `aZKS` being created and stored in the underlying database
implementation. The problem here is that this behavior happened on every error, even those which
do not definitively indicate that the read for an `aZKS` passed but a record was not found.

For example, if a connection issue occurs when connecting to the storage layer, the current implementation
could wrongly overwrite an existing `aZKS`. In such a scenario, the original read for the `aZKS` fails
during `Directory` initialization, but a subsequent write succeeds. This can result in clobbered records
and unexpected behavior.

**Proposed/New**
--
With this change, we update the initialization logic for the `Directory` type to only create an `aZKS` if
the returned `Err` indicates that the read was successful, but an `aZKS` was not found. In all other instances,
we propagate the `Err` back to the caller instead of swallowing it and wrongly creating an `aZKS`.
@dillongeorge dillongeorge force-pushed the prevent-azks-clobbering branch from a1848e6 to 7c70de3 Compare April 5, 2024 00:00
@dillongeorge dillongeorge merged commit 3ce5335 into facebook:main Apr 5, 2024
@dillongeorge dillongeorge deleted the prevent-azks-clobbering branch August 30, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants