Trailing slash on realm URL can happen, and breaks `/events`

[gnprice]

[Original title: "Message list won't update, on iOS on physical device". Better characterized below after some debugging.]

AFAIK this doesn't appear in any actual release we've made -- but it happens on 25.7.121 and 25.8.122 (aka current master), in their respective alpha release builds, on my iPhone 7 running iOS 12.3. So we can't make a release until it's fixed.

It also happens in a debug build on that same device.

It doesn't happen in a debug build on an iPhone XR simulator running iOS 12.4... nor on an iPhone 7 simulator running iOS 12.4.

The symptoms are:

• When looking at a message list, it (usually!) fails to update when something changes -- a message arrives, a reaction, etc.
• When you send a message, it goes through fine... but the "outbox" object, which shows up as a message with a spinner on it, sticks around. It's still there even if you leave the message list and come back.

[gnprice]

Ohhh -- it's that we're just not getting events.

First clue to this was on looking in the debug console, seeing this happening over and over:

{route: "events?queue_id=1563767601%3A50342&last_event_id=-1", params: {…}, httpStatus: 404, json: undefined}

But wait, why was I seeing updates if I navigated out of the message list and then back to it? ... It's because we were doing a new fetch. I'm not entirely sure why that is -- don't we avoid those if we're all caught up? the log shows we had caughtUp of {older: false, newer: true} for the narrow in question, hmm -- but the log shows we were, and that certainly explains getting the updates then.

I thought I'd spotted an update another time, too, while still looking at the message list. Maybe I'd somehow provoked another fetch? Or maybe I'd just confused that with seeing an update after navigating away and back.

Anyway, that certainly explains all the symptoms. Next question: what's in turn causing it.

[gnprice]

Puzzling. Odd facts include:

• It works fine for (my usual account on) chat.zulip.org. Where it doesn't work is a test org on kandra-test.zulipchat.com.
  • Possibly another symptom of Make sure the app doesn't break with the new delivery_email field #3196, somehow? That test org still has that setting enabled.
• I'm not seeing anything corresponding to these requests when I look in the zulipchat.com server logs. There are no 404s at all in /var/log/zulip/tornado.log (which has numerous 200 lines for successful requests of the same type); presumably that means these requests aren't making it to Tornado, but I don't see any sign of them in /var/log/nginx/{access,error}.log either.

[gnprice]

Aha! The error seems to be...

in the case where it's failing, the request is going to a URL like https://example.zulipchat.com//api/v1/events?queue_id=1234567890%3A12345&last_event_id=-1. With a double slash at the start of the path.

I think this naturally happens if the realm value we've saved has a trailing slash.

And, indeed, the Redux log shows that is the case for the realm value on that account on that device; but not for the czo account, nor for the account as I've saved it in the simulators where this didn't reproduce.

Phew. Well, that won't be too hard to resolve, then.

[gnprice]

OK, at this point there are two puzzles, both of which go to the question of how widespread this is.

• How did I manage to have a realm value ending in a slash? In the current code, that should be impossible because -- well, SmartUrlInput (which we use for entering the URL that becomes the realm value) is a bit intricate, but among other things we apply fixRealmUrl to the URL, and that among other things strips a trailing slash.

  Going through the history, that logic was added in commit 507839a, of 2017-05-11. ... Ah, but git describe --contains says it only went out in 1.0.11 -- of 2017-06-28. By then I'd surely have installed the app on my main personal device (having started work at Zulip earlier that month); and, via an iCloud backup/restore, that data was inherited by the iPhone where I'm seeing the issue. OK, that's explained then.

  • Ha, and also the logic only strips a single trailing slash. So by entering two trailing slashes at login, I've just reproduced the issue on my Android phone, too.

• What changed now? This extra slash has presumably been there for over two years, on my successive iPhones, every time I use this test org there. Which... at a minimum I use that device to smoke-test each release, and I might use czo but I'm pretty sure I've often used this test org there. Might not have been today or this week that something changed to trigger this, but surely in the last few months.

In any case, no longer a critical priority, because it's hard to trigger unless you have an install that's over two years old. I might not be the only one, though -- we should do something to clean these up.

[gnprice]

@timabbott Do you know a reason why this might have started failing recently? Summarizing what I found above:

• A request to something like https://example.zulipchat.com//api/v1/events?queue_id=1234567890%3A12345&last_event_id=-1 -- note the double-slash -- returns 404, and doesn't make it to tornado.log at all.

• Other routes (//api/v1/register and everything else) are fine, though; only events is affected.

• I've almost surely been making these double-slash requests every time I use my test org, on my physical iPhone, for the past two years. I'm not certain just how recently I would have noticed if it broke (it's possible the last few times I've tested things on my iPhone have all been on czo instead of that test org)... but I'd be surprised if it was already broken by a few months ago.

Some change in our nginx and/or load-balancer setup, perhaps?

[gnprice]

Things I expect we'll want to do to clean this up:

• Tweak fixRealmUrl so it strips any number of trailing slashes.
• Add a migration that strips trailing slashes on realm in all existing accounts.

[timabbott]

Woah, that's weird. I am not aware of server-side changes that might have caused this; git log -p zproject/urls.py doesn't show any recent relevant changes. If it were affecting all routes, I would have speculated that this could be a change in upstream Django or Tornado that we took; here's what our code looks like:

    # used to register for an event queue in tornado                                                 
    url(r'^register$', rest_dispatch,                                                                
        {'POST': 'zerver.views.events_register.events_register_backend'}),                           
                                                                                                     
    # events -> zerver.tornado.views                                                                 
    url(r'^events$', rest_dispatch,                                                                  
        {'GET': 'zerver.tornado.views.get_events',                                                   
         'DELETE': 'zerver.tornado.views.cleanup_event_queue'}),                                     

So those should behave the same. A Tornado upgrade might be the source; zerver/tornado/application.py defines the Tornado URLs, and /register isn't one of them. Not much to test with that theory, since only /events is user-facing.