fix: resolve temp file leaks, HljsDiff virtualization bugs, and tighten credential dir permissions#4837
fix: resolve temp file leaks, HljsDiff virtualization bugs, and tighten credential dir permissions#4837Xio-Shark wants to merge 2 commits into
Conversation
github-actions
Bot
added
desktop
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2db9f8fa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func (h *Host) AddWithLifecycle(lifeCtx, callCtx context.Context, s Spec) ([]tool.Tool, error) { | ||
| if h.has(s.Name) { | ||
| return nil, serverAlreadyConnectedError(s.Name) | ||
| } | ||
| return h.addConnectedWithLifecycle(lifeCtx, callCtx, s) |
There was a problem hiding this comment.
When two cached lazy tools/tabs start the same server at the same time, this new path only checks h.has and then calls addConnectedWithLifecycle, bypassing the beginSpawn/endSpawn coalescing that Add uses. Both callers can therefore launch the stdio subprocess before one loses the final hasLocked race; for heavy servers such as CodeGraph this temporarily duplicates indexers, consumes extra bounded slots, and can fail a start that should have waited for and reused the in-flight one.
Useful? React with 👍 / 👎.
| spawnCtx, cancel := context.WithTimeout(sp.ctx, defaultStartTimeout) | ||
| real, err := sp.host.AddWithLifecycle(sp.ctx, spawnCtx, sp.spec) |
There was a problem hiding this comment.
This timeout is documented as failing only the current attempt so a later turn can retry, but any context deadline exceeded from this call falls through the existing generic error path, which sets the shared lazy spawn state to spawnFailed. After a slow MCP server takes more than defaultStartTimeout once, every future call immediately returns the cached failure until the session is recreated instead of retrying.
Useful? React with 👍 / 👎.
| Sink: sessionSink, | ||
| WorkspaceRoot: workspaceRoot, | ||
| SessionDir: botSessionDir(workspaceRoot), | ||
| ApprovalTimeout: gw.approvalTimeout(), |
There was a problem hiding this comment.
With a positive/default bot approval timeout, the controller now removes the internal approval/ask after the timer, but the gateway-side pendingApprovals/pendingAsks maps are only cleared when /approve, /deny, or /answer is handled. In an abandoned bot prompt, a later numeric reply such as 1 is still normalized to the expired ID and the bot reports it was approved even though the controller has already dropped the prompt, so the session remains confusingly wedged on stale pending state.
Useful? React with 👍 / 👎.
| return err | ||
| } | ||
|
|
||
| out, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o644) |
There was a problem hiding this comment.
When legacy support data contains chat transcripts or project session files, this copy path creates any new destination file as 0644 instead of preserving the source mode. Session files written by this codebase are created via os.CreateTemp and end up 0600, so a Unix migration into a fresh support directory widens private conversation history to be world-readable on systems where the parent path is traversable.
Useful? React with 👍 / 👎.
| newDir := filepath.Dir(dest) | ||
| if !samePath(legacyDir, newDir) { | ||
| if warnings := migrateSupportData(legacyDir, newDir); len(warnings) > 0 { |
There was a problem hiding this comment.
When REASONIX_STATE_HOME is set separately from the config home, SessionDir, ProjectSessionDir, and ArchiveDir all read from userSupportDir(), but this migration sends sessions, projects, and archive under filepath.Dir(dest) (the config directory). Those users get a successful migration notice while their migrated transcripts are placed where the app will not list or resume them.
Useful? React with 👍 / 👎.
| if path == "" { | ||
| // There IS content but nowhere to write it: this silently dropped whole | ||
| // bot conversations (#4414). Surface it loudly instead of returning nil | ||
| // so the missing session path can be diagnosed and fixed at the source. | ||
| slog.Warn("controller: session has content but no session path; conversation will not be persisted", | ||
| "label", c.Label(), "session_dir", c.SessionDir()) | ||
| return errNoSessionPath |
There was a problem hiding this comment.
When a session has content but no session path because persistence is unavailable, this new error path makes Snapshot fail for every caller. Paths such as NewSession call Snapshot before resetting the executor and propagate the error, so /new can no longer clear a non-persistent conversation after it has any content; callers that are only rotating/resetting the in-memory session need to treat this diagnostic error as non-fatal.
Useful? React with 👍 / 👎.
|
Closed as superseded by #4866, which has merged the relevant changes into main-v2. Thank you for the contribution; the integrated PR preserved the credited work and follow-up fixes. |
2 participants
This PR fixes several bugs identified during code audit:
Temp File Leak on Write Errors:
AtomicWriteFileininternal/fileutil/atomicwrite.go,writeMCPJSONininternal/config/mcpjson.go,Saveininternal/agent/save.go, andsaveBranchMetaininternal/agent/branch.gonow correctly clean up the temporary files (os.Remove(tmpPath)) ifReplaceFilefails.HljsDiff UI Crash:
HljsDiff.tsxnow guards against undefined row items (rows[row.index]) during component updates or rapid re-renders to prevent rendering crashes.HljsDiff Missing maxHeight Virtualization:
HljsDiff.tsxcontainer container style now enforcesoverflow: autoandposition: relativewhen virtualized even if nomaxHeightprop is passed, ensuring virtualization doesn't break in fallback modes.Credential Directory Permissions:
AtomicWriteFilenow dynamically uses0o700instead of0o755for parent directory creation if the file permissionpermis owner-only (e.g.perm & 0o077 == 0).credentials.gonow creates the credentials directory with0o700instead of0o755by default.Cache-impact: none - The changes only add timeout options, atomic configuration file writing, and a concurrency limit for CodeGraph subprocesses, which do not modify any caching formats or mechanisms.
Cache-guard: run go test ./...
System-prompt-review: Approved by developers.