Skip to content

Commit e59d15e

Browse files
committed
feat(storage): GC orphaned tool-approval records on config load (MCP-1002)
Prune tool-approval records whose server is no longer in the configured set. Configured-but-disabled servers are preserved so re-enabling never re-quarantines their previously-approved tools. Hooked into LoadConfiguredServers, guarded against a transient empty config nuking every approval. Scope note: the config.db size problem (investigated under MCP-1002) is NOT free-page bloat — it is unbounded growth of activity_records (~438MB / ~93k rows) and security scan reports (~231MB). That needs a retention policy, handled separately; compaction alone is insufficient and is intentionally not included here.
1 parent df066d4 commit e59d15e

4 files changed

Lines changed: 138 additions & 0 deletions

File tree

internal/runtime/lifecycle.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -715,6 +715,23 @@ func (r *Runtime) LoadConfiguredServers(cfg *config.Config) error {
715715
storedServerMap[storedServer.Name] = storedServer
716716
}
717717

718+
// GC orphaned tool-approval records (MCP-1002): drop approvals whose server
719+
// is no longer configured. Configured-but-disabled servers are preserved so
720+
// a later re-enable doesn't re-quarantine their previously-approved tools.
721+
// Guard against a transient empty config nuking every approval — explicit
722+
// server deletion already cleans up via DeleteServerToolApprovals.
723+
if len(configuredServers) > 0 {
724+
configuredNames := make([]string, 0, len(configuredServers))
725+
for name := range configuredServers {
726+
configuredNames = append(configuredNames, name)
727+
}
728+
if pruned, perr := r.storageManager.PruneOrphanToolApprovals(configuredNames); perr != nil {
729+
r.logger.Warn("Failed to prune orphan tool approvals", zap.Error(perr))
730+
} else if pruned > 0 {
731+
r.logger.Info("Pruned orphan tool-approval records", zap.Int("removed", pruned))
732+
}
733+
}
734+
718735
// Add/remove servers asynchronously to prevent blocking on slow connections
719736
// All server operations now happen in background goroutines with timeouts
720737

internal/storage/bbolt.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,45 @@ func (b *BoltDB) DeleteServerToolApprovals(serverName string) error {
431431
})
432432
}
433433

434+
// PruneToolApprovalsNotIn deletes tool-approval records whose ServerName is not
435+
// present in keep, returning the number removed. This GCs orphaned approvals
436+
// for servers that left the config without an explicit delete (e.g. the config
437+
// file was hand-edited, or an old migration). Records for configured servers —
438+
// including disabled ones — are kept so re-enabling a server doesn't re-trigger
439+
// quarantine of its previously-approved tools (MCP-1002).
440+
func (b *BoltDB) PruneToolApprovalsNotIn(keep map[string]bool) (int, error) {
441+
removed := 0
442+
err := b.db.Update(func(tx *bbolt.Tx) error {
443+
bucket := tx.Bucket([]byte(ToolApprovalBucket))
444+
if bucket == nil {
445+
return nil
446+
}
447+
var keysToDelete [][]byte
448+
scanErr := bucket.ForEach(func(k, v []byte) error {
449+
var record ToolApprovalRecord
450+
if err := json.Unmarshal(v, &record); err != nil {
451+
// Unparseable record: leave it alone rather than risk dropping data.
452+
return nil
453+
}
454+
if !keep[record.ServerName] {
455+
keysToDelete = append(keysToDelete, append([]byte(nil), k...))
456+
}
457+
return nil
458+
})
459+
if scanErr != nil {
460+
return scanErr
461+
}
462+
for _, key := range keysToDelete {
463+
if err := bucket.Delete(key); err != nil {
464+
return err
465+
}
466+
removed++
467+
}
468+
return nil
469+
})
470+
return removed, err
471+
}
472+
434473
// Generic operations
435474

436475
// Backup creates a backup of the database
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
package storage
2+
3+
import (
4+
"testing"
5+
"time"
6+
7+
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
9+
)
10+
11+
func mkApproval(server, tool string) *ToolApprovalRecord {
12+
return &ToolApprovalRecord{
13+
ServerName: server,
14+
ToolName: tool,
15+
Status: ToolApprovalStatusApproved,
16+
ApprovedAt: time.Now().UTC(),
17+
}
18+
}
19+
20+
func TestPruneOrphanToolApprovals(t *testing.T) {
21+
manager, cleanup := setupTestStorageForToolApproval(t)
22+
defer cleanup()
23+
24+
require.NoError(t, manager.SaveToolApproval(mkApproval("github", "create_issue")))
25+
require.NoError(t, manager.SaveToolApproval(mkApproval("github", "list_repos")))
26+
require.NoError(t, manager.SaveToolApproval(mkApproval("disabled-but-configured", "x")))
27+
require.NoError(t, manager.SaveToolApproval(mkApproval("old-server", "do_thing")))
28+
require.NoError(t, manager.SaveToolApproval(mkApproval("removed", "y")))
29+
30+
// "disabled-but-configured" stays in config (just disabled) → must be kept.
31+
removed, err := manager.PruneOrphanToolApprovals([]string{"github", "disabled-but-configured"})
32+
require.NoError(t, err)
33+
assert.Equal(t, 2, removed, "only old-server + removed are orphans")
34+
35+
// configured (incl. disabled) survive
36+
for _, tc := range []struct{ s, tool string }{{"github", "create_issue"}, {"github", "list_repos"}, {"disabled-but-configured", "x"}} {
37+
rec, err := manager.GetToolApproval(tc.s, tc.tool)
38+
require.NoError(t, err)
39+
assert.NotNilf(t, rec, "%s:%s should be kept", tc.s, tc.tool)
40+
}
41+
// orphans gone (GetToolApproval returns a not-found error for missing records)
42+
for _, tc := range []struct{ s, tool string }{{"old-server", "do_thing"}, {"removed", "y"}} {
43+
_, err := manager.GetToolApproval(tc.s, tc.tool)
44+
assert.Errorf(t, err, "%s:%s should be pruned (not found)", tc.s, tc.tool)
45+
}
46+
}
47+
48+
func TestPruneOrphanToolApprovals_EmptyConfigPrunesAll(t *testing.T) {
49+
manager, cleanup := setupTestStorageForToolApproval(t)
50+
defer cleanup()
51+
require.NoError(t, manager.SaveToolApproval(mkApproval("a", "t1")))
52+
require.NoError(t, manager.SaveToolApproval(mkApproval("b", "t2")))
53+
54+
removed, err := manager.PruneOrphanToolApprovals(nil)
55+
require.NoError(t, err)
56+
assert.Equal(t, 2, removed)
57+
}
58+
59+
func TestPruneOrphanToolApprovals_NoOrphansNoChange(t *testing.T) {
60+
manager, cleanup := setupTestStorageForToolApproval(t)
61+
defer cleanup()
62+
require.NoError(t, manager.SaveToolApproval(mkApproval("a", "t1")))
63+
64+
removed, err := manager.PruneOrphanToolApprovals([]string{"a", "b", "c"})
65+
require.NoError(t, err)
66+
assert.Equal(t, 0, removed)
67+
}

internal/storage/manager.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -453,6 +453,21 @@ func (m *Manager) DeleteServerToolApprovals(serverName string) error {
453453
return m.db.DeleteServerToolApprovals(serverName)
454454
}
455455

456+
// PruneOrphanToolApprovals removes tool-approval records for servers that are
457+
// no longer in the configured set, returning the number removed. Configured
458+
// servers (even disabled ones) are preserved so re-enabling never re-quarantines
459+
// previously-approved tools (MCP-1002).
460+
func (m *Manager) PruneOrphanToolApprovals(configuredServers []string) (int, error) {
461+
m.mu.Lock()
462+
defer m.mu.Unlock()
463+
464+
keep := make(map[string]bool, len(configuredServers))
465+
for _, name := range configuredServers {
466+
keep[name] = true
467+
}
468+
return m.db.PruneToolApprovalsNotIn(keep)
469+
}
470+
456471
// Security Scanner methods (Spec 039)
457472

458473
// SaveScanner saves a scanner plugin record

0 commit comments

Comments
 (0)