Backmerge Commcare 2.61.3

[avazirna]

Product Description

Backmerge Commcare 2.61.3

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR introduces performance monitoring for file encryption operations and improves location freshness handling. It adds tracing instrumentation around file-encryption writes in multiple storage layers, updates location controller logic to check for stale locations before processing, extracts location freshness utilities into top-level functions in CommCareLocationController, modifies crash reporting to return actual user information instead of null, adds a preference key for controlling stale location behavior, and bumps the app version.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• [Hotfix] Add perf monitoring trace for file encryption time #3496: Adds the same performance tracing for file-encryption with CCPerfMonitoring constants and stopFileEncryptionTracing usage across EncryptionIO, HybridFileBackedSqlStorage, SaveToDiskTask, and FileUtil.
• [Hotfix] Discard stale locations based on an App Setting #3495: Modifies the same location-freshness logic with top-level functions like isLocationFresh/shouldDiscardLocation/logStaleLocationSaved and updates their call sites in location controllers.
• Fix user info missing in Crashlytics reports (Hotfix) #3494: Modifies getUserForCrashes in ReportingUtils to return the actual username when available instead of null.

Suggested labels

skip-integration-tests

Suggested reviewers

• shubham1g5
• Jignesh-dimagi
• conroy-ricketts

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description severely lacks required sections including Technical Summary, Feature Flag, Safety Assurance, and Review checklist; only a minimal Product Description is provided.	Add Technical Summary (with ticket/document links and rationale), Safety Assurance (safety story, test coverage, QA plan), Feature Flag section if applicable, and complete the Review checklist with appropriate labels.
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.70% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Backmerge Commcare 2.61.3' clearly describes the main change, which is a version bump from 2.61.2 to 2.61.3 and backmerge of related fixes.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch commcare_2.61

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@app/src/org/commcare/models/encryption/EncryptionIO.java`:
- Line 42: In EncryptionIO, replace the use of InputStream.available() to
determine file size (the line assigning fileSize from is.available()) with the
actual File.length() on the File used to open that InputStream (e.g., use
inputFile.length()), converting or validating the long-to-int cast if the code
expects an int and guarding against null/zero files; ensure you reference the
same File instance that produced the InputStream and update any related logic
that assumes available() semantics.
- Around line 35-46: The method EncryptionIO.encryptFile starts a trace (trace)
but does not guarantee CCPerfMonitoring.INSTANCE.stopFileEncryptionTracing(...)
is called if StreamsUtil.writeFromInputToOutputNew throws; wrap the file I/O and
write call in a try-finally where in finally you always call
stopFileEncryptionTracing(trace, fileSize, sourceFilePath) and also ensure both
streams (os from createFileOutputStream(...) and is = new FileInputStream(...))
are closed safely (null-check and close inside finally or use try-with-resources
equivalent) so the trace is stopped and resources are released on exceptions as
well.

🧹 Nitpick comments (2)

app/src/org/commcare/location/CommCareLocationController.kt (1)

34-40: Consider the side effect of unconditional logging.

shouldDiscardLocation logs a non-fatal exception every time a stale location is received, regardless of whether the location will be discarded. This is likely intentional for monitoring purposes, but worth confirming this is the desired behavior—especially since logStaleLocationSaved may log again downstream if the location is not discarded and subsequently saved.

Also, minor style nit: add a space after if on line 35.

Minor style fix

 fun shouldDiscardLocation(location: Location): Boolean {
-    if(!isLocationFresh(location)) {
+    if (!isLocationFresh(location)) {
         Logger.exception("Received a stale location", getStaleLocationException(location))
         return HiddenPreferences.shouldDiscardStaleLocations()
     }
     return false
 }

app/src/org/commcare/location/CommCareFusedLocationController.kt (1)

38-45: Capture lastLocation in a local variable to avoid multiple property accesses.

result.lastLocation is accessed twice (line 39 and line 41). While it's likely stable, accessing it once and storing in a local variable is safer and cleaner.

Proposed fix

         object : LocationCallback() {
             override fun onLocationResult(result: LocationResult) {
-                result.lastLocation ?: return
+                val location = result.lastLocation ?: return
                 Logger.log(LogTypes.TYPE_MAINTENANCE, "Received location update")
-                if (shouldDiscardLocation(result.lastLocation!!)) {
+                if (shouldDiscardLocation(location)) {
                     return
                 }
-                mCurrentLocation = result.lastLocation
+                mCurrentLocation = location
                 mListener?.onLocationResult(mCurrentLocation!!)
             }
         }

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Trace not stopped on exception path.

If an exception occurs during the write operation, the trace will never be stopped. Consider wrapping in a try-finally to ensure the trace is always stopped.

Proposed fix

     public static void encryptFile(String sourceFilePath, String destPath, SecretKeySpec symetricKey) throws IOException {
         Trace trace = CCPerfMonitoring.INSTANCE.startTracing(CCPerfMonitoring.TRACE_FILE_ENCRYPTION_TIME);
 
         OutputStream os;
         FileInputStream is;
-        os = createFileOutputStream(destPath, symetricKey);
-        is = new FileInputStream(sourceFilePath);
-        int fileSize = is.available();
-        StreamsUtil.writeFromInputToOutputNew(is, os);
-
-        CCPerfMonitoring.INSTANCE.stopFileEncryptionTracing(trace, fileSize, sourceFilePath);
+        File sourceFile = new File(sourceFilePath);
+        long fileSize = sourceFile.length();
+        try {
+            os = createFileOutputStream(destPath, symetricKey);
+            is = new FileInputStream(sourceFilePath);
+            StreamsUtil.writeFromInputToOutputNew(is, os);
+        } finally {
+            CCPerfMonitoring.INSTANCE.stopFileEncryptionTracing(trace, fileSize, sourceFilePath);
+        }
     }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	public static void encryptFile(String sourceFilePath, String destPath, SecretKeySpec symetricKey) throws IOException {
	Trace trace = CCPerfMonitoring.INSTANCE.startTracing(CCPerfMonitoring.TRACE_FILE_ENCRYPTION_TIME);
	OutputStream os;
	FileInputStream is;
	os = createFileOutputStream(destPath, symetricKey);
	is = new FileInputStream(sourceFilePath);
	int fileSize = is.available();
	StreamsUtil.writeFromInputToOutputNew(is, os);
	CCPerfMonitoring.INSTANCE.stopFileEncryptionTracing(trace, fileSize, sourceFilePath);
	}
	public static void encryptFile(String sourceFilePath, String destPath, SecretKeySpec symetricKey) throws IOException {
	Trace trace = CCPerfMonitoring.INSTANCE.startTracing(CCPerfMonitoring.TRACE_FILE_ENCRYPTION_TIME);
	OutputStream os;
	FileInputStream is;
	File sourceFile = new File(sourceFilePath);
	long fileSize = sourceFile.length();
	try {
	os = createFileOutputStream(destPath, symetricKey);
	is = new FileInputStream(sourceFilePath);
	StreamsUtil.writeFromInputToOutputNew(is, os);
	} finally {
	CCPerfMonitoring.INSTANCE.stopFileEncryptionTracing(trace, fileSize, sourceFilePath);
	}
	}

🤖 Prompt for AI Agents

In `@app/src/org/commcare/models/encryption/EncryptionIO.java` around lines 35 -
46, The method EncryptionIO.encryptFile starts a trace (trace) but does not
guarantee CCPerfMonitoring.INSTANCE.stopFileEncryptionTracing(...) is called if
StreamsUtil.writeFromInputToOutputNew throws; wrap the file I/O and write call
in a try-finally where in finally you always call
stopFileEncryptionTracing(trace, fileSize, sourceFilePath) and also ensure both
streams (os from createFileOutputStream(...) and is = new FileInputStream(...))
are closed safely (null-check and close inside finally or use try-with-resources
equivalent) so the trace is stopped and resources are released on exceptions as
well.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use File.length() instead of InputStream.available() for accurate file size.

is.available() returns an estimate of bytes that can be read without blocking, not the actual file size. For accurate file size metrics, use the file's length directly.

Proposed fix

     public static void encryptFile(String sourceFilePath, String destPath, SecretKeySpec symetricKey) throws IOException {
         Trace trace = CCPerfMonitoring.INSTANCE.startTracing(CCPerfMonitoring.TRACE_FILE_ENCRYPTION_TIME);
 
         OutputStream os;
         FileInputStream is;
+        File sourceFile = new File(sourceFilePath);
+        long fileSize = sourceFile.length();
         os = createFileOutputStream(destPath, symetricKey);
         is = new FileInputStream(sourceFilePath);
-        int fileSize = is.available();
         StreamsUtil.writeFromInputToOutputNew(is, os);
 
         CCPerfMonitoring.INSTANCE.stopFileEncryptionTracing(trace, fileSize, sourceFilePath);
     }

🤖 Prompt for AI Agents

In `@app/src/org/commcare/models/encryption/EncryptionIO.java` at line 42, In
EncryptionIO, replace the use of InputStream.available() to determine file size
(the line assigning fileSize from is.available()) with the actual File.length()
on the File used to open that InputStream (e.g., use inputFile.length()),
converting or validating the long-to-int cast if the code expects an int and
guarding against null/zero files; ensure you reference the same File instance
that produced the InputStream and update any related logic that assumes
available() semantics.

[avazirna]

@damagatchi retest this please

[avazirna]

@damagatchi retest this please

[avazirna]

@damagatchi retest this please