Skip to content

Commit b910f45

Browse files
author
David Noble
committed
refactor(13.0(n) Phase 3.5): parent pointer -> ID; retryPolicy / errorAction on executableUnit
Structural refactor making executableUnit's containment serializable and extending every executable unit (Node, Subgraph) with retryPolicy and errorAction fields per plan-doc D5 / D11. executableUnit (pkg/op/executable_unit.go, full rewrite) - parent *Subgraph -> parentID string. By-ID rather than by-pointer because containment must round-trip through plan.save / plan.load (pointers don't serialize). Graph.Root has parentID == "" (the only unit with an empty parentID after wiring). Cross-Graph reuse works because graph.Root.ID is the constant "root" across all Graphs, so an Invocation that's a root child in two different Graphs stamps the same parentID value both times — idempotent. - retryPolicy *RetryPolicy and errorAction ExecutableUnit fields added. Nil retryPolicy means no retry; nil errorAction defaults to the flow.Provider.Failed sentinel at dispatch time. - Accessors: ParentID() / RetryPolicy() / SetRetryPolicy() / ErrorAction() / SetErrorAction(). - stampParent(newParentID) is the package-private helper that AddChild calls. Idempotent: calling again with the same parentID succeeds silently; calling with a different non-empty parentID panics — within a single Graph context, a unit can be a child of only one Subgraph at a time. The conflict panic catches accidental double-parenting bugs. Subgraph (pkg/op/subgraph.go) - AddChild updated to call stampParent (was direct pointer assignment on child.Node.parent / child.Subgraph.parent). - Subgraph.Retry *RetryPolicy field removed. Retry now lives on the embedded executableUnit via retryPolicy; callers access through RetryPolicy() / SetRetryPolicy() instead of struct field access. Node (pkg/op/graph.go) - Node.Retry *RetryPolicy field removed for the same reason. Same accessor migration. Retry-field migration across the codebase - pkg/op/marshal.go: Node and Subgraph JSON/YAML marshalers updated to route Retry envelope field through RetryPolicy() (read) and SetRetryPolicy() (write). The "retry" JSON/YAML field stays — only the Go-side accessor changed. - pkg/op/executor.go: sg.Retry references migrated to sg.RetryPolicy() in executeSubgraph's retry-loop logic. - pkg/op/starlarkbridge/task_builder.go: node.Retry assignment migrated to node.SetRetryPolicy(...). - pkg/op/starlarkbridge/promise.go: same migration for p.node.Retry. - cmd/lore/lore/builder.go: sg.Retry == nil check and assignment in buildPackageNodes migrated to sg.RetryPolicy() / sg.SetRetryPolicy(). marshal.go deserialization bypass sites migrated to AddChild Three sites where Subgraph.Children was bulk-assigned from the unmarshaled payload now iterate and call AddChild per child, so each deserialized child gets its parentID re-stamped to its enclosing Subgraph's ID. This closes the Phase 3 "5 bypass sites" list partially — see deferred items below for the other two. - applyPayload (Graph deserialization, graph payload to g.Root) - Subgraph.UnmarshalJSON - Subgraph.UnmarshalYAML Tests - pkg/op/subgraph_test.go: 4 Parent() test sites migrated to ParentID() string-equality assertions. Fixed-point semantics preserved (graph root has ParentID() == ""). Deferred (intentionally NOT in this commit; require Phase 5 bridge work) - cmd/lore/lore/builder.go:341 — buildPackageNodes moves children from graph.Root into a freshly-created phase Subgraph. AddChild's parent-conflict check would reject this re-parenting. The clean fix is for the bridge to know its target Subgraph at script-execution time and emit directly into the phase Subgraph; that requires the bridge target work that's Phase 5 scope. Until then, this site keeps bulk Children manipulation; the parentID on moved children stays stale ("root"). - cmd/devlore-test/devloretest/runner.go:438 — wrapUngroupedNodes same shape. Same Phase 5 dependency. - addNativePMNodes mechanical migration: small, but related to the above bridge work; landing together makes the buildPackageNodes story coherent. - Subgraph.Parameters() kwargs-binding filter: per plan-doc D8 the bridge should stamp boundVariables on a Subgraph at classification time, and Subgraph.Parameters() should subtract those from the returned surface. Bridge work; Phase 5. - Bridge kwarg classification (body= / items= / error_action= / retry_policy= reserved kwargs + parent-stamping at body inclusion) — depends on the uniform Subgraph(items, kwargs) Go signature for flow's container methods. Both Phase 5 scope. Build state make build / make vet clean. pkg/op + all provider packages green. Pre-existing cmd/devlore-test / cmd/star/star failures (13.0(n) graph- executor harness scope) unchanged by this commit. Issue: #283
1 parent c4e028a commit b910f45

10 files changed

Lines changed: 205 additions & 121 deletions

File tree

‎cmd/lore/lore/builder.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -324,8 +324,8 @@ func buildPackageNodes(graph *op.Graph, pkg *lorepackage.Release, targetPlatform
324324
if err != nil {
325325
return fmt.Errorf("subgraph %q: %w", phaseName, err)
326326
}
327-
if retryPolicy != nil && sg.Retry == nil {
328-
sg.Retry = retryPolicy
327+
if retryPolicy != nil && sg.RetryPolicy() == nil {
328+
sg.SetRetryPolicy(retryPolicy)
329329
}
330330
case *lorepackage.NativePMAction:
331331
if err := addNativePMNodes(graph, pkg, a, reg); err != nil {

‎pkg/op/executable_unit.go‎

Lines changed: 78 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,19 @@
33

44
package op
55

6+
import (
7+
"fmt"
8+
9+
"github.com/NobleFactor/devlore-cli/pkg/assert"
10+
)
11+
612
// region ExecutableUnit interface
713

814
// ExecutableUnit is anything the executor can dispatch — a Node or a Subgraph.
915
//
1016
// Nodes and subgraphs share one ID space and are interchangeable wherever a reference is valid (Phase 7
11-
// invariant). Parameters reports the unit's input surface and is consumed by flow combinators (e.g., gather)
12-
// to discover the body's expected input.
17+
// invariant). Parameters reports the unit's input surface and is consumed by flow combinators (e.g.,
18+
// gather) to discover the body's expected input.
1319
type ExecutableUnit interface {
1420
ID() string
1521
Parameters() []Parameter
@@ -21,18 +27,26 @@ type ExecutableUnit interface {
2127

2228
// executableUnit is the shared state embedded by Node and Subgraph.
2329
//
24-
// All fields are unexported; callers read through the ID() / Parameters() / Parent() methods and write
25-
// through the constructors (NewNode, NewSubgraph), plan-time hooks (Node.Bind), and the
26-
// (s *Subgraph).AddChild method that stamps the parent pointer.
30+
// All fields are unexported; callers read through the ID() / Parameters() / ParentID() / RetryPolicy() /
31+
// ErrorAction() methods and write through the constructors (NewNode, NewSubgraph), plan-time hooks
32+
// (Node.Bind), and the (s *Subgraph).AddChild method that stamps the parent ID.
33+
//
34+
// parentID identifies the enclosing Subgraph by ID rather than pointer (plan-doc D11). By-ID rather than
35+
// by-pointer because containment must round-trip through plan.save / plan.load — pointers don't
36+
// serialize. The graph's Root subgraph has parentID == "" (the only unit with an empty parentID after
37+
// it has been wired into a Graph). Cross-Graph reuse works because graph.Root.ID is the constant "root"
38+
// across all Graphs, so an Invocation that's a root child in two different Graphs stamps the same
39+
// parentID value both times — idempotent.
2740
//
28-
// The parent pointer is the forward-compatibility scaffolding for the frame-based execution model
29-
// (plan-doc D11). It models structural ownership: each executable unit knows the Subgraph that
30-
// contains it. The graph's Root subgraph has parent == nil; everything else has a non-nil parent
31-
// after AddChild wires it in.
41+
// retryPolicy and errorAction are plan-doc D5 / D11 fields — every executable unit can carry them. Nil
42+
// retryPolicy means no retry; nil errorAction defaults to the flow.Provider.Failed sentinel at dispatch
43+
// time.
3244
type executableUnit struct {
33-
id string
34-
parameters []Parameter
35-
parent *Subgraph
45+
id string
46+
parameters []Parameter
47+
parentID string
48+
retryPolicy *RetryPolicy
49+
errorAction ExecutableUnit
3650
}
3751

3852
// ID returns the identifier.
@@ -45,11 +59,58 @@ func (e *executableUnit) ID() string { return e.id }
4559
// surface dynamically via a graph-walk (plan-doc D3).
4660
func (e *executableUnit) Parameters() []Parameter { return e.parameters }
4761

48-
// Parent returns the Subgraph that owns this executable unit, or nil for the graph root.
62+
// ParentID returns the ID of the enclosing Subgraph, or the empty string for the graph root (or for a
63+
// unit that has not yet been added to any parent).
4964
//
5065
// Returns:
51-
// - *Subgraph: the owning subgraph, or nil if this is the graph's root or has not yet been added
52-
// to any parent.
53-
func (e *executableUnit) Parent() *Subgraph { return e.parent }
66+
// - string: the parent Subgraph's ID, or "".
67+
func (e *executableUnit) ParentID() string { return e.parentID }
5468

55-
// endregion
69+
// RetryPolicy returns this unit's retry policy, or nil when no policy is configured.
70+
//
71+
// Returns:
72+
// - *RetryPolicy: the configured retry policy, or nil.
73+
func (e *executableUnit) RetryPolicy() *RetryPolicy { return e.retryPolicy }
74+
75+
// SetRetryPolicy sets this unit's retry policy.
76+
//
77+
// Parameters:
78+
// - p: the retry policy to set. Pass nil to disable retry.
79+
func (e *executableUnit) SetRetryPolicy(p *RetryPolicy) { e.retryPolicy = p }
80+
81+
// ErrorAction returns the failure handler for this unit, or nil when no error action is configured.
82+
//
83+
// Returns:
84+
// - ExecutableUnit: the configured error-action unit, or nil. Nil defaults to the flow.Provider.Failed
85+
// sentinel at dispatch time.
86+
func (e *executableUnit) ErrorAction() ExecutableUnit { return e.errorAction }
87+
88+
// SetErrorAction sets the failure-handler unit.
89+
//
90+
// Parameters:
91+
// - ea: the failure-handling executable unit. Pass nil to use the default flow.Provider.Failed sentinel.
92+
func (e *executableUnit) SetErrorAction(ea ExecutableUnit) { e.errorAction = ea }
93+
94+
// stampParent sets this unit's parentID with idempotency.
95+
//
96+
// Calling again with the same parentID succeeds silently; calling with a different non-empty parentID
97+
// panics — within a single Graph context, a unit can be a child of only one Subgraph at a time. Cross-
98+
// Graph reuse via the constant "root" ID for graph.Root is the use case the idempotency permits.
99+
//
100+
// Parameters:
101+
// - newParentID: the parent Subgraph's ID to stamp. Must not be empty (asserted).
102+
func (e *executableUnit) stampParent(newParentID string) {
103+
104+
assert.True("newParentID not empty", newParentID != "")
105+
106+
if e.parentID != "" && e.parentID != newParentID {
107+
panic(fmt.Sprintf(
108+
"executableUnit %q already has parentID %q; cannot re-parent to %q",
109+
e.id, e.parentID, newParentID,
110+
))
111+
}
112+
113+
e.parentID = newParentID
114+
}
115+
116+
// endregion

‎pkg/op/executor.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -283,8 +283,8 @@ func (e *GraphExecutor) executeSubgraph(ctx context.Context, graph *Graph, sg *S
283283

284284
maxAttempts := 1
285285

286-
if sg.Retry != nil {
287-
maxAttempts += sg.Retry.MaxAttempts
286+
if sg.RetryPolicy() != nil {
287+
maxAttempts += sg.RetryPolicy().MaxAttempts
288288
}
289289

290290
ec := graph.RuntimeEnvironment()
@@ -294,8 +294,8 @@ func (e *GraphExecutor) executeSubgraph(ctx context.Context, graph *Graph, sg *S
294294
for attempt := 0; attempt < maxAttempts; attempt++ {
295295

296296
// Apply backoff delay before retries (not before first attempt)
297-
if attempt > 0 && sg.Retry != nil {
298-
delay := sg.Retry.ComputeDelay(attempt - 1)
297+
if attempt > 0 && sg.RetryPolicy() != nil {
298+
delay := sg.RetryPolicy().ComputeDelay(attempt - 1)
299299
if delay > 0 {
300300
select {
301301
case <-ec.Context.Done():

‎pkg/op/graph.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -507,9 +507,6 @@ type Node struct {
507507
// ProviderReceiverType from the registry, constructs the provider, and dispatches via Method.Do.
508508
Receiver string
509509

510-
// Retry is the retry policy for this node (nil = no retry).
511-
Retry *RetryPolicy
512-
513510
// Slots holds input values for this node, ordered by method parameter position.
514511
Slots []*Slot
515512

‎pkg/op/marshal.go‎

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,12 @@ func (g *Graph) applyPayload(p *graphPayload) {
109109
if g.Root == nil {
110110
g.Root = NewSubgraph("root")
111111
}
112-
g.Root.Children = p.Children
112+
113+
g.Root.Children = nil
114+
for _, child := range p.Children {
115+
g.Root.AddChild(child)
116+
}
117+
113118
g.Root.Edges = p.Edges
114119
g.Checksum = p.Checksum
115120
g.Collisions = p.Collisions
@@ -146,7 +151,7 @@ func (n *Node) MarshalJSON() ([]byte, error) {
146151
Error: n.Error,
147152
Layer: n.Layer,
148153
Origin: n.Origin,
149-
Retry: n.Retry,
154+
Retry: n.RetryPolicy(),
150155
Slots: n.Slots,
151156
Timestamp: n.Timestamp,
152157
})
@@ -177,7 +182,7 @@ func (n *Node) UnmarshalJSON(data []byte) error {
177182
n.Error = payload.Error
178183
n.Layer = payload.Layer
179184
n.Origin = payload.Origin
180-
n.Retry = payload.Retry
185+
n.SetRetryPolicy(payload.Retry)
181186
n.Slots = payload.Slots
182187
n.Timestamp = payload.Timestamp
183188
return nil
@@ -204,7 +209,7 @@ func (n *Node) MarshalYAML() (any, error) {
204209
Error: n.Error,
205210
Layer: n.Layer,
206211
Origin: n.Origin,
207-
Retry: n.Retry,
212+
Retry: n.RetryPolicy(),
208213
Slots: n.Slots,
209214
Timestamp: n.Timestamp,
210215
}, nil
@@ -235,7 +240,7 @@ func (n *Node) UnmarshalYAML(unmarshal func(any) error) error {
235240
n.Error = payload.Error
236241
n.Layer = payload.Layer
237242
n.Origin = payload.Origin
238-
n.Retry = payload.Retry
243+
n.SetRetryPolicy(payload.Retry)
239244
n.Slots = payload.Slots
240245
n.Timestamp = payload.Timestamp
241246
return nil
@@ -264,7 +269,7 @@ func (s *Subgraph) MarshalJSON() ([]byte, error) {
264269
Status: s.Status,
265270
Children: s.Children,
266271
Edges: s.Edges,
267-
Retry: s.Retry,
272+
Retry: s.RetryPolicy(),
268273
Compensate: s.Compensate,
269274
Attempts: s.Attempts,
270275
State: s.State,
@@ -293,9 +298,14 @@ func (s *Subgraph) UnmarshalJSON(data []byte) error {
293298
s.executableUnit = executableUnit{id: payload.ID}
294299
s.Name = payload.Name
295300
s.Status = payload.Status
296-
s.Children = payload.Children
301+
302+
s.Children = nil
303+
for _, child := range payload.Children {
304+
s.AddChild(child)
305+
}
306+
297307
s.Edges = payload.Edges
298-
s.Retry = payload.Retry
308+
s.SetRetryPolicy(payload.Retry)
299309
s.Compensate = payload.Compensate
300310
s.Attempts = payload.Attempts
301311
s.State = payload.State
@@ -322,7 +332,7 @@ func (s *Subgraph) MarshalYAML() (any, error) {
322332
Status: s.Status,
323333
Children: s.Children,
324334
Edges: s.Edges,
325-
Retry: s.Retry,
335+
Retry: s.RetryPolicy(),
326336
Compensate: s.Compensate,
327337
Attempts: s.Attempts,
328338
State: s.State,
@@ -351,9 +361,14 @@ func (s *Subgraph) UnmarshalYAML(unmarshal func(any) error) error {
351361
s.executableUnit = executableUnit{id: payload.ID}
352362
s.Name = payload.Name
353363
s.Status = payload.Status
354-
s.Children = payload.Children
364+
365+
s.Children = nil
366+
for _, child := range payload.Children {
367+
s.AddChild(child)
368+
}
369+
355370
s.Edges = payload.Edges
356-
s.Retry = payload.Retry
371+
s.SetRetryPolicy(payload.Retry)
357372
s.Compensate = payload.Compensate
358373
s.Attempts = payload.Attempts
359374
s.State = payload.State

0 commit comments

Comments
 (0)