-
Notifications
You must be signed in to change notification settings - Fork 7
Unexpected TimeTravel in SongBook Studio #272
Comments
Also, for the record, I remember one other time that @elsigh and I noticed more clientIDs talking to the server than we expected. We though we only had one live client, but we saw two client IDs in Firestore db (or something like that). So there might be a bug in the creation of client IDs. |
Possibly more likely, or one of the above in combination with: the client has fresher data than the diffserver. Client sends pull request with lastmutationid N and stateid S. Diffserver does clientview request, gets a 401. Diffserver is now going to try to send the client the best diff it has: https://github.com/rocicorp/diff-server/blob/6528d96191aa0f633e2b4df3e8fd9afd78f96702/serve/pull.go#L99. If it can't find the client's state, it does a full sync: https://github.com/rocicorp/diff-server/blob/6528d96191aa0f633e2b4df3e8fd9afd78f96702/db/diff.go#L59. But... if the diffserver's head for this client is empty, it is going to try to sync back to mutationid 0 and the initial server state. Doh. The client rightfully disallows going backwards in time from N to 0 and beginsync errors. When it errors the fact that the client view returned 401 is lost. (This last part is a classic example of why errors should be signaled within the rpc response, rather than outside of it. More grist for the #119 mill.) This is a bug in the diffserver. The diffserver should refuse to send a patch if its head state lastmutationid is < the client's. This gets tricky though: what to do when they are equal? On one hand: we have to send the patch because the client view could have returned us new state with the same lastmutationid (state changed out of band). On the other hand suppose the client gets (LMID1, SSID1) from the diffserver. Then the client pulls new state (LMID1, SSID2) from the diffserver. Something happens in the diffserver where it forgets about (LMID1, SSID2). Client pulls again; the diffserver thinks (LMID1, SSID1) is head and so sends a diff to SSID1. Client has gone backwards. We happen to know that this kind of failure is unlikely in the diffserver so for now we can ignore it and always return the state the diffserver has if last mutation id is >= what the client has. But we should address it once the current dust settles. I am going to timebox 45m to try to fix the diffserver issue rn on my personal laptop, will see how that goes. BTW aaron as you know logging is hardcoded to info: Line 28 in d2e03db
|
Note: seems like the diffserver needs an explicit way to tell the client nop. I'm going to have to reflect the client's lmid and ssid back to it for now. please keep this issue open even after the proximate error has been fixed until i can properly spin out followups we need |
On Sun, Nov 15, 2020 at 2:11 PM Phritz ***@***.***> wrote:
If it can't find the client's state, it does a full sync:
https://github.com/rocicorp/diff-server/blob/6528d96191aa0f633e2b4df3e8fd9afd78f96702/db/diff.go#L59.
How could this happen? The client has a previous lastMutationID. So it
successfully synced before.
… But... if the diffserver's head for this client is empty, it is going to
try to sync back to mutationid 0 and the initial server state.
How could that happen? Where did the head go?
Doh. The client rightfully disallows going backwards in time from N to 0
and beginsync errors. When it errors the fact that the client view returned
401 is lost. (This last part is a classic example of why errors should be
signaled *within* the rpc response, rather than outside of it. More grist
for the #119 <#119> mill.)
This is a bug in the diffserver. The diffserver should refuse to send a
patch if its head state lastmutationid is < the client's. This gets tricky
though: what to do when they are equal? On one hand: we have to send the
patch because the client view could have returned us new state with the
same lastmutationid (state changed out of band). On the other hand suppose
the client gets (LMID1, SSID1) from the diffserver. Then the client pulls
new state (LMID1, SSID2) from the diffserver. Something happens in the
diffserver where it forgets about (LMID1, SSID2). Client pulls again; the
diffserver thinks (LMID1, SSID1) is head and so sends a diff to SSID1.
Client has gone backwards. We happen to know that this kind of failure is
unlikely in the diffserver so for now we can ignore it and always return
the state the diffserver has if last mutation id is >= what the client has.
But we should address it once the current dust settles.
I am going to timebox 45m to try to fix the diffserver issue rn on my
personal laptop, will see how that goes.
BTW aaron as you know logging is hardcoded to info:
https://github.com/rocicorp/repc/blob/d2e03db36a9a436d32da60abc9a863543384ba18/src/wasm.rs#L28.
We're not going to get any useful information at that level. Suggest
setting to debug and rebuilding. (Assume head of master is in an
incompatible state.) And then we really need to get on #243
<#243>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#272 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAATUBBMNQ77UZBWAVIQAYDSQBU3ZANCNFSM4TWPTECA>
.
|
because the diffserver treats what the clientview says as the source of truth, and will happily overwrite its head with whatever the clientview says, no matter whether it is sensible or not. so like if diffserver has state with lmid 22 and the client view returns state with lmid 10, the diffserver will happily treat that as canonical. the client will reject it. plausible explanation for what happened here:
i'm a little fuzzy on how the client view could return a lmid > 0 at some point and then start returning 0. if that didn't happen, i'm worried because how did the client get a lmid > 0? clearly, we need to proof the protocol better against data layer mistakes. above i said that the diffserver should not attempt to send the client a diff if the client's lmid is > what the diffserver has. that will require the client to send along its lmid which is both a repc and diffserver change. seems like we maybe should also not let the diffserver go back in time, dont accept a clientview with lmid < what the diffserver has. medium term we need to have structured errors at dispatch at least so we can signal the 401 regardless of sync success. |
(read the edited version of my last comment, corrected a few errors) BTW this issue is not going to be sorted today, or maybe even tomorrow. But the good news is that the problems here only happen if the clientview does crazy stuff. If you nuke the client and reset server lmid to 0 and the server returns the lmid faithfully then replicache should work fine. (until you hit the next bug ;) |
Thank you for this analysis, it is amazing. It is definitely true that @elsigh's data layer went from returning 0 for lmid to returning >0. However, I can't think of any reason it would have flapped back. @elsigh is it possible you deployed a client view that returned zero at some point after the one that returned the correct value? Otherwise I buy this. |
ok mystery solved: the clientview did return lmid 0 after 43 for some reason (old instance, stale data, dunno). here are followups:
for discussion (comments please):
|
👊🏽
Seems worthwhile. Have to be careful to not slow down Replicache itself :-/.
Kind of ambivalent on this. I'm fine with just using Noms, but if you'd rather have a webview that's fine with me, you're going to be the main customer.
-1 from me. I don't think most customers will have such a version available.
|
OK you're right, easy for a given client id. But we couldn't do that for all clients of a given customer because we don't keep that association anywhere. I think we should keep that association both ways (account id => client ids and also clientid => account id). The account service would be the place for that if it was running but for now the diffserver could could keep it in a separate map in noms. |
On Tue, Nov 17, 2020 at 8:22 AM Phritz ***@***.***> wrote:
OK you're right, easy for a given client id. But we couldn't do that for
all clients of a given customer because we don't keep that association
anywhere.
noms ds <customer-db> | xargs -I{} noms ds -d <customer-db>::{}
:)
I think we should keep that association both ways (account id => client
ids and also clientid => account id). The account service would be the
place for that if it was running but for now the diffserver could could
keep it in a separate map in noms.
We don't have an easy way to go from client id -> account id because Noms
is non-relational :-/. Actually given just a client ID there's no good way
(in just the diff server) to even find it. You need to know the account id
too.
… —
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#272 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAATUBDX33JHCNCHNT6OKSLSQK5ORANCNFSM4TWPTECA>
.
|
We get both in the pull request (client id comes in the body and account id comes in the Authorization header). |
spun out rocicorp/diff-server#78 to store clientid/account association. we get get to it later. think we are done here |
@elsigh got stuck on his Replicache integration because push and pull were failing with auth errors, but the
getDataLayerAuth
callback was also not getting called.I looked at the logs and saw "TimeTravelProhibited" in the console (sadly I don't have a screencap of this -- @elsigh ?)
When we looked at the Pull response we saw that the ClientView response was HTTP 401, and that was correctly getting sent down to client, and the lastMutationID was 0.
But the client had synced before so its base lastMutationID was > 0, thus time travel, and therefore sync was failing before it got to reauthing.
It seems like the only way this can happen is if either the client ID changes on the client somehow, or if the client data gets dropped on the server. It's a mystery how either of these could occur.
The text was updated successfully, but these errors were encountered: