Skip to content

Commit 804106d

Browse files
mrnuggetmrnugget
authored
Deprecate old ExecutionCache file formats (#474)
* Deprecate old ExecutionCache file formats This fixes https://github.com/sourcegraph/sourcegraph/issues/17306 by deleting support for old cache files. The ticket is slightly wrong, since the previous code did already delete these files, so we don't have to deprecate and then delete support. We already transitioned to the new cache file format in 3.24 and 3.25 now. The next release, 3.26, will only read the new cache format. * Add changelog entry
1 parent 18446ae commit 804106d

File tree

3 files changed

+14
-185
lines changed

3 files changed

+14
-185
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ All notable changes to `src-cli` are documented in this file.
1515

1616
### Changed
1717

18+
- Deprecated cache file formats are not read by `src campaign [apply|preview]` anymore.
19+
1820
### Fixed
1921

2022
### Removed

‎internal/campaigns/execution_cache.go‎

Lines changed: 12 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"crypto/sha256"
66
"encoding/base64"
77
"encoding/json"
8-
"fmt"
98
"io/ioutil"
109
"os"
1110
"path/filepath"
@@ -91,75 +90,15 @@ func (c ExecutionDiskCache) Get(ctx context.Context, key ExecutionCacheKey) (exe
9190
return result, false, err
9291
}
9392

94-
// We try to be backwards compatible and see if we also find older cache
95-
// files.
96-
//
97-
// There are three different cache versions out in the wild and to be
98-
// backwards compatible we read all of them.
99-
//
100-
// In Sourcegraph/src-cli 3.26 we can remove the code here and simply read
101-
// the cache from `path`, since all the old cache files should be deleted
102-
// until then.
103-
globPattern := strings.TrimSuffix(path, cacheFileExt) + ".*"
104-
matches, err := filepath.Glob(globPattern)
105-
if err != nil {
106-
return result, false, err
107-
}
108-
109-
switch len(matches) {
110-
case 0:
111-
// Nothing found
93+
if _, err := os.Stat(path); os.IsNotExist(err) {
11294
return result, false, nil
113-
case 1:
114-
// One cache file found
115-
if err := c.readCacheFile(matches[0], &result); err != nil {
116-
return result, false, err
117-
}
118-
119-
// If it's an old cache file, we rewrite the cache and delete the old file
120-
if isOldCacheFile(matches[0]) {
121-
if err := c.Set(ctx, key, result); err != nil {
122-
return result, false, errors.Wrap(err, "failed to rewrite cache in new format")
123-
}
124-
if err := os.Remove(matches[0]); err != nil {
125-
return result, false, errors.Wrap(err, "failed to remove old cache file")
126-
}
127-
}
128-
129-
return result, true, err
130-
131-
default:
132-
// More than one cache file found.
133-
// Sort them so that we'll can possibly read from the one with the most
134-
// current version.
135-
sortCacheFiles(matches)
136-
137-
newest := matches[0]
138-
toDelete := matches[1:]
139-
140-
// Read from newest
141-
if err := c.readCacheFile(newest, &result); err != nil {
142-
return result, false, err
143-
}
144-
145-
// If the newest was also an older version, we write a new version...
146-
if isOldCacheFile(newest) {
147-
if err := c.Set(ctx, key, result); err != nil {
148-
return result, false, errors.Wrap(err, "failed to rewrite cache in new format")
149-
}
150-
// ... and mark the file also as to-be-deleted
151-
toDelete = append(toDelete, newest)
152-
}
153-
154-
// Now we clean up the old ones
155-
for _, path := range toDelete {
156-
if err := os.Remove(path); err != nil {
157-
return result, false, errors.Wrap(err, "failed to remove old cache file")
158-
}
159-
}
95+
}
16096

161-
return result, true, nil
97+
if err := c.readCacheFile(path, &result); err != nil {
98+
return result, false, err
16299
}
100+
101+
return result, true, nil
163102
}
164103

165104
// sortCacheFiles sorts cache file paths by their "version", so that files
@@ -178,50 +117,14 @@ func (c ExecutionDiskCache) readCacheFile(path string, result *executionResult)
178117
return err
179118
}
180119

181-
switch {
182-
case strings.HasSuffix(path, ".v3.json"):
183-
// v3 of the cache: we cache the diff and the outputs produced by the step.
184-
if err := json.Unmarshal(data, result); err != nil {
185-
// Delete the invalid data to avoid causing an error for next time.
186-
if err := os.Remove(path); err != nil {
187-
return errors.Wrap(err, "while deleting cache file with invalid JSON")
188-
}
189-
return errors.Wrapf(err, "reading cache file %s", path)
120+
if err := json.Unmarshal(data, result); err != nil {
121+
// Delete the invalid data to avoid causing an error for next time.
122+
if err := os.Remove(path); err != nil {
123+
return errors.Wrap(err, "while deleting cache file with invalid JSON")
190124
}
191-
return nil
192-
193-
case strings.HasSuffix(path, ".diff"):
194-
// v2 of the cache: we only cached the diff, since that's the
195-
// only bit of data we were interested in.
196-
result.Diff = string(data)
197-
result.Outputs = map[string]interface{}{}
198-
// Conversion is lossy, though: we don't populate result.StepChanges.
199-
result.ChangedFiles = &StepChanges{}
200-
201-
return nil
202-
203-
case strings.HasSuffix(path, ".json"):
204-
// v1 of the cache: we cached the complete ChangesetSpec instead of just the diffs.
205-
var spec ChangesetSpec
206-
if err := json.Unmarshal(data, &spec); err != nil {
207-
// Delete the invalid data to avoid causing an error for next time.
208-
if err := os.Remove(path); err != nil {
209-
return errors.Wrap(err, "while deleting cache file with invalid JSON")
210-
}
211-
return errors.Wrapf(err, "reading cache file %s", path)
212-
}
213-
if len(spec.Commits) != 1 {
214-
return errors.New("cached result has no commits")
215-
}
216-
217-
result.Diff = spec.Commits[0].Diff
218-
result.Outputs = map[string]interface{}{}
219-
result.ChangedFiles = &StepChanges{}
220-
221-
return nil
125+
return errors.Wrapf(err, "reading cache file %s", path)
222126
}
223-
224-
return fmt.Errorf("unknown file format for cache file %q", path)
127+
return nil
225128
}
226129

227130
func (c ExecutionDiskCache) Set(ctx context.Context, key ExecutionCacheKey, result executionResult) error {

‎internal/campaigns/execution_cache_test.go‎

Lines changed: 0 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -139,12 +139,6 @@ func TestExecutionDiskCache(t *testing.T) {
139139
Outputs: map[string]interface{}{},
140140
}
141141

142-
onlyDiff := executionResult{
143-
Diff: testDiff,
144-
ChangedFiles: &StepChanges{},
145-
Outputs: map[string]interface{}{},
146-
}
147-
148142
t.Run("cache contains v3 cache file", func(t *testing.T) {
149143
cache := ExecutionDiskCache{Dir: cacheTmpDir(t)}
150144

@@ -169,76 +163,6 @@ func TestExecutionDiskCache(t *testing.T) {
169163
}
170164
assertCacheMiss(t, cache, cacheKey1)
171165
})
172-
173-
t.Run("cache contains v1 cache file", func(t *testing.T) {
174-
cache := ExecutionDiskCache{Dir: cacheTmpDir(t)}
175-
176-
// Empty cache, no hit
177-
assertCacheMiss(t, cache, cacheKey1)
178-
179-
// Simulate old cache file lying around in cache
180-
oldFilePath := writeV1CacheFile(t, cache, cacheKey1, testDiff)
181-
182-
// Cache hit, but only for the diff
183-
assertCacheHit(t, cache, cacheKey1, onlyDiff)
184-
185-
// And the old file should be deleted
186-
assertFileDeleted(t, oldFilePath)
187-
// .. but we should still get a cache hit, because we rewrote the cache
188-
assertCacheHit(t, cache, cacheKey1, onlyDiff)
189-
})
190-
191-
t.Run("cache contains v2 cache file", func(t *testing.T) {
192-
cache := ExecutionDiskCache{Dir: cacheTmpDir(t)}
193-
194-
// Empty cache, no hit
195-
assertCacheMiss(t, cache, cacheKey1)
196-
197-
// Simulate old cache file lying around in cache
198-
oldFilePath := writeV2CacheFile(t, cache, cacheKey1, testDiff)
199-
200-
// Now we get a cache hit, but only for the diff
201-
assertCacheHit(t, cache, cacheKey1, onlyDiff)
202-
203-
// And the old file should be deleted
204-
assertFileDeleted(t, oldFilePath)
205-
// .. but we should still get a cache hit, because we rewrote the cache
206-
assertCacheHit(t, cache, cacheKey1, onlyDiff)
207-
})
208-
209-
t.Run("cache contains one old and one v3 cache file", func(t *testing.T) {
210-
cache := ExecutionDiskCache{Dir: cacheTmpDir(t)}
211-
212-
// Simulate v2 and v3 files in cache
213-
oldFilePath := writeV1CacheFile(t, cache, cacheKey1, testDiff)
214-
215-
if err := cache.Set(ctx, cacheKey1, value); err != nil {
216-
t.Fatalf("cache.Set returned unexpected error: %s", err)
217-
}
218-
219-
// Cache hit
220-
assertCacheHit(t, cache, cacheKey1, value)
221-
222-
// And the old file should be deleted
223-
assertFileDeleted(t, oldFilePath)
224-
})
225-
226-
t.Run("cache contains multiple old cache files", func(t *testing.T) {
227-
cache := ExecutionDiskCache{Dir: cacheTmpDir(t)}
228-
229-
// Simulate v1 and v2 files in cache
230-
oldFilePath1 := writeV1CacheFile(t, cache, cacheKey1, testDiff)
231-
oldFilePath2 := writeV1CacheFile(t, cache, cacheKey1, testDiff)
232-
233-
// Now we get a cache hit, but only for the diff
234-
assertCacheHit(t, cache, cacheKey1, onlyDiff)
235-
236-
// And the old files should be deleted
237-
assertFileDeleted(t, oldFilePath1)
238-
assertFileDeleted(t, oldFilePath2)
239-
// .. but we should still get a cache hit, because we rewrote the cache
240-
assertCacheHit(t, cache, cacheKey1, onlyDiff)
241-
})
242166
}
243167

244168
func TestSortCacheFiles(t *testing.T) {

0 commit comments

Comments
 (0)