◀ Merged tasks: T5332.

Thanks, epriestley. Is this still being looked into as a potential bug? Disabling externally hosted subversion repos isnt really a viable solution for most people, I wouldnt think.

This is definitely a bug, but one we haven't been able to reproduce yet.

Could any of the settings at the page below pertain to this?

config/group/garbagecollector/

Mine are currently:
gcdaemon.ttl.herald-transcripts
Number of seconds to retain Herald transcripts for.
Current Value: 2592000
gcdaemon.ttl.daemon-logs
Number of seconds to retain Daemon logs for.
Current Value: 604800
gcdaemon.ttl.differential-parse-cache
Number of seconds to retain Differential parse caches for.
Current Value: 1209600
gcdaemon.ttl.markup-cache
Number of seconds to retain Markup cache entries for.
Current Value: 2592000
gcdaemon.ttl.task-archive
Number of seconds to retain archived background tasks for.
Current Value: 1209600
gcdaemon.ttl.general-cache
Number of seconds to retain general cache entries for.
Current Value: 2592000
gcdaemon.ttl.conduit-logs
Number of seconds to retain Conduit call logs for.
Current Value: 15552000

No, those are all unrelated.

The expectation is that bin/ssh-connect will write these files, use them to execute an ssh command, then clean them up when it exits. This should be the only way these files are written, and they should automatically be destroyed when the process exits. On my machine, this appears to happen. From @wrotte's tests, the cleanup mechanism seems to work properly (P1040) but even explicit destruction of the keyfile did not (P1041). I don't have a plausible explanation for this.

You can reproduce the circumstances which are expected to create the key like this:

$ PHABRICATOR_CREDENTIAL=PHID-CDTL-xxxx SVN_SSH=/path/to/phabricator/bin/ssh-connect svn --non-interactive info ssh://remote.com/path/to/repo

Where:

• PHID-CDTL-xxxx is the PHID of the Passphrase credential with the key. This isn't easy to obtain, but you can query the phabricator_passphrase database.
• /path/to/phabricator/bin/ssh-connect is the path to bin/ssh-connect in Phabricator.
• ssh://remote.com/path/to/repo is the SVN remote URI.

Under the hood, this will run svn, which will invoke ssh-connect in order to run the underlying ssh process. ssh-connect will read the credential identifier from the environment, load the credential, write it to disk, and then invoke an ssh -l <user> -i <key> <uri> command to establish the connection.

When ssh-connect exits, it is supposed to clean up the temporary file.

I might investigate a bit more in a bit, I've just been slammed lately and haven't had much available time to put towards the care and feeding of my phab install.

No worries! I'll look into this some more too when I have a chance.

It could be related:

Steps to reproduce:

1. Add a credential for a remote repository. (SSH Key)
2. Destroy the credential.
3. Voila!

The destroyed credential keeps attached to the remote uri. An uncaught exception raises so the temp file is never deleted:

Call to a member function openEnvelope() on null in .../phabricator/src/applications/passphrase/keys/PassphraseSSHKey.php on line 29

getSecret() returns null:

if (!$this->keyFile) {
  $temporary_file = new TempFile('passphrase-ssh-key');

  Filesystem::changePermissions($temporary_file, 0600);

  Filesystem::writeFile(
    $temporary_file,
    $credential->getSecret()->openEnvelope());

  $this->keyFile = $temporary_file;
}

Hope it helps!

That's extremely helpful, thanks! I'll get that fixed up.

Although I would expect the temporary file to be destroyed even if the exception is raised. This is still a bug in any case, since we shouldn't write the file in the first place.

Oh, except that when you call a method on null we don't actually get an exception because PHP is sort of derp and everything just dies abruptly. This script reproduces the issue, at least potentially:

<?php

require_once 'scripts/__init_script__.php';

function write_file($fatal) {
  $f = new TempFile();
  echo "Writing file: {$f}\n";

  if ($fatal) {
    echo "Fataling!\n";
    Filesystem::writeFile($f, fatal_now());
  } else {
    echo "Throwing!\n";
    Filesystem::writeFile($f, throw_exception());
  }
}

function throw_exception() {
  throw new Exception("X");
}

function fatal_now() {
  $o = null;
  $o->m();
}

write_file(!empty($argv[1]));

When run as:

$ php -f test.php

...we throw, which destroys the temporary file.

However, when run as:

$ php -f test.php 1

...we fatal by calling a method on null, which does not.

Cool and fast! Thanks!

@wotte / @wassere, if you still see this locally, can you check if that fixed your issue?

I'm going to tentatively assume that it's either directly connected to that bug or caused by a similar issue and redirect this to finding ways to clean up temporary files more aggressively. It's bad that we don't destroy them when we hit multiple fatals.

I'm still working through my backlog after a long vacation to Iceland, but I'll nuke the cron job I had cleaning out the test directory and see if we still see the behavior.

Cool. A possible first-degree approximation is checking if the files contain private key material -- I'm not sure if we ever did that. If they're empty, that's a smoking gun for this being the issue.

I think D10329 will resolve this regardless, though, hopefully.

Thats awesome, thanks guys! I had a cron job deleting these file hourly as well. Please let us know if the issue is indeed fixed with Evan's commit.

I'm still getting this problem with the latest pull :(
A few hundred thousand copies of my key every week :(

Not sure exactly where to start looking into it, or things to try to fix it?

Edit: I have 6 externally hosted repos running in my phabricator-install and updates appear correctly inside phabricator, indicating that the operation itself seems to work fine, even though the temporary key file is left in /tmp/ afterwards. The repos are accessed over svn+ssh with a shared, stored key.

I'm seeing this on the current version in my environment also. As oyvindselbek mentioned, everything seems to be working (updates are happening) except for the cleanup of the temp files.

I started troubleshooting this a bit, but I'm hung up at the proc_open call in PhutilExecPassThru. It seems like neither the error handler nor output ever get restored after a successful call to proc_open, so any items logged or exceptions thrown never show up after that point. I'm not sure if this is expected behavior or a bug, but it sure makes troubleshooting difficult.

I expect that there's an error occurring with the deletion, but we never see it because of the redirection.

Anyway, I can reproduce this at will in our environment, so if I can provide any additional info, run test cases, etc., please let me know.