native build directories hardlinking file fetcher de-duplicates in-flight download requests #206

HongboDu-at · 2026-01-07T19:22:30Z

From internal usage, this reduces fetching inputs(~4GB) time from ~60s to ~20s.

…ight download requests

aspect-workflows · 2026-01-07T19:25:18Z

Test

16 test targets passed

Targets

//pkg/blobstore:blobstore_test [k8-fastbuild]                                         233ms
//pkg/builder:builder_test [k8-fastbuild]                                             256ms
//pkg/cas:cas_test [k8-fastbuild]                                                     411ms
//pkg/cleaner:cleaner_test [k8-fastbuild]                                             180ms
//pkg/clock:clock_test [k8-fastbuild]                                                 197ms
//pkg/filesystem/pool:pool_test [k8-fastbuild]                                        310ms
//pkg/filesystem/virtual/fuse:fuse_test [k8-fastbuild]                                199ms
//pkg/filesystem/virtual/nfsv4:nfsv4_test [k8-fastbuild]                              333ms
//pkg/filesystem/virtual:virtual_test [k8-fastbuild]                                  529ms
//pkg/filesystem:filesystem_test [k8-fastbuild]                                       585ms
//pkg/runner:runner_test [k8-fastbuild]                                               156ms
//pkg/scheduler/initialsizeclass:initialsizeclass_test [k8-fastbuild]                 535ms
//pkg/scheduler/platform:platform_test [k8-fastbuild]                                 370ms
//pkg/scheduler/routing:routing_test [k8-fastbuild]                                   121ms
//pkg/scheduler:scheduler_test [k8-fastbuild]                                         439ms
//pkg/sync:sync_test [k8-fastbuild]                                                   164ms

Total test execution time was 5s. 3 tests (15.8%) were fully cached saving 418ms.

EdSchouten · 2026-01-07T21:00:27Z

pkg/cas/hardlinking_file_fetcher.go

+		select {
+		case <-d.wait:
+			if d.err != nil {
+				return d.err


This is incorrect. A caller of GetFile() may receive an error that was produced by another invocation. This should not happen, as errors may also include things like context cancelation, etc.

Updated to retry if it failed because of context cancelation

nit: bb_storage/pkg/auth/remoteAuthorizer.authorizeSingle also retries but has the same kind of flaw. Consider the case where the timeout is 55s, downloading the file takes 50s, one call is coming in every 30s and that the first call is cancelled after 35s. Then all retries for the later requests will also fail because when the retry starts, the context has less than 50s left.

0s RPC 0 30s RPC 1 45s RPC 0 cancelled, RPC 1 retries 60s RPC 2 85s RPC 1 times out (5s left to download), RPC 2 retries 90s RPC 3 115 RPC 2 times out (15s left to download), RPC 3 retries 120s RPC 4 115 RPC 3 times out (15s left to download), RPC 4 retries ...

One solution is to manually cancel a context.Background() when there are no requests left waiting. I don't know how complex the implementation and tests for this kind of solution will be or if this is just not worth bothering with.

Can't we just eliminate the error propagation logic entirely? Just change that downloads map to:

downloads map[string]<-chan struct{}

Only use that to wait an existing download to complete.

Thanks for the suggestion. Updated to use the new map.

EdSchouten · 2026-01-07T21:03:03Z

pkg/cas/hardlinking_file_fetcher.go

+	ff.downloads[key] = d
+	ff.downloadsLock.Unlock()
+
+	downloadErr := ff.base.GetFile(ctx, blobDigest, directory, name, isExecutable)


Am I correct that there is still a slight race here, where files get downloaded unnnecessarily/redundantly? Shouldn't we do another tryLinkFromCache() at this specific point?

Good catch. Added.

…undant download

moroten · 2026-01-08T07:53:19Z

pkg/cas/hardlinking_file_fetcher.go

+
+type download struct {
+	wait chan struct{}
+	err  error


Suggested change

err error

// err is available only when wait has been closed.

err error

moroten · 2026-01-08T07:55:34Z

pkg/cas/hardlinking_file_fetcher.go

+}
+
+type download struct {
+	wait chan struct{}


nit: It is possible to be more strict here with wait <-chan struct{} and further down use

wait := make(chan struct{}) d = &download{wait: wait} close(wait)

Thanks for the suggestion.

moroten · 2026-01-08T08:02:02Z

pkg/cas/hardlinking_file_fetcher.go

+	d = &download{wait: make(chan struct{})}
+	ff.downloads[key] = d
+	ff.downloadsLock.Unlock()


ff.downloadsLock does not necessarily need to be locked when setting d.err as it is only to be read after d.wait has been closed. This means that defer can be used.

Suggested change

d = &download{wait: make(chan struct{})}

ff.downloads[key] = d

ff.downloadsLock.Unlock()

wait := make(chan struct{})

d = &download{wait: wait}

ff.downloads[key] = d

ff.downloadsLock.Unlock()

defer func() {

ff.downloadsLock.Lock()

delete(ff.downloads, key)

ff.downloadsLock.Unlock()

close(wait)

}()

Thanks for the suggestion. Updated to use defer. The code is cleaner.

moroten · 2026-01-08T08:06:17Z

pkg/cas/hardlinking_file_fetcher.go

+	wasMissing, err = ff.tryLinkFromCache(key, directory, name)
+	if err == nil {
+		// File appeared in cache, no download needed.
+		ff.downloadsLock.Lock()
+		delete(ff.downloads, key)
+		ff.downloadsLock.Unlock()
+		close(d.wait)
+		return nil
+	} else if !wasMissing {
+		// tryLinkFromCache had a real error (not just missing).
+		ff.downloadsLock.Lock()
+		d.err = err
+		delete(ff.downloads, key)
+		ff.downloadsLock.Unlock()
+		close(d.wait)
 		return err
 	}


See comment on line 125.

Suggested change

wasMissing, err = ff.tryLinkFromCache(key, directory, name)

if err == nil {

// File appeared in cache, no download needed.

ff.downloadsLock.Lock()

delete(ff.downloads, key)

ff.downloadsLock.Unlock()

close(d.wait)

return nil

} else if !wasMissing {

// tryLinkFromCache had a real error (not just missing).

ff.downloadsLock.Lock()

d.err = err

delete(ff.downloads, key)

ff.downloadsLock.Unlock()

close(d.wait)

return err

}

wasMissing, d.err = ff.tryLinkFromCache(key, directory, name)

if !wasMissing {

// Either the file appeared in cache, no download needed,

// or tryLinkFromCache had a real error (not just missing).

return d.err

}

moroten · 2026-01-08T08:36:03Z

pkg/cas/hardlinking_file_fetcher.go

+		select {
+		case <-d.wait:
+			if d.err != nil {
+				return d.err


nit: bb_storage/pkg/auth/remoteAuthorizer.authorizeSingle also retries but has the same kind of flaw. Consider the case where the timeout is 55s, downloading the file takes 50s, one call is coming in every 30s and that the first call is cancelled after 35s. Then all retries for the later requests will also fail because when the retry starts, the context has less than 50s left.

0s RPC 0 30s RPC 1 45s RPC 0 cancelled, RPC 1 retries 60s RPC 2 85s RPC 1 times out (5s left to download), RPC 2 retries 90s RPC 3 115 RPC 2 times out (15s left to download), RPC 3 retries 120s RPC 4 115 RPC 3 times out (15s left to download), RPC 4 retries ...

One solution is to manually cancel a context.Background() when there are no requests left waiting. I don't know how complex the implementation and tests for this kind of solution will be or if this is just not worth bothering with.

HongboDu-at

Thank you for the review.

HongboDu-at · 2026-01-09T00:39:47Z

pkg/cas/hardlinking_file_fetcher.go

+}
+
+type download struct {
+	wait chan struct{}


Thanks for the suggestion.

HongboDu-at · 2026-01-09T00:41:31Z

pkg/cas/hardlinking_file_fetcher.go

+		select {
+		case <-d.wait:
+			if d.err != nil {
+				return d.err


Thanks for the suggestion. Updated to use the new map.

HongboDu-at · 2026-01-09T00:41:55Z

pkg/cas/hardlinking_file_fetcher.go

+	d = &download{wait: make(chan struct{})}
+	ff.downloads[key] = d
+	ff.downloadsLock.Unlock()


Thanks for the suggestion. Updated to use defer. The code is cleaner.

native build directories hardlinking file fetcher de-duplicates in-fl…

44ac449

…ight download requests

EdSchouten reviewed Jan 7, 2026

View reviewed changes

Callers retry download for context cancellation; fix one possible red…

5c7e965

…undant download

HongboDu-at requested a review from EdSchouten January 7, 2026 23:13

moroten reviewed Jan 8, 2026

View reviewed changes

use downloads map[string]<-chan struct{} and defer

cc74acf

HongboDu-at commented Jan 9, 2026

View reviewed changes

	err error
	// err is available only when wait has been closed.
	err error

native build directories hardlinking file fetcher de-duplicates in-flight download requests #206

Are you sure you want to change the base?

native build directories hardlinking file fetcher de-duplicates in-flight download requests #206

Conversation

HongboDu-at commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aspect-workflows bot commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HongboDu-at left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HongboDu-at commented Jan 7, 2026 •

edited

Loading

aspect-workflows bot commented Jan 7, 2026 •

edited

Loading