diff --git a/platform/differ.go b/platform/differ.go index a925129f..74d98f50 100644 --- a/platform/differ.go +++ b/platform/differ.go @@ -88,13 +88,25 @@ func ComputePlan(ctx context.Context, p interfaces.IaCProvider, desired []interf // emitted synchronously since they don't need the provider. var creates []interfaces.PlanAction type modCandidate struct { - spec interfaces.ResourceSpec - rs interfaces.ResourceState - hash string // precomputed configHash(spec.Config); reused by classifyModification + spec interfaces.ResourceSpec + rs interfaces.ResourceState + hash string // precomputed configHash(spec.Config); reused by classifyModification + hashable bool // true when configHashErr succeeded; false → cache-bypass for this candidate } var candidates []modCandidate for _, spec := range desired { - hash := configHash(spec.Config) + // Use configHashErr to detect non-marshalable spec.Config values + // so we can bypass the diff-cache for those resources. For the + // stored ResolvedConfigHash we fall back to the legacy + // configHash (sha256-of-best-effort) when configHashErr fails; + // that preserves byte-for-byte stability with any persisted + // plans + ConfigHash values produced under the pre-T3.6 + // implementation, while the hashable flag gates cache + // participation for the modification path. + hash, hashErr := configHashErr(spec.Config) + if hashErr != nil { + hash = configHash(spec.Config) + } if rs, exists := currentMap[spec.Name]; !exists { creates = append(creates, interfaces.PlanAction{ Action: "create", @@ -102,7 +114,12 @@ func ComputePlan(ctx context.Context, p interfaces.IaCProvider, desired []interf ResolvedConfigHash: hash, }) } else { - candidates = append(candidates, modCandidate{spec: spec, rs: rs, hash: hash}) + candidates = append(candidates, modCandidate{ + spec: spec, + rs: rs, + hash: hash, + hashable: hashErr == nil, + }) } } @@ -116,7 +133,7 @@ func ComputePlan(ctx context.Context, p interfaces.IaCProvider, desired []interf g.SetLimit(planDiffConcurrency()) for i := range candidates { g.Go(func() error { - return classifyModification(gctx, p, candidates[i].spec, candidates[i].rs, candidates[i].hash, &mods[i]) + return classifyModification(gctx, p, candidates[i].spec, candidates[i].rs, candidates[i].hash, candidates[i].hashable, &mods[i]) }) } if err := g.Wait(); err != nil { @@ -180,8 +197,13 @@ func ComputePlan(ctx context.Context, p interfaces.IaCProvider, desired []interf // via the legacy ConfigHash compare. The hash argument is the // precomputed configHash(spec.Config), threaded in by the caller so // the per-candidate hashing happens once during candidate-bucketing -// rather than redundantly here on every Diff dispatch. -func classifyModification(ctx context.Context, p interfaces.IaCProvider, spec interfaces.ResourceSpec, rs interfaces.ResourceState, hash string, out **interfaces.PlanAction) error { +// rather than redundantly here on every Diff dispatch. The hashable +// argument is true when configHashErr succeeded for spec.Config; when +// false, the cache is bypassed for this resource (same defensive +// treatment as the empty-ProviderID path) — collapsing all +// non-marshalable inputs to the constant sha256("") hash would risk +// cache-key collisions across distinct resources. +func classifyModification(ctx context.Context, p interfaces.IaCProvider, spec interfaces.ResourceSpec, rs interfaces.ResourceState, hash string, hashable bool, out **interfaces.PlanAction) error { rsCopy := rs // Nil-provider fallback: legacy ConfigHash compare. @@ -231,25 +253,45 @@ func classifyModification(ctx context.Context, p interfaces.IaCProvider, spec in // Always re-dispatch in that case; the cost is one extra Diff call, // not correctness. var diff *interfaces.DiffResult - cacheable := rs.ProviderID != "" + // cacheable = (1) ProviderID is the disambiguator across same-Type + // resources (see the empty-ProviderID bypass docstring just above + // the if-block) AND (2) we can deterministically hash both + // spec.Config and rs.Outputs (configHashErr surfaces marshal + // failures on non-YAML-derivable values so we don't collapse all + // unmarshalable inputs to the constant sha256("") hash and serve + // each other's cached DiffResult). Bypass cases all fall through + // to the unconditional driver.Diff dispatch below. + cacheable := rs.ProviderID != "" && hashable var ( - cache diffcache.Cache - key diffcache.Key + cache diffcache.Cache + key diffcache.Key + shaOut string ) + if cacheable { + var outErr error + shaOut, outErr = configHashErr(rs.Outputs) + if outErr != nil { + // Outputs contained a non-marshalable value; bypass cache + // for this resource (same defensive treatment as the + // empty-ProviderID and unhashable-spec.Config paths) and + // re-Diff every time. + cacheable = false + } + } if cacheable { // getDiffCache lives inside this branch so the empty-ProviderID - // bypass path is fully side-effect free — no sync.Once init - // firing, no atomic load, and (most importantly) no eager - // lazy-construction of the filesystem cache backend creating - // ~/.cache/wfctl/diff/ on the operator's machine for resources - // that won't use the cache anyway. + // AND unhashable-inputs bypass paths are fully side-effect free + // — no sync.Once init firing, no atomic load, and (most + // importantly) no eager lazy-construction of the filesystem + // cache backend creating ~/.cache/wfctl/diff/ on the operator's + // machine for resources that won't use the cache anyway. cache = getDiffCache() key = diffcache.Key{ PluginVersion: pluginVersionKey(p), Type: spec.Type, ProviderID: rs.ProviderID, SHAConfig: hash, - SHAOutputs: configHash(rs.Outputs), + SHAOutputs: shaOut, } if cached, hit := cache.Get(key); hit { c := cached @@ -473,6 +515,20 @@ func ConfigHash(config map[string]any) string { // configHash returns a deterministic SHA-256 hex hash of a config map. // Keys are explicitly sorted before marshalling so the hash is stable across // Go's randomised map-iteration order — matching the DO plugin's pattern. +// +// configHash IGNORES json.Marshal errors. If the config contains a value +// json.Marshal cannot encode (channels, functions, cycles, custom types +// with broken MarshalJSON), this collapses to the constant +// sha256() = e3b0c4429... hash. That preserves the legacy "did +// anything change" comparison semantics for YAML-derived configs +// (YAML can't express those types, so the failure mode is unreachable +// in practice for those callers) and keeps the exported ConfigHash +// byte-for-byte stable against any persisted ResolvedConfigHash values +// computed under the pre-T3.6 implementation. Cache-key callers MUST +// use configHashErr instead — collapsing all unmarshalable inputs to +// the same constant hash would cause cache-key collisions and let two +// genuinely-different resources serve each other's cached DiffResult. +// See the differ.go:235 comment block for the cache-bypass plumbing. func configHash(config map[string]any) string { if len(config) == 0 { return "" @@ -494,6 +550,45 @@ func configHash(config map[string]any) string { return fmt.Sprintf("%x", sha256.Sum256(data)) } +// configHashErr is the error-aware variant of configHash, used by the +// diff-cache keying path so non-marshalable inputs bypass caching for +// that resource (rather than silently collapsing to the constant +// sha256("") hash and risking cache-key collisions across distinct +// resources). When err != nil, the returned string is the empty +// hash and callers MUST NOT use it for cache keys; they should +// re-Diff every time for that resource (same defensive bypass as +// the empty-ProviderID path documented at differ.go:235). +// +// In practice IaC configs come from YAML and cannot contain +// non-marshalable values (channels, functions, cycles), so this +// path is unreachable for the common case. Defensive coverage is +// for custom-provider Outputs that could conceivably contain +// types with broken MarshalJSON, or for future code paths that +// pass non-YAML-derived configs into ComputePlan. +func configHashErr(config map[string]any) (string, error) { + if len(config) == 0 { + return "", nil + } + keys := make([]string, 0, len(config)) + for k := range config { + keys = append(keys, k) + } + sort.Strings(keys) + type kv struct { + K string + V any + } + ordered := make([]kv, len(keys)) + for i, k := range keys { + ordered[i] = kv{K: k, V: config[k]} + } + data, err := json.Marshal(ordered) + if err != nil { + return "", fmt.Errorf("configHash: marshal: %w", err) + } + return fmt.Sprintf("%x", sha256.Sum256(data)), nil +} + // planID generates a simple unique plan ID based on current time. func planID() string { return fmt.Sprintf("plan-%d", time.Now().UnixNano()) diff --git a/platform/differ_cache_test.go b/platform/differ_cache_test.go index 2d23d5d3..ca5ba427 100644 --- a/platform/differ_cache_test.go +++ b/platform/differ_cache_test.go @@ -575,3 +575,97 @@ func TestComputePlan_NilDiffResult_CachesAsZeroValue(t *testing.T) { t.Errorf("after second ComputePlan: Diff calls = %d, want 1 (cache hit on zero-value DiffResult; round-5 fix)", got) } } + +// TestComputePlan_UnhashableInputs_BypassCache pins the round-9 fix: +// when spec.Config or rs.Outputs contains a non-marshalable value +// (channel, function, cycle), configHashErr surfaces the marshal +// failure and ComputePlan bypasses the diff cache for that resource — +// rather than collapsing all unmarshalable inputs to the constant +// sha256("") hash and risking cache-key collisions across distinct +// resources serving each other's cached DiffResult. +// +// In practice IaC configs come from YAML and cannot contain these +// types, so this is defensive coverage for custom-provider Outputs +// or future code paths. +func TestComputePlan_UnhashableInputs_BypassCache(t *testing.T) { + setDiffCacheForTest(t, diffcache.NewMemory()) + + driver := &cacheTestDriver{diff: &interfaces.DiffResult{NeedsUpdate: true}} + provider := &cacheTestProvider{name: "fake", version: "0.0.0-test", driver: driver} + + // Two resources, both with non-marshalable values in Outputs (a + // channel, which json.Marshal rejects). Without the bypass, both + // would hash to the constant sha256("") and the second resource + // would be served the first's cached DiffResult. + unmarshalable := make(chan int) + desired := []interfaces.ResourceSpec{ + {Name: "vpc-a", Type: "infra.vpc", Config: map[string]any{"region": "nyc3"}}, + {Name: "vpc-b", Type: "infra.vpc", Config: map[string]any{"region": "sfo3"}}, + } + current := []interfaces.ResourceState{ + {Name: "vpc-a", Type: "infra.vpc", ProviderID: "pid-a", Outputs: map[string]any{"poison": unmarshalable}}, + {Name: "vpc-b", Type: "infra.vpc", ProviderID: "pid-b", Outputs: map[string]any{"poison": unmarshalable}}, + } + + if _, err := ComputePlan(context.Background(), provider, desired, current); err != nil { + t.Fatalf("ComputePlan: %v", err) + } + if got := driver.diffCount.Load(); got != 2 { + t.Errorf("Diff calls = %d, want 2 (one per resource on first pass)", got) + } + // Second pass: both resources should re-Diff (cache bypassed), + // not share a cached entry. Without the round-9 fix, both + // resources collapse to the same SHAOutputs="" key on the first + // pass, only one cache.Put happens (overwriting), and the + // second pass hits cache for both — diffCount would stop at 3 + // (one re-dispatch + one bogus hit). + if _, err := ComputePlan(context.Background(), provider, desired, current); err != nil { + t.Fatalf("second ComputePlan: %v", err) + } + if got := driver.diffCount.Load(); got != 4 { + t.Errorf("Diff calls after 2nd ComputePlan = %d, want 4 (no cache hits when Outputs are unhashable; round-9 fix)", got) + } +} + +// TestConfigHashErr_PropagatesMarshalFailure is a unit-level pin for +// configHashErr. Marshalable inputs return (hash, nil); a non- +// marshalable value (channel) returns ("", non-nil-err) so cache-key +// callers can deterministically bypass the cache. +// +// Backward compatibility: the un-suffixed configHash silently swallows +// the marshal error and returns the sha256-of-best-effort-bytes hash +// (matches the pre-T3.6 implementation byte-for-byte) so any persisted +// ResolvedConfigHash / ConfigHash values computed under the old +// behavior stay stable. configHashErr is the strict variant that must +// be used for cache keys; configHash is the legacy ABI. +func TestConfigHashErr_PropagatesMarshalFailure(t *testing.T) { + good := map[string]any{"region": "nyc3", "size": 4} + gotHash, gotErr := configHashErr(good) + if gotErr != nil { + t.Fatalf("configHashErr(good): unexpected err: %v", gotErr) + } + if len(gotHash) != 64 { + t.Errorf("configHashErr(good): hash len = %d, want 64 (sha256 hex)", len(gotHash)) + } + + bad := map[string]any{"poison": make(chan int)} + gotHash, gotErr = configHashErr(bad) + if gotErr == nil { + t.Errorf("configHashErr(bad): expected non-nil error for unmarshalable input; got hash=%q", gotHash) + } + if gotHash != "" { + t.Errorf("configHashErr(bad): hash = %q, want empty string when err != nil", gotHash) + } + + // Backward-compatible wrapper: configHash returns the legacy + // sha256-of-best-effort hash for unmarshalable inputs (matches the + // pre-T3.6 implementation byte-for-byte; cache callers MUST use + // configHashErr instead). The exact value here is sha256(nil), + // which is what json.Marshal followed by sha256.Sum256 produces + // when Marshal returns nil + err. + got := configHash(bad) + const sha256OfNil = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" + if got != sha256OfNil { + t.Errorf("configHash(bad) = %q, want %q (legacy sha256(nil) — pre-T3.6 byte-for-byte stability)", got, sha256OfNil) + } +}