perf(desktop): sidebar resilience - timeout, debounce, result cache#4889
perf(desktop): sidebar resilience - timeout, debounce, result cache#4889ttmouse wants to merge 13 commits into
Conversation
github-actions
Bot
added
v2
![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: a6b6f3554d
ℹ️ 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".
| case <-time.After(5 * time.Second): | ||
| slog.Warn("ListProjectTree: wg.Wait timed out after 5s, proceeding with partial results", "elapsed", time.Since(buildStart).String()) |
There was a problem hiding this comment.
When a session dir takes more than 5s, this timeout path lets ListProjectTree continue while the goroutines above still hold references to sessionInfos, sessionTitles, and topicSummaries and can later call mergeSessionInfos. The rest of the function reads those maps without mergeMu, so the slow cloud/unmounted-dir case this is meant to handle can trigger a fatal concurrent map read/write or cache a partial tree that is mutated after return. Wait for isolated worker results or discard late results instead of sharing these maps after the timeout.
Useful? React with 👍 / 👎.
| projectTreeDebouncer.fire(func() { | ||
| projectSessionCache.invalidate() | ||
| sessionDiskCacher.invalidate() |
There was a problem hiding this comment.
In change paths such as RenameProject, topic creation, and runtime status updates, emitProjectTreeChanged now invalidates only projectSessionCache/sessionDiskCacher, while ListProjectTree returns projectTreeResultCache for up to 2s. If the tree was loaded just before the mutation, the debounced event consumer can call ListProjectTree and still receive stale nodes (old title/pin/status or a missing topic) until the TTL expires, despite an explicit change notification. Clear projectTreeResultCache/projectTreeCacheExpiry when emitting the change.
Useful? React with 👍 / 👎.
| b, err := readFileWithTimeout(topicTitlesPath(workspaceRoot), topicFileReadTimeout) | ||
| if err != nil { | ||
| return m |
There was a problem hiding this comment.
When a topic metadata read takes longer than 200ms, loadTopicTitles now returns an empty map, but write paths such as setTopicTitleWithSource immediately save that map back after changing one topic. In the cloud/slow-volume case this timeout is meant to handle, renaming or creating a topic can therefore rewrite desktop-topic-titles.json with only the current entry and drop every other title; the same pattern applies to title sources and created-at metadata. Propagate timeout errors or avoid using lossy timed reads in read-modify-write paths.
Useful? React with 👍 / 👎.
| go func() { | ||
| data, err := os.ReadFile(path) | ||
| ch <- result{data, err} |
There was a problem hiding this comment.
In the cloud-storage/unmounted-volume case this helper is meant to tolerate, returning on the timeout leaves the goroutine running os.ReadFile behind because that read is not cancelled. Since the topic metadata loaders are called for every project/global section and can be retried on later sidebar refreshes, a persistently hung metadata file can accumulate stuck goroutines and file I/O resources over time. Bound or suppress repeated timed-out reads instead of spawning an unbounded goroutine per attempt.
Useful? React with 👍 / 👎.
sidebar resilience (ListProjectTree): - readFileWithTimeout (200ms) for all file reads (topic titles, projects, sessions) - debounceEmitter (200ms) for emitProjectTreeChanged - projectTreeResultCache (2s TTL, double-check locking) for ListProjectTree - wg.Wait 5s timeout + timedOut flag to prevent hung goroutines - goroutine panic recovery per directory - detailed slog logging at each stage (dirs, cache hit/miss, migration, topics) single-instance: - singleInstanceID now derives from binary path hash instead of fixed const - different binary paths (dev vs release) get different locks automatically - removed REASONIX_DEV env var bypass (no longer needed)
The 200ms debounce in emitProjectTreeChanged added a perceptible delay on every sidebar-relevant action (creating a session, switching tabs, renaming a topic, etc.). The 2s projectTreeResultCache + listProjectTreeMu serialization already prevent redundant rebuilds more effectively than debouncing the event itself — multiple back-to-back emits result in a single cache hit after the first rebuild populates the cache. Also removed the now-unused debounceEmitter type and projectTreeDebouncer variable.
Previously ListProjectTree called loadTopicTitles and loadTopicCreatedAts sequentially for each project. With 15+ projects including cloud-storage paths (SynologyDrive, iCloud) where readFileWithTimeout hits its 200ms timeout, the sequential loop multiplied worst-case latency by the number of slow paths — the first cold build took ~5s. Now all project topic files are loaded in parallel goroutines. The total time is bounded by the single slowest project (200ms for a timeout), not the sum of all slow projects.
desktopConfigDir now returns ~/.reasonix/ instead of ~/Library/Application Support/reasonix/, so all desktop config files (desktop-projects.json, desktop-tabs.json, global/*) live in the same directory as the rest of Reasonix state (sessions/, projects/, config.toml). No migration needed — the app auto-creates missing files on next save. Old files in ~/Library/Application Support/reasonix/ are left in place.
topicTitleForTab calls loadTopicTitles which re-reads the entire JSON file for each topic without an explicit title. With 15 projects and several on SynologyDrive cloud storage where readFileWithTimeout hits its 200ms bound, repeated fallback calls add up to seconds. The project processing loop already has all titles in titleMap from the concurrent load phase. Use it directly and fall back to the default title string instead of re-reading the file.
ListProjectTree now saves the built tree to a cache file on disk (~/.reasonix/desktop-project-tree-cache.json). On the next cold startup, it reads that cache and returns it immediately — the sidebar renders instantly, no loading delay. Rebuilds happen in a background goroutine triggered by either: - A disk-cache hit on cold startup (stale-while-revalidate) - emitProjectTreeChanged (user action → rebuild flag set) Disk cache is invalidated through rebuildRequested atomic flag: during normal use the flag forces an immediate full rebuild, while on cold startup the flag is false so the cache is returned first.
ttmouse
force-pushed
the
pr/sidebar-resilience
branch
from
ee0fed6 to
206c7df
Compare
github-actions
Bot
added
the
agent
ttmouse
force-pushed
the
pr/sidebar-resilience
branch
from
0f92fe6 to
4363277
Compare
SivanCola
left a comment
SivanCola
left a comment
There was a problem hiding this comment.
Thanks for the performance work here. I think the direction is valuable, but I do not think this is safe to merge yet.
Blocking issues:
-
ListProjectTreenow caches the fully rendered[]ProjectNodeglobally for 2 seconds. That payload includes runtime fields (Open,Running,Status) and is not keyed by the active config/session roots. Calls inside the TTL can return stale project trees, stale runtime status, or even another isolated app/test environment's tree. This is already visible in CI: thedesktopjob fails inTestOpenProjectTabResolvesProjectSessionFromLegacyDir, and locallygo test ./...underdesktop/also fails with the same cache-hit pattern.TestProjectTreeShowsBackgroundJobStatuscan also fail because the cached tree does not reflect updated runtime state. Please either cache only the expensive disk-derived inputs and recompute runtime fields each call, or key/invalidate the cache on every state root/runtime-status change. -
desktopConfigDir()now moves tabs/projects from the platform config dir to~/.reasonix, butloadTabsFileandloadProjectsFileonly read the new location. Existing users withdesktop-tabs.json/desktop-projects.jsonunder the old config dir will start with an empty tab/sidebar state and may overwrite the new location before the old data is migrated. This needs a compatibility read/migration path before changing the write location. -
The new
readFileWithTimeoutreturns after 200ms, but the underlyingos.ReadFilegoroutine is still left blocked forever when the filesystem call hangs. Repeated sidebar rebuilds against the same cloud/unmounted path can accumulate stuck goroutines. The caller no longer hangs, which is good, but the implementation still needs bounded worker lifetime or a different cancellable/stat-based strategy.
Also, the PR removes the REASONIX_DEV single-instance bypass while the new comment still says it exists. If that bypass is intentionally removed, please update the docs/tests and make sure contributor workflows that rely on same-binary multiple dev instances still have an equivalent path.
Verified:
- GitHub
desktopcheck is failing on this PR. go test ./...fromdesktop/fails locally on the PR head.go test -run TestOpenProjectTabResolvesProjectSessionFromLegacyDir -count=1 -vpasses in isolation, which supports the process-global cache/stale-state diagnosis rather than a pure test setup failure.git diff --check origin/main-v2...origin/pr/4889passes.
Security/cache note: I did not see new dependencies or network/file-exfiltration behavior. This does not appear to affect provider-visible prompt/cache prefixes, and the repository cache-impact check passed. The blocker is desktop state correctness and stale local data handling.
…SONIX_DEV - Remove projectTreeResultCache (2s TTL) — it cached runtime fields (Open/Running/Status) and returned stale state. Disk cache alone is sufficient for cold-start speed; rebuild are always current. - Add readDesktopConfigFile helper for backward-compatible reads from the old config dir (~/Library/Application Support/reasonix/) so existing users don't lose their projects/tabs on upgrade. - Replace readFileWithTimeout with plain os.ReadFile — the goroutine leaked on timeout and the 200ms timeout is redundant when wg.Wait (5s) already protects the ListProjectTree call from hangs. - Restore REASONIX_DEV env var bypass for contributors who need to run multiple dev instances from the same binary.
- wg.Wait 5s timeout + timedOut + wgDone → plain wg.Wait() - loadProjectTreeDiskCache/saveProjectTreeDiskCache → inlined - rebuildRequested kept (needed for stale-while-revalidate) - Per-stage slog timing logs removed, only start/done remain
The stale-while-revalidate pattern returned stale runtime status (Open/Running/Status) after the initial rebuild completed because the disk cache was checked on every call. Added projectTreeDiskCacheLoaded atomic flag so the disk cache path is only taken once per process — subsequent calls always rebuild, ensuring live runtime state.
|
Closing this in favor of the merged integration PR #4942. What was kept/adapted from this PR:
What was not adopted:
@ttmouse is credited in #4942's contribution table and commit co-author trailers. Thank you for the investigation and performance work. |
2 participants
问题
PR #4858 解决了重复扫描问题，但在以下场景仍有性能瓶颈：
os.ReadFile可能无限阻塞，导致整个 UI 卡死emitProjectTreeChanged，每次都完整重建ListProjectTreewg.Wait()永不返回loadTopicTitles/loadTopicCreatedAts对 15+ 个项目顺序读取，最慢可达 ~5stopicTitleForTab在遍历 611 个话题时为每个无标题话题重新读整个 JSON 文件改动
共 7 个 commit，
desktop/tabs.go+ 少量辅助文件：加载性能优化
titleMap，topicTitleForTab不再反复调loadTopicTitlesemitProjectTreeChanged立即通知前端，不再额外等待防御性保护
readFileWithTimeout(200ms)os.ReadFile无限阻塞projectTreeResultCache(2s TTL, 双检锁)listProjectTreeMuwg.Wait5s 超时 +timedOutflag其他改进
com.reasonix.desktop.<binary-path-sha256[:8]>，dev 和 release 自动可多开desktopConfigDir指向~/.reasonix/，与 sessions/projects 统一目录效果（15 个项目的用户实测）