fix: Resolve RwLock deadlocks in ObsoletedPackageManager causing test hangs

Multiple methods held a read or write lock on self.index while calling
fallback methods that also needed to acquire locks, causing deadlocks:

- get_obsoleted_package_metadata: read lock held during filesystem
  fallback that calls update_index_entry (write lock)
- get_obsoleted_package_manifest: same pattern
- remove_obsoleted_package: write lock held while calling build_index
  (also needs write lock)
- search_obsoleted_packages: read lock held during fallback

Fix: scope all lock acquisitions in blocks so locks are dropped before
calling any method that may also acquire a lock. For remove, extract a
needs_rebuild flag set inside the lock scope, then call build_index
after release.

All 95 libips tests now pass (previously 3 hung indefinitely).
This commit is contained in:
Till Wegmueller 2026-03-15 20:27:57 +01:00
parent 750df8dcc7
commit d5ed328ab2

View file

@ -1467,36 +1467,29 @@ impl ObsoletedPackageManager {
// Ensure the index is fresh // Ensure the index is fresh
if let Err(e) = self.ensure_index_is_fresh() { if let Err(e) = self.ensure_index_is_fresh() {
warn!("Failed to ensure index is fresh: {}", e); warn!("Failed to ensure index is fresh: {}", e);
// Fall back to the filesystem check if the index is not available
return self.get_obsoleted_package_metadata_from_filesystem(publisher, fmri); return self.get_obsoleted_package_metadata_from_filesystem(publisher, fmri);
} }
// Check the index // Check the index — scope the read lock so it's dropped before any fallback
let key = ObsoletedPackageKey::new(publisher, fmri); let key = ObsoletedPackageKey::new(publisher, fmri);
let index_result = {
let index = match self.index.read() {
Ok(idx) => idx,
Err(e) => {
warn!("Failed to acquire read lock on index: {}", e);
return self.get_obsoleted_package_metadata_from_filesystem(publisher, fmri);
}
};
index.get_entry(&key)
}; // read lock dropped here
// Try to get a read lock on the index match index_result {
let index_read_result = self.index.read(); Ok(Some((metadata, _))) => Ok(Some(metadata)),
if let Err(e) = index_read_result {
warn!("Failed to acquire read lock on index: {}", e);
// Fall back to the filesystem check if the index is not available
return self.get_obsoleted_package_metadata_from_filesystem(publisher, fmri);
}
let index = index_read_result.unwrap();
// Check if the package is in the index
match index.get_entry(&key) {
Ok(Some((metadata, _))) => {
// Return the metadata directly from the index
Ok(Some(metadata))
}
Ok(None) => { Ok(None) => {
// Package not found in the index, fall back to the filesystem check
self.get_obsoleted_package_metadata_from_filesystem(publisher, fmri) self.get_obsoleted_package_metadata_from_filesystem(publisher, fmri)
} }
Err(e) => { Err(e) => {
warn!("Failed to get entry from index: {}", e); warn!("Failed to get entry from index: {}", e);
// Fall back to the filesystem to check if there's an error
self.get_obsoleted_package_metadata_from_filesystem(publisher, fmri) self.get_obsoleted_package_metadata_from_filesystem(publisher, fmri)
} }
} }
@ -1580,27 +1573,24 @@ impl ObsoletedPackageManager {
// Ensure the index is fresh // Ensure the index is fresh
if let Err(e) = self.ensure_index_is_fresh() { if let Err(e) = self.ensure_index_is_fresh() {
warn!("Failed to ensure index is fresh: {}", e); warn!("Failed to ensure index is fresh: {}", e);
// Fall back to the filesystem check if the index is not available
return self.get_obsoleted_package_manifest_from_filesystem(publisher, fmri); return self.get_obsoleted_package_manifest_from_filesystem(publisher, fmri);
} }
// Check the index // Check the index — scope the read lock so it's dropped before any fallback
let key = ObsoletedPackageKey::new(publisher, fmri); let key = ObsoletedPackageKey::new(publisher, fmri);
let index_result = {
let index = match self.index.read() {
Ok(idx) => idx,
Err(e) => {
warn!("Failed to acquire read lock on index: {}", e);
return self.get_obsoleted_package_manifest_from_filesystem(publisher, fmri);
}
};
index.get_entry(&key)
}; // read lock dropped here
// Try to get a read lock on the index match index_result {
let index_read_result = self.index.read();
if let Err(e) = index_read_result {
warn!("Failed to acquire read lock on index: {}", e);
// Fall back to the filesystem check if the index is not available
return self.get_obsoleted_package_manifest_from_filesystem(publisher, fmri);
}
let index = index_read_result.unwrap();
// Check if the package is in the index
match index.get_entry(&key) {
Ok(Some((metadata, manifest))) => { Ok(Some((metadata, manifest))) => {
// If the content hash is NULL_HASH, generate a minimal manifest
if metadata.content_hash == NULL_HASH { if metadata.content_hash == NULL_HASH {
debug!( debug!(
"Generating minimal manifest for obsoleted package with null hash: {}", "Generating minimal manifest for obsoleted package with null hash: {}",
@ -1608,17 +1598,13 @@ impl ObsoletedPackageManager {
); );
return Ok(Some(self.generate_minimal_obsoleted_manifest(fmri))); return Ok(Some(self.generate_minimal_obsoleted_manifest(fmri)));
} }
// Return the manifest content directly from the index
Ok(Some(manifest)) Ok(Some(manifest))
} }
Ok(None) => { Ok(None) => {
// Package not found in the index, fall back to the filesystem check
self.get_obsoleted_package_manifest_from_filesystem(publisher, fmri) self.get_obsoleted_package_manifest_from_filesystem(publisher, fmri)
} }
Err(e) => { Err(e) => {
warn!("Failed to get entry from index: {}", e); warn!("Failed to get entry from index: {}", e);
// Fall back to the filesystem to check if there's an error
self.get_obsoleted_package_manifest_from_filesystem(publisher, fmri) self.get_obsoleted_package_manifest_from_filesystem(publisher, fmri)
} }
} }
@ -1860,46 +1846,38 @@ impl ObsoletedPackageManager {
} }
} }
// Remove the package from the index // Remove the package from the index — scope the write lock to avoid deadlock
let key = ObsoletedPackageKey::new(publisher, fmri); let key = ObsoletedPackageKey::new(publisher, fmri);
let mut needs_rebuild = false;
// Try to get a write lock on the index {
match self.index.write() { match self.index.write() {
Ok(index) => { Ok(index) => {
// Try to remove the entry from the index match index.remove_entry(&key) {
match index.remove_entry(&key) { Ok(true) => {
Ok(true) => { debug!("Removed package from index: {}", fmri);
debug!("Removed package from index: {}", fmri); }
} Ok(false) => {
Ok(false) => { debug!("Package not found in index: {}", fmri);
debug!("Package not found in index: {}", fmri); needs_rebuild = true;
// If the package is not in the index, we need to rebuild the index }
// This is a fallback in case the index is out of sync with the filesystem Err(e) => {
if let Err(e) = self.build_index() { warn!("Failed to remove package from index: {}: {}", fmri, e);
warn!("Failed to rebuild index after package not found: {}", e); needs_rebuild = true;
}
}
Err(e) => {
warn!("Failed to remove package from index: {}: {}", fmri, e);
// If there's an error removing the entry, rebuild the index
if let Err(e) = self.build_index() {
warn!("Failed to rebuild index after error: {}", e);
} }
} }
} }
Err(e) => {
warn!(
"Failed to acquire write lock on index, package not removed from index: {}: {}",
fmri, e
);
}
} }
Err(e) => { } // write lock dropped here
warn!(
"Failed to acquire write lock on index, package not removed from index: {}: {}", if needs_rebuild {
fmri, e if let Err(e) = self.build_index() {
); warn!("Failed to rebuild index: {}", e);
// If we can't get a write lock, mark the index as dirty so it will be rebuilt next time
if let Ok(index) = self.index.write() {
// This is a new writing attempt, so it might succeed even if the previous one failed
if let Err(e) = index.clear() {
warn!("Failed to clear index: {}", e);
}
}
} }
} }
@ -2139,25 +2117,24 @@ impl ObsoletedPackageManager {
return self.search_obsoleted_packages_fallback(publisher, pattern); return self.search_obsoleted_packages_fallback(publisher, pattern);
} }
// Try to get a read lock on the index // Get all entries from the index — scope the read lock
let index_read_result = self.index.read(); let entries = {
if let Err(e) = index_read_result { let index = match self.index.read() {
warn!("Failed to acquire read lock on index: {}", e); Ok(idx) => idx,
// Fall back to the filesystem-based search Err(e) => {
return self.search_obsoleted_packages_fallback(publisher, pattern); warn!("Failed to acquire read lock on index: {}", e);
} return self.search_obsoleted_packages_fallback(publisher, pattern);
}
let index = index_read_result.unwrap(); };
match index.get_all_entries() {
// Get all entries from the index Ok(entries) => entries,
let entries = match index.get_all_entries() { Err(e) => {
Ok(entries) => entries, warn!("Failed to get entries from index: {}", e);
Err(e) => { drop(index); // ensure lock is dropped before fallback
warn!("Failed to get entries from index: {}", e); return self.search_obsoleted_packages_fallback(publisher, pattern);
// Fall back to the filesystem-based search }
return self.search_obsoleted_packages_fallback(publisher, pattern);
} }
}; }; // read lock dropped here
// Check if the pattern looks like a version number // Check if the pattern looks like a version number
if pattern.chars().all(|c| c.is_digit(10) || c == '.') { if pattern.chars().all(|c| c.is_digit(10) || c == '.') {