Update error handling of ArcanistLiberateWorkflow to detect failure from running rebuild-map.php in a separate process.

Thanks!

Remove log() entirely, let filesystem errors escape outwardly.

I think it's reasonable to remove the try/catch and just let the exception escape. It (the try-catch + log) isn't consistent with the approach to error handling in the rest of the codebase, and I can't think of any valid reason to continue here after a write failure.

Good catch!

Change approach - remove --verbose, remove --quiet, remove most instances of log() being used, keep the instance when an error is encountered.

Thanks!

I didn't make a task for this as it's a fairly small change and the reproduction/test case is also fairly simple.

One tangential thing I noticed from the error messages in these cases, in DiffusionHistoryQueryConduitAPIMethod the getMercurialResult() function executes this hg command

$path_args = array();
if (strlen($path)) {
  $path_args[] = $path;
  $revset_arg = hgsprintf(
    'reverse(ancestors(%s))',
    $commit_hash);
} else {
  $revset_arg = hgsprintf(
    'reverse(ancestors(%s)) and branch(%s)',
    $drequest->getBranch(),
    $commit_hash);
}

With just the updates to the future handling this is now the landing page I see with missing commits:

I tested this out and it works properly, in that there's no noticeable difference between the behavior with this change and without but including the new future updates

Sigh maybe update arcanist too. I ran arc lint before arc diff and it said no lints...

Try again but with arcanist on master

Rebased onto changes (no merge needed tho)

I'm working on testing out the issue before/after this change

I'll do that

This looks great to me now, as long as I didn't miss anything with the Future stuff and it does what it's supposed to on top of D21720 + D21721.

I'd favor this change:

I hadn't considered that having this still use http requests could be done in parallel vs. the clustered running them in serial being faster (theoretically)

Yeah -- isReady() should really be called something else nowadays (it's, uh, "doTheThingThisFutureDoes()" pretty much), but it would be a backward-compatibility break and I shuffled enough stuff around in T11968 that I didn't want to press my luck.

Does that mean the ConduitClientFuture doesn't begin execution until resolve() is called or some other internal mechanism triggered by iterating a FutureIterator?

Right: creating a future doesn't start it (it doesn't make any network connections, launch any subprocesses, etc). Technically, you can start a future explicitly by calling $future->start(), but there's little/no reason to ever do so.

Even if we started futures on creation, the process is ultimately still single-threaded and we don't get any kind of I/O handling "for free" in PHP, so execution must pass into most Futures regularly to make meaningful progress on them: for example, if you just start() an HTTP future and don't return to it, it will probably get about as far as opening a network connection but not actually send any bytes over the wire, so it isn't really executing fully in parallel. Subprocess futures usually don't need as much upkeep as network futures, but they can still stall if the stderr buffer on the child process fills up and the parent process hasn't returned execution into the Future to read the buffer.

This might or might not be helpful, but the rough model here is that "hardpoints" are an open slot that some particular data may be loaded into. The physical analogy is a "hardpoint" on fighter aircraft where a particular missile or bomb may be attached.

oops

I looked at Future to see how isReady() plays into other parts and this looks good! Thank you!

Restore parallel future resolution

Does that mean the ConduitClientFuture doesn't begin execution until resolve() is called or some other internal mechanism triggered by iterating a FutureIterator?

The desired/intended approach is to implement ArcanistMercurialCommitSymbolCommitHardpointQuery, following ArcanistGitCommitSymbolCommitHardpointQuery. Alas, this is a significantly more complicated change (but might make life easier down the road).

I did have a suspicion about a missing hardpoint query for Mercurial but the only one I was tracing through was HARDPOINT_REVISIONREFS which I think maps to (or, I mapped to) ArcanistMercurialWorkingCopyRevisionHardpointQuery which looks to be appropriately implemented and mostly matches the Git variant. Though I don't have a solid mental model of "hardpoints".

Ah, that's helpful. I found a repro, it's specific to the behavior of the ImmediateFuture pathway for non-clustered resolution of Conduit "calls". This should force it from any configuration/state, I think:

When implemented correctly, a Future should not throw until it is resolved (at least, for any kind of "result of trying to resolve the future" error), so the Iterator loop should never throw. Some older Futures were less careful about this, but I believe major futures were pulled in line after T11968. From cursory inspection, I think the behavior of ConduitClientFuture is correct, and I can't immediately get the iterator loop to throw an exception (see T13666#256232 for my test case and observed behavior).

Does that mean the ConduitClientFuture doesn't begin execution until resolve() is called or some other internal mechanism triggered by iterating a FutureIterator? I will definitely add this back in.

I'm able to reproduce this but I'll investigate further

The desired/intended approach is to implement ArcanistMercurialCommitSymbolCommitHardpointQuery, following ArcanistGitCommitSymbolCommitHardpointQuery. Alas, this is a significantly more complicated change (but might make life easier down the road).

I think I did conflate this partially - namely that this isn't directly caused by the issue discussed in T13365. Where this issue came up for me is while testing out D21715, I removed the head commit from the on-disk repo which caused the landing page to not render properly:

Thanks!

This construction:

I'll have some more detail in D21717 in a minute, but I can't immediately reproduce this, I think? If I intentionally break diffusion.historyquery like this:

In D21715#276590, @cspeckmim wrote:

I attempted to load the repository page and saw an exception due to a missing commit.

I don't think this is expected behavior but I'll make a new task for it since it's outside the scope of this change.

Created T13666

I attempted to load the repository page and saw an exception due to a missing commit.

I don't think this is expected behavior but I'll make a new task for it since it's outside the scope of this change.

In Mercurial, I'm not exactly sure how wide this window is. Since you normally can't strip commits in the remote by pushing (AFAIK?)

This is true, though it does change a bit if we enable evolve server-side. Then when commits/branches are pruned when pushing/pulling those obsolete markers will also be transferred, allowing similar disappearing commits (retrievable via —hidden). However I believe by default mercurial does not allow pruning public changesets (would require forcibly changing phases on client/server, or running a server/branch in non-publishing mode). Supporting evolve/topics server-side is probably large enough that it would be its own big project. So I think this assumption holds up well.

(D21715 is still a draft, but I left a couple of more specific comments there.)

Does this refer to when multiple dependent commits are pushed and then stripped from the on-disk state?

Another caveat here is that if you destroy commits directly in Phabricator's working copy in certain states, it's possible it won't be able to trace ancestry to find all the commits you destroyed in order to mark them unreachable. However, this window is narrow. For Git, you can bin/repository mark-reachable to rebuild reachability if you know you've done a bunch of dangerous mutations to the on-disk state.

Thank you for figuring out the appropriate change!

Thanks!

I think this is probably the cleanest fix, but I only tested event imports (which now appear to work). If you want to test Maniphest with subtypes configured (to make sure it doesn't break) and send me a revision with this change, I'll review it. Otherwise, I'll do that testing when I get a chance and land this if nothing crops up.