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 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/README.md b/README.md index f6ecbf1..03413f9 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 | @@ -101,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 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 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/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/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..b084817 --- /dev/null +++ b/e2e/harness.go @@ -0,0 +1,211 @@ +// 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} + t.Cleanup(term.kill) + 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 +} + +// 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 { + // 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() + // 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() { + 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, + "BROWSER=true", + ) +} + +// 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 ) 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..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" @@ -216,13 +217,20 @@ 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 { +// +// 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), + 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) + } +} 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) }