EmbeddingSupport API + lib_runner for Embedding/LSP/tool support by lagergren · Pull Request #545 · xtclang/xvm

lagergren · 2026-08-28T08:47:24Z

A review vehicle, not a merge request. Opened on @cpurdy's branch so the design has somewhere to be commented on. @cpurdy / @ggleyzer: retarget, take it over, or close it as you like.

What this reviews

2568d6be4 — LspSupport (compile ×2, run ×2, a Control handle, the TC-xx diagnostic vocabulary), lib_runner as the Container-0 supervisor, and per-run consoles as named native resources.

I rewrote this description because the original one was written against 4fb0c8505 and had gone stale: it described ToolConnector.java (renamed since), called lib_runner a stub, and listed five // TODO GG sites. None of those remain — the branch works.

The findings below came from wiring my XtcEngine embedding to this branch and running existing tests through it, not from reading.

One worth answering before this lands

A standard XDK module cannot run under the runner. runner.x:161 uses BasicResourceProvider, which has no case for Directory or FileStore, so TestFiles (manualTests/src/main/x/files.x, in testModuleNames at manualTests/build.gradle.kts:467) fails with Invalid resource: Key: storage, FileStore. Switching to PassThroughResourceProvider fixes availability but gives up per-run isolation, since every run then resolves to container zero's instances. So neither stock provider is right, and the underlying gap is that runTask(template, repository, consoleId) has no way to say "this run gets its own resources" — the same gap as the UnsupportedOperationException at LspSupport.java:475. Detail in the diff.

Two things here I'd keep

The ErrorListener threaded through every compile/run entry point, and the TC-xx code vocabulary with documented %1/%2 params. A long-running host already owns a diagnostic sink, wants messages as they're produced and correlated with its own request, and wants something to switch on other than message text.

Everything else is inline on the diff, and most of it is small.

lagergren

Field-by-field notes, anchored to the code. The short version: all six non-final fields are written exactly once, so the class has no genuinely re-assignable state - and the three that are read without the lock are exactly the three that are logically write-once. Making them final/immutable is the same fix as the thread-safety gap, not a separate style preference.

Happy to push a commit for any of this, or leave it entirely to you - it's your branch.

ggleyzer · 2026-09-04T20:40:48Z

I think it's ready for a formal review now; un-drafting it

Analysis only. Nothing raised on PR #545, nothing committed to cpurdy/LSPAPI, and the ../lspapi checkout is unmodified - the hardening items are written to be taken up as a structured review or a sub-branch. Their single idea is to move container creation out of Java and into a long-lived Ecstasy runner app, where TaskRegistry and Task are services so the language provides the serialization rather than Java locking. It was impossible before for three reasons, each removed by one change: MainContainer could not be called into with a result (now invokeAsync), a run could not be given its own console (now xExternalConsole plus concurrent, dynamically mutable resource maps), and a run could not be given its own repository (now carried on the xCoreRepository handle). The old manualTests runner.x created every container at once from a single fiber with no lifecycle at all, which is where the startup races come from. It was a fixture, not a design. The compiler is intended to stay a containerless Java API - LspSupport.compile never touches the connector, and LspCompiler subclasses the real CLI compiler so the two paths cannot drift. Container zero is only for runs. For XtcEngine that means dropping NestedContainer.createForHost, which exists in neither master nor LSPAPI and is this branch's own invention, booting the runner app, posting runTask and returning a Control. It should not adopt their singleton. Concurrency: the class javadoc claims thread safety that the code does not provide - four plain fields written under a static lock and read in six places without it - but that is latent in their supported single-threaded scenario, since only the calling thread reads them. The supported scenario has a different and active defect: TaskRegistry.tasks is never pruned and Task.container is never cleared, so consecutive runs in a hot VM accumulate a task and a container each, forever. That is independently the same shape as T15 on this branch's compile side. Also records seven hardening items, the largest being to collapse the four configuration fields into one immutable record behind a volatile so that "half-configured" stops being a representable state. Two of my own first readings were wrong and are marked where corrected: the auto-configure path is safe because DirRepository defines value equality, and InterpreterControl's non-volatile taskId/consoleId are safely published by the executor submission in watch().

…able H11 was the what-is-not-a-smell appendix sitting last but reading out of sequence; it becomes H15. H3b was described in the text but missing from the summary table. Restates at the head of the table that these are recorded in this document only - nothing raised on PR #545, nothing committed to cpurdy/LSPAPI.

Three things had gone stale now that the migration has been carried out. The status header said analysis only, which was true when written and is no longer: nothing has been raised on PR #545 and their checkout is still unmodified, but the migration itself has landed in this branch, and four findings - H5, H19, H20, H21 - were found by doing it rather than by reading, which is why they carry test evidence. Part 3 read as a prescription for what XtcEngine has to become; it has become it, so it is marked as history pointing at the outcome. Part 6 read as a plan. It now opens with a status ledger recording what landed, what was adapted and why, and what did not: Control as the run handle and waitForTask instead of polling are not done, and per-run injections are blocked on H19. Test state is stated there too - 672 tests, 2 failing, both left red because they report upstream defects rather than local breakage.

A running order for one session, with every line reference verified against 2568d6b. The disposition rule is that only uncontroversial self-contained changes get committed directly, anything that changes a design decision goes in a sub-branch so it can be accepted or rejected as a unit, and master defects do not belong in the PR at all. Ordered so the expensive conversations happen first: H19, a standard XDK module cannot run under the runner, then H21, container zero caching op-info across runs, then H5's registry retention, then the configuration and console sub-branches, then the mechanical commits. Includes what to say in the framing, because two things change the tone of the whole review: the singleton is load-bearing rather than lazy, since 144 templates in master carry a mutable static INSTANCE and the runtime therefore cannot host two connectors, and the significant findings came from wiring the engine to the branch rather than from reading it. Also lists which tests to ask for in priority order, starting with any automated test at all, and what to answer if asked whether to take this branch's engine instead - no, take their runner model, and lift back only the narrow pieces that are genuinely better here.

Captures the state a fresh session needs: which repo and branch, that ../lspapi is read-only and currently unmodified, what has already been migrated, and that the two failing tests are failing deliberately because they report upstream defects and must not be weakened. Points at the playbook for disposition and the analysis for evidence, and says not to re-derive the findings, since several contain corrections of earlier wrong readings that should not be reintroduced.

The remaining PR #545 findings mostly do not apply here - runner.x already declares `public/private Boolean running`, invokeAsync is already typed, and there is no EmbeddingSupport, LspSupport or LspTest in this branch, nor any static mutable state in org.xvm.api. But auditing for the same hazard SHAPE rather than the same file found it in our own equivalent, which is the point of doing the audit. InterpreterConnector - whose javadoc already records that it was lifted from the LSPAPI branch - carried the identical pattern: m_containerMain, m_containerLast and m_fStarted, all non-volatile, in a class with zero synchronized. The writers are the caller's thread (loadModule, start, join); the readers include runtime threads via getMainContainer() and diagnosticContainer(). With no happens-before edge, a reader can see a started connector that still looks unstarted, or - worse, because it fails somewhere else entirely - a container that join() has already cleared. All three are volatile now, and the write ORDER is documented because it is what makes the pairing correct: start() sets the container before the flag, so a reader observing m_fStarted is guaranteed to see the container it implies. Same shape as the shutdown future's publication. :javatools:test green: 712 tests, 0 failures.

…ol work like xunit

lagergren · 2026-09-11T11:19:47Z

Approved. This is definitely enough for me to take it from here now, merge whenever you feel like it.

lagergren commented Aug 28, 2026

View reviewed changes

lagergren assigned cpurdy Aug 28, 2026

lagergren requested a review from cpurdy August 28, 2026 08:58

ggleyzer force-pushed the cpurdy/LSPAPI branch 5 times, most recently from fa0daa7 to 2568d6b Compare September 3, 2026 19:47