diff --git a/internal/sandbox/scope.go b/internal/sandbox/scope.go index af9b364ec..6372b8c46 100644 --- a/internal/sandbox/scope.go +++ b/internal/sandbox/scope.go @@ -19,6 +19,17 @@ type Scope struct { workspaceRoot string readRoots []string extraRoots []string + // tempReads / tempWrites count how many LIVE temporary grants depend on a + // root, so one holder's cleanup cannot revoke another's access. + // + // Without them a temporary grant was add-then-remove with no notion of who + // still needed it: the second caller to ask for a root already present got a + // NO-OP undo, and the first caller's cleanup removed the root out from under + // it. Two read-only tools in the same parallel batch, both blocked on the + // same directory, is exactly that shape — and read-only tools are precisely + // the ones the batch runs concurrently. + tempReads map[string]int + tempWrites map[string]int } // NewScope builds a scope for workspaceRoot plus the given extra roots. The @@ -81,15 +92,88 @@ func (s *Scope) Add(path string) (string, error) { } s.mu.Lock() defer s.mu.Unlock() - for _, existing := range append([]string{s.workspaceRoot}, s.extraRoots...) { - if pathWithinRoot(existing, root) { + // COVERED BY WHAT. extraRoots holds temporary write roots alongside + // permanent ones, so "something already covers this" was not the question + // worth asking: a permanent grant made over a path a temporary holder + // happened to cover recorded nothing, and vanished the moment that holder + // released. A session-scoped grant outliving the request that prompted it is + // the entire difference between Add and AddTemporaryWrite. + // + // Permanent coverage is looked for FIRST and across every root, because a + // path can be covered twice and the temporary cover must not decide the + // answer when a permanent one is also present. + if s.permanentWriteRootCoversLocked(root) { + return root, nil + } + for _, existing := range s.extraRoots { + // The same root, held temporarily: promote it in place. Its holders' + // releases become no-ops, which is what permanence means here. + if existing == root && s.temporaryWriteLocked(existing) { + delete(s.tempWrites, root) return root, nil } } + // Either uncovered, or covered only by a BROADER temporary root that is + // going to be released. Recording the narrower root in its own right is what + // survives that release. s.extraRoots = append(s.extraRoots, root) return root, nil } +// temporaryWriteLocked reports whether root is held by a temporary write grant +// rather than a session-scoped one. Callers must hold the lock. +func (s *Scope) temporaryWriteLocked(root string) bool { + _, temporary := s.tempWrites[root] + return temporary +} + +// permanentWriteRootCoversLocked reports whether root sits under write authority +// that outlives every temporary holder: the workspace root, or a session-scoped +// grant. Coverage by a TEMPORARY write root deliberately does not count — +// it is going to be released, so nothing recorded on the strength of it +// survives. Callers must hold the lock. +// +// One helper rather than a copy per caller because Add, AddRead and +// AddTemporaryRead all turn on the same question, and three copies of "covered, +// but by what" is how one of them ends up answering it differently. +// permanentReadRootCoversLocked reports whether root is already readable by a +// grant that outlives every temporary holder. +// +// SLICE ORDER MUST NOT DECIDE THIS. The old check walked readRoots and stopped at +// the first entry covering the path, so a BROAD TEMPORARY root sitting earlier in +// the slice shadowed a narrower PERMANENT one later — and the caller was handed a +// reference on somebody else's grant when it needed no reference at all. Asking +// the whole question first, over both lists, makes the answer independent of +// insertion order. +// +// Write roots count: a grant that permits writing permits reading the same tree. +func (s *Scope) permanentReadRootCoversLocked(root string) bool { + if s.permanentWriteRootCoversLocked(root) { + return true + } + for _, existing := range s.readRoots { + if pathWithinRoot(existing, root) && !s.temporaryReadLocked(existing) { + return true + } + } + return false +} + +func (s *Scope) permanentWriteRootCoversLocked(root string) bool { + for _, existing := range append([]string{s.workspaceRoot}, s.extraRoots...) { + if pathWithinRoot(existing, root) && !s.temporaryWriteLocked(existing) { + return true + } + } + return false +} + +// temporaryReadLocked is temporaryWriteLocked for read roots. +func (s *Scope) temporaryReadLocked(root string) bool { + _, temporary := s.tempReads[root] + return temporary +} + // AddRead grants read-only access under path. If the path is already covered by // a writable root, no separate read root is stored. func (s *Scope) AddRead(path string) (string, error) { @@ -99,11 +183,19 @@ func (s *Scope) AddRead(path string) (string, error) { } s.mu.Lock() defer s.mu.Unlock() - if s.writeRootCoversLocked(root) { + // Same rule as Add: coverage that is going to be released is not coverage a + // permanent grant can rely on, whichever side of the read/write boundary it + // sits on. A permanent read covered only by a temporary WRITE root died with + // that root just as surely. + if s.permanentWriteRootCoversLocked(root) { + return root, nil + } + if s.permanentReadRootCoversLocked(root) { return root, nil } for _, existing := range s.readRoots { - if pathWithinRoot(existing, root) { + if existing == root && s.temporaryReadLocked(existing) { + delete(s.tempReads, root) return root, nil } } @@ -118,16 +210,63 @@ func (s *Scope) AddTemporaryRead(path string) (string, func(), error) { } s.mu.Lock() defer s.mu.Unlock() - if s.writeRootCoversLocked(root) { + // Genuinely permanent — the workspace root, or a session-scoped grant. It + // outlives this reader, so there is nothing to release. + if s.permanentWriteRootCoversLocked(root) { return root, func() {}, nil } - for _, existing := range s.readRoots { - if pathWithinRoot(existing, root) { - return root, func() {}, nil - } + // A TEMPORARY write root covering this path is NOT borrowed, and this is the + // one place the distinction bites hardest. + // + // The dependency on that root is real — the write holder's cleanup must not + // revoke this reader, which is the defect the refcount exists to close, + // reached across the read/write boundary. But taking a reference on the + // WRITE root pays for the lifetime with the authority: the reference keeps + // the root in extraRoots, extraRoots is what Roots() feeds validate(), and + // validate() is WRITE authorization. So a reader outliving its writer went on + // writing anywhere under a write grant that had already ended — not merely + // under the path it asked to read. + // + // One reference cannot be both the lifetime and the capability. The lifetime + // is kept below, as a temporary READ root for the path this caller actually + // asked for. readRoots feeds ReadRoots(), which confers no write authority — + // validate() authorises writes from workspaceRoot plus extraRoots, and this + // path never enters that list. So the reader survives its writer with read + // authority and only read authority. + // + // That claim is the written justification for the whole design, so it has to + // stay exhaustive as the accessors change. It has already been wrong once, in + // both directions: an earlier draft said readRoots fed ReadRoots() and nothing + // else while ExtraReadRoots() also read it, and that accessor has since been + // removed as belonging to a different change. + if s.permanentReadRootCoversLocked(root) { + return root, func() {}, nil } + if s.tempReads == nil { + s.tempReads = map[string]int{} + } + // THE SAME ROOT, ANOTHER HOLDER: one entry, two references. Only an EXACT + // match shares — a narrower request must not, which is the whole point below. + if _, held := s.tempReads[root]; held { + s.tempReads[root]++ + return root, oncePerHolder(func() { s.releaseTemporaryRead(root) }), nil + } + // A NARROWER REQUEST RECORDS ITS OWN ROOT, even under a broader temporary + // read that currently covers it. + // + // Borrowing the covering root's reference kept this reader alive by keeping + // the BROAD ROOT alive, and readRoots feeds validateRead and the native + // sandbox profile. So once the broad reader released, a caller that had asked + // to read one subdirectory was left able to read the whole tree its neighbour + // had asked for, siblings included. Reported by @jatmn, and it is the read + // twin of the write escalation fixed above: the refcount key decides not only + // WHEN an entry disappears but WHICH path stays authorised while it lives. s.readRoots = append(s.readRoots, root) - return root, func() { s.removeReadRoot(root) }, nil + if s.tempReads == nil { + s.tempReads = map[string]int{} + } + s.tempReads[root] = 1 + return root, oncePerHolder(func() { s.releaseTemporaryRead(root) }), nil } func (s *Scope) AddTemporaryWrite(path string) (string, func(), error) { @@ -137,32 +276,107 @@ func (s *Scope) AddTemporaryWrite(path string) (string, func(), error) { } s.mu.Lock() defer s.mu.Unlock() - if s.writeRootCoversLocked(root) { + // A PERMANENT grant is the only cover that needs no bookkeeping: it outlives + // every temporary holder, so there is nothing to keep alive and nothing to + // release. + if s.permanentWriteRootCoversLocked(root) { return root, func() {}, nil } + if s.tempWrites == nil { + s.tempWrites = map[string]int{} + } + // THE SAME ROOT, ANOTHER HOLDER: one entry, two references. Only an exact + // match shares an entry — a NARROWER request must not, which is the whole + // point below. + if _, held := s.tempWrites[root]; held { + s.tempWrites[root]++ + return root, oncePerHolder(func() { s.releaseTemporaryWrite(root) }), nil + } + // A NARROWER REQUEST RECORDS ITS OWN ROOT, even when a broader temporary + // grant currently covers it. + // + // Borrowing a reference on the covering root kept this holder alive, which + // was the point, but it kept the COVERING ROOT alive to do it — and + // extraRoots is what validate() authorises writes from. So once the broad + // holder released, a caller that had asked to write one subdirectory was + // left holding write authority over the whole tree its neighbour had asked + // for, including siblings it never named. Reported by CodeRabbit, and it is + // the write-side twin of the read-side escalation @jatmn found: one + // reference cannot be both a lifetime and a capability, whichever side of + // the read/write boundary it sits on. + // + // Recording the requested root separately gives each holder exactly the + // authority it asked for and a lifetime of its own. The nesting costs an + // extra entry in extraRoots while both are live, which is redundant but not + // wrong — the broader root already permits everything the narrower one does. s.extraRoots = append(s.extraRoots, root) - return root, func() { s.removeWriteRoot(root) }, nil + s.tempWrites[root] = 1 + return root, oncePerHolder(func() { s.releaseTemporaryWrite(root) }), nil } -func (s *Scope) writeRootCoversLocked(root string) bool { - for _, existing := range append([]string{s.workspaceRoot}, s.extraRoots...) { - if pathWithinRoot(existing, root) { - return true - } - } - return false -} - -func (s *Scope) removeReadRoot(root string) { +// releaseTemporaryRead drops ONE holder's reference and removes the root only +// when the last one is gone. +// +// The count alone cannot make this safe, and an earlier comment here claimed it +// could — that each undo is called exactly once, and flooring at zero handled +// the rest. Flooring stops the count going negative; it does not stop one +// holder's second call consuming a DIFFERENT holder's reference. With two +// readers, calling the first's undo twice took the count 2 -> 1 -> 0 and removed +// a root the second was still using. Idempotency belongs to the closure, which +// is the thing that knows whose reference it is, so every undo handed out is +// wrapped in oncePerHolder. +func (s *Scope) releaseTemporaryRead(root string) { s.mu.Lock() - defer s.mu.Unlock() + remaining, tracked := s.tempReads[root] + if !tracked { + s.mu.Unlock() + return + } + remaining-- + if remaining > 0 { + s.tempReads[root] = remaining + s.mu.Unlock() + return + } + // Both mutations under ONE hold. Dropping the lock between them opened a + // window where the root was still in readRoots but no longer in tempReads: + // a concurrent AddTemporaryRead landing there reads it as a PERMANENT root, + // hands its caller a no-op undo, and then this call strips the root — so + // that caller believes it holds access it has already silently lost. + delete(s.tempReads, root) s.readRoots = removeScopeRoot(s.readRoots, root) + s.mu.Unlock() } -func (s *Scope) removeWriteRoot(root string) { +// releaseTemporaryWrite is releaseTemporaryRead for write roots. Two functions +// rather than one generic helper because they guard different slices and the +// generic version would take the slice by name — which is how the wrong one +// gets passed. +func (s *Scope) releaseTemporaryWrite(root string) { s.mu.Lock() - defer s.mu.Unlock() + remaining, tracked := s.tempWrites[root] + if !tracked { + s.mu.Unlock() + return + } + remaining-- + if remaining > 0 { + s.tempWrites[root] = remaining + s.mu.Unlock() + return + } + // Same single-hold rule as releaseTemporaryRead above. + delete(s.tempWrites, root) s.extraRoots = removeScopeRoot(s.extraRoots, root) + s.mu.Unlock() +} + +// oncePerHolder makes one holder's undo safe to call more than once. The +// reference it drops is that holder's own, so a second call must do nothing +// rather than reach into the shared count and take somebody else's. +func oncePerHolder(release func()) func() { + var once sync.Once + return func() { once.Do(release) } } func removeScopeRoot(roots []string, root string) []string { diff --git a/internal/sandbox/scope_extra_read_test.go b/internal/sandbox/scope_extra_read_test.go new file mode 100644 index 000000000..90977b6d8 --- /dev/null +++ b/internal/sandbox/scope_extra_read_test.go @@ -0,0 +1,56 @@ +package sandbox + +import ( + "path/filepath" + "testing" +) + +// THE ANTI-ESCALATION PROPERTY. A read grant (AddRead, where --add-read-dir lands) +// is READABLE but NOT WRITABLE. This is what makes propagating a parent's +// request_permissions read grant to a child safe: the child can audit the granted +// path but cannot modify it. Emitting the grant as a WRITE root (--add-dir) would +// break exactly this. +func TestAReadGrantIsReadableButNotWritable(t *testing.T) { + scope, granted := grantOutsideDefaults(t) + root, err := scope.AddRead(granted) + if err != nil { + t.Fatalf("AddRead: %v", err) + } + target := filepath.Join(root, "audit-me.go") + + if block := scope.validateRead(target); block != nil { + t.Fatalf("a read grant did not allow READING %q: %v — the audit would fail 'outside the workspace'", target, block) + } + if block := scope.validate(target); block == nil { + t.Fatalf("a read grant ALLOWED WRITING %q — a read grant escalated to write", target) + } +} + +// grantOutsideDefaults returns a scope and a directory that scope does not +// already cover, both under test-owned storage. +// +// THE CAUSE, NOT THE SYMPTOM. NewScope seeds the system temporary directory as a +// permanent write root, so a plain t.TempDir() grant is writable before any +// grant exists and the anti-escalation assertions below prove nothing. The first +// fix placed the fixture under os.UserHomeDir() to find ground the defaults do +// not own. That worked and bought two problems: a hermetic runner may expose +// HOME read-only, so these tests failed at MkdirTemp before asserting anything, +// and on an ordinary machine they wrote into the developer's real home and left +// zero-scope-* debris whenever a run was interrupted. Reported by @jatmn. +// +// Building the Scope directly removes the seeded defaults, which is what made +// HOME necessary in the first place. t.TempDir() is then outside by +// construction, the fixture is hermetic, and nothing touches HOME. +func grantOutsideDefaults(t *testing.T) (*Scope, string) { + t.Helper() + workspace := resolvedFixturePath(t, t.TempDir()) + granted := resolvedFixturePath(t, t.TempDir()) + scope := &Scope{workspaceRoot: workspace} + // PROVED AGAINST THIS SCOPE. The point of the helper is that the grant is + // what changes access, so the target has to begin unauthorised — asserted + // here once rather than trusted in every caller. + if scope.validate(filepath.Join(granted, "probe.txt")) == nil { + t.Fatalf("%s is writable before any grant exists; the assertions below would prove nothing", granted) + } + return scope, granted +} diff --git a/internal/sandbox/scope_temp_refcount_test.go b/internal/sandbox/scope_temp_refcount_test.go new file mode 100644 index 000000000..32a291cec --- /dev/null +++ b/internal/sandbox/scope_temp_refcount_test.go @@ -0,0 +1,595 @@ +package sandbox + +import ( + "os" + "path/filepath" + "slices" + "sync" + "testing" +) + +// scopeOutsideDefaults builds a Scope directly instead of through NewScope. +// +// NewScope adds the system temp directory as a write root, so a target under +// t.TempDir() is already covered and AddTemporaryRead returns before it ever +// touches the refcount. A test built that way exercises none of this and passes +// against the bug — which is how the first version of this test passed. +func scopeOutsideDefaults(t *testing.T) (*Scope, string) { + t.Helper() + workspace := t.TempDir() + target := filepath.Join(t.TempDir(), "shared") + if err := os.MkdirAll(target, 0o700); err != nil { + t.Fatalf("mkdir target: %v", err) + } + // Built directly, NOT via NewScope: real directories are needed because + // normalizeScopeRoot requires them to exist, but NewScope's default temp + // write root would then cover the target and short-circuit the path. + return &Scope{workspaceRoot: workspace}, target +} + +// The refcount and the root list have to move together. +// +// Release used to delete the refcount, drop the lock, then strip the root in a +// second acquisition. In that window the root was still in readRoots but no +// longer in tempReads, so a concurrent AddTemporaryRead read it as a PERMANENT +// root and handed its caller a no-op undo — then the release stripped it and +// that caller silently lost access it believed it held. +func TestReleasingATemporaryReadCannotStripALiveGrant(t *testing.T) { + scope, target := scopeOutsideDefaults(t) + + // First holder establishes the root. + first, releaseFirst, err := scope.AddTemporaryRead(target) + if err != nil { + t.Fatalf("AddTemporaryRead: %v", err) + } + if len(scope.tempReads) == 0 { + t.Fatal("precondition: the grant should be refcounted, not covered by a default root") + } + + // Second holder takes a reference on the same root. + second, releaseSecond, err := scope.AddTemporaryRead(target) + if err != nil { + t.Fatalf("AddTemporaryRead (second): %v", err) + } + + // The first holder leaving must not revoke the second's access. + releaseFirst() + if block := scope.validateRead(second); block != nil { + t.Fatalf("the second holder lost its grant when the first released: %v", block) + } + + // Only the last release retires the root. + releaseSecond() + if block := scope.validateRead(first); block == nil { + t.Error("the root outlived its last holder") + } +} + +// Under contention the same invariant has to hold: while a grant is live, its +// root is readable. +func TestTemporaryReadGrantsSurviveConcurrentReleases(t *testing.T) { + scope, target := scopeOutsideDefaults(t) + + var wg sync.WaitGroup + var mu sync.Mutex + var failures []string + + for i := 0; i < 24; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 200; j++ { + granted, undo, err := scope.AddTemporaryRead(target) + if err != nil { + mu.Lock() + failures = append(failures, "AddTemporaryRead: "+err.Error()) + mu.Unlock() + return + } + if block := scope.validateRead(granted); block != nil { + mu.Lock() + failures = append(failures, "root not readable while a grant was live") + mu.Unlock() + } + undo() + } + }() + } + wg.Wait() + + if len(failures) > 0 { + t.Fatalf("%d failures, first: %s", len(failures), failures[0]) + } +} + +// A read covered by a TEMPORARY WRITE root must take a reference too. +// +// permanentWriteRootCoversLocked scans extraRoots, and AddTemporaryWrite appends +// temporary write roots there — so "covered by a write root" does not mean +// "covered permanently". Without a reference the write holder's cleanup silently +// revoked the reader's access: the same defect the refcount closes, reached +// across the read/write boundary instead of within one side of it. +func TestAReadCoveredByATemporaryWriteSurvivesItsRelease(t *testing.T) { + workspace := t.TempDir() + outer := filepath.Join(t.TempDir(), "outer") + inner := filepath.Join(outer, "inner") + if err := os.MkdirAll(inner, 0o700); err != nil { + t.Fatal(err) + } + // Built directly for the same reason scopeOutsideDefaults is: NewScope's + // default temp write root would cover the fixture and short-circuit this. + scope := &Scope{workspaceRoot: workspace} + + _, releaseWrite, err := scope.AddTemporaryWrite(outer) + if err != nil { + t.Fatalf("temporary write: %v", err) + } + readRoot, releaseRead, err := scope.AddTemporaryRead(inner) + if err != nil { + t.Fatalf("temporary read: %v", err) + } + + covered := func() bool { + for _, root := range scope.ReadRoots() { + if pathWithinRoot(root, readRoot) { + return true + } + } + return false + } + if !covered() { + t.Fatal("the reader did not hold access before any release") + } + + // The WRITE holder finishes first; the reader has not released. + releaseWrite() + if !covered() { + t.Errorf("the write holder's cleanup revoked a live read grant on %s", readRoot) + } + + // And once the reader releases too, the root is genuinely gone. + releaseRead() + if covered() { + t.Errorf("the root outlived its last holder: %v", scope.ReadRoots()) + } +} + +func hasExtraRoot(scope *Scope, root string) bool { + for _, existing := range writeRootsBeyondWorkspace(scope) { + if existing == root { + return true + } + } + return false +} + +// A PERMANENT GRANT MUST OUTLIVE THE TEMPORARY ONE IT LANDED ON TOP OF. +// +// extraRoots holds temporary write roots alongside permanent ones, so Add asked +// only "is this already covered" — and a session-scoped grant made while a +// temporary holder happened to cover the path recorded nothing, then vanished +// when that holder released. Outliving the request that prompted it is the +// entire difference between Add and AddTemporaryWrite. +func TestAPermanentGrantSurvivesTheTemporaryOneItCovered(t *testing.T) { + t.Run("write, same root", func(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + _, releaseTemp, err := scope.AddTemporaryWrite(outside) + if err != nil { + t.Fatal(err) + } + if _, err := scope.Add(outside); err != nil { + t.Fatal(err) + } + releaseTemp() + if !hasExtraRoot(scope, outside) { + t.Error("the temporary holder's release revoked a permanent write grant") + } + }) + + t.Run("read, same root", func(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + _, releaseTemp, err := scope.AddTemporaryRead(outside) + if err != nil { + t.Fatal(err) + } + if _, err := scope.AddRead(outside); err != nil { + t.Fatal(err) + } + releaseTemp() + if !hasReadRoot(scope, outside) { + t.Error("the temporary holder's release revoked a permanent read grant") + } + }) + + // The nested shape: a BROAD temporary root covering a NARROW permanent one. + // Promoting in place cannot help here — the narrow root has to be recorded + // in its own right, or it goes when the broad one does. + t.Run("broad temporary over narrow permanent", func(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + inner := filepath.Join(outside, "inner") + if err := os.MkdirAll(inner, 0o700); err != nil { + t.Fatal(err) + } + _, releaseBroad, err := scope.AddTemporaryWrite(outside) + if err != nil { + t.Fatal(err) + } + if _, err := scope.Add(inner); err != nil { + t.Fatal(err) + } + releaseBroad() + covered := false + for _, existing := range writeRootsBeyondWorkspace(scope) { + if pathWithinRoot(existing, inner) { + covered = true + } + } + if !covered { + t.Error("a narrower permanent grant died with the broader temporary root it sat under") + } + }) + + // THE READ MIRROR OF BOTH SHAPES ABOVE. AddRead makes two claims that the + // subtests so far only made for Add: the loop over extraRoots skips coverage + // that is a temporary WRITE, and the loop over readRoots skips coverage that + // is a temporary READ. Each claim is a separate branch, and neither was + // exercised from the read side. + t.Run("permanent read under a temporary write", func(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + _, releaseWrite, err := scope.AddTemporaryWrite(outside) + if err != nil { + t.Fatal(err) + } + if _, err := scope.AddRead(outside); err != nil { + t.Fatal(err) + } + releaseWrite() + if !hasReadRoot(scope, outside) { + t.Error("a temporary write holder's release revoked a permanent read grant") + } + }) + + t.Run("broad temporary read over narrow permanent read", func(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + inner := filepath.Join(outside, "inner") + if err := os.MkdirAll(inner, 0o700); err != nil { + t.Fatal(err) + } + _, releaseBroad, err := scope.AddTemporaryRead(outside) + if err != nil { + t.Fatal(err) + } + if _, err := scope.AddRead(inner); err != nil { + t.Fatal(err) + } + releaseBroad() + if block := scope.validateRead(filepath.Join(inner, "audit-me.go")); block != nil { + t.Errorf("a narrower permanent read died with the broader temporary read it sat under: %v", block) + } + }) +} + +// ONE HOLDER'S UNDO DROPS ONE HOLDER'S REFERENCE, HOWEVER OFTEN IT IS CALLED. +// +// The count flooring at zero stops it going negative; it does not stop a +// holder's second call consuming somebody else's reference. With two readers, +// calling the first's undo twice took the count 2 -> 1 -> 0 and removed a root +// the second was still using — the exact revocation this refcount exists to +// prevent, reached through a duplicate call rather than a sibling's cleanup. +// Deferred cleanups in a retry path are how a real caller does this by accident. +func TestOneHoldersUndoIsIdempotent(t *testing.T) { + scope, workspace, outside := scopeOutsideRoots(t) + _, undoFirst, err := scope.AddTemporaryRead(outside) + if err != nil { + t.Fatal(err) + } + _, undoSecond, err := scope.AddTemporaryRead(outside) + if err != nil { + t.Fatal(err) + } + undoFirst() + undoFirst() + undoFirst() + if !hasReadRoot(scope, outside) { + t.Fatal("repeated calls to one holder's undo revoked a live holder's access") + } + undoSecond() + if hasReadRoot(scope, outside) { + t.Error("the root outlived its last holder") + } + + // The write side takes the same rule. Built directly for the same reason the + // helper is: NewScope would seed the system temp directory as a permanent + // write root, which already covers this fixture and makes the grant vacuous. + writeScope := &Scope{workspaceRoot: workspace} + _, undoWriteFirst, err := writeScope.AddTemporaryWrite(outside) + if err != nil { + t.Fatal(err) + } + _, undoWriteSecond, err := writeScope.AddTemporaryWrite(outside) + if err != nil { + t.Fatal(err) + } + undoWriteFirst() + undoWriteFirst() + if !hasExtraRoot(writeScope, outside) { + t.Fatal("repeated calls to one write holder's undo revoked a live holder's access") + } + undoWriteSecond() + if hasExtraRoot(writeScope, outside) { + t.Error("the write root outlived its last holder") + } +} + +// temporaryWriteFixture builds a nested pair of real directories and a scope +// that covers neither: an OUTER directory a temporary write grant will take, +// and an INNER one below it that a temporary read grant will ask for. +// +// Built directly rather than through NewScope, for the reason +// scopeOutsideDefaults documents: NewScope seeds the system temp directory as a +// PERMANENT write root, which would cover the whole fixture and send every +// branch under test down the "genuinely permanent" path instead. Both paths are +// returned symlink-resolved because normalizeScopeRoot resolves what it stores, +// and on macOS the fixture's /var spelling is a symlink to /private/var. +func temporaryWriteFixture(t *testing.T) (scope *Scope, workspace, outer, inner string) { + t.Helper() + outer = filepath.Join(t.TempDir(), "outer") + inner = filepath.Join(outer, "inner") + if err := os.MkdirAll(inner, 0o700); err != nil { + t.Fatal(err) + } + workspace = resolvedFixturePath(t, t.TempDir()) + return &Scope{workspaceRoot: workspace}, workspace, resolvedFixturePath(t, outer), resolvedFixturePath(t, inner) +} + +func resolvedFixturePath(t *testing.T, path string) string { + t.Helper() + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + t.Fatalf("resolve fixture %s: %v", path, err) + } + return resolved +} + +// A READER MUST NOT INHERIT ITS WRITER'S AUTHORITY. +// +// The reader's dependency on a covering TEMPORARY WRITE root was recorded as a +// reference on that root — one reference standing for both the lifetime and the +// capability. Release the WRITER first and the reference kept the path in +// extraRoots, which is the list Roots() feeds validate(), so the surviving +// read-only holder could WRITE anywhere below a write grant that had already +// ended — including paths it never asked to read. +// +// This is TestAReadGrantIsReadableButNotWritable's property for temporary +// grants: read authority stays read authority, whichever holder leaves first. +func TestAReaderOutlivingItsWriterKeepsReadAuthorityOnly(t *testing.T) { + t.Run("the writer releases first", func(t *testing.T) { + scope, workspace, outer, inner := temporaryWriteFixture(t) + + writeRoot, releaseWrite, err := scope.AddTemporaryWrite(outer) + if err != nil { + t.Fatalf("temporary write: %v", err) + } + if writeRoot != outer { + t.Fatalf("temporary write granted %q, want %q", writeRoot, outer) + } + readRoot, releaseRead, err := scope.AddTemporaryRead(inner) + if err != nil { + t.Fatalf("temporary read: %v", err) + } + if readRoot != inner { + t.Fatalf("temporary read granted %q, want %q", readRoot, inner) + } + // The reader holds a read root of its own and NO reference on the write + // root. Both halves matter: the first is what survives the writer, the + // second is what stops the write root surviving with it. + // + // Errorf, not Fatalf: these say WHY the behavior below breaks, and a + // bookkeeping check that aborts the run hides the behavior it explains. + scope.mu.RLock() + writeHolders := scope.tempWrites[outer] + readHolders := scope.tempReads[inner] + scope.mu.RUnlock() + if writeHolders != 1 { + t.Errorf("tempWrites[%s] = %d, want 1: the reader took a reference on the WRITE root", outer, writeHolders) + } + if readHolders != 1 { + t.Errorf("tempReads[%s] = %d, want 1: the reader kept no read root of its own", inner, readHolders) + } + + // The WRITE holder finishes; only the read-only holder is left. + releaseWrite() + + if got := writeRootsBeyondWorkspace(scope); len(got) != 0 { + t.Errorf("ExtraRoots() = %v, want none: the reader held the write root open", got) + } + if got, want := scope.ReadRoots(), []string{workspace, inner}; !slices.Equal(got, want) { + t.Errorf("ReadRoots() = %v, want %v", got, want) + } + + target := filepath.Join(inner, "audit-me.go") + if block := scope.validateRead(target); block != nil { + t.Errorf("the writer's cleanup revoked a live read grant on %q: %v", target, block) + } + block := scope.validate(target) + if block == nil { + t.Fatalf("a read-only holder may still WRITE %q after the write grant ended", target) + } + if block.Code != BlockOutsideWorkspace { + t.Errorf("write block code = %q, want %q", block.Code, BlockOutsideWorkspace) + } + if block.Path != target { + t.Errorf("write block path = %q, want %q", block.Path, target) + } + // The escalation was never confined to the path the reader asked for — + // a borrowed reference carries the whole covering root — so a sibling + // the reader never named has to be denied too. + sibling := filepath.Join(outer, "sibling.txt") + if block := scope.validate(sibling); block == nil { + t.Errorf("a read-only holder of %q may still WRITE %q, which it never asked to read", inner, sibling) + } + + releaseRead() + if got, want := scope.ReadRoots(), []string{workspace}; !slices.Equal(got, want) { + t.Errorf("ReadRoots() = %v, want %v: the read root outlived its last holder", got, want) + } + }) + + // A CONTROL, NOT COVERAGE FOR THE FINDING. This subtest passes on the + // unfixed tree too — the escalation needs the WRITER to go first, which the + // subtest above exercises. It earns its place by pinning that the opposite + // release order was not broken while fixing the dangerous one, and it is + // labelled so nobody counts it as evidence the finding is closed. + t.Run("the reader releases first (control: passes unfixed)", func(t *testing.T) { + scope, _, outer, inner := temporaryWriteFixture(t) + + _, releaseWrite, err := scope.AddTemporaryWrite(outer) + if err != nil { + t.Fatalf("temporary write: %v", err) + } + _, releaseRead, err := scope.AddTemporaryRead(inner) + if err != nil { + t.Fatalf("temporary read: %v", err) + } + + // The other order, which the fix must not break: the writer keeps FULL + // authority for as long as it holds the grant. + releaseRead() + buildLog := filepath.Join(outer, "build.log") + if block := scope.validate(buildLog); block != nil { + t.Fatalf("the reader's cleanup revoked the live write grant on %q: %v", buildLog, block) + } + if got, want := writeRootsBeyondWorkspace(scope), []string{outer}; !slices.Equal(got, want) { + t.Errorf("ExtraRoots() = %v, want %v", got, want) + } + + releaseWrite() + if block := scope.validate(buildLog); block == nil { + t.Errorf("the write root %q outlived its only holder", outer) + } + if block := scope.validateRead(filepath.Join(inner, "audit-me.go")); block == nil { + t.Errorf("the read root %q outlived its only holder", inner) + } + }) +} + +// A NARROWER WRITE HOLDER MUST NOT INHERIT ITS NEIGHBOUR'S TREE. +// +// The write-side twin of the read-side escalation above, and the same root +// cause: a nested holder used to take a reference on the BROADER temporary root +// that happened to cover it. That kept the nested holder alive, which was the +// point, but it kept the covering root alive to do it — and extraRoots is what +// validate() authorises writes from. Once the broad holder released, a caller +// that had asked for one subdirectory held write authority over the whole tree, +// siblings included. Reported by CodeRabbit. +func TestANestedWriteHolderKeepsOnlyItsOwnSubtree(t *testing.T) { + scope, _, outsideBase := scopeOutsideRoots(t) + outer := filepath.Join(outsideBase, "outer") + inner := filepath.Join(outer, "inner") + sibling := filepath.Join(outer, "sibling") + for _, dir := range []string{outer, inner, sibling} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + _, releaseOuter, err := scope.AddTemporaryWrite(outer) + if err != nil { + t.Fatalf("temporary write on the broad root: %v", err) + } + _, releaseInner, err := scope.AddTemporaryWrite(inner) + if err != nil { + t.Fatalf("temporary write on the nested root: %v", err) + } + + // THE BROAD HOLDER GOES FIRST — the nested holder outliving it is what + // exposes whose authority it is actually holding. + releaseOuter() + + if block := scope.validate(filepath.Join(inner, "mine.txt")); block != nil { + t.Errorf("the nested holder lost the subtree it asked for: %v", block.Reason) + } + if block := scope.validate(filepath.Join(sibling, "not-mine.txt")); block == nil { + t.Error("the nested holder can write a SIBLING it never asked for: the broad holder's authority outlived the broad holder") + } + if block := scope.validate(filepath.Join(outer, "not-mine.txt")); block == nil { + t.Error("the nested holder can write the covering root itself after its holder released") + } + + releaseInner() + if block := scope.validate(filepath.Join(inner, "mine.txt")); block == nil { + t.Error("the nested root outlived its only holder") + } +} + +// The opposite order, as a control: the nested holder releasing first must not +// disturb the broad holder that is still live. +func TestANestedWriteHolderReleasingFirstLeavesTheBroadGrant(t *testing.T) { + scope, _, outsideBase := scopeOutsideRoots(t) + outer := filepath.Join(outsideBase, "outer") + inner := filepath.Join(outer, "inner") + for _, dir := range []string{outer, inner} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + _, releaseOuter, err := scope.AddTemporaryWrite(outer) + if err != nil { + t.Fatal(err) + } + _, releaseInner, err := scope.AddTemporaryWrite(inner) + if err != nil { + t.Fatal(err) + } + + releaseInner() + if block := scope.validate(filepath.Join(inner, "still-ok.txt")); block != nil { + t.Errorf("the nested holder's release revoked the broad holder's coverage: %v", block.Reason) + } + releaseOuter() + if block := scope.validate(filepath.Join(inner, "gone.txt")); block == nil { + t.Error("the tree outlived every holder") + } +} + +// TWO HOLDERS OF THE SAME ROOT SHARE ONE ENTRY. Only an exact match shares — +// a narrower request must not, which is what the escalation test above pins. +func TestTwoHoldersOfTheSameWriteRootShareOneEntry(t *testing.T) { + scope, _, outsideBase := scopeOutsideRoots(t) + target := filepath.Join(outsideBase, "shared") + if err := os.MkdirAll(target, 0o700); err != nil { + t.Fatal(err) + } + _, releaseFirst, err := scope.AddTemporaryWrite(target) + if err != nil { + t.Fatal(err) + } + _, releaseSecond, err := scope.AddTemporaryWrite(target) + if err != nil { + t.Fatal(err) + } + + releaseFirst() + if block := scope.validate(filepath.Join(target, "still-held.txt")); block != nil { + t.Errorf("one holder's release revoked the other's grant on the same root: %v", block.Reason) + } + releaseSecond() + if block := scope.validate(filepath.Join(target, "gone.txt")); block == nil { + t.Error("the shared root outlived both holders") + } +} + +// writeRootsBeyondWorkspace is Roots() without the workspace entry. +// +// These tests need to see which BEYOND-WORKSPACE write roots a scope currently +// holds. There was an ExtraRoots() accessor that answered exactly this, but it +// had no production caller — it belongs to #829's child-grant propagation, whose +// consumer side is not on this branch, and @jatmn was right that carrying it here +// leaves unrelated production surface in a PR scoped to refcounting. The +// inspection is a test concern, so it lives in the tests. +func writeRootsBeyondWorkspace(scope *Scope) []string { + all := scope.Roots() + if len(all) == 0 { + return nil + } + return append([]string(nil), all[1:]...) +} diff --git a/internal/sandbox/scope_temporary_test.go b/internal/sandbox/scope_temporary_test.go new file mode 100644 index 000000000..728fe04e6 --- /dev/null +++ b/internal/sandbox/scope_temporary_test.go @@ -0,0 +1,366 @@ +package sandbox + +import ( + "os" + "path/filepath" + "sync" + "testing" +) + +// scopeOutsideRoots builds a workspace and an unrelated directory that no +// default write root already covers. +// +// NOT t.TempDir(). /tmp and $TMPDIR are default write roots, so a temporary +// grant for a path under one is a no-op — every assertion here would pass +// against code that grants nothing at all. That is RULES §2.3 in its exact +// form, and it caught the first version of this test. +// scopeOutsideRoots builds a workspace and a directory outside it, both under +// test-owned storage, and returns a Scope that treats the second as genuinely +// outside. +// +// THE FIXTURE PROBLEM AND ITS ACTUAL CAUSE. NewScope seeds the system temporary +// directory as a permanent write root, so an ordinary t.TempDir() grant is +// covered before any temporary grant exists and every assertion built on it is +// vacuous. The first fix for that reached into os.UserHomeDir() to find ground +// the defaults do not already own — which worked, and bought two new problems: +// a hermetic runner may expose HOME read-only, so the tests failed at MkdirTemp +// before asserting anything; and on an ordinary machine they wrote fixtures into +// the developer's real home and left zeromax-scope-* debris behind whenever a +// run was interrupted. Reported by @jatmn. +// +// The cause is the seeded defaults, not the location, so the Scope is built +// DIRECTLY here — the same pattern scopeOutsideDefaults and temporaryWriteFixture +// already use. With no defaults seeded, t.TempDir() is outside by construction, +// the fixture is hermetic, and nothing touches HOME. +// +// Paths are symlink-resolved because the scope stores resolved roots and macOS +// hands out /var/... temp directories that resolve to /private/var/...; an +// unresolved fixture path compares unequal to the root the scope just recorded. +func scopeOutsideRoots(t *testing.T) (scope *Scope, workspace string, outside string) { + t.Helper() + workspace = resolvedFixturePath(t, t.TempDir()) + outside = filepath.Join(resolvedFixturePath(t, t.TempDir()), "outside") + // FATAL, NOT SKIP. Every caller of this helper asserts an AUTHORIZATION + // boundary, so a skipped fixture means that assertion never ran and the build + // reported green anyway. t.TempDir() already failed the test if it could not + // provide storage, so a subdirectory failing under it is a broken machine. + if err := os.MkdirAll(outside, 0o700); err != nil { + t.Fatalf("cannot prepare %s: %v", outside, err) + } + scope = &Scope{workspaceRoot: workspace} + // PROVED AGAINST THIS SCOPE, not against production's default list. The point + // of the helper is that a grant here changes something, and the only thing + // that can answer that is the scope the test will actually use. Checking + // defaultTempWriteRoots() instead asks about a scope nobody builds here — and + // says "prove nothing" for a t.TempDir() path that this scope, seeded with no + // defaults, genuinely does not cover. + // + // Asserting the NEGATIVE up front is also what keeps every caller honest: it + // establishes that the target begins unauthorised, so a later success is the + // grant's doing and not the fixture's placement. + if scope.validate(filepath.Join(outside, "probe.txt")) == nil { + t.Fatalf("%s is writable before any grant exists, so a temporary grant here would prove nothing", outside) + } + if scope.validateRead(filepath.Join(outside, "probe.txt")) == nil { + t.Fatalf("%s is readable before any grant exists, so a temporary read grant here would prove nothing", outside) + } + return scope, workspace, outside +} + +func hasReadRoot(scope *Scope, root string) bool { + for _, existing := range scope.ReadRoots() { + if existing == root { + return true + } + } + return false +} + +// THE DEFECT: one holder's cleanup revoked another's access. Two read-only +// tools in the same parallel batch, both blocked on the same directory, is +// exactly this — and read-only tools are the ones the batch runs concurrently. +func TestATemporaryReadSurvivesASiblingsCleanup(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + // The probe that the fix must make unnecessary: without it this reads + // true -> false. + if hasReadRoot(scope, outside) { + t.Fatal("the outside root is already covered; this test proves nothing") + } + + _, undoA, err := scope.AddTemporaryRead(outside) + if err != nil { + t.Fatal(err) + } + _, undoB, err := scope.AddTemporaryRead(outside) + if err != nil { + t.Fatal(err) + } + if !hasReadRoot(scope, outside) { + t.Fatal("the grant did not take effect") + } + + undoA() + if !hasReadRoot(scope, outside) { + t.Fatal("A's cleanup revoked the root while B still held a grant") + } + undoB() + if hasReadRoot(scope, outside) { + t.Fatal("the root outlived its last holder") + } +} + +// A NARROWER REQUEST KEEPS ITS OWN SUBTREE AND NOTHING WIDER. +// +// THIS TEST USED TO ASSERT THE OPPOSITE, and the earlier version is worth +// recording because it is how the escalation survived a review round. It was +// written to prove a LIFETIME property — the broad holder's cleanup must not +// revoke a live narrow holder — and it proved that by asserting the BROAD root +// was still present afterwards. That is the defect stated as a requirement: the +// narrow holder was keeping its neighbour's whole tree alive in order to keep +// itself alive, so the surviving reader could read siblings it never asked for. +// +// Lifetime and capability are two properties and one reference cannot carry +// both. The assertions below name them separately: the requested path stays +// readable while its holder lives, AND a sibling under the released broad root +// is denied as soon as the holder that authorised it exits. Reported by @jatmn. +func TestANarrowerReaderKeepsOnlyItsOwnSubtree(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + nested := filepath.Join(outside, "nested") + sibling := filepath.Join(outside, "sibling") + for _, dir := range []string{nested, sibling} { + // FATAL, NOT SKIP. These directories are the fixture, not a precondition + // the environment might reasonably withhold — skipping on them reports a + // broken setup as a passing run. + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + + _, undoBroad, err := scope.AddTemporaryRead(outside) + if err != nil { + t.Fatal(err) + } + _, undoNarrow, err := scope.AddTemporaryRead(nested) + if err != nil { + t.Fatal(err) + } + + // THE BROAD HOLDER GOES FIRST. The narrow holder outliving it is what exposes + // whose authority it is actually holding. + undoBroad() + if block := scope.validateRead(filepath.Join(nested, "mine.txt")); block != nil { + t.Errorf("the broad holder's cleanup revoked the subtree the narrow holder asked for: %v", block.Reason) + } + if block := scope.validateRead(filepath.Join(sibling, "not-mine.txt")); block == nil { + t.Error("the narrow reader can read a SIBLING it never asked for: the broad holder's authority outlived the broad holder") + } + if block := scope.validateRead(filepath.Join(outside, "not-mine.txt")); block == nil { + t.Error("the narrow reader can read the covering root itself after its holder released") + } + + undoNarrow() + if block := scope.validateRead(filepath.Join(nested, "mine.txt")); block == nil { + t.Error("the nested root outlived its only holder") + } +} + +// The opposite order, as a control: the narrow reader releasing first must not +// disturb the broad reader that is still live. +func TestANarrowerReaderReleasingFirstLeavesTheBroadGrant(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + nested := filepath.Join(outside, "nested") + if err := os.MkdirAll(nested, 0o700); err != nil { + t.Fatal(err) + } + _, undoBroad, err := scope.AddTemporaryRead(outside) + if err != nil { + t.Fatal(err) + } + _, undoNarrow, err := scope.AddTemporaryRead(nested) + if err != nil { + t.Fatal(err) + } + + undoNarrow() + if block := scope.validateRead(filepath.Join(nested, "still-ok.txt")); block != nil { + t.Errorf("the narrow holder's release revoked the broad holder's coverage: %v", block.Reason) + } + undoBroad() + if block := scope.validateRead(filepath.Join(nested, "gone.txt")); block == nil { + t.Error("the tree outlived every holder") + } +} + +// TWO READERS OF THE SAME ROOT SHARE ONE ENTRY. Only an exact match shares; a +// narrower one never does, which is what the escalation test above pins. +func TestTwoReadersOfTheSameRootShareOneEntry(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + _, undoFirst, err := scope.AddTemporaryRead(outside) + if err != nil { + t.Fatal(err) + } + _, undoSecond, err := scope.AddTemporaryRead(outside) + if err != nil { + t.Fatal(err) + } + + undoFirst() + if block := scope.validateRead(filepath.Join(outside, "still-held.txt")); block != nil { + t.Errorf("one reader's release revoked the other's grant on the same root: %v", block.Reason) + } + undoSecond() + if block := scope.validateRead(filepath.Join(outside, "gone.txt")); block == nil { + t.Error("the shared root outlived both holders") + } +} + +// A NARROW PERMANENT READ IS NOT BORROWED FROM A BROAD TEMPORARY ONE. The old +// scan walked readRoots in order and stopped at the first entry covering the +// path, so a broad TEMPORARY root earlier in the slice shadowed a narrower +// PERMANENT one later — and the caller took a reference it did not need on a +// grant that was about to end. +func TestAPermanentReadIsNotShadowedByABroadTemporaryOne(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + nested := filepath.Join(outside, "nested") + if err := os.MkdirAll(nested, 0o700); err != nil { + t.Fatal(err) + } + // The broad TEMPORARY grant is taken first, so it sits earlier in readRoots. + _, undoBroad, err := scope.AddTemporaryRead(outside) + if err != nil { + t.Fatal(err) + } + if _, err := scope.AddRead(nested); err != nil { + t.Fatal(err) + } + + undoBroad() + if block := scope.validateRead(filepath.Join(nested, "mine.txt")); block != nil { + t.Errorf("a PERMANENT read grant died with a broad temporary one that merely preceded it: %v", block.Reason) + } +} + +// A PERMANENT root is not refcounted and must never be removed by a temporary +// holder's cleanup — the undo for a request it already covers is genuinely +// nothing. +func TestATemporaryGrantNeverRevokesAPermanentRoot(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + if _, err := scope.AddRead(outside); err != nil { + t.Fatal(err) + } + _, undo, err := scope.AddTemporaryRead(outside) + if err != nil { + t.Fatal(err) + } + undo() + if !hasReadRoot(scope, outside) { + t.Fatal("a temporary holder's cleanup removed a permanent read root") + } +} + +// The same rule for WRITE roots, or a write grant keeps the bug reads no longer +// have. +func TestATemporaryWriteSurvivesASiblingsCleanup(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + covered := func() bool { + for _, existing := range scope.Roots() { + if existing == outside { + return true + } + } + return false + } + _, undoA, err := scope.AddTemporaryWrite(outside) + if err != nil { + t.Fatal(err) + } + _, undoB, err := scope.AddTemporaryWrite(outside) + if err != nil { + t.Fatal(err) + } + if !covered() { + t.Fatal("the write grant did not take effect") + } + undoA() + if !covered() { + t.Fatal("A's cleanup revoked the write root while B still held a grant") + } + undoB() + if covered() { + t.Fatal("the write root outlived its last holder") + } +} + +// UNDER REAL CONCURRENCY, which is the shape the parallel tool batch produces: +// eight holders taking and releasing the same root in overlapping windows. The +// root must be present the whole time at least one holder has it, and gone once +// the last releases. +func TestConcurrentHoldersOfOneRoot(t *testing.T) { + scope, _, outside := scopeOutsideRoots(t) + + const holders = 8 + var wg sync.WaitGroup + start := make(chan struct{}) + release := make(chan struct{}) + // TWO PER HOLDER, because each one can report at both hasReadRoot checks. At + // one slot per holder the channel fills after eight reports, the ninth send + // blocks forever, wg.Done() is never reached and wg.Wait() hangs — so the + // test would DEADLOCK rather than fail in exactly the case it exists to + // catch, since nothing drains failures until after the wait. + failures := make(chan string, 2*holders) + // Signalled by EVERY holder once its own AddTemporaryRead has returned, + // failure included: the gate below waits for all of them, so a holder that + // returned early without signalling would hang this test rather than fail it. + acquired := make(chan struct{}, holders) + for i := 0; i < holders; i++ { + wg.Add(1) + go func() { + defer wg.Done() + <-start + _, undo, err := scope.AddTemporaryRead(outside) + acquired <- struct{}{} + if err != nil { + failures <- err.Error() + return + } + // Every holder must SEE its own grant for as long as it holds it. + if !hasReadRoot(scope, outside) { + failures <- "a holder could not see the root it had just been granted" + } + <-release + if !hasReadRoot(scope, outside) { + failures <- "a holder lost the root while still holding it" + } + undo() + }() + } + close(start) + // EVERY holder, not the first one. Spinning on hasReadRoot only waited for + // somebody to hold the root, and the spin is tight enough to win that race + // against the goroutines still being scheduled: measured over 200 runs, 194 + // of them peaked at a single simultaneous holder and none ever reached + // eight. A test named for concurrent holders was testing one holder at a + // time, and would have passed against an implementation with no refcount at + // all. Counting the acquisitions makes the overlap real instead of hoped for. + for i := 0; i < holders; i++ { + <-acquired + } + close(release) + wg.Wait() + close(failures) + for failure := range failures { + t.Fatal(failure) + } + if hasReadRoot(scope, outside) { + t.Fatal("the root outlived every holder") + } + // AND THE BOOKKEEPING IS EMPTY, not merely invisible. hasReadRoot answers + // what a caller can see; a refcount entry left behind at zero is not visible + // that way and would still leak, one map entry per acquire/release cycle. + scope.mu.Lock() + leftover, stillCounted := scope.tempReads[outside] + scope.mu.Unlock() + if stillCounted { + t.Fatalf("the refcount table kept an entry for %q after every holder released: %v", outside, leftover) + } +}