When updating revisions in response to commits, reuse previously generated diffs
2dfe79cfc713
Actions

Description

When updating revisions in response to commits, reuse previously generated diffs

Summary:
Fixes T10968. In rare situations, we can generate a diff, then hit an error which causes this update to fail.

When it does, we tend to get stuck in a loop creating diffs, which can fill the database up with garbage. We saw this once in the Phacility cluster, and one instance hit it, too.

Instead: when we create a diff, keep track of which commit we generated it from. The next time through, reuse it if we already built it.

Test Plan:

Used bin/differential attach-commit <commit> <revision> to hit this code.
Simulated a filesystem write failure, saw the diff get reused.
Also did a normal update, which worked properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10968

Differential Revision: https://secure.phabricator.com/D17164

Details

Provenance

epriestley	Authored on Jan 9 2017, 7:39 PM
epriestley	Pushed on Jan 9 2017, 8:13 PM

Reviewer

chad

Differential Revision

D17164: When updating revisions in response to commits, reuse previously generated diffs

Parents

rP27ecedd1d5d8: Use some more human-readable Conduit keys in updated API methods

Branches

Unknown

Tags

Unknown

Tasks

T10968: File write failure during daemon operations caused daemons to populate a few too many diffs

Build Status

Buildable 15165
Build 19922: Run Core Tests

Event Timeline

epriestley committed rP2dfe79cfc713: When updating revisions in response to commits, reuse previously generated diffs (authored by epriestley).Jan 9 2017, 8:13 PM

epriestley added a task: T10968: File write failure during daemon operations caused daemons to populate a few too many diffs.

Harbormaster completed building B15165: rP2dfe79cfc713: When updating revisions in response to commits, reuse previously generated diffs.Jan 9 2017, 8:15 PM

jcox added a subscriber: jcox.Jan 20 2017, 8:06 PM

jcox added inline comments.

/src/applications/differential/query/DifferentialDiffQuery.php
123	lol. this specific change caused on of our unit tests to start failing, throwing EXCEPTION (AphrontParameterQueryException): Array for %Ld conversion is empty. Query: id IN (%Ld) It turns out that the unit test was not doing what it thought it was doing. It also exposed the fact that one of our extensions was slightly broken in a somewhat dangerous way. So... thanks for fixing one of our bugs :P

The !== null also catches this bug:

$thing = id(new ThingQuery())
  ->setViewer($viewer)
  ->withIDs($list_of_ids)
  ->execute();

...which means "load several million diffs" if $list_of_ids might be empty. It would be nice to have a less magical incantation to catch this stuff, but I think we've pretty much solved it in practice by sticking !== null everywhere.