Skip to content

Commit d60a69d

Browse files
Aaron Hometa-codesync[bot]
authored andcommitted
Fix package_id_to_targets not cleaned during target removal
Summary: remove_target and remove_targets_batch leave dangling entries in package_id_to_targets when targets are removed from the graph. This means the package-to-target mapping accumulates stale target IDs over time, which corrupts serialized graphs via --output-graph and prevents us from using package_id_to_targets as a reliable source of truth for removal detection. For batch removal, iterate package_id_to_targets and retain only targets not in the removal set. For single removal, scan package_id_to_targets to find and remove the target from its package. Reviewed By: aniketmathur Differential Revision: D95091082 fbshipit-source-id: 3c46abd433bcd03c9e638aa0c86eb446a3e0be03
1 parent 117eb0a commit d60a69d

File tree

1 file changed

+96
-0
lines changed

1 file changed

+96
-0
lines changed

td_util/src/buck/target_graph.rs

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,7 @@ impl TargetGraph {
409409
self.remove_regular_target(target_id);
410410
}
411411

412+
self.remove_target_from_package(target_id);
412413
self.target_id_to_deps.remove(&target_id);
413414
self.target_id_to_ci_srcs.remove(&target_id);
414415
self.target_id_to_ci_srcs_must_match.remove(&target_id);
@@ -420,6 +421,22 @@ impl TargetGraph {
420421
self.minimized_targets.remove(&target_id);
421422
}
422423

424+
fn remove_target_from_package(&self, target_id: TargetId) {
425+
for mut entry in self.package_id_to_targets.iter_mut() {
426+
let targets = entry.value_mut();
427+
let Some(pos) = targets.iter().position(|&id| id == target_id) else {
428+
continue;
429+
};
430+
targets.swap_remove(pos);
431+
if targets.is_empty() {
432+
let package_id = *entry.key();
433+
drop(entry);
434+
self.package_id_to_targets.remove(&package_id);
435+
}
436+
return;
437+
}
438+
}
439+
423440
fn remove_ci_hint_target(&self, target_id: TargetId) {
424441
// Clean up CI hint edges from dedicated maps
425442
if let Some(affected) = self.get_ci_hint_affected(target_id) {
@@ -464,6 +481,7 @@ impl TargetGraph {
464481

465482
self.clean_ci_hint_edges_for_removed_targets(targets_to_remove);
466483
self.clean_rdeps_for_removed_targets(targets_to_remove);
484+
self.clean_package_targets_for_removed_targets(targets_to_remove);
467485
self.clean_per_target_data(targets_to_remove);
468486
}
469487

@@ -508,6 +526,13 @@ impl TargetGraph {
508526
});
509527
}
510528

529+
fn clean_package_targets_for_removed_targets(&self, targets_to_remove: &HashSet<TargetId>) {
530+
self.package_id_to_targets.retain(|_, targets| {
531+
targets.retain(|id| !targets_to_remove.contains(id));
532+
!targets.is_empty()
533+
});
534+
}
535+
511536
fn clean_per_target_data(&self, targets_to_remove: &HashSet<TargetId>) {
512537
for &target_id in targets_to_remove {
513538
self.target_id_to_deps.remove(&target_id);
@@ -1614,4 +1639,75 @@ mod tests {
16141639

16151640
assert!(graph.get_minimized_target(target).is_some());
16161641
}
1642+
1643+
#[test]
1644+
fn batch_remove_cleans_package_id_to_targets() {
1645+
let graph = TargetGraph::new();
1646+
1647+
let package_id = graph.store_package("fbcode//pkg");
1648+
let target_a = graph.store_target("fbcode//pkg:a");
1649+
let target_b = graph.store_target("fbcode//pkg:b");
1650+
let survivor = graph.store_target("fbcode//pkg:survivor");
1651+
1652+
graph.add_target_to_package(package_id, target_a);
1653+
graph.add_target_to_package(package_id, target_b);
1654+
graph.add_target_to_package(package_id, survivor);
1655+
1656+
for &id in &[target_a, target_b, survivor] {
1657+
store_minimized_stub(&graph, id);
1658+
}
1659+
1660+
let to_remove: HashSet<TargetId> = [target_a, target_b].into_iter().collect();
1661+
graph.remove_targets_batch(&to_remove);
1662+
1663+
let remaining = graph.get_targets_in_package(package_id).unwrap();
1664+
assert_eq!(remaining, vec![survivor]);
1665+
}
1666+
1667+
#[test]
1668+
fn batch_remove_cleans_package_id_to_targets_across_packages() {
1669+
let graph = TargetGraph::new();
1670+
1671+
let pkg1 = graph.store_package("fbcode//pkg1");
1672+
let pkg2 = graph.store_package("fbcode//pkg2");
1673+
let target_a = graph.store_target("fbcode//pkg1:a");
1674+
let target_b = graph.store_target("fbcode//pkg2:b");
1675+
let survivor = graph.store_target("fbcode//pkg2:survivor");
1676+
1677+
graph.add_target_to_package(pkg1, target_a);
1678+
graph.add_target_to_package(pkg2, target_b);
1679+
graph.add_target_to_package(pkg2, survivor);
1680+
1681+
for &id in &[target_a, target_b, survivor] {
1682+
store_minimized_stub(&graph, id);
1683+
}
1684+
1685+
let to_remove: HashSet<TargetId> = [target_a, target_b].into_iter().collect();
1686+
graph.remove_targets_batch(&to_remove);
1687+
1688+
assert!(graph.get_targets_in_package(pkg1).is_none());
1689+
let remaining = graph.get_targets_in_package(pkg2).unwrap();
1690+
assert_eq!(remaining, vec![survivor]);
1691+
}
1692+
1693+
#[test]
1694+
fn remove_target_cleans_package_id_to_targets() {
1695+
let graph = TargetGraph::new();
1696+
1697+
let package_id = graph.store_package("fbcode//pkg");
1698+
let target_a = graph.store_target("fbcode//pkg:a");
1699+
let target_b = graph.store_target("fbcode//pkg:b");
1700+
1701+
graph.add_target_to_package(package_id, target_a);
1702+
graph.add_target_to_package(package_id, target_b);
1703+
1704+
for &id in &[target_a, target_b] {
1705+
store_minimized_stub(&graph, id);
1706+
}
1707+
1708+
graph.remove_target(target_a);
1709+
1710+
let remaining = graph.get_targets_in_package(package_id).unwrap();
1711+
assert_eq!(remaining, vec![target_b]);
1712+
}
16171713
}

0 commit comments

Comments
 (0)