Details

Reviewers

michal
kamil

Commits

rCOMM30a703aec0fd: Implement unencrypted log capture without attachments

Summary

This differential implements API that enables to monitor and capture backup logs. At this point logs are unencrypted, without attachments. Future differentials will improve those deficiencies. This diff doesn't break the app since log capture is not actually called yet.

How does log capture work?

We attach session to database connection and session starts to accumulate logs.
Each time we call captureLogs we read logID from metadata table in database, store log to file identified by this logID and finally write incremented logID to the database.

Test Plan

Add a call to DatabaseManager::getQueryExecutor().captureBackupLogs() just before DatabaseManager::getQueryExecutor().commitTransaction() in BaseDataStore.h.
Use the app: make drafts, send messages, update threads colours etc..
Periodically download app container from XCode.
Ensure that there are created backup-0-log-xxx files that contain unreadable bytes mixed with readable strings with database table names and database entities fields values.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

marcin created this revision.Jan 26 2024, 6:42 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 26 2024, 6:42 AM

marcin added a parent revision: D10833: Bump SQLCipher Amalgamation version to enable sqlite3 session extension.Jan 26 2024, 6:43 AM

marcin edited the summary of this revision. (Show Details)Jan 26 2024, 6:50 AM

marcin edited the test plan for this revision. (Show Details)

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 26 2024, 7:00 AM

Harbormaster failed remote builds in B26182: Diff 36164!

marcin edited the summary of this revision. (Show Details)Jan 26 2024, 7:02 AM

marcin edited the summary of this revision. (Show Details)Jan 26 2024, 7:04 AM

marcin added inline comments.Jan 26 2024, 7:12 AM

native/cpp/CommonCpp/DatabaseManagers/SQLiteConnectionManager.cpp
12 ↗	(On Diff #36164)	I don't want to create new file for each log. I think we should rather accumulate `X` logs in a single file and the create new file with new log id. This constant is `X`. We can set it to arbitrary number.
114 ↗	(On Diff #36164)	This method handles the case where we were storing logs to file identified by certain `logID` but the app was closed/killed before `logsCount` exceeded threshold and/or before it could update `logID` in the database. In such case we have to increment `logID` and start new file.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
643 ↗	(On Diff #36164)	This code is not needed anymore.
native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.h
24 ↗	(On Diff #36164)	I decided to couple database connection and logs session ina single object to avoid the possibility that connection is closed before the session is deleted. According to SQLite docs deleting a session (or doing anything with it) if the connection it was created for was closed results in undefined behaviour (which possibly means segmentation fault).

marcin added a child revision: D10837: Implement attachments capture.Jan 26 2024, 7:16 AM

marcin added inline comments.Jan 26 2024, 7:19 AM

native/cpp/CommonCpp/DatabaseManagers/SQLiteConnectionManager.cpp
150 ↗	(On Diff #36164)	I plan to run this call in separate thread as a part of ENG-6226. However I will first do some performance tests to measure how much time does it take on average to capture log and see how important it actually is to introduce additional thread.

Fix Android compilation

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 26 2024, 8:07 AM

Harbormaster failed remote builds in B26189: Diff 36171!

Fix iOS CI

Harbormaster completed remote builds in B26201: Diff 36189.Jan 26 2024, 9:45 AM

marcin requested review of this revision.Jan 26 2024, 9:45 AM

Use connection manager on both native and web but with different implementations
Change aproach to log capture that will allow us to ensure no log is missed

Harbormaster completed remote builds in B26388: Diff 36483.Jan 31 2024, 2:57 PM

marcin edited the summary of this revision. (Show Details)Feb 1 2024, 3:03 AM

marcin edited the test plan for this revision. (Show Details)

marcin added inline comments.Feb 1 2024, 3:07 AM

native/cpp/CommonCpp/DatabaseManagers/NativeConnectionManager.cpp
104–105 ↗	(On Diff #36483)	Session object is not "zeroed" after capturing the log (see here name) - reading from the same session multiple times will give us all captured changes from the beginning of session instantiation. Therefore we have to delete and recreate session after capturing logs.

marcin mentioned this in D10911: Capture backup logs at the end of each data store transaction..Feb 1 2024, 4:36 AM

michal accepted this revision.Feb 2 2024, 9:50 AM

michal added inline comments.

native/cpp/CommonCpp/DatabaseManagers/NativeConnectionManager.cpp
89 ↗	(On Diff #36483)	You are calling `sqlite3session_patchset` but everything is named as `changeset` which is also a term applicable to the sqlite session api. Could you rename `changeset` -> `patchset`?
104–105 ↗	(On Diff #36483)	Can you add a code comment with this information? I feel like it's very non-obvious
native/cpp/CommonCpp/DatabaseManagers/NativeConnectionManager.h
6 ↗	(On Diff #36483)	Nit: maybe `NativeSQliteConnectionManager`?

This revision is now accepted and ready to land.Feb 2 2024, 9:50 AM

Actually one question: logs won't be captured if backupID isn't set. It's set during compaction creation and only primary device will run this code. So the code is correct, but it will currently still create a session on all devices, including secondary devices. Additionally, on the secondary device, the session will never be cleared (detached). Do you think this could have a performance impact/ it's worth only enabling session if we have created a compaction?

This revision now requires changes to proceed.Feb 2 2024, 10:12 AM

marcin added inline comments.Feb 5 2024, 3:13 AM

native/cpp/CommonCpp/DatabaseManagers/NativeConnectionManager.h
6 ↗	(On Diff #36483)	It was actually my initial idea but I was afraid that reviewers will find it too verbose.

kamil added inline comments.Feb 5 2024, 2:22 PM

native/android/app/CMakeLists.txt
220–221 ↗	(On Diff #36483)	shouldn't we add the same flags to `run_emscripten.sh`? I know it won't be used but just to make sure we use the same engine configs
native/cpp/CommonCpp/DatabaseManagers/NativeConnectionManager.cpp
54 ↗	(On Diff #36483)	Wondering if using `std::ofstream::write()` without `std::string` and `operator<<` isn't better when it comes to binary data, also maybe it's faster? I remember having some issues in the past with it doing something similar in C++, especially with null characters but since you test it it's probably okay.
98–100 ↗	(On Diff #36483)	is it possible to have `changesetPtr` not null but `changesetSize == 0`? In that case shouldn't we call `sqlite3_free(changesetPtr);`?