1210625 - meta/global modified timestamp is not updated on successful PUT

Status: RESOLVED FIXED

Description

[Nick Alexander :nalexander [he/him]]

After a 200 PUT to storage/meta/global, I see the 'modified' timestamp is not being updated.  The 'meta' record of info/collections is being updated.

To reproduce, find a way to look at storage/meta/global and info/collections.  I used a hacked up version of https://github.com/edmoz/fxa-sync-client; I intend to upstream my changes shortly.

Then, connect Desktop to a test Firefox Account.  Force a sync.  Observe that meta/global is present on the server, and that info/collections reflects the record's modified timestamp.

Go to Sync preferences and deselect some engines; force a sync.  Observe that meta/global has been updated on the server; that meta/global's modified timestamp has *not* been updated; and that info/collections reflects a timestamp newer than the record's modified timestamp.

Comment 1

[Nick Alexander :nalexander [he/him]]

I observe that Android carefully uses the fetch timestamp and not the record modified field: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/stage/FetchMetaGlobalStage.java?offset=0#32

rnewman: are you aware of this inconsistency on the server?  Do you have any tickets or context to add?  (It appears not; your iOS code uses the record modified field, and suffers for it.)

Comment 2

[Richard Newman [:rnewman]]

This sounds like a server bug, and it sounds like the kind of thing that would cause bugs similar to Bug 692620.

---
X-Last-Modified

    This header gives the last-modified time of the target resource as seen during processing of the request, and will be included in all success responses (200, 201, 204). When given in response to a write request, this will be equal to the server’s current time and to the new last-modified time of any BSOs created or changed by the request.
---

---
modified 	none 	float, 2 decimal places 	The timestamp at which this object was last modified, in seconds since UNIX epoch (1970-01-01 00:00:00 UTC). This is set automatically by the server according to its own clock; any client-supplied value for this field is ignored.
---


So the server is using the client-supplied "modified" attribute?

Comment 3

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #1)
> I observe that Android carefully uses the fetch timestamp and not the record
> modified field:

It does so instead of using the info/collections response that's already held.

I don't think this is deliberately working around this bug… and, of course, it's still incorrect when the modified stamp of the record is compared to a persisted value.

Comment 4

[Ryan Kelly [:rfkelly]]

> So the server is using the client-supplied "modified" attribute?

Yes, it looks like the memcached storage engine copies over any modified stamp provided by the client:

  https://github.com/mozilla-services/server-syncstorage/blob/master/syncstorage/storage/memcached.py#L690

The plain SQL storage engine does not, which would explain why we see this for meta/global but not other record types.

Comment 5

[Ryan Kelly [:rfkelly]]

The memcached backend deliberately copies the client-provided modified time, but I've no idea why.  I just removed the code and it hasn't broken any testcases.  *shrug*

:zaach, since you're working on sync stuff these days, want to r? this relatively trivial server-side patch?

Comment 6

[Ryan Kelly [:rfkelly]]

Created attachment 8668892 [details] [review]
remove buggy and apparently-point code that copies the client timestamp

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #3)
> (In reply to Nick Alexander :nalexander from comment #1)
> > I observe that Android carefully uses the fetch timestamp and not the record
> > modified field:
> 
> It does so instead of using the info/collections response that's already
> held.
> 
> I don't think this is deliberately working around this bug… and, of course,
> it's still incorrect when the modified stamp of the record is compared to a
> persisted value.

I assume you mean it /would/ be incorrect, 'cuz we don't do this in the Android Sync code.  At no time do we use the modified stamp of the two records we persist (meta/global and crypto/keys).  We always use the X-Weave-Timestamp of the record fetch, and compare against the corresponding entry in info/collections.  That should be always safe.  Do you agree?

Comment 8

[Richard Newman [:rnewman]]

Yes. 

There are three timestamps available at that moment: the I/c we're holding, the response timestamp, and the record timestamp. 

The latter two should be identical; that they are not is this bug. The comment in that code refers to not using I/c, rather than predicting this bug.

Comment 9

[Richard Newman [:rnewman]]

Note that the meta timestamp can be later than m/g in Sync 1.1, because of the migration sentinel. In that instance we'll do redundant work, comparing m/g by content when we should be able to discard a non-change by timestamp.

Comment 10

[Ryan Kelly [:rfkelly]]

https://github.com/mozilla-services/server-syncstorage/commit/ec3cd2e5f60709d5ad875df52462fbc9d9cd7185