Validate runtime update controls before persisting - #514
Conversation
|
PR Title: Validate runtime update controls before persisting Commit: 本次变更将 supervisor.go 中 UpdateSecret 与 UpdateConfig 针对 on-demand 运行时模式的拒绝检查（rejectOnDemandRuntimeControl）从"实例变更之后"提前到"变更之前"。旧实现中，对 on-demand 服务传 restart=true 的更新请求会先持久化新的 secret/config（UpdateConfig 还会先写磁盘 config.json），随后才被拒绝并返回 ErrUnsupportedRuntimeControl，形成"报错但状态已被修改"的部分更新。新实现确保被拒绝的请求不产生任何实例变更，并新增 runtime_update_test.go 验证 store 中实例未被修改。该检查为只读（重新 GetService），仅作用于 restart=true 路径，对非 on-demand 服务与 restart=false 的普通更新行为不变，修复方向正确。唯一不足是新增测试仅断言了 store 中的 ConfigJSON/SecretJSON 未变，未覆盖 UpdateConfig 的磁盘 config.json 写入通道，存在回归保护缺口。 |
![monkeyscan[bot]](https://avatars.githubusercontent.com/u/231811742?s=60&v=4){kind=small}
| } | ||
| if string(got.ConfigJSON) != string(originalConfig) || string(got.SecretJSON) != string(originalSecret) { | ||
| t.Fatalf("rejected runtime control mutated instance: %+v", got) | ||
| } |
There was a problem hiding this comment.
新增测试未验证被拒绝的 UpdateConfig 不会写入磁盘 config.json
本次修复的目标是保证运行时控制被拒绝时实例不发生任何变更。UpdateConfig 存在两条变更通道：磁盘上的 config.json（通过 writeInstanceConfig 写入实例工作目录）与 store 中的 ConfigJSON/ConfigSHA256。新测试仅断言 store 中 ConfigJSON 与 SecretJSON 未变，未断言磁盘 config.json 未被写入。旧代码中 writeInstanceConfig 在拒绝检查之前执行，被拒绝的请求会先污染磁盘文件；新代码虽然把拒绝检查提前到 writeInstanceConfig 之前，但测试没有锁住这一行为。若将来有人把拒绝检查重新移到 writeInstanceConfig 之后、UpsertInstance 之前，本测试仍会通过，而磁盘 config.json 会被静默改写，导致实例下次冷启动使用错误配置，回归风险未被该测试覆盖。
Problem code:
Changed code at internal/supervisor/runtime_update_test.go:35-41
Recommendation:
在测试断言中补充对磁盘 config.json 的检查：读取 sup.InstanceWorkdir("instance")/config.json，若文件存在则其内容必须仍等于 originalConfig（不存在也可接受，说明 writeInstanceConfig 未被调用）。这样与 store 断言共同覆盖两条变更通道。
Suggested diff:
diff --git a/internal/supervisor/runtime_update_test.go b/internal/supervisor/runtime_update_test.go
--- a/internal/supervisor/runtime_update_test.go
+++ b/internal/supervisor/runtime_update_test.go
@@ -3,6 +3,8 @@
import (
"context"
"errors"
+ "os"
+ "path/filepath"
"testing"
"octobus/internal/domain"
@@ -36,5 +38,11 @@
if string(got.ConfigJSON) != string(originalConfig) || string(got.SecretJSON) != string(originalSecret) {
t.Fatalf("rejected runtime control mutated instance: %+v", got)
}
+ diskConfig, err := os.ReadFile(filepath.Join(sup.InstanceWorkdir("instance"), "config.json"))
+ if err == nil {
+ if string(diskConfig) != string(originalConfig) {
+ t.Fatalf("rejected runtime control wrote disk config: %s", diskConfig)
+ }
+ } else if !os.IsNotExist(err) {
+ t.Fatalf("read disk config: %v", err)
+ }
}
|
PR Title: Validate runtime update controls before persisting Commit: 本次改动仅涉及 internal/supervisor/supervisor.go:
|
| } | ||
| if restoreErr != nil { | ||
| return domain.Instance{}, fmt.Errorf("persist config: %w; restore previous config: %v", err, restoreErr) | ||
| } |
There was a problem hiding this comment.
UpdateConfig 失败回滚在并发更新同一实例时可能把磁盘配置覆盖回陈旧值，造成与 DB 不一致
新增回滚逻辑在 UpsertInstance 失败时把磁盘 config.json 恢复为函数开头读到的 previousConfig（或直接删除）。但 UpdateConfig 全程没有 per-instance 锁或版本校验（supervisor 内 s.mu 只保护 procs/generations，配置更新并不持锁）。当同一 instance 的两个 UpdateConfig 并发执行、其中一个 UpsertInstance 失败时：成功方已把 config.json 写成新配置并提交 DB；失败方回滚时依据自己早先读到的陈旧 previousConfig，会把 config.json 覆盖回旧内容（或 os.Remove 掉成功方刚写入的文件），导致磁盘 config.json 与 DB ConfigJSON 不一致。在下次 startWithAttempt 用 DB 重写 config.json 之前，以 config.json 为 --config 启动的子进程可能按旧配置运行。startWithAttempt 每次启动前都会从 DB 重写 config.json，说明 DB 才是权威、磁盘只是启动快照；基于磁盘旧值回滚而不是基于 DB 值，在并发/历史不一致场景会把磁盘拉向错误方向。
Problem code:
Changed code at internal/supervisor/supervisor.go:200-211
Recommendation:
对同一 instance 的配置更新加串行化（例如 per-instance mutex，或基于 generation/版本号的乐观校验），使回滚只可能撤销自己这次的写入，而不会覆盖其他并发请求已成功提交的配置；或在回滚前重新读取并确认 config.json 仍等于本次写入内容后再执行恢复。
| if err := tmp.Close(); err != nil { | ||
| return err | ||
| } | ||
| return os.Rename(tmpPath, filepath.Join(workdir, "config.json")) |
There was a problem hiding this comment.
writeInstanceConfig 原子写缺少 rename 后的父目录 fsync，断电/崩溃后配置可能回退
新的原子写实现已对临时文件调用 tmp.Sync() 并在 rename 后返回，但未对父目录（workdir）执行 fsync。rename 对应的目录项是否落盘不受保证：系统断电/崩溃后目录项可能未持久化，config.json 可能回退为旧内容或缺失，而调用方（UpdateConfig/CreateInstance）随后已把新配置写入 DB，从而出现磁盘 config.json 与 DB ConfigJSON 不一致。该改动特意加了 tmp.Sync()，说明作者关注持久性，但目录 fsync 缺失使持久性保证不完整。
Problem code:
Changed code at internal/supervisor/supervisor.go:654-675
Recommendation:
在 os.Rename 成功后打开 workdir 目录并调用 Sync()，以持久化目录项、确保原子替换在崩溃恢复后仍然生效；若文件系统不支持目录 Sync（如某些平台），可忽略对应错误并注释说明。
|
Follow-up hardening after review:
Verification: |
1 participant
问题
UpdateConfig和UpdateSecret在完成文件/数据库写入后，才检查 On-demand 服务是否支持 restart。影响
客户端收到失败响应时，配置或 Secret 其实已经被修改，造成 API 语义与持久化状态不一致；自动化重试可能进一步覆盖数据并造成审计误判。
修复内容
验证
go test ./internal/supervisor -run TestRuntimeControlRejectionPrecedesInstanceMutation通过。