@avivey - When arc patch runs itself recursively. The thing I realized is arc patch first tries to get the patch without using credentials (but it still requires a conduit), and then when it realizes the request is rejected due to missing credentials, it manually adds them by calling authenticateConduit. After that it calls run, which in turn calls buildChildWorkflow. Since the parent buildChildWorkflow doesn't know about the already established conduit which has its credentials already set ($workflow->conduit being set) it simply tries to reset it causing $workflow->setConduitCredentials($this->conduitCredentials); call to throw the You may not set new credentials after authenticating conduit. error since credentials were already set manually by arc patch.

Does this make sense? I may be wrong about this trace but it looks very much like the case.

Ok, this is a little heavy on context. tl;dr: This is a symptom.

We have a leaky abstraction, where buildWorkflow() is cached, returning an already-built and used workflow instance. Overwriting the conduit is a symptom of this cache.

Similar, and worse, behaviors, will turn up in more complicated flows, where the same Workflow class is expected to be run in multiple configurations (possibly in parallel). One hypothetical example could be a cross-repository-diff-workflow; arc patch-with-dependencies is obviously another one.

A possible solution might be to have buildWorkflow() return a new instance, which will seal this leak, but that might have more side-effects to consider.

Building a separate Conduit client also feels weird to me -- it feels surprising that workflows are cached (which feels pointless and fragile) but the Conduit client is not (which feels expected and desirable).

I suspect these decisions are cruft or poor judgement from ages long past, not meaningful architectural choices.

In particular, it seems very likely that the workflow caching is a consequence of moving ArcanistConfiguration to use PhutilClassMapQuery (which caches, while PhutilSymbolLoader did not), and that the one-line fix is probably to add clone to ArcanistConfiguration::buildWorkflow(). The structure of this class generally predates modern/common use of modular classes and doesn't do a good job of separating the concepts of "load the available implementations for read-only operations" vs "give me an instance of something so I can write to it". This is something we do a somewhat weak job of in general, but probably ~90% of the extension points are always read-only and I think we've always caught the need to clone the other 10%, so far. This might merit some thought and formalization of conventions if it occurs a couple more times, though.

In this case, I'd say the best quick fix is to clone in either buildWorkflow() or buildAllWorkflows(), to restore the pre-PhutilClassMapQuery semantics. It appears that all callers of buildAllWorkflows() use it in read-only way, so putting it in buildWorkflow() is likely sufficient:

diff --git a/src/configuration/ArcanistConfiguration.php b/src/configuration/ArcanistConfiguration.php
index 91d0dc6..e66b793 100644
--- a/src/configuration/ArcanistConfiguration.php
+++ b/src/configuration/ArcanistConfiguration.php
@@ -31,7 +31,7 @@ class ArcanistConfiguration extends Phobject {
       $command = 'version';
     }
 
-    return idx($this->buildAllWorkflows(), $command);
+    return clone idx($this->buildAllWorkflows(), $command);
   }
 
   public function buildAllWorkflows() {

Can you confirm that fixes the arc patch issue?

Beyond the immediate issue, we should probably eventually:

share a single Conduit client across workflows, at least by default;
eventually realign arc extension points with other modern APIs, but this will break some users -- this likely makes sense concurrent with T5055;
keep an eye on further clone issues and consider technical or conventional approaches to reducing their frequency and impact if they continue occuring.

This revision now requires changes to proceed.Sep 11 2015, 4:24 AM

Basically, just what @avivey said, with one additional note: the move to PhutilClassMapQuery in rARC0e095787 (less than a month ago) is almost certainly the root cause of the caching, so reverting this is almost certainly straightforward and not risking years of weird side effects coming home to roost.

@epriestley - so are you saying I should simply revert rARC0e0957872083daa38309204ad9774568c9fcdd5c? I mean that looks like a nice patch and given all the context you and @avivey shared (thanks a ton btw for that), I may be able to prevent caching "properly".

Which direction would you prefer?