amadan
activator: the mint's bucket read runs outside adminTokenMu, and the decision is the locked read's alone (final re-review)
ConfiguredKeyDigest ran under adminTokenMu, so a bucket that was slow or down for one row's first wake held the lock every other row's mint waits on, up to ten seconds an attempt. ensureInstanceKey now reads in two phases: outside the lock it reads both files and, if a mint looks next, asks the bucket and keeps its verdict; under the lock it reads the files again and decides from that read only. A key that appeared meanwhile is present or configured and nothing is minted; a mint goes ahead only when the locked read still finds no key and the bucket was asked and said none; a mint that looks next without a verdict defers. Create-only, fail-closed and defer-on-unknown hold. Tests: a blocked read for one host does not block another's mint; a key that appears between the phases is kept. The plan's delivery rule and backfill-cost gap say where the read now runs.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+208
−37
docs/superpowers/plans/2026-10-10-instance-key-provision.md+2 −2internal/activator/instancekey.go+86 −35internal/activator/instancekey_delivery_test.go+120 −0
diff --git a/docs/superpowers/plans/2026-10-10-instance-key-provision.md b/docs/superpowers/plans/2026-10-10-instance-key-provision.md| index e8cf5bdd9..6251cd5c6 100644 |
| --- a/docs/superpowers/plans/2026-10-10-instance-key-provision.md |
| +++ b/docs/superpowers/plans/2026-10-10-instance-key-provision.md |
| @@ -197,7 +197,7 @@ Each ruling ends with its cost if wrong. Rulings 1 to 12 are rev 1's, amended wh |
| **5. A key from the app's configuration wins, and the platform mints none (amended for finding 4; the delivery rule added by the final fix wave).** If the effective env (ruling 13) takes its key from `<host>.config.env`, the mint writes nothing and logs once an hour. Minting would put a key into `<host>.env`, which overrides the configured one, and change the app's key under its data. *Cost if wrong:* an app that sets the name and later unsets it gets a fresh platform key at its next start, by its own action. Reserving the name instead was open question 2; the provisional default is not to. |
| -*The delivery rule (final review I2, Astra's P2).* The files say what this box has delivered, not what the configuration sets: the fast-converge pass reconciles a row ready and syncs its configuration last, so a first wake can run before `<host>.config.env` is rendered, and a key minted then shadows the configured one for good once it arrives. `SyncExec` removes the file for a route that was never configured, so the file's existence cannot be the delivery signal. The mint therefore asks the bucket, as the replica gate does (`ExecBackend.ConfiguredKey`, `appconfig.ConfiguredKeyDigest`), once, under `adminTokenMu`, only when nothing delivered here sets a key and a mint is next: a key the configuration sets and this box has not delivered defers the launch (`ErrInstanceKeyDeferred`), so does a bucket that cannot be read, and a configuration that sets no key lets the mint go ahead. The launch fails and is retried; the config sync's next pass delivers the file and the next start finds the key configured. A key already delivered, in either file, costs no read. Which boxes have a configuration to read follows the config guard (`configguard.go`): no deployment store under `CARLOS_NO_SEALED_CONFIG` is no configuration source, and the mint proceeds; no store otherwise is an error. *Cost:* a delayed first start while the configuration is in flight (seconds, one pass), and one bucket read per instance, ever. The window the read cannot close is a key configured after the read and before the append, the same in-flight window ruling 27 names for replicas; a key configured after an instance has minted is a rotation, which this plan does not support. |
| +*The delivery rule (final review I2, Astra's P2).* The files say what this box has delivered, not what the configuration sets: the fast-converge pass reconciles a row ready and syncs its configuration last, so a first wake can run before `<host>.config.env` is rendered, and a key minted then shadows the configured one for good once it arrives. `SyncExec` removes the file for a route that was never configured, so the file's existence cannot be the delivery signal. The mint therefore asks the bucket, as the replica gate does (`ExecBackend.ConfiguredKey`, `appconfig.ConfiguredKeyDigest`), once, only when nothing delivered here sets a key and a mint is next: a key the configuration sets and this box has not delivered defers the launch (`ErrInstanceKeyDeferred`), so does a bucket that cannot be read, and a configuration that sets no key lets the mint go ahead. The read is two-phase (the re-review's finding): outside `adminTokenMu`, the files are read and, if a mint looks next, the bucket is asked; under the lock the files are read again and the decision is that read's alone. A key that appeared in either file meanwhile is present or configured and nothing is minted; a mint goes ahead only when the locked read still finds no key and the bucket was asked and said none; a mint that looks next under the lock without a verdict defers, and the next attempt asks. So a bucket that is slow or down for one row never holds the lock every other row's mint waits on. The launch fails and is retried; the config sync's next pass delivers the file and the next start finds the key configured. A key already delivered, in either file, costs no read. Which boxes have a configuration to read follows the config guard (`configguard.go`): no deployment store under `CARLOS_NO_SEALED_CONFIG` is no configuration source, and the mint proceeds; no store otherwise is an error. *Cost:* a delayed first start while the configuration is in flight (seconds, one pass), and one bucket read per instance, ever. The window the read cannot close is a key configured after the read and before the append, the same in-flight window ruling 27 names for replicas; a key configured after an instance has minted is a rotation, which this plan does not support. |
| **6. Durability before the first start.** |
| - *Legacy boxes:* nothing can publish today, and region HA cannot be on, because it requires host claims. The key's durability is the box's disk, now fsynced, and a move's sealed archive. A dead legacy box loses its keys with its data; that is today's state, unchanged (row 10). |
| @@ -5257,7 +5257,7 @@ Question 5 (restarts inside a claim session) is withdrawn: its premise was wrong |
| - **The non-linux `safefile.Write` fsyncs the file, not the directory** (Task 5's review). Boxes are Linux; dev only. |
| - **The delivery rule's window (final fix wave).** The mint reads the bucket and then appends; a key configured between the two is shadowed, as it would be by a mint one second before the operator's change. The read narrows the gap from "until the next config sync" to the length of one mint; nothing short of reserving the name closes it, and a configured key on an app that has minted is a rotation either way. |
| - **A box with a deployment store and no `CARLOS_SECRETS_KEY` never mints for an app whose configuration sets a key** (final fix wave). Its config sync loop is off, so the file is never delivered, and the mint defers at every start with a line naming the deferral. The box is misconfigured, as the config guard already says of it for sealed values; the fix is the key. |
| -- **The backfill's cost at agent start (final fix wave).** Each keyless row with an own file costs one bucket read before its key is minted, serial under `adminTokenMu`, before the activator starts. The pass has a one-minute budget, so a box whose bucket is unreachable at start is held at most a minute and the rows past the budget mint at their next start. With the bucket up it is a few hundred milliseconds a row, once. |
| +- **The backfill's cost at agent start (final fix wave).** Each keyless row with an own file costs one bucket read before its key is minted, serial in the backfill's loop (outside `adminTokenMu`, so a launch's mint on another row is not held), before the activator starts. The pass has a one-minute budget, so a box whose bucket is unreachable at start is held at most a minute and the rows past the budget mint at their next start. With the bucket up it is a few hundred milliseconds a row, once. |
| - **The second read asymmetry on the flat path (final review minor 6, #273).** `instanceEnvParts` reads a flat own file through `readEnvFile`, a plain `os.ReadFile`, right after the mint refused a link or a FIFO at it through `openOwnEnvNoFollow`; a link planted between the two is followed by the read. The pre-branch shape, as the token's. Listed on #273 so the token fix closes both with one shared open; the first asymmetry (the unbounded flat configuration read) is closed. |
| ## Follow-ups this plan does not build |
diff --git a/internal/activator/instancekey.go b/internal/activator/instancekey.go| index 8c1126e21..9573777eb 100644 |
| --- a/internal/activator/instancekey.go |
| +++ b/internal/activator/instancekey.go |
| @@ -366,37 +366,96 @@ func syncInstanceEnv(f *os.File, dir, path string) error { |
| return nil |
| } |
| -// ensureInstanceKey is the shared body. It holds adminTokenMu, the lock |
| -// every mint into <host>.env takes, so a key append and a token append can |
| -// never interleave. create false is the backfill: an absent file is |
| -// InstanceKeyNoEnvFile and nothing is created. configured is asked, under |
| -// the lock, only when nothing delivered here sets a key and a mint is |
| -// next: the one bucket read a row ever costs. |
| -func ensureInstanceKey(ctx context.Context, dir, name string, beneath, create bool, configured configuredKeyReader) (InstanceKeyOutcome, error) { |
| - path := filepath.Join(dir, name) |
| - adminTokenMu.Lock() |
| - defer adminTokenMu.Unlock() |
| - // Both files are read the same guarded way: through instancefs on the |
| - // nested layout, and on the flat one through readOwnEnv, which refuses |
| - // a link or a FIFO at either name and caps the read. |
| +// readInstanceEnvPair reads an own env file and its rendered configuration |
| +// the same guarded way on both layouts: through instancefs on the nested |
| +// one, and on the flat one through readOwnEnv, which refuses a link or a |
| +// FIFO at either name and caps the read. exists is false when the own file |
| +// is absent (raw nil); an absent configuration is nil cfg and no error. |
| +func readInstanceEnvPair(dir, name string, beneath bool) (raw, cfg []byte, exists bool, err error) { |
| read := func(n string) ([]byte, error) { |
| if beneath { |
| return instancefs.ReadBeneathLimit(dir, n, ownEnvReadLimit) |
| } |
| return readOwnEnv(filepath.Join(dir, n)) |
| } |
| - raw, err := read(name) |
| + raw, err = read(name) |
| switch { |
| case errors.Is(err, os.ErrNotExist): |
| - if !create { |
| - return InstanceKeyNoEnvFile, nil |
| - } |
| + raw = nil |
| case err != nil: |
| - return InstanceKeyFailed, fmt.Errorf("read %s: %w", path, err) |
| + return nil, nil, false, fmt.Errorf("read %s: %w", filepath.Join(dir, name), err) |
| + default: |
| + exists = true |
| } |
| - cfg, err := read(configEnvName(name)) |
| + cfg, err = read(configEnvName(name)) |
| if err != nil && !errors.Is(err, os.ErrNotExist) { |
| - return InstanceKeyFailed, fmt.Errorf("read %s: %w", filepath.Join(dir, configEnvName(name)), err) |
| + return nil, nil, false, fmt.Errorf("read %s: %w", filepath.Join(dir, configEnvName(name)), err) |
| + } |
| + return raw, cfg, exists, nil |
| +} |
| + |
| +// bucketVerdict is what the bucket said before the lock was taken: asked |
| +// is whether a mint looked next on that read and the bucket was consulted; |
| +// deferral is its answer when the mint must wait (a configured key not yet |
| +// delivered, or a read that failed), nil when it may go ahead. |
| +type bucketVerdict struct { |
| + asked bool |
| + deferral error |
| +} |
| + |
| +// ensureInstanceKey is the shared body. It holds adminTokenMu, the lock |
| +// every mint into <host>.env takes, so a key append and a token append can |
| +// never interleave. create false is the backfill: an absent file is |
| +// InstanceKeyNoEnvFile and nothing is created. |
| +// |
| +// The bucket is read in two phases, so a slow or unreachable bucket for |
| +// one row never holds the lock every other row's mint waits on. Outside |
| +// the lock: read the files, and if a mint looks next, ask the bucket |
| +// (configured; the one read a row ever costs). Under the lock: read the |
| +// files again and decide from that read alone. A key that appeared in |
| +// either file since is present or configured, and nothing is minted; a |
| +// mint goes ahead only when the locked read still finds no key and the |
| +// bucket was asked and said none. A mint that looks next under the lock |
| +// without a verdict (the first read saw a key that has since gone) defers |
| +// rather than mint on nothing, and the next attempt asks. Create-only, |
| +// fail-closed and defer-on-unknown hold on every path. |
| +func ensureInstanceKey(ctx context.Context, dir, name string, beneath, create bool, configured configuredKeyReader) (InstanceKeyOutcome, error) { |
| + path := filepath.Join(dir, name) |
| + // Phase one, outside the lock. Nothing delivered here sets a key. The |
| + // configuration may still: a first wake can run before the config |
| + // sync's first render of <host>.config.env (the fast-converge pass |
| + // reconciles the row ready and syncs config last), and a key minted |
| + // in that gap would shadow the configured one for good once it |
| + // arrived (Effective: the own file wins). So ask the bucket whether |
| + // the configuration sets a key now. One it sets is not here yet: |
| + // defer, and the next start finds it delivered. A bucket that cannot |
| + // be read cannot say, and defers the same way. With no reader there |
| + // is no configuration source, and nothing can be on its way. A read |
| + // error here is left for the locked read to report. |
| + var pre bucketVerdict |
| + if configured != nil { |
| + if raw, cfg, exists, err := readInstanceEnvPair(dir, name, beneath); err == nil && (exists || create) { |
| + if _, layer := instancekey.Effective(parseEnvBytes(cfg), parseEnvBytes(raw)); layer == instancekey.LayerNone { |
| + pre.asked = true |
| + digest, err := configured(ctx) |
| + switch { |
| + case err != nil: |
| + pre.deferral = fmt.Errorf("%w: its configuration could not be read: %w", ErrInstanceKeyDeferred, err) |
| + case digest != "": |
| + pre.deferral = fmt.Errorf("%w: its configuration sets %s and this box has not delivered it yet (the next config sync does, and the next start finds it)", ErrInstanceKeyDeferred, InstanceKeyVar) |
| + } |
| + } |
| + } |
| + } |
| + // Phase two, under the lock: the decision, from this read only. |
| + adminTokenMu.Lock() |
| + defer adminTokenMu.Unlock() |
| + raw, cfg, exists, err := readInstanceEnvPair(dir, name, beneath) |
| + if err != nil { |
| + return InstanceKeyFailed, err |
| + } |
| + if !exists && !create { |
| + return InstanceKeyNoEnvFile, nil |
| } |
| // The decision is the effective env's: the key the launch will hand |
| // the app, by the same rule mergeEnv applies. |
| @@ -425,23 +484,15 @@ func ensureInstanceKey(ctx context.Context, dir, name string, beneath, create bo |
| } |
| return InstanceKeyPresent, nil |
| } |
| - // Nothing delivered here sets a key. The configuration may still: a |
| - // first wake can run before the config sync's first render of |
| - // <host>.config.env (the fast-converge pass reconciles the row ready |
| - // and syncs config last), and a key minted in that gap would shadow |
| - // the configured one for good once it arrived (Effective: the own file |
| - // wins). So ask the bucket now, and mint only when the configuration |
| - // sets no key. A key it sets is not here yet: defer, and the next |
| - // start finds it delivered. A bucket that cannot be read cannot say, |
| - // and defers the same way. With no reader there is no configuration |
| - // source, and nothing can be on its way. |
| + // A mint is next. It goes ahead only on the bucket's word from phase |
| + // one; without it (the files changed between the two reads) nothing |
| + // is known, and the mint defers rather than go on nothing. |
| if configured != nil { |
| - digest, err := configured(ctx) |
| switch { |
| - case err != nil: |
| - return InstanceKeyFailed, fmt.Errorf("%w: its configuration could not be read: %w", ErrInstanceKeyDeferred, err) |
| - case digest != "": |
| - return InstanceKeyFailed, fmt.Errorf("%w: its configuration sets %s and this box has not delivered it yet (the next config sync does, and the next start finds it)", ErrInstanceKeyDeferred, InstanceKeyVar) |
| + case !pre.asked: |
| + return InstanceKeyFailed, fmt.Errorf("%w: its env files changed while the configuration was being read (retried at the next start)", ErrInstanceKeyDeferred) |
| + case pre.deferral != nil: |
| + return InstanceKeyFailed, pre.deferral |
| } |
| } |
| minted, err := mintInstanceKey() |
diff --git a/internal/activator/instancekey_delivery_test.go b/internal/activator/instancekey_delivery_test.go| index a928e5b5d..5b5b0f97b 100644 |
| --- a/internal/activator/instancekey_delivery_test.go |
| +++ b/internal/activator/instancekey_delivery_test.go |
| @@ -184,3 +184,123 @@ func TestFirstStartWaitsForTheConfiguredKey(t *testing.T) { |
| t.Fatal("a key was minted beside the configured one") |
| } |
| } |
| + |
| +// The re-review's finding: the bucket read runs outside adminTokenMu, so a |
| +// bucket that is slow or blocked for one row's first wake never holds the |
| +// lock every other row's mint waits on. Host A's read blocks until the |
| +// test releases it; host B mints meanwhile; A then mints too. |
| +func TestASlowBucketReadForOneHostDoesNotBlockAnothersMint(t *testing.T) { |
| + b := &ExecBackend{DataDir: t.TempDir()} |
| + release := make(chan struct{}) |
| + b.ConfiguredKey = func(ctx context.Context, rt registry.Route) (string, error) { |
| + if rt.Host == "a.example.com" { |
| + select { |
| + case <-release: |
| + case <-ctx.Done(): |
| + return "", ctx.Err() |
| + } |
| + } |
| + return "", nil |
| + } |
| + a := registry.Route{Host: "a.example.com", Kind: "instance"} |
| + bb := registry.Route{Host: "b.example.com", Kind: "instance"} |
| + for _, rt := range []registry.Route{a, bb} { |
| + if err := os.WriteFile(filepath.Join(b.DataDir, rt.Host+".env"), []byte("X=1\n"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + aDone := make(chan error, 1) |
| + go func() { |
| + _, err := b.ensureInstanceKeyFor(context.Background(), a, b.pathsFor(a)) |
| + aDone <- err |
| + }() |
| + 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 |
| + }() |
| + select { |
| + case err := <-bDone: |
| + if err != nil { |
| + t.Fatalf("host B's mint: %v", err) |
| + } |
| + case <-time.After(2 * time.Second): |
| + t.Fatal("host B's mint waited on host A's bucket read") |
| + } |
| + select { |
| + case <-aDone: |
| + t.Fatal("host A minted before its bucket answered") |
| + default: |
| + } |
| + close(release) |
| + select { |
| + case err := <-aDone: |
| + if err != nil { |
| + t.Fatalf("host A's mint after the bucket answered: %v", err) |
| + } |
| + case <-time.After(2 * time.Second): |
| + t.Fatal("host A never minted") |
| + } |
| + for _, rt := range []registry.Route{a, bb} { |
| + raw, _ := os.ReadFile(filepath.Join(b.DataDir, rt.Host+".env")) |
| + if len(keyLines(raw)) != 1 { |
| + t.Fatalf("%s: %d key lines, want one", rt.Host, len(keyLines(raw))) |
| + } |
| + } |
| +} |
| + |
| +// A key that appears between the two phases is never overwritten: the |
| +// bucket read happens outside the lock, so the files it looked at may |
| +// change before the decision. The locked read decides. Here the bucket |
| +// reader itself, standing in for a concurrent writer, puts a key into the |
| +// own file, or delivers the configuration with one, before it answers. |
| +func TestAKeyThatAppearsDuringTheBucketReadIsKept(t *testing.T) { |
| + key := strings.Repeat("K", 43) + "=" |
| + for _, tc := range []struct { |
| + name string |
| + plant func(dataDir, host string) |
| + want InstanceKeyOutcome |
| + }{ |
| + {"into the own file", func(d, h string) { |
| + f, err := os.OpenFile(filepath.Join(d, h+".env"), os.O_APPEND|os.O_WRONLY, 0) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + f.WriteString(InstanceKeyVar + "=" + key + "\n") |
| + f.Close() |
| + }, InstanceKeyPresent}, |
| + {"delivered in the configuration", func(d, h string) { |
| + if err := os.WriteFile(filepath.Join(d, h+".config.env"), []byte(InstanceKeyVar+"="+key+"\n"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + }, InstanceKeyFromConfig}, |
| + } { |
| + t.Run(tc.name, func(t *testing.T) { |
| + b := &ExecBackend{DataDir: t.TempDir()} |
| + rt := registry.Route{Host: "shop.example.com", Kind: "instance"} |
| + own := filepath.Join(b.DataDir, rt.Host+".env") |
| + if err := os.WriteFile(own, []byte("X=1\n"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + reads := 0 |
| + b.ConfiguredKey = func(context.Context, registry.Route) (string, error) { |
| + reads++ |
| + tc.plant(b.DataDir, rt.Host) |
| + return "", nil // the bucket says: no configured key, mint away |
| + } |
| + got, err := b.ensureInstanceKeyFor(context.Background(), rt, b.pathsFor(rt)) |
| + if err != nil || got != tc.want || reads != 1 { |
| + t.Fatalf("ensure = %v, %v after %d reads; want %v", got, err, reads, tc.want) |
| + } |
| + raw, _ := os.ReadFile(own) |
| + ownKeys := keyLines(raw) |
| + if tc.want == InstanceKeyPresent && (len(ownKeys) != 1 || ownKeys[0] != key) { |
| + t.Fatalf("own file keys %d, want the one that appeared, kept", len(ownKeys)) |
| + } |
| + if tc.want == InstanceKeyFromConfig && len(ownKeys) != 0 { |
| + t.Fatal("a key was minted beside the one the configuration delivered") |
| + } |
| + }) |
| + } |
| +} |