3 ms·

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

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.
    - hn_throwaway_99 2y ago
      > needing two subscribed users in a single test is not that likely. Apparently not.
- jbattle 2y ago
  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
  - Ozzie_osman 2y ago
    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.
  - zeroonetwothree 2y ago
    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?
    - jbattle 2y ago
      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).
    - hn_throwaway_99 2y ago
      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.
      - 2y ago

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.
  - hn_throwaway_99 2y ago
    > needing two subscribed users in a single test is not that likely. Apparently not.
jbattle 2y ago
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
- Ozzie_osman 2y ago
  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.
- zeroonetwothree 2y ago
  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?
  - jbattle 2y ago
    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).
  - hn_throwaway_99 2y ago
    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.
    - 2y ago