Skip to content

cre-7120: decode core defaults per call to avoid shared mutable state #24052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mchain0 wants to merge 1 commit into
base: develop
Choose a base branch
from
+51 −8
Open

cre-7120: decode core defaults per call to avoid shared mutable state #24052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions core/config/docs/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,10 @@ import (
"github.com/smartcontractkit/chainlink/v2/core/config/toml"
)

var defaults toml.Core

func init() {
if err := configdoc.DefaultsOnly(strings.NewReader(coreTOML), &defaults, commonconfig.DecodeTOML); err != nil {
func CoreDefaults() (c toml.Core) {
if err := configdoc.DefaultsOnly(strings.NewReader(coreTOML), &c, commonconfig.DecodeTOML); err != nil {
log.Fatalf("Failed to initialize defaults from docs: %v", err)
}
}

func CoreDefaults() (c toml.Core) {
c.SetFrom(&defaults)

@jmank88 jmank88 Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we fix this in SetFrom? That is already meant to be doing a deep clone.

@jmank88 jmank88 Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we just got sloppy with setting pointers instead of copying values? I think we want to fix that regardless

@jmank88 jmank88 Oct 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that it often hasn't mattered in the past since we would just point to a new place rather than update the value anyways.

@pavel-raykov pavel-raykov Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, do you mean a fix like #24059 ?

@mchain0 mchain0 Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can cover the entire problem in SetFrom. CoreDefaults() decodes coreTOML fresh each call - every value in the returned tree is a brand-new allocation from the decoder. This is a complete deep copy by construction — no aliasing anywhere, including URL internals and slice elements.

I experimented with SetFrom, copying within like:

func copyPtr[T any](p *T) *T {
       if p == nil {
               return nil
       }
       v := *p
       return &v
}

but that would only de-reference only one layer, it's a shallow copy of the value: anything the value contains by reference still aliases the source - e.g. commonconfig.URL's inner *url.Userinfo, slice/map elements', etc.

Doing full generic copy in SetFrom as truly recursive clone would be always flawed in this case: reflection-based recursive cloner, it would stop at leaves it can't safely clone - e.g. commonconfig.Duration (unexported d time); marshal->unmarshal round-trip would loose fields with "-" (e.g. Core.AppID uuid.UUID).

Current codebase doesn't support deep clone copy cleanly today.

The only true full-deep-copy primitive I can see here is TOML decode.

If you agree the best course of action imho would be to keep per-call decode, and maybe add a doc comment on Core.SetFrom (or the toml package) stating the actual contract: "overlay merge; copies pointers for set fields — the source must be short-lived or caller-owned; do not alias long-lived globals through it"

c.Database.DriverName = pgcommon.DriverPostgres // not user visible - overridden for tests only
c.Tracing.Attributes = make(map[string]string)
return c
Expand Down
12 changes: 12 additions & 0 deletions core/config/docs/defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,21 @@ package docs
import (
"testing"

"github.com/stretchr/testify/assert"

"github.com/smartcontractkit/chainlink-common/pkg/config/configtest"
)

func TestCoreDefaults_notNil(t *testing.T) {
configtest.AssertFieldsNotNil(t, CoreDefaults())
}

func TestCoreDefaults_distinctInstances(t *testing.T) {
t.Parallel()
a := CoreDefaults()
b := CoreDefaults()
*a.Insecure.DevWebServer = true
*a.AuditLogger.Enabled = true
assert.False(t, *b.Insecure.DevWebServer)
assert.False(t, *b.AuditLogger.Enabled)
}
37 changes: 37 additions & 0 deletions core/services/chainlink/config_general_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"maps"
"net/url"
"strings"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -83,6 +84,42 @@ func TestTOMLGeneralConfig_InsecureConfig(t *testing.T) {
})
}

func TestGeneralConfigOpts_ConcurrentOverrideAndNew(t *testing.T) {
t.Parallel()

var wg sync.WaitGroup
wg.Add(2)

go func() {
defer wg.Done()
for range 100 {
_, err := GeneralConfigOpts{
OverrideFn: func(c *Config, s *Secrets) {
*c.Insecure.DevWebServer = true
*c.AuditLogger.Enabled = true
},
}.New()
if err != nil {
t.Error(err)
return
}
}
}()

go func() {
defer wg.Done()
for range 100 {
opts := GeneralConfigOpts{}
if _, err := opts.New(); err != nil {
t.Error(err)
return
}
}
}()

wg.Wait()
}

func TestValidateDB(t *testing.T) {
t.Setenv(string(env.Config), "")

Expand Down
Loading