28

When checking the timeline of a post I noticed that "Post Closed" events are now attributed to Community, and the close voters were moved to the "comment" column. Before, all close voters were listed in the "by" column. The same happened for "Post Reopened".

However, the event was also turned into an "edited" event, with Community adding all the post content, which is a bit confusing.


Before (screenshot borrowed from this question):

"Reopened" timeline entry, voters listed in "by" column

Now (question):

4
  • 14
    We figured it out, we think. Turns out that one can break a whole lot of stuff with one little != instead of ==. We won't be able to push this tonight because we need to make sure that fixing this doesn't have unintended consequences elsewhere, but we'll be able to get it fixed ASAP tomorrow. Thank you so much for reporting this bug.
    – kristinalustig StaffMod
    Sep 14 at 23:34
  • 11
    To steal an old joke: "Cost of changing != to ==: $1.00. Cost of knowing which != to change: $999.00."
    – Kyle Pollard StaffMod
    Sep 15 at 1:11
  • @kristinalustig: Did you add some kind of regression test for this (or for some larger scope - covering more than the very specific)? Sep 15 at 11:33
  • 2
    @P.Mort.-forgotClayShirky_q That's up next!
    – kristinalustig StaffMod
    Sep 15 at 14:29
18

This has been fixed!

This took a while to triage because it touched a ton of different functions I wasn't used to poking around in, including the post history and post history revision generation.

The Bug

The source of the problem was a function that checked if the post history entry had any content changes. This is a function that effectively hasn't been touched since Atwood first committed it 12 years ago. The original function was:

public bool HasContent()
{
    return !(string.IsNullOrEmpty(Tags) && string.IsNullOrEmpty(Title) && !string.IsNullOrEmpty(Body));
}

This function was then modified to add a new component involving checking for some new feature that we haven't shipped. It looked similar to this:

public bool HasContent()
{
    return !(NewFeature != NewFeature.None && string.IsNullOrEmpty(Tags) && string.IsNullOrEmpty(Title) && !string.IsNullOrEmpty(Body));
}

This function is kinda hard to read though - applying De Morgan's law we can negate the entire statement to make it a little more readable:

public bool HasContent()
{
    return NewFeature == NewFeature.None || !string.IsNullOrEmpty(Tags) || !string.IsNullOrEmpty(Title) || !string.IsNullOrEmpty(Body);
}

The bug here is now pretty clear - we consider anything that has the new feature set to none as having its content changed. Because NewFeature was an enum, we have the behavior of default(NewFeature) == 0 == NewFeature.None, which meant that all post history items had this value set to NewFeature.None. This meant that all post history items were now considered to have content.

There's a function that transforms the list of post history entries to revision entries, and it depended on this check. With this buggy behavior, it started doing unpredictable things like displaying extra revisions for reopens or showing weird things in the post's timeline. Lots of other things depend of this content check working, so I'm certain that other features on the site were impacted too.

The approximate period of time this bug was live was:

  • Start: 2021-09-14 13:50 UTC
  • End: 2021-09-15 15:31 UTC

Next Steps

  • I've added items on our technical debt backlog to take care of adding tests/refactoring the HasContent function, as well as adding tests for the dependencies of HasContent that were demonstrating the buggy behavior. This bug is something that automated tests can and should catch - we're just missing test coverage here.
  • During this buggy period, the revision counts were off. Some post history items were being counted as a revision, so this threw off anything that referenced a revision by its number. It's possible that we may be able to make a database fix that fixes rollbacks pointing at invalid revisions, but we're not able to commit resources to fixing this at this time. For now, we're okay with any rollback that occurred showing the fallback of "revision 0" during the impacted time. The data is still there and recoverable if we changed each affected rollback to point to the nearest revision before it, but for now it will continue to point at the history item with no associated revision. If there is a larger problem or we've misunderstood the impact here, please let us know and we can reprioritize appropriately.
5
  • 1
    we're just missing test coverage here. ... if it is not too embarrassing mind giving a ballpark number where your coverage is at?
    – rene
    Sep 15 at 18:05
  • 2
    @rene No numbers to share, but it's below where we'd like it. It's a priority of the engineering group to improve how we do testing and get better code coverage. I'm personally part of our internal testing working group, running an internal engineering book club for the book Working Effectively with Legacy Code, and trying to add tests to any work I do here on the Public Platform team. We've got a really great velocity, but still have lots of work ahead.
    – Kyle Pollard StaffMod
    Sep 15 at 18:44
  • 3
    As of 2021-09-15T210820Z+0, the edit grace period is still on the order of 0 seconds (not 300 seconds (5 minutes)). Sep 15 at 21:09
  • Interestingly, this seems to have completely screwed up revision histories on the (defunct) app, as the bug clearly took effect, but the fix hasn't Sep 16 at 3:15
  • 1
    We do not have a goal of 100% test coverage, not planning on sharing any numbers related to this (when the unstated judgment is that of your are below 100%, you still have more to do).
    – Yaakov Ellis StaffMod
    Sep 16 at 17:01
16

This bug seems to be much wider in scope, affecting actual site functionality, and not only display.

Not only closing events are affected, but other events (locking, deleting, etc.) are also affected.

See the below screenshot of a question's revision history:

All post modified events show as edits

When actions by multiple people take place, the "edit" is attributed to the Community user. When a binding action takes place, the "edit" is attributed to whoever took the action.

Also, I'm unable to rollback to prior revisions from before a closing/reopening, (un)deleting, or (un)locking action took place - clicking the Rollback button on such a prior revision produces an error message "Unable to rollback". (It turns out I wasn't reproducing this correctly - I was able to rollback to a prior revision, just not those that happen to contain the same content or are the current revision with text.)

Finally, this bug breaks links to specific revisions on posts - as the revision numbers are changed, and the URLs only contain the revision number and not the ID, a link to, say, revision 5 which took place after a history event, will link to the wrong revision, as that revision is now numbered 6.

9
  • 1
    Do you happen to have the exact text of the error message that you got when you tried to roll something back? We're digging into this bug right now and having the exact text would help us narrow it down. Thank you!
    – kristinalustig StaffMod
    Sep 14 at 22:01
  • 4
    @kristinalustig Seems I was kind of wrong. This is the question highlighted here. Attempting to rollback to revision 3 (with the current text) produces an error message "Unable to rollback to the current revision". Attempting it on revision 2 or any revision 4+ produces an error message "Unable to rollback because the revisions are identical." Rolling back to revision 1 worked, resulting in revision 11, and I self-reverted that rollback. Sep 14 at 23:19
  • 5
    Copying from above: we figured it out, we think. Turns out that one can break a whole lot of stuff with one little != instead of ==. We won't be able to push this tonight because we need to make sure that fixing this doesn't have unintended consequences elsewhere, but we'll be able to get it fixed ASAP tomorrow.
    – kristinalustig StaffMod
    Sep 14 at 23:34
  • As of 2021-09-15T151245Z+0, the edit grace period is 0 seconds, at least on Stack Overflow main (but does require a change to the content (that functionality didn't change)). Sep 15 at 15:13
  • @kristinalustig It seems that the fix for this bug (deployed recently) broke the edit summary for the last rollback I performed on that post. It shows as "Rollback to Revision 0"; it should show as "Rollback to Revision 4". Sep 15 at 16:42
  • 1
    @SonictheAnonymousHedgehog we're just discussing that now - the buggy behavior meant that all of the revision counts were off as the reopen/close entries were being shown as revisions. Just about to post an answer with some more context.
    – Kyle Pollard StaffMod
    Sep 15 at 17:24
  • 1
    @SonictheAnonymousHedgehog We've got a good understanding of what's broken here - it looks like your revision from the question you linked was a rollback to the Post Closed history item, which was a revision during the buggy period, but is no longer a revision after we fixed the bug. Thankfully we're referencing history item GUIDs and not the specific revision number in our database entries here, so we may be able to fix the data on any history items pointing at non-existent revisions during this period.
    – Kyle Pollard StaffMod
    Sep 15 at 17:38
  • 1
    @SonictheAnonymousHedgehog After some investigation, we're not planning on tackling the last bug in your post right now, but we might come back to it in the future. See my answer for more details.
    – Kyle Pollard StaffMod
    Sep 15 at 18:38
  • 1
    @KylePollard It seems the fix for this bug caused another bug: meta.stackexchange.com/questions/369891/… Sep 15 at 20:14

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .