From 9bf0e1bcccdec30bcc3d21fe248094e292ef11b4 Mon Sep 17 00:00:00 2001 From: Stefan Ernst <24554551+stefan-ernst@users.noreply.github.com> Date: Mon, 2 Nov 2026 15:04:47 +0100 Subject: [PATCH] fix(agents): change the model connection, readiness, and version atomically (WI-1803) UpdateAgentConfig rewrote instructions (Draft + version bump) and then changed the LLM connection in a separate write that touched neither. A concurrent ActivateStudioProfile could validate the old connection and mark the profile Ready before the new connection was stored, leaving an unvalidated model Ready. Write the instructions, connection, and runner image in one statement with a single Draft/version transition, and guard activation on the profile version that was validated: a change during validation now yields a version conflict instead of marking the changed definition Ready. --- .../workspace_agent_binding_repository.go | 72 +++++++++---------- internal/services/agent_profile_service.go | 18 ++++- internal/services/binding_service.go | 19 +++-- 3 files changed, 59 insertions(+), 50 deletions(-) diff --git a/internal/repository/workspace_agent_binding_repository.go b/internal/repository/workspace_agent_binding_repository.go index 501794df7..f6967cdd5 100644 --- a/internal/repository/workspace_agent_binding_repository.go +++ b/internal/repository/workspace_agent_binding_repository.go @@ -252,15 +252,17 @@ func (r *WorkspaceAgentBindingRepository) FindByActingUser(ctx context.Context, return b, nil } -// SetLifecycle changes only persisted lifecycle state. Runtime-affecting -// configuration methods own profile-version increments; validating a Draft -// and marking it Ready does not create another definition version. -func (r *WorkspaceAgentBindingRepository) SetLifecycle(ctx context.Context, id, workspaceID int, lifecycle models.AgentLifecycle) (int64, error) { +// SetLifecycleIfVersion changes persisted lifecycle state only when the profile +// version still matches the one that was validated. A concurrent configuration +// change bumps the version, so this refuses to mark a changed definition Ready +// using validation of an older one. Runtime-affecting configuration methods own +// profile-version increments; activating a Draft does not create another version. +func (r *WorkspaceAgentBindingRepository) SetLifecycleIfVersion(ctx context.Context, id, workspaceID int, lifecycle models.AgentLifecycle, expectedVersion int) (int64, error) { res, err := r.db.ExecWriteContext(ctx, ` UPDATE workspace_agent_bindings SET lifecycle = ?, updated_at = CURRENT_TIMESTAMP - WHERE id = ? AND workspace_id = ? AND lifecycle <> 'archived' - `, lifecycle, id, workspaceID) + WHERE id = ? AND workspace_id = ? AND lifecycle <> 'archived' AND profile_version = ? + `, lifecycle, id, workspaceID, expectedVersion) if err != nil { return 0, fmt.Errorf("set binding lifecycle: %w", err) } @@ -361,19 +363,34 @@ func (r *WorkspaceAgentBindingRepository) Restore(ctx context.Context, id, works return n, nil } -// UpdateInstructions rewrites a binding's custom instructions, scoped by -// workspace (WI-258). -func (r *WorkspaceAgentBindingRepository) UpdateInstructions(ctx context.Context, id, workspaceID int, instructions string) error { - _, err := r.db.ExecWriteContext(ctx, ` - UPDATE workspace_agent_bindings - SET instructions = ?, - lifecycle = CASE WHEN lifecycle = 'archived' THEN lifecycle ELSE 'draft' END, - profile_version = profile_version + 1, - updated_at = CURRENT_TIMESTAMP - WHERE id = ? AND workspace_id = ? - `, instructions, id, workspaceID) - if err != nil { - return fmt.Errorf("update binding instructions: %w", err) +// UpdateAgentConfig atomically rewrites the Agent Studio overview fields — +// instructions, model connection, and runner image — and, in the same +// statement, invalidates readiness and bumps the profile version once. One +// write keeps a concurrent activation from validating the old connection and +// marking the profile Ready before the new connection is stored. A nil +// connection or runner image preserves the current value; a non-nil empty +// runner image clears the override. +func (r *WorkspaceAgentBindingRepository) UpdateAgentConfig(ctx context.Context, id, workspaceID int, instructions string, llmConnectionID *int, runnerImage *string) error { + set := []string{ + "instructions = ?", + "lifecycle = CASE WHEN lifecycle = 'archived' THEN lifecycle ELSE 'draft' END", + "profile_version = profile_version + 1", + "updated_at = CURRENT_TIMESTAMP", + } + args := []any{instructions} + if llmConnectionID != nil { + set = append(set, "llm_connection_id = ?") + args = append(args, *llmConnectionID) + } + if runnerImage != nil { + set = append(set, "runner_image = ?") + args = append(args, nullStringArg(*runnerImage)) + } + args = append(args, id, workspaceID) + //nolint:gosec // G201: only fixed column fragments are interpolated; every value is bound + query := "UPDATE workspace_agent_bindings SET " + strings.Join(set, ", ") + " WHERE id = ? AND workspace_id = ?" + if _, err := r.db.ExecWriteContext(ctx, query, args...); err != nil { + return fmt.Errorf("update agent config: %w", err) } return nil } @@ -425,23 +442,6 @@ func (r *WorkspaceAgentBindingRepository) UpdateConfig(ctx context.Context, b *m return nil } -// UpdateLLMConnection rewrites a binding's model connection in place, scoped by -// (id, workspace_id). It does not touch the lifecycle or profile version: the -// caller's instruction rewrite in UpdateAgentConfig owns the single Draft/version -// transition for the whole configuration change. -func (r *WorkspaceAgentBindingRepository) UpdateLLMConnection(ctx context.Context, id, workspaceID, connectionID int) error { - _, err := r.db.ExecWriteContext(ctx, ` - UPDATE workspace_agent_bindings - SET llm_connection_id = ?, - updated_at = CURRENT_TIMESTAMP - WHERE id = ? AND workspace_id = ? - `, connectionID, id, workspaceID) - if err != nil { - return fmt.Errorf("update binding llm connection: %w", err) - } - return nil -} - // UpdateRunnerImage rewrites a binding's custom runner image, scoped by // workspace. An empty image clears the override (NULL = the runner's default // windshift-agent image) (WI-450). diff --git a/internal/services/agent_profile_service.go b/internal/services/agent_profile_service.go index fa3d180a7..db8b6da56 100644 --- a/internal/services/agent_profile_service.go +++ b/internal/services/agent_profile_service.go @@ -74,6 +74,10 @@ type ProfileValidationError struct { type ProfileValidationResult struct { Ready bool `json:"ready"` Errors []ProfileValidationError `json:"errors"` + // ProfileVersion is the definition version the checks ran against. + // Activation only succeeds while the stored version still matches it, so a + // configuration change during validation cannot be marked Ready. + ProfileVersion int `json:"profile_version"` } // UpdateStudioProfileRequest carries the mutable overview fields. Profile type @@ -526,7 +530,7 @@ func (s *BindingService) ValidateStudioProfile(ctx context.Context, workspaceID, return nil, fmt.Errorf("load agent profile: %w", err) } - result := &ProfileValidationResult{Errors: []ProfileValidationError{}} + result := &ProfileValidationResult{Errors: []ProfileValidationError{}, ProfileVersion: binding.ProfileVersion} var active, isAgent bool if err := s.db.QueryRowContext(ctx, ` SELECT COALESCE(is_active, false), COALESCE(is_agent, false) @@ -649,15 +653,23 @@ func (s *BindingService) ActivateStudioProfile(ctx context.Context, workspaceID, if err != nil { return nil, nil, err } + return s.activateStudioProfile(ctx, workspaceID, bindingID, validation) +} + +// activateStudioProfile transitions a validated definition to Ready, but only +// while the stored profile version still matches the one the validation ran +// against. A concurrent configuration change bumps the version, so this refuses +// to mark a changed definition Ready using validation of an older one. +func (s *BindingService) activateStudioProfile(ctx context.Context, workspaceID, bindingID int, validation *ProfileValidationResult) (*models.WorkspaceAgentBinding, *ProfileValidationResult, error) { if !validation.Ready { return nil, validation, ErrAgentProfileValidationFailed } - rows, err := s.repo.SetLifecycle(ctx, bindingID, workspaceID, models.AgentLifecycleReady) + rows, err := s.repo.SetLifecycleIfVersion(ctx, bindingID, workspaceID, models.AgentLifecycleReady, validation.ProfileVersion) if err != nil { return nil, validation, err } if rows == 0 { - return nil, validation, ErrBindingUnavailable + return nil, validation, ErrAgentProfileVersionConflict } binding, err := s.repo.Get(ctx, bindingID) if err != nil { diff --git a/internal/services/binding_service.go b/internal/services/binding_service.go index 1e88af7be..6a948ce65 100644 --- a/internal/services/binding_service.go +++ b/internal/services/binding_service.go @@ -723,18 +723,15 @@ func (s *BindingService) UpdateAgentConfig(ctx context.Context, workspaceID, bin return ErrLLMConnectionInvalid } } - if err := s.repo.UpdateInstructions(ctx, bindingID, workspaceID, instructions); err != nil { - return err - } - if llmConnectionID != nil { - if err := s.repo.UpdateLLMConnection(ctx, bindingID, workspaceID, *llmConnectionID); err != nil { - return err - } - } + var runnerImageArg *string if runnerImage != nil { - if err := s.repo.UpdateRunnerImage(ctx, bindingID, workspaceID, newRunnerImage); err != nil { - return err - } + runnerImageArg = &newRunnerImage + } + // One atomic write so the model connection, readiness invalidation, and the + // single version bump land together; a concurrent activation must never see + // the old connection with a fresh version. + if err := s.repo.UpdateAgentConfig(ctx, bindingID, workspaceID, instructions, llmConnectionID, runnerImageArg); err != nil { + return err } if s.skills == nil { if len(skillIDs) > 0 {