test(cmd/gc): add requireNoLeakedDoltAfter helper for dolt-leak detection#1795
Open
quad341 wants to merge 5 commits intogastownhall:mainfrom
Open
test(cmd/gc): add requireNoLeakedDoltAfter helper for dolt-leak detection#1795quad341 wants to merge 5 commits intogastownhall:mainfrom
quad341 wants to merge 5 commits intogastownhall:mainfrom
Conversation
Child 1 of 4 from ga-ich5z (parent ga-3ski1) — establish the canonical
L1 path helper and the strict-on-parse, lenient-on-read accessor for
project identity.
- ProjectIdentityPath(scopeRoot) centralizes "<scopeRoot>/.beads/identity.toml"
so doctor, error messages, and reconcile name the file consistently.
- ReadProjectIdentity loads the L1 file with BurntSushi/toml strict
parsing (md.Undecoded() rejects unknown top-level keys and unknown
keys inside [project]). Returns ("", false, nil) for: missing file,
missing [project] section, empty/whitespace-only project.id —
callers must treat all four as "L1 not yet populated" (legacy rig).
Returns wrapped error for malformed TOML, unknown keys, and IO
failures (errors.Is(err, os.ErrNotExist) does not match — IsNotExist
is the file-absent short-circuit, not an error path).
Tests cover designer §5 subtests A1-A12 + C1-C2: 12 read behaviors
(missing, valid, trim, empty/whitespace id, missing section, malformed
TOML, extra top-level/project keys, permission error, comments, UTF-8
round-trip) plus 2 path behaviors (joins scope root, canonicalizes
trailing slash). Permission-error subtest is skipped under root.
Out of scope here, per ga-401s4 acceptance:
- WriteProjectIdentity (child 2, ga-7o5mb)
- TestNoExternalIdentityWriters lint guard (child 3, ga-b4gug)
- gitignore !.beads/identity.toml entries (child 4, ga-4tg3j — closed)
Adds WriteProjectIdentity to the contract package — slice 2 of 4 of the identity contract foundation (parent ga-ich5z). Built on top of slice 1 (ga-401s4) which introduced the read path. WriteProjectIdentity validates the id (rejects empty, whitespace-only, and ids containing newline/CR/quote/backslash), creates the .beads/ directory if missing, and writes via fsys.WriteFileIfChangedAtomic so the operation is atomic and idempotent. Tests B1-B10 from designer §5 cover body byte-equality, idempotent writes (no inode churn), overwrite-changes-inode, all four input rejections, dotbeads dir creation, round-trip through ReadProject- Identity, and concurrent same-id safety. Existing A and C subtests unchanged. -count=1 -race passes; go vet and golangci-lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
TestNoExternalIdentityWriters walks the repo and asserts no .go file outside internal/beads/contract/ contains the literal "identity.toml". This is subtest D1 from designer §5 (ga-ich5z) and the third of four slices implementing the identity-contract foundation. V1 implementation is a coarse byte-level grep — the bead spec explicitly defers AST-level filtering. False positives are absorbed through identityWriterAllowlist (currently empty by design). Skip list covers vendor/.git/node_modules/_test.go (per spec) plus repo-local defensive entries (.gc/.claude/worktrees) for performance. Verified the test catches violations with a temporary probe file outside the contract package; all current production code is clean because every existing mention lives in identity.go (the writer) or identity_test.go (test fixtures, exempt as a _test.go file). Runs in ~30ms with race detector clean. golangci-lint 0 issues.
Adds a dolt-leak detector that snapshots live `dolt sql-server` PIDs via /proc at registration time and re-scans in t.Cleanup. Any new PID present at cleanup is reported via t.Errorf with PID and argv so the spawn site is traceable. The helper degrades to a no-op on hosts without /proc. Wired into the three leaf city.toml writers in city_runtime_test.go: writeCityRuntimeConfigNamed, writeCityRuntimeConfigWithShutdownTimeout, writeCityRuntimeConfigWithIncludes. Every test that writes a city runtime config now gets the leak check automatically. Pairs with the existing clearInheritedBeadsEnv: that helper prevents the leak by stripping inherited GC_BEADS=bd before the city.toml is written; this helper catches any leak that slips through. Also fixes a gap in writeCityRuntimeConfigWithShutdownTimeout, which was missing the clearInheritedBeadsEnv call that its three siblings already had. Verified against TestCityRuntimeReload* (the original ga-de27g regression target): all 13 tests pass with no false-positive leak reports. Helper reuses the existing discoverDoltProcesses() reaper discovery — no duplicated /proc-walking logic. Out of scope: - Regression test that deliberately spawns a leaked dolt to verify the helper t.Errorf's. Filed as a needs-tests bead for the validator. - Race in TestCityRuntimeManualReloadPanicAfterReloadKeepsReloadReplyAndClears (order_dispatch.go:81 logDispatchError concurrent bytes.Buffer writes); pre-existing, unrelated to this change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Verdict: PASS at HEAD 79b3e64 on quad341:builder/ga-b4gug-1. Test-only addition; not semantically dependent on identity chain but stacked on PR gastownhall#1794 because the builder authored both. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
status/needs-triage
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
randy-release-manager
Bot
added
kind/chore
julianknutsen
added
status/needs-review-auto
6 tasks
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
3 participants
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.
What this changes
Adds a test-only
requireNoLeakedDoltAfterhelper that snapshots livedolt sql-serverPIDs (via/proc) at registration time and re-scansin
t.Cleanup. Any new PID present at cleanup is reported viat.Errorfwith PID + argv, surfacing the dolt-leak class of bugs thathave repeatedly caused OOMs and identity-drift incidents in the past
(see parent
ga-de27g, relatedga-3ski1).The helper is wired into
writeCityRuntimeConfig*writers incity_runtime_test.go, so all 14 callers — including the 13TestCityRuntimeReload*tests — get auto-coverage without the beadhaving to enumerate them.
Review notes
cmd/gc/path_helpers_test.go(+59) andcmd/gc/city_runtime_test.go(+4) for the writer integration.cmd/gc— no public API.clearInheritedBeadsEnvgap filled inwriteCityRuntimeConfigWithShutdownTimeout; this was uncoveredduring integration.
follow-up
ga-vux42u(validator's responsibility per role boundary— builders don't author new tests against tests they wrote).
Test plan
go build ./...,go vet ./...,golangci-lint run ./cmd/gc/clean.go test ./cmd/gc/ -run '^TestCityRuntimeReload' -count=1PASS (13/13 in 2.96s).release-gates/ga-ytbdp8-dolt-leak-helper-gate.md🤖 Deployed by actual-factory (stacked on #1794)
Need help on this PR? Tag
@codesmithwith what you need.