255 lines
15 KiB
Markdown
255 lines
15 KiB
Markdown
# 架构决策记录：QA 交叉验证发现的架构级遗漏
|
||
|
||
> 日期：2026-05-11
|
||
> 决策人：TechLead
|
||
> 代码基线：ee3a31e
|
||
> 关联文档：docs/REMEDIATION_TASK_BOARD_2026-05-06.md
|
||
|
||
---
|
||
|
||
## 0. 决策总览
|
||
|
||
| 编号 | 问题 | 选择方案 | 优先级 |
|
||
|---|---|---|---|
|
||
| D-01 | callback_target 契约漂移 | ① 删除 CallbackTarget 字段及其使用 | P1 |
|
||
| D-02 | outbox 并发 claim 缺失 | ① 增加行级锁（UPDATE RETURNING 实现） | P1 |
|
||
| D-03 | platform webhook 非严格事务外盒 | ② 在文档/代码中明确标注"最终一致性"边界 | P1 |
|
||
| D-04 | E2E 稳定性根因 | QA 分析 → TechLead 确认方向 → Engineer 修复 | P1 |
|
||
|
||
---
|
||
|
||
## 1. D-01：callback_target 契约漂移
|
||
|
||
### 1.1 现状
|
||
- `internal/service/platformevents/builder.go:31-46` 从 `meta.CallbackTarget` 读取并写入 `event.CallbackTarget`
|
||
- `internal/domain/platformevent/event.go:41` 定义 `CallbackTarget string` 字段，`Validate()` 强制非空
|
||
- `internal/platformadapter/types.go:25` `PlatformInboundMeta` 定义 `CallbackTarget string`
|
||
- `internal/service/platformdelivery/worker.go:111` 投递时直接使用固定的 `w.CallbackURL`，**完全不消费** `event.CallbackTarget`
|
||
- `internal/app/app.go:169-171` worker 初始化按平台只配一个 `CallbackBaseURL`，无多目标路由模型
|
||
|
||
### 1.2 方案选择：① 删除 CallbackTarget 字段及其使用
|
||
|
||
#### 理由
|
||
当前平台配置模型（`config.PlatformAdapterProfileConfig`）是**每平台一个固定回调地址**，且没有多目标回调的产品需求证据。保留一个"模型字段存在、校验强制非空、但运行时零消费"的伪能力，会持续造成：
|
||
1. 数据模型与运行时行为不一致（契约漂移）
|
||
2. 新成员阅读代码时的认知负担
|
||
3. 未来重构时误以为已支持多目标路由
|
||
|
||
删除该字段是**低成本、可回退**的清理动作。若未来确有同一平台多目标回调需求，届时必然伴随配置模型重构（从单 URL 到路由表），同步恢复字段即可。
|
||
|
||
#### 风险
|
||
- 回滚成本：需从 git 历史恢复字段，改动面可控。
|
||
- 数据兼容性：DB 中已有 `callback_target` 列，本次只清理 Go 层代码，DB 列暂不删除（避免对已部署环境做破坏性 schema 变更），后续通过独立 migration 清理。
|
||
|
||
#### 回退策略
|
||
若产品侧在本周内提出多目标回调需求，立即中止删除，改为方案②（worker 按 CallbackTarget 路由到配置表 `cs_platform_callbacks`）。
|
||
|
||
### 1.3 文件级落点
|
||
|
||
| 文件 | 位置 | 动作 |
|
||
|---|---|---|
|
||
| `internal/domain/platformevent/event.go` | L41 | 删除 `CallbackTarget string` 字段 |
|
||
| `internal/domain/platformevent/event.go` | L63-65 | 删除 `Validate()` 中 `CallbackTarget` 非空校验 |
|
||
| `internal/platformadapter/types.go` | L25 | 删除 `PlatformInboundMeta.CallbackTarget` |
|
||
| `internal/service/platformevents/builder.go` | L15 | 删除 `defaultCallbackTarget` 常量 |
|
||
| `internal/service/platformevents/builder.go` | L31-34 | 删除 `callbackTarget` 读取与兜底逻辑 |
|
||
| `internal/service/platformevents/builder.go` | L46 | 删除 `CallbackTarget` 赋值 |
|
||
| `internal/store/postgres/platform_event_store.go` | L52,58 | 删除 `INSERT` 中 `callback_target` 列与参数 |
|
||
| `internal/store/postgres/platform_event_store.go` | L80,106 | 删除 `SELECT` 中 `callback_target` 列与扫描 |
|
||
| `internal/store/postgres/platform_event_store.go` | L185-186 | 删除 `dead_letter` 插入中 `callback_target` |
|
||
| `internal/service/platformevents/builder_test.go` | 各 `CallbackTarget: "default"` | 删除相关断言与字段 |
|
||
| `internal/store/postgres/platform_event_store_test.go` | 各 `CallbackTarget: "default"` | 删除相关字段 |
|
||
| `internal/service/platformdelivery/worker_test.go` | 各 `CallbackTarget: "default"` | 删除相关字段 |
|
||
| `internal/domain/platformevent/event_test.go` | 各 `CallbackTarget: "default"` | 删除相关字段 |
|
||
|
||
> **注意**：`db/migration/0002_platform_event_outbox.up.sql` 本次不修改；DB 列保留留空运行，待后续专门做 schema 清理 migration。
|
||
|
||
### 1.4 QA 可验证项
|
||
1. `grep -r "CallbackTarget" --include="*.go" internal/` 返回空（除可能存在的 vendor 外）
|
||
2. `go test ./internal/... -count=1` 全部通过
|
||
3. `go build ./...` 通过
|
||
|
||
---
|
||
|
||
## 2. D-02：outbox 并发 claim 缺失
|
||
|
||
### 2.1 现状
|
||
- `internal/store/postgres/platform_event_store.go:78-86` `ListDue` 使用普通 `SELECT ... ORDER BY ... LIMIT`
|
||
- 无 `FOR UPDATE SKIP LOCKED`，多实例部署时多个 worker 会读到同一批事件，导致**重复投递**
|
||
- `MarkDelivered` / `MarkRetry` / `MarkDeadLetter` 是独立 SQL，存在竞态窗口
|
||
|
||
### 2.2 方案选择：① 增加行级锁（UPDATE RETURNING 实现）
|
||
|
||
#### 理由
|
||
显式声明"单实例约束"（方案②）会限制架构扩展性，与生产级目标不符。纯 `SELECT ... FOR UPDATE SKIP LOCKED` 若不在事务内保持锁则无意义（`ListDue` 返回后事务结束，锁释放）。
|
||
|
||
最简生产级实现是把 `ListDue` 改造为**事务内 claim 模式**：
|
||
```sql
|
||
UPDATE cs_platform_event_outbox
|
||
SET status = 'claiming', updated_at = NOW()
|
||
WHERE id IN (
|
||
SELECT id FROM cs_platform_event_outbox
|
||
WHERE platform = $1 AND status IN ('pending','retrying') AND next_attempt_at <= $2
|
||
ORDER BY next_attempt_at ASC, occurred_at ASC, created_at ASC, id ASC
|
||
LIMIT $3
|
||
FOR UPDATE SKIP LOCKED
|
||
)
|
||
RETURNING *
|
||
```
|
||
- 一行 SQL 完成"筛选 + 加锁 + 状态变更 + 返回"
|
||
- 其他实例并发时因 `SKIP LOCKED` 自动跳过已锁行
|
||
- worker 投递完成后，再调用 `MarkDelivered` / `MarkRetry` 将状态从 `claiming` 迁移到终态
|
||
|
||
#### 风险
|
||
- **claim 悬挂**：worker 在 `claiming` 后 crash，事件会永远卡在 `claiming`。需要新增**claim 超时回收机制**（如每 30 秒把超过 5 分钟的 `claiming` 重置为 `retrying`）。
|
||
- 新增 `claiming` 状态需要修改 DB CHECK 约束，需新增 migration。
|
||
|
||
#### 回退策略
|
||
若实现周期超过 2 人日，可先合并 `claiming` 状态与超时回收逻辑，但保留单实例部署注释作为兜底声明。
|
||
|
||
### 2.3 文件级落点
|
||
|
||
| 文件 | 位置 | 动作 |
|
||
|---|---|---|
|
||
| `db/migration/0003_outbox_claiming_status.up.sql` | 新增 | 新增 migration：将 `'claiming'` 加入 `chk_cs_platform_event_outbox_status`；建议同时加 `idx_cs_platform_event_outbox_claiming_timeout` 索引 (`status, updated_at`) |
|
||
| `internal/domain/platformevent/event.go` | L12-16 | 新增 `StatusClaiming Status = "claiming"`；`Validate()` 中允许该状态 |
|
||
| `internal/store/postgres/platform_event_store.go` | L78-86 | `ListDue` 改为事务内执行上述 `UPDATE ... RETURNING` 模式 |
|
||
| `internal/store/postgres/platform_event_store.go` | 新增方法 | 新增 `ReleaseStaleClaims(ctx, timeout time.Duration) (int, error)`：将超时的 `claiming` 重置为 `retrying` |
|
||
| `internal/service/platformdelivery/worker.go` | L60-80 `Start` | 在 `Start` 中新增一个额外 ticker（如每 30 秒），调用 `ReleaseStaleClaims` |
|
||
| `internal/service/platformdelivery/worker.go` | L93-102 `RunOnce` | 保持现有 `ListDue` 调用方式（接口名不变，内部实现已改），投递成功后正常 `MarkDelivered`/`MarkRetry` |
|
||
|
||
### 2.4 QA 可验证项
|
||
1. **并发无重复投递**：在测试中启动两个 worker（两个 goroutine 或两个进程），注入 100 条事件，验证 callback server 收到的 event_id 无重复。
|
||
2. **claim 回收**：模拟 worker crash（在 `ListDue` 后、投递前 panic），等待 claim 超时后，验证事件被重新置为 `retrying` 并最终被成功投递。
|
||
3. `go test ./internal/service/platformdelivery/... -count=1` 通过。
|
||
|
||
---
|
||
|
||
## 3. D-03：platform webhook 非严格事务外盒
|
||
|
||
### 3.1 现状
|
||
- `internal/http/handlers/platform_webhook_handler.go:86` 调用 `h.dialog.Process`
|
||
- `internal/http/handlers/platform_webhook_handler.go:100` 调用 `h.eventWriter.InsertPendingBatch`
|
||
- 两者**不在同一事务**
|
||
- `dialog.Process` 成功但 outbox 写入失败时，业务状态（Session/Ticket/Audit）已持久化，但下游收不到事件
|
||
|
||
### 3.2 方案选择：② 在文档/代码中明确标注"最终一致性"边界
|
||
|
||
#### 理由
|
||
当前 `dialog.Process` **内部本身也没有事务外盒**：
|
||
- `SessionRepository.GetOrCreate`（独立 SQL）
|
||
- `DedupRepository.TryRecord`（独立 SQL）
|
||
- `TicketRepository.Create`（独立 SQL）
|
||
- `SessionRepository.Save`（独立 SQL）
|
||
- `AuditRepository.Add`（独立 SQL）
|
||
|
||
各 repository 调用均为独立连接操作。单独把 outbox 写入拉进 dialog 事务是**局部优化**，不能解决 dialog 内部一致性问题。要真正做到严格事务外盒，需要重构整个 repository 层，引入 `UnitOfWork` 或 `TxProvider`，改动面大、风险高、与当前稳定性优先目标不符。
|
||
|
||
当前阶段更务实的选择是：
|
||
1. **明确承认最终一致性边界**
|
||
2. 利用现有机制降低风险：outbox 写入失败会返回 HTTP 500，平台方可重试；`Dedup.TryRecord` 可防止重复处理导致的业务状态重复变更
|
||
|
||
#### 风险
|
||
- 小概率场景：dialog 成功（Session/Ticket 已变）+ outbox 失败 → 下游无事件。但该场景会返回 500，平台 webhook 调用方可重试；重试时 dedup 保证业务状态不再变更，outbox 会重新写入。
|
||
- 若平台方不重试，则出现状态与事件不一致。需在 API 文档中明确告知平台方必须处理 500 重试。
|
||
|
||
#### 回退策略
|
||
未来当 repository 层统一引入 `UnitOfWork / sql.Tx` 支持后，可将 dialog 全链路 + outbox 纳入同一事务。届时从文档和代码注释中移除"最终一致性"标注。
|
||
|
||
### 3.3 文件级落点
|
||
|
||
| 文件 | 位置 | 动作 |
|
||
|---|---|---|
|
||
| `internal/http/handlers/platform_webhook_handler.go` | L86-103 之间 | 增加注释块，说明 `dialog.Process` 与 `outbox.InsertPendingBatch` 不在同一事务，架构定位为最终一致性；outbox 失败返回 500，由调用方重试 |
|
||
| `internal/service/dialog/service.go` | 文件头部或 `Process` 上方 | 增加注释，说明当前 repository 层无统一事务外盒，各存储操作为最终一致性 |
|
||
| 本文档 | - | 作为正式架构决策记录留存 |
|
||
|
||
### 3.4 QA 可验证项
|
||
1. 阅读 `platform_webhook_handler.go` 与 `dialog/service.go` 注释，确认存在"最终一致性"说明。
|
||
2. 无需要运行的专属测试（本决策为架构声明，非代码功能变更）。
|
||
|
||
---
|
||
|
||
## 4. D-04：E2E 稳定性根因分析与修复路径
|
||
|
||
### 4.1 现状
|
||
- `test/e2e/sub2api_callback_flow_test.go` 反复出现：
|
||
- 事件顺序异常：`reply.generated` 先于 `message.received`
|
||
- `sql: database is closed`
|
||
|
||
### 4.2 根因分析（TechLead 预评估，需 QA 确认）
|
||
|
||
#### 根因 A：`sql: database is closed`
|
||
- 测试函数内 `defer db.Close()`（L150）在 `t.Cleanup(application.Shutdown)`（L102-104）之前执行
|
||
- Go 执行顺序：函数体 defer（LIFO）→ `t.Cleanup`（LIFO）
|
||
- 结果：DB 先关闭 → application worker 仍在运行 → worker 访问已关闭的 DB → `database is closed`
|
||
|
||
#### 根因 B：事件顺序异常
|
||
- `platformdelivery.Worker.RunOnce` 是**串行** `for` 循环逐个投递（worker.go:98-102）
|
||
- 但 callback server 是 `httptest.Server`，对每个 HTTP 请求启动**独立 goroutine**
|
||
- 测试中的 callback handler 用 `mu.Lock` 保护 `received` 切片，但**锁的获取顺序 ≠ HTTP 请求发起顺序**
|
||
- 结果：`message.processing` 的请求 handler 可能先拿到锁，先于 `message.received` 被 append
|
||
|
||
### 4.3 修复方向（TechLead 确认）
|
||
|
||
| 根因 | 修复方向 |
|
||
|---|---|
|
||
| `database is closed` | 调整测试生命周期：将 `db.Close` 从 `defer` 移入 `t.Cleanup`，并确保其在 `application.Shutdown` **之后**执行；或让 `application` 复用测试创建的 `sql.DB` 并由 `Shutdown` 统一关闭 |
|
||
| 事件顺序异常 | **方案一（推荐）**：callback server 改为同步队列模式（channel + 单 goroutine 消费），保证收到的顺序与 worker 发出顺序一致；**方案二**：断言逻辑改为按语义排序后比较（不依赖 received 原始顺序） |
|
||
|
||
### 4.4 执行路径
|
||
|
||
1. **QA 负责**：复跑测试并抓取完整日志，确认上述两个根因与预评估一致；输出根因分析报告（含时间线、goroutine 竞争证据）。
|
||
2. **TechLead 负责**：审核 QA 根因报告，确认修复方向（本 ADR 已预先确认方向如上）。
|
||
3. **Engineer 负责**：按确认后的方向修改测试代码；若 QA 发现新的根因，TechLead 重新评估方向。
|
||
|
||
### 4.5 文件级落点
|
||
|
||
| 文件 | 位置 | 动作 |
|
||
|---|---|---|
|
||
| `test/e2e/sub2api_callback_flow_test.go` | L149-150 | 移除 `defer db.Close()`；改为在 `t.Cleanup` 中关闭，且注册顺序晚于 `application.Shutdown` 的 Cleanup |
|
||
| `test/e2e/sub2api_callback_flow_test.go` | L157-166 | callback server handler 改为同步队列：用带缓冲 channel 收集事件，断言侧从 channel 按顺序读取 |
|
||
| `test/e2e/sub2api_callback_flow_test.go` | L209-230 | `waitForSessionEvents` / 断言逻辑适配同步队列模式 |
|
||
|
||
### 4.6 QA 可验证项
|
||
1. `go test ./test/e2e/... -run TestSub2APICallbackFlow -count=10` 连续 10 次全部通过
|
||
2. 无 `database is closed` 报错
|
||
3. 无事件顺序失败
|
||
|
||
---
|
||
|
||
## 5. 与旧 Remediation Board 的兼容性评估
|
||
|
||
| 本 ADR 编号 | Remediation Board 对应项 | 兼容性 |
|
||
|---|---|---|
|
||
| D-01 | D-03 / I-03（callback_target 契约漂移） | **兼容并细化**。Board 要求"删除伪能力或落地多目标路由"，本 ADR 明确选择删除，给出完整文件级落点。 |
|
||
| D-02 | D-04 / I-04（outbox 并发 claim） | **兼容并升级**。Board 要求"至少明确单实例约束，或设计 claim/skip locked 方案"，本 ADR 选择生产级 claim 方案（UPDATE RETURNING），比最低要求更进取，但落点可控。 |
|
||
| D-03 | I-05（transactional outbox） | **兼容**。Board 允许"评估并落地更严格方案，或明确记录当前非强一致边界"，本 ADR 选择后者并给出理由（dialog 内部本身无事务）。 |
|
||
| D-04 | V-02（E2E 稳定性复跑） | **兼容并细化**。Board 要求"形成重复执行证据"，本 ADR 给出明确根因假设、修复方向和 QA/Engineer 分工。 |
|
||
|
||
**结论**：本 ADR 的所有决策均可在不推翻 Remediation Board 的前提下执行，是对 Board 中设计段/实现段任务的具体技术收口。
|
||
|
||
---
|
||
|
||
## 6. 阶段门控结论
|
||
|
||
| 检查项 | 结论 | 说明 |
|
||
|---|---|---|
|
||
| D-01 设计是否可进入实现 | **通过** | 删除方向明确，文件级落点已枚举，回退策略清晰 |
|
||
| D-02 设计是否可进入实现 | **通过** | UPDATE RETURNING 方案已确定，需 Engineer 评估 claim 超时回收实现复杂度（预计不超过 1 人日） |
|
||
| D-03 设计是否可进入实现 | **通过** | 仅代码注释与文档标注，无功能代码变更，风险最低 |
|
||
| D-04 是否可进入 Engineer 修复 | **条件通过** | 需 QA 先输出根因分析报告（预计 0.5 人日），TechLead 二次确认后 Engineer 执行 |
|
||
| 整体是否可进入实现段 | **通过** | D-01/D-02/D-03 可并行启动；D-04 按"QA 分析 → TechLead 确认 → Engineer 修复"串行执行 |
|
||
|
||
---
|
||
|
||
## 7. 最短执行顺序建议
|
||
|
||
1. **并行启动**：
|
||
- Engineer A：D-01（删除 CallbackTarget）
|
||
- Engineer B：D-03（最终一致性注释）+ D-02（claim 机制）
|
||
2. **QA 同步启动**：D-04 根因分析（跑测试、抓日志、确认根因）
|
||
3. **QA 产出根因报告后**：TechLead 10 分钟内确认方向
|
||
4. **Engineer B 接着做**：D-04 测试修复
|
||
5. **全部完成后**：QA 跑 V-02 稳定性复跑（`go test ./... -count=5`）作为阶段门禁
|