Batch: DRY extractions for issues #504, #511, #508, #516#695
Conversation
…nate cleanup duplication - Extract trackPID() and untrackPID() from 3 inline copy-paste sites - start(), shutdownProcess(), kill() now call shared helpers - Eliminates risk of new cleanup paths forgetting PID tracking Fixes hrygo#504
…inate/Kill/Wait lock pattern - Extract withProc (error) and withProcCode (int, error) to eliminate repeated lock-get-nil-unlock pattern across 3 methods - Terminate/Kill use withProc, Wait uses withProcCode - Reduces 18 Lock/Unlock calls to 6, single source of truth for lock protocol Fixes hrygo#511
…extract Upsert JSON helpers - Remove Compact() from Store interface — it was never called via the interface; SQLiteStore retains it as a concrete method - Remove pgStore Compact no-op - Remove mockStore Compact stub - Extract marshalSessionJSON() and upsertTimeout() to eliminate Upsert JSON serialization duplication between SQLite and PG backends Fixes hrygo#508
… duplication - Add apiKeyStoreBase with shared list/get/delete/Invalidator/SetInvalidator - Add prepareQuery() for dialect rebind and ensureAPIKey() for key generation - Add withLock func field to abstract write serialization (writeMu vs identity) - SQLite apiKeyUserStore embeds base, keeps create/update (LastInsertId + app timestamps) - PG pgStore embeds base, keeps create/update (RETURNING id + NOW()) - Eliminates ~50 lines of copy-pasted CRUD logic between backends Fixes hrygo#516
hotplex-ai
left a comment
There was a problem hiding this comment.
Code Review — hotplex-ai
Verdict: ✅ APPROVE | P0:0 P1:0 P2:0 P3:3
4 个 DRY 提取均正确保持了原始语义,无正确性/安全/并发回归。重构干净,行为等价。
P3 — 建议性清理(不阻塞合并)
-
e2e mockStore 残留 Compact 方法 (
e2e/helper_test.go) —Compact已从session.Store接口移除,但 e2e 测试的 mockStore 仍保留该方法。无害死代码,建议同步清理保持一致性。 -
apiKeyUserStore.writeMu 成为死字段 (
internal/admin/apikey_handlers.go:131) — 重构后writeMu仅在构造函数中用于初始化base.withLock,之后apiKeyUserStore上的writeMu字段不再被任何方法读取。可移除该字段以消除死代码。 -
PG create/update 跳过 withLock (
internal/admin/apikey_pg_store.go) —delete经由 basewithLock(PG 下为 identity),但create/update直接执行未经过withLock。当前功能无差异,但如果未来withLock加入 metrics/logging,PG 的写操作将被遗漏。建议统一模式或添加注释说明有意跳过。
P3-1: Remove dead Compact() from e2e mockStore (interface removed in 4623dbb) P3-2: Remove dead writeMu field from apiKeyUserStore (absorbed into apiKeyStoreBase.withLock) P3-3: Wrap PG create/update in withLock for consistency with delete
admin/apikey: replace rebind/withLock function fields with dbutil.Dialect and *sqlutil.WriteMu — eliminates nil-sentinel checks and uses existing dialect-aware types from the codebase. gateway/tests: remove dead Compact() and DeleteExpiredEvents() from ctrl_test.go and conn_test.go mock stores (removed from Store interface in 4623dbb). messaging/tts: extract trackPID/untrackPID helpers on MossProcess, consistent with stt.PersistentSTT pattern established in b7bb9871.
P3 code review optimizations: - admin: remove prepareQuery wrapper, use dialect.Rebind() directly (consistent with session/cron/eventstore stores) - proc: extract TrackPID/UntrackPID shared helpers from stt/tts/manager - base: unify withProc/withProcCode into generic withProcResult[T] Net: 6 files, -29 lines
hotplex-ai
left a comment
There was a problem hiding this comment.
Code Review — hotplex-ai
Verdict: ✅ APPROVE | P0:0 P1:0 P2:1 P3:2
DRY 重构质量高,行为完全保留,无回归风险。泛型 withProcResult[T] 设计干净,PID helpers 提取合理,session 序列化/超时统一正确。
P2 (1)
docs/specs/Dual-Database-Support-Spec.md:97—Compact已从session.Store接口移除但 spec 仍记载方法数=9 并列出Compact,需同步更新。 [80%]
P3 (2)
-
internal/admin/apikey_handlers.go:73—apiKeyStoreBase.list()未使用dialect.Rebind()(查询无 bind 参数故无实际影响,但与同结构体get()/delete()风格不一致)。 [80%] -
internal/worker/base/worker.go:74—withProcResult在fn(p)返回 error 时不清除w.Proc——这是对旧代码行为的精确保留(旧Wait()/Terminate()/Kill()均如此),非回归,但值得注意:进程已退出后Proc引用不会被释放。 [75%]
Summary
4 high-ROI DRY extractions targeting repeated code patterns across worker, messaging, session, and admin modules.
Closes #504, #511, #508, #516
Changes by Issue
#504 — messaging/stt: PersistentSTT PID tracker cleanup DRY
trackPID()anduntrackPID()helpers on PersistentSTTstart(),shutdownProcess(),kill()#511 — worker/base: Terminate/Kill/Wait lock pattern DRY
withProc()(error) andwithProcCode()(int, error) helperswithProc, Wait useswithProcCode#508 — session/store: Compact ISP violation + Upsert DRY
Compact()fromStoreinterface (never called via interface)marshalSessionJSON()andupsertTimeout()to eliminate Upsert JSON serialization duplication#516 — admin: dual-store CRUD DRY
apiKeyStoreBasewith sharedlist/get/delete/Invalidator/SetInvalidatorprepareQuery()for dialect rebind andensureAPIKey()for key generationwithLockfunc field to abstract write serialization (writeMu vs identity)create/updatewhere dialect differences are realTesting
make test-short— all packages passmake lint— 0 issuesmake build— compiles cleanlyCommits
4 atomic commits, one per issue:
refactor(messaging/stt): extract trackPID/untrackPID helpersrefactor(worker/base): extract withProc/withProcCode helpersrefactor(session/store): remove Compact from Store interface + extract Upsert helpersMerged from #694