Honestly I'm not sure why ChatGPT has anything to do with this problem.
I remember making the exact same mistake (accidentally using a single function call in a schema) back in 2010. No LLMs required.
The bigger culprit is probably a lack of testing / debugging. This error would immediately get caught if you simply registered twice on a test instance.
In a world where this entire codebase wasn't generated by ChatGPT, you'd have engineers familiar with the various parts of the system to quickly identify and fix the problem.
Testing and debugging isn't just a matter of stepping through code, it's an exercise of seeing where your mental model of the codebase is faulty versus the current reality of it.
I've encountered similar problems and they'd be fixed in a matter of hours, not days.
This is spot on, the issue is not the mistake per se but in creating a code base that the team themselves are not familiar with. With some intern or team member generated code you can sit down with them and have them walk you through the code and introduce you to their reasoning, but you can't do that with an LLM. The author even admits to just mimicking the existing structure that the LLM started with when they had to expand, which sounds like a first commit for a team member first getting familiar with some new code.
Part of the benefit of being able to write your own code is that you can do it in a way that clicks for you. Hopefully this lets you debug and extend it efficiently. I don't know why someone would squander this opportunity.
> In a world where this entire codebase wasn't generated by ChatGPT, you'd have engineers familiar with the various parts of the system to quickly identify and fix the problem.
I wonder why this victim didn't ask ChatGPT to identify and fix the problem...
> Honestly I'm not sure why ChatGPT has anything to do with this problem.
Because if humans had written and reviewed the code, multiple team members would have had to have learned Python, and SQLAlchemy specifically, which, even if the mistake was initially made as many times as ChatGPT did, there would have been multiple independent opportunities for it to be caught and questioned and the relevant knowledge shared during development.
ChatGPT may be able to crank out immense volumes of superficially functional code, but if its your only “teammate” that understands the libraries used and touches the code, its a huge single point of failure.
> This error would immediately get caught if you simply registered twice on a test instance
Friendly reminder: check if your codebase is actually testing this!
One of the interesting consequence of running unit tests with a fresh database everytime is that problems related to unique constraints seldom get caught by unit tests.
> Honestly I'm not sure why ChatGPT has anything to do with this problem.
I remember making the exact same mistake (accidentally using a single function call in a schema) back in 2010. No LLMs required.
If you're treating ChatGPT code differently than your interns' code, you're going to miss some serious issues regardless of which one isn't under a spotlight.
I had to fix some intern code once, and… well, I can't give too many details, but I will say that an FAQ shouldn't consist entirely of quotations from a TV show from a different country in a language the app doesn't support.
i can see missing this during testing since it relies on doing the flow twice. though i have a hard time imagining a team of people not figuring this out in less than 5 days
Their take-away is that when people tell them ChatGPT produces excellent code you have a nice example to the contrary. The business had many failures, not the least of which is that `default = ... foo() ...` should have jumped out in a cursory glance at the code as needing further inspection, almost no matter which language the developers in question are most comfortable with.
There is a comment higher up in this thread [1] about how "ChatGPT 4-o can spot the error immediately" which I believe exemplifies the point the parent is making, namely, that any criticism of ChatGPT is met with "that's only because you didn't use enough of it", aka the "more cowbell" defense.
If HN is to be believed you should use ChatGPT to generate your data insertion code, to port it to a different language, and to ask for which error it made when you asked before. What to do if an error slips through in this version is always an exercise left for the reader.
I'm not familiar with this library, how does `text("(now())")` evaluate and why there are extra parentheses? And should that be a lambda expression as well, so that `create_date` isn't just the timestamp when the python process was started?
IIRC, the difference is server_default vs default. One is generated DB-side, the other in the Python code. Might be wrong on that, but that's my recollection
I’ll note that the bug is in the `id` column, but the `created_date` is likely passing the string “now()” to invoke SQL’s NOW(), deferring the timestamp creation to the database.
I'm not familiar with the library either, but that seems to be a SQL expression executed on the database server. It's basically a copy-paste from the official documentation[0]. So no, not a lambda expression, because it's not computed in Python.
As to the extra parentheses: I bet that's a force-of-habit thing to prevent potential issues. For example, it seems Sqlite requires them for exactly this kind of default definition[1]. It could also read to nasty bugs when the lack of parentheses in the resulting SQL could result in a different parse than expected[2]. Adding them just-to-be-safe isn't the worst thing to do.
"Note: I want to preface this by saying yes the practices here are bad and could have been avoided. This was from a different time under large time constraints. Please read with that in mind"
These "constraints" are why I'm terrified of subscribing to software
I have immense respect for the OP for writing up the story, and even more so for giving this preface. It's really useful to know what mistakes other people make, but can be quite embarrassing to tell others about mistakes you've made. Thanks, OP.
A local cinema's shitty website charged me and, immediately after, my wifi disconnected for a few seconds. This somehow crashed their server entirely for several minutes, failing to send me the tickets even when it got back up.
It took me several threatening emails to make them understand they had already taken my money and I wasn't going to try again until I got a refund. Now I'm paranoid any time I purchase online at mediocre shops.
out of curiosity, what's the technical solution? My guess: add an idempotency key to the request and a message queue? Then when you try to consume it, you check whether that request was made previously.
I tried subway online ordering once almost a year ago. The last final screen crashed and deleted the entire order from my cart. Anyways, you're probably wondering if the order actually went through but subway's UI was shit and nearly tricked me into replacing the same order a second time: yes.
On one hand, thanks for being honest about a story of how this bug came to be.
On the other hand, I don’t think advertising the fact that the company introduced a major bug from copy and pasting ChatGPT code around and that they spent a week being unable to even debug why it was failing.
I don’t know much about this startup, but this blog post had the opposite effect of all of the other high quality post-mortem posts I’ve read lately. Normally the goal of these posts is to demonstrate your team’s rigor and engineering skills, not reveal that ChatGPT is writing your code and your engineers can’t/won’t debug it for a very long time despite knowing that it’s costing them signups.
It read like no one really knew what they were doing. "We just let it generate the code and everything seemed to work" is certainly not a good way to market your company.
> It read like no one really knew what they were doing. "We just let [devs] generate the code and everything seemed to work" is certainly not a good way to [whatever].
Except, have you met startup devs? This is by and large the "move fast then unbreak things" approach.
It’s the natural outcome of SV types denigrating the value of education.
Forget knowing anything, just come up with a nice pitch deck and let the LLM write the stack.
Not wholly surprised these people are YC backed. I’ve got the impression YC don’t place much weight on technical competence, assuming you can just hire the talent if you know how to sell.
Well, now replace “hire some talent” with “get a GPT subscription and YOLO”, and you get the foundation these companies of tomorrow are going to be built on.
Which hey, maybe that’s right and they know something I don’t.
For that matter, has OP even met the HN accepted wisdom? "No one knows what they're doing, everyone's faking it, it's fine if you are too" -- so don't take it as a red flag when your fumbling around keeps blowing up, because it surely must work that way everywhere else.
Its very humbling coming out of startup-land and working with big tech engineers and realizing their tooling runs circles around everybody else and enables them to be much more precise with their work and scale, though it isn't without trade-offs.
Yeah but a lot of that is just the accrual of improvements that is possible with a lot of resources over a long period of time.
People working in "big tech" aren't fundamentally better at building reliable tools and systems; the time and resource constraints are entirely different.
The big tech tooling probably cost tens of millions of dollars to create, and probably had a couple $10k mistakes on the way to getting it written and running.
It’s possible to move fast the same way, but break less things than this. For example, in this case, they said that they introduced tests to mitigate this. I can assure you that introducing tests takes more time than Google searches to check in like 2 minutes what each lines really does.
The idea of moving fast is to have extensive logs and alerts so you fix all error fasts while they appear without "wasting time" with long expensive tests in a phase where things change every day.
5 days to find out you have "duplicate key" errors in the db is the opposite of fast
Eh I imagine they looked over the code as well, doing code review -- and at first glance, the code looks reasonable. I certainly wasn't able to catch the bug even though I tried to find it (and I was given a tiny collection of lines and the knowledge that there's a bug there!).
If anything, I think this says something about how dangerous ChatGPT and similar tools are: reading code is harder than writing code, and when you use ChatGPT, your role stops being that of a programmer and becomes that of a code reviewer. Worse, LLMs are excellent at producing plausible output (I mean that's literally all they do), which means the bugs will look like plausibly correct code as well.
I don't think this is indicative of people who don't know what they're doing. I think this is indicative of people using "AI" tools to help with programming at all.
> I don't think this is indicative of people who don't know what they're doing. I think this is indicative of people using "AI" tools to help with programming at all.
I think using AI tools to write production code is probably indicative of people who don't really know what they are doing.
The best way not to have subtle bugs is to think deeply about your code, not subcontract it out -- whether that is to people far away who both cannot afford to think as deeply about your code and aren't as invested in it, or to an AI that is often right and doesn't know the difference between correct and incorrect.
It's just a profound abrogation of good development principles to behave this way. And where is the benefit in doing this repeatedly? You're just going to end up with a codebase nobody really owns on a cognitive level.
At least when you look at a StackOverflow answer you see the discussion around it from other real people offering critiques!
ETA in advance: and yes, I understand all the comparison points about using third party libraries, and all the left-pad stuff (don't get me started on NPM). But the point stands: the best way not to have bugs is to own your code. To my mind, anyone who is using ChatGPT in this way -- to write whole pieces of business logic, not just to get inspiration -- is failing at their one job. If it's to be yours, it has to come from the brain of someone who is yours too. This is an embarrassing and damaging admission and there is no way around it.
ETA (2): code review, as a practice, only works when you and the people who wrote the code have a shared understanding of the context and the goal of the code and are roughly equally invested in getting code through review. Because all the niche cases are illuminated by those discussions and avoided in advance. The less time you've spent on this preamble, the less effective the code review will be. It's a matter of trust and culture as much as it's a matter of comparing requirements with finished code.
Notwithstanding the fact that compilers did not fall out of the sky and very much have people that own them at the cognitive level, I think this is still a different situation.
With a compiler you can expect a more or less one to one translation between source code and the operation of the resulting binary with some optimizations. When some compiler optimization causes undesired behavior, this too is a very difficult problem to solve.
Intentionally 10xing this type of problem by introducing a fuzzy translation between human language and source code then 1000xing it by repeating it all over the codebase just seems like a bad decision.
Right. I mean... I sometimes think that Webpack is a malign, inscrutable intelligence! :-)
But at least it's supposed to be deterministic. And there's a chance someone else will be able to explain the inner workings in a way I can repeatably test.
Yes, and when compilers fail, it's a very complex problem to solve, that usually requires many hours from experienced dev. Luckily,
(1) Compilers are reproducible (or at least repeatable), so you can share your problem with other, and they can help.
(2) For common languages, there are multiple compilers and multiple optimization options, which (and that's _very important_) produce identically-behaving programs - so you can try compiling same program with different settings, and if they differ, you know compiler is bad.
(3) The compilers are very reliable, and bugs when compiler succeeds, but generates invalid code are even rarer - in many years of my career, I've only seen a handful of them.
Compare to LLMs, which are non-reproducible, each one is giving a different answer (and that's by design) and finally have huge appear-to-succeed-but-produce-bad-output error rate, with value way more than 1%. If you had a compiler that bad, you'd throw it away in disgust and write in assembly language.
I totally disagree with this. You might as well argue that we shouldn't use code-completion of any kind because you might accidentally pick the wrong dependency or import. Or perhaps we shouldn't use any third-party libraries at all because you can use them to write reasonable-looking but incorrect code? Heck, why even bother using a programming language at all since we don't "own" how it's interpreted or compiled? Ultimately I agree that using third-party tools saves time at the cost of potentially introducing some types of bugs. (Note that said tools may also help you avoid other types of bugs!) But it's clearly a tradeoff (and one where we've collectively disagreed with you the vast, vast majority of the time) and boiling that down to AI=bad misses the forest for the trees.
This is an error that should probably have been caught just based upon the color of the text when it was typed/pasted into the source code. Of the uuid() call was in quotes, it would have appeared as text. When you’re blindly using so much copy/pasted code (regardless of the source), it’s really easy to miss errors like this.
But our existing tools are already built to help us avoid this.
Back in the day, I used a tool from a group in Google called “error-prone”. It was great at catching things like this (and Lorne goal NPE in Java). It would examine code before compiling to find common errors like this. I wish we had more “quick” check tools for more languages.
> This is an error that should probably have been caught just based upon the color of the text when it was typed/pasted into the source code. Of the uuid() call was in quotes, it would have appeared as text.
It's not in quotes. It's a function call.
The issue is that the function call happens once, when you define the class, rather than happening each time you instantiate the class.
I'm a some-time Django developer and... I caught the bug instantly. Once I saw it was model/ORM code it was the first thing I looked for.
I say that not to brag because (a) default args is a known python footgun area already and (b) I'd hope most developers with any real Django or SQLAlchemy experience would have caught this pretty quick. I guess I'm just suggesting that maybe domain experience is actually worth something?
I've since moved on to primarily working with Java, so it's been a few years since working with Django on a daily basis and the default still jumped out to me immediately. Experience and domain knowledge is so important, especially when you need to evaluate ChatGPT's code for quality and correctness.
Also, where were their tests in the first place? Or am I expecting too much there?
This is why I typically only use LLMs in programming as a semi-intelligent doc delver with requests like, “give me an example of usage of X API with Y language on Z platform”.
Not only does it goof up less frequently on small focused snippets like this, it also requires me to pick the example apart and pay close enough attention to it that goofups don’t slip by as easily and it gets committed to memory more readily than with copypasting or LLM-backed autocomplete.
I don't know Python or SQLAlchemy that great, though I do have the benefit of it being cut down to a small amount of code and being told there was a bug there. That said, I didn't see the actual bug, but I did mentally flag that as something I ought to look up how it actually behaved. It's suspicious that some columns used `default` with what looks like Python code while others used `server_default` with what appears to be strings that look more like database engine code. If I was actually responsible for this, I'd want to dig into why there is that difference and where and when that code actually runs.
It's also the case that "code review" covers a lot of things, from quickly skimming the code and saying eh, it's probably fine, to deeply reading and ensuring that you fully understand the behavior in all possible cases of every line of code. The latter is much more effective, but probably not nearly as common as it ought to be.
> It read like no one really knew what they were doing.
In my experience, hardly anyone in software does know what they're doing, for sufficiently rigorous values of "know what you're doing." We all read about other people's stupid mistakes, and think "haha, I would never have done that, because I know about XYZ!" And then we go off and happily make some equally stupid mistake because we don't know about ABC.
An awful lot of mistakes are made because one didn't know something that would have enabled one to avoid it. Not knowing what you don't know is difficult to work around.
I dunno. I tend to annoy people when taking on jobs by telling people what I am concerned about and do not understand, and then sharing with them the extent to which I have managed to allay my own concerns through research.
I turn down a lot of jobs I don't feel confident with; maybe more than I should.
This is getting more common. I have already had people try to tell me how something works from a chat gpt summary. This would have led to us taking a completely different direction… 5 minutes of reading the actual docs and I found out they were wrong.
Now at a new company i have caught several people copy pasting gpt code that is just horrendous.
It seems like this is where the industry is headed. The only thing i have found gpt to be good at is solving interview questions although it still uses phantom functions about 50% of the time. The future is bumming me out.
Like people who post "here's what ChatGPTx said" instead of their own answer. Quite literally, what is the point?
However, I don't think it's really bad for the technical industries long term. It probably does mean that some companies with loose internal quality control and enough shiftless employees pasting enough GPT spew without oversight will go to the wall because their software became unmaintainable and not useful, but this already happens. It's probably not hugely worse than the flood of bootcamp victims who wrote fizzbuzz in Python, get recruited by a credulous, cheap or desperate company and proceed to trash the place if not adequately supervised. If you can't detect bad work being committed, that's mostly on the company, not ChatGPT. Yes, it may make it harder, a bit, but it was oversight you should already have been prepared to have, doubly so if you can't trust employee output. It also probably implies strong QA, which is already a prerequisite of a solid product company.
Normal interest rates coming back will cut away companies that waste everyone's time and money by overloading themselves on endlessly compounding technical debt.
No, the idea is that historically-normal interest rates around the 5-10% mark won't be conducive to free VC cash being sprayed around for start-ups to wank themselves silly over "piv-iterating" endlessly over spamming complete nonsense and using headcount and office shininess as a substitute for useful and robust products.
Yes, it makes the barrier higher even for good products and helps entrench incumbents, but short of a transnational revolution, the macroeconomic system is what is it and you can only chose to find the good things in it or give up entirely.
I don't really care about them marketing their company, but, Jesus, seriously, that's how software is going to be written now. TBH, I'm not really sure if it's that much different from how it was, but it sounds just... fabulous.
> It read like no one really knew what they were doing.
Then no one knows what they are doing. I really don't know any company that doesn't make what could be considered rookie mistakes by some armchair "developer" here on HN.
In a way, it does give you an opportunity to think about what you appreciate in a detailed postmortem - not just a single cause, but human and organizational factors too, and an attempt to figure out explicit mitigations. I’ll admit the informality and the breezy tone here made me go “woah, they’re a bit cavalier…”
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.
...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.
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
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
More importantly, what was the motivation behind a rewrite from TypeScript to Python? From the article
Our project was originally full stack NextJS but we wanted to first migrate everything to Python/FastAPI.
Seems like this entire mess could've been avoided if they had stuck with their existing codebase, which seemed to have been satisfying their business requirements.
Simply put - if you want get the best out of the framework, you need to host it in Vercel. Otherwise, there are better options for frameworks. No need to ”fight it”.
You will find many issues from GitHub which are not considered because they would make the framework ”better” or easier to use on other clouds.
Making things worse in a free offering by a company to profit from premium offerings by the same company its the pinacle of capitalism, reminds me of a recurrent joke I have with a friend while playing Call Of Duty, that they will get greedier and soon will sell not only character's skins but also shaders/textures for the maps, oh so you want to see something better than placeholder textures? We have the DLC just for you!
Madden has a monopoly license for NFL content. For a decade the biggest complaint was how they gate kept rosters behind the yearly re-release. Eventually they allowed roster sharing but they put it behind the most god awful inept UI you could possibly imagine such that practically casual gamers wouldn't bother with it.
Then Madden came out with Madden Ultimate Team (like trading cards MTX) and have been neglecting non-MUT modes ever since. They don't explicitly regress the rest of their game, they just commit resources to that effect.
Its like malicious compliance. They don't embrace, extend, extinguish, but they get a similar effect just with resourcing, layoffs, whatever.
There is no explicit vendor lock-in, but features of the framework are designed heavily towards Vercel-specific features.
The SST team actually has an open-next project[1] that does a ton of work to shim most of the Vercel-specific features into an AWS deployment. I don't think it has full parity and it's a third party adapter from a competing host. The fact that it's needed at all is a sign of now closely tied Next and Vercel are.
Implementing the requested feature would make the framework much better and easier to use when self-hosted elsewhere. But there is neglection to resolve the issue. This is just one case.
My guess, when I read it, was this would permit them
to independently scale the backend on some commodity capacity provider, and then their Nextjs frontend becomes just another React app. OP didn’t mention what their product was, but if it’s AI-adjacent then a python backend doesn’t sound like a terrible idea.
Or if you really want to rewrite your back end, why not just use Express? It would be wildly quicker to rewrite than switching languages. That along with the article makes me question the competency of the company. They got customers, sure, but in the long run these decisions will pile up.
- They were under large time constraints, but decided a full rewrite to a completely different stack was a good idea.
- They copy-pasted a whole bunch of code, tested it manually once locally, once in production, and called it a day.
- The debugging procedure for this issue so significant it made them dread waking up involved... testing it once and moving on. Every day.
The bug is pretty easy to miss, but should also be trivial to diagnose the moment you look at the error message, and trivial to reproduce if you just try more than once.
Ironically one of my frequent GPT questions is “X is supposed to do Y, but is doing Z. What logs should I look at and what error messages to keep an eye out for?”
I'd rather they admit a mistake and learn a lesson from it even if it isn't a good thing to advertise. That said, I agree that you are identifying a more important issue here but I also think you are being a bit too subtle about even if I agree with what you are saying. The real lesson that they should have learned from this ordeal is to never push code directly into production --- period. The article never mentions using a testbed or sandbox beforehand, and I kinda feel like they learned a good lesson but it may in fact be the wrong lesson to learn here.
I don't see how testbed/sandbox would have helped, unless they'd also have a dedicated QA person _and_ configured their sandbox so have dramatically fewer instances.
Because I can see "create a new subscription" in the manual test plan, but not "create 5x new subscription".
> configured their sandbox so have dramatically fewer instances
May be a wise thing to do anyway. I would even advocate for the extreme, one instance in sandbox. I've seen quite a variety of bugs that would be easier to detect and debug with one instance — from values accidentally persisted between requests (OP is one example of this very broad class of bugs), to a thundering herd bug catched in staging due to less instances and more worker threads per instance. But can't remember even one "distributed" bug caught in staging.
> they'd also have a dedicated QA person
This is a new feature, and it's user-facing, business-critical and inherently fragile (external service integration, though it broke for another reason). I hope multiple people manually run such things end to end before deploying to prod, even if none of them are dedicated QA?
> trivial to reproduce if you just try more than once
A lot more than once: they had 40 instances of their app, and the bug was only triggered by getting two requests on the same instance.
A bunch of developers including me once spent a whole weekend trying to reproduce a bug that was affecting production and/or guess from the logs where to look for it. Monday morning, team lead called a meeting, asked for everything we could find out, and… Opened the app in six tabs simultaneously and pressed the button in question in one of the tabs. And it froze! Knowing how to reproduce on our computers, we found and fixed the bug in the next 30 minutes.
The fact that they couldn't find it by looking at error logs is weird to me.
This is an entirely forgivable error but should have been found the first time they got an email about it:
"Oh, look, the error logs have a duplicate key exception for the primary key, how do we generate primary keys.... (facepalm)"
Funnily enough, I saw the error in their snippet as soon as I read it but dismissed it thinking there was some new-fangled python feature which allowed that to work like the function def defines that default= accepts only functions so the function gets passed? -- I haven't kept up with modern python and that sounds cool and I figured the bug couldn't be THAT simple.
Guess: the logs were on an ec2 instance that was thrown away regularly, and the overnight reports didn't give reproduce steps or timestamps; so when they checked it "works fine".
There's value in having your backtrace surfaced to end users rather than swallowing an exception and displaying "didn't work".
it was on some temporary AWS service like lambda or something? (We had eight ECS tasks on AWS, all running five instances of our backend), but, regardless logs should be somewhere persistent.
If they weren't, that should be the first thing you fix.
This is a pretty common mistake with sqlalchemy whether you’re using ChatGPT or not. I learned the same lesson years ago, although I caught it while testing. I write plenty of python and I just don’t often pass functions in as parameters. In this case you need to!
For something like this where you’re generating a unique id and probably need it in every model, it’s better to write a new Base model that includes things like your unique id, created/changed at timestamps, etc. and then subclass it for every model. Basically, write your model boilerplate once and inherit it. This way you can only fuck this up once and you’re bound to catch it early before you make this mistake in a later addition like subscription management.
Yea, more of an issue of how this python library can cause misunderstandings, and ChatGPT failing in the same misunderstanding that would have been made by an engineer who lacks experience in that library.
This mistake would have happened even if they did not use ChatGPT.
I wonder are there linters to detect those types of mistakes for SQLAlchemy. Even though I'm aware of such pitfalls, it's nice if linters can catch them cause I'm not confident to cache them all the time during code review.
Some linters like pyright can identify dangerous defaults in function call, like `def hello(x=[]): pass` (mutable values shouldn't be a default). Linter plugins for widely-used and critical libraries like SQLAlchemy are nice to have.
The mutable-default-arguments issue is easy for a third-party to linter to catch because it doesn't require any specific knowledge about primary keys and databases. Are there static typing plugins for other common packages that would catch issues like this?
Ah, but it’s not a characteristic of SQLAlchemy tho. It’s how Python evaluates statements. Both Peewee and the Django ORM work on the same principle with default values.
The intent is to pass a callable, not to call a function and populate an argument with what it returns.
Correct, it's not specific to sqlalchemy - I'm just saying I notice this a lot with sqlalchemy. Probably because it was the first significant bug I had to figure out how to fix when I introduced it in one of my first apps. I guess we never forget the first time we shot ourselves in the foot.
Honestly same can be said about a lot of frameworks. You will pry my vanilla JS debugged with print statements hand-coded in vi from these hands only when they're cold and dead.
Yeah, I have a workflow where I inject JavaScript into arbitrary webpages and store the results. There’s no substitute for vanilla js knowledge when you’re in the weeds.
The bad thing is what they did, not that the disclosed it.
I agree that this is probably to their disadvantage, but I would much rather have people admitting their faults than hiding them. If everyone did this the world would be better.
Of course the best solution is to not have faults but that is like saying that the solution to being poor is to have lots of money. It's much easier to say than do.
The bad thing is their engineering culture and not anything technical. We all make mistakes, the question is how we fix them. Look a the last sentences of the post:
> Yes we should have done more testing. Yes we shouldn't have copy pasted code. Yes we shouldn't have pushed directly to main. Regardless, I don't regret the experience.
None of those are unconditionally bad! Every project I've worked on could use more testing; we all copy-pasted code at least occasionally, and pushing to main is fine in some circumstances.
The real problem is that they went live, but their tooling (or knowledge how to use it) was so bad it took 5 days to solve the simple issue; and meanwhile, they kept pushing new code ("10-20 commits/day") while their customers were suffering. This is what really causes the reputation hit.
That's an alright takeaway: the team made a rookie mistake and then they made a PR mistake by oversharing.
Otherwise, I think this comment thread is a classic example why company engineering blogs choose to be boring. Better ten articles that have some useful information, than a single article that allows the commentariat to pile on and ruin your reputation.
I think it’s an unfair takeaway. I have over a decade of experience and still had to stare at the line to find the bug. If that makes them incompetent, I stand with them. It’s a bug I’ve seen people make in other contexts, not just chatbots.
The AI angle is probably why people are piling on. There’s a latent fear that AI will take our jobs, and this is a great way to skewer home that we’re still needed. For now.
The one thing I will say is that it probably wouldn’t take me days to track it down. But that’s only because I have lots of experience dealing with bugs the way that The Wolf deals with backs of cars. When you’re trying to run a startup on top of everything else, it can be easy to miss.
I’m happy they gave us a glimpse of early stage growing pains, and I don’t think this was a PR fumble. It shows that lots of people want what they’re making, which is roughly the only thing that matters.
On the one hand it does seem like a fairly inexperienced organization with some pretty undercooked release and testing processes, but on the other hand all that stuff is ultimately fixable. This is a relatively harmless way of learning that lesson. Admitting a problem is the first step toward fixing it.
A culture of ass-covering is much harder to fix, and will definitely get in the way of addressing these types of issues
> Better ten articles that have some useful information, than a single article that allows the commentariat to pile on and ruin your reputation.
Pile-on aside, the problem with this blog article is that it doesn't really have much of a useful takeaway.
They didn't even really talk the offending line in detail. They didn't really talk about what did to fix their engineering pipelines. It was just a story about how they let ChatGPT write some code, the code was buggy, and the bug was hard to spot because they relied on customers e-mailing them about it in a way that only happened when they were sleeping.
It's not really a postmortem, it's a story about fast and loose startup times. Which could be interesting in itself, except it's being presented more as an engineering postmortem blog minus the actionable lessons.
That's why everyone is confused about why this company posted this as a lesson: The lesson is obvious and, frankly, better left as a quiet story for the founders to chuckle about to their friends.
These criticisms about engineering PR are too heavy handed. Great engineers solve problems and describe problems without finger pointing to place blame. In fact I think that the worst engineers I’ve worked with are the ones most often reaching for someone to place it on.
CEO thoughts: "Oh post-morten are always well received, I should write one for that very really basic bug we had and how we took 5 days to find it, and forget to mention how we fix it or how we have changed our structure so that it never happen again"
Also the CEO: "remember to be defensive on reddit comments saying how we are a small 1 million dollar backed startup and how it's normal do to this king of rookies mistake to be fast."
either way it sounds like they're stuck with poor testing around their code. that shows the devs probably don't understand the code which means fixing or changing things in the future will take longer.
In a typical web based app, schema is the one and only thing that you should be paranoid about. Writing it by hand is important for the same reason typing your password to confirm a big transaction is important. The time and thought going into it is worth its weight in gold. I would rather write it with one finger twice over, than giving it to ChatGPT.
I understand how the mistake was made, it seems relatively easy to slip by even when writing code without ChatGPT.
But what I don't understand is how this wasn't caught after the first failure? Does this company not have any logging? Shouldn't the fact the backend is attempting to reuse UUIDs be immediately obvious from observing the error?
They didn’t even know there was an error until the customers came ringing. You always want to know what errors happened before your customers do, logging, alerting, any monitoring at all would have helped them here.
UUIDs are just 128-bit values. They might be conventionally encoded for humans as hex, but storing them as 36-byte (plus a few more for length) strings is a pointless waste of both space and performance.
It is most likely not a performance issues right now, but every pessismisation compound and catch up to you eventually.
36B vs 16B today, tomorrow you need an array of it, and now it isn't cache aligned, and more than twice the overhead.
Most likely instead of manipulating a 36B fixed length string, it is handled as a dynamic string, for extra runtime memory allocations, most likely consuming at least 64B per allocation. Etc etc.
Do this all over the codebase and now you know why all the moderne software is a sloth on what was a supercomputer 30y ago.
Probably not, but this is something that would have taken all of ten seconds to get right. And the size and performance impact is multiplied at every tier of their application.
It’s also not just the size itself. Despite being fixed-size in practice, these are variable-sized strings in application code which now means gajillions of pointless allocations and indirection for everything. There are a ton of knock-on performance consequences here, all on the most heavily-used columns in your data model.
Worst of all should they actually succeed, this is going to be absolutely excruciating to fix.
IFF they’re using MySQL (doubtful), it’s a common mistake due to there not being a native type, and needing to know to use BINARY(16) and casting the string back and forth in code.
But in either case – MySQL or Postgres – they’ve still made the classic mistake of using a UUID as a PK, which will tank performance one way or another. They’ll notice right around when it starts to matter.
it wasn't 5 days of only working on this one problem though, it was over 5 calendar days. even if something is gonna take me one day to implement, it's gonna take three days to get enough focus time between all the other meetings and fires to put out in order to actually get a day's worth of coding done
Yeah, I agree with this here. I think it's totally reasonable that something as specific as multiple Stripe subscriptions wouldn't be exercised by normal unit testing; as mentioned in the post, this wouldn't have been an easy error to reproduce via an acceptance test; and I think the focus on ChatGPT is overblown (by both the OP and everyone else) and mistakenly passing a String instead of a Callable to a function that accepts either happens all the time. My gut instinct is that not using an ORM would have prevented this particular issue, but that may just be my bias against ORMs speaking; one could easily imagine a similar bug occurring in a non-database context. My real conclusion is that all the folks crowing that they would have definitely caught this bug are either much better engineers than I am, or (more likely) are just a bit deluded about their own abilities.
I am also very confused about the apparent lack of logging or recourse to logging. It's been a while, but if I recall correctly ECS should automatically propagate the resulting Duplicate Key exceptions which were presumably occurring to CloudWatch without a bunch of additional configuration - was that not happening? If it was happening, did no one think to go check what types of Exceptions were happening overnight?
I guarantee you that they _will_ have another production bug like this sometime in the future (every fast paced project will). You'd hope this next one wont take 5 days to identify.
The way you want to build and lead an engineering team is so that the individual engineer’s approach to the problem doesn’t matter. The idea is to establish deployment safety and ops mechanisms so Type 2 isn’t even possible and your Sr. Engineers coach the jr. engineers in how to use these mechanisms effectively through training and runbooks. The idea of COE is to figure out what mechanisms (availability tests, metrics/alarms, logs, unit tests) were missing so that you can fill those gaps.
More like how switching to Python (and not meeting Python's high testing requirements) cost you $10k. This is a well known Python foot-gun and a developer could easily have made that mistake too.
agreed, though if a developer had manually made the mistake they might have realized the problem in less than 5 days. copy paste a bunch of ai generated code into your project and no one can try to deduce where the problem might lie once something goes wrong
though a little logging would also have gone a long way. I don't really get how this could have taken 5 days to find, since they knew exactly where the problem was
Agreed. Also Pylint has a lint for exactly this mistake so they didn't even set up the standard linting / static type checking tools which are absolutely a must with Python.
Would "we screwed up by blindly trusting ChatGPT" annoy you less? Because that's how I read it.
Or more specially, given the context: "We were in a rush to translate a bunch of code and ChatGPT was doing such an impressive job helping that we became complacent and forgot that it just parrots back text it has seen before with something that looks like intelligence but without actual comprehension. So when it copied a common bug, we weren't paying enough attention to catch it."
I dont get it , line 56 every time it was invoked would call the uuid4 function to generate a unique id isn’t it? Or was the issue due to the uuid4 function not getting invoked for any reason?
I'm not a Pythonista, but I think the deal is that line 56 was only executed once, at class definition, so every time the server spun up, you got a new uuid that could only use once
Ah so it would get invoked one time for the first user that resulted in a task (container ) initialization on the backed ECS cluster and once all 40 of the tasks were running new users will just reuse one of the running containers and already instantiated object in memory.
The issue is that uuid4() would be called a single each time the app was launched when that code was first loaded. Each record produced by an individual instance of the app would have the same ID.
Since this looks like table definition code it likely would run once per db migration. So in all likelihood it ran once when they created the database tables which itself essentially ended up with a definition like “default=89abcdef-0000-0000-0000-fedcba987654”
Line 56 is executed as the file is loaded. Simplified, the line is essentially:
id = Column(default=str(uuid.uuid4()))
As written, a UUID is generated once and used to set a class-level attribute. Each Python process would generate a unique value, so it wouldn't be immediately obvious. Most of the time Python's ability to run code as a file is loaded is helpful, but this is one the well known gotchas.
Although I'm not a SQL Alchemy user, I assume the fix is essentially the same as it would be for Django. So the correct code would have been essentially:
id = Column(default=uuid.uuid4)
Instead of executing `uuid4()` and caching a single UUID value, it would have executed `uuid4()` each time a new object was created.
Interesting that more people weren't asking for more explanation on this. I feel like the article did a poor job but other commenters explained it well. More commonly I see this with `def f(x=list()):...` or similar. Linters will even warn about it. It really shows a lack of Python knowledge, so I'll jump on the bandwagon of rewriting this with such pressure was a bad idea.
You should generally be setting attributes in the init method of your class, unless you want it to act like a class attribute - shared by all instances. The exception is some special types of classes such as dataclasses or pydantic objects, which do it automatically but require a post init method if you want to do anything more than set values passed to the constructor.
Tbh, if they were writing something for prod they should have factored out the uuid generation and passed values into a pydantic dataclass w/ validators for all the fields. Just sayin.
This is a great example of why SQLAlchemy is a terrible ORM, and chatGPT is not alone in making the same mistake as millions of engineers, in fact likely where it learned the mistake.
default = python code evaluates the default value as necessary for each new record
server_default = the initial CREATE TABLE uses this computed (from python) value, thus the hardcoded UUID. They also could have done server_default=text("uuid_generate_v4()") if they had that corresponding module installed on postgres.
I'm not sure what you think SQLalchemy can fix here? There has to be an option to pass a static default value and the library does not have a visibility into the parse tree. What's the proposed solution?
One solution would be to have a separate default (static value) and default_factory (callable that returns a value) keyword argument, and make a static default on a PK (or other unique) column, by default, be an error.
Yes, because you're creating a table and the value needs to be encoded as string in the CRATE TABLE query. You could in theory pass a lambda which is disassembled in SQLalchemy, then checked against the pattern returning a constant, then the constant itself gets used... But I'm not sure SQLalchemy would go that way. It's not as crazy as Linq.
To be explicit: You're not seeing a function called later in python. You're setting an attribute on a database table.
They could also have done default=uuid.uuid4 to have a new id each time, or default=lambda : str(uuid.uuid4()). It's not really related to whether or not it's a database default.
It seems quite unfair to place the blame on SQLAlchemy here, or even Python.
Even a statically typed language wouldn't prevent this kind of issue - the author of the code is the only person who can decide when they mean "use this exact string each time" or "use this function to give me a new string each time".
I suppose a column description API could follow the dataclass style definition, with different argument names for default and default_func. That would (I think) prevent this from happening.
> This is a great example of why SQLAlchemy is a terrible ORM
I feel like "terrible ORM" is a tautology.
This issue highlighted in one respect a fundamental problem with all ORMs and why I despise them so much - they're the absolute worst of a leaky abstraction, and as I like to say "they make the easiest stuff a bit easier, and they make the harder stuff way harder". E.g. in this specific case they don't prevent you from needing to know the details of default column value initialization, but apparently they must have also obscured a simple duplicate key constraint to some level that it took 5 days to find the root cause of this bug.
Just learn SQL. You'll need to know it anyway even if you do use an ORM, the basics aren't hard, the skill is much more transferable than the esoteric details of some ORM, and there are lots of good libraries that make dealing directly with SQL easy and safe (slonik for postgres is a favorite of mine, but there are other similar ones).
even though it's in high level web stuff, that's a classic heisenbug -- one that appears intermittently due to some hidden process that requires an expert to understand.
That's what I don't get. Five days to query CloudWatch logging? This should have been caught before the first email even came in. "Gee, isn't it strange how we get these spikes in stderr output on our backend last night?"
Dear god, I thought I was taking crazy pills. After saying the same thing (a rewrite this early is insane) I was scanning the comments and no one else was pointing this out. I have no clue what would drive someone to rewrite this early (with or without customers) for what is effectively a lateral move (node to python). If you had hundreds of customers and wanted to rewrite in Go or similar then maybe (I still question even that).
Who needs static analysis when you can put your trust in a magic robot?
(This is one thing that baffles me about the “let’s use LLMs to code” movement; a lot of the proponents don’t seem to be just adding it as a tool (I don’t think it’s a terribly _useful_ tool, but whatever, tastes differ), but using it as the only tool, discarding 50 years worth of progress.)
> This is the eye opener for me, how is a startup justifying a re-write when they don't even have customers?
In my case (with a real project I'm working on now), it'd be due to realizing that C# is a great language and has a good runtime and web frameworks, but at the same time drags down development velocity and has some pain points which just keep mounting, such as needing to create bunches of different DTO objects yet AutoMapper refusing to work with my particular versions of everything and project configuration, as well as both Entity Framework and the JSON serializer/deserializer giving me more trouble than it's worth.
Could the pain points be addressed through gradual work, which oftentimes involves various hacks and deep dives in the docs, as well as upgrading a bunch of packages and rewriting configuration along the way? Sure. But I'm human and the human desire is to grab a metaphorical can of gasoline, burn everything down and make the second system better (of course, it might not actually be better, just have different pain points, while not even doing everything the first system did, nor do it correctly).
Then again, even in my professional career, I get the same feeling whenever I look at any "legacy" or just cumbersome system and it does take an active, persistent effort on my part to not give in to the part of my brain that is screaming for a rewrite. Sometimes rewrites actually go great (or architectural changes, such as introducing containers), more often than not everything goes down in a ball of flames and/or endless amounts of work.
I'm glad that I don't give in, outside of the cases where I know with a high degree of confidence that it would improve things for people, either how the system runs, or the developer experience for others.
You don't need to make DTOs when you don't have to, using AutoMapper is considered a bad practice and is heavily discouraged (if you do have to use a tool like that, there are alternatives like Mapperly which are zero-cost to use and will give you built-time information on what doesn't map without having to run the application).
Hell, most simple applications could do with just a single layer - schema registration in EF Core is mapping, or at most two, one for DB and one for response contracts.
Just do it the simplest way you can. I understand that culture in some companies might be a problem, and it's been historically an issue plaguing .NET, spilling over, originally, from Java enterprise world. But I promise you there are teams which do not do this kind of nonsense.
Things really have improved since .NET Framework days, EF Core productivity wise, while similar in its strong areas, is pretty much an entirely new solution everywhere else.
> You don't need to make DTOs when you don't have to, using AutoMapper is considered a bad practice and is heavily discouraged (if you do have to use a tool like that, there are alternatives like Mapperly which are zero-cost to use and will give you built-time information on what doesn't map without having to run the application).
The thing is, that you'll probably have entities mapped against the database schema with data that must only conditionally be shown to the users. For example, when an admin user requests OrderDetails then you'll most likely want to show all of the fields, but when an external user makes that request, you'll only want to show some of the fields (and not leak that those fields even exist).
DTOs have always felt like the right way to do that, however this also means that for every distinct type of user you might have more than one object per DB table. Furthermore, if you generate the EF entity mappings from the schema (say, if you handle migrations with a separate tool that has SQL scripts in it), then you won't make separate entities for the same table either. Ergo, it must be handled downstream somewhere.
Plus, sometimes you can't return the EF entities for serialization into JSON anyways, since you might need to introduce some additional parsing logic, to get them into a shape that the front end wants (e.g. if you have a status display field or something, the current value of which is calculated based on 5-10 database fields or other stuff). Unless it's a DB view that you select things from as-is, though if you don't select data based on that criteria, you can get away by doing it in the back end.
Not to say that some of those can't be worked around, but I can't easily handwave those use cases away either. In Java, MapStruct works and does so pretty well: https://mapstruct.org/ I'd rather do something like that, than ask ChatGPT to transpose stuff from DDL or whatever, or waste time manually doing that.
I'll probably look into Mapperly next, thanks! The actual .NET runtime is good and tools like Rider make it quite pleasant.
C# differs quite a bit from Java particularly in surrounding libraries, and there is a learning curve to things...the expectation that it's another Java ends up misleading many people :)
I'm sure MapStruct would also require you to handle differences in data presentation, in a similar way you would have to do with Automapper (or Mapperly). .NET generally puts more emphasis on "boilerplate-free happy path + predictable behavior" so you don't have autowire, but also don't have to troubleshoot autowire issues, and M.E.DI is straightforward to use, as an example. In terms of JSON (with System.Text.Json), you can annotate schema (if it's code-first) with attributes and nullability, so that the request for OrderDetails returns only what is available per given access rights scope. In either case different scopes of access to the same data and presentation of such is a complex topic.
Single-layer case might be a bit extreme - I did use it in a microservice-based architecture as PTSD coping strategy after being hard burned by a poor team environment that insisted on misusing DDD and heavy layering for logic that fits into a single Program.cs, doing a huge disservice to the platform.
Another popular mapping library is Mapster: https://github.com/MapsterMapper/Mapster, it is more focused on convenience compared to Mapperly at some performance tradeoff, but is still decently fast (unlike AutoMapper which is just terrible).
For fast DTO declaration, you can also use positional records e.g. record User(string Name, DateOnly DoB); but you may already be aware of those, noting this for completeness mostly.
Overall, it's a tradeoff between following suboptimal practices of the past and taking much harder stance on enforcing simplicity that may clash with the culture in a specific team.
Though in practice in decent languages it's much less likely you'd write your own `any -> any, any`-typed library for whatever (in this case DB interactions), and use a strongly typed one in which this would at least have been a much more explicit mistake to make.
But this isn't an `any -> any` case. They passed in a default value, as a string, which is the correct type for a default value for this column. Even with very strong typing they wouldn't have got a type error here right?
If the language in question disallows function overloading, this distinction becomes very clear. For an example, see Rust option's unwrap_or[1] vs unwrap_or_else.
Imagine trusting ChatGPT for something important. OMG. My apologies, But I cannot feel any kind of sorry. Even for my private projects, I hardly trust on it since it allucinates a lot.
I have a lot of employees ask about how they will create value in light of AI that can do more and more of the things that have been central to their careers, and the answer is usually that they will do different things than before, and perhaps more of them, by leveraging these tools but also that they are responsible for the quality of work product, the tool is not.
That’s always been the case, but there is so much more surface area for human and tool interactions now that we have tools that are so generalized.
Good for them for sharing the story, countless others have them but not sharing them.
During the work day, this was fine. We probably committed 10-20 times a day
That's... scary, to put it mildly. I wonder how many of those are fixes to things broken by previous commits. Then again, I work on software where the average is far less than one commit per day, although it's a mature product. Nonetheless, "slow down and think" is probably good advice in this case.
One pearl of current software wisdom is "don't think, just do", as a corollary to "move fast, and unbreak things later". Never mind that the cost of unbreaking things is usually far higher than whatever expenses were notionally saved by going to plaid in a hurry.
No, a lack of monitoring cost you $10K. Your app was throwing a database exception and nobody was alerted that this was not only happening, but happening continuously and in large volumes. Such an alert would have made this a 5-minute investigation rather than 5 days.
If you haven't fixed that alerting deficiency, then you haven't really fixed anything.
TBH, if the backend were written in Go, this probably wouldn’t have happened to the extent it did. Somewhere in a log a descriptive error would have shown up.
One of the reasons I use Go whenever possible is that it removes a lot of the classic Python footguns. If you are going to rewrite your backend from
Javascript, why would you rewrite it in another untyped, error-prone language?
Yeah, each have their benefits. I run all my go projects through golangci-lint which is required for merge to master/main, so not checking the error value would not have made it to prod.
I suppose there are probably similar checkers for Python that would have caught the passing of a scalar value instead of a function.
Perhaps this is an argument for mandatory linting in CI.
What did they say was the thinking behind it? defer tx.Rollback() would make sense, but defer tx.Commit() is nonsensical, regardless of whether or not the error is handled. It seems apparent that the problem there isn't forgetting to check an error, but that someone got their logic all mixed up, confusing rollback with commit.
It's not pure nonsense, it works in the happy path, and it matches the pattern of how people often handle file IO in go.
f, err := os.OpenFile(...)
defer f.Close()
... which is another place most gophers ignore errors incorrectly. Just like the "defer tx.Commit()" example, it's collocating the idea of setup and cleanup together.
Those two patterns are so similar, python handles them in the same way:
with db.begin() as conn: # implicit transaction, gets automatically committed
with open(...) as f: # implicit file open + close pair, automatically closed
You're of course right that go requires more boiler-plate to do the right thing, but the wrong code has no compiler errors and works in the happy path, and fits how people think about the problem in other languages with sane RAII constructs, so of course people will write it.
Unrelated but I’ve never really understood what to do with an error that gets returned by Close() aside from logging it. I’ve also never experienced that error being returned and don’t really understand what could even cause that.
A write is not guaranteed to be written if close fails. You almost certainly want to do something with the error state to ensure that the data gets safely stored in that case.
> it's collocating the idea of setup and cleanup together.
Okay, sure, but Rollback is the cleanup function. You always want to rollback – you only sometimes want to commit. I suppose this confirms that someone got their logic mixed up.
This phrase highlights the confusion. If you learned SQL before Go, you want to rollback only on error.
Every time I write `defer tx.Rollback()`, I cringe and have to remind myself that yes, it's actually ok to call a method called `Rollback` after succesfully writing data.
If you learned SQL before Go, you'd know the pattern works just fine and is arguably a good sanity check.
BEGIN;
INSERT INTO ...
COMMIT;
ROLLBACK;
It is not some kind of Go-ism. The Go database/sql package actually executes ROLLBACK in the SQL engine. Check out the error returned by it.
Perhaps you mean learned SQL in the context of languages that consider a failed rollback an exception? In that case one needs to be careful to not rollback, else be stricken to handling the exception, which programmers seem to hate doing.
Fair enough, although it wouldn't matter if it did. Nothing would change other than some unnecessary computation would occur. SQL supports it just fine. Anyone who learned SQL first would know that quite well.
Fair as well. It's just sending a command which you know will fail, and even relying on the fact it fails, feels wrong, whether it sends a request over network or not.
BTW I checked and it's not an error in Postgres, only a warning. Still not something I would want in my database logs for the happy path.
You know it will fail if you didn't screw up anything else.
But it's a good sanity check/safety measure to call it anyway incase you made a mistake elsewhere. An errant rollback is more likely to be caught in testing than a dangling transaction.
I think deploying and then going to sleep is the red flag here. They should have deployed the change at 9am or something and had the workday to monitor issues.
While I agree, it wouldn't have helped. The first n sign-ups per commit worked. So the problem didn't manifest during the day, only when they stopped committing.
Now granted, if they'd carried on doing stuff (but not redeployed production) it may have shown up in say mid-aftenoon, or not depending on volume.
And of course the general guideline of not deploying anything to production early in the day, or on Friday, is still valid.
You can deploy and go to sleep if you have monitoring and alerting and someone getting paged. It shouldn't be a human monitoring for issues anyway, so the only reason to choose 9am over bedtime should be that you don't want to risk a late night page, not that someone will actually be checking up actively on the deployment.
For a big launch, you can have your entire team looking at issues in realtime, rather than a small number of people responding to pages in the middle of the night. I think if it's worth it to you there's a big difference.
That is, if you know what to monitor. When you launch an entirely new feature, you often don't know all the ways it will break.
And it's not trivial to set up a correct alert for this one. Simple HTTP 5xx threshold wouldn't work because it's high enough to not wake you up whenever cloud provider restarts Postgres, it's too high to catch this. You need either per-endpoint failure rate alerts or something else more clever.
Yeah if one instance created one ID, then any integration tests creating more than one user would have failed. There were no testing or logging on a system with live users while doing a refactor between two dynamic languages
This gets more and more common, companies and founders does not think about the infrastructure since they believe that their cloud provider of choice is going to do it for them with it's magic.
As soon as you expect paying customers in your system you need to have someone with the knowledge and experience to deal with infrastructure. That means logging, monitoring, alerting, security etc.
Sure not having tests, is bad. Doing thing with AI without triple checking is dangerous.
But not having error logging/alerts on your db ? That's the crazy part.
This is a new product, is not legacy code from 20 years ago when they thought it was a neat idea to just throw stuff at the db raw, and check for db errors to do data validation, so alerts are hard because there's so many expected errrors.
Agreed. I'm chuckling because I was wrestling with this exact same bug on a FastAPI project last night. I caught it because I have a habit of submitting API endpoints multiple times with the same data to see how the database reacts. Got a key collision when I tried to submit the endpoint the second time and figured out that setup didn't create a new UUID each time.
Unit tests are good, yes. Monitoring is also good. But just taking 30 seconds to do some manual testing will catch a LOT of unexpected behavior.
1. Get it working: write the code for the desired behavior, not worrying about making it beautiful, testable, whatever.
2. Get it working well: manually testing and finding edge cases, refactoring to get it testable and writing tests to solidify behavior.
3. Get it working fast: optimizing it to be as fast as I need it to be (can sometimes skip this step). No tests should change here, but only new tests.
So I understand right, there are two solutions that would have handled this before it even got to prod or at least found it in prod fast.
1. Bunch of tests that simulate exactly the scenario of signups. Hundreds of them actually inserting db records with maybe some kind of dummy stripe code.
2. Logs of the actual uuid for each person.
The second would never have been used since the tests would have caught this bug. But are important anyway.
Seems a bit rich to blame the absence of these two things on chatgpt. That’s just immature engineering practices.
Im guessing they used all those credits to set up those instances, but never took the extra step to add log ingestion or any kind of monitoring. Unique constraint violations peaking should have at least sent some kind of mail or slack notification a few hours after release (putting aside the "it didnt happen during the day because we push to prod several times daily" - which is insane in its own right).
Nothing here really sounds like GPTs fault to me. The issue is something that could easily have been done by a human and missed in PR.
525 comments
[ 3.1 ms ] story [ 341 ms ] threadBookmarked for the next time we're told ChatGPT's code error rate is acceptable because we review its code just like an intern's.
I remember making the exact same mistake (accidentally using a single function call in a schema) back in 2010. No LLMs required.
The bigger culprit is probably a lack of testing / debugging. This error would immediately get caught if you simply registered twice on a test instance.
Testing and debugging isn't just a matter of stepping through code, it's an exercise of seeing where your mental model of the codebase is faulty versus the current reality of it.
I've encountered similar problems and they'd be fixed in a matter of hours, not days.
I wonder why this victim didn't ask ChatGPT to identify and fix the problem...
Because if humans had written and reviewed the code, multiple team members would have had to have learned Python, and SQLAlchemy specifically, which, even if the mistake was initially made as many times as ChatGPT did, there would have been multiple independent opportunities for it to be caught and questioned and the relevant knowledge shared during development.
ChatGPT may be able to crank out immense volumes of superficially functional code, but if its your only “teammate” that understands the libraries used and touches the code, its a huge single point of failure.
Friendly reminder: check if your codebase is actually testing this!
One of the interesting consequence of running unit tests with a fresh database everytime is that problems related to unique constraints seldom get caught by unit tests.
So /that's/ where ChatGPT learned it! :)
I had to fix some intern code once, and… well, I can't give too many details, but I will say that an FAQ shouldn't consist entirely of quotations from a TV show from a different country in a language the app doesn't support.
If HN is to be believed you should use ChatGPT to generate your data insertion code, to port it to a different language, and to ask for which error it made when you asked before. What to do if an error slips through in this version is always an exercise left for the reader.
[1] https://news.ycombinator.com/item?id=40630906
As to the extra parentheses: I bet that's a force-of-habit thing to prevent potential issues. For example, it seems Sqlite requires them for exactly this kind of default definition[1]. It could also read to nasty bugs when the lack of parentheses in the resulting SQL could result in a different parse than expected[2]. Adding them just-to-be-safe isn't the worst thing to do.
[0]: https://docs.sqlalchemy.org/en/13/core/metadata.html
[1]: https://github.com/sqlalchemy/sqlalchemy/issues/4474
[2]: https://github.com/sqlalchemy/sqlalchemy/issues/5344
These "constraints" are why I'm terrified of subscribing to software
The previous world where you buying per user seat licenses for hundreds and hundreds of dollars wasn't great.
We had race conditions where we would charge users twice
This has made me paranoid that any time I see timeout or error related to money I assume it went through and come back later.
It took me several threatening emails to make them understand they had already taken my money and I wasn't going to try again until I got a refund. Now I'm paranoid any time I purchase online at mediocre shops.
On the other hand, I don’t think advertising the fact that the company introduced a major bug from copy and pasting ChatGPT code around and that they spent a week being unable to even debug why it was failing.
I don’t know much about this startup, but this blog post had the opposite effect of all of the other high quality post-mortem posts I’ve read lately. Normally the goal of these posts is to demonstrate your team’s rigor and engineering skills, not reveal that ChatGPT is writing your code and your engineers can’t/won’t debug it for a very long time despite knowing that it’s costing them signups.
Except, have you met startup devs? This is by and large the "move fast then unbreak things" approach.
Forget knowing anything, just come up with a nice pitch deck and let the LLM write the stack.
Not wholly surprised these people are YC backed. I’ve got the impression YC don’t place much weight on technical competence, assuming you can just hire the talent if you know how to sell.
Well, now replace “hire some talent” with “get a GPT subscription and YOLO”, and you get the foundation these companies of tomorrow are going to be built on.
Which hey, maybe that’s right and they know something I don’t.
My early rant against this mentality: https://news.ycombinator.com/item?id=19214749
People working in "big tech" aren't fundamentally better at building reliable tools and systems; the time and resource constraints are entirely different.
5 days to find out you have "duplicate key" errors in the db is the opposite of fast
If anything, I think this says something about how dangerous ChatGPT and similar tools are: reading code is harder than writing code, and when you use ChatGPT, your role stops being that of a programmer and becomes that of a code reviewer. Worse, LLMs are excellent at producing plausible output (I mean that's literally all they do), which means the bugs will look like plausibly correct code as well.
I don't think this is indicative of people who don't know what they're doing. I think this is indicative of people using "AI" tools to help with programming at all.
I think using AI tools to write production code is probably indicative of people who don't really know what they are doing.
The best way not to have subtle bugs is to think deeply about your code, not subcontract it out -- whether that is to people far away who both cannot afford to think as deeply about your code and aren't as invested in it, or to an AI that is often right and doesn't know the difference between correct and incorrect.
It's just a profound abrogation of good development principles to behave this way. And where is the benefit in doing this repeatedly? You're just going to end up with a codebase nobody really owns on a cognitive level.
At least when you look at a StackOverflow answer you see the discussion around it from other real people offering critiques!
ETA in advance: and yes, I understand all the comparison points about using third party libraries, and all the left-pad stuff (don't get me started on NPM). But the point stands: the best way not to have bugs is to own your code. To my mind, anyone who is using ChatGPT in this way -- to write whole pieces of business logic, not just to get inspiration -- is failing at their one job. If it's to be yours, it has to come from the brain of someone who is yours too. This is an embarrassing and damaging admission and there is no way around it.
ETA (2): code review, as a practice, only works when you and the people who wrote the code have a shared understanding of the context and the goal of the code and are roughly equally invested in getting code through review. Because all the niche cases are illuminated by those discussions and avoided in advance. The less time you've spent on this preamble, the less effective the code review will be. It's a matter of trust and culture as much as it's a matter of comparing requirements with finished code.
You could say the same about the output of a compiler. No one owns that at a cognitive level. They own it at a higher level - the source code.
Same thing here. You own the output of the AI at a cognitive level, because you own the prompts that created it.
Except, for starters, that you're not using the LLM to replace a compiler.
You're using it to replace a teammate.
Notwithstanding the fact that compilers did not fall out of the sky and very much have people that own them at the cognitive level, I think this is still a different situation.
With a compiler you can expect a more or less one to one translation between source code and the operation of the resulting binary with some optimizations. When some compiler optimization causes undesired behavior, this too is a very difficult problem to solve.
Intentionally 10xing this type of problem by introducing a fuzzy translation between human language and source code then 1000xing it by repeating it all over the codebase just seems like a bad decision.
But at least it's supposed to be deterministic. And there's a chance someone else will be able to explain the inner workings in a way I can repeatably test.
(1) Compilers are reproducible (or at least repeatable), so you can share your problem with other, and they can help.
(2) For common languages, there are multiple compilers and multiple optimization options, which (and that's _very important_) produce identically-behaving programs - so you can try compiling same program with different settings, and if they differ, you know compiler is bad.
(3) The compilers are very reliable, and bugs when compiler succeeds, but generates invalid code are even rarer - in many years of my career, I've only seen a handful of them.
Compare to LLMs, which are non-reproducible, each one is giving a different answer (and that's by design) and finally have huge appear-to-succeed-but-produce-bad-output error rate, with value way more than 1%. If you had a compiler that bad, you'd throw it away in disgust and write in assembly language.
Colour me cynical but I don't feel like pretending the future is here only to have to have to fix its blind incompetence.
It is possible to use libraries correctly.
It is not possible to use AI correctly. It is only possible to correct its inevitable mistakes.
But our existing tools are already built to help us avoid this.
Back in the day, I used a tool from a group in Google called “error-prone”. It was great at catching things like this (and Lorne goal NPE in Java). It would examine code before compiling to find common errors like this. I wish we had more “quick” check tools for more languages.
It's not in quotes. It's a function call.
The issue is that the function call happens once, when you define the class, rather than happening each time you instantiate the class.
I say that not to brag because (a) default args is a known python footgun area already and (b) I'd hope most developers with any real Django or SQLAlchemy experience would have caught this pretty quick. I guess I'm just suggesting that maybe domain experience is actually worth something?
Also, where were their tests in the first place? Or am I expecting too much there?
Not only does it goof up less frequently on small focused snippets like this, it also requires me to pick the example apart and pay close enough attention to it that goofups don’t slip by as easily and it gets committed to memory more readily than with copypasting or LLM-backed autocomplete.
It's also the case that "code review" covers a lot of things, from quickly skimming the code and saying eh, it's probably fine, to deeply reading and ensuring that you fully understand the behavior in all possible cases of every line of code. The latter is much more effective, but probably not nearly as common as it ought to be.
In my experience, hardly anyone in software does know what they're doing, for sufficiently rigorous values of "know what you're doing." We all read about other people's stupid mistakes, and think "haha, I would never have done that, because I know about XYZ!" And then we go off and happily make some equally stupid mistake because we don't know about ABC.
I turn down a lot of jobs I don't feel confident with; maybe more than I should.
An LLM never will.
Now at a new company i have caught several people copy pasting gpt code that is just horrendous.
It seems like this is where the industry is headed. The only thing i have found gpt to be good at is solving interview questions although it still uses phantom functions about 50% of the time. The future is bumming me out.
However, I don't think it's really bad for the technical industries long term. It probably does mean that some companies with loose internal quality control and enough shiftless employees pasting enough GPT spew without oversight will go to the wall because their software became unmaintainable and not useful, but this already happens. It's probably not hugely worse than the flood of bootcamp victims who wrote fizzbuzz in Python, get recruited by a credulous, cheap or desperate company and proceed to trash the place if not adequately supervised. If you can't detect bad work being committed, that's mostly on the company, not ChatGPT. Yes, it may make it harder, a bit, but it was oversight you should already have been prepared to have, doubly so if you can't trust employee output. It also probably implies strong QA, which is already a prerequisite of a solid product company.
Normal interest rates coming back will cut away companies that waste everyone's time and money by overloading themselves on endlessly compounding technical debt.
Yes, it makes the barrier higher even for good products and helps entrench incumbents, but short of a transnational revolution, the macroeconomic system is what is it and you can only chose to find the good things in it or give up entirely.
Yeah I've seen this and i hate it. If i wanted to know what chatgpt said I'd just ask it myself.
Everyone using C++20 compilers: side-glancing monkey.
Then no one knows what they are doing. I really don't know any company that doesn't make what could be considered rookie mistakes by some armchair "developer" here on HN.
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.
...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.
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.
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.
But it took about 5 minutes, and 4 of those minutes were waiting for Kibana to load.
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.
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.
Apparently not.
create subscriptions with and without overlapping effective windows
Those seem like very basic tests that would have highlighted the underlying issue
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).
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.
Volume/load testing (or really, any decent acceptance testing) would catch it.
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?
Leaking information between test runs can actually make things pass by accident.
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.
Perhaps they are doing some AI thing and want to have python everywhere.
There is well-hidden vendor-lock when using NextJS, at least.
You will find many issues from GitHub which are not considered because they would make the framework ”better” or easier to use on other clouds.
I can imagine worse, too! They haven't even really started turning that knob yet.
Madden has a monopoly license for NFL content. For a decade the biggest complaint was how they gate kept rosters behind the yearly re-release. Eventually they allowed roster sharing but they put it behind the most god awful inept UI you could possibly imagine such that practically casual gamers wouldn't bother with it.
Then Madden came out with Madden Ultimate Team (like trading cards MTX) and have been neglecting non-MUT modes ever since. They don't explicitly regress the rest of their game, they just commit resources to that effect.
Its like malicious compliance. They don't embrace, extend, extinguish, but they get a similar effect just with resourcing, layoffs, whatever.
Do you mind naming some NextJS alternatives without potential vendor lock in? Mulling a change in my fe.
https://remix.run/
The SST team actually has an open-next project[1] that does a ton of work to shim most of the Vercel-specific features into an AWS deployment. I don't think it has full parity and it's a third party adapter from a competing host. The fact that it's needed at all is a sign of now closely tied Next and Vercel are.
[1] https://github.com/sst/open-next
It is not an issue if you host in Vercel.
Implementing the requested feature would make the framework much better and easier to use when self-hosted elsewhere. But there is neglection to resolve the issue. This is just one case.
The lock-in here is the added developer time and complexity vs. just paying premium.
See CF docs for what the url should be https://developers.cloudflare.com/images/transform-images/tr... Next.js docs should tell you how to write the loader
- They were under large time constraints, but decided a full rewrite to a completely different stack was a good idea.
- They copy-pasted a whole bunch of code, tested it manually once locally, once in production, and called it a day.
- The debugging procedure for this issue so significant it made them dread waking up involved... testing it once and moving on. Every day.
The bug is pretty easy to miss, but should also be trivial to diagnose the moment you look at the error message, and trivial to reproduce if you just try more than once.
Because I can see "create a new subscription" in the manual test plan, but not "create 5x new subscription".
May be a wise thing to do anyway. I would even advocate for the extreme, one instance in sandbox. I've seen quite a variety of bugs that would be easier to detect and debug with one instance — from values accidentally persisted between requests (OP is one example of this very broad class of bugs), to a thundering herd bug catched in staging due to less instances and more worker threads per instance. But can't remember even one "distributed" bug caught in staging.
> they'd also have a dedicated QA person
This is a new feature, and it's user-facing, business-critical and inherently fragile (external service integration, though it broke for another reason). I hope multiple people manually run such things end to end before deploying to prod, even if none of them are dedicated QA?
A lot more than once: they had 40 instances of their app, and the bug was only triggered by getting two requests on the same instance.
A bunch of developers including me once spent a whole weekend trying to reproduce a bug that was affecting production and/or guess from the logs where to look for it. Monday morning, team lead called a meeting, asked for everything we could find out, and… Opened the app in six tabs simultaneously and pressed the button in question in one of the tabs. And it froze! Knowing how to reproduce on our computers, we found and fixed the bug in the next 30 minutes.
This is an entirely forgivable error but should have been found the first time they got an email about it:
"Oh, look, the error logs have a duplicate key exception for the primary key, how do we generate primary keys.... (facepalm)"
Funnily enough, I saw the error in their snippet as soon as I read it but dismissed it thinking there was some new-fangled python feature which allowed that to work like the function def defines that default= accepts only functions so the function gets passed? -- I haven't kept up with modern python and that sounds cool and I figured the bug couldn't be THAT simple.
There's value in having your backtrace surfaced to end users rather than swallowing an exception and displaying "didn't work".
> There's value in having your backtrace surfaced to end users
If they weren't, that should be the first thing you fix.
For something like this where you’re generating a unique id and probably need it in every model, it’s better to write a new Base model that includes things like your unique id, created/changed at timestamps, etc. and then subclass it for every model. Basically, write your model boilerplate once and inherit it. This way you can only fuck this up once and you’re bound to catch it early before you make this mistake in a later addition like subscription management.
This mistake would have happened even if they did not use ChatGPT.
Some linters like pyright can identify dangerous defaults in function call, like `def hello(x=[]): pass` (mutable values shouldn't be a default). Linter plugins for widely-used and critical libraries like SQLAlchemy are nice to have.
The intent is to pass a callable, not to call a function and populate an argument with what it returns.
The time people spend learning the quirks of an ORM is much better put into learning SQL.
I agree that this is probably to their disadvantage, but I would much rather have people admitting their faults than hiding them. If everyone did this the world would be better.
Of course the best solution is to not have faults but that is like saying that the solution to being poor is to have lots of money. It's much easier to say than do.
> Yes we should have done more testing. Yes we shouldn't have copy pasted code. Yes we shouldn't have pushed directly to main. Regardless, I don't regret the experience.
None of those are unconditionally bad! Every project I've worked on could use more testing; we all copy-pasted code at least occasionally, and pushing to main is fine in some circumstances.
The real problem is that they went live, but their tooling (or knowledge how to use it) was so bad it took 5 days to solve the simple issue; and meanwhile, they kept pushing new code ("10-20 commits/day") while their customers were suffering. This is what really causes the reputation hit.
Otherwise, I think this comment thread is a classic example why company engineering blogs choose to be boring. Better ten articles that have some useful information, than a single article that allows the commentariat to pile on and ruin your reputation.
The AI angle is probably why people are piling on. There’s a latent fear that AI will take our jobs, and this is a great way to skewer home that we’re still needed. For now.
The one thing I will say is that it probably wouldn’t take me days to track it down. But that’s only because I have lots of experience dealing with bugs the way that The Wolf deals with backs of cars. When you’re trying to run a startup on top of everything else, it can be easy to miss.
I’m happy they gave us a glimpse of early stage growing pains, and I don’t think this was a PR fumble. It shows that lots of people want what they’re making, which is roughly the only thing that matters.
On the one hand it does seem like a fairly inexperienced organization with some pretty undercooked release and testing processes, but on the other hand all that stuff is ultimately fixable. This is a relatively harmless way of learning that lesson. Admitting a problem is the first step toward fixing it.
A culture of ass-covering is much harder to fix, and will definitely get in the way of addressing these types of issues
Pile-on aside, the problem with this blog article is that it doesn't really have much of a useful takeaway.
They didn't even really talk the offending line in detail. They didn't really talk about what did to fix their engineering pipelines. It was just a story about how they let ChatGPT write some code, the code was buggy, and the bug was hard to spot because they relied on customers e-mailing them about it in a way that only happened when they were sleeping.
It's not really a postmortem, it's a story about fast and loose startup times. Which could be interesting in itself, except it's being presented more as an engineering postmortem blog minus the actionable lessons.
That's why everyone is confused about why this company posted this as a lesson: The lesson is obvious and, frankly, better left as a quiet story for the founders to chuckle about to their friends.
Also the CEO: "remember to be defensive on reddit comments saying how we are a small 1 million dollar backed startup and how it's normal do to this king of rookies mistake to be fast."
but then perhaps coding the entire thing with ChatGPT saved the company more than 10K, so they came out well ahead
or maybe tons of other bugs lurk that will cost the company well over 10K over the long run
But what I don't understand is how this wasn't caught after the first failure? Does this company not have any logging? Shouldn't the fact the backend is attempting to reuse UUIDs be immediately obvious from observing the error?
UUIDs are just 128-bit values. They might be conventionally encoded for humans as hex, but storing them as 36-byte (plus a few more for length) strings is a pointless waste of both space and performance.
I don't think 128 bits vs 36 byte performance it's a main concern right now
36B vs 16B today, tomorrow you need an array of it, and now it isn't cache aligned, and more than twice the overhead.
Most likely instead of manipulating a 36B fixed length string, it is handled as a dynamic string, for extra runtime memory allocations, most likely consuming at least 64B per allocation. Etc etc.
Do this all over the codebase and now you know why all the moderne software is a sloth on what was a supercomputer 30y ago.
It’s also not just the size itself. Despite being fixed-size in practice, these are variable-sized strings in application code which now means gajillions of pointless allocations and indirection for everything. There are a ton of knock-on performance consequences here, all on the most heavily-used columns in your data model.
Worst of all should they actually succeed, this is going to be absolutely excruciating to fix.
But in either case – MySQL or Postgres – they’ve still made the classic mistake of using a UUID as a PK, which will tank performance one way or another. They’ll notice right around when it starts to matter.
I am also very confused about the apparent lack of logging or recourse to logging. It's been a while, but if I recall correctly ECS should automatically propagate the resulting Duplicate Key exceptions which were presumably occurring to CloudWatch without a bunch of additional configuration - was that not happening? If it was happening, did no one think to go check what types of Exceptions were happening overnight?
I guarantee you that they _will_ have another production bug like this sometime in the future (every fast paced project will). You'd hope this next one wont take 5 days to identify.
Specifically asking why did it take so long to detect and why did it take so long to diagnose is useful in these situations.
Type 1 tries to find the error message and figure out what it really, really means by breaking down the error message and system.
Type 2 does trial and error on random related things until the problem goes away.
I hate to say that I've seen way more type 2s engineers than type 1, but maybe I’m working at the wrong companies.
Here we are talking 1.65 MILLION CAD $ backed YC company
though a little logging would also have gone a long way. I don't really get how this could have taken 5 days to find, since they knew exactly where the problem was
Or more specially, given the context: "We were in a rush to translate a bunch of code and ChatGPT was doing such an impressive job helping that we became complacent and forgot that it just parrots back text it has seen before with something that looks like intelligence but without actual comprehension. So when it copied a common bug, we weren't paying enough attention to catch it."
Yes, but that line is only evaluated once when the class is declared (aka when the application starts), it's not evaluated for every instance.
Although I'm not a SQL Alchemy user, I assume the fix is essentially the same as it would be for Django. So the correct code would have been essentially:
Instead of executing `uuid4()` and caching a single UUID value, it would have executed `uuid4()` each time a new object was created.Sure,
```
UUID4 = uuid.uuid4()
def default_id():
```Tbh, if they were writing something for prod they should have factored out the uuid generation and passed values into a pydantic dataclass w/ validators for all the fields. Just sayin.
default = python code evaluates the default value as necessary for each new record
server_default = the initial CREATE TABLE uses this computed (from python) value, thus the hardcoded UUID. They also could have done server_default=text("uuid_generate_v4()") if they had that corresponding module installed on postgres.
Separate table definition (DDL) from row-insertion (DML) definitions along with their corresponding defaults.
Does there, though? You can always set `Column(default = lambda: 1234)`
To be explicit: You're not seeing a function called later in python. You're setting an attribute on a database table.
[0]: https://docs.sqlalchemy.org/en/20/core/defaults.html
It seems quite unfair to place the blame on SQLAlchemy here, or even Python.
Even a statically typed language wouldn't prevent this kind of issue - the author of the code is the only person who can decide when they mean "use this exact string each time" or "use this function to give me a new string each time".
I suppose a column description API could follow the dataclass style definition, with different argument names for default and default_func. That would (I think) prevent this from happening.
I feel like "terrible ORM" is a tautology.
This issue highlighted in one respect a fundamental problem with all ORMs and why I despise them so much - they're the absolute worst of a leaky abstraction, and as I like to say "they make the easiest stuff a bit easier, and they make the harder stuff way harder". E.g. in this specific case they don't prevent you from needing to know the details of default column value initialization, but apparently they must have also obscured a simple duplicate key constraint to some level that it took 5 days to find the root cause of this bug.
Just learn SQL. You'll need to know it anyway even if you do use an ORM, the basics aren't hard, the skill is much more transferable than the esoteric details of some ORM, and there are lots of good libraries that make dealing directly with SQL easy and safe (slonik for postgres is a favorite of mine, but there are other similar ones).
This is the eye opener for me, how is a startup justifying a re-write when they don't even have customers?
Perhaps also the tooling because any remotely decent IDE should show an error there, let alone the potential warnings of some code analysis software.
(This is one thing that baffles me about the “let’s use LLMs to code” movement; a lot of the proponents don’t seem to be just adding it as a tool (I don’t think it’s a terribly _useful_ tool, but whatever, tastes differ), but using it as the only tool, discarding 50 years worth of progress.)
In my case (with a real project I'm working on now), it'd be due to realizing that C# is a great language and has a good runtime and web frameworks, but at the same time drags down development velocity and has some pain points which just keep mounting, such as needing to create bunches of different DTO objects yet AutoMapper refusing to work with my particular versions of everything and project configuration, as well as both Entity Framework and the JSON serializer/deserializer giving me more trouble than it's worth.
Could the pain points be addressed through gradual work, which oftentimes involves various hacks and deep dives in the docs, as well as upgrading a bunch of packages and rewriting configuration along the way? Sure. But I'm human and the human desire is to grab a metaphorical can of gasoline, burn everything down and make the second system better (of course, it might not actually be better, just have different pain points, while not even doing everything the first system did, nor do it correctly).
Then again, even in my professional career, I get the same feeling whenever I look at any "legacy" or just cumbersome system and it does take an active, persistent effort on my part to not give in to the part of my brain that is screaming for a rewrite. Sometimes rewrites actually go great (or architectural changes, such as introducing containers), more often than not everything goes down in a ball of flames and/or endless amounts of work.
I'm glad that I don't give in, outside of the cases where I know with a high degree of confidence that it would improve things for people, either how the system runs, or the developer experience for others.
Hell, most simple applications could do with just a single layer - schema registration in EF Core is mapping, or at most two, one for DB and one for response contracts.
Just do it the simplest way you can. I understand that culture in some companies might be a problem, and it's been historically an issue plaguing .NET, spilling over, originally, from Java enterprise world. But I promise you there are teams which do not do this kind of nonsense.
Things really have improved since .NET Framework days, EF Core productivity wise, while similar in its strong areas, is pretty much an entirely new solution everywhere else.
The thing is, that you'll probably have entities mapped against the database schema with data that must only conditionally be shown to the users. For example, when an admin user requests OrderDetails then you'll most likely want to show all of the fields, but when an external user makes that request, you'll only want to show some of the fields (and not leak that those fields even exist).
DTOs have always felt like the right way to do that, however this also means that for every distinct type of user you might have more than one object per DB table. Furthermore, if you generate the EF entity mappings from the schema (say, if you handle migrations with a separate tool that has SQL scripts in it), then you won't make separate entities for the same table either. Ergo, it must be handled downstream somewhere.
Plus, sometimes you can't return the EF entities for serialization into JSON anyways, since you might need to introduce some additional parsing logic, to get them into a shape that the front end wants (e.g. if you have a status display field or something, the current value of which is calculated based on 5-10 database fields or other stuff). Unless it's a DB view that you select things from as-is, though if you don't select data based on that criteria, you can get away by doing it in the back end.
Not to say that some of those can't be worked around, but I can't easily handwave those use cases away either. In Java, MapStruct works and does so pretty well: https://mapstruct.org/ I'd rather do something like that, than ask ChatGPT to transpose stuff from DDL or whatever, or waste time manually doing that.
I'll probably look into Mapperly next, thanks! The actual .NET runtime is good and tools like Rider make it quite pleasant.
I'm sure MapStruct would also require you to handle differences in data presentation, in a similar way you would have to do with Automapper (or Mapperly). .NET generally puts more emphasis on "boilerplate-free happy path + predictable behavior" so you don't have autowire, but also don't have to troubleshoot autowire issues, and M.E.DI is straightforward to use, as an example. In terms of JSON (with System.Text.Json), you can annotate schema (if it's code-first) with attributes and nullability, so that the request for OrderDetails returns only what is available per given access rights scope. In either case different scopes of access to the same data and presentation of such is a complex topic.
Single-layer case might be a bit extreme - I did use it in a microservice-based architecture as PTSD coping strategy after being hard burned by a poor team environment that insisted on misusing DDD and heavy layering for logic that fits into a single Program.cs, doing a huge disservice to the platform.
Another popular mapping library is Mapster: https://github.com/MapsterMapper/Mapster, it is more focused on convenience compared to Mapperly at some performance tradeoff, but is still decently fast (unlike AutoMapper which is just terrible).
For fast DTO declaration, you can also use positional records e.g. record User(string Name, DateOnly DoB); but you may already be aware of those, noting this for completeness mostly.
Overall, it's a tradeoff between following suboptimal practices of the past and taking much harder stance on enforcing simplicity that may clash with the culture in a specific team.
Though in practice in decent languages it's much less likely you'd write your own `any -> any, any`-typed library for whatever (in this case DB interactions), and use a strongly typed one in which this would at least have been a much more explicit mistake to make.
Generally, static languages will just culturally be less likely to have this kind of invisible "T | (() -> T)" overload.
[1] https://doc.rust-lang.org/std/option/enum.Option.html#method...)
That’s always been the case, but there is so much more surface area for human and tool interactions now that we have tools that are so generalized.
Good for them for sharing the story, countless others have them but not sharing them.
That's... scary, to put it mildly. I wonder how many of those are fixes to things broken by previous commits. Then again, I work on software where the average is far less than one commit per day, although it's a mature product. Nonetheless, "slow down and think" is probably good advice in this case.
If you haven't fixed that alerting deficiency, then you haven't really fixed anything.
Programming when everything works is easy, it's handling the problems that makes it hard.
"Under construction "
Looks like the OP removed the post?
EDIT: Found archived copy of post: http://web.archive.org/web/20240609213809/https://asim.bearb...
One of the reasons I use Go whenever possible is that it removes a lot of the classic Python footguns. If you are going to rewrite your backend from Javascript, why would you rewrite it in another untyped, error-prone language?
In go, I've definitely seen:
That would have masked this error so it didn't get logged by the application.In python, if you ignore an exception entirely, like I did that error above, you instead get an exception logged by default.
Python's exceptions also include line numbers, where as Go errors by default wouldn't show you _which_ object has a conflict, even if you logged it.
In general, python's logs are way better than Go's, and exceptions make it way harder to ignore errors entirely than Go's strategy.
I suppose there are probably similar checkers for Python that would have caught the passing of a scalar value instead of a function.
Perhaps this is an argument for mandatory linting in CI.
What did they say was the thinking behind it? defer tx.Rollback() would make sense, but defer tx.Commit() is nonsensical, regardless of whether or not the error is handled. It seems apparent that the problem there isn't forgetting to check an error, but that someone got their logic all mixed up, confusing rollback with commit.
It's not pure nonsense, it works in the happy path, and it matches the pattern of how people often handle file IO in go.
... which is another place most gophers ignore errors incorrectly. Just like the "defer tx.Commit()" example, it's collocating the idea of setup and cleanup together.Those two patterns are so similar, python handles them in the same way:
You're of course right that go requires more boiler-plate to do the right thing, but the wrong code has no compiler errors and works in the happy path, and fits how people think about the problem in other languages with sane RAII constructs, so of course people will write it.If you are only reading, then sure, who cares?
Okay, sure, but Rollback is the cleanup function. You always want to rollback – you only sometimes want to commit. I suppose this confirms that someone got their logic mixed up.
This phrase highlights the confusion. If you learned SQL before Go, you want to rollback only on error.
Every time I write `defer tx.Rollback()`, I cringe and have to remind myself that yes, it's actually ok to call a method called `Rollback` after succesfully writing data.
Perhaps you mean learned SQL in the context of languages that consider a failed rollback an exception? In that case one needs to be careful to not rollback, else be stricken to handling the exception, which programmers seem to hate doing.
No: https://github.com/golang/go/blob/beaf7f3282c2548267d3c89441...
BTW I checked and it's not an error in Postgres, only a warning. Still not something I would want in my database logs for the happy path.
But it's a good sanity check/safety measure to call it anyway incase you made a mistake elsewhere. An errant rollback is more likely to be caught in testing than a dangling transaction.
Now granted, if they'd carried on doing stuff (but not redeployed production) it may have shown up in say mid-aftenoon, or not depending on volume.
And of course the general guideline of not deploying anything to production early in the day, or on Friday, is still valid.
And it's not trivial to set up a correct alert for this one. Simple HTTP 5xx threshold wouldn't work because it's high enough to not wake you up whenever cloud provider restarts Postgres, it's too high to catch this. You need either per-endpoint failure rate alerts or something else more clever.
As soon as you expect paying customers in your system you need to have someone with the knowledge and experience to deal with infrastructure. That means logging, monitoring, alerting, security etc.
DevOps.. amateurs.
But not having error logging/alerts on your db ? That's the crazy part.
This is a new product, is not legacy code from 20 years ago when they thought it was a neat idea to just throw stuff at the db raw, and check for db errors to do data validation, so alerts are hard because there's so many expected errrors.
Unit tests are good, yes. Monitoring is also good. But just taking 30 seconds to do some manual testing will catch a LOT of unexpected behavior.
1. Get it working: write the code for the desired behavior, not worrying about making it beautiful, testable, whatever.
2. Get it working well: manually testing and finding edge cases, refactoring to get it testable and writing tests to solidify behavior.
3. Get it working fast: optimizing it to be as fast as I need it to be (can sometimes skip this step). No tests should change here, but only new tests.
1. Bunch of tests that simulate exactly the scenario of signups. Hundreds of them actually inserting db records with maybe some kind of dummy stripe code.
2. Logs of the actual uuid for each person.
The second would never have been used since the tests would have caught this bug. But are important anyway.
Seems a bit rich to blame the absence of these two things on chatgpt. That’s just immature engineering practices.
Nothing here really sounds like GPTs fault to me. The issue is something that could easily have been done by a human and missed in PR.