google / ground-android

Ground mobile data collection app for Android
http://groundplatform.org
Apache License 2.0
243 stars 114 forks source link

[Sync status] App crashes when sync status screen open and transitioning from "MEDIA_UPLOADED" to "COMPLETE" #2314

Closed gino-m closed 2 weeks ago

gino-m commented 6 months ago

After disabling Airplane mode:

From Crashlytics:

Fatal Exception: com.google.android.ground.persistence.local.LocalDataConsistencyException: Unknown jobId in submission mutation 1
       at com.google.android.ground.persistence.local.room.converter.ConverterExtKt.toModelObject(ConverterExt.kt:341)
       at com.google.android.ground.persistence.local.room.stores.RoomSubmissionStore$getAllSurveyMutationsFlow$$inlined$map$1$2.emit(Emitters.kt:224)
       at kotlinx.coroutines.flow.internal.SafeCollectorKt$emitFun$1.invoke(SafeCollector.kt:15)
       at kotlinx.coroutines.flow.internal.SafeCollectorKt$emitFun$1.invoke(SafeCollector.kt:15)
       at kotlinx.coroutines.flow.internal.SafeCollector.emit(SafeCollector.kt:87)
       at kotlinx.coroutines.flow.internal.SafeCollector.emit(SafeCollector.kt:66)
       at kotlinx.coroutines.flow.FlowKt__ChannelsKt.emitAllImpl$FlowKt__ChannelsKt(Channels.kt:37)
       at kotlinx.coroutines.flow.FlowKt__ChannelsKt.access$emitAllImpl$FlowKt__ChannelsKt(Channels.kt:1)
       at kotlinx.coroutines.flow.FlowKt__ChannelsKt$emitAllImpl$1.invokeSuspend(:14)
       at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
       at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
       at android.os.Handler.handleCallback(Handler.java:959)
       at android.os.Handler.dispatchMessage(Handler.java:100)
       at android.os.Looper.loopOnce(Looper.java:232)
       at android.os.Looper.loop(Looper.java:317)
       at android.app.ActivityThread.main(ActivityThread.java:8501)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:552)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:878)

Note that conversions shouldn't crash app. We should always catch all errors and return Result instead of bare values.

The data is uploaded, but the status screen then gets stuck with the following:

image

@scolsen FYI

jcqli commented 6 months ago

@scolsen This is related to the sync status that you had worked on, do you have bandwidth to look into this by Wed?

scolsen commented 5 months ago

We should always catch all errors and return Result instead of bare values.

Yes, definitely. I'd go so far as to say exceptions are now a code smell since we have error value types. In general, error value types will be much better for us given that we use functional streams extensively, which do not cohere well with exceptions.

In the meantime, I took a simpler route and just added a catch call. I'm not sure what the underlying cause is here. I've added some additional debugging to the error message in efforts to help repro—looking at the code, we just copy the previous mutation and only update its state, so it's not obvious to me where/why the Job ID would change if that's actually the cause of the exception here.

jcqli commented 5 months ago

https://github.com/google/ground-android/pull/2333 is a workaround for this crash, I'd like to leave this open so that we can get to the root issue (but considered "done" for the 3/13 freeze)

jcqli commented 5 months ago

Based on my prev comment, going to pull this into iteration 7

gino-m commented 4 weeks ago

@scolsen I think this has already been fixed, can you kindly confirm?

gino-m commented 2 weeks ago

Closing as inactive.