Summary:
See discussion in D8773. Three small adjustments which should help prevent this kind of issue:
- When queueing followup tasks, hold them on the worker until we finish the task, then queue them only if the work was successful.
- Increase the default lease time from 60 seconds to 2 hours. Although most tasks finish in far fewer than 60 seconds, the daemons are generally stable nowadays and these short leases don't serve much of a purpose. I think they also date from an era where lease expiry and failure were less clearly distinguished.
- Increase the default wait-after-failure from 60 seconds to 5 minutes. This largely dates from the MetaMTA era, where Facebook ran services with high failure rates and it was appropriate to repeatedly hammer them until things went through. In modern infrastructure, such failures are rare.
Test Plan:
- Verified that tasks queued properly after the main task was updated.
- Verified that leases default to 7200 seconds.
- Intentionally failed a task and verified default 300 second wait before retry.
- Removed all default leases shorter than 7200 seconds (there was only one).
- Checked all the wait before retry implementations for anything much shorter than 5 minutes (they all seem reasonable).
Reviewers: btrahan, sowedance
Reviewed By: sowedance
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8774
Summary:
Recently we see issues with huge commits (branch cuts for www) where people received hundreds of emails for the same commit. By checking all the active and archived tasks related to such commits, I saw the following pattern:
- The commit itself is marked as importStatus = 15 which means all the processing was actually done;
- In archived tasks, I see one PhabricatorRepositorySvnCommitMessageParserWorker, one PhabricatorRepositorySvnCommitChangeParserWorker, followed by many PhabricatorRepositoryCommitHeraldWorker, which means that the PhabricatorRepositoryCommitOwnersWorker (who schedule those herald tasks) was never done;
- PhabricatorRepositoryCommitOwnersWorker is always active (for days) with failureCount = 0;
- In daemon log I see a lot of lease expire exception for PhabricatorRepositoryCommitOwnersWorker.
So to me it looks like the following happened:
- Everything is fine until we schedule the PhabricatorRepositoryCommitOwnersWorker
- PhabricatorRepositoryCommitOwnersWorker actually successfully finished but its running time exceed 60s. Before it finishes, it scheduled the PhabricatorRepositoryCommitHeraldWorker task
- When we try to archive it, the lease expiration exception happened. As a result, it stayed active and will be picked up immediately since it is in the head of the queue
- The two steps above repeat forever until we kill it
I am not sure why we want to check lease expiration when we are archiving the task. For now I am giving the worker a little more time since parsing half million affected path needs some time..
Test Plan: Patched in our production and it worked.
Reviewers: lifeihuang, JoelB, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8773
Summary:
The mbstring extension for PHP is not a dependency to any of the already
listed packages on RHEL-like systems, and is needed by Phabricator
(showing a "install mbstring" message as the first thing if it is not
installed)
RHEL seems to have some extra steps to allow php-mbstring to be installed, though:
http://snippets.roozbehk.com/post/35750940300/php-mbstring-missing-on-red-hat-enterprise-linux-6
PS: disabled lint for this change because of the already >80 chars long "yum install" string
Test Plan:
* Created a new container with docker using both centos:6.4 and fedora:20 images
* Ran install script
* Started httpd and mysqld services
* Browsed to server's address
* Got error message
* Installed php-mbstring & restarted httpd
* Works
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8772
Summary: Ref T3551. Since we now require repositories in order to perform policy checks, things that did loads properly don't need to load this data explicitly.
Test Plan: Edited a product, cut a new branch.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3551
Differential Revision: https://secure.phabricator.com/D8769
Summary:
Ref T3551. Releeph has old-style `loadX()` methods; get rid of one of them.
Differential has a couple of copies of this too, clean them up.
Test Plan:
- Viewed various differential revisions (with and without projects).
- Viewed and edited Releeph products.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3551
Differential Revision: https://secure.phabricator.com/D8768
Summary:
Fixes T3657. We no longer construct ambiguous URIs, so product names are no longer restricted.
Also fix some minor URI construction stuff.
Test Plan: Created a product called "branch".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3657
Differential Revision: https://secure.phabricator.com/D8767
Summary:
Ref T3657. General changes here:
- Removes `ReleephProjectController`, which is the source of T3657.
- Mostly moves requests from "RQ" as a monogram to "Y" (looks like a merge, mnemonic for "yank"?, we don't have too many characters left). This should be essentially only cosmetic. This reduces ambiguity with "rQ" and "R123", which are current and future repository monograms. This will continue in the next few diffs.
- Makes requests implement policies correctly.
Test Plan: Created, edited, browsed requests.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3657
Differential Revision: https://secure.phabricator.com/D8766
Summary:
Ref T4045. We have a lot of direct queries against the hunk table right now. These are messy, not really policy-aware, and limit our options on T4045.
This query is unusual (it requires changesets, and does not accept IDs). This keeps us from having to load changeset -> diff -> revision in order to do policy checks. We could also fix this with smarter policy checks and caching, but I'd rather not open that can of worms for now. This object is very low level and relatively unusual, and this small deviation from convention seems like the cleanest cut to make to keep this from snowballing.
Test Plan: Used Herald dry runs to verify that the affected rules still output the same data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045
Differential Revision: https://secure.phabricator.com/D8765
Summary: Ref T4045. These three methods are fairly copy-pastey. Provide a more formal DifferentialHunk API for querying various types of line ranges.
Test Plan: Used test console to verify that "added content", "removed content", and "changed content" rules still produce the same data.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4045
Differential Revision: https://secure.phabricator.com/D8764
Summary: Fixes T4787, decreased z-index on calendar badges so that they don't sit on top of notification dropdowns when dropdowns are expanded. Not sure why the badges had z-index 10, but please let me know if there was a more substantial reason for this.
Test Plan: If neither notification dropdowns have content, create enough messages to populate at least 5 rows, open calendar, expand messages dropdown, verify that underlying calendar date badges do not appear over the dropdown.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4787
Differential Revision: https://secure.phabricator.com/D8770
Summary:
Ref T3644. Ref T3657. Ref T3549. Basically:
- Move these controllers to modern query/policy infrastructure.
- Move them to consistent, ID-based URIs.
- Rename "Project" to "Product"; "Pick Request" to "Pull Request".
- Clean up a few UI things here and there.
Test Plan:
- Created and edited branches.
- Opened and closed branches.
- Viewed branch history.
- Searched within a branch.
- Browsed to branches from products.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3644, T3549, T3657
Differential Revision: https://secure.phabricator.com/D8646
Summary: Ref T4714, tooltips in fullscreen mode need special math, due to fixed position throwing off position of tooltips.
Test Plan: Create work board, create several tasks, create several columns, drag tasks among columns and within columns. Create a paste, check tooltips in comment box show and are positioned correctly. Fullscreen comment box. Verify tooltips still show and position correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4714
Differential Revision: https://secure.phabricator.com/D8762
Summary: Fixes T4774. With the new code and configuration instructions downplaying the role of arcanist project we weren't writing affected paths at all! I had this issue on my installation - no affected paths were written. We seem to always have the repository now though if we can see it, so not too bad of a fix.
Test Plan: updated a diff and was able to browse in diffusion.
Reviewers: epriestley, bitglue
Reviewed By: epriestley, bitglue
Subscribers: bitglue, epriestley, Korvin
Maniphest Tasks: T4774
Differential Revision: https://secure.phabricator.com/D8757
Summary: Fixes T2328. Note the audit part is fixed now.
Test Plan: Tried to reproduce the audit issue by raising my own commit as a problem; it showed up before code changes! Made a diff with my self as author and reviewer; it showed up as expected
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T2328
Differential Revision: https://secure.phabricator.com/D8755
Summary: Fixes T3576
Test Plan: made a countdown and it looked right on view. edited it and it had the right values pre and post edit.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3576
Differential Revision: https://secure.phabricator.com/D8754
Summary: We make a silly query for every commit if you copy/paste a diff.
Test Plan: Copy/pasted diffs now render in fewer than 30 seconds.
Reviewers: btrahan, spicyj
Reviewed By: btrahan, spicyj
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8758
Summary: I haven't been able to understand why this isn't set by default in production environments (since it is recommended to do so anyway).
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8743
Summary: ...add a "renderingTarget" to FeedStory and use it as appropos. Overall, not a ton of changes was necessary to make this work. I think this could be made to be even cleaner by going through each and every feed story and re-implementing as necessary with the full toolset available. But this is good enough for now I think, and just something to keep cleaning up when we're in here. Fixes T4630.
Test Plan: made a task. gave it a token. viewed my feed - saw stories. used conduit.feed.query with mode == 'text' and got good readable results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: spicyj, epriestley, Korvin
Maniphest Tasks: T4630
Differential Revision: https://secure.phabricator.com/D8750
Summary: ...use the prefab stuff as it does fancier things than we were doing. Only trick then really is to pass username and the map of handle phids => icons to the client so prefab can work nicely. Fixes T4775.
Test Plan: made a herald rule with projects and users. Saw nice icons. Reloaded page and still saw nice icons.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4775
Differential Revision: https://secure.phabricator.com/D8749
Summary:
Ref T4786. This doesn't fully fix the issue since there's no way to make channels public yet, but gets some of the infrastructure more up to date.
- Allow public access to the list and log controllers.
- Implement proper policy checks in the Events (this has no practical impact on the only controller that loads this stuff, it's just for general/future purposes).
- Remove a old-style unused method for building page frames.
Test Plan: Viewed log list and log details as logged-in and logged out users.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4786
Differential Revision: https://secure.phabricator.com/D8746
Summary:
When we generate account tokens for CSRF keys and email verification, one of the inputs we use is the user's password hash. Users won't always have a password hash, so this is a weak input to key generation. This also couples CSRF weirdly with auth concerns.
Instead, give users a dedicated secret for use in token generation which is used only for this purpose.
Test Plan:
- Ran upgrade scripts.
- Verified all users got new secrets.
- Created a new user.
- Verified they got a secret.
- Submitted CSRF'd forms, they worked.
- Adjusted the CSRF token and submitted CSRF'd forms, verified they don't work.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8748
Summary: Makes the default 3 rows on mobile devices, with larger fonts. Differential/Audit is much better, Maniphest is maybe a sidegrade depending on setup.
Test Plan: test maniphest, audit, differential list items.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8741
Summary: Ref T4780. This no longer works and barely ever worked. T4237 is now the relevant task.
Test Plan: Grepped for `reconcile.php`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4780
Differential Revision: https://secure.phabricator.com/D8739
Summary: This is still too rough to enable by default. We can turn it on for `secure.phabricator.com` and tweak it a bit first, and then release it in general with higher defaults and more sensible behavior in edge cases.
Test Plan: Loaded some pages.
Reviewers: btrahan, spicyj
Reviewed By: spicyj
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8736
Summary:
I added a getTaskPriorityColor function to the ManiphestTaskPriority class which returns the color set in the maniphest config for the given priority.
This is in preparation for a change to arcanist which will allow it to display the priority color (if it is a supported color) upon running `arc tasks`.
Fixed some linting issues
Test Plan:
Invoke the maniphest.info method from conduit and ensure that:
* The priorityColor property is given in the json
* the priorityColor property is set correctly
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8734
Summary: Fixes T4755. This also includes putting in a note that Google might ToS you to use the Google+ API. Lots of code here as there was some repeated stuff between OAuth1 and OAuth2 so I made a base OAuth with less-base OAuth1 and OAuth2 inheriting from it. The JIRA provider remains an independent mess and didn't get the notes field thing.
Test Plan: looked at providers and read pretty instructions.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4755
Differential Revision: https://secure.phabricator.com/D8726
Summary: Fixes T4777. We technically support `?projects=...` already, but parse it in an unusual way and apply old, awkward, excessively strict lookups to it.
Test Plan: Used reasonable, standard, human-readable strings to prefill `?projects=` and got the results I expected.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4777
Differential Revision: https://secure.phabricator.com/D8733
Summary:
Arcanist is currently displaying all tasks as closed when invoking `arc tasks`.
This is because arcanist is setting the display to closed if there is anything in the `status` property. Adding an isClosed property will allow arcanist to properly display open/closed status on tasks by checking against the isClosed property. The isClosed property will be set according to the closed property that is set on each status in maniphest.
Test Plan:
Invoke the conduit maniphest.info method on any task and insure that:
# The isClosed property is included in the properties
# that it is set properly according to the statuses set for maniphest.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4744
Differential Revision: https://secure.phabricator.com/D8731
Summary:
Fixes T3923. On `secure.phabricator.com`, we occasionally get slowed to a crawl when someone runs a security scanner against us, or 5 search bots decide to simultaneously index every line of every file in Diffusion.
Every time a user makes a request, give their IP address some points. If they get too many points in 5 minutes, start blocking their requests automatically for a while.
We give fewer points for logged in requests. We could futher refine this (more points for a 404, more points for a really slow page, etc.) but let's start simply.
Also, provide a mechanism for configuring this, and configuring the LB environment stuff at the same time (this comes up rarely, but we don't have a good answer right now).
Test Plan: Used `ab` and reloading over and over again to hit rate limits. Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T3923
Differential Revision: https://secure.phabricator.com/D8713
Summary: Bad news @cpojer @tomo. IE8 doesn't like you.
Test Plan: Load a diff in IE8; see changes and don't get JS errors.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, tomo, Korvin, cpojer
Differential Revision: https://secure.phabricator.com/D8728
Summary:
Fixes T4759.
Turns out Chrome on windows doesn't really like the word joiner character. We'll switch back to zwsp but make it `position: absolute;` so it doesn't turn into a line break.
Test Plan: Looked at diffs in IE9 and Chrome Windows. Made sure copying still works as expected.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4759
Differential Revision: https://secure.phabricator.com/D8727
Summary: Fixes T4772. We weren't parsing generated public keys properly, and were storing them in the wrong format.
Test Plan:
- Updated a private key.
- Generated a public key.
- Saved the public key.
- Used a generated private key to authenticate.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4772
Differential Revision: https://secure.phabricator.com/D8721
Summary:
Fixes T4773. For config settings of type `list<string>`, `set`, or `list<regex>`, the "defaults" table and "examples" aren't always in the same format you should actually use when changing the setting.
This is pretty confusing. Instead, always show the settings in the desired format. For example, if the user should enter a newline-separated list, show them a newline separated list.
Test Plan:
- Grepped for `list<string>`, `list<regex>`, and `'set'`; verified all the config had the right example format (most already did).
- Viewed config settings of various kinds, including custom settings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4773
Differential Revision: https://secure.phabricator.com/D8725
Summary: wasn't working due to some type issues. Fixes T4756. I also made it display nicer while I was debugging this.
Test Plan: created a herald rule to block changes that added refs. git tag -a "test" -m "test test"; git push origin test got me blocked!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4756
Differential Revision: https://secure.phabricator.com/D8724
Summary: Fixes T3208. This forces us to bind+search even if there are no anonymous credentials.
Test Plan: Checked the box, saved the form. Unchecked the box, saved the form. LDAP??
Reviewers: Firehed
Reviewed By: Firehed
Subscribers: epriestley
Maniphest Tasks: T3208
Differential Revision: https://secure.phabricator.com/D8723
Summary:
See <https://github.com/facebook/phabricator/pull/563>.
I think this secondary construction of a `$user` is very old, and predates subsequent changes which cause a proper user to construct earlier, so using the user on the `$request` should (I think) always work. I couldn't immediately find any cases where it does not.
Test Plan: With `debug.stop-on-redirect` set, hit various redirects, like jump-naving to T1. Got a proper stop dialog.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8718
Summary: Ref T182. This feature rarely/never works and is on the balance enormously confusing to users (see <https://github.com/facebook/phabricator/issues/566>). If installs have somehow made it sort of work, they can comment this line out for now until we have time to make this work more reasonably.
Test Plan: Looked at a revision in Differential.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D8719
Summary: Fixes T3426. This describes all the weird stuff we've got, at least. We can expand this as we get more contributors or after writing CSS lint.
Test Plan: Read document.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T3426
Differential Revision: https://secure.phabricator.com/D8720
Summary: Ref T4342. Puts meta="referrer" on everything.
Test Plan: In Safari, used the Charles http proxy to verify this change actually stops referrers from being sent.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4342
Differential Revision: https://secure.phabricator.com/D8712
Summary:
Fixes T4736. Currently, we incorrectly skip the `writeImportStatusFlag()` call if publishing is disabled (the `herald-disabled`) check. This means we don't flag the commit as imported, and don't move the pipeline forward correctly.
Instead, we only want to skip the owners stuff, not the pipeline stuff. Move that to a method.
(Also fix a nearby TODO now that we have a permanent failure exception.)
Test Plan:
- Used `scripts/repository/reparse.php --owners ...` to execute this code, fiddled with things to hit both the disabled and enabled branches and verified the flag stuff is still reached.
- Faked the exceptions and made sure they raise correctly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4736
Differential Revision: https://secure.phabricator.com/D8715
Summary: I also changed PhabricatorApplicationTransactionFeedStory and the TokenGivenFeedStory to include only the title/first line of the feed story, which is more convenient (previously, strip_tags gave a multi-line story without even any linebreaks) and more consistent with the other story types.
Test Plan: Added a requestbin URL to feed.http-hooks, commented on a Differential, and saw storyText equal to "alpert added a comment to D2: c." in the POST data it received.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4630
Differential Revision: https://secure.phabricator.com/D8710
Summary: From IRC, this is sometimes helpful for debugging if there's a mailing list issue or something like that. For example, it can show "To" and "Cc".
Test Plan: Got some email, saw headers in it.
Reviewers: dctrwatson, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8708
Summary:
This text is overly repetitive and is not super important. Keeps the other states. Also
- Easier to parse reviewers now
- Mobile is less janky
Test Plan:
reload my list of diffs
{F138756}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8707
Summary: Fixes T4687. This was also pretty easy...!
Test Plan: made a package with a test user as owner. added package as owner. looked right on commit page. logged in as test user and verified audit showed up on home page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4687
Differential Revision: https://secure.phabricator.com/D8705
