Skip to content

fix(db): make the compaction lock token unique independent of the clock - #1217

Merged
Dumbris merged 2 commits into
mainfrom
claude/kind-hugle-6fa6d4
Sep 8, 2026
Merged

Dumbris merged 2 commits into
mainfrom
claude/kind-hugle-6fa6d4

Conversation

@Dumbris

@Dumbris Dumbris commented Sep 6, 2026

Copy link
Copy Markdown
Member

Problem

TestCompactLockReleaseDoesNotRemoveSomeoneElsesLock fails on the windows-latest Build Binaries job of PR Build, blocking every PR. It is pre-existing and unrelated to any branch: introduced by f95fc14 (#1214), and #1215 — which touches only .github/RELEASE_NOTICE.md — failed with the byte-identical message (run 33946755580, job 101259930963).

Root cause

acquireCompactLock stamped the lock file with

token := fmt.Sprintf("pid=%d\nstarted=%s\n", os.Getpid(), time.Now().UTC().Format(time.RFC3339Nano))

and release compares string(current) != token to decide whether the lock is still ours.

On Linux/macOS time.Now() has nanosecond resolution, so two acquisitions in one process always differ. Windows' system clock is coarse (~0.5–15 ms); the test acquires A, removes the file, and acquires B back-to-back with no work between them, so both land in the same tick. Same PID + same timestamp = identical token, A's release believes B's lock is its own, and deletes it. The test running in 0.00s matches.

This is a real bug, not just a flaky test: the token is the only thing standing between a stale-lock cleanup and two concurrent compactions on the same BBolt database, and on Windows it can genuinely collide.

Fix

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 untouched.

Test

TestCompactLockTokenIsUniqueWithinOneClockTick acquires, releases, re-acquires, and compares the two lock files with the clock-derived started= line stripped — so it reproduces what Windows sees and bites on a nanosecond clock too, instead of passing vacuously everywhere but Windows.

Verified failing before the fix:

db_cmd_test.go:578: two acquisitions produced the same clock-independent token "pid=45729";
on a coarse clock A's release would delete B's lock

and passing after, alongside the original test. go test ./cmd/mcpproxy/ green, -race green, GOOS=windows go test -c builds, golangci-lint run --config .github/.golangci.yml ./cmd/mcpproxy/... reports 0 issues.

🤖 Generated with Claude Code

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>
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 6, 2026 •

Copy link
Copy Markdown

Deploying mcpproxy-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9feb4ec
Status: ✅  Deploy successful!
Preview URL: https://b819c6b4.mcpproxy-docs.pages.dev
Branch Preview URL: https://claude-kind-hugle-6fa6d4.mcpproxy-docs.pages.dev

View logs

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cmd/mcpproxy/db_cmd.go 42.85% 3 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

📦 Build Artifacts

Workflow Run: View Run
Branch: claude/kind-hugle-6fa6d4

Available Artifacts

  • archive-darwin-amd64 (29 MB)
  • archive-darwin-arm64 (26 MB)
  • archive-linux-amd64 (17 MB)
  • archive-linux-arm64 (16 MB)
  • archive-windows-amd64 (29 MB)
  • archive-windows-arm64 (26 MB)
  • frontend-dist-pr (0 MB)
  • installer-dmg-darwin-amd64 (23 MB)
  • installer-dmg-darwin-arm64 (21 MB)

How to Download

Option 1: GitHub Web UI (easiest)

  1. Go to the workflow run page linked above
  2. Scroll to the bottom "Artifacts" section
  3. Click on the artifact you want to download

Option 2: GitHub CLI

gh run download 34095478154 --repo smart-mcp-proxy/mcpproxy-go

Note: Artifacts expire in 14 days.

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>
@Dumbris

Dumbris commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

Pushed 9feb4ec, porting the test seam from #1221 (opened as a duplicate of this PR, now closed).

The fix itself is unchanged. What this adds is that TestCompactLockReleaseDoesNotRemoveSomeoneElsesLock — the test that exposed the bug — was still 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.

compactLockNow is now a seam, frozen in both lock tests. A frozen clock is the Windows condition (coarse granularity, two acquisitions back-to-back), so the collision is exercised on every platform rather than by luck. That also lets TestCompactLockTokenIsUniqueWithinOneClockTick compare whole tokens instead of stripping the started= line to guess what Windows would collide on.

Mutation-checked on macOS — with the nonce reverted, both tests fail, the pre-existing one with the exact windows-latest message:

--- FAIL: TestCompactLockReleaseDoesNotRemoveSomeoneElsesLock
    A's release removed B's lock, so a third compaction could start: stat …/config.db.compact.lock: no such file or directory
--- FAIL: TestCompactLockTokenIsUniqueWithinOneClockTick
    two acquisitions produced the same token "pid=47741\nstarted=2026-09-05T12:00:00Z\n"

Also verified: go test -count=1 ./cmd/mcpproxy/ green, -race green, GOOS=windows go build ./... and go test -c build, golangci-lint v2 with .github/.golangci.yml → 0 issues.

@Dumbris
Dumbris merged commit c93f794 into main Sep 8, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants