From 48a5bb89f2f5e0a0d30a1d5a9f7433e4f54e1fa4 Mon Sep 17 00:00:00 2001 From: AskaEth Date: Tue, 13 Oct 2026 21:07:46 +0800 Subject: [PATCH] =?UTF-8?q?fix(config):=20owner=5Flane.json=20=E5=B9=B6?= =?UTF-8?q?=E5=8F=91=E8=90=BD=E7=9B=98=E4=B8=8D=E5=86=8D=E6=8D=9F=E5=9D=8F?= =?UTF-8?q?/=E4=B8=A2=E6=9B=B4=E6=96=B0=EF=BC=88review=20p1=EF=BC=89?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit review p1：Set 的固定 tmp 名（path+".tmp"）+ 落盘发生在 mu.Unlock() 之后 ⇒ 并发 Set 撞同一临时文件把 JSON 写坏（load 退回默认「甲」、主人刚说的「少说点」 白说且无提示），或 rename 完成顺序与新快照先后不一致而丢更新。 修法（两条都取，各司其职）： - 落盘挪进锁内（Set 全程持 s.mu）：并发 Set 完全串行 ⇒ 快照与 rename 顺序一致， 不可能丢更新。落盘是一个小 JSON，持锁 I/O 代价可忽略；读侧读内存、不读文件。 - 每次用 os.CreateTemp 新建临时文件（仓库同款先例 internal/user/save.go）， 不再撞固定 tmp 名；CreateTemp 以 0600 打开并再 chmod 一次，任一步失败都清掉 tmp。 - 旧 save(快照) 换成 saveLocked()（调用方须持锁，直接 marshal s.cfg）。 测试：新增并发用例 TestOwnerLaneStoreConcurrentSetKeepsAllUpdates（24 轮 × 24 写者）， 断言 ① 文件始终是合法 JSON、② 每一次成功的 Set 都能在最终文件读到（不丢更新）。 修前红（原实现在同一用例上）：5/5 轮复现，两种失败模式都抓到 —— "并发落盘把配置写坏了（文件不是合法 JSON）：invalid character '\"' after top-level value" 与 "丢更新 —— 键 qq-obv11|g-N-M 不在最终文件里"；修后连续 3 次绿，-race 下亦 ok。 Bridge.SetOwnerLanePolicy 无需改动：它在 ownerLaneMu 下设内存后调注入的 store， 串行化由 store 侧保证。 --- .../runtimeconfig/channels_ownerlane.go | 55 +++++++++++++------ .../runtimeconfig/channels_ownerlane_test.go | 53 ++++++++++++++++++ 2 files changed, 92 insertions(+), 16 deletions(-) diff --git a/internal/config/runtimeconfig/channels_ownerlane.go b/internal/config/runtimeconfig/channels_ownerlane.go index f1b5f0c..8833ac6 100644 --- a/internal/config/runtimeconfig/channels_ownerlane.go +++ b/internal/config/runtimeconfig/channels_ownerlane.go @@ -138,10 +138,17 @@ func (s *OwnerLaneStore) Snapshot() OwnerLaneFile { } // Set 写入某个 (platform, groupID) 的策略并落盘。groupID 为空表示写全局 default 档。 -// 内存先改（行为立即生效），再尽力落盘；落盘失败返回错误（调用方留可见 WARN）， -// 但内存里的新档位仍然生效 —— 「改不动磁盘」不该让已经做出的行为决定回退。 +// +// 内存改动与**落盘都在同一把锁内**（review p1）：并发 Set 因此完全串行 —— 既不会两个 +// 写者去写同一个临时文件把 JSON 写坏，也不会因为原子 rename 的完成先后与各快照的新旧 +// 不一致而丢掉「后发生的那次更新」。落盘量是一个小 JSON，持锁期间的 I/O 代价可忽略； +// 读侧（PolicyFor）读的是内存、不碰文件，因此不会被这次落盘抖动。 +// +// 内存先改、失败返回错误（调用方留可见 WARN），但内存里的新档位仍然生效 —— +// 「改不动磁盘」不该让已经做出的行为决定回退。 func (s *OwnerLaneStore) Set(platform, groupID string, p OwnerLanePolicy) error { s.mu.Lock() + defer s.mu.Unlock() if groupID == "" { s.cfg.Default = p } else { @@ -150,20 +157,18 @@ func (s *OwnerLaneStore) Set(platform, groupID string, p OwnerLanePolicy) error } s.cfg.Groups[OwnerLaneGroupKey(platform, groupID)] = p } - snapshot := OwnerLaneFile{Default: s.cfg.Default, Groups: make(map[string]OwnerLanePolicy, len(s.cfg.Groups))} - for k, v := range s.cfg.Groups { - snapshot.Groups[k] = v - } - s.mu.Unlock() - - if err := s.save(snapshot); err != nil { + if err := s.saveLocked(); err != nil { return fmt.Errorf("runtimeconfig: save owner lane config %s: %w", s.path, err) } return nil } -// save 原子写出配置（临时文件 + rename），使并发读者永远不会看到半份文件。 -func (s *OwnerLaneStore) save(f OwnerLaneFile) error { +// saveLocked 原子写出当前配置。调用方**必须持有 s.mu**。 +// +// 它每次都用**新建的临时文件**（`os.CreateTemp`，仓库同款先例见 internal/user/save.go） +// 再原子 rename：即便有别的进程/goroutine 也在写同一个目录，也不会撞同一个临时文件名， +// 而半份文件永远不会以 s.path 的名字出现。 +func (s *OwnerLaneStore) saveLocked() error { if s.path == "" { return nil } @@ -173,15 +178,33 @@ func (s *OwnerLaneStore) save(f OwnerLaneFile) error { return fmt.Errorf("mkdir: %w", err) } } - raw, err := json.MarshalIndent(f, "", " ") + raw, err := json.MarshalIndent(s.cfg, "", " ") if err != nil { return fmt.Errorf("marshal: %w", err) } - tmp := s.path + ".tmp" - if err := os.WriteFile(tmp, raw, 0o600); err != nil { - return fmt.Errorf("write %s: %w", tmp, err) + tmp, err := os.CreateTemp(dir, filepath.Base(s.path)+".*.tmp") + if err != nil { + return fmt.Errorf("create temp in %s: %w", dir, err) } - if err := os.Rename(tmp, s.path); err != nil { + tmpName := tmp.Name() + // CreateTemp 以 0600 打开；显式再 chmod 一次，使落盘文件不因 umask 变化而更宽。 + // 任一步失败都删掉临时文件，绝不留垃圾。 + if err := tmp.Chmod(0o600); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpName) + return fmt.Errorf("chmod %s: %w", tmpName, err) + } + if _, err := tmp.Write(raw); err != nil { + _ = tmp.Close() + _ = os.Remove(tmpName) + return fmt.Errorf("write %s: %w", tmpName, err) + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmpName) + return fmt.Errorf("close %s: %w", tmpName, err) + } + if err := os.Rename(tmpName, s.path); err != nil { + _ = os.Remove(tmpName) return fmt.Errorf("rename: %w", err) } return nil diff --git a/internal/config/runtimeconfig/channels_ownerlane_test.go b/internal/config/runtimeconfig/channels_ownerlane_test.go index df8a2bf..1ce0001 100644 --- a/internal/config/runtimeconfig/channels_ownerlane_test.go +++ b/internal/config/runtimeconfig/channels_ownerlane_test.go @@ -1,8 +1,11 @@ package runtimeconfig import ( + "encoding/json" + "fmt" "os" "path/filepath" + "sync" "testing" ) @@ -79,3 +82,53 @@ func TestOwnerLaneStoreCorruptFileFallsBackToDefaults(t *testing.T) { t.Fatalf("坏配置下策略 = %+v, want 默认档", got) } } + +// TestOwnerLaneStoreConcurrentSetKeepsAllUpdates 钉住并发 Set 的安全性（review p1）： +// 落盘必须与内存改动**同一把锁**、且用**新建的临时文件**，否则两个写者会撞同一个 +// tmp 名把 JSON 写坏（⇒ load 退回默认「甲」、主人刚说的「少说点」白说且无提示）， +// 或者 rename 的完成顺序与新快照的先后不一致，丢掉后发生的更新。 +// +// 修前必红（原实现：固定 `path+".tmp"` + mu.Unlock() 之后落盘），断言： +// ① 每一轮结束后文件都是**合法 JSON**； +// ② 本轮写过的**每一个**键都能在最终文件里读到（不丢更新）。 +func TestOwnerLaneStoreConcurrentSetKeepsAllUpdates(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "config", "owner_lane.json") + s := NewOwnerLaneStore(path, nil) + + const ( + rounds = 24 + writers = 24 + ) + for round := 0; round < rounds; round++ { + var wg sync.WaitGroup + for i := 0; i < writers; i++ { + key := fmt.Sprintf("g-%d-%d", round, i) + wg.Add(1) + go func(i int) { + defer wg.Done() + // 写者的返回值故意不看：修前它会因 tmp 被别的写者 rename 走而报错， + // 但那正是本用例要靠**文件内容**抓住的失败。 + _ = s.Set("qq-obv11", key, OwnerLanePolicy{ReplyToUnaddressed: true, MinReplyGapSec: i}) + }(i) + } + wg.Wait() + + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("round %d: 读回 owner_lane.json 失败：%v", round, err) + } + var f OwnerLaneFile + if err := json.Unmarshal(raw, &f); err != nil { + t.Fatalf("round %d: 并发落盘把配置写坏了（文件不是合法 JSON）：%v\n内容=%q", round, err, raw) + } + for i := 0; i < writers; i++ { + key := OwnerLaneGroupKey("qq-obv11", fmt.Sprintf("g-%d-%d", round, i)) + if _, ok := f.Groups[key]; !ok { + t.Fatalf("round %d: 丢更新 —— 键 %s 不在最终文件里（并发的某次 Set 没落盘）", round, key) + } + } + } +} +