TML-2984: surface config-load failures on the config URI; retain the last-good project on reload failure#974
TML-2984: surface config-load failures on the config URI; retain the last-good project on reload failure#974SevInf wants to merge 14 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe language server preserves the last successfully loaded project during failed config reloads, publishes config-load diagnostics, clears them after recovery or project removal, and tests concurrent failures, pull clients, cleanup, and superseded loads. ChangesConfig failure recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigWatcher
participant LanguageServer
participant LSPClient
ConfigWatcher->>LanguageServer: startProjectLoad
LanguageServer->>LanguageServer: retain lastGood project state
LanguageServer->>LSPClient: publish config-load-failure diagnostic
LanguageServer->>LanguageServer: restore lastGood project
LanguageServer->>LSPClient: publish retained project diagnostics
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
size-limit report 📦
|
@prisma-next/extension-author-tools
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/middleware-cache
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/extension-postgis
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/extension-supabase
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/cli-telemetry
@prisma-next/config-loader
@prisma-next/emitter
@prisma-next/language-server
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/1-framework/3-tooling/language-server/src/server.ts (1)
328-345: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPrevent superseded load failures from clearing the current project entry.
resolveProjectIfLoadable()unconditionally callsstopManagingProject(configPath)on any rejection, so a stale load failure can delete a newermanagedProjectsentry that already replaced it. Gate that cleanup on the rejected promise still being the current load, and add a test assertion that the replacement project survives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/language-server/src/server.ts` around lines 328 - 345, Update resolveProjectIfLoadable’s rejection cleanup so stopManagingProject(configPath) runs only when the rejected promise still matches the current managedProjects loading entry. Preserve the replacement entry when a superseded load fails, and add a test assertion confirming the newer project remains managed.
🧹 Nitpick comments (1)
packages/1-framework/3-tooling/language-server/src/server.ts (1)
86-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider exporting
CONFIG_LOAD_FAILED_CODE.The test file hardcodes the literal
'PRISMA_NEXT_CONFIG_LOAD_FAILED'instead of importing this constant, so the two can drift silently if this value ever changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/language-server/src/server.ts` around lines 86 - 89, Export CONFIG_LOAD_FAILED_CODE from server.ts so tests and other consumers can import the single source of truth instead of hardcoding the error code literal. Keep its current value unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/1-framework/3-tooling/language-server/src/server.ts`:
- Around line 78-83: Clarify the `lastGood` documentation to state that a failed
first load does not publish or serve a project state and leaves documents
unmanaged, while still publishing the config-failure diagnostic. Replace the
ambiguous “publishes nothing” wording without changing the
`publishConfigFailure` behavior or load handling.
- Around line 228-269: The failure branch in startProjectLoad must not restore
lastGood when hasManagedDocuments(configPath) is false. After
publishConfigFailure, delete the managedProjects entry when no documents remain;
only restore lastGood and return it when managed documents still exist,
preventing stale project resurrection and repeated diagnostics.
---
Outside diff comments:
In `@packages/1-framework/3-tooling/language-server/src/server.ts`:
- Around line 328-345: Update resolveProjectIfLoadable’s rejection cleanup so
stopManagingProject(configPath) runs only when the rejected promise still
matches the current managedProjects loading entry. Preserve the replacement
entry when a superseded load fails, and add a test assertion confirming the
newer project remains managed.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/language-server/src/server.ts`:
- Around line 86-89: Export CONFIG_LOAD_FAILED_CODE from server.ts so tests and
other consumers can import the single source of truth instead of hardcoding the
error code literal. Keep its current value unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 85eab191-8cf1-4d35-88f4-3195c162051e
⛔ Files ignored due to path filters (4)
projects/lsp-interpreter-diagnostics/plans/plan.mdis excluded by!projects/**projects/lsp-interpreter-diagnostics/slices/06-config-failure/briefs/d1-r1.mdis excluded by!projects/**projects/lsp-interpreter-diagnostics/slices/06-config-failure/spec.mdis excluded by!projects/**projects/lsp-interpreter-diagnostics/trace.jsonlis excluded by!projects/**
📒 Files selected for processing (2)
packages/1-framework/3-tooling/language-server/src/server.tspackages/1-framework/3-tooling/language-server/test/server.test.ts
1bbb66b to
f399fd6
Compare
| const hadLoadedProject = | ||
| existing?.status === 'loaded' || | ||
| (existing?.status === 'loading' && existing.hadLoadedProject); | ||
| const lastGood = |
There was a problem hiding this comment.
Holy mother of clean code, 3 levels of nested ternaries. Are you proud of yourself?
Looks like you need good old helper function for both lastGood and configFiled
…etention) Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
A failed project load publishes one diagnostic on the config-file URI at document start (push channel unconditionally -- the config file belongs to the TypeScript language service, so pull never reaches us), cleared on the next successful load or when the project is dropped for document-lifecycle reasons. The loading entry now carries the previous ProjectState: a broken reload restores it so open schema documents keep their parse, symbol-table, and interpreter diagnostics, while a failed first load keeps todays no-project behavior. Superseded loads stay silent. Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
Review response: the parallel URI map dissolves into entry state -- a failed first load records a `failed` entry (absence-like for project resolution, memory of the published marker), the retention case carries configFailed on the loaded entry, and the in-flight entry threads the bit so chained loads clear correctly. A failure that lands after the last document closed deletes the entry outright: no resurrection, no zombie marker. The failed-load funnels now unmanage documents only when the failure is the config's current state, so a stale rejection cannot touch a newer entry. CONFIG_LOAD_FAILED_CODE is exported for tests. Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
…arker) Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
…and-forget Review-settled simplification: LSP clients keep per-server diagnostic state, so empty publishes are harmless and the configFailed bookkeeping bought nothing the failed status does not already provide. Every current-load success and every settled-entry drop now publishes an unconditional clear on the config URI. Publish-counting pins re-pin to non-empty publish counts; retention pins wait for the non-empty marker specifically. Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
…et clears) Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
Review response: the last-good derivation reads as a plain function over the three entry states, reused by the hadProject check in stopManagingProject. Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
c1987f9 to
843eca9
Compare
| } catch { | ||
| stopManagingProject(configPath); | ||
| // Unmanage documents only when the failure is the config's current | ||
| // state — a stale load's rejection must not touch a newer entry. |
There was a problem hiding this comment.
Stale load rejection can not touch a newer entry because all loads get the queued
There was a problem hiding this comment.
Thanks for writing entire novel. If what you say is true then the queue is incorrectly implemented. Rejection of load N must not be handled after either success or failure of load N+1
…trim narration Review response: the currency gate stays -- load queuing orders execution, but a superseded load rejection is delivered to awaiters that grabbed the old promise before the synchronous entry replacement, so an ungated funnel would delete the newer entry mid-flight. The gate comment now carries that interleaving. Narrating and usage-enumerating comment fragments trimmed. Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
Per-review comment pass: variant enumeration, next-line narration, and condition restatements removed from server.ts; test comments trimmed to the non-obvious rationale by the parallel pass. The lastGood member doc keeps its review-requested sentence. Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
…ome pure readers Review-settled restructure: the load chain failure handler -- already ordered strictly before any successor load by the queue -- now owns first-load-failure unmanagement alongside the marker publish, so the awaiter catches carry no side effects and no currency guards. stopManagingProject folds into an unmanageDocuments helper; the per-read association drops on failure disappear with it. The superseded-failure survivor scenario is now deterministic: the newer load result serves directly, pinned at exactly two resolutions. Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
Slice 06 of the lsp-interpreter-diagnostics project (TML-2984, follows #939/#948/#971/#972) — the final build slice: config failures become visible and non-destructive.
Changes
language-server/src/server.ts): any throw from the project-load flow (loadConfig,createControlStack, input resolution — opaque runtime errors from executed TypeScript) surfaces as one diagnostic on the config-file URI at (0,0)–(0,1) (PRISMA_NEXT_CONFIG_LOAD_FAILED, message carries the error text). Published via push unconditionally — the config file belongs to the TypeScript language service, so pull never reaches this server for it (tsserverconfigFileDiagprecedent). Exactly-once per failed load is structural (the diagnostic publishes from the single shared load promise, guarded byisCurrentLoadso superseded loads stay silent). Cleared on the next successful load and when the last document under the config closes.ProjectState; a failed reload resolves with the retained project — schema documents keep serving their full diagnostics (parse, symbol-table, and interpreter findings through the retained context) with the config diagnostic alongside, instead of today's wipe-everything (rust-analyzerswitch_workspacesprecedent: only switch to a broken workspace if you have none at all). A failed first load keeps today's behavior exactly.Why
Before this slice, breaking your config while editing meant every schema diagnostic silently vanished — the server dropped the project and cleared the markers, leaving no clue why. Now the editor tells you two things at once: the config is broken (marker on the config file itself, position (0,0) since no spans exist into executed TS) and your schema work continues against the last working configuration. One deliberate re-pin: the test that encoded destroy-on-reload was rewritten to encode retention — that behavior change is this slice's entire charter.
One residual, by design of the lazy load lifecycle: after a failed first load with no schema documents open, the config marker persists until a schema document reopens (no load fires for a config with no open inputs). Self-heals with any document open; exercised in the upcoming playground QA.
Project spec + plan live under
projects/lsp-interpreter-diagnostics/(deleted at project close-out).Summary by CodeRabbit
Bug Fixes
PRISMA_NEXT_CONFIG_LOAD_FAILEDconfiguration-load-failure diagnostics exactly once per failing config, with deduplication across concurrent loads and suppression for superseded failures.Tests