3 ms·

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 man

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.

crooked-v 2y ago
> 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.
- TheRoque 2y ago
  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.
  - zo1 2y ago
    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.
    - sgarland 2y ago
      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.
jackspawn 2y ago
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
- viraptor 2y ago
  Oh definitely. Their ops experience seems very low. But I've found it extremely uncommon to see anything better in smaller projects.
hn_throwaway_99 2y ago
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.
- qeternity 2y ago
  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.
- viraptor 2y ago
  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.
  - hn_throwaway_99 2y ago
    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.
    - viraptor 2y ago
      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.
dragonwriter 2y ago
> 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.
- viraptor 2y ago
  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.
brigadier132 2y ago
> 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?
- viraptor 2y ago
  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.
- zeroonetwothree 2y ago
  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.
  - brigadier132 2y ago
    Of course your tests shouldn't be sensitive to external state. Why would other tests running affect your test?
    - viraptor 2y ago
      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.
      - brigadier132 2y ago
        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.
brown9-2 2y ago
looking at the query logs for the nighttime period should have made the bug fairly obvious
- 8n4vidtmkvmk 2y ago
  They even said they had sentry set up.. they'd notice the duplicate key error immediately.
  - ihumanable 2y ago
    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.
8n4vidtmkvmk 2y ago
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.

crooked-v 2y ago

> 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.

TheRoque 2y ago
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.
- zo1 2y ago
  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.
  - sgarland 2y ago
    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.

jackspawn 2y ago

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

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

hn_throwaway_99 2y ago

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.

qeternity 2y ago
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.
viraptor 2y ago
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.
- hn_throwaway_99 2y ago
  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.
  - viraptor 2y ago
    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.

dragonwriter 2y ago

> 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.

viraptor 2y ago
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.

brigadier132 2y ago

> 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?

viraptor 2y ago
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.
zeroonetwothree 2y ago
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.
- brigadier132 2y ago
  Of course your tests shouldn't be sensitive to external state. Why would other tests running affect your test?
  - viraptor 2y ago
    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.
    - brigadier132 2y ago
      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.

brown9-2 2y ago

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

8n4vidtmkvmk 2y ago
They even said they had sentry set up.. they'd notice the duplicate key error immediately.
- ihumanable 2y ago
  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.

8n4vidtmkvmk 2y ago

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.