Reject duplicate store names in StoreManager#2533
Open
heyitsalec wants to merge 1 commit into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
heyitsalec
force-pushed
the
store-manager-reject-duplicate-names
branch
from
341870e to
b972366
Compare
heyitsalec
force-pushed
the
store-manager-reject-duplicate-names
branch
from
b972366 to
2dcd9a5
Compare
StoreManager::add_store silently overwrote an existing store when a duplicate name was inserted into its RwLock<HashMap>, so a config with two stores sharing a name would drop the first without warning. Make add_store fallible: it now guards on the name already being present and returns Code::AlreadyExists instead of overwriting, and returns Ok(()) on the happy path. The uniqueness invariant thus lives on the type that owns the map, defending every caller rather than just the config loader. The production wiring site err_tips the failure with the offending store name, and all test call sites are updated to the fallible signature. An inline unit test proves the duplicate name is rejected. Ref: TraceMachina#1998
heyitsalec
force-pushed
the
store-manager-reject-duplicate-names
branch
from
2dcd9a5 to
7b3d2be
Compare
| store_manager.add_store("foo", memory_store.clone()); | ||
| store_manager | ||
| .add_store("foo", memory_store.clone()) | ||
| .unwrap(); |
Collaborator
There was a problem hiding this comment.
Would be better to use expect with a reason here, but otherwise an awesome patch, thanks.
| impl RootMetricsComponent for StoreManager {} | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
Collaborator
There was a problem hiding this comment.
Sorry, the structure of the repo is to split tests out. Could this be moved to nativelink-store/tests/store_manager_test.rs please?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
StoreManager::add_storeregistered stores withHashMap::insert, whichsilently overwrites any existing entry. A config declaring two stores with the
same
namedropped the first store with no error.add_storenow checks whether the name is already registeredand returns
Err(Code::AlreadyExists)instead of overwriting. I opted for enforcing uniqueness onStoreManager(rather than only in the config loader) to protect every caller and composes with the existing parse-timecheck_store_conflict. The startup wiring loop propagates the error witherr_tip, so a duplicate-name config now fails fast at startup with a clear message instead of silently discarding a store.Fixes #1998
Type of change
How Has This Been Tested?
Added an inline unit test in
store_manager.rs(
add_store_rejects_duplicate_name) that registers a store twice under the samename and asserts the second call returns
Code::AlreadyExists. All existingadd_storecall sites were updated to the fallible signature.relying on this PR's CI (
bazel test //..., which also runsrustfmt) as the verification gaterather than a local run. Happy to iterate quickly if CI flags anything.
Checklist
bazel test //...passes locallygit amendsee some docsThis change is