[APP-16062] Reland Remove-Reconfigure + cascade fix consolidation#6098
Open
danielbotros wants to merge 3 commits into
Open
[APP-16062] Reland Remove-Reconfigure + cascade fix consolidation#6098danielbotros wants to merge 3 commits into
danielbotros wants to merge 3 commits into
Conversation
… only (viamrobotics#5997)" This reverts commit c7fc683.
…ascade through cascadeRebuildDependentsOf
…necessary snapshot capture
2571829 to
72050f3
Compare
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
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.
Summary
Relands Remove-Reconfigure after #5997
Commits
1. Revert of #5997 (
8e54770cf)Restores the post-#5944 state — Remove-Reconfigure + cascade fix together — by reverting the previous PR. The
git revertproduced merge conflicts in four files due to drift on main during the bake window (primarily from #6041, RSDK-13417, "skip weak/optional dep reconfigure when unchanged"). Resolved by preserving #6041's snapshot-tracking + skip-when-unchanged optimization adapted to Remove-Reconfigure semantics.Main conflicts:
robot/impl/resource_manager.go— 3 conflict regions:Remote-resource discovery (around line 305):
nodeAlreadyExistsbranching. HEAD had an invertedif !nodeAlreadyExistscondition with an unconditionalSwapResourcelater in the function; parent hadif nodeAlreadyExists { SwapResource } else { ... }. Took HEAD's structure — its end-of-blockSwapResourceadvances the clock for both new and existing nodes, which snapshot tracking depends on. Parent only advanced for existing. Side fix: parent'sgraph_node.goadds anincrementClock boolparameter toSwapResource, so the post-merge call site needed, trueappended.processResourcedependency fetch (around line 1125). HEAD usedgetDependenciesWithWeakOptionalSnapshot; parent used the simplergetDependencies. Initially merged hybrid (snapshot variant, discard deps), later reverted to parent verbatim (_, err := lr.getDependencies(...)) — see the fix in commit 3.processResourcerebuild path (lines 1140-1167): in-place reconfigure vs. always-rebuild. HEAD had a biggNode.ResourceModel() == conf.Modelblock callingmoduleManager.ReconfigureResourceorcurrentRes.ReconfigurewithMustRebuildErrorfallback, returning(resource.Resource, bool, error). Parent had no in-place path and always rebuilt, returning(resource.Resource, error). Took parent entirely — Remove-Reconfigure removes the publicReconfiguremethod andmoduleManager.ReconfigureResource, so the in-place branch can't exist. Function signature is back to 2-value; caller'smarkChildrenForUpdatebecomes unconditional.robot/impl/local_robot.go— 1 conflict region:updateWeakAndOptionalDependentsaction (around line 1180). HEAD had skip-when-unchanged guard (weakOptionalDepClocksEqual) plus an in-placeReconfigure. Parent had no skip guard and aBuiltInReconfigurefor internal resources and inline rebuild for any other resource. Hybrid: kept HEAD's skip guard + "handling weak/optional update" log and took parent'sBuiltInReconfigure/ inline-rebuild branch.Test-file conflicts in
robot/impl/optional_dependencies_test.goandrobot/impl/robot_reconfigure_weak_dependencies_test.goare not enumerated here (many); they were resolved by resetting both test files to parent and selectively restoring the new tests/helpers #6041 added.2. Cleanup (
d73416078)I noticed some rebuild/cascade logic duplication within
rebuildResource,cascadeRebuildDependentsOf, andReconfigureResourceso I took the chance to clean it up. I mergedrebuildResourceandrebuildResourceWithVisitedinto a singlerebuildResourcethat takes avisitedparameter (nil from outer callers, threaded from recursive ones), and replace the inline cascade walk inside the rebuild path with a call tocascadeRebuildDependentsOf(now also taking avisitedparameter). Both theaddResourcepost-add hook and therebuildResourcepost-rebuild step now go through the same cascade implementation.3. Post-merge audit (
257182945)A combing pass over the merge product to fix problems my initial conflict resolution introduced:
Removed redundant snapshot capture in
processResource. My initial merge for conflict 2 + conflict 3 capturedweakOptionalSnapshotinprocessResourceand calledgNode.SetLastWeakOptionalDepsClocks(weakOptionalSnapshot)after a successful rebuild. ButnewResourcealready records the snapshot with its own freshly-captured value taken at construction time. Reverted both conflicts to parent verbatim.newResourceis the single authoritative place that records the snapshot.Restored missing tests dropped during the conflict reset.
Comment audit across both test files. Many comments retained pre-[APP-16062] Reapply Remove Reconfigure + Rebuild stale module-internal dependents when a dependency is re-added #5944 narrative referring to a resource-level
Reconfigureor pre-RSDK-13417: Skip weak/optional dependency reconfigure when resolved set is unchanged #6041 narrative claimingupdateWeakAndOptionalDependentsalways fires a rebuild. I rewrote these to reflect the latest set of changes.