The branch "master" has been updated.
The following is a summary of the commits.

from: 684a62dd1cd30881a9e5c08d692474f841a312c4

43d4890 If the collection wasn't synched, we won't have it in the cache.

-----------------------------------------------------------------------

commit 43d489041ee279c3a7fda84d068c02f8dd33b330
Author: Michael J Rubinsky <mrubinsk at horde.org>
Date:   Wed Aug 28 00:33:07 2013 -0400

     If the collection wasn't synched, we won't have it in the cache.

  framework/ActiveSync/lib/Horde/ActiveSync/Collections.php |    6 ------
  1 files changed, 0 insertions(+), 6 deletions(-)

http://git.horde.org/horde-git/-/commit/43d489041ee279c3a7fda84d068c02f8dd33b330

yes :)

I think it's better to remove the non-working code in updatePingableFlag(),
otherwise we might get the wrong idea in a few months / years if
we need to look into this part of the code again.
(because Tizen or so does the wrong thing(tm))

Heh. Of course, now that you mention it, that seems completely 
obvious. If the collection was never synchronized, of course it won't 
be in the syncCache (unless the PING request was issued immediately 
after the first, syncKey == 0,  SYNC for the collection.

OTOH, it looks like the part that was wrong then was faking the 
detected changes to force the SYNC, since that is essentially the only 
difference in the code path (the collection is still not marked as 
PINGABLE). So, iOS is doing the Wrong Thing (tm) in PINGING the 
collection, but doing the Right Thing (tm)  by not expecting the 
server to do anything with the request.  I guess it wouldn't hurt to 
explicitly iterate $this->_collections and look for empty synckeys as 
a separate step. There shouldn't be that many of them to cause any 
extra overhead.
So, from your point of view, the ticket is resolved?

I've tested it now. It does work, but not as anticipated :o)

When the ping is processed we don't have the folder in the sync cache yet.
We added the collection in memory to "this->_collections" but it's not
part of the collection we get from the syncCache.

Please see the attached log, I've added a logger to dump
$this->_collections and $collections in updatePingableFlag().


Later on everything is fine, the device sends a Sync
for the folder and another Ping request.  Wild guess:
Through the modified behavior (we don't tell the client there are changes),
the device is behaving "correctly" now.

This was intentional and is what caused the PING request to terminate. 
Since, technically, it's an invalid request we were terminating the 
PING, and telling the client to issue a full SYNC.

This has now been changed to simply ignore the invalid request and 
behave as though the "bad" collection simply wasn't included in the 
request (while keeping it marked as PINGABLE so when it *is* 
available, we will start PINGing it).

Probably after Thursday, I'll better not pull this fix in before
the release to all productive boxes on Thursday ;)

Though I'm wondering why the previous code did a

----------------
     $this->setGetChangesFlag($id);
     $dataavailable = true;
----------------

in case of an "xxx_InvalidRequest" exception was thrown?
Was that a copy'n'paste error or intentional?

Thomas,

Can you check if these changes fix the pingable flag from getting lost 
in your case?

Changes have been made in Git (master):

commit ca2e7d4b96fa693891eccb5594b2cfe5eea4bbfa
Author: Michael J Rubinsky <mrubinsk@horde.org>
Date:   Mon Aug 26 12:02:13 2013 -0400

     Another place to work around broken clients.

     Broken clients like e.g., iOS, send a request to PING a collection
     before the collection is synchronized. This is a violation of the
     protocol (see MS-ASCMD 3.1.5.6). To work around this, continue
     to mark the collection as "pingable", but ignore the collection
     (and the exception that is thrown when we try to initialize the
     state).

     Possibly "fixes" Bug: 12605.

  .../lib/Horde/ActiveSync/Collections.php           |   15 ++++++++++++---
  1 files changed, 12 insertions(+), 3 deletions(-)

http://git.horde.org/horde-git/-/commit/ca2e7d4b96fa693891eccb5594b2cfe5eea4bbfa

Ok, something interesting happened: I keep the ActiveSync log file
open on the second screen and watch it from time to time.

The folders that went into "ping" state by accessing them
were "unsubscribed" from the PIM again:
The device sent a new "<Ping>" request without those folders,
now it only included the ones I initially configured.

So this is probably a feature from Apple: If you access a folder,
it adds it to the <Ping> list. This is reverted a few _hours_ later.

Let's do nothing for now from the horde side ;)

Eww. I know there is a PING status response for too many requested 
folders. I don't think we have a configuration for that yet though. If 
you create an enhancement request so I don't forget it, I'll add it.

I've reverted my patch after your verbose explanation how it should work.

The iOS 4 client sends every folder I ever accessed in the <Ping> 
folder list. Yuk.

Yup, that's probably a side effect of the patch.

My concern is, that since this is an undefined condition, if we don't 
send *something* back to the client about the unsync'd collection then 
it will not issue a full sync on it. I guess only testing will answer 
this, but I really hate working around broken client behavior with 
server responses that are essentially undefined for the context.
...and that's perfectly fine. What the client should do in this case 
is issue a SYNC request (which can even occur while any previous PING 
is still running) for the newly pushable folders. This will invalidate 
the cache that the PING command uses, so the PING will eventually 
terminate with the "Detected changes by another process" log notice, 
and when the client issues the next PING, it should be a full request 
that includes the newly pingable folders.
Only with your patch :)

The existing behavior is to only flag the collections that were 
included in the current PING requests FOLDERS element ($_collections), 
not all known collections ($collections in the foreach loop).

grrr, please forget about this. I've just browsed through
the logs and the client actually pings the folder after accessing it.
Hopefully it does reset that somehow to my configured
folder list , I don't want it to ping 100 folder or so every 15 seconds.

We don't have to ping the folder if we don't have a synckey yet
(as you mentioned we can't send changes anyway), we just skip it until 
we have a synckey.

We could modify the "<Ping>" handling to mark all wanted folders
as pingable unconditionally and remove the "pingable" flag
from any folder that is not in the list.

If the PIM sends "<Ping>" without a folder list, we check
which pingable folders have a sync key and skip those without one.
I don't think the new strategy costs any extra performance than the 
current one.


The broken client in question is iOS 4, I'll have to retest how iOS 6 behaves.
Though it's not that uncommon to configure an account, may be open the
INBOX to see if it's initially working and then go back to the settings menu
and select a bunch of folders than should be pushed through.


Looking through the code how updatePingable() works, I think
it sets the 'pingable' flag for any folder in the collection with a 
'synckey' field?!
Isn't that NOT the desired behavior?

That would explain why I see getServerChanges() calls
for any folder I've accessed on the PIM, even though
I just set the push mode for three of them...

For reference, here is the pertinent section of the specs requiring a 
previous SYNC:

http://msdn.microsoft.com/en-us/library/jj916344(v=exchg.80).aspx

If this is indeed what some clients are doing maybe instead of 
ignoring the empty synckey in the ping request and letting it fall 
through to raising an invalid request exception, we should just mark 
the collection as having found changes so the client can issue a true 
SYNC?

Note that I'm not sure how a client will handle a PING response 
indicating changes in a collection that has not yet been synced, so 
this would require some testing.

It shouldn't do this. A SYNC is a MUST before a PING.
We couldn't PING the folder anyway, even if it was marked as PINGABLE 
until we have a valid SYNC performed on the collection, so I'm not 
sure what value keeping track of the client's error in sending the 
PING for collection would be.  I'll have to reread the specs again to 
be sure I'm not missing something here, but I'm 99% positive that this 
is broken client behavior.

Why? The only time we need to update the flag is if we received the 
FOLDER elements (otherwise, it's an empty request so the collections 
we are marking as pingable are the same ones we just asked the cache 
if they were pingable).

  Also updatePingable()
The current object *should* be saved at the end of the PING request so 
this shouldn't be needed. However, looking at the code it appears that 
it's only saved if the PING detects changes. We should ensure it's 
saved for non-empty ping requests as well. Perhaps this is why the 
collection lost the flag?

If the client is issuing a PING for a collection that has not yet been 
SYNCed, then this is a client bug/issue. You can't PING a collection 
until it's been SYNCed.
DId the folder previously have a SYNC performed on it, before the PING?
It's not a nasty side effect, it's the intended behavior - see my 
earlier reply.
If the synckey doesn't match, what *should* happen is that an 
appropriate error code is sent back to the client indicating this 
(don't have the codes handy at the moment to know exactly which one). 
There is a specific code for telling the client that we can't accept 
an empty PING request. Once the client gets this code it should NOT 
issue another empty PING request, but (depending on the code we send) 
either a full SYNC, a FOLDERSYNC, or a full PING.
I'll have to look at this...

when the PIM requests a PING for a folder it didn't sync yet (=fresh account),
the synckey will be empty and we won't set the collection as pingable.

If I'm not mistaken, we need an enum (=state machine) here:
0 => not pingable
1 => PIM wants ping but we don't have a synckey
2 => pingable

what do you think?

ok, got it! I didn't see your reply while writing mine.

Note: There is no 'synckey' field in the collection.
It's called 'lastsynckey' !

Turns out $this->_collections was more or less ok.

I just did the following:
- Re-create the account on the PIM
- Subscribed (=ping) a folder I didn't sync in yet.

    The code path that handles a "<Ping>" with a folder list
    first adds the folder via addCollection() so we have a sync key for sure.

    Later in that code path we call updatePingableFlag().
    In fact that's the only place we call updatePingableFlag() at all.

    -> the newly subscribed folder was flagged as NOT PINGABLE.

Since collections::addCollection() does not update the collection in 
the SyncCache,
updatePingable() flag relied on the side effect of using
the $this->_collections array directly while working on the cache. Nasty.

There's more: If a collection gets reset because the sync key does not 
match anymore
and the PIM sends a <Ping> without a folder list, we will not set the 
PINGABLE flag again.


-> IMHO we should run collections->updatePingable() on every PING request
right before the "// Start waiting for changes" line. Also updatePingable()
should modify the cache _and_ the current object.

This isn't correct. The idea behind the "PINGABLE" flag is to remember 
which collections were requested in the last full PING request so that 
when the client issues an empty PING request the next time around we 
know which collections to ask for. Just because we have a collection 
(in the cache) with a synckey does not mean that we want to PING it.

This method should only be called during a full PING request (one in 
which all collections the client is interested in PINGing is 
included). $this->_collections contains all the *currently* loaded 
collections that were requested during the current request. 
$collections (in the loop) contains ALL collections we know about.

I think what is happening here is that for some reason, the client is 
issuing a PING request without the email folder, so the email 
collection's ping flag is reset. Why it is issuing this request is 
another story...wild guess - maybe an error in the email collection at 
some point prompted the client to stop trying?

Hi,

I just noticed that I had to open the email application on my iPod to 
see new messages,
email push was broken somehow. Reading through the logs I noticed it 
complained the INBOX "collection" is not pingable.

It turned out that the function to update the pingable state of a folder
a) checked the wrong key name and b) checked it on the wrong variable.
Two bugs in one line :o)

Attached patch fixes the issue.

I guess we need to fix this up in the database or use some other
kind of mechanism to trigger an updatePingableFlag() call:
I had to disable and re-enable email sync on the iPod to fix it.

Thomas