fix(agent): take the workspace-map walk off the turn's critical path (M1) - #218
Merged
Merged
Conversation
Every turn assembled its context by walking the session working directory synchronously, on the tokio worker driving the turn, with no timeout, no cancellation point and no log line. The 2026-09-10 test drive of main measured what that costs: with the default working directory ($HOME on macOS) the walk blocked in std::fs::read_dir -> __opendir2 under ~/Library/Group Containers and the turn never reached the provider at all. 100% of /usr/bin/sample samples sat in that one stack across four captures; the daemon idled at 0.0% CPU while the composer said "Thinking" for 8m32s. Stop could not end it, and each wedged turn leaked its worker for the life of the process. Four rules, each pinned by a test: - No filesystem syscall on the async path. The walk, the root's stat and canonicalize all move into the blocking task. The cache is therefore keyed on the path as given and the fast path is TTL-only; the mtime early-invalidation it replaces cost a stat on the turn's path. - The walk runs under spawn_blocking with a finite budget (CONTEXT_WORKSPACE_SUMMARY_BUDGET_MS, 1500ms). On lapse the turn proceeds without a map, one WARN names the directory and the lever, and the negative is cached for the TTL. The budget cannot be set to zero. - The wait is tied to the turn's cancellation token, threaded down from reply_internal through assemble_turn_context, inject_moim and collect_moim, so Stop is honoured immediately. - Single-flight per root. A blocked std::fs call cannot be cancelled, so a wedged thread is lost; the marker bounds that to one thread per root rather than one per turn, and later turns are served the last good map. A home directory is also no longer walked at all, along with filesystem roots and the OS/cloud-sync trees (~/Library, ~/AppData, Group Containers, CloudStorage, Mobile Documents, FileProvider). Those trees are pruned mid-walk too, and the walk no longer crosses a mount point. Pre-existing: neither file changed in f350cdf..c539264.
Two things a self-review caught in the previous commit. The walk task cached its result unconditionally, so a configured CONTEXT_WORKSPACE_SUMMARY_TTL_SECS of 0 — which has always meant "cache nothing" — would still leave an entry a later turn could be served off the over-budget path. Both the task's write and the over-budget stamp now respect it, so the setting means one thing again. clear_cache had no caller left: the mtime test that used it is gone, and the new tests deliberately key off a root of their own instead, because clearing process-global state from one test breaks whichever test is running beside it. Said so where the helper used to be.
…dirs AppData went onto the opaque-roots list as the obvious Windows counterpart to ~/Library. It is not one. std::env::temp_dir() on Windows is %USERPROFILE%\AppData\Local\Temp, so refusing that subtree refuses every scratch workspace on one platform and not the other two — and empties the walk in every build_summary test that runs there, since they all build their fixture with tempfile::tempdir(). Nothing under AppData blocks the way a File Provider mount does. The component rules (Group Containers, CloudStorage, Mobile Documents, FileProvider) are what capture the measured hazard, and they match wherever they appear, Windows included. Caught by reading, not by CI, and the reason is worth recording: the Windows runner's TEMP is C:\Users\RUNNER~1\... against a USERPROFILE of C:\Users\runneradmin, so the 8.3 short name means the prefix test might not have matched and the job could have passed on luck. Adds the regression test both ways: skip_reason on this platform's real temp dir, and a Windows-shaped scratch path under an injected home, so the rule is asserted where the platform cannot hide it. Reverting the list and simulating the Windows layout fails it with `left: Some(OpaqueTree) right: None`.
This was referenced Sep 10, 2026
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
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.
Fixes finding M1 (HIGH) of the 2026-09-10 test drive of merged
main. The renderer half of the same failure, M2, is #214.Pre-existing.
git log f350cdfc..c5392640 --name-only | grep -iE 'moim|workspace_summary'returns nothing — neither file changed in that range. This is reported at HIGH because it made an ordinary one-line turn unusable on the merged tree and would do the same onmaintoday.The finding
Every turn assembled its context by walking the session working directory synchronously, on the tokio worker driving the turn, with no timeout, no cancellation point and no log line. With the default working directory —
$HOMEon macOS — the walk blocked instd::fs::read_dir→__opendir2under~/Library/Group Containers(Dropbox, iCloud, GlobalProtect, Office app-group containers) and the turn never reached the provider at all.100% of
/usr/bin/samplesamples sat in that one stack across four captures in two sessions. The daemon idled at 0.0% CPU while the composer said "Thinking" for 8m32s; zero provider sockets ever opened.Stopcould not end it, and each wedged turn leaked its worker for the life of the process — the thread and its three descriptors were still held twenty minutes later, after the chat had been deleted.Two hypotheses were tested and falsified by the test drive and are not re-litigated here: the tree is not slow (a shell
findover the same directory returns 11,849 entries in 3.4 s) and it is not a code-signing problem (re-signing with the Developer ID changed nothing).The fix
Four rules, each pinned by a test that hangs or fails without it.
statandcanonicalizeall move into the blocking task, where a wedge costs a pool thread instead of the turn. Two consequences that look like oversights and are not: the cache is keyed on the working directory as given rather than canonicalised, and the fast path is TTL-only.spawn_blockingwith a finite budget (CONTEXT_WORKSPACE_SUMMARY_BUDGET_MS, default 1500 ms). On lapse the turn proceeds without a map, one WARN names the directory and both levers, and the negative is cached for the TTL so the next turn pays nothing. The budget cannot be configured to zero — that would be a way to reinstate the hang.reply_internalthroughassemble_turn_context,inject_moimandcollect_moim, soStopis honoured immediately. The walk itself cannot be cancelled; the wait for it can.std::fscall has no cancellation point, so a wedged thread is lost. The marker bounds that loss to one thread per root instead of one per turn, and later turns are served the last good map. A walk that never returns therefore never releases its marker, deliberately.A home directory is no longer walked at all, along with filesystem roots and the OS/cloud-sync trees (
~/Library,~/AppData,Group Containers,CloudStorage,Mobile Documents,FileProvider). Those trees are pruned mid-walk too, and the walk no longer crosses a mount point. TheLibrary/AppDatarules are matched home-relative, so a project directory namedLibrary/stays walkable.The alternative — defaulting
CONTEXT_WORKSPACE_SUMMARYoff for$HOME— was considered and rejected in the doc: it would leave the setting readingtruewhile behaving asfalse, and would say nothing about~/Library/CloudStorage, which is not the home directory and wedges just as hard.Behaviour change worth flagging
The cache's mtime early-invalidation is gone. It cost a
statof the root on the turn's async path, which is the class of call this PR exists to remove. The cache is now purely TTL-based, so a new top-level file surfaces in the map withinCONTEXT_WORKSPACE_SUMMARY_TTL_SECS(30 s) rather than immediately. The map already advertises itself as "slightly stale".Fail-before evidence
The behavioural tests inject the walk as a closure rather than trying to conjure a stalling directory, because one cannot be created from a test: a FIFO does not stall a walk (
walkdironlylstats it), and an unresponsive mount is not something a unit test may arrange. Each experiment below reverts one rule in the committed source and runs the tests against it.Rule 2 + 3 reverted (
let outcome = Wait::Finished(handle.await.ok().flatten())— the pre-fix unbounded, uncancellable await). Both tests hang; the run was killed at 300 s:Rule 4 reverted (single-flight guard deleted):
Pruning reverted (
filter_entry+same_file_systemdeleted):Gates
cargo test -p biorouter --lib -- workspace_summary moim agents::agentcargo test -p biorouter --libcargo fmt --check./scripts/clippy-lint.shtoo_many_linesbaseline and the TLS checkBIOROUTER_DISABLE_KEYRING=trueon every run.Runtime verification
A sandboxed daemon built from this branch (
BIOROUTER_PATH_ROOT, seeded config,versa_azure/gpt-5.5-2026-04-24), withCONTEXT_WORKSPACE_SUMMARYunset — the failing configuration.Reply with the single word ready.in a chat with working dir/Users/wguready. Before: never (8m32s and counting, four measurements)sample <daemon pid>after the turns__opendir2/read_dir/ignore::walk/workspace_summarylsofon the daemonGroup Containersworkspace map: no file map for this chat because the working directory is the home directory, which is not a workspace, working_dir: /Users/wguZZUNIQUEMARKER.txt, lib.rs, with no tool callPOST /agent/cancelon a running turn{"cancelled":true,"settled":true}, HTTP 200, in 0.05 s — the daemon-side half of M2The 2.4–3.9 s range matches the test drive's own A/B measurement with the lever forced on (2.7–3.8 s), so the fix costs nothing that the workaround did not.
Not verified here: M2's composer behaviour in the real GUI, which needs both branches and a packaged app. #214 covers it with vitest.
Docs
New living doc
docs/agent-loop/workspace-map.md, indexed indocs/agent-loop/README.md, perdocs/contributing/documentation-style.md. It records the four rules, the "a home directory is not a workspace" decision and the alternative that was rejected, the settings table, what a turn without a map looks like, and why the tests inject the walk.Second commit — self-review pass
Two things a read-back of the first commit caught, neither found by a test:
CONTEXT_WORKSPACE_SUMMARY_TTL_SECSof0— which has always meant "cache nothing" — would still leave an entry the over-budget path could serve. Both the task's write and the over-budget stamp respect it now, so the setting means one thing again.clear_cachehad no caller left. The mtime test that used it is gone, and the new tests key off a root of their own instead, because clearing process-global state from one test breaks whichever test runs beside it. The reason is recorded where the helper used to be, so it does not come back.Gates re-run after it:
cargo fmt --checkclean,cargo test -p biorouter --lib3781 passed,./scripts/clippy-lint.shexit 0, andcargo test -p biorouter --lib -- workspace_summary moim agents::agent186 passed.Third commit — a Windows hazard caught by reading, not by CI
AppDatawent onto the opaque-roots list as the obvious Windows counterpart to~/Library. It is not one:std::env::temp_dir()on Windows is%USERPROFILE%\AppData\Local\Temp, so refusing that subtree refuses every scratch workspace on Windows — and empties the walk in everybuild_summarytest that runs there, since they all build their fixture withtempfile::tempdir().Nothing under
AppDatablocks the way a File Provider mount does. The component rules —Group Containers,CloudStorage,Mobile Documents,FileProvider— are what capture the measured hazard, and they match wherever they appear, Windows included.Worth recording why CI might not have caught it: the Windows runner's
TEMPisC:\Users\RUNNER~1\…against aUSERPROFILEofC:\Users\runneradmin, so the 8.3 short name means the prefix test might not have matched andtest (windows-latest)could have passed on luck.The regression test asserts the rule twice —
skip_reasonagainst this platform's real temp directory, and a Windows-shaped scratch path against an injected home, so the assertion holds on macOS and Linux too. Reverting the list and simulating the Windows layout fails it:Gates after it:
cargo fmt --checkclean,cargo test -p biorouter --lib3782 passed,./scripts/clippy-lint.shexit 0.🤖 Generated with Claude Code