Skip to content

Commit 131841b

Browse files
authored
Fix 'n' value returned by filecache.Write (#12012)
I don't think any callers rely on this value being correct, but if we're gonna return a value here, it may as well be the correct value?
1 parent 7f16472 commit 131841b

2 files changed

Lines changed: 37 additions & 3 deletions

File tree

enterprise/server/remote_execution/filecache/filecache.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -912,7 +912,10 @@ func (c *fileCache) Read(ctx context.Context, node *repb.FileNode) ([]byte, erro
912912
// Write atomically writes the given bytes to filecache.
913913
func (c *fileCache) Write(ctx context.Context, node *repb.FileNode, b []byte) (n int, err error) {
914914
if c.checkClosed() {
915-
return 0, nil
915+
// Like AddFile, treat writes as best-effort during shutdown: report
916+
// success to avoid failing callers, even though the data is not persisted
917+
// to the cache once the filecache is closed.
918+
return len(b), nil
916919
}
917920
tmp, err := c.tempPath(node.GetDigest().GetHash())
918921
if err != nil {
@@ -932,7 +935,7 @@ func (c *fileCache) Write(ctx context.Context, node *repb.FileNode, b []byte) (n
932935
if err := c.AddFile(ctx, node, tmp); err != nil {
933936
return 0, err
934937
}
935-
return n, nil
938+
return len(b), nil
936939
}
937940

938941
type verifiedWriter struct {

enterprise/server/remote_execution/filecache/filecache_test.go

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1268,15 +1268,46 @@ func TestFileCacheWriteCleansUpTempFile(t *testing.T) {
12681268
rn, buf := testdigest.RandomCASResourceBuf(t, 1024)
12691269
node := &repb.FileNode{Digest: rn.GetDigest()}
12701270

1271-
_, err = fc.Write(ctx, node, buf)
1271+
// Write the bytes through the convenience API and expect it to report the
1272+
// full accepted byte count.
1273+
n, err := fc.Write(ctx, node, buf)
12721274
require.NoError(t, err)
1275+
require.Equal(t, len(buf), n)
12731276

1277+
// The temporary staging file should be removed after the file is added to
1278+
// the cache.
12741279
pattern := filepath.Join(fc.TempDir(), rn.GetDigest().GetHash()+".*.tmp")
12751280
matches, err := filepath.Glob(pattern)
12761281
require.NoError(t, err)
12771282
require.Empty(t, matches, "expected temp file(s) to be deleted: %v", matches)
12781283
}
12791284

1285+
func TestFileCacheWriteAfterClose(t *testing.T) {
1286+
flags.Set(t, "executor.delete_filecache_on_unclean_shutdown", true)
1287+
ctx := context.Background()
1288+
fcDir := testfs.MakeTempDir(t)
1289+
outputDir := testfs.MakeTempDir(t)
1290+
fc, err := filecache.NewFileCache(fcDir, 100000, false)
1291+
require.NoError(t, err)
1292+
fc.WaitForDirectoryScanToComplete()
1293+
1294+
rn, buf := testdigest.RandomCASResourceBuf(t, 1024)
1295+
node := &repb.FileNode{Digest: rn.GetDigest()}
1296+
1297+
// Once the cache is closing, Write should silently accept the bytes so
1298+
// callers do not fail just because cache population is best-effort.
1299+
require.NoError(t, fc.Close())
1300+
n, err := fc.Write(ctx, node, buf)
1301+
require.NoError(t, err)
1302+
require.Equal(t, len(buf), n)
1303+
1304+
// The accepted bytes are intentionally dropped rather than added to the
1305+
// cache after shutdown starts.
1306+
linkedPath := filepath.Join(outputDir, "linked-after-close")
1307+
require.False(t, fc.FastLinkFile(ctx, node, linkedPath))
1308+
require.NoFileExists(t, linkedPath)
1309+
}
1310+
12801311
func TestTrackExternalDirectory_Basic(t *testing.T) {
12811312
ctx := context.Background()
12821313
fcDir := testfs.MakeTempDir(t)

0 commit comments

Comments
 (0)