From 4da7e406d761d0e049912058ab8ab361c9f4d49b Mon Sep 17 00:00:00 2001 From: shrutiyam-glitch Date: Fri, 28 Aug 2026 16:51:31 -0700 Subject: [PATCH 1/2] cleanup --- cmd/ateapi/internal/store/atepg/atepg.go | 92 +++++++++++-------- cmd/ateapi/internal/store/store.go | 7 +- .../internal/store/storecontract/contract.go | 5 +- 3 files changed, 59 insertions(+), 45 deletions(-) diff --git a/cmd/ateapi/internal/store/atepg/atepg.go b/cmd/ateapi/internal/store/atepg/atepg.go index 3b3145c201..b24bb758a3 100644 --- a/cmd/ateapi/internal/store/atepg/atepg.go +++ b/cmd/ateapi/internal/store/atepg/atepg.go @@ -188,19 +188,6 @@ type querier interface { Exec(ctx context.Context, sql string, args ...any) (pgconn.CommandTag, error) } -// TODO: EOL this in favor of setCreateMetadata -func newCreateMetadata(atespace, name string) *ateapipb.ResourceMetadata { - now := timestamppb.Now() - return &ateapipb.ResourceMetadata{ - Atespace: atespace, - Name: name, - Uid: uuid.NewString(), - Version: 1, - CreateTime: now, - UpdateTime: now, - } -} - func setCreateMetadata(metadata *ateapipb.ResourceMetadata) { metadata.Uid = uuid.NewString() metadata.Version = 1 @@ -208,14 +195,6 @@ func setCreateMetadata(metadata *ateapipb.ResourceMetadata) { metadata.UpdateTime = metadata.CreateTime } -// TODO: EOL this in favor of setUpdateMetadata -func newUpdateMetadata(current *ateapipb.ResourceMetadata) *ateapipb.ResourceMetadata { - metadata := proto.Clone(current).(*ateapipb.ResourceMetadata) - metadata.Version++ - metadata.UpdateTime = timestamppb.Now() - return metadata -} - // validateProtoMetadataMatchesColumns verifies that the metadata in the database // matches the metadata in the proto. func validateProtoMetadataMatchesColumns(resource string, metadata *ateapipb.ResourceMetadata, uid string, version int64) error { @@ -272,7 +251,12 @@ func (p *Persistence) CreateAtespace(ctx context.Context, atespace *ateapipb.Ate name := atespace.GetMetadata().GetName() dbAtespace := proto.Clone(atespace).(*ateapipb.Atespace) - dbAtespace.Metadata = newCreateMetadata("", name) + if dbAtespace.Metadata == nil { + dbAtespace.Metadata = &ateapipb.ResourceMetadata{} + } + // Atespaces are global-scoped, so the atespace is always empty. + dbAtespace.Metadata.Atespace = "" + setCreateMetadata(dbAtespace.Metadata) protoBytes, err := proto.Marshal(dbAtespace) if err != nil { @@ -384,7 +368,10 @@ func (p *Persistence) DeleteAtespace(ctx context.Context, name string) (*ateapip func (p *Persistence) CreateActorTemplate(ctx context.Context, template *ateapipb.ActorTemplate) (*ateapipb.ActorTemplate, error) { atespace, name := template.GetMetadata().GetAtespace(), template.GetMetadata().GetName() dbTemplate := proto.Clone(template).(*ateapipb.ActorTemplate) - dbTemplate.Metadata = newCreateMetadata(atespace, name) + if dbTemplate.Metadata == nil { + dbTemplate.Metadata = &ateapipb.ResourceMetadata{} + } + setCreateMetadata(dbTemplate.Metadata) protoBytes, err := proto.Marshal(dbTemplate) if err != nil { return nil, fmt.Errorf("marshaling actor template: %w", err) @@ -464,7 +451,10 @@ func (p *Persistence) UpdateActorTemplate(ctx context.Context, templateRef resou if err := validateUpdateActorTemplateMutation(templateBeforeMutation, dbTemplate); err != nil { return nil, err } - dbTemplate.Metadata = newUpdateMetadata(templateBeforeMutation.GetMetadata()) + if dbTemplate.Metadata == nil { + dbTemplate.Metadata = &ateapipb.ResourceMetadata{} + } + setUpdateMetadata(dbTemplate.Metadata, templateBeforeMutation.GetMetadata()) updatedBytes, err := proto.Marshal(dbTemplate) if err != nil { return nil, fmt.Errorf("marshaling actor template: %w", err) @@ -582,11 +572,12 @@ func (p *Persistence) CreateActor(ctx context.Context, actor *ateapipb.Actor) (* atespace := actor.GetMetadata().GetAtespace() name := actor.GetMetadata().GetName() - // TODO: doing a full clone here is wasteful - the caller already has to - // make modifications to the actor before passing it in, so we can safely - // mutate it in place. This breaks some of the contract tests, so we can - // fix it later. - dbActor := proto.Clone(actor).(*ateapipb.Actor) + // The actor is mutated in place: the caller already builds a dedicated + // object to pass in, so a defensive clone is wasted work. + dbActor := actor + if dbActor.Metadata == nil { + dbActor.Metadata = &ateapipb.ResourceMetadata{} + } setCreateMetadata(dbActor.Metadata) protoBytes, err := proto.Marshal(dbActor) @@ -840,7 +831,14 @@ func (p *Persistence) listActorsGlobal(ctx context.Context, pageSize int32, page func (p *Persistence) CreateEgressPolicy(ctx context.Context, actorRef resources.ActorRef, policy *ateapipb.EgressPolicy) (*ateapipb.EgressPolicy, error) { dbPolicy := proto.Clone(policy).(*ateapipb.EgressPolicy) - dbPolicy.Metadata = newCreateMetadata(actorRef.Atespace, "default") + if dbPolicy.Metadata == nil { + dbPolicy.Metadata = &ateapipb.ResourceMetadata{} + } + // The policy is a singleton nested under its actor: its identity comes + // from the actor, not from caller-supplied metadata. + dbPolicy.Metadata.Atespace = actorRef.Atespace + dbPolicy.Metadata.Name = "default" + setCreateMetadata(dbPolicy.Metadata) protoBytes, err := proto.Marshal(dbPolicy) if err != nil { return nil, fmt.Errorf("marshaling egress policy: %w", err) @@ -954,7 +952,10 @@ func (p *Persistence) CreateActorSnapshot(ctx context.Context, snapshot *ateapip atespace := snapshot.GetMetadata().GetAtespace() name := snapshot.GetMetadata().GetName() dbSnapshot := proto.Clone(snapshot).(*ateapipb.ActorSnapshot) - dbSnapshot.Metadata = newCreateMetadata(atespace, name) + if dbSnapshot.Metadata == nil { + dbSnapshot.Metadata = &ateapipb.ResourceMetadata{} + } + setCreateMetadata(dbSnapshot.Metadata) protoBytes, err := proto.Marshal(dbSnapshot) if err != nil { @@ -1123,7 +1124,10 @@ func (p *Persistence) CreateActorSnapshotTag(ctx context.Context, snapshotRef re tagAtespace := tag.GetMetadata().GetAtespace() tagName := tag.GetMetadata().GetName() dbTag := proto.Clone(tag).(*ateapipb.ActorSnapshotTag) - dbTag.Metadata = newCreateMetadata(tagAtespace, tagName) + if dbTag.Metadata == nil { + dbTag.Metadata = &ateapipb.ResourceMetadata{} + } + setCreateMetadata(dbTag.Metadata) dbTag.Snapshot = &ateapipb.ObjectRef{Atespace: snapshotAtespace, Name: snapshotName} protoBytes, err := proto.Marshal(dbTag) if err != nil { @@ -1232,9 +1236,12 @@ func (p *Persistence) UpdateActorSnapshotTag(ctx context.Context, tagRef resourc if err := validateUpdateActorSnapshotTagMutation(tagBeforeMutation, dbTag); err != nil { return nil, fmt.Errorf("%w: %w", store.ErrImmutableField, err) } - // Stored metadata is authoritative; discard any metadata edits made by the - // closure and derive the next revision from the state this attempt read. - dbTag.Metadata = newUpdateMetadata(tagBeforeMutation.GetMetadata()) + // Stored server-assigned metadata is authoritative; the next revision is + // derived from the state this attempt read. + if dbTag.Metadata == nil { + dbTag.Metadata = &ateapipb.ResourceMetadata{} + } + setUpdateMetadata(dbTag.Metadata, tagBeforeMutation.GetMetadata()) updatedBytes, err := proto.Marshal(dbTag) if err != nil { @@ -1280,8 +1287,12 @@ func (p *Persistence) DeleteActorSnapshotTag(ctx context.Context, tagRef resourc func (p *Persistence) CreateWorker(ctx context.Context, worker *ateapipb.Worker) (*ateapipb.Worker, error) { dbWorker := proto.Clone(worker).(*ateapipb.Worker) + if dbWorker.Metadata == nil { + dbWorker.Metadata = &ateapipb.ResourceMetadata{} + } // Workers are global-scoped, so the atespace is always empty. - dbWorker.Metadata = newCreateMetadata("", worker.GetMetadata().GetName()) + dbWorker.Metadata.Atespace = "" + setCreateMetadata(dbWorker.Metadata) protoBytes, err := proto.Marshal(dbWorker) if err != nil { @@ -1372,9 +1383,12 @@ func (p *Persistence) UpdateWorker(ctx context.Context, name string, preconditio if err := store.CheckWorkerMutation(workerBeforeMutation, dbWorker); err != nil { return nil, err } - // Stored metadata is authoritative; discard any metadata edits made by - // the closure and derive the next revision from the row we locked. - dbWorker.Metadata = newUpdateMetadata(workerBeforeMutation.GetMetadata()) + // Stored server-assigned metadata is authoritative; the next revision + // is derived from the row we locked. + if dbWorker.Metadata == nil { + dbWorker.Metadata = &ateapipb.ResourceMetadata{} + } + setUpdateMetadata(dbWorker.Metadata, workerBeforeMutation.GetMetadata()) protoBytes, err := proto.Marshal(dbWorker) if err != nil { diff --git a/cmd/ateapi/internal/store/store.go b/cmd/ateapi/internal/store/store.go index ac5e02f85c..d023583f32 100644 --- a/cmd/ateapi/internal/store/store.go +++ b/cmd/ateapi/internal/store/store.go @@ -68,9 +68,10 @@ var ( // Interface defines the contract for the persistence layer storing actor state. type Interface interface { // Stores a new actor in suspended state and returns the stored resource with - // server-assigned metadata (uid, version, timestamps). The input is not - // mutated. Returns ErrAlreadyExists if key is taken, or - // ErrFailedPrecondition if the actor's atespace does not exist. + // server-assigned metadata (uid, version, timestamps). The input may be + // mutated in place: the caller passes a dedicated object, so the store + // stamps its metadata rather than cloning. Returns ErrAlreadyExists if key + // is taken, or ErrFailedPrecondition if the actor's atespace does not exist. CreateActor(ctx context.Context, actor *ateapipb.Actor) (*ateapipb.Actor, error) // Fetches an actor by reference. Returns ErrNotFound if missing. diff --git a/cmd/ateapi/internal/store/storecontract/contract.go b/cmd/ateapi/internal/store/storecontract/contract.go index 9a9337723f..f72a342945 100644 --- a/cmd/ateapi/internal/store/storecontract/contract.go +++ b/cmd/ateapi/internal/store/storecontract/contract.go @@ -357,9 +357,8 @@ func runActorContractTests(t *testing.T, setup func(t *testing.T) store.Interfac t.Errorf("CreateActor returned unset create/update time") } - if actor.GetMetadata().GetUid() != "" || actor.GetMetadata().GetVersion() != 0 { - t.Errorf("CreateActor must not mutate its input, got metadata %v", actor.GetMetadata()) - } + // CreateActor may stamp the input's metadata in place instead of + // cloning; the caller passes a dedicated object. got, err := s.GetActor(ctx, resources.ActorRefFromActor(actor)) if err != nil { From ef214e321a00227e72b6ae051df5030326da01fb Mon Sep 17 00:00:00 2001 From: shrutiyam-glitch Date: Tue, 1 Sep 2026 16:00:00 -0700 Subject: [PATCH 2/2] remove cloning of resources --- cmd/ateapi/internal/store/atepg/atepg.go | 12 ++++++++---- cmd/ateapi/internal/store/store.go | 12 ++++++------ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/cmd/ateapi/internal/store/atepg/atepg.go b/cmd/ateapi/internal/store/atepg/atepg.go index 628eb0252a..a17c31c12c 100644 --- a/cmd/ateapi/internal/store/atepg/atepg.go +++ b/cmd/ateapi/internal/store/atepg/atepg.go @@ -250,7 +250,8 @@ func pgErrConstraint(err error) string { func (p *Persistence) CreateAtespace(ctx context.Context, atespace *ateapipb.Atespace) (*ateapipb.Atespace, error) { name := atespace.GetMetadata().GetName() - dbAtespace := proto.Clone(atespace).(*ateapipb.Atespace) + // The atespace is mutated in place: callers pass a dedicated object. + dbAtespace := atespace if dbAtespace.Metadata == nil { dbAtespace.Metadata = &ateapipb.ResourceMetadata{} } @@ -367,7 +368,8 @@ func (p *Persistence) DeleteAtespace(ctx context.Context, name string) (*ateapip func (p *Persistence) CreateActorTemplate(ctx context.Context, template *ateapipb.ActorTemplate) (*ateapipb.ActorTemplate, error) { atespace, name := template.GetMetadata().GetAtespace(), template.GetMetadata().GetName() - dbTemplate := proto.Clone(template).(*ateapipb.ActorTemplate) + // The template is mutated in place: callers pass a dedicated object. + dbTemplate := template if dbTemplate.Metadata == nil { dbTemplate.Metadata = &ateapipb.ResourceMetadata{} } @@ -830,7 +832,8 @@ func (p *Persistence) listActorsGlobal(ctx context.Context, pageSize int32, page // --- Actor egress policies --- func (p *Persistence) CreateEgressPolicy(ctx context.Context, actorRef resources.ActorRef, policy *ateapipb.EgressPolicy) (*ateapipb.EgressPolicy, error) { - dbPolicy := proto.Clone(policy).(*ateapipb.EgressPolicy) + // The policy is mutated in place: callers pass a dedicated object. + dbPolicy := policy if dbPolicy.Metadata == nil { dbPolicy.Metadata = &ateapipb.ResourceMetadata{} } @@ -1286,7 +1289,8 @@ func (p *Persistence) DeleteActorSnapshotTag(ctx context.Context, tagRef resourc // --- Workers --- func (p *Persistence) CreateWorker(ctx context.Context, worker *ateapipb.Worker) (*ateapipb.Worker, error) { - dbWorker := proto.Clone(worker).(*ateapipb.Worker) + // The worker is mutated in place: callers pass a dedicated object. + dbWorker := worker if dbWorker.Metadata == nil { dbWorker.Metadata = &ateapipb.ResourceMetadata{} } diff --git a/cmd/ateapi/internal/store/store.go b/cmd/ateapi/internal/store/store.go index 52113ce4d2..1cbfbe5482 100644 --- a/cmd/ateapi/internal/store/store.go +++ b/cmd/ateapi/internal/store/store.go @@ -160,8 +160,8 @@ type Interface interface { DeleteActorSnapshotTag(ctx context.Context, tagRef resources.ActorSnapshotTagRef) (*ateapipb.ActorSnapshotTag, error) // Stores a new atespace and returns the stored resource with server-assigned - // metadata (uid, version, timestamps). The input is not mutated. Returns - // ErrAlreadyExists if the name is taken. + // metadata (uid, version, timestamps). The input may be mutated in place. + // Returns ErrAlreadyExists if the name is taken. CreateAtespace(ctx context.Context, atespace *ateapipb.Atespace) (*ateapipb.Atespace, error) // Fetches an atespace by name. Returns ErrNotFound if missing. @@ -176,8 +176,8 @@ type Interface interface { DeleteAtespace(ctx context.Context, name string) (*ateapipb.Atespace, error) // Stores a new ActorTemplate and returns the stored resource with - // server-assigned metadata (uid, version, timestamps). The input is not - // mutated. Returns ErrAlreadyExists if the (atespace, name) is taken. + // server-assigned metadata (uid, version, timestamps). The input may be + // mutated in place. Returns ErrAlreadyExists if the (atespace, name) is taken. CreateActorTemplate(ctx context.Context, template *ateapipb.ActorTemplate) (*ateapipb.ActorTemplate, error) // Fetches an ActorTemplate by reference. Returns ErrNotFound if missing. @@ -200,8 +200,8 @@ type Interface interface { DeleteActorTemplate(ctx context.Context, templateRef resources.ActorTemplateRef) (*ateapipb.ActorTemplate, error) // Registers a new idle worker and returns the stored resource with - // server-assigned metadata (uid, version, timestamps). The input is not - // mutated. Returns ErrAlreadyExists if already registered. + // server-assigned metadata (uid, version, timestamps). The input may be + // mutated in place. Returns ErrAlreadyExists if already registered. CreateWorker(ctx context.Context, worker *ateapipb.Worker) (*ateapipb.Worker, error) // Fetches worker state by name. Returns ErrNotFound if missing.