refactor(cache): improve singleton pattern defensive programming (Issue #4)

Refactored CacheService.getInstance() to make handler registration more
atomic and easier to reason about. This prevents theoretical race conditions
and improves code maintainability.

Changes:
- Calculate needsNewInstance and needsHandlerRegistration upfront
- Set handlersRegistered=true BEFORE instance creation (defensive programming)
- Clear separation of concerns: flag setting → instance creation → handler registration
- Added comprehensive comments explaining the defensive pattern
- Added non-null assertion with explanatory comment for return value
- Added 4 comprehensive tests for singleton pattern:
  - Same instance on multiple calls
  - Handlers registered only once
  - New instance after close
  - Rapid getInstance calls without race condition

Benefits:
- More maintainable code with clearer intent
- Prevents future bugs from async refactoring
- Easier to understand the singleton pattern flow
- Better defensive programming practices
- Test coverage ensures pattern remains correct

Technical Note:
Node.js is single-threaded for synchronous code, so true race conditions
don't occur with the current synchronous constructor. However, this defensive
pattern makes the code more robust against future changes (e.g., async
initialization) and follows best practices for singleton patterns.

Test Coverage:
- All 1446 tests passing (100% pass rate)
- Added 4 new singleton pattern tests
- Test count increased: 1442 → 1446 tests

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

Author: sjsyrek

CHANGELOG.md

@@ -27,6 +27,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - Added 6 comprehensive tests covering single/multi-language validation
   - Location: `src/cli/commands/translate.ts:81-92` (validation method), multiple call sites
 
+### Changed
+- **Defensive Programming: CacheService Singleton Pattern** - Improved code robustness (Issue #4)
+  - Refactored getInstance() to set handlersRegistered flag before instance creation
+  - Makes handler registration more atomic and easier to reason about
+  - Calculates needsNewInstance and needsHandlerRegistration upfront
+  - Sets handlersRegistered=true immediately after checking, before any side effects
+  - Prevents theoretical race conditions in async scenarios (defensive programming)
+  - Added comprehensive comments explaining the defensive pattern
+  - Added 4 new tests verifying singleton behavior and handler registration
+  - **Impact**: More maintainable code with clearer intent, prevents future bugs
+  - **Note**: Node.js is single-threaded, so actual race conditions don't occur, but this makes code more robust
+  - Location: `src/storage/cache.ts:58-102` (getInstance method)
+
 ### Fixed
 - **Critical: Duplicate text handling in batch translation** - Fixed data loss bug for duplicate inputs
   - When input array contained duplicate texts (e.g., `["Hello", "Hello", "World"]`), only the last occurrence received translation

src/storage/cache.ts

@@ -58,34 +58,47 @@ export class CacheService {
   /**
    * Get singleton cache instance
    * Uses a shared instance to prevent resource leaks
+   * Fix for Issue #4: Improved defensive programming with atomic handler registration
    */
   static getInstance(options?: CacheServiceOptions): CacheService {
-    if (!CacheService.instance || CacheService.instance.isClosed) {
+    // Check if we need to create a new instance
+    const needsNewInstance = !CacheService.instance || CacheService.instance.isClosed;
+
+    // Register handlers atomically before instance creation for defensive programming
+    // Set flag first to prevent any theoretical race conditions (even though Node.js is single-threaded)
+    const needsHandlerRegistration = needsNewInstance && !CacheService.handlersRegistered;
+    if (needsHandlerRegistration) {
+      // Set flag immediately to make the operation atomic
+      CacheService.handlersRegistered = true;
+    }
+
+    // Create instance if needed
+    if (needsNewInstance) {
       CacheService.instance = new CacheService(options);
+    }
 
-      // Register cleanup handlers only once to prevent memory leaks
-      if (!CacheService.handlersRegistered) {
-        CacheService.handlersRegistered = true;
-
-        // Register cleanup on process exit (use 'once' to prevent duplicate handlers)
-        process.once('exit', () => {
-          CacheService.instance?.close();
-        });
-
-        // Handle unexpected termination
-        process.once('SIGINT', () => {
-          CacheService.instance?.close();
-          process.exit(0);
-        });
-
-        process.once('SIGTERM', () => {
-          CacheService.instance?.close();
-          process.exit(0);
-        });
-      }
+    // Register cleanup handlers only once to prevent memory leaks
+    // This happens after instance creation to ensure instance exists
+    if (needsHandlerRegistration) {
+      // Register cleanup on process exit (use 'once' to prevent duplicate handlers)
+      process.once('exit', () => {
+        CacheService.instance?.close();
+      });
+
+      // Handle unexpected termination
+      process.once('SIGINT', () => {
+        CacheService.instance?.close();
+        process.exit(0);
+      });
+
+      process.once('SIGTERM', () => {
+        CacheService.instance?.close();
+        process.exit(0);
+      });
     }
 
-    return CacheService.instance;
+    // Instance is guaranteed to exist at this point (either already existed or just created)
+    return CacheService.instance!;
   }
 
   /**

tests/unit/cache-service.test.ts

@@ -473,4 +473,78 @@ describe('CacheService', () => {
       newService.close();
     });
   });
+
+  describe('singleton pattern (Issue #4)', () => {
+    it('should return same instance on multiple getInstance calls', () => {
+      const instance1 = CacheService.getInstance({ dbPath: testCachePath });
+      const instance2 = CacheService.getInstance({ dbPath: testCachePath });
+
+      expect(instance1).toBe(instance2);
+
+      instance1.close();
+    });
+
+    it('should register exit handlers only once', () => {
+      // Spy on process.once to verify handlers are only registered once
+      const processOnceSpy = jest.spyOn(process, 'once');
+
+      // Reset the singleton (access private static field via any cast)
+      (CacheService as any).instance = null;
+      (CacheService as any).handlersRegistered = false;
+
+      // Call getInstance multiple times
+      const instance1 = CacheService.getInstance({ dbPath: testCachePath });
+      const instance2 = CacheService.getInstance({ dbPath: testCachePath });
+      const instance3 = CacheService.getInstance({ dbPath: testCachePath });
+
+      // Should only have registered handlers once (3 calls: exit, SIGINT, SIGTERM)
+      expect(processOnceSpy).toHaveBeenCalledTimes(3);
+
+      // All instances should be the same
+      expect(instance1).toBe(instance2);
+      expect(instance2).toBe(instance3);
+
+      processOnceSpy.mockRestore();
+      instance1.close();
+    });
+
+    it('should create new instance after close', () => {
+      const instance1 = CacheService.getInstance({ dbPath: testCachePath });
+      instance1.close();
+
+      const instance2 = CacheService.getInstance({ dbPath: testCachePath });
+
+      // Should be a different instance since first was closed
+      expect(instance1).not.toBe(instance2);
+
+      instance2.close();
+    });
+
+    it('should handle rapid getInstance calls without race condition', () => {
+      // Reset singleton
+      (CacheService as any).instance = null;
+      (CacheService as any).handlersRegistered = false;
+
+      // Spy on handler registration
+      const processOnceSpy = jest.spyOn(process, 'once');
+
+      // Simulate rapid calls (even though Node.js is single-threaded,
+      // this tests the logic is correct)
+      const instances = [];
+      for (let i = 0; i < 10; i++) {
+        instances.push(CacheService.getInstance({ dbPath: testCachePath }));
+      }
+
+      // All should be the same instance
+      for (let i = 1; i < instances.length; i++) {
+        expect(instances[i]).toBe(instances[0]);
+      }
+
+      // Handlers should only be registered once (3 calls: exit, SIGINT, SIGTERM)
+      expect(processOnceSpy).toHaveBeenCalledTimes(3);
+
+      processOnceSpy.mockRestore();
+      instances[0]?.close();
+    });
+  });
 });