Skip to content

config: MovePlugin options not validated mutex, not locked, not atomic #259

@peteski22

Description

@peteski22

Summary

Three related defects in PluginConfig.MovePlugin / movePlugin / newMoveOptions (internal/config/plugin_config.go):

  1. WithBefore, WithAfter, and WithPosition are not validated as mutually exclusive. Caller can pass all three; only one is honoured (switch at the end of movePlugin picks beforeafterposition, first non-nil wins). Others are silently dropped with no error.
  2. internal/config has no mutex. grep -r sync.Mutex internal/config/ returns nothing. Concurrent mutation of the plugin slices is racy.
  3. The sequence moveToCategory then positional move is not atomic. If the positional step fails, the plugin is left in the new category at an arbitrary (appended) position with no rollback.

Proposal

  • newMoveOptions: return an error if more than one of before, after, position is set.
  • Add a mutex (likely sync.Mutex on the parent config that owns PluginConfig, or on PluginConfig itself) covering all mutating operations.
  • Make cross-category + positional moves atomic: snapshot state, attempt both steps, roll back on failure; or compute the full target state first and apply in one step.

Acceptance criteria

  • Unit test: passing two of WithBefore/WithAfter/WithPosition returns a descriptive error.
  • Race test (go test -race) exercising concurrent MovePlugin calls passes.
  • Unit test: a failing positional step after a successful category change leaves the original config unchanged.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions