This should query by uriHash = %s ... PhabricatorHash::digestForIndex($img_uri), since that column is the one with a key on it. Once the table gets big, query using a keyed column will execute much more quickly than this query.

(You can also omit LIMIT 1 since there's a unique key, and if we get more than one result back somehow it's probably good for loadOneWhere() to throw an exception instead of picking the first result.)

You can executeOne() above (instead of execute()) to avoid this (although maybe you're trying to dodge policy exceptions)?

For now, you could just throw an exception here (similar to what we'll do later on).

Actually, can you restructure things a bit so that the code is shared in both cases? Maybe something like:

$response = $this->getExternalResponse($cached_external_request);
if ($response) {
  return $response;
}

// <...do all the cache stuff...>

return $this->getExternalResponse($freshly_generated_external_request);

Maybe it can't look quite like that, but I think the logic for "turn an ExternalRequest object into a response" is the same in both cases, and it should be possible to consolidate them.

We should perform these validations before checking the cache so that if there's, say, a bug in requireValidRemoteURIForLink() then we can just push a fix and it will apply retroactively. By doing these checks later, existing security issues/buggy data already in the cache might remain active after a bugfix.

This seems fine to me.

I think it's probably possible for this to race:

• Clear the cache.
• Load a page with two (or more) references to the same image.

I suspect both pages will fairly regularly get through the initial check (cache is empty) and run the request, then whichever one is a little slower will fail here with an AphrontDuplicateKeyQueryException when it tries to save a second copy of the same record.

Some ways we could deal with this:

1. Do nothing for now until we see that it's actually an issue.
2. Catch AphrontDuplicateKeyQueryException, load the colliding record, pretend that one is the one we saved.
3. Hold a lock based on the URI while making the request.

I think (1) is fine for now since multiple copies of an image on a page will be rare.

When this kind of request-based race happens we usually do something in the vein of (2), but I think there are at least some arguments for doing (3) instead here: if we don't lock, the duplicate requests will write duplicate files; and they'll incur duplicate points on the user's rate limit; and they'll make duplicate requests to the remote server. These costs are small but higher than the costs we usually face when choosing to catch the exception and load the colliding record.

The lock would look something like this:

try_the_cache();
$lock = PhabricatorGlobalLock::newLock($lock_name_based_on_URI_hash);
try_the_cache_again();
make_the_request_and_write_the_data_and_cache();
$lock->unlock();

Handling this is probably worth splitting out into a followup change, since I'd guess it will be somewhat hard to hit this in the first place.

To slightly reduce the amount of code here, you could create this object earlier, and then share it in both the success and failure branches.

Maybe call this "PhabricatorFileExternalRequestGarbageCollector"? Using "Temp" in the name and "tempttl" as the key confuses it with "Temporary Files", which are a separate type of file with a TTL on the file itself.

We should also put key_tll here on the ttl column, so the GC query can take advantage of it.

I anticipate probably wanting to query this table when viewing the file detail page ("Was this an external request?") so it would also be reasonable to add a key_file on filePHID, although it's possible we won't actually use that so we could also wait until we're sure we really want it.