amadan
activator: the two-phase ensure's unasked-mint deferral is pinned by a test, and the slow-read test waits for host A to be inside its read before host B starts.
Re-review 2 found the deferral for a mint that looks next only under the lock (a key seen by the first read and gone by the locked one, or a keyless file appearing after the backfill's first read) survived having its condition disabled: no test reached it. instanceKeyHooks gains betweenPhases, nil in production, run after the first read and before adminTokenMu, and TestAMintThatLooksNextOnlyUnderTheLockDefers drives both cases: deferred, the bucket unread, the file untouched. The slow-read test replaced its 50 ms sleep with a signal from host A's reader, so a loaded box cannot let host B finish first and pass without proving anything.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+70
−8
internal/activator/instancekey.go+14 −7internal/activator/instancekey_delivery_test.go+56 −1
diff --git a/internal/activator/instancekey.go b/internal/activator/instancekey.go| index 9573777eb..b683e65c3 100644 |
| --- a/internal/activator/instancekey.go |
| +++ b/internal/activator/instancekey.go |
| @@ -145,18 +145,22 @@ func syncDir(dir string) error { |
| // adminTokenMu. |
| var instanceKeyWrite = func(f *os.File, b []byte) (int, error) { return f.Write(b) } |
| -// instanceKeyHooks are the two test seams on the launch path's hot path, |
| -// both nil in production; the sync and write seams above are what the |
| +// instanceKeyHooks are the three test seams on the launch path's hot path, |
| +// all nil in production; the sync and write seams above are what the |
| // durability tests need and stay on their own. beforeOpen runs under |
| // adminTokenMu after the read and the decision and before the ensure |
| // opens the file to append or settle, so a test can change the file |
| // between the two and pin the open's own symlink refusal, not only the |
| -// read's. replicaGate runs at the start of every replica gate, after the |
| -// env was read, so a test can change the env files between the read and |
| -// the gate, as a concurrent config sync can. |
| +// read's. betweenPhases runs after the ensure's first read (and its |
| +// bucket read, if any) and before it takes adminTokenMu, so a test can |
| +// change the files between the two phases as a concurrent writer can. |
| +// replicaGate runs at the start of every replica gate, after the env was |
| +// read, so a test can change the env files between the read and the |
| +// gate, as a concurrent config sync can. |
| var instanceKeyHooks struct { |
| - beforeOpen func(path string) |
| - replicaGate func() |
| + beforeOpen func(path string) |
| + betweenPhases func(path string) |
| + replicaGate func() |
| } |
| func mintInstanceKey() (string, error) { |
| @@ -447,6 +451,9 @@ func ensureInstanceKey(ctx context.Context, dir, name string, beneath, create bo |
| } |
| } |
| } |
| + if instanceKeyHooks.betweenPhases != nil { |
| + instanceKeyHooks.betweenPhases(path) |
| + } |
| // Phase two, under the lock: the decision, from this read only. |
| adminTokenMu.Lock() |
| defer adminTokenMu.Unlock() |
diff --git a/internal/activator/instancekey_delivery_test.go b/internal/activator/instancekey_delivery_test.go| index 5b5b0f97b..35ffc7d9d 100644 |
| --- a/internal/activator/instancekey_delivery_test.go |
| +++ b/internal/activator/instancekey_delivery_test.go |
| @@ -192,8 +192,10 @@ func TestFirstStartWaitsForTheConfiguredKey(t *testing.T) { |
| func TestASlowBucketReadForOneHostDoesNotBlockAnothersMint(t *testing.T) { |
| b := &ExecBackend{DataDir: t.TempDir()} |
| release := make(chan struct{}) |
| + entered := make(chan struct{}) |
| b.ConfiguredKey = func(ctx context.Context, rt registry.Route) (string, error) { |
| if rt.Host == "a.example.com" { |
| + close(entered) |
| select { |
| case <-release: |
| case <-ctx.Done(): |
| @@ -214,9 +216,13 @@ func TestASlowBucketReadForOneHostDoesNotBlockAnothersMint(t *testing.T) { |
| _, err := b.ensureInstanceKeyFor(context.Background(), a, b.pathsFor(a)) |
| aDone <- err |
| }() |
| + select { |
| + case <-entered: // A is inside its bucket read before B starts |
| + case <-time.After(2 * time.Second): |
| + t.Fatal("host A never reached its bucket read") |
| + } |
| bDone := make(chan error, 1) |
| go func() { |
| - time.Sleep(50 * time.Millisecond) // let A reach its read first |
| _, err := b.ensureInstanceKeyFor(context.Background(), bb, b.pathsFor(bb)) |
| bDone <- err |
| }() |
| @@ -304,3 +310,52 @@ func TestAKeyThatAppearsDuringTheBucketReadIsKept(t *testing.T) { |
| }) |
| } |
| } |
| + |
| +// The other side of the two phases: a mint that looks next only under the |
| +// lock has no bucket verdict, and defers rather than mint on nothing. The |
| +// first read saw a key in the own file (or, on the backfill, no file at |
| +// all), so the bucket was not asked; by the locked read the key is gone |
| +// (or a keyless file has appeared). Nothing is minted, the bucket is |
| +// still unasked, and the next attempt asks it. |
| +func TestAMintThatLooksNextOnlyUnderTheLockDefers(t *testing.T) { |
| + key := strings.Repeat("K", 43) + "=" |
| + for _, tc := range []struct { |
| + name string |
| + before string // the own file at the first read; "" is no file |
| + after string // the own file at the locked read |
| + create bool |
| + }{ |
| + {"a key seen by the first read is gone", "X=1\n" + InstanceKeyVar + "=" + key + "\n", "X=1\n", true}, |
| + {"a keyless file appears after the backfill's first read", "", "X=1\n", false}, |
| + } { |
| + t.Run(tc.name, func(t *testing.T) { |
| + b := &ExecBackend{DataDir: t.TempDir()} |
| + bucket := &bucketKey{} |
| + b.ConfiguredKey = bucket.read |
| + rt := registry.Route{Host: "shop.example.com", Kind: "instance"} |
| + own := filepath.Join(b.DataDir, rt.Host+".env") |
| + if tc.before != "" { |
| + if err := os.WriteFile(own, []byte(tc.before), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + instanceKeyHooks.betweenPhases = func(string) { |
| + if err := os.WriteFile(own, []byte(tc.after), 0o600); err != nil { |
| + t.Error(err) |
| + } |
| + } |
| + t.Cleanup(func() { instanceKeyHooks.betweenPhases = nil }) |
| + got, err := b.ensureInstanceKeyAt(context.Background(), rt, b.pathsFor(rt), tc.create, "minted") |
| + if got != InstanceKeyFailed || !errors.Is(err, ErrInstanceKeyDeferred) { |
| + t.Fatalf("ensure = %v, %v; want deferred", got, err) |
| + } |
| + if bucket.reads != 0 { |
| + t.Fatalf("the bucket was read %d times, want 0", bucket.reads) |
| + } |
| + raw, _ := os.ReadFile(own) |
| + if string(raw) != tc.after { |
| + t.Fatalf("own file changed by the deferred ensure (%d bytes, want %d)", len(raw), len(tc.after)) |
| + } |
| + }) |
| + } |
| +} |