From 66d2b5bdeec52d27d223e0d815f5a930fe4f64bf Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 13:45:59 -0400 Subject: [PATCH 1/6] fix(plugin): CLITargetsOf returns no binary for a manifest without a commands block (D-0669) --- internal/plugin/manifestv2/cli_targets.go | 8 +++++++ .../plugin/manifestv2/cli_targets_test.go | 23 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/internal/plugin/manifestv2/cli_targets.go b/internal/plugin/manifestv2/cli_targets.go index 1c0762ae1..bbb8aac9c 100644 --- a/internal/plugin/manifestv2/cli_targets.go +++ b/internal/plugin/manifestv2/cli_targets.go @@ -90,7 +90,15 @@ func CheckCLITargets(plugin string, targets []CLITarget) error { // CLITargetsOf is CLITargets over a parsed manifest through its compatibility // projection, which is what a released CLI reads from a v2 plugin.json. +// +// A manifest without a commands block publishes no binary: the projection +// carries cliCommands but cannot carry a v1 non-cli pluginType, so reading +// cliCommands alone would build nself- for a service plugin that the +// installer's v1 path gives none (D-0669). func CLITargetsOf(m *Manifest) []CLITarget { + if m.Commands == nil { + return nil + } p := Projection(m) names := make([]string, 0, len(p.CLICommands)) for _, c := range p.CLICommands { diff --git a/internal/plugin/manifestv2/cli_targets_test.go b/internal/plugin/manifestv2/cli_targets_test.go index ccf72ec4f..655c519b0 100644 --- a/internal/plugin/manifestv2/cli_targets_test.go +++ b/internal/plugin/manifestv2/cli_targets_test.go @@ -25,3 +25,26 @@ func TestCLITargets(t *testing.T) { } } } + +// TestCLITargetsOfNonCLIType proves a service plugin that lists cliCommands +// yields no target (D-0669): the v2 conversion drops pluginType, so only the +// commands block says the plugin publishes a binary, as the installer's v1 path +// reads pluginType service. +func TestCLITargetsOfNonCLIType(t *testing.T) { + const svc = `{"name":"retro-gaming","version":"1.0.0","description":"d","category":"gaming","pluginType":"service","cliCommands":[{"name":"server","description":"d"}]}` + m, err := ParseQuiet([]byte(svc)) + if err != nil { + t.Fatal(err) + } + if got := CLITargetsOf(m); got != nil { + t.Errorf("service plugin with cliCommands: got %v want none", got) + } + const cli = `{"name":"tenant","version":"1.0.0","description":"d","category":"infrastructure","pluginType":"cli","binaryName":"nself-tenant","cliCommands":[{"name":"tenant","description":"d"},{"name":"billing","description":"d"}]}` + m, err = ParseQuiet([]byte(cli)) + if err != nil { + t.Fatal(err) + } + if got := CLITargetsOf(m); len(got) != 2 { + t.Errorf("cli plugin: got %v want 2 targets", got) + } +} From 7a1cc802bf4927fc7f3370170d9e0ba1b3241a17 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 13:47:37 -0400 Subject: [PATCH 2/6] fix(plugin): read and write the catalog cache body and ETag under one lock (D-0875) --- internal/plugin/catalog/cache.go | 15 +++++- internal/plugin/catalog/cache_lock.go | 43 ++++++++++++++++ internal/plugin/catalog/cache_lock_other.go | 12 +++++ internal/plugin/catalog/cache_lock_unix.go | 38 ++++++++++++++ .../plugin/catalog/cache_lock_unix_test.go | 50 +++++++++++++++++++ internal/plugin/catalog/client_test.go | 40 +++++++++++++++ 6 files changed, 197 insertions(+), 1 deletion(-) create mode 100644 internal/plugin/catalog/cache_lock.go create mode 100644 internal/plugin/catalog/cache_lock_other.go create mode 100644 internal/plugin/catalog/cache_lock_unix.go create mode 100644 internal/plugin/catalog/cache_lock_unix_test.go diff --git a/internal/plugin/catalog/cache.go b/internal/plugin/catalog/cache.go index 6d708ad93..a3f14c000 100644 --- a/internal/plugin/catalog/cache.go +++ b/internal/plugin/catalog/cache.go @@ -8,7 +8,9 @@ package catalog // catalog.etag beside it, both mode 0600. // Constraints: the ETag file is removed before the body is replaced and // written after it, so a crash between the two leaves a body with no ETag -// (an unconditional refetch), never a new body paired with an old ETag. +// (an unconditional refetch), never a new body paired with an old ETag. Both +// files are read and written under one lock (cache_lock.go), so concurrent +// processes cannot pair one response's body with another's ETag (D-0875). // Writes are best effort: a read-only cache dir never fails a command. import ( @@ -55,6 +57,10 @@ func readCache(dir string) (*cached, *Catalog, bool) { if dir == "" { return nil, nil, false } + // Best effort: with no lock (dir missing, read-only) the read goes ahead. + if unlock, err := lockCache(dir); err == nil { + defer unlock() + } path := filepath.Join(dir, bodyFile) info, err := os.Stat(path) if err != nil || !info.Mode().IsRegular() || info.Size() > maxBody { @@ -77,6 +83,13 @@ func writeCache(dir string, body []byte, etag string) { if dir == "" || os.MkdirAll(dir, 0o755) != nil { return } + // No lock, no write: an unserialised write is what pairs a body with the + // wrong ETag, and the cache is optional. + unlock, err := lockCache(dir) + if err != nil { + return + } + defer unlock() if err := os.Remove(filepath.Join(dir, etagFile)); err != nil && !errors.Is(err, fs.ErrNotExist) { return } diff --git a/internal/plugin/catalog/cache_lock.go b/internal/plugin/catalog/cache_lock.go new file mode 100644 index 000000000..072af7048 --- /dev/null +++ b/internal/plugin/catalog/cache_lock.go @@ -0,0 +1,43 @@ +package catalog + +// cache_lock.go — serialise access to the on-disk catalog pair. +// +// Purpose: the body and the ETag are two files, so two writers (or a writer and +// a reader) could interleave and pair one response's body with another's ETag; +// a later 304 would then keep the wrong body (D-0875). Every read and write of +// the pair holds one lock. +// Inputs: the cache directory. Outputs: an unlock func. +// Constraints: a process-wide mutex covers goroutines of one process; an flock +// on catalog.lock beside the files covers other processes (cache_lock_unix.go; +// a no-op without flock, where the mutex still holds). The lock is never held +// across a network call. + +import ( + "os" + "path/filepath" + "sync" +) + +const lockFile = "catalog.lock" + +var cacheMu sync.Mutex + +// lockCache takes the cache lock for dir. The returned func releases it. An +// error means the cross-process lock could not be taken; nothing is held then. +func lockCache(dir string) (func(), error) { + cacheMu.Lock() + f, err := os.OpenFile(filepath.Join(dir, lockFile), os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + cacheMu.Unlock() + return nil, err + } + if err := flockFile(f); err != nil { + _ = f.Close() + cacheMu.Unlock() + return nil, err + } + return func() { + _ = f.Close() // closing drops the flock + cacheMu.Unlock() + }, nil +} diff --git a/internal/plugin/catalog/cache_lock_other.go b/internal/plugin/catalog/cache_lock_other.go new file mode 100644 index 000000000..41f6b00c9 --- /dev/null +++ b/internal/plugin/catalog/cache_lock_other.go @@ -0,0 +1,12 @@ +//go:build !(linux || darwin || freebsd || netbsd || openbsd || dragonfly) + +package catalog + +// Purpose: the catalog cache lock on platforms without flock (Windows). +// Constraints: a no-op; the process-wide mutex in lockCache still serialises +// every goroutine of one process. Windows runs the unix build under WSL2 (VMI). + +import "os" + +// flockFile is a no-op without flock. +func flockFile(*os.File) error { return nil } diff --git a/internal/plugin/catalog/cache_lock_unix.go b/internal/plugin/catalog/cache_lock_unix.go new file mode 100644 index 000000000..b4ef1b1be --- /dev/null +++ b/internal/plugin/catalog/cache_lock_unix.go @@ -0,0 +1,38 @@ +//go:build linux || darwin || freebsd || netbsd || openbsd || dragonfly + +package catalog + +// Purpose: the cross-process flock of the catalog cache (unix build). +// Constraints: syscall.Flock only. The kernel drops the lock when the last +// descriptor closes, process death included. The pattern is +// internal/invoke/audit_lock_unix.go. + +import ( + "errors" + "os" + "syscall" + "time" +) + +// lockWait bounds how long a cache user waits for another process's lock: a +// holder stuck forever must not hang a plugin command. +var lockWait = 5 * time.Second + +// flockFile takes an exclusive flock on f, waiting at most lockWait. +func flockFile(f *os.File) error { + deadline := time.Now().Add(lockWait) + for { + err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX|syscall.LOCK_NB) + switch { + case err == nil: + return nil + case errors.Is(err, syscall.EINTR): + continue + case !errors.Is(err, syscall.EWOULDBLOCK): + return err + case time.Now().After(deadline): + return errors.New("the catalog cache stayed locked by another process") + } + time.Sleep(10 * time.Millisecond) + } +} diff --git a/internal/plugin/catalog/cache_lock_unix_test.go b/internal/plugin/catalog/cache_lock_unix_test.go new file mode 100644 index 000000000..98931acb7 --- /dev/null +++ b/internal/plugin/catalog/cache_lock_unix_test.go @@ -0,0 +1,50 @@ +//go:build linux || darwin || freebsd || netbsd || openbsd || dragonfly + +package catalog + +import ( + "os" + "path/filepath" + "syscall" + "testing" + "time" +) + +// TestWriteCacheWaitsForOtherProcessLock holds the flock through a separate +// descriptor, as another nself process would, and proves a cache write waits +// for it instead of interleaving (D-0875). +func TestWriteCacheWaitsForOtherProcessLock(t *testing.T) { + dir := t.TempDir() + f, err := os.OpenFile(filepath.Join(dir, lockFile), os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + t.Fatal(err) + } + defer f.Close() //nolint:errcheck // test descriptor + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil { + t.Fatal(err) + } + done := make(chan struct{}) + go func() { + writeCache(dir, sampleCatalog(1), `"e1"`) + close(done) + }() + select { + case <-done: + t.Fatal("writeCache finished while another process held the cache lock") + case <-time.After(150 * time.Millisecond): + } + if _, err := os.Stat(filepath.Join(dir, bodyFile)); err == nil { + t.Fatal("body was written under another process's lock") + } + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_UN); err != nil { + t.Fatal(err) + } + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("writeCache did not resume after the lock was released") + } + if c, _, ok := readCache(dir); !ok || c.etag != `"e1"` { + t.Fatalf("cache after release: ok=%v etag=%q", ok, c.etag) + } +} diff --git a/internal/plugin/catalog/client_test.go b/internal/plugin/catalog/client_test.go index ab3804596..d07cb6bad 100644 --- a/internal/plugin/catalog/client_test.go +++ b/internal/plugin/catalog/client_test.go @@ -3,12 +3,14 @@ package catalog import ( "context" "errors" + "fmt" "net/http" "net/http/httptest" "os" "path/filepath" "strings" "sync" + "sync/atomic" "testing" "time" @@ -192,3 +194,41 @@ func TestCommandHintReadsCacheOnly(t *testing.T) { t.Fatal("unknown command offered as a hint") } } + +// TestWriteCacheConcurrentKeepsPair proves racing writers and readers never see +// one response's body with another's ETag (D-0875). Without the cache lock a +// later 304 would keep the wrong body. +func TestWriteCacheConcurrentKeepsPair(t *testing.T) { + dir := t.TempDir() + const writers, rounds = 8, 60 + pairOK := func(c *cached) bool { + if c.etag == "" { + return true // a body with no ETag is a safe crash window + } + for n := 0; n < writers; n++ { + if c.etag == fmt.Sprintf(`"e%d"`, n) { + return strings.Contains(string(c.body), fmt.Sprintf("Reverse proxy %d", n)) + } + } + return false + } + var wg sync.WaitGroup + var bad atomic.Int32 + for w := 0; w < writers; w++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < rounds; i++ { + writeCache(dir, sampleCatalog(w), fmt.Sprintf(`"e%d"`, w)) + if c, _, ok := readCache(dir); ok && !pairOK(c) { + bad.Add(1) + } + } + }() + } + wg.Wait() + c, _, ok := readCache(dir) + if !ok || !pairOK(c) || bad.Load() != 0 { + t.Fatalf("body and ETag diverged: final ok=%v pair=%v, %d bad reads", ok, ok && pairOK(c), bad.Load()) + } +} From 2ed41960c21f2b42cbdfde4e23101ffd3542bd31 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 13:48:48 -0400 Subject: [PATCH 3/6] ci(golden-path): size the step 10 baseline for claw's six licensed dependency downloads (D-0307) --- scripts/golden-path.sh | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/scripts/golden-path.sh b/scripts/golden-path.sh index c7fdebf62..92df35e0c 100755 --- a/scripts/golden-path.sh +++ b/scripts/golden-path.sh @@ -82,7 +82,16 @@ declare -A STEP_BASELINE=( [7]=15 # doctor --quick [8]=5 # license set (must precede the licensed plugin installs below) [9]=30 # plugin install ai - [10]=30 # plugin install claw + # claw installs six licensed dependencies (notify, google, mux, cron, voice, + # browser) and then itself: seven downloads from ping.nself.org. Per download + # the latency is bimodal: ~1s when ping is warm (step 10 took 4-6s on + # 2026-10-10 and 2026-10-11) and ~20s when it is slow (step 9, a single + # download, took 23-26s). Slow mode makes step 10 take 108s, 110s and 128s + # (runs 38202476017, 38087589999, 39192880837), so the old baseline of 30 + # (fail at 90s) failed on ping latency with every container healthy. 60 -> + # warn 90s, fail 180s: room for seven slow downloads, and the warn line is the + # signal that ping.nself.org is slow (D-0307). + [10]=60 # plugin install claw (+ 6 licensed dependencies) [11]=10 # admin start [12]=5 # curl health # Step 13 is not a bare probe: it runs `nself build` + `nself start`, which From 1809d8198e545334ca1ae744d0165ee55e4fc5b3 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 13:56:16 -0400 Subject: [PATCH 4/6] fix(release): bump fixture cases get a live TMPDIR so bump-version.sh mktemp works on GNU coreutils (D-0831) --- scripts/release/fixtures/cases4.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/release/fixtures/cases4.sh b/scripts/release/fixtures/cases4.sh index f258ae14d..27b57e66b 100644 --- a/scripts/release/fixtures/cases4.sh +++ b/scripts/release/fixtures/cases4.sh @@ -13,6 +13,8 @@ NEWH=bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb # mk_bump_env: a nself/{cli,admin} tree at 1.4.13 with the golden tests present and a stub go. mk_bump_env() { B=$(mktemp -d "$BASE/bump.XXXXXX"); C=$B/nself/cli; A=$B/nself/admin + # mk_env's TMPDIR was deleted with its env; bump-version.sh's `mktemp -d` needs a live one (GNU mktemp fails without). + mkdir -p "$BASE/tmp"; export TMPDIR=$BASE/tmp mkdir -p "$C/scripts" "$C/internal/version" "$C/sdk/go" "$C/sdk/ts" "$C/sdk/ts-sdk" "$C/sdk/py" "$C/.github" \ "$C/internal/build" "$C/cmd/commands" "$A/src/lib" "$B/bin" cp "$ROOT/scripts/bump-version.sh" "$C/scripts/" @@ -55,6 +57,7 @@ gold_all() { local g; for g in $GOLD_PATHS; do grep -q "$1" "$C/$g" || return 1; # 13. bump regenerates the goldens, then re-runs the tests plain ------------------------- mk_bump_env run_bump 9.9.9 +check "bump runs with a TMPDIR that exists" test -d "$TMPDIR" check "bump exits 0" test "$RC" -eq 0 check "bump updated version.go" grep -q '"9.9.9"' "$C/internal/version/version.go" check "bump regenerated all four goldens" gold_all "build-version $NEWH" From 3d4812a647cdc697007701fcefce80db7cabe006 Mon Sep 17 00:00:00 2001 From: admin Date: Sun, 25 Oct 2026 15:02:13 -0400 Subject: [PATCH 5/6] fix(plugin): CLITargetsOf reads the plugin.json compat keys so cliCommands without a pluginType still publish binaries (D-0669) --- internal/plugin/manifestv2/cli_targets.go | 34 +++++++++----- .../plugin/manifestv2/cli_targets_test.go | 47 +++++++++++-------- tools/manifestv2migrate/main.go | 8 +++- tools/manifestv2migrate/targets_test.go | 20 ++++++++ 4 files changed, 75 insertions(+), 34 deletions(-) diff --git a/internal/plugin/manifestv2/cli_targets.go b/internal/plugin/manifestv2/cli_targets.go index bbb8aac9c..121063f1b 100644 --- a/internal/plugin/manifestv2/cli_targets.go +++ b/internal/plugin/manifestv2/cli_targets.go @@ -1,6 +1,7 @@ package manifestv2 import ( + "encoding/json" "strings" "github.com/nself-org/cli/internal/errs" @@ -88,21 +89,28 @@ func CheckCLITargets(plugin string, targets []CLITarget) error { return nil } -// CLITargetsOf is CLITargets over a parsed manifest through its compatibility -// projection, which is what a released CLI reads from a v2 plugin.json. +// CLITargetsOf is CLITargets over the compatibility keys of a plugin.json, the +// ones a released CLI's installer reads: name, pluginType, binaryName and +// cliCommands. A v1 file carries them as written and a v2 file carries them +// derived (Projection), so both give what the installer publishes. // -// A manifest without a commands block publishes no binary: the projection -// carries cliCommands but cannot carry a v1 non-cli pluginType, so reading -// cliCommands alone would build nself- for a service plugin that the -// installer's v1 path gives none (D-0669). -func CLITargetsOf(m *Manifest) []CLITarget { - if m.Commands == nil { - return nil +// It reads the file rather than a parsed Manifest because normalizing a v1 file +// drops a non-cli pluginType, and a service plugin that lists cliCommands has no +// binary on the installer's v1 path while a file with cliCommands and no +// pluginType has one per command (D-0669). +func CLITargetsOf(data []byte) ([]CLITarget, error) { + var v struct { + Name string `json:"name"` + PluginType string `json:"pluginType"` + BinaryName string `json:"binaryName"` + CLICommands []CLICommand `json:"cliCommands"` + } + if err := json.Unmarshal(data, &v); err != nil { + return nil, invalid("plugin.json", err.Error()) } - p := Projection(m) - names := make([]string, 0, len(p.CLICommands)) - for _, c := range p.CLICommands { + names := make([]string, 0, len(v.CLICommands)) + for _, c := range v.CLICommands { names = append(names, c.Name) } - return CLITargets(m.Name, p.PluginType, p.BinaryName, names) + return CLITargets(v.Name, v.PluginType, v.BinaryName, names), nil } diff --git a/internal/plugin/manifestv2/cli_targets_test.go b/internal/plugin/manifestv2/cli_targets_test.go index 655c519b0..a519feaf7 100644 --- a/internal/plugin/manifestv2/cli_targets_test.go +++ b/internal/plugin/manifestv2/cli_targets_test.go @@ -26,25 +26,34 @@ func TestCLITargets(t *testing.T) { } } -// TestCLITargetsOfNonCLIType proves a service plugin that lists cliCommands -// yields no target (D-0669): the v2 conversion drops pluginType, so only the -// commands block says the plugin publishes a binary, as the installer's v1 path -// reads pluginType service. -func TestCLITargetsOfNonCLIType(t *testing.T) { - const svc = `{"name":"retro-gaming","version":"1.0.0","description":"d","category":"gaming","pluginType":"service","cliCommands":[{"name":"server","description":"d"}]}` - m, err := ParseQuiet([]byte(svc)) - if err != nil { - t.Fatal(err) - } - if got := CLITargetsOf(m); got != nil { - t.Errorf("service plugin with cliCommands: got %v want none", got) - } - const cli = `{"name":"tenant","version":"1.0.0","description":"d","category":"infrastructure","pluginType":"cli","binaryName":"nself-tenant","cliCommands":[{"name":"tenant","description":"d"},{"name":"billing","description":"d"}]}` - m, err = ParseQuiet([]byte(cli)) - if err != nil { - t.Fatal(err) +// TestCLITargetsOf proves the targets match the installer's v1 rule for the +// plugin.json as written (D-0669): a service plugin that lists cliCommands has +// none, a plugin with cliCommands and no pluginType has one per command, and +// the v2 form of each gives the same. +func TestCLITargetsOf(t *testing.T) { + const meta = `"version":"1.0.0","description":"d","category":"infrastructure"` + cases := []struct { + name, doc string + want int + }{ + {"service with cliCommands", `{"name":"retro-gaming",` + meta + `,"pluginType":"service","cliCommands":[{"name":"server","description":"d"}]}`, 0}, + {"cliCommands, no pluginType", `{"name":"tenant",` + meta + `,"cliCommands":[{"name":"tenant","description":"d"},{"name":"billing","description":"d"}]}`, 2}, + {"cli type", `{"name":"tenant",` + meta + `,"pluginType":"cli","binaryName":"nself-tenant","cliCommands":[{"name":"tenant","description":"d"},{"name":"billing","description":"d"}]}`, 2}, } - if got := CLITargetsOf(m); len(got) != 2 { - t.Errorf("cli plugin: got %v want 2 targets", got) + for _, c := range cases { + got, err := CLITargetsOf([]byte(c.doc)) + if err != nil || len(got) != c.want { + t.Errorf("%s: got %v, %v want %d targets", c.name, got, err, c.want) + } + if c.want == 0 { + continue // a v2 file has no pluginType to say the plugin is not cli + } + if m, err := ParseQuiet([]byte(c.doc)); err != nil { + t.Errorf("%s: parse: %v", c.name, err) + } else if b, err := Marshal(m); err != nil { + t.Errorf("%s: marshal: %v", c.name, err) + } else if got, err := CLITargetsOf(b); err != nil || len(got) != c.want { + t.Errorf("%s: v2 form: got %v, %v want %d targets", c.name, got, err, c.want) + } } } diff --git a/tools/manifestv2migrate/main.go b/tools/manifestv2migrate/main.go index ba384cfef..5ac2aefd2 100644 --- a/tools/manifestv2migrate/main.go +++ b/tools/manifestv2migrate/main.go @@ -193,12 +193,16 @@ func runCheck(path string, data []byte, stderr io.Writer) int { } func runTargets(data []byte, stdout, stderr io.Writer) int { - m, err := manifestv2.ParseQuiet(data) + if _, err := manifestv2.ParseQuiet(data); err != nil { + say(stderr, "manifestv2migrate: %v\n", err) + return 1 + } + targets, err := manifestv2.CLITargetsOf(data) if err != nil { say(stderr, "manifestv2migrate: %v\n", err) return 1 } - for _, t := range manifestv2.CLITargetsOf(m) { + for _, t := range targets { say(stdout, "%s\t%s\n", t.Binary, t.Command) } return 0 diff --git a/tools/manifestv2migrate/targets_test.go b/tools/manifestv2migrate/targets_test.go index e041e8be1..4e9751091 100644 --- a/tools/manifestv2migrate/targets_test.go +++ b/tools/manifestv2migrate/targets_test.go @@ -45,3 +45,23 @@ func TestTargetsSingleCommandV2(t *testing.T) { t.Errorf("single v2: %d %q %s", code, out, e) } } + +// TestTargetsPluginTypeParity proves -targets follows the installer's v1 rule +// on the file as written: cliCommands without a pluginType publish one binary +// each, and a service plugin's cliCommands publish none (D-0669). +func TestTargetsPluginTypeParity(t *testing.T) { + const meta = `"version":"1.0.0","description":"d","category":"infrastructure"` + cases := []struct{ name, doc, want string }{ + {"no pluginType", `{"name":"tenant",` + meta + `,"cliCommands":[{"name":"tenant","description":"d"},{"name":"billing","description":"d"}]}`, "nself-tenant\ttenant\nnself-billing\tbilling\n"}, + {"service", `{"name":"retro",` + meta + `,"pluginType":"service","cliCommands":[{"name":"server","description":"d"}]}`, ""}, + } + for _, c := range cases { + in := filepath.Join(t.TempDir(), "plugin.json") + if err := os.WriteFile(in, []byte(c.doc), 0o644); err != nil { + t.Fatal(err) + } + if code, out, e := runTool(t, "-in", in, "-targets"); code != 0 || out != c.want { + t.Errorf("%s: %d %q %s want %q", c.name, code, out, e, c.want) + } + } +} From 6cf9956e1d07b9a50ee24b2d482d2a03f610a297 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 15:19:42 -0400 Subject: [PATCH 6/6] fix(plugin): readCache drops the ETag when the cache lock cannot be taken (D-0875) --- internal/plugin/catalog/cache.go | 11 ++++++-- .../plugin/catalog/cache_lock_unix_test.go | 26 +++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/internal/plugin/catalog/cache.go b/internal/plugin/catalog/cache.go index a3f14c000..af6c596de 100644 --- a/internal/plugin/catalog/cache.go +++ b/internal/plugin/catalog/cache.go @@ -57,8 +57,12 @@ func readCache(dir string) (*cached, *Catalog, bool) { if dir == "" { return nil, nil, false } - // Best effort: with no lock (dir missing, read-only) the read goes ahead. + // Without the lock the body can still be read (it is replaced atomically) + // but the ETag cannot be trusted to belong to it, so it is dropped: the + // next fetch is unconditional, never a 304 against the wrong body (D-0875). + locked := false if unlock, err := lockCache(dir); err == nil { + locked = true defer unlock() } path := filepath.Join(dir, bodyFile) @@ -74,7 +78,10 @@ func readCache(dir string) (*cached, *Catalog, bool) { if err != nil { return nil, nil, false } - etag, _ := os.ReadFile(filepath.Join(dir, etagFile)) + var etag []byte + if locked { + etag, _ = os.ReadFile(filepath.Join(dir, etagFile)) + } return &cached{body: body, etag: strings.TrimSpace(string(etag)), at: info.ModTime()}, cat, true } diff --git a/internal/plugin/catalog/cache_lock_unix_test.go b/internal/plugin/catalog/cache_lock_unix_test.go index 98931acb7..a61df6d2a 100644 --- a/internal/plugin/catalog/cache_lock_unix_test.go +++ b/internal/plugin/catalog/cache_lock_unix_test.go @@ -48,3 +48,29 @@ func TestWriteCacheWaitsForOtherProcessLock(t *testing.T) { t.Fatalf("cache after release: ok=%v etag=%q", ok, c.etag) } } + +// TestReadCacheDropsETagWhenLockTimesOut holds the flock through a separate +// descriptor past lockWait: the read must not return an ETag it could not read +// under the lock, or a later 304 would keep the wrong body (D-0875). +func TestReadCacheDropsETagWhenLockTimesOut(t *testing.T) { + dir := t.TempDir() + writeCache(dir, sampleCatalog(1), `"e1"`) + f, err := os.OpenFile(filepath.Join(dir, lockFile), os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + t.Fatal(err) + } + defer f.Close() //nolint:errcheck // test descriptor + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil { + t.Fatal(err) + } + old := lockWait + lockWait = 50 * time.Millisecond + defer func() { lockWait = old }() + c, _, ok := readCache(dir) + if !ok { + t.Fatal("body should still be readable while the lock is held elsewhere") + } + if c.etag != "" { + t.Fatalf("etag %q returned without the cache lock", c.etag) + } +}