fix: clear opcache after app extraction

[kesselb]

• Resolves: [Bug]: Internal server error after update to 31.0.5 #52977

Summary

Note

Since I usually do not update apps from the app store on my development system, I have not tested this yet. If you believe this change makes sense, I will try to find some time to test it. Triggering such an error requires a bit of luck.

clear opcache post app update extraction to prevent outdated files issues.

opcache.validate_timestamps=0 disables automated file modification checks.

TODO

• Try testing it 🙈

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[MichaIng]

My system has opcache.validate_timestamps=0.

Before this patch:

1. I see cached scripts of the calendar app with above 1k hits:
   
2. I update the calendar app via web interface
3. I see the cache entry has not been reset:
   
   Probably a bad example, since this script did not change, but generally it is hence possible that outdated script versions run after an app update. Even with opcache.validate_timestamps=1, but opcache.revalidate_freq=5 or whatever, outdated calendar app scripts were loaded at least once, possibly causing unnecessary error log entries and such, or worse.

After this patch (and manually invalidating lib/private/Installer.php 😄):

1. I see cached scripts of the notes app with above 1k hits:
   
2. I update the notes app via web interface
3. I see these and all scripts freshly re-added to the cache:
   

I should have taken a files which did actually change between these versions, but it shows the cache reset works as intended, and indeed the previous behavior caused unnecessary problems with opcache.validate_timestamps=0, and at best unnecessary short-term issues and error logs even with short revalidation intervals. I am happy we found this, thanks for the fix 👍.

EDIT: That scripts always show has been invalidated with opcache.validate_timestamps=0 is a bug in the GUI: amnuts/opcache-gui#92

[nickvergessen]

Since I usually do not update apps from the app store on my development system, I have not tested this yet. If you believe this change makes sense

I think it does not solve the problem, as the app and therefore some event listeners, services, etc might be loaded already with old data. That does not refresh with opcache clear…?

[MichaIng]

But any page load/navigation would re-initiate those, isn't it? I mean sure any detached background jobs like a running image preview generation or such could theoretically finish, but how many apps implement such which takes significant time to survive throughout an app update? If so, a real solution would be probably a hook or such for apps to terminate anything they run, before doing the update. If I remember right, apps are already disabled temporarily on update, isn't it?

In any case, such is not related to OPcache, so OPcache related issues when updating apps via web UI are solved with this, which is an enhancement.

[nickvergessen]

The big issue is that the update/upgrade process itself is still broken and migrations still can not reference new code…

[MichaIng]

Well, "boken" sounds a bit harsh, I mean I rarely face any issues with app updates, despite opcache.validate_timestamps=0. Biggest problem was/is always Nextcloud Office updates with built-in CODE server, that often requires/required a PHP restart and sometimes manually terminating CODE server instances 😅. Quite possible that this PR fixes that part already, or reduces the chance that manual intervention is needed. EDIT: Ah wait, that server with Nextcloud Office has invalidation active with 5 seconds interval.

[nickvergessen]

Well, "broken" sounds a bit harsh

• Well it does break, if you run app updates with cron or whatever your instance is down from that moment onwards. Happened to our instance 2 weeks ago: Recent update may break due to missing method collectives#1763

[MichaIng]

Hmm, maybe CLI should write a flag into database while running, so updates/upgrades can check and wait for it to disappear, before doing any file changes. Similarly, updates/upgrades could deploy a flag which prevents CLI from running. Maintenance mode in config.php does not work reliably due to OPcache, but a database flag should do?

However, I am voting for not letting this discussion delay this PR to be merged. It is enhancement in its own, even if it does not solve all possible issues.

[skjnldsv]

I think it makes sense.
While more can be done on the matter, it can always be done as follow-up 👌

[solracsf]

I would place it one line above to call it even early, but fine be me 😆

[MichaIng]

/backport to stable31

[MichaIng]

/backport to stable30

[backportbot]

The backport to stable31 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable31
git pull origin stable31

# Create the new backport branch
git checkout -b backport/53210/stable31

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick c7b69931

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/53210/stable31

Error: Failed to check for changes with origin/stable31: No changes found in backport branch

---

Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

[backportbot]

The backport to stable30 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable30
git pull origin stable30

# Create the new backport branch
git checkout -b backport/53210/stable30

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick c7b69931

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/53210/stable30

Error: Failed to check for changes with origin/stable30: No changes found in backport branch

---

Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

[solracsf]

/backport to stable31

[backportbot]

The backport to stable31 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable31
git pull origin stable31

# Create the new backport branch
git checkout -b backport/53210/stable31

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick c7b69931

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/53210/stable31

Error: Failed to check for changes with origin/stable31: No changes found in backport branch

---

Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

[MichaIng]

Hmm, somehow this backportbot diff test gives wrong result 🤔: nextcloud/backportbot@b3b62e4#diff-b7dac73310d09b8ffa7d22f401022ef1a9ade956e7bdf1a196aca871cc669785R45

[MichaIng]

/backport to stable31

[backportbot]

The backport to stable31 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable31
git pull origin stable31

# Create the new backport branch
git checkout -b backport/53210/stable31

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick c7b69931

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/53210/stable31

Error: Failed to check for changes with origin/stable31: No changes found in backport branch

---

Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

[MichaIng]

Manual backports:

• [stable31] fix: clear opcache after app extraction #53591
• [stable30] fix: clear opcache after app extraction #53593