4 ms·

In the general case you are right, but in this specific case... this test case should have been covered. Even a rudimentary integration test would have caught t

In the general case you are right, but in this specific case... this test case should have been covered. Even a rudimentary integration test would have caught this bug. It sounds like the user story is: - A user runs a query - The user does not have any queries left in their plan - They are billed. - When the user runs the query again they are not billed a second time. I'm struggling to imagine how the test would have failed to catch this. Maybe it was unit tested but with a database containing just one user? Maybe they got very unlucky and the correct user was randomly chosen?

kortex 5y ago
Yeah, they might have just had unit tests with mocked db, or there wasn't enough combinatorial variety. It's insufficient to test the lack of double billing - you have to specifically induce a race condition, assert the race occurred, and that the interlock prevented double spend. It's tricky. Personally I'd try to engineer around race condition entirely, like some scheme with tokens and pagination. I agree, this sort of thing seems like it should be extra-well covered, but, y'know, move fast and double charge folks.
lucumo 5y ago
You're demonstrating the opposite point perfectly. That test scenario would NOT have catched the issue. The problem wasn't that requesters that should've gotten billed didn't. The problem wasn't even that requesters that shouldn't have gotten billed did.[1] The problem was that clients OTHER than the requester got billed. You can, of course, write test cases that check your entire database for unintended state changes, but I struggle to find that a reasonable amount of effort. Especially since you'd have to do that for all code paths. That will very quickly cost a large multiple of the 73k this bug caused. The adequate monitoring and quick response they did here is probably a very good trade-off. Like it or not, production is ALWAYS your last test. Issues are less costly if you realise that, than if you don't. [1] Though for this specific bug, that scenario would've failed too, and might've triggered an extra look at the code.
- brian_cloutier 5y ago
  > You can, of course, write test cases that check your entire database for unintended state changes, but I struggle to find that a reasonable amount of effort. I think you're overthinking this. I'm not suggesting they should have looked for any unintentional state changes, we both agree that is overkill (until you start doing FP). > Though for this specific bug, that scenario would've failed too, and might've triggered an extra look at the code. Yes, exactly. For this specific bug even the simplest test would have failed, which would have caused someone to take a look at what was happening. You are correct that if the bug had been more complicated, such as causing both the proper user AND an additional random user to be charged, then it's unlikely a reasonable level of testing would have caught it. > The adequate monitoring and quick response they did here is probably a very good trade-off. We agree that there is a trade-off here, and if sacrificing some correctness is what it takes to win you a much higher velocity then they probably made the correct trade off; nobody died as a result of this bug. But... surely you see there are some cheap steps they could have taken which would have caught this bug? Not all bugs, but this specific bug. - Write integration tests for important behaviors, such as charging users! - Make sure those integration tests run in an environment which closely simulates production.

kortex 5y ago

Yeah, they might have just had unit tests with mocked db, or there wasn't enough combinatorial variety. It's insufficient to test the lack of double billing - you have to specifically induce a race condition, assert the race occurred, and that the interlock prevented double spend. It's tricky. Personally I'd try to engineer around race condition entirely, like some scheme with tokens and pagination. I agree, this sort of thing seems like it should be extra-well covered, but, y'know, move fast and double charge folks.

lucumo 5y ago

You're demonstrating the opposite point perfectly. That test scenario would NOT have catched the issue. The problem wasn't that requesters that should've gotten billed didn't. The problem wasn't even that requesters that shouldn't have gotten billed did.[1] The problem was that clients OTHER than the requester got billed. You can, of course, write test cases that check your entire database for unintended state changes, but I struggle to find that a reasonable amount of effort. Especially since you'd have to do that for all code paths. That will very quickly cost a large multiple of the 73k this bug caused. The adequate monitoring and quick response they did here is probably a very good trade-off. Like it or not, production is ALWAYS your last test. Issues are less costly if you realise that, than if you don't. [1] Though for this specific bug, that scenario would've failed too, and might've triggered an extra look at the code.

brian_cloutier 5y ago
> You can, of course, write test cases that check your entire database for unintended state changes, but I struggle to find that a reasonable amount of effort. I think you're overthinking this. I'm not suggesting they should have looked for any unintentional state changes, we both agree that is overkill (until you start doing FP). > Though for this specific bug, that scenario would've failed too, and might've triggered an extra look at the code. Yes, exactly. For this specific bug even the simplest test would have failed, which would have caused someone to take a look at what was happening. You are correct that if the bug had been more complicated, such as causing both the proper user AND an additional random user to be charged, then it's unlikely a reasonable level of testing would have caught it. > The adequate monitoring and quick response they did here is probably a very good trade-off. We agree that there is a trade-off here, and if sacrificing some correctness is what it takes to win you a much higher velocity then they probably made the correct trade off; nobody died as a result of this bug. But... surely you see there are some cheap steps they could have taken which would have caught this bug? Not all bugs, but this specific bug. - Write integration tests for important behaviors, such as charging users! - Make sure those integration tests run in an environment which closely simulates production.