fix(web): revalidate paths without a build and bound the path cache
- answer If-None-Match with a 304 after the access check and before the
list is loaded, so a revalidation never triggers a build
- distinct validators for the gzip ("paths-<sha>-gz") and plain
("paths-<sha>") variants, compared against the one that would be served
- cache an exact-capacity body so the byte budget is real
- run builds in their own goroutine; every caller waits on done or its
ctx, so a disconnecting client is released; a panic becomes an error
- key builds in flight by repository and commit so a failure of one
repository is not shared with another- commit
- 515aa1e7f3e9a47a07560235c6f813f3d9bab665
- author
- brice · · 2026-10-11 19:15 UTC
- parent
- aff5730
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +315 -49
M cmd/ermite/web_files_test.go +5 -2
| @@ -439,7 +439,7 @@ func TestWebPaths(t *testing.T) { | |||
| 439 | 439 | assert.Equal(t, "no-cache", gz.hdr.Get("Cache-Control")) | |
| 440 | 440 | assert.Equal(t, "Accept-Encoding", gz.hdr.Get("Vary")) | |
| 441 | 441 | etag := gz.hdr.Get("ETag") | |
| 442 | - | assert.Equal(t, `"paths-`+sha+`"`, etag) | |
| 442 | + | assert.Equal(t, `"paths-`+sha+`-gz"`, etag) | |
| 443 | 443 | got := decodeList(t, gunzip(t, gz.body)) | |
| 444 | 444 | assert.Equal(t, want, got.Paths, "git ls-tree -r order") | |
| 445 | 445 | assert.False(t, got.Truncated) | |
| @@ -449,13 +449,16 @@ func TestWebPaths(t *testing.T) { | |||
| 449 | 449 | require.Equal(t, 200, plain.status) | |
| 450 | 450 | assert.Empty(t, plain.hdr.Get("Content-Encoding")) | |
| 451 | 451 | assert.Equal(t, want, decodeList(t, plain.body).Paths) | |
| 452 | - | assert.Equal(t, etag, plain.hdr.Get("ETag")) | |
| 452 | + | assert.Equal(t, `"paths-`+sha+`"`, plain.hdr.Get("ETag"), "another variant, another validator") | |
| 453 | 453 | ||
| 454 | 454 | // Revalidation: 304, still through the access rule. | |
| 455 | 455 | nm := raw(map[string]string{"If-None-Match": etag, "Accept-Encoding": "gzip"}) | |
| 456 | 456 | assert.Equal(t, http.StatusNotModified, nm.status) | |
| 457 | 457 | assert.Empty(t, nm.body) | |
| 458 | 458 | assert.Equal(t, "no-cache", nm.hdr.Get("Cache-Control")) | |
| 459 | + | nm = raw(map[string]string{"If-None-Match": plain.hdr.Get("ETag")}) | |
| 460 | + | assert.Equal(t, http.StatusNotModified, nm.status, "identity validator, identity body") | |
| 461 | + | assert.Equal(t, 200, raw(map[string]string{"If-None-Match": etag}).status, "the gzip validator does not match the plain body") | |
| 459 | 462 | assert.Equal(t, 200, raw(map[string]string{"If-None-Match": `"paths-other"`}).status) | |
| 460 | 463 | ||
| 461 | 464 | // Only a full commit id: abbreviations, refs, upper case and unknown ids. | |
| @@ -439,7 +439,7 @@ func TestWebPaths(t *testing.T) { | |||||
| 439 | assert.Equal(t, "no-cache", gz.hdr.Get("Cache-Control")) | 439 | assert.Equal(t, "no-cache", gz.hdr.Get("Cache-Control")) | ||
| 440 | assert.Equal(t, "Accept-Encoding", gz.hdr.Get("Vary")) | 440 | assert.Equal(t, "Accept-Encoding", gz.hdr.Get("Vary")) | ||
| 441 | etag := gz.hdr.Get("ETag") | 441 | etag := gz.hdr.Get("ETag") | ||
| 442 | - | assert.Equal(t, `"paths-`+sha+`"`, etag) | 442 | + | assert.Equal(t, `"paths-`+sha+`-gz"`, etag) |
| 443 | got := decodeList(t, gunzip(t, gz.body)) | 443 | got := decodeList(t, gunzip(t, gz.body)) | ||
| 444 | assert.Equal(t, want, got.Paths, "git ls-tree -r order") | 444 | assert.Equal(t, want, got.Paths, "git ls-tree -r order") | ||
| 445 | assert.False(t, got.Truncated) | 445 | assert.False(t, got.Truncated) | ||
| @@ -449,13 +449,16 @@ func TestWebPaths(t *testing.T) { | |||||
| 449 | require.Equal(t, 200, plain.status) | 449 | require.Equal(t, 200, plain.status) | ||
| 450 | assert.Empty(t, plain.hdr.Get("Content-Encoding")) | 450 | assert.Empty(t, plain.hdr.Get("Content-Encoding")) | ||
| 451 | assert.Equal(t, want, decodeList(t, plain.body).Paths) | 451 | assert.Equal(t, want, decodeList(t, plain.body).Paths) | ||
| 452 | - | assert.Equal(t, etag, plain.hdr.Get("ETag")) | 452 | + | assert.Equal(t, `"paths-`+sha+`"`, plain.hdr.Get("ETag"), "another variant, another validator") |
| 453 | 453 | ||||
| 454 | // Revalidation: 304, still through the access rule. | 454 | // Revalidation: 304, still through the access rule. | ||
| 455 | nm := raw(map[string]string{"If-None-Match": etag, "Accept-Encoding": "gzip"}) | 455 | nm := raw(map[string]string{"If-None-Match": etag, "Accept-Encoding": "gzip"}) | ||
| 456 | assert.Equal(t, http.StatusNotModified, nm.status) | 456 | assert.Equal(t, http.StatusNotModified, nm.status) | ||
| 457 | assert.Empty(t, nm.body) | 457 | assert.Empty(t, nm.body) | ||
| 458 | assert.Equal(t, "no-cache", nm.hdr.Get("Cache-Control")) | 458 | assert.Equal(t, "no-cache", nm.hdr.Get("Cache-Control")) | ||
| 459 | + | nm = raw(map[string]string{"If-None-Match": plain.hdr.Get("ETag")}) | |||
| 460 | + | assert.Equal(t, http.StatusNotModified, nm.status, "identity validator, identity body") | |||
| 461 | + | assert.Equal(t, 200, raw(map[string]string{"If-None-Match": etag}).status, "the gzip validator does not match the plain body") | |||
| 459 | assert.Equal(t, 200, raw(map[string]string{"If-None-Match": `"paths-other"`}).status) | 462 | assert.Equal(t, 200, raw(map[string]string{"If-None-Match": `"paths-other"`}).status) | ||
| 460 | 463 | ||||
| 461 | // Only a full commit id: abbreviations, refs, upper case and unknown ids. | 464 | // Only a full commit id: abbreviations, refs, upper case and unknown ids. | ||
M internal/adapters/web/pathcache.go +36 -23
| @@ -5,7 +5,10 @@ package web | |||
| 5 | 5 | import ( | |
| 6 | 6 | "container/list" | |
| 7 | 7 | "context" | |
| 8 | + | "runtime/debug" | |
| 8 | 9 | "sync" | |
| 10 | + | ||
| 11 | + | "github.com/bnema/zerowrap" | |
| 9 | 12 | ) | |
| 10 | 13 | ||
| 11 | 14 | const ( | |
| @@ -21,7 +24,7 @@ const ( | |||
| 21 | 24 | // entry is right for every repository and every visitor; who may read it is | |
| 22 | 25 | // decided before the cache is asked (Server.paths). | |
| 23 | 26 | // | |
| 24 | - | // Concurrent misses for one commit share a single build. | |
| 27 | + | // Concurrent misses for one commit of one repository share a single build. | |
| 25 | 28 | type pathCache struct { | |
| 26 | 29 | mu sync.Mutex | |
| 27 | 30 | budget int | |
| @@ -29,7 +32,7 @@ type pathCache struct { | |||
| 29 | 32 | max int // largest entry kept | |
| 30 | 33 | lru *list.List // of *pathEntry, most recently used first | |
| 31 | 34 | byID map[string]*list.Element | |
| 32 | - | building map[string]*pathBuild | |
| 35 | + | building map[string]*pathBuild // by repository and commit | |
| 33 | 36 | } | |
| 34 | 37 | ||
| 35 | 38 | type pathEntry struct { | |
| @@ -98,44 +101,54 @@ func (c *pathCache) putLocked(id string, body []byte) { | |||
| 98 | 101 | } | |
| 99 | 102 | ||
| 100 | 103 | // load returns the list of a commit: from the cache, or from build, which | |
| 101 | - | // runs once however many callers ask at the same time. The caller that | |
| 102 | - | // starts the build runs it; the others wait for it, or for their own ctx. | |
| 103 | - | func (c *pathCache) load(ctx context.Context, id string, build func() ([]byte, error)) ([]byte, error) { | |
| 104 | + | // runs once however many callers of a repository ask at the same time. The | |
| 105 | + | // build runs in its own goroutine, so no caller is held by it: each one, | |
| 106 | + | // the one that started it included, waits for the build or for its own ctx. | |
| 107 | + | // | |
| 108 | + | // Builds in progress are keyed by repository and commit, though the cache is | |
| 109 | + | // keyed by commit only: a failure of one repository (the commit is missing | |
| 110 | + | // there, its access rule changed) must not be handed to the requests of | |
| 111 | + | // another one that would have succeeded. | |
| 112 | + | func (c *pathCache) load(ctx context.Context, repo, id string, build func() ([]byte, error)) ([]byte, error) { | |
| 113 | + | key := repo + "\x00" + id // a repository name has no NUL | |
| 104 | 114 | c.mu.Lock() | |
| 105 | 115 | if b, ok := c.getLocked(id); ok { | |
| 106 | 116 | c.mu.Unlock() | |
| 107 | 117 | return b, nil | |
| 108 | 118 | } | |
| 109 | - | if f, ok := c.building[id]; ok { | |
| 110 | - | c.mu.Unlock() | |
| 111 | - | select { | |
| 112 | - | case <-f.done: | |
| 113 | - | return f.body, f.err | |
| 114 | - | case <-ctx.Done(): | |
| 115 | - | return nil, ctx.Err() | |
| 116 | - | } | |
| 119 | + | f, ok := c.building[key] | |
| 120 | + | if !ok { | |
| 121 | + | f = &pathBuild{done: make(chan struct{})} | |
| 122 | + | c.building[key] = f | |
| 123 | + | go c.run(ctx, key, id, f, build) | |
| 117 | 124 | } | |
| 118 | - | f := &pathBuild{done: make(chan struct{})} | |
| 119 | - | c.building[id] = f | |
| 120 | 125 | c.mu.Unlock() | |
| 126 | + | select { | |
| 127 | + | case <-f.done: | |
| 128 | + | return f.body, f.err | |
| 129 | + | case <-ctx.Done(): | |
| 130 | + | return nil, ctx.Err() | |
| 131 | + | } | |
| 132 | + | } | |
| 121 | 133 | ||
| 134 | + | // run executes a build and publishes it. A panic of build is logged and | |
| 135 | + | // handed to the waiters as errBuildPanicked: it cannot propagate, since | |
| 136 | + | // nobody above this goroutine could recover it and it would end the process. | |
| 137 | + | func (c *pathCache) run(ctx context.Context, key, id string, f *pathBuild, build func() ([]byte, error)) { | |
| 122 | 138 | defer func() { | |
| 123 | - | // Also on a panic of build: nobody waits for ever. | |
| 124 | 139 | if r := recover(); r != nil { | |
| 125 | - | f.err = errBuildPanicked | |
| 126 | - | c.finish(id, f) | |
| 127 | - | panic(r) | |
| 140 | + | f.body, f.err = nil, errBuildPanicked | |
| 141 | + | zerowrap.Ctx(ctx).Error().Interface("panic", r).Str("stack", string(debug.Stack())).Msg("path list build panicked") | |
| 128 | 142 | } | |
| 143 | + | c.finish(key, id, f) | |
| 129 | 144 | }() | |
| 130 | 145 | f.body, f.err = build() | |
| 131 | - | c.finish(id, f) | |
| 132 | - | return f.body, f.err | |
| 133 | 146 | } | |
| 134 | 147 | ||
| 135 | 148 | // finish publishes a build: stored when it worked, then released to the waiters. | |
| 136 | - | func (c *pathCache) finish(id string, f *pathBuild) { | |
| 149 | + | func (c *pathCache) finish(key, id string, f *pathBuild) { | |
| 137 | 150 | c.mu.Lock() | |
| 138 | - | delete(c.building, id) | |
| 151 | + | delete(c.building, key) | |
| 139 | 152 | if f.err == nil { | |
| 140 | 153 | c.putLocked(id, f.body) | |
| 141 | 154 | } | |
| @@ -5,7 +5,10 @@ package web | |||||
| 5 | import ( | 5 | import ( | ||
| 6 | "container/list" | 6 | "container/list" | ||
| 7 | "context" | 7 | "context" | ||
| 8 | + | "runtime/debug" | |||
| 8 | "sync" | 9 | "sync" | ||
| 10 | + | ||||
| 11 | + | "github.com/bnema/zerowrap" | |||
| 9 | ) | 12 | ) | ||
| 10 | 13 | ||||
| 11 | const ( | 14 | const ( | ||
| @@ -21,7 +24,7 @@ const ( | |||||
| 21 | // entry is right for every repository and every visitor; who may read it is | 24 | // entry is right for every repository and every visitor; who may read it is | ||
| 22 | // decided before the cache is asked (Server.paths). | 25 | // decided before the cache is asked (Server.paths). | ||
| 23 | // | 26 | // | ||
| 24 | - | // Concurrent misses for one commit share a single build. | 27 | + | // Concurrent misses for one commit of one repository share a single build. |
| 25 | type pathCache struct { | 28 | type pathCache struct { | ||
| 26 | mu sync.Mutex | 29 | mu sync.Mutex | ||
| 27 | budget int | 30 | budget int | ||
| @@ -29,7 +32,7 @@ type pathCache struct { | |||||
| 29 | max int // largest entry kept | 32 | max int // largest entry kept | ||
| 30 | lru *list.List // of *pathEntry, most recently used first | 33 | lru *list.List // of *pathEntry, most recently used first | ||
| 31 | byID map[string]*list.Element | 34 | byID map[string]*list.Element | ||
| 32 | - | building map[string]*pathBuild | 35 | + | building map[string]*pathBuild // by repository and commit |
| 33 | } | 36 | } | ||
| 34 | 37 | ||||
| 35 | type pathEntry struct { | 38 | type pathEntry struct { | ||
| @@ -98,44 +101,54 @@ func (c *pathCache) putLocked(id string, body []byte) { | |||||
| 98 | } | 101 | } | ||
| 99 | 102 | ||||
| 100 | // load returns the list of a commit: from the cache, or from build, which | 103 | // load returns the list of a commit: from the cache, or from build, which | ||
| 101 | - | // runs once however many callers ask at the same time. The caller that | 104 | + | // runs once however many callers of a repository ask at the same time. The |
| 102 | - | // starts the build runs it; the others wait for it, or for their own ctx. | 105 | + | // build runs in its own goroutine, so no caller is held by it: each one, |
| 103 | - | func (c *pathCache) load(ctx context.Context, id string, build func() ([]byte, error)) ([]byte, error) { | 106 | + | // the one that started it included, waits for the build or for its own ctx. |
| 107 | + | // | |||
| 108 | + | // Builds in progress are keyed by repository and commit, though the cache is | |||
| 109 | + | // keyed by commit only: a failure of one repository (the commit is missing | |||
| 110 | + | // there, its access rule changed) must not be handed to the requests of | |||
| 111 | + | // another one that would have succeeded. | |||
| 112 | + | func (c *pathCache) load(ctx context.Context, repo, id string, build func() ([]byte, error)) ([]byte, error) { | |||
| 113 | + | key := repo + "\x00" + id // a repository name has no NUL | |||
| 104 | c.mu.Lock() | 114 | c.mu.Lock() | ||
| 105 | if b, ok := c.getLocked(id); ok { | 115 | if b, ok := c.getLocked(id); ok { | ||
| 106 | c.mu.Unlock() | 116 | c.mu.Unlock() | ||
| 107 | return b, nil | 117 | return b, nil | ||
| 108 | } | 118 | } | ||
| 109 | - | if f, ok := c.building[id]; ok { | 119 | + | f, ok := c.building[key] |
| 110 | - | c.mu.Unlock() | 120 | + | if !ok { |
| 111 | - | select { | 121 | + | f = &pathBuild{done: make(chan struct{})} |
| 112 | - | case <-f.done: | 122 | + | c.building[key] = f |
| 113 | - | return f.body, f.err | 123 | + | go c.run(ctx, key, id, f, build) |
| 114 | - | case <-ctx.Done(): | |||
| 115 | - | return nil, ctx.Err() | |||
| 116 | - | } | |||
| 117 | } | 124 | } | ||
| 118 | - | f := &pathBuild{done: make(chan struct{})} | |||
| 119 | - | c.building[id] = f | |||
| 120 | c.mu.Unlock() | 125 | c.mu.Unlock() | ||
| 126 | + | select { | |||
| 127 | + | case <-f.done: | |||
| 128 | + | return f.body, f.err | |||
| 129 | + | case <-ctx.Done(): | |||
| 130 | + | return nil, ctx.Err() | |||
| 131 | + | } | |||
| 132 | + | } | |||
| 121 | 133 | ||||
| 134 | + | // run executes a build and publishes it. A panic of build is logged and | |||
| 135 | + | // handed to the waiters as errBuildPanicked: it cannot propagate, since | |||
| 136 | + | // nobody above this goroutine could recover it and it would end the process. | |||
| 137 | + | func (c *pathCache) run(ctx context.Context, key, id string, f *pathBuild, build func() ([]byte, error)) { | |||
| 122 | defer func() { | 138 | defer func() { | ||
| 123 | - | // Also on a panic of build: nobody waits for ever. | |||
| 124 | if r := recover(); r != nil { | 139 | if r := recover(); r != nil { | ||
| 125 | - | f.err = errBuildPanicked | 140 | + | f.body, f.err = nil, errBuildPanicked |
| 126 | - | c.finish(id, f) | 141 | + | zerowrap.Ctx(ctx).Error().Interface("panic", r).Str("stack", string(debug.Stack())).Msg("path list build panicked") |
| 127 | - | panic(r) | |||
| 128 | } | 142 | } | ||
| 143 | + | c.finish(key, id, f) | |||
| 129 | }() | 144 | }() | ||
| 130 | f.body, f.err = build() | 145 | f.body, f.err = build() | ||
| 131 | - | c.finish(id, f) | |||
| 132 | - | return f.body, f.err | |||
| 133 | } | 146 | } | ||
| 134 | 147 | ||||
| 135 | // finish publishes a build: stored when it worked, then released to the waiters. | 148 | // finish publishes a build: stored when it worked, then released to the waiters. | ||
| 136 | - | func (c *pathCache) finish(id string, f *pathBuild) { | 149 | + | func (c *pathCache) finish(key, id string, f *pathBuild) { |
| 137 | c.mu.Lock() | 150 | c.mu.Lock() | ||
| 138 | - | delete(c.building, id) | 151 | + | delete(c.building, key) |
| 139 | if f.err == nil { | 152 | if f.err == nil { | ||
| 140 | c.putLocked(id, f.body) | 153 | c.putLocked(id, f.body) | ||
| 141 | } | 154 | } | ||
M internal/adapters/web/pathcache_test.go +79 -5
| @@ -94,16 +94,16 @@ func TestPathCacheLoadBuildsOnce(t *testing.T) { | |||
| 94 | 94 | wg.Add(1) | |
| 95 | 95 | go func() { | |
| 96 | 96 | defer wg.Done() | |
| 97 | - | b, err := c.load(context.Background(), "x", build) | |
| 97 | + | b, err := c.load(context.Background(), "r", "x", build) | |
| 98 | 98 | assert.NoError(t, err) | |
| 99 | 99 | got[i] = b | |
| 100 | 100 | }() | |
| 101 | 101 | } | |
| 102 | 102 | // A waiter whose context ends does not stop the others. | |
| 103 | - | require.Eventually(t, func() bool { c.mu.Lock(); defer c.mu.Unlock(); return c.building["x"] != nil }, time.Second, time.Millisecond) | |
| 103 | + | require.Eventually(t, func() bool { c.mu.Lock(); defer c.mu.Unlock(); return c.building["r\x00x"] != nil }, time.Second, time.Millisecond) | |
| 104 | 104 | ctx, cancel := context.WithCancel(context.Background()) | |
| 105 | 105 | cancel() | |
| 106 | - | _, err := c.load(ctx, "x", build) | |
| 106 | + | _, err := c.load(ctx, "r", "x", build) | |
| 107 | 107 | assert.ErrorIs(t, err, context.Canceled) | |
| 108 | 108 | close(release) | |
| 109 | 109 | wg.Wait() | |
| @@ -116,12 +116,86 @@ func TestPathCacheLoadBuildsOnce(t *testing.T) { | |||
| 116 | 116 | assert.Equal(t, "list", string(b)) | |
| 117 | 117 | ||
| 118 | 118 | boom := errors.New("boom") | |
| 119 | - | _, err = c.load(context.Background(), "y", func() ([]byte, error) { return nil, boom }) | |
| 119 | + | _, err = c.load(context.Background(), "r", "y", func() ([]byte, error) { return nil, boom }) | |
| 120 | 120 | assert.ErrorIs(t, err, boom) | |
| 121 | 121 | _, ok = c.get("y") | |
| 122 | 122 | assert.False(t, ok, "a failure is not cached") | |
| 123 | - | b, err = c.load(context.Background(), "y", func() ([]byte, error) { return []byte("ok"), nil }) | |
| 123 | + | b, err = c.load(context.Background(), "r", "y", func() ([]byte, error) { return []byte("ok"), nil }) | |
| 124 | 124 | require.NoError(t, err) | |
| 125 | 125 | assert.Equal(t, "ok", string(b), "the next request builds again") | |
| 126 | 126 | assert.Empty(t, c.building) | |
| 127 | 127 | } | |
| 128 | + | ||
| 129 | + | // A build that panics is handed to its waiters as an error, leaves nothing | |
| 130 | + | // in flight or cached, and does not take the process down. | |
| 131 | + | func TestPathCacheLoadPanic(t *testing.T) { | |
| 132 | + | c := newPathCache(1000, 200) | |
| 133 | + | started, release := make(chan struct{}), make(chan struct{}) | |
| 134 | + | var once sync.Once | |
| 135 | + | build := func() ([]byte, error) { | |
| 136 | + | once.Do(func() { close(started) }) | |
| 137 | + | <-release | |
| 138 | + | panic("boom") | |
| 139 | + | } | |
| 140 | + | errs := make(chan error, 2) | |
| 141 | + | for range 2 { | |
| 142 | + | go func() { | |
| 143 | + | _, err := c.load(context.Background(), "r", "x", build) | |
| 144 | + | errs <- err | |
| 145 | + | }() | |
| 146 | + | } | |
| 147 | + | <-started | |
| 148 | + | require.Eventually(t, func() bool { c.mu.Lock(); defer c.mu.Unlock(); return len(c.building) == 1 }, time.Second, time.Millisecond) | |
| 149 | + | close(release) | |
| 150 | + | for range 2 { | |
| 151 | + | assert.ErrorIs(t, <-errs, errBuildPanicked) | |
| 152 | + | } | |
| 153 | + | assert.Empty(t, c.building) | |
| 154 | + | _, ok := c.get("x") | |
| 155 | + | assert.False(t, ok, "a panic is not cached") | |
| 156 | + | ||
| 157 | + | b, err := c.load(context.Background(), "r", "x", func() ([]byte, error) { return []byte("ok"), nil }) | |
| 158 | + | require.NoError(t, err) | |
| 159 | + | assert.Equal(t, "ok", string(b), "the next request builds again") | |
| 160 | + | } | |
| 161 | + | ||
| 162 | + | // The caller that starts a build is released with its context too, and the | |
| 163 | + | // build goes on for the others. | |
| 164 | + | func TestPathCacheLoadStarterLeaves(t *testing.T) { | |
| 165 | + | c := newPathCache(1000, 200) | |
| 166 | + | release := make(chan struct{}) | |
| 167 | + | build := func() ([]byte, error) { <-release; return []byte("list"), nil } | |
| 168 | + | ctx, cancel := context.WithCancel(context.Background()) | |
| 169 | + | done := make(chan error, 1) | |
| 170 | + | go func() { _, err := c.load(ctx, "r", "x", build); done <- err }() | |
| 171 | + | require.Eventually(t, func() bool { c.mu.Lock(); defer c.mu.Unlock(); return len(c.building) == 1 }, time.Second, time.Millisecond) | |
| 172 | + | cancel() | |
| 173 | + | select { | |
| 174 | + | case err := <-done: | |
| 175 | + | assert.ErrorIs(t, err, context.Canceled) | |
| 176 | + | case <-time.After(time.Second): | |
| 177 | + | t.Fatal("the starter is held by its build") | |
| 178 | + | } | |
| 179 | + | close(release) | |
| 180 | + | require.Eventually(t, func() bool { _, ok := c.get("x"); return ok }, time.Second, time.Millisecond) | |
| 181 | + | assert.Empty(t, c.building) | |
| 182 | + | } | |
| 183 | + | ||
| 184 | + | // A failure belongs to the repository that built it: another repository | |
| 185 | + | // asking for the same commit builds its own. | |
| 186 | + | func TestPathCacheLoadFailureStaysInRepo(t *testing.T) { | |
| 187 | + | c := newPathCache(1000, 200) | |
| 188 | + | started, release := make(chan struct{}), make(chan struct{}) | |
| 189 | + | boom := errors.New("boom") | |
| 190 | + | failing := make(chan error, 1) | |
| 191 | + | go func() { | |
| 192 | + | _, err := c.load(context.Background(), "a", "x", func() ([]byte, error) { close(started); <-release; return nil, boom }) | |
| 193 | + | failing <- err | |
| 194 | + | }() | |
| 195 | + | <-started | |
| 196 | + | b, err := c.load(context.Background(), "b", "x", func() ([]byte, error) { return []byte("list"), nil }) | |
| 197 | + | require.NoError(t, err) | |
| 198 | + | assert.Equal(t, "list", string(b)) | |
| 199 | + | close(release) | |
| 200 | + | assert.ErrorIs(t, <-failing, boom) | |
| 201 | + | } | |
| @@ -94,16 +94,16 @@ func TestPathCacheLoadBuildsOnce(t *testing.T) { | |||||
| 94 | wg.Add(1) | 94 | wg.Add(1) | ||
| 95 | go func() { | 95 | go func() { | ||
| 96 | defer wg.Done() | 96 | defer wg.Done() | ||
| 97 | - | b, err := c.load(context.Background(), "x", build) | 97 | + | b, err := c.load(context.Background(), "r", "x", build) |
| 98 | assert.NoError(t, err) | 98 | assert.NoError(t, err) | ||
| 99 | got[i] = b | 99 | got[i] = b | ||
| 100 | }() | 100 | }() | ||
| 101 | } | 101 | } | ||
| 102 | // A waiter whose context ends does not stop the others. | 102 | // A waiter whose context ends does not stop the others. | ||
| 103 | - | require.Eventually(t, func() bool { c.mu.Lock(); defer c.mu.Unlock(); return c.building["x"] != nil }, time.Second, time.Millisecond) | 103 | + | require.Eventually(t, func() bool { c.mu.Lock(); defer c.mu.Unlock(); return c.building["r\x00x"] != nil }, time.Second, time.Millisecond) |
| 104 | ctx, cancel := context.WithCancel(context.Background()) | 104 | ctx, cancel := context.WithCancel(context.Background()) | ||
| 105 | cancel() | 105 | cancel() | ||
| 106 | - | _, err := c.load(ctx, "x", build) | 106 | + | _, err := c.load(ctx, "r", "x", build) |
| 107 | assert.ErrorIs(t, err, context.Canceled) | 107 | assert.ErrorIs(t, err, context.Canceled) | ||
| 108 | close(release) | 108 | close(release) | ||
| 109 | wg.Wait() | 109 | wg.Wait() | ||
| @@ -116,12 +116,86 @@ func TestPathCacheLoadBuildsOnce(t *testing.T) { | |||||
| 116 | assert.Equal(t, "list", string(b)) | 116 | assert.Equal(t, "list", string(b)) | ||
| 117 | 117 | ||||
| 118 | boom := errors.New("boom") | 118 | boom := errors.New("boom") | ||
| 119 | - | _, err = c.load(context.Background(), "y", func() ([]byte, error) { return nil, boom }) | 119 | + | _, err = c.load(context.Background(), "r", "y", func() ([]byte, error) { return nil, boom }) |
| 120 | assert.ErrorIs(t, err, boom) | 120 | assert.ErrorIs(t, err, boom) | ||
| 121 | _, ok = c.get("y") | 121 | _, ok = c.get("y") | ||
| 122 | assert.False(t, ok, "a failure is not cached") | 122 | assert.False(t, ok, "a failure is not cached") | ||
| 123 | - | b, err = c.load(context.Background(), "y", func() ([]byte, error) { return []byte("ok"), nil }) | 123 | + | b, err = c.load(context.Background(), "r", "y", func() ([]byte, error) { return []byte("ok"), nil }) |
| 124 | require.NoError(t, err) | 124 | require.NoError(t, err) | ||
| 125 | assert.Equal(t, "ok", string(b), "the next request builds again") | 125 | assert.Equal(t, "ok", string(b), "the next request builds again") | ||
| 126 | assert.Empty(t, c.building) | 126 | assert.Empty(t, c.building) | ||
| 127 | } | 127 | } | ||
| 128 | + | ||||
| 129 | + | // A build that panics is handed to its waiters as an error, leaves nothing | |||
| 130 | + | // in flight or cached, and does not take the process down. | |||
| 131 | + | func TestPathCacheLoadPanic(t *testing.T) { | |||
| 132 | + | c := newPathCache(1000, 200) | |||
| 133 | + | started, release := make(chan struct{}), make(chan struct{}) | |||
| 134 | + | var once sync.Once | |||
| 135 | + | build := func() ([]byte, error) { | |||
| 136 | + | once.Do(func() { close(started) }) | |||
| 137 | + | <-release | |||
| 138 | + | panic("boom") | |||
| 139 | + | } | |||
| 140 | + | errs := make(chan error, 2) | |||
| 141 | + | for range 2 { | |||
| 142 | + | go func() { | |||
| 143 | + | _, err := c.load(context.Background(), "r", "x", build) | |||
| 144 | + | errs <- err | |||
| 145 | + | }() | |||
| 146 | + | } | |||
| 147 | + | <-started | |||
| 148 | + | require.Eventually(t, func() bool { c.mu.Lock(); defer c.mu.Unlock(); return len(c.building) == 1 }, time.Second, time.Millisecond) | |||
| 149 | + | close(release) | |||
| 150 | + | for range 2 { | |||
| 151 | + | assert.ErrorIs(t, <-errs, errBuildPanicked) | |||
| 152 | + | } | |||
| 153 | + | assert.Empty(t, c.building) | |||
| 154 | + | _, ok := c.get("x") | |||
| 155 | + | assert.False(t, ok, "a panic is not cached") | |||
| 156 | + | ||||
| 157 | + | b, err := c.load(context.Background(), "r", "x", func() ([]byte, error) { return []byte("ok"), nil }) | |||
| 158 | + | require.NoError(t, err) | |||
| 159 | + | assert.Equal(t, "ok", string(b), "the next request builds again") | |||
| 160 | + | } | |||
| 161 | + | ||||
| 162 | + | // The caller that starts a build is released with its context too, and the | |||
| 163 | + | // build goes on for the others. | |||
| 164 | + | func TestPathCacheLoadStarterLeaves(t *testing.T) { | |||
| 165 | + | c := newPathCache(1000, 200) | |||
| 166 | + | release := make(chan struct{}) | |||
| 167 | + | build := func() ([]byte, error) { <-release; return []byte("list"), nil } | |||
| 168 | + | ctx, cancel := context.WithCancel(context.Background()) | |||
| 169 | + | done := make(chan error, 1) | |||
| 170 | + | go func() { _, err := c.load(ctx, "r", "x", build); done <- err }() | |||
| 171 | + | require.Eventually(t, func() bool { c.mu.Lock(); defer c.mu.Unlock(); return len(c.building) == 1 }, time.Second, time.Millisecond) | |||
| 172 | + | cancel() | |||
| 173 | + | select { | |||
| 174 | + | case err := <-done: | |||
| 175 | + | assert.ErrorIs(t, err, context.Canceled) | |||
| 176 | + | case <-time.After(time.Second): | |||
| 177 | + | t.Fatal("the starter is held by its build") | |||
| 178 | + | } | |||
| 179 | + | close(release) | |||
| 180 | + | require.Eventually(t, func() bool { _, ok := c.get("x"); return ok }, time.Second, time.Millisecond) | |||
| 181 | + | assert.Empty(t, c.building) | |||
| 182 | + | } | |||
| 183 | + | ||||
| 184 | + | // A failure belongs to the repository that built it: another repository | |||
| 185 | + | // asking for the same commit builds its own. | |||
| 186 | + | func TestPathCacheLoadFailureStaysInRepo(t *testing.T) { | |||
| 187 | + | c := newPathCache(1000, 200) | |||
| 188 | + | started, release := make(chan struct{}), make(chan struct{}) | |||
| 189 | + | boom := errors.New("boom") | |||
| 190 | + | failing := make(chan error, 1) | |||
| 191 | + | go func() { | |||
| 192 | + | _, err := c.load(context.Background(), "a", "x", func() ([]byte, error) { close(started); <-release; return nil, boom }) | |||
| 193 | + | failing <- err | |||
| 194 | + | }() | |||
| 195 | + | <-started | |||
| 196 | + | b, err := c.load(context.Background(), "b", "x", func() ([]byte, error) { return []byte("list"), nil }) | |||
| 197 | + | require.NoError(t, err) | |||
| 198 | + | assert.Equal(t, "list", string(b)) | |||
| 199 | + | close(release) | |||
| 200 | + | assert.ErrorIs(t, <-failing, boom) | |||
| 201 | + | } | |||
M internal/adapters/web/paths.go +42 -19
| @@ -33,7 +33,9 @@ type pathList struct { | |||
| 33 | 33 | // changes for it, but who may read it does, so every request is authorized | |
| 34 | 34 | // first (ResolveRevision runs the access rule) and only then served from the | |
| 35 | 35 | // cache of lists. Caching follows raw: a visitor revalidates every time, with | |
| 36 | - | // an ETag that makes it a 304; the owner's copy is private and never stored. | |
| 36 | + | // an ETag that makes it a 304 (answered before the list is loaded, so it never | |
| 37 | + | // waits for a build); the owner's copy is private and never stored. The ETag | |
| 38 | + | // differs for the gzip and the plain variant. | |
| 37 | 39 | func (s *Server) paths(w http.ResponseWriter, r *http.Request) { | |
| 38 | 40 | ctx, cancel := context.WithTimeout(r.Context(), requestTimeout) | |
| 39 | 41 | defer cancel() | |
| @@ -53,25 +55,46 @@ func (s *Server) paths(w http.ResponseWriter, r *http.Request) { | |||
| 53 | 55 | s.fail(ctx, w, r, errNotFound) | |
| 54 | 56 | return | |
| 55 | 57 | } | |
| 56 | - | body, err := s.pathCache.load(ctx, sha, func() ([]byte, error) { return s.buildPaths(ctx, repo, sha) }) | |
| 58 | + | ||
| 59 | + | // The validator names the variant that would be served: the gzip bytes | |
| 60 | + | // and the plain JSON are different representations. | |
| 61 | + | gz := acceptsGzip(r.Header.Get("Accept-Encoding")) | |
| 62 | + | etag := `"paths-` + sha + `"` | |
| 63 | + | if gz { | |
| 64 | + | etag = `"paths-` + sha + `-gz"` | |
| 65 | + | } | |
| 66 | + | // Revalidated every time, as raw is: a repository can turn private, and a | |
| 67 | + | // shared cache must not keep serving its files. The ETag keeps it cheap. | |
| 68 | + | validators := func(h http.Header) { | |
| 69 | + | h.Set("Vary", "Accept-Encoding") | |
| 70 | + | h.Set("ETag", etag) | |
| 71 | + | cacheHeaders(ctx, h, "no-cache", sessionCache) | |
| 72 | + | } | |
| 73 | + | // Access is settled: a revalidation is answered without the list, so it | |
| 74 | + | // never waits for a build. | |
| 75 | + | if etagMatches(r.Header.Get("If-None-Match"), etag) { | |
| 76 | + | validators(w.Header()) | |
| 77 | + | w.WriteHeader(http.StatusNotModified) | |
| 78 | + | return | |
| 79 | + | } | |
| 80 | + | body, err := s.pathCache.load(ctx, repo, sha, func() ([]byte, error) { return s.buildPaths(ctx, repo, sha) }) | |
| 57 | 81 | if err != nil { | |
| 58 | 82 | s.fail(ctx, w, r, err) | |
| 59 | 83 | return | |
| 60 | 84 | } | |
| 85 | + | var plain *gzip.Reader | |
| 86 | + | if !gz { | |
| 87 | + | if plain, err = gzip.NewReader(bytes.NewReader(body)); err != nil { | |
| 88 | + | s.fail(ctx, w, r, err) | |
| 89 | + | return | |
| 90 | + | } | |
| 91 | + | } | |
| 61 | 92 | ||
| 62 | 93 | h := w.Header() | |
| 63 | 94 | h.Set("Content-Type", "application/json") | |
| 64 | 95 | h.Set("X-Content-Type-Options", "nosniff") | |
| 65 | - | h.Set("Vary", "Accept-Encoding") | |
| 66 | - | h.Set("ETag", `"paths-`+sha+`"`) | |
| 67 | - | // Revalidated every time, as raw is: a repository can turn private, and a | |
| 68 | - | // shared cache must not keep serving its files. The ETag keeps it cheap. | |
| 69 | - | cacheHeaders(ctx, h, "no-cache", sessionCache) | |
| 70 | - | if etagMatches(r.Header.Get("If-None-Match"), h.Get("ETag")) { | |
| 71 | - | w.WriteHeader(http.StatusNotModified) | |
| 72 | - | return | |
| 73 | - | } | |
| 74 | - | if acceptsGzip(r.Header.Get("Accept-Encoding")) { | |
| 96 | + | validators(h) | |
| 97 | + | if gz { | |
| 75 | 98 | h.Set("Content-Encoding", "gzip") | |
| 76 | 99 | h.Set("Content-Length", strconv.Itoa(len(body))) | |
| 77 | 100 | w.WriteHeader(http.StatusOK) | |
| @@ -80,14 +103,9 @@ func (s *Server) paths(w http.ResponseWriter, r *http.Request) { | |||
| 80 | 103 | } | |
| 81 | 104 | return | |
| 82 | 105 | } | |
| 83 | - | zr, err := gzip.NewReader(bytes.NewReader(body)) | |
| 84 | - | if err != nil { | |
| 85 | - | s.fail(ctx, w, r, err) | |
| 86 | - | return | |
| 87 | - | } | |
| 88 | 106 | w.WriteHeader(http.StatusOK) | |
| 89 | 107 | if r.Method != http.MethodHead { | |
| 90 | - | _, _ = io.Copy(w, zr) | |
| 108 | + | _, _ = io.Copy(w, plain) | |
| 91 | 109 | } | |
| 92 | 110 | } | |
| 93 | 111 | ||
| @@ -119,7 +137,12 @@ func (s *Server) buildPaths(ctx context.Context, repo, sha string) ([]byte, erro | |||
| 119 | 137 | if err := zw.Close(); err != nil { | |
| 120 | 138 | return nil, err | |
| 121 | 139 | } | |
| 122 | - | return buf.Bytes(), nil | |
| 140 | + | // Not buf.Bytes(): the buffer's spare capacity would stay in the cache | |
| 141 | + | // without being counted in its budget. (bytes.Clone may round the | |
| 142 | + | // capacity up, so the copy is exact.) | |
| 143 | + | body := make([]byte, buf.Len()) | |
| 144 | + | copy(body, buf.Bytes()) | |
| 145 | + | return body, nil | |
| 123 | 146 | } | |
| 124 | 147 | ||
| 125 | 148 | // acceptsGzip reports whether an Accept-Encoding header allows gzip. | |
| @@ -33,7 +33,9 @@ type pathList struct { | |||||
| 33 | // changes for it, but who may read it does, so every request is authorized | 33 | // changes for it, but who may read it does, so every request is authorized | ||
| 34 | // first (ResolveRevision runs the access rule) and only then served from the | 34 | // first (ResolveRevision runs the access rule) and only then served from the | ||
| 35 | // cache of lists. Caching follows raw: a visitor revalidates every time, with | 35 | // cache of lists. Caching follows raw: a visitor revalidates every time, with | ||
| 36 | - | // an ETag that makes it a 304; the owner's copy is private and never stored. | 36 | + | // an ETag that makes it a 304 (answered before the list is loaded, so it never |
| 37 | + | // waits for a build); the owner's copy is private and never stored. The ETag | |||
| 38 | + | // differs for the gzip and the plain variant. | |||
| 37 | func (s *Server) paths(w http.ResponseWriter, r *http.Request) { | 39 | func (s *Server) paths(w http.ResponseWriter, r *http.Request) { | ||
| 38 | ctx, cancel := context.WithTimeout(r.Context(), requestTimeout) | 40 | ctx, cancel := context.WithTimeout(r.Context(), requestTimeout) | ||
| 39 | defer cancel() | 41 | defer cancel() | ||
| @@ -53,25 +55,46 @@ func (s *Server) paths(w http.ResponseWriter, r *http.Request) { | |||||
| 53 | s.fail(ctx, w, r, errNotFound) | 55 | s.fail(ctx, w, r, errNotFound) | ||
| 54 | return | 56 | return | ||
| 55 | } | 57 | } | ||
| 56 | - | body, err := s.pathCache.load(ctx, sha, func() ([]byte, error) { return s.buildPaths(ctx, repo, sha) }) | 58 | + | |
| 59 | + | // The validator names the variant that would be served: the gzip bytes | |||
| 60 | + | // and the plain JSON are different representations. | |||
| 61 | + | gz := acceptsGzip(r.Header.Get("Accept-Encoding")) | |||
| 62 | + | etag := `"paths-` + sha + `"` | |||
| 63 | + | if gz { | |||
| 64 | + | etag = `"paths-` + sha + `-gz"` | |||
| 65 | + | } | |||
| 66 | + | // Revalidated every time, as raw is: a repository can turn private, and a | |||
| 67 | + | // shared cache must not keep serving its files. The ETag keeps it cheap. | |||
| 68 | + | validators := func(h http.Header) { | |||
| 69 | + | h.Set("Vary", "Accept-Encoding") | |||
| 70 | + | h.Set("ETag", etag) | |||
| 71 | + | cacheHeaders(ctx, h, "no-cache", sessionCache) | |||
| 72 | + | } | |||
| 73 | + | // Access is settled: a revalidation is answered without the list, so it | |||
| 74 | + | // never waits for a build. | |||
| 75 | + | if etagMatches(r.Header.Get("If-None-Match"), etag) { | |||
| 76 | + | validators(w.Header()) | |||
| 77 | + | w.WriteHeader(http.StatusNotModified) | |||
| 78 | + | return | |||
| 79 | + | } | |||
| 80 | + | body, err := s.pathCache.load(ctx, repo, sha, func() ([]byte, error) { return s.buildPaths(ctx, repo, sha) }) | |||
| 57 | if err != nil { | 81 | if err != nil { | ||
| 58 | s.fail(ctx, w, r, err) | 82 | s.fail(ctx, w, r, err) | ||
| 59 | return | 83 | return | ||
| 60 | } | 84 | } | ||
| 85 | + | var plain *gzip.Reader | |||
| 86 | + | if !gz { | |||
| 87 | + | if plain, err = gzip.NewReader(bytes.NewReader(body)); err != nil { | |||
| 88 | + | s.fail(ctx, w, r, err) | |||
| 89 | + | return | |||
| 90 | + | } | |||
| 91 | + | } | |||
| 61 | 92 | ||||
| 62 | h := w.Header() | 93 | h := w.Header() | ||
| 63 | h.Set("Content-Type", "application/json") | 94 | h.Set("Content-Type", "application/json") | ||
| 64 | h.Set("X-Content-Type-Options", "nosniff") | 95 | h.Set("X-Content-Type-Options", "nosniff") | ||
| 65 | - | h.Set("Vary", "Accept-Encoding") | 96 | + | validators(h) |
| 66 | - | h.Set("ETag", `"paths-`+sha+`"`) | 97 | + | if gz { |
| 67 | - | // Revalidated every time, as raw is: a repository can turn private, and a | |||
| 68 | - | // shared cache must not keep serving its files. The ETag keeps it cheap. | |||
| 69 | - | cacheHeaders(ctx, h, "no-cache", sessionCache) | |||
| 70 | - | if etagMatches(r.Header.Get("If-None-Match"), h.Get("ETag")) { | |||
| 71 | - | w.WriteHeader(http.StatusNotModified) | |||
| 72 | - | return | |||
| 73 | - | } | |||
| 74 | - | if acceptsGzip(r.Header.Get("Accept-Encoding")) { | |||
| 75 | h.Set("Content-Encoding", "gzip") | 98 | h.Set("Content-Encoding", "gzip") | ||
| 76 | h.Set("Content-Length", strconv.Itoa(len(body))) | 99 | h.Set("Content-Length", strconv.Itoa(len(body))) | ||
| 77 | w.WriteHeader(http.StatusOK) | 100 | w.WriteHeader(http.StatusOK) | ||
| @@ -80,14 +103,9 @@ func (s *Server) paths(w http.ResponseWriter, r *http.Request) { | |||||
| 80 | } | 103 | } | ||
| 81 | return | 104 | return | ||
| 82 | } | 105 | } | ||
| 83 | - | zr, err := gzip.NewReader(bytes.NewReader(body)) | |||
| 84 | - | if err != nil { | |||
| 85 | - | s.fail(ctx, w, r, err) | |||
| 86 | - | return | |||
| 87 | - | } | |||
| 88 | w.WriteHeader(http.StatusOK) | 106 | w.WriteHeader(http.StatusOK) | ||
| 89 | if r.Method != http.MethodHead { | 107 | if r.Method != http.MethodHead { | ||
| 90 | - | _, _ = io.Copy(w, zr) | 108 | + | _, _ = io.Copy(w, plain) |
| 91 | } | 109 | } | ||
| 92 | } | 110 | } | ||
| 93 | 111 | ||||
| @@ -119,7 +137,12 @@ func (s *Server) buildPaths(ctx context.Context, repo, sha string) ([]byte, erro | |||||
| 119 | if err := zw.Close(); err != nil { | 137 | if err := zw.Close(); err != nil { | ||
| 120 | return nil, err | 138 | return nil, err | ||
| 121 | } | 139 | } | ||
| 122 | - | return buf.Bytes(), nil | 140 | + | // Not buf.Bytes(): the buffer's spare capacity would stay in the cache |
| 141 | + | // without being counted in its budget. (bytes.Clone may round the | |||
| 142 | + | // capacity up, so the copy is exact.) | |||
| 143 | + | body := make([]byte, buf.Len()) | |||
| 144 | + | copy(body, buf.Bytes()) | |||
| 145 | + | return body, nil | |||
| 123 | } | 146 | } | ||
| 124 | 147 | ||||
| 125 | // acceptsGzip reports whether an Accept-Encoding header allows gzip. | 148 | // acceptsGzip reports whether an Accept-Encoding header allows gzip. | ||
A internal/adapters/web/paths_test.go +153 -0
| @@ -0,0 +1,153 @@ | |||
| 1 | + | // SPDX-License-Identifier: AGPL-3.0-or-later | |
| 2 | + | ||
| 3 | + | package web | |
| 4 | + | ||
| 5 | + | import ( | |
| 6 | + | "compress/gzip" | |
| 7 | + | "context" | |
| 8 | + | "io" | |
| 9 | + | "net/http" | |
| 10 | + | "net/http/httptest" | |
| 11 | + | "strings" | |
| 12 | + | "sync/atomic" | |
| 13 | + | "testing" | |
| 14 | + | ||
| 15 | + | "connectrpc.com/connect" | |
| 16 | + | "github.com/stretchr/testify/assert" | |
| 17 | + | "github.com/stretchr/testify/require" | |
| 18 | + | ||
| 19 | + | pb "bnema.dev/ermite-api/gen/ermite" | |
| 20 | + | "bnema.dev/ermite-api/gen/ermite/ermiteconnect" | |
| 21 | + | ) | |
| 22 | + | ||
| 23 | + | // pathsGit is a Git client that resolves any revision to itself and counts | |
| 24 | + | // the lists it builds. | |
| 25 | + | type pathsGit struct { | |
| 26 | + | ermiteconnect.GitServiceClient | |
| 27 | + | lists atomic.Int32 | |
| 28 | + | } | |
| 29 | + | ||
| 30 | + | func (g *pathsGit) ResolveRevision(_ context.Context, req *connect.Request[pb.ResolveRevisionRequest]) (*connect.Response[pb.ResolveRevisionResponse], error) { | |
| 31 | + | return connect.NewResponse(&pb.ResolveRevisionResponse{CommitId: req.Msg.GetRevision()}), nil | |
| 32 | + | } | |
| 33 | + | ||
| 34 | + | func (g *pathsGit) ListPaths(_ context.Context, req *connect.Request[pb.ListPathsRequest]) (*connect.Response[pb.ListPathsResponse], error) { | |
| 35 | + | g.lists.Add(1) | |
| 36 | + | return connect.NewResponse(&pb.ListPathsResponse{CommitId: req.Msg.GetRevision(), Paths: []string{"a.go", "b/c.go"}}), nil | |
| 37 | + | } | |
| 38 | + | ||
| 39 | + | // A revalidation is answered after the access check and before the list is | |
| 40 | + | // loaded: it never builds. Each variant has its own validator. | |
| 41 | + | func TestPathsRevalidationDoesNotBuild(t *testing.T) { | |
| 42 | + | git := &pathsGit{} | |
| 43 | + | srv, err := New(Config{ | |
| 44 | + | Repos: failing{}, Git: git, | |
| 45 | + | PublicURL: "https://forge.example", SourceURL: "https://forge.example/ermite", Version: "v1", | |
| 46 | + | }) | |
| 47 | + | require.NoError(t, err) | |
| 48 | + | sha := strings.Repeat("a", 40) | |
| 49 | + | get := func(hdr map[string]string) *httptest.ResponseRecorder { | |
| 50 | + | req := httptest.NewRequest(http.MethodGet, "/r/paths/"+sha, nil) | |
| 51 | + | for k, v := range hdr { | |
| 52 | + | req.Header.Set(k, v) | |
| 53 | + | } | |
| 54 | + | rec := httptest.NewRecorder() | |
| 55 | + | srv.ServeHTTP(rec, req) | |
| 56 | + | return rec | |
| 57 | + | } | |
| 58 | + | gzTag, plainTag := `"paths-`+sha+`-gz"`, `"paths-`+sha+`"` | |
| 59 | + | ||
| 60 | + | for _, tt := range []struct{ name, ae, inm string }{ | |
| 61 | + | {"gzip", "gzip", gzTag}, | |
| 62 | + | {"identity", "", plainTag}, | |
| 63 | + | {"weak", "gzip", "W/" + gzTag}, | |
| 64 | + | {"any", "", "*"}, | |
| 65 | + | } { | |
| 66 | + | rec := get(map[string]string{"Accept-Encoding": tt.ae, "If-None-Match": tt.inm}) | |
| 67 | + | assert.Equal(t, http.StatusNotModified, rec.Code, tt.name) | |
| 68 | + | assert.Empty(t, rec.Body.String(), tt.name) | |
| 69 | + | assert.Equal(t, "no-cache", rec.Header().Get("Cache-Control"), tt.name) | |
| 70 | + | assert.Equal(t, "Accept-Encoding", rec.Header().Get("Vary"), tt.name) | |
| 71 | + | assert.NotEmpty(t, rec.Header().Get("ETag"), tt.name) | |
| 72 | + | } | |
| 73 | + | assert.Zero(t, git.lists.Load(), "a 304 never builds") | |
| 74 | + | ||
| 75 | + | // The validator of the other variant does not match. | |
| 76 | + | assert.Equal(t, http.StatusOK, get(map[string]string{"Accept-Encoding": "gzip", "If-None-Match": plainTag}).Code) | |
| 77 | + | assert.Equal(t, http.StatusOK, get(map[string]string{"If-None-Match": gzTag}).Code) | |
| 78 | + | assert.EqualValues(t, 1, git.lists.Load(), "built once, then cached") | |
| 79 | + | ||
| 80 | + | rec := get(map[string]string{"Accept-Encoding": "gzip"}) | |
| 81 | + | assert.Equal(t, gzTag, rec.Header().Get("ETag")) | |
| 82 | + | assert.Equal(t, "gzip", rec.Header().Get("Content-Encoding")) | |
| 83 | + | zr, err := gzip.NewReader(rec.Body) | |
| 84 | + | require.NoError(t, err) | |
| 85 | + | out, err := io.ReadAll(zr) | |
| 86 | + | require.NoError(t, err) | |
| 87 | + | assert.JSONEq(t, `{"paths":["a.go","b/c.go"],"truncated":false}`, string(out)) | |
| 88 | + | ||
| 89 | + | rec = get(nil) | |
| 90 | + | assert.Equal(t, plainTag, rec.Header().Get("ETag")) | |
| 91 | + | assert.Empty(t, rec.Header().Get("Content-Encoding")) | |
| 92 | + | assert.JSONEq(t, `{"paths":["a.go","b/c.go"],"truncated":false}`, rec.Body.String()) | |
| 93 | + | } | |
| 94 | + | ||
| 95 | + | // The cached list holds no spare capacity. | |
| 96 | + | func TestBuildPathsCachesATightBody(t *testing.T) { | |
| 97 | + | srv, err := New(Config{Repos: failing{}, Git: &pathsGit{}, PublicURL: "https://forge.example"}) | |
| 98 | + | require.NoError(t, err) | |
| 99 | + | body, err := srv.buildPaths(context.Background(), "r", strings.Repeat("a", 40)) | |
| 100 | + | require.NoError(t, err) | |
| 101 | + | assert.Equal(t, len(body), cap(body)) | |
| 102 | + | } | |
| 103 | + | ||
| 104 | + | func TestAcceptsGzip(t *testing.T) { | |
| 105 | + | for _, tt := range []struct { | |
| 106 | + | header string | |
| 107 | + | want bool | |
| 108 | + | }{ | |
| 109 | + | {"", false}, | |
| 110 | + | {"gzip", true}, | |
| 111 | + | {"GZip", true}, | |
| 112 | + | {"deflate, gzip", true}, | |
| 113 | + | {"deflate, br", false}, | |
| 114 | + | {"gzip;q=0", false}, | |
| 115 | + | {"gzip; q=0", false}, | |
| 116 | + | {"gzip;q=0.0", false}, | |
| 117 | + | {"gzip ; q = 0", false}, | |
| 118 | + | {"gzip;q=0.5", true}, | |
| 119 | + | {"gzip;q=1", true}, | |
| 120 | + | {"identity, gzip;q=0", false}, | |
| 121 | + | {"gzip;q=0, deflate", false}, | |
| 122 | + | {"gzip;q=bad", true}, | |
| 123 | + | {"x-gzip", false}, | |
| 124 | + | // A wildcard is not read: the finder asks for gzip by name, and the | |
| 125 | + | // plain body is always right. | |
| 126 | + | {"*", false}, | |
| 127 | + | } { | |
| 128 | + | assert.Equal(t, tt.want, acceptsGzip(tt.header), "%q", tt.header) | |
| 129 | + | } | |
| 130 | + | } | |
| 131 | + | ||
| 132 | + | func TestEtagMatches(t *testing.T) { | |
| 133 | + | const tag = `"paths-x"` | |
| 134 | + | for _, tt := range []struct { | |
| 135 | + | header string | |
| 136 | + | want bool | |
| 137 | + | }{ | |
| 138 | + | {"", false}, | |
| 139 | + | {tag, true}, | |
| 140 | + | {"W/" + tag, true}, | |
| 141 | + | {`"other"`, false}, | |
| 142 | + | {`"a", ` + tag, true}, | |
| 143 | + | {`"a",` + tag + `,"b"`, true}, | |
| 144 | + | {`"a", W/` + tag, true}, | |
| 145 | + | {`"a", "b"`, false}, | |
| 146 | + | {"*", true}, | |
| 147 | + | {`"a", *`, true}, | |
| 148 | + | {`"paths-x-gz"`, false}, | |
| 149 | + | {`w/` + tag, false}, | |
| 150 | + | } { | |
| 151 | + | assert.Equal(t, tt.want, etagMatches(tt.header, tag), "%q", tt.header) | |
| 152 | + | } | |
| 153 | + | } | |
| @@ -0,0 +1,153 @@ | |||||
| 1 | + | // SPDX-License-Identifier: AGPL-3.0-or-later | |||
| 2 | + | ||||
| 3 | + | package web | |||
| 4 | + | ||||
| 5 | + | import ( | |||
| 6 | + | "compress/gzip" | |||
| 7 | + | "context" | |||
| 8 | + | "io" | |||
| 9 | + | "net/http" | |||
| 10 | + | "net/http/httptest" | |||
| 11 | + | "strings" | |||
| 12 | + | "sync/atomic" | |||
| 13 | + | "testing" | |||
| 14 | + | ||||
| 15 | + | "connectrpc.com/connect" | |||
| 16 | + | "github.com/stretchr/testify/assert" | |||
| 17 | + | "github.com/stretchr/testify/require" | |||
| 18 | + | ||||
| 19 | + | pb "bnema.dev/ermite-api/gen/ermite" | |||
| 20 | + | "bnema.dev/ermite-api/gen/ermite/ermiteconnect" | |||
| 21 | + | ) | |||
| 22 | + | ||||
| 23 | + | // pathsGit is a Git client that resolves any revision to itself and counts | |||
| 24 | + | // the lists it builds. | |||
| 25 | + | type pathsGit struct { | |||
| 26 | + | ermiteconnect.GitServiceClient | |||
| 27 | + | lists atomic.Int32 | |||
| 28 | + | } | |||
| 29 | + | ||||
| 30 | + | func (g *pathsGit) ResolveRevision(_ context.Context, req *connect.Request[pb.ResolveRevisionRequest]) (*connect.Response[pb.ResolveRevisionResponse], error) { | |||
| 31 | + | return connect.NewResponse(&pb.ResolveRevisionResponse{CommitId: req.Msg.GetRevision()}), nil | |||
| 32 | + | } | |||
| 33 | + | ||||
| 34 | + | func (g *pathsGit) ListPaths(_ context.Context, req *connect.Request[pb.ListPathsRequest]) (*connect.Response[pb.ListPathsResponse], error) { | |||
| 35 | + | g.lists.Add(1) | |||
| 36 | + | return connect.NewResponse(&pb.ListPathsResponse{CommitId: req.Msg.GetRevision(), Paths: []string{"a.go", "b/c.go"}}), nil | |||
| 37 | + | } | |||
| 38 | + | ||||
| 39 | + | // A revalidation is answered after the access check and before the list is | |||
| 40 | + | // loaded: it never builds. Each variant has its own validator. | |||
| 41 | + | func TestPathsRevalidationDoesNotBuild(t *testing.T) { | |||
| 42 | + | git := &pathsGit{} | |||
| 43 | + | srv, err := New(Config{ | |||
| 44 | + | Repos: failing{}, Git: git, | |||
| 45 | + | PublicURL: "https://forge.example", SourceURL: "https://forge.example/ermite", Version: "v1", | |||
| 46 | + | }) | |||
| 47 | + | require.NoError(t, err) | |||
| 48 | + | sha := strings.Repeat("a", 40) | |||
| 49 | + | get := func(hdr map[string]string) *httptest.ResponseRecorder { | |||
| 50 | + | req := httptest.NewRequest(http.MethodGet, "/r/paths/"+sha, nil) | |||
| 51 | + | for k, v := range hdr { | |||
| 52 | + | req.Header.Set(k, v) | |||
| 53 | + | } | |||
| 54 | + | rec := httptest.NewRecorder() | |||
| 55 | + | srv.ServeHTTP(rec, req) | |||
| 56 | + | return rec | |||
| 57 | + | } | |||
| 58 | + | gzTag, plainTag := `"paths-`+sha+`-gz"`, `"paths-`+sha+`"` | |||
| 59 | + | ||||
| 60 | + | for _, tt := range []struct{ name, ae, inm string }{ | |||
| 61 | + | {"gzip", "gzip", gzTag}, | |||
| 62 | + | {"identity", "", plainTag}, | |||
| 63 | + | {"weak", "gzip", "W/" + gzTag}, | |||
| 64 | + | {"any", "", "*"}, | |||
| 65 | + | } { | |||
| 66 | + | rec := get(map[string]string{"Accept-Encoding": tt.ae, "If-None-Match": tt.inm}) | |||
| 67 | + | assert.Equal(t, http.StatusNotModified, rec.Code, tt.name) | |||
| 68 | + | assert.Empty(t, rec.Body.String(), tt.name) | |||
| 69 | + | assert.Equal(t, "no-cache", rec.Header().Get("Cache-Control"), tt.name) | |||
| 70 | + | assert.Equal(t, "Accept-Encoding", rec.Header().Get("Vary"), tt.name) | |||
| 71 | + | assert.NotEmpty(t, rec.Header().Get("ETag"), tt.name) | |||
| 72 | + | } | |||
| 73 | + | assert.Zero(t, git.lists.Load(), "a 304 never builds") | |||
| 74 | + | ||||
| 75 | + | // The validator of the other variant does not match. | |||
| 76 | + | assert.Equal(t, http.StatusOK, get(map[string]string{"Accept-Encoding": "gzip", "If-None-Match": plainTag}).Code) | |||
| 77 | + | assert.Equal(t, http.StatusOK, get(map[string]string{"If-None-Match": gzTag}).Code) | |||
| 78 | + | assert.EqualValues(t, 1, git.lists.Load(), "built once, then cached") | |||
| 79 | + | ||||
| 80 | + | rec := get(map[string]string{"Accept-Encoding": "gzip"}) | |||
| 81 | + | assert.Equal(t, gzTag, rec.Header().Get("ETag")) | |||
| 82 | + | assert.Equal(t, "gzip", rec.Header().Get("Content-Encoding")) | |||
| 83 | + | zr, err := gzip.NewReader(rec.Body) | |||
| 84 | + | require.NoError(t, err) | |||
| 85 | + | out, err := io.ReadAll(zr) | |||
| 86 | + | require.NoError(t, err) | |||
| 87 | + | assert.JSONEq(t, `{"paths":["a.go","b/c.go"],"truncated":false}`, string(out)) | |||
| 88 | + | ||||
| 89 | + | rec = get(nil) | |||
| 90 | + | assert.Equal(t, plainTag, rec.Header().Get("ETag")) | |||
| 91 | + | assert.Empty(t, rec.Header().Get("Content-Encoding")) | |||
| 92 | + | assert.JSONEq(t, `{"paths":["a.go","b/c.go"],"truncated":false}`, rec.Body.String()) | |||
| 93 | + | } | |||
| 94 | + | ||||
| 95 | + | // The cached list holds no spare capacity. | |||
| 96 | + | func TestBuildPathsCachesATightBody(t *testing.T) { | |||
| 97 | + | srv, err := New(Config{Repos: failing{}, Git: &pathsGit{}, PublicURL: "https://forge.example"}) | |||
| 98 | + | require.NoError(t, err) | |||
| 99 | + | body, err := srv.buildPaths(context.Background(), "r", strings.Repeat("a", 40)) | |||
| 100 | + | require.NoError(t, err) | |||
| 101 | + | assert.Equal(t, len(body), cap(body)) | |||
| 102 | + | } | |||
| 103 | + | ||||
| 104 | + | func TestAcceptsGzip(t *testing.T) { | |||
| 105 | + | for _, tt := range []struct { | |||
| 106 | + | header string | |||
| 107 | + | want bool | |||
| 108 | + | }{ | |||
| 109 | + | {"", false}, | |||
| 110 | + | {"gzip", true}, | |||
| 111 | + | {"GZip", true}, | |||
| 112 | + | {"deflate, gzip", true}, | |||
| 113 | + | {"deflate, br", false}, | |||
| 114 | + | {"gzip;q=0", false}, | |||
| 115 | + | {"gzip; q=0", false}, | |||
| 116 | + | {"gzip;q=0.0", false}, | |||
| 117 | + | {"gzip ; q = 0", false}, | |||
| 118 | + | {"gzip;q=0.5", true}, | |||
| 119 | + | {"gzip;q=1", true}, | |||
| 120 | + | {"identity, gzip;q=0", false}, | |||
| 121 | + | {"gzip;q=0, deflate", false}, | |||
| 122 | + | {"gzip;q=bad", true}, | |||
| 123 | + | {"x-gzip", false}, | |||
| 124 | + | // A wildcard is not read: the finder asks for gzip by name, and the | |||
| 125 | + | // plain body is always right. | |||
| 126 | + | {"*", false}, | |||
| 127 | + | } { | |||
| 128 | + | assert.Equal(t, tt.want, acceptsGzip(tt.header), "%q", tt.header) | |||
| 129 | + | } | |||
| 130 | + | } | |||
| 131 | + | ||||
| 132 | + | func TestEtagMatches(t *testing.T) { | |||
| 133 | + | const tag = `"paths-x"` | |||
| 134 | + | for _, tt := range []struct { | |||
| 135 | + | header string | |||
| 136 | + | want bool | |||
| 137 | + | }{ | |||
| 138 | + | {"", false}, | |||
| 139 | + | {tag, true}, | |||
| 140 | + | {"W/" + tag, true}, | |||
| 141 | + | {`"other"`, false}, | |||
| 142 | + | {`"a", ` + tag, true}, | |||
| 143 | + | {`"a",` + tag + `,"b"`, true}, | |||
| 144 | + | {`"a", W/` + tag, true}, | |||
| 145 | + | {`"a", "b"`, false}, | |||
| 146 | + | {"*", true}, | |||
| 147 | + | {`"a", *`, true}, | |||
| 148 | + | {`"paths-x-gz"`, false}, | |||
| 149 | + | {`w/` + tag, false}, | |||
| 150 | + | } { | |||
| 151 | + | assert.Equal(t, tt.want, etagMatches(tt.header, tag), "%q", tt.header) | |||
| 152 | + | } | |||
| 153 | + | } | |||