Address auto-standby review feedback

Author: sjmiller609

cmd/api/api/auto_standby.go

@@ -26,7 +26,7 @@ func toDomainAutoStandbyPolicy(policy *oapi.AutoStandbyPolicy) (*autostandby.Pol
 	if policy.IgnoreDestinationPorts != nil {
 		out.IgnoreDestinationPorts = make([]uint16, 0, len(*policy.IgnoreDestinationPorts))
 		for _, port := range *policy.IgnoreDestinationPorts {
-			if port < 0 || port > 65535 {
+			if port < 1 || port > 65535 {
 				return nil, fmt.Errorf("auto_standby.ignore_destination_ports must be between 1 and 65535")
 			}
 			out.IgnoreDestinationPorts = append(out.IgnoreDestinationPorts, uint16(port))

cmd/api/api/instances_test.go

@@ -636,6 +636,48 @@ func TestUpdateInstance_MapsAutoStandbyPatch(t *testing.T) {
 	assert.True(t, *instance.AutoStandby.Enabled)
 }
 
+func TestUpdateInstance_RejectsZeroAutoStandbyIgnoreDestinationPort(t *testing.T) {
+	t.Parallel()
+	svc := newTestService(t)
+
+	origMgr := svc.InstanceManager
+	now := time.Now()
+	mockMgr := &captureUpdateManager{Manager: origMgr}
+	svc.InstanceManager = mockMgr
+
+	resolved := &instances.Instance{
+		StoredMetadata: instances.StoredMetadata{
+			Id:             "inst-update-auto-standby",
+			Name:           "inst-update-auto-standby",
+			Image:          "docker.io/library/alpine:latest",
+			CreatedAt:      now,
+			HypervisorType: hypervisor.TypeCloudHypervisor,
+		},
+		State: instances.StateStopped,
+	}
+	enabled := true
+	idleTimeout := "10m"
+	ignoreDestinationPorts := []int{0}
+
+	resp, err := svc.UpdateInstance(mw.WithResolvedInstance(ctx(), resolved.Id, resolved), oapi.UpdateInstanceRequestObject{
+		Id: resolved.Id,
+		Body: &oapi.UpdateInstanceRequest{
+			AutoStandby: &oapi.AutoStandbyPolicy{
+				Enabled:                &enabled,
+				IdleTimeout:            &idleTimeout,
+				IgnoreDestinationPorts: &ignoreDestinationPorts,
+			},
+		},
+	})
+	require.NoError(t, err)
+
+	badReq, ok := resp.(oapi.UpdateInstance400JSONResponse)
+	require.True(t, ok, "expected 400 response")
+	assert.Equal(t, "invalid_auto_standby", badReq.Code)
+	assert.Contains(t, badReq.Message, "between 1 and 65535")
+	assert.Nil(t, mockMgr.lastReq)
+}
+
 func TestUpdateInstance_RequiresBody(t *testing.T) {
 	t.Parallel()
 	svc := newTestService(t)

lib/instances/metadata_clone.go

@@ -0,0 +1,11 @@
+package instances
+
+func cloneMetadata(src *metadata) *metadata {
+	if src == nil {
+		return nil
+	}
+
+	return &metadata{
+		StoredMetadata: cloneStoredMetadataForFork(src.StoredMetadata),
+	}
+}

lib/instances/update.go

@@ -43,11 +43,13 @@ func (m *manager) updateInstance(ctx context.Context, id string, req UpdateInsta
 	if len(req.Env) > 0 && inst.State != StateRunning && inst.State != StateInitializing {
 		return nil, fmt.Errorf("%w: instance must be running or initializing to update env (current state: %s)", ErrInvalidState, inst.State)
 	}
+	nextMeta := meta
 	if req.AutoStandby != nil {
-		meta.AutoStandby = cloneAutoStandbyPolicy(req.AutoStandby)
+		nextMeta = cloneMetadata(meta)
+		nextMeta.AutoStandby = cloneAutoStandbyPolicy(req.AutoStandby)
 	}
 	if len(req.Env) == 0 {
-		if err := m.saveMetadata(meta); err != nil {
+		if err := m.saveMetadata(nextMeta); err != nil {
 			return nil, fmt.Errorf("save metadata: %w", err)
 		}
 
@@ -60,16 +62,16 @@ func (m *manager) updateInstance(ctx context.Context, id string, req UpdateInsta
 		return updated, nil
 	}
 
-	prevEnv := cloneEnvMap(meta.Env)
-	nextEnv := cloneEnvMap(meta.Env)
+	prevEnv := cloneEnvMap(nextMeta.Env)
+	nextEnv := cloneEnvMap(nextMeta.Env)
 	if nextEnv == nil {
 		nextEnv = make(map[string]string)
 	}
 	for k, v := range req.Env {
 		nextEnv[k] = v
 	}
 
-	if err := validateCredentialEnvBindings(meta.Credentials, nextEnv); err != nil {
+	if err := validateCredentialEnvBindings(nextMeta.Credentials, nextEnv); err != nil {
 		return nil, err
 	}
 
@@ -79,7 +81,7 @@ func (m *manager) updateInstance(ctx context.Context, id string, req UpdateInsta
 		return nil, fmt.Errorf("egress proxy service unavailable")
 	}
 
-	if err := applyUpdatedInstanceEnv(ctx, log, id, meta, prevEnv, nextEnv, m.saveMetadata, svc); err != nil {
+	if err := applyUpdatedInstanceEnv(ctx, log, id, nextMeta, prevEnv, nextEnv, m.saveMetadata, svc); err != nil {
 		return nil, err
 	}
 

lib/instances/update_test.go

@@ -181,3 +181,61 @@ func TestApplyUpdatedInstanceEnvReturnsRollbackFailure(t *testing.T) {
 	assert.Equal(t, prevEnv, meta.Env)
 	require.Len(t, svc.calls, 2)
 }
+
+func TestApplyUpdatedInstanceEnvSavesAutoStandbyAlongsideEnvWithoutMutatingOriginal(t *testing.T) {
+	t.Parallel()
+
+	original := &metadata{
+		StoredMetadata: StoredMetadata{
+			Id:            "inst-autostandby-copy",
+			NetworkEgress: &NetworkEgressPolicy{Enabled: true},
+			Credentials: map[string]CredentialPolicy{
+				"OUTBOUND_OPENAI_KEY": {
+					Source: CredentialSource{Env: "OUTBOUND_OPENAI_KEY"},
+					Inject: []CredentialInjectRule{{
+						As: CredentialInjectAs{
+							Header: "Authorization",
+							Format: "Bearer ${value}",
+						},
+					}},
+				},
+			},
+			Env: map[string]string{"OUTBOUND_OPENAI_KEY": "old"},
+			AutoStandby: &autostandby.Policy{
+				Enabled:     false,
+				IdleTimeout: "5m0s",
+			},
+		},
+	}
+	updated := cloneMetadata(original)
+	updated.AutoStandby = &autostandby.Policy{
+		Enabled:                true,
+		IdleTimeout:            "10m0s",
+		IgnoreSourceCIDRs:      []string{"10.0.0.0/8"},
+		IgnoreDestinationPorts: []uint16{22},
+	}
+
+	prevEnv := cloneEnvMap(updated.Env)
+	nextEnv := map[string]string{"OUTBOUND_OPENAI_KEY": "new"}
+	svc := &fakeUpdateInstanceRulesService{}
+
+	var saved *metadata
+	err := applyUpdatedInstanceEnv(context.Background(), nil, updated.Id, updated, prevEnv, nextEnv, func(meta *metadata) error {
+		saved = cloneMetadata(meta)
+		return nil
+	}, svc)
+	require.NoError(t, err)
+
+	require.NotNil(t, saved)
+	require.NotNil(t, saved.AutoStandby)
+	assert.True(t, saved.AutoStandby.Enabled)
+	assert.Equal(t, "10m0s", saved.AutoStandby.IdleTimeout)
+	assert.Equal(t, []string{"10.0.0.0/8"}, saved.AutoStandby.IgnoreSourceCIDRs)
+	assert.Equal(t, []uint16{22}, saved.AutoStandby.IgnoreDestinationPorts)
+	assert.Equal(t, nextEnv, saved.Env)
+
+	require.NotNil(t, original.AutoStandby)
+	assert.False(t, original.AutoStandby.Enabled)
+	assert.Equal(t, "5m0s", original.AutoStandby.IdleTimeout)
+	assert.Equal(t, map[string]string{"OUTBOUND_OPENAI_KEY": "old"}, original.Env)
+}