RFC: fix config sync application and retry semantics

[michaelw]

This fixes a pre-existing config sync bug where server-synced role, workflow, and provider definitions could be validated without becoming the live in-memory config, and then hardens that merged sync path against stale-snapshot lost updates.

How to Read

• Look at the test results (comments below)
• start with the first commit only; two concerns: (1) Apply* results get thrown away?, and (2) sparse diffs would cause zeroed values that the validation would throw away anyway.

If that makes sense:

• second commit highlights a potential concurrency issue (lost updates when switching from RLock to Lock sections).

What changed

• apply the full merged server state directly in MergeConfiguration, instead of routing that merged state back through the partial-patch helper
• keep applyPatch as the helper for real partial section diffs, with shared normalize/store helpers underneath
• add generation-based retry around the merged sync path only, with definitions-only compare/commit and detached snapshots for retry isolation

Residual gap

Definition maps are now treated as immutable snapshots for sync and reload purposes, but broader nested-mutation and lock-discipline cleanup is still tracked separately in #306. The skipped evidence tests in this branch document that remaining gap without widening this PR.

Validation

• go test ./internal/config
• go test ./...

[michaelw]

Upstream red/green evidence for the sync application fix:

On untouched upstream/main, I overlaid the first-commit regression test file from this branch and ran:

go test ./internal/config -run 'TestMergeConfiguration_|TestApplyPatch_'

That repro fails on the pre-fix code with the expected sync-application breakages:

--- FAIL: TestMergeConfiguration_ServerSendsNewRoles
    expected synced role to be stored locally

--- FAIL: TestMergeConfiguration_ServerSendsUpdatedRole
    expected: "Can edit and publish"
    actual  : "Can edit"

--- FAIL: TestMergeConfiguration_ServerSendsNewWorkflows
    expected synced workflow to be stored locally

--- FAIL: TestMergeConfiguration_ServerSendsUpdatedWorkflow
    expected: "Updated workflow"
    actual  : "Existing workflow"

--- FAIL: TestMergeConfiguration_ServerSendsNewProviders
    expected synced provider to be stored locally

--- FAIL: TestMergeConfiguration_ServerSendsUpdatedProvider
    expected: "Updated provider description"
    actual  : "Old provider description"

--- FAIL: TestMergeConfiguration_PartialConfig_OnlyRoles
    expected synced role to be stored locally

--- FAIL: TestApplyPatch_AppliesRoles
    map[string]models.Role(nil) does not contain "new-role"

Representative constructed fixtures from those tests:

// new role
local := Roles.Definitions = nil
server := RegistrationResponse{
  Roles: {Definitions: {"admin": {Name: "admin", Enabled: true}}},
}
// expected after MergeConfiguration:
// Roles.Definitions == {"admin": {Name: "admin", Enabled: true}}

// updated role
local := Roles.Definitions = {
  "editor":    {Name: "editor", Description: "Can edit", Enabled: true},
  "untouched": {Name: "untouched", Description: "Keep me", Enabled: true},
}
server := RegistrationResponse{
  Roles: {Definitions: {"editor": {Name: "editor", Description: "Can edit and publish", Enabled: true}}},
}
// expected after MergeConfiguration:
// editor.Description == "Can edit and publish"
// untouched still present

// new workflow
local := Workflows.Definitions = nil
server := RegistrationResponse{
  Workflows: {Definitions: {"approval": makeTestWorkflow("approval", "Handles approvals")}},
}
// expected after MergeConfiguration:
// Workflows.Definitions["approval"].Description == "Handles approvals"

// updated provider
local := Providers.Definitions = {
  "mock-primary": {Name: "mock-primary", Description: "Old provider description", Provider: "mock", Enabled: true},
  "mock-extra":   {Name: "mock-extra", Description: "Keep me", Provider: "mock", Enabled: true},
}
server := RegistrationResponse{
  Providers: {Definitions: {"mock-primary": {Name: "mock-primary", Description: "Updated provider description", Provider: "mock", Enabled: true}}},
}
// expected after MergeConfiguration:
// mock-primary.Description == "Updated provider description"
// mock-extra still present

The same focused cases pass on this branch:

go test ./internal/config -run 'TestMergeConfiguration_ServerSends(NewRoles|UpdatedRole|NewWorkflows|UpdatedWorkflow|NewProviders|UpdatedProvider|PartialConfig_OnlyRoles)|TestApplyPatch_AppliesRoles'

So the red/green story here is: upstream computes and validates the incoming synced definitions, but the live c.Roles.Definitions, c.Workflows.Definitions, and c.Providers.Definitions maps are not updated. This branch fixes that caller behavior and then hardens the sync path separately in the second commit.

[michaelw]

the defs returned by ApplyRoles get thrown away here?

[michaelw]

this patch is sparse AIUI, contains only fields that changed:

{
  "roles": {
    "definitions": {
      "editor": {
        "description": "Can edit and publish"
      }
    }
  }
}

[michaelw]

I think Unmarshal will do the wrong thing here for the example above:

ConfigPatchRequest{
    RoleConfig: &RoleConfig{
        Definitions: map[string]models.Role{
            "editor": {
                Name:        "",
                Description: "Can edit and publish",
                Enabled:     false,
            },
        },
    },
}

We can't discriminate whether Name: "" was an intended change, or just the zero default value backfilled by Unmarshal.

[michaelw]

This is the ugly bit: mergedConfig is always a full patch, so that no fields are zeroed. There is no point to apply the patch as before, hence use applyMergedConfig below.

[michaelw]

Here the changes are applied.

(The tricky bit here is that between the RLock above, and the Lock section in storeRoleDefinitions, some other update could have happened. That's addressed in the second commit of this branch.)