From 665c271197ee478c54015d28be9934550eac31e1 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Tue, 7 Jul 2026 22:18:23 -0400 Subject: [PATCH 1/2] feat(state): scoped state serializer preserving sibling components on concurrent writes Add WriteScopedState, which node-patches only the addressed (component, env) leaf and preserves every sibling subtree as a verbatim YAML node, so a concurrent finalizer cannot drop another component's state. Reimplement WriteManifestState as a delete-aware single-component wrapper (reconcile to the map: patch present keys, delete keys present in the fetched bytes but absent from the map) so the publish transition still drops the prerelease marker and single-component output stays byte-identical. Widen the reserved ComponentState to the full EnvState the lifecycle needs. Refs #282. Signed-off-by: Joshua Temple --- internal/config/scopedstate_test.go | 319 +++++++++++++++++ internal/config/statemerge.go | 329 ++++++++++++++++-- internal/config/types.go | 14 +- internal/promote/finalize.go | 11 + internal/statewrite/scoped_concurrent_test.go | 81 +++++ 5 files changed, 715 insertions(+), 39 deletions(-) create mode 100644 internal/config/scopedstate_test.go create mode 100644 internal/statewrite/scoped_concurrent_test.go diff --git a/internal/config/scopedstate_test.go b/internal/config/scopedstate_test.go new file mode 100644 index 00000000..5959f742 --- /dev/null +++ b/internal/config/scopedstate_test.go @@ -0,0 +1,319 @@ +package config + +import ( + "strings" + "testing" + + "gopkg.in/yaml.v3" +) + +// referenceWholeNodeReplace reproduces the pre-scoped-serializer WriteManifestState +// algorithm verbatim: it replaces the whole `state` node with a fresh marshal of +// the typed map (sorted keys, delete-by-omission) and the `latest_release` node +// with the typed release (nil deletes). The scoped wrapper must match its bytes +// exactly for every single-component case, so this is the byte-identity oracle. +func referenceWholeNodeReplace(t *testing.T, current []byte, manifestKey string, state map[string]*EnvState, latest *LatestReleaseState) []byte { + t.Helper() + if manifestKey == "" { + manifestKey = DefaultManifestKey + } + var doc yaml.Node + if err := yaml.Unmarshal(current, &doc); err != nil { + t.Fatalf("reference unmarshal: %v", err) + } + root := documentMapping(&doc) + if root == nil { + root = &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} + doc = yaml.Node{Kind: yaml.DocumentNode, Content: []*yaml.Node{root}} + } + section := mappingValue(root, manifestKey) + if section == nil || section.Kind != yaml.MappingNode { + section = &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} + setMappingValue(root, manifestKey, section) + } + if len(state) == 0 { + deleteMappingKey(section, "state") + } else { + node, err := valueNode(state) + if err != nil { + t.Fatalf("reference state node: %v", err) + } + setMappingValue(section, "state", node) + } + if latest == nil { + deleteMappingKey(section, "latest_release") + } else { + node, err := valueNode(latest) + if err != nil { + t.Fatalf("reference latest node: %v", err) + } + setMappingValue(section, "latest_release", node) + } + data, err := yaml.Marshal(&doc) + if err != nil { + t.Fatalf("reference marshal: %v", err) + } + return data +} + +// publishManifest carries a prerelease marker and several envs in sorted order, +// as a prior single-component write would have produced. +const publishManifest = `ci: + config: + trunk_branch: main + environments: + - dev + - staging + - prod + state: + dev: + sha: devsha + version: v1.2.0 + prerelease: + sha: rcsha + version: v1.2.0-rc.1 + prod: + sha: prodsha + version: v1.1.0 + staging: + sha: stagingsha + version: v1.2.0 +` + +// TestWriteManifestState_PublishDropsPrereleaseByteIdentical is the load-bearing +// delete-aware golden. The publish transition sets state["release"] and deletes +// state["prerelease"] from the typed map (mirroring overlayOwnedState). Fed bytes +// that still carry a prerelease node, the wrapper must drop prerelease AND match +// the whole-state-node-replace oracle byte for byte. A naive patch-only wrapper +// (patch present keys, never delete) would leave the stale prerelease node and +// fail this test. +func TestWriteManifestState_PublishDropsPrereleaseByteIdentical(t *testing.T) { + // The overlay result: prerelease deleted, release added, envs retained. + final := map[string]*EnvState{ + "dev": {SHA: "devsha", Version: "v1.2.0"}, + "staging": {SHA: "stagingsha", Version: "v1.2.0"}, + "prod": {SHA: "prodsha", Version: "v1.1.0"}, + "release": {SHA: "rcsha", Version: "v1.2.0"}, + } + latest := &LatestReleaseState{Version: "v1.2.0", SHA: "rcsha", ReleasedOn: "2026-01-01T00:00:00Z"} + + got, err := WriteManifestState([]byte(publishManifest), "ci", final, latest) + if err != nil { + t.Fatalf("WriteManifestState: %v", err) + } + + if strings.Contains(string(got), "prerelease") { + t.Fatalf("publish transition left a stale prerelease node:\n%s", got) + } + + want := referenceWholeNodeReplace(t, []byte(publishManifest), "ci", final, latest) + if string(got) != string(want) { + t.Fatalf("publish output not byte-identical to whole-node-replace oracle\n--- got ---\n%s\n--- want ---\n%s", got, want) + } +} + +// TestWriteManifestState_ByteIdenticalAcrossCases pins the wrapper to the +// whole-node-replace oracle across the ordinary single-component cases: a normal +// env update, the empty-state reset (state key removed), and a latest_release +// write. These lock byte-identity beyond the publish-delete path. +func TestWriteManifestState_ByteIdenticalAcrossCases(t *testing.T) { + cases := []struct { + name string + src string + key string + state map[string]*EnvState + latest *LatestReleaseState + }{ + { + name: "env update", + src: publishManifest, + key: "ci", + state: map[string]*EnvState{"dev": {SHA: "newsha", Version: "v2.0.0"}}, + }, + { + name: "empty state reset removes key", + src: publishManifest, + key: "ci", + state: nil, + }, + { + name: "write latest release", + src: publishManifest, + key: "ci", + state: map[string]*EnvState{"prod": {SHA: "s"}}, + latest: &LatestReleaseState{Version: "v3.0.0", SHA: "rsha"}, + }, + { + name: "new env inserted out of order", + src: publishManifest, + key: "ci", + state: map[string]*EnvState{"aaa": {SHA: "a"}, "zzz": {SHA: "z"}, "dev": {SHA: "d"}}, + latest: nil, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := WriteManifestState([]byte(tc.src), tc.key, tc.state, tc.latest) + if err != nil { + t.Fatalf("WriteManifestState: %v", err) + } + want := referenceWholeNodeReplace(t, []byte(tc.src), tc.key, tc.state, tc.latest) + if string(got) != string(want) { + t.Fatalf("not byte-identical\n--- got ---\n%s\n--- want ---\n%s", got, want) + } + }) + } +} + +// multiComponentManifest carries two components under state.components. Component +// B's staging leaf holds an unmodeled key (custom_leaf_marker) that the binary's +// typed EnvState does not model. +const multiComponentManifest = `ci: + config: + trunk_branch: main + environments: + - staging + - prod + components: + api: + path: services/api + tag_prefix: api-v + web: + path: services/web + tag_prefix: web-v + state: + components: + api: + staging: + sha: apistaging + version: api-v1.0.0 + web: + staging: + sha: webstaging + version: web-v2.0.0 + custom_leaf_marker: keep-me-verbatim +` + +// TestWriteScopedState_PreservesSiblingComponentUnmodeledKey is the #389-class +// regression at component depth: a scoped write for (api, staging) must leave +// component web's subtree, including the unmodeled custom_leaf_marker key, present +// byte for byte. The serializer must never re-marshal state.components from a +// typed map. +func TestWriteScopedState_PreservesSiblingComponentUnmodeledKey(t *testing.T) { + got, err := WriteScopedState([]byte(multiComponentManifest), "ci", + StateWrite{Component: "api", Env: "staging", State: &EnvState{SHA: "apistaging-new", Version: "api-v1.1.0"}}) + if err != nil { + t.Fatalf("WriteScopedState: %v", err) + } + + if !strings.Contains(string(got), "custom_leaf_marker: keep-me-verbatim") { + t.Fatalf("sibling component web's unmodeled key was dropped:\n%s", got) + } + + top := parseTop(t, got) + state := top["ci"].(map[string]any)["state"].(map[string]any) + comps := state["components"].(map[string]any) + web := comps["web"].(map[string]any)["staging"].(map[string]any) + if web["custom_leaf_marker"] != "keep-me-verbatim" { + t.Errorf("web.staging.custom_leaf_marker = %v, want keep-me-verbatim", web["custom_leaf_marker"]) + } + if web["sha"] != "webstaging" { + t.Errorf("web.staging.sha = %v, want webstaging (verbatim)", web["sha"]) + } + api := comps["api"].(map[string]any)["staging"].(map[string]any) + if api["sha"] != "apistaging-new" { + t.Errorf("api.staging.sha = %v, want apistaging-new", api["sha"]) + } +} + +// TestWriteScopedState_MultiComponentRoundTripStable writes several +// (component, env) leaves and asserts re-applying the identical writes produces +// stable bytes (idempotent), and that a fresh write is order-preserving. +func TestWriteScopedState_MultiComponentRoundTripStable(t *testing.T) { + base := `ci: + config: + trunk_branch: main + state: + components: + api: + staging: + sha: s1 +` + writes := []StateWrite{ + {Component: "api", Env: "prod", State: &EnvState{SHA: "p1", Version: "api-v1.0.0"}}, + {Component: "web", Env: "staging", State: &EnvState{SHA: "w1"}}, + } + first, err := WriteScopedState([]byte(base), "ci", writes...) + if err != nil { + t.Fatalf("first write: %v", err) + } + second, err := WriteScopedState(first, "ci", writes...) + if err != nil { + t.Fatalf("second write: %v", err) + } + if string(first) != string(second) { + t.Fatalf("re-applying identical writes is not idempotent\n--- first ---\n%s\n--- second ---\n%s", first, second) + } + + top := parseTop(t, first) + comps := top["ci"].(map[string]any)["state"].(map[string]any)["components"].(map[string]any) + if comps["api"].(map[string]any)["staging"].(map[string]any)["sha"] != "s1" { + t.Errorf("api.staging.sha not preserved: %#v", comps["api"]) + } + if comps["api"].(map[string]any)["prod"].(map[string]any)["sha"] != "p1" { + t.Errorf("api.prod.sha not written: %#v", comps["api"]) + } + if comps["web"].(map[string]any)["staging"].(map[string]any)["sha"] != "w1" { + t.Errorf("web.staging.sha not written: %#v", comps["web"]) + } +} + +// TestWriteScopedState_EmptyCollapse asserts that deleting the last env of a +// component removes components., and deleting the last component removes the +// components mapping and then the state key, so a manifest returning to no +// component state collapses cleanly. +func TestWriteScopedState_EmptyCollapse(t *testing.T) { + // Delete web's only env: web disappears, api survives. + afterWeb, err := WriteScopedState([]byte(multiComponentManifest), "ci", + StateWrite{Component: "web", Env: "staging", State: nil}) + if err != nil { + t.Fatalf("delete web.staging: %v", err) + } + top := parseTop(t, afterWeb) + comps := top["ci"].(map[string]any)["state"].(map[string]any)["components"].(map[string]any) + if _, ok := comps["web"]; ok { + t.Errorf("empty component web should have collapsed: %#v", comps) + } + if _, ok := comps["api"]; !ok { + t.Errorf("api must survive web deletion: %#v", comps) + } + + // Now delete api's only env too: components and state collapse away entirely. + afterAll, err := WriteScopedState(afterWeb, "ci", + StateWrite{Component: "api", Env: "staging", State: nil}) + if err != nil { + t.Fatalf("delete api.staging: %v", err) + } + top2 := parseTop(t, afterAll) + ci := top2["ci"].(map[string]any) + if _, ok := ci["state"]; ok { + t.Errorf("state key should collapse when no component state remains: %#v", ci["state"]) + } + // config must survive the collapse. + if _, ok := ci["config"]; !ok { + t.Error("config dropped during collapse") + } +} + +// TestWriteScopedState_RejectsMixedForms guards decision 7.1: a single manifest +// never carries both single-component and component-scoped state, so a call that +// mixes the forms is a programming error and must be rejected rather than silently +// dropping the whole state node. +func TestWriteScopedState_RejectsMixedForms(t *testing.T) { + _, err := WriteScopedState([]byte(publishManifest), "ci", + StateWrite{Env: "dev", State: &EnvState{SHA: "x"}}, + StateWrite{Component: "api", Env: "staging", State: &EnvState{SHA: "y"}}) + if err == nil { + t.Fatal("expected an error when mixing single-component and component-scoped writes") + } +} diff --git a/internal/config/statemerge.go b/internal/config/statemerge.go index fafba4ff..636678ef 100644 --- a/internal/config/statemerge.go +++ b/internal/config/statemerge.go @@ -6,27 +6,74 @@ import ( "gopkg.in/yaml.v3" ) -// WriteManifestState rewrites manifest bytes in place, replacing only the -// mutable state subtree (the `state` and `latest_release` keys under -// manifestKey) and leaving every other key untouched. +// StateWrite describes a single scoped mutation of the state (or latest_release) +// subtree. Exactly one form is addressed per write, discriminated by Env: // -// State writers (reset, promote/finalize, hotfix/finalize, rollback) only ever -// change `state` and `latest_release`; the rest of the manifest is read-only at -// write time. Earlier writers re-marshaled the typed CICDFile, which silently -// dropped any key the running binary does not model (for example a config field -// added in a newer cascade release). Operating on the parsed YAML node and -// touching only the two mutable keys preserves all other content verbatim, -// including configuration this binary does not model and any comments. +// - Env != "": a state directive. State != nil sets the addressed env leaf; +// State == nil deletes it. Component == "" targets the single-component +// state. form (byte-identical to today); a non-empty Component targets +// state.components.., carrying a full EnvState. +// - Env == "": a latest_release directive. Latest != nil sets it; Latest == nil +// deletes it. Component == "" targets the top-level latest_release; a +// non-empty Component targets latest_release.components.. // -// state is written when non-empty and the `state` key is removed when empty, -// matching the previous `omitempty` behavior; the same rule applies to -// latest_release against nil. manifestKey defaults to DefaultManifestKey when -// empty. -func WriteManifestState(current []byte, manifestKey string, state map[string]*EnvState, latest *LatestReleaseState) ([]byte, error) { +// A manifest is either entirely single-component (all Component == "") or +// entirely component-scoped (all Component != ""); the two forms never coexist in +// one manifest, so WriteScopedState rejects a call that mixes them. +type StateWrite struct { + // Component is the component name, or "" for the single-component form. + Component string + // Env is the environment key being addressed, or "" for a latest_release + // directive. + Env string + // State is the env leaf value for a state directive (Env != ""). nil deletes + // the env key. + State *EnvState + // Latest is the release value for a latest_release directive (Env == ""). nil + // deletes the release record. + Latest *LatestReleaseState +} + +// WriteScopedState rewrites manifest bytes, applying writes against the parsed +// YAML node tree and touching only the addressed value nodes. Every sibling +// subtree, and any key the binary does not model, is preserved verbatim as its +// original parsed node. +// +// The single-component form (all writes with Component == "") reconciles the +// whole `state` node from the union of set writes so its bytes stay identical to +// the historical whole-state-node replacement: the map marshal is sorted, and a +// key absent from the set writes is dropped, matching the previous omitempty +// delete semantics. There are no unmodeled siblings under a single-component +// `state` node to preserve, so a whole-node rebuild is the byte-faithful choice. +// +// The component-scoped form (Component != "") node-patches only the addressed +// state.components.. (or latest_release.components.) leaf, +// leaving every other component and env as its original node. A sibling component +// the binary cannot model is preserved precisely because it is never deserialized +// into a typed value. Deleting the last env of a component removes the component +// mapping, and deleting the last component removes the components mapping and the +// now-empty state key, so a manifest returning to no component state collapses +// cleanly. +// +// manifestKey defaults to DefaultManifestKey when empty. YAML parse and encode +// failures are wrapped with %w. +func WriteScopedState(current []byte, manifestKey string, writes ...StateWrite) ([]byte, error) { if manifestKey == "" { manifestKey = DefaultManifestKey } + hasSingle, hasComponent := false, false + for _, w := range writes { + if w.Component == "" { + hasSingle = true + } else { + hasComponent = true + } + } + if hasSingle && hasComponent { + return nil, fmt.Errorf("scoped state write mixes single-component and component-scoped forms in one manifest") + } + var doc yaml.Node if err := yaml.Unmarshal(current, &doc); err != nil { return nil, fmt.Errorf("parsing manifest for state write: %w", err) @@ -46,24 +93,12 @@ func WriteManifestState(current []byte, manifestKey string, state map[string]*En setMappingValue(root, manifestKey, section) } - if len(state) == 0 { - deleteMappingKey(section, "state") - } else { - node, err := valueNode(state) - if err != nil { - return nil, fmt.Errorf("encoding state for state write: %w", err) - } - setMappingValue(section, "state", node) - } - - if latest == nil { - deleteMappingKey(section, "latest_release") - } else { - node, err := valueNode(latest) - if err != nil { - return nil, fmt.Errorf("encoding latest_release for state write: %w", err) + if hasComponent { + if err := applyComponentWrites(section, writes); err != nil { + return nil, err } - setMappingValue(section, "latest_release", node) + } else if err := applySingleComponentWrites(section, writes); err != nil { + return nil, err } data, err := yaml.Marshal(&doc) @@ -73,6 +108,224 @@ func WriteManifestState(current []byte, manifestKey string, state map[string]*En return data, nil } +// applySingleComponentWrites reconciles the whole single-component `state` node +// from the union of set writes (byte-identical to the historical whole-node +// replacement) and applies the top-level latest_release directive. +func applySingleComponentWrites(section *yaml.Node, writes []StateWrite) error { + state := make(map[string]*EnvState) + haveStateDirective := false + for _, w := range writes { + if w.Env == "" { + continue // latest_release directive, handled below + } + haveStateDirective = true + if w.State != nil { + state[w.Env] = w.State + } + // A State == nil delete contributes no key, so the rebuilt node omits it. + } + + if haveStateDirective { + if len(state) == 0 { + deleteMappingKey(section, "state") + } else { + node, err := valueNode(state) + if err != nil { + return fmt.Errorf("encoding state for state write: %w", err) + } + setMappingValue(section, "state", node) + } + } + + for _, w := range writes { + if w.Env != "" { + continue + } + if w.Latest == nil { + deleteMappingKey(section, "latest_release") + } else { + node, err := valueNode(w.Latest) + if err != nil { + return fmt.Errorf("encoding latest_release for state write: %w", err) + } + setMappingValue(section, "latest_release", node) + } + } + return nil +} + +// applyComponentWrites node-patches the addressed state.components.. +// and latest_release.components. leaves, preserving every other component +// and env verbatim, then collapses any mapping the writes emptied. +func applyComponentWrites(section *yaml.Node, writes []StateWrite) error { + for _, w := range writes { + if w.Env != "" { + if err := applyComponentStateLeaf(section, w); err != nil { + return err + } + continue + } + if err := applyComponentLatestLeaf(section, w); err != nil { + return err + } + } + collapseComponentContainer(section, "state") + collapseComponentContainer(section, "latest_release") + return nil +} + +// applyComponentStateLeaf sets or deletes state.components... +func applyComponentStateLeaf(section *yaml.Node, w StateWrite) error { + if w.State == nil { + if comps := existingChild(section, "state", "components"); comps != nil { + if comp := mappingValue(comps, w.Component); comp != nil && comp.Kind == yaml.MappingNode { + deleteMappingKey(comp, w.Env) + } + } + return nil + } + node, err := valueNode(w.State) + if err != nil { + return fmt.Errorf("encoding component state for state write: %w", err) + } + comp := mappingChild(mappingChild(mappingChild(section, "state"), "components"), w.Component) + setMappingValue(comp, w.Env, node) + return nil +} + +// applyComponentLatestLeaf sets or deletes latest_release.components.. +func applyComponentLatestLeaf(section *yaml.Node, w StateWrite) error { + if w.Latest == nil { + if comps := existingChild(section, "latest_release", "components"); comps != nil { + deleteMappingKey(comps, w.Component) + } + return nil + } + node, err := valueNode(w.Latest) + if err != nil { + return fmt.Errorf("encoding component latest_release for state write: %w", err) + } + comps := mappingChild(mappingChild(section, "latest_release"), "components") + setMappingValue(comps, w.Component, node) + return nil +} + +// collapseComponentContainer removes emptied nesting under key (state or +// latest_release): empty per-component mappings, an empty components mapping, and +// finally the empty container key itself, so a manifest returning to no component +// state never emits a dangling `state.components: {}`. +func collapseComponentContainer(section *yaml.Node, key string) { + container := mappingValue(section, key) + if container == nil || container.Kind != yaml.MappingNode { + return + } + comps := mappingValue(container, "components") + if comps != nil && comps.Kind == yaml.MappingNode { + for i := 0; i+1 < len(comps.Content); { + val := comps.Content[i+1] + if val.Kind == yaml.MappingNode && len(val.Content) == 0 { + name := comps.Content[i].Value + deleteMappingKey(comps, name) + continue // indices shifted; re-examine this position + } + i += 2 + } + if len(comps.Content) == 0 { + deleteMappingKey(container, "components") + } + } + if len(container.Content) == 0 { + deleteMappingKey(section, key) + } +} + +// existingChild returns the nested child mapping at parent.key.child without +// creating it, or nil when either level is absent or not a mapping. +func existingChild(parent *yaml.Node, key, child string) *yaml.Node { + mid := mappingValue(parent, key) + if mid == nil || mid.Kind != yaml.MappingNode { + return nil + } + leaf := mappingValue(mid, child) + if leaf == nil || leaf.Kind != yaml.MappingNode { + return nil + } + return leaf +} + +// WriteManifestState rewrites manifest bytes, replacing only the mutable state +// subtree (the `state` and `latest_release` keys under manifestKey) and leaving +// every other key untouched. It is the single-component wrapper over +// WriteScopedState, retained so its existing single-component callers and byte +// output are unchanged. +// +// State writers (reset, promote/finalize, hotfix/finalize, rollback) only ever +// change `state` and `latest_release`; the rest of the manifest is read-only at +// write time. Earlier writers re-marshaled the typed CICDFile, which silently +// dropped any key the running binary does not model (for example a config field +// added in a newer cascade release). Operating on the parsed YAML node and +// touching only the two mutable keys preserves all other content verbatim, +// including configuration this binary does not model and any comments. +// +// Its contract is reconcile to the map, not patch the map: it writes every +// `state.` present in state and, by rebuilding the whole state node from +// state alone, drops any env or marker key (notably `prerelease`) present in the +// fetched bytes but absent from the map. This delete-awareness is load-bearing +// because the publish transition drops the prerelease marker by omission from the +// map (overlayOwnedState). state is written when non-empty and the `state` key is +// removed when empty, matching the previous omitempty behavior; the same rule +// applies to latest_release against nil. manifestKey defaults to +// DefaultManifestKey when empty. +func WriteManifestState(current []byte, manifestKey string, state map[string]*EnvState, latest *LatestReleaseState) ([]byte, error) { + writes := make([]StateWrite, 0, len(state)+1) + for env, st := range state { + writes = append(writes, StateWrite{Env: env, State: st}) + } + // Explicit deletes for env/marker keys present in the fetched bytes but absent + // from the map, so the reconcile-to-map contract drops them (for example the + // prerelease marker on a publish). The single-component rebuild would drop them + // regardless, but expressing them as StateWrite deletes keeps the wrapper's + // contract explicit and independent of that rebuild detail. + for _, env := range fetchedStateEnvKeys(current, manifestKey) { + if _, kept := state[env]; !kept { + writes = append(writes, StateWrite{Env: env, State: nil}) + } + } + // Always reconcile latest_release: nil deletes, non-nil sets. + writes = append(writes, StateWrite{Latest: latest}) + return WriteScopedState(current, manifestKey, writes...) +} + +// fetchedStateEnvKeys returns the direct child keys of the single-component +// `state` node in current, or nil when absent. Used by WriteManifestState to +// derive the explicit env/marker deletes its reconcile-to-map contract emits. +func fetchedStateEnvKeys(current []byte, manifestKey string) []string { + if manifestKey == "" { + manifestKey = DefaultManifestKey + } + var doc yaml.Node + if err := yaml.Unmarshal(current, &doc); err != nil { + return nil + } + root := documentMapping(&doc) + if root == nil { + return nil + } + section := mappingValue(root, manifestKey) + if section == nil || section.Kind != yaml.MappingNode { + return nil + } + state := mappingValue(section, "state") + if state == nil || state.Kind != yaml.MappingNode { + return nil + } + keys := make([]string, 0, len(state.Content)/2) + for i := 0; i+1 < len(state.Content); i += 2 { + keys = append(keys, state.Content[i].Value) + } + return keys +} + // documentMapping returns the top-level mapping node of a parsed YAML document, // or nil when the document is empty or its root is not a mapping. func documentMapping(doc *yaml.Node) *yaml.Node { @@ -106,6 +359,18 @@ func setMappingValue(m *yaml.Node, key string, val *yaml.Node) { m.Content = append(m.Content, keyNode, val) } +// mappingChild returns the value node for key in m, creating an empty mapping in +// place (appended) when the key is absent or its existing value is not a mapping. +// It lets a scoped write synthesize a missing state -> components -> path. +func mappingChild(m *yaml.Node, key string) *yaml.Node { + v := mappingValue(m, key) + if v == nil || v.Kind != yaml.MappingNode { + v = &yaml.Node{Kind: yaml.MappingNode, Tag: "!!map"} + setMappingValue(m, key, v) + } + return v +} + // deleteMappingKey removes the key/value pair for key from a mapping node when // present. func deleteMappingKey(m *yaml.Node, key string) { diff --git a/internal/config/types.go b/internal/config/types.go index 1b77e3ed..97bb916c 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -1242,13 +1242,13 @@ type ComponentConfig struct { Extra map[string]any `yaml:",inline" json:"-"` } -// ComponentState is the reserved per-component recorded-state entry. -type ComponentState struct { - Version string `yaml:"version,omitempty" json:"version,omitempty"` - SHA string `yaml:"sha,omitempty" json:"sha,omitempty"` - CommittedAt string `yaml:"committed_at,omitempty" json:"committed_at,omitempty"` - CommittedBy string `yaml:"committed_by,omitempty" json:"committed_by,omitempty"` -} +// ComponentState is the per-component recorded-state entry. It is the full +// EnvState surface: the multi-component form serializes state.components.. +// as a complete EnvState (version, sha, builds, deploys, external, ref, base_sha, +// patches, previous ring), which the lifecycle needs for promotion, hotfix, +// rollback, and divergence to work per component. It aliases EnvState so the wide +// shape is defined in exactly one place; the reserved name is retained. +type ComponentState = EnvState // ComponentReleaseState is the reserved per-component published-release entry. type ComponentReleaseState struct { diff --git a/internal/promote/finalize.go b/internal/promote/finalize.go index 2ff17e5c..e436c7fa 100644 --- a/internal/promote/finalize.go +++ b/internal/promote/finalize.go @@ -445,6 +445,15 @@ func (f *Finalizer) writeStateViaAPI(message string) error { // latest_release) so a concurrent finalizer's keys on into are preserved. It is // re-appliable: CommitWithRetry calls it again against re-fetched trunk bytes on // a 409, and each call deterministically re-derives the same owned keys. +// +// The publish transition deletes the prerelease marker from into.State by +// omission. That deletion is realized through WriteManifestState's +// reconcile-to-map contract: the wrapper rebuilds the state node from into.State +// alone and emits an explicit StateWrite delete (State == nil) for any env or +// marker key present in the fetched bytes but absent from the map, so the dropped +// prerelease key is removed rather than surviving as a stale node. The caller must +// therefore express a removal as a map deletion here, not rely on any whole-node +// replacement. func (f *Finalizer) overlayOwnedState(into *config.CICDFile) { if into.State == nil { into.State = make(map[string]*config.EnvState) @@ -457,6 +466,8 @@ func (f *Finalizer) overlayOwnedState(into *config.CICDFile) { if f.promotionResult != nil && f.promotionResult.ReleaseAction == "publish" { into.LatestRelease = f.cicdFile.LatestRelease into.State["release"] = f.cicdFile.State["release"] + // Removal by map deletion; the wrapper's reconcile-to-map contract turns + // this into an explicit node delete (see the doc comment above). delete(into.State, "prerelease") } } diff --git a/internal/statewrite/scoped_concurrent_test.go b/internal/statewrite/scoped_concurrent_test.go new file mode 100644 index 00000000..81ceda00 --- /dev/null +++ b/internal/statewrite/scoped_concurrent_test.go @@ -0,0 +1,81 @@ +package statewrite + +import ( + "testing" + + "github.com/stablekernel/cascade/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCommitWithRetry_TwoComponentsSameEnvMerge is the adversarial concurrency +// case for the scoped serializer: two finalize jobs advance the SAME env +// (staging) for DIFFERENT components (api and web). Component api commits first; +// component web's first PUT loses the optimistic-lock race (409), re-fetches +// api's committed bytes, and re-applies its own scoped write on top. Because each +// writer addresses a disjoint state.components..staging leaf, the final +// manifest must carry BOTH leaves with their exact fields and neither may drop +// the other. This is the sibling-survival property the node-disjoint scoped +// write buys for free under CommitWithRetry. +func TestCommitWithRetry_TwoComponentsSameEnvMerge(t *testing.T) { + // The branch already carries component api's staging leaf, as if api's + // finalize won the race and committed first. + const startManifest = `ci: + config: + trunk_branch: main + environments: + - staging + components: + api: + path: services/api + tag_prefix: api-v + web: + path: services/web + tag_prefix: web-v + state: + components: + api: + staging: + sha: apisha + version: api-v1.0.0 +` + fake := &fakeContents{ + content: startManifest, + sha: "sha-0", + putErrs: []error{rawConflict()}, // web's first PUT 409s, second succeeds + } + + // web's finalize: a re-appliable scoped write for (web, staging). It re-parses + // nothing of api's leaf; it only patches its own node, so re-applying it on top + // of api's committed bytes preserves api verbatim. + webMutate := func(current []byte) ([]byte, error) { + return config.WriteScopedState(current, "ci", + config.StateWrite{Component: "web", Env: "staging", State: &config.EnvState{SHA: "websha", Version: "web-v2.0.0"}}) + } + + var slept int + err := CommitWithRetry(Options{ + Client: fake, + Repo: "owner/name", + Path: ".github/manifest.yaml", + Ref: "main", + Message: "chore: record web staging state", + Mutate: webMutate, + Sleep: noSleep(&slept), + }) + require.NoError(t, err) + + assert.Equal(t, 2, fake.gets, "web must re-fetch after the 409") + assert.Equal(t, 2, fake.puts, "web must re-PUT after the 409") + + // Both components' staging leaves survive with their exact fields. Parse the + // merged manifest and assert the typed model carries both components. + file, err := config.ParseManifestBytes([]byte(fake.content), "ci") + require.NoError(t, err) + require.NotNil(t, file.State, "state must be present after the merge") + + assert.Contains(t, fake.content, "apisha", "api's committed leaf must survive web's re-applied write") + assert.Contains(t, fake.content, "websha", "web's leaf must be written") + assert.Contains(t, fake.content, "api-v1.0.0", "api leaf fields intact") + assert.Contains(t, fake.content, "web-v2.0.0", "web leaf fields intact") +} From 67a28b8cf7caa6fc31393de8782c2c42a61bb850 Mon Sep 17 00:00:00 2001 From: Joshua Temple Date: Tue, 7 Jul 2026 22:31:57 -0400 Subject: [PATCH 2/2] fix(state): avoid overflow-prone capacity arithmetic in WriteManifestState CodeQL go/allocation-size-overflow flagged the len(state)+1 make capacity. The delete loop and latest write append past that hint regardless, so size the slice to len(state) and drop the arithmetic. Signed-off-by: Joshua Temple --- internal/config/statemerge.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/statemerge.go b/internal/config/statemerge.go index 636678ef..d763195b 100644 --- a/internal/config/statemerge.go +++ b/internal/config/statemerge.go @@ -277,7 +277,7 @@ func existingChild(parent *yaml.Node, key, child string) *yaml.Node { // applies to latest_release against nil. manifestKey defaults to // DefaultManifestKey when empty. func WriteManifestState(current []byte, manifestKey string, state map[string]*EnvState, latest *LatestReleaseState) ([]byte, error) { - writes := make([]StateWrite, 0, len(state)+1) + writes := make([]StateWrite, 0, len(state)) for env, st := range state { writes = append(writes, StateWrite{Env: env, State: st}) }