Architectural cleanup: extract httpapi, fix sync supervisor races, installer --version#47
Merged
Merged
Conversation
extractTitle's filename fallback was unreachable — every caller passed "" and applied the filename fallback itself (index.go:92, :173). Drop the parameter, simplify the contract to 'frontmatter title -> first # heading -> ""'. While here, tighten parsePage: - remove unused doc and fmEnd locals - drop the now-redundant second return value from stripFrontmatter - inline the single use of links into the struct literal
The line printed 'MCP endpoint: http://<addr>/mcp' but no /mcp route is registered on the HTTP server — MCP is stdio-only. Drop the misleading output.
The file has always been a plain REST client for /api/* endpoints. The 'mcp' name was a confusing inheritance from earlier design — the web UI never speaks MCP (only AI agents do, via stdio). Rename the module and tighten its doc comment to make the boundary explicit. All imports updated; no behavior change.
Adds two small methods so an HTTP supervisor can update a running sync.Manager without bouncing the whole process: - Reload(newCfg): swap the in-memory config and rebuild targets under the existing lock. Background loop keeps running. - Interval(): expose the captured tick interval so callers can detect changes that require a full restart (the ticker is created at Start time and can't be retuned in place). Both methods are read-mostly and respect the existing m.mu discipline.
Move every HTTP handler out of cmd/mind-map/main.go (~270 LOC of inline closures over runHTTPServer's locals) into a dedicated internal/httpapi package. main.go is now just wiring: open the wiki, load the config, construct an httpapi.Server, listen. The new package owns: - All /api/* handlers as methods on *Server (testable on their own). - A small sync supervisor (startSyncLocked / stopSyncLocked / applyConfigLocked) that reconciles the running sync.Manager with the current config: enabled<->disabled transitions, interval changes, and in-place mapping reloads. - The /api/settings PUT handler now hot-applies config changes through the supervisor. Previously, settings only took effect after the user hit /api/restart and the binary self-exec'd. /api/restart still works (it's the only way to apply changes that need a full process restart), but the common case no longer requires it. - The static file handler (embedded WebFS or 'not built' placeholder). Adds 7 black-box tests covering create/get/list/search, update/delete, validation errors, settings round-trip, sync supervisor reaction to config changes, and the not-built placeholder. These exercise the full mux + middleware stack against a real *wiki.Wiki on a t.TempDir(). No behavior change to the wire protocol — the OpenAPI surface is byte- for-byte identical apart from /api/settings now applying changes live.
The supervisor introduced in the previous commit had two real concurrency bugs that could surface under /api/restart or SIGINT: 1. Sync managers were started with context.Background() instead of a server-scoped context. An HTTP-level shutdown (StopCh closed) did not propagate into in-flight git operations — fetch/push could keep running past the point where the server thought it was gone, and on /api/restart the new process could touch the same shadow clone directory concurrently with the old one. 2. sync.Manager.Start writes m.cancel/m.done without internal locking. The supervisor called Start under s.mu and Stop under s.mu, but in the (real) race where applyConfig stashes a new mgr and releases the lock before its Start runs, shutdown could grab the same mgr and call Stop on it concurrently with applyConfig's Start. That's a data race on the sync.Manager fields per the sync package's contract. Fixes: - Introduce rootCtx/rootCancel on the Server. All sync manager contexts are derived from rootCtx so HTTP shutdown propagates into git. - Add actionMu, a separate mutex held strictly around Start/Stop calls. applyConfig and shutdown both acquire it for the blocking work, so the two can never race on the same manager. - Add shuttingDown flag set under s.mu so applyConfig short-circuits if a settings PUT lands during teardown — the config still hits disk (for the next start) but no manager goroutine is spawned. - runAction re-checks shuttingDown under s.mu before Start, avoiding a noisy log + wasted MkdirAll in the race window where shutdown wins the actionMu after applyConfig released s.mu. Also: - writeJSON now logs at Warn (not Debug) when JSON encoding fails. A persistent encode failure points at a real bug; Debug was too quiet. - 'settings saved and applied' log message simplified to 'settings saved' since applyConfig may legitimately skip the apply during shutdown. Tests: - New TestShutdownStopsSync exercises the StopCh -> shutdown -> idempotent re-close path. - New TestSettingsChangeDuringShutdown verifies a PUT racing with shutdown produces no orphan sync goroutine; the WARN log fires; the status endpoint reports no manager. - All tests pass under go test -race (the race detector caught earlier iterations of this fix; the current design is clean).
Both install scripts now accept an explicit version, useful for testing prereleases without making them latest: curl ... | bash -s -- --version v0.49 MINDMAP_VERSION=v0.49 curl ... | bash irm ... | iex # with $env:MINDMAP_VERSION = 'v0.49' set first Resolution precedence: CLI flag/param > env var > GitHub 'latest' API. PowerShell specifics: - Add a -Version parameter (PowerShell-native, used in non-piped form). - When the script self-elevates via Start-Process, propagate the version pin through MINDMAP_VERSION so the elevated session sees it. Param binding doesn't survive 'irm | iex'. - Document the env var workaround in the .EXAMPLE block of the help header for the piped form. No change to the default behavior — both scripts still install the latest release if no version is specified.
…inary
GitHub's /releases/latest/download/<asset> path is a redirect that only
points at the latest release. When a user pins --version v0.49, swapping
'latest' for the tag in the redirect path produces a 404 — versioned
assets live at /releases/download/<tag>/<asset> instead. The previous
commit got the binary URL right (it was already tag-based) but left two
'latest'-tied fetches:
- install.ps1's self-relaunch (admin elevation) re-downloads install.ps1.
If a version was pinned, the elevated session was still grabbing the
latest install.ps1. The propagated MINDMAP_VERSION made the binary
correct, but the script logic might not have matched.
- Both scripts fetched SKILL.md from raw.githubusercontent.com/.../main/.
SKILL.md describes the binary's tool surface; pinning v0.49 while
installing main's SKILL.md is a real mismatch for users testing
prereleases or rolling back.
Both fetches now use the same git ref / tag as the binary. Tags work as
git refs on raw.githubusercontent.com, so SKILL_URL just substitutes
${VERSION}. install.ps1's elevation path picks the correct URL shape
based on whether $Version is set.
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
1 participant
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.
Follow-up to the architecture review of the codebase. Eight focused commits, each independently reviewable.
Summary
refactor(wiki): drop dead extractTitle filename paramfix(service): remove stale '/mcp' endpoint advert in 'service start'refactor(webui): rename mcp.ts -> api.tsfeat(sync): add Manager.Reload + Interval for hot-reloadrefactor(http): extract handlers into internal/httpapi + hot-reload syncfix(http): make sync supervisor shutdown-safe; tighten encode loggingfeat(installer): support pinning a specific release versionfix(installer): use pinned tag for all release assets, not just the binaryHighlights
internal/httpapiextraction (commits 4-6)runHTTPServerincmd/mind-map/main.gowas ~270 LOC of inline closures over local state, none of which was unit-testable on its own. Extracted into a dedicatedinternal/httpapipackage with aServerstruct, 7 black-box tests covering the full mux + middleware stack against a real*wiki.Wiki.Hot-reload sync (commit 5)
PUT /api/settingspreviously was a no-op until the user hit/api/restartand the binary self-exec'd. Now the supervisor reconciles enable/disable, interval changes, and in-place mapping reloads live./api/restartstill works as an escape hatch.Shutdown-safety bug fix (commit 6)
The naive supervisor introduced two real concurrency holes:
Sync used
context.Background(). An HTTP-level shutdown didn't propagate into in-flight git operations. On/api/restart, the old process could still be mid-git-op when the new process started — both touching the same shadow clone directory. Real risk of corruption.sync.Manager.Startis not thread-safe. It writesm.cancel/m.donewithout locking. In the narrow window whereapplyConfigstashed a new manager and releaseds.mubeforeStartran,shutdown()could grab the same manager and callStopconcurrently withStart. Race detector territory.Fixed by:
rootCtx/rootCancelowned by the server; all sync contexts derive from itactionMumutex held strictly aroundStart/Stopcalls so they can never raceshuttingDownflag so a settings PUT during teardown writes config (preserves intent for next start) but refuses to spawn a fresh sync goroutineTestShutdownStopsSyncandTestSettingsChangeDuringShutdowntests, all green undergo test -raceInstaller
--version(commits 7-8)Both scripts now accept a version pin for testing prereleases without making them latest:
Commit 8 fixes a subtle URL bug: GitHub's
/releases/latest/download/<asset>is a redirect, not a path you can substitute a tag into. Versioned assets live at/releases/download/<tag>/<asset>. The binary URL was already correct; the PowerShell self-elevation re-fetch and both scripts'SKILL.mdfetches were not. Now everything fetched by the installer matches the binary's version.Verification
All run inside the project's devcontainer:
go vet ./...cleango test ./...— every package greengo test -race ./...— clean (the race detector caught earlier iterations of the supervisor fix)webuibuilds (npm run build)--helpworksbash -n install.shsyntax-clean; logic walkthrough validates flag/env/precedencepwshin devcontainer)Behavior changes worth noting on review
PUT /api/settingsnow applies sync config live instead of being a no-op until/api/restart. Disabling, enabling, swapping remotes, and interval changes all propagate without restart.mind-map service startno longer prints theMCP endpoint: .../mcpline — there's no/mcproute, so it was misleading.PUT /api/settingslog line changed from"settings saved and applied"to"settings saved"(it may legitimately skip the apply during shutdown).writeJSONencode errors now log at Warn (was Debug) — a persistent encode failure usually points at a real bug.Everything else is pure refactor — same routes, same JSON shapes, same exit codes.