Phabricator Binary diff issue

Fixing Phabricator’s empty binary files bug

For about two and a half years, Phabricator had a bug where binary files in landed revisions would silently turn into empty files. The bug was filed in November 2023 as Bug 1865760, I figured out the root cause within a couple of weeks, and then it sat in “I know how to fix this, but actually shipping the fix is painful” purgatory until May 2026, when Claude Code helped me skip the painful part.

The bug

When a patch lands from Lando into one of our repositories - Mercurial or Git, the bug affects both - Phabricator notices the new commit and updates the revision with a freshly-generated diff. That regenerated diff has a creationMethod of commit, and for any binary files in the change, it is empty. The non-binary parts of the diff are fine - it’s specifically binary content (images, audio test fixtures, .zip, .pdf, .lz4 blobs) that gets blown away.

The reporter, :glandium, filed the bug with a concrete example on D188673:

• Diff 777519: what Lando operated on - has binary content.
• Diff 777524: what Phabricator recorded as the landing - binary content is gone.
• Diff 789018: a relanding attempt, derived from 777524 - also bogus, broke the build.

On its own this is a cosmetic annoyance: the patch landed correctly, the repository has the right bytes, and the revision page in Phabricator is just lying about what landed. The problem is what happens next - any time anything tries to re-use the patch after that commit diff has been written to the revision.

Two specific paths into this:

• Uplifts (historically). Uplifts via Lando used to re-use the most recent diff on a revision when creating the uplift on a release branch. After a landing, the most recent diff was the broken one, so the uplifted patch had empty binaries. This path is no longer affected: uplifts now go through moz-phab, which creates a fresh diff from the developer’s local copy instead of re-using the commit diff on the original revision.
• Re-landings after a backout. This is the painful one. A developer lands a properly-approved patch, the sheriff backs it out for an intermittent failure that has nothing to do with their work, and when they re-land - same patch, same revision, no changes - the patch picked up by Lando now has empty binary content. The original author has done nothing wrong, and the second landing’s symptoms (broken tests, missing assets, mysterious CI failures) look completely disconnected from the backout that caused them.

The only way out, until the underlying bug was fixed, was to update the revision with a fresh diff before re-using it. If you happened to still have your local clone open and ran moz-phab submit again with any change, you’d get a non-commit diff and everything would work fine. If you just clicked “re-land” or asked for an uplift, you’d hit the bug.

The result, over the lifetime of this bug, was a steady drip of “my unit test zip is empty in CI” and “the uplifted PDF is 0 bytes” incidents. Sheriffs had to manually patch things up several times. Comment 6 and comment 8 in the bug are good examples - a .zip of test fixtures wiped out on backout, a PDF reset to 0 bytes during uplift.

The full re-land path that produces the broken landing looks like this:

Step 4 is the root cause - Phabricator regenerating the diff and skipping the binary re-attach. Everything from step 5 down is just the world catching up with that broken diff sitting on the revision.

Debugging

The clue is in the differential.querydiffs API output. If you compare a good diff (creationMethod: moz-phab-hg) against a bad one (creationMethod: commit) for the same binary file, the metadata field is where they diverge.

A moz-phab-created changes object for a binary file:

{
  "id": "7062701",
  "metadata": {
    "old:binary-phid": null,
    "old:file:size": 0,
    "old:file:mime-type": "",
    "new:binary-phid": "PHID-FILE-kz6liay6wbz3isrm4bgy",
    "new:file:size": 1024,
    "new:file:mime-type": "audio/x-wav",
    "hash.effect": "rV3hKCon1ogp"
  },
  "currentPath": "dom/media/webaudio/test/waveformatextensible.wav",
  ...
}

The commit-created object for the same file:

{
  "id": "7064830",
  "metadata": [],
  "currentPath": "dom/media/webaudio/test/waveformatextensible.wav",
  ...
}

That metadata object contains a new:binary-phid pointing at the actual uploaded file blob in Phabricator. Without it, the file is effectively unlinked - Phabricator has no idea what bytes the binary refers to, so it renders as empty.

Tracing through the Phabricator source from the commit side leads to DifferentialDiffExtractionEngine.php. The flow is:

• Pull the raw diff from the commit via diffusion.rawdiffquery.
• Parse it into changes objects with ArcanistDiffParser->parseDiff.
• Build a new DifferentialDiff from those changes with creationMethod set to commit.
• Save the diff and attach it to the revision.

Right before the save, there’s a comment in the Phabricator source that has been sitting there for years:

// TODO: Attach binary files.

That’s the bug. The parser correctly identifies the binary change and sets the fileType to binary, but nothing ever uploads the actual bytes or attaches the resulting file PHID into metadata. The parsed diff is structurally aware that there’s a binary file, but it has no link back to the blob.

Stalled Progress

Once you know where the gap is, the question is what to put there. Phabricator’s own Arcanist client already has a function that does exactly this work: ArcanistDiffWorkflow::uploadFilesForChanges. Given an array of changes objects, it filters for binary ones, uploads each file to Phabricator, and writes the resulting PHIDs back into the metadata field. Perfect fit.

Except ArcanistDiffWorkflow is built to be invoked as the arc diff command, not used as a library from inside Phabricator. It has interactive prompts on the command line for various error paths. To use it from DifferentialDiffExtractionEngine I’d need to either:

1. Pipe a no_prompt-style argument all the way through so the prompts become non-fatal exceptions, then drive the workflow as if it were a CLI invocation from the server-side extraction engine. Touches a lot of surface area, and the workflow is not designed to be re-entered like that.
2. Reimplement the binary upload directly using ConduitCall and the file upload APIs, using moz-phab’s implementation as a reference. Less surface area but a lot of code to write from scratch, and easy to get the metadata format subtly wrong.

Both options were a multi-week commitment with real risk of dead-ending. Phab-dev was also broken for testing at the time, blocked by Bug 1868612, which made the prospect of writing that much speculative code without a fast feedback loop unappealing.

So in December 2023 I shipped a Lando-side workaround for uplifts specifically: when creating an uplift, query differential.querydiffs for the revision, find the most recent diff with a creationMethod that isn’t commit, and use the metadata from that older diff to relink the binary PHIDs into the new uplift diff. This bypassed the broken commit diff entirely for the uplift path.

That covered the worst symptom but left the underlying bug in place. Backouts-and-relands on autoland kept hitting it. The fix sat as scratch notes in this very Obsidian vault, and every six months the bug would get a new comment from someone bitten by it.

Claude Code unblocking the fix

In May 2026 I pointed Claude Code at the notes from my original December 2023 investigation - the same content that lives in the section above - and asked it to take a swing at the fix.

The thing that had blocked me wasn’t really understanding; it was the cost of writing a careful integration with ArcanistDiffWorkflow against a flaky dev environment. With Claude Code I could skip the agonizing prior-art integration approach entirely and instead build the smaller, more direct version: in the extraction engine, after parsing the raw diff, iterate the changes objects, find the binary ones, upload their bytes via the existing Phabricator file APIs, and write the resulting PHIDs into the metadata field. This is the option-2 reimplementation that I’d previously written off as too much code to ship without confidence. Claude Code can write that code in an afternoon, and I could focus on verifying it rather than typing it.

The PR is mozilla-conduit/phabricator#87 (“Bug 1865760: upload binary files to commit diffs”). I deployed it to Phab-dev and ran an end-to-end reproduction on D4105:

• The initial diff contains the binary content.
• The commit created by the initial landing contains the binary content.
• The diff Phabricator generates from that landing contains the binary content.
• The commit is backed out and the revision is re-opened.
• The commit is re-landed without any changes, and the binary content is non-empty in the subsequent landing.
• The diff Phabricator generates from the re-landing also contains the expected binary content.

That last step is the one that has been broken since the bug was filed. With the fix in place, the re-landed commit has the right bytes and the regenerated diff has the right metadata.

Takeaways

A few things I want to remember from this one:

• “I know how to fix it” is not the same as “I can ship the fix.” The Phabricator code path was identified within two weeks. What stalled was the engineering cost of the cleanest integration, against an unreliable dev environment, with no urgent forcing function. Writing the smaller, less-clean version was always technically available; it just wasn’t worth the typing effort on its own.
• Notes are infrastructure. The investigation notes I took in 2023 - originally just for myself - were what made the 2026 fix cheap. Without them I’d have re-done the entire differential.querydiffs archaeology before writing a line of code. Future-me thanks past-me, and Claude Code thanks both of us.

See also: Uplift Automation, Lando, Phabricator.