diff --git a/e2e/agentnetwork/settings_bootstrap_test.go b/e2e/agentnetwork/settings_bootstrap_test.go index 5f74684c5..74d8c1c0c 100644 --- a/e2e/agentnetwork/settings_bootstrap_test.go +++ b/e2e/agentnetwork/settings_bootstrap_test.go @@ -251,7 +251,10 @@ func TestSettingsConditionalWrites(t *testing.T) { // The delete is conditional too, and refusing a stale one leaves the // endpoint claimed. - require.Error(t, fresh.DeleteSettingsIfMatch(ctx, etag), "a stale precondition must refuse the delete") + err = fresh.DeleteSettingsIfMatch(ctx, etag) + require.Error(t, err, "a stale precondition must refuse the delete") + require.True(t, rest.IsPreconditionFailed(err), + "the delete must be refused for staleness rather than for a state guard or a server error, got: %v", err) stillThere, err := fresh.GetSettings(ctx) require.NoError(t, err, "read after the refused delete must succeed") assert.Equal(t, planned.Endpoint, stillThere.Endpoint, "the refused delete must leave the endpoint claimed") diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 5e4274910..0b7c4ed09 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -545,6 +545,13 @@ func (m *managerImpl) DeleteBudgetRule(ctx context.Context, accountID, userID, r return nil } +// stalePreconditionMsg is the refusal both conditional settings writes return. +// Shared so the two cannot drift: DeleteSettings answers 412 for its state +// guards as well, so the message is the only thing telling a client that it is +// working from an old read rather than tripping over providers or a serving +// proxy. +const stalePreconditionMsg = "if-match precondition failed: the settings have changed since they were read; GET them again and retry" + // UpdateSettings replaces the mutable account-level settings — the collection // toggles and retention — on the account's row. The identity fields (Domain, // ProxyAddress) are assigned at bootstrap (CreateSettings) and immutable: the @@ -589,7 +596,7 @@ func (m *managerImpl) UpdateSettings(ctx context.Context, userID string, setting // working from an old read" is the more accurate answer than "the // endpoint is immutable". if !precondition.Matches(existing.ETag()) { - return status.Errorf(status.PreconditionFailed, "if-match precondition failed: the settings have changed since they were read; GET them again and retry") + return status.Errorf(status.PreconditionFailed, "%s", stalePreconditionMsg) } // The identity echo is compared leniently (trimmed, case-insensitive): @@ -679,7 +686,7 @@ func (m *managerImpl) DeleteSettings(ctx context.Context, accountID, userID stri // before the state guards: a caller working from an old read should // learn that first, not be told about providers it may not know exist. if !precondition.Matches(existing.ETag()) { - return status.Errorf(status.PreconditionFailed, "if-match precondition failed: the settings have changed since they were read; GET them again and retry") + return status.Errorf(status.PreconditionFailed, "%s", stalePreconditionMsg) } providers, err := tx.GetAccountAgentNetworkProviders(ctx, store.LockingStrengthNone, accountID) diff --git a/management/internals/modules/agentnetwork/settings_etag_test.go b/management/internals/modules/agentnetwork/settings_etag_test.go index db54df5e9..b892ee88c 100644 --- a/management/internals/modules/agentnetwork/settings_etag_test.go +++ b/management/internals/modules/agentnetwork/settings_etag_test.go @@ -79,12 +79,18 @@ func TestUpdateSettingsPreconditionSerializesConcurrentWriters(t *testing.T) { // a diff and is about to write the whole object back would. shared := created.ETag() + // noWrite is a retention value neither writer sends and the API would + // never store, so an assertion that lands on it is a test bug rather than + // a silently satisfied comparison. Zero would not do: the API documents 0 + // as "keep indefinitely", so it is a value the row could legitimately hold. + const noWrite = -1 + var ( - wg sync.WaitGroup - start = make(chan struct{}) - errs = make([]error, 2) - wrote = []int{7, 21} - winner = make([]int, 2) + wg sync.WaitGroup + start = make(chan struct{}) + errs = make([]error, 2) + wrote = []int{7, 21} + returned = []int{noWrite, noWrite} ) for i := range 2 { wg.Add(1) @@ -94,17 +100,19 @@ func TestUpdateSettingsPreconditionSerializesConcurrentWriters(t *testing.T) { updated, err := f.manager.UpdateSettings(ctx, userID, updateFor(created, wrote[i]), ifMatch(t, shared)) errs[i] = err if err == nil { - winner[i] = updated.AccessLogRetentionDays + returned[i] = updated.AccessLogRetentionDays } }() } close(start) wg.Wait() - succeeded := 0 + succeeded, winner := 0, noWrite for i, err := range errs { if err == nil { succeeded++ + winner = wrote[i] + assert.Equal(t, wrote[i], returned[i], "the winner's response must carry what it sent") continue } assert.Truef(t, isPreconditionFailed(err), @@ -115,7 +123,7 @@ func TestUpdateSettingsPreconditionSerializesConcurrentWriters(t *testing.T) { // The row must carry the winner's value and nothing blended. stored, err := f.store.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, accountID) require.NoError(t, err, "the row must survive the race") - assert.Contains(t, winner, stored.AccessLogRetentionDays, + assert.Equal(t, winner, stored.AccessLogRetentionDays, "the stored row must be exactly what the winning writer sent") assert.NotEqual(t, shared, stored.ETag(), "the surviving row must derive a new validator") } diff --git a/management/internals/modules/agentnetwork/types/settings.go b/management/internals/modules/agentnetwork/types/settings.go index 89fa47d2d..66028df86 100644 --- a/management/internals/modules/agentnetwork/types/settings.go +++ b/management/internals/modules/agentnetwork/types/settings.go @@ -98,6 +98,19 @@ const etagLength = 16 // held across that gap would then authorize a write against what is really a // different resource. CreatedAt is what distinguishes the re-bootstrapped row. // +// CreatedAt is hashed at whole-second precision because the validator has to +// agree across a store round-trip. A freshly bootstrapped row derives its +// validator in memory, from a time.Time carrying nanoseconds, while every +// later comparison derives it from a row read back out of the store — and the +// engines truncate: PostgreSQL to microseconds, MySQL DATETIME to whole +// seconds without an fsp. At nanosecond precision the two never agree again, +// so the validator a bootstrap hands out is permanently unusable. Seconds is +// the floor every supported engine preserves. The cost is that a delete and +// re-bootstrap within the same second, onto the same endpoint and the same +// toggles, derives the same validator; a labeled bootstrap draws a fresh +// random label, so that needs a self-addressed endpoint reclaimed inside one +// second. +// // Adding a field to Settings means deciding whether it belongs here; the // field-count guard in the tests is what forces that decision. func (s *Settings) ETag() string { @@ -109,7 +122,7 @@ func (s *Settings) ETag() string { s.EnablePromptCollection, s.RedactPii, s.AccessLogRetentionDays, - s.CreatedAt.UnixNano(), + s.CreatedAt.Unix(), ) return hex.EncodeToString(h.Sum(nil))[:etagLength] } diff --git a/management/internals/modules/agentnetwork/types/settings_test.go b/management/internals/modules/agentnetwork/types/settings_test.go index 2a552f03e..d911cee27 100644 --- a/management/internals/modules/agentnetwork/types/settings_test.go +++ b/management/internals/modules/agentnetwork/types/settings_test.go @@ -11,8 +11,10 @@ import ( // etagSettings is a fully populated settings row — every hashed field set to a // distinctive value — so a mutation test can flip exactly one thing at a time. +// The timestamp carries sub-second precision on purpose: a whole-second value +// would make the precision test below pass without proving anything. func etagSettings() *Settings { - created := time.Date(2026, 8, 11, 9, 30, 0, 0, time.UTC) + created := time.Date(2026, 8, 11, 9, 30, 0, 123456789, time.UTC) return &Settings{ AccountID: "acc-1", Domain: "cool-otter.eu.proxy.netbird.io", @@ -73,7 +75,7 @@ func TestSettings_ETagSensitivity(t *testing.T) { {"prompt collection", func(s *Settings) { s.EnablePromptCollection = false }}, {"redact pii", func(s *Settings) { s.RedactPii = false }}, {"retention", func(s *Settings) { s.AccessLogRetentionDays = 14 }}, - {"created at", func(s *Settings) { s.CreatedAt = s.CreatedAt.Add(time.Nanosecond) }}, + {"created at", func(s *Settings) { s.CreatedAt = s.CreatedAt.Add(time.Second) }}, // Moving characters across the Domain/ProxyAddress boundary leaves // the two fields' concatenation byte-identical, so this case passes // only because the tuple is delimited. @@ -120,6 +122,37 @@ func TestSettings_ETagExclusions(t *testing.T) { assert.Equal(t, baseline, touched.ETag(), "a write that changed nothing must not move the validator") } +// TestSettings_ETagSurvivesTimestampTruncation pins the store round-trip the +// validator has to survive. A freshly bootstrapped row derives its validator +// in memory, from a time.Time carrying nanoseconds; every later comparison +// derives it from a row read back out of the store, and the engines truncate +// on the way through — PostgreSQL to microseconds, MySQL DATETIME to whole +// seconds without an fsp. If the hash is sensitive below its coarsest engine's +// precision, the validator a bootstrap hands out never matches again and the +// documented "conditional PUT without an intervening GET" is a permanent 412. +// +// Asserted here on the type rather than through a store, so it holds without +// running the suite against every engine — which is what let this through the +// first time, since sqlite preserves nanoseconds and every other test uses it. +func TestSettings_ETagSurvivesTimestampTruncation(t *testing.T) { + inMemory := etagSettings() + require.NotZero(t, inMemory.CreatedAt.Nanosecond(), "the fixture must carry sub-second precision to prove anything") + + for name, truncation := range map[string]time.Duration{ + "postgres (microseconds)": time.Microsecond, + "mysql (milliseconds)": time.Millisecond, + "mysql datetime (seconds)": time.Second, + } { + t.Run(name, func(t *testing.T) { + roundTripped := etagSettings() + roundTripped.CreatedAt = roundTripped.CreatedAt.Truncate(truncation) + + assert.Equal(t, inMemory.ETag(), roundTripped.ETag(), + "a validator derived before the write must still match one derived after reading the row back") + }) + } +} + // TestSettings_ETagOfDefaults covers the pre-bootstrap view, which GET serves // as a real representation and therefore validates like one. It must derive // without panicking on the zero CreatedAt, and it must not collide with a