Skip to content

Commit b5495f0

Browse files
abhaysoodclaude
abhaysood
and
claude
committed
fix(android): prevent export of events with missing data files
Event data files can be deleted between batch creation and JSON serialization during export, causing null event data to be sent to the server, resulting in a nil pointer dereference panic. - DataCleanupService now only deletes files for unbatched events - NetworkClient skips events with missing data files during JSON serialization as a defense measure - Remove redundant pre-export file validation from Exporter - Remove unused FileStorage.validateFile Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 70e762d commit b5495f0

File tree

9 files changed

+272
-58
lines changed

9 files changed

+272
-58
lines changed

‎android/measure/src/main/java/sh/measure/android/exporter/Exporter.kt‎

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -144,24 +144,8 @@ internal class ExporterImpl(
144144
"Exporter: exporting batch ${batch.batchId} with ${events.size} events and ${spans.size} spans",
145145
)
146146

147-
// Remove events that have an invalid file path
148-
val validEvents = events.filter {
149-
if (it.serializedDataFilePath != null) {
150-
val isValid = fileStorage.validateFile(it.serializedDataFilePath)
151-
if (!isValid) {
152-
logger.log(
153-
LogLevel.Error,
154-
"Exporter: failed to read event data, discarding event ${it.eventId}",
155-
)
156-
}
157-
isValid
158-
} else {
159-
true
160-
}
161-
}
162-
163-
val response = networkClient.execute(batch.batchId, validEvents, spans)
164-
handleEventsExportResponse(response, batch.batchId, validEvents, spans)
147+
val response = networkClient.execute(batch.batchId, events, spans)
148+
handleEventsExportResponse(response, batch.batchId, events, spans)
165149

166150
return response is HttpResponse.Success
167151
}

‎android/measure/src/main/java/sh/measure/android/exporter/NetworkClient.kt‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,19 @@ internal class NetworkClientImpl(
118118
) {
119119
sink.writeUtf8("{\"events\":[")
120120

121-
eventPackets.forEachIndexed { index, event ->
122-
if (index > 0) sink.writeUtf8(",")
121+
var first = true
122+
eventPackets.forEach { event ->
123+
if (event.serializedDataFilePath != null &&
124+
fileStorage.getFile(event.serializedDataFilePath) == null
125+
) {
126+
logger.log(
127+
LogLevel.Error,
128+
"Exporter: event data file missing, skipping event ${event.eventId}",
129+
)
130+
return@forEach
131+
}
132+
if (!first) sink.writeUtf8(",")
133+
first = false
123134
writeEventPacket(sink, event)
124135
}
125136

@@ -194,10 +205,8 @@ internal class NetworkClientImpl(
194205
}
195206

196207
val source = file.inputStream().source()
197-
try {
208+
source.use { source ->
198209
sink.writeAll(source)
199-
} finally {
200-
source.close()
201210
}
202211
}
203212

‎android/measure/src/main/java/sh/measure/android/storage/DataCleanupService.kt‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -153,9 +153,11 @@ internal class DataCleanupServiceImpl(
153153
}
154154
database.getOldestSession()?.let {
155155
if (it != currentSessionId) {
156-
val eventIds = database.getEventsForSession(it)
157-
fileStorage.deleteEventsIfExist(eventIds)
158-
val attachmentIds = database.getAttachmentsForEvents(eventIds)
156+
// Only delete files for events not in a batch. Batched events
157+
// are being exported and their files will be cleaned up after export.
158+
val unbatchedEventIds = database.getUnbatchedEventsForSession(it)
159+
fileStorage.deleteEventsIfExist(unbatchedEventIds)
160+
val attachmentIds = database.getAttachmentsForEvents(unbatchedEventIds)
159161
fileStorage.deleteAttachmentsIfExist(attachmentIds)
160162

161163
// deleting sessions from db will also delete events, spans and attachments for the session

‎android/measure/src/main/java/sh/measure/android/storage/Database.kt‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,13 @@ internal interface Database : Closeable {
9494
*/
9595
fun getEventsForSession(session: String): List<String>
9696

97+
/**
98+
* Returns event IDs belonging to a session that are not part of any batch.
99+
*
100+
* @param session The session ID.
101+
*/
102+
fun getUnbatchedEventsForSession(session: String): List<String>
103+
97104
/**
98105
* Returns the total count of events across all sessions.
99106
*/
@@ -568,6 +575,18 @@ internal class DatabaseImpl(
568575
return eventIds
569576
}
570577

578+
override fun getUnbatchedEventsForSession(session: String): List<String> {
579+
val eventIds = mutableListOf<String>()
580+
readableDatabase.rawQuery(Sql.getUnbatchedEventsForSession(session), null).use {
581+
while (it.moveToNext()) {
582+
val eventIdIndex = it.getColumnIndex(EventTable.COL_ID)
583+
val eventId = it.getString(eventIdIndex)
584+
eventIds.add(eventId)
585+
}
586+
}
587+
return eventIds
588+
}
589+
571590
override fun getEventsCount(): Int {
572591
val count: Int
573592
readableDatabase.rawQuery(Sql.getEventsCount(), null).use {

‎android/measure/src/main/java/sh/measure/android/storage/DbConstants.kt‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,15 @@ internal object Sql {
306306
WHERE ${EventTable.COL_SESSION_ID} = '$session'
307307
""".trimIndent()
308308

309+
fun getUnbatchedEventsForSession(session: String): String = """
310+
SELECT e.${EventTable.COL_ID}
311+
FROM ${EventTable.TABLE_NAME} e
312+
LEFT JOIN ${EventsBatchTable.TABLE_NAME} eb
313+
ON e.${EventTable.COL_ID} = eb.${EventsBatchTable.COL_EVENT_ID}
314+
WHERE e.${EventTable.COL_SESSION_ID} = '$session'
315+
AND eb.${EventsBatchTable.COL_EVENT_ID} IS NULL
316+
""".trimIndent()
317+
309318
fun getAttachmentsForEvents(events: List<String>): String = """
310319
SELECT ${AttachmentV1Table.COL_ID}
311320
FROM ${AttachmentV1Table.TABLE_NAME}

‎android/measure/src/main/java/sh/measure/android/storage/FileStorage.kt‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -95,10 +95,6 @@ internal interface FileStorage {
9595
*/
9696
fun getConfigPath(): String
9797

98-
/**
99-
* Validates that the file exists.
100-
*/
101-
fun validateFile(path: String): Boolean
10298
}
10399

104100
private const val MEASURE_DIR = "measure"
@@ -199,11 +195,6 @@ internal class FileStorageImpl(
199195

200196
override fun getConfigPath(): String = "$rootDir/$MEASURE_DIR/$CONFIG_FILE_NAME"
201197

202-
override fun validateFile(path: String): Boolean {
203-
val file = getFile(path)
204-
return file != null && file.exists() && file.length() > 0
205-
}
206-
207198
override fun getFile(path: String): File? {
208199
val file = File(path)
209200
return when {

‎android/measure/src/test/java/sh/measure/android/exporter/ExporterTest.kt‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -402,29 +402,6 @@ internal class ExporterTest {
402402
)
403403
}
404404

405-
@Test
406-
fun `filters out events with invalid serialized data file path`() {
407-
whenever(networkClient.execute(any(), any(), any())).thenReturn(HttpResponse.Success())
408-
409-
insertSessionInDb("session1")
410-
insertEventInDb("session1", "event1", sampled = true)
411-
insertEventInDb(
412-
sessionId = "session1",
413-
eventId = "event2",
414-
sampled = true,
415-
serializedDataFilePath = "/nonexistent/path/data.json",
416-
)
417-
insertBatchInDb("batch1", eventIds = setOf("event1", "event2"))
418-
419-
exporter.export()
420-
421-
verify(networkClient).execute(
422-
eq("batch1"),
423-
argThat { size == 1 && first().eventId == "event1" },
424-
any(),
425-
)
426-
}
427-
428405
@Test
429406
fun `uploads attachments after events export`() {
430407
val responseJson = attachmentResponseJson("attachment-1")

‎android/measure/src/test/java/sh/measure/android/exporter/NetworkClientTest.kt‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,18 @@
11
package sh.measure.android.exporter
22

3+
import okio.Buffer
34
import org.junit.Assert.assertEquals
45
import org.junit.Assert.assertTrue
56
import org.junit.Test
67
import org.mockito.ArgumentMatchers.anyString
78
import org.mockito.Mockito.mock
89
import org.mockito.Mockito.`when`
910
import org.mockito.kotlin.any
11+
import org.mockito.kotlin.argumentCaptor
1012
import org.mockito.kotlin.eq
13+
import org.mockito.kotlin.isNull
1114
import org.mockito.kotlin.verify
15+
import sh.measure.android.events.EventType
1216
import sh.measure.android.fakes.FakeConfigProvider
1317
import sh.measure.android.fakes.NoopLogger
1418
import sh.measure.android.logger.LogLevel
@@ -165,4 +169,101 @@ class NetworkClientTest {
165169
any(),
166170
)
167171
}
172+
173+
@Test
174+
fun `execute skips events with missing data file and logs error`() {
175+
val mockLogger = mock<Logger>()
176+
val client = NetworkClientImpl(
177+
logger = mockLogger,
178+
fileStorage = fileStorage,
179+
httpClient = httpClient,
180+
configProvider = configProvider,
181+
).apply {
182+
init(apiKey = "secret", baseUrl = "http://localhost:8080")
183+
}
184+
185+
val event = EventPacket(
186+
eventId = "event-1",
187+
sessionId = "session-1",
188+
timestamp = "2024-01-01T00:00:00.000Z",
189+
type = EventType.EXCEPTION,
190+
userTriggered = false,
191+
serializedData = null,
192+
serializedDataFilePath = "missing-file.json",
193+
serializedAttachments = null,
194+
serializedAttributes = "{}",
195+
serializedUserDefinedAttributes = null,
196+
)
197+
198+
`when`(fileStorage.getFile("missing-file.json")).thenReturn(null)
199+
200+
val jsonWriterCaptor = argumentCaptor<(okio.BufferedSink) -> Unit>()
201+
`when`(httpClient.sendJsonRequest(anyString(), anyString(), any(), any())).thenReturn(
202+
HttpResponse.Success(),
203+
)
204+
205+
client.execute("batch123", listOf(event), emptyList())
206+
207+
verify(httpClient).sendJsonRequest(anyString(), anyString(), any(), jsonWriterCaptor.capture())
208+
val buffer = Buffer()
209+
jsonWriterCaptor.firstValue.invoke(buffer)
210+
val json = buffer.readUtf8()
211+
212+
assertEquals("{\"events\":[],\"spans\":[]}", json)
213+
verify(mockLogger).log(
214+
eq(LogLevel.Error),
215+
eq("Exporter: event data file missing, skipping event event-1"),
216+
isNull(),
217+
)
218+
}
219+
220+
@Test
221+
fun `execute includes events with valid inline data alongside skipped events`() {
222+
val event1 = EventPacket(
223+
eventId = "event-1",
224+
sessionId = "session-1",
225+
timestamp = "2024-01-01T00:00:00.000Z",
226+
type = EventType.STRING,
227+
userTriggered = false,
228+
serializedData = "{\"key\":\"value\"}",
229+
serializedDataFilePath = null,
230+
serializedAttachments = null,
231+
serializedAttributes = "{}",
232+
serializedUserDefinedAttributes = null,
233+
)
234+
235+
val event2 = EventPacket(
236+
eventId = "event-2",
237+
sessionId = "session-1",
238+
timestamp = "2024-01-01T00:00:01.000Z",
239+
type = EventType.EXCEPTION,
240+
userTriggered = false,
241+
serializedData = null,
242+
serializedDataFilePath = "missing-file.json",
243+
serializedAttachments = null,
244+
serializedAttributes = "{}",
245+
serializedUserDefinedAttributes = null,
246+
)
247+
248+
`when`(fileStorage.getFile("missing-file.json")).thenReturn(null)
249+
250+
val jsonWriterCaptor = argumentCaptor<(okio.BufferedSink) -> Unit>()
251+
`when`(httpClient.sendJsonRequest(anyString(), anyString(), any(), any())).thenReturn(
252+
HttpResponse.Success(),
253+
)
254+
255+
networkClient.execute("batch123", listOf(event1, event2), emptyList())
256+
257+
verify(httpClient).sendJsonRequest(anyString(), anyString(), any(), jsonWriterCaptor.capture())
258+
val buffer = Buffer()
259+
jsonWriterCaptor.firstValue.invoke(buffer)
260+
val json = buffer.readUtf8()
261+
262+
// event-1 should be present, event-2 should be skipped
263+
assertTrue(json.contains("\"id\":\"event-1\""))
264+
assertTrue(!json.contains("\"id\":\"event-2\""))
265+
// Verify valid JSON structure - no trailing/leading commas
266+
assertTrue(json.startsWith("{\"events\":[{"))
267+
assertTrue(json.contains("}],\"spans\":[]}"))
268+
}
168269
}

0 commit comments

Comments
 (0)