Skip to content

atepg(cleanup): update the in-place metadata setter migration #1377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shrutiyam-glitch wants to merge 4 commits into
base: main
Choose a base branch
from
+54 −49
Open

atepg(cleanup): update the in-place metadata setter migration #1377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4da7e40
cleanup
shrutiyam-glitch Aug 28, 2026
c510635
Merge branch 'main' into dv-cleanup
shrutiyam-glitch Aug 31, 2026
18b4257
Merge branch 'main' into dv-cleanup
shrutiyam-glitch Sep 1, 2026
ef214e3
remove cloning of resources
shrutiyam-glitch Sep 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 42 additions & 37 deletions cmd/ateapi/internal/store/atepg/atepg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,34 +188,13 @@ 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
metadata.CreateTime = timestamppb.Now()
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 {
Expand Down Expand Up @@ -271,8 +250,14 @@ 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)
dbAtespace.Metadata = newCreateMetadata("", name)
// The atespace is mutated in place: callers pass a dedicated object.
dbAtespace := atespace

@thockin Tim Hockin (thockin) Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just get rid of dbAtespace and use atespace?

if dbAtespace.Metadata == nil {

@thockin Tim Hockin (thockin) Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what conditions would we end up here with nil metadata? If that is the case, we don't know the name of the atespace to create, we certainly can't proceed. This should have been validated already?

I see similar patterns elsewhere -- we should make them all consistent.

dbAtespace.Metadata = &ateapipb.ResourceMetadata{}
}
// Atespaces are global-scoped, so the atespace is always empty.
dbAtespace.Metadata.Atespace = ""

@thockin Tim Hockin (thockin) Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been validated already, right? If this is set and we get this far, there's a problem. Doing this here implies that this is acceptable, and I think it's impossible?

setCreateMetadata(dbAtespace.Metadata)

protoBytes, err := proto.Marshal(dbAtespace)
if err != nil {
Expand Down Expand Up @@ -383,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 {

@thockin Tim Hockin (thockin) Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we really CAN'T be in here without metadata because we don't have a name!

dbTemplate.Metadata = &ateapipb.ResourceMetadata{}
}
Expand Down Expand Up @@ -588,11 +574,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 {

@thockin Tim Hockin (thockin) Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as elsewhere - we really CAN'T be in here without metadata because we don't have a name!

dbActor.Metadata = &ateapipb.ResourceMetadata{}
}
setCreateMetadata(dbActor.Metadata)

protoBytes, err := proto.Marshal(dbActor)
Expand Down Expand Up @@ -845,8 +832,16 @@ 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)
dbPolicy.Metadata = newCreateMetadata(actorRef.Atespace, "default")
// The policy is mutated in place: callers pass a dedicated object.
dbPolicy := policy
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)
Expand Down Expand Up @@ -960,7 +955,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 {
Expand Down Expand Up @@ -1129,7 +1127,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 {
Expand Down Expand Up @@ -1238,9 +1239,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 {
Expand Down Expand Up @@ -1285,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{}
}
Expand Down
19 changes: 10 additions & 9 deletions cmd/ateapi/internal/store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,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.
Expand Down Expand Up @@ -159,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.
Expand All @@ -175,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.
Expand All @@ -199,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.
Expand Down
5 changes: 2 additions & 3 deletions cmd/ateapi/internal/store/storecontract/contract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,9 +356,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 {
Expand Down
Loading