Hacker News Reader

Comment by viraptor | Hacker News Reader

They spent 5 days. The bug type is pretty common and could easily be done by a developer. (It's a similar class to the singleton default argument issue that many people complain about) Meh, I don't mind the cautionary tale and don't think chatgpt was even relevant.

It's actually a tricky bug, because usual tests wouldn't catch it (db wiped for good isolation) and many ways of manual testing would restart the service (and reset the value) on any change and prevent you from seeing it. Ideally there would be a lint that catches this situation for you.

View on HN

TBH, while I definitely could see this being an easy bug to write, something is definitely wrong if it took 5 days to identify the root cause of this bug.

That is, I'm struggling to understand how a dive into the logs wouldn't show that all of these inserts were failing with duplicate key constraint violations. At that point at least I'd think you'd be able to narrow down the bug to a problem with key generation, at which point you're 90% of the way there.

I also don't agree that "usual tests wouldn't catch it (db wiped for good isolation)". I'd think that you'd have at least one test case that inserted multiple users within that single test.

Yeah, we have quite literally caught bugs like this in 5 minutes in prod not bc we made a mistake, but bc a customer’s internal API made a schema change without telling us and our database constraints and logging protected us.

But it took about 5 minutes, and 4 of those minutes were waiting for Kibana to load.

The bug was in multiple subscriptions not just users. And I can't think of one non-contrived reason to do it. Even when testing the visibility/access of subscriptions between users you need 2 users, but only one subscription.

What? This doesn't make any sense:

1. First, if you look at the code they posted, they had the same bug on line 45 where they create new Stripe customers.

2. The issue is not multiple subscriptions per user (again, if you look at the code, you'll see each Subscription has one foreign key user_id column). The problem is if you had multiple subscriptions (each from different users) created from the same backend instance then they'd get the same PK.

Not every user needs a stripe customer. I'm creating the stripe entries only on subscription in my app.

Your second point is true, but I don't see what it changes. Most automated unit/integration testing would just wipe the database between tests and needing two subscribed users in a single test is not that likely.

> needing two subscribed users in a single test is not that likely.

Apparently not.

create a subscription for a test user. delete it. Make sure you can create another subscription for the same user.

create subscriptions with and without overlapping effective windows

Those seem like very basic tests that would have highlighted the underlying issue

Or add some debug logging? 5 days into a revenue-block bug, if I can't repro manually or via tests, I would have logged the hell out of this code. No code path or metric would be spared.

Hindsight is 20/20. It’s always easy to think of tests that would have caught the issue. Can you think of tests that will catch the next issue though?

I'd argue that a suite of tests that exercise all reasonably likely scenarios is table stakes. And would have caught this particular bug.

I'm not talking about 100% branch coverage, but 100% coverage of all happy paths and all unhappy paths that a user might reasonably bump into.

OK maybe not 100% of the scenarios the entire system expresses, but pretty darn close for the business critical flows (signups, orders, checkouts, whatever).

Sure, hindsight is 20/20, but a bunch of these comments are replying to the assertion "And I can't think of one non-contrived reason to do it" (have a single test case with multiple subscriptions). That's the assertion I think is totally weird - I can think of tons of non-contrived reasons to have multiple subscriptions in a single test case.

I wouldn't pillory someone if they left out a test case like this, but neither would I assert that a test case like this is for some reason unthinkable or some outlandish edge case.

Go on then - so far the examples I've seen don't make sense in the context of stripe.

Have a look at stripe API. You don't delete subscriptions. You change them to free plan instead / cancel and then resume later. This for would not result in deletion of the entry. You also can update to change the billing date, so no overlapping subscriptions are needed. Neither test would result in the described bug.

agree, but this sounds like it would produce logs/error messages which could then lead to a solution, quicker... if the logs were captured and propagated sufficiently

Oh definitely. Their ops experience seems very low. But I've found it extremely uncommon to see anything better in smaller projects.

looking at the query logs for the nighttime period should have made the bug fairly obvious

They even said they had sentry set up.. they'd notice the duplicate key error immediately.

I read the part where they said they poured through "hundreds of sentry logs" and immediately was like "no you didn't."

This is not an error that would be difficult to spot in an error aggregator, it would throw some sort of constraint error with a reasonable error message.

> db wiped for good isolation

Why? In fact, not having good isolation would have caught this bug. Generate random emails for each test. Why would you test on a completely new db as if that is what will happen in the real world?

It's extremely common. You want to know that objects/rows from one test don't bleed into another by accident. It allows you to write more strict and simpler assetions - like "there's one active user in the database" after a few changes, rather than "this specific user is in the database and active and those other ones are not anymore".

Leaking information between test runs can actually make things pass by accident.

It makes your tests more robust. Generally you don’t want tests that are too sensitive to external state since they will fail spuriously and become useless.

Of course your tests shouldn't be sensitive to external state. Why would other tests running affect your test?

They shouldn't. But they do. We're not perfectly spherical developers and we all make mistakes. Sometimes it's also extremely tricky to figure out what state is leaking, especially is it's an access race issue and happens only for some tests and very rarely. If you haven't seen that happening, you just need to work on larger projects.

I've worked at several large companies. In our e2e tests we did not create isolated dbs per test. If the test failed because of other tests running that's a bug in the test and that person would get automatically @mentioned on slack and they would have to fix the build.

> It's actually a tricky bug, because usual tests wouldn't catch it (db wiped for good isolation)

Volume/load testing (or really, any decent acceptance testing) would catch it.

Load testing - yes, but it's not that usual unfortunately. (Even though it should be) Acceptance testing - again, maybe, if they use 20 or so subscriptions in one batch, which may not be the case.

A functioning dev env would have caught this if they manually tested more than once. Typically you dont run 40 dev instances. Or a staging environment.

> and could easily be done by a developer

...who didn't know how the ORM they were using worked. That's what makes them look so bad here: nobody knew how it worked, not even at the surface level of knowing what the SQL actually generated by the tool looks like.

In their defense, I find SQLAlchemy syntax quite horrible, and I always have to look up everything. It also got a 2.0 release recently which changes some syntax (good luck guessing which version ChatGPT will use), and makes the process even more annoying.

SQLAlchemy syntax is ridiculously obvious and straightforward as long as you're not doing anything weird.

The takeaway here is that they weren't mature enough to realize they were, in fact, doing something "weird". I.e. Using UUIDs for PKs, because hey "Netflix does it so we have to too! Oh and we need an engineering blog to advertise it".

Edit. More clarity about why the UUID is my point of blame: If they had used a surrogate and sequential Integer PK for their tables, they would never have to tell SQLAlchemy what the default would be, it's implied because it's the standard and non-weird behavior that doesn't include a footgun.

Unfortunately, UUID as PK is an extremely common pattern these days, because devs love to believe that they’ll need a distributed DB, and also that you can’t possibly use integers with such a setup. The former is rarely true, the latter is blatantly false.

Reply on news.ycombinator.com