Skip to content

fix(agent): take the workspace-map walk off the turn's critical path (M1) - #218

Merged
Broccolito merged 3 commits into
mainfrom
fix/workspace-summary-off-critical-path
Sep 10, 2026
Merged

fix(agent): take the workspace-map walk off the turn's critical path (M1)#218
Broccolito merged 3 commits into
mainfrom
fix/workspace-summary-off-critical-path

Conversation

@Broccolito

@Broccolito Broccolito commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

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 on main today.

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 — $HOME on macOS — the walk blocked in std::fs::read_dir__opendir2 under ~/Library/Group Containers (Dropbox, iCloud, GlobalProtect, Office app-group containers) and the turn never reached the provider at all.

Agent::reply_internal -> assemble_turn_context -> moim::inject_moim
  -> ExtensionManager::collect_moim -> workspace_summary::build_summary
    -> ignore::walk::Walk::next -> std::fs::read_dir -> __opendir2 -> open$NOCANCEL

100% of /usr/bin/sample samples 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. Stop could 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 find over 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.

  1. No filesystem syscall on the async path. The walk, the root's stat and canonicalize all 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.
  2. The walk runs under spawn_blocking with 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.
  3. The wait is tied to the turn's cancellation token, threaded from reply_internal through assemble_turn_context, inject_moim and collect_moim, so Stop is honoured immediately. The walk itself cannot be cancelled; the wait for it can.
  4. Single-flight per root. A blocked std::fs call 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. The Library/AppData rules are matched home-relative, so a project directory named Library/ stays walkable.

The alternative — defaulting CONTEXT_WORKSPACE_SUMMARY off for $HOME — was considered and rejected in the doc: it would leave the setting reading true while behaving as false, 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 stat of 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 within CONTEXT_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 (walkdir only lstats 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:

running 2 tests
test ...::a_cancelled_turn_returns_immediately_and_leaves_the_walk_running has been running for over 60 seconds
test ...::a_stalled_walk_returns_within_the_budget_instead_of_holding_the_turn has been running for over 60 seconds
EXIT=124

Rule 4 reverted (single-flight guard deleted):

panicked: single-flight failed: a second walk of this root was started
assertion `left == right` failed: no walk was started   left: 1  right: 0
assertion `left == right` failed: the last good map is served rather than nothing
  left: None  right: Some("first map")

Pruning reverted (filter_entry + same_file_system deleted):

panicked: nothing under an opaque component is read: Workspace file map (...):
Group Containers/
  group.com.example/
    blocked.txt
keep.txt

Gates

Gate Result
cargo test -p biorouter --lib -- workspace_summary moim agents::agent 34 passed, 0 failed
cargo test -p biorouter --lib 3781 passed, 0 failed, 2 ignored
cargo fmt --check clean
./scripts/clippy-lint.sh exit 0, including the too_many_lines baseline and the TLS check

BIOROUTER_DISABLE_KEYRING=true on every run.

Runtime verification

A sandboxed daemon built from this branch (BIOROUTER_PATH_ROOT, seeded config, versa_azure / gpt-5.5-2026-04-24), with CONTEXT_WORKSPACE_SUMMARY unset — the failing configuration.

Check Result
Reply with the single word ready. in a chat with working dir /Users/wgu 2.39 s, 2.76 s, 3.87 s — model replied ready. Before: never (8m32s and counting, four measurements)
sample <daemon pid> after the turns zero frames matching __opendir2 / read_dir / ignore::walk / workspace_summary
lsof on the daemon zero descriptors under Group Containers
Skip log line fired once, not per turn: workspace map: no file map for this chat because the working directory is the home directory, which is not a workspace, working_dir: /Users/wgu
The map still works where it should a session in a temp project answered "list the file names in the workspace file map you were given" with ZZUNIQUEMARKER.txt, lib.rs, with no tool call
POST /agent/cancel on a running turn {"cancelled":true,"settled":true}, HTTP 200, in 0.05 s — the daemon-side half of M2

The 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 in docs/agent-loop/README.md, per docs/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:

  • 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 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_cache had 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 --check clean, cargo test -p biorouter --lib 3781 passed, ./scripts/clippy-lint.sh exit 0, and cargo test -p biorouter --lib -- workspace_summary moim agents::agent 186 passed.

Third commit — a Windows hazard caught by reading, not by CI

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 Windows — 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.

Worth recording why CI might not have caught it: 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 test (windows-latest) could have passed on luck.

The regression test asserts the rule twice — skip_reason against 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:

assertion `left == right` failed: a temp dir must get a workspace map: .../AppData/Local/Temp/.tmpABC123
  left: Some(OpaqueTree)  right: None

Gates after it: cargo fmt --check clean, cargo test -p biorouter --lib 3782 passed, ./scripts/clippy-lint.sh exit 0.

🤖 Generated with Claude Code

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`.
@Broccolito
Broccolito merged commit ba22486 into main Sep 10, 2026
16 checks passed
@Broccolito
Broccolito deleted the fix/workspace-summary-off-critical-path branch September 10, 2026 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant