Survive a build that does not have every plug-in - #2179
Open
jcelerier wants to merge 27 commits into
Open
Conversation
jcelerier
force-pushed
the
stack/plugin-robustness
branch
2 times, most recently
from
August 9, 2026 12:37
a4c2303 to
ccea1df
Compare
jcelerier
marked this pull request as ready for review
August 9, 2026 19:33
jcelerier
force-pushed
the
stack/plugin-robustness
branch
2 times, most recently
from
August 11, 2026 18:26
53df11c to
df7fd30
Compare
Adds a test suite recording what score currently does when a document
references a process or protocol the running build does not have.
This is routine rather than exotic: protocols and processes are
registered conditionally inside plug-ins that ship everywhere, so Syphon
and Spout are both compiled into score-plugin-gfx under #if, and a macOS
document opened on Windows names a protocol that build cannot make.
The suite is a baseline, not a specification -- it asserts today's
behaviour, which is wrong in several places, so that the fixes can be
written against measured facts rather than assumptions. It records that:
- DeviceSettings desyncs the DataStream when the protocol is absent:
the payload is written inline and undelimited, so a reader without
the factory cannot skip it and lands mid-payload on checkDelimiter.
- The JSON path instead drops the payload silently, so a round-trip
through a build lacking the protocol destroys the device settings.
- Polymorphic payloads (processes, ports) *are* length-delimited, so
preserving an unknown object verbatim is possible there.
- There is no fallback for missing layer, process or port factories.
- checkAndUpdateJson cannot see a factory missing inside a plugin that
is present, so it reports such a document as fully loadable.
Protocols are registered conditionally inside plug-ins that ship
everywhere: Syphon and Spout are both compiled into score-plugin-gfx
under #if. So a document authored on macOS routinely names a protocol a
Windows build cannot instantiate, and the plugin-level check in
checkAndUpdateJson cannot even see it -- score_plugin_gfx is present on
both.
What happened then depended on the format, and both outcomes were bad:
- JSON dropped the protocol-specific members while reading and wrote
none back out. Opening and re-saving a document on a machine without
the protocol replaced the device with a husk carrying the right name
and UUID and no configuration at all, silently, for everyone.
- DataStream skipped the payload but still expected the trailing
delimiter, so it landed mid-payload, SIGTRAPped through
SCORE_BREAKPOINT and then blamed the file for being corrupt.
JSON now keeps the members verbatim in DeviceSettings::opaqueSettings
and writes them back unchanged, so a document survives a round-trip
through a build that lacks the protocol and still works on one that has
it. This is retroactive: the payload is already stored as flat siblings
of Name and Protocol, so existing files are preserved with no format
change.
The binary format cannot be fixed the same way -- the payload is inline
with no length prefix and .scorebin carries no version to migrate from
-- so it now reports which device and protocol are involved and points
at .score, instead of trapping. The symmetric case, where the writer had
no factory either and wrote no payload, is detected and still round-trips
untouched; only an actually-unreadable payload fails.
Both replication policies deserialized and instantiated commands coming
off the socket with no error handling at all:
score::CommandData cmd;
DataStreamWriter writer{m.data};
writer.writeTo(cmd);
stack.redoAndPushQuiet(m_ctx.app.instantiateUndoCommand(cmd));
Peers do not necessarily run the same build -- Syphon exists only on
macOS, Spout only on Windows, and both are compiled into a plug-in that
ships everywhere -- so a peer can legitimately send a command we cannot
read or cannot instantiate. Every one of those took the process down:
instantiateUndoCommand aborts in debug and throws in release, and the
throw escaped a Qt signal handler. This was not client-only; the master
had the identical handler, so a client adding a Spout device killed the
host mid-show.
Commands from peers now go through applyRemoteCommand, which reports
rather than aborts. instantiateUndoCommandIfAvailable is the checked
counterpart of instantiateUndoCommand: the abort is right for a local
programming error and wrong for untrusted input from the network.
Failing to apply is not recoverable by ignoring it. The two ends are
then editing different documents, and since paths are resolved by
position, later commands would silently retarget the wrong objects. So a
client that cannot apply a relayed command marks itself diverged and
stops applying anything further, including undo/redo and index moves,
until the session is rejoined.
The master's failure is a different situation: it declines the command
and does not relay it, so it and every other client stay consistent and
only the sender is ahead. It therefore marks nothing and replies
/command/rejected, on which the sender marks itself diverged.
Full-document resync is deliberately not attempted here: it means
hot-swapping a live document under an open presenter, panels, selection
and undo stack, and no such path exists.
Processes come from plug-ins that are not the same everywhere: VST and
LV2 do not exist in the wasm build, JIT needs x86_64, and several are
compiled in conditionally even on desktop. Until now a process whose
factory was absent could not be loaded at all -- deserialize_interface
called loadMissing, and all four implementations of it were
"SCORE_TODO; return nullptr;". The process was silently dropped, and
saving from that machine wrote the document back out without it.
That makes a document unsafe to open anywhere it is not fully
understood, which is precisely what a shared or remotely-edited document
has to survive. OpaqueProcessModel stands in for the absent process and
holds the two things the plain base cannot:
- the concrete key of what it replaces, returned from concreteKey().
Forging that identity is the point: saving writes the original UUID,
so the file still names the real process and it comes back intact on
a machine that has the plug-in.
- the plug-in's own serialized data, re-emitted untouched.
Ports are pulled out of the payload and rebuilt as real ports rather
than kept as bytes, so cables to the process still resolve and its
controls still hold and report values. That only works in JSON, where
they sit under known keys; the binary format writes them at a
process-specific offset with nothing to locate them by, so a binary
payload is kept whole and the stand-in has no ports.
Telling our members from the plug-in's is by name, which can drift if
ProcessModel gains one, so a test serializes a real process and checks
the list still matches.
deserialize_interface now hands the concrete key to loadMissing; without
it a stand-in cannot know what it is standing in for. All five
implementations take it, though only the process one uses it so far.
LayerFactoryList grows an explicit fallback tier: findDefaultFactory
returned the first factory whose matches() accepted, iterating an
unordered map, so a catch-all entry would have shadowed real factories at
random. Fallbacks are now skipped during matching and used only when
nothing claimed the process.
An OpaqueProcessModel has to be displayable: the interval presenters build header and footer delegates from findDefaultFactory without checking the result, and LayerData asserts on it. The obvious fix -- a catch-all layer factory -- is wrong twice over. findDefaultFactory returns the first factory whose matches() accepts while iterating an unordered map, so a catch-all would shadow real factories at random. And plenty of ordinary processes have no layer factory at all and resolve to nullptr today; handing them a default layer would start drawing them in slots where they are deliberately not drawn. So the fallback never takes part in matching, and is reached only through the overload that has the process itself, and only when that process is a stand-in. The key overload keeps returning nullptr exactly as before. The four sites that dereference the result unchecked now pass the process rather than its key, which is what makes that possible.
VST and LV2 bring their own control port types along with the process itself, so a build without them meets unknown ports as well as unknown processes. Reconstructing such a process aborted outright: writePorts passed SCORE_ABORT as the failure path of ArrayEntitySerializer. That made the stand-in process added in the previous commit useless for exactly the case it was written for. Ports are now read by writePorts itself rather than through deserialize_interface. That is what makes a stand-in possible at all: the generic path cannot tell an inlet from an outlet, since only the caller knows which of the two arrays it is filling, so the direction is supplied as a template argument here. OpaqueInlet and OpaqueOutlet keep the key of the port they replace and the plug-in's data verbatim, like OpaqueProcessModel. They also keep the port's id, which matters more than it looks: ids are what cables resolve against, so a stand-in that renumbered its port would silently break every cable pointing at it. A port that cannot be read at all is now dropped with a warning rather than taking the process down.
checkAndUpdateJson refused any document listing a plug-in this build lacks. That guarantee was never real: the check works on plug-in keys, and factories are registered conditionally *inside* plug-ins that ship everywhere, so a macOS document using Syphon passed it happily on Windows -- score_plugin_gfx is present on both. It rejected the cases it could see and waved through the ones that actually happen. Meanwhile it made a document unopenable on any machine missing any plug-in it mentions, which is every machine once builds differ by platform, and unopenable is the one outcome from which nothing can be recovered. Now that a process, its ports and a device all survive an absent factory by keeping their data verbatim, the document can be opened instead. The caller is told which plug-ins are missing so it can say so. A plug-in *older* than the file's is still refused: there the factory is found, and would read data written in a format it does not know. Not yet covered: a document plug-in whose factory is missing is still dropped rather than preserved. Only score-plugin-js, deviceexplorer and the network addon provide those.
Unlike a whole process, a port can be recovered from a binary payload: deserialize_interface gives each polymorphic object its own length-delimited blob, so the tail after the key is exactly this port's data and nothing else. The test patches the factory key inside a serialized port in place -- the blob is [length][16-byte key][data] -- which is what a build that does have the plug-in would have written, and checks the stand-in keeps the id and writes the bytes back unchanged. An unknown *process* remains unrecoverable in binary: its ports sit at an offset each process picks for itself, with nothing to locate them by, so the payload is kept whole and the stand-in has no ports. Fixing that means moving port serialization into the ProcessModel base so it lands at a known offset, which changes the format for every existing file -- and .scorebin carries no version to migrate from.
The WebSocket API sets any device parameter, drives transport, and
evaluates arbitrary JavaScript through its Console message. It had no
authentication of any kind, a port fixed at 10212, and bound to
QHostAddress::Any -- so on any machine with score open, anyone on the
network could take it over. The Console message alone is remote code
execution as whoever runs score.
Worse, the Enabled setting did not gate the socket at all. The server
listened from the Receiver constructor and was never closed; Enabled
only decided whether the interval tree was published. Turning remote
control off left the port open.
Now:
- The socket is opened and closed with the setting, and reopened when
any of the settings that shape it change.
- It binds loopback only unless deliberately opened to the network.
- Clients present a token, generated on first use so that "no
password" is not a state reachable by leaving defaults alone. An
empty token serves nobody rather than serving everybody.
- Scripting is refused unless explicitly allowed, separately from
being reachable at all: it grants control of the machine, not of the
score.
The token rides on the connection URL rather than in a handshake
message. A browser cannot set headers on a WebSocket, and the shipped
HTML remote takes its endpoint from a text field, so it keeps working by
pasting a different address rather than needing new code.
The fixtures target is defined by tests/fixtures, which is processed after src/, so a test declared from a plug-in or an add-on could not link it and failed to find score_test/App.hpp. It is header-only, so hand those the include path directly and let the tests declared later keep linking the target.
Devices with asyncConnect open a modal dialog, disable the main window and wait for connectionChanged with no bound on how long. A device that never answers -- an OSCQuery server that is not up, a MIDI port that disappeared -- held the application there for as long as it ran, with only a Cancel button and nobody necessarily present to press it. It is reachable unattended: --ui-debug runs with a GUI. Nothing downstream tells "gave up" apart from "cancelled": both leave the device unconnected, which score already handles.
score already wrote "<PROJECT>:" and "<LIBRARY>:" into save files, but
resolving and relativizing them were two functions that had to be kept
agreeing by hand, and one of them was wrong.
Deciding whether a file sits inside the project folder was a prefix
test:
if(!docpath.isEmpty() && path.startsWith(docpath))
So a document at /a/proj referring to /a/proj2/sound.wav was told the
file was inside its own folder, and stored "<PROJECT>:2/sound.wav" --
which resolves to /a/proj/2/sound.wav. A sibling directory whose name
merely starts the same way silently rewrote the path to a different
file. The same test was used for the library root.
score::Uri puts the two directions in one place, with containment that
requires a component boundary rather than a shared prefix, and that
compares without regard to case where the filesystem does -- on Windows
and macOS the old comparison could fail to notice a file was inside the
project at all and store an absolute path instead.
Adds "<CACHE>:" for content-addressed media: a name derived from the
file's contents rather than its location, so the same media is the same
entry on every machine that has it and a machine that lacks it can be
told exactly what to fetch. Nothing writes those yet.
locateFilePath and relativizeFilePath keep their signatures and are now
thin wrappers, so all 53 call sites get the fix without touching them.
Asking for a file is not the same everywhere. The browser has no synchronous dialog and no filesystem to name, so the bytes arrive later and have to be written somewhere the rest of score can open by path. One call site had worked that out and grown a #ifdef'd helper of its own, buried in ControlWidgets.hpp; every other one called QFileDialog::getOpenFileName directly and returned nothing there. That helper moves to score/widgets/FileDialog.hpp, gains a plural form, and the callers that were silently broken now use it: the image list chooser could not add images at all in the wasm build. Directories are a different matter, and pretending otherwise is what the old code did. There is nothing to point at when files arrive one at a time from a picker, so selectExistingDirectory says so instead of returning an empty string that reads as a cancellation and leaves the caller waiting. DocumentManager keeps its own dialogs for now: its save path returns a bool that the quit sequence uses to decide whether to abort, so making it asynchronous means restructuring close, save-on-close and crash-restore, which is worth doing on its own rather than in passing.
Document plug-ins carry whole subsystems' worth of state -- the network add-on keeps its groups and per-object metadata in one -- and there was nowhere to put that when the plug-in was absent. loadMissing returned nullptr, the load did SCORE_TODO, and saving wrote the document back without it. A session document opened once by a peer without the add-on came back with its groups gone. This was the last of the five polymorphic kinds still dropping data; processes, ports and devices were handled in earlier commits. Their base writes nothing but the key, so in JSON everything else in the object belongs to the plug-in, and in the binary format the whole tail of its blob does. The capture itself is now shared rather than written out three times: score/serialization/OpaquePayload.hpp holds the two shapes preservation takes -- members we do not own in JSON, the remaining bytes in the binary format -- and DeviceSettings, the process and port stand-ins and this one all use it.
The other four polymorphic kinds now keep data they cannot read. A segment does not, and the two SCORE_TODOs left behind made that look like an oversight rather than a decision. A segment is asked for its value at a point, and those answers are played. A stand-in would have to invent them, so what came out would be automation that runs and is wrong, rather than automation that is visibly missing. It also cannot arise: segments come only from score-plugin-curve, so a build without it has no curves to hold them.
Peers in a session mirror each other by exchanging commands, so which ones exist is part of what makes two builds able to work together. The command store was reachable only through instantiateUndoCommand, which answers about one command at a time and only once it is too late to do anything but fail.
A score refers to things that live on a machine -- sound files, shaders, the project folder -- and score has always reached them by opening a path. That assumes the machine running the score is the machine being typed at, which is the assumption this whole line of work is about removing: a score playing on a headless box is edited from a laptop, and score in a browser has no filesystem at all. score::Environment is the asking, separated from the answering. LocalEnvironment is what score has always done, behind it. Every call is asynchronous, including the ones LocalEnvironment answers before returning. Not because a local read is slow, but because a remote one cannot be made synchronous and a browser cannot even open a file picker without returning first: an interface that let callers wait would be one only the local implementation could satisfy. isLocal() exists so that code with a good reason to open a path itself can ask first, rather than calling resolve() and quietly getting an empty string.
Environment existed but nothing handed one out, so every caller still resolved paths as though the score were on this machine. A document now owns one -- local unless something replaces it -- and locateFilePath asks it instead of resolving directly. For a document on this machine nothing changes. For one being edited through a session it returns nothing, which is the truth: there is no local path that leads to those files, and an empty string is better than one that looks plausible and opens nothing. Callers that need the bytes rather than a path ask the environment for them. Migrating the ones that can is separate work; this is what makes it possible to tell which ones those are.
locateFilePath goes through the environment now, and deserialization resolves paths -- a sound process turns its stored <PROJECT>: reference into something it can open while being read. That happens from the constructors, and the environment was created in init(), which every loading constructor runs afterwards. So opening any document with a sound file in it dereferenced a null unique_ptr and made a virtual call through it. docs/main-page.score segfaults on the previous commit. One constructor never calls init() at all, so its documents had no environment for their whole life. Created on demand instead, which does not depend on anyone remembering the order. The test opens that same document, and crashes without this.
It was one QByteArray holding either JSON text or binary bytes, with nothing recording which, and serialize_impl chose a branch from the format being *written*. So a payload read from .score and written to the binary format went out as raw JSON text inside a binary blob, and one read from .scorebin written to JSON failed to parse and was silently dropped -- losing exactly the data this is for. That is not a corner case. A document read from .score is written to the binary format on every autosave, and moving an interval serialises its processes to the binary format and rebuilds them from those bytes (dataStructures.cpp). Dragging a box would have emptied every stand-in inside it, and with the ports gone every cable to them is deleted on reload. OpaquePayload now carries its format and wraps itself when written into the other one, so score's own round-trips are exact in all four directions. What wrapping cannot do is make the plug-in able to read it: a .scorebin saved from a document that came from .score holds the plug-in's JSON inside a binary blob, and only score knows that. Moving a document between machines that differ should use .score, which never needs wrapping. The round-trip test now injects members a plug-in would have written -- it built its input with this build's own serializer and asserted that no data survived no data -- and names them individually afterwards, since rapidjson's operator== ignores member order and cannot see a payload that came back rearranged.
A path names an object by position and name, and nothing guaranteed the object standing there was the type the path was written for. try_find did a static_cast and find a safe_cast -- an abort in debug, a blind cast in release. That is not theoretical now that a build without a plug-in loads its processes and ports as stand-ins: they occupy the same place under the same ids, so a path written for the real type resolves to one. Process::SetControlValue holds a Path<ControlInlet> and lives in a library every build has, so a peer without the plug-in that provided the port receives that command, resolves the path to an OpaqueInlet, and writes a value through a pointer to an object of another type. Nothing threw, so the divergence handling never saw it. try_find returns null, which is what "try" should have meant. find throws, so a command replayed from another peer reports rather than aborts. The cache is only filled when the type matched.
…ase from ~QWebSocket emits disconnected() synchronously, which lands in socketDisconnected and erases from m_clients -- mutating the container being walked, and handing a socket that is being destroyed to handlers that then write to it. This was teardown-only until the socket started being closed whenever a setting changes, which is the point of making remote control switchable at all: turning it off with a client connected takes the same path.
locateFilePath returning empty for a document whose files are elsewhere was a worse answer than the plausible-but-wrong path it replaced. Several callers write the result straight back into the model -- a JS process replaces its root, an image list its paths, Pd its file -- and relativize it again on save. Handing them nothing erased the reference for every machine, including the one that could have resolved it. It returns the stored reference untouched now. Callers that want the bytes should ask the environment, which can fetch them across a session; callers that want a path can ask whether it is local first. Also: a stand-in process declares TimeIndependent. Nothing here knows how to rescale a plug-in's data, so the parent duration changing must not be taken to change it -- without this the interval rewrote a stand-in's duration on every resize while its contents stayed as they were. The processes most often standing in like this, VST and LV2, declare it themselves. And the drift test now counts members across two round-trips. It only checked that every name in baseMemberNames is written by a real process, which catches a removal; a member score *gains* would be captured into the payload and written by the base both, growing the object by one duplicate per save. Comparing values cannot see that, since the first of a duplicate pair reads back correctly.
…annot It defaulted to a tokenless URL and had no onclose or onerror, so a refused connection looked exactly like nothing happening. Also says plainly, where the binary DeviceSettings reader decides whether the writer had the protocol, that testing the next four bytes for the delimiter is a guess: a payload that happens to begin with them reads as having none. Nothing better is available without a format change, and .scorebin has no version field to migrate on.
Three places asked the process factory list for the key a process reports and dereferenced the answer. For a stand-in that key is the absent plug-in's, so the answer is null: opening a document containing one and showing it crashed in LayerData::addView, reading a descriptor to build a tooltip. Found by building for WebAssembly and opening a document there -- the build that is missing plug-ins by construction, and the case this whole line of work is for. The desktop tests loaded a stand-in but never displayed one, and showing it is what an application does immediately afterwards. The test now asks for the interval to be displayed rather than only loading the file. Without that it passed with the crash reinstated, since loading builds the presenter but not the layers.
loadDeviceFromNode keeps the node in the tree when the factory is missing, so the explorer routinely holds device nodes with no DeviceInterface behind them -- a macOS document opened on Windows, or anything at all in the wasm build. Everything that reached for the implementation went through DeviceList::device(), which is a SCORE_ASSERT on the lookup failing. The worst of those is DeviceExplorerModel::data(): it asks the device whether it is connected, so the explorer aborted on repaint. Editing or removing an address aborted too, which means the document could not carry an address for a protocol it was authored with. Use findDevice() and skip the implementation when there is none; the model update still runs, since the document is what has to survive a save. Phase 3's terminal role makes this the normal case rather than an edge one -- it has no device implementations at all. Also make score::question/information/warning require a main window and not merely applicationSettings.gui. An embedder that builds the application without a window leaves gui set, so exec() blocked with nobody to dismiss the box: reporting the missing protocol hung the process. Flipping gui in MinimalApplication instead was tried and is wrong -- it changes which factories get registered, and disarms RegressionNullPresenterSelectionTest, which pins gui-without-presenter as the precondition of the bug it guards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011wXVcUPzvcDTVzLpWJzeqP
jcelerier
force-pushed
the
stack/plugin-robustness
branch
from
August 18, 2026 01:08
df7fd30 to
29ba3a8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The first layer of the remote-editing work, extracted because none of it
is about sessions: it is about opening a document, or applying a command,
that names a plug-in this build does not have. That happens today whenever
a score is exchanged between platforms or builds — protocols and processes
are registered conditionally — and the old behaviour was to drop the
object, or to abort.
kept rather than discarded, and says so (
incomplete()), instead ofsilently becoming an empty object
protocol payloads are length-delimited like every other polymorphic
kind, so a reader without the factory can skip one by length
Path<T>checks that what it found is the type it asked fortests/unit/HeterogeneousBuildTest.cppcovers the round trips: byte-identicalre-serialization, injected foreign members, and cross-format trips.
Verified to contain no reference to any session, peer or network-plugin type.
Draft: the layer above is what has been exercised end to end; this one has
been tested as part of that whole, not built in isolation.