Skip to content

fix(redis-lock): URL parsing fix + lock domain single-flight (PR-1)#709

Closed
jlin53882 wants to merge 2 commits intoCortexReach:masterfrom
jlin53882:pr-1/redis-url-and-single-flight
Closed

fix(redis-lock): URL parsing fix + lock domain single-flight (PR-1)#709
jlin53882 wants to merge 2 commits intoCortexReach:masterfrom
jlin53882:pr-1/redis-url-and-single-flight

Conversation

@jlin53882
Copy link
Copy Markdown
Contributor

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

變更範圍

  • 新增 :parseRedisUrl() 修復 URL 解析 bug
    • 正確處理 legacy (host:port)、auth、rediss://、query string 格式
    • 舊實作 破壞含密碼/TLS/query 的 URL
  • 新增 :13 個 URL parsing 測試案例(全部通過)
  • 修改 :加入 getLockDomain() single-flight 決策
    • 全程序只決定一次 Redis 或 file lock
    • 避免 concurrent request 在 init 完成前看到不同 lock domain
    • 若 redis-lock.js 不存在或匯入失敗,預設用 file lock(不影響現有行為)

PR 拆分設計背景

本 PR 為 #704 拆分方案的第一階段(PR-1/3)。完整設計見:PR704_SPLIT_DESIGN_v3.md

PR 內容 狀態
PR-1(本PR) URL parsing + single-flight
PR-2 RedisLockManager 主體實作
PR-3 整合測試 + CI

對抗式審查(Claude Code)

  • ✅ URL parsing 13 個測試案例全部通過
  • ✅ 不影響現有 file lock 行為(工廠失敗時正確降級)

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: ecda19381b

ℹ️ 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
* 目前工廠函式在 Redis 不可用時正確返回 null,確保現有 file lock 行為不受影響。
*/

import Redis from 'ioredis';
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Declare ioredis as a runtime dependency

src/redis-lock.ts now has a top-level import 'ioredis', but this package is not added to package.json/package-lock.json. In a clean install, importing this module will throw ERR_MODULE_NOT_FOUND, so any future use of determineLockDomain() silently falls back to file lock and the new URL-parsing test cannot run when dependencies are installed from lockfile. Please add ioredis as a declared dependency in this same change.

Useful? React with 👍 / 👎.

@jlin53882
Copy link
Copy Markdown
Contributor Author

Codex 對抗性審查問題回應

感謝 Codex 的對抗性審查。以下是逐項回應:


🔴 Critical — Dead Integration(已修復 ✅)

Codex 聲稱: 沒被使用,Redis locking 完全沒有接入寫入路徑。

驗證結果:✅ 屬實。 確實被定義但從未被任何寫入路徑呼叫。

修復內容(commit ):

  • :新增 +
  • :新增 + 統一路由方法
  • 6 個 write path( / / / / / )全部從 改為
  • 目前 RedisLockManager 只有骨架(PR-2 才會有 acquire/release),Redis path 目前 fallback 到 file lock,避免行為改變

🟡 Medium — Connection Leak(已修復 ✅)

Codex 聲稱:健康檢查成功後 Redis client 連線不關閉,會 leak。

驗證結果:✅ 方向正確,但描述不夠精確。真正問題是:健康檢查成功後 被 return,但沒有任何地方儲存這個 reference,後續根本無法使用同一個連線。

修復內容

  • 模組級別儲存,manager reference 不再被丟棄

🟡 Medium — Sticky Fallback(不成立 ❌)

Codex 聲稱:Redis 單次失敗後永久放棄,即使後來恢復也不重試。

驗證結果:❌ 不成立。Codex 把兩件事搞混了:

  1. 第一次成功後永遠用 Redis(這是正確的設計)
  2. 單次失敗後永久放棄(這不是事實)

是刻意設計,用 single-flight 避免 lock domain 分裂(req-A 用 Redis lock,req-B 用 file lock)。File lock failure 不 retry 是對的,避免 Redis 掛掉時觸發 storm。


❌ 不成立 — 測試檔 SyntaxError

Codex 聲稱: 有 JS SyntaxError,無法執行。

驗證結果:❌ 不成立。測試檔完全正常,執行結果:13/13 通過。Codex 的診斷是錯誤的。


完整問題清單

# 問題 狀態
1 Dead integration — Redis locking 沒接入 store.ts ✅ 已修復
2 Redis connection leak — manager reference 丟失 ✅ 已修復
3 Sticky fallback — 單次失敗永久放棄 ❌ 不成立
4 測試檔無效 — SyntaxError ❌ 不成立

PR #709 審查流程說明

Codex 做的是 spec 符合性掃描("實作符合描述嗎"),但沒有 cross-file code path tracing 的視角,所以才會同時出現「漏抓真正的問題」和「誤判正常的行為」。這是對抗性審查的價值所在——它挑戰的是整個假設框架,而不只驗證實作細節。

@jlin53882
Copy link
Copy Markdown
Contributor Author

測試檔已加入 CI manifest ✅

commit 6df6831 已將 test/redis-url-parsing.test.mjs 加入官方 CI:

檔案 變更
package.json test script 加入測試
scripts/ci-test-manifest.mjs storage-and-schema 群組加入測試
scripts/verify-ci-test-manifest.mjs EXPECTED_BASELINE 加入測試

CI 執行方式:

npm test                        # 含 redis-url-parsing.test.mjs
npm run test:storage-and-schema  # 只跑 storage-and-schema 群組

@jlin53882 jlin53882 force-pushed the pr-1/redis-url-and-single-flight branch from 6df6831 to ff10636 Compare April 29, 2026 03:00
@jlin53882 jlin53882 closed this May 1, 2026
@jlin53882 jlin53882 force-pushed the pr-1/redis-url-and-single-flight branch from ff10636 to 0545c91 Compare May 1, 2026 15:07
jlin53882 added 2 commits May 1, 2026 23:10
PR-1 基礎設施(URL parsing + single-flight decision):

新增:
- src/redis-lock.ts:parseRedisUrl() 修復 URL 解析 bug
  - 正確處理 legacy (host:port)、auth、rediss://、query string
  - 舊實作 .replace('redis://','') 破壞含密碼/TLS/query 的 URL
- test/redis-url-parsing.test.mjs:13 個測試案例

修改:
- src/store.ts:加入 getLockDomain() single-flight 決策
  - 全程序只決定一次 Redis 或 file lock
  - 避免 concurrent request 在 init 完成前看到不同 lock domain
  - 若 redis-lock.js 不存在或匯入失敗,預設用 file lock(不影響現有行為)
@jlin53882 jlin53882 reopened this May 1, 2026
@jlin53882
Copy link
Copy Markdown
Contributor Author

⚠️ 已由 PR #739 取代

本 PR (#709) 的內容已重新整理並推送至 #739

本 PR 維護者問題處理狀況

問題 等級 PR #739 狀態
ioredis 未宣告為 dependency P1 已修復 — package.json 已加入 ioredis ^5.10.1
Probe Redis 連線未釋放 P2 ⚠️ 待處理 — PR #739 尚有 open P2 comment

PR #739#739

@jlin53882 jlin53882 closed this May 3, 2026
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