Skip to content

feat(lsp): polish — graceful shutdown, log level, upstream error-format canaries - #10

Merged
klaidliadon merged 6 commits into
masterfrom
ridl-lsp-hardening/polish
Jun 20, 2026
Merged

klaidliadon merged 6 commits into
masterfrom
ridl-lsp-hardening/polish

Conversation

@klaidliadon

@klaidliadon klaidliadon commented Jun 20, 2026 •

Copy link
Copy Markdown
Collaborator

Final hardening layer: a supervised/containerized server now shuts down cleanly on a signal and reports a spec-correct exit code, logging is tunable for field debugging, and the fragile string-coupling to the upstream parser's error wording is pinned so an upstream bump fails CI loudly instead of silently breaking diagnostics.

Third of the prod-readiness stack. Depends on #8 and #9 — merge those first.

⚠️ Stack: #8 → #9 → this PR. Do not merge before #9.


Changes (audit findings)

  • I4 — pin upstream error-format coupling (ridl/parser.go, tests): diagnostics depend on the exact wording/shape of upstream webrpc parser errors (a schema error: version is required… substring match, and a line:col: regex). There's no typed-error alternative upstream, so a wording change would silently break import handling and collapse diagnostic positions. Named the matched substrings as constants and added canaries (TestVersionRequiredErrorFormat, TestUpstreamErrorFormatCanary) that fail CI on drift.
  • S1 — graceful shutdown (cmd/ridl-lsp/main.go, lsp/server.go): derive the root context from signal.NotifyContext(SIGINT, SIGTERM) so the Docker ENTRYPOINT shuts down cleanly; the exit handler reports the LSP-spec exit code (0 if shutdown was received, else 1). Lifecycle methods are dispatched synchronously so the exit code can't lose a race with the transport EOF (see review note).
  • S2 — configurable log level (cmd/ridl-lsp/main.go): RIDL_LSP_LOG_LEVEL (debug/info/warn/error); an invalid value is reported and ignored rather than failing startup.
  • chore (S3/S4/S5): overlayContents snapshots docs.All() once; memFileInfo.ModTime returns a zero time (deterministic, parser ignores it); git describe falls back to --always/dev and Docker defaults VERSION=dev for untagged builds.

Test plan

$ go test -race ./...
Go test: 139 passed in 6 packages

$ go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint run ./... -c .golangci.yml
0 issues.

New tests: error-format canaries (parser_test.go, upstream_error_format_test.go), exit-code contract (lifecycle_test.go unit + exit_e2e_test.go subprocess), log-level parsing (main_test.go).

Review

Self-review caught an Important issue: the first exit-code cut exited non-zero on any stream close without shutdown — wrong, because editors routinely tear down by just closing the pipe. Reworked so the exit code is owned by the exit notification. A Codex adversarial pass (via agent-comms, 2 rounds) then reproduced a subtler race — AsyncHandler runs exit in a goroutine that loses to EOF on conn.Done() — fixed by dispatching shutdown+exit synchronously in the read loop, with an end-to-end subprocess test. Security-review: no findings.


Stack: ridl-lsp-hardening

  1. fix(lsp): crash-safety — panic recovery, layout canary, race+lint CI #8
  2. perf(lsp): parse-pipeline hardening — immutable docs, partial-result cache, cancellation, workspace pruning #9
  3. feat(lsp): polish — graceful shutdown, log level, upstream error-format canaries #10 ◀ this PR

This stack is managed with sdf.

@klaidliadon
klaidliadon force-pushed the ridl-lsp-hardening/parse-pipeline branch from 5c318c4 to 855168c Compare June 20, 2026 07:05
@klaidliadon
klaidliadon force-pushed the ridl-lsp-hardening/polish branch from 7940cb5 to 28a9590 Compare June 20, 2026 07:05
Base automatically changed from ridl-lsp-hardening/parse-pipeline to master June 20, 2026 07:09
Diagnostics depend on the exact wording/shape of upstream webrpc parser errors: isVersionOptionalSchemaError substring-matches the schema-validation message, and errorToDiagnostic regex-extracts line:col from the error text. There is no typed-error alternative upstream, so an upstream wording change would silently break import handling and collapse diagnostic positions to a line-1 smear (audit I4).

- Name the matched substrings as constants documenting the coupling.

- TestVersionRequiredErrorFormat pins the version-required message; TestUpstreamErrorFormatCanary pins that a positioned error still yields a line:col prefix errorToDiagnostic can parse. Either drifting fails CI.
main used context.Background() with no signal handling, and Shutdown/Exit were no-ops (audit S1). A supervised or containerized server (the Docker ENTRYPOINT) had no clean shutdown path, and the process always exited 0 regardless of protocol state.

- Derive the root context from signal.NotifyContext(SIGINT, SIGTERM) and close the connection on signal.

- Track whether shutdown was received; on a client-closed stream, exit non-zero if it was not (LSP spec), exit 0 after a signal.
Logging was hardcoded to zap production (info), with no way to quiet or to raise verbosity for field debugging (audit S2). Read the level from RIDL_LSP_LOG_LEVEL (debug/info/warn/error); an invalid value is reported and ignored rather than failing startup.
- overlayContents called docs.All() twice (double lock + alloc); call it once (audit S3).

- memFileInfo.ModTime returned time.Now(), making in-memory overlay metadata non-deterministic; return a zero time, since only content is read (audit S4).

- git describe --tags failed builds outside a tagged checkout (shallow clone, fork); fall back to --always then 'dev', and default Docker VERSION to dev (audit S5).
Self-review caught that the first S1 cut exited non-zero whenever the stream closed without a prior shutdown — but editors routinely tear down a stdio server by just closing the pipe (no shutdown/exit), so that flagged every normal close as an error in supervisor/CI logs.

Per the LSP spec the exit-code rule belongs to the exit notification: the Exit handler now exits 0 if shutdown was received, else 1 (via an injectable exit func so the contract is unit-tested). A bare stream close or an OS signal exits 0.
Codex review (reproduced via subprocess) found the exit code was racy: protocol.Handlers runs every request through AsyncHandler in its own goroutine, so on 'exit' + immediate EOF, conn.Done() could return from main (exit 0) before the async Exit handler reached os.Exit(1). Dispatching only exit synchronously would still race an async 'shutdown' setting its flag.

Dispatch both shutdown and exit synchronously in the read loop, in arrival order: shutdown sets the flag, then exit reads it and terminates — all before the trailing EOF. Add an end-to-end subprocess test asserting exit-without-shutdown=1 and shutdown-then-exit=0.
@klaidliadon
klaidliadon force-pushed the ridl-lsp-hardening/polish branch from 28a9590 to d558be9 Compare June 20, 2026 07:09
@klaidliadon
klaidliadon merged commit 7b00890 into master Jun 20, 2026
1 check passed
@klaidliadon
klaidliadon deleted the ridl-lsp-hardening/polish branch June 20, 2026 07:12
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