Summary:
Ref T10849. This enforces a global 30-second per-query time limit for anything not coming from the CLI.
If we run into another issue with MySQL hanging in the future, this should prevent it from being nearly as bad as it was.
Test Plan:
- Set value to 0, verified the UI threw an exception immediately.
- Set value back to 30, browsed around a bunch of pages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10849
Differential Revision: https://secure.phabricator.com/D15799
Summary:
Ref T4292. This is a required step in configuring a cluster: document and explain it.
Previously `bin/almanac register` could //also// add and trust keys. I've removed this capability since I think it's needless and complicated. If there's some real use for it eventually, we could add a `bin/almanac add-key` or whatever. The workflow is simpler and has better guard rails that point you in the correct direction now.
Test Plan:
- Read documentation.
- Ran `bin/almanac` with various good/bad flags.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15795
Summary: This gets over-escaped instead of bolded right now, but I only ever hit it when exporting/importing and never both cleaning it up.
Test Plan: Ran `bin/repository move-paths`, saw bolded "Move" instead of ANSI escape sequences.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15797
Summary:
Ref T10860. This allows us to recover if the connection to the database is lost during a push.
If we lose the connection to the master database during a push, we would previously freeze the repository. This is very safe, but not very operator-friendly since you have to go manually unfreeze it.
We don't need to be quite this aggressive about freezing things. The repository state is still consistent after we've "upgraded" the lock by setting `isWriting = 1`, so we're actually fine even if we lost the global lock.
Instead of just freezing the repository immediately, sit there in a loop waiting for the master to come back up for a few minutes. If it recovers, we can release the lock and everything will be OK again.
Basically, the changes are:
- If we can't release the lock at first, sit in a loop trying really hard to release it for a while.
- Add a unique lock identifier so we can be certain we're only releasing //our// lock no matter what else is going on.
- Do the version reads on the same connection holding the lock, so we can be sure we haven't lost the lock before we do that read.
Test Plan:
- Added a `sleep(10)` after accepting the write but before releasing the lock so I could run `mysqld stop` and force this issue to occur.
- Pushed like this:
```
$ echo D >> record && git commit -am D && git push
[master 707ecc3] D
1 file changed, 1 insertion(+)
# Push received by "local001.phacility.net", forwarding to cluster host.
# Waiting up to 120 second(s) for a cluster write lock...
# Acquired write lock immediately.
# Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"...
# Acquired read lock immediately.
# Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local001.phacility.net".
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 254 bytes | 0 bytes/s, done.
Total 3 (delta 1), reused 0 (delta 0)
BEGIN SLEEP
```
- Here, I stopped `mysqld` from the CLI in another terminal window.
```
END SLEEP
# CRITICAL. Failed to release cluster write lock!
# The connection to the master database was lost while receiving the write.
# This process will spend 300 more second(s) attempting to recover, then give up.
```
- Here, I started `mysqld` again.
```
# RECOVERED. Link to master database was restored.
# Released cluster write lock.
To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git
2cbf87c..707ecc3 master -> master
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15792
Summary:
Ref T10860. At least in Git over SSH, we can freely echo a bunch of stuff to stderr and Git will print it to the console, so we can tell users what's going on.
This should make debugging, etc., easier. We could tone this down a little bit once things are more stable if it's a little too chatty.
Test Plan:
```
$ echo D >> record && git commit -am D && git push
[master ca5efff] D
1 file changed, 1 insertion(+)
# Push received by "local001.phacility.net", forwarding to cluster host.
# Waiting up to 120 second(s) for a cluster write lock...
# Acquired write lock immediately.
# Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"...
# Acquired read lock immediately.
# Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local001.phacility.net".
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 256 bytes | 0 bytes/s, done.
Total 3 (delta 1), reused 0 (delta 0)
To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git
8616189..ca5efff master -> master
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15791
Summary: Ref T10860. This doesn't change anything, it just separates all this stuff out of `PhabricatorRepository` since I'm planning to add a bit more state to it and it's already pretty big and fairly separable.
Test Plan: Pulled, pushed, browsed Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15790
Summary:
Fixes T10865.
- Mock descriptions did not markup.
- Image descriptions did not get a proper container `<div />`.
Test Plan:
- Created a mock with remarkup in the mock description and in an image description.
- Viewed mock detail.
- Saw list styles render properly in both mock description and image description.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10865
Differential Revision: https://secure.phabricator.com/D15793
Summary: Fixes T9560. We suggest a root-owned location, but users who choose their own location instead can run into trouble.
Test Plan:
- Changed parent directory to have an non-root owner, verified that `ssh` no longer worked.
- Changed parent directory back to a root owner, verified `ssh` worked again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9560
Differential Revision: https://secure.phabricator.com/D15794
Summary: Fixes T10863. See that task for discussion.
Test Plan:
- Configured `aphlict` with no "logs".
- Started `aphlict`.
- Before change: exception.
- After change: worked.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10863
Differential Revision: https://secure.phabricator.com/D15788
Summary: Fixes T10857. This documentation did not accurately reflect proper configuration: in the Aphlict config, SSL is inferred from the presence of `ssl.*` configuration.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10857
Differential Revision: https://secure.phabricator.com/D15787
Summary:
Ref T4292. Sometimes, we may not have a working copy for a repository. The easiest way to get into this condition is to deactivate a repository.
We could try to clone + fetch in this case, but that's kind of complex, and there's an easy command that administrators can run manually. For now, just tell them to do that.
This affects the inactive repositories on `secure`, like rGITCOINS.
Test Plan: Removed working copy, got message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15786
Summary:
Ref T7789. Currently, we use different viewers if you have `security.alternate-file-domain` configured vs if you do not.
This is largely residual from the days of one-time-tokens, and can cause messy configuration-dependent bugs like the one in T7789#172057.
Instead, always use the omnipotent viewer. Knowledge of the secret key alone is sufficient to access a file.
Test Plan:
- Disabled `security.alternate-file-domain`.
- Reproduced an issue similar to the one described on T7789.
- Applied change.
- Clean LFS interaction.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15784
Summary:
Ref T4292. When the daemons make a query for repository information, we need to make sure the working copy on disk is up to date before we serve the response, since we might not have the inforamtion we need to respond otherwise.
We do this automatically for almost all Diffusion methods, but this particular method is a little unusual and does not get this check for free. Add this check.
Test Plan:
- Made this code throw.
- Ran `bin/repository reparse --message ...`, saw the code get hit.
- Ran `bin/repository lookup-user ...`, saw this code get hit.
- Made this code not throw.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15783
Summary: I've possibly seen a couple of `aphlict` processes exit under suspicious circumstances (maybe?). Make sure any PHP errors get captured into the log.
Test Plan:
- Added an exception after forking.
- Before change: vanished into thin air.
- After change: visible in the log.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15782
Summary:
Fixes T10843. In a multi-server setup, we can do this:
- Two servers, A and B.
- You push an update.
- A gets pushed first.
- After A has been pushed, but before B has been pushed, a user loads a page from A.
- It generates resource URIs like `/stuff/new/package.css`.
- Those requests hit B.
- B doesn't have the new resources yet.
- It responds with old resources.
- Your CDN caches things. You now have a poisoned CDN: old data is saved in a new URL.
To try to avoid this with as little work as possible and generally make it hard to get wrong, check the URL hash against the hash we would generate.
If they don't match, serve our best guess at the resource, but don't cache it. This should make things mostly keep working during the push, but prevent caches from becoming poisoned, and everyone should get a working version of everything after the push finishes.
Test Plan:
- `curl`'d a resource, got a cacheable one.
- Changed the hash a little, `curl`'d again. This time: valid resource, but not cacheable.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10843
Differential Revision: https://secure.phabricator.com/D15775
Summary: Ref T4292. This adds a new high-level overview panel.
Test Plan: {F1238854}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15772
Summary: Fixes T8897
Test Plan: Open any list view of Calendar events, every event should only show "Attending: ..." with users who are attending event.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T8897
Differential Revision: https://secure.phabricator.com/D15779
Summary: This still wasn't quite right -- a link like `[[ Porcupine Facts ]]` with a space would not lookup correctly, and would render as `porcupine_facts`.
Test Plan: Verified that `[[ Porcupine Facts ]]` now works correctly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15780
Summary: Some of these had the wrong `@group` header.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15778
Summary: Fixes T10845.
Test Plan: Verified that `[[ quack ]]` and `[[ QUACK ]]` both work. Previously, the link had to exactly match the capitalization of the target.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10845
Differential Revision: https://secure.phabricator.com/D15777
Summary: Fixes T10844. After recent changes to Aphlict (T6915 and T10697), `./bin/status` needs to be aware of the configuration file. As such, it is now necessary to run `./bin/aphlict status --config /path/to/config.json` rather than `./bin/aphlict status`.
Test Plan: Ran `./bin/aphlict start ...` and `./bin/aphlict status` and saw "Aphlict (`$PID`) is running".
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10844
Differential Revision: https://secure.phabricator.com/D15776
Summary:
Fixes T8952. When Herald changes subscribers, it is zzzzz very boring.
When users change subscribers, it is still super boring (more boring than a merge, for example).
Test Plan: Viewed feed, saw fewer Herald stories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8952
Differential Revision: https://secure.phabricator.com/D15774
Summary:
Ref T8952. Currently, when an application (most commonly Herald, but sometimes Drydock, Diffusion, etc) publishes a feed story, we get an empty grey box for it in feed.
Instead, give the story a little application icon kind of "profile picture"-like thing.
Test Plan:
Here's how it looks:
{F1239003}
Feel free to tweak/counter-diff.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8952
Differential Revision: https://secure.phabricator.com/D15773
Summary:
Ref T10751. Add support tooling for manually prying your way out of trouble if disaster strikes.
Refine documentation, try to refer to devices as "devices" more consistently instead of sometimes calling them "nodes".
Test Plan: Promoted and demoted repository devices with `bin/repository thaw`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15768
Summary:
Ref T10751. Make the UI more useful and explain what failure states mean and how to get out of them.
The `bin/repository thaw` command does not exist yet, I'll write that soon.
Test Plan: {F1238241}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15766
Summary: Fixes T10744
Test Plan: Create recurring event, cancel one instance, cancel the parent event, reinstate event. Wording in the reinstating dialog should be clear about reinstating only instances that haven't been individually cancelled.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10744
Differential Revision: https://secure.phabricator.com/D15770
Summary: Fixes T9295
Test Plan: Create event, open datepicker for start date, choose 1/31/2016, open datepicker again, click right button to scroll month. New suggested date should be 2/29/2016
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T9295
Differential Revision: https://secure.phabricator.com/D15727
Summary: This is completely obsoleted by `owners.search`. See D15472.
Test Plan: Viewed API method in UI console.
Reviewers: avivey, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15769
Summary:
Fixes T10840. When rendering mail, this rule wasn't falling through in quite the right way.
Also adjust where the rules are for this so the special styles show up in Maniphest, etc.
Test Plan:
Made this comment:
{F1238266}
Which produced this HTML:
{F1238267}
...and sent this mail:
{F1238283}
Reviewers: hach-que, chad
Reviewed By: chad
Maniphest Tasks: T10840
Differential Revision: https://secure.phabricator.com/D15767
Summary: Ref T10751. This cleans this up so it's a little more modern, and fixes a possible bad access on the log detail page.
Test Plan: Viewed push log list, viewed push log detail.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15765
Summary: Ref T10751. There are still some missing support tools here, but explain some of this a little better.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15764
Summary:
Ref T10751. Provide some guidance on replicas and promotion.
I'm not trying to walk administrators through the gritty details of this. It's not too complex, they should understand it, and the MySQL documentation is pretty thorough.
Test Plan: Read documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10751
Differential Revision: https://secure.phabricator.com/D15763
Summary: This will stop breaking if you have subscribers and tags when updating a revision (`Error parsing field "Subscribers": The objects you have listed include objects which do not exist (Tags:)`), which I broke in D15749.
Test Plan: run through arc-diff --update that failed earlier.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15762
Summary:
Ref T4292. Right now, repository versions only get marked when a write happens.
This potentially creates a problem: if I pushed all the sync code to `secure` and enabled `secure002` as a repository host, the daemons would create empty copies of all the repositories on that host.
Usually, this would be fine. Most repositories have already received a write on `secure001`, so that working copy has a verison and is a leader.
However, when a write happened to a rarely-used repository (say, rKEYSTORE) that hadn't received any write recently, it might be sent to `secure002` randomly. Now, we'd try to figure out if `secure002` has the most up-to-date copy of the repository or not.
We wouldn't be able to, since we don't have any information about which node has the data on it, since we never got a write before. The old code could guess wrong and decide that `secure002` is a leader, then accept the write. Since this would bump the version on `secure002`, that would //make// it an authoritative leader, and `secure001` would synchronize from it passively (or on the next read or write), which would potentially destroy data.
Instead:
- Refuse to continue in situations like this.
- When a repository is on exactly one device, mark it as a leader with version "0".
- When a repository is created into a cluster service, mark its version as "0" on all devices (they're all leaders, since the repository is empty).
This should mean that we won't lose data no matter how much weird stuff we run into.
Test Plan:
- In single-node mode, used `repository update` to verify that `0` was written properly.
- With multiple nodes, used `repository update` to verify that we refuse to continue.
- Created a new repository, verified versions were initialized correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15761
Summary: Ref T4292. When we write a push log, also log which node received the request.
Test Plan: {F1230467}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15759
Summary:
Ref T4292. We currently synchronize hosted, clustered, Git repositories when we receive an SSH pull or push.
Additionally:
- Synchronize before HTTP reads and writes.
- Synchronize reads before Conduit requests.
We could relax Conduit eventually and allow Diffusion to say "it's OK to give me stale data".
We could also redirect some set of these actions to just go to the up-to-date host instead of connecting to a random host and synchronizing it. However, this potentially won't work as well at scale: if you have a larger number of servers, it sends all of the traffic to the leader immediately following a write. That can cause "thundering herd" issues, and isn't efficient if replicas are in different geographical regions and the write just went to the east coast but most clients are on the west coast. In large-scale cases, it's better to go to the local replica, wait for an update, then serve traffic from it -- particularly given that writes are relatively rare. But we can finesse this later once things are solid.
Test Plan:
- Pushed and pulled a Git repository over HTTP.
- Browsed a Git repository from the web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15758
Summary:
Ref T4292. Before we write or read a hosted, clustered Git repository over SSH, check if another version of the repository exists on another node that is more up-to-date.
If such a version does exist, fetch that version first. This allows reads and writes of any node to always act on the most up-to-date code.
Test Plan: Faked my way through this and got a fetch via `bin/repository update`; this is difficult to test locally and needs more work before we can put it in production.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15757
Summary:
Ref T4292. When you run `git fetch` and connect to, say, `repo001.west.company.com`, we'll look at the current version of the repository in other nodes in the cluster.
If `repo002.east.company.com` has a newer version of the repository, we'll fetch that version first, then respond to your request.
To do this, we need to run `git fetch repo002.east.company.com ...` and have that connect to the other host and be able to fetch data.
This change allows us to run `PHABRICATOR_AS_DEVICE=1 git fetch ...` to use device credentials to do this fetch. (Device credentials are already supported and used, they just always connect as a user right now, but these fetches should be doable without having a user. We will have a valid user when you run `git fetch` yourself, but we won't have one if the daemons notice that a repository is out of date and want to update it, so the update code should not depend on having a user.)
Test Plan:
```
$ PHABRICATOR_AS_DEVICE=1 ./bin/ssh-connect local.phacility.com
Warning: Permanently added 'local.phacility.com' (RSA) to the list of known hosts.
PTY allocation request failed on channel 0
phabricator-ssh-exec: Welcome to Phabricator.
You are logged in as device/daemon.phacility.net.
You haven't specified a command to run. This means you're requesting an interactive shell, but Phabricator does not provide an interactive shell over SSH.
Usually, you should run a command like `git clone` or `hg push` rather than connecting directly with SSH.
Supported commands are: conduit, git-lfs-authenticate, git-receive-pack, git-upload-pack, hg, svnserve.
Connection to local.phacility.com closed.
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15755
Summary:
Ref T4292. This consolidates code for figuring out which user we should connect to hosts with.
Also narrows a lock window.
Test Plan: Browsed Diffusion, pulled and pushed through an SSH proxy.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15754
Summary:
In calendar, dashboard, diffusion, diviner, feed, fund,
maniphest, pholio, ponder, and slowvote use the term 'tags' if possible.
This intenctionally skips diffusion, differential, and the projects application itself.
Ref T10326 Ref T10349
Test Plan: inspection on a running, locally modified, system
Reviewers: avivey, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10835, T10326, T10349
Differential Revision: https://secure.phabricator.com/D15753
Summary:
Ref T10832. In practice, `git --version` is not a useful test for this issue:
- Vendors like Debian have backported the patch into custom versions like `0.0.0.1-debian-lots-of-patches.3232`.
- Vendors like Ubuntu distribute multiple different versions which report the same string from `git --version`, some of which are patched and some of which are not.
In other cases, we can perform an empirical test for the vulnerability. Here, we can not, because we can't write a 2GB path in a reasonable amount of time.
Since vendors (other than Apple) //generally// seem to be on top of this and any warning we try to raise based on `git --version` will frequently be incorrect, don't raise this warning.
I'll note this in the changelog instead.
Test Plan: Looked at setup issues, no more warning for vulnerable git version.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10832
Differential Revision: https://secure.phabricator.com/D15756
Summary:
Ref T4292. Ref T10366. Depends on D15751. Today, generating repository commands is purely a function of the repository, so they use protocols and credentials based on the repository configuration.
For example, a repository with an SSH "remote URI" always generate SSH "remote commands".
This needs to change in the future:
- After T10366, repositories won't necessarily just have one type of remote URI. They can only have one at a time still, but the repository itself won't change based on which one is currently active.
- For T4292, I need to generate intracluster commands, regardless of repository configuration. These will have different protocols and credentials.
Prepare for these cases by separating out command construction, so they'll be able to generate commands in a more flexible way.
Test Plan:
- Added unit tests.
- Browsed diffusion.
- Ran `bin/phd debug pull` to pull a bunch of repos.
- Ran daemons.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292, T10366
Differential Revision: https://secure.phabricator.com/D15752
Summary:
Trigger daemon is trying to find the next event to invoke before sleeping, but the query includes already-elapsed triggers.
It then tries to sleep for 0 seconds.
Test Plan:
On a new instance, schedule a single trigger of type `PhabricatorOneTimeTriggerClock` to a very near time.
Use top to see trigger daemon not going to 100% CPU once the event has elapsed.
Reviewers: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15750
Summary: Users can't find the "Tags" field in the Edit Menu; Added keyword "Tag".
Test Plan: Looked in Edit page; I think this shouldn't change anything else?
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15749
Summary: Ref T4292. This will let the UI and future `bin/repository` tools give administrators more tools to understand problems when reporting or resolving them.
Test Plan:
- Pushed fully clean repository.
- Pushed previously-pushed repository.
- Forced write to abort, inspected useful information in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15748
Summary:
Ref T4292. Small fixes:
- There was a bug with the //first// write, where we'd write 1 but expect 0. Fix this.
- Narrow the window where we hold the `isWriting` lock: we don't need to wait for the client to finish.
- Release the lock even if something throws.
- Use a more useful variable name.
Test Plan:
- Made new writes to a fresh cluster repository.
- Made sequential writes.
- Made concurrent writes.
- Made good writes and bad writes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15747
Summary:
Fixes T10830.
- The return code from `storage adjust` did not propagate correct.
- There was one column issue which I missed the first time around because I had a bunch of unrelated stuff locally.
Test Plan:
- Ran `bin/storage upgrade -f` with failures, used `echo $?` to make sure it exited nonzero.
- Got fully clean `bin/storage adjust` by dropping all my extra local tables.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10830
Differential Revision: https://secure.phabricator.com/D15746
Summary: Ref T10832. Raise a setup warning for out-of-date versions of `git`.
Test Plan: {F1224632}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10832
Differential Revision: https://secure.phabricator.com/D15745
