[Bug?]: --mode=update-lockfile does not remove old yarn cache entry

[jurre]

Self-service

• I'd be willing to implement a fix

Describe the bug

Running yarn add <package@version> --mode=update-lockfile for a new version of an existing package already in the lockfile and yarn cache adds the new cached version of the new package to the yarn cache as expected, but it does not remove the old entry.

I am not sure if this is actually working as intended, but for Dependabot, we would like to be able to update the yarn cache for users, and have it behave similar to how they would manually perform the update. I think users would expect the old version to be removed. I would absolutely understand if this is a limit of the current implementation, as I don't know much about the implementation details, but I figured I'd ask.

We can work around this by running in the default mode, and so far performance seems acceptable so it's not a big problem, but given that the mode seems to have been specifically designed for our usecase (thank you for that!), I wanted to start a conversation with y'all.

To reproduce

yarn-lockfile-only % yarn add [email protected]
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed in 0s 263ms
➤ YN0000: ┌ Fetch step
➤ YN0013: │ lodash@npm:4.0.0 can't be found in the cache and will be fetched from the remote registry
➤ YN0000: └ Completed in 1s 617ms
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed in 0s 426ms
➤ YN0000: Done in 2s 314ms
yarn-lockfile-only % ls -la .yarn/cache
total 768
drwxr-xr-x  4 jurre  staff     128 Sep 22 13:56 .
drwxr-xr-x  4 jurre  staff     128 Sep 22 13:56 ..
-rw-r--r--  1 jurre  staff      26 Sep 22 13:56 .gitignore
-rw-r--r--  1 jurre  staff  386100 Sep 22 13:56 lodash-npm-4.0.0-c23a1add6a-918d7eb804.zip
yarn-lockfile-only % yarn add [email protected] --mode=update-lockfile
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed in 0s 225ms
➤ YN0000: ┌ Fetch step
➤ YN0013: │ lodash@npm:4.17.21 can't be found in the cache and will be fetched from the remote registry
➤ YN0000: └ Completed
➤ YN0000: ┌ Link step
➤ YN0073: │ Skipped due to mode=update-lockfile
➤ YN0000: └ Completed
➤ YN0000: Done with warnings in 0s 258ms
yarn-lockfile-only % ls -la .yarn/cache
total 1976
drwxr-xr-x  5 jurre  staff     160 Sep 22 13:57 .
drwxr-xr-x  4 jurre  staff     128 Sep 22 13:56 ..
-rw-r--r--  1 jurre  staff      26 Sep 22 13:56 .gitignore
-rw-r--r--  1 jurre  staff  386100 Sep 22 13:56 lodash-npm-4.0.0-c23a1add6a-918d7eb804.zip
-rw-r--r--  1 jurre  staff  617983 Sep 22 13:57 lodash-npm-4.17.21-6382451519-eb835a2e51.zip
yarn-lockfile-only % yarn add [email protected]
➤ YN0000: ┌ Resolution step
➤ YN0000: └ Completed
➤ YN0000: ┌ Fetch step
➤ YN0019: │ lodash-npm-4.0.0-c23a1add6a-918d7eb804.zip appears to be unused - removing
➤ YN0000: └ Completed
➤ YN0000: ┌ Link step
➤ YN0000: └ Completed in 0s 808ms
➤ YN0000: Done in 0s 855ms
yarn-lockfile-only % ls -la .yarn/cache
total 1216
drwxr-xr-x  4 jurre  staff     128 Sep 22 14:47 .
drwxr-xr-x  4 jurre  staff     128 Sep 22 13:56 ..
-rw-r--r--  1 jurre  staff      26 Sep 22 13:56 .gitignore
-rw-r--r--  1 jurre  staff  617983 Sep 22 13:57 lodash-npm-4.17.21-6382451519-eb835a2e51.zip

I tried to set up a sherlock thing, but I couldn't get it to work, apologies, here's what I was trying to do:

const {promises: {readdir}} = require(`fs`);

await packageJsonAndInstall({
  dependencies: {lodash: `4.0.0`},
});

const cacheFolder = await yarn(`config`, `get`, `cacheFolder`);

await yarn(`add`, `[email protected]`, `--mode`, `update-lockfile`);

const files = readdir(cacheFolder, (_, files) => {
    expect(files).toContain(`lodash-npm-4.17.21-6382451519-eb835a2e51.zip`);
    expect(files).not.toContain(`lodash-npm-4.0.0-c23a1add6a-918d7eb804.zip`);
});

Environment

System:
    OS: macOS 12.5.1
    CPU: (16) x64 Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
  Binaries:
    Node: 12.22.6 - /private/var/folders/zt/b61qxk5s73gctrlmls3hjmz80000gn/T/xfs-cf4427f1/node
    Yarn: 3.2.1 - /private/var/folders/zt/b61qxk5s73gctrlmls3hjmz80000gn/T/xfs-cf4427f1/yarn
    npm: 8.5.1 - ~/.nvm/versions/node/v12.22.6/bin/npm

Additional context

No response

[jurre]

Gentle ping on this, my main question is if deleting the old cache entry is desirable, and maybe some guidance on where to start looking and we'd be happy to try and get a PR up.

[arcanis]

Interesting - it seems we explicitly guard against cleaning obsolete cache entries, but I don't remember why ... that would seem like a bug to me, but then why did we add this check? 🤔

Do you remember details, @ylemkimon?

[jeffwidman]

@arcanis did you ever hear from @ylemkimon ?

[ylemkimon]

Sorry, I missed the previous ping.

With mode=update-lockfile, fetchEverything fetches only missing packages:

berry/packages/yarnpkg-core/sources/Project.ts

Lines 968 to 969 in 94e4069

	if (mode === InstallMode.UpdateLockfile)
	locatorHashes = locatorHashes.filter(locatorHash => !this.storedChecksums.has(locatorHash));

and therefore cacheCleanup cannot determine whether packages are needed or not.

Gentle ping on this, my main question is if deleting the old cache entry is desirable, and maybe some guidance on where to start looking and we'd be happy to try and get a PR up.

In that case, I think only the link step can be skipped, e.g., a new mode skip-link:

    await opts.report.startTimerPromise(`Link step`, async () => {
-     if (opts.mode === InstallMode.UpdateLockfile) {
+     if (opts.mode === InstallMode.UpdateLockfile || opts.mode === InstallMode.SkipLink) {
        opts.report.reportWarning(MessageName.UPDATE_LOCKFILE_ONLY_SKIP_LINK, `Skipped due to ${formatUtils.pretty(this.configuration, `mode=update-lockfile`, formatUtils.Type.CODE)}`);
        return;
      }