diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md index 2a0b524..2d276fe 100644 --- a/.agents/skills/code-review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -1,12 +1,12 @@ --- name: code-review -description: Review the changes since a fixed point (commit, branch, tag, or merge-base) along two axes — Standards (does the code follow this repo's documented coding standards?) and Spec (does the code match what the originating issue/PRD asked for?). Runs both reviews in parallel sub-agents and reports them side by side. Use when the user wants to review a branch, a PR, work-in-progress changes, or asks to "review since X". +description: Review the changes since a fixed point (commit, branch, tag, or merge-base) along two axes — Standards (does the code follow this repo's documented coding standards?) and Spec (does the code match what the originating issue/spec asked for?). Runs both reviews in parallel sub-agents and reports them side by side. Use when the user wants to review a branch, a PR, work-in-progress changes, or asks to "review since X". --- Two-axis review of the diff between `HEAD` and a fixed point the user supplies: - **Standards** — does the code conform to this repo's documented coding standards? -- **Spec** — does the code faithfully implement the originating issue / PRD / spec? +- **Spec** — does the code faithfully implement the originating issue / spec? Both axes run as **parallel sub-agents** so they don't pollute each other's context, then this skill aggregates their findings. @@ -28,7 +28,7 @@ Look for the originating spec, in this order: 1. Issue references in the commit messages (`#123`, `Closes #45`, GitLab `!67`, etc.) — fetch via the workflow in `docs/agents/issue-tracker.md`. 2. A path the user passed as an argument. -3. A PRD/spec file under `docs/`, `specs/`, or `.scratch/` matching the branch name or feature. +3. A spec file under `docs/`, `specs/`, or `.scratch/` matching the branch name or feature. 4. If nothing is found, ask the user where the spec is. If they say there isn't one, the **Spec** sub-agent will skip and report "no spec available". ### 3. Identify the standards sources @@ -57,8 +57,6 @@ Each smell reads *what it is* → *how to fix*; match it against the diff: ### 4. Spawn both sub-agents in parallel -Send a single message with two `Agent` tool calls. Use the `general-purpose` subagent for both. - **Standards sub-agent prompt** — include: - The full diff command and commit list. diff --git a/.agents/skills/setup-matt-pocock-skills/SKILL.md b/.agents/skills/setup-matt-pocock-skills/SKILL.md index aecf855..4cbdd0f 100644 --- a/.agents/skills/setup-matt-pocock-skills/SKILL.md +++ b/.agents/skills/setup-matt-pocock-skills/SKILL.md @@ -37,7 +37,7 @@ Lead each section with the recommended answer so the user can accept it in a wor **Section A — Issue tracker.** -> Explainer: The "issue tracker" is where issues live for this repo. Skills like `to-tickets`, `triage`, `to-spec`, and `qa` read from and write to it — they need to know whether to call `gh issue create`, write a markdown file under `.scratch/`, or follow some other workflow you describe. Pick the place you actually track work for this repo. +> Explainer: The "issue tracker" is where issues live for this repo. Skills like `to-tickets`, `triage`, and `to-spec` read from and write to it — they need to know whether to call `gh issue create`, write a markdown file under `.scratch/`, or follow some other workflow you describe. Pick the place you actually track work for this repo. Default posture: these skills were designed for GitHub. If a `git remote` points at GitHub, propose that. If a `git remote` points at GitLab (`gitlab.com` or a self-hosted host), propose GitLab. Otherwise (or if the user prefers), offer: diff --git a/.agents/skills/setup-matt-pocock-skills/issue-tracker-github.md b/.agents/skills/setup-matt-pocock-skills/issue-tracker-github.md index 82cfbf5..bf595e2 100644 --- a/.agents/skills/setup-matt-pocock-skills/issue-tracker-github.md +++ b/.agents/skills/setup-matt-pocock-skills/issue-tracker-github.md @@ -1,6 +1,6 @@ # Issue tracker: GitHub -Issues and PRDs for this repo live as GitHub issues. Use the `gh` CLI for all operations. +Issues and specs for this repo live as GitHub issues. Use the `gh` CLI for all operations. ## Conventions diff --git a/.agents/skills/setup-matt-pocock-skills/issue-tracker-gitlab.md b/.agents/skills/setup-matt-pocock-skills/issue-tracker-gitlab.md index 8a54714..95e49cb 100644 --- a/.agents/skills/setup-matt-pocock-skills/issue-tracker-gitlab.md +++ b/.agents/skills/setup-matt-pocock-skills/issue-tracker-gitlab.md @@ -1,6 +1,6 @@ # Issue tracker: GitLab -Issues and PRDs for this repo live as GitLab issues. Use the [`glab`](https://gitlab.com/gitlab-org/cli) CLI for all operations. +Issues and specs for this repo live as GitLab issues. Use the [`glab`](https://gitlab.com/gitlab-org/cli) CLI for all operations. ## Conventions diff --git a/.agents/skills/setup-matt-pocock-skills/issue-tracker-local.md b/.agents/skills/setup-matt-pocock-skills/issue-tracker-local.md index 6b124b3..fbda5e0 100644 --- a/.agents/skills/setup-matt-pocock-skills/issue-tracker-local.md +++ b/.agents/skills/setup-matt-pocock-skills/issue-tracker-local.md @@ -1,6 +1,6 @@ # Issue tracker: Local Markdown -Issues and specs (you may know a spec as a PRD) for this repo live as markdown files in `.scratch/`. +Issues and specs for this repo live as markdown files in `.scratch/`. ## Conventions diff --git a/.agents/skills/to-spec/SKILL.md b/.agents/skills/to-spec/SKILL.md index f3cca8d..3fd6495 100644 --- a/.agents/skills/to-spec/SKILL.md +++ b/.agents/skills/to-spec/SKILL.md @@ -4,7 +4,7 @@ description: Turn the current conversation into a spec and publish it to the pro disable-model-invocation: true --- -This skill takes the current conversation context and codebase understanding and produces a spec (you may know this document as a PRD). Do NOT interview the user — just synthesize what you already know. +This skill takes the current conversation context and codebase understanding and produces a spec. Do NOT interview the user — just synthesize what you already know. The issue tracker and triage label vocabulary should have been provided to you — run `/setup-matt-pocock-skills` if not. diff --git a/.agents/skills/triage/SKILL.md b/.agents/skills/triage/SKILL.md index be47b78..e67b7bb 100644 --- a/.agents/skills/triage/SKILL.md +++ b/.agents/skills/triage/SKILL.md @@ -73,7 +73,7 @@ Show counts and a one-line summary per item. Let the maintainer pick. 3. **Verify the claim.** Before any grilling, check that the claim holds up. For a bug, reproduce it from the reporter's steps. For a PR, confirm the diff does what it claims — check it out, run the relevant tests or commands. Report what happened: confirmed (with code path), failed, or insufficient detail (a strong `needs-info` signal). A confirmed verification makes a much stronger agent brief. -4. **Grill (if needed).** If the request needs fleshing out, run the `/grilling` and `/domain-modeling` skills together — grill it into shape one question at a time, sharpening domain terms and updating `CONTEXT.md`/ADRs inline as decisions land. +4. **Grill (if needed).** If the request needs fleshing out, run the `/grilling` and `/domain-modeling` skills together — grill it into shape a round of questions at a time, sharpening domain terms and updating `CONTEXT.md`/ADRs inline as decisions land. 5. **Apply the outcome:** - `ready-for-agent` — post an agent brief comment ([AGENT-BRIEF.md](AGENT-BRIEF.md)). diff --git a/.agents/skills/wayfinder/SKILL.md b/.agents/skills/wayfinder/SKILL.md index 42e3644..e4984ed 100644 --- a/.agents/skills/wayfinder/SKILL.md +++ b/.agents/skills/wayfinder/SKILL.md @@ -14,7 +14,7 @@ Wayfinder is **planning** by default: each ticket resolves a decision, and the m ## Refer by name -Every map and ticket is an issue, so it has a **name** — its title. In everything the human reads — narration, the map's Decisions-so-far — refer to it by that name, never by a bare id, number, or slug. A wall of `#42, #43, #44` is illegible; names read at a glance. The id and URL don't vanish — a name wraps its link — but they ride *inside* the name, never stand in for it. +Every map and ticket is an issue, so it has a **name** — its title. In everything the human reads — narration, the map's Decisions-so-far — refer to it by that name, never by a bare id, number, or slug. A wall of `#42, #43, #44` is illegible; names read at a glance. The id and URL don't vanish — a name wraps its link — but they ride _inside_ the name, never stand in for it. ## The Map @@ -72,12 +72,12 @@ The answer isn't part of the body — it's recorded on resolution (see [Work thr ## Ticket Types -Every ticket is either **HITL** — human in the loop, worked *with* a human who speaks for themselves — or **AFK**, driven by the agent alone. A HITL ticket only resolves through that live exchange; the agent never stands in for the human's side of it (a grilling agent that answers its own questions has broken this). +Every ticket is either **HITL** — human in the loop, worked _with_ a human who speaks for themselves — or **AFK**, driven by the agent alone. A HITL ticket only resolves through that live exchange; the agent never stands in for the human's side of it (a grilling agent that answers its own questions has broken this). - **Research** (AFK): Reading documentation, third-party APIs, or local resources like knowledge bases to surface a fact a decision waits on. Resolved by a `/research` **subagent**. Use when knowledge outside the current working directory is required. - **Prototype** (HITL): Raise the fidelity of the discussion by making a cheap, rough, concrete artifact to react to — an outline, a rough take, a stub, or UI/logic code via the /prototype skill. Links the prototype as an asset. Use when "how should it look" or "how should it behave" is the key question. -- **Grilling** (HITL): Conversation via the /grilling and /domain-modeling skills, one question at a time. The default case. -- **Task** (HITL or AFK): Manual work that must happen before a *decision* can be made — nothing to decide, prototype, or research, but the discussion is blocked until it's done. Signing up for a service so its API can be judged, provisioning access, moving data so its shape can be seen. This is the one type that *does* rather than decides — and it earns its place by unblocking a decision, not by delivering the destination. The agent drives it alone where it can (AFK); otherwise it hands the human a precise checklist (HITL). Resolved when the work is done; the answer records what was done and any resulting facts (credentials location, new URLs, row counts) later tickets depend on. +- **Grilling** (HITL): Conversation. The default case. Always invoke the /grilling and /domain-modeling skills. +- **Task** (HITL or AFK): Manual work that must happen before a _decision_ can be made — nothing to decide, prototype, or research, but the discussion is blocked until it's done. Signing up for a service so its API can be judged, provisioning access, moving data so its shape can be seen. This is the one type that _does_ rather than decides — and it earns its place by unblocking a decision, not by delivering the destination. The agent drives it alone where it can (AFK); otherwise it hands the human a precise checklist (HITL). Resolved when the work is done; the answer records what was done and any resulting facts (credentials location, new URLs, row counts) later tickets depend on. ## Fog of war diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml index 63113f0..4bb6304 100644 --- a/.github/workflows/pr-checks.yml +++ b/.github/workflows/pr-checks.yml @@ -103,6 +103,22 @@ jobs: go vet ./... + - name: Cross-build the shell for every supported platform + # The gate below runs on Linux only, so a file behind a build tag for + # another platform is never compiled by it. The session lock is the + # first such seam, and a Windows-only file that stops compiling would + # otherwise reach a release unnoticed. + if: needs.changes.outputs.shell == 'true' + shell: bash + run: | + set -euo pipefail + + for target in windows/amd64 darwin/amd64 darwin/arm64; do + echo "Building for ${target}" + GOOS="${target%%/*}" GOARCH="${target##*/}" go build ./... + GOOS="${target%%/*}" GOARCH="${target##*/}" go vet ./... + done + - name: Lint shell if: needs.changes.outputs.shell == 'true' uses: golangci/golangci-lint-action@v9 diff --git a/docs/adr/0005-os-advisory-lock-for-session-rotation.md b/docs/adr/0005-os-advisory-lock-for-session-rotation.md new file mode 100644 index 0000000..fd5f3e3 --- /dev/null +++ b/docs/adr/0005-os-advisory-lock-for-session-rotation.md @@ -0,0 +1,73 @@ +# ADR 0005: OS Advisory Lock for Session Rotation + +**Status:** Accepted + +Refresh-token rotation must stay single-writer across concurrent `wso2` +invocations: a rotating issuer retires the token it was presented, so two +processes refreshing one session leave the store holding a token the issuer has +already replaced, and the next command refuses `auth.login_required`. The lock +that guarantees this is an OS advisory lock — `flock(2)` on Unix, +`LockFileEx` on Windows, reached through `golang.org/x/sys` behind a +`tryLock` seam. The kernel ties ownership to an open file descriptor and +releases it when the process exits however it exits, so an abandoned lock is +impossible by construction and there is no staleness to detect. + +Two consequences look like defects and are not. + +**The lock file is created once and never unlinked** — releasing closes the +descriptor and leaves the file in place. Removing it is the original bug in a +new costume: a waiter that already holds a descriptor to that inode would lock +a file no longer at the path, and a second process creating a fresh inode there +would lock successfully alongside it. The files are empty and bounded by the +number of credential references, so nothing accumulates that matters. Do not +add cleanup. + +**A held lock reports `auth.login_required`**, not a distinct busy code, +because the stable problem-code list is closed and the recovery — retry the +command — is identical. Whether that list may grow is a separate open question +and not a property of the locking mechanism. + +## Considered Options + +- The original design made the lock the *existence* of a file, with an + `O_EXCL` create to claim it and an age-based takeover so a crashed process + could not block the reference forever. `O_EXCL` is atomic, but the takeover + was `Stat` then `Remove` then create, with nothing atomic across the three: + two processes that both saw the same abandoned file could each evict the + other's live lock and both enter the critical section. No amount of + repair fixes a heuristic whose job disappears entirely under a kernel lock. +- `github.com/gofrs/flock` covers both platforms behind one API and would have + avoided a build-tagged seam, but it would add a genuinely new module to a + graph that deliberately carried four direct dependencies at the time of this + decision. `golang.org/x/sys` was already present as an indirect dependency at + the version required, so promoting it to direct adds nothing to the graph and + costs roughly thirty lines per platform. +- A blocking acquire would be simpler than a non-blocking attempt on a retry + loop, but it discards the bounded wait: the shell reports a busy session as a + typed, recoverable refusal rather than hanging on a peer that may never + finish. + +## Consequences + +The waiting deadline moved from 5 to 45 seconds. Under the previous +implementation a waiter never truly blocked on a healthy holder, so 5 seconds +was never tested against a real critical section. That section now spans a +token refresh round trip to the issuer, which the `auth` package bounds at 30 +seconds, so the waiter's patience has to stay strictly greater than that bound: +at equal values a holder that is merely slow outlives the waiter, and an +ordinary wait becomes a spurious refusal. The two constants live in packages +that cannot import each other, so the coupling is carried by a comment on each +— raising one means raising the other. + +During an upgrade, an old binary and a new one do not interlock. The old one +takes the lock by creating the file and releases it by removing the file, which +the new one is holding a kernel lock on; once that file is gone, the next new +invocation creates a fresh inode at the same path and locks it successfully +alongside the first. The window is any machine where two CLI versions coexist — +a pinned install, a CI image mid-rollout — and it closes once the old binary is +gone. Nothing in the design can prevent it, because the old binary's behavior is +already fixed; it is recorded here so the double-entry is recognised as the +rollout artifact it is rather than a defect in this lock. + +Advisory locks are unreliable on NFS-mounted filesystems. The state root is a +local user directory and a networked home directory is out of scope. diff --git a/go.mod b/go.mod index 5e7a127..c21c9db 100644 --- a/go.mod +++ b/go.mod @@ -7,11 +7,11 @@ require ( github.com/go-jose/go-jose/v4 v4.1.4 github.com/zalando/go-keyring v0.2.8 golang.org/x/oauth2 v0.36.0 + golang.org/x/sys v0.27.0 ) require ( github.com/danieljoos/wincred v1.2.3 // indirect github.com/godbus/dbus/v5 v5.2.2 // indirect - golang.org/x/sys v0.27.0 // indirect google.golang.org/protobuf v1.36.11 // indirect ) diff --git a/internal/auth/session/lock.go b/internal/auth/session/lock.go index 4536822..144b6e9 100644 --- a/internal/auth/session/lock.go +++ b/internal/auth/session/lock.go @@ -17,8 +17,6 @@ package session import ( - "errors" - "io/fs" "os" "path/filepath" "time" @@ -27,11 +25,18 @@ import ( const ( // lockRetryInterval is how often a waiting writer re-attempts the lock. lockRetryInterval = 25 * time.Millisecond - // lockDeadline is how long a writer waits before giving up. - lockDeadline = 5 * time.Second - // lockStaleAfter is the age past which a held lock is presumed abandoned - // by a crashed process and taken over. - lockStaleAfter = 30 * time.Second + // lockDeadline is how long a writer waits before giving up. The critical + // section spans a token refresh round trip to the issuer, so the wait has + // to outlast a slow deployment rather than a local file operation. + // + // It must stay strictly greater than auth.grantDeadline, which bounds that + // round trip: at equal values a holder that is merely slow outlives the + // waiter's patience, and an ordinary wait becomes a spurious refusal. The + // constant is not derived from auth.grantDeadline because auth imports this + // package, so the coupling is stated here instead — raising one means + // raising the other. The margin covers the keyring read and write that + // bracket the round trip inside the same critical section. + lockDeadline = 45 * time.Second ) // WithLock runs fn while holding the per-reference advisory file lock. @@ -49,32 +54,32 @@ func (s Store) WithLock(ref string, fn func() error) error { return fn() } -// acquireLock takes the advisory lock at path, retrying until the deadline. +// acquireLock takes the kernel advisory lock on the file at path, retrying the +// non-blocking attempt until the deadline. // -// The lock is the existence of the file: O_EXCL creation is atomic on every -// filesystem the shell targets. A lock file older than lockStaleAfter is -// treated as abandoned — removed once, then creation is retried like any -// other attempt. +// The lock is held by the kernel against the open file descriptor, not by the +// existence of the file, so the file is created once and never unlinked: +// release closes the descriptor and leaves the file in place. Removing it +// would reintroduce the race it replaced — a waiter holding a descriptor to +// the old inode and a newcomer creating a fresh one at the same path would +// both lock successfully. See ADR 0005. func acquireLock(path string) (release func(), err error) { + file, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return nil, lockFailed() + } deadline := time.Now().Add(lockDeadline) - tookOverStale := false for { - file, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) - if err == nil { + locked, lockErr := tryLock(file) + if lockErr != nil { _ = file.Close() - return func() { _ = os.Remove(path) }, nil - } - if !errors.Is(err, fs.ErrExist) { return nil, lockFailed() } - if !tookOverStale { - if info, statErr := os.Stat(path); statErr == nil && time.Since(info.ModTime()) > lockStaleAfter { - tookOverStale = true - _ = os.Remove(path) - continue - } + if locked { + return func() { _ = file.Close() }, nil } if time.Now().After(deadline) { + _ = file.Close() return nil, lockBusy() } time.Sleep(lockRetryInterval) diff --git a/internal/auth/session/lock_unix.go b/internal/auth/session/lock_unix.go new file mode 100644 index 0000000..afc6c02 --- /dev/null +++ b/internal/auth/session/lock_unix.go @@ -0,0 +1,45 @@ +// Copyright (c) 2026, WSO2 LLC. (https://www.wso2.com). +// +// WSO2 LLC. licenses this file to you under the Apache License, +// Version 2.0 (the "License"); you may not use this file except +// in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//go:build unix + +package session + +import ( + "errors" + "os" + + "golang.org/x/sys/unix" +) + +// tryLock takes an exclusive advisory lock on file without blocking. It +// reports whether the lock was taken; a peer already holding it is contention, +// not an error, and is reported as (false, nil). +// +// The kernel ties the lock to the open file description and drops it when the +// process exits however it exits, so a lock held by a crashed process cannot +// outlive it. +func tryLock(file *os.File) (bool, error) { + err := unix.Flock(int(file.Fd()), unix.LOCK_EX|unix.LOCK_NB) + switch { + case err == nil: + return true, nil + case errors.Is(err, unix.EWOULDBLOCK): + return false, nil + default: + return false, err + } +} diff --git a/internal/auth/session/lock_windows.go b/internal/auth/session/lock_windows.go new file mode 100644 index 0000000..1cc6f37 --- /dev/null +++ b/internal/auth/session/lock_windows.go @@ -0,0 +1,52 @@ +// Copyright (c) 2026, WSO2 LLC. (https://www.wso2.com). +// +// WSO2 LLC. licenses this file to you under the Apache License, +// Version 2.0 (the "License"); you may not use this file except +// in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//go:build windows + +package session + +import ( + "errors" + "os" + + "golang.org/x/sys/windows" +) + +// tryLock takes an exclusive advisory lock on file without blocking. It +// reports whether the lock was taken; a peer already holding it is contention, +// not an error, and is reported as (false, nil). +// +// Windows ties the lock to the file handle and releases it when the handle is +// closed, including the implicit close the kernel performs when a process +// dies, so a lock held by a crashed process cannot outlive it. +func tryLock(file *os.File) (bool, error) { + var overlapped windows.Overlapped + err := windows.LockFileEx( + windows.Handle(file.Fd()), + windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY, + 0, + 1, 0, // lock one byte: the range need only be non-empty. + &overlapped, + ) + switch { + case err == nil: + return true, nil + case errors.Is(err, windows.ERROR_LOCK_VIOLATION), errors.Is(err, windows.ERROR_IO_PENDING): + return false, nil + default: + return false, err + } +} diff --git a/internal/auth/session/session_test.go b/internal/auth/session/session_test.go index b68c699..d32e039 100644 --- a/internal/auth/session/session_test.go +++ b/internal/auth/session/session_test.go @@ -18,8 +18,11 @@ package session_test import ( "errors" + "fmt" "os" + "os/exec" "path/filepath" + "runtime" "sync" "sync/atomic" "testing" @@ -159,29 +162,164 @@ func TestWithLockReleasesOnError(t *testing.T) { } } -func TestWithLockTakesOverStaleLock(t *testing.T) { +// writersPerRound is the burst size one round throws at a single reference. +const writersPerRound = 32 + +// TestWithLockNeverOverlapsWithAbandonedLockFile pins the property the lock +// exists for: no two writers on one credential reference run fn at the same +// time, including when a lock file left behind by a crashed process is already +// sitting at the path. +// +// Each round also asserts that every writer eventually ran. Exclusion on its +// own is a weak oracle — a lock that admits one writer and refuses the other +// thirty-one satisfies it — and that is exactly what a contended acquisition +// that stops retrying looks like. +func TestWithLockNeverOverlapsWithAbandonedLockFile(t *testing.T) { keyring.MockInit() + // Each round is an independent burst of writers arriving together on a + // reference whose lock file was left behind by a crashed process. Rounds + // run in parallel, on their own state roots, because admitting a second + // writer is a race rather than a certainty: one overlap in any round is a + // failure of the property. + for round := range 60 { + t.Run(fmt.Sprintf("round-%02d", round), func(t *testing.T) { + t.Parallel() + runAbandonedLockRound(t) + }) + } +} + +func runAbandonedLockRound(t *testing.T) { + t.Helper() root := t.TempDir() store := session.Store{StateRoot: root} - // Leave a lock file behind, as a crashed process would, with an old mtime. - stale := filepath.Join(root, "cli", "locks", "acme-cloud-login.lock") - if err := os.MkdirAll(filepath.Dir(stale), 0o700); err != nil { + + // Age the abandoned file well past any staleness threshold, so every + // writer in the burst sees the same abandoned lock. + abandoned := filepath.Join(root, "cli", "locks", "acme-cloud-login.lock") + if err := os.MkdirAll(filepath.Dir(abandoned), 0o700); err != nil { t.Fatalf("mkdir: %v", err) } - if err := os.WriteFile(stale, nil, 0o600); err != nil { - t.Fatalf("seed stale lock: %v", err) + if err := os.WriteFile(abandoned, nil, 0o600); err != nil { + t.Fatalf("seed abandoned lock: %v", err) + } + old := time.Now().Add(-time.Hour) + if err := os.Chtimes(abandoned, old, old); err != nil { + t.Fatalf("age abandoned lock: %v", err) } - old := time.Now().Add(-time.Minute) - if err := os.Chtimes(stale, old, old); err != nil { - t.Fatalf("age stale lock: %v", err) + + var mu sync.Mutex + inside, maxInside, entered := 0, 0, 0 + var group sync.WaitGroup + start := make(chan struct{}) + for range writersPerRound { + group.Add(1) + go func() { + defer group.Done() + <-start + _ = store.WithLock("acme-cloud-login", func() error { + mu.Lock() + inside++ + entered++ + if inside > maxInside { + maxInside = inside + } + mu.Unlock() + + time.Sleep(2 * time.Millisecond) + + mu.Lock() + inside-- + mu.Unlock() + return nil + }) + }() } + close(start) + group.Wait() + + mu.Lock() + peak, ran := maxInside, entered + mu.Unlock() + if peak != 1 { + t.Fatalf("lock admitted %d concurrent writers on one reference", peak) + } + // Exclusion alone is satisfied by a lock that admits one writer and refuses + // the rest, which is what a contended acquisition that never retries does. + // Every writer must also get its turn before the deadline. + if ran != writersPerRound { + t.Fatalf("only %d of %d writers ran: the lock refused contention instead of waiting for it", + ran, writersPerRound) + } +} + +// TestWithLockSurvivesKilledHolder covers the crash case end to end: a process +// killed while holding the lock must leave nothing that blocks the next +// acquisition. +func TestWithLockSurvivesKilledHolder(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the helper relies on POSIX process signalling") + } + keyring.MockInit() + root := t.TempDir() + + held := filepath.Join(root, "held") + helper := exec.Command(os.Args[0], "-test.run=TestHelperHoldsSessionLock", "-test.timeout=60s") + helper.Env = append(os.Environ(), lockHelperEnv+"="+root) + if err := helper.Start(); err != nil { + t.Fatalf("start helper: %v", err) + } + defer func() { + _ = helper.Process.Kill() + _, _ = helper.Process.Wait() + }() + + deadline := time.Now().Add(20 * time.Second) + for { + if _, err := os.Stat(held); err == nil { + break + } + if time.Now().After(deadline) { + t.Fatal("helper never took the lock") + } + time.Sleep(10 * time.Millisecond) + } + + if err := helper.Process.Kill(); err != nil { + t.Fatalf("kill helper: %v", err) + } + _, _ = helper.Process.Wait() + + store := session.Store{StateRoot: root} ran := false if err := store.WithLock("acme-cloud-login", func() error { ran = true; return nil }); err != nil { - t.Fatalf("stale lock was not taken over: %v", err) + t.Fatalf("lock not recovered after holder was killed: %v", err) } if !ran { - t.Fatal("writer never ran after takeover") + t.Fatal("writer never ran after holder was killed") + } +} + +// lockHelperEnv carries the state root into the helper subprocess and is what +// makes TestHelperHoldsSessionLock do anything at all. +const lockHelperEnv = "WSO2_CLI_TEST_LOCK_HELPER_ROOT" + +// TestHelperHoldsSessionLock is not a test: it is the subprocess body for +// TestWithLockSurvivesKilledHolder. It takes the lock, signals that it holds +// it, and blocks until it is killed. +func TestHelperHoldsSessionLock(t *testing.T) { + root := os.Getenv(lockHelperEnv) + if root == "" { + t.Skip("helper process only") } + keyring.MockInit() + store := session.Store{StateRoot: root} + _ = store.WithLock("acme-cloud-login", func() error { + if err := os.WriteFile(filepath.Join(root, "held"), nil, 0o600); err != nil { + return err + } + select {} + }) } func assertProblemCode(t *testing.T, err error, code string) { diff --git a/skills-lock.json b/skills-lock.json index 6d48e0b..e32f244 100644 --- a/skills-lock.json +++ b/skills-lock.json @@ -5,7 +5,7 @@ "source": "mattpocock/skills", "sourceType": "github", "skillPath": "skills/engineering/code-review/SKILL.md", - "computedHash": "31d149a480eaa68c11e32f5ee77f0fd0b98a906834d531d881d502352edd0b8e" + "computedHash": "1d5e7242a98674635be79ab310cdda0e555d36a9903d2526ebf4263d0b7997ca" }, "grill-with-docs": { "source": "mattpocock/skills", @@ -29,7 +29,7 @@ "source": "mattpocock/skills", "sourceType": "github", "skillPath": "skills/engineering/setup-matt-pocock-skills/SKILL.md", - "computedHash": "74e894a3509e2676d4cdb771c8eace087092430635e845e02a9cc2f757c552a4" + "computedHash": "f9c2b933dda18eea572e96f6ebbcd5b30e0d1bfdc074af53cc99728dc5f0bdac" }, "teach": { "source": "mattpocock/skills", @@ -41,7 +41,7 @@ "source": "mattpocock/skills", "sourceType": "github", "skillPath": "skills/engineering/to-spec/SKILL.md", - "computedHash": "0f544cd0c099c06f0dd0b7b9ee98b4237218e7e95fd3d3c02e791efbaf74bacb" + "computedHash": "7e07d4cfabd1a4f61627ebe2705601784fae7b9aca7e13af3e201ace75738200" }, "to-tickets": { "source": "mattpocock/skills", @@ -53,13 +53,13 @@ "source": "mattpocock/skills", "sourceType": "github", "skillPath": "skills/engineering/triage/SKILL.md", - "computedHash": "7c923b6a677cfe689500721f08307e2a4c46797ff169dc55ef6c34a431a0d533" + "computedHash": "1bc5349f0b61e19a6df61496b5a03d88cd69595dd303353f87945997a9e4a884" }, "wayfinder": { "source": "mattpocock/skills", "sourceType": "github", "skillPath": "skills/engineering/wayfinder/SKILL.md", - "computedHash": "c9e18cefd77b6b5b0ee35f59ce2fd96359aa2d2f02e3479e12c873c5402d9d43" + "computedHash": "f343ecf46157cb645a5494644418308ad95391e9fc696faa47ae5a412bf5f6e4" } } }