Sounds like a bug. 

From a recent cron run:


17:20:00.338
Deleting &som.Annotation{KeyDigest:"0287a1bd6fdb1d0ff431445c90d11a9d81a46984", Key:"", Bugs:[]string(nil), Comments:[]som.Comment(nil), SnoozeTime:0, ModificationTime:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}
17:20:00.338
Deleting &som.Annotation{KeyDigest:"6b5d26eea0122d8caa64c2002a47da1bb3725468", Key:"", Bugs:[]string(nil), Comments:[]som.Comment(nil), SnoozeTime:0, ModificationTime:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}
17:20:00.338
Deleting &som.Annotation{KeyDigest:"644b9853f9cfaf57ead1783b9d4a308dc1a94812", Key:"", Bugs:[]string(nil), Comments:[]som.Comment(nil), SnoozeTime:0, ModificationTime:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}
17:20:00.338
Deleting &som.Annotation{KeyDigest:"58454cfb06479a8b61a83ba52f4d2368fca46211", Key:"", Bugs:[]string(nil), Comments:[]som.Comment(nil), SnoozeTime:0, ModificationTime:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}
17:20:00.338
Deleting &som.Annotation{KeyDigest:"27cbdfbcf8f7dcea1355d3bbed64052a59b3fb92", Key:"", Bugs:[]string(nil), Comments:[]som.Comment(nil), SnoozeTime:0, ModificationTime:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}
17:20:00.338
Deleting &som.Annotation{KeyDigest:"8275ebfcad433256458b97efdb1255d281c1f667", Key:"", Bugs:[]string(nil), Comments:[]som.Comment(nil), SnoozeTime:0, ModificationTime:time.Time{sec:0, nsec:0, loc:(*time.Location)(nil)}}
17:20:00.482
deleted 6 annotations

Oh, I think I understand why this happens. There is only one modification time for annotations that gets updated every time an annotation is added/removed. So as long as people keep linking new bugs, snoozing, or adding new comments, old annotations get propagated. 

A simple fix is to just make use the of the time field in comments and change the flushOldAnnotations cron to delete comments based on their time as well. 

Though this problem does also surface that the same problem would happen with old, irrelevant bugs (which is what made us add the cron to remove old annotations in the first place). So I think I should maybe also add a timestamp to each bug and do the same for them as well perhaps? Then ModificationTime would only apply to snoozing. 

Sean/Stephen, WDYT? 

(Also noting that this bug was specifically mentioned to be confusing in a sheriffing survey response) 

adding a timestamp to the bug SGTM

Hmm, I tried adding a time to bugs, but this is a bit less straightforward than I thought at first. Filtering comments/bugs based on time doesn't really work since they're represented as list properties in the Datastore. 

So, I could try pulling all annotations and filtering them on the Go side, but I think this is probably a bad idea (Datastore doesn't recommend querying for more than ~100 entities at a time if I recall?). I think I might end up adding an additional field that's more like "firstModified" rather than lastModified, so that I could use it to find annotations that contain old comments. 

But I think this is starting to get a bit convoluted, and from https://bugs.chromium.org/p/chromium/issues/detail?id=639171 , it sounds like the cron to clean up old annotations was a bit of a short term solution to begin with. 

So I will hold off on this for now and try to come up with a better way to handle this. 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra.git/+/0baa0c880dc0afc6e075d646230d327348a54fee

commit 0baa0c880dc0afc6e075d646230d327348a54fee
Author: Tiff Zhang <zhangtiff@google.com>
Date: Tue Jan 31 17:49:22 2017

SoM: Hide old comments.

BUG= 684199 

Change-Id: Ibe56c4ba8c6eb2828a9f46fedf56f73e29592a71
Reviewed-on: https://chromium-review.googlesource.com/434862
Reviewed-by: Sean McCullough <seanmccullough@chromium.org>
Commit-Queue: Tiffany Zhang <zhangtiff@chromium.org>

[modify] https://crrev.com/0baa0c880dc0afc6e075d646230d327348a54fee/go/src/infra/appengine/sheriff-o-matic/elements/som-annotation-manager-behavior.html
[modify] https://crrev.com/0baa0c880dc0afc6e075d646230d327348a54fee/go/src/infra/appengine/sheriff-o-matic/elements/som-annotations/som-annotations.js
[modify] https://crrev.com/0baa0c880dc0afc6e075d646230d327348a54fee/go/src/infra/appengine/sheriff-o-matic/test/som-annotations-test.html

For now, the main user concern here should be solved. I'll merge this into the previous issue about deleting old annotations.