MInor Tweaks for Cache and Lazy Load

[shubham1g5]

Technical Summary

A few minor tweaks and pending feedback for cache and lazy load feature. Review commit by commit.

Labels and Review

• Do we need to enhance the manual QA test coverage ? If yes, the "QA Note" label is set correctly
• Does the PR introduce any major changes worth communicating ? If yes, the "Release Note" label is set and a "Release Note" is specified in PR description.
• Risk label is set correctly
• The set of people pinged as reviewers is appropriate for the level of risk of the change

cross-request: dimagi/commcare-core#1463

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request removes the scheduleEntityCacheInvalidation() method from the CommCareApplication class and redirects cache invalidation responsibilities to the PrimeEntityCacheHelper class. Calls previously made to the removed method in both SyncCapableCommCareActivity and DataPullTask are now replaced with calls to the new scheduleEntityCacheInvalidation() method in PrimeEntityCacheHelper. Additionally, the EntityCacheInvalidationWorker now includes a conditional check to process shallow records only if the entity cache is not empty and ensures that the Prime cache worker is scheduled if a session is active. A minor change in EntityLoaderHelper replaces the factory method cancelLoading() with markAsCancelled() to update the cancellation handling. Overall, the changes refactor how and when the entity cache is invalidated and scheduled within the application.

Sequence Diagram(s)

sequenceDiagram
    participant Activity as SyncActivity/DataPullTask
    participant PECH as PrimeEntityCacheHelper
    participant WM as WorkManager
    participant ECU as EntityCacheInvalidationWorker

    Activity->>PECH: scheduleEntityCacheInvalidation()
    PECH->>WM: Enqueue one-time work request (with ENTITY_CACHE_INVALIDATION_REQUEST)
    WM->>ECU: Execute doWork()
    alt Cache is not empty
        ECU->>ECU: processShallowRecords()
    else Cache is empty
        ECU->>ECU: Skip processing shallow records
    end
    alt Session is active
        ECU->>PECH: schedulePrimeEntityCacheWorker()
    end

Loading

Possibly related PRs

• Update pull_request_template.md #2946: Directly related as it involves removal of scheduleEntityCacheInvalidation() from CommCareApplication and its replacement, shifting cache invalidation responsibility.
• Cache and Lazy-Load Modifications #2928: Connected through modifications that replace calls to the removed method with a new implementation in PrimeEntityCacheHelper.
• Expires entity cache for related cases #2944: Pertinent due to the transition from the old cache invalidation approach to the new mechanism managed by PrimeEntityCacheHelper.

Suggested labels

cross requested, QA Note

Suggested reviewers

• OrangeAndGreen

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c97a152 and 448fa70.

📒 Files selected for processing (6)

• app/src/org/commcare/CommCareApplication.java (1 hunks)
• app/src/org/commcare/activities/SyncCapableCommCareActivity.java (2 hunks)
• app/src/org/commcare/tasks/DataPullTask.java (2 hunks)
• app/src/org/commcare/tasks/EntityCacheInvalidationWorker.kt (2 hunks)
• app/src/org/commcare/tasks/EntityLoaderHelper.kt (1 hunks)
• app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (2 hunks)

🔇 Additional comments (10)

app/src/org/commcare/tasks/EntityCacheInvalidationWorker.kt (2)

17-19: Good improvement: Added null check before processing shallow records

Adding this check to verify that the entity storage cache is not empty before processing shallow records is a good defensive programming practice. This prevents unnecessary processing when there's nothing to process.

---

23-28: Good addition: Ensuring Prime worker is always scheduled

Adding a finally block to ensure the Prime worker is scheduled regardless of the task's outcome is a good improvement. This makes the code more robust by guaranteeing the Prime worker runs when a session is active, even if the current task encounters an exception.

app/src/org/commcare/tasks/EntityLoaderHelper.kt (1)

114-114: Method name changed to better reflect its functionality

The change from cancelLoading() to markAsCancelled() likely better reflects what the method actually does - marking the factory as cancelled rather than necessarily stopping any ongoing loading process immediately.

app/src/org/commcare/activities/SyncCapableCommCareActivity.java (2)

23-23: Added import for PrimeEntityCacheHelper

Added import statement to support the class reference on line 265.

---

265-265: Improved responsibility delegation for cache invalidation

Changing from CommCareApplication.instance().scheduleEntityCacheInvalidation() to PrimeEntityCacheHelper.scheduleEntityCacheInvalidation() improves separation of concerns by moving cache-related functionality to a dedicated helper class.

app/src/org/commcare/CommCareApplication.java (1)

807-809: Improved cache management approach

This change replaces the previous inline cache invalidation with a more structured approach: first clearing the state of the cache helper, then scheduling the prime entity cache worker. This better separates responsibilities between different components.

app/src/org/commcare/tasks/DataPullTask.java (1)

454-454: LGTM - Clean refactoring of cache invalidation responsibility

The change appropriately moves the cache invalidation responsibility from CommCareApplication to the PrimeEntityCacheHelper class, which centralizes cache-related operations. This is a good architectural improvement.

app/src/org/commcare/tasks/PrimeEntityCacheHelper.kt (3)

59-59: LGTM - Good constant naming

Clear and descriptive constant name that follows the existing naming convention.

---

67-77: LGTM - Added beneficial condition check

Good improvement to check if entity caching is enabled before scheduling the worker. This avoids unnecessary work when caching is disabled.

---

84-96: LGTM - Well-implemented cache invalidation method

This new method properly handles entity cache invalidation:

1. Checks if entity caching is enabled
2. Creates a non-expedited work request (appropriate for this operation)
3. Uses ExistingWorkPolicy.REPLACE to ensure only the most recent invalidation request is considered

The implementation aligns with best practices for WorkManager usage.

✨ Finishing Touches

• 📝 Generate Docstrings

---

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

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[shubham1g5]

@coderabbitai full review

[shubham1g5]

@damagatchi retest this please

[shubham1g5]

@coderabbitai full review

[avazirna]

Looks good to me!

[ctsims]

Honestly, I don't like that these two methods refer to different static singletons with different lifecycles. I'm worried this code has the risk of further spreading out access to static / singleton lifecycles while also being fairly likely to be executed in contexts where those lifecycles are changing (right after login, or when a user has completed a form and is logging out for the day).

My guess here is that it's better to make "the other half" of any calls like this that depend on multiple singleton contexts get pushed into one of them, so that at least there's no ambiguity at runtime about which memory space or sandbox the second call is going to be running against.

[shubham1g5]

Not completely sure if I got you right but did you mean like this ?