carlos / platform Public

review: serialize the two config loops' SyncExec, pin touch-failure-never-fails-the-write

The hourly configSyncLoop and the 3s configGenLoop could interleave SyncExec
passes through syncexec's fixed <host>.config.env.tmp path, cross-renaming
stale content with the generation cache already advanced. One mutex in a
shared wrapper closes it. Also: the spec §1 property that a failed generation
touch never fails a landed manifest write gains the test it was missing, and
the PUT door's headline comment stops overstating its atomicity.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013WVANvB83eHrRANRpEL8mn

Paul Campbell 2 months ago 41e9b3ad0c7a714d6573177959eb75e7dc173cd6 parent 0cdd7bb

3 files changed, +70 −3

internal/appconfig/generation_test.go +40 −0
internal/console/apiconfig.go +5 −1
internal/host/agent.go +25 −2

diff --git a/internal/appconfig/generation_test.go b/internal/appconfig/generation_test.go

index db6dd83ad..5b1c6ed05 100644

--- a/internal/appconfig/generation_test.go

+++ b/internal/appconfig/generation_test.go

@@ -210,3 +210,43 @@ func testEnvelope(t *testing.T) string {

}

return envelope

}

+// genFailingStore fails Put only for one app's GENERATION key — the shape

+// TestTouchFailureNeverFailsTheWrite needs: the manifest write must succeed

+// while the follow-up touch cannot.

+type genFailingStore struct {

+ release.Store

+ genKey string

+ err error

+func (g *genFailingStore) Put(ctx context.Context, key string, body []byte, ifMatch string) (string, error) {

+ if key == g.genKey {

+ return "", g.err

+ }

+ return g.Store.Put(ctx, key, body, ifMatch)

+// TestTouchFailureNeverFailsTheWrite pins the spec §1 property touchLogged

+// exists for: when the manifest write LANDS and only the generation touch

+// fails, the caller sees success — returning an error would send an operator

+// re-running a write that is already live, and the real cost of the failed

+// touch is only that convergence falls back to the hourly pass.

+func TestTouchFailureNeverFailsTheWrite(t *testing.T) {

+ ctx := context.Background()

+ s := &genFailingStore{

+ Store: release.NewDirStore(t.TempDir()),

+ genKey: GenKey("bab", "keymail"),

+ err: errors.New("bucket is on fire"),

+ }

+ if err := SetEnv(ctx, s, "bab", "keymail", map[string]string{"PORT": "8080"}); err != nil {

+ t.Fatalf("SetEnv with a failing generation touch = %v, want nil (the manifest write landed)", err)

+ }

+ m, err := Get(ctx, s, "bab", "keymail")

+ if err != nil || m.Vars["PORT"].Value != "8080" {

+ t.Fatalf("manifest did not land: %v %v", m, err)

+ }

+ if _, _, err := s.Get(ctx, GenKey("bab", "keymail")); !errors.Is(err, os.ErrNotExist) {

+ t.Fatal("generation object exists despite the failing Put — the fixture proves nothing")

+ }

diff --git a/internal/console/apiconfig.go b/internal/console/apiconfig.go

index 4ea224e5a..ef762382e 100644

--- a/internal/console/apiconfig.go

+++ b/internal/console/apiconfig.go

@@ -257,7 +257,11 @@ func (s *Server) handleAPIConfigGet(w http.ResponseWriter, r *http.Request) {

// EVERY REFUSAL RUNS BEFORE THE FIRST STORE CALL. That is not tidiness: a

// request carrying one good var and one malformed one must leave the bundle

// exactly as it found it, or an operator retrying a corrected request would

-// be reasoning about a bundle that already half-moved. It also means a

+// be reasoning about a bundle that already half-moved. (The invariant is

+// about REFUSALS, not store failures: a request carrying both a set/unset

+// half and exec_delivery is two CAS writes — see the comment at the second

+// one — so a store failure between them can land the first half alone. The

+// CLI never sends both halves in one request.) It also means a

// garbage envelope is caught HERE, while the person who pasted it is still

// looking at their terminal — accepted now, it would stall the app's whole

// config at render time, hours later, on every host, out of sight

diff --git a/internal/host/agent.go b/internal/host/agent.go

index b2a30e6d8..bfef5ab14 100644

--- a/internal/host/agent.go

+++ b/internal/host/agent.go

@@ -64,6 +64,7 @@ package host

import (

"context"

+ "crypto/ecdh"

"crypto/ed25519"

"errors"

"flag"

@@ -78,6 +79,7 @@ import (

"path"

"path/filepath"

"strings"

+ "sync"

"time"

"github.com/carlosframework/platform/internal/activator"

@@ -254,12 +256,33 @@ func configSyncLoop(reg *registry.Registry, svc *activator.Service, dd string) {

for {

time.Sleep(delay)

delay = time.Hour + rand.N(10*time.Minute)

- if err := appconfig.SyncExec(ctx, reg, store, priv, dd, bounce(svc)); err != nil {

+ if err := syncExecSerialized(ctx, reg, store, priv, dd, svc); err != nil {

log.Printf("agent: config sync: %v", err)

}

+// configSyncMu serializes the two config loops' SyncExec passes against each

+// other. They can genuinely overlap — hourly pass mid-flight when the 3s

+// loop notices a fresh generation — and syncExecHost writes every route's

+// file through a FIXED temp path (<host>.config.env.tmp, syncexec.go), so

+// two interleaved passes could cross-rename each other's content: the loser

+// lands STALE bytes after the winner's fresh ones, with the generation cache

+// already advanced past the change, leaving nothing to re-fire until the

+// next hourly pass. Serializing in-process closes that outright; the root

+// adopt path is a different process writing a different namespace

+// (/etc/carlos/apps) and is not this mutex's problem.

+var configSyncMu sync.Mutex

+// syncExecSerialized is the one way either config loop runs SyncExec — the

+// mutex lives in a shared wrapper, not at the call sites, so a future third

+// caller cannot forget it.

+func syncExecSerialized(ctx context.Context, reg *registry.Registry, store release.Store, priv *ecdh.PrivateKey, dd string, svc *activator.Service) error {

+ configSyncMu.Lock()

+ defer configSyncMu.Unlock()

+ return appconfig.SyncExec(ctx, reg, store, priv, dd, bounce(svc))

// bounce builds the restart callback SyncExec calls for a route whose

// rendered config actually changed. Shared by BOTH config loops (hourly

// configSyncLoop and the fast configGenLoop) rather than duplicated per

@@ -435,7 +458,7 @@ func configGenLoop(reg *registry.Registry, svc *activator.Service, dd string) {

if !changed {

continue

}

- if err := appconfig.SyncExec(ctx, reg, store, priv, dd, bounce(svc)); err != nil {

+ if err := syncExecSerialized(ctx, reg, store, priv, dd, svc); err != nil {

log.Printf("agent: config generation sync: %v", err)

}