mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-29 00:03:09 +00:00
fix(ai): correct headless lint ownership, staleness and timeouts
Review findings on the headless linter: - An editor can adopt a model headless linting created, so attachment now decides who owns a buffer rather than who created it. The editor likewise applies its own content when adopting an existing model, instead of displaying whatever was last linted. - Markers are cleared when an owned model is rewritten: they outlive the content they describe, and a matching count let the settle loop return the previous version's diagnostics. - Eviction no longer drops the URI it is keeping, which had left a live model untracked and subsequently treated as belonging to an editor. - A language server that stalls is named in the result rather than contributing an empty list, and no longer discards what other servers already reported. - A client abandoned at its deadline is stopped, so it cannot keep holding the global command names every later lint of that language needs. - Deno's import map is built from the path its document actually has. - Pre-lint network calls are bounded; lints of one URI queue behind each other, so a hung request would otherwise wedge that item indefinitely. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
0236c3b145
commit
aaa7601950
@@ -107,7 +107,7 @@
|
||||
} from './lint/typescriptExtraLibs'
|
||||
import { createWindmillAta, genAtaRoot } from './lint/typescriptAta'
|
||||
import { readModelMarkers } from './lint/markers'
|
||||
import { buildDenoImportMap, lspServersFor } from './lint/lspLanguageConfig'
|
||||
import { buildDenoImportMap, hasLanguageServers, lspServersFor } from './lint/lspLanguageConfig'
|
||||
import { aiChatManager } from './copilot/chat/AIChatManager.svelte'
|
||||
import type { Selection } from 'monaco-editor'
|
||||
import { canHavePreprocessor, getPreprocessorModuleCode } from '$lib/script_helpers'
|
||||
@@ -980,15 +980,7 @@
|
||||
export async function reloadWebsocket() {
|
||||
await closeWebsockets()
|
||||
|
||||
if (
|
||||
!useWebsockets ||
|
||||
!(
|
||||
(lang == 'typescript' && scriptLang === 'deno') ||
|
||||
lang == 'python' ||
|
||||
lang == 'go' ||
|
||||
lang == 'shell'
|
||||
)
|
||||
) {
|
||||
if (!useWebsockets || !hasLanguageServers(lang, scriptLang)) {
|
||||
return
|
||||
}
|
||||
console.log('reloadWebsocket')
|
||||
@@ -1146,11 +1138,7 @@
|
||||
denoImportMap = buildDenoImportMap(await genAtaRoot($workspaceStore ?? ''), filePath)
|
||||
}
|
||||
|
||||
const servers = lspServersFor({ editorLang: lang, scriptLang, denoImportMap })
|
||||
if (servers.length === 0) {
|
||||
closeWebsockets()
|
||||
}
|
||||
for (const server of servers) {
|
||||
for (const server of lspServersFor({ editorLang: lang, scriptLang, denoImportMap })) {
|
||||
await connectToLanguageServer(
|
||||
server.url,
|
||||
server.name,
|
||||
@@ -1359,6 +1347,12 @@
|
||||
if (!nmodel) {
|
||||
throw err
|
||||
}
|
||||
// Headless linting creates models at this same canonical URI, so the existing
|
||||
// one can hold whatever content was last linted. Show what this editor was
|
||||
// given, not that.
|
||||
if (nmodel.getValue() !== (code ?? '')) {
|
||||
nmodel.setValue(code ?? '')
|
||||
}
|
||||
model = nmodel
|
||||
}
|
||||
model.updateOptions(lang == 'python' ? { tabSize: 4, insertSpaces: true } : updateOptions)
|
||||
@@ -1620,8 +1614,8 @@
|
||||
modelUri: uri,
|
||||
absolutePathExtraLibs,
|
||||
isCancelled: () => destroyed,
|
||||
registerLocalModels: () => editor != undefined,
|
||||
onLocalFileRegistered: () => {
|
||||
if (!editor) return
|
||||
try {
|
||||
model?.setValue(model.getValue())
|
||||
} catch (e) {
|
||||
|
||||
@@ -3832,7 +3832,7 @@ async function getLintErrors(args: LintTargetArgs, ctx: WriteDraftCtx): Promise<
|
||||
|
||||
let response = formatScriptLintResult(result)
|
||||
if (result.unavailableServers?.length) {
|
||||
response += `\n\nNote: the ${result.unavailableServers.join(' and ')} language server${result.unavailableServers.length > 1 ? 's were' : ' was'} unavailable, so some problems may not be listed. This usually means an editor is already open on a ${target.language} item.`
|
||||
response += `\n\nNote: the ${result.unavailableServers.join(' and ')} language server${result.unavailableServers.length > 1 ? 's did' : ' did'} not respond, so some problems may not be listed. Treat a clean result as inconclusive.`
|
||||
}
|
||||
if (result.contentMismatch) {
|
||||
response += `\n\nNote: an editor is currently open on this code and its buffer differs from the draft. The results above are for what that editor shows.`
|
||||
|
||||
@@ -47,8 +47,7 @@ export interface HeadlessLintOutcome extends ScriptLintResult {
|
||||
unavailableServers?: string[]
|
||||
}
|
||||
|
||||
// Models we created ourselves, oldest first. An editor may later adopt one by URI, so
|
||||
// eviction must never dispose a model that is attached to an editor.
|
||||
// Models we created ourselves, least recently used first.
|
||||
const ownedModels: string[] = []
|
||||
const MAX_OWNED_MODELS = 4
|
||||
|
||||
@@ -56,11 +55,29 @@ const absolutePathExtraLibs = new Map<string, { dispose: () => void }>()
|
||||
const ataByKey = new Map<string, (source: string) => Promise<void>>()
|
||||
const pendingByUri = new Map<string, Promise<HeadlessLintOutcome>>()
|
||||
|
||||
function withDeadline(promise: Promise<unknown>, ms: number): Promise<unknown> {
|
||||
return Promise.race([promise, new Promise((resolve) => setTimeout(resolve, ms))])
|
||||
}
|
||||
|
||||
function touchOwnedModel(uriString: string) {
|
||||
const at = ownedModels.indexOf(uriString)
|
||||
if (at >= 0) ownedModels.splice(at, 1)
|
||||
ownedModels.push(uriString)
|
||||
}
|
||||
|
||||
function evictOwnedModels(keepUri: string) {
|
||||
while (ownedModels.length > MAX_OWNED_MODELS) {
|
||||
const candidate = ownedModels.shift()
|
||||
if (!candidate || candidate === keepUri) continue
|
||||
if (!candidate) continue
|
||||
if (candidate === keepUri) {
|
||||
// Still in use: keep tracking it, or a later lint would mistake it for a model
|
||||
// owned by an editor and refuse to update its content.
|
||||
ownedModels.push(candidate)
|
||||
continue
|
||||
}
|
||||
const model = meditor.getModel(Uri.parse(candidate))
|
||||
// An editor may have adopted it in the meantime; disposing it would close the
|
||||
// user's buffer.
|
||||
if (model && !model.isAttachedToEditor()) {
|
||||
model.dispose()
|
||||
}
|
||||
@@ -127,32 +144,41 @@ async function lintOne(
|
||||
let model = meditor.getModel(uri)
|
||||
let contentMismatch = false
|
||||
if (model) {
|
||||
const isOurs = ownedModels.includes(uriString)
|
||||
if (isOurs) {
|
||||
if (model.getValue() !== req.content) model.setValue(req.content)
|
||||
} else {
|
||||
// An editor owns this model: its content is what the user sees and what they would
|
||||
// see the markers for. Lint it as-is rather than overwriting their buffer.
|
||||
// Attachment decides, not who created it: an editor can adopt a model we made, and
|
||||
// its buffer is what the user sees and would save. Never write to it.
|
||||
if (model.isAttachedToEditor() || !ownedModels.includes(uriString)) {
|
||||
contentMismatch = model.getValue() !== req.content
|
||||
} else if (model.getValue() !== req.content) {
|
||||
model.setValue(req.content)
|
||||
// Markers from the previous content survive until revalidation finishes, and a
|
||||
// matching marker count would let the wait below settle on them.
|
||||
meditor.setModelMarkers(model, editorLang, [])
|
||||
touchOwnedModel(uriString)
|
||||
}
|
||||
} else {
|
||||
model = meditor.createModel(req.content, editorLang, uri)
|
||||
ownedModels.push(uriString)
|
||||
touchOwnedModel(uriString)
|
||||
evictOwnedModels(uriString)
|
||||
}
|
||||
|
||||
// Bounded: these fetch over the network, and lints of one URI are serialized behind
|
||||
// each other, so one hung request would wedge the tool for that item indefinitely.
|
||||
if (editorLang === 'typescript') {
|
||||
await Promise.all([
|
||||
ensureResourceTypeNamespace(req.workspace, req.scriptLang).catch((e) =>
|
||||
console.error('headlessLint: resource types unavailable', e)
|
||||
),
|
||||
ensureCustomWmillTypes(req.workspace).catch(() => undefined)
|
||||
])
|
||||
await withDeadline(
|
||||
Promise.all([
|
||||
ensureResourceTypeNamespace(req.workspace, req.scriptLang),
|
||||
ensureCustomWmillTypes(req.workspace)
|
||||
]).catch((e) => console.error('headlessLint: extra libs unavailable', e)),
|
||||
timeoutMs
|
||||
)
|
||||
}
|
||||
|
||||
if (req.scriptLang === 'bun' || req.scriptLang === 'bunnative') {
|
||||
await acquireTypes(req, uriString).catch((e) =>
|
||||
console.error('headlessLint: type acquisition failed', e)
|
||||
await withDeadline(
|
||||
acquireTypes(req, uriString).catch((e) =>
|
||||
console.error('headlessLint: type acquisition failed', e)
|
||||
),
|
||||
timeoutMs
|
||||
)
|
||||
}
|
||||
|
||||
@@ -164,6 +190,11 @@ async function acquireTypes(req: HeadlessLintRequest, uriString: string): Promis
|
||||
const key = `${req.workspace}:${uriString}`
|
||||
let ata = ataByKey.get(key)
|
||||
if (!ata) {
|
||||
while (ataByKey.size >= MAX_OWNED_MODELS) {
|
||||
const oldest = ataByKey.keys().next().value
|
||||
if (oldest === undefined) break
|
||||
ataByKey.delete(oldest)
|
||||
}
|
||||
ata = await createWindmillAta({
|
||||
root: await genAtaRoot(req.workspace),
|
||||
scriptPath: req.path,
|
||||
@@ -214,15 +245,16 @@ async function countExpectedMarkers(uri: Uri, editorLang: string): Promise<numbe
|
||||
}
|
||||
}
|
||||
|
||||
// The editor's own lint paths sleep a fixed delay and hope. Instead, ask the worker how
|
||||
// many diagnostics it has and wait for that many markers to be published — which also
|
||||
// terminates promptly for clean code, where no marker-change event ever fires.
|
||||
// Ask the worker how many diagnostics it has and wait for that many markers to be
|
||||
// published. Clean code never fires a marker-change event, so there is no event to wait
|
||||
// on and a count is the only signal that validation has actually run.
|
||||
async function waitForMarkersToSettle(uri: Uri, editorLang: string, timeoutMs: number) {
|
||||
const expected = await countExpectedMarkers(uri, editorLang)
|
||||
if (expected === undefined) return
|
||||
const deadline = Date.now() + timeoutMs
|
||||
while (Date.now() < deadline) {
|
||||
const published = meditor.getModelMarkers({ resource: uri, owner: editorLang }).length
|
||||
if (expected !== undefined && published === expected) return
|
||||
if (published === expected) return
|
||||
await new Promise((r) => setTimeout(r, 50))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -57,12 +57,20 @@ function startClient(
|
||||
return new Promise((resolve) => {
|
||||
let settled = false
|
||||
let webSocket: WebSocket
|
||||
let client: MonacoLanguageClient | undefined
|
||||
|
||||
const finish = (v: StartedClient | undefined) => {
|
||||
if (settled) return
|
||||
if (settled) {
|
||||
// The deadline already gave up on this client. Stop it, or it keeps holding
|
||||
// the global command names it registered and every later lint of this
|
||||
// language reports the server as unavailable.
|
||||
if (v) void stopClient(v)
|
||||
return
|
||||
}
|
||||
settled = true
|
||||
clearTimeout(timer)
|
||||
if (!v) {
|
||||
if (client) void stopClient({ server, client, webSocket, pushedDiagnostics: Promise.resolve() })
|
||||
try {
|
||||
webSocket?.close()
|
||||
} catch {}
|
||||
@@ -93,7 +101,7 @@ function startClient(
|
||||
let resolvePushed: () => void = () => {}
|
||||
const pushedDiagnostics = new Promise<void>((r) => (resolvePushed = r))
|
||||
|
||||
const client = new MonacoLanguageClient({
|
||||
client = new MonacoLanguageClient({
|
||||
name: `headless-${server.name}`,
|
||||
messageTransports: { reader, writer },
|
||||
clientOptions: {
|
||||
@@ -226,17 +234,16 @@ export async function lintWithLsp(req: {
|
||||
}): Promise<LspLintResult> {
|
||||
await initializeVscode('headlessLsp')
|
||||
|
||||
const uriString = computeModelUri(
|
||||
computeModelPath(undefined, req.scriptLang),
|
||||
req.scriptLang,
|
||||
req.editorLang
|
||||
)
|
||||
const modelPath = computeModelPath(undefined, req.scriptLang)
|
||||
const uriString = computeModelUri(modelPath, req.scriptLang, req.editorLang)
|
||||
const uri = Uri.parse(uriString)
|
||||
const model = meditor.createModel(req.content, req.editorLang, uri)
|
||||
|
||||
// Built from where the document actually lives, not the workspace path: the map's
|
||||
// relative entries are derived from the document's own depth.
|
||||
const denoImportMap =
|
||||
req.scriptLang === 'deno'
|
||||
? buildDenoImportMap(await genAtaRoot(req.workspace), req.path)
|
||||
? buildDenoImportMap(await genAtaRoot(req.workspace), modelPath)
|
||||
: undefined
|
||||
const servers = lspServersFor({
|
||||
editorLang: req.editorLang,
|
||||
@@ -257,29 +264,40 @@ export async function lintWithLsp(req: {
|
||||
)
|
||||
}
|
||||
|
||||
// A server is done when it has answered the diagnostic request. If it answers with
|
||||
// nothing it may be a publisher instead, so give its push a bounded chance rather
|
||||
// than concluding the document is clean.
|
||||
const pulled = await withDeadline(
|
||||
Promise.all(
|
||||
started.map(async (s) => {
|
||||
const items = await pullDiagnostics(s, uri)
|
||||
if (items.length === 0) {
|
||||
await withDeadline(s.pushedDiagnostics, PUSH_FALLBACK_MS, undefined)
|
||||
}
|
||||
return items
|
||||
})
|
||||
).then((r) => r.flat()),
|
||||
timeoutMs,
|
||||
[] as meditor.IMarker[]
|
||||
// Each server is awaited on its own deadline: one that stalls must not discard
|
||||
// what the others already reported, and must be named rather than counted as
|
||||
// having found nothing.
|
||||
const stalled: string[] = []
|
||||
const perServer = await Promise.all(
|
||||
started.map(async (s) => {
|
||||
const items = await withDeadline(
|
||||
(async () => {
|
||||
const pulled = await pullDiagnostics(s, uri)
|
||||
// A server that answers with nothing may be a publisher instead, so give
|
||||
// its push a bounded chance before concluding the document is clean.
|
||||
if (pulled.length === 0) {
|
||||
await withDeadline(s.pushedDiagnostics, PUSH_FALLBACK_MS, undefined)
|
||||
}
|
||||
return pulled
|
||||
})(),
|
||||
timeoutMs,
|
||||
undefined
|
||||
)
|
||||
if (items === undefined) {
|
||||
stalled.push(s.server.name)
|
||||
return []
|
||||
}
|
||||
return items
|
||||
})
|
||||
)
|
||||
|
||||
// Pushed diagnostics have already been turned into markers on the model.
|
||||
const pushedMarkers = meditor.getModelMarkers({ resource: uri })
|
||||
const unavailableServers = servers
|
||||
.filter((s) => !started.some((st) => st.server.name === s.name))
|
||||
.map((s) => s.name)
|
||||
return { ...dedupe([...pushedMarkers, ...pulled]), unavailableServers }
|
||||
const unavailableServers = [
|
||||
...servers.filter((s) => !started.some((st) => st.server.name === s.name)).map((s) => s.name),
|
||||
...stalled
|
||||
]
|
||||
return { ...dedupe([...pushedMarkers, ...perServer.flat()]), unavailableServers }
|
||||
} finally {
|
||||
await Promise.all(started.map(stopClient))
|
||||
model.dispose()
|
||||
|
||||
@@ -11,7 +11,7 @@ export interface LspServerConfig {
|
||||
middleware?: (params: any, token: any, next: any) => any
|
||||
}
|
||||
|
||||
export function lspLanguagesFor(editorLang: string, scriptLang: string | undefined): boolean {
|
||||
export function hasLanguageServers(editorLang: string, scriptLang: string | undefined): boolean {
|
||||
return (
|
||||
(editorLang === 'typescript' && scriptLang === 'deno') ||
|
||||
editorLang === 'python' ||
|
||||
|
||||
@@ -36,6 +36,8 @@ export interface WindmillAtaOptions {
|
||||
modelUri: string
|
||||
absolutePathExtraLibs: Map<string, { dispose: () => void }>
|
||||
isCancelled?: () => boolean
|
||||
/** Whether relative-import files should be materialized as Monaco models. */
|
||||
registerLocalModels?: () => boolean
|
||||
/** Called after a relative-import model is registered, to nudge revalidation. */
|
||||
onLocalFileRegistered?: () => void
|
||||
}
|
||||
@@ -67,13 +69,15 @@ export async function createWindmillAta(
|
||||
opts.absolutePathExtraLibs.get(_path)?.dispose()
|
||||
opts.absolutePathExtraLibs.set(_path, typescriptDefaults.addExtraLib(code, _path))
|
||||
}
|
||||
const localModel = meditor.getModel(nuri)
|
||||
if (localModel) {
|
||||
localModel.setValue(code)
|
||||
} else {
|
||||
meditor.createModel(code, 'typescript', nuri)
|
||||
if (opts.registerLocalModels?.() ?? true) {
|
||||
const localModel = meditor.getModel(nuri)
|
||||
if (localModel) {
|
||||
localModel.setValue(code)
|
||||
} else {
|
||||
meditor.createModel(code, 'typescript', nuri)
|
||||
}
|
||||
opts.onLocalFileRegistered?.()
|
||||
}
|
||||
opts.onLocalFileRegistered?.()
|
||||
}
|
||||
|
||||
await initWasmTs()
|
||||
|
||||
@@ -59,6 +59,6 @@ export function applyCustomWmillTypes(data: CustomWmillTypesData): () => void {
|
||||
}
|
||||
}
|
||||
|
||||
export async function ensureCustomWmillTypes(workspace: string): Promise<() => void> {
|
||||
return applyCustomWmillTypes(await fetchCustomWmillTypesData(workspace))
|
||||
export async function ensureCustomWmillTypes(workspace: string): Promise<void> {
|
||||
applyCustomWmillTypes(await fetchCustomWmillTypesData(workspace))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user