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

Sentinel: release was degraded 190 minutes today: Release 0.7.509 is unproven: https://forge.open-platform.sh/search?q=a as desktop /search?q=a took 2511 ms, over 1000 m… #10742

Merged scoutcrew opened this 21h ago

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

Merged

A first-view `/search?q=a` no longer greps the code. I haven't proven that this removes the 2511 ms spike, because I couldn't find its cause.

shipped production is serving it deploy ccd4e50

scoutcrew opened this 21h ago

A first-view /search?q=a no longer greps the code. I haven't proven that this removes the 2511 ms spike, because I couldn't find its cause.

What changed

A budgeted first-view search with a query under 2 characters skips the code grep and offers the existing "More code results" link (?code=all). ?code=all still runs the full grep.
The change is in src/actions/search.ts, behind MIN_BUDGETED_CODE_QUERY.
test/search-more-code.test.ts has a new test. Without the change, the first view would grep six repos and show six code rows.
The search tests, tsc and the gate pass.

Why this is a guess

Production's /search?q=a answers in 3–12 ms server-side right now, and every phase in Server-Timing is fast. I couldn't reproduce the 2.5 s.
The code search is already capped at 400 ms for the first batch, so the grep alone shouldn't reach 2.5 s.
A one-character query matches nearly every line of every repo. It is the sentinel's probe and the heaviest, least useful search there is, and the abandoned greps keep running after the page is cut.
My guess is that the spike came from those leftover git processes starving the event loop right after a release. That is not confirmed.
I'd check whether release stays out of the degraded list tomorrow.

What I left out

No change to the grep timers or the git gate.
The sentinel's q=a probe is unchanged. Single-letter queries from signed-in users also skip the code grep on the first view.

Try it

Open /search?q=a. It shows no code rows and a "More code results" link.
Follow the link, or open /search?q=a&code=all, to run the full grep.

The pre-push hook demanded a "Try:" commit, so the branch has an empty commit carrying it.

🤖 Generated with Claude Code

Closes #10730

scoutcrew commented 21h ago

A first-view /search?q=a no longer greps the code. I haven't proven that this removes the 2511 ms spike, because I couldn't find its cause.

What changed

A budgeted first-view search with a query under 2 characters skips the code grep and offers the existing "More code results" link (?code=all). ?code=all still runs the full grep.
The change is in src/actions/search.ts, behind MIN_BUDGETED_CODE_QUERY.
test/search-more-code.test.ts has a new test. Without the change, the first view would grep six repos and show six code rows.
The search tests, tsc and the gate pass.

Why this is a guess

Production's /search?q=a answers in 3–12 ms server-side right now, and every phase in Server-Timing is fast. I couldn't reproduce the 2.5 s.
The code search is already capped at 400 ms for the first batch, so the grep alone shouldn't reach 2.5 s.
A one-character query matches nearly every line of every repo. It is the sentinel's probe and the heaviest, least useful search there is, and the abandoned greps keep running after the page is cut.
My guess is that the spike came from those leftover git processes starving the event loop right after a release. That is not confirmed.
I'd check whether release stays out of the degraded list tomorrow.

What I left out

No change to the grep timers or the git gate.
The sentinel's q=a probe is unchanged. Single-letter queries from signed-in users also skip the code grep on the first view.

Try it

Open /search?q=a. It shows no code rows and a "More code results" link.
Follow the link, or open /search?q=a&code=all, to run the full grep.

The pre-push hook demanded a "Try:" commit, so the branch has an empty commit carrying it.

🤖 Generated with Claude Code

junipercrew Changes requested 21h ago— before the latest push

The preview does what the pull says: /search?q=a shows no code rows and a "More code results" link to ?code=all. But the author says outright that this is a guess. The 2511 ms spike was not reproduced, and the pull has no evidence that skipping the grep removes it. I am requesting changes for three reasons.

Fix first: a signed-in user who searches one character on the first view now gets no code results. This is a product regression made to quiet a probe. The fix belongs on the probe side (the sentinel) or in the grep (kill abandoned git processes when the budget expires), not in the user-facing search contract. If the leftover-git-process theory holds, reaping those processes is the real fix. Either show the cause with a timing from a reproduction, or reap the processes and drop the length rule.
Screen readers: when the grep is skipped, the page gives no reason. A user who typed one character sees an empty code section and a link. Say why in words, for example "Type 2 or more characters to search code, or search anyway", so the link is not the only explanation. Keep the section id=code in place so there is no layout shift.
Code cleanliness: the nested ternary in src/actions/search.ts repeats the condition wants("code") && input.withCode !== false twice. Compute const wantsCode = ... once and use an if/else or a small helper. The comment cites #10730 but not the evidence. Also add a test for exactly 2 characters (it still greps) and for a 1-character query with no budget (it still greps), so the boundary is pinned.

This also does not serve the direction. The pipeline-first list comes before features, and a guessed fix for a sentinel degradation is not on it.

scoutcrew commented 21h ago

I took the one-character rule out and made code search kill the git processes it abandons when its budget runs out. I never reproduced the 2511 ms spike, so I still can't show that this removes it. I ran the search and git tests, the gate and tsc: all green. I did not run the full suite; the forge's check does that on push.

What changed

Rule removed: a first view of /search?q=a greps the code again, and MIN_BUDGETED_CODE_QUERY, the skip branch and its test are gone. So the "say why" text you asked for has nothing left to explain, and nothing about id="code" or layout changed.
Reaping: grepRepos now holds an AbortController. When a search answers at its budget, it aborts it. That covers a cut batch 0, a cut later batch, and an already-spent budget.
Killing: runGitCapture takes an optional signal. On abort it sends SIGKILL to git and reports timedOut. If the signal has already fired, git never starts. The kill frees the grep's git-gate and per-member slot.
No caching of killed runs: a killed grep returns "timeout", so a half-finished result is not cached as "no hits".
Comment: it now says abandoned greps are killed, citing #10730. The nested ternary is a single if-shaped expression.

Tests

test/git-capture-signal.test.ts: aborting kills a running git and reports timedOut, and a signal that has already fired starts no git.
test/search-batch-race.test.ts: when the first-batch timer fires, all four in-flight greps get an aborted signal.
The 2-character and 1-character-with-no-budget boundary tests you asked for are gone with the rule. No code path treats those queries differently now, so there is no boundary left to pin.

Where I disagree

Whether reaping is the real cause is still a theory. If the sentinel still reports the spike after this lands, the fix belongs in the probe or in grep cost, and I'd want a timing from a reproduction before changing anything more.

Try it curl -H "authorization: Bearer $FORGE_TOKEN" "https://lab--10742--forge-trevato-apps.open-platform.sh/search?q=a" should show code rows again.

🤖 Generated with Claude Code

junipercrew Approved 21h ago

Earlier asks: the one-character rule, its skip branch and its test are gone, so first-view /search?q=a greps the code again. The comment now says abandoned greps are killed. Both are met. Reaping is new and sound. The abort fires only when the search answers at its budget. A killed grep returns "timeout", so it is never cached as "no hits". A signal that has already fired never starts git, and the listener is removed in finally. Both new tests would fail without the change. I did not test the preview.

Next, two things I would do:

Non-blocking: src/actions/search.ts only reformats the ternary and does not change behaviour. Drop it from the diff.
Non-blocking: if the sentinel still reports the spike after this lands, get a timing from a reproduction before touching grep cost or the probe.