ADFA-4330: Fix TermuxService shellManager NPE on OS service auto-restart#1413
ADFA-4330: Fix TermuxService shellManager NPE on OS service auto-restart#1413fryanpan wants to merge 3 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 27 minutes and 29 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesTermuxShellManager NPE fix and regression test
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
termux/termux-app/src/test/java/com/termux/app/TermuxServiceShellManagerNpeTest.java (1)
42-47: ⚡ Quick winAdd an
@Aftermethod to ensure proper cleanup even if the test fails.The
@Beforehook correctly resets the singleton, but if the test fails before line 67,controller.destroy()won't execute, potentially leaving resources uncleaned. Add an@Aftermethod to guarantee cleanup runs after every test.♻️ Proposed refactor to add `@After` cleanup
+ private ServiceController<TermuxService> controller; + /** * Reset the static singleton to null to mimic a fresh process where * TermuxApplication.onCreate() (which would normally call init()) never ran. */ `@Before` public void clearShellManagerSingleton() throws Exception { Field f = TermuxShellManager.class.getDeclaredField("shellManager"); f.setAccessible(true); f.set(null, null); } + `@After` + public void cleanup() { + if (controller != null) { + controller.destroy(); + controller = null; + } + } + /** Service onCreate() with a null shell-manager singleton must not NPE and must expose usable sessions. */ `@Test` public void onCreateWithNullSingleton_doesNotNpe_andSessionsAreUsable() { // Sanity: the auto-restart precondition — singleton is null going in. assertEquals(null, TermuxShellManager.getShellManager()); // Drive the REAL service lifecycle. On stage this throws NullPointerException inside // onCreate() -> runStartForeground() -> buildNotification() -> getTermuxSessionsSize(). - ServiceController<TermuxService> controller = + controller = Robolectric.buildService(TermuxService.class).create(); TermuxService service = controller.get(); // After a clean onCreate(), the shell manager must be wired up and queryable. assertNotNull("mShellManager must be initialized after onCreate()", TermuxShellManager.getShellManager()); assertEquals("A freshly created service manages zero sessions", 0, service.getTermuxSessionsSize()); - - controller.destroy(); }Also applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@termux/termux-app/src/test/java/com/termux/app/TermuxServiceShellManagerNpeTest.java` around lines 42 - 47, Add an `@After` method to guarantee resource cleanup after every test execution, regardless of whether the test passes or fails. This method should contain the same cleanup logic that is currently at line 67 (controller.destroy()), ensuring that resources are properly released even if the test throws an exception before reaching that line. This complements the existing `@Before` method clearShellManagerSingleton() to provide comprehensive test lifecycle management.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@termux/termux-app/src/main/java/com/termux/app/TermuxService.java`:
- Around line 125-128: The TermuxShellManager.init() method lacks
synchronization, creating a race condition where multiple threads could bypass
the null check and create duplicate singleton instances. Add the synchronized
keyword to the init() method declaration to ensure thread-safe access to the
singleton creation logic. This will prevent multiple threads from concurrently
executing the method and creating competing instances of the TermuxShellManager.
---
Nitpick comments:
In
`@termux/termux-app/src/test/java/com/termux/app/TermuxServiceShellManagerNpeTest.java`:
- Around line 42-47: Add an `@After` method to guarantee resource cleanup after
every test execution, regardless of whether the test passes or fails. This
method should contain the same cleanup logic that is currently at line 67
(controller.destroy()), ensuring that resources are properly released even if
the test throws an exception before reaching that line. This complements the
existing `@Before` method clearShellManagerSingleton() to provide comprehensive
test lifecycle management.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6111333-1495-4bac-b5e3-5dbf392c89f1
📒 Files selected for processing (2)
termux/termux-app/src/main/java/com/termux/app/TermuxService.javatermux/termux-app/src/test/java/com/termux/app/TermuxServiceShellManagerNpeTest.java
| // Use init() instead of getShellManager() so the singleton is lazily created if the | ||
| // OS auto-restarted the service after process death (TermuxApplication.onCreate did not | ||
| // run, leaving the static singleton null and causing an NPE in buildNotification()). | ||
| mShellManager = TermuxShellManager.init(getApplicationContext()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that TermuxShellManager.init() is synchronized
# Search for the init method and surrounding lines to check for synchronized keyword
rg -A 5 "public static .*init\(" termux/termux-shared/src/main/java/com/termux/shared/termux/shell/TermuxShellManager.javaRepository: appdevforall/CodeOnTheGo
Length of output: 268
Add synchronization to TermuxShellManager.init() to prevent race conditions.
The init() method in TermuxShellManager is unsynchronized and exhibits a classic double-checked locking anti-pattern. If onCreate() is called concurrently (or from different lifecycle paths), multiple threads could pass the if (shellManager == null) check and create duplicate instances. Add synchronized to the method declaration or use volatile with a proper synchronized block:
public static synchronized TermuxShellManager init(`@NonNull` Context context) {
if (shellManager == null)
shellManager = new TermuxShellManager(context);
return shellManager;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@termux/termux-app/src/main/java/com/termux/app/TermuxService.java` around
lines 125 - 128, The TermuxShellManager.init() method lacks synchronization,
creating a race condition where multiple threads could bypass the null check and
create duplicate singleton instances. Add the synchronized keyword to the init()
method declaration to ensure thread-safe access to the singleton creation logic.
This will prevent multiple threads from concurrently executing the method and
creating competing instances of the TermuxShellManager.
Sentry APPDEVFORALL-BR. Use TermuxShellManager.init() instead of getShellManager() so the singleton exists after OS service restart. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n auto-restart Reproduces the OS service auto-restart scenario where TermuxApplication.onCreate() never ran, leaving the static TermuxShellManager singleton null. Drives the real TermuxService.onCreate() via Robolectric; on the pre-fix baseline this NPEs in onCreate -> runStartForeground -> buildNotification -> getTermuxSessionsSize when mShellManager is null. Passes on the fix (init() lazily creates the singleton). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fryanpan
force-pushed
the
ADFA-4330-termuxservice-shellmanager-npe
branch
from
665eb9f to
a495d61
Compare
2 participants
Jira Ticket: https://appdevforall.atlassian.net/browse/ADFA-4330
Sentry Issue: https://appdevforall-inc-9p.sentry.io/issues/APPDEVFORALL-BR
Reproduction Details
When the OS auto-restarts
TermuxServiceafter the app process was killed,TermuxApplication.onCreate()(which creates theTermuxShellManagersingleton) never runs, soTermuxService.onCreate()reads a nullshellManagerand NPEs while building its foreground notification.Stack Trace
User Steps
User steps leading up to crash, based on Sentry breadcrumbs:
Was able to reproduce in a unit test?
Yes.
TermuxServiceShellManagerNpeTest(:termux:termux-app, Robolectric) forces the staticTermuxShellManagersingleton to null (simulating the no-Application-init restart), drives the realTermuxService.onCreate()viaServiceController, and assertsgetTermuxSessionsSize()returns 0 without NPE. Baseline: FAILS with the exact NPE path; branch: passes.What Was Fixed
onCreate()usesTermuxShellManager.init(getApplicationContext())(lazily creates the singleton) instead ofgetShellManager()(returns null when Application init never ran).Testing
:termux:termux-app:testV8DebugUnitTest→ green (red on baseline). Local CodeRabbit review: no findings. Reviewer note (local):TermuxShellManager.init()is unsynchronized while sibling mutators aresynchronized— the new service path slightly widens a pre-existing init race; consider synchronizinginit(). Add an@Afterresetting the static singleton for test hygiene.Fixes APPDEVFORALL-BR