818087 - Records set to Start announcing at a time later than creation date are not fetched.

Status: RESOLVED FIXED

(Cloud Services :: Server: Product Announcements Campaign Manager, defect)

Description

[Tracy Walker [:tracy]]

If a record is created to start at some point in the future.  When that start time is hit, the record is not found for display.

Tested this by creating a record scheduled to Start announcing ~5 minutes after the start time.  Wait for time to pass.  The announcement is not fetched.

Setting start time to now or  in the past works fine.

Comment 1

[Richard Newman [:rnewman]]

Start time is what the server should be using to decide whether to serve up an announcement. Creation time is an implementation detail.

Comment 2

[Tracy Walker [:tracy]]

Yesterday I created an announcement with a scheduled start time for this morning.  This morning I turned on the device.  At start up and regularly there after, the device tries to fetch announcements as expected.  But the announcement I expect to get is not found.  

This should probably be a blocker. However, since it can be worked around by making start time not in the future, I'll leave it at critical.

Comment 3

[Richard Newman [:rnewman]]

Might as well put this on JR's plate. Ain't nobody else gonna do it!

Comment 4

[Tracy Walker [:tracy]]

An announcement I had set for future DID come across. But not exactly when I expected it too.  Needs more investigation on my end as to what sort of clock/time zone skew is in play.  Will let you know what I am able to uncover.

Comment 5

[JR Conlin [:jrconlin,:jconlin]]

Sorry for the delay, No idea why this didn't pop in my mail stream.

The problem is a logic error in my sql. I check created time later than last accessed. For normal records, this prevents publication duplication, but messes with scheduled records. The fix is reasonably easy, but will require a bit of extra coding and possibly a fix to the data store.

Comment 6

[JR Conlin [:jrconlin,:jconlin]]

Based off of discussions with rnewman, I've applied a patch to the code. 
https://github.com/jrconlin/campaign_manager/commit/7ea75074ec39afcd0a471c7a4520aa24e17694e6

This has been pushed to the AWS server.

Comment 7

[Tracy Walker [:tracy]]

Is there a minimum time into the future for using this setting?  just asking for testing purposes.

Comment 8

[JR Conlin [:jrconlin,:jconlin]]

The proposed patch looks to see if the start time of the campaign plus the requested amount of time idle is greater than now minus the reported idle period. The time the record was created has no bearing on this.

You should be able to test by backdating the start time of a new record plus idle time to be less than now minus current idle time.

Comment 9

[Tracy Walker [:tracy]]

reopening, announcements no longer work at all.

Comment 10

[Richard Newman [:rnewman]]

(In reply to JR Conlin [:jrconlin,:jconlin] from comment #8)
> The proposed patch looks to see if the start time of the campaign plus the
> requested amount of time idle is greater than now minus the reported idle
> period.

That's the wrong way around. You want to return an item if its start time is *less than* now, but greater than If-Modified-Since.

That's the simple case.

Then we modify that by including idle. We want to return an item precisely once. A client could arrive and be immediately ready by already being idle, or it could take *multiple fetches* where an item is a candidate before we consider the client old enough.

That means our evaluation needs to account for the minimum idle time.

Consider an item that requires idle > 1, start_time = n.

First fetch:

  t1 = n + 0.5
  idle = 0.5
  IMS = n - 0.5

We don't return the record, because idle is too low. IMS now = t1.

The second fetch is at

  t2 = n + 1.5
  idle = 1.5
  IMS = t1 = n + 0.5

Our idle time is met, so we want to return the record now.


Our constraints:

  1. start_time + idle_min > IMS
  2. idle >= idle_min
  3. now >= start_time
  (4. start_time + idle_min < end_time  -- sanity invariant)

The next fetch is at

  t3 = n + 2.5
  idle = 2.5
  IMS = t2 = n + 1.5

We don't want to return the record, which is good: our first inequality does not match:

  n + 1 > t2


Now let's consider the second case: a new client that's already been idle for ages arrives moments after the start time. (This is the problematic case, but fortunately it should be rare.)

  t1 = n + 0.01
  idle = 10
  IMS = n - 0.5

Let's work through the equations:

  n + 1 > n - 0.5                     == check
  idle > idle_min                     == check
  now > start_time                    == check

so we return the item.

Next fetch:

  t2 = n + 0.51
  idle = 10.5
  IMS = t1 = n + 0.01

Let's work through the equations again.

  n + 1 > n + 0.01      == check
  10.5 > 1              == check
  now > start_time      == check

Uh oh! So we need another constraint. In order to prevent redisplay in this case, we need one of these things to be true:

 1. Clients fetch no more frequently than idle_min, or are guaranteed to fetch only once within the window. (Break condition 1).
 2. Clients can be told to back off when an item requiring idle handling is returned.
 3. We somehow involve a maximum idle time in the filtering.
 4. We return a Date header that is fast by idle_min, ensuring that condition 1 cannot be true in any subsequent fetch.

Better solutions are welcome.

(The thorough solution is for the client to track records that it should subsequently skip, but we elected to keep the client dumb… I'm happy to address that for Firefox 20/21.)

Comment 11

[Tracy Walker [:tracy]]

This is still occurring for me.

I am starting to feel this may be user error. If I set a start time in the past, the announcement isn't fetched. I am using HTTP-date format of Thu, 13 Dec 2012 18:10:00 GMT

Comment 12

[Tracy Walker [:tracy]]

It's not me.  I set a start time with relative date of -1h. It set start time to Thu, 13 Dec 2012 01:00:00 GMT (not what I'd expect for time) and clients did not pick up the message.

Comment 13

[JR Conlin [:jrconlin,:jconlin]]

Yep, looking into this. Odd thing is that on my dev environment, things are working correctly. Tracy, I'm going to try messing around a bit with the prod db to see if there's an issue with the RDS db.

Comment 14

[JR Conlin [:jrconlin,:jconlin]]

Looking into this a bit deeper, I don't think that there's a bug.

Right now, the way that If-Modified-Since acts, is a marker to say "The last time I asked for data." If that marker is after a campaign's start date (regardless of when it was created) then it's presumed that a campaign has already been seen by the client and it's not returned. 

E.g Let's say I've got the following:

record    created   start   
1         0900               // No start time defined
2         0915      1000     // start in 45 minutes from created
3         0930      0800     // start an hour before created

If the clock is 0920 and  If no If-Modified-Since header is sent, the user would get record 1. 
If the clock is 0940 and If-Modified-Since = 0920, No records would be returned, because it was presumed that the user would have already seen records 1 and 3.
If the clock is 1020 and If-Modified-Since = 0940, The user gets record 2.
If the clock is 1020 and there is no IMS header sent, then the user would get all three.

Now, I could argue that I should not allow "backdating" records in the UI in order to prevent confusion like this, but that's a different issue.

Comment 15

[Tracy Walker [:tracy]]

Well, there is not a bug here now.  It has started working. * shrugs with a :-) *



just so I'm clearly understanding this.  I should not see any announcements at all if I just installed Fennec (and the test add-on) on a device, correct?  Assuming all campaign announcements have a start date earlier than the first client fetch.

Comment 16

[Richard Newman [:rnewman]]

> 1         0900               // No start time defined

You should not allow users to create a record without a start time. Creation time is an implementation detail; start time really matters!

> Now, I could argue that I should not allow "backdating" records in the UI in
> order to prevent confusion like this, but that's a different issue.

Yes. Don't allow back-dating; it's meaningless. The earliest start time should be "now".