Config sync can overwrite newer local changes with a stale snapshot result

[michaelw]

Summary

The sync path can overwrite a newer local config change with a stale remote-derived result because it reads a snapshot, normalizes off-lock, and then unconditionally writes that result back later.

This is distinct from the basic "sync result not applied" bug. Even after the apply path is corrected, the algorithm still allows stale-snapshot lost updates.

Minimal repro

I checked out the sync-apply fix only (without any generation/retry protection), added a focused repro test, and ran:

go test ./internal/config -run 'TestSyncStaleSnapshotOverwritesConcurrentRoleUpdate'

Repro test intent:

1. capture the same role snapshot the pre-retry sync path uses
2. derive the normalized remote result from that snapshot
3. apply a newer local write
4. commit the stale normalized result anyway
5. observe the newer local write being overwritten

Representative failing output:

--- FAIL: TestSyncStaleSnapshotOverwritesConcurrentRoleUpdate (0.00s)
    sync_concurrency_issue_test.go:56:
        expected: "local-newer"
        actual  : "remote-update"
        Messages:    stale sync result should not overwrite a newer local update

Analysis

The problem is not just "concurrent work exists"; the current algorithm has a specific stale-snapshot window:

• snapshot current config
• merge/normalize off-lock
• write the result later with no compare-and-swap or retry

If local config changes in that window, the older derived result can win.

Scope

This issue is only about stale-snapshot lost updates in the sync path.

Please keep separate from:

• the base bug where sync results were not applied to live config at all
• broader lock-discipline cleanup across config readers/writers

[michaelw]

Exact temporary repro test source used for the stale-snapshot evidence run on the apply-fix-only state:

package config

import (
	"testing"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	"github.com/thand-io/agent/internal/models"
)

func TestSyncStaleSnapshotOverwritesConcurrentRoleUpdate(t *testing.T) {
	config := newSyncTestConfig(t,
		map[string]models.Role{
			"editor": {Name: "editor", Description: "old", Enabled: true},
		},
		nil, nil, "",
	)

	// Capture the role snapshot the way the pre-retry sync path does before
	// it normalizes and later writes the merged definitions back.
	config.mu.RLock()
	current := RoleConfig{
		Path:        config.Roles.Path,
		URL:         config.Roles.URL,
		Vault:       config.Roles.Vault,
		Definitions: config.Roles.Definitions,
	}
	config.mu.RUnlock()

	var merged RoleConfig
	err := mergeConfigSection(current, RoleConfig{
		Definitions: map[string]models.Role{
			"editor": {Name: "editor", Description: "remote-update", Enabled: true},
		},
	}, &merged)
	require.NoError(t, err)

	defs, err := config.ApplyRoles([]*models.RoleDefinitions{{
		Roles: merged.Definitions,
	}})
	require.NoError(t, err)

	// Simulate a newer local write that lands after the stale snapshot was taken.
	config.mu.Lock()
	config.Roles.Definitions = map[string]models.Role{
		"editor": {Name: "editor", Description: "local-newer", Enabled: true},
	}
	config.mu.Unlock()

	// Pre-retry behavior: commit the stale normalized result anyway.
	config.mu.Lock()
	config.Roles.Definitions = defs
	config.mu.Unlock()

	role := config.Roles.Definitions["editor"]
	assert.Equal(t, "local-newer", role.Description, "stale sync result should not overwrite a newer local update")
}