Page MenuHomePhabricator

CleanupUploadStash returns status code 1 when temp folder not present
Closed, ResolvedPublic

Description

The CleanupUploadStash script checks if there are any orphaned temp files in ''/local-temp'' directory. If the directory doesn't exist the script returns exit code 1(default from the fatalError method).
There are a few cases where this behaviour might be unexpected:

  1. Used backend implementation doesn't use this folder and/or any local folder
  2. Running Mediawiki in the cloud without persistent storage (cleaning up temp isn't needed)

If the temp folder doesn't exist, there is nothing to cleanup locally. This means the script finished successfully

Event Timeline

Change 693866 had a related patch set uploaded (by Abador; author: Abador):

[mediawiki/core@master] CleanupUploadStash: return exit code 0 when local temp repo is not present

https://gerrit.wikimedia.org/r/693866

The code in question is something like this:

$repo = MediaWikiServices::getInstance()->getRepoGroup()->getLocalRepo();
$tempRepo = $repo->getTempRepo();
$dir = $tempRepo->getZonePath( 'public' );
$iterator = $tempRepo->getBackend()->getFileList( [ 'dir' => $dir ] );
if ( $iterator === null ) {
    $this->fatalError( "Could not get file listing." );
}
if ( strpos( $dir, '/local-temp' ) === false ) {
    $this->fatalError( "Temp repo is not using the temp container.", 0 );
}
// go through $iterator, delete files

AIUI this doesn't have anything with local directory existence; getZonePath merely reads from configuration. This seems to be a check against a misconfigured temp repo, although I'm not sure why a different container name would be a misconfiguration, and the $wgLocalFileRepo/$wgForeignFileRepos documentation is no help there (doesn't even mention container/zone related configuration options).

As far as I can see there are two possibilities here where the fatal error is triggered:

  • The local temp zone is not set up, ie. $tempRepo->getZonePath( 'public' ) returns null. I don't think this happens (the FileRepo constructor sets it up if not manually configured), but if it does somehow, skipping the cleanup without fataling does seem like the appropriate thing to do.
  • The container name is customized (someone has set something like $wgLocalFileRepo['zones']['temp']['container'] = 'custom';). While that seems like a pointless customization to do, it isn't disallowed and the temp files still need to be deleted, so this doesn't seem like an error condition and there is no reason to abort execution.

That said, I don't know this part of the multimedia code well. @aaron might have a better answer.

Change 706294 had a related patch set uploaded (by Abador; author: Abador):

[mediawiki/core@master] CleanupUploadStash: change fatalError to output and continue temp file cleanup if temp directory isn't located in local-temp

https://gerrit.wikimedia.org/r/706294

Change 693866 merged by jenkins-bot:

[mediawiki/core@master] CleanupUploadStash: return exit code 0 when local temp repo is not present

https://gerrit.wikimedia.org/r/693866

Change 706294 merged by jenkins-bot:

[mediawiki/core@master] CleanupUploadStash: change fatalError to output and continue temp file cleanup if temp directory isn't located in local-temp

https://gerrit.wikimedia.org/r/706294

Tgr assigned this task to Abador86.