18 ms·
I bet the post-mortem for this is going to be fun. How this wasn't covered by multiple unit tests, a code review and a roll out strategy is.... impressive.
by Traster 5y ago
I bet the post-mortem for this is going to be fun. How this wasn't covered by multiple unit tests, a code review and a roll out strategy is.... impressive.
- sneak 5y agoBitwise & versus logical && is a classic, right up there with an assignment in a comparison (when an equality check is intended), = for ==. That this was missed is pretty surprising, given that it's Google and the stakes involved in the encryption/key management code in a secure platform device. I wonder if we'll even get a postmortem, as this simply cannot happen unless several someones all Seriously Fucked Up simultaneously. - code error (expected, humans are fallible) - review error (less expected, this is the kind of thing code review exists for) - static analysis / linting check failure - integration test failure (interactive login no longer works on this build) This is a massive, overlapping fuckup. Heads should probably roll here, especially given that when you opt in to ChromeOS as a user, Google now has "one job" (DFIU).
- james_in_the_uk 5y agoStraight up negligence.
- brundolf 5y agoEspecially since it's so easy to lint for. This isn't some cross-file memory mishandling bug, it's a single token that's being used where it shouldn't be
- mdoms 5y ago> Heads should probably roll here, I disagree. It's obviously a major balls up by multiple people, but at the end of the day this is an organizational failure. You don't fire people for this, you learn from it and fix the organizational holes that caused it.
- sneak 5y agoSomeone is responsible for the organizational units and their processes. By that logic, almost no individual failure is a firing offense. Many people in the organization are responsible for ensuring that the processes themselves overlap in ways that exclude the possibility of failures like this. I'm talking about the meta-failure in management's oversight of the processes. There is belt person, and there is suspenders person, and there is a third person who makes sure there are both belt person and suspenders person on staff and that they don't go on vacation at the same time. Somewhere above the team lead and below the CEO there is someone who didn't do their job.
- nicoburns 5y ago> By that logic, almost no individual failure is a firing offence I'd probably agree with that. Firing should be for extreme negligence, or sustained underperformance.
- idiotsecant 5y ago>extreme negligence If this doesn't qualify, what does?
- sneak 5y agoI would say this is negligence, but perhaps not extremely so.
- anyfoo 5y agoYou are all very quick to judge. I don't work for Google so I don't know what happened, but consider the possibility that this may have been a freak accident. That piece of code for example might only be on a path that is not activated when testing, or it may be executed but not have the same fatal effect. And before anyone comes along and says "well, every possible path should be tested then": That would be absolutely impossible even if the halting problem wasn't a thing (which it very much is). Even if you somehow have utopian "100% code coverage", that does not mean you have tested every possible input resulting in every possible state combination. I bet every one of us has a story about an old bug that only manifested years[1] later because of the confluence of many unfortunate things, one that takes half an hour to tell. [1] Or even decades, in some cases that then made it to the HN front page.
- nogridbag 5y agoI've seen the = instead of == in the wild with disastrous results: the assignment was on an ORM backed object, e.g. // accidentally mutates all names to Sam and persists to the DB myList.filter { myDomainObj.name = 'Sam' } But it makes me wonder why our programming languages would use characters which can often lead to this type of error.
- sneak 5y agoIf you consider a linter part of a programming language, most, when properly configured, do not permit this. It's just the syntax that does; professionals do not rely on syntax validity alone but employ linting, code review, static analysis, and integrations tests on top. The gradient of professionalism in our industry is very wide, and the slope is quite shallow. To your point, however, many languages (such as Go, developed and used by Google, the organization under discussion) have designed their syntax now to completely avoid this type of error even being possible.
- tmp_anon_22 5y ago> type of error even being possible I mean its still possible to forget the ":" character and its still possible to mentally scan a PR and see "=" and miss that it should have been a "==".
- edflsafoiewq 5y ago= vs == isn't possible in Go because of the statement/expression distinction. The benefits of allowing assignment to be an expression are too small compared to the problems it causes. x == 1 // oops, meant = // x == 1 evaluated but not used if x = 1 { ... } // oops, meant == // syntax error: assignment x = 1 used as value
- tmp_anon_22 5y agoAh I see your point. Appreciate the correction.
- 5y ago
- fierro 5y agoglances at screen LGTM
- kps 5y agoI'm quite surprised there isn't a compiler warning for a bitwise operator at the root of a condition. I've been writing `if ((flags & FLAG) != 0)` for decades years for nothing?!?
- exikyut 5y agoHmm. In all seriousness, you could probably build a lint rule/tool to allow only the format you note.
- Guidii 5y ago"Heads should probably roll here..." Google has a blameless post-mortem process (see https://sre.google/sre-book/postmortem-culture/ https://sre.google/sre-book/postmortem-culture/) which essentially boils down to: 0) We want to fix the situation. By the time the post-mortem happens, that should already be done. 1) We want to make sure this never happens again. In order to do that we need to understand as much as we can about what happened. 2) We want everyone to share everything they know about the event. If the participants worry about fallout, they will be compelled to cover up. 3) Most failures happen at a number of points, so you need to dig in deeply if you want to get full value out of the "event". 4) Good post-mortems lead to process fixes to reduce (or eliminate?) the chance of similar events, or to make fixes throughout the code base. The other thing is that after the post-mortem, all of the folks involved have learned from it. Usually they're better engineers because of that expensive lesson you've just paid for. Disclaimer: Googler, participant in several post-mortems, opinions my own.
- tmp_anon_22 5y agoEveryone should support blameless postmortems. I'm more ruing the impending "oh we're soooo sorry" PR babble where it reads as a subtle "** off pleb".
- Mindwipe 5y agoBlameless post-mortems are good, but there have been rather a lot of bootloader level disasters on ChromeOS/Android and where they intersect at this point and it is probably worth wondering why this process does not seem to be generating sufficient improvement to make customer devices work better. One notes that even years later Google still haven't figured out why the Pixel C occasionally just forgot it's own passcode and encryption key and locked users out. Thousands of reports, and publicly acknowledged as an issue. Never addressed. Pixel devices have had multiple bootloader issues and are currently experiencing a widespread Widevine downgrade for months and not fixed.
- TechBro8615 5y agoSounds nice, but how much is this really true in practice? If one guy on the team seriously screws up, won't the rest of the team naturally trust him less?
- agilob 5y agoRedHat once committed wrong value of MTWO variable, it was 2, there was another variable called TWO which also had value of 2. It was fixed a few months later with a commit like "change value of MTWO to -2".Sorry, but can't find it find now
- khazhoux 5y ago> this simply cannot happen unless several someones all Seriously Fucked Up simultaneously. This is exactly how any and every bug enters a mature software product: it slips past multiple layers. > This is a massive, overlapping fuckup. Heads should probably roll here Some process should change, to fix the problem. But firing people is not necessarily the best solution.
- chovybizzass 5y agoi've never worked on a project (even TDD) that did that level of testing detail.
- shirro 5y agoThis isn't the failure of an individual. It is a failure of management.