Cached meta/global is used if storage/meta/global is deleted

RESOLVED FIXED

Status

()

Firefox for iOS
Build & Test
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

unspecified
All
iOS
Dependency tree / graph

Firefox Tracking Flags

(fxios1.1+)

Details

Attachments

(1 attachment)

48 bytes, text/x-github-pull-request
nalexander
: review+
Details | Review | Splinter Review
(Assignee)

Description

2 years ago
2015-10-09 22:43:26.310 [Debug] [SyncStateMachine.swift:663] advance() > Cached meta/global fetched at 1444455806170, newer than server modified 0. Using cached meta/global.


I noticed that testEngineConfigurations is failing. This is why: we delete meta/global, but -- because the collection is empty but exists -- our test server returns a timestamp of zero.

There are two things we might be doing wrong here. (Possibly both!)

1. If the collection exists but is empty, its modified time should be the instant of deletion. I don't remember if this is true, and the Sync API doc doesn't answer it.

2. If our cached timestamp is later than the server modified time for the collection, it's just as bad as if the server time is later -- we should re-fetch.


Ryan, can you answer #1, ideally also updating the info/collections section of the 1.5 API doc? We can then make our test server match.
(Assignee)

Comment 1

2 years ago
I probably broke this test with Bug 1213318, but at least one of the test, the mock server, and the app code are wrong.

I think I get the assignee, and Nick gets a needinfo!
Assignee: nobody → rnewman
Blocks: 1213318, 1167010
Status: NEW → ASSIGNED
Flags: needinfo?(nalexander)
(Assignee)

Comment 2

2 years ago
Ryan: needinfo for Comment 0.
Flags: needinfo?(rfkelly)
(In reply to Richard Newman [:rnewman] from comment #0)
> 2015-10-09 22:43:26.310 [Debug] [SyncStateMachine.swift:663] advance() >
> Cached meta/global fetched at 1444455806170, newer than server modified 0.
> Using cached meta/global.
> 
> 
> I noticed that testEngineConfigurations is failing. This is why: we delete
> meta/global, but -- because the collection is empty but exists -- our test
> server returns a timestamp of zero.

Ah yes, I ran into this, and handled it by fudging MockSyncServer to not include present-but-empty collections in info/collections.  That is, if MockSyncServer.collections["meta"] == [], I didn't include {"meta":"0"} in info/collections.  Your extract-helper refactor probably changed that.

I justified my handling by reading in the Sync docs or a ticket -- no reference to hand -- that the Sync server kept a collection timestamp independent of the set of collection records.  I didn't want to build that for the MockSyncServer, hence the above.  If my interpretation is correct, this ticket is "only" a failing test, not failure in the wild.

> There are two things we might be doing wrong here. (Possibly both!)
> 
> 1. If the collection exists but is empty, its modified time should be the
> instant of deletion. I don't remember if this is true, and the Sync API doc
> doesn't answer it.

This is what I arrived at, via docs, ticket, or rfkelly directly.

> 2. If our cached timestamp is later than the server modified time for the
> collection, it's just as bad as if the server time is later -- we should
> re-fetch.

I'm not sure I follow.  I think the expedient thing here is to special case info/collections["collection"] == 0, since that means the collection is empty.

> Ryan, can you answer #1, ideally also updating the info/collections section
> of the 1.5 API doc? We can then make our test server match.

N.b.: I expect this applies to the caching of meta/global and crypto/keys, which both use the collection timestamp and will both be susceptible to 0.
Flags: needinfo?(nalexander)
(Assignee)

Comment 4

2 years ago
(In reply to Nick Alexander :nalexander from comment #3)

> Ah yes, I ran into this, and handled it by fudging MockSyncServer to not
> include present-but-empty collections in info/collections.  That is, if
> MockSyncServer.collections["meta"] == [], I didn't include {"meta":"0"} in
> info/collections.  Your extract-helper refactor probably changed that.

Yeah. We shouldn't do that: as far as possible, we should try to replicate the behavior of the API correctly.


> I justified my handling by reading in the Sync docs or a ticket -- no
> reference to hand -- that the Sync server kept a collection timestamp
> independent of the set of collection records.

It does: there's a difference between deleting all of the records in a collection and deleting the collection itself.

---
Collections are created implicitly when a BSO is stored in them for the first time. They continue to exist until they are explicitly deleted, even if they no longer contain any BSOs.
---

---
Each collection has a last-modified time that is updated whenever an item in that collection is modified or deleted. It will always be less than or equal to the overall last-modified time.
---

---
The last-modified time is guaranteed to be monotonically increasing and can be used for coordination and conflict management as described in Concurrency and Conflict Management.
---

---
DELETE https://<endpoint-url>/storage/<collection>

    Deletes an entire collection.

    After executing this request, the collection will not appear in the output of GET /info/collections and calls to GET /storage/<collection> will return an empty list.

---

and so on.


> If my interpretation is correct,
> this ticket is "only" a failing test, not failure in the wild.

Most likely, yes… but of course it's hard to tell if the client is buggy without replicating against a real server, because we know that the test server doesn't behave quite like the real thing.



> > 2. If our cached timestamp is later than the server modified time for the
> > collection, it's just as bad as if the server time is later -- we should
> > re-fetch.
> 
> I'm not sure I follow.  I think the expedient thing here is to special case
> info/collections["collection"] == 0, since that means the collection is
> empty.

Based on re-reading, it looks like this should never be the case: timestamps are always monotonically increasing.
(Assignee)

Comment 5

2 years ago
Created attachment 8672331 [details] [review]
Pull req.

This makes the tests pass. We now handle deletes correctly, and track collection timestamps independently of records.
Attachment #8672331 - Flags: review?(nalexander)
Comment on attachment 8672331 [details] [review]
Pull req.

lgtm.
Attachment #8672331 - Flags: review?(nalexander) → review+
(Assignee)

Comment 7

2 years ago
bb00145
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Component: Sync → Build & Test
Flags: needinfo?(rfkelly)
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Blocks: 1212431
> 1. If the collection exists but is empty, its modified time should be the instant of deletion.
> I don't remember if this is true, and the Sync API doc doesn't answer it.

This is indeed correct; I added a bit more to the docs in https://github.com/mozilla-services/docs/commit/9e4e5f0e0042a850bfd8f0cb204773063367ca79
You need to log in before you can comment on or make changes to this bug.