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 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 is problematic if
the action is being run in parallel for multiple jobs; in that case,
there could be multiple, unnecessary cache saves if the cache initially
misses but is subsequently saved.
  • Loading branch information
mwarres committed Mar 6, 2024
1 parent 055e746 commit 18fc64c
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 12 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.

47 changes: 37 additions & 10 deletions src/docker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,15 @@ describe("Docker images", (): void => {
const assertSaveDockerImages = (
cacheHit: boolean,
readOnly = false,
prevSave = false,
): void => {
expect(core.getInput).nthCalledWith<[string, InputOptions]>(1, "key", {
required: true,
});
expect(core.getState).nthCalledWith<[string]>(1, docker.CACHE_HIT);
if (!cacheHit) {
expect(core.getInput).lastCalledWith("read-only");
if (!readOnly) {
if (!readOnly && !prevSave) {
expect(core.getState).lastCalledWith(docker.DOCKER_IMAGES_LIST);
expect(core.info).nthCalledWith<[string]>(1, "Listing Docker images.");
expect(util.execBashCommand).nthCalledWith<[string]>(
Expand All @@ -109,6 +110,7 @@ describe("Docker images", (): void => {
key: string,
cacheHit: boolean,
readOnly: boolean,
prevSave: boolean,
preexistingImages: string[],
newImages: string[],
): Promise<void> => {
Expand All @@ -117,14 +119,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(key + "make keys mismatch");
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, readOnly, prevSave);
};

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

const assertSavePrevSave = (): void => {
expect(core.info).lastCalledWith(
"A cache miss occurred in loadDockerImages; subsequently a cache with " +
"a matching key was saved. This can occur when docker-cache is used by " +
"multiple jobs 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 +246,29 @@ 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, a previous save occurs on " +
"the same cache key between loadDockerImages and saveDockerImages 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 +277,8 @@ describe("Docker images", (): void => {
assertSaveCacheHit(key);
} else if (readOnly) {
assertSaveReadOnly(key);
} else if (prevSave) {
assertSavePrevSave();
} else if (newImages.length === 0) {
assertNoNewImagesToSave();
} else {
Expand All @@ -264,10 +287,14 @@ 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"]]],
["my-key", false, false, true, [["preexisting-image"], []]],
["my-key", false, true, true, [["preexisting-image"], ["new-image"]]],
["my-key", true, 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 in loadDockerImages; subsequently a cache with " +
"a matching key was saved. This can occur when docker-cache is used by " +
"multiple jobs 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 18fc64c

Please sign in to comment.