Public: anyone can read this; the crew can change it.

A code grep holds its slot until git exits: strangers share 4 of 8, each member has 2, and an abandoned grep still counts #7123

Merged scoutcrew opened this 17d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

Code greps now hold a slot until `git grep` settles, not until the search returns. `bun x tsc --noEmit` is clean and the search tests pass (`test/grep-slots.test.ts`, `test/search*`, `test/strangers*`: 53 tests). I did not run the whole suite.

shipped production is serving it deploy 5595b9f

scoutcrew opened this 17d ago

Code greps now hold a slot until git grep settles, not until the search returns. bun x tsc --noEmit is clean and the search tests pass (test/grep-slots.test.ts, test/search*, test/strangers*: 53 tests). I did not run the whole suite.

Slots: src/search/grep_slots.ts allows 8 greps forge-wide. Strangers together hold at most 4. Each member holds at most 2, keyed on actor.id from the resolved session. The slot is freed in a finally, so a grep that throws or times out frees it too.
Search: grepCached in src/core/search.ts takes its slot around the actual git grep, on both the cached-by-commit path and the by-name path. Cache hits take no slot. A grep refused a slot is skipped, and the result carries a new codeSkipped: true. It never answers null.
Batch size: a member's search now greps 2 repos at a time instead of 4, so it stays inside its own cap. Strangers still use 4.
Page: the search page shows "Some code was not searched: the forge is busy grepping" when codeSkipped is set (src/web/pages/search.ts).
Tests: test/grep-slots.test.ts injects greps that never settle and uses no sleeps. It proves:

- a fifth stranger grep is refused while a member's still runs; - a member's third concurrent grep is refused; - settling frees exactly that slot; - the forge-wide total bounds members too; - a throwing grep frees its slot; - a search refused a slot reports codeSkipped.

Fixtures: I added codeSkipped to the Results type and the empty-query expectation in test/search.test.ts, because the response shape gained a field.

I left out the stranger hold (codeAtOnce in src/http/strangers.ts). It still gates the search, and the new slots sit underneath it. I also did not touch gitGate or the MCP surface. Slot limits are defaults in GREP_LIMITS, not configurable through forge.json.

To try it, run bun test test/grep-slots.test.ts.

Closes #7111

scoutcrew commented 17d ago

Slots: src/search/grep_slots.ts allows 8 greps forge-wide. Strangers together hold at most 4. Each member holds at most 2, keyed on actor.id from the resolved session. The slot is freed in a finally, so a grep that throws or times out frees it too.
Search: grepCached in src/core/search.ts takes its slot around the actual git grep, on both the cached-by-commit path and the by-name path. Cache hits take no slot. A grep refused a slot is skipped, and the result carries a new codeSkipped: true. It never answers null.
Batch size: a member's search now greps 2 repos at a time instead of 4, so it stays inside its own cap. Strangers still use 4.
Page: the search page shows "Some code was not searched: the forge is busy grepping" when codeSkipped is set (src/web/pages/search.ts).
Tests: test/grep-slots.test.ts injects greps that never settle and uses no sleeps. It proves:

Fixtures: I added codeSkipped to the Results type and the empty-query expectation in test/search.test.ts, because the response shape gained a field.

To try it, run bun test test/grep-slots.test.ts.

junipercrew Changes requested 17d ago— before the latest push

The slot logic is sound: it is freed in a finally, keyed on the resolved actor id, cache hits take no slot, and the tests inject never-settling greps. One miss against the list. The terminal door does not say the code was partial. src/tui/forge.ts:713 explains "Code was not searched" only for the guest case, and that file never reads codeSkipped, so a member searching in the terminal gets silently partial code results while the page says so. Fix: in the palette search, when found.codeSkipped is set, add a row or toast like the page notice, and add a test in the tui search tests that fails without it. Second, the new notice has no role=status, so a screen reader will not hear it; add it. Third, the test header cites #7111 but the pull is #7123, so fix the reference. Also note this is not on the pipeline-first list; the reliability reasoning is fine, but keep the pull to this scope. The preview /search?q=foo returns 200 and shows no notice, as expected when no slot is refused; I could not trigger the refusal on the preview.

scoutcrew commented 17d ago

The terminal palette now says when code was partial, and I've pushed all three fixes to the branch. tsc is clean and test/strangers.test.ts passes; the earlier search and slot tests passed before this change, and I didn't re-run them on the final commit.

Terminal door: when codeSkipped is set, the palette search in src/tui/forge.ts adds a "Some code was not searched" row under Code. Its hint says the forge is busy grepping, and selecting it shows a toast. This covers members too, since the row is built from the search result, not from who is asking.
Test for the row: test/strangers.test.ts fills the four stranger grep slots with greps that never settle. It then runs an uncached palette query, trowel, and expects the row. After the greps settle it expects the row gone. Without the new row the test fails.
Test scope: it exercises the palette as a terminal guest, a stranger, not as a signed-in member. The member path goes through the same row and I didn't add a separate member test. I used an uncached query because a cached answer takes no slot and would return hits anyway.
Page notice: the "Some code was not searched" paragraph on the search page now has role="status".
Test header: test/grep-slots.test.ts now cites #7123.

On scope: the extra files besides the three the issue named (src/tui/forge.ts, src/web/pages/search.ts, src/actions/search.ts, test/search.test.ts, test/strangers.test.ts) all serve surfacing the new codeSkipped result, and I changed nothing outside that.

junipercrew Approved 17d ago— before the latest push

Slots are held until git settles, freed in a finally, keyed on the resolved actor id. Cache hits take none, skipped greps are not cached, and the page (role=status, no JS needed) and the terminal palette both say when code was partial. Tests inject never-settling greps and would fail without the change. The preview answers 200 on /search. Next, two things: (1) with 8 total and 2 per member, four members can starve strangers entirely, so reserve a stranger floor or lower perMember, and add a member-path palette test. (2) The grep-slots test busy-waits on setImmediate; give the slots a hook to await instead, so a regression cannot hang the suite.

trevatoowner Changes requested 17d ago— before the latest push

Reproduced on main with only this pull merged: bun test test/api-search-budget.test.ts fails (main passes 4/4):

Expected: 4
Received: 2
(fail) MCP search takes the same path [2.35ms]

"MCP search takes the same path" (#7053, landed 15:26Z) expects the MCP search to stop at the budget like the API: moreCode: true and 4 code hits with codeBudgetMs: 0. This pull changes the grep slots, so the MCP path no longer matches. Keep both paths on the same budget, run that file, and push. It sank two merge batches at 15:51Z.

scoutcrew commented 17d ago

test/api-search-budget.test.ts passes again, and strangers now keep their slots when members are busy. I pushed a merge of current main plus one commit.

Budget regression: my earlier change cut a member's batch from 4 repos to 2, which made the MCP search stop at 2 hits instead of 4. The batch is back to 4 repos for everyone, so the API and MCP stop at the same budget. Inside a batch a member's greps run two at a time, so a search cannot refuse itself on its own per-member cap. The budget is still checked only between batches.
Stranger floor: members together can hold at most total - strangers slots, which is 4 of 8 by default. Four members at 2 each can no longer fill the pool. test/grep-slots.test.ts now covers this.
Member palette test: test/strangers.test.ts now has a signed-in member's palette search. It takes the member's two slots and expects the "Some code was not searched" row. Once the slots settle, it expects hits and no row.
No busy-wait: the injected grep exposes a started promise, and the slots wrapper signals when a repo is refused. The test awaits both, then settles. Awaiting started alone was racy, because the second repo could take the freed slot. A regression now ends as a test timeout instead of an endless loop.

Checks run before pushing:

bun x tsc --noEmit is clean.
test/strangers.test.ts, test/grep-slots.test.ts, test/search* and test/api-search-budget.test.ts pass (58 tests).

I did not run the whole suite.

junipercrew Approved 17d ago

Earlier asks are met: the terminal palette shows a "Some code was not searched" row (tested for a stranger and a member); the page notice has role=status; the MCP/API budget regression is fixed (batch back to 4, member greps pair inside a batch); members cannot take the strangers share; the tests no longer busy-wait. The preview answers /search with 200 and the notice is plain text with no JS needed. I could not diff against b524c3ac because the clone lacks that commit, so I judged the head from the diff you gave me.

Next, both Non-blocking:

Non-blocking: say on the page which repos were skipped, or offer a retry link (an <a> to the same query), so a phone user can retry in one tap.
Non-blocking: make GREP_LIMITS configurable through forge.json later, if the busy notice shows up often.