From 0df5f1e83aac98be038c7fc77d54cdf0de53a477 Mon Sep 17 00:00:00 2001 From: Guy J Grigsby Date: Tue, 22 Sep 2026 17:56:02 +0000 Subject: [PATCH 1/8] aperture: version source builds from the release tag A make install or go build binary reported a commit height (B29), so a running binary could not be matched to the release it came from. The goreleaser build already stamped the tag, so the height scheme only ever showed on source builds, which is every build a dev runs. git describe gives the tag on a release commit and the tag plus distance past it otherwise, for both the Makefile ldflags and the init() fallback plain go build exercises. Keeping B-numbers would have cost nothing to write and stayed unmatchable against the release list forever. --- Makefile | 4 ++-- cmd/aperture/main.go | 26 ++++++++++---------------- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/Makefile b/Makefile index 7525a73..5f61486 100644 --- a/Makefile +++ b/Makefile @@ -1,14 +1,14 @@ .PHONY: build test lint check clean install BUILD_DATE := $(shell date -u +"%Y-%m-%dT%H:%M:%SZ") -GIT_HEIGHT := $(shell git rev-list --count HEAD 2>/dev/null || echo 0) +GIT_VERSION := $(shell git describe --tags --always --dirty 2>/dev/null || echo dev) GIT_DESC := $(shell git describe --always) ifneq ($(shell git status --porcelain),) GIT_DESC := $(GIT_DESC)-dirty endif -LDFLAGS := -X main.buildVersion=B$(GIT_HEIGHT) -X main.buildCommit=$(GIT_DESC) -X main.buildDate=$(BUILD_DATE) +LDFLAGS := -X main.buildVersion=$(GIT_VERSION) -X main.buildCommit=$(GIT_DESC) -X main.buildDate=$(BUILD_DATE) build: go build -ldflags "$(LDFLAGS)" -o .build/aperture ./cmd/aperture diff --git a/cmd/aperture/main.go b/cmd/aperture/main.go index f37997e..b928cf3 100644 --- a/cmd/aperture/main.go +++ b/cmd/aperture/main.go @@ -47,8 +47,8 @@ func init() { } if buildVersion == "B0-dev" { - if height := gitCommitHeight(); height != "" { - buildVersion = "B" + height + if desc := gitDescribe(); desc != "" { + buildVersion = desc } else if info.Main.Version != "" && info.Main.Version != "(devel)" { buildVersion = info.Main.Version } @@ -79,14 +79,17 @@ func init() { } } -func gitCommitHeight() string { +// gitDescribe reports the release version of the checkout containing this +// source file: the tag on an exact release commit, or the nearest tag with +// the distance and commit appended. It returns "" outside a checkout. +func gitDescribe() string { _, file, _, ok := runtime.Caller(0) if !ok { return "" } for dir := filepath.Dir(file); ; dir = filepath.Dir(dir) { if _, err := os.Stat(filepath.Join(dir, ".git")); err == nil { - return gitCommitHeightInDir(dir) + return gitDescribeInDir(dir) } parent := filepath.Dir(dir) if parent == dir { @@ -95,23 +98,14 @@ func gitCommitHeight() string { } } -func gitCommitHeightInDir(dir string) string { - cmd := exec.Command("git", "rev-list", "--count", "HEAD") +func gitDescribeInDir(dir string) string { + cmd := exec.Command("git", "describe", "--tags", "--always", "--dirty") cmd.Dir = dir out, err := cmd.Output() if err != nil { return "" } - height := strings.TrimSpace(string(out)) - if height == "" { - return "" - } - for _, r := range height { - if r < '0' || r > '9' { - return "" - } - } - return height + return strings.TrimSpace(string(out)) } // startRunLog points slog at the run log and returns a function that closes From 4c261fa44496900da56d1d606df8fcd839b6caed Mon Sep 17 00:00:00 2001 From: Guy J Grigsby Date: Tue, 22 Sep 2026 18:10:32 +0000 Subject: [PATCH 2/8] e2e: add end-to-end suite driving the built binary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every existing test exercises internals in-process; nothing executed the shipped binary, so the flag-before-TUI ordering, the PTY handoff to a launched agent, and the return to the picker after the child exits were all untested at the level users hit them. Four tests against the real build: -version, a bad endpoint failing before the TUI takes the terminal, the Pi happy path against a fake Aperture (httptest /v1/models) with a stub pi on PATH recording argv and capturing the generated provider extension while it exists, and the unreachable-endpoint banner. Runs hermetically: temp HOME/XDG, PATH of the stub dir plus system dirs only so host agent binaries cannot leak into the picker. creack/pty is promoted from the module graph; no new third-party code. TERM=dumb because startup asks the terminal its color profile and a PTY never answers — with xterm each run paid the five-second query timeout. Skipped: bridge flows (need a control plane), install flows (network), Windows PTYs. --- e2e/e2e_test.go | 128 ++++++++++++++++++++++++++++++++ e2e/harness.go | 193 ++++++++++++++++++++++++++++++++++++++++++++++++ go.mod | 1 + 3 files changed, 322 insertions(+) create mode 100644 e2e/e2e_test.go create mode 100644 e2e/harness.go diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go new file mode 100644 index 0000000..fbb9aac --- /dev/null +++ b/e2e/e2e_test.go @@ -0,0 +1,128 @@ +package e2e + +import ( + "io" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +var apertureBin string + +// TestMain builds the binary under test once. The tests exercise what we +// ship, so they run the real build rather than recompiling main's guts +// into the test binary. +func TestMain(m *testing.M) { + dir, err := os.MkdirTemp("", "aperture-e2e") + if err != nil { + panic(err) + } + apertureBin = filepath.Join(dir, "aperture") + build := exec.Command("go", "build", "-o", apertureBin, "../cmd/aperture") + if out, err := build.CombinedOutput(); err != nil { + panic(string(out)) + } + code := m.Run() + _ = os.RemoveAll(dir) + os.Exit(code) +} + +// modelsJSON is the smallest GET /v1/models payload that walks one provider +// through discovery: one provider, one model, one wire endpoint. With one +// of each, the launch flow asks no follow-up questions — selecting the +// client launches it. +const modelsJSON = `{ + "object": "list", + "data": [ + { + "id": "test-model", + "supported_endpoints": ["/v1/responses"], + "metadata": {"provider": {"id": "test-provider", "name": "Test Provider", "upstream": "test"}} + } + ] +}` + +// fakeAperture serves the discovery contract and nothing else. +func fakeAperture(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/v1/models" { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, modelsJSON) + })) + t.Cleanup(srv.Close) + return srv +} + +func TestVersion(t *testing.T) { + stdout, _, err := run(t, apertureBin, hermeticEnv(t, ""), "-version") + if err != nil { + t.Fatalf("-version: %v", err) + } + if stdout == "" || stdout == "B0-dev" { + t.Errorf("version output = %q, want a release version", stdout) + } +} + +// A bad endpoint URL has to fail before the TUI takes the terminal: the +// script that passed it reads stderr and the exit code, not a painted error. +func TestBadEndpointExitsBeforeTUI(t *testing.T) { + _, stderr, err := run(t, apertureBin, hermeticEnv(t, ""), "-endpoint", "://nope") + exitErr, ok := err.(*exec.ExitError) + if !ok || exitErr.ExitCode() != 1 { + t.Fatalf("exit = %v, want exit code 1", err) + } + if !strings.Contains(stderr, "aperture:") { + t.Errorf("stderr = %q, want the failure reported", stderr) + } +} + +// The happy path: connect to a fake Aperture, pick the only installed +// client, and watch the launch reach the stub binary with the generated +// provider extension pointing back at the fake Aperture. +func TestLaunchPi(t *testing.T) { + srv := fakeAperture(t) + binDir := t.TempDir() + recordDir := installStubPi(t, binDir) + env := append(hermeticEnv(t, binDir), "APERTURE_E2E_RECORD="+recordDir) + + term := spawn(t, apertureBin, []string{"-endpoint", srv.URL}, env) + // With the stub as the only installed client, the picker opens with Pi + // as row [1] and no quick-select. + term.waitFor(t, "Which editor do you want to use?") + term.waitFor(t, "[1] Pi") + term.send("1") + + // One provider, one backend, one model: no follow-up menus, the + // selection launches straight into the stub. + argv := waitForFile(t, filepath.Join(recordDir, "argv")) + extension := waitForFile(t, filepath.Join(recordDir, "extension")) + + // "q" quits only from the root menu: a clean exit here also proves the + // TUI took the terminal back after the child exited. + term.send("q") + term.waitExit(t) + + if !strings.Contains(argv, "-e\n") { + t.Errorf("stub argv = %q, want the extension loaded with -e", argv) + } + if !strings.Contains(extension, srv.URL) { + t.Errorf("extension routes to %q, want the Aperture at %s", extension, srv.URL) + } +} + +// An unreachable endpoint paints the failure banner rather than hanging or +// exiting; the launcher stays up so the user can pick another endpoint. +func TestUnreachableEndpoint(t *testing.T) { + term := spawn(t, apertureBin, []string{"-endpoint", "http://127.0.0.1:1"}, hermeticEnv(t, "")) + term.waitFor(t, "Could not reach") + term.send("\x03") + term.waitExit(t) +} diff --git a/e2e/harness.go b/e2e/harness.go new file mode 100644 index 0000000..ece45ac --- /dev/null +++ b/e2e/harness.go @@ -0,0 +1,193 @@ +// Package e2e exercises the built aperture binary end to end: real process, +// real PTY, a fake Aperture over HTTP, and stub agent binaries on PATH. +// Nothing here touches the user's real home, config, or tailnet. +package e2e + +import ( + "bytes" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/charmbracelet/x/ansi" + "github.com/creack/pty" +) + +// terminal is a running aperture process attached to a PTY, with the output +// stream captured for assertions. +type terminal struct { + cmd *exec.Cmd + ptmx *os.File + + mu sync.Mutex + out bytes.Buffer +} + +// spawn starts bin on a PTY with args and env and begins capturing output. +func spawn(t *testing.T, bin string, args []string, env []string) *terminal { + t.Helper() + cmd := exec.Command(bin, args...) + cmd.Env = env + ptmx, err := pty.StartWithSize(cmd, &pty.Winsize{Rows: 40, Cols: 120}) + if err != nil { + t.Fatalf("spawning %s: %v", bin, err) + } + term := &terminal{cmd: cmd, ptmx: ptmx} + go func() { + var buf [4096]byte + for { + n, err := ptmx.Read(buf[:]) + if n > 0 { + term.mu.Lock() + term.out.Write(buf[:n]) + term.mu.Unlock() + } + if err != nil { + return + } + } + }() + return term +} + +// send types keys into the terminal. +func (term *terminal) send(keys string) { + if _, err := term.ptmx.WriteString(keys); err != nil { + // The process may have exited between the last waitFor and this + // send; waitExit reports the real outcome. + return + } +} + +// screen is everything the process has printed so far, escape sequences +// stripped, so assertions match words rather than terminal control bytes. +func (term *terminal) screen() string { + term.mu.Lock() + defer term.mu.Unlock() + return ansi.Strip(term.out.String()) +} + +// waitFor blocks until substr appears on screen, failing with the full +// screen after a generous timeout. The timeout covers bridge bring-up on a +// slow CI box; locally every screen arrives in milliseconds. +func (term *terminal) waitFor(t *testing.T, substr string) { + t.Helper() + deadline := time.Now().Add(15 * time.Second) + for time.Now().Before(deadline) { + if strings.Contains(term.screen(), substr) { + return + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("timed out waiting for %q; screen:\n%s", substr, term.screen()) +} + +// waitExit expects the process to exit 0 within the timeout. +func (term *terminal) waitExit(t *testing.T) { + t.Helper() + done := make(chan error, 1) + go func() { done <- term.cmd.Wait() }() + select { + case err := <-done: + term.ptmx.Close() + if err != nil { + t.Fatalf("exit: %v; screen:\n%s", err, term.screen()) + } + case <-time.After(15 * time.Second): + _ = term.cmd.Process.Kill() + t.Fatalf("process still running; screen:\n%s", term.screen()) + } +} + +// run executes bin without a PTY and returns its streams. For the flag +// paths that resolve before the TUI takes the terminal. +func run(t *testing.T, bin string, env []string, args ...string) (string, string, error) { + t.Helper() + cmd := exec.Command(bin, args...) + cmd.Env = env + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + err := cmd.Run() + return stdout.String(), stderr.String(), err +} + +// hermeticEnv builds the environment for one run: a throwaway home so +// config, state, the run log and generated extensions land in a temp dir, +// and a PATH of binDir plus the system dirs only. The system dirs keep git +// reachable for version stamping while hiding every agent binary the dev +// box happens to have installed, so the client picker shows exactly the +// stubs the test installed. +// +// TERM=dumb, not xterm: startup asks the terminal its color profile and a +// PTY never answers, which costs every run a five-second query timeout. +// Assertions strip escape sequences anyway, so color buys nothing here. +func hermeticEnv(t *testing.T, binDir string) []string { + t.Helper() + home := t.TempDir() + drop := map[string]bool{ + "HOME": true, "XDG_CONFIG_HOME": true, "TERM": true, "PATH": true, + "APERTURE_ENDPOINT": true, "APERTURE_BRIDGE": true, + } + var env []string + for _, e := range os.Environ() { + k, _, _ := strings.Cut(e, "=") + if !drop[k] { + env = append(env, e) + } + } + path := string(filepath.ListSeparator) + "/usr/bin" + string(filepath.ListSeparator) + "/bin" + if binDir != "" { + path = binDir + path + } + return append(env, + "HOME="+home, + "XDG_CONFIG_HOME="+filepath.Join(home, ".config"), + "TERM=dumb", + "PATH="+path, + ) +} + +// stubPi is a stand-in for the pi binary: it records its argv and +// environment, and captures the generated provider extension while it still +// exists — aperture removes the extension when the child exits. +const stubPi = `#!/bin/sh +printf '%s\n' "$@" > "$APERTURE_E2E_RECORD/argv" +env > "$APERTURE_E2E_RECORD/env" +prev="" +for a in "$@"; do + if [ "$prev" = "-e" ]; then + cat "$a" > "$APERTURE_E2E_RECORD/extension" + fi + prev="$a" +done +` + +// installStubPi writes the stub into binDir and returns the directory the +// stub records into. +func installStubPi(t *testing.T, binDir string) string { + t.Helper() + recordDir := t.TempDir() + if err := os.WriteFile(filepath.Join(binDir, "pi"), []byte(stubPi), 0o755); err != nil { + t.Fatal(err) + } + return recordDir +} + +// waitForFile blocks until path exists and is non-empty. +func waitForFile(t *testing.T, path string) string { + t.Helper() + deadline := time.Now().Add(15 * time.Second) + for time.Now().Before(deadline) { + if data, err := os.ReadFile(path); err == nil && len(data) > 0 { + return string(data) + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("timed out waiting for %s", path) + return "" +} diff --git a/go.mod b/go.mod index 3a697de..0af0e28 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/charmbracelet/bubbletea v1.3.10 github.com/charmbracelet/lipgloss v1.1.0 github.com/charmbracelet/x/ansi v0.11.6 + github.com/creack/pty v1.1.24 golang.org/x/sys v0.47.0 tailscale.com v1.102.3 ) From 492405b38f007e236a992bd5347864b44a1e3b00 Mon Sep 17 00:00:00 2001 From: Guy J Grigsby Date: Tue, 22 Sep 2026 18:11:56 +0000 Subject: [PATCH 3/8] docs: require unit and e2e tests with new functionality --- AGENTS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 4321ff0..4cffa00 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -120,6 +120,8 @@ license-compatible, no GPL/AGPL; name the one chosen, or why none fit. never poll the control plane or the LocalAPI in a tight loop. - Work consciously skipped is said out loud, never left as a TODO comment or as speculative code. +- Unit tests and e2e tests need to be included with all new functionality. + Test the expected ideal behavior, not the current implementation details. Being lazy about the solution is the goal. Being lazy about understanding it is not: trace the flow a change touches before picking an approach, because the From f5e9edf78d25b61d69a8e85480cb3d5c4c5cebd7 Mon Sep 17 00:00:00 2001 From: Guy J Grigsby Date: Mon, 21 Sep 2026 20:24:51 +0000 Subject: [PATCH 4/8] bridges: give each concurrent process its own Machine slot APT-330: parallel aperture sessions in bridge mode shared one state directory per bridge, so every process registered the same node key and the control plane handed the session to the newest registrant. The older processes looked Running locally while their dials reached nothing, surfacing as hangs and "api error", and the shared directory was being written concurrently on top. A Machine's identity is now numbered by slot. Slot 1 keeps the existing state directory and hostname so authorized devices carry over; slots 2 and up get suffixed siblings. A process claims the lowest free slot with an exclusive non-blocking lock on a lock file under bridges/locks/ when it builds the node and holds it until the Machine closes or is destroyed. Lock files live outside the state directory so removal never deletes an open lock and a claimant's inode cannot be deleted under it. Machines.Destroy logs out every slot the bridge has on disk and fails before any logout when a live process holds one, naming the conflict. Evicting a running session is the silent kill this change removes, not a removal feature. Rejected the shared daemon (a wire protocol, trust boundary and daemon lifecycle for a first iteration) and ephemeral per-process nodes (key expiry forces a browser login per launch). ADR 0006 records the call and the revisit conditions. --- docs/adr/0006-one-machine-slot-per-process.md | 71 ++++++ docs/specs/bridge-resource-lifecycle.md | 29 ++- docs/specs/connection-context-map.md | 6 +- docs/specs/connection-contracts.md | 12 + docs/specs/connection-domain-model.md | 15 +- internal/bridges/attempt_test.go | 2 +- internal/bridges/helpers_test.go | 8 +- internal/bridges/lifecycle_test.go | 44 ++-- internal/bridges/machine.go | 111 ++++++--- internal/bridges/machine_test.go | 16 +- internal/bridges/machines.go | 7 +- internal/bridges/node.go | 8 +- internal/bridges/remove.go | 68 +++++- internal/bridges/route_test.go | 2 +- internal/bridges/security_test.go | 4 +- internal/bridges/slot.go | 71 ++++++ internal/bridges/slot_test.go | 225 ++++++++++++++++++ internal/bridges/slot_unix.go | 22 ++ internal/bridges/slot_windows.go | 26 ++ internal/config/settings.go | 51 +++- internal/config/state_test.go | 44 +++- internal/tui/removal.go | 33 ++- internal/tui/removal_test.go | 4 +- 23 files changed, 766 insertions(+), 113 deletions(-) create mode 100644 docs/adr/0006-one-machine-slot-per-process.md create mode 100644 internal/bridges/slot.go create mode 100644 internal/bridges/slot_test.go create mode 100644 internal/bridges/slot_unix.go create mode 100644 internal/bridges/slot_windows.go diff --git a/docs/adr/0006-one-machine-slot-per-process.md b/docs/adr/0006-one-machine-slot-per-process.md new file mode 100644 index 0000000..ac34f89 --- /dev/null +++ b/docs/adr/0006-one-machine-slot-per-process.md @@ -0,0 +1,71 @@ +# 0006. One Machine slot per concurrent process + +Status: accepted +Date: 2026-09-21 + +## Why? + +APT-330: five to ten agent sessions each running aperture in bridge mode leave +only the last-started one working; the rest hang and their clients get +"api error". Every process used the same state directory for the same Bridge, +so every process registered the same node key under the same hostname. The +control plane treats same-key connections as one node and hands the session to +the newest registrant; the older processes keep reporting themselves Running +locally while their dials silently reach nothing. The state directory was also +written concurrently, which risks corruption on top. The in-process rule "two +Machines for one Bridge would open the same state directory" never covered the +two-process case, and the failure mode it produced was silent. + +## Decision + +1. A Bridge's Machine identity is numbered by slot. Slot 1 keeps the existing + state directory and hostname (`bridges/`, + `aperture-cli-`); slots 2 and up get sibling directories + `bridges/-N` and hostnames `aperture-cli--N`. Existing + users keep the device they already authorized. +2. A process claims the lowest free slot when it builds the node, and holds + it until the Machine is closed or destroyed. The claim is an exclusive + non-blocking lock on `bridges/locks/-.lock` (flock where + there is flock, LockFileEx on Windows), held open for the node's life. + Lock files live outside the state directory so removal never deletes an + open lock and a claimant's inode can never be deleted under it. +3. `Machines.Destroy` logs out every slot the bridge has on disk. A slot + locked by a live process makes the whole removal fail before anything is + logged out, naming the conflict, because evicting a running session is + exactly the silent kill this ADR exists to remove. +4. Cap: 100 slots per bridge. Past that the process errors instead of + probing forever. +5. Slot claiming emits no event and writes no settings. It is recorded in + the contracts as deliberately silent. + +## Consequences + +Each concurrent process is its own device in the admin console: ten parallel +agent sessions are ten devices named `aperture-cli-` through +`-10`. They accumulate when processes die unclosed (the devices go offline +and stay listed) and re-authorize only when a slot has never been authorized +before — once per slot, not once per launch. The failure mode for a +contended bridge moves from silent eviction to either a fresh slot (normal +case) or a named error (removal). + +## Rejected + +- One shared bridge daemon per machine, CLI instances dialing it over a local + socket — the correct architecture and roughly what `tailscaled` already is; + it costs a wire protocol, a trust boundary and a daemon lifecycle this CLI + does not otherwise have, for a first iteration that needed to make parallel + sessions work this week. +- A fresh ephemeral node per process — key expiry on ephemeral nodes means a + browser login on every launch, which is worse than the bug for the reported + workflow. +- Failing loudly on the second process with no sharing at all — converts + silent breakage into loud breakage; the report asked for the sessions to + work, not to be refused. + +## Revisit when + +The admin-console litter or the slot cap becomes the problem someone files, +or a daemon is wanted for its own reasons (credential renewal, one +connection to share). The lock files and slot layout are additive on top of +slot 1's legacy directory, so a daemon migration does not orphan existing +state. diff --git a/docs/specs/bridge-resource-lifecycle.md b/docs/specs/bridge-resource-lifecycle.md index db1ef46..682eb23 100644 --- a/docs/specs/bridge-resource-lifecycle.md +++ b/docs/specs/bridge-resource-lifecycle.md @@ -1,15 +1,18 @@ # Bridge resource lifecycle -Creating a bridge produces three things. Removing one destroys one of them. -The survivors are a device in the user's admin console and a directory on -their disk. Decision: [ADR 0002](../adr/0002-bridge-removal-destroys-the-machine.md). +Creating a bridge produces three things per Machine slot it opens. Removing +one destroys all of them. The survivors are devices in the user's admin +console and directories on their disk. Decisions: +[ADR 0002](../adr/0002-bridge-removal-destroys-the-machine.md), +[ADR 0006](../adr/0006-one-machine-slot-per-process.md). ## What a bridge creates | Resource | Created by | First exists | Removed by | |---|---|---|---| -| Machine `aperture-cli-` | `tsnet.Server` registering | first successful `Activate` | nothing | -| `$UserConfigDir/aperture/bridges/` | tsnet, from `Server.Dir` | first `Activate`, successful or not | nothing | +| Machine `aperture-cli-[-N]` | `tsnet.Server` registering | first successful `Activate` of that slot | `Machines.Destroy` | +| `$UserConfigDir/aperture/bridges/[-N]` | tsnet, from `Server.Dir` | first `Activate` of that slot, successful or not | `Machines.Destroy` | +| `bridges/locks/-.lock` | the slot claim, when a node is built | first `Activate` of that slot | nothing; the lock is held open, the file content empty | | `config.Bridge` | `AddBridge` (`global.go:178`) | the moment a name is typed | `RemoveBridge` (`global.go:219`) | The device outlives the process because `newTSNetNode` (`node.go`) sets no @@ -21,7 +24,9 @@ installer cleanup. `Machine.LeaveTailnet` and `Machine.Destroy` are the only callers of `Logout`, and its comment already names the failure mode: a close without a logout leaves the -device orphaned rather than removed. +device orphaned rather than removed. Each concurrent process holds its own +slot (ADR 0006), so parallel sessions register sibling devices rather than +evicting each other on the control plane. ## Where a bridge can be removed @@ -45,12 +50,12 @@ login nobody finished, and is the one case with no device to clean up. ## What destroying it needs -`Machine.destroy`, on the aggregate that owns the node -([domain model](connection-domain-model.md#machine)): `Logout`, `Close`, then -discard the state directory, which is the Machine's own persistence. -`Machine.Destroy` is the entry point, reached through `Machines.Destroy`, because -destruction has to hold the Machine like every other -operation on that node. +`Machines.Destroy` logs out every slot the bridge has on disk: for each, a +Machine on that slot does `Logout`, `Close`, then discards the slot's state +directory, which is that identity's own persistence. A slot locked by a live +process fails the whole removal before anything is logged out — evicting a +running session is the failure ADR 0006 exists to remove, not a removal +feature. The state directory goes last and only when the logout succeeded: it holds the node key, which is what a later attempt would need to deregister the device. diff --git a/docs/specs/connection-context-map.md b/docs/specs/connection-context-map.md index 4112122..c5189ee 100644 --- a/docs/specs/connection-context-map.md +++ b/docs/specs/connection-context-map.md @@ -20,7 +20,8 @@ The context boundaries below also describe the proposed broader event refactor. | Gateway | The address a client is finally told to send requests to. The Endpoint URL when no Bridge is involved, the Route's local end when one is. | The Endpoint. Only equal to it in the direct case. | | Route | The local door to one Endpoint through one Machine: a `127.0.0.1:0` listener reverse-proxying over the Machine. | A tailnet route or subnet route. | | Bridge | The thing the user configures and sees in the picker: id, display name, last tailnet joined. Persisted. | The running tsnet node. | -| Machine | What this program runs on the user's tailnet for one Bridge: registers, may need a login, gets an address, carries dials, and shows up under Machines in their admin console. Outlives any one Attempt. | The Bridge record. The proxy. The computer aperture is running on. | +| Machine | What this program runs on the user's tailnet for one Bridge and one Slot: registers, may need a login, gets an address, carries dials, and shows up under Machines in their admin console. Outlives any one Attempt. | The Bridge record. The proxy. The computer aperture is running on. | +| Slot | Which of a Bridge's Machine identities a process holds, numbered from 1. Slot 1 keeps the original unsuffixed state directory and hostname; slot N adds a `-N` suffix to both. Held by an exclusive lock on a lock file, claimed when the node is built, released when the Machine closes or is destroyed. | A tailnet device. The lock file is locked, never the state directory. | | Machines | The process's Machines, one per Bridge. Where a Machine is created and where they are all closed. | A manager. It does no network work of its own. | | Login Link | The URL that authorizes a Machine. `https` only, no whitespace, opened in a browser or copied. | Any URL in a log line. | | Phase | What the Attempt is waiting on right now, named for what the user is waiting for. | `ipn.State`. | @@ -128,7 +129,8 @@ two while a login is outstanding, then to one: it exits when the state leaves |---|---| | Endpoint list, active endpoint | stored, `settings.json` | | Bridge id, name, last tailnet | stored, `settings.json` | -| Machine tailnet credentials | stored by tsnet under the bridge state dir, never by us | +| Machine tailnet credentials | stored by tsnet under the slot's bridge state dir, never by us | +| Slot lock | a held-open file lock under `bridges/locks/`, process lifetime, file content empty | | Machine, Route | transient, process lifetime, keyed by bridge id | | Connection Attempt, Phase, Progress | transient, attempt lifetime | | Gateway | transient, overwritten per successful Attempt | diff --git a/docs/specs/connection-contracts.md b/docs/specs/connection-contracts.md index d833410..6d07646 100644 --- a/docs/specs/connection-contracts.md +++ b/docs/specs/connection-contracts.md @@ -149,11 +149,23 @@ Everything else persisted is unconditional: `Endpoint.URL`, `Endpoint.BridgeID` (empty means direct, which is a real value and not an absence), `Bridge.ID`, `Bridge.Name`. +A Bridge's Machine identity lives outside settings, in the config directory: +one state directory per Slot, `bridges/` for slot 1 and +`bridges/-N` after, holding the node key tsnet wrote; and one lock file +per slot under `bridges/locks/`, empty, held locked by the process running +that slot. The state directory is the durable evidence a Machine exists +(`HasMachine` reads it); the lock is the durable evidence one is running +(`Machines.Destroy` reads it). + ## Cross-check With no API and no DDL, the cross-check reduces to: every aggregate transition emits an event, or is recorded here as deliberately silent. +Deliberately silent: claiming and releasing a Slot. It is process-local +fencing between CLI instances, not a fact about the Bridge or the tailnet, +and no surface displays it. + | Transition | Event | Note | |---|---|---| | ConnectionAttempt → `StartingMachine` | `PhaseEntered` | | diff --git a/docs/specs/connection-domain-model.md b/docs/specs/connection-domain-model.md index 267e47d..7747452 100644 --- a/docs/specs/connection-domain-model.md +++ b/docs/specs/connection-domain-model.md @@ -232,7 +232,8 @@ across Attempts, so it cannot be owned by any one of them. | Field | Type | Note | |---|---|---| -| `bridge` | `config.Bridge` | Identity is its ID. At most one Machine per Bridge. | +| `bridge` | `config.Bridge` | Identity is its ID. At most one Machine per Bridge in a process. | +| `slot` | `int` | Which Slot it holds, zero until a node is built. | | `tailnet` | `string` | The network joined, empty until the netmap lands and after leaving. | | `routes` | `map[string]*Route` | Keyed by target URL. | @@ -242,7 +243,8 @@ Behaviors: `Open(ctx, emit) error`, `RouteTo(ctx, url, emit) (*Route, error)`, Invariants: - A Route can only be created through an open Machine. `RouteTo` fails rather than starts a node. -- One operation at a time, cleanup included. Two Machines for one Bridge would open the same state directory, so only `Machines` creates them. +- One operation at a time, cleanup included. Within a process only `Machines` creates Machines, one per Bridge; across processes the Slot lock keeps one state directory to one process ([ADR 0006](../adr/0006-one-machine-slot-per-process.md)). +- The Slot is claimed when the node is built and held until `Close` or `Destroy`: a slot's directory may only be opened by a process holding its lock. `LeaveTailnet` keeps the Slot, so a reopen keeps the identity it had. - `LeaveTailnet` logs out before closing: credentials live behind the node's own LocalAPI, so a close without a logout silently reuses them next time. `Destroy` also discards the state directory, last and only on success, because it holds the key a later attempt needs to deregister. - Closing closes every Route first. - Exactly one IPN bus watch per Machine. @@ -282,11 +284,14 @@ tailnet with a same-named node. ## Machines Collection. The process's Machines, one per Bridge, and the only place a -Machine is created. Getting a member does no network work. +Machine is created. Getting a member does no network work; a member claims +its Slot only when a node is built, so `For` never takes a lock. Behaviors: `For(Bridge) (*Machine, error)`, which creates an idle member on first use and refuses after `Close`; `Close() error`, which closes every -member and lets concurrent callers share one result. +member and lets concurrent callers share one result; `Destroy(ctx, bridge, +emit) error`, which logs out every Slot the bridge has on disk and refuses, +before logging anything out, while a live process holds one. Invariants: at most one Machine per Bridge ID. A Bridge ID that is not the generated `bridge-` shape is refused before it can become a hostname. @@ -301,7 +306,7 @@ registered for it go together, Machine first (ADR 0002). Four functions in |---|---|---| | `CheckRemovable(settings, bridge, endpoint)` | update loop | Returns an error when the endpoint, or the bare bridge, cannot be removed: the active endpoint, or a bridge an endpoint still connects through. | | `WillDestroyMachine(settings, bridge, endpoint)` | update loop | Reports whether removing the endpoint, or the bare bridge, logs a device out of a tailnet: the bridge started a Machine and no other endpoint connects through it. | -| `Machines.Destroy(ctx, bridge, emit)` | any goroutine | The bounded logout (ADR 0002 decision 6). Returns an error when the tailnet refuses or does not answer in time. Writes nothing. | +| `Machines.Destroy(ctx, bridge, emit)` | any goroutine | The bounded logout (ADR 0002 decision 6), once per Slot the bridge has on disk. Fails before any logout when another process holds a Slot, naming the conflict; otherwise returns an error when a tailnet refuses or does not answer in time. Writes nothing. | | `RemoveFromSettings(settings, bridge, endpoint)` | update loop | Deletes the endpoint, then the bridge when nothing connects through it. Called only after `Destroy` returned nil, or when nothing needs destroying. | | `Machines.Tailnet(bridge)` | update loop | The tailnet the running Machine reports, else the one saved on the bridge. | diff --git a/internal/bridges/attempt_test.go b/internal/bridges/attempt_test.go index 976fb65..076b20d 100644 --- a/internal/bridges/attempt_test.go +++ b/internal/bridges/attempt_test.go @@ -70,7 +70,7 @@ func switchingAttempt(t *testing.T, logoutErr error) (*Attempt, *config.Global, } m := NewMachines(false) t.Cleanup(func() { m.Close() }) - m.newNode = func(config.Bridge, string, func(string, ...any), func(string, ...any)) tailnetNode { + m.newNode = func(config.Bridge, int, string, func(string, ...any), func(string, ...any)) tailnetNode { return &fakeNode{logoutErr: logoutErr, upErr: errors.New("no login yet")} } return a, g, m diff --git a/internal/bridges/helpers_test.go b/internal/bridges/helpers_test.go index 28bae43..1f6784d 100644 --- a/internal/bridges/helpers_test.go +++ b/internal/bridges/helpers_test.go @@ -34,12 +34,10 @@ func switchTailnet(ms *Machines, ctx context.Context, bridge config.Bridge, emit return mc.LeaveTailnet(ctx, emit) } +// destroyMachine removes the bridge's machines the way the TUI does: through +// Machines.Destroy, which logs out every slot the bridge has on disk. func destroyMachine(ms *Machines, ctx context.Context, bridge config.Bridge, emit func(connection.Event)) error { - mc, err := ms.For(bridge) - if err != nil { - return err - } - return mc.Destroy(ctx, emit) + return ms.Destroy(ctx, bridge, emit) } func tailnetOf(ms *Machines, bridgeID string) string { diff --git a/internal/bridges/lifecycle_test.go b/internal/bridges/lifecycle_test.go index b6d7b64..196c1fa 100644 --- a/internal/bridges/lifecycle_test.go +++ b/internal/bridges/lifecycle_test.go @@ -12,6 +12,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "testing" "time" @@ -54,7 +55,7 @@ func TestActivateWaitsForCancelledNodeCleanup(t *testing.T) { replacement := &fakeNode{backendAddr: backend.Listener.Addr().String()} m := NewMachines(false) calls := 0 - m.newNode = func(config.Bridge, string, func(string, ...any), func(string, ...any)) tailnetNode { + m.newNode = func(config.Bridge, int, string, func(string, ...any), func(string, ...any)) tailnetNode { calls++ if calls == 1 { return n @@ -121,7 +122,7 @@ func TestSwitchTailnetDoesNotRequireAuthorization(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) n := &needsLoginNode{fakeNode: &fakeNode{}} m := NewMachines(false) - m.newNode = func(config.Bridge, string, func(string, ...any), func(string, ...any)) tailnetNode { return n } + m.newNode = func(config.Bridge, int, string, func(string, ...any), func(string, ...any)) tailnetNode { return n } defer m.Close() ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) defer cancel() @@ -139,7 +140,7 @@ func TestCloseCancelsStartupBeforeClosingNode(t *testing.T) { close(n.releaseUp) close(n.releaseClose) m := NewMachines(false) - m.newNode = func(config.Bridge, string, func(string, ...any), func(string, ...any)) tailnetNode { return n } + m.newNode = func(config.Bridge, int, string, func(string, ...any), func(string, ...any)) tailnetNode { return n } ctx, cancel := context.WithCancel(context.Background()) defer cancel() bridge := config.Bridge{ID: "bridge-abcdef", Name: "Work"} @@ -189,7 +190,7 @@ func TestConcurrentCloseSharesCompletionAndError(t *testing.T) { t.Setenv("XDG_CONFIG_HOME", t.TempDir()) node := &closingNode{fakeNode: &fakeNode{}, closing: make(chan struct{}), release: make(chan struct{}), err: closeErr} m := NewMachines(false) - m.newNode = func(config.Bridge, string, func(string, ...any), func(string, ...any)) tailnetNode { return node } + m.newNode = func(config.Bridge, int, string, func(string, ...any), func(string, ...any)) tailnetNode { return node } if _, err := activateMachine(m, context.Background(), config.Bridge{ID: "bridge-abcdef"}, "http://100.64.0.2", nil); err != nil { t.Fatal(err) } @@ -235,7 +236,7 @@ func TestCloseEmptyManager(t *testing.T) { // started once, holding the node key that names the registered device. func stateDir(t *testing.T, bridgeID string) string { t.Helper() - dir, err := config.BridgeStateDir(bridgeID) + dir, err := config.BridgeStateDir(bridgeID, 1) if err != nil { t.Fatal(err) } @@ -257,7 +258,7 @@ func TestDestroyLeavesNoMachineBehind(t *testing.T) { } n := &fakeNode{} m := NewMachines(false) - m.newNode = func(config.Bridge, string, func(string, ...any), func(string, ...any)) tailnetNode { return n } + m.newNode = func(config.Bridge, int, string, func(string, ...any), func(string, ...any)) tailnetNode { return n } defer m.Close() if err := destroyMachine(m, context.Background(), bridge, nil); err != nil { @@ -282,7 +283,7 @@ func TestDestroyKeepsStateWhenTheTailnetRefuses(t *testing.T) { dir := stateDir(t, bridge.ID) n := &fakeNode{logoutErr: errors.New("control plane said no")} m := NewMachines(false) - m.newNode = func(config.Bridge, string, func(string, ...any), func(string, ...any)) tailnetNode { return n } + m.newNode = func(config.Bridge, int, string, func(string, ...any), func(string, ...any)) tailnetNode { return n } defer m.Close() if err := destroyMachine(m, context.Background(), bridge, nil); err == nil || !strings.Contains(err.Error(), "control plane said no") { @@ -302,7 +303,7 @@ func TestDestroySkipsABridgeThatNeverStarted(t *testing.T) { t.Fatal("an unstarted bridge reports a machine") } m := NewMachines(false) - m.newNode = func(config.Bridge, string, func(string, ...any), func(string, ...any)) tailnetNode { + m.newNode = func(config.Bridge, int, string, func(string, ...any), func(string, ...any)) tailnetNode { t.Error("started a node to remove a bridge that never had one") return &fakeNode{} } @@ -324,11 +325,12 @@ func TestDestroyReturnsAtTheDeadlineWhileCloseHangs(t *testing.T) { stateDir(t, bridge.ID) n := &pendingNode{fakeNode: &fakeNode{}, closing: make(chan struct{}), releaseClose: make(chan struct{})} m := NewMachines(false) - nodes := 0 - m.newNode = func(config.Bridge, string, func(string, ...any), func(string, ...any)) tailnetNode { - // The hanging node once; the reopen after it gets an ordinary one. - nodes++ - if nodes == 1 { + var nodes atomic.Int32 + m.newNode = func(config.Bridge, int, string, func(string, ...any), func(string, ...any)) tailnetNode { + // The hanging node once; the reopen after it gets an ordinary one. The + // reopen runs while the destroy's close is still hanging, so this count + // is shared between goroutines. + if nodes.Add(1) == 1 { return n } return &fakeNode{status: tailnetStatus("ai.example.ts.net.", "100.64.0.2")} @@ -347,19 +349,19 @@ func TestDestroyReturnsAtTheDeadlineWhileCloseHangs(t *testing.T) { t.Fatal("Destroy did not return at its deadline while Close hung") } - // Still held: a reopen waits for the close to finish. + // The reopen does not wait for the stuck close: slot 1 is still locked by + // the destroy's cleanup, so the Open claims the next slot rather than + // opening a state directory under a close still running. opened := make(chan error, 1) go func() { _, err := activateMachine(m, context.Background(), bridge, "http://ai", nil); opened <- err }() select { case err := <-opened: - t.Fatalf("Open ran while the destroy's close was still hanging: %v", err) - case <-time.After(100 * time.Millisecond): - } - close(n.releaseClose) - select { - case <-opened: + if err != nil { + t.Fatalf("Open: %v", err) + } case <-time.After(2 * time.Second): - t.Fatal("Open never ran after the close finished") + t.Fatal("Open waited on the stuck destroy instead of taking the next slot") } + close(n.releaseClose) m.Close() } diff --git a/internal/bridges/machine.go b/internal/bridges/machine.go index 79b82ba..8212c2a 100644 --- a/internal/bridges/machine.go +++ b/internal/bridges/machine.go @@ -4,10 +4,10 @@ import ( "context" "errors" "fmt" - "io/fs" "log/slog" "net" "os" + "strconv" "sync" "time" @@ -47,6 +47,12 @@ type Machine struct { node tailnetNode routes map[string]*Route ev *eventRelay + + // slot is the bridge slot this Machine claimed when it first started a + // node, and release frees its lock. Both are zero while the Machine has + // never run one, and again after Destroy discards the slot's identity. + slot int + release func() } func newMachine(bridge config.Bridge, ms *Machines) *Machine { @@ -59,10 +65,32 @@ func newMachine(bridge config.Bridge, ms *Machines) *Machine { } } -// MachineName returns the hostname the bridge's node registers under, which -// is the device name the tailnet shows. Removal has to name the same thing -// the admin console does, or a user told to delete it by hand cannot find it. -func MachineName(bridgeID string) string { return "aperture-cli-" + bridgeID } +// MachineName returns the hostname the bridge's node registers under for a +// slot, which is the device name the tailnet shows. Removal has to name the +// same thing the admin console does, or a user told to delete it by hand +// cannot find it. Slot 1 keeps the name existing devices registered under. +func MachineName(bridgeID string, slot int) string { + name := "aperture-cli-" + bridgeID + if slot > 1 { + name += "-" + strconv.Itoa(slot) + } + return name +} + +// MachineNames returns the device names of every slot the bridge has on +// disk, in slot order, for screens that tell the user what a removal logs +// out. +func MachineNames(bridgeID string) ([]string, error) { + slots, err := config.BridgeStateSlots(bridgeID) + if err != nil { + return nil, err + } + names := make([]string, len(slots)) + for i, slot := range slots { + names[i] = MachineName(bridgeID, slot) + } + return names, nil +} // HasMachine reports whether the bridge ever started a node. tsnet creates // the state directory on first use, so a missing directory is the only @@ -70,12 +98,8 @@ func MachineName(bridgeID string) string { return "aperture-cli-" + bridgeID } // serve: it is a display hint, saved after verification and cleared before a // switch. func HasMachine(bridgeID string) bool { - dir, err := config.BridgeStateDir(bridgeID) - if err != nil { - return false - } - _, err = os.Stat(dir) - return !errors.Is(err, fs.ErrNotExist) + slots, err := config.BridgeStateSlots(bridgeID) + return err == nil && len(slots) > 0 } // begin takes the Machine for one operation and returns the context the @@ -255,24 +279,28 @@ func (mc *Machine) LeaveTailnet(ctx context.Context, emit func(connection.Event) return nil } -// Destroy logs the Machine out of its tailnet and deletes the state directory -// holding its login. Destroy touches no settings. The Bridge record is the -// only thing naming the device, so the caller removes it after Destroy -// returns nil and keeps it otherwise (ADR 0002). +// Destroy logs the Machine out of its tailnet and deletes the state +// directory holding its login. Destroy touches no settings. The Bridge record +// is the only thing naming the device, so the caller removes it after +// Destroy returns nil and keeps it otherwise (ADR 0002). It covers the slot +// this Machine holds; Machines.Destroy covers the slots past processes left +// behind. // // A Machine that never started has no device and must not start one to find // out. Bring-up would demand the interactive login that is being removed. // // The state directory goes last and only on success. It holds the node key, -// which a later attempt needs to deregister the device. +// which a later attempt needs to deregister the device. The slot stays locked +// until the work finishes: a claimant while it runs would mint a fresh +// identity that the removal then deletes. // // Destroy returns when the work is done or ctx ends, whichever is first. // Logout takes ctx but the node's Close does not, and a close that hangs must -// not hold the caller past its deadline. The Machine stays held until the -// work finishes, so the next operation waits rather than opening the state -// directory under a close still running. +// not hold the caller past its deadline. Within the process the Machine stays +// held until the work finishes; another process never opens the directory +// under a close still running because the slot lock outlasts the return. func (mc *Machine) Destroy(ctx context.Context, emit func(connection.Event)) error { - stateDir, err := config.BridgeStateDir(mc.bridge.ID) + stateDir, err := config.BridgeStateDir(mc.bridge.ID, mc.slot) if err != nil { return err } @@ -301,12 +329,18 @@ func (mc *Machine) Destroy(ctx context.Context, emit func(connection.Event)) err } } -// destroyHeld does Destroy's work. The caller holds the Machine. +// destroyHeld does Destroy's work. The caller holds the Machine. Whatever the +// outcome, the slot is released when the work finishes: until then the lock +// is the only thing keeping another process from opening the state directory +// under a close still running, and after it the identity is gone or the +// caller is retrying with a fresh claim. Destroy may already have returned at +// its deadline, so nothing outside this goroutine may touch the slot. func (mc *Machine) destroyHeld(ctx context.Context, stateDir string, ev events) error { - if mc.node == nil && !HasMachine(mc.bridge.ID) { + if mc.node == nil && mc.slot == 0 { mc.setTailnet("") return nil } + defer mc.releaseSlot() if err := mc.initNode(ev); err != nil { return err } @@ -327,8 +361,8 @@ func (mc *Machine) destroyHeld(ctx context.Context, stateDir string, ev events) // Close ends the Machine for the process. It interrupts the running // operation, waits for that operation to finish cleaning up, closes the node -// and every Route, and refuses further operations. Close is safe to call more -// than once. +// and every Route, frees the Machine's slot, and refuses further operations. +// Close is safe to call more than once. func (mc *Machine) Close() error { mc.mu.Lock() mc.closed = true @@ -338,11 +372,27 @@ func (mc *Machine) Close() error { mc.mu.Unlock() mc.turn <- struct{}{} defer func() { <-mc.turn }() - return mc.shutdownNode() + err := mc.shutdownNode() + mc.releaseSlot() + return err +} + +// releaseSlot frees the lock guarding the Machine's slot. The caller holds +// the turn. +func (mc *Machine) releaseSlot() { + if mc.release != nil { + mc.release() + mc.release = nil + } + mc.slot = 0 } // initNode constructs a node without waiting for login. The caller holds the // turn. Only Open follows initNode with BringUp. +// +// Constructing a node is what claims the bridge's slot: the claim is a lock +// on the slot number, so a second aperture process opening the same bridge +// takes the next number and never the same node key. func (mc *Machine) initNode(ev events) error { mc.ev.forwardTo(ev) if mc.node != nil { @@ -351,7 +401,14 @@ func (mc *Machine) initNode(ev events) error { if mc.machines.newNode == nil { return fmt.Errorf("bridge node is not configured") } - stateDir, err := config.BridgeStateDir(mc.bridge.ID) + if mc.slot == 0 { + slot, release, err := claimSlot(mc.bridge.ID) + if err != nil { + return err + } + mc.slot, mc.release = slot, release + } + stateDir, err := config.BridgeStateDir(mc.bridge.ID, mc.slot) if err != nil { return err } @@ -365,7 +422,7 @@ func (mc *Machine) initNode(ev events) error { events(mc.ev.emit).notef(format, args...) } } - mc.node = mc.machines.newNode(mc.bridge, stateDir, logNotes, logNotes) + mc.node = mc.machines.newNode(mc.bridge, mc.slot, stateDir, logNotes, logNotes) if mc.node == nil { return fmt.Errorf("bridge node is not configured") } diff --git a/internal/bridges/machine_test.go b/internal/bridges/machine_test.go index 2fbd110..88411ba 100644 --- a/internal/bridges/machine_test.go +++ b/internal/bridges/machine_test.go @@ -133,7 +133,7 @@ func TestActivateDebugDiagnostics(t *testing.T) { node := &fakeNode{status: status, dialErr: errors.New("lookup aperture on 127.0.0.53:53: no such host")} m := NewMachines(true) m.peerWait, m.peerWaitInterval = 5*time.Millisecond, time.Millisecond - m.newNode = func(_ config.Bridge, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { + m.newNode = func(_ config.Bridge, _ int, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { return node } defer m.Close() @@ -185,7 +185,7 @@ func TestActivateDebugDiagnostics(t *testing.T) { func TestActivateClosesNodeWhenUpFails(t *testing.T) { node := &fakeNode{upErr: errors.New("login failed")} m := NewMachines(false) - m.newNode = func(_ config.Bridge, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { + m.newNode = func(_ config.Bridge, _ int, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { return node } @@ -227,7 +227,7 @@ func TestActivateNormalLoggingOmitsDebugDiagnostics(t *testing.T) { backendAddr := strings.TrimPrefix(backend.URL, "http://") node := &fakeNode{status: status, backendAddr: backendAddr} m := NewMachines(false) - m.newNode = func(_ config.Bridge, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { + m.newNode = func(_ config.Bridge, _ int, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { return node } defer m.Close() @@ -276,7 +276,7 @@ func TestActivateWaitsForPeerMapBeforeDialing(t *testing.T) { m := NewMachines(true) m.peerWaitInterval = time.Millisecond - m.newNode = func(_ config.Bridge, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { + m.newNode = func(_ config.Bridge, _ int, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { return node } defer m.Close() @@ -330,7 +330,7 @@ func TestActivateLogsTheLoginLinkBeforeItIsUsable(t *testing.T) { } m := NewMachines(false) - m.newNode = func(_ config.Bridge, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { + m.newNode = func(_ config.Bridge, _ int, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { return node } defer m.Close() @@ -534,7 +534,7 @@ func TestActivateRecordsTailnet(t *testing.T) { defer backend.Close() m := NewMachines(false) - m.newNode = func(_ config.Bridge, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { + m.newNode = func(_ config.Bridge, _ int, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { return &fakeNode{ backendAddr: backend.Listener.Addr().String(), status: &ipnstate.Status{CurrentTailnet: &ipnstate.TailnetStatus{Name: "corp.example.com"}}, @@ -580,7 +580,7 @@ func TestSwitchTailnet(t *testing.T) { } var replacement *fakeNode - f.manager.newNode = func(_ config.Bridge, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { + f.manager.newNode = func(_ config.Bridge, _ int, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { replacement = &fakeNode{backendAddr: backend.Listener.Addr().String()} return replacement } @@ -632,7 +632,7 @@ func activate(t *testing.T, backend *httptest.Server) activatedFixture { t.Helper() var f activatedFixture f.manager = NewMachines(false) - f.manager.newNode = func(_ config.Bridge, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { + f.manager.newNode = func(_ config.Bridge, _ int, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { f.node = &fakeNode{ backendAddr: backend.Listener.Addr().String(), status: tailnetStatus("aperture.tailnet.", "100.64.0.2"), diff --git a/internal/bridges/machines.go b/internal/bridges/machines.go index 5e76adf..80fe1f4 100644 --- a/internal/bridges/machines.go +++ b/internal/bridges/machines.go @@ -20,8 +20,9 @@ const ( // Machines holds the process's Machines, one per Bridge, and is the only // place a Machine is created. Two Machines for one Bridge would open the same -// state directory. Getting a member does no network work. Close ends every -// member and refuses new ones. +// state directory; across processes the slot lock keeps one directory to one +// process. Getting a member does no network work. Close ends every member and +// refuses new ones. type Machines struct { mu sync.Mutex byBridge map[string]*Machine @@ -33,7 +34,7 @@ type Machines struct { // node's peer map before giving up and resolving it the way tsnet would. peerWait time.Duration peerWaitInterval time.Duration - newNode func(bridge config.Bridge, stateDir string, userLogf, debugLogf func(string, ...any)) tailnetNode + newNode func(bridge config.Bridge, slot int, stateDir string, userLogf, debugLogf func(string, ...any)) tailnetNode } // NewMachines returns an empty collection. When debug is true, verbose tsnet diff --git a/internal/bridges/node.go b/internal/bridges/node.go index c51b1da..638956b 100644 --- a/internal/bridges/node.go +++ b/internal/bridges/node.go @@ -216,13 +216,13 @@ func (n *tsnetNode) Close() error { } // newTSNetNode returns the node factory production Machines use. Each node is -// a tsnet.Server on the bridge's state directory, named the way the admin +// a tsnet.Server on the slot's state directory, named the way the admin // console will show it. -func newTSNetNode(debug bool) func(bridge config.Bridge, stateDir string, userLogf, debugLogf func(string, ...any)) tailnetNode { - return func(bridge config.Bridge, stateDir string, userLogf, debugLogf func(string, ...any)) tailnetNode { +func newTSNetNode(debug bool) func(bridge config.Bridge, slot int, stateDir string, userLogf, debugLogf func(string, ...any)) tailnetNode { + return func(bridge config.Bridge, slot int, stateDir string, userLogf, debugLogf func(string, ...any)) tailnetNode { s := &tsnet.Server{ Dir: stateDir, - Hostname: MachineName(bridge.ID), + Hostname: MachineName(bridge.ID, slot), UserLogf: userLogf, } if debug { diff --git a/internal/bridges/remove.go b/internal/bridges/remove.go index f65e0b2..8139229 100644 --- a/internal/bridges/remove.go +++ b/internal/bridges/remove.go @@ -47,25 +47,77 @@ func WillDestroyMachine(g *config.Global, bridge config.Bridge, endpoint config. return true } -// Destroy logs the bridge's Machine out of its tailnet and discards its -// login, waiting at most destroyTimeout for the tailnet to answer. Destroy -// writes no settings. The caller removes the bridge's records only after -// Destroy returns nil: the records are the only thing naming the device, and -// a failed or timed-out logout must stay retryable (ADR 0002). +// Destroy logs out every Machine the bridge has on disk — one per slot a +// process has claimed — and discards their logins, waiting at most +// destroyTimeout for the tailnet to answer. Destroy writes no settings. The +// caller removes the bridge's records only after Destroy returns nil: the +// records are the only thing naming the devices, and a failed or timed-out +// logout must stay retryable (ADR 0002). +// +// Every slot is claimed before any logout runs: a slot another aperture +// process holds means that process is using the bridge, and refusing the +// whole removal beats logging a live session out from under its user. func (ms *Machines) Destroy(ctx context.Context, bridge config.Bridge, emit func(connection.Event)) error { - mc, err := ms.For(bridge) - if err != nil { + if err := validateBridgeID(bridge.ID); err != nil { return err } ctx, cancel := context.WithTimeout(ctx, destroyTimeout) defer cancel() - err = mc.Destroy(ctx, emit) + err := ms.destroySlots(ctx, bridge, emit) if errors.Is(err, context.DeadlineExceeded) { return fmt.Errorf("the tailnet did not answer within %s: %w", destroyTimeout, err) } return err } +// destroySlots does Destroy's work without bounding it. +func (ms *Machines) destroySlots(ctx context.Context, bridge config.Bridge, emit func(connection.Event)) error { + slots, err := config.BridgeStateSlots(bridge.ID) + if err != nil { + return err + } + own := ms.lookup(bridge.ID) + claims := map[int]func(){} + defer func() { + for _, release := range claims { + release() + } + }() + for _, slot := range slots { + if own != nil && slot == own.slot { + continue + } + release, err := claimSlotNumber(bridge.ID, slot) + if errors.Is(err, errSlotHeld) { + return fmt.Errorf("bridge %s is in use by another aperture process; close it there before removing the bridge", bridge.Name) + } + if err != nil { + return err + } + claims[slot] = release + } + if own != nil { + if err := own.Destroy(ctx, emit); err != nil { + return err + } + } + for _, slot := range slots { + release, claimed := claims[slot] + if !claimed { + continue + } + delete(claims, slot) + mc := newMachine(bridge, ms) + mc.slot, mc.release = slot, release + // The temp Machine releases the slot when its Destroy work finishes, + // whatever the outcome; Destroy may return at its deadline first. + if err := mc.Destroy(ctx, emit); err != nil { + return err + } + } + return nil +} + // Tailnet returns the tailnet name the bridge's running Machine reports, // falling back to the name saved on the bridge. A bridge that switched // tailnets this session keeps a stale saved name until the next verified diff --git a/internal/bridges/route_test.go b/internal/bridges/route_test.go index 611be0c..79836da 100644 --- a/internal/bridges/route_test.go +++ b/internal/bridges/route_test.go @@ -28,7 +28,7 @@ func TestRouteDropsClientAuthorization(t *testing.T) { node := &fakeNode{backendAddr: strings.TrimPrefix(backend.URL, "http://")} m := NewMachines(false) - m.newNode = func(_ config.Bridge, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { + m.newNode = func(_ config.Bridge, _ int, _ string, _ func(string, ...any), _ func(string, ...any)) tailnetNode { return node } defer m.Close() diff --git a/internal/bridges/security_test.go b/internal/bridges/security_test.go index f26ce7d..4e9c251 100644 --- a/internal/bridges/security_test.go +++ b/internal/bridges/security_test.go @@ -82,7 +82,7 @@ func TestProxyRequiresExplicitSharedPeerName(t *testing.T) { node := &sharedPeerNode{fakeNode: &fakeNode{status: status}, backend: backend.Listener.Addr().String()} m := NewMachines(false) m.peerWait = 0 - m.newNode = func(config.Bridge, string, func(string, ...any), func(string, ...any)) tailnetNode { return node } + m.newNode = func(config.Bridge, int, string, func(string, ...any), func(string, ...any)) tailnetNode { return node } defer m.Close() for _, target := range []string{"http://ai", "http://ai.attacker-tail.ts.net"} { t.Run(target, func(t *testing.T) { @@ -151,7 +151,7 @@ func TestRunLogOmitsLoginCapabilities(t *testing.T) { r.notify(unhealthyLogin("request failed: " + authURL)) case "backend and startup error": m := NewMachines(true) - m.newNode = func(_ config.Bridge, _ string, userLogf, debugLogf func(string, ...any)) tailnetNode { + m.newNode = func(_ config.Bridge, _ int, _ string, userLogf, debugLogf func(string, ...any)) tailnetNode { userLogf("To authenticate, visit: %s", authURL) debugLogf("Received auth URL: %q", "HTTPS://login.tailscale.com/a/"+secret) return &fakeNode{upErr: errors.New("authorization failed at " + authURL)} diff --git a/internal/bridges/slot.go b/internal/bridges/slot.go new file mode 100644 index 0000000..fc11c9a --- /dev/null +++ b/internal/bridges/slot.go @@ -0,0 +1,71 @@ +package bridges + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/tailscale/aperture-cli/internal/config" +) + +// A Slot is one numbered node identity of a Bridge: its own state directory, +// node key and tailnet hostname. A process claims the lowest free Slot when +// its Machine first starts a node and holds the Slot's lock for as long as +// the Machine may use it. One state directory is one node key, and the +// control plane hands the node to whichever process registered last; the +// lock is what keeps two aperture processes from silently evicting each +// other's session (APT-330). +// +// Locks live outside the state directories they guard, so Destroy can remove +// a directory while its lock file is still held open — on Windows an open +// file inside the directory would make the removal fail. +const maxSlots = 100 + +// errSlotHeld reports a Slot another aperture process has locked. +var errSlotHeld = errors.New("slot is held by another aperture process") + +// claimSlot returns the lowest free slot of the bridge and the function that +// releases it. +func claimSlot(bridgeID string) (int, func(), error) { + for slot := range maxSlots { + release, err := claimSlotNumber(bridgeID, slot+1) + if errors.Is(err, errSlotHeld) { + continue + } + return slot + 1, release, err + } + return 0, nil, fmt.Errorf("more than %d aperture processes on bridge %s", maxSlots, bridgeID) +} + +// claimSlotNumber locks exactly slot, or returns errSlotHeld. +func claimSlotNumber(bridgeID string, slot int) (func(), error) { + dir, err := config.BridgeStateDir(bridgeID, slot) + if err != nil { + return nil, err + } + suffix := strings.TrimPrefix(bridgeID, "bridge-") + locksDir := filepath.Join(filepath.Dir(dir), "locks") + if err := os.MkdirAll(locksDir, 0o700); err != nil { + return nil, err + } + lockPath := filepath.Join(locksDir, fmt.Sprintf("%s-%d.lock", suffix, slot)) + f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return nil, err + } + held, err := tryLockSlot(f) + if err != nil { + f.Close() + return nil, err + } + if !held { + f.Close() + return nil, errSlotHeld + } + return func() { + unlockSlot(f) + f.Close() + }, nil +} diff --git a/internal/bridges/slot_test.go b/internal/bridges/slot_test.go new file mode 100644 index 0000000..62e97a7 --- /dev/null +++ b/internal/bridges/slot_test.go @@ -0,0 +1,225 @@ +package bridges + +import ( + "context" + "os" + "strings" + "sync" + "testing" + + "github.com/tailscale/aperture-cli/internal/config" +) + +// isolateConfig points the bridge state directories at a per-test throwaway +// config dir, on every platform os.UserConfigDir reads. +func isolateConfig(t *testing.T) { + t.Helper() + tmp := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", tmp) + t.Setenv("APPDATA", tmp) +} + +// APT-330: two aperture processes opening the same bridge must not open the +// same state directory. One directory is one node key, and the control plane +// hands the node to the process that registered last, leaving every earlier +// session dialed into a dead node. +func TestConcurrentProcessesGetDistinctSlots(t *testing.T) { + isolateConfig(t) + bridge := config.Bridge{ID: "bridge-aaaa01", Name: "Work"} + var mu sync.Mutex + var dirs []string + newNode := func(_ config.Bridge, _ int, dir string, _, _ func(string, ...any)) tailnetNode { + mu.Lock() + dirs = append(dirs, dir) + mu.Unlock() + return &fakeNode{} + } + // Two Machines collections stand in for two aperture processes. + p1 := NewMachines(false) + p1.newNode = newNode + defer p1.Close() + p2 := NewMachines(false) + p2.newNode = newNode + defer p2.Close() + + m1, err := p1.For(bridge) + if err != nil { + t.Fatal(err) + } + if err := m1.Open(context.Background(), nil); err != nil { + t.Fatal(err) + } + m2, err := p2.For(bridge) + if err != nil { + t.Fatal(err) + } + if err := m2.Open(context.Background(), nil); err != nil { + t.Fatal(err) + } + if len(dirs) != 2 { + t.Fatalf("nodes constructed = %d, want 2", len(dirs)) + } + if dirs[0] == dirs[1] { + t.Fatalf("both processes opened %s: one node key, the second registration evicts the first (APT-330)", dirs[0]) + } +} + +// A closed Machine frees its slot, so the next process to open the bridge +// reuses slot 1 rather than minting a new device on the tailnet. +func TestClosedMachineReleasesItsSlot(t *testing.T) { + isolateConfig(t) + bridge := config.Bridge{ID: "bridge-bbbb02", Name: "Work"} + var mu sync.Mutex + var dirs []string + newNode := func(_ config.Bridge, _ int, dir string, _, _ func(string, ...any)) tailnetNode { + mu.Lock() + dirs = append(dirs, dir) + mu.Unlock() + return &fakeNode{} + } + + p1 := NewMachines(false) + p1.newNode = newNode + m1, err := p1.For(bridge) + if err != nil { + t.Fatal(err) + } + if err := m1.Open(context.Background(), nil); err != nil { + t.Fatal(err) + } + if err := p1.Close(); err != nil { + t.Fatal(err) + } + + p2 := NewMachines(false) + p2.newNode = newNode + defer p2.Close() + m2, err := p2.For(bridge) + if err != nil { + t.Fatal(err) + } + if err := m2.Open(context.Background(), nil); err != nil { + t.Fatal(err) + } + if len(dirs) != 2 { + t.Fatalf("nodes constructed = %d, want 2", len(dirs)) + } + if dirs[0] != dirs[1] { + t.Fatalf("second process opened %s after the first closed %s: slot was not released", dirs[1], dirs[0]) + } +} + +func TestMachineNameNumbersSlotsFromTwo(t *testing.T) { + first := MachineName("bridge-abcdef", 1) + if first != "aperture-cli-bridge-abcdef" { + t.Errorf("MachineName(slot 1) = %q, want the name existing devices already have", first) + } + third := MachineName("bridge-abcdef", 3) + if third != "aperture-cli-bridge-abcdef-3" { + t.Errorf("MachineName(slot 3) = %q, want aperture-cli-bridge-abcdef-3", third) + } +} + +// Destroy logs out every slot the bridge has on disk, not only the one this +// process ran, and removes every slot's state directory. +func TestDestroyLogsOutEverySlot(t *testing.T) { + isolateConfig(t) + bridge := config.Bridge{ID: "bridge-cccc03", Name: "Work"} + var nodes []*fakeNode + newNode := func(_ config.Bridge, _ int, _ string, _, _ func(string, ...any)) tailnetNode { + n := &fakeNode{} + nodes = append(nodes, n) + return n + } + // Two processes ran the bridge at once, leaving two slots on disk. The + // state directory is what tsnet would have created on start; the fake + // node never makes one. + var dirs []string + var past []*Machines + for range 2 { + p := NewMachines(false) + p.newNode = func(b config.Bridge, slot int, dir string, u, d func(string, ...any)) tailnetNode { + dirs = append(dirs, dir) + return newNode(b, slot, dir, u, d) + } + mc, err := p.For(bridge) + if err != nil { + t.Fatal(err) + } + if err := mc.Open(context.Background(), nil); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(dirs[len(dirs)-1], 0o700); err != nil { + t.Fatal(err) + } + past = append(past, p) + } + if dirs[0] == dirs[1] { + t.Fatalf("both processes opened %s", dirs[0]) + } + for _, p := range past { + if err := p.Close(); err != nil { + t.Fatal(err) + } + } + if !HasMachine(bridge.ID) { + t.Fatal("two slots opened, HasMachine = false") + } + + destroyer := NewMachines(false) + destroyer.newNode = newNode + defer destroyer.Close() + if err := destroyer.Destroy(context.Background(), bridge, nil); err != nil { + t.Fatal(err) + } + + loggedOut := 0 + for _, n := range nodes { + loggedOut += n.loggedOut + } + if loggedOut != 2 { + t.Errorf("logouts across slots = %d, want 2", loggedOut) + } + if HasMachine(bridge.ID) { + t.Error("state directories survived Destroy") + } +} + +// Destroy refuses while another process holds a slot: logging its node out +// from under it is the eviction this change exists to stop. +func TestDestroyRefusesSlotHeldByAnotherProcess(t *testing.T) { + isolateConfig(t) + bridge := config.Bridge{ID: "bridge-dddd04", Name: "Work"} + newNode := func(_ config.Bridge, _ int, _ string, _, _ func(string, ...any)) tailnetNode { + return &fakeNode{} + } + p1 := NewMachines(false) + p1.newNode = newNode + defer p1.Close() + mc, err := p1.For(bridge) + if err != nil { + t.Fatal(err) + } + if err := mc.Open(context.Background(), nil); err != nil { + t.Fatal(err) + } + // The state directory tsnet would have created on start. + dir, err := config.BridgeStateDir(bridge.ID, 1) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + + p2 := NewMachines(false) + p2.newNode = newNode + defer p2.Close() + err = p2.Destroy(context.Background(), bridge, nil) + if err == nil || !strings.Contains(err.Error(), "another aperture process") { + t.Fatalf("Destroy error = %v, want refusal naming the process still using the bridge", err) + } + if !HasMachine(bridge.ID) { + t.Error("the refused Destroy still removed state") + } +} diff --git a/internal/bridges/slot_unix.go b/internal/bridges/slot_unix.go new file mode 100644 index 0000000..ce9ba35 --- /dev/null +++ b/internal/bridges/slot_unix.go @@ -0,0 +1,22 @@ +//go:build unix + +package bridges + +import ( + "errors" + "os" + + "golang.org/x/sys/unix" +) + +func tryLockSlot(f *os.File) (bool, error) { + err := unix.Flock(int(f.Fd()), unix.LOCK_EX|unix.LOCK_NB) + if errors.Is(err, unix.EWOULDBLOCK) { + return false, nil + } + return err == nil, err +} + +func unlockSlot(f *os.File) error { + return unix.Flock(int(f.Fd()), unix.LOCK_UN) +} diff --git a/internal/bridges/slot_windows.go b/internal/bridges/slot_windows.go new file mode 100644 index 0000000..05d5f3e --- /dev/null +++ b/internal/bridges/slot_windows.go @@ -0,0 +1,26 @@ +//go:build windows + +package bridges + +import ( + "errors" + "os" + + "golang.org/x/sys/windows" +) + +// One byte of the lock file is enough: the file exists per slot, so its +// content carries nothing. +func tryLockSlot(f *os.File) (bool, error) { + err := windows.LockFileEx(windows.Handle(f.Fd()), + windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, + 0, 1, 0, new(windows.Overlapped)) + if errors.Is(err, windows.ERROR_LOCK_VIOLATION) { + return false, nil + } + return err == nil, err +} + +func unlockSlot(f *os.File) error { + return windows.UnlockFileEx(windows.Handle(f.Fd()), 0, 1, 0, new(windows.Overlapped)) +} diff --git a/internal/config/settings.go b/internal/config/settings.go index 6176957..e00f5ac 100644 --- a/internal/config/settings.go +++ b/internal/config/settings.go @@ -11,6 +11,8 @@ import ( "fmt" "os" "path/filepath" + "slices" + "strconv" "strings" "tailscale.com/atomicfile" @@ -88,8 +90,15 @@ func defaultSettings() Settings { } } -// BridgeStateDir returns the tsnet state directory for a bridge ID. -func BridgeStateDir(id string) (string, error) { +// BridgeStateDir returns the tsnet state directory for one slot of a bridge. +// Every concurrent aperture process running the bridge claims its own slot, +// because one directory is one node key and the control plane hands the node +// to whichever process registered last. Slot 1 keeps the path existing +// bridges already have; further slots get a numeric sibling. +func BridgeStateDir(id string, slot int) (string, error) { + if slot < 1 { + return "", fmt.Errorf("slot %d: slots number from 1", slot) + } dir, err := os.UserConfigDir() if err != nil { return "", err @@ -98,7 +107,43 @@ func BridgeStateDir(id string) (string, error) { if suffix == "" { return "", fmt.Errorf("bridge ID is empty") } - return filepath.Join(dir, "aperture", "bridges", suffix), nil + base := filepath.Join(dir, "aperture", "bridges", suffix) + if slot == 1 { + return base, nil + } + return fmt.Sprintf("%s-%d", base, slot), nil +} + +// BridgeStateSlots returns the numbers of the bridge's slots that have a +// state directory on disk, in order. Removal walks it: every slot is a node +// the bridge registered, and each needs its own logout. +func BridgeStateSlots(id string) ([]int, error) { + base, err := BridgeStateDir(id, 1) + if err != nil { + return nil, err + } + var slots []int + if isDir(base) { + slots = append(slots, 1) + } + matches, err := filepath.Glob(base + "-*") + if err != nil { + return nil, err + } + for _, match := range matches { + slot, err := strconv.Atoi(strings.TrimPrefix(match, base+"-")) + if err != nil || slot < 2 || !isDir(match) { + continue + } + slots = append(slots, slot) + } + slices.Sort(slots) + return slots, nil +} + +func isDir(path string) bool { + info, err := os.Stat(path) + return err == nil && info.IsDir() } func newBridgeID(existing []Bridge) (string, error) { diff --git a/internal/config/state_test.go b/internal/config/state_test.go index 39daca5..9745875 100644 --- a/internal/config/state_test.go +++ b/internal/config/state_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "slices" "strings" "testing" @@ -292,7 +293,7 @@ func TestBridgeStateDir(t *testing.T) { t.Setenv("HOME", tmp) t.Setenv("XDG_CONFIG_HOME", filepath.Join(tmp, ".config")) - got, err := config.BridgeStateDir("bridge-abcdef") + got, err := config.BridgeStateDir("bridge-abcdef", 1) if err != nil { t.Fatal(err) } @@ -304,6 +305,47 @@ func TestBridgeStateDir(t *testing.T) { if got != want { t.Errorf("BridgeStateDir = %q, want %q", got, want) } + + third, err := config.BridgeStateDir("bridge-abcdef", 3) + if err != nil { + t.Fatal(err) + } + if want := want + "-3"; third != want { + t.Errorf("BridgeStateDir(slot 3) = %q, want %q", third, want) + } +} + +func TestBridgeStateSlots(t *testing.T) { + tmp := t.TempDir() + t.Setenv("HOME", tmp) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(tmp, ".config")) + + slots, err := config.BridgeStateSlots("bridge-abcdef") + if err != nil || len(slots) != 0 { + t.Fatalf("BridgeStateSlots(nothing on disk) = %v, %v", slots, err) + } + + base, err := config.BridgeStateDir("bridge-abcdef", 1) + if err != nil { + t.Fatal(err) + } + for _, dir := range []string{base, base + "-10", base + "-2", base + "-junk"} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + // A file with a slot's name is not a slot. + if err := os.WriteFile(base+"-4", []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + + slots, err = config.BridgeStateSlots("bridge-abcdef") + if err != nil { + t.Fatal(err) + } + if !slices.Equal(slots, []int{1, 2, 10}) { + t.Errorf("BridgeStateSlots = %v, want [1 2 10]", slots) + } } func TestClientConfig_TypedStore(t *testing.T) { diff --git a/internal/tui/removal.go b/internal/tui/removal.go index e5b342c..5ebd2cc 100644 --- a/internal/tui/removal.go +++ b/internal/tui/removal.go @@ -2,6 +2,7 @@ package tui import ( "context" + "strings" "time" tea "github.com/charmbracelet/bubbletea" @@ -67,14 +68,15 @@ func (m *model) remove(bridge config.Bridge, ep config.Endpoint) menu.Result { } // removeBridgeMenu asks the user to confirm. Removal is irreversible from -// here and logs a device out of the user's tailnet, so the screen names the -// device the way the admin console does. +// here and logs a device out of the user's tailnet — one per slot a process +// has claimed — so the screen names the devices the way the admin console +// does. func (m *model) removeBridgeMenu(bridge config.Bridge, ep config.Endpoint) *menu.Menu { - preamble := "Bridge " + bridge.Name + " is the device " + bridges.MachineName(bridge.ID) + preamble := "Bridge " + bridge.Name + " is " + devicePhrase(bridge.ID) if name := m.machines.Tailnet(bridge); name != "" { preamble += " on " + name } - preamble += ".\n\nRemoving it logs that device out of the tailnet and discards the login stored on this machine. " + + preamble += ".\n\nRemoving it logs those devices out of the tailnet and discards the logins stored on this machine. " + "Connecting through a bridge of this name again is a new device and a new login." return &menu.Menu{ Title: "Remove bridge " + bridge.Name + "?", @@ -154,19 +156,34 @@ func (m *model) bridgeRemoved(msg bridgeRemovedMsg) (tea.Model, tea.Cmd) { } // removalFailedMessage tells the user the connection is unchanged and how to -// finish the job: retry here, or delete the device by name in the admin -// console. A bare error leaves them hunting for a machine whose name this +// finish the job: retry here, or delete the devices by name in the admin +// console. A bare error leaves them hunting for machines whose names this // program chose. func (m *model) removalFailedMessage(bridge config.Bridge, err error) string { msg := "Could not remove bridge " + bridge.Name + ": " + err.Error() + "\n\nThe connection is unchanged. Removing it again retries the logout. " + - "If the device " + bridges.MachineName(bridge.ID) + "If " + devicePhrase(bridge.ID) if name := m.machines.Tailnet(bridge); name != "" { msg += " is still on " + name } else { msg += " is still registered" } - return msg + " after that, delete it from the Tailscale admin console." + return msg + " after that, delete them from the Tailscale admin console." +} + +// devicePhrase names the bridge's devices the way the admin console does. +// The names come from the state directories on disk; a bridge whose +// directories vanished mid-removal still names its first slot, the device a +// retry would go and find. +func devicePhrase(bridgeID string) string { + names, err := bridges.MachineNames(bridgeID) + if err != nil || len(names) == 0 { + return "the device " + bridges.MachineName(bridgeID, 1) + } + if len(names) == 1 { + return "the device " + names[0] + } + return "the devices " + strings.Join(names, ", ") } // afterRemoval returns the user to a list that no longer shows what they diff --git a/internal/tui/removal_test.go b/internal/tui/removal_test.go index 2ab0a2a..4492e8e 100644 --- a/internal/tui/removal_test.go +++ b/internal/tui/removal_test.go @@ -29,7 +29,7 @@ func withFakeDestroy(t *testing.T, fn func(context.Context, config.Bridge) error // what says this bridge registered a device. func startedBridge(t *testing.T, id string) { t.Helper() - dir, err := config.BridgeStateDir(id) + dir, err := config.BridgeStateDir(id, 1) if err != nil { t.Fatal(err) } @@ -221,7 +221,7 @@ func TestDestroyTimeoutKeepsTheConnectionAndNamesTheDevice(t *testing.T) { if m.step != stepError { t.Errorf("step = %v, want the timeout reported", m.step) } - for _, want := range []string{bridges.MachineName(row.bridge.ID), "corp.example.com", "did not answer"} { + for _, want := range []string{bridges.MachineName(row.bridge.ID, 1), "corp.example.com", "did not answer"} { if !strings.Contains(m.errMsg, want) { t.Errorf("message %q does not name %q", m.errMsg, want) } From 528511dcc524dec11118f4661660b7cf87f757f7 Mon Sep 17 00:00:00 2001 From: Guy J Grigsby Date: Mon, 21 Sep 2026 20:53:54 +0000 Subject: [PATCH 5/8] bridges: authorize fresh Machine slots from TS_AUTHKEY ADR 0006 makes every concurrent process its own device, so a fresh slot needed one browser login each before it could join. With TS_AUTHKEY set, tsnet registers the slot non-interactively; a reusable key covers every slot an agent fleet claims. Once a slot has registered, its state directory carries the credentials and the key is not consulted again for it. --- internal/bridges/node.go | 8 ++++++++ internal/bridges/node_test.go | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+) create mode 100644 internal/bridges/node_test.go diff --git a/internal/bridges/node.go b/internal/bridges/node.go index 638956b..24413d9 100644 --- a/internal/bridges/node.go +++ b/internal/bridges/node.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" "net" + "os" "github.com/tailscale/aperture-cli/internal/config" "github.com/tailscale/aperture-cli/internal/connection" @@ -218,11 +219,18 @@ func (n *tsnetNode) Close() error { // newTSNetNode returns the node factory production Machines use. Each node is // a tsnet.Server on the slot's state directory, named the way the admin // console will show it. +// +// TS_AUTHKEY, when set, authorizes a fresh slot without the browser login: +// every concurrent process is its own device (ADR 0006), so unattended +// sessions need a reusable key or each new slot asks for a login once. Once +// a slot is registered its state directory carries the credentials and the +// key is not consulted again for it. func newTSNetNode(debug bool) func(bridge config.Bridge, slot int, stateDir string, userLogf, debugLogf func(string, ...any)) tailnetNode { return func(bridge config.Bridge, slot int, stateDir string, userLogf, debugLogf func(string, ...any)) tailnetNode { s := &tsnet.Server{ Dir: stateDir, Hostname: MachineName(bridge.ID, slot), + AuthKey: os.Getenv("TS_AUTHKEY"), UserLogf: userLogf, } if debug { diff --git a/internal/bridges/node_test.go b/internal/bridges/node_test.go new file mode 100644 index 0000000..fd828ec --- /dev/null +++ b/internal/bridges/node_test.go @@ -0,0 +1,20 @@ +package bridges + +import ( + "testing" + + "github.com/tailscale/aperture-cli/internal/config" +) + +func TestNewTSNetNodeReadsAuthKeyFromEnv(t *testing.T) { + t.Setenv("TS_AUTHKEY", "tskey-auth-test") + + node := newTSNetNode(false)(config.Bridge{ID: "bridge-abcdef"}, 1, t.TempDir(), nil, nil) + ts, ok := node.(*tsnetNode) + if !ok { + t.Fatalf("newTSNetNode returned %T, want *tsnetNode", node) + } + if ts.server.AuthKey != "tskey-auth-test" { + t.Errorf("AuthKey = %q, want the TS_AUTHKEY value", ts.server.AuthKey) + } +} From de2e718a545c085b9b8c81965083c096dc888f81 Mon Sep 17 00:00:00 2001 From: Guy J Grigsby Date: Mon, 21 Sep 2026 21:14:53 +0000 Subject: [PATCH 6/8] README: document per-process slots and TS_AUTHKEY APT-330 taught users the hard way that concurrent sessions evict each other; the fix (ADR 0006) and the TS_AUTHKEY escape hatch for unattended sessions were only discoverable by reading the code. --- README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/README.md b/README.md index f6ecbf1..0c84657 100644 --- a/README.md +++ b/README.md @@ -78,6 +78,12 @@ A bridge is on one tailnet at a time. `Switch tailnet` logs it out, which remove If verification fails, the endpoint remains configured for retry or editing, and any previous working endpoint remains active. +### Concurrent sessions + +Each aperture process on a bridge is its own device on the tailnet. The first process uses the bridge's original identity; each additional one registers a numbered sibling (`aperture-cli--2` and up). Sharing one identity would let the control plane hand the session to whichever process registered last, silently cutting off the others. + +A fresh device needs one login. Set `TS_AUTHKEY` to a reusable auth key and new devices authorize without the browser; devices already registered keep their credentials on disk and never consult the key again. The key decides which tailnet a fresh device joins, so use a key from the tailnet your Aperture is on. + ### Flags | Flag | Environment | Description | From ba67171280af38585587c07c8b3ac6ee82f47968 Mon Sep 17 00:00:00 2001 From: Guy J Grigsby Date: Mon, 21 Sep 2026 21:17:48 +0000 Subject: [PATCH 7/8] README: list every environment variable in one table The flags table only names the two vars that mirror flags; TS_AUTHKEY, the Codex install vars and the incidental reads were undocumented. --- README.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/README.md b/README.md index 0c84657..03413f9 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,20 @@ Neither is made the saved active endpoint until the connection works, so an unreachable URL passed on the command line does not displace the one that does work. +### Environment variables + +Everything the launcher reads from the environment: + +| Variable | Description | +|----------|-------------| +| `APERTURE_ENDPOINT` | Aperture URL to open on, instead of the saved one. `-endpoint` wins over it. | +| `APERTURE_BRIDGE` | Connect through the bridge with this name, creating it if there is none. `-bridge` wins over it. | +| `TS_AUTHKEY` | Tailscale auth key that authorizes a fresh bridge device without the browser login. Read only when a new device registers; see [Concurrent sessions](#concurrent-sessions). | +| `CODEX_INSTALL_DIR` | Extra directory searched for a standalone Codex binary, matching the Codex installer's own variable. | +| `CODEX_HOME` | Extra Codex home searched for the standalone install layout, matching Codex's own variable. | +| `TMUX`, `TERM` | Detected, not set by you for aperture: picks the escape-sequence wrapping that carries a copied login link through tmux or screen. | +| `LOCALAPPDATA` | Windows only: locates the Claude Desktop configuration. | + ## Development ```sh From b97d3d67499fcea52ee1f547b0509fe5b72be690 Mon Sep 17 00:00:00 2001 From: Guy J Grigsby Date: Tue, 22 Sep 2026 20:34:43 +0000 Subject: [PATCH 8/8] e2e: cover bridge concurrency (ADR 0006) through the built binary The suite skipped bridge flows on the assumption they need a control plane. The slot contract turned out to be local: state dirs, held flocks, and menu screens, all observable without a login ever completing. Four tests, one per ADR 0006 decision: concurrent processes claim distinct numbered slots, a dead process's slot is reclaimed before a new one is minted, removal fails naming the conflict while a slot is live and destroys every slot once it is gone, and the 100-slot cap errors instead of probing. The cap test holds the 100 locks itself because an exclusive flock is exactly what a live process presents, which keeps 101 real processes out of the run. Two harness additions this needed: spawn now reaps parked processes via cleanup (a bridge waiting on login never exits on its own), and hermeticEnv drops TS_AUTHKEY and forces BROWSER=true (bridge attempts reach the login-link phase, so a dev box would otherwise authorize fresh slots against a real tailnet or pop browser tabs during make check). Not covered: two sessions actually reaching Aperture through their slots. That is the data plane and does need credentials the test environment does not have. --- e2e/bridge_concurrency_test.go | 285 +++++++++++++++++++++++++++++++++ e2e/harness.go | 18 +++ 2 files changed, 303 insertions(+) create mode 100644 e2e/bridge_concurrency_test.go diff --git a/e2e/bridge_concurrency_test.go b/e2e/bridge_concurrency_test.go new file mode 100644 index 0000000..0d05878 --- /dev/null +++ b/e2e/bridge_concurrency_test.go @@ -0,0 +1,285 @@ +//go:build unix + +package e2e + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "regexp" + "strings" + "syscall" + "testing" + "time" + + "golang.org/x/sys/unix" +) + +// The bridge concurrency contract (ADR 0006, README "Concurrent sessions") +// tested through the built binary: every expectation here is a file the +// program writes, a lock it holds, or a screen it paints. The data plane is +// out of reach — without tailnet credentials no test finishes a login — so +// "both sessions reach Aperture" is not asserted, only the local half of the +// contract: distinct slots, held locks, blocked removal, the 100-slot cap. + +// isRunning reports whether the spawned process is alive. +func (term *terminal) isRunning() bool { + return term.cmd.Process.Signal(syscall.Signal(0)) == nil +} + +// waitForState polls cond until it holds, failing with what after timeout. +// For assertions on the filesystem, where there is no screen text to wait +// on. Removal gets a long budget: logout is a control-plane round trip. +func waitForState(t *testing.T, what string, timeout time.Duration, cond func() bool) { + t.Helper() + deadline := time.Now().Add(timeout) + for time.Now().Before(deadline) { + if cond() { + return + } + time.Sleep(50 * time.Millisecond) + } + t.Fatalf("timed out waiting for %s", what) +} + +// apertureConfigDir recovers the aperture configuration directory from a +// hermetic environment, so the test can assert on the files the run writes. +func apertureConfigDir(t *testing.T, env []string) string { + t.Helper() + for _, e := range env { + if dir, ok := strings.CutPrefix(e, "XDG_CONFIG_HOME="); ok { + return filepath.Join(dir, "aperture") + } + } + t.Fatal("environment has no XDG_CONFIG_HOME") + return "" +} + +// lockPath is the lock file for one slot of the bridge, e.g. +// bridges/locks/70da61-2.lock. +func lockPath(configDir, suffix string, slot int) string { + return filepath.Join(configDir, "bridges", "locks", fmt.Sprintf("%s-%d.lock", suffix, slot)) +} + +// slotSuffix reads the bridge's slot prefix back off disk: the name of its +// slot-1 lock file with "-1.lock" trimmed, e.g. locks/70da61-1.lock → 70da61. +func slotSuffix(configDir string) (string, bool) { + matches, _ := filepath.Glob(lockPath(configDir, "*", 1)) + if len(matches) == 0 { + return "", false + } + return strings.TrimSuffix(filepath.Base(matches[0]), "-1.lock"), true +} + +// waitForFirstSlot waits for a process to claim slot 1 and returns the +// bridge's slot suffix. +func waitForFirstSlot(t *testing.T, configDir string) string { + t.Helper() + var suffix string + waitForState(t, "slot 1 to be claimed", 15*time.Second, func() bool { + s, ok := slotSuffix(configDir) + if !ok { + return false + } + suffix = s + return isLocked(t, lockPath(configDir, suffix, 1)) + }) + return suffix +} + +// isLocked reports whether the slot lock file at path is held by a live +// process, by attempting the same non-blocking exclusive flock the slot +// claim takes (ADR 0006 decision 2). A missing file is not held. +func isLocked(t *testing.T, path string) bool { + t.Helper() + f, err := os.Open(path) + if errors.Is(err, fs.ErrNotExist) { + return false + } + if err != nil { + t.Fatalf("open %s: %v", path, err) + } + defer f.Close() + switch err = unix.Flock(int(f.Fd()), unix.LOCK_EX|unix.LOCK_NB); { + case err == nil: + if err := unix.Flock(int(f.Fd()), unix.LOCK_UN); err != nil { + t.Fatalf("unlock %s: %v", path, err) + } + return false + case errors.Is(err, unix.EWOULDBLOCK): + return true + default: + t.Fatalf("probe %s: %v", path, err) + return false + } +} + +// removeBridgeViaMenu drives a freshly spawned launcher from the +// getting-started screen to the bridge removal confirmation: connection +// options, the endpoint row reached through bridge "work", remove, confirm. +func removeBridgeViaMenu(t *testing.T, term *terminal) { + t.Helper() + term.waitFor(t, "Retry connection") + term.send("3") + term.waitFor(t, "Aperture Endpoints") + row := regexp.MustCompile(`\[(\d+)\][^\n]*via work`).FindStringSubmatch(term.screen()) + if row == nil { + t.Fatalf("no endpoint via work in:\n%s", term.screen()) + } + term.send(row[1]) + term.waitFor(t, "Remove connection") + term.send("4") + term.waitFor(t, "Remove bridge work?") + term.send("y") +} + +// ADR 0006 decisions 1 and 2: each concurrent process on a bridge claims its +// own numbered slot — its own state directory and held lock — instead of +// every process registering the same node key and letting the control plane +// hand the session to whichever registered last. +func TestConcurrentProcessesGetOwnSlots(t *testing.T) { + env := hermeticEnv(t, "") + dir := apertureConfigDir(t, env) + + first := spawn(t, apertureBin, []string{"-bridge", "work"}, env) + suffix := waitForFirstSlot(t, dir) + + second := spawn(t, apertureBin, []string{"-bridge", "work"}, env) + waitForState(t, "second process to claim slot 2", 15*time.Second, func() bool { + return isLocked(t, lockPath(dir, suffix, 2)) + }) + + if !first.isRunning() || !second.isRunning() { + t.Fatalf("both processes stay running: first=%v second=%v", first.isRunning(), second.isRunning()) + } + if !isLocked(t, lockPath(dir, suffix, 1)) { + t.Error("second process released the first's slot") + } + for _, d := range []string{suffix, suffix + "-2"} { + if _, err := os.Stat(filepath.Join(dir, "bridges", d)); err != nil { + t.Errorf("state directory %s: %v", d, err) + } + } +} + +// ADR 0006 decision 2: a slot is held for the node's life and released when +// the process dies, so the next process claims the lowest free slot rather +// than minting a new device per launch. +func TestSlotReusedAfterProcessDeath(t *testing.T) { + env := hermeticEnv(t, "") + dir := apertureConfigDir(t, env) + + spawn(t, apertureBin, []string{"-bridge", "work"}, env) + suffix := waitForFirstSlot(t, dir) + + second := spawn(t, apertureBin, []string{"-bridge", "work"}, env) + waitForState(t, "second process to claim slot 2", 15*time.Second, func() bool { + return isLocked(t, lockPath(dir, suffix, 2)) + }) + + second.kill() + waitForState(t, "slot 2 to be released", 10*time.Second, func() bool { + return !isLocked(t, lockPath(dir, suffix, 2)) + }) + + spawn(t, apertureBin, []string{"-bridge", "work"}, env) + waitForState(t, "third process to reclaim slot 2", 15*time.Second, func() bool { + return isLocked(t, lockPath(dir, suffix, 2)) + }) + if _, err := os.Stat(lockPath(dir, suffix, 3)); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("slot 3 claimed while slot 2 was free (stat: %v)", err) + } + if _, err := os.Stat(filepath.Join(dir, "bridges", suffix+"-3")); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("state directory for slot 3 created while slot 2 was free (stat: %v)", err) + } +} + +// ADR 0006 decision 3: removing a bridge whose slot is held by a live +// process fails before anything is logged out, naming the conflict; once +// the process is gone, the same removal destroys every slot. +func TestRemovalBlockedByLiveProcess(t *testing.T) { + env := hermeticEnv(t, "") + dir := apertureConfigDir(t, env) + + holder := spawn(t, apertureBin, []string{"-bridge", "work"}, env) + suffix := waitForFirstSlot(t, dir) + + // The menu instance gets an unreachable endpoint so its startup attempt + // fails fast everywhere, tailnet or not, and lands on the menu. + menuEnv := append([]string{"APERTURE_ENDPOINT=http://127.0.0.1:9"}, env...) + menu := spawn(t, apertureBin, nil, menuEnv) + removeBridgeViaMenu(t, menu) + menu.waitFor(t, "in use by another aperture process") + + settings := filepath.Join(dir, "settings.json") + if b, err := os.ReadFile(settings); err != nil || !strings.Contains(string(b), suffix) { + t.Errorf("blocked removal changed settings: %v", err) + } + if _, err := os.Stat(filepath.Join(dir, "bridges", suffix)); err != nil { + t.Error("blocked removal discarded the state directory:", err) + } + if !isLocked(t, lockPath(dir, suffix, 1)) { + t.Error("blocked removal released the live process's slot") + } + + holder.kill() + waitForState(t, "slot 1 to be released", 10*time.Second, func() bool { + return !isLocked(t, lockPath(dir, suffix, 1)) + }) + + // Driven the same way with no live process, removal now destroys the + // bridge. + retry := spawn(t, apertureBin, nil, menuEnv) + removeBridgeViaMenu(t, retry) + waitForState(t, "bridge removal to finish", 90*time.Second, func() bool { + b, err := os.ReadFile(settings) + if err != nil || strings.Contains(string(b), suffix) { + return false + } + _, err = os.Stat(filepath.Join(dir, "bridges", suffix)) + return errors.Is(err, fs.ErrNotExist) + }) + if b, err := os.ReadFile(settings); err != nil || !strings.Contains(string(b), "http://ai") { + t.Errorf("removal took the direct endpoints with it: %v", err) + } +} + +// ADR 0006 decision 4: past 100 claimed slots the process errors instead of +// probing forever. The test holds the locks itself — an exclusive flock on +// the slot file is exactly what a live process presents (decision 2). +func TestSlotCap(t *testing.T) { + env := hermeticEnv(t, "") + dir := apertureConfigDir(t, env) + + first := spawn(t, apertureBin, []string{"-bridge", "work"}, env) + suffix := waitForFirstSlot(t, dir) + first.kill() + waitForState(t, "slot 1 to be released", 10*time.Second, func() bool { + return !isLocked(t, lockPath(dir, suffix, 1)) + }) + + var held []*os.File + t.Cleanup(func() { + for _, f := range held { + f.Close() + } + }) + for i := range 100 { + f, err := os.OpenFile(lockPath(dir, suffix, i+1), os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + t.Fatalf("create slot %d lock: %v", i+1, err) + } + if err := unix.Flock(int(f.Fd()), unix.LOCK_EX|unix.LOCK_NB); err != nil { + t.Fatalf("hold slot %d: %v", i+1, err) + } + held = append(held, f) + } + + capped := spawn(t, apertureBin, []string{"-bridge", "work"}, env) + capped.waitFor(t, "more than 100 aperture processes on bridge") + if _, err := os.Stat(lockPath(dir, suffix, 101)); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("slot 101 claimed past the cap (stat: %v)", err) + } +} diff --git a/e2e/harness.go b/e2e/harness.go index ece45ac..b084817 100644 --- a/e2e/harness.go +++ b/e2e/harness.go @@ -37,6 +37,7 @@ func spawn(t *testing.T, bin string, args []string, env []string) *terminal { t.Fatalf("spawning %s: %v", bin, err) } term := &terminal{cmd: cmd, ptmx: ptmx} + t.Cleanup(term.kill) go func() { var buf [4096]byte for { @@ -54,6 +55,18 @@ func spawn(t *testing.T, bin string, args []string, env []string) *terminal { return term } +// kill ends the process if it has not already exited. Terminals parked on a +// screen that never exits (a bridge waiting on login) are reaped here; in +// suites that end in waitExit every call is a no-op. +func (term *terminal) kill() { + if term.cmd.Process == nil { + return + } + _ = term.cmd.Process.Kill() + _ = term.cmd.Wait() + _ = term.ptmx.Close() +} + // send types keys into the terminal. func (term *terminal) send(keys string) { if _, err := term.ptmx.WriteString(keys); err != nil { @@ -129,9 +142,13 @@ func run(t *testing.T, bin string, env []string, args ...string) (string, string func hermeticEnv(t *testing.T, binDir string) []string { t.Helper() home := t.TempDir() + // TS_AUTHKEY would authorize fresh bridge slots against the developer's + // real tailnet; BROWSER is set to true below rather than dropped because + // the login-link opener runs $BROWSER, and true is a no-op. drop := map[string]bool{ "HOME": true, "XDG_CONFIG_HOME": true, "TERM": true, "PATH": true, "APERTURE_ENDPOINT": true, "APERTURE_BRIDGE": true, + "TS_AUTHKEY": true, "BROWSER": true, } var env []string for _, e := range os.Environ() { @@ -149,6 +166,7 @@ func hermeticEnv(t *testing.T, binDir string) []string { "XDG_CONFIG_HOME="+filepath.Join(home, ".config"), "TERM=dumb", "PATH="+path, + "BROWSER=true", ) }