opencode/picoclaw
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: update in-memory config atomically on PUT to prevent stale reads

Browse source 
handlePutConfig was only saving to disk without updating s.cfg in memory,
so GET /api/config returned stale values until the server restarted.
Now holds a write lock and saves to file first, then copies to memory
only on success, ensuring concurrent GETs block until both are done.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Kohei 2026-02-27 23:29:30 +09:00
parent e03671184a
commit df3035aa14
3 changed files with 43 additions and 12 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

23
pkg/config/config.go
View file

@ -310,7 +310,16 @@ func LoadConfig(path string) (*Config, error) {
func SaveConfig(path string, cfg *Config) error {
cfg.mu.RLock()
defer cfg.mu.RUnlock()
return saveConfigLocked(path, cfg)
}
// SaveConfigLocked writes cfg to path without acquiring cfg's mutex.
// Use this when the caller manages synchronization externally.
func SaveConfigLocked(path string, cfg *Config) error {
return saveConfigLocked(path, cfg)
}
func saveConfigLocked(path string, cfg *Config) error {
data, err := json.MarshalIndent(cfg, "", " ")
if err != nil {
return err
@ -324,9 +333,23 @@ func SaveConfig(path string, cfg *Config) error {
return os.WriteFile(path, data, 0600)
}
func (c *Config) Lock() { c.mu.Lock() }
func (c *Config) Unlock() { c.mu.Unlock() }
func (c *Config) RLock() { c.mu.RLock() }
func (c *Config) RUnlock() { c.mu.RUnlock() }
// CopyFrom copies all configuration fields from src into c.
// The caller must hold c's write lock. src's mutex is not acquired.
func (c *Config) CopyFrom(src *Config) {
c.LLM = src.LLM
c.Agents = src.Agents
c.Channels = src.Channels
c.Gateway = src.Gateway
c.Tools = src.Tools
c.Heartbeat = src.Heartbeat
c.RateLimits = src.RateLimits
}
func (c *Config) WorkspacePath() string {
c.mu.RLock()
defer c.mu.RUnlock()

10
pkg/gateway/handlers.go
View file

@ -67,7 +67,15 @@ func (s *Server) handlePutConfig(w http.ResponseWriter, r *http.Request) {
return
}
if err := config.SaveConfig(s.configPath, &newCfg); err != nil {
// Hold write lock: save file first, then update memory only on success.
// Any concurrent GET /api/config will block until both are done.
s.cfg.Lock()
err = config.SaveConfigLocked(s.configPath, &newCfg)
if err == nil {
s.cfg.CopyFrom(&newCfg)
}
s.cfg.Unlock()
if err != nil {
writeJSONError(w, http.StatusInternalServerError, "failed to save config: "+err.Error())
return
}

22
pkg/gateway/server_test.go
View file

@ -1303,9 +1303,9 @@ func TestHandlePutConfig_MultipleSectionsSimultaneous(t *testing.T) {
}
}
// --- PUT /api/config: in-memory config not updated ---
// --- PUT /api/config: in-memory config updated immediately ---
func TestHandlePutConfig_InMemoryConfigUnchanged(t *testing.T) {
func TestHandlePutConfig_InMemoryConfigUpdated(t *testing.T) {
cfg := config.DefaultConfig()
cfg.LLM.Model = "original"
@ -1323,9 +1323,9 @@ func TestHandlePutConfig_InMemoryConfigUnchanged(t *testing.T) {
t.Fatalf("status = %d, want 200", rr.Code)
}
// In-memory config should still have old value (disk is updated, restart reloads)
if s.cfg.LLM.Model != "original" {
t.Errorf("in-memory model = %q, want %q (should remain unchanged until restart)", s.cfg.LLM.Model, "original")
// In-memory config should be updated immediately
if s.cfg.LLM.Model != "updated" {
t.Errorf("in-memory model = %q, want %q", s.cfg.LLM.Model, "updated")
}
// Disk should have new value
@ -1645,7 +1645,7 @@ func TestHandlePutConfig_MCPMapUpdate(t *testing.T) {
}
}
func TestHandlePutConfig_MCPMapInMemoryUnchanged(t *testing.T) {
func TestHandlePutConfig_MCPMapInMemoryUpdated(t *testing.T) {
cfg := config.DefaultConfig()
cfg.Tools.MCP = map[string]config.MCPServerConfig{
"original": {Command: "orig", Enabled: true},
@ -1665,12 +1665,12 @@ func TestHandlePutConfig_MCPMapInMemoryUnchanged(t *testing.T) {
t.Fatalf("status = %d, want 200", rr.Code)
}
// In-memory config should NOT have the added server
if _, found := s.cfg.Tools.MCP["added"]; found {
t.Error("in-memory MCP should not contain 'added' (deep copy should prevent mutation)")
// In-memory config should now contain the added server
if _, found := s.cfg.Tools.MCP["added"]; !found {
t.Error("in-memory MCP should contain 'added' after PUT")
}
if len(s.cfg.Tools.MCP) != 1 {
t.Errorf("in-memory MCP count = %d, want 1", len(s.cfg.Tools.MCP))
if len(s.cfg.Tools.MCP) != 2 {
t.Errorf("in-memory MCP count = %d, want 2", len(s.cfg.Tools.MCP))
}
}