Permalink
Switch branches/tags
Nothing to show
Commits on Apr 23, 2017
  1. Stabilize sort order for Remarkup block rules

    Summary:
    Fixes T10929. Currently, the ordering of Remarkup block rules is ambiguous (multiple rules may have the same priority number) and depends on sort stability (if elements with the same value have their ordered retained by the sort).
    
    Sort stability changed between PHP5 and PHP7, so the result of functions like `asort()` changed too if some of the values are the same.
    
    Currently, some tests fail and some edge-case beahviors differ under PHP7 because of this. Particularly, the Remarkup literal test case fails under PHP 7.1 locally, because `<space><space>%%%` gets interpreted as a code block instead of a literal block.
    
    To fix this:
    
      - Use `msortv()` with `PhutilSortVector`, which is a stable sort.
      - Sort on `<priority, ClassName>`. The class name is guaranteed unique so this ordering is unambiguous.
    
    Test Plan:
    Previously, a literal block test case failed on PHP7.1 locally:
    
    {F4920739}
    
    This test now passes.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T10929
    
    Differential Revision: https://secure.phabricator.com/D17771
    epriestley committed Apr 23, 2017
  2. Make "fail quietly" flag in DeferredLog a little more quiet

    Summary:
    Ref T12611. Currently, access logs have a "setFailQuietly(true)" flag set on them (rationale: the webserver should still work if the log volume fills up or logs are otherwise not correctly configured) but it's only somewhat quiet.
    
    Make it quieter when the log directory itself (not just the logfile) doesn't exist or can't be created.
    
    (If installs run into confusion with this, we could more reasonably raise a setup warning instead.)
    
    Test Plan: With a missing logfile directory, saw a log about being unable to log instead of an exception.
    
    Reviewers: chad, amckinley
    
    Reviewed By: amckinley
    
    Maniphest Tasks: T12611
    
    Differential Revision: https://secure.phabricator.com/D17777
    epriestley committed Apr 23, 2017
Commits on Apr 13, 2017
  1. Make the query compiler emit intermediate tokens

    Summary:
    Ref T12137. Ref T12003. Ref T2632. Ref T7860. In all these tasks, we want to examine or interact with the query //after// it is parsed, but before it is compiled.
    
    Make the compiler emit an intermediate parsed representation so we can do that.
    
    (The next change fixes Phabricator for this new API.)
    
    Test Plan: Ran unit tests, see also next change.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12137, T12003, T7860, T2632
    
    Differential Revision: https://secure.phabricator.com/D17669
    epriestley committed Apr 12, 2017
Commits on Apr 10, 2017
  1. Reject ambiguous URIs with unescaped "#" or "?" in username/password …

    …parts
    
    Summary:
    Fixes T12526. These URIs are ambiguous and nonstandard, and different versions of different clients parse them differently.
    
    Instead of trying to get this right across PHP versions, just reject these outright. No normal user will ever expect these to work.
    
    Test Plan: Ran unit tests in PHP 7.1, got clean results. See T12526 for more discussion.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12526
    
    Differential Revision: https://secure.phabricator.com/D17647
    epriestley committed Apr 10, 2017
  2. Make Calendar recurrence sets return the indexes of recurrence dates …

    …properly
    
    Summary:
    Ref T11816. Currently, if you specify a start time, you get back indexes starting from that start time (0, 1, ...).
    
    This can cause some bugs elsewhere when querying for recurring event ranges.
    
    Instead, return the indexes consistently, even if there is a start time.
    
    Test Plan: Added unit tests, made them pass. Fixed an existing unit test.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T11816
    
    Differential Revision: https://secure.phabricator.com/D17644
    epriestley committed Apr 10, 2017
Commits on Mar 25, 2017
  1. Allow daemons to read their own idle state

    This sets up the next change, which will make the Taskmaster hibernate
    only if it has been idle for more than a minimum amount of time.
    
    Auditors: chad
    epriestley committed Mar 25, 2017
  2. Don't hibernate daemons if we'd sleep for less than 30 seconds

    In some production situations, PullLocal daemons are going up and down
    fairly frequently. Instead, only hibernate if we'll be down for a while.
    
    Auditors: chad
    epriestley committed Mar 25, 2017
  3. When daemons hibernate, mark them as not busy

    The idle timer was not being reset on a hibernation exit vs a scaledown exit,
    so the pool could get sort of "stuck" relaunching the second daemon. Instead,
    reset the idle timer on hibernation exit.
    
    `bin/phd debug task --pool 4` now scales down cleanly instead of trying to
    relaunch the second daemon after its scales down if the first one is
    hibernating.
    
    Auditors: chad
    epriestley committed Mar 25, 2017
Commits on Mar 24, 2017
  1. Clean up a few more daemon behaviors

    Summary:
    Ref T12298.
    
      - If //all// daemons in //all// pools are hibernating already, we may exit immediately without sending EXIT events and fail to update the web UI.
      - A `max()` should be `min()` ("sleep for at most 180 seconds", not "sleep for at least 180 seconds").
      - If there are no daemons in a pool (unlikely/theoretical), we might not set the shutdown flag on the pool correctly.
      - An autoscale message may actually mean that a pool is exiting.
    
    Test Plan:
      - Ran `bin/phd debug task`, waited for daemon to hibernate, killed it, saw it vanish from daemon console.
      - Saw it hibernate for 60s, not 180s.
      - Uh, killed a pool real fast and nothing broke? This one is hard/impossible to test.
      - Read the newer autoscale message.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12298
    
    Differential Revision: https://secure.phabricator.com/D17560
    epriestley committed Mar 24, 2017
  2. Send EXIT events more consistently from daemons

    Summary:
    Ref T12298. When we `bin/phd restart` daemons, we currently may miss sending EXIT events for daemons which aren't actively running at the time, since the EXIT event only happens when the subprocess exits, not if we tear down a waiting/sleeping/hibernating daemon.
    
    The most common case of this is hibernating daemons. A less common case is daemons which have exited because of an error (usually, taskmasters).
    
    In these cases, the web UI doesn't get cleaned up properly. Instead, send EXIT from any kind of teardown so we knock daemons out of the web UI more often.
    
    Test Plan: Ran `bin/phd restart` a bunch, saw fewer hanging daemons in the web UI at `/daemon/`.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12298
    
    Differential Revision: https://secure.phabricator.com/D17553
    epriestley committed Mar 24, 2017
  3. Don't awaken or scale pools during daemon shutdown

    Summary:
    Ref T12298. Currently, we may attempt to un-hibernate or scale-up daemon pools during shutdown, which is silly and counterproductive.
    
    Instead, just shut down.
    
    Test Plan: Ran `bin/phd restart`, saw stuff shut down more cleanly and consistently.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12298
    
    Differential Revision: https://secure.phabricator.com/D17551
    epriestley committed Mar 24, 2017
Commits on Mar 23, 2017
  1. Clean up overseer modules slightly and provide a throttling support m…

    …ethod
    
    Summary:
    Ref T12298. Make it slightly easier to write overseer modules:
    
      - You don't need to implement shouldReloadDaemons(), since you can now just implement shouldWakePool() and get useful behavior.
      - It looks like most modules want to do checks only every X seconds, where X is at least somewhat module/use-case dependent, so add a helper for that.
    
    Test Plan: Ran `bin/phd debug pull --trace`, saw config check fire properly every 10 seconds after updating it to use `shouldThrottle()` in the next change.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12298
    
    Differential Revision: https://secure.phabricator.com/D17539
    epriestley committed Mar 23, 2017
  2. Allow daemon modules to awake hibernating daemons from slumber

    Summary:
    Ref T12298. For repository and taskmaster daemons, before we let them hibernate, we want to be able to wake them up if a new task comes in or a repository gets an update request.
    
    If we can't do this, the first action (commit, comment, etc) after a lull of activity may take several minutes to actually get processed by the daemons since we have to wait around for them to wake up on their own.
    
    With this mechanism, daemon overseers can periodically run some queries (e.g., are there new tasks?) to see if pools should wake from hibernation.
    
    Test Plan: Wrote a fake module to awaken trigger daemons by rolling dice, ran `bin/phd debug trigger`, saw it reawaken them successfully after random intervals.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12298
    
    Differential Revision: https://secure.phabricator.com/D17538
    epriestley committed Mar 23, 2017
Commits on Mar 18, 2017
  1. Call setsid() unconditionally after daemonizing

    Ref T12355. This was alright locally, but in production the system is not
    thrilled about `posix_isatty()` from the daemonized process:
    
    ```
    ERROR 2: posix_isatty(): expects argument 1 to be a valid stream resource
    ```
    
    After daemonizing we never expect `STDOUT` to be a TTY, so skip the check.
    
    Auditors: chad
    epriestley committed Mar 18, 2017
  2. Call setsid() after the overseer dameonizes

    Ref T12355. See also D15812. I believe we're catching a stray SIGHUP
    because we're still in the process group of the SSH terminal.
    
    Test Plan: Ran `bin/phd debug`, `bin/phd start`, `bin/phd stop` locally.
    
    Auditors: chad
    epriestley committed Mar 18, 2017
  3. Add some additional startup logging to daemons

    Ref T12355. The only reproduction case I have for this issue is in production,
    so try to improve logging in order to hunt it down.
    
    Auditors: chad
    epriestley committed Mar 18, 2017
Commits on Mar 4, 2017
  1. Support timeouts for Phage commands

    Summary: Ref T2794. Occasionally stuff hangs for whatever reason. Provide a crude per-command timeout as a general tool for dealing with this.
    
    Test Plan: Added some sleep and ran with `--timeout`, saw commands get killed and reported as failed.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T2794
    
    Differential Revision: https://secure.phabricator.com/D17463
    epriestley committed Mar 4, 2017
Commits on Mar 3, 2017
  1. Fix an issue with FileFinder finding "." as a directory

    Summary: See D17430. This is buggy and hasn't come up before because we don't usually use the "d" (directory) mode.
    
    Test Plan: Added a unit test and made it pass.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Differential Revision: https://secure.phabricator.com/D17455
    epriestley committed Mar 3, 2017
Commits on Feb 28, 2017
  1. Minor fix presumably identified by PHPStan

    Summary: Address comments from @vrana, see D14512#207302.
    
    Test Plan: Ran unit tests.
    
    Reviewers: epriestley, #blessed_reviewers
    
    Reviewed By: epriestley, #blessed_reviewers
    
    Subscribers: Korvin, Firehed, vrana
    
    Differential Revision: https://secure.phabricator.com/D17435
    joshuaspence committed Feb 28, 2017
  2. Fix an issue where daemons can stick in a pool

    Summary:
    Ref T12331. Root problem is the `unset($daemon)` just unsets a key in a local variable, rather than actually discarding the daemon.
    
    Add some logging to make this more clear and clean up some `%d` vs `%s` stuff.
    
    Test Plan:
      - Ran a pool with `bin/phd debug task --pool N`, let it scale up, let it scale down, used `bin/worker flood` to add more tasks.
      - Before patch, the ghost daemons took up the slots in the pool.
      - After patch, the ghost daemons got removed properly and were replaced with fresh daemons.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12331
    
    Differential Revision: https://secure.phabricator.com/D17433
    epriestley committed Feb 28, 2017
  3. Add more autoscale pool logging to daemons in verbose mode

    Summary:
    Ref T12331. This is intended to make T12331 easier to debug because I'm having difficulty reproducing the issue locally.
    
    Autoscale pools now log, and don't update more than once per second.
    
    Test Plan: Ran `bin/phd debug task --pool 4`, saw pool scaling logs.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12331
    
    Differential Revision: https://secure.phabricator.com/D17432
    epriestley committed Feb 28, 2017
Commits on Feb 25, 2017
Commits on Feb 24, 2017
  1. Allow daemons to "hibernate", reducing pool size to 0 for a time

    Summary:
    Ref T12298. This change is the libphutil changes to support hibernation. The next change makes the Trigger daemon hibernate.
    
    We already support having daemons exit, waiting a little while, then restarting them. We do this when configuration changes, by sending the daemons a signal. They exit, then we restart them a few seconds later.
    
    This allows daemons to send a message to the overseer saying "when I exit, wait X seconds to restart me". Then they can exit and hibernate for a while before the overseer restarts them normally.
    
    Test Plan: See next change.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12298
    
    Differential Revision: https://secure.phabricator.com/D17407
    epriestley committed Feb 24, 2017
Commits on Feb 22, 2017
  1. Add limit (maximum simultaneous commands) and throttle (delay between…

    … commands) to Phage
    
    Summary:
    Ref T12218. Ref T2794. This just provides a couple of general purpose tools to slow Phage down a bit if the speed is getting TOO EXTREME.
    
    In particular, `secure` updates //from itself// so if all 4 nodes get hit simultaneously the deploy doesn't work. It deploys cleanly with `--limit 1`.
    
    A separate change to the `phage remote` workflow in Instances exposes these options as flags:
    
      - `--limit`: Run only this many commands simultaneously.
      - `--throttle`: Wait this long between starting commands (for example, `--throttle 0.25` will only start 4 commands per second).
    
    Test Plan:
      - Ran `phage remote status` on `secure` with various `--throttle` and `--limit` arguments, and without any arguments.
      - Ran `phage remote upgrade --limit 1` on `secure`, got a clean upgrade.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12218, T2794
    
    Differential Revision: https://secure.phabricator.com/D17396
    epriestley committed Feb 22, 2017
  2. Reorganize PhutilDaemon into Overseers, Pools and Daemons in libphutil

    Summary:
    Ref T12298. Ultimate goal is to let daemon pools scale down to 0 daemons when they aren't doing anything. This doesn't do that yet (and attempts to change no behavior).
    
    This reorganizes the daemon code to make this change easier. Currently, some daemons are in pools and some are not. Intead, make everything a pool (the Pull and Trigger daemons just get pools with maximum size 1).
    
    New object hiearchy is:
    
      - Overseer
        - Pool
          - Daemon
    
    ...and those objects each pretty much take care of their own stuff, instead of having daemons reach up into the Overseer to dispatch events back to themselves.
    
    Test Plan:
      - Ran `phd debug`, `phd start`, `phd stop`, `phd status`, etc.
      - Reviewed daemons in web console.
      - Changed config, saw daemons automatically restart.
      - Sent daemons signals.
      - Used `--trace`, activated memory tracing, activated verbose mode.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T12298
    
    Differential Revision: https://secure.phabricator.com/D17389
    epriestley committed Feb 20, 2017
Commits on Feb 19, 2017
  1. In Phage, don't sit in a loop once we've read all messages from an agent

    Summary: Ref T2794. The logic is intended to read "loop until we're done processing messages", but is currently missing a `break;`. This doesn't affect behavior yet, just makes us eat way too much CPU.
    
    Test Plan: Saw `time phage remote --hosts db001-16 status` drop from 99% CPU time to <10%.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T2794
    
    Differential Revision: https://secure.phabricator.com/D17387
    epriestley committed Feb 19, 2017
  2. Record command exit status on Execute objects in Phage

    Summary:
    Ref T2794. With Phacility-specific changes elsewhere, this allows me to add some helpful summary output to the current `phage` UI.
    
    I'm thinking about having a "plan" (a DOM-like document describing which commands to execute where) produce a "report" document, because a command may actually have multiple exit statuses (for example, if it was automatically retried after a failure). But this makes actually using `phage` to do things quite a bit easier for now.
    
    Test Plan: Ran some cluster operations and, with changes elsewhere, got more useful high-level reporting about overall command state.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T2794
    
    Differential Revision: https://secure.phabricator.com/D17386
    epriestley committed Feb 19, 2017
  3. Fix a couple bad fprintf() method calls in Phage

    Summary: Ref T2794. These currently don't do the right thing if you run a command that prints "%" chacters.
    
    Test Plan: Ran some command which outout "%" characters, no more warnings.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T2794
    
    Differential Revision: https://secure.phabricator.com/D17385
    epriestley committed Feb 18, 2017
Commits on Feb 18, 2017
  1. Fix errors found by PHPStan

    Test Plan: None.
    
    Reviewers: #blessed_reviewers, epriestley
    
    Reviewed By: #blessed_reviewers, epriestley
    
    Subscribers: epriestley
    
    Differential Revision: https://secure.phabricator.com/D17375
    vrana committed with vrana Feb 18, 2017
  2. Add PhageActions to libphutil

    Summary:
    Ref T2794. I'm not 100% sold on the design of this and it doesn't interact with any other code yet, but get deployment-related code upstream.
    
    The overall approach I'm taking here is that you build a "plan", which is like a DOM tree of stuff you want to do, then execute the plan.
    
    The only plan we build or execute right now goes like this:
    
      - Phage Plan
        - Launch a local Phage agent
          - Have that agent run a bunch of commands
    
    ...but I think this approach should be flexible enough to accommodate more complicated deployment processes in the future.
    
    Test Plan: Successfully deployed `secure` without typing a bunch of different commands.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T2794
    
    Differential Revision: https://secure.phabricator.com/D17380
    epriestley committed Feb 17, 2017
  3. Make Phage agents stream output continuously

    Summary:
    Ref T2794. Currently, the Phage agent waits until the remote command completes before sending output back to the controller.
    
    Instead, stream output continuously.
    
    Also work around some kind of weirdness with error reporting that I hit but couldn't immediately figure out (maybe PHP7-related?).
    
    Test Plan: Ran `bin/phage remote --hosts ... upgrade`, etc., and saw a steady stream of output across multiple hosts.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T2794
    
    Differential Revision: https://secure.phabricator.com/D17378
    epriestley committed Feb 17, 2017
  4. In PhutilLogFileChannel, don't log empty messages

    Summary:
    Ref T2794. The LogFile channel is used to debug another channel by sending all the messages to a logfile.
    
    We perform empty (zero-length) reads over nonblocking channels in various reasonable situations. Currently, these get written to the logfile as empty lines. However, they are probably never useful.
    
    Instead, only write data if it has nonzero length.
    
    Test Plan: After this change, debugging output from the experimental Phage stuff no longer fills up the protocol logfile with tons of empty messages.
    
    Reviewers: chad
    
    Reviewed By: chad
    
    Maniphest Tasks: T2794
    
    Differential Revision: https://secure.phabricator.com/D17368
    epriestley committed Feb 16, 2017
Commits on Feb 16, 2017
  1. Prevent backtracking on long JSON strings with escape codes

    Summary:
    The JSONLintLexer class uses a series of regular expression rules to verify that
    lexed JSON tokens are formatted correctly.  The string check attempts to express
    that strings are well-formed by matching:
      - Start of token
      - A double-quote
      - A repetition of zero or more of:
        - A two-character escape sequence (i.e. `\n`)
        - A six-character unicode codepoint (i.e. `\u1234`)
        - Any number of printable characters that are not `\` or `"`.
      - A closing quote
    
    Key to this regex is that the three alternatives that make up the core of the
    string are non-overlapping sets.  The PCRE engine which backs PHP, however, does
    not note this, and assumes that it must thus mark each run though this
    alternation as a possible point to backtrack to.  To do so, internally it uses a
    stack frame.  Long strings can thus cause the process to run out of stack space;
    this can be replicated via:
    
        preg_match('{^(?:x|y)*}', str_repeat("x", 10000));
    
    PCRE does not consider this type of stack exhaustion a bug -- see
    https://bugs.exim.org/show_bug.cgi?id=979
    
    Specifically, long JSON strings with many escape codes (each of which
    necessitates a stack frame) can trigger this stack exhaustion in the PCRE
    engine, and thus segfault the PHP process.  This failure mode does not trigger
    on PHP 7.0 and up, which default to using PCRE's JIT mode.
    
    Force the PCRE engine to realize that the alternation cases when parsing the
    JSON string are non-overlapping, and that backtracking across them is never an
    option, by using the non-backtracking `(?>x|y)` capture form, along with the
    non-backtracking repetition symbols `++` and `*+`.  These allow PCRE to use a
    tail-recursive form, thus preventing stack exhaustion.
    
    Test Plan:
    In a repository configured to lint JSON files:
    ```
    perl -le 'print q|{"k":"|.(q|a\\n|x10000).q|"}|' > test.json
    arc lint test.json
    ```
    ...no longer segfaults.
    
    Reviewers: #blessed_reviewers, epriestley
    
    Reviewed By: #blessed_reviewers, epriestley
    
    Subscribers: Korvin, epriestley
    
    Differential Revision: https://secure.phabricator.com/D17363
    alexmv committed Feb 15, 2017
  2. Remove unused class "PhutilConsoleConcatenatedView"

    Summary:
    See D14136. This class is never used anywhere and wouldn't work anyway.
    
    I believe the thing it was intended to do (merging views into a single string) ended up in the base class, `PhutilConsoleView`.
    
    Test Plan:
      - Grepped for `PhutilConsoleConcatenatedView` in libphutil, Arcanist, and Phabricator.
    
    Reviewers: chad, vrana
    
    Reviewed By: vrana
    
    Differential Revision: https://secure.phabricator.com/D17365
    epriestley committed Feb 16, 2017
  3. Fix errors found by PHPStan

    Test Plan:
    Ran `phpstan analyze --autoload-file=src/__phutil_library_init__.php -c phpstan.neon -l 1 src/` with `phpstan.neon` containing:
    
      parameters:
          excludes_analyse:
              - */__tests__/*
    
    Reviewers: #blessed_reviewers, epriestley
    
    Reviewed By: #blessed_reviewers, epriestley
    
    Subscribers: Korvin
    
    Differential Revision: https://secure.phabricator.com/D17364
    vrana committed with vrana Feb 16, 2017