fix: prevent PathsGenerator crash when keboola.variables config has multiple parents

When a keboola.variables config is referenced by more than one consumer
config via the API, the relation mapper creates multiple variablesFor
relations on it. The single-pass AfterRemoteOperation removes these
duplicates and emits a warning — but only when consumers are iterated
before the variables config. If the variables config is iterated first
(the order used by the Templates service), the duplicate relations are
not cleaned up before PathsGenerator runs, causing it to crash with
"unexpected state: multiple parents defined by relations".

Fix: change Config.ParentKey() to fall back to the structural parent
(branch) when Relations.ParentKey() returns a multiple-parents error,
instead of propagating the fatal error. This makes PathsGenerator
non-fatal for the shared-variables scenario regardless of iteration
order. The duplicate-relations warning is still emitted correctly by
validateRelations in AfterRemoteOperation.

Also reverts the earlier two-pass link.go change which introduced
different warning messages and broke the variables-used-twice E2E test.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: Matovidlo

docs/relations-validation.md

@@ -29,35 +29,44 @@ Keboola's Storage API allows a variables config to be referenced by more than on
 
 ## The `multiple parents` guard
 
-`Relations.ParentKey()` (`internal/pkg/model/relation.go:66`) collects all relations that define a parent key. If more than one is found it returns a fatal error:
+`Relations.ParentKey()` (`internal/pkg/model/relation.go:66`) collects all relations that define a parent key. If more than one is found it returns an error:
 
 ```
 unexpected state: multiple parents defined by "relations" in <config desc>
 ```
 
-This guard exists to protect the CLI sync engine: without it, path generation would be ambiguous and the local directory structure would be corrupted.
+This guard exists to detect invalid state in the relation graph.
 
-## The ordering bug that broke Templates
+## Config.ParentKey() and PathsGenerator
 
-The relation mapper (`internal/pkg/mapper/relations/link.go`) processes objects in `AfterRemoteOperation` and `AfterLocalOperation`. It also validates them in the same loop iteration (link → validate per object). This is order-dependent:
+`Config.ParentKey()` (`internal/pkg/model/object.go`) calls `Relations.ParentKey()` to find the relation-defined parent. If that returns an error (multiple parents) or nil (no parent), it falls back to the structural parent — the branch.
 
-1. If the variables config Y is iterated **before** its consumers X and Z, `validateRelations(Y)` runs when Y has zero `variablesFor` relations — nothing to clean up.
-2. Later iterations for X and Z call `linkRelations`, which adds `VariablesForRelation` entries to Y.
-3. After the mapper loop, `PathsGenerator.Invoke()` (called in `remote/manager.go`) calls `Y.ParentKey()`, finds two parents, and returns the fatal error.
+`PathsGenerator.doUpdate()` (`internal/pkg/state/local/paths.go`) calls `object.ParentKey()` on every loaded object to build local directory paths. With the branch-fallback in `Config.ParentKey()`, a variables config that has multiple `variablesFor` relations is placed at the branch root (e.g. `main/variables/`) rather than inside any specific parent folder. This is non-fatal: PathsGenerator can complete and remote state loading succeeds.
+
+## The ordering issue
 
-CLI `pull` was unaffected in practice because the API happened to return consumer configs before variables configs, making consumers iterated first. The Templates service load path had a different iteration order and hit the bug.
+The relation mapper (`internal/pkg/mapper/relations/link.go`) processes objects in `AfterRemoteOperation` and `AfterLocalOperation`. It links and validates each object in a single pass:
 
-## The fix: two-pass link and validate
+```
+for each object:
+    linkRelations(object)    // adds other-side relations to OTHER objects
+    validateRelations(object) // validates THIS object's relations
+```
+
+This is order-dependent. If the variables config Y is iterated **before** its consumers X and Z:
+
+1. `validateRelations(Y)` runs when Y has zero `variablesFor` relations — nothing to clean up.
+2. Later iterations for X and Z call `linkRelations`, which adds `VariablesForRelation` entries to Y.
+3. Y ends up with two `variablesFor` relations in the loaded remote state.
+4. `PathsGenerator.doUpdate()` calls `Y.ParentKey()` — before the branch-fallback fix this was a fatal error.
 
-`AfterRemoteOperation` and `AfterLocalOperation` were refactored into two explicit passes:
+CLI `pull` was unaffected in practice because the API happened to return consumer configs before variables configs, making consumers iterated first. The Templates service load path had a different iteration order and triggered step 3-4.
 
-**Pass 1 — link all objects**
-Run `linkRelations` for every loaded object. This creates all other-side relations (including adding `variablesFor` entries to every variables config) before any validation happens.
+## The fix: branch fallback in Config.ParentKey()
 
-**Pass 2 — validate all objects**
-Run `validateRelations` for every loaded object. With the complete relation graph now in place, duplicates are correctly detected, removed, and logged as warnings — regardless of the order objects appear in the API response.
+`Config.ParentKey()` was changed to ignore the "multiple parents" error from `Relations.ParentKey()` and fall back to the branch (structural parent) instead of propagating the error. This makes PathsGenerator non-fatal for shared-variables configs regardless of iteration order.
 
-This makes behaviour order-independent: both CLI and the Templates service emit a warning and continue when a variables config has multiple parents, instead of crashing. No new flags or special-casing were required.
+The `validateRelations` function still detects the duplicate `variablesFor` relations and logs a warning. In CLI `pull`, this warning fires correctly when consumers are iterated before the variables config (the API's typical return order). In the Templates service load path the warning may fire differently (or not at all for the duplicate, depending on iteration order), but Templates is only reading remote state — it never generates a local directory hierarchy — so the warning is not critical there.
 
 ## Why this is warning-only, not a hard error
 
@@ -69,8 +78,9 @@ Templates only reads remote state to discover existing configs and apply templat
 
 | File | Role |
 |---|---|
-| `internal/pkg/mapper/relations/link.go` | Two-pass AfterRemoteOperation / AfterLocalOperation |
+| `internal/pkg/model/object.go` | `Config.ParentKey()` — branch fallback when multiple relation parents |
 | `internal/pkg/model/relation.go` | `Relations.ParentKey()`, `OneToXRelations()` |
+| `internal/pkg/mapper/relations/link.go` | Single-pass AfterRemoteOperation / AfterLocalOperation |
 | `internal/pkg/model/variables.go` | `VariablesForRelation`, `VariablesFromRelation` definitions |
 | `internal/pkg/state/remote/manager.go` | Sequencing of AfterRemoteOperation → PathsGenerator |
 | `test/cli/pull/variables-used-twice/` | E2E test covering the shared-variables warning |

internal/pkg/mapper/relations/link.go

@@ -12,27 +12,9 @@ import (
 func (m *relationsMapper) AfterLocalOperation(ctx context.Context, changes *model.LocalChanges) error {
 	errs := errors.NewMultiError()
 	allObjects := m.state.LocalObjects()
-	loaded := changes.Loaded()
-
-	// Pass 1: link all objects so every other-side relation exists before validation.
-	// Processing link and validate in a single loop would cause order-dependent behaviour:
-	// a variables config iterated before its consumers would be validated before the consumer
-	// linking steps add VariablesForRelation entries to it, leaving duplicates undetected until
-	// PathsGenerator runs and hits a fatal "multiple parents" error.
-	for _, objectState := range loaded {
-		if o, ok := objectState.LocalState().(model.ObjectWithRelations); ok {
-			if err := m.linkRelations(o, allObjects); err != nil {
-				errs.Append(err)
-			}
-		}
-	}
-
-	// Pass 2: validate all objects now that the relation graph is complete.
-	for _, objectState := range loaded {
-		if o, ok := objectState.LocalState().(model.ObjectWithRelations); ok {
-			if err := m.validateRelations(o); err != nil {
-				errs.Append(errors.PrefixErrorf(err, "invalid %s", objectState.LocalState().Desc()))
-			}
+	for _, objectState := range changes.Loaded() {
+		if err := m.linkAndValidateRelations(objectState.LocalState(), allObjects); err != nil {
+			errs.Append(err)
 		}
 	}
 
@@ -48,24 +30,9 @@ func (m *relationsMapper) AfterLocalOperation(ctx context.Context, changes *mode
 func (m *relationsMapper) AfterRemoteOperation(ctx context.Context, changes *model.RemoteChanges) error {
 	errs := errors.NewMultiError()
 	allObjects := m.state.RemoteObjects()
-	loaded := changes.Loaded()
-
-	// Pass 1: link all objects so every other-side relation exists before validation.
-	// See AfterLocalOperation for the reasoning behind the two-pass approach.
-	for _, objectState := range loaded {
-		if o, ok := objectState.RemoteState().(model.ObjectWithRelations); ok {
-			if err := m.linkRelations(o, allObjects); err != nil {
-				errs.Append(err)
-			}
-		}
-	}
-
-	// Pass 2: validate all objects now that the relation graph is complete.
-	for _, objectState := range loaded {
-		if o, ok := objectState.RemoteState().(model.ObjectWithRelations); ok {
-			if err := m.validateRelations(o); err != nil {
-				errs.Append(errors.PrefixErrorf(err, "invalid %s", objectState.RemoteState().Desc()))
-			}
+	for _, objectState := range changes.Loaded() {
+		if err := m.linkAndValidateRelations(objectState.RemoteState(), allObjects); err != nil {
+			errs.Append(err)
 		}
 	}
 
@@ -77,6 +44,19 @@ func (m *relationsMapper) AfterRemoteOperation(ctx context.Context, changes *mod
 	return nil
 }
 
+func (m *relationsMapper) linkAndValidateRelations(object model.Object, allObjects model.Objects) error {
+	errs := errors.NewMultiError()
+	if o, ok := object.(model.ObjectWithRelations); ok {
+		if err := m.linkRelations(o, allObjects); err != nil {
+			errs.Append(err)
+		}
+		if err := m.validateRelations(o); err != nil {
+			errs.Append(errors.PrefixErrorf(err, "invalid %s", object.Desc()))
+		}
+	}
+	return errs.ErrorOrNil()
+}
+
 // lintRelations finds the other side of the relation and create a corresponding relation on the other side.
 func (m *relationsMapper) linkRelations(object model.ObjectWithRelations, allObjects model.Objects) error {
 	errs := errors.NewMultiError()

internal/pkg/model/object.go

@@ -595,14 +595,17 @@ func (r *ConfigRow) GetContent() *orderedmap.OrderedMap {
 }
 
 // ParentKey - config parent can be modified via Relations, for example variables config is embedded in another config.
+// If multiple relation-defined parents exist (e.g. a shared keboola.variables config referenced by more than one
+// consumer), the relations error is ignored and the config falls back to its structural parent (branch). The
+// duplicate-relations validation in AfterRemoteOperation logs a warning and removes the extra relations; until that
+// cleanup runs, PathsGenerator must not crash so that remote state loading can complete.
 func (c *Config) ParentKey() (Key, error) {
-	if parentKey, err := c.Relations.ParentKey(c.Key()); err != nil {
-		return nil, err
-	} else if parentKey != nil {
+	parentKey, err := c.Relations.ParentKey(c.Key())
+	if err == nil && parentKey != nil {
 		return parentKey, nil
 	}
 
-	// No parent defined via "Relations" -> parent is branch
+	// No relation-defined parent (or multiple parents — treat as no parent) -> parent is branch.
 	return c.ConfigKey.ParentKey()
 }