Summary: Fixes T9117. Adds a migration to remove ponder vote data.
Test Plan: I added a bunch of lines to phabricator_user.edge with type 18 and they were successfully removed by this patch
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Maniphest Tasks: T9117
Differential Revision: https://secure.phabricator.com/D16452
Summary: Ref T11132. This is a new default default (no dashboard) homepage. It offers (Diffs) (Tasks) (Repositories) in the main column and (Feed) in the side column. No NUX stuff, No logged out public view (upcoming diff). This should be complete, but unclear how to bucketize Differential.
Test Plan: Test new account's default homepage.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16449
Summary: Ref T8628. Updates people controllers for handleRequest
Test Plan: Viewed the people list, viewed the activity logs, then went through the approval process for a new user account.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Maniphest Tasks: T8628
Differential Revision: https://secure.phabricator.com/D16451
Summary: Fixes T8850. Previously, if a user's preamble script mangled `$_SERVER['REMOTE_ADDR']` or somehow set it to `null`, the user would get errors when performing certain actions. Now those errors shouldn't occur, and instead the user will be warned that there is a setup issue related to their preamble script.
Test Plan: Create a preamble script that contains `$_SERVER['REMOTE_ADDR'] = null;` then navigate to /config/issue/. There should be a warning there about `REMOTE_ADDR` not being available.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, yelirekim, epriestley
Maniphest Tasks: T8850
Differential Revision: https://secure.phabricator.com/D16450
Summary: Ref T10951. This adds the application name as an attribute below the document type in the UI for doc type search.
Test Plan: Verify that the application name appears as an attribute on the document type results.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T10951
Differential Revision: https://secure.phabricator.com/D16446
Summary: Ref T10951. This diff removes uninstalled applications from the result set for DocumentType restults
Test Plan: Uninstall an application (diviner for example), then go to the document type search menu and ensure that the uninstalled application doesn't show up.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T10951
Differential Revision: https://secure.phabricator.com/D16445
Summary: Fixes T10999. Now MFA will be required for all email address related operations.
Test Plan: Ensure that adding and removing email addresses now requires you to enter high security mode.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T10999
Differential Revision: https://secure.phabricator.com/D16444
Summary: Previously, the chatbot docs instructed users to get certificates for the conduit API and put the cert in a `conduit.cert` config key. In order to get the chatbot to work, I needed to instead get an API key and put it in the `conduit.token` config entry.
Test Plan: Doc fix. Tried the new documented way and it worked.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D16443
Summary: These were blank, from last week's shenanigans.
Test Plan: View homepage settings, see icons.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16447
Summary: Fixes T11508. The config entry `remarkup.ignored-object-names` already contains a blacklist of object names that should be ignored in the web UI. This change makes that blacklist also apply to the chatbot. This makes it possible to have a chatbot ignore things like V1, V2, Q1 and any other phrases the user may not want to generate links to objects.
Test Plan: Create objects (tasks, slowvotes, etc.) then mention the object names in chat (with the bot running). The bot should respond with helpful links to the given objects. Then add the object names to the blacklist through the config web UI. This apparently triggers the bot to restart itself. Then mention the object names in chat again. The bot should no longer respond with links because those object names have been added to the blacklist regex.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T11508
Differential Revision: https://secure.phabricator.com/D16442
Summary: Ref T11522. This explains how to actually use `bin/repository hint`.
Test Plan: Read the document. Used `bin/repository hint` as directed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11522
Differential Revision: https://secure.phabricator.com/D16441
Summary:
Ref T11522. This tries to reduce the cost of rewriting a repository by making handles smarter about rewritten commits.
When a handle references an unreachable commit, try to load a rewrite hint for the commit. If we find one, change the handle name to "OldHash > NewHash" to provide a strong hint that the commit was rewritten and that copy/pasting the old hash (say, to the CLI) won't work.
I think this notation isn't totally self-evident, but users can click it to see the big error message on the page, and it's at least obvious that something weird is going on, which I think is the important part.
Some possible future work:
- Not sure this ("Recycling Symbol") is the best symbol? Seems sort of reasonable but mabye there's a better one.
- Putting this information directly on the hovercard could help explain what this means.
Test Plan: {F1780719}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11522
Differential Revision: https://secure.phabricator.com/D16437
Summary:
Ref T11522. When a commit is no longer reachable from any branch/tag, we currently show a "this has been deleted" message.
Instead, go further: check if there is a "rewritten" hint pointing at a commit the current commit was rewritten into. If we find one, show a message about that instead.
(This isn't super pretty, just getting it working for now. I expect to revisit this UI in T9713 if we don't get to it before that.)
Test Plan: {F1780703}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11522
Differential Revision: https://secure.phabricator.com/D16436
Summary: Ref T11522. This migrates any "badcommit" data (which probably only exists at Facebook and on 1-2 other installs in the wild) to the new "hint" table.
Test Plan:
- Wrote some bad commit annotations to the badcommit table.
- Viewed them in the web UI and used `bin/repository reparse --change ...` to reparse them. Saw "this is bad" messages.
- Ran migration, verified that valid "badcommit" rows were successfully migrated to become "hint" rows.
- Viewed the new web UI and re-parsed the change, saw "unreadable commit" messages.
- Viewed a good commit; reparsed a good commit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11522
Differential Revision: https://secure.phabricator.com/D16435
Summary:
Ref T11522. This provides storage for tracking rewritten commits (new feature) and unreadable commits (existing feature, but really hacky).
This doesn't do anything yet, just adds a table and a CLI tool for updating it. I'll document the tool once it works. You just pipe in some JSON, but I need to document the format.
Test Plan:
- Piped JSON for "none", "rewritten" and "unreadable" hints into `bin/repository hint`.
- Examined the database to see that the table was written properly.
- Tried to pipe bad JSON in, invalid hint types, etc. Got reasonable human-readable error messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11522
Differential Revision: https://secure.phabricator.com/D16434
Summary: Getting rid of some code! This method has no callsites so it should be safe to remove completely. Ref T9690
Test Plan: Removed method and clicked around to make sure nothing broke.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: yelirekim, epriestley
Maniphest Tasks: T9690
Differential Revision: https://secure.phabricator.com/D16439
Summary: Adds a schema patch that removes conduit_connectionlog. This table hasn't been used in 8ish months so it's probably safe to get rid of.
Test Plan: Apply the patch locally and confirm that the table does indeed get dropped.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16438
Summary: Removed call to the deprecated buildStandardPageResponse method from XHProfProfileController
Test Plan: Install, configure, and use XHProf. I'll need some guidance with this
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D16432
Summary:
After D16431, listing the same `@annotation` multiple times makes the docblock parser return a list.
We have some resources which list `@requires` or `@provides` several times, but don't handle the new parser properly. Make the code more flexible, since this is a reasonable way to specify the annotations.
See also D16432. This produces a failure in this form:
```
[2016-08-23 21:10:15] ERROR 2: trim() expects parameter 1 to be string, array given at [/core/data/drydock/workingcopy-74/repo/phabricator/src/applications/celerity/CelerityResourceMapGenerator.php:236]
2 arcanist(head=master, ref.master=89e8b4852384), phabricator(head=6c940fb71b0a8850c6a1b7f5fc642a8f8135a76a, ref.master=b521f2349e46), phutil(head=master, ref.master=237549280f08)
3 #0 trim(array) called at [<phabricator>/src/applications/celerity/CelerityResourceMapGenerator.php:236]
4 #1 CelerityResourceMapGenerator::getProvidesAndRequires(string, string) called at [<phabricator>/src/applications/celerity/CelerityResourceMapGenerator.php:193]
5 #2 CelerityResourceMapGenerator::rebuildTextResources(CelerityPhabricatorResources, CelerityResourceTransformer) called at [<phabricator>/src/applications/celerity/CelerityResourceMapGenerator.php:54]
6 #3 CelerityResourceMapGenerator::generate() called at [<phabricator>/src/__tests__/PhabricatorCelerityTestCase.php:16]
7 #4 PhabricatorCelerityTestCase::testCelerityMaps()
8 #5 call_user_func_array(array, array) called at [<arcanist>/src/unit/engine/phutil/PhutilTestCase.php:492]
9 #6 PhutilTestCase::run() called at [<arcanist>/src/unit/engine/PhutilUnitTestEngine.php:69]
10 #7 PhutilUnitTestEngine::run() called at [<arcanist>/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php:147]
11 #8 ArcanistConfigurationDrivenUnitTestEngine::run() called at [<arcanist>/src/workflow/ArcanistUnitWorkflow.php:167]
12 #9 ArcanistUnitWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]
```
Test Plan: Ran `bin/celerity map`, no more warnings and no change to the actual map.
Reviewers: joshuaspence, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16433
Summary: Added a brand new shiny cat fact
Test Plan: Pulled up a project with motivator installed and nothing broke
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D16430
Summary: Switches over to new property UI boxes, splits core and apps into separate pages. Move Versions into "All Settings". I think there is some docs I likely need to update here as well.
Test Plan: Click on each item in the sidebar, see new headers.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16429
Summary:
Ref T7148. The automated export process runs this via daemon, which can't answer "Y" to this prompt. Let it "--force" instead.
(Some of my test instances didn't have any repositories, which is why I didn't catch this sooner.)
Test Plan: Ran `bin/repository move-paths --force ...`, saw change applied without a prompt.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7148
Differential Revision: https://secure.phabricator.com/D16426
Summary: Ref T11132. Will work on CSS tomorrow, but wanted to rough in the UI Steps to get guidance. Not sure what you have in mind for the "app" part, if you want to explain it and I build or you build.
Test Plan: Visit each page and click on links. Very rough and unfinished.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16419
Summary: Ref T11132. Adds a background color option to PHUIIconView, for use whereever, and NUX. Also normalize icon placement for mixed image/icon result list.
Test Plan: Test in UIExamples, and Global Settings.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16424
Summary:
Fixes T11490. Currently, this query can not use a key and the table size may be quite large.
Adjust the query so it can use a key for both selection and ordering, and add that key.
Test Plan: Ran `EXPLAIN` on the old query in production, then added the key and ran `EXPLAIN` on the new query. Saw key in use, and "rows" examined drop from 29,273 to 15.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11490
Differential Revision: https://secure.phabricator.com/D16423
Summary: Fixes T11493. This code is a little bit weird/clever, simplify it so that we always cast the handles to an array early on.
Test Plan: {F1767668}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11493
Differential Revision: https://secure.phabricator.com/D16422
Summary: Fixes T11501. Let's you pass in a full PHUIIconView or just the icon name to give ObjectListItem a large icon.
Test Plan: Alamanac, Applications, Drydock, Settings, Search Typeahead, Config page...
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11501
Differential Revision: https://secure.phabricator.com/D16421
Summary:
Ref T11501. This method was removed in D16418, but still has some callsites. I know of four:
- Config
- Settings
- Drydock main page
- Almanac main page
Since I might be missing some and it's close to the release cut, just put the method back for now until we can clean it up more properly.
Test Plan: Viewed Settings, Config, Drydock, Almanac. No more fatal on this method being missing.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11501
Differential Revision: https://secure.phabricator.com/D16420
Summary: I don't think we use footicons, removing that CSS. States were added but only used in Auth, convert them to statusIcon instead.
Test Plan: Visit Auth, UIExamples, grep for `setState`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16418
Summary: Fixes T11491, I believe these are the only two that need to be specified.
Test Plan: Checked Create pages for diffs, tasks, projects, events, blogs, login, etc. Mobile/Desktop.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11491
Differential Revision: https://secure.phabricator.com/D16416
Summary: Ref T11132, Ref T11478. Builds out a basic PHUICMSView and Guides Application, no content / modules.
Test Plan: Go to /guides/, see blank states for new guides.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11132, T11478
Differential Revision: https://secure.phabricator.com/D16414
Summary:
Ref T6996. Depends on D16407. This does the same stuff as D16407, but for `bin/storage renamespace`. In particular:
- Support writing directly to a file (so we can get good errors on failure).
- Support in-process compression.
Also add support for reading out of a `storage dump` subprocess, so we don't have to do a dump-to-disk + renamespace + compress dance and can just stream out of MySQL directly to a compressed file on disk.
This is used in the second stage of instance exports (see T7148).
It would be nice to share more code with `bin/storage dump`, and possibly to just make this a flag for it, although we still do need to do the file-based version when importing (vs exporting). I figured that was better left for another time.
Test Plan:
Ran `bin/storage renamespace --live --output x --from A --to B --compress --overwrite` and similar commands.
Verified that a compressed, renamespaced dump came out of the other end.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6996
Differential Revision: https://secure.phabricator.com/D16410
Summary:
Ref T6996. If you do this kind of thing in the shell, you don't get a good error by default if the `dump` command fails:
```
$ bin/storage dump | gzip > output.sql.gz
```
This can be worked around with some elaborate bash tricks, but they're really clunky and uninintuitive.
We also need to do this in several places (while writing backups; while performing exports), and I don't want to copy clunky bash tricks all over the codebase.
Instead, provide `--output` and `--compress` flags which just do this processing inside `bin/storage dump`. It will fail appropriately if any of the underlying operations fail. This also makes the write a little safer (refuses to overwrite) and the code more reusable.
Test Plan:
- Did three dumps, with no flags, `--output`, and `--output --compress`.
- Verified all three took similar amounts of time and were identical except for "Date Exported" timestamps in comments (except that the compressed one was compressed).
- Used `gunzip` to examine the compressed one, verified it was really compressed.
- Faked a write error, saw properly command behavior (clean up file + exit with error).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6996
Differential Revision: https://secure.phabricator.com/D16407
Summary:
Fixes T11488. I broke this in D16360, I think by doing a little extra refactoring after testing it.
This code is very old, before commits always needed to have repositories attached in order to do policy checks.
Modernize it by mostly just using the repository which is present on the Commit object, and using the existing edge cache.
Test Plan: Ran a commit through the Herald test adapter.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11488
Differential Revision: https://secure.phabricator.com/D16413
Summary: Caught this while linking to it from D16405.
Test Plan: Consulted a dictionary.
Reviewers: chad, alexmv
Reviewed By: alexmv
Differential Revision: https://secure.phabricator.com/D16406
Summary:
Fixes T11487. Improve documentation for three situations:
- When you configure a cluster behind a load balancer, all requests are trusted but not all have an "X-Forwarded-For" header. Change the suggested snippet to read this header only if it exists.
- When a request goes through a series of load balancers (as with a CDN) they can end up writing a list of IPs to the header. Parse these.
- Remove the "rate limiting" stuff -- this got disabled/removed a long time ago and is misleading/incorrect.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11487
Differential Revision: https://secure.phabricator.com/D16403
Summary: Fixes T11484. These mechanisms aren't necessarily obvious and make sense to document here.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11484
Differential Revision: https://secure.phabricator.com/D16404
Summary:
Fixes T11480. This cleans up the error logs a little by quieting three common errors which are really malformed requests:
- The CSRF error happens when bots hit anything which does write checks.
- The "wrong cookie domain" errors happen when bots try to use the `security.alternate-file-domain` to browse stuff like `/auth/start/`.
- The "no phcid" errors happen when bots try to go through the login flow.
All of these are clearly communicated to human users, commonly encountered by bots, and not useful to log.
I collapsed the `CSRFException` type into a standard malformed request exception, since nothing catches it and I can't really come up with a reason why anything would ever care.
Test Plan:
Hit each error through some level of `curl -H ...` and/or fakery. Verified that they showed to users before/after, but no longer log.
Hit some other real errors, verified that they log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11480
Differential Revision: https://secure.phabricator.com/D16402
Summary: This needs an `isset()` for cases when authority and packages don't completely overlap.
Test Plan:
- With a package set to trigger autoreview, created a revision.
- Observed error log, saw no more error.
- Saw package trigger autoreview properly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16398
Summary:
Fixes T11468. I was able to find //a// reproduction case for the issue, at least, described in my comments there.
When the server sends back position information for columns we aren't displaying, for whatever reason, just ignore that information.
Future work in T4900 to do more synchronization may revisit this.
Test Plan: See T11468#190177 for steps. Did that stuff with this patch, got a clean edit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11468
Differential Revision: https://secure.phabricator.com/D16397
Summary:
Ref T11473. If you write a method like `get_stuff(ids)` and then call it with an empty list of IDs, you can end up passing an empty constraint to Conduit.
If you run a `*.search` method with such a constraint, like this one:
```
{
"ids": []
}
```
...we have three possible beahviors:
# Treat it like the user passed no constraint (basically, ignore the constraint).
# Respect the constraint (return no results).
# Error.
Currently, we do (1). However, this is pretty confusing and I think clearly the worst option, since it means `get_stuff(array())` in client code will often tend to return a ton of results.
We could do (2) instead, but this is also sort of confusing (it may not be obvious why nothing matched, even though it's an application bug) and I think most reasonable client code should be doing an `if ($ids)` test: this test makes clients a little more complicated, but they can save a network call, and I think they often need to do this test anyway (for example, to show the user a different message).
This implements (3), and just considers these to be errors: this is the least tricky behavior, it's consistent with what we do in PHP, makes fairly good sense, and the only cost for this is that client code may need to be slightly more complex, but this slightly more complex code is usually better code.
Test Plan: Ran Conduit `*.search` queries with `"ids":[]` and `"phids":[]`, got sensible errors instead of runaway result sets.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11473
Differential Revision: https://secure.phabricator.com/D16396
Summary:
Ref T11473. When running `harbormaster.build.search` with a `buildables` constraint, the constraint doesn't get passed to the Query and so currently has no effect.
This piece of logic was just accidentally omitted from D16356. It is probably not used anywhere today and doesn't show up in the UI, so it's easy to overlook (I missed it in review, too).
Test Plan: Ran `harbormaster.build.search` with a `buildables` constraint, got expected filtering.
Reviewers: yelirekim, chad
Reviewed By: chad
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11473
Differential Revision: https://secure.phabricator.com/D16395
Summary: Fixes T11457, I guess. It feels wierd when you drag it a bit, and Im not positive if it's the calc css or javascript.
Test Plan: Enable filetree, shorten browser, see scrolling appear if files too long.
Reviewers: avivey, epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11457
Differential Revision: https://secure.phabricator.com/D16393
Summary:
When I wrote this the first time, only hosted repositories could be clustered.
This check wasn't removed when I allowed observed repositories to be clustered in D15986.
Test Plan:
Reloaded {nav Config > Repository Servers} page, saw more stuff locally.
Reviewed the cardinal digits between 1 and 17, inclusive.
Reviewers: chad, avivey
Reviewed By: avivey
Differential Revision: https://secure.phabricator.com/D16392
Summary:
The cluster synchronization code runs either actively (before returning a response to `git clone`, for example) or passively (routinely, as the daemons update reposiories).
The active sync runs as the web user (if running `git clone http://...`) or the VCS user (if running `git clone ssh://...`). But the passive sync runs as the daemon user.
All of these sync processes need to run actual commands as the daemon user (`git fetch ...`).
For the active ones, we must `sudo`.
For the passive ones, we're already the right user. We run the same code, and end up trying to sudo to ourselves, which `sudo` isn't happy about by default.
Depending on how `sudo` is configured and which users things are running as this might work anyway, but it's silly and if it doesn't work it requires you to go make non-obvious, weird config changes that are unintuitive and somewhat nonsensical. This is probably worse on the balance than adding a bit of complexity to the code.
Instead, test which user we're running as. If it's already the right user, don't sudo.
Test Plan:
- Ran `bin/repository update --trace` as daemon user, saw no more `sudo`.
- Ran a `git clone` to make sure that didn't break.
Reviewers: chad, avivey
Reviewed By: avivey
Differential Revision: https://secure.phabricator.com/D16391
Summary:
I converted this call incorrectly in D16092. We should pass the `PhutilURI` object, not the string version of it.
Specifically, this resulted in hitting an error like this if a replica needed synchronization:
```
[2016-08-11 21:22:37] EXCEPTION: (InvalidArgumentException) Argument 1 passed to DiffusionCommandEngine::setURI() must be an instance of PhutilURI, string given, called in...
#0 PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/diffusion/protocol/DiffusionCommandEngine.php:52]
#1 DiffusionCommandEngine::setURI(string) called at [<phabricator>/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php:601]
...
```
Test Plan: Clusterized an observed repository, demoted a node, ran `bin/repository update Rxxx` to update, saw no typehint fatal.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16390
Summary:
Ref T11458. Depends on D16388. Currently, we're very aggressive about closing connections in the taskmaster daemons.
This can end up taking up a lot of resources. In particular, because the outgoing port for outbound connections normally can not be reused for 60 seconds after a connection closes, we may exhaust outbound ports on the host if there's a big queue full of stuff that's being processed very quickly.
At a minimum, we //always// are holding open a `worker` connection, which we always need again right away. So even in the best case we end up opening/closing this about once per second and each daemon takes up about ~60 outbound ports when it should take up ~1.
So, make two adjustments:
- First, only close connections which we haven't issued a query on in the last 60 seconds. This should prevent us from closing connections that we'll need again immediately in most cases. In the worst case, we shouldn't be eating up any extra ports under default TCP behavior.
- Second, explicitly close connections. We were relying on implicit/GC behavior (maybe as a holdover from very long ago, before we got connection wrappers in place?), which probably did about the same thing but isn't as predictable and can't be profiled or instrumented.
Test Plan:
This is somewhat difficult to test completely convincingly in isolation since the problem behavior depends on production scales and the workload, and to some degree on configuration.
I tested that this stuff baiscally works by adding logging to connect/close and running the daemons, verifying that they churned connections a lot before this change (e.g., ~1/s even at no load) and churn rarely afterward (e.g., almost never at no load).
I ran some workload through them to make sure I didn't completely break anything.
The best real test is just seeing how production responds. Current inbound/outbound connections on `secure001` are 1,200:
```
secure001 $ netstat -t | grep :mysql | wc -l
1164
```
Current outbound from `repo001` are 18,600:
```
repo001 $ netstat -t | grep :mysql | wc -l
18663
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11458
Differential Revision: https://secure.phabricator.com/D16389
