Skip to content

Commit 6130f3a

Browse files
DzmitryFomchyngithub-actions[bot]
DzmitryFomchyn
authored and
github-actions[bot]
committed
Copilot fixes (#11619) (#11649)
* Fix PeriodicHistoryCleanupWorker * Fix copilot metadata endedAt writing. * Reduce copilot uploading delay * Don't retry copilot upload job for non-existing file * Increase copilot-error event version * Code review comments * Changelog GitOrigin-RevId: 33a1535c31df5f917561b3b6bc83d93dda8abbf3
1 parent 8bdaa92 commit 6130f3a

File tree

9 files changed

+322
-121
lines changed

9 files changed

+322
-121
lines changed

‎changelog/unreleased/bugfixes/11619.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
- Fixed Copilot issues that caused recordings to be lost.

‎copilot/src/main/java/com/mapbox/navigation/copilot/HistoryAttachmentsUtils.kt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ internal object HistoryAttachmentsUtils {
6565
File(from.parent, filename).also { from.renameTo(it) }
6666
}
6767

68+
fun create(parent: String?, child: String): File = File(parent, child)
69+
6870
private fun decode(str: String): JSONObject {
6971
val requiredLength = (str.length - 1) / 4 * 4 + 4
7072
return JSONObject(

‎copilot/src/main/java/com/mapbox/navigation/copilot/MapboxCopilotImpl.kt‎

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.mapbox.navigation.copilot
22

3+
import android.annotation.SuppressLint
34
import android.app.Application
45
import android.content.Context
56
import android.os.SystemClock
@@ -51,6 +52,10 @@ import java.io.File
5152
* MapboxCopilot.
5253
*
5354
* @property mapboxNavigation
55+
*
56+
* TODO there may remain synchronisation issues:
57+
* - make sure activeSession is accessed from only one thread
58+
* - writing current session file should also be synchronised
5459
*/
5560
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
5661
internal class MapboxCopilotImpl(
@@ -77,6 +82,7 @@ internal class MapboxCopilotImpl(
7782
}
7883
}
7984
private var startSessionTime: Long = 0
85+
8086
private var activeSession: CopilotSession = CopilotSession()
8187
private val finishedSessions = mutableListOf<CopilotSession>()
8288
private val filepaths = HistoryFiles(applicationContext)
@@ -257,14 +263,13 @@ internal class MapboxCopilotImpl(
257263
recording = recording,
258264
)
259265
logD("startRecording $activeSession")
260-
saveCopilotSession()
266+
activeSession.saveCopilotSession()
261267

262268
mapboxNavigation.registerArrivalObserver(arrivalObserver)
263269
restartRecordingHistoryJob = mainJobController.scope.launch {
264270
while (true) {
265271
delay(maxHistoryFileLengthMilliseconds)
266272
restartRecordingHistory()
267-
saveCopilotSession()
268273
}
269274
}
270275
}
@@ -277,8 +282,10 @@ internal class MapboxCopilotImpl(
277282
return eventJson
278283
}
279284

285+
@SuppressLint("ImplicitSamInstance")
280286
private fun restartRecordingHistory() {
281-
val session = activeSession.copy(endedAt = currentUtcTime())
287+
val session = copyAndSaveActiveSession { copy(endedAt = currentUtcTime()) }
288+
282289
logD("stopRecording $session")
283290
copilotHistoryRecorder.stopRecording { historyFilePath ->
284291
historyFilePath ?: return@stopRecording
@@ -295,10 +302,13 @@ internal class MapboxCopilotImpl(
295302
}
296303

297304
// when restarting recording we inherit previous session info and just update start time
298-
activeSession = activeSession.copy(
299-
recording = recording,
300-
startedAt = currentUtcTime(),
301-
)
305+
activeSession = copyAndSaveActiveSession {
306+
copy(
307+
recording = recording,
308+
startedAt = currentUtcTime(),
309+
endedAt = "",
310+
)
311+
}
302312
logD("startRecording $activeSession")
303313
}
304314

@@ -307,7 +317,7 @@ internal class MapboxCopilotImpl(
307317
return
308318
}
309319
if (hasFeedback || !shouldSendHistoryOnlyWithFeedback) {
310-
val session = activeSession.copy(endedAt = currentUtcTime())
320+
val session = copyAndSaveActiveSession { copy(endedAt = currentUtcTime()) }
311321

312322
pushOnActiveGuidance()
313323
pushDriveEndsEvent()
@@ -330,7 +340,7 @@ internal class MapboxCopilotImpl(
330340
}
331341
stopRecording { historyFilePath ->
332342
delete(File(historyFilePath))
333-
deleteCopilotSession()
343+
activeSession.deleteCopilotSession()
334344
activeSession = CopilotSession()
335345
}
336346
}
@@ -354,14 +364,24 @@ internal class MapboxCopilotImpl(
354364
}
355365
}
356366

357-
private fun saveCopilotSession() = mainJobController.scope.launch(Dispatchers.IO) {
358-
val file = File(filepaths.copilotAbsolutePath(), activeSession.saveFilename())
359-
file.writeText(activeSession.toJson())
367+
private fun copyAndSaveActiveSession(
368+
update: CopilotSession.() -> CopilotSession,
369+
): CopilotSession {
370+
val session = activeSession.update()
371+
session.saveCopilotSession()
372+
return session
360373
}
361374

362-
private fun deleteCopilotSession() = mainJobController.scope.launch(Dispatchers.IO) {
363-
delete(File(filepaths.copilotAbsolutePath(), activeSession.saveFilename()))
364-
}
375+
private fun CopilotSession.saveCopilotSession() =
376+
mainJobController.scope.launch(Dispatchers.IO) {
377+
val file = File(filepaths.copilotAbsolutePath(), saveFilename())
378+
file.writeText(toJson())
379+
}
380+
381+
private fun CopilotSession.deleteCopilotSession() =
382+
mainJobController.scope.launch(Dispatchers.IO) {
383+
delete(File(filepaths.copilotAbsolutePath(), saveFilename()))
384+
}
365385

366386
private fun stopRecording(callback: (String) -> Unit) {
367387
restartRecordingHistoryJob?.cancel()
@@ -450,7 +470,11 @@ internal class MapboxCopilotImpl(
450470
internal fun reportCopilotError(description: String) {
451471
logD("reportCopilotError($description)", LOG_CATEGORY)
452472
StandaloneNavigationTelemetry.getOrCreate().sendEvent(
453-
StandaloneCustomEvent(type = "copilot-error", payload = description),
473+
StandaloneCustomEvent(
474+
type = "copilot-error",
475+
payload = description,
476+
customEventVersion = "2.0",
477+
),
454478
)
455479
}
456480
}

‎copilot/src/main/java/com/mapbox/navigation/copilot/internal/CopilotSession.kt‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import com.mapbox.navigation.base.internal.options.getOwner
99
import com.mapbox.navigation.base.internal.utils.MapboxOptionsUtil.getTokenForService
1010
import com.mapbox.navigation.base.options.EventsAppMetadata
1111
import com.mapbox.navigation.base.options.NavigationOptions
12+
import com.mapbox.navigation.copilot.HistoryAttachmentsUtils.attachmentFilename
1213
import com.mapbox.navigation.copilot.HistoryAttachmentsUtils.retrieveNavNativeSdkVersion
1314
import com.mapbox.navigation.copilot.HistoryAttachmentsUtils.retrieveNavSdkVersion
1415
import com.mapbox.navigation.copilot.HistoryAttachmentsUtils.retrieveOwnerFrom
@@ -49,6 +50,12 @@ data class CopilotSession(
4950

5051
companion object {
5152

53+
val CopilotSession.attachmentFile: File
54+
get() = File(File(recording).parent, attachmentFilename(this))
55+
56+
val CopilotSession.recordingFile: File
57+
get() = File(recording)
58+
5259
fun fromJson(json: String): Result<CopilotSession> = runCatching {
5360
Gson().fromJson(json, CopilotSession::class.java)
5461
}

‎copilot/src/main/java/com/mapbox/navigation/copilot/work/HistoryUploadWorker.kt‎

Lines changed: 83 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
1919
import com.mapbox.navigation.copilot.AttachmentMetadata
2020
import com.mapbox.navigation.copilot.HistoryAttachmentsUtils
2121
import com.mapbox.navigation.copilot.HistoryAttachmentsUtils.attachmentFilename
22+
import com.mapbox.navigation.copilot.HistoryAttachmentsUtils.create
2223
import com.mapbox.navigation.copilot.HistoryAttachmentsUtils.generateSessionId
2324
import com.mapbox.navigation.copilot.HistoryAttachmentsUtils.rename
2425
import com.mapbox.navigation.copilot.HttpServiceProvider
@@ -35,6 +36,7 @@ import com.mapbox.navigation.copilot.internal.CopilotSession
3536
import com.mapbox.navigation.copilot.internal.PushStatus
3637
import com.mapbox.navigation.copilot.internal.saveFilename
3738
import com.mapbox.navigation.utils.internal.logD
39+
import com.mapbox.navigation.utils.internal.logE
3840
import com.mapbox.navigation.utils.internal.logW
3941
import kotlinx.coroutines.Dispatchers
4042
import kotlinx.coroutines.suspendCancellableCoroutine
@@ -48,7 +50,7 @@ import kotlin.coroutines.resume
4850
*/
4951
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
5052
internal class HistoryUploadWorker(
51-
context: Context,
53+
private val context: Context,
5254
private val workerParams: WorkerParameters,
5355
) : CoroutineWorker(context, workerParams) {
5456

@@ -65,14 +67,29 @@ internal class HistoryUploadWorker(
6567
val copilotSession = copilotSessionFrom(workerParams.inputData)
6668
val recordingFile =
6769
rename(File(copilotSession.recording), attachmentFilename(copilotSession))
68-
val sessionFile = File(recordingFile.parent, copilotSession.saveFilename())
70+
val sessionFile = create(recordingFile.parent, copilotSession.saveFilename())
71+
72+
logD(
73+
"HistoryUploadWorker.doWork(). " +
74+
"Session: $copilotSession, " +
75+
"runAttemptCount: $runAttemptCount",
76+
)
77+
78+
if (copilotSession.endedAt.isEmpty()) {
79+
reportCopilotError(
80+
"Passed copilot session has empty endedAt date: $copilotSession, " +
81+
"Copilot dir files: ${recordingFile.parentFile?.listFiles()?.map { it.name }}",
82+
)
83+
}
6984

7085
if (!recordingFile.exists()) {
7186
reportCopilotError(
7287
"History file does not exist: ${recordingFile.name}. " +
7388
"Copilot session: $copilotSession. " +
7489
"Copilot dir files: ${recordingFile.parentFile?.listFiles()?.map { it.name }}",
7590
)
91+
cleanup(copilotSession, recordingFile, sessionFile)
92+
return@withContext Result.failure()
7693
}
7794

7895
if (!sessionFile.exists()) {
@@ -107,15 +124,13 @@ internal class HistoryUploadWorker(
107124
if (uploadHistoryFile(uploadOptions)) {
108125
success(copilotSession)
109126
logD("Result.success(${recordingFile.name}|${sessionFile.name})")
110-
delete(recordingFile)
111-
delete(sessionFile)
127+
cleanup(copilotSession, recordingFile, sessionFile)
112128
Result.success()
113129
} else {
114130
failure(copilotSession)
115131
if (runAttemptCount >= MAX_RUN_ATTEMPT_COUNT) {
116132
logW("Result.failure(${recordingFile.name}|${sessionFile.name})")
117-
delete(recordingFile)
118-
delete(sessionFile)
133+
cleanup(copilotSession, recordingFile, sessionFile)
119134
Result.failure()
120135
} else {
121136
logD(
@@ -129,9 +144,37 @@ internal class HistoryUploadWorker(
129144
}
130145
}
131146

132-
private fun delete(file: File) {
147+
private fun cleanup(
148+
copilotSession: CopilotSession,
149+
recordingFile: File,
150+
sessionFile: File,
151+
) {
152+
if (!delete(sessionFile)) {
153+
reportCopilotError("Can't delete session file")
154+
}
155+
156+
if (!delete(recordingFile)) {
157+
reportCopilotError("Can't delete recording file")
158+
}
159+
160+
/**
161+
* Cancels any pending upload jobs that may have been scheduled by
162+
* [PeriodicHistoryCleanupWorker].
163+
*
164+
* NOTE: TODO This does not fully guarantee that a new job won’t be scheduled if
165+
* [PeriodicHistoryCleanupWorker] is currently running.
166+
*/
167+
cancelScheduledUploading(context, copilotSession)
168+
}
169+
170+
private fun delete(file: File): Boolean {
133171
logD("Deleting ${file.name}")
134-
HistoryAttachmentsUtils.delete(file)
172+
return try {
173+
HistoryAttachmentsUtils.delete(file)
174+
} catch (e: SecurityException) {
175+
logE("Deleting ${file.name} error: $e")
176+
false
177+
}
135178
}
136179

137180
private suspend fun uploadHistoryFile(
@@ -215,6 +258,7 @@ internal class HistoryUploadWorker(
215258

216259
private fun logD(msg: String) = logD("[upload] [$id] $msg", LOG_CATEGORY)
217260
private fun logW(msg: String) = logW("[upload] [$id] $msg", LOG_CATEGORY)
261+
private fun logE(msg: String) = logE("[upload] [$id] $msg", LOG_CATEGORY)
218262

219263
internal companion object {
220264

@@ -235,23 +279,30 @@ internal class HistoryUploadWorker(
235279

236280
/**
237281
* Max backoff delay is limited by [WorkRequest.MAX_BACKOFF_MILLIS] (5 hours = 18 000 seconds).
282+
* With [MAX_RUN_ATTEMPT_COUNT] = 15, there are 15 total attempts (1 initial + 14 retries).
238283
* The total cumulative retry time could be approximately 20 hours
239284
* (it can be more depending on system delays):
240285
*
241-
* 300 * 2⁰ = 300
242-
* 300 * 2¹ = 600
243-
* 300 * 2² = 1200
244-
* 300 * 2³ = 2400
245-
* 300 * 2⁴ = 4800
246-
* 300 * 2⁵ = 9600
247-
* 300 * 2⁶ = 19200 -> capped to 18000
248-
* 300 * 2⁷ = 38400 -> capped to 18000
249-
* 300 × 2⁸ = 76800 -> caped to 18000
286+
* Attempt 1: runs immediately (no delay)
287+
* Delay before attempt 2: 10 * 2⁰ = 10
288+
* Delay before attempt 3: 10 * 2¹ = 20
289+
* Delay before attempt 4: 10 * 2² = 40
290+
* Delay before attempt 5: 10 * 2³ = 80
291+
* Delay before attempt 6: 10 * 2⁴ = 160
292+
* Delay before attempt 7: 10 * 2⁵ = 320
293+
* Delay before attempt 8: 10 * 2⁶ = 640
294+
* Delay before attempt 9: 10 * 2⁷ = 1280
295+
* Delay before attempt 10: 10 * 2⁸ = 2560
296+
* Delay before attempt 11: 10 * 2⁹ = 5120
297+
* Delay before attempt 12: 10 * 2¹⁰ = 10240
298+
* Delay before attempt 13: 10 * 2¹¹ = 20480 -> capped to 18000
299+
* Delay before attempt 14: 10 * 2¹² = 40960 -> capped to 18000
300+
* Delay before attempt 15: 10 * 2¹³ = 81920 -> capped to 18000
250301
*
251-
* Total delay is 72900 seconds ≈ 20 hours
302+
* Total cumulative delay is 74470 seconds ≈ 20 hours
252303
*/
253-
const val MAX_RUN_ATTEMPT_COUNT = 9
254-
private const val DELAY_IN_SECONDS = 300L // 5 minutes
304+
const val MAX_RUN_ATTEMPT_COUNT = 15
305+
private const val DELAY_IN_SECONDS = 10L
255306

256307
/**
257308
* uploadHistory
@@ -260,7 +311,6 @@ internal class HistoryUploadWorker(
260311
context: Context,
261312
copilotSession: CopilotSession,
262313
) {
263-
val workName = "copilot-upload.${copilotSession.recording}"
264314
val workRequest = OneTimeWorkRequestBuilder<HistoryUploadWorker>()
265315
.setConstraints(requireInternet())
266316
.setBackoffCriteria(BackoffPolicy.EXPONENTIAL, DELAY_IN_SECONDS, TimeUnit.SECONDS)
@@ -269,9 +319,20 @@ internal class HistoryUploadWorker(
269319
.build()
270320

271321
WorkManager.getInstance(context)
272-
.enqueueUniqueWork(workName, ExistingWorkPolicy.KEEP, workRequest)
322+
.enqueueUniqueWork(
323+
copilotSession.workName(),
324+
ExistingWorkPolicy.KEEP,
325+
workRequest,
326+
)
273327
}
274328

329+
fun cancelScheduledUploading(context: Context, copilotSession: CopilotSession) {
330+
WorkManager.getInstance(context)
331+
.cancelUniqueWork(copilotSession.workName())
332+
}
333+
334+
private fun CopilotSession.workName() = "copilot-upload.$recording"
335+
275336
private fun requireInternet() =
276337
Constraints.Builder().setRequiredNetworkType(NetworkType.CONNECTED).build()
277338

0 commit comments

Comments
 (0)