15 ms·

This is the multi-million dollar .unwrap() story. In a critical path of infrastructure serving a significant chunk of the internet, calling .unwrap() on a Resul

by ojosilva 10mo ago

This is the multi-million dollar .unwrap() story. In a critical path of infrastructure serving a significant chunk of the internet, calling .unwrap() on a Result means you're saying "this can never fail, and if it does, crash the thread immediately."The Rust compiler forced them to acknowledge this could fail (that's what Result is for), but they explicitly chose to panic instead of handle it gracefully. This is textbook "parse, don't validate" anti-pattern. I know, this is "Monday morning quarterbacking", but that's what you get for an outage this big that had me tied up for half a day.

shadowgovt 10mo ago
In addition, it looks like this system wasn't on any kind of 1%/10%/50%/100% rollout gating. Such a rollout would trivially have shown the poison input killing tasks.
- penteract 10mo ago
  To me it reads like there was a gradual rollout of the faulty software responsible for generating the config files, but those files are generated on approximately one machine, then propogated across the whole network every 5 minutes. > Bad data was only generated if the query ran on a part of the cluster which had been updated. As a result, every five minutes there was a chance of either a good or a bad set of configuration files being generated and rapidly propagated across the network.
- helloericsf 10mo ago
  Not a DBA, how do you do DB permission rollout gating?
  - shadowgovt 10mo ago
    It looks like changing the permissions triggered creation of a new feature file, and it was ingestion of that file leading to blowing a size limit that crashed the systems. The file should be versioned and rollout of new versions should be staged. (There is definitely a trade-off; often times in the security critical path, you want to go as fast as possible because changes may be blocking a malicious actor. But if you move too fast, you break things. Here, they had a potential poison input in the pathway for synchronizing this state and Murphy's Law suggests it was going to break eventually, so the question becomes "How much damage can we tolerate when it does?")
    - dwattttt 10mo ago
      > It looks like changing the permissions triggered creation of a new feature file, and it was ingestion of that file leading to blowing a size limit that crashed the systems. That feature file is generated every 5 minutes at all times; the change to permissions was rolled out gradually over the clickhouse cluster, and whether a bad version of that file was generated depended on whether the part of the cluster that had the bad permissions generated the file.
wrs 10mo ago
It seems people have a blind spot for unwrap, perhaps because it's so often used in example code. In production code an unwrap or expect should be reviewed exactly like a panic. It's not necessarily invalid to use unwrap in production code if you would just call panic anyway. But just like every unsafe block needs a SAFETY comment, every unwrap in production code needs an INFALLIBILITY comment. clippy::unwrap_used can enforce this.
- dist1ll 10mo ago
  > every unwrap in production code needs an INFALLIBILITY comment. clippy::unwrap_used can enforce this. How about indexing into a slice/map/vec? Should every `foo[i]` have an infallibility comment? Because they're essentially `get(i).unwrap()`.
  - danielheath 10mo ago
    I mean... yeah, in general. That's what iterators are for.
  - tux3 10mo ago
    Usually you'd want to write almost all your slice or other container iterations with iterators, in a functional style. For the 5% of cases that are too complex for standard iterators? I never bother justifying why my indexes are correct, but I don't see why not. You very rarely need SAFETY comments in Rust because almost all the code you write is safe in the first place. The language also gives you the tool to avoid manual iteration (not just for safety, but because it lets the compiler eliminate bounds checks), so it would actually be quite viable to write these comments, since you only need them when you're doing something unusual.
    - dist1ll 10mo ago
      For iteration, yes. But there's other cases, like any time you have to deal with lots of linked data structures. If you need high performance, chances are that you'll have to use an index+arena strategy. They're also common in mathematical codebases.
arccy 10mo ago
if you make it easy to be lazy and panic vs properly handling the error, you've designed a poor language
- yoyohello13 10mo ago
  So… basically every language ever? Except maybe Haskell.
  - dkersten 10mo ago
    And Gleam
  - yakshaving_jgt 10mo ago
    It's easy to cause this kind of failure in Haskell also.
- otterley 10mo ago
  https://en.wikipedia.org/wiki/Crash-only_software https://en.wikipedia.org/wiki/Crash-only_software
  - nine_k 10mo ago
    Works when you have the Erlang system that does graceful handing for you: reporting, restarting.
- SchwKatze 10mo ago
  Unwrap isn't a synonym for laziness, it's just like an assertion, when you do unwrap() you're saying the Result should NEVER fail, and if does, it should abort the whole process. What was wrong was the developer assumption, not the use of unwrap.
  - dietr1ch 10mo ago
    > What was wrong was the developer assumption, not the use of unwrap. How many times can you truly prove that an `unwrap()` is correct and that you also need that performance edge? Ignoring the performance aspect that often comes from a hat-trick, to prove such a thing you need to be wary of the inner workings of a call giving you a `Return`. That knowledge is only valid at the time of writing your `unwrap()`, but won't necessarily hold later. Also, aren't you implicitly forcing whoever changes the function to check for every smartass dev that decided to `unwrap` at their callsite? That's bonkers.
deleted 10mo ago
[deleted]
cvhc 10mo ago
Some languages and style guides simply forbid throwing exceptions without catching / proper recovery. Google C++ bans exceptions and the main mechanism for propogating errors is `absl::Status` which the caller has to check. Not familiar with Rust but it seems unwrap is such a thing that would be banned.
- pdimitar 10mo ago
  There are even lints for this but people get impatient and just override them or fight for them to no longer be the default. As usual: people problem, not a tech problem. In the last years a lot of strides have been made. But people will be people.
  - tonyhart7 10mo ago
    and people make mistake at some point machine would be better in coding because well machine code is machine instruction task same like chess, engine is better than human grandmaster because its solvable math field coding is no different
    - aw1621107 10mo ago
      > same like chess, engine is better than human grandmaster because its solvable math field Might be worth noting that your description of chess is slightly incorrect. Chess technically isn't solved in the sense that the optimal move is known for any arbitrary position is known; it's just that chess engines are using what amounts to a fancy brute force for most of the game and the combination of hardware and search algorithm produces a better result than the human brain does. As such, chess engines are still capable of making mistakes, even if actually exploiting them is a challenge.
      - tonyhart7 10mo ago
        No ?????? because these thing called BEST MOVE and BAD MOVE there in chess "chess engines are still capable of making mistakes", I'm sorry no inaccurate yes but not mistake
vlovich123 10mo ago
To be fair, this failed in the non-rust path too because the bot management returned that all traffic was a bot. But yes, FL2 needs to catch panics from individual components but I’m not sure if failing open is necessarily that much better (it was in this case but the next incident could easily be the result of failing open). But more generally you could catch the panic at the FL2 layer to make that decision intentional - missing logic at that layer IMHO.
- hedora 10mo ago
  Catching panic probably isn’t a great idea if there’s any unsafe code in the system. (Do the unsafe blocks really maintain heap invariants if across panics?)
  - vlovich123 10mo ago
    Unsafe blocks have nothing to do with it. Yes - they maintain all the same invariants as safe blocks or those unsafe blocks are unsound regardless of panics. But there’s millions of way to architect this (eg a supervisor process that notices which layer in FL2 is crashing and just completely disables that layer when it starts up the proxy again. There’s challenges here because then you have to figure out what constitutes a perma crashing (eg what if it’s just 20% of all sites? Do you disable?). And in the general case you have the fail open/fail close decision anyway which you should just annotate individual layers with. But the bigger change is to make sure that config changes roll out gradually instead of all at once. That’s the source of 99% of all widespread outages
    - Feathercrown 10mo ago
      Incremental config changes sounds like it could lead to a LOT of bugs
      - vlovich123 10mo ago
        Incremental in terms of 1% of the fleet using it, then 5% etc. this is standard course. Another option is to make sure that config changes that fail to parse continue using the old config instead of resulting in an unusable service.
ajross 10mo ago
I'm not completely sure I agree. I mean, I do agree about the .unwrap() culture being a bug trap. But I don't think this example qualifies. The root cause here was that a file was mildly corrupt (with duplicate entries, I guess). And there was a validation check elsewhere that said "THIS FILE IS TOO BIG". But if that's a validation failure, well, failing is correct? What wasn't correct was that the failure reached production. What should have happened is that the validation should have been a unified thing and whatever generated the file should have flagged it before it entered production. And that's not an issue with function return value API management. The software that should have bailed was somewhere else entirely, and even there an unwrap explosion (in a smoke test or pre-release pass or whatever) would have been fine.
- crote 10mo ago
  It sounds to me like there was validation, but the system wasn't designed for the validation to ever fail - at which point crashing is the only remaining option. You've essentially turned it into an assertion error rather than a parsing/validation error. Ideally every validation should have a well-defined failure path. In the case of a config file rotation, validation failure of the new config could mean keeping the old config and logging a high-priority error message. In the case of malformed user-provided data, it might mean dropping the request and maybe logging it for security analysis reasons. In the case of "pi suddenly equals 4" checks the most logical approach might be to intentionally crash, as there's obviously something seriously wrong and application state has corrupted in such a way that any attempt to continue is only going to make things worse. But in all cases there's a reason behind the post-validation-failure behavior. At a certain point leaving it up to "whatever happens on .unwrap() failure" isn't good enough anymore.
ChrisMarshallNY 10mo ago
Swift has implicit unwrap (!), and explicit unwrap (?). I don't like to use implicit unwrap. Even things that are guaranteed to be there, I treat as explicit (For example, (self.view?.isEnabled ?? false), in a view controller, instead of self.view.isEnabled). I always redefine @IBOutlets from: @IBOutlet weak var someView! to: @IBOutlet weak var someView? I'm kind of a "belt & suspenders" type of guy.
- monocularvision 10mo ago
  So what happens if it ends up being nil? How does your app react? In this particular case, I would rather crash. It’s easier to spot in a crash report and you get a nice stack trace. Silent failure is ultimately terrible for users. Note: for the things I control I try to very explicitly model state in such a way as I never need to force unwrap at all. But for things beyond my control like this situation, I would rather end the program than continue with a state of the world I don’t understand.
  - Pulcinella 10mo ago
    Yeah @IBOutlets are generally the one thing that are allowed to be implicitly-unwrapped optionals. They go along with using storyboards & xibs files with Interface Builder. I agree that you really should just crash if you are attempting to access one and it is nil. Either you have done something completely incorrect with regards to initializing and accessing parts of your UI and want to catch that in development, or something has gone horribly, horribly, horribly with UIKit/AppKit and storyboard/xib files are not being loaded properly by the system.
    - ChrisMarshallNY 10mo ago
      > … you really should just crash if … See my above/below comment. A good tool for catching stuff during development, is the humble assert()[0]. We can use precondition()[1], to do the same thing, in ship code. The main thing is, is to remain in control, as much as possible. Rather than let the PC leave the stack frame, throw the error immediately when it happens. [0] https://docs.swift.org/swift-book/documentation/the-swift-programming-language/thebasics/#Debugging-with-Assertions https://docs.swift.org/swift-book/documentation/the-swift-pr... [1] https://docs.swift.org/swift-book/documentation/the-swift-programming-language/thebasics/#Enforcing-Preconditions https://docs.swift.org/swift-book/documentation/the-swift-pr...
smj-edison 10mo ago
Isn't the point of this article that pieces of infrastructure don't go down to root causes, but due to bad combinations of components that are correct individually? After reading "engineering a safer world", I find root cause analysis rather reductionistic, because it wasn't just an unwrap, it was that the payload was larger than normal, because of a query that didn't select by database, because a clickhouse made more databases visible. Hard to say "it was just due to an unwrap" imo. Especially in terms of how to fix an issue going forwards. I think the article lists a lot of good ideas, that aren't just "don't unwrap", like enabling more global kill switches for features, or eliminating the ability for core dumps or other error reports to overwhelm system resources.
- brianpan 10mo ago
  You're right. A good postmortem/root cause analysis would START from "unwrap" and continue from there. You might start with a basic timeline of what happened, then you'd start exploring: why did this change affect so many customers (this would be a line of questioning to find a potential root cause), why did it take so long to discover or recover (this might be multiple lines of questioning), etc.
AgentME 10mo ago
This is assuming that the process could have done anything sensible while it had the malformed feature file. It might be in this case that this was one configuration file of several and maybe the program could have been built to run with some defaults when it finds this specific configuration invalid, but in the general case, if a program expects a configuration file and can't do anything without it, panicking is a normal thing to do. There's no graceful handling (beyond a nice error message) a program like Nginx could do on a syntax error in its config. The real issue is further up the chain where the malformed feature file got created and deployed without better checks.
- aloha2436 10mo ago
  > panicking is a normal thing to do I do not think that if the bot detection model inside your big web proxy has a configuration error it should panic and kill the entire proxy and take 20% of the internet with it. This is a system that should fail gracefully and it didn't. > The real issue Are there single "real issues" with systems this large? There are issues being created constantly (say, unwraps where there shouldn't be, assumptions about the consumers of the database schema) that only become apparent when they line up.
- JeremyNT 10mo ago
  Exactly! Sometimes exploding is simply the least bad option, and is an entirely sensible approach.
  - jgilias 10mo ago
    In this case it definitely wasn’t the least bad option though.
- WD-42 10mo ago
  Yea, Rust is safe but it’s not magic. However Nginx doesn’t panic on malformed config. It exits with hopefully a helpful error code and message. The question is then could the cloudflare code have exited cleanly in a way that made recovery easier instead of just straight panicking.
butvacuum 10mo ago
It rang more as "A/B deployments are pointless if you can't tell if a downstream failure is related." To me.
nrhrjrjrjtntbt 10mo ago
I wonder what happens if they handle it gracefully? sounds like performance degradation (better than reliability degradation!). Also wonder with a sharded system why are they not slow rolling out changes and monitoring?
ironman1478 10mo ago
I'm not a fan of rust, but I don't think that is the only takeaway. All systems have assumptions about their input and if the assumption is violated, it has to be caught somewhere. It seems like it was caught too deep in the system. Maybe the validation code should've handled the larger size, but also the db query produced something invalid. That shouldn't have ever happened in the first place.
- asa400 10mo ago
  > It seems like it was caught too deep in the system. Agreed, that's also my takeaway. I don't see the problem being "lazy programmers shouldn't have called .unwrap()". That's reductive. This is a complex system and complex system failures aren't monocausal. The function in question could have returned a smarter error rather than panicking, but what then? An invariant was violated, and maybe this system, at this layer, isn't equipped to take any reasonable action in response to that invariant violation and dying _is_ the correct thing to do. But maybe it could take smarter action. Maybe it could be restarted into a known good state. Maybe this service could be supervised by another system that would have propagated its failure back to the source of the problem, alerting operators that a file was being generated in such a way that violated consumer invariants. Basically, I'm describing a more Erlang model of failure. Regardless, a system like this should be able to tolerate (or at least correctly propagate) a panic in response to an invariant violation.
- 9rx 10mo ago
  The takeaway here isn’t about Rust itself, but that the Rust marketing crew’s claims that we constantly read on HN and elsewhere about the Result type magically saving you from making mistakes is not a good message to send.
  - tuetuopay 10mo ago
    They would also tell you that .unwrap() has no place in production code, and should receive as much scrutiny as an `unsafe` block in code review :) The point of option is the crash path is more verbose and explicit than the crash-free path. It takes more code to check for NULL in C or nil in Go; it takes more code in Rust to not check for Err.
guluarte 10mo ago
it's usually because of fail fast and fail hard, in theory critical bugs will be caught in dev/test
jcalvinowens 10mo ago
> This is the multi-million dollar .unwrap() story. That's too semantic IMHO. The failure mode was "enforced invariant stopped being true". If they'd written explicit code to fail the request when that happened, the end result would have been exactly the same.
- echelon 10mo ago
  [flagged]
  - abigailphoebe 10mo ago
    blaming the language is not the way to approach this. if an engineer writes bad code that’s the engineers fault, not the languages. this was bad code that should have never hit production, it is not a rust language issue.
    - echelon 10mo ago
      No. Don't say "you're holding it wrong". The language says "safe" on the tin. It advertises safety. This shouldn't be possible. This is a null pointer. In Rust. Unwrap needs to die. We should all fight to remove it.
      - dafelst 10mo ago
        panics are safe, what are you talking about? It is nothing like a null pointer.
      - aw1621107 10mo ago
        > The language says "safe" on the tin. It advertises safety. Rust advertises memory safety (and other closely related things, like no UB, data race safety, etc.). I don't think it's made any promises about hard guarantees of other kinds of safety.
      - abigailphoebe 10mo ago
        you either misunderstand the rust ethos or are intentionally misrepresenting it. safe refers to memory safety. once again, if you write bad code, that’s your fault, not the languages. this is a feature of rust that was used incorrectly.
antonvs 10mo ago
> This is textbook "parse, don't validate" anti-pattern. How so? “Parse, don’t validate” implies converting input into typed values that prevent representation of invalid state. But the parsing still needs to be done correctly. An unchecked unwrap really has nothing to do with this.
- kccqzy 10mo ago
  GP completely misunderstands “parse, don’t validate” and also calls it an anti-pattern. GP clearly has no idea what this is.
rafaelmn 10mo ago
That's such a bad take after reading the article. If you're going to write a system that preallocates and is based on hard assumptions about max size - the panic/unwrap approach is reasonable. The config bug reaching prod without this being caught and pinpointed immediately is the strange part.
- kevin_thibedeau 10mo ago
  It's reasonable when testing protocols exercise the panic scenario. This is the problem with punting on error recovery. Nobody checks faults that propagate across domains of responsibility.
- AtNightWeCode 10mo ago
  Exactly. The newbie mistake in SQL is also way worse than this. But the whole design is also bad. Clearly implementing things at the wrong place. And, it took like over an hour between the problem started til my sites went down. That is just crazy.
- thatoneengineer 10mo ago
  I agree there's no way to soft-error this, though "truncate and raise an alert" is arguably the better pattern.
slanterns 10mo ago
> Today, many friends pinged me saying Cloudflare was down. As a core developer of the first generation of Cloudflare FL, I'd like to share some thoughts. > This wasn't an attack, but a classic chain reaction triggered by “hidden assumptions + configuration chains” — permission changes exposed underlying tables, doubling the number of lines in the generated feature file. This exceeded FL2's memory preset, ultimately pushing the core proxy into panic. > Rust mitigates certain errors, but the complexity in boundary layers, data flows, and configuration pipelines remains beyond the language's scope. The real challenge lies in designing robust system contracts, isolation layers, and fail-safe mechanisms. > Hats off to Cloudflare's engineers—those on the front lines putting out fires bear the brunt of such incidents. > Technical details: Even handling the unwrap correctly, an OOM would still occur. The primary issue was the lack of contract validation in feature ingest. The configuration system requires “bad → reject, keep last-known-good” logic. > Why did it persist so long? The global kill switch was inadequate, preventing rapid circuit-breaking. Early suspicion of an attack also caused delays. > Why not roll back software versions or restart? > Rollback isn't feasible because this isn't a code issue—it's a continuously propagating bad configuration. Without version control or a kill switch, restarting would only cause all nodes to load the bad config faster and accelerate crashes. > Why not roll back the configuration? > Configuration lacks versioning and functions more like a continuously updated feed. As long as the ClickHouse pipeline remains active, manually rolling back would result in new corrupted files being regenerated within minutes, overwriting any fixes. https://x.com/guanlandai/status/1990967570011468071 https://x.com/guanlandai/status/1990967570011468071
- anonymous908213 10mo ago
  This tweet thread invokes genuine despair in me. Do we really have to outsource even our tweets to LLMs? Really? I mean, I get spambots and the like tweeting mass-produced slop. But what compels a former engineer of the company in question to offer LLM-generated "insight" to the outage? Why? For what purpose? * For clarity, I am aware that the original tweets are written in Chinese, and they still have the stench of LLM writing all over them; it's not just the translation provided in the above comment.
  - 10mo ago
abalone 10mo ago
I’ve led multiple incident responses at a FAANG, here’s my take. The fundamental problem here is not Rust or the coding error. The problem is: 1. Their bot management system is designed to push a configuration out to their entire network rapidly. This is necessary so they can rapidly respond to attacks, but it creates risk as compared to systems that roll out changes gradually. 2. Despite the elevated risk of system wide rapid config propagation, it took them 2 hours to identify the config as the proximate cause, and another hour to roll it back. SOP for stuff breaking is you roll back to a known good state. If you roll out gradually and your canaries break, you have a clear signal to roll back. Here was a special case where they needed their system to rapidly propagate changes everywhere, which is a huge risk, but didn’t quite have the visibility and rapid rollback capability in place to match that risk. While it’s certainly useful to examine the root cause in the code, you’re never going to have defect free code. Reliability isn’t just about avoiding bugs. It’s about understanding how to give yourself clear visibility into the relationship between changes and behavior and the rollback capability to quickly revert to a known good state. Cloudflare has done an amazing job with availability for many years and their Rust code now powers 20% of internet traffic. Truly a great team.
- ignoramous 10mo ago
  > Their bot management system is designed to push a configuration out to their entire network rapidly. Once every 5m is not "rapidly". It isn't uncommon for configuration systems to do it every few seconds [0]. > While it’s certainly useful to examine the root cause in the code. Believe the issue is as much an output from a periodic run (clickhouse query) caused by (on the surface, an unrelated change) causing this failure. That is, the system that validated the configuration (FL2) was different to the one that generated it (ML Bot Management DB). Ideally, it is the system that vends a complex configuration that also vends & tests the library to consume it, or the system that consumes it, does so as if it was "tasting" the configuration first before devouring it unconditionally [1]. Of course, as with all distributed system failures, this is all easier said and done in hindsight. [0] Avoiding overload in distributed systems by putting the smaller service in control (pg 4), https://d1.awsstatic.com/builderslibrary/pdfs/Avoiding%20overload%20in%20distributed%20systems%20by%20putting%20the%20smaller%20service%20in%20control-Joe%20Magerramov.pdf https://d1.awsstatic.com/builderslibrary/pdfs/Avoiding%20ove... [1] Lessons from CloudFront (2016), https://youtube.com/watch?v=n8qQGLJeUYA&t=1050 https://youtube.com/watch?v=n8qQGLJeUYA&t=1050
hoppp 10mo ago
You write so much rust you causally apply unwrap now to everything? Rust compiler is a god of sorts, or at least a law of nature haha Way to comment and go instantly off topic
throwaway38294 10mo ago
This is a bummer. The unwrap()'ing function already returned a result and should have just propagated the error. Presumably the caller could have handled more sensibly than just panic'ing.
ozgrakkurt 10mo ago
Not panicking code is tedious to write. It is not realistic to expect everything to be non panic. There is a reason that panicking exists in the first place. Them calling unwrap on a limit check is the real issue imo. Everything that takes in external input should assume it is bad input and should be fuzz tested imo. In the end, what is the point of having a limit check if you are just unwrapping on it
- cube00 10mo ago
  > Not panicking code is tedious to write. Using the question mark operator [1] and even adding in some anyhow::context goes a long way to being able to fail fast and return an Err rather then panicking. Sure you need to handle Results all the way up the stack but it forces you to think about how those nested parts of your app will fail as you travel back up the stack. [1]: https://doc.rust-lang.org/rust-by-example/std/result/question_mark.html https://doc.rust-lang.org/rust-by-example/std/result/questio...
pjmlp 10mo ago
Which is something I will bookmark for the usual Rust doesn't do exceptions discussions, except it kind of does even if called differently.
karel-3d 10mo ago
As a gopher I never understand why is there so many unwraps in an average rust code. Average Go code has much less panics than Rust has unwraps, which are functionally equivalent.
- richardwhiuk 10mo ago
  Because Go silently gives you zero/null instead
  - ergocoder 10mo ago
    which mean an unexpected behavior could go unnoticed for a long time. I'd prefer a loud crash over that.
    - karel-3d 10mo ago
      Well look at the failure modes in the original article. In the original PHP code, all worked, only it didn't properly check for bots. The new Rust code did a loud crash and took off half of the internet.
  - karel-3d 10mo ago
    Idiomatically, it gives you `err` and you do `if err != nil {return err}`. While in rust you mostly do `.unwrap` and panic. It's not in the type system, but it's idiomatic
    - richardwhiuk 10mo ago
      Get a key from a map and forget to check the error.
- speedgoose 10mo ago
  The average golang code segfaults by design.
- selfmodruntime 10mo ago
  I love Go and write a ton of it. I've had real segfaults quite a lot.
branko_d 10mo ago
Safe things should be easy, dangerous things should be hard. This .unwrap() sounds too easy for what it does, certainly much easier than having an entire try..catch block with an explicit panic. Full disclosure: I don't actually know Rust.
- kettlecorn 10mo ago
  I don't think 'unwrap' is inherently the problem. Any project has to reason about what sort of errors can be tolerated gracefully and which cannot. Unwrap is reasonable in scenarios you expect to never be reached, because otherwise your code will be full of all sorts of possible permutations and paths that are harder to reason about and may cascade into extremely nuanced or subtle errors. Rust also has a version of unwrap called "expect" where you provide a string that logs why the unwrap occurred. It's similar, but for pieces of code that are crucial it could be a good idea to require all 'unwraps' to instead be 'expects' so that people at least are forced to write down a reason why they believe the unwrap can never be reached.
__bax 10mo ago
git blame on .unwrap() line
selfmodruntime 10mo ago
While this is true, I wish that Rust had more of a first-class support for `no_panic`. Every solution we do have is hacky. I wish that I could guarantee that there were no panic calls anywhere in a code path.
gwd 10mo ago
> This is the multi-million dollar .unwrap() story. While there are certainly many things to admire about Rust, this is why I prefer Golang's "noisy" error handling. In golang that would be either: feature_values, err := features.append_with_names(...) And the compiler would have complained that this value of `err` was unused; or you'd write: feature_values, _ := features.append_with_names(...) And it would be far more obvious that an error message is being ignored. (Renaming `unwrap` to `unwrapOrPanic` would probably help too.)
- mamp 10mo ago
  I haven't been writing Rust for that long (about 2 years) but every time I see .unwrap() I read it as 'panic in production'. Clippy needs to have harder checks on unwrap.
- zero_shift 10mo ago
  But I could screw it up in Go, if I made the same assumptions fvs, err := features.AppendWithNames(..) if err != nil { // this will NEVER break panic(err) } Ultimately I don't think language design can be the sole line of defence against system failures; it can only guide developers to think about error cases
  - gwd 10mo ago
    Right, but the point isn't to make errors impossible; the point is to have them be 1) less likely to write, and 2) easier to spot on review. People's biggest complaints about golang's errors: 1. You have to _TYPE_OUT_ what to do on EVERY.SINGLE.ERROR. SOO BOORING! 2. They clutter up the code and make it look ugly. Rust is so much cleaner and more convenient (they say)! Just add ?, or .unwrap()! Well, with ".unwrap()", you can type it fast enough that you're on to the next problem before it occurs to your brain to think about what to do if there is an error. Whereas, in golang, by the time you type in, "if err != nil {", you've broken the flow enough that now you're much more likely to be thinking, "Hmm, could this ever fail? What should we do if it does?" That break in flow is annoying, but necessary. And ".unwrap()" looks so unassuming, it's easy to overlook on review; that "panic()" looks a lot more dangerous, and again, would be more likely to trigger a reviewer into thinking, "Wait, is it OK if this thing panics? Is this really so unlikely to happen?" Renaming it `.unwrap_or_panic()` would probably help with both.
twhitmore 10mo ago
Interesting to see Rust error handling flunk out in practice. It may be that forcing handling at every call tends to makes code verbose, and devs insensitized to bad practice. And the diagnostic Rust provided seems pretty garbage. There is bad practice here too -- config failure manifesting as request failure, lack of failing to safe, unsafe rollout, lack of observability. Back to language design & error handling. My informed view is that robustness is best when only major reliability boundaries need to be coded. This the "throw, don't catch" principle with the addition of catches on key reliability boundaries -- typically high-level interactions where you can meaningfully answer a failure. For example, this system could have a total of three catch clauses "Error Loading Config" which fails to safe, "Error Handling Request" which answers 5xx, and "Socket Error" which closes the HTTP connection.
- Ciantic 10mo ago
  > It may be that forcing handling at every call tends to makes code verbose Rust has a lot of helpers to make it less verbose, even that error they demonstrate could've been written in some form `...code()?` with `?` helper that would have propagated the error forwards. However I do acknowledge that writing Error types is boring sometimes so people don't bother to change their error types and just unwrap. But even my dinghy little apps for my personal use I do simple serach `unwrap` and make sure I have as few as possible.
- hypeatei 10mo ago
  I don't understand how your takeaway is that this is a language flaw other than to assume that you have some underlying disdain for Rust. That's fine, but state it clearly please. The end result would've been the exact same if they "handled" the error: a bunch of 500s. The language being used doesn't matter if an invariant in your system is broken.
andy_ppp 10mo ago
This is why the Erlang/Elixir methodology of having supervision and letting things crash gracefully is so useful. You can either handle every single error gracefully or handle crashing gracefully - it's much easier and more realistic in large codebases to do the later.
- tuetuopay 10mo ago
  This would not have helped: the code would crash before doing anything useful at all. If anything, the "crash early" mentality may even be nefarious: instead of handling the error and keeping the old config, you would spin on trying to load a broken config on startup.
  - asa400 10mo ago
    Continuing only makes sense for cases you know you can handle. _In theory_ they could have used the old config, but maybe there are reasons that’s not possible in Cloudflare’s setup. Whether or not that’s an invariant violation or just an error that can be handled and recovered from is a matter of opinion in system design. And crashing on an invariant violation is exactly the right thing to do rather than proceed in an undefined state.
    - tuetuopay 10mo ago
      Given the context and what the configuration file contains, I'd argue it's mission-critical for the software to keep running with the previous configuration. Otherwise you might shutdown the internet. Honestly, I'm pretty sure their pre-rewrite version had such logic, and it was forgotten or still on the TODO pile for the rewrite version. At a previous job (cloud provider), we've had exactly this kind of issue, with exactly the same root cause. The entrypoint for the whole network had a set of rules (think a NAT gateway) that were reloaded periodically from the database. Someone rewrote that bit of plumbing from Python to Go. Someone else performed a database migration. Suddenly, the plumbing could not find the data, and pushed an empty file to prod. The rewrite lacked "if empty, do nothing and raise an alert", that the previous one had. I'll let you imagine what happened next :)
NoboruWataya 10mo ago
They should link this article in the docs for `unwrap()`.
sphericalkat 10mo ago
Handling the error still would've returned a 5xx in this case, since the config file was still over the limit of features the service could handle.
BrtByte 10mo ago
Feels like a case where safety guarantees of Rust lulled them into thinking the edge cases were covered
peanut-walrus 10mo ago
I wonder if similar to infrastructure resilience, code resilience is also required for critical services that can never go down? Instead of relying on a single implementation for a critical service, have multiple independent implementations in different languages. Back when I was running my own DNS servers, I did always ensure that primary and secondary were running on different platforms and different software.
meltyness 10mo ago
tokio default behavior within a task is to ignore panics, such as an Err/None unwrap, and only crash that task, so it's impact limited so that's nice, maybe that's where the snowblindness came from. it'd be kinda hard to amend the clippy lints to ignore coroutine unwraps but still pipe up on system ones. i guess. edit: i think they'd have to be "solely-task-color-flavored" so definitely probably not trivial to infer
quotemstr 10mo ago
If the error had been an exception instead of a result, could have bubbled up I have been saying for years that Rust botched error handling in unfixable ways. I will go to the grave believing Rust fumbled. The design of the Rust language encourages people to use unwrap() to turn foreseeable runtime problems into fatal errors. It's the path of least resistance, so people will take it. Rust encourages developers to consider only the happy path. No wonder it's popular among people who've never had to deal with failure. All of the concomitant complexity--- Result, ?, the test thing, anyhow, the inability for stdlib to report allocation failure --- is downstream of a fashion statement against exceptions Rust cargo-culted from Go. The funniest part is that Rust does have exceptions. It just calls them panics. So Rust code has to deal with the ergonomic footgun of Result but pays anyway for the possibility of exceptions. (Sure, you can compile with panic=abort. You can't count on it.) I could not be more certain that Rust should have been a language with exceptions, not Result, and that error objects are a gross antipattern we'll regret for decades.
- Veliladon 10mo ago
  Errors work just like exceptions especially if you use the ? operator and let the error bubble up the chain. This is the Rust equivalent of an unhandled exception and the ripcord being pulled.
  - quotemstr 10mo ago
    In C++, functions are error-colored by default. You write "noexcept" if you want your function to be infallible-colored instead. (You usually want to make a function infallible if you're using your noexcept function as part of a cleanup path or part of a container interface that allows for more optimizations of it knows certain container operations are infallible.) Rust makes infallibility the syntactic default and makes you write Result to indicate fallibility. People often don't want to color their functions this way. Guess what happens when a programmer is six levels deep in infallible-colored function calls and does something that can fail. .unwrap() Guess what, in Rust, is fallible? Mutex acquire. Guess what you need to do often on infallible cleanup paths? Mutex acquire.
otabdeveloper4 10mo ago
Oh come on, stop spreading FUD. Rust programs are 100% immune to crashes and bugs, they have memory safety (c). Also, exception handling is hard and lame. We don't need exceptions, just add a "match" block after every line in your program.
- JuniperMesos 10mo ago
  What's the point of this sarcastic comment? Do you think that some people claim that Rust's memory safety guarantees mean that a Rust program is incapable of crashing or having a bug? This is a dumb thing to claim certainly, but I'm not aware of anyone actually making this claim. I'm also not sure what you're getting at with the comment about exception handling being lame. I think the ML/Haskell inspired model that Rust uses of having a parameterized Result type for fallible operations is generally better than exceptions for a variety of reasons (although maybe better Exception semantics could help with some of this), but what does this have to do with match blocks?
  - otabdeveloper4 10mo ago
    > Do you think that some people claim that Rust's memory safety guarantees mean that a Rust program is incapable of crashing or having a bug? Undoubtedly yes. > ...but what does this have to do with match blocks? You tell me. You're the one advocating for placing one after every single function call.
echelon 10mo ago
> This is the multi-million dollar .unwrap() story. First multi-million dollar .unwrap() story.
torginus 10mo ago
Say what you want exception haters, but at least in exceptions-as-default languages, the decision of a particular issues is fatal to the whole program can be decided centrally at a high level, and not every choice is forced to be up to individual discretion.
- underdeserver 10mo ago
  But you can do the same thing with Rust, by piping up Results.
torginus 10mo ago
By the way - does this discussion matter and were they wrong to use unwrap()? The way they wrote the code means that having more than 200 features is a hard non-transient error - even if they recovered from it, it meant they'd have had the same error when the code got to the same place. I'm sure when the process crashed, k8s restarted the pod or something - then it reran the same piece of code and crashed in the same place. While I don't necessarily agree with crashing as business strategy, I don't think that doing anything other than either dropping the extra rules or allocating more memory - neither of which the original code was built to do (probably by design). The code made the local hard assumption that there won't ever be more than 200 rules and its okay to crash if that count is exceeded. If you design your code around an invariant never being violated (which is fine), you have to make it clear on a higher level that they did. This isn't a Rust problem (though Rust does make it easy to do the wrong thing here imo)
- grogers 10mo ago
  Instead of crashing when applying the new config, it's more common to simply ignore the new config if it cannot be applied. You keep running in the last known good state. Operators then get alerts about the failures and can diagnose and resolve the underlying issue. That's not always foolproof, e.g. a freshly (re)started process doesn't have any prior state it can fall back to, so it just hard crashes. But restarts are going to be rate limited anyways, so even then there is time to mitigate the issue before it becomes a large scale outage

shadowgovt 10mo ago

In addition, it looks like this system wasn't on any kind of 1%/10%/50%/100% rollout gating. Such a rollout would trivially have shown the poison input killing tasks.

penteract 10mo ago
To me it reads like there was a gradual rollout of the faulty software responsible for generating the config files, but those files are generated on approximately one machine, then propogated across the whole network every 5 minutes. > Bad data was only generated if the query ran on a part of the cluster which had been updated. As a result, every five minutes there was a chance of either a good or a bad set of configuration files being generated and rapidly propagated across the network.
helloericsf 10mo ago
Not a DBA, how do you do DB permission rollout gating?
- shadowgovt 10mo ago
  It looks like changing the permissions triggered creation of a new feature file, and it was ingestion of that file leading to blowing a size limit that crashed the systems. The file should be versioned and rollout of new versions should be staged. (There is definitely a trade-off; often times in the security critical path, you want to go as fast as possible because changes may be blocking a malicious actor. But if you move too fast, you break things. Here, they had a potential poison input in the pathway for synchronizing this state and Murphy's Law suggests it was going to break eventually, so the question becomes "How much damage can we tolerate when it does?")
  - dwattttt 10mo ago
    > It looks like changing the permissions triggered creation of a new feature file, and it was ingestion of that file leading to blowing a size limit that crashed the systems. That feature file is generated every 5 minutes at all times; the change to permissions was rolled out gradually over the clickhouse cluster, and whether a bad version of that file was generated depended on whether the part of the cluster that had the bad permissions generated the file.

wrs 10mo ago

It seems people have a blind spot for unwrap, perhaps because it's so often used in example code. In production code an unwrap or expect should be reviewed exactly like a panic. It's not necessarily invalid to use unwrap in production code if you would just call panic anyway. But just like every unsafe block needs a SAFETY comment, every unwrap in production code needs an INFALLIBILITY comment. clippy::unwrap_used can enforce this.

dist1ll 10mo ago
> every unwrap in production code needs an INFALLIBILITY comment. clippy::unwrap_used can enforce this. How about indexing into a slice/map/vec? Should every `foo[i]` have an infallibility comment? Because they're essentially `get(i).unwrap()`.
- danielheath 10mo ago
  I mean... yeah, in general. That's what iterators are for.
- tux3 10mo ago
  Usually you'd want to write almost all your slice or other container iterations with iterators, in a functional style. For the 5% of cases that are too complex for standard iterators? I never bother justifying why my indexes are correct, but I don't see why not. You very rarely need SAFETY comments in Rust because almost all the code you write is safe in the first place. The language also gives you the tool to avoid manual iteration (not just for safety, but because it lets the compiler eliminate bounds checks), so it would actually be quite viable to write these comments, since you only need them when you're doing something unusual.
  - dist1ll 10mo ago
    For iteration, yes. But there's other cases, like any time you have to deal with lots of linked data structures. If you need high performance, chances are that you'll have to use an index+arena strategy. They're also common in mathematical codebases.

arccy 10mo ago

if you make it easy to be lazy and panic vs properly handling the error, you've designed a poor language

yoyohello13 10mo ago
So… basically every language ever? Except maybe Haskell.
- dkersten 10mo ago
  And Gleam
- yakshaving_jgt 10mo ago
  It's easy to cause this kind of failure in Haskell also.
otterley 10mo ago
https://en.wikipedia.org/wiki/Crash-only_software https://en.wikipedia.org/wiki/Crash-only_software
- nine_k 10mo ago
  Works when you have the Erlang system that does graceful handing for you: reporting, restarting.
SchwKatze 10mo ago
Unwrap isn't a synonym for laziness, it's just like an assertion, when you do unwrap() you're saying the Result should NEVER fail, and if does, it should abort the whole process. What was wrong was the developer assumption, not the use of unwrap.
- dietr1ch 10mo ago
  > What was wrong was the developer assumption, not the use of unwrap. How many times can you truly prove that an `unwrap()` is correct and that you also need that performance edge? Ignoring the performance aspect that often comes from a hat-trick, to prove such a thing you need to be wary of the inner workings of a call giving you a `Return`. That knowledge is only valid at the time of writing your `unwrap()`, but won't necessarily hold later. Also, aren't you implicitly forcing whoever changes the function to check for every smartass dev that decided to `unwrap` at their callsite? That's bonkers.

deleted 10mo ago

[deleted]

cvhc 10mo ago

Some languages and style guides simply forbid throwing exceptions without catching / proper recovery. Google C++ bans exceptions and the main mechanism for propogating errors is `absl::Status` which the caller has to check. Not familiar with Rust but it seems unwrap is such a thing that would be banned.

pdimitar 10mo ago
There are even lints for this but people get impatient and just override them or fight for them to no longer be the default. As usual: people problem, not a tech problem. In the last years a lot of strides have been made. But people will be people.
- tonyhart7 10mo ago
  and people make mistake at some point machine would be better in coding because well machine code is machine instruction task same like chess, engine is better than human grandmaster because its solvable math field coding is no different
  - aw1621107 10mo ago
    > same like chess, engine is better than human grandmaster because its solvable math field Might be worth noting that your description of chess is slightly incorrect. Chess technically isn't solved in the sense that the optimal move is known for any arbitrary position is known; it's just that chess engines are using what amounts to a fancy brute force for most of the game and the combination of hardware and search algorithm produces a better result than the human brain does. As such, chess engines are still capable of making mistakes, even if actually exploiting them is a challenge.
    - tonyhart7 10mo ago
      No ?????? because these thing called BEST MOVE and BAD MOVE there in chess "chess engines are still capable of making mistakes", I'm sorry no inaccurate yes but not mistake

vlovich123 10mo ago

To be fair, this failed in the non-rust path too because the bot management returned that all traffic was a bot. But yes, FL2 needs to catch panics from individual components but I’m not sure if failing open is necessarily that much better (it was in this case but the next incident could easily be the result of failing open). But more generally you could catch the panic at the FL2 layer to make that decision intentional - missing logic at that layer IMHO.

hedora 10mo ago
Catching panic probably isn’t a great idea if there’s any unsafe code in the system. (Do the unsafe blocks really maintain heap invariants if across panics?)
- vlovich123 10mo ago
  Unsafe blocks have nothing to do with it. Yes - they maintain all the same invariants as safe blocks or those unsafe blocks are unsound regardless of panics. But there’s millions of way to architect this (eg a supervisor process that notices which layer in FL2 is crashing and just completely disables that layer when it starts up the proxy again. There’s challenges here because then you have to figure out what constitutes a perma crashing (eg what if it’s just 20% of all sites? Do you disable?). And in the general case you have the fail open/fail close decision anyway which you should just annotate individual layers with. But the bigger change is to make sure that config changes roll out gradually instead of all at once. That’s the source of 99% of all widespread outages
  - Feathercrown 10mo ago
    Incremental config changes sounds like it could lead to a LOT of bugs
    - vlovich123 10mo ago
      Incremental in terms of 1% of the fleet using it, then 5% etc. this is standard course. Another option is to make sure that config changes that fail to parse continue using the old config instead of resulting in an unusable service.

ajross 10mo ago

I'm not completely sure I agree. I mean, I do agree about the .unwrap() culture being a bug trap. But I don't think this example qualifies. The root cause here was that a file was mildly corrupt (with duplicate entries, I guess). And there was a validation check elsewhere that said "THIS FILE IS TOO BIG". But if that's a validation failure, well, failing is correct? What wasn't correct was that the failure reached production. What should have happened is that the validation should have been a unified thing and whatever generated the file should have flagged it before it entered production. And that's not an issue with function return value API management. The software that should have bailed was somewhere else entirely, and even there an unwrap explosion (in a smoke test or pre-release pass or whatever) would have been fine.

crote 10mo ago
It sounds to me like there was validation, but the system wasn't designed for the validation to ever fail - at which point crashing is the only remaining option. You've essentially turned it into an assertion error rather than a parsing/validation error. Ideally every validation should have a well-defined failure path. In the case of a config file rotation, validation failure of the new config could mean keeping the old config and logging a high-priority error message. In the case of malformed user-provided data, it might mean dropping the request and maybe logging it for security analysis reasons. In the case of "pi suddenly equals 4" checks the most logical approach might be to intentionally crash, as there's obviously something seriously wrong and application state has corrupted in such a way that any attempt to continue is only going to make things worse. But in all cases there's a reason behind the post-validation-failure behavior. At a certain point leaving it up to "whatever happens on .unwrap() failure" isn't good enough anymore.

ChrisMarshallNY 10mo ago

Swift has implicit unwrap (!), and explicit unwrap (?). I don't like to use implicit unwrap. Even things that are guaranteed to be there, I treat as explicit (For example, (self.view?.isEnabled ?? false), in a view controller, instead of self.view.isEnabled). I always redefine @IBOutlets from: @IBOutlet weak var someView! to: @IBOutlet weak var someView? I'm kind of a "belt & suspenders" type of guy.

monocularvision 10mo ago
So what happens if it ends up being nil? How does your app react? In this particular case, I would rather crash. It’s easier to spot in a crash report and you get a nice stack trace. Silent failure is ultimately terrible for users. Note: for the things I control I try to very explicitly model state in such a way as I never need to force unwrap at all. But for things beyond my control like this situation, I would rather end the program than continue with a state of the world I don’t understand.
- Pulcinella 10mo ago
  Yeah @IBOutlets are generally the one thing that are allowed to be implicitly-unwrapped optionals. They go along with using storyboards & xibs files with Interface Builder. I agree that you really should just crash if you are attempting to access one and it is nil. Either you have done something completely incorrect with regards to initializing and accessing parts of your UI and want to catch that in development, or something has gone horribly, horribly, horribly with UIKit/AppKit and storyboard/xib files are not being loaded properly by the system.
  - ChrisMarshallNY 10mo ago
    > … you really should just crash if … See my above/below comment. A good tool for catching stuff during development, is the humble assert()[0]. We can use precondition()[1], to do the same thing, in ship code. The main thing is, is to remain in control, as much as possible. Rather than let the PC leave the stack frame, throw the error immediately when it happens. [0] https://docs.swift.org/swift-book/documentation/the-swift-programming-language/thebasics/#Debugging-with-Assertions https://docs.swift.org/swift-book/documentation/the-swift-pr... [1] https://docs.swift.org/swift-book/documentation/the-swift-programming-language/thebasics/#Enforcing-Preconditions https://docs.swift.org/swift-book/documentation/the-swift-pr...

smj-edison 10mo ago

Isn't the point of this article that pieces of infrastructure don't go down to root causes, but due to bad combinations of components that are correct individually? After reading "engineering a safer world", I find root cause analysis rather reductionistic, because it wasn't just an unwrap, it was that the payload was larger than normal, because of a query that didn't select by database, because a clickhouse made more databases visible. Hard to say "it was just due to an unwrap" imo. Especially in terms of how to fix an issue going forwards. I think the article lists a lot of good ideas, that aren't just "don't unwrap", like enabling more global kill switches for features, or eliminating the ability for core dumps or other error reports to overwhelm system resources.

brianpan 10mo ago
You're right. A good postmortem/root cause analysis would START from "unwrap" and continue from there. You might start with a basic timeline of what happened, then you'd start exploring: why did this change affect so many customers (this would be a line of questioning to find a potential root cause), why did it take so long to discover or recover (this might be multiple lines of questioning), etc.

AgentME 10mo ago

This is assuming that the process could have done anything sensible while it had the malformed feature file. It might be in this case that this was one configuration file of several and maybe the program could have been built to run with some defaults when it finds this specific configuration invalid, but in the general case, if a program expects a configuration file and can't do anything without it, panicking is a normal thing to do. There's no graceful handling (beyond a nice error message) a program like Nginx could do on a syntax error in its config. The real issue is further up the chain where the malformed feature file got created and deployed without better checks.

aloha2436 10mo ago
> panicking is a normal thing to do I do not think that if the bot detection model inside your big web proxy has a configuration error it should panic and kill the entire proxy and take 20% of the internet with it. This is a system that should fail gracefully and it didn't. > The real issue Are there single "real issues" with systems this large? There are issues being created constantly (say, unwraps where there shouldn't be, assumptions about the consumers of the database schema) that only become apparent when they line up.
JeremyNT 10mo ago
Exactly! Sometimes exploding is simply the least bad option, and is an entirely sensible approach.
- jgilias 10mo ago
  In this case it definitely wasn’t the least bad option though.
WD-42 10mo ago
Yea, Rust is safe but it’s not magic. However Nginx doesn’t panic on malformed config. It exits with hopefully a helpful error code and message. The question is then could the cloudflare code have exited cleanly in a way that made recovery easier instead of just straight panicking.

butvacuum 10mo ago

It rang more as "A/B deployments are pointless if you can't tell if a downstream failure is related." To me.

nrhrjrjrjtntbt 10mo ago

I wonder what happens if they handle it gracefully? sounds like performance degradation (better than reliability degradation!). Also wonder with a sharded system why are they not slow rolling out changes and monitoring?

ironman1478 10mo ago

I'm not a fan of rust, but I don't think that is the only takeaway. All systems have assumptions about their input and if the assumption is violated, it has to be caught somewhere. It seems like it was caught too deep in the system. Maybe the validation code should've handled the larger size, but also the db query produced something invalid. That shouldn't have ever happened in the first place.

asa400 10mo ago
> It seems like it was caught too deep in the system. Agreed, that's also my takeaway. I don't see the problem being "lazy programmers shouldn't have called .unwrap()". That's reductive. This is a complex system and complex system failures aren't monocausal. The function in question could have returned a smarter error rather than panicking, but what then? An invariant was violated, and maybe this system, at this layer, isn't equipped to take any reasonable action in response to that invariant violation and dying _is_ the correct thing to do. But maybe it could take smarter action. Maybe it could be restarted into a known good state. Maybe this service could be supervised by another system that would have propagated its failure back to the source of the problem, alerting operators that a file was being generated in such a way that violated consumer invariants. Basically, I'm describing a more Erlang model of failure. Regardless, a system like this should be able to tolerate (or at least correctly propagate) a panic in response to an invariant violation.
9rx 10mo ago
The takeaway here isn’t about Rust itself, but that the Rust marketing crew’s claims that we constantly read on HN and elsewhere about the Result type magically saving you from making mistakes is not a good message to send.
- tuetuopay 10mo ago
  They would also tell you that .unwrap() has no place in production code, and should receive as much scrutiny as an `unsafe` block in code review :) The point of option is the crash path is more verbose and explicit than the crash-free path. It takes more code to check for NULL in C or nil in Go; it takes more code in Rust to not check for Err.

guluarte 10mo ago

it's usually because of fail fast and fail hard, in theory critical bugs will be caught in dev/test

jcalvinowens 10mo ago

> This is the multi-million dollar .unwrap() story. That's too semantic IMHO. The failure mode was "enforced invariant stopped being true". If they'd written explicit code to fail the request when that happened, the end result would have been exactly the same.

echelon 10mo ago
[flagged]
- abigailphoebe 10mo ago
  blaming the language is not the way to approach this. if an engineer writes bad code that’s the engineers fault, not the languages. this was bad code that should have never hit production, it is not a rust language issue.
  - echelon 10mo ago
    No. Don't say "you're holding it wrong". The language says "safe" on the tin. It advertises safety. This shouldn't be possible. This is a null pointer. In Rust. Unwrap needs to die. We should all fight to remove it.
    - dafelst 10mo ago
      panics are safe, what are you talking about? It is nothing like a null pointer.
    - aw1621107 10mo ago
      > The language says "safe" on the tin. It advertises safety. Rust advertises memory safety (and other closely related things, like no UB, data race safety, etc.). I don't think it's made any promises about hard guarantees of other kinds of safety.
    - abigailphoebe 10mo ago
      you either misunderstand the rust ethos or are intentionally misrepresenting it. safe refers to memory safety. once again, if you write bad code, that’s your fault, not the languages. this is a feature of rust that was used incorrectly.

antonvs 10mo ago

> This is textbook "parse, don't validate" anti-pattern. How so? “Parse, don’t validate” implies converting input into typed values that prevent representation of invalid state. But the parsing still needs to be done correctly. An unchecked unwrap really has nothing to do with this.

kccqzy 10mo ago
GP completely misunderstands “parse, don’t validate” and also calls it an anti-pattern. GP clearly has no idea what this is.

rafaelmn 10mo ago

That's such a bad take after reading the article. If you're going to write a system that preallocates and is based on hard assumptions about max size - the panic/unwrap approach is reasonable. The config bug reaching prod without this being caught and pinpointed immediately is the strange part.

kevin_thibedeau 10mo ago
It's reasonable when testing protocols exercise the panic scenario. This is the problem with punting on error recovery. Nobody checks faults that propagate across domains of responsibility.
AtNightWeCode 10mo ago
Exactly. The newbie mistake in SQL is also way worse than this. But the whole design is also bad. Clearly implementing things at the wrong place. And, it took like over an hour between the problem started til my sites went down. That is just crazy.
thatoneengineer 10mo ago
I agree there's no way to soft-error this, though "truncate and raise an alert" is arguably the better pattern.

slanterns 10mo ago

> Today, many friends pinged me saying Cloudflare was down. As a core developer of the first generation of Cloudflare FL, I'd like to share some thoughts. > This wasn't an attack, but a classic chain reaction triggered by “hidden assumptions + configuration chains” — permission changes exposed underlying tables, doubling the number of lines in the generated feature file. This exceeded FL2's memory preset, ultimately pushing the core proxy into panic. > Rust mitigates certain errors, but the complexity in boundary layers, data flows, and configuration pipelines remains beyond the language's scope. The real challenge lies in designing robust system contracts, isolation layers, and fail-safe mechanisms. > Hats off to Cloudflare's engineers—those on the front lines putting out fires bear the brunt of such incidents. > Technical details: Even handling the unwrap correctly, an OOM would still occur. The primary issue was the lack of contract validation in feature ingest. The configuration system requires “bad → reject, keep last-known-good” logic. > Why did it persist so long? The global kill switch was inadequate, preventing rapid circuit-breaking. Early suspicion of an attack also caused delays. > Why not roll back software versions or restart? > Rollback isn't feasible because this isn't a code issue—it's a continuously propagating bad configuration. Without version control or a kill switch, restarting would only cause all nodes to load the bad config faster and accelerate crashes. > Why not roll back the configuration? > Configuration lacks versioning and functions more like a continuously updated feed. As long as the ClickHouse pipeline remains active, manually rolling back would result in new corrupted files being regenerated within minutes, overwriting any fixes. https://x.com/guanlandai/status/1990967570011468071 https://x.com/guanlandai/status/1990967570011468071

anonymous908213 10mo ago
This tweet thread invokes genuine despair in me. Do we really have to outsource even our tweets to LLMs? Really? I mean, I get spambots and the like tweeting mass-produced slop. But what compels a former engineer of the company in question to offer LLM-generated "insight" to the outage? Why? For what purpose? * For clarity, I am aware that the original tweets are written in Chinese, and they still have the stench of LLM writing all over them; it's not just the translation provided in the above comment.
- 10mo ago

abalone 10mo ago

I’ve led multiple incident responses at a FAANG, here’s my take. The fundamental problem here is not Rust or the coding error. The problem is: 1. Their bot management system is designed to push a configuration out to their entire network rapidly. This is necessary so they can rapidly respond to attacks, but it creates risk as compared to systems that roll out changes gradually. 2. Despite the elevated risk of system wide rapid config propagation, it took them 2 hours to identify the config as the proximate cause, and another hour to roll it back. SOP for stuff breaking is you roll back to a known good state. If you roll out gradually and your canaries break, you have a clear signal to roll back. Here was a special case where they needed their system to rapidly propagate changes everywhere, which is a huge risk, but didn’t quite have the visibility and rapid rollback capability in place to match that risk. While it’s certainly useful to examine the root cause in the code, you’re never going to have defect free code. Reliability isn’t just about avoiding bugs. It’s about understanding how to give yourself clear visibility into the relationship between changes and behavior and the rollback capability to quickly revert to a known good state. Cloudflare has done an amazing job with availability for many years and their Rust code now powers 20% of internet traffic. Truly a great team.

ignoramous 10mo ago
> Their bot management system is designed to push a configuration out to their entire network rapidly. Once every 5m is not "rapidly". It isn't uncommon for configuration systems to do it every few seconds [0]. > While it’s certainly useful to examine the root cause in the code. Believe the issue is as much an output from a periodic run (clickhouse query) caused by (on the surface, an unrelated change) causing this failure. That is, the system that validated the configuration (FL2) was different to the one that generated it (ML Bot Management DB). Ideally, it is the system that vends a complex configuration that also vends & tests the library to consume it, or the system that consumes it, does so as if it was "tasting" the configuration first before devouring it unconditionally [1]. Of course, as with all distributed system failures, this is all easier said and done in hindsight. [0] Avoiding overload in distributed systems by putting the smaller service in control (pg 4), https://d1.awsstatic.com/builderslibrary/pdfs/Avoiding%20overload%20in%20distributed%20systems%20by%20putting%20the%20smaller%20service%20in%20control-Joe%20Magerramov.pdf https://d1.awsstatic.com/builderslibrary/pdfs/Avoiding%20ove... [1] Lessons from CloudFront (2016), https://youtube.com/watch?v=n8qQGLJeUYA&t=1050 https://youtube.com/watch?v=n8qQGLJeUYA&t=1050

hoppp 10mo ago

You write so much rust you causally apply unwrap now to everything? Rust compiler is a god of sorts, or at least a law of nature haha Way to comment and go instantly off topic

throwaway38294 10mo ago

This is a bummer. The unwrap()'ing function already returned a result and should have just propagated the error. Presumably the caller could have handled more sensibly than just panic'ing.

ozgrakkurt 10mo ago

Not panicking code is tedious to write. It is not realistic to expect everything to be non panic. There is a reason that panicking exists in the first place. Them calling unwrap on a limit check is the real issue imo. Everything that takes in external input should assume it is bad input and should be fuzz tested imo. In the end, what is the point of having a limit check if you are just unwrapping on it

cube00 10mo ago
> Not panicking code is tedious to write. Using the question mark operator [1] and even adding in some anyhow::context goes a long way to being able to fail fast and return an Err rather then panicking. Sure you need to handle Results all the way up the stack but it forces you to think about how those nested parts of your app will fail as you travel back up the stack. [1]: https://doc.rust-lang.org/rust-by-example/std/result/question_mark.html https://doc.rust-lang.org/rust-by-example/std/result/questio...

pjmlp 10mo ago

Which is something I will bookmark for the usual Rust doesn't do exceptions discussions, except it kind of does even if called differently.

karel-3d 10mo ago

As a gopher I never understand why is there so many unwraps in an average rust code. Average Go code has much less panics than Rust has unwraps, which are functionally equivalent.

richardwhiuk 10mo ago
Because Go silently gives you zero/null instead
- ergocoder 10mo ago
  which mean an unexpected behavior could go unnoticed for a long time. I'd prefer a loud crash over that.
  - karel-3d 10mo ago
    Well look at the failure modes in the original article. In the original PHP code, all worked, only it didn't properly check for bots. The new Rust code did a loud crash and took off half of the internet.
- karel-3d 10mo ago
  Idiomatically, it gives you `err` and you do `if err != nil {return err}`. While in rust you mostly do `.unwrap` and panic. It's not in the type system, but it's idiomatic
  - richardwhiuk 10mo ago
    Get a key from a map and forget to check the error.
speedgoose 10mo ago
The average golang code segfaults by design.
selfmodruntime 10mo ago
I love Go and write a ton of it. I've had real segfaults quite a lot.

branko_d 10mo ago

Safe things should be easy, dangerous things should be hard. This .unwrap() sounds too easy for what it does, certainly much easier than having an entire try..catch block with an explicit panic. Full disclosure: I don't actually know Rust.

kettlecorn 10mo ago
I don't think 'unwrap' is inherently the problem. Any project has to reason about what sort of errors can be tolerated gracefully and which cannot. Unwrap is reasonable in scenarios you expect to never be reached, because otherwise your code will be full of all sorts of possible permutations and paths that are harder to reason about and may cascade into extremely nuanced or subtle errors. Rust also has a version of unwrap called "expect" where you provide a string that logs why the unwrap occurred. It's similar, but for pieces of code that are crucial it could be a good idea to require all 'unwraps' to instead be 'expects' so that people at least are forced to write down a reason why they believe the unwrap can never be reached.

__bax 10mo ago

git blame on .unwrap() line

selfmodruntime 10mo ago

While this is true, I wish that Rust had more of a first-class support for `no_panic`. Every solution we do have is hacky. I wish that I could guarantee that there were no panic calls anywhere in a code path.

gwd 10mo ago

> This is the multi-million dollar .unwrap() story. While there are certainly many things to admire about Rust, this is why I prefer Golang's "noisy" error handling. In golang that would be either: feature_values, err := features.append_with_names(...) And the compiler would have complained that this value of `err` was unused; or you'd write: feature_values, _ := features.append_with_names(...) And it would be far more obvious that an error message is being ignored. (Renaming `unwrap` to `unwrapOrPanic` would probably help too.)

mamp 10mo ago
I haven't been writing Rust for that long (about 2 years) but every time I see .unwrap() I read it as 'panic in production'. Clippy needs to have harder checks on unwrap.
zero_shift 10mo ago
But I could screw it up in Go, if I made the same assumptions fvs, err := features.AppendWithNames(..) if err != nil { // this will NEVER break panic(err) } Ultimately I don't think language design can be the sole line of defence against system failures; it can only guide developers to think about error cases
- gwd 10mo ago
  Right, but the point isn't to make errors impossible; the point is to have them be 1) less likely to write, and 2) easier to spot on review. People's biggest complaints about golang's errors: 1. You have to _TYPE_OUT_ what to do on EVERY.SINGLE.ERROR. SOO BOORING! 2. They clutter up the code and make it look ugly. Rust is so much cleaner and more convenient (they say)! Just add ?, or .unwrap()! Well, with ".unwrap()", you can type it fast enough that you're on to the next problem before it occurs to your brain to think about what to do if there is an error. Whereas, in golang, by the time you type in, "if err != nil {", you've broken the flow enough that now you're much more likely to be thinking, "Hmm, could this ever fail? What should we do if it does?" That break in flow is annoying, but necessary. And ".unwrap()" looks so unassuming, it's easy to overlook on review; that "panic()" looks a lot more dangerous, and again, would be more likely to trigger a reviewer into thinking, "Wait, is it OK if this thing panics? Is this really so unlikely to happen?" Renaming it `.unwrap_or_panic()` would probably help with both.

twhitmore 10mo ago

Interesting to see Rust error handling flunk out in practice. It may be that forcing handling at every call tends to makes code verbose, and devs insensitized to bad practice. And the diagnostic Rust provided seems pretty garbage. There is bad practice here too -- config failure manifesting as request failure, lack of failing to safe, unsafe rollout, lack of observability. Back to language design & error handling. My informed view is that robustness is best when only major reliability boundaries need to be coded. This the "throw, don't catch" principle with the addition of catches on key reliability boundaries -- typically high-level interactions where you can meaningfully answer a failure. For example, this system could have a total of three catch clauses "Error Loading Config" which fails to safe, "Error Handling Request" which answers 5xx, and "Socket Error" which closes the HTTP connection.

Ciantic 10mo ago
> It may be that forcing handling at every call tends to makes code verbose Rust has a lot of helpers to make it less verbose, even that error they demonstrate could've been written in some form `...code()?` with `?` helper that would have propagated the error forwards. However I do acknowledge that writing Error types is boring sometimes so people don't bother to change their error types and just unwrap. But even my dinghy little apps for my personal use I do simple serach `unwrap` and make sure I have as few as possible.
hypeatei 10mo ago
I don't understand how your takeaway is that this is a language flaw other than to assume that you have some underlying disdain for Rust. That's fine, but state it clearly please. The end result would've been the exact same if they "handled" the error: a bunch of 500s. The language being used doesn't matter if an invariant in your system is broken.

andy_ppp 10mo ago

This is why the Erlang/Elixir methodology of having supervision and letting things crash gracefully is so useful. You can either handle every single error gracefully or handle crashing gracefully - it's much easier and more realistic in large codebases to do the later.

tuetuopay 10mo ago
This would not have helped: the code would crash before doing anything useful at all. If anything, the "crash early" mentality may even be nefarious: instead of handling the error and keeping the old config, you would spin on trying to load a broken config on startup.
- asa400 10mo ago
  Continuing only makes sense for cases you know you can handle. _In theory_ they could have used the old config, but maybe there are reasons that’s not possible in Cloudflare’s setup. Whether or not that’s an invariant violation or just an error that can be handled and recovered from is a matter of opinion in system design. And crashing on an invariant violation is exactly the right thing to do rather than proceed in an undefined state.
  - tuetuopay 10mo ago
    Given the context and what the configuration file contains, I'd argue it's mission-critical for the software to keep running with the previous configuration. Otherwise you might shutdown the internet. Honestly, I'm pretty sure their pre-rewrite version had such logic, and it was forgotten or still on the TODO pile for the rewrite version. At a previous job (cloud provider), we've had exactly this kind of issue, with exactly the same root cause. The entrypoint for the whole network had a set of rules (think a NAT gateway) that were reloaded periodically from the database. Someone rewrote that bit of plumbing from Python to Go. Someone else performed a database migration. Suddenly, the plumbing could not find the data, and pushed an empty file to prod. The rewrite lacked "if empty, do nothing and raise an alert", that the previous one had. I'll let you imagine what happened next :)

NoboruWataya 10mo ago

They should link this article in the docs for `unwrap()`.

sphericalkat 10mo ago

Handling the error still would've returned a 5xx in this case, since the config file was still over the limit of features the service could handle.

BrtByte 10mo ago

Feels like a case where safety guarantees of Rust lulled them into thinking the edge cases were covered

peanut-walrus 10mo ago

I wonder if similar to infrastructure resilience, code resilience is also required for critical services that can never go down? Instead of relying on a single implementation for a critical service, have multiple independent implementations in different languages. Back when I was running my own DNS servers, I did always ensure that primary and secondary were running on different platforms and different software.

meltyness 10mo ago

tokio default behavior within a task is to ignore panics, such as an Err/None unwrap, and only crash that task, so it's impact limited so that's nice, maybe that's where the snowblindness came from. it'd be kinda hard to amend the clippy lints to ignore coroutine unwraps but still pipe up on system ones. i guess. edit: i think they'd have to be "solely-task-color-flavored" so definitely probably not trivial to infer

quotemstr 10mo ago

If the error had been an exception instead of a result, could have bubbled up I have been saying for years that Rust botched error handling in unfixable ways. I will go to the grave believing Rust fumbled. The design of the Rust language encourages people to use unwrap() to turn foreseeable runtime problems into fatal errors. It's the path of least resistance, so people will take it. Rust encourages developers to consider only the happy path. No wonder it's popular among people who've never had to deal with failure. All of the concomitant complexity--- Result, ?, the test thing, anyhow, the inability for stdlib to report allocation failure --- is downstream of a fashion statement against exceptions Rust cargo-culted from Go. The funniest part is that Rust does have exceptions. It just calls them panics. So Rust code has to deal with the ergonomic footgun of Result but pays anyway for the possibility of exceptions. (Sure, you can compile with panic=abort. You can't count on it.) I could not be more certain that Rust should have been a language with exceptions, not Result, and that error objects are a gross antipattern we'll regret for decades.

Veliladon 10mo ago
Errors work just like exceptions especially if you use the ? operator and let the error bubble up the chain. This is the Rust equivalent of an unhandled exception and the ripcord being pulled.
- quotemstr 10mo ago
  In C++, functions are error-colored by default. You write "noexcept" if you want your function to be infallible-colored instead. (You usually want to make a function infallible if you're using your noexcept function as part of a cleanup path or part of a container interface that allows for more optimizations of it knows certain container operations are infallible.) Rust makes infallibility the syntactic default and makes you write Result to indicate fallibility. People often don't want to color their functions this way. Guess what happens when a programmer is six levels deep in infallible-colored function calls and does something that can fail. .unwrap() Guess what, in Rust, is fallible? Mutex acquire. Guess what you need to do often on infallible cleanup paths? Mutex acquire.

otabdeveloper4 10mo ago

Oh come on, stop spreading FUD. Rust programs are 100% immune to crashes and bugs, they have memory safety (c). Also, exception handling is hard and lame. We don't need exceptions, just add a "match" block after every line in your program.

JuniperMesos 10mo ago
What's the point of this sarcastic comment? Do you think that some people claim that Rust's memory safety guarantees mean that a Rust program is incapable of crashing or having a bug? This is a dumb thing to claim certainly, but I'm not aware of anyone actually making this claim. I'm also not sure what you're getting at with the comment about exception handling being lame. I think the ML/Haskell inspired model that Rust uses of having a parameterized Result type for fallible operations is generally better than exceptions for a variety of reasons (although maybe better Exception semantics could help with some of this), but what does this have to do with match blocks?
- otabdeveloper4 10mo ago
  > Do you think that some people claim that Rust's memory safety guarantees mean that a Rust program is incapable of crashing or having a bug? Undoubtedly yes. > ...but what does this have to do with match blocks? You tell me. You're the one advocating for placing one after every single function call.

echelon 10mo ago

> This is the multi-million dollar .unwrap() story. First multi-million dollar .unwrap() story.

torginus 10mo ago

Say what you want exception haters, but at least in exceptions-as-default languages, the decision of a particular issues is fatal to the whole program can be decided centrally at a high level, and not every choice is forced to be up to individual discretion.

underdeserver 10mo ago
But you can do the same thing with Rust, by piping up Results.

torginus 10mo ago

By the way - does this discussion matter and were they wrong to use unwrap()? The way they wrote the code means that having more than 200 features is a hard non-transient error - even if they recovered from it, it meant they'd have had the same error when the code got to the same place. I'm sure when the process crashed, k8s restarted the pod or something - then it reran the same piece of code and crashed in the same place. While I don't necessarily agree with crashing as business strategy, I don't think that doing anything other than either dropping the extra rules or allocating more memory - neither of which the original code was built to do (probably by design). The code made the local hard assumption that there won't ever be more than 200 rules and its okay to crash if that count is exceeded. If you design your code around an invariant never being violated (which is fine), you have to make it clear on a higher level that they did. This isn't a Rust problem (though Rust does make it easy to do the wrong thing here imo)

grogers 10mo ago
Instead of crashing when applying the new config, it's more common to simply ignore the new config if it cannot be applied. You keep running in the last known good state. Operators then get alerts about the failures and can diagnose and resolve the underlying issue. That's not always foolproof, e.g. a freshly (re)started process doesn't have any prior state it can fall back to, so it just hard crashes. But restarts are going to be rate limited anyways, so even then there is time to mitigate the issue before it becomes a large scale outage