Skip to content

fix(redis-lock): URL parsing fix + lock domain single-flight (PR-1, Issue #704)#739

Open
jlin53882 wants to merge 6 commits intoCortexReach:masterfrom
jlin53882:fix/pr704-redis-url-parsing-v2
Open

fix(redis-lock): URL parsing fix + lock domain single-flight (PR-1, Issue #704)#739
jlin53882 wants to merge 6 commits intoCortexReach:masterfrom
jlin53882:fix/pr704-redis-url-parsing-v2

Conversation

@jlin53882
Copy link
Copy Markdown
Contributor

@jlin53882 jlin53882 commented May 3, 2026

PR-1: URL parsing fix + lock domain single-flight

What Changed

  • New: src/redis-lock.ts — parseRedisUrl() + determineLockDomain() single-flight

    • parseRedisUrl(): handles legacy URLs without scheme (e.g. "localhost:6379" → "redis://localhost:6379")
    • Has scheme URLs (redis://, rediss://, redis://?tls=true, etc.): passed directly to ioredis unchanged
    • Old implementation used .replace('redis://', '') which broke rediss:// URLs (removed "re" leaving "s://...") and TLS query strings
    • determineLockDomain(): single-flight decision — all concurrent callers share the same Promise, so the whole process decides redis vs file lock only once
    • If redis-lock.js import fails, defaults to 'file'
  • New: test/redis-url-parsing.test.mjs — 7 test cases (all passing)

    • Legacy without scheme: "localhost:6379" → "redis://localhost:6379"
    • Standard redis:// + auth: preserved unchanged
    • TLS rediss://: preserved unchanged (was broken before)
    • TLS + Auth: preserved unchanged
    • Query string redis://host:6379?tls=true: preserved unchanged (was broken before)
    • IPv4 IP:port: scheme added
    • IPv6 [::1]:port: scheme added
  • Modified: src/store.ts — removed dead code getLockDomain() (was not called by anyone)

  • Modified: scripts/verify-ci-test-manifest.mjs — registered redis-url-parsing.test.mjs

  • Modified: package.json — added ioredis ^5.10.1 devDependency (module-level import requires it)

PR Split Context

This PR is Phase 1 of 3 for Issue #704 (Redis distributed lock).

Phase Content Status
PR-1 (this PR) URL parsing + single-flight done
PR-2 RedisLockManager full implementation pending
PR-3 Integration tests + CI pending

Verification

node test/redis-url-parsing.test.mjs
# 7/7 passing

@jlin53882 jlin53882 force-pushed the fix/pr704-redis-url-parsing-v2 branch from e993f78 to 8ac8206 Compare May 3, 2026 12:00
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e993f781cd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/redis-lock.ts Outdated
Comment on lines +63 to +67
const manager = await createRedisLockManager();
if (manager && await manager.isHealthy()) {
_lockDomainDecision = 'redis';
return 'redis';
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Release probe Redis connection before returning

When determineLockDomain() detects a healthy Redis instance, it returns 'redis' but drops the manager without calling disconnect(). In that path, the underlying ioredis socket/timers stay alive for the lifetime of the process, which can leak a connection and keep short-lived CLI/test processes from exiting cleanly once this function is used. If this function is only meant to decide lock domain, the health-check client should be explicitly closed (or retained and reused).

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ P2 已修復

determineLockDomain() 在確認 Redis 健康後,現在會主動呼叫 manager.disconnect() 釋放 probe client,避免 connection leak 和 process 無法乾淨退出的問題。

Fix commit: 7472e3f

if (manager && await manager.isHealthy()) {
  _lockDomainDecision = 'redis';
  await manager.disconnect(); // 釋放 probe client,避免 connection leak
  return 'redis';
}

PR #739 現有兩個 P1/P2 維護者問題均已解決。

jlin53882 pushed a commit to jlin53882/memory-lancedb-pro that referenced this pull request May 3, 2026
…connection leak

Hotfix for P2: determineLockDomain() now calls manager.disconnect()
after confirming Redis is healthy, releasing the probe connection.

Refs: chatgpt-codex-connector[bot] PR CortexReach#739 review comment
@jlin53882 jlin53882 force-pushed the fix/pr704-redis-url-parsing-v2 branch 2 times, most recently from 8a33f9c to 15a14e8 Compare May 5, 2026 12:21
jlin53882 added a commit to jlin53882/memory-lancedb-pro that referenced this pull request May 5, 2026
…dis-url-parsing.test.mjs + redis-lock.ts

PR CortexReach#739 核心變更(rebase 過程中被遺漏):
- src/redis-lock.ts: parseRedisUrl() URL parsing + determineLockDomain() single-flight
- test/redis-url-parsing.test.mjs: URL parsing 單元測試
- scripts/verify-ci-test-manifest.mjs: 新增 issue606 entry
- scripts/ci-test-manifest.mjs: 修正 entry 順序(606→680→736→492→704)
@jlin53882 jlin53882 force-pushed the fix/pr704-redis-url-parsing-v2 branch from 15a14e8 to f185132 Compare May 5, 2026 12:27
jlin53882 added a commit to jlin53882/memory-lancedb-pro that referenced this pull request May 5, 2026
…dis-url-parsing.test.mjs + redis-lock.ts

PR CortexReach#739 核心變更(rebase 過程中被遺漏):
- src/redis-lock.ts: parseRedisUrl() URL parsing + determineLockDomain() single-flight
- test/redis-url-parsing.test.mjs: URL parsing 單元測試
- scripts/verify-ci-test-manifest.mjs: 新增 issue606 entry
- scripts/ci-test-manifest.mjs: 修正 entry 順序(606→680→736→492→704)
@jlin53882 jlin53882 force-pushed the fix/pr704-redis-url-parsing-v2 branch from f185132 to 84904e4 Compare May 5, 2026 12:33
jlin53882 added 6 commits May 5, 2026 22:04
- Add redis-lock.ts (new file) with isRedisConnectionError, RedisUnavailableError, RedisLockManager, createRedisLockManager
- Add C1 compare-and-swap initPromise guard in getRedisLockManager()
- Add H1 instanceof guard before Symbol.for check in runWithFileLock()
- Add H2 depth=3 documentation in isRedisConnectionError()
- Add H4 remove pre-flight ping, let first SET() fail naturally
- Add N2 nodeTmpdir() function call fix
- Extract runWithFileLockCore() as internal fallback for file lock
…ce, Option E tests

Issue 3 fix: parseRedisUrl() now uses URL API to extract hostname/port/db
separately. DB selection (/0, /1, /2...) is preserved instead of being
eaten by replace(). Added /^\d+$/ validation to reject non-numeric DB
values (e.g. /abc) with warning log.

Issue 4 fix: Lock key namespaced by dbPath hash (hashString).
RedisLockManager stores _lockNamespace from config.dbPath.
store.ts passes this.config.dbPath to getRedisLockManager().

Issue 5 fix: Test coverage for Option E runtime behavior.
- acquire() throws RedisUnavailableError when redis is null
- isHealthy() returns false when redis is null
- Lock key namespace with dbPath works without crash

Adversarial review by Claude Code confirmed:
- Issue 3: NaN silent fallback bug found and fixed
- Issue 4: hashString collision risk low (1.6M space); no module-level cache bug
- Issue 5: Option E is a deliberate trade-off (consistency over availability)
@jlin53882 jlin53882 force-pushed the fix/pr704-redis-url-parsing-v2 branch from e5af922 to dcc8663 Compare May 5, 2026 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant