Fixes srch-765
When navigating to `/cody/dashboard` and the new web app is enabled, the
user gets a "Not found" error. This doesn't happen in the React app.
The page doesn't exist in the new web app, hence the server should fall
back to the old web app but that doesn't happen.
Why? Because the server doesn't know about `/cody/dashboard` either. It
only exists in the React app.
Instead the server interprets this path as a (non-existing) repository
page. When the new web app is enabled, repository pages are handled by
the new web app, but since neither the repo nor the page exist in the
new web app, the aforementioned error is thrown.
Configuring this route on the server makes it so that the React app is
served instead.
## Test plan
Manual testing. Going to https://sourcegraph.test:3443/cody/dashboard
loads the React app.
Follow up to #64207. In our old search semantics, quotes were
interpreted literally. So a query like `"sourcegraph"` would match only
strings like `fmt.Println("sourcegraph")`. Now, both single and double
quotes are used for escaping, and mean that the contents should be
searched exactly.
This PR makes sure to boost matches on quoted terms in result ranking.
This way, users familiar with the old syntax are more likely to find
what they're after.
## Test plan
Adapted unit tests. Re-tested all queries from #64207 manually, plus
these ones:
* `'sourcegraph'`
* `"sourcegraph"`
A long time ago, we dropped the requirement for frontend to have any
disk for caching. Our helm deployments use read-only rootFSes, so this
wouldn't even work.
This PR aims to make that clearer by removing some last remnants of
those times.
Test plan: Frontend starts locally and integration tests pass in CI.
In #64215, we removed support for smart search. We continued to allow
the `smart` search mode, but just execute with `precise` behavior.
However, we started to throw an error for `patterntype:lucky` (which is
the old way of specifying smart search). It turns out that `lucky` made
its way into some search links and settings, causing those searches to
fail.
This PR restores support for `lucky`, and remaps it to `standard`. This
preserves the semantics as much as possible (since smart search attempts
a 'standard' search as its first rule).
Closes SRCH-840
tl;dr: Everything is derived from `window.context.svelteKit.knownRoutes`
*buckle up*
### Context
After the new web app was enabled by default on dotcom I looked at
dotcom data in Sentry to see if there was anything out of the ordinary.
I noticed a high number of errors related to resolving repository
information. The most common non-existing repository that was reported
was `-/sign-out`.
Of course this shouldn't happen. `/-/sign-out` is the sign out URL and
shouldn't be interpreted as a repository.
Currently we prevent SvelteKit from interpreting specific paths as
repositories by using the `reporev` parameter validator:
```ts
// src/params/reporev.ts
const topLevelPaths = [
'insights',
'search-jobs',
'saved-searches',
// ... many more here
]
const topLevelPathRegex = new RegExp(`^(${topLevelPaths.join('|')})($|/)`)
// This ensures that we never consider paths containing /-/ and pointing
// to non-existing pages as repo name
export const match: ParamMatcher = param => {
// Note: param doesn't have a leading slash
return !topLevelPathRegex.test(param) && !param.includes('/-/')
}
```
`-/sign-out` was not in that list, including others which have been
added since this file was originally created.
Adding it would have been the simple solution but having to maintain
this list manually has always irked me. Now we have a better way...
### Production changes
#### Client side
It turns out we are already sending a list of known routes to the client
in `window.context.svelteKit.knownRoutes` to basically do the same
thing: test whether a given path is a page or a repository.
```ts
// src/lib/navigation.ts
let knownRoutesRegex: RegExp | undefined
function getKnownRoutesRegex(): RegExp {
if (!knownRoutesRegex) {
knownRoutesRegex = new RegExp(`(${window.context?.svelteKit?.knownRoutes?.join(')|(')})`)
}
return knownRoutesRegex
}
// ...
export function isRouteEnabled(pathname: string): boolean {
let foundRoute: SvelteKitRoute | undefined
// [...]
if (foundRoute) {
if (foundRoute.isRepoRoot) {
// Check known routes to see if there is a more specific route than the repo root.
// If yes then we should load the React app (if the more specific route was enabled
// it would have been found above).
return !getKnownRoutesRegex().test(pathname)
}
return true
}
return false
}
```
Why do we have the `reporev` validator and `isRouteEnabled`? They
basically do the same thing (check whether a path is a known page or a
repository) the first (`reporev`) is used by SvelteKit to determine
which route a path corresponds to (i.e. to navigate to the repository
page or whether the page doesn't exist) and the second one
(`isRouteEnabled`) is used after the route resolution but before route
loading. It's used to trigger a full page refresh to fetch the React
version if necessary.
Anyways, since we already have a list of known pages from the server,
the parameter validator should just use it too. Except it didn't work,
which made the following server side changes necessary.
#### Server
We register routes in multiple places. Right now `knownRoutes` is
populated from the router created in
`cmd/frontend/internal/app/ui/router.go`, but this does not include
`/-/sign-out`. This route (and others) are defined in
`cmd/frontend/internal/app/router/router.go` (I don't know what the
difference is). I extended the sveltekit route registration code so
that's possible to register routes from multiple routers.
I couldn't test it yet however because I currently can't get Sourcegraph
to run locally (Camden tested it; it works).
### Development mode changes
After the above changes, navigating to a React only page in development,
e.g. `/insights` would show a 'repo not found error' because
`window.context.svelteKit.knownRoutes` was empty in development. So
every non-existing page would be interpreted as missing repository.
Hardcoding a list of known pages *again* just for development seemed to
be a step backwards. So I spent quite a bit of time to find a way to
extract the JS context object from the HTML page returned by the origin
server and inject it into the HTML page generated by SvelteKit, similar
to how we do it for the React app.
Additionally the value of `window.context.svelteKit.knownRoutes` is now
used to setup the proxy to non-supported pages from the server, so we
don't have to maintain this regex anymore either:
```ts
// Proxy requests to specific endpoints to a real Sourcegraph
// instance.
'^(/sign-(in|out)|/.assets|/-|/.api|/.auth|/search/stream|/users|/notebooks|/insights|/batch-changes|/contexts)|/-/(raw|own|code-graph|batch-changes|settings)(/|$)': { ... }
```
This means that requesting non-svelte pages will also return the
corresponding React version in development.
## Test plan
Manual testing.
Relates to #64186
With this PR we only show `83 out of 120 tasks` if the search job is
currently processing. In all other states, we don't show this stat. This
is a consequence of the janitor job I recently added, because after
aggregation, this data is not available anymore. User's can still
inspect the logs and download results to get a detailed view of which
revisions were searched.
I also remove an unnecessary dependency of the download links on the job
state.
## Test plan:
I ran a search job locally and confirmed that the progress message is
only visible while the job is processing and that logs and downloads are
always available.
## Changelog
- Show detailed progress only while job is in status "processing"
- Remove dependency of download links on job state
Instead of re-generating and re-compiling the mocks 3 times,
consolidate them into a single storemocks package.
This should also provide better code navigation in the editor.
Previously, for metrics reporting, we were querying the number of
queued+errored records every 5 seconds. For large queues +
heavy contention, this scan itself takes a few seconds, despite using
an index. Reduce the frequency of this scan as we
don't need updates to the queue size every 5 seconds.
## Test plan
Check that frequency of `COUNT(*)` is reduced on Sourcegraph.com once
this patch makes its way to Sourcegraph.com
The current logging is outdated. This PR is not a complete rewrite. It
tries to keep semantics mostly as-is but it is more generous when it
comes to what we log. (For example we didn't log `(AND foo bar)` before)
IMO this is a good compromise we can ship quickly and then take some
time to figure out how to distinguish suggestions, searches, code-intel
and whether it is a good idea to mix search types and result types.
Updates:
- Prefer to log result types, fall back to pattern type. (This is
largely the same logic as before but we don't bail for complex queries)
- Don't guess repo and file results types if not explicitly specified.
We loose a bit of information here but it keeps the code much cleaner.
- Allow "AND" and "OR" operators.
The updated test case gives a good overview of how we would log things
in the future.
Kept as-is:
- I left "new" and "legacy" logging in place. I am not sure how we use
the target stores right now so I wanted to keep this as-is for now.
However we should see if we can retire the legacy logging.
- The previous and current logic translates `type:file` queries to
pattern types. I guess this makes sense from the perspective of how
things are implemented in the backend.
- "type:path" is logged as "file".
## Test plan:
Updated unit test
This PR fixes Cody autocomplete in some situations, fixing
[PRIME-426](https://linear.app/sourcegraph/issue/PRIME-426/autocomplete-completely-broken-in-main-with-and-sometimes-without-the).
Our story begins with this PR:
https://github.com/sourcegraph/sourcegraph/pull/63886. The PR updated
the `CodyLLMConfigurationResolver.Provider` GraphQL endpoint to no
longer return the "provider" that was put into the site configuration,
but to instead return the _display name_ of the provider, or if multiple
were configured the string "various".
```diff
- func (c *codyLLMConfigurationResolver) Provider() string {
- return string(c.config.Provider)
- }
+ func (c *codyLLMConfigurationResolver) Provider() string {
+ if len(r.modelconfig.Providers) != 1 {
+ return "various"
+ }
+ return r.modelconfig.Providers[0].DisplayName
+}
```
This change was wrong on several levels, and unfortunately stemmed from
the original author (me) not tracking down and understanding how the
GraphQL endpoint was actually used.
Once we discovered the problem, we quickly rectified this by changing
the behavior to the more correct version of returning the Provider ID of
the code completion model with
https://github.com/sourcegraph/sourcegraph/pull/64165:
```go
func (r *codyLLMConfigurationResolver) Provider() string {
return string(r.modelconfig.DefaultModels.CodeCompletion.ProviderID())
}
```
However, after some more testing we discovered yet another edge case. We
didn't realize that the provider "sourcegraph" (which is how you
configure Sourcegraph to use Cody Gateway, using the older site config
method) was required in some scenarios on the Cody client.
So the new logic, of returning the Provider ID was incorrect. (Because
we report the _model_ provider, e.g. "anthropic". Not the _API_
provider, e.g. "sourcegraph"/"Cody Gateway".)
With this change, we should just have the behavior we had initially:
returning whatever the admin had configured in the
"completions.provider" section of the site config. If only the newer
modelconfig settings were used, we return "sourcegraph" if using Cody
Gatway. And if not, just return the Provider ID. (Though in some
situations even that will lead to incorrect results, because ultimately
we need to update the client here.)
## Test plan
I wasn't able to test this super-well manually, and am relying on
Stephen and Taras' knowledge of how the client uses this today.
## Changelog
NA
When I added this, for some reason I thought it was site admin only. As
it turns out, it shouldn't be gated to site admins only, and it makes it
particularly annoying on dotcom.
Reverts sourcegraph/sourcegraph#64014
We decided that including this in the August 7th release would be rushed
after diving deeper into the project. Reverting this in order to
re-write the functionality, especially the data loading components.
## Test Plan
- this is a revert PR. Everything will work as before.
There is no need to have a separate image file for dark mode. The SVG
file can handle color switching itself.
## Test plan
Switched theme in the user menu and in the browser dev tools.
Nothing outside frontend needs to import this package, to properly signify that we move it to internal where 90% of the other packages reside, and avoid ambiguity on "what service is running what code exactly".
Test plan: Just moved a package, Go compiler doesn't complain.
This PR simplifies and consolidates the remaining parts of the various registry packages, and moves everything under cmd/frontend/internal for better encapusation.
This also removes the need to _ import packages which feels very brittle.
Test plan: Go compiler doesn't complain about moved code.
To make things more explicit and remove the global variable, this is now passed down to where it's needed.
It is a bit messy right now, since it's used deep in the serve-handler but that just highlights better where
it's actually used IMO. As a next step, I want to get rid of the requirement to indicate server-restart
required, so we should be able to drop a bunch of the prop drilling here.
Test plan: Still compiles, E2E test works.
The problem was that we were incorrectly not doing
a "stack push" operation when encountering a comment,
but we were doing a pop which triggered a change in the
source range for the next source range (this is done to
avoid overlap between the current occurrence and the
subsequent occurrence that will be emitted).
This is fixed by not ignoring comment markers. As a consequence, we will
emit a few more occurrences than earlier, but I don't think it should be a
big problem in practice. We can also fuse these if needed.
<!-- PR description tips:
https://www.notion.so/sourcegraph/Write-a-good-pull-request-description-610a7fd3e613496eb76f450db5a49b6e
-->
Closes
https://linear.app/sourcegraph/issue/CODY-2758/[autocomplete-latency]-add-circuit-breaker-in-cody-gateway-to-handle
This PR introduces an in-memory model availability tracker to ensure we
do not send consequent requests to the currently unavailable LLM
providers.
The tracker maintains a history of error records for each model. It
evaluates these records to determine whether the model is available for
new requests. The evaluation follows these steps:
1. For every request to an upstream provider, the tracker records any
errors that occur. Specifically, it logs timeout errors (when a request
exceeds its deadline) and responses with a 429 status code (Too Many
Requests).
2. These error records are stored in a circular buffer for each model.
This buffer holds a fixed number of records, ensuring efficient memory
usage.
3. The tracker calculates the failure ratio by analyzing the stored
records. It checks the percentage of errors within a specified
evaluation window and compares this against the total number of recent
requests.
4. Based on the calculated failure ratio, the tracker decides whether
the model is available:
- Model Unavailable: If the ratio of failures (timeouts or 429 status
codes) exceeds a predefined threshold (X%), the model is marked as
unavailable. In this state, the system does not send new requests to the
upstream provider.
When a model is unavailable, the system immediately returns an error
status code, typically a 503 Service Unavailable, to the client. This
informs the client that the service is temporarily unavailable due to
upstream issues.
- Model Available: If the failure ratio is within acceptable limits, the
system proceeds with sending the request to the upstream provider.
This PR suggests considering a model unavailable if **95% of the last
100 requests within the past minute** either time out or return a 429
status code. I am not sure about these exact values and suggest them as
a starting point for discussion
## Test plan
- Added unit tests
- CI
<!-- REQUIRED; info at
https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles
-->
## Changelog
<!-- OPTIONAL; info at
https://www.notion.so/sourcegraph/Writing-a-changelog-entry-dd997f411d524caabf0d8d38a24a878c
-->
This only contains one commit which reduces how often we call scan in
indexserver on dotcom.
- acacc5eda1 shards: only trigger rescan on .zoekt files changing
Test Plan: tested in zoekt CI
Searcher doesn't speak to the database nor has it for a long time. See
https://github.com/sourcegraph/sourcegraph/pull/61463
Test Plan: The following command is empty
go run ./dev/depgraph/ summary internal/database | grep 'cmd/searcher'
The `getSourceRange` function can have a range translation failure,
in which case it would be wrong to propagate the range directly
without the commit information. So skip the ranges in that case.
Introduces basic (and incomplete) UI support for perforce, including displaying changelist ids,
listing changelists, and removing references to commits and branches.
The URL path for streaming blame is ambiguous when the file path
contains `/stream/`. The pattern looks like
`/blame/<repo_name>@<revision>/stream/file/path.txt`. However, if the
file contains `/stream/`, the revision is matched greedily up until the
last stream, so we end up with a revision that looks like
`81af3g/stream/my/path`, which is in invalid revision that results in a
404.
This makes the URL pattern unambiguous by adding a `/-/` element after
the revision, which is not allowed in a revision name and acts as a path
separator. So now, the pattern looks like
`/blame/<repo_name>@<revision>/-/stream/file/path.txt`.
Note, this is a public-facing breaking change, but I don't think it
really matters since I don't expect any customers use our streaming
blame API
Automatically generated PR to update package lockfiles for Sourcegraph
base images.
Built from Buildkite run
[#285676](https://buildkite.com/sourcegraph/sourcegraph/builds/285676).
## Test Plan
- CI build verifies image functionality
Co-authored-by: Buildkite <buildkite@sourcegraph.com>
With SRCH-802 we saw error messages when batch changes may still retry
and resolve the problem. To give a better distinction between resolvable
and non-resolvable errors, I'm changing the colour variation from error
to warning if the changeset has not been marked as `FAILED` yet.
Before:
<img width="913" alt="Screenshot 2024-08-02 at 12 44 27"
src="https://github.com/user-attachments/assets/b192c5e9-d23b-460c-82a7-a039edbca3f5">
After:
<img width="1355" alt="Screenshot 2024-08-02 at 12 36 23"
src="https://github.com/user-attachments/assets/02e231a7-168a-4fe9-bd3b-014d810fd236">
## Test plan
Manual testing
## Changelog
- Batch changes that are still retrying now show a warning instead of an
error.
ResolveRevision additionally adds a "^0" to the spec to make sure we
actually check if it exists. This is important, since most uses of the
ChangedFiles API pass in commit shas which do not get resolved by git
unless the "^0" is present.
I noticed this since hybrid search in searcher started to fail if the
commit was missing. In particular Zoekt for empty repositories uses a
fake SHA of 404aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa so when a repo
started getting cloned hybrid search would fail until it was indexed.
Hybrid search explicitly checks for the NotFound error, so hybrid search
failing was a a regression from moving away from the SymbolDiff API.
