From 29eff3b207eb9425ef899c169b8277c6f8d8544c Mon Sep 17 00:00:00 2001 From: Brad Ison Date: Tue, 11 Aug 2026 12:24:53 +0200 Subject: [PATCH] [management] Derive the settings ETag at a precision the store preserves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The validator a bootstrap returns was permanently unusable on PostgreSQL. POST derives it from the in-memory row, whose CreatedAt carries nanoseconds, while every later comparison derives it from a row read back out of the store — and PostgreSQL truncates timestamps to microseconds, MySQL DATETIME to whole seconds without an fsp. The two never agreed again, so the documented "conditional PUT without an intervening GET" answered 412 forever. Hashing CreatedAt at whole-second precision fixes it at the root: seconds is the floor every supported engine round-trips, so the validator no longer depends on which store is behind it. The cost is that a delete and re-bootstrap inside the same second, onto the same endpoint with the same toggles, derives the same validator — which needs a self-addressed endpoint reclaimed within one second, since a labeled bootstrap draws a fresh label. The whole suite ran green against this bug, because every test uses the sqlite test store and sqlite preserves nanoseconds. The regression guard is therefore on the type rather than through a store: it asserts the validator is unchanged by truncation at each engine's precision, so it holds without running the suite against all three. Also from review: the concurrency test recorded the winning writer's value in a slice left zero for the loser, and zero is a retention the API documents as "keep indefinitely" — so the assertion could be satisfied without matching the writer that won. It now uses a sentinel and asserts equality. The stale-delete e2e assertion accepted any error, which a server error or a state-guard refusal would have satisfied; it now requires a precondition failure. And the 412 message both conditional writes return is a single constant, since on delete the message is the only thing separating staleness from the state guards. --- e2e/agentnetwork/settings_bootstrap_test.go | 5 ++- .../internals/modules/agentnetwork/manager.go | 11 +++++- .../agentnetwork/settings_etag_test.go | 24 ++++++++---- .../modules/agentnetwork/types/settings.go | 15 +++++++- .../agentnetwork/types/settings_test.go | 37 ++++++++++++++++++- 5 files changed, 78 insertions(+), 14 deletions(-) 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