fix(hip-0025): thread context through updateAndWait and chartPath through nested subcharts

Two findings from Copilot's review of the consolidated commit on 2026-05-03:

- updateAndWait silently dropped its context arg (named `_`), so sequenced
  upgrades and rollbacks never honored cancellation through Build/Update/Wait.
  Thread the context through and add ctx.Done() select gates matching
  createAndWait.

- GroupManifestsByDirectSubchart used the bare chart name as path prefix.
  At top level this is fine because manifest names are rooted there, but on
  recursion into a subchart the call passed the subchart's own name (e.g.
  "sub") while manifest names are still rooted at the top-level chart
  ("parent/charts/sub/charts/nested/..."). Result: 3+ deep nesting flattened
  into the parent subchart's batch, silently bypassing nested sequencing.

  Fix: thread chartPath through deployChartLevel and deleteChartLevelReverse
  so each recursion level sees the full path-prefix it should match against.
  Add a unit test exercising the deeper-recursion case and update the
  uninstall test that had documented this as "a known limitation."

Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>

pkg/action/sequencing.go

@@ -45,32 +45,29 @@ func computeDeadline(timeout time.Duration) time.Time {
 }
 
 // GroupManifestsByDirectSubchart groups manifests by the direct subchart they belong to.
-// The current chart's own manifests are returned under the empty string key "".
-// Subcharts are keyed by their immediate name under the first `<chartName>/charts/<subchart>/`
-// segment found in the manifest source path.
-// Nested subcharts (e.g., `<chartName>/charts/sub/charts/nested/`) are grouped under
-// the direct subchart name ("sub"), since nested sequencing is handled recursively.
-func GroupManifestsByDirectSubchart(manifests []releaseutil.Manifest, chartName string) map[string][]releaseutil.Manifest {
+// chartPath is the full path-prefix for the current chart level — at the top level
+// it is the chart name (e.g. "parent"); at deeper recursion levels it is the joined
+// path through "/charts/" segments (e.g. "parent/charts/sub").
+// The current chart level's own manifests are returned under the empty string key "".
+// Direct subcharts are keyed by their immediate directory name under
+// "<chartPath>/charts/<subchart>/". Nested grandchildren are grouped under their
+// direct subchart parent ("sub"), since nested sequencing is handled recursively.
+func GroupManifestsByDirectSubchart(manifests []releaseutil.Manifest, chartPath string) map[string][]releaseutil.Manifest {
 	result := make(map[string][]releaseutil.Manifest)
-	if chartName == "" {
-		// Fallback: assign everything to parent
+	if chartPath == "" {
 		result[""] = append(result[""], manifests...)
 		return result
 	}
 
-	chartsPrefix := chartName + "/charts/"
+	chartsPrefix := chartPath + "/charts/"
 	for _, m := range manifests {
 		if !strings.HasPrefix(m.Name, chartsPrefix) {
-			// Parent chart manifest
 			result[""] = append(result[""], m)
 			continue
 		}
-		// Extract the direct subchart name (first segment after "<chartName>/charts/")
 		rest := m.Name[len(chartsPrefix):]
-		// rest is like "subchart1/templates/deploy.yaml" or "subchart1/charts/nested/..."
 		subchartName, _, ok := strings.Cut(rest, "/")
 		if !ok {
-			// Unlikely: a file directly under charts/ with no subdirectory
 			result[""] = append(result[""], m)
 			continue
 		}
@@ -126,10 +123,15 @@ type sequencedDeployment struct {
 // It first handles subcharts in dependency order (recursively), then deploys the
 // parent chart's own resource-group batches.
 func (s *sequencedDeployment) deployChartLevel(ctx context.Context, chrt *chartv2.Chart, manifests []releaseutil.Manifest) error {
-	// Group manifests by direct subchart
-	grouped := GroupManifestsByDirectSubchart(manifests, chrt.Name())
+	return s.deployChartLevelAt(ctx, chrt, manifests, chrt.Name())
+}
+
+// deployChartLevelAt is the recursive worker. chartPath tracks the manifest
+// path-prefix of the current chart level so nested subcharts route correctly:
+// top-level "parent" → child "parent/charts/sub" → grandchild "parent/charts/sub/charts/nested".
+func (s *sequencedDeployment) deployChartLevelAt(ctx context.Context, chrt *chartv2.Chart, manifests []releaseutil.Manifest, chartPath string) error {
+	grouped := GroupManifestsByDirectSubchart(manifests, chartPath)
 
-	// Build subchart DAG and deploy in topological order
 	dag, err := chartutil.BuildSubchartDAG(chrt)
 	if err != nil {
 		return fmt.Errorf("building subchart DAG for %s: %w", chrt.Name(), err)
@@ -140,7 +142,6 @@ func (s *sequencedDeployment) deployChartLevel(ctx context.Context, chrt *chartv
 		return fmt.Errorf("getting subchart batches for %s: %w", chrt.Name(), err)
 	}
 
-	// Deploy each subchart batch in order
 	for batchIdx, batch := range batches {
 		for _, subchartName := range batch {
 			subManifests := grouped[subchartName]
@@ -148,11 +149,8 @@ func (s *sequencedDeployment) deployChartLevel(ctx context.Context, chrt *chartv
 				continue
 			}
 
-			// Find the subchart chart object for recursive nested sequencing
 			subChart := findSubchart(chrt, subchartName)
 			if subChart == nil {
-				// Subchart not found in chart object (may have been disabled or aliased differently)
-				// Fall back to flat resource-group deployment for these manifests
 				s.cfg.Logger().Warn("subchart not found in chart dependencies; deploying without subchart sequencing",
 					"subchart", subchartName,
 					"batch", batchIdx,
@@ -163,14 +161,13 @@ func (s *sequencedDeployment) deployChartLevel(ctx context.Context, chrt *chartv
 				continue
 			}
 
-			// Recursively deploy the subchart (handles its own nested subcharts and resource-groups)
-			if err := s.deployChartLevel(ctx, subChart, subManifests); err != nil {
+			subPath := chartPath + "/charts/" + subchartName
+			if err := s.deployChartLevelAt(ctx, subChart, subManifests, subPath); err != nil {
 				return fmt.Errorf("deploying subchart %s: %w", subchartName, err)
 			}
 		}
 	}
 
-	// Deploy parent chart's own resources (after all subchart batches complete)
 	parentManifests := grouped[""]
 	if len(parentManifests) > 0 {
 		if err := s.deployResourceGroupBatches(ctx, parentManifests); err != nil {
@@ -392,7 +389,12 @@ func (s *sequencedDeployment) createAndWait(ctx context.Context, manifests []rel
 
 // updateAndWait applies an upgrade batch using KubeClient.Update() and waits for readiness.
 // It matches current (old) resources by objectKey to compute the per-batch diff.
-func (s *sequencedDeployment) updateAndWait(_ context.Context, manifests []releaseutil.Manifest) error {
+func (s *sequencedDeployment) updateAndWait(ctx context.Context, manifests []releaseutil.Manifest) error {
+	select {
+	case <-ctx.Done():
+		return ctx.Err()
+	default:
+	}
 	if len(manifests) == 0 {
 		return nil
 	}
@@ -414,9 +416,12 @@ func (s *sequencedDeployment) updateAndWait(_ context.Context, manifests []relea
 		return fmt.Errorf("stripping sequencing annotations: %w", err)
 	}
 
-	// Find the subset of current (old) resources that are represented in this batch.
-	// Update() will handle creates (target resources not in matchingCurrent) and
-	// updates (resources in both). Deletions are handled separately after all batches.
+	select {
+	case <-ctx.Done():
+		return ctx.Err()
+	default:
+	}
+
 	targetKeys := make(map[string]bool, len(target))
 	for _, r := range target {
 		targetKeys[objectKey(r)] = true
@@ -440,6 +445,12 @@ func (s *sequencedDeployment) updateAndWait(_ context.Context, manifests []relea
 	}
 	s.createdResources = append(s.createdResources, result.Created...)
 
+	select {
+	case <-ctx.Done():
+		return ctx.Err()
+	default:
+	}
+
 	return s.waitForResources(target, manifests)
 }
 

pkg/action/sequencing_test.go

@@ -312,6 +312,24 @@ func TestSequencing_GroupManifestsByDirectSubchart(t *testing.T) {
 	require.Len(t, grouped["database"], 2)
 }
 
+// TestSequencing_GroupManifestsByDirectSubchart_Nested verifies that when called
+// with a deeper chartPath (i.e., during recursion into a subchart), nested
+// grandchildren are routed to the correct subchart key instead of being merged
+// into the parent batch.
+func TestSequencing_GroupManifestsByDirectSubchart_Nested(t *testing.T) {
+	manifests := []releaseutil.Manifest{
+		makeTestManifest("db-own", "parent/charts/database/templates/one.yaml", nil),
+		makeTestManifest("cache", "parent/charts/database/charts/cache/templates/one.yaml", nil),
+	}
+
+	grouped := GroupManifestsByDirectSubchart(manifests, "parent/charts/database")
+
+	require.Len(t, grouped[""], 1, "database's own resources should be under the empty key")
+	require.Len(t, grouped["cache"], 1, "nested cache subchart should be routed under its own key")
+	require.Equal(t, "parent/charts/database/templates/one.yaml", grouped[""][0].Name)
+	require.Equal(t, "parent/charts/database/charts/cache/templates/one.yaml", grouped["cache"][0].Name)
+}
+
 func TestSequencing_BuildManifestYAML(t *testing.T) {
 	yaml := buildManifestYAML([]releaseutil.Manifest{
 		makeTestManifest("one", "chart/templates/one.yaml", nil),

pkg/action/uninstall.go

@@ -341,11 +341,11 @@ func (u *Uninstall) sequencedDeleteManifests(chrt *chart.Chart, manifests []rele
 	if chrt == nil {
 		return u.deleteResourceGroupBatchesReverse(manifests, waiter, deadline)
 	}
-	return u.deleteChartLevelReverse(chrt, manifests, waiter, deadline)
+	return u.deleteChartLevelReverseAt(chrt, manifests, waiter, deadline, chrt.Name())
 }
 
-func (u *Uninstall) deleteChartLevelReverse(chrt *chart.Chart, manifests []releaseutil.Manifest, waiter kube.Waiter, deadline time.Time) (kube.ResourceList, []error) {
-	grouped := GroupManifestsByDirectSubchart(manifests, chrt.Name())
+func (u *Uninstall) deleteChartLevelReverseAt(chrt *chart.Chart, manifests []releaseutil.Manifest, waiter kube.Waiter, deadline time.Time, chartPath string) (kube.ResourceList, []error) {
+	grouped := GroupManifestsByDirectSubchart(manifests, chartPath)
 
 	allDeleted, errs := u.deleteResourceGroupBatchesReverse(grouped[""], waiter, deadline)
 	if len(errs) > 0 {
@@ -375,11 +375,12 @@ func (u *Uninstall) deleteChartLevelReverse(chrt *chart.Chart, manifests []relea
 
 			subchart := findSubchart(chrt, subchartName)
 			var deleted kube.ResourceList
+			subPath := chartPath + "/charts/" + subchartName
 			if subchart == nil {
 				u.cfg.Logger().Warn("subchart not found in chart dependencies during sequenced uninstall; falling back to flat resource-group deletion", "subchart", subchartName)
 				deleted, errs = u.deleteResourceGroupBatchesReverse(subchartManifests, waiter, deadline)
 			} else {
-				deleted, errs = u.deleteChartLevelReverse(subchart, subchartManifests, waiter, deadline)
+				deleted, errs = u.deleteChartLevelReverseAt(subchart, subchartManifests, waiter, deadline, subPath)
 			}
 			allDeleted = append(allDeleted, deleted...)
 			if len(errs) > 0 {

pkg/action/uninstall_sequenced_test.go

@@ -231,11 +231,11 @@ func TestUninstall_Sequenced_WithSubcharts(t *testing.T) {
 	_, err := uninstall.Run(rel.Name)
 	require.NoError(t, err)
 
-	// Nested subcharts within a subchart's subtree are flattened into one delete
-	// batch because GroupManifestsByDirectSubchart receives full-path manifest
-	// names ("parent/charts/bar/charts/nginx/...") that don't match the nested
-	// chart prefix ("bar/charts/"). This is a known limitation.
-	assert.Equal(t, [][]string{{"ConfigMap/parent"}, {"ConfigMap/nginx", "ConfigMap/bar"}}, client.deleteCalls)
+	// Reverse-DAG uninstall: parent first, then bar (its own resources before
+	// recursing into bar's subchart batch), then nginx (innermost). With the
+	// chart-path threading fix, nested subcharts route through their proper
+	// subtree level instead of flattening.
+	assert.Equal(t, [][]string{{"ConfigMap/parent"}, {"ConfigMap/bar"}, {"ConfigMap/nginx"}}, client.deleteCalls)
 	assert.Equal(t, client.deleteCalls, client.deleteWaitCalls)
 }