refactor progress

packages/cache/BEST_PRACTICES.md

@@ -0,0 +1,332 @@
+# Cache package – best practices
+
+Working document. Append new lessons as they arise.
+
+---
+
+## 1. Extract shared logic into focused, single-responsibility files
+
+When two or more providers (KV, Redis, InMemory) share identical logic, extract it
+into a dedicated file rather than duplicating it. Each extracted file should own
+exactly one concern:
+
+| File | Concern |
+|---|---|
+| `CacheProviderTypes.tsx` | Shared interfaces consumed by providers and callers (`CacheLogger`, `CacheTelemetry`, `CacheKeyClassifier`) |
+| `CacheSerialization.tsx` | JSON parse/stringify helpers with error handling and truncation |
+| `CacheLockValidation.tsx` | Lock key/token format validation, token generation, key formatting |
+| `CacheKeyClassification.tsx` | Mapping a cache key to a telemetry dimension string |
+| `RedisClientTypes.tsx` | `RedisClient` and `RedisPipeline` interface contracts |
+
+**Why:** duplicated code drifts apart over time, bugs get fixed in one copy but not
+another, and readers have to mentally diff near-identical methods.
+
+---
+
+## 2. Separate public contract types from implementation files
+
+Types that callers or tests need to import (`CacheLogger`, `CacheTelemetry`,
+`RedisClient`) belong in their own files – not buried inside a provider class.
+
+**Rule of thumb:** if a type appears in an `import type` statement in a file that is
+not the defining file, it deserves its own home.
+
+**Before:**
+```tsx
+// KVCacheProvider.tsx defines AND exports CacheLogger
+export interface CacheLogger { ... }
+export class KVCacheProvider { ... }
+```
+
+**After:**
+```tsx
+// CacheProviderTypes.tsx – sole owner of the type
+export interface CacheLogger { ... }
+
+// KVCacheProvider.tsx – imports the type
+import type {CacheLogger} from '@fluxer/cache/src/CacheProviderTypes';
+```
+
+---
+
+## 3. Use an `instrumented` helper to collapse telemetry boilerplate
+
+When every method in a class follows the same start/try/record-success/catch/record-error
+pattern, extract it into a single private method:
+
+```tsx
+private async instrumented<T>(
+  operation: string,
+  key: string,
+  fn: () => Promise<T>,
+  statusResolver?: (result: T) => string,
+): Promise<T> { ... }
+```
+
+Each call site then becomes one line of intent instead of 25 lines of ceremony:
+
+```tsx
+async get<T>(key: string): Promise<T | null> {
+  return this.instrumented('get', key, async () => {
+    const value = await this.client.get(key);
+    if (value == null) return null;
+    return safeJsonParse<T>(value, this.logger);
+  }, (result) => (result == null ? 'miss' : 'hit'));
+}
+```
+
+**Why:** the telemetry pattern was copy-pasted across `get`, `set`, `delete` with
+only the operation name and status differing. A helper makes the unique logic visible
+and the boilerplate invisible.
+
+---
+
+## 4. Keep serialisation consistent
+
+Use `serializeValue()` and `safeJsonParse()` from `CacheSerialization.tsx` inside the
+cache package rather than bare `JSON.stringify` / `JSON.parse`. Benefits:
+
+- Parse errors are caught and logged with truncated values, not swallowed or thrown
+  raw.
+- A single place to add future concerns (metrics on parse failures, encoding changes,
+  etc.).
+- Callers outside the cache package doing their own `JSON.stringify` for HTTP bodies,
+  worker queues, etc. do **not** need these helpers – they are cache-specific.
+
+---
+
+## 5. Centralise validation and make it reusable
+
+Lock key and token validation was copy-pasted across all three providers with identical
+regexes and error messages. `CacheLockValidation.tsx` now owns:
+
+- `validateLockKey(key)` – throws on bad format
+- `validateLockToken(token)` – throws on bad format
+- `generateLockToken()` – `randomBytes(16).toString('hex')`
+- `formatLockKey(key)` – returns `lock:${key}`
+
+**Guideline:** if the same regex or format string appears in more than one file, it
+should be a named export in a shared module.
+
+---
+
+## 6. Import from the defining file ("horse's mouth")
+
+Per project conventions: all callsites import directly from the file that defines the
+symbol. Never re-export. Never create barrel files.
+
+```tsx
+// Good
+import type {CacheLogger} from '@fluxer/cache/src/CacheProviderTypes';
+
+// Bad – importing a type from a file that re-exports it
+import type {CacheLogger} from '@fluxer/cache/src/providers/KVCacheProvider';
+```
+
+When moving a type to a new home, grep for every import and update it in the same
+change. Do not leave the old export as a compatibility shim.
+
+---
+
+## 7. Provider-specific config stays in the provider file
+
+`KVCacheProviderConfig` stays in `KVCacheProvider.tsx` because it references
+`IKVProvider`, which is specific to that implementation. Only types shared across
+multiple providers get extracted.
+
+**Heuristic:** if a type is only imported by a single file and its tests, it belongs
+in that file.
+
+---
+
+## 8. Verify refactors with the full test suite, formatter, and type checker
+
+After any structural refactor, run in order:
+
+```bash
+cd packages/cache && pnpm test          # all existing tests pass
+pnpm biome check --write packages/cache/src/  # formatting (run from repo root)
+cd packages/cache && pnpm typecheck     # type checking
+```
+
+Then spot-check downstream consumers:
+
+```bash
+cd packages/api && pnpm typecheck
+```
+
+Pre-existing failures in downstream packages are acceptable so long as **none of the
+errors reference the refactored package**.
+
+---
+
+## 9. Do not over-extract
+
+Not every use of `randomBytes` or `JSON.stringify` in the codebase needs to use the
+cache package's helpers. Cache serialisation helpers are for **cache values**. Other
+domains (SSO tokens, CSP nonces, HTTP bodies, worker queues) have their own
+serialisation needs and should not couple to the cache package.
+
+**Rule:** extract shared code within a bounded context (the cache package), not across
+unrelated domains.
+
+---
+
+## 10. Naming conventions for extracted files
+
+Follow the project's descriptive-filename convention. Avoid generic names:
+
+| Good | Bad |
+|---|---|
+| `CacheLockValidation.tsx` | `Validation.tsx` |
+| `CacheKeyClassification.tsx` | `Utils.tsx` |
+| `CacheProviderTypes.tsx` | `Types.tsx` |
+| `RedisClientTypes.tsx` | `RedisTypes.tsx` |
+
+Domain-prefix every file so it is unambiguous in search results and import
+auto-complete.
+
+---
+
+## 11. Avoid backwards-compatibility shims
+
+When moving exports to new files, do not leave behind re-exports or aliases in the old
+location. This creates indirection and lets stale imports survive indefinitely.
+Instead:
+
+1. Move the definition.
+2. Update every import site in the same commit.
+3. Delete the old export entirely.
+
+---
+
+## 12. Keep the abstract contract stable
+
+`ICacheService` and `CacheMSetEntry` stay in `ICacheService.tsx`. They define the
+public contract that all providers implement and all consumers depend on. Refactoring
+provider internals should never change this file.
+
+---
+
+## 13. One import block, no blank lines inside
+
+Per project conventions, keep a single contiguous import block at the top of each file
+with no blank lines or code between imports. Let biome handle ordering.
+
+```tsx
+// Good
+import {classifyKeyType} from '@fluxer/cache/src/CacheKeyClassification';
+import {formatLockKey, generateLockToken} from '@fluxer/cache/src/CacheLockValidation';
+import type {CacheLogger} from '@fluxer/cache/src/CacheProviderTypes';
+import {safeJsonParse, serializeValue} from '@fluxer/cache/src/CacheSerialization';
+import {ICacheService} from '@fluxer/cache/src/ICacheService';
+```
+
+---
+
+## 14. Scope of `safeJsonParse` logger parameter
+
+`safeJsonParse` accepts an optional `CacheLogger`. The KV provider passes its logger
+so parse errors are reported. The Redis provider passes nothing (logs to console as a
+fallback in the provider itself – now removed in favour of silent null return). The
+InMemory provider does not call `safeJsonParse` at all because it stores native values,
+not serialised strings.
+
+When adding a new provider, decide up front whether parse errors should be logged and
+pass the logger accordingly.
+
+---
+
+## 15. Watch for non-atomic operations
+
+The Redis provider's `acquireLock` was (and remains, pending a proper fix) non-atomic:
+
+```tsx
+await this.client.set(lockKey, token);
+await this.client.expire(lockKey, ttlSeconds);
+```
+
+Between these two calls, the lock exists without a TTL. If the process crashes, the
+lock is held forever. The KV provider uses `SET ... EX ... NX` in a single command,
+which is correct. When implementing distributed primitives, always prefer atomic
+operations.
+
+---
+
+## 16. Downstream callsites should use shared utilities
+
+After extracting shared logic, audit the rest of the codebase for consumers that
+duplicate the same logic inline. Common patterns:
+
+- `lock:${key}` string templates → `formatLockKey(key)`
+- `crypto.randomBytes(16).toString('hex')` → `generateLockToken()`
+- `Math.random().toString(36)...` → `generateLockToken()` (also a security fix)
+- Mock implementations with inline lock validation → import from `CacheLockValidation`
+
+Update downstream callsites in the same change to prevent the shared code from
+becoming an orphan.
+
+---
+
+## 17. Never use Math.random() for tokens
+
+`Math.random()` is not cryptographically secure. For any token used in locking,
+authentication, or session management, use `crypto.randomBytes()` or the shared
+`generateLockToken()` utility.
+
+```tsx
+// bad
+const token = Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15);
+
+// good
+import {generateLockToken} from '@fluxer/cache/src/CacheLockValidation';
+const token = generateLockToken();
+```
+
+---
+
+## 18. Remove no-op string operations
+
+Watch for `.replace()` calls that silently match nothing:
+
+```tsx
+// bad — 'deletion_queue:rebuild_lock' doesn't contain 'lock:' as a prefix
+// .replace('lock:', '') matches nothing and returns the original string unchanged
+REBUILD_LOCK_KEY.replace('lock:', '')
+
+// good — pass the key directly
+REBUILD_LOCK_KEY
+```
+
+No-op operations are a sign the author assumed a different key format. Verify the
+format and remove the dead code.
+
+---
+
+## 19. Mock implementations should use shared utilities
+
+Test mocks (like `MockCacheService`) that implement lock acquisition should use the
+same shared utilities as real providers:
+
+```tsx
+// bad — inline duplication
+const lockKey = `lock:${key}`;
+const token = crypto.randomBytes(16).toString('hex');
+
+// good — shared utilities keep mocks consistent
+import {formatLockKey, generateLockToken} from '@fluxer/cache/src/CacheLockValidation';
+const lockKey = formatLockKey(key);
+const token = generateLockToken();
+```
+
+This ensures mocks produce tokens in the same format as real providers and that
+format changes propagate automatically.
+
+---
+
+## Changelog
+
+| Date | Change |
+|---|---|
+| 2026-02-06 | Initial version from cache package refactoring |
+| 2026-02-06 | Added downstream cleanup lessons (sections 16–19) |