Closed
Bug 1210625
Opened 9 years ago
Closed 9 years ago
meta/global modified timestamp is not updated on successful PUT
Categories
(Cloud Services Graveyard :: Server: Sync, defect)
Cloud Services Graveyard
Server: Sync
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: nalexander, Assigned: rfkelly)
References
Details
Attachments
(1 file)
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.
Reporter | ||
Comment 1•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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
Comment 3•9 years ago
|
||
(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•9 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 | ||
Comment 5•9 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•9 years ago
|
||
Attachment #8668892 -
Flags: review?(zack.carter)
Reporter | ||
Comment 7•9 years ago
|
||
(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)
Comment 8•9 years ago
|
||
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)
Comment 9•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8668892 -
Flags: review?(zack.carter) → review+
Assignee | ||
Comment 10•9 years ago
|
||
https://github.com/mozilla-services/server-syncstorage/commit/ec3cd2e5f60709d5ad875df52462fbc9d9cd7185
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•1 year ago
|
Product: Cloud Services → Cloud Services Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•