Rebuild the terminal interface around nodes and increase UX - #117
Noah-Tervalon-Nvidia wants to merge 20 commits into
Conversation
A worker reply can be megabytes where a control message is bytes, and a frame over a hop's cap is not a dropped message: bufio.Scanner cannot resync past an over-long line, so the read loop ends and the peer dies silently while the child keeps running. The cap only works if every hop on a path agrees, so declare it once in shared/jsonrpc and use it on the three hops that carry those replies -- the broker's generic worker links, engine-manager's inbound codec, and the terminal client's link to the broker. The purpose-built links keep their own smaller caps, sized to what those workers actually send. Engine-manager is unchanged in value, having already been at 8 MiB. The broker's worker links and the terminal client move up from the 1 MiB default. Signed-off-by: Terve <ntervalon@nvidia.com>
Both front ends need to browse the models an engine can download, and only one of them could: the desktop app carried its own Electron-side model hub, so the terminal interface had no way to offer the same thing. Putting the catalog behind engine:catalog gives both clients one implementation, next to the engine manager that performs the pull. One curated source per engine, because there is no generic GGUF registry worth browsing. Ollama has no public catalog API, so the list is scraped by a developer, reviewed as a diff, committed, and compiled in with go:embed -- serving it touches no network. LM Studio's catalog is the lmstudio-community Hugging Face org, whose repo ids are exactly what lms get accepts, so it is fetched live, cached for six hours, coalesced across concurrent callers, and backed off after a failure so a dead upstream is not re-dialled per call. The catalog is filtered for the platform the models will install on, not the one serving it: MLX quantizations only install on Apple Silicon, so those rows are marked appleOnly and dropped for a non-darwin target, and the reply echoes the platform it filtered for. Signed-off-by: Terve <ntervalon@nvidia.com>
The Add Model browse was assembled in the main process: a committed Ollama list bundled into the Electron bundle, plus a live Hugging Face fetch. The terminal interface could not reach any of it, so the two front ends would have had to maintain separate catalogs of the same models. Replace the module with a thin relay over engine:catalog. What remains here is the boundary work the renderer needs and the backend should not care about: mapping EngineType to the manifest's engine name, translating the platform to GOOS -- Node says win32 where Go says windows -- and normalizing rows, dropping any without a pull-ready id rather than offering a model that cannot be downloaded. A catalog failure returns an empty hub rather than an error. The modal then shows its empty state, which is the honest reading for a user who can do nothing about a dead upstream. The scraper stays in desktop/scripts: it is a TypeScript development tool and only its output crosses into the services tree. Signed-off-by: Terve <ntervalon@nvidia.com>
The Inference Demo's HTTP client was built by its own script into its own tools/ resource directory, which only the desktop app knew about. The terminal interface runs the same demo and resolves everything beside its own executable, so a second location meant it could not find the client at all. Build it into cli-bin with the binaries it sits beside. It is still not a service -- no entry in versions.json, no supervision -- so it carries the services version rather than a component version, and it is named explicitly in the expected-file set so cli-bin stays an exact inventory rather than merely gaining an exemption. Fingerprint what actually goes into a binary while here. Only .go files were hashed, so regenerating an embedded asset -- the engine manager's Ollama catalogue, now -- left the build believing cli-bin was current, and it shipped the previous binary with nothing to indicate the list was stale. The dispatcher's own sources sit outside the services tree and get walked for the same reason. Signed-off-by: Terve <ntervalon@nvidia.com>
A bufio.Scanner is finished after a read error, including an over-long line, which it cannot skip past. Treating that like a frame the client merely failed to parse meant calling Scan again forever: the read loop spun at full speed while the interface went on claiming the service was ready. Name the unrecoverable case so the loop can tell the difference and report the disconnect, and let the supervisor wait for the broker's own teardown before force-killing it -- the workers it is shutting down are the ones holding the files a reset is about to delete. Signed-off-by: Terve <ntervalon@nvidia.com>
Three things every view needs and none of them owned: a table that lays its columns out against the width it was actually given, a status line that expires on its own, and helpers that force a block of content to an exact number of rows. Each exists because the alternative kept going wrong. Columns summed to more than the terminal and the last one was cut; a status line set during a thirty-five second operation vanished six seconds in, leaving a screen indistinguishable from a dropped keypress; and content that rendered one row too many pushed the footer off the bottom. Signed-off-by: Terve <ntervalon@nvidia.com>
A node is the unit an operator reasons about, so the tabs become Nodes, Jobs, Service, Errors, and Logs, and everything specific to one machine hangs off its row rather than living in a tab of its own. The ten tabs this replaces spread one machine's facts across four of them: its engines in one, its models in another, its ports in a third, its cluster standing in a fourth. The five views only exist together -- the shell sizes them, they share its status line and frame budget, and the tab set is the change -- so this lands as one commit. Read in the order the pull request gives, which follows the data rather than the alphabet. Much of it is about claiming no more than the backend guarantees. A proxy port change reports the port actually bound rather than the one requested, because a running engine holding that port wins and the broker resolves it elsewhere. Clearing an error is only offered where clearing sticks -- delete-by-id on the reporting node -- so a peer's entry names the node to clear it from instead of silently reverting on the next sync. The cluster name is labelled as this machine's own. Node presence follows the broker's snapshot rather than a timestamp that never advanced. Every table gets its columns at construction. The broker replays a baseline snapshot as soon as a view subscribes, which can arrive before the first WindowSizeMsg, and bubbles indexes its column slice per row cell -- so rows against a zero-column table panicked rather than rendering empty. Against the unified proxy the two identities are kept apart deliberately. The Service tab's worker row keys on the process, engines.ProxyComponent, because one nvpair-proxy hosts every facade and the broker reports one crash for it; the facade views key on each engine's ComponentName, which is what addressing "ollama-proxy:nodes/list" means. Getting that backwards is silent in both directions, so the invariant is asserted rather than described. Signed-off-by: Terve <ntervalon@nvidia.com>
PAIR carries three numbers and the update check can only use one of them. The published tags are named for the release version in desktop/package.json; the services suite version and this component's own version describe parts of the build. The suite version is currently the larger number, so comparing it against the feed would not merely be wrong but silently wrong -- every check concluding this build is ahead and saying nothing, forever. Rename the symbol to ui.ReleaseVersion to match the vocabulary the release tooling now uses, and stamp it from desktop/package.json in both build paths: services/build.sh and .bat for a tarball install, and the desktop's build-modular-binaries for the copy inside cli-bin. A -X against a symbol path that does not exist fails silently, so both were verified by reading the value back out of the built binary rather than by reading the flag. Signed-off-by: Terve <ntervalon@nvidia.com>
The engine manager gained editable launch settings: the arguments and environment an engine starts with, alongside its server port and the client-facing proxy port. The desktop app can edit all three; the terminal interface could edit neither the arguments nor, on a peer, the ports. "a" on an engine opens its arguments in the notation LAUNCH_TEXT.md defines. Nothing is saved directly. The draft goes to engine:preview-settings, which normalizes the text and reports per-field errors, a port conflict, and whether applying restarts the engine; the commit then carries the settings the preview returned rather than the text that was typed, so what lands is what was checked. A restart is armed and confirmed with y, like the other interruptions. The two port fields move onto the same path. They were writing through engine:set-port and the proxy's own set-port, which meant two writers with no shared revision: the last to finish won and neither could tell it had lost. All three fields are now one revision-checked write, which is also what makes them editable on a peer -- the backend relays it, and whether a particular engine will accept the write is the snapshot's Editable answer rather than a rule kept here that would drift from it. Ports stay honest across the move. A port is a request, not a promise: a running engine outranks the proxy, so the backend binds elsewhere and reports the difference as the effective port. The outcome is read from the snapshot that follows the write rather than from the write's own reply, which only says the request was accepted -- and it is reported once, for a change made here, because those snapshots also arrive when anyone else saves. Signed-off-by: Terve <ntervalon@nvidia.com>
The terminal interface guide still described the screen it replaced, and three claims elsewhere had become false: that the model hub lives in Electron, that the demo is the desktop app's alone, and that per-engine arguments are outside the contract. All three are now the opposite of true. The guide gains the startup-arguments editor and loses its own contradiction -- it documented the Jobs tab's test traffic in one section and denied having any in another. Ports are no longer described as local-only, because they are not, and the note about a port being a request rather than a promise moves into the guide where an operator will meet it. The architecture rule gains the rule worth keeping: the three settings fields are one revision-checked write, and editability is the snapshot's answer rather than something a front end decides for itself. Both halves are there because both were got wrong here first. Signed-off-by: Terve <ntervalon@nvidia.com>
services/build.sh now stages inference-dispatcher beside the components, and the check compares that directory against versions.json exactly -- so it fails on a binary that is deliberately not a component. Name the exception rather than loosening the comparison. The check exists to fail when the build script and the manifest disagree about a component, and a pattern that skipped anything unexpected would stop doing that. One extra declared name keeps a second undeclared binary failing. Signed-off-by: Terve <ntervalon@nvidia.com>
Every colour adapts to the terminal's background, but the text drawn on top of the accent did not: it was a fixed black, which suits the pale blue chosen for a dark terminal and is unreadable on the dark blue chosen for a light one. It applied to the active tab and the selected table row -- the row the operator is looking at, on every tab -- so a light theme made the interface hard to use rather than merely off-colour. Pair the foreground with the background so both flip together, and assert the pairing: a fixed foreground over an adaptive background is legible in exactly one of the two terminals, and which one is decided by a detection this program does not control. Add --appearance for when that detection is wrong. It works by asking the terminal for its background and waiting for an answer; a terminal that does not reply -- common over SSH and inside tmux -- leaves lipgloss assuming dark. auto still detects, light and dark state it outright, and anything else is refused rather than quietly meaning auto, which would strand the one operator who reached for the flag. Signed-off-by: Terve <ntervalon@nvidia.com>
Saving an engine's settings failed with "a request identifier is required" after the check had already passed, which read as the interface calling its own validated change invalid. The identifier is the backend's idempotency key. It records a receipt against it, replays return the original outcome, and a replay carrying different settings is refused -- so a commit without one is rejected outright. Only the commit needs it, which is why the preview succeeded first and the failure arrived at the point of saving. It is minted when the change is judged rather than when it is sent, so a change held for a restart confirmation keeps the identifier it was judged with and confirming is a replay rather than a second write. The test asserts the marshalled request, not the Go field. `requestId` is omitempty: unset, it does not travel as an empty string, it vanishes from the object -- which is exactly how this shipped with a struct field that looked correct and a payload that was missing it. Signed-off-by: Terve <ntervalon@nvidia.com>
Auto-detection did not work, so the colours had to be declared by hand on any terminal that is not dark. lipgloss resolves the background once, lazily, the first time an adaptive colour is needed -- which is during the first render. By then Bubble Tea owns the terminal and is reading stdin, so the terminal's reply to the query goes to its reader, the query times out having learned nothing, and lipgloss falls back to assuming dark. Force it at startup instead, while stdin is still ours, and let the sync.Once keep the answer for every later frame. Measured in a pty against a terminal that answers with a light background: it is now detected as light, and the first frame arrives in 0.19s. The query runs alongside broker startup rather than in front of it, because a terminal that never answers costs five seconds -- termenv's timeout is a constant, and the query cannot be abandoned early since it owns the terminal until it returns. That wait is not new: clean develop pauses the same five seconds on the same terminal, and it is unrelated to --appearance. Also correct what the documentation claims about when detection fails. SSH is fine, because the query and its reply travel the connection like anything else. It is screen and tmux that cannot answer, by their own design, along with terminals that do not implement the query at all. Signed-off-by: Terve <ntervalon@nvidia.com>
The first settings change on a machine succeeded and every one after it failed with "settings changed on this device; reload before applying", until the screen was closed and reopened. The broker stamps each snapshot with the node's UUID. This screen matched that against the node argument the local RPCs take, which is empty precisely because they are local -- so every push for this machine was discarded. The cached revision stayed at whatever the first read returned while the real one moved on with each save, and a write carries the revision it was based on. Match on the row's key, which is the UUID discovery reports and the one the broker sends. Verified against a running broker: the snapshot's nodeId and discovery's hostUuid are the same value, and the engine whose port had just been changed was sitting at revision 2 against a cache still holding 1. Make the failure recoverable while here. A revision the backend will not accept cannot be repaired by repeating the write, so a rejected save now drops the stale snapshot and re-reads it, and says what happened in terms of what to do next rather than repeating the backend's instruction to reload -- which the screen has by then already carried out. Signed-off-by: Terve <ntervalon@nvidia.com>
A node's detail screen replaces the whole tab, and it stayed open when the operator switched away. Coming back to Nodes put them inside whichever machine they had last opened rather than on the list they asked for, with the list a keypress further away and nothing on screen saying why. Leaving a tab now returns it to its own top-level screen. Expressed as an optional interface, like the two a view already opts into, because this is not something every tab should do: a selection or a filter is where the operator left off and worth keeping. It is for a view that replaces itself with something else entirely. Unconditional on the way out, which is safe because a view holding a text field or an armed confirmation captures the keyboard -- the tab cannot be changed out from under one, so there is no half-finished state to discard. Signed-off-by: Terve <ntervalon@nvidia.com>
A successful engine port change was reported as a failure: "engine stayed on :1237 - :1238 is taken", while the engine was in fact listening on 1238 and nothing at all was on 1237. The verdict was inferred by comparing the saved ports against the effective ones. The effective engine port is observed at the moment the apply replies, and an engine that restarts onto its new port finishes after that -- LM Studio's server re-launches under launchd, detached, so the manager loses sight of it and the reading lags a change behind. Reading failure into that lag invented both the failure and a cause for it. The snapshot already carries the backend's own verdict as a phase, with a reason when it failed. Use those. The proxy port is still compared, because that one is read live from the proxy process rather than observed in passing, so a proxy that could not take the port it was given is a real difference worth reporting -- stated as where it landed, without asserting why. Signed-off-by: Terve <ntervalon@nvidia.com>
|
I know this is a MAASSSSIIIVVEEE PR. I'm happy to scope it down into smaller components if that's desired, however, most of the large diffs are effectively copying pieces from the UI and putting them in go rather than typescript. |
An inbound request was both a pinned status line and a row of the frame, in two different wordings. Two prompts for one fact read as two requests, and the pinned one outranked the status line, so nothing that happened next could be reported there. It also described only the first state. Pressing accept does not finish anything -- it opens the PIN field -- so the prompt went on offering "a to accept" while the PIN it actually wanted sat on the line below, telling the operator to do the thing they had just done. One prompt, in the frame, that follows the request: who is asking and which keys answer, then which machine is being accepted and that its PIN is wanted. The status line is left free to report the outcome. Signed-off-by: Terve <ntervalon@nvidia.com>
A machine PAIR is not paired with reported "spark-182c is not answering - no engine list available", directly above a hardware readout for that same machine updating every two seconds. Both were true at once because they travel different paths. Telemetry comes from an endpoint that needs no pairing; engines come over pin-based mTLS, which only succeeds against a peer we hold a pin for. The call was made anyway, and its failure is indistinguishable from silence once it has failed -- so the one cause the screen could not observe was the one it reported. Decide before asking instead. A node outside the cluster is not asked, and the pane says which relationship it has and what would change it: pair from the Nodes tab, leave the other cluster first, or wait for the handshake. The models pane stops blaming a stopped engine for the same gap -- models come from what the node advertises over discovery, and whether an engine is running is exactly what cannot be seen from outside the cluster. Signed-off-by: Terve <ntervalon@nvidia.com>
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Reviewed the manual-node/persistence intersection with #60 on current b843b95. The overhaul does not supersede the broker-persistence fix: nvpair-manual-nodes is still the authoritative in-memory worker, the broker README still documents that a worker restart loses entries, and the desktop still owns manual-nodes.json replay.
The new node-first TUI looks compatible with moving that durability into the broker. It continues to use node/add, node/remove and nodes/list unchanged, and the unified row deliberately retains manualID separately from the stable node key, so removing a discovered+manual node sends the worker alias rather than its HostUUID. I do not see a blocker in this part of #117, and #60 still looks like the right follow-up once this lands.
The engines pane said a node's engines were not visible from here, directly above a model list whose ENGINE column read "Ollama" for every row. Both came from the same place and only one was right. Discovery carries which engine serves each model and needs no pairing to do it, so the attribution is known for any machine on the network. What pairing buys is the engines' state and their controls -- not their existence, which the screen was already showing. Say that instead: name the engines the node advertises, point at the list below, and promise only what pairing would actually add. Signed-off-by: Terve <ntervalon@nvidia.com>
| // Only after this screen's own write, and only once. These snapshots also | ||
| // arrive unprompted whenever anyone else changes settings, and a note firing on | ||
| // each would be noise about something the operator did not do. | ||
| func (d *nodeDetail) reportSettingsOutcome(snap enginesettings.Snapshot) { |
There was a problem hiding this comment.
Settings apply reports a verdict before the backend has one:
reportSettingsOutcome treats the first snapshot it sees as the outcome, but
the broker publishes a snapshot as soon as it accepts a write — Phase: "applying" — and only then stops the engine and does the work. There's no
applying case in the switch, so that first snapshot falls through to
default and we report "settings saved" before anything has actually been
applied.
Two things fall out of that, and both are visible to the operator.
The port comparison one line above default fires on the applying snapshot.
At that moment EffectiveProxyPort is still the old port and
Settings.ProxyPort is the new one, so they differ by definition, and a
perfectly healthy change reports "endpoint is on :<old>, not the
:<new> you asked for". The operator is told their change failed, on the
exact path where it's working.
The other one is quieter and worse. Because the await is cleared on that first
snapshot, the real succeeded / failed snapshot that arrives afterwards is
ignored. If the apply genuinely fails — port already bound, engine won't come
back up — we've already said "saved", and we never correct it. The screen's
last word on the subject is wrong, and the operator has no reason to go
looking.
As for when it bites: any change that restarts the engine, which is exactly
the case the confirm prompt exists for, and any proxy port change at all. The
one case that's fine today is a write where the settings already match, since
the broker short-circuits that straight to succeeded without ever passing
through applying.
For what it's worth, I think this is the same class of bug the commit message
set out to fix. Moving from "compare the ports" to "read the phase" was the
right call — it just doesn't yet handle the phase that isn't an answer.
There's a second, smaller thing in the same gate. Correlation is
d.settingsAwaited != snap.Engine, i.e. engine name only. The comment on line
608 says these snapshots arrive "from whoever changed it — this screen, the
desktop app, or another operator". So if someone touches the same engine from
the desktop app while the TUI is waiting, their snapshot consumes our await
and we report their result as ours.
Suggested fix
The good news is that none of this needs a backend change. Snapshot already
carries RequestID, the broker stamps it before publishing the applying
snapshot, and the terminal update only touches Phase, Error, and
AppliedRevision — so it survives onto the snapshot we actually care about.
We're just not reading the field.
Three changes, all in nodedetail.go:
- Store the request id alongside the engine when arming the await, and
require both to match. - Return early unless the phase is
succeededorfailed. I'd suggest
matching those two positively rather than excludingapplying, so a phase
added later can't be read as a verdict either. That also handles the empty
phase a never-configured engine reports, for free. - Move the port comparison inside the success branch, where the effective
port is actually a reading of where the proxy landed.
Roughly:
func (d *nodeDetail) reportSettingsOutcome(snap enginesettings.Snapshot) {
awaited := d.settingsAwaited
if awaited == nil || snap.Engine != awaited.engine {
return
}
// The broker stamps each snapshot with the request that produced it, so
// somebody else's change to this same engine is distinguishable rather
// than being consumed as this screen's result.
if snap.RequestID != awaited.requestID {
return
}
// Accepted is not applied. The broker publishes "applying" before it stops
// the engine, so there is no verdict yet and nothing to say.
if snap.Phase != settingsPhaseSucceeded && snap.Phase != settingsPhaseFailed {
return
}
d.settingsAwaited = nil
label := d.engineLabel(snap.Engine)
if snap.Phase == settingsPhaseFailed {
// The backend's own words. It knows why; this screen would be guessing.
if snap.Error != "" {
d.status.error("%s settings: %s", label, snap.Error)
return
}
d.status.error("%s settings were not applied", label)
return
}
// Only now is the effective port a reading of where the proxy landed
// rather than of where it still was when the write was accepted.
if snap.EffectiveProxyPort != 0 && snap.EffectiveProxyPort != snap.Settings.ProxyPort {
d.status.error("%s endpoint is on :%d, not the :%d you asked for",
label, snap.EffectiveProxyPort, snap.Settings.ProxyPort)
return
}
d.status.ok("%s settings saved", label)
}That needs settingsAwaited to become a small struct holding the engine and
the request id instead of a bare engine name, both arming sites updated, and a
settingsPhaseSucceeded / settingsPhaseApplying constant next to the
existing settingsPhaseFailed.
One thing the above doesn't fix on its own: callTimeout is 35 s while an
apply can legitimately take minutes, so the RPC times out while the phase is
still applying, and the error path clears the await and reports failure.
You'd be trading a false success for a false failure at 35 s, with the correct
verdict then dropped because nothing is waiting for it any more. I've raised
that separately — I think they want to land together, and the error path there
needs to tell a deadline (still in flight, keep waiting) apart from a real
rejection like a revision mismatch (clear and report, as it does now).
Last thing: nothing in the branch sends phase: "applying" at all, so a test
for it would be worth adding alongside — particularly the case where the
applying snapshot's effective port differs from the requested one and we
should stay quiet.
Where this is
services/nvpair-tui/ui/nodedetail.go:1265-1285—reportSettingsOutcome:
the missingapplyingcase, and thedefaultbranch reporting "saved"services/nvpair-tui/ui/nodedetail.go:1266— the engine-name-only
correlation gateservices/nvpair-tui/ui/nodedetail.go:1279— the effective-port comparison
that fires duringapplyingservices/nvpair-tui/ui/nodedetail.go:103-106—settingsAwaited, the
field that would hold the request idservices/nvpair-tui/ui/nodedetail.go:1114and:1297— the two places the
await is armedservices/nvpair-tui/ui/nodedetail.go:500-509— the apply-error path that
clears the await (this is the one coupled to the 35 s timeout)services/nvpair-tui/ui/nodedetail.go:1289—settingsPhaseFailed, where
the other phase constants would go
| // see judgeSettingsPreview, which both substitutes the normalized settings and | ||
| // drops the resolution. This does not re-check either, because a commit that | ||
| // quietly repaired its own request would hide the bug that produced it. | ||
| func applyEngineSettingsCmd(client *rpc.Client, req enginesettings.Request) tea.Cmd { |
There was a problem hiding this comment.
engine:apply-settings gets 35 s against a backend budget of 11–13 minutes
Every request in the TUI goes through the one call helper, which uses a
single callTimeout of 35 s. The comment explains that as covering "the
broker's slowest relay (the 30s cluster-manager path) plus headroom", which is
right for the calls it was reasoned about — but engine:apply-settings is not
one of them.
The shared ladder in shared/enginesettings/timeouts.go sizes this path at
10.5 minutes for configure, 11 for the owner-side operation, 12 for a client
call, and 13 for one relayed on a peer's behalf. An apply that stops the
engine, rebinds a port, and waits for readiness is expected to sit in there for
minutes. We give it 35 seconds.
What the operator sees: they change the engine port, confirm the restart
prompt, and about 35 seconds later the screen tells them the save failed — on
the path where it's working normally. The broker doesn't stop; "once accepted,
completion is target-owned", so the port swap carries on to completion behind a
screen that has just reported it as failed. When the real reply arrives the
client has already dropped its pending entry, so the answer is discarded.
Then it compounds: the error path clears the await and deletes the cached
snapshot before re-reading. So the terminal succeeded snapshot that follows
has nothing waiting for it and goes unreported, and the operator is left
looking at a failure for an operation that succeeded. The natural response is
to retry, and since a fresh requestId is minted per preview, the retry isn't
recognised as the same attempt by the broker's receipt dedup.
When it bites: any change that restarts the engine — which is exactly the case
the confirm prompt exists for — and more or less always on a remote apply,
since the relay budget is 13 minutes for a reason. A change that doesn't
restart anything is usually fast enough to sneak in under the deadline, which
is probably why this hasn't shown up in manual testing.
Suggested fix
The pattern for this is already in the branch, which I think makes it a small
change. classifyOpResult turns a context.DeadlineExceeded on a long
operation into detached rather than a failure, and the comment above
longRunningOps makes the argument better than I can:
Reporting a failure is actively misleading: the operator sees "load failed"
at the same moment the model finishes loading.
That's this bug. engine:apply-settings just doesn't go through
classifyOpResult — it has its own decode, and its error path treats a
deadline the same as a rejection.
Two parts, and I'd suggest both:
First, give the call a budget that comes from the shared ladder instead of the
default. No new constant needed — the TUI already imports the package these
live in:
// callWithin is call with an explicit budget, for requests whose duration is
// set by the work rather than by the RPC.
func callWithin(client *rpc.Client, budget time.Duration, method string, params any,
decode func(*rpc.Message, error) tea.Msg) tea.Cmd {
return func() tea.Msg {
ctx, cancel := context.WithTimeout(context.Background(), budget)
defer cancel()
msg, err := client.Call(ctx, method, params)
return decode(msg, err)
}
}with call delegating to it at callTimeout, and:
func applyEngineSettingsCmd(client *rpc.Client, req enginesettings.Request) tea.Cmd {
// The budgets the target is working to. Taking them from the shared
// package rather than restating a number here is what keeps this from
// drifting the next time the readiness probe is retuned.
budget := enginesettings.CallBudget
if req.NodeID != "" {
budget = enginesettings.RelayBudget
}
return callWithin(client, budget, "engine:apply-settings", req, ...)
}Second, even with the right budget, a deadline here shouldn't read as a
rejection — so handle it before the generic error path:
case engineSettingsAppliedMsg:
if errors.Is(msg.err, context.DeadlineExceeded) {
// Accepted work is target-owned: the broker does not abandon a
// half-finished port swap because we stopped listening. Stay
// armed for the terminal snapshot rather than calling it failed.
d.status.busy("%s settings are still applying...", d.engineLabel(msg.engine))
return nil, true
}
if msg.err != nil {
// ... existing revision-mismatch handling, unchanged
}Keeping the snapshot rather than deleting it matters in that branch: the
delete is right for a revision mismatch, where the cached snapshot really is
stale, but wrong for a deadline, where it's still the basis the in-flight
write was made against.
This overlaps with the applying-phase comment I left on
reportSettingsOutcome — that one removes a premature success, this one
removes a premature failure, and I don't think either is complete alone. Fix
only the phase handling and the operator still gets a false failure at 35 s;
fix only the timeout and the applying snapshot still reports "saved" instantly.
One question, separate from the above
start, stop, and restart are excluded from longRunningOps, and there's
a test pinning that, so I assume it's deliberate. But waitReady takes its
timeout from the manifest's probe and the bundled Ollama probe is described
elsewhere as permitting ten minutes — so is engine:start actually guaranteed
to reply inside 35 s? If a cold start behind a slow probe can exceed it, it
belongs in the same bucket. Genuinely asking; I haven't checked which value
ships in the manifest.
Where this is
services/nvpair-tui/ui/rpccmd.go:18—callTimeout, the 35 s constantservices/nvpair-tui/ui/rpccmd.go:79-86—call, the single budget every
request sharesservices/nvpair-tui/ui/enginesettings.go:136-137—
applyEngineSettingsCmd, the call that needs its own budgetservices/nvpair-tui/ui/nodedetail.go:500-509— the error path that treats a
deadline as a rejection, clears the await, and deletes the cached snapshotservices/nvpair-tui/ui/engineswire.go:140-149—engineOpMsg.detached, the
state that already exists for thisservices/nvpair-tui/ui/engineswire.go:196-222—longRunningOpsand
classifyOpResult, the treatment settings should getservices/nvpair-tui/ui/engineswire_test.go:137— the test pinning
start/stop/restartas non-detached (the open question above)
There was a problem hiding this comment.
Rewriting my earlier comment on this — I checked the history and callTimeout
predates this branch, so the constant isn't yours. What's new is a call that
can't live inside it, which I think makes this narrower than I first framed it.
Every request goes through the one call helper at 35 s. The shared ladder in
shared/enginesettings/timeouts.go sizes the settings path at 10.5 minutes for
configure, 11 for the owner-side operation, 12 for a client call, and 13 for
one relayed on a peer's behalf. An apply that stops the engine, rebinds a port,
and waits for readiness is expected to sit in there for minutes.
What the operator sees: they change the engine port, confirm the restart
prompt, and ~35 seconds later the screen says the save failed — on the path
where it's working. The broker doesn't stop, since "once accepted, completion
is target-owned", so the change carries on behind a screen that just reported
it as failed. The real reply is then discarded, because the client has already
dropped its pending entry.
It compounds from there: the error path clears the await and deletes the
cached snapshot before re-reading, so the terminal succeeded snapshot has
nothing waiting for it and goes unreported too.
Worst on a remote apply, where the relay budget is 13 minutes for a reason. A
change that doesn't restart anything usually sneaks in under the deadline,
which is probably why it hasn't shown up in manual testing.
Suggested fix
I'd keep this off the shared constant — every other caller is fine at 35 s —
and give the new call its own budget. The numbers are already importable from
the package you're using for Request and Snapshot:
// callWithin is call with an explicit budget, for requests whose duration is
// set by the work rather than by the RPC.
func callWithin(client *rpc.Client, budget time.Duration, method string, params any,
decode func(*rpc.Message, error) tea.Msg) tea.Cmd {
return func() tea.Msg {
ctx, cancel := context.WithTimeout(context.Background(), budget)
defer cancel()
msg, err := client.Call(ctx, method, params)
return decode(msg, err)
}
}func applyEngineSettingsCmd(client *rpc.Client, req enginesettings.Request) tea.Cmd {
budget := enginesettings.CallBudget
if req.NodeID != "" {
budget = enginesettings.RelayBudget
}
return callWithin(client, budget, "engine:apply-settings", req, ...)
}Second, a deadline here still shouldn't read as a rejection, so handle it
before the generic error path:
case engineSettingsAppliedMsg:
if errors.Is(msg.err, context.DeadlineExceeded) {
// Accepted work is target-owned: the broker does not abandon a
// half-finished port swap because we stopped listening.
d.status.busy("%s settings are still applying...", d.engineLabel(msg.engine))
return nil, true
}
if msg.err != nil {
// ... existing revision-mismatch handling, unchanged
}Keeping the snapshot matters in that branch — deleting it is right for a
revision mismatch, where it really is stale, and wrong for a deadline, where
it's still the basis the in-flight write was made against.
Worth noting you already have this pattern: classifyOpResult turns a
DeadlineExceeded on a long operation into detached rather than a failure,
and the comment above longRunningOps makes the argument better than I can —
"the operator sees 'load failed' at the same moment the model finishes
loading". Settings just doesn't route through it.
This pairs with the applying-phase comment: that one removes a premature
success, this one a premature failure, and neither is complete alone.
Where this is
services/nvpair-tui/ui/rpccmd.go:18and:79-86—callTimeoutand the
sharedcallservices/nvpair-tui/ui/enginesettings.go:136-137— the call needing its own
budgetservices/nvpair-tui/ui/nodedetail.go:500-509— the error pathservices/nvpair-tui/ui/engineswire.go:196-222—classifyOpResult, the
treatment this should getservices/shared/enginesettings/timeouts.go:12-17— the budgets, importableservices/nvpair-ui-broker/enginesettings.go:565-570— "completion is
target-owned"
| return lines | ||
| } | ||
|
|
||
| func (d *nodeDetail) CapturingInput() bool { |
There was a problem hiding this comment.
Settings input capture (covers both the confirm prompt and the misdirected write)
The settings flow's two new waiting states aren't covered by CapturingInput()
CapturingInput() guards catalog and pending, but the settings flow adds
two more states where the screen is waiting on the operator or the backend —
settingsConfirm and settingsWanted — and neither is covered. That produces
two separate problems.
The restart confirm can be answered by accident. While "press y to confirm"
is up, CapturingInput() returns false, so the shell's own bindings still
fire. Your comment on pending describes precisely what happens next:
Without this the shell's own bindings still fired, so tab or a digit switched
away and left the action armed behind an off-screen prompt — to be confirmed
by whatever the operator pressed on returning.
That's now true again for the settings confirm, except the armed action here
restarts an engine.
Settings can be written to the wrong engine. Same gap, worse outcome. While
the engine:get-settings read is in flight, settingsWanted is set but
d.mode is still detailInputNone, so CapturingInput() is false and the
engine list still moves under the arrow keys. Then:
- operator presses the settings key on Ollama — "reading settings..."
- while it's in flight, they press down; the cursor moves to LM Studio
- the Ollama snapshot arrives and opens the field for Ollama, as intended
- they type a port and press enter
submitSettingscallsd.selectedEngine(), which reads the cursor — so
the request is built against LM Studio's snapshot
The value typed for one engine is applied to the other. On a proxy port that's
a live endpoint move on an engine the operator wasn't even looking at.
Suggested fix
Add both states to the guard, which fixes the confirm directly and closes the
window that makes the misdirected write reachable:
func (d *nodeDetail) CapturingInput() bool {
if d.catalog != nil {
return true
}
if d.pending != nil {
return true
}
// An armed settings confirm answers the next key for the same reason, and
// a settings read in flight is about to open a field against the engine it
// was asked for — the selection must not move out from under either.
if d.settingsConfirm != nil || d.settingsWanted != nil {
return true
}
return d.mode != detailInputNone
}I'd also make submitSettings resolve the engine from the field rather than
the cursor, so the write can't be misdirected even if some other path reopens
that window. The field is already opened from the snapshot's engine at the
settingsWanted match, so carrying that engine on the open state and reading
it back on submit would make the two agree by construction.
Where this is
services/nvpair-tui/ui/nodedetail.go:389-404—CapturingInput(), missing
both statesservices/nvpair-tui/ui/nodedetail.go:399-401— thependingcomment
describing the failureservices/nvpair-tui/ui/nodedetail.go:1024—submitSettingsresolving the
engine from the cursorservices/nvpair-tui/ui/nodedetail.go:1361-1367—selectedEngine(), which
is the table cursorservices/nvpair-tui/ui/nodedetail.go:487-492— the field opening for the
snapshot's engine, which is what diverges from the cursor
| // Serve the cache when it is fresh, and also when a recent attempt failed: | ||
| // re-fetching on every call against a dead upstream just made each caller | ||
| // wait out the full timeout for the same stale answer. | ||
| haveList := len(c.lmStudio) > 0 |
There was a problem hiding this comment.
Catalog backoff on a cold cache
A never-reachable catalog upstream is re-dialled on every open
The failure backoff is gated on haveList:
backingOff := haveList && time.Since(c.lmFailed) < catalogRetryAfterFailureSo it only applies once a fetch has succeeded at least once. On a cold cache —
offline, air-gapped, or Hugging Face blocked by a proxy — haveList is false,
backingOff is false, and every call goes back out to the network. Which is
the case the comment directly above says it's there to prevent:
re-fetching on every call against a dead upstream just made each caller wait
out the full timeout for the same stale answer.
The inflight guard still coalesces concurrent callers, so this isn't a
thundering herd — but each sequential open of Add Model pays the full fetch
timeout before showing an empty list. On a machine that can't reach Hugging
Face at all, that's every single time, forever, with no state in which it
improves.
Given this binary is aimed at headless and remote servers, I'd expect
restricted egress to be common rather than exceptional.
Suggested fix
Drop haveList from the backoff condition and let the cold-cache path back off
on its own terms. The two uses want different things: fresh genuinely needs a
list to serve, but backingOff is about not re-dialling a dead upstream, which
is just as true when there's nothing cached. The existing "no models" reply
covers the interim.
Where this is
services/nvpair-engine-manager/catalog.go:347-354—haveList,fresh,
backingOff, and the early returnservices/nvpair-engine-manager/catalog.go:343-345— the comment stating the
intent this misses
| @@ -0,0 +1,1274 @@ | |||
| // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
There was a problem hiding this comment.
// TestLateSettingsReplyDoesNotOpenAnotherNodesEditor covers a settings read
// completing after the operator has left its node and requested the same
// engine's settings on another node.
func TestLateSettingsReplyDoesNotOpenAnotherNodesEditor(t *testing.T) {
v := newNodesView(nil)
v.SetSize(100, 30)
first := newNodeDetail(nil, nodeRow{key: "node-a", name: "A", presence: presenceOnline})
first.SetSize(100, 30)
first.engines = []engineStatus{{Engine: "ollama", Installed: true}}
first.refreshEngines()
v.detail = first
if cmd := first.editSetting(&first.engines[0], detailInputEnginePort); cmd == nil {
t.Fatal("node A did not start a settings read")
}
v.Update(tea.KeyMsg{Type: tea.KeyEsc})
if v.detail != nil {
t.Fatal("node A detail did not close while its settings read was pending")
}
second := newNodeDetail(nil, nodeRow{key: "node-b", name: "B", presence: presenceOnline})
second.SetSize(100, 30)
second.engines = []engineStatus{{Engine: "ollama", Installed: true}}
second.refreshEngines()
v.detail = second
if cmd := second.editSetting(&second.engines[0], detailInputEnginePort); cmd == nil {
t.Fatal("node B did not start its own settings read")
}
stale := ollamaSettings()
stale.NodeID = "node-a"
stale.Settings.ServerPort = 11500
v.Update(engineSettingsMsg{snapshot: stale})
if second.mode != detailInputNone {
t.Errorf("node A's reply opened node B's editor with port %q", second.input.Value())
}
if _, cached := second.settings["ollama"]; cached {
t.Error("node A's settings reply was cached on node B")
}
if second.settingsWanted == nil {
t.Error("node A's reply consumed node B's pending settings edit")
}
}
This test fails because the settings changes aren't checked by who sent them.
If node A’s read returns after the operator opens node B and requests the same engine’s settings, B can display A’s values and submit them to B using A’s revision. Matching revisions could allow an unintended overwrite.
| } | ||
|
|
||
| func TestNormalizeCatalogEngine(t *testing.T) { | ||
| cases := map[string]string{ |
There was a problem hiding this comment.
nit: maybe rewrite this with a test helper instead of a loop and a map.
| ModifiedAt string `json:"modified_at"` | ||
| Size uint64 `json:"size"` | ||
| Digest string `json:"digest"` | ||
| Details struct { |
There was a problem hiding this comment.
nit: I'd prefer to avoid anonymous structs where feasible.
| q.Set("author", lmStudioAuthor) | ||
| q.Set("sort", "downloads") | ||
| q.Set("direction", "-1") | ||
| q.Set("limit", fmt.Sprint(lmStudioLimit)) |
There was a problem hiding this comment.
Is it ok we don't have paging here and only (potentially) return an incomplete list? Might be worth a comment justifying that here or on lmStudioLimit
|
|
||
| // filterForPlatform drops models that cannot install on the target. | ||
| // | ||
| // MLX quantizations are Apple's framework and only run on Apple Silicon; `lms |
There was a problem hiding this comment.
I'm a bit confused. How/where do we filter out the Intel Mac models?
| @@ -0,0 +1,726 @@ | |||
| // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
There was a problem hiding this comment.
I think there's a case where two pinned toast messages interfere with each other. This fails for me locally:
// TestInboundInviteDoesNotHideOutboundPIN checks that a second live pairing
// request does not make the PIN for our still-pending outbound invite unreadable.
func TestInboundInviteDoesNotHideOutboundPIN(t *testing.T) {
v := newNodesView(nil)
v.SetSize(100, 30)
v.Update(nodeInviteMsg{name: "peer", inviteID: "outbound", pin: "123456"})
if !contains(v.View(), "PIN 123456") {
t.Fatal("outbound PIN was not visible before the inbound invite arrived")
}
params, err := json.Marshal(clusterInvite{InviteID: "inbound", FromNodeName: "other peer"})
if err != nil {
t.Fatalf("encode inbound invite: %v", err)
}
v.Update(NotificationMsg{Msg: &rpc.Message{Method: "cluster:invite-received", Params: params}})
if v.outboundInviteID != "outbound" {
t.Fatalf("outbound invite is no longer pending: %q", v.outboundInviteID)
}
if v.inbound == nil || v.inbound.InviteID != "inbound" {
t.Fatalf("inbound invite was not recorded: %#v", v.inbound)
}
if got := v.View(); !contains(got, "PIN 123456") {
t.Errorf("live outbound PIN disappeared after an unrelated inbound invite: %q", got)
}
}
| // an age it invited exactly the wrong reading — a steadily climbing number | ||
| // beside "this machine", whose reachability is never in question. STATUS is the | ||
| // reachability verdict, and it has better evidence behind it. | ||
| func nodesColumns(w int) []table.Column { |
There was a problem hiding this comment.
These columns add up to more than minTerminalWidth (40). Is that going to cause an overrun?
|
|
||
| // workloadColumns is the job table's layout, shared by construction and resize | ||
| // so the two cannot drift. | ||
| func workloadColumns(w int) []table.Column { |
There was a problem hiding this comment.
These columns add up to more than minTerminalWidth (40). Is that going to cause an overrun?
| switch { | ||
| case strings.HasPrefix(msg.Method, "lmstudio-proxy:"): | ||
| idx = 1 | ||
| case strings.HasPrefix(msg.Method, "proxy:"): |
There was a problem hiding this comment.
desktop/docs/services-api.md says ollama is things like ollama-proxy:subscribe, not with a proxy prefix. The test also uses this "proxy" prefix, but does that reflect reality?
| // key rather than lost. | ||
| func (v *jobsView) Init() tea.Cmd { | ||
| return tea.Batch( | ||
| call(v.client, "workloads:subscribe", nil, func(_ *rpc.Message, err error) tea.Msg { |
There was a problem hiding this comment.
https://pkg.go.dev/charm.land/bubbletea/v2#Batch apparently runs these in no particular order, so subscribing might not be first. Could that cause a problem if it happens last out of this list?
4 participants
Description
Rebuilds
nvpair-tuiaround a node-first tab layout and closes the gaps against the desktop application, then moves the model catalogue into the backend so both front ends browse one implementation.A node is the unit an operator reasons about, so the tabs become Nodes, Jobs, Service, Errors, and Logs, and everything specific to one machine hangs off its row. The ten tabs this replaces spread one machine's facts across four of them: its engines in one, its models in another, its ports in a third, its cluster standing in a fourth.
Much of the rest is about claiming no more than the backend guarantees. A port change reports the port actually bound rather than the one requested. Clearing an error is only offered where clearing sticks. The cluster name is labelled as this machine's own. Node presence follows the broker's snapshot instead of a timestamp that never advanced.
How to review this
Seventeen commits, in dependency order. Four are prerequisites, one is the rewrite, and the rest are separable features and fixes. The rewrite (
5381e2f1) is 51 files and is best read in the order below — it follows the data rather than the alphabet.The five views only exist together: the shell sizes them, they share its status line and frame budget, and the tab set is the change. Splitting them further would have meant authoring intermediate versions of
nodedetail.goandjobs.gothat never existed, so the guided read is offered instead.Read the commits in this order
6b377e78frame cap single-sourced across four hops — 5 filese99efb05engine:catalogin the engine manager — the+109kis the committed Ollama list; reviewcatalog.goandcatalog_test.go001b0e4adesktop served from that catalogue — the−109kis the same list leaving Electronc414dbf0oneinference-dispatcher, fromcli-bin9a61f3debroker stream errors reported as disconnectse70fbd21shared frame primitives (table, status line, row budget)5381e2f1the rewrite — see the file order belowec7a6c84release version stamped for the update notice144e472fengine launch-arguments editor, ports onto the same write32738dcddocumentation and the architecture rule953a9ab0CI's staged-binary check learns about the demo clienteaedf559light-terminal readability7f5610datherequestIda settings commit requires67ca9630background detected before Bubble Tea takes stdinfbb37e0alocal settings snapshots kept current94d041d2leaving the Nodes tab returns to the list33b5470csettings verdict taken from the backendCommits 12–17 came from running the build on real hardware. Four of them fix defects in commit 9; each says what the backend actually does, which is worth reading even where the diff is small.
Read the rewrite's files in this order
Start with the shape — 400 lines, and the rest follows from it:
ui/ui.go— the tab set. The whole change in 27 linesui/model.go— the shell: frame budget, tab switching, the notice rowui/keys.go— why the bindings are what they areui/view.go(unchanged) — the contract the five views implementThe data, before the screens that render it:
ui/nodeswire.go— the discovery, cluster, and manual wire shapesui/nodesmodel.go— merging those three feeds onto one key. The heart of the Nodes tabui/nodenames.go— resolving a node id to a nameui/engineswire.go— engine status and model inventory shapesThe tabs, simplest first:
ui/logs.go,ui/errors.go— smallest, and they establish the view idiomsui/service.go— worker liveness, log level, data resetui/proxystatus.go— per-engine facade readiness and portsui/jobs.go— the endpoints and live inference workui/nodes.go— the merged node table, filtering, pairing keysui/invite.go— pairing events scoped to their sessionThe drill-down and its two satellites — the largest file, read last:
ui/nodedetail.go— engines and models for one machineui/nodetelemetry.go— the direct/v1/node-infopollui/catalog.go— the downloadable-model browserui/demo.go,ui/demoschedule.go— the Inference DemoThen the tests, which read as the specification:
ui/framebudget_test.go— no view overflows its frame at any supported sizeui/table_test.go— every table has columns at constructionui/nodesmodel_test.go,ui/nodedetail_test.go,ui/service_test.go— the per-area behaviour_test.gofilesDeletions need no reading:
cluster.go,engines.go,health.go,manualnodes.go,proxies.go,settings.go,workloads.goare the tabs the five replace.Scope
Included: the terminal interface rewrite, its Inference Demo and update notice;
engine:catalogreplacing the Electron-only model hub; oneinference-dispatcherincli-bin; an editor for an engine's launch arguments with the two port fields moved onto the same revision-checked write; documentation.Excluded: no change to routing, scheduling, or proxy behaviour; none to pairing or trust; the desktop renderer's settings UI is untouched, only its catalogue source moved.
Validation
Everything CI runs, run locally on macOS arm64, Go 1.26.3, Node 22:
node scripts/spdx-headers.mjs— 1039 checked, 0 missingnpm --prefix desktop run verify:build-scripts,service-contracts:check,typecheck,lint,dead-code:check,test:unit(230 passing)npm --prefix desktop run build:modular-binaries -- --force— 13 binaries-raceacrossshared,nvpair-tui,nvpair-ui-broker,nvpair-engine-managercd services/tests && go test ./... -count=1 -timeout=20m— passes, no failuresservices/build.sh, then the staged-binary comparison this branch updatesTwo failures are pre-existing and reproduce identically on
developin a clean worktree:TestUninstallTerminatesRunningInstanceinnvpair-engine-manager, andservices/shared/splitlisten/splitlisten_test.gois notgofmt-clean.Manual, on a Mac and a DGX Spark over SSH: pairing, engine lifecycle, model download, the demo, and the settings editor. The
ui.ReleaseVersionstamp was read back out of both binary sets, since a-Xagainst a wrong symbol path fails silently. Terminal background detection was checked in a pty against a terminal that answers the query and one that ignores it.Risk
The engine settings path is the part to look at. The two port fields previously wrote through
engine:set-portand the proxy's ownset-port, which meant two writers with no shared revision — the last to finish won and neither could tell it had lost. All three fields are now one revision-checked write throughengine:preview-settingsandengine:apply-settings. That also makes them editable on a peer, which the old path refused because it had no remote form; whether a given engine accepts the write is the snapshot'sEditableanswer rather than a rule kept in the front end.jsonrpc.WorkerFrameBytesraises the inbound frame cap on three hops that carry large worker replies, from the 1 MiB default to 8 MiB. The Ollama catalogue is roughly 1.9 MiB and could not previously cross them; an over-long frame is a terminal read error rather than a dropped message, so the hops have to agree.No wire-format removals, no persisted-data migrations, no change to how nodes authenticate to each other.
Release intent
Changelog title
A rebuilt terminal interface, and one model catalogue for both front ends
Changelog body
ppairs,aaccepts,ffinds a machine by address.Bumps
Checklist
git commit -s), certifying the Developer Certificate of Origin.services/versions.jsonis written by automation — do not edit it by hand.