#485 - Fix NPE in syncWithRemoteQueue when message removed during sync

[franco-zalamena-iterable]

Summary

• Adds a null check for localMessage after re-fetching from storage in IterableInAppManager.syncWithRemoteQueue
• Guards against a race condition where the message is removed between the existence check (storage.getMessage() != null) and the subsequent retrieval, which caused a NullPointerException when calling isRead() on a null reference

Test plan

• Verify that syncWithRemoteQueue no longer throws NPE when a message is removed from storage between the existence check and re-fetch
• Verify that the normal sync flow (message exists and is updated) still works correctly
• Verify that read-state syncing from remote to local still functions as expected
• Run existing unit tests for IterableInAppManager to confirm no regressions

🤖 Generated with Claude Code

[franco-zalamena-iterable]

PR Analysis

Problem: NullPointerException at IterableInAppManager.syncWithRemoteQueue line 330 because localMessage.isRead() is called on a null reference. The original code calls storage.getMessage() twice — once for the existence check and once to retrieve the object — creating a TOCTOU (time-of-check-time-of-use) race where the message can be removed between the two calls (e.g., via removeMessage(), which is synchronized while syncWithRemoteQueue is not).

Ideal fix plan:

• Collapse the two storage.getMessage() calls into a single call and store the result, eliminating the TOCTOU race
• Use if/else instead of two separate if blocks with inverted conditions (dead-obvious code simplification)
• Add a null guard for remoteQueue entries as defensive programming (the caller already filters nulls, so this is belt-and-suspenders)
• Add a null guard for storage.getMessages() entries in the second loop for consistency
• Consider making syncWithRemoteQueue synchronized to fully close the race with removeMessage — without it, a concurrent removeMessage call could still modify storage mid-iteration
• Add a unit test that exercises the race condition (e.g., a storage mock that returns null on the second call)

What the PR did:

• Collapsed the double storage.getMessage() call into a single call with an if/else — directly fixing the TOCTOU bug
• Added a null guard for entries in the remoteQueue iteration
• Added a null guard for entries in the storage.getMessages() iteration
• Extracted storage.getMessages() into a local variable for the second loop (minor readability improvement)

Assessment:

• Root cause identified: yes — the double fetch from storage with a race window was correctly identified
• Fix correctness: partially correct — the TOCTOU gap is eliminated for the isRead() call, but syncWithRemoteQueue is still not synchronized, so a concurrent removeMessage can still mutate storage mid-iteration (e.g., ConcurrentModificationException on the storage.getMessages() loop, or a message disappearing between the containsKey check and removeMessage call). The proper fix should also add synchronized to this method to match removeMessage.
• Missed: (1) syncWithRemoteQueue should be synchronized to fully prevent concurrent modification from removeMessage or other synchronized methods. (2) No unit test was added to cover the null/race scenario.
• Wrong assessment: none — the PR description accurately describes the problem.
• Tests: needed but missing — a test with a mock storage that returns null on a second getMessage() call would validate the fix and prevent regression.