Repository navigation
fix(storage): bound the tool-call history, stop erasing configured zeros, add db compact - #1214
Merged
Merged
Conversation
… zeros Closes the other half of the 940MB config.db in #1173. #1176 — the per-server server_<id>_tool_calls buckets held ~432MB of that 940MB and were unbounded in every direction: RecordToolCall did CreateBucketIfNotExists + Put with no size cap, no count cap, no age prune and no eviction, and ToolCallRecord.Response is a bare interface{} holding the whole upstream CallToolResult. Nothing ever deleted a bucket either — CleanupStaleServerData exists but has no caller — so a server removed from the config left its entire call history behind forever. Bound it at the single write seam, so every producer is covered rather than the ones a caller remembered to cap: - a per-record cap on the marshalled response AND arguments. Over the cap the field becomes {truncated, original_bytes, preview, note} and the record carries response_truncated + response_bytes. It stays a JSON OBJECT on purpose: contracts.ToolCallRecord.Response is a documented payload on four OAS endpoints and httpapi.maskToolCallRecord walks it as structured JSON, so retyping it to a string would be a breaking API change. - eviction of the oldest records past a per-server count, in the same transaction as the write, so the bucket can never exceed the cap even briefly. The walk goes backwards from the newest key and stops at the cap, so its cost is bounded by the cap rather than the bucket size; Stats().KeyN is deliberately unused because inside the write transaction it does not count the Put that just happened. - bucket deletion on server removal, and an orphan sweep beside the existing PruneOrphanToolApprovals. The sweep takes server NAMES, as its sibling does, but these buckets are keyed by server IDENTITY — the SHA-256 GenerateServerID computes from a server's attributes — so it resolves one to the other through server_identities. A live run proved this is not theoretical: the first version compared bucket keys against names, which marks every live history an orphan, and it deleted a real bucket on restart. The unit tests missed it because they passed the name AS the ServerID; they now register real identities and assert the id is not the name. An empty identity store aborts the sweep — that state means "not populated yet", not "every server is gone". #1175 — three config fields carried omitempty on a numeric type while 0 was a documented meaningful value, so SaveConfig's json.MarshalIndent deleted an operator's explicit 0 and the field reverted to its default on the next write: max_result_size_chars ("Set to 0 to disable"), activity_max_size_mb ("0=disabled") and observability.tracing.sample_rate. All three become tri-state pointers with Effective…() readers. Dropping omitempty from the int is not equivalent — that materialises an absent key as an explicit 0 and turns the feature off for everyone who never set it. The defaults live in the readers, not only in DefaultConfig(), because two REST write paths decode a client document into a ZERO-valued config.Config (handlePatchConfig's `var merged config.Config`, UnmaskLiveConfigDocument's `&config.Config{}`) and never see the constructor. One trap the conversion had to clear: runtime.New gated SetMaxResponseSize behind `if … || cfg.ActivityMaxSizeMB >= 0`, a clause that is trivially true for a plain int and was the only reason the 64KB activity cap from #1174 ran at all. A naive pointer rewrite would have silently disabled it for every config that omits the key, and the existing default-path test would not have caught it because it constructs ActivityService directly. The per-record caps are now applied unconditionally, outside the retention guard where they never belonged. A reflection guard test walks the whole Config tree and fails on any numeric field with omitempty that is not on an allowlist with a stated reason. It found 14 more; each was checked against its doc comment or read site and every one genuinely treats 0 and absent as the same value. mcpproxy db compact / db stats — the caps bound future growth, but BBolt never returns freed pages to the OS, so an already-large config.db stays large. Offline compaction via bbolt.Compact (already pinned, no new dependency) with the atomic temp-file+rename recipe from config/loader.go, and db stats to say whether it is worth running. A locked database fails fast with exit 3 and a remediation line rather than retrying. The output is explicit that the default run frees no disk: the retained config.db.bak holds the original inode, so "reclaimed" describes the file and the filesystem gets nothing back until the backup is deleted. That matters most to the user who ran out of disk in the first place. Verified on a real instance (isolated port/data-dir/config, stdio upstream): a 300KB response stored as a 2.2KB truncated object with response_bytes=300065; a 300KB argument likewise; 8 calls against a cap of 5 leaving exactly 5 records; history surviving a restart (3 before, 3 after — 5 before / 0 after with the identity bug); and compaction taking 4.0MB to 2.0MB with every record, the upstream server and its tools intact afterwards. Closes #1175, closes #1176 Related #1173
Independent reviews by opencode gpt-5.6-sol and codex gpt-5.6-sol over the
three areas of the previous commit. Every finding is fixed, triaged with a
reason, or pinned by a test that fails without the fix.
REPLAY (found by codex; the worst of the set). Capping stored arguments made
POST /api/v1/tool-calls/{id}/replay unsafe: with no argument override it
re-dispatches the STORED arguments, which for a capped record are the
{truncated, original_bytes, preview} placeholder. A replayed call_tool_write or
call_tool_destructive would have reached a real system with nonsense. Records
now carry arguments_truncated and replay refuses them with a message telling
the caller to supply arguments explicitly.
BACKUP COULD DESTROY THE LIVE DATABASE (codex). copyFile — the fallback when
os.Link fails — did os.WriteFile straight onto the destination. That path can
BE a hard link to config.db (an earlier run's .bak, or a concurrent
compaction's), so the "backup" step could truncate the database it was backing
up. It now writes a temp file and renames, which replaces the directory entry
and leaves any other link to the old inode intact.
THE SWAP WAS UNGUARDED (both). bbolt's lock is released before the rename — the
file has to be closed for a rename to work on Windows — leaving the destructive
half of a minutes-long operation unprotected. A core starting in that window
ends up holding the old, unlinked inode and every write it makes disappears.
Two fixes: a whole-command lock file (O_CREATE|O_EXCL, atomic everywhere, no
build-tagged syscall) so two compactions cannot overlap at all, and a
pre-replace check that the source is unchanged and unheld. The check opens
READ-ONLY — a read-write open makes bbolt commit a freelist flush on a database
whose freelist was never persisted, modifying the very file the check exists to
prove untouched.
ORPHAN SWEEP DELETED LIVE HISTORY (opencode). An identity whose JSON would not
unmarshal was skipped while still counting the identity store as populated, so
a live server's hashed bucket became unresolvable and was swept. The bucket key
IS the identity id, so an unreadable record can be spared without reading it.
Such a bucket can then never be reclaimed, which is the right way round —
losing a live server's history is worse than leaking a dead one's — and it is
now logged rather than silent.
TRUNCATION MARKER IGNORED THE CAP (codex). The placeholder carried a fixed 2KB
preview whatever the configured cap was, so tool_call_max_response_size: 1024
produced a "truncated" record of ~2.3KB. The preview now shrinks to fit,
leaving room for the envelope.
TWO ALLOWLIST ENTRIES WERE FALSE (codex). The guard test claimed 0 and absent
agree for activity_retention_days and activity_max_records. They did not: an
omitted key took DefaultConfig's 90 days / 100000 records, while an explicit 0
was ignored by SetRetentionConfig and fell through to that service's own 7 days
/ 10000 — a tenth of the documented retention, which a save then silently
reverted. EffectiveActivityRetentionDays / EffectiveActivityMaxRecords make the
two agree on the documented default. docker_isolation.timeout also differs, but
its only reader renders it in a doctor payload, so it stays allowlisted with
that as the stated reason rather than an untrue equivalence.
GUARD TEST WAS BLIND UNDER -tags server (opencode). Four server-edition fields
are invisible to an untagged reflection walk; all four are normalised by
ServerEditionConfig.Validate and are now allowlisted with that citation. Run
this test under both tags.
Smaller: chmod moved before the destination fsync so the mode is durable with
the data; a failed directory fsync warns instead of being dropped; a .bak that
cannot be removed after a failed rename is reported rather than swallowed; a
partial fallback copy is cleaned up.
Analysed and left alone, with the reasoning recorded in the code: eviction
relies on bytewise key order matching chronological order, which holds because
UnixNano is a fixed 19 digits from 2001 to 2262 — documented so a future key
format change cannot break it silently.
Process notes. opencode's first run on the db command exited 0 with NO verdict
after being refused reads outside its launch directory; bbolt's source was
staged in-repo and it was re-run. Codex could not write anywhere, so its
mutation testing was static; those neuters were run here instead — including
one that exposed a vacuous test of my own, which locked the database too early
and passed on the injected error rather than on the guard.
Verified again on a real instance after every fix: a 200KB response and a 200KB
argument both stored as bounded placeholders with response_truncated,
response_bytes and arguments_truncated set; both records surviving a restart
with no orphan sweep firing.
Related #1175, #1176, #1173
Deploying mcpproxy-docs with
|
| Latest commit: |
6dc2df1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://409cc6b4.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-storage-bounds-compactio.mcpproxy-docs.pages.dev |
…action step The release body is generated from commit messages and never reads CHANGELOG.md, so anything a user must ACT on has to go through .github/RELEASE_NOTICE.md. Two things qualify here. The #1173 storage fixes bound future growth but cannot shrink an existing file — BBolt does not return freed pages to the OS — so a user sitting on a 940MB config.db has to run 'mcpproxy db compact' themselves, with mcpproxy stopped, and then delete the backup that the default run keeps. Without that step they upgrade and see no improvement. And two config keys quietly changed meaning: activity_retention_days: 0 and activity_max_records: 0 used to fall through to internal defaults far smaller than the documented ones (7 days / 10000 records instead of 90 / 100000). That is a fix, but anyone who leaned on 0 to keep the log small needs to say what they meant. Related #1173, #1175, #1176
…k ownership
Second opencode review round, on the fixes from the first.
THE TRUNCATION MARKER COULD EXCEED ITS OWN CAP. previewBytes budgeted the
preview by RAW length against a fixed 512-byte envelope reserve, but the
preview is JSON embedded in JSON: every quote and backslash is escaped a second
time on the way in, so a quote-heavy payload roughly doubles. Measured: a
1024-byte cap produced a 1243-byte record, a 4096-byte cap produced 4308. The
placeholder now halves its preview until the MARSHALLED object fits, and falls
back to a bare {truncated, original_bytes} — about 45 bytes — for caps too
small to hold the note. Estimating the encoded size from the raw size was the
mistake; it now measures.
The existing test passed through all of this because it used strings.Repeat("q")
as filler, which escapes to nothing. It now uses quotes and backslashes and
covers caps from 64 bytes up, and it reproduces the exact 1243/4308 figures
when the fix is reverted.
RELEASING THE COMPACTION LOCK COULD REMOVE SOMEONE ELSE'S. The release closure
deleted the pathname unconditionally. Sequence: someone clears A's
stale-looking lock by hand, B takes a fresh one, A finishes and deletes the
pathname — which is now B's — and a third compaction starts alongside B. The
lock file now carries a pid+timestamp token and release removes it only while
it still contains that token.
THE PRE-REPLACE CHECK NOW RUNS AS LATE AS POSSIBLE, after the backup rather
than before it, so the window between "the source is unheld" and the rename is
microseconds instead of spanning backup creation. It cannot be closed
completely — the file must be closed for a rename to work on Windows, so no
lock can span the rename — and that residual is now stated in the code rather
than implied. Aborting there also cleans up the backup it had just made.
Also recorded rather than fixed: the staleness check compares size and mtime,
not a content hash, because hashing a multi-gigabyte file to guard a
microsecond window would cost more than the operation it protects. An in-place
write preserving both would slip past it — but making one requires an open
handle, which is what the exclusive-open check refuses.
Verified on a real instance: db stats reporting 1.3MB reclaimable, a second
compaction refused by the lock with the remediation message, a real compaction
taking 4.0MB to 2.0MB, the lock file cleaned up afterwards, and every record
still readable through the API.
Related #1176
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…g on Windows The test stat'ed the database BEFORE opening the holder. Opening a bbolt database can write to it — a freelist flush, which is what happens on Windows — so by the time the pre-replace check ran, the file had legitimately changed and the check aborted on its staleness half rather than its lock half. Both halves abort the swap, which is the property that matters, but this case exists to pin the LOCK detection specifically, and it asserted the error type to prove it. Stat after the holder is open, so the file looks unchanged and only the lock check can fire. The Windows failure also cancelled both macOS jobs through the matrix, which is why three builds went red for one cause.
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 33941185690 --repo smart-mcp-proxy/mcpproxy-go
|
Dumbris
added a commit
that referenced
this pull request
Sep 8, 2026
…ck (#1217) * fix(db): make the compaction lock token unique independent of the clock acquireCompactLock stamped its lock file with pid + time.Now(), and release compared that string to decide whether the lock was still ours. On Linux and macOS time.Now() has nanosecond resolution, so two acquisitions in one process always differ. Windows' system clock is coarse (~0.5-15ms): two back-to-back acquisitions land in the same tick and produce a byte-identical token, so A's release deletes B's lock and a third compaction can start on the same BBolt database. This surfaced as TestCompactLockReleaseDoesNotRemoveSomeoneElsesLock failing on every windows-latest "Build Binaries" job since #1214, blocking unrelated PRs (#1215 touched only a markdown file and failed identically), but the collision is real outside the test too — the token is the only thing standing between a stale-lock cleanup and two concurrent compactions. Add a 16-byte crypto/rand nonce to the token. pid and started stay: they are what a human reads out of a stale lock file. The lock protocol and the compaction flow are otherwise untouched. The new test strips the clock-derived line before comparing, so it bites on a nanosecond clock too rather than passing vacuously everywhere but Windows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(db): freeze the clock so the compaction-lock tests bite off Windows TestCompactLockReleaseDoesNotRemoveSomeoneElsesLock is the test that exposed this bug, and it stayed clock-dependent: on a nanosecond-resolution platform the two acquisitions get different timestamps, so it passed for the wrong reason and only failed on windows-latest, intermittently. Add compactLockNow as a seam and freeze it in both lock tests. A frozen clock is precisely the Windows condition — coarse granularity with two acquisitions back-to-back — so the collision is now exercised on every platform rather than by luck. With the nonce reverted, the pre-existing test reproduces the exact windows-latest failure on macOS. That also lets the new uniqueness test compare whole tokens instead of stripping the started= line to guess what Windows would collide on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Dumbris
added a commit
that referenced
this pull request
Sep 9, 2026
Specs 104 (auto routing mode) and 105 (agent-token scope hardening) were written and cross-reviewed on 2026-09-07 but never committed; five merged PRs already cite 105 by name as their acceptance contract. This lands both and records them in roadmap.yaml so the DAG reflects what shipped and what is left. Spec 105 — agent-token scope hardening. The Spec 104 review verified the scoped-token promise against the code one surface at a time and found eight places where a legitimately narrow token could learn about or act on servers outside its grant. Five fix sessions ran from that review and merged on 2026-09-08: #1223 target tier, #1224 tail_log, #1225 set_profile, #1226 read_cache provenance, #1227 prompt owner and deleted-pin enumeration. Each was live-verified against a baseline binary and reviewed to a clean verdict. The epic records those five as done tasks and carries eight todo tasks, one per remaining FR group, taken from the "Follow-ups / Spec 105 gaps" checklists in those PR bodies: FR-005 retrieve_tools scoping, FR-008 direct publication identity, FR-010 refusal shapes, the FR-003/FR-004 selectable- profile predicate for unpinned tokens, FR-002 legacy cache invalidation, FR-007 log-record attribution, FR-009 producer-side identity, and the FR-011/013/014 regression suite. Spec 104 — auto routing mode. Measures the three candidate surfaces per session on the catalog that session will actually see and serves the richest rung under a token budget. It depends on 105: its FR-016 states the invariant those corrections make true. P1 is US1-4, P2 is US5-6. PR evidence sits on the child tasks rather than the epic, because gen-roadmap.py --check-github reads an all-merged epic pr: list as "this epic is done" and the scope-hardening epic is not. Also flips activity-storage-bounds to done: #1174 wired the 64KB write-path cap and #1214 bounded the per-server tool_calls buckets, stopped omitempty erasing configured zeros and added db compact, closing #1175 and #1176. The specs/README index rows are appended for both, though that table has been stale since 043 and does not list 044-103. Validation: gen-roadmap.py --check-github reports 0 errors. The one warning (mcp-2026-upgrade has no pr: link or child tasks) is pre-existing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Description
Closes the other half of the 940MB
config.dbreported in #1173. PR #1174 already capped the activity log's response text; this handles the rest.#1176 — the per-server tool-call history was unbounded in every direction.
RecordToolCalldidCreateBucketIfNotExists+Putwith no size cap, no count cap, no age prune and no eviction, andToolCallRecord.Responseis a bareinterface{}holding the whole upstreamCallToolResult. These buckets held ~432MB of the reporter's 940MB. Nothing ever deleted one either —CleanupStaleServerDataexists but has no caller — so a server removed from the config left its entire call history behind forever.Bounded at the single write seam, so every producer is covered rather than the ones a caller remembered to cap:
{truncated, original_bytes, preview, note}and the record carriesresponse_truncated/response_bytes/arguments_truncated. It stays a JSON object on purpose:contracts.ToolCallRecord.Responseis a documented payload on four OAS endpoints andhttpapi.maskToolCallRecordwalks it as structured JSON, so retyping it to a string would be a breaking API change;PruneOrphanToolApprovals.#1175 — three config fields silently erased an explicitly configured
0.max_result_size_chars("Set to 0 to disable"),activity_max_size_mb("0=disabled") andobservability.tracing.sample_ratecarriedomitemptyon a numeric type, soSaveConfig'sjson.MarshalIndentdeleted the operator's0and the field reverted to its default on the next write. All three become tri-state pointers withEffective…()readers. Droppingomitemptyfrom the int is not equivalent — that materialises an absent key as an explicit0and turns the feature off for everyone who never set it.A reflection guard test walks the whole
Configtree and fails on any numeric field withomitemptythat is not on an allowlist with a stated reason. It found 14 more; each was checked against its doc comment or read site.mcpproxy db compact/db stats— the caps bound future growth, but BBolt never returns freed pages to the OS, so an already-largeconfig.dbstays large. Offline compaction viabbolt.Compact(already pinned; no new dependency) with the atomic temp-file+rename recipe fromconfig/loader.go. The output is explicit that the default run frees no disk: the retainedconfig.db.bakholds the original inode, so "reclaimed" describes the file and the filesystem gets nothing back until the backup is deleted — which matters most to the user who ran out of disk in the first place.Notes for the reviewer
Three defects here were only findable by running the thing, and are the ones most worth a second look:
Identity vs. name keying. The orphan sweep and the delete-server hook were first written against server names, but these buckets are keyed by the SHA-256
GenerateServerIDhash. A live restart deleted real history (5 records → 0). Both paths now resolve throughserver_identities. The unit tests had passed because they used the name as theServerID; they now register real identities and assert the id is not the name.Replay. Capping stored arguments made
POST /api/v1/tool-calls/{id}/replayunsafe — with no argument override it re-dispatches the stored arguments, which for a capped record are the placeholder. A replayedcall_tool_write/destructivewould have reached a real system with nonsense. Records now carryarguments_truncatedand replay refuses them.The compaction swap. bbolt's lock is released before the rename (the file must be closed for a rename to work on Windows), leaving the destructive half of a minutes-long operation unguarded, and the backup's
os.Linkfallback wrote straight onto a path that can be a hard link to the live database. Now: a whole-command lock file, a read-only pre-replace check that the source is unchanged and unheld, and a copy that writes a temp file and renames.One behaviour change worth noting:
activity_retention_daysandactivity_max_recordspreviously diverged between0and absent — an omitted key tookDefaultConfig's 90 days / 100000 records, while an explicit0was ignored bySetRetentionConfigand fell through to that service's own 7 days / 10000. Both now resolve to the documented default.docker_isolation.timeoutalso differs between0and absent and is deliberately left allowlisted: its only reader renders it in adoctorpayload, so nothing observes the difference. Give it a real reader and it must be converted.Testing
Verified on a real instance (isolated port,
--data-dirand--config, stdio upstream — nottest-api-e2e.sh, whose exit trap blanket-kills every running core):response_bytes=300065; a 300KB argument likewise;db statsrefusing while the core is up (exit 3) and reporting reclaimable space when it is not;Automated:
go build ./...,go vet ./...andgo vet -tags server ./...,golangci-lint run --config .github/.golangci.yml(0 issues),-raceon the new storage tests, the fullinternal/storage,internal/config(both tags),internal/runtime,internal/httpapi,internal/contracts,cmd/mcpproxyandinternal/serveredition/...suites.Every new test was verified to fail without its fix. One of them did not, and was rewritten: it locked the database too early and passed on the injected error rather than on the guard under test.
Cross-model review
Reviewed independently by
opencodeandcodex, both on gpt-5.6-sol, over three separate briefs (storage / config / db command). Their high-severity findings barely overlapped — codex found the replay defect and the backup-truncation path, opencode found the sweep deleting history behind an unreadable identity and the guard test being blind under-tags server. Two allowlist entries in the guard test were provably false and were fixed rather than reworded. Every finding is fixed, or triaged with the reason recorded in the code.Closes #1175, closes #1176
Related #1173