Skip to content

Commit

Permalink
feat(action): Verify no matching cache before save
Browse files Browse the repository at this point in the history
Prior to saving cache, verify that there is no cache with a matching key
already cached. Previously, we checked if a cache key matching our cache
was present in the load step; if not, we would then proceed to attempt
to save the cache in the save step (assuming we were not in read-only
mode and that there were indeed new Docker images to save). This was
problematic when the action was run in parallel; in that case, there
were multiple, unnecessary cache save attempts when the cache initially
missed but was subsequently saved.
  • Loading branch information
mwarres committed Mar 13, 2024
1 parent 055e746 commit c50da5e
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 17 deletions.
2 changes: 1 addition & 1 deletion dist/main/index.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/post/index.js

Large diffs are not rendered by default.

62 changes: 47 additions & 15 deletions src/docker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ describe("Docker images", (): void => {

const assertSaveDockerImages = (
cacheHit: boolean,
key: string,
readOnly = false,
prevSave = false,
): void => {
expect(core.getInput).nthCalledWith<[string, InputOptions]>(1, "key", {
required: true,
Expand All @@ -95,12 +97,20 @@ describe("Docker images", (): void => {
if (!cacheHit) {
expect(core.getInput).lastCalledWith("read-only");
if (!readOnly) {
expect(core.getState).lastCalledWith(docker.DOCKER_IMAGES_LIST);
expect(core.info).nthCalledWith<[string]>(1, "Listing Docker images.");
expect(util.execBashCommand).nthCalledWith<[string]>(
1,
'docker image list --format "{{ .Repository }}:{{ .Tag }}"',
);
expect(cache.restoreCache).lastCalledWith([""], key, [], {
lookupOnly: true,
});
if (!prevSave) {
expect(core.getState).lastCalledWith(docker.DOCKER_IMAGES_LIST);
expect(core.info).nthCalledWith<[string]>(
1,
"Listing Docker images.",
);
expect(util.execBashCommand).nthCalledWith<[string]>(
1,
'docker image list --format "{{ .Repository }}:{{ .Tag }}"',
);
}
}
}
};
Expand All @@ -109,6 +119,7 @@ describe("Docker images", (): void => {
key: string,
cacheHit: boolean,
readOnly: boolean,
prevSave: boolean,
preexistingImages: string[],
newImages: string[],
): Promise<void> => {
Expand All @@ -117,14 +128,19 @@ describe("Docker images", (): void => {
if (!cacheHit) {
core.getInput.mockReturnValueOnce(readOnly.toString());
if (!readOnly) {
core.getState.mockReturnValueOnce(preexistingImages.join("\n"));
const images = preexistingImages.concat(newImages);
util.execBashCommand.mockResolvedValueOnce(images.join("\n"));
if (prevSave) {
cache.restoreCache.mockResolvedValueOnce(key);
} else {
cache.restoreCache.mockResolvedValueOnce(undefined);
core.getState.mockReturnValueOnce(preexistingImages.join("\n"));
const images = preexistingImages.concat(newImages);
util.execBashCommand.mockResolvedValueOnce(images.join("\n"));
}
}
}
await docker.saveDockerImages();

assertSaveDockerImages(cacheHit, readOnly);
assertSaveDockerImages(cacheHit, key, readOnly, prevSave);
};

const assertCacheNotSaved = (): void => {
Expand All @@ -147,6 +163,15 @@ describe("Docker images", (): void => {
assertCacheNotSaved();
};

const assertSavePrevSave = (key: string): void => {
expect(core.info).lastCalledWith(
"A cache miss occurred during the initial attempt to load Docker " +
`images, but subsequently a cache with a matching key, ${key}, was saved. ` +
"This can occur when run in parallel. Not saving cache.",
);
assertCacheNotSaved();
};

const assertNoNewImagesToSave = (): void => {
expect(util.execBashCommand).toHaveBeenCalledTimes(1);
expect(core.info).lastCalledWith("No Docker images to save");
Expand Down Expand Up @@ -230,24 +255,28 @@ describe("Docker images", (): void => {
);

testProp(
"are saved unless cache hit, in read-only mode, or new Docker image list is empty",
"are saved unless cache hit, in read-only mode, cache already saved, or " +
"new Docker image list is empty",
[
fullUnicodeString(),
boolean(),
boolean(),
boolean(),
uniquePair(dockerImages(), dockerImages()),
],
async (
key: string,
cacheHit: boolean,
readOnly: boolean,
prevSave: boolean,
[preexistingImages, newImages]: [string[], string[]],
): Promise<void> => {
jest.clearAllMocks();
await mockedSaveDockerImages(
key,
cacheHit,
readOnly,
prevSave,
preexistingImages,
newImages,
);
Expand All @@ -256,6 +285,8 @@ describe("Docker images", (): void => {
assertSaveCacheHit(key);
} else if (readOnly) {
assertSaveReadOnly(key);
} else if (prevSave) {
assertSavePrevSave(key);
} else if (newImages.length === 0) {
assertNoNewImagesToSave();
} else {
Expand All @@ -264,10 +295,11 @@ describe("Docker images", (): void => {
},
{
examples: [
["my-key", false, false, [["preexisting-image"], ["new-image"]]],
["my-key", false, false, [["preexisting-image"], []]],
["my-key", false, true, [["preexisting-image"], ["new-image"]]],
["my-key", true, false, [["preexisting-image"], ["new-image"]]],
["my-key", false, false, false, [["preexisting-image"], ["new-image"]]],
["my-key", false, false, false, [["preexisting-image"], []]],
["my-key", false, true, false, [["preexisting-image"], ["new-image"]]],
["my-key", true, false, false, [["preexisting-image"], ["new-image"]]],
["my-key", false, false, true, [["preexisting-image"], ["new-image"]]],
],
},
);
Expand Down
11 changes: 11 additions & 0 deletions src/docker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,17 @@ const saveDockerImages = async (): Promise<void> => {
`Cache miss occurred on the primary key ${key}. Not saving cache as ` +
"read-only option was selected.",
);
/* Check if a cache with our key has been saved between when we checked in
* loadDockerImages and now.
*/
} else if (
key === (await restoreCache([""], key, [], { lookupOnly: true }))
) {
info(
"A cache miss occurred during the initial attempt to load Docker " +
`images, but subsequently a cache with a matching key, ${key}, was saved. ` +
"This can occur when run in parallel. Not saving cache.",
);
} else {
const preexistingImages = getState(DOCKER_IMAGES_LIST).split("\n");
info("Listing Docker images.");
Expand Down

0 comments on commit c50da5e

Please sign in to comment.