Skip to content

Commit e7a66cb

Browse files
tonistiigitonistiigi
committed
http: fix release race between cache and snapshot
Signed-off-by: Tonis Tiigi <[email protected]>
1 parent 7bb9231 commit e7a66cb

File tree

7 files changed

+281
-47
lines changed

7 files changed

+281
-47
lines changed

‎client/client_test.go‎

Lines changed: 106 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,7 @@ var allTests = []func(t *testing.T, sb integration.Sandbox){
243243
testMetadataOnlyLocal,
244244
testGitResolveSourceMetadata,
245245
testHTTPResolveSourceMetadata,
246+
testHTTPPruneAfterCacheKey,
246247
}
247248

248249
func TestIntegration(t *testing.T) {
@@ -3223,13 +3224,13 @@ func testBuildHTTPSource(t *testing.T, sb integration.Sandbox) {
32233224

32243225
modTime := time.Now().Add(-24 * time.Hour) // avoid falso positive with current time
32253226

3226-
resp := httpserver.Response{
3227+
resp := &httpserver.Response{
32273228
Etag: identity.NewID(),
32283229
Content: []byte("content1"),
32293230
LastModified: &modTime,
32303231
}
32313232

3232-
server := httpserver.NewTestServer(map[string]httpserver.Response{
3233+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
32333234
"/foo": resp,
32343235
})
32353236
defer server.Close()
@@ -3293,7 +3294,7 @@ func testBuildHTTPSource(t *testing.T, sb integration.Sandbox) {
32933294
require.NoError(t, err)
32943295
require.NoError(t, gw.Close())
32953296
gzipBytes := buf.Bytes()
3296-
respGzip := httpserver.Response{
3297+
respGzip := &httpserver.Response{
32973298
Etag: identity.NewID(),
32983299
Content: gzipBytes,
32993300
LastModified: &modTime,
@@ -3373,18 +3374,18 @@ func testBuildHTTPSourceEtagScope(t *testing.T, sb integration.Sandbox) {
33733374
modTime := time.Now().Add(-24 * time.Hour) // avoid falso positive with current time
33743375

33753376
sharedEtag := identity.NewID()
3376-
resp := httpserver.Response{
3377+
resp := &httpserver.Response{
33773378
Etag: sharedEtag,
33783379
Content: []byte("content1"),
33793380
LastModified: &modTime,
33803381
}
3381-
resp2 := httpserver.Response{
3382+
resp2 := &httpserver.Response{
33823383
Etag: sharedEtag,
33833384
Content: []byte("another"),
33843385
LastModified: &modTime,
33853386
}
33863387

3387-
server := httpserver.NewTestServer(map[string]httpserver.Response{
3388+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
33883389
"/one/foo": resp,
33893390
"/two/foo": resp2,
33903391
})
@@ -3468,13 +3469,13 @@ func testBuildHTTPSourceAuthHeaderSecret(t *testing.T, sb integration.Sandbox) {
34683469

34693470
modTime := time.Now().Add(-24 * time.Hour) // avoid false positive with current time
34703471

3471-
resp := httpserver.Response{
3472+
resp := &httpserver.Response{
34723473
Etag: identity.NewID(),
34733474
Content: []byte("content1"),
34743475
LastModified: &modTime,
34753476
}
34763477

3477-
server := httpserver.NewTestServer(map[string]httpserver.Response{
3478+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
34783479
"/foo": resp,
34793480
})
34803481
defer server.Close()
@@ -3509,13 +3510,13 @@ func testBuildHTTPSourceHostTokenSecret(t *testing.T, sb integration.Sandbox) {
35093510

35103511
modTime := time.Now().Add(-24 * time.Hour) // avoid false positive with current time
35113512

3512-
resp := httpserver.Response{
3513+
resp := &httpserver.Response{
35133514
Etag: identity.NewID(),
35143515
Content: []byte("content1"),
35153516
LastModified: &modTime,
35163517
}
35173518

3518-
server := httpserver.NewTestServer(map[string]httpserver.Response{
3519+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
35193520
"/foo": resp,
35203521
})
35213522
defer server.Close()
@@ -3550,13 +3551,13 @@ func testBuildHTTPSourceHeader(t *testing.T, sb integration.Sandbox) {
35503551

35513552
modTime := time.Now().Add(-24 * time.Hour) // avoid falso positive with current time
35523553

3553-
resp := httpserver.Response{
3554+
resp := &httpserver.Response{
35543555
Etag: identity.NewID(),
35553556
Content: []byte("content1"),
35563557
LastModified: &modTime,
35573558
}
35583559

3559-
server := httpserver.NewTestServer(map[string]httpserver.Response{
3560+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
35603561
"/foo": resp,
35613562
})
35623563
defer server.Close()
@@ -12066,19 +12067,19 @@ func testHTTPResolveSourceMetadata(t *testing.T, sb integration.Sandbox) {
1206612067

1206712068
modTime := time.Now().Add(-24 * time.Hour) // avoid falso positive with current time
1206812069

12069-
resp := httpserver.Response{
12070+
resp := &httpserver.Response{
1207012071
Etag: identity.NewID(),
1207112072
Content: []byte("content1"),
1207212073
LastModified: &modTime,
1207312074
}
1207412075

12075-
resp2 := httpserver.Response{
12076+
resp2 := &httpserver.Response{
1207612077
Etag: identity.NewID(),
1207712078
Content: []byte("content2"),
1207812079
ContentDisposition: "attachment; filename=\"my img.jpg\"",
1207912080
}
1208012081

12081-
server := httpserver.NewTestServer(map[string]httpserver.Response{
12082+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
1208212083
"/foo": resp,
1208312084
"/bar": resp2,
1208412085
})
@@ -12116,6 +12117,96 @@ func testHTTPResolveSourceMetadata(t *testing.T, sb integration.Sandbox) {
1211612117
require.NoError(t, err)
1211712118
}
1211812119

12120+
func testHTTPPruneAfterCacheKey(t *testing.T, sb integration.Sandbox) {
12121+
// this test depends on hitting race condition in internal functions.
12122+
// If debugging and expecting failure you can add small sleep in beginning of source/http.Exec() to hit reliably
12123+
ctx := sb.Context()
12124+
c, err := New(ctx, sb.Address())
12125+
require.NoError(t, err)
12126+
defer c.Close()
12127+
12128+
resp := &httpserver.Response{
12129+
Etag: identity.NewID(),
12130+
Content: []byte("content1"),
12131+
}
12132+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
12133+
"/foo": resp,
12134+
})
12135+
defer server.Close()
12136+
12137+
done := make(chan struct{})
12138+
12139+
startScan := make(chan struct{})
12140+
stopScan := make(chan struct{})
12141+
pauseScan := make(chan struct{})
12142+
12143+
go func() {
12144+
// attempt to prune the HTTP record in between cachekey and snapshot
12145+
defer close(done)
12146+
for {
12147+
select {
12148+
case <-startScan:
12149+
scan:
12150+
for {
12151+
select {
12152+
case <-pauseScan:
12153+
break scan
12154+
default:
12155+
du, err := c.DiskUsage(ctx)
12156+
require.NoError(t, err)
12157+
for _, entry := range du {
12158+
if entry.Description == "http url "+server.URL+"/foo" {
12159+
if !entry.InUse {
12160+
t.Logf("entry no longer in use, pruning")
12161+
err = c.Prune(ctx, nil)
12162+
require.NoError(t, err)
12163+
12164+
resp.Etag = identity.NewID()
12165+
resp.Content = []byte("content2")
12166+
}
12167+
}
12168+
}
12169+
}
12170+
}
12171+
case <-stopScan:
12172+
return
12173+
}
12174+
}
12175+
}()
12176+
12177+
const iterations = 10
12178+
for range iterations {
12179+
startScan <- struct{}{}
12180+
resp.Etag = identity.NewID()
12181+
resp.Content = []byte("content1")
12182+
_, err = c.Build(ctx, SolveOpt{}, "test", func(ctx context.Context, c gateway.Client) (*gateway.Result, error) {
12183+
st := llb.Scratch().File(llb.Copy(llb.HTTP(server.URL+"/foo"), "foo", "bar"))
12184+
def, err := st.Marshal(sb.Context())
12185+
if err != nil {
12186+
return nil, err
12187+
}
12188+
resp, err := c.Solve(ctx, gateway.SolveRequest{
12189+
Definition: def.ToPB(),
12190+
})
12191+
if err != nil {
12192+
return nil, err
12193+
}
12194+
12195+
return resp, nil
12196+
}, nil)
12197+
require.NoError(t, err)
12198+
12199+
pauseScan <- struct{}{}
12200+
12201+
err = c.Prune(ctx, nil)
12202+
require.NoError(t, err)
12203+
12204+
checkAllReleasable(t, c, sb, false)
12205+
}
12206+
close(stopScan)
12207+
<-done
12208+
}
12209+
1211912210
func runInDir(dir string, cmds ...string) error {
1212012211
for _, args := range cmds {
1212112212
var cmd *exec.Cmd

‎frontend/dockerfile/dockerfile_addchecksum_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,11 @@ func testAddChecksum(t *testing.T, sb integration.Sandbox) {
2828
f := getFrontend(t, sb)
2929
f.RequiresBuildctl(t)
3030

31-
resp := httpserver.Response{
31+
resp := &httpserver.Response{
3232
Etag: identity.NewID(),
3333
Content: []byte("content1"),
3434
}
35-
server := httpserver.NewTestServer(map[string]httpserver.Response{
35+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
3636
"/foo": resp,
3737
})
3838
defer server.Close()

‎frontend/dockerfile/dockerfile_test.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2509,12 +2509,12 @@ RUN echo foo-contents> /foo
25092509
err := os.WriteFile(filepath.Join(srcDir, "Dockerfile"), dockerfile, 0600)
25102510
require.NoError(t, err)
25112511

2512-
resp := httpserver.Response{
2512+
resp := &httpserver.Response{
25132513
Etag: identity.NewID(),
25142514
Content: dockerfile,
25152515
}
25162516

2517-
server := httpserver.NewTestServer(map[string]httpserver.Response{
2517+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
25182518
"/df": resp,
25192519
})
25202520
defer server.Close()
@@ -3061,18 +3061,18 @@ func testDockerfileADDFromURL(t *testing.T, sb integration.Sandbox) {
30613061

30623062
modTime := time.Now().Add(-24 * time.Hour) // avoid falso positive with current time
30633063

3064-
resp := httpserver.Response{
3064+
resp := &httpserver.Response{
30653065
Etag: identity.NewID(),
30663066
Content: []byte("content1"),
30673067
}
30683068

3069-
resp2 := httpserver.Response{
3069+
resp2 := &httpserver.Response{
30703070
Etag: identity.NewID(),
30713071
LastModified: &modTime,
30723072
Content: []byte("content2"),
30733073
}
30743074

3075-
server := httpserver.NewTestServer(map[string]httpserver.Response{
3075+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
30763076
"/foo": resp,
30773077
"/": resp2,
30783078
})
@@ -3271,12 +3271,12 @@ COPY t.tar.gz /
32713271
require.Equal(t, buf2.Bytes(), dt)
32723272

32733273
// ADD from URL doesn't extract
3274-
resp := httpserver.Response{
3274+
resp := &httpserver.Response{
32753275
Etag: identity.NewID(),
32763276
Content: buf2.Bytes(),
32773277
}
32783278

3279-
server := httpserver.NewTestServer(map[string]httpserver.Response{
3279+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
32803280
"/t.tar.gz": resp,
32813281
})
32823282
defer server.Close()
@@ -4707,11 +4707,11 @@ func testAddURLChmod(t *testing.T, sb integration.Sandbox) {
47074707
f := getFrontend(t, sb)
47084708
f.RequiresBuildctl(t)
47094709

4710-
resp := httpserver.Response{
4710+
resp := &httpserver.Response{
47114711
Etag: identity.NewID(),
47124712
Content: []byte("content1"),
47134713
}
4714-
server := httpserver.NewTestServer(map[string]httpserver.Response{
4714+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
47154715
"/foo": resp,
47164716
})
47174717
defer server.Close()
@@ -4952,12 +4952,12 @@ COPY foo bar
49524952

49534953
require.NoError(t, w.Flush())
49544954

4955-
resp := httpserver.Response{
4955+
resp := &httpserver.Response{
49564956
Etag: identity.NewID(),
49574957
Content: buf.Bytes(),
49584958
}
49594959

4960-
server := httpserver.NewTestServer(map[string]httpserver.Response{
4960+
server := httpserver.NewTestServer(map[string]*httpserver.Response{
49614961
"/myurl": resp,
49624962
})
49634963
defer server.Close()

‎source/containerimage/pull.go‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,14 +87,18 @@ func mainManifestKey(desc ocispecs.Descriptor, platform ocispecs.Platform, layer
8787
}
8888

8989
func (p *puller) CacheKey(ctx context.Context, jobCtx solver.JobContext, index int) (cacheKey string, imgDigest string, cacheOpts solver.CacheOpts, cacheDone bool, err error) {
90+
var g session.Group
91+
if jobCtx != nil {
92+
g = jobCtx.Session()
93+
}
9094
var getResolver pull.SessionResolver
9195
switch p.ResolverType {
9296
case ResolverTypeRegistry:
93-
resolver := resolver.DefaultPool.GetResolver(p.RegistryHosts, p.Ref, "pull", p.SessionManager, jobCtx.Session()).WithImageStore(p.ImageStore, p.Mode)
97+
resolver := resolver.DefaultPool.GetResolver(p.RegistryHosts, p.Ref, "pull", p.SessionManager, g).WithImageStore(p.ImageStore, p.Mode)
9498
p.Resolver = resolver
9599
getResolver = func(g session.Group) remotes.Resolver { return resolver.WithSession(g) }
96100
case ResolverTypeOCILayout:
97-
resolver := getOCILayoutResolver(p.store, p.SessionManager, jobCtx.Session())
101+
resolver := getOCILayoutResolver(p.store, p.SessionManager, g)
98102
p.Resolver = resolver
99103
// OCILayout has no need for session
100104
getResolver = func(g session.Group) remotes.Resolver { return resolver }
@@ -207,14 +211,18 @@ func (p *puller) CacheKey(ctx context.Context, jobCtx solver.JobContext, index i
207211
}
208212

209213
func (p *puller) Snapshot(ctx context.Context, jobCtx solver.JobContext) (ir cache.ImmutableRef, err error) {
214+
var g session.Group
215+
if jobCtx != nil {
216+
g = jobCtx.Session()
217+
}
210218
var getResolver pull.SessionResolver
211219
switch p.ResolverType {
212220
case ResolverTypeRegistry:
213-
resolver := resolver.DefaultPool.GetResolver(p.RegistryHosts, p.Ref, "pull", p.SessionManager, jobCtx.Session()).WithImageStore(p.ImageStore, p.Mode)
221+
resolver := resolver.DefaultPool.GetResolver(p.RegistryHosts, p.Ref, "pull", p.SessionManager, g).WithImageStore(p.ImageStore, p.Mode)
214222
p.Resolver = resolver
215223
getResolver = func(g session.Group) remotes.Resolver { return resolver.WithSession(g) }
216224
case ResolverTypeOCILayout:
217-
resolver := getOCILayoutResolver(p.store, p.SessionManager, jobCtx.Session())
225+
resolver := getOCILayoutResolver(p.store, p.SessionManager, g)
218226
p.Resolver = resolver
219227
// OCILayout has no need for session
220228
getResolver = func(g session.Group) remotes.Resolver { return resolver }

‎source/http/source.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -382,7 +382,14 @@ func (hs *httpSourceHandler) resolveMetadata(ctx context.Context, jobCtx solver.
382382
if err != nil {
383383
return nil, err
384384
}
385-
ref.Release(context.TODO())
385+
cleanup := func() error {
386+
return ref.Release(context.TODO())
387+
}
388+
if jobCtx != nil {
389+
jobCtx.Cleanup(cleanup)
390+
} else {
391+
cleanup()
392+
}
386393

387394
var modTime *time.Time
388395
if modTimeStr := resp.Header.Get("Last-Modified"); modTimeStr != "" {

0 commit comments

Comments
 (0)