meta/global modified timestamp is not updated on successful PUT

RESOLVED FIXED

Status

--
major
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nalexander, Assigned: rfkelly)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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.)
Flags: needinfo?(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?
Severity: normal → major
Component: Firefox Sync: Backend → Server: Sync
Flags: needinfo?(rnewman) → needinfo?(rfkelly)
OS: Unspecified → All
Hardware: Unspecified → All
(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.
(Assignee)

Comment 4

3 years ago
> 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.
Assignee: nobody → rfkelly
No longer blocks: 1167010
Flags: needinfo?(rfkelly)
(Assignee)

Updated

3 years ago
Blocks: 1167010
(Assignee)

Comment 5

3 years ago
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?
(Assignee)

Comment 6

3 years ago
Created attachment 8668892 [details] [review]
remove buggy and apparently-point code that copies the client timestamp
Attachment #8668892 - Flags: review?(zack.carter)
(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?
Flags: needinfo?(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.
Flags: needinfo?(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.
Attachment #8668892 - Flags: review?(zack.carter) → review+
(Assignee)

Comment 10

3 years ago
https://github.com/mozilla-services/server-syncstorage/commit/ec3cd2e5f60709d5ad875df52462fbc9d9cd7185
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Blocks: 1211749
You need to log in before you can comment on or make changes to this bug.