fix(workspace): consume targeted live globe state #761

Merged
admin merged 3 commits from fix/workspace-stream-targeted-globe-state into dev 2026-10-30 20:31:05 +00:00
Owner

Summary

  • Consume the authenticated shared NODE stream for the owner-only globe pin, WatchWorkers for worker details, and the node snapshot for the public city estimate.
  • Remove repeated GetNodeLocation, GetNodeInfo, and ListWorkers requests on every node/peer snapshot; retain one-shot fallbacks only while a stream is unavailable.
  • Preserve stable globe state when peer updates do not change a location and document the stream/retry behavior.

Part of #741; this does not complete its broader multi-client acceptance criteria.

Verification

  • npm --prefix apps/sisyphus run typecheck
  • npm --prefix apps/sisyphus test
  • npm --prefix apps/sisyphus run build
  • make lint
  • actionlint
  • git diff --check
## Summary - Consume the authenticated shared `NODE` stream for the owner-only globe pin, `WatchWorkers` for worker details, and the node snapshot for the public city estimate. - Remove repeated `GetNodeLocation`, `GetNodeInfo`, and `ListWorkers` requests on every node/peer snapshot; retain one-shot fallbacks only while a stream is unavailable. - Preserve stable globe state when peer updates do not change a location and document the stream/retry behavior. Part of #741; this does not complete its broader multi-client acceptance criteria. ## Verification - `npm --prefix apps/sisyphus run typecheck` - `npm --prefix apps/sisyphus test` - `npm --prefix apps/sisyphus run build` - `make lint` - `actionlint` - `git diff --check`
Author
Owner

Workspace globe data now follows the targeted live sources: the authenticated shared NODE stream for the owner pin, WatchWorkers for worker metadata, and the existing node snapshot for the public city estimate. This removes per-snapshot GetNodeInfo/GetNodeLocation/ListWorkers calls and keeps one-shot recovery only for stream outages. This is a focused follow-up under #741; the broader client-sync acceptance is still open. /cc @rvnminers-A-and-N

Workspace globe data now follows the targeted live sources: the authenticated shared `NODE` stream for the owner pin, `WatchWorkers` for worker metadata, and the existing node snapshot for the public city estimate. This removes per-snapshot `GetNodeInfo`/`GetNodeLocation`/`ListWorkers` calls and keeps one-shot recovery only for stream outages. This is a focused follow-up under #741; the broader client-sync acceptance is still open. /cc @rvnminers-A-and-N
Author
Owner

Follow-up validation caught and fixed an additional conversion gap: browser NodeSnapshot mapping was dropping cityEstimate from both the initial GetNodeInfo response and WatchNodeState updates. That field now survives both conversion paths (and the dev proxy type), so the Workspace can actually consume it. Re-ran typecheck, all client tests, build, lint, actionlint and diff checks successfully.

Follow-up validation caught and fixed an additional conversion gap: browser `NodeSnapshot` mapping was dropping `cityEstimate` from both the initial GetNodeInfo response and WatchNodeState updates. That field now survives both conversion paths (and the dev proxy type), so the Workspace can actually consume it. Re-ran typecheck, all client tests, build, lint, actionlint and diff checks successfully.
Author
Owner

Refreshed this PR onto current dev (including #748), resolving the shared-state enum overlap by preserving both NODE and MODEL_PROFILES. Validation on the refreshed branch passed: client typecheck, all 235 client tests, production build, make lint, actionlint, and git diff --check. Fresh CI is now running.

/cc @rvnminers-A-and-N

Refreshed this PR onto current `dev` (including #748), resolving the shared-state enum overlap by preserving both `NODE` and `MODEL_PROFILES`. Validation on the refreshed branch passed: client typecheck, all 235 client tests, production build, `make lint`, `actionlint`, and `git diff --check`. Fresh CI is now running. /cc @rvnminers-A-and-N
Author
Owner

Coordination note: #769 extends shared-state-domain.ts with the MODEL_CATALOG mapping (domain 16) on top of the #741 stream stack; this PR adds the NODE mapping (domain 1). There is no semantic collision, but the merged helper needs to retain both additions. I will leave this branch untouched and reconcile the two after the current CI/review sequence settles. /cc @rvnminers-A-and-N

Coordination note: #769 extends `shared-state-domain.ts` with the `MODEL_CATALOG` mapping (domain 16) on top of the #741 stream stack; this PR adds the `NODE` mapping (domain 1). There is no semantic collision, but the merged helper needs to retain both additions. I will leave this branch untouched and reconcile the two after the current CI/review sequence settles. /cc @rvnminers-A-and-N
rvnminers-A-and-N left a comment
Owner

Really like where this lands. There are no more GetNodeLocation/GetNodeInfo/ListWorkers calls on every peer tick. I checked that the NODE stream carries owner_location for the token holder and that pin edits fire locationChanged(), so the pin stays live.

Two things before it can go in:

  • It conflicts with dev again: #749 and #750 touched shared-state-domain.ts and its test.
  • In that same file, the case 1: case 'SHARED_STATE_DOMAIN_NODE': return 'node' block appears twice (lines 7-10). It looks like a leftover from the last conflict resolution. It's harmless, but the second copy is dead code and no-duplicate-case would flag it.

Since the whole #741 stack also edits that switch, do you want to land this first and rebase the stack on it, or fold the NODE mapping into the bottom of the stack? Also, on a node that isn't coordinating, does WatchWorkers end with an error? If so, the workspace would retry it every 30 s while visible. I wanted to check whether that's expected.

Really like where this lands. There are no more GetNodeLocation/GetNodeInfo/ListWorkers calls on every peer tick. I checked that the NODE stream carries `owner_location` for the token holder and that pin edits fire `locationChanged()`, so the pin stays live. Two things before it can go in: - It conflicts with `dev` again: #749 and #750 touched `shared-state-domain.ts` and its test. - In that same file, the `case 1: case 'SHARED_STATE_DOMAIN_NODE': return 'node'` block appears twice (lines 7-10). It looks like a leftover from the last conflict resolution. It's harmless, but the second copy is dead code and `no-duplicate-case` would flag it. Since the whole #741 stack also edits that switch, do you want to land this first and rebase the stack on it, or fold the NODE mapping into the bottom of the stack? Also, on a node that isn't coordinating, does `WatchWorkers` end with an error? If so, the workspace would retry it every 30 s while visible. I wanted to check whether that's expected.
Author
Owner

I folded the workspace worker feed into the authenticated shared-state stream as well. When a node has no worker/pool service, WORKERS now reports one unavailable-domain event and keeps the stream open; a later successful pool join wakes it, instead of the page retrying WatchWorkers every backoff interval. Transport failures still use bounded reconnect, with a one-shot worker-list fallback only during the outage.

I also rebased this to a clean, linear branch on current dev. The shared-state domain mapper now has exactly one NODE case (it was duplicated in the old conflict resolution). The renderer receives the per-domain error without triggering a reconnect loop, so no-coordinator worker availability is expected and handled.

/cc @rvnminers-A-and-N

I folded the workspace worker feed into the authenticated shared-state stream as well. When a node has no worker/pool service, `WORKERS` now reports one unavailable-domain event and keeps the stream open; a later successful pool join wakes it, instead of the page retrying `WatchWorkers` every backoff interval. Transport failures still use bounded reconnect, with a one-shot worker-list fallback only during the outage. I also rebased this to a clean, linear branch on current `dev`. The shared-state domain mapper now has exactly one NODE case (it was duplicated in the old conflict resolution). The renderer receives the per-domain error without triggering a reconnect loop, so no-coordinator worker availability is expected and handled. /cc @rvnminers-A-and-N
Author
Owner

Rebased the three workspace live-state commits onto current dev. The overlap with the merged shared-state enum is resolved while retaining both NODE and LOCATION_DISCOVERY; the duplicate NODE case is absent. The focused shared-state-domain test passes. Fresh CI is starting; the temporary checkout has no standalone dependency install, so I did not claim a full renderer test run. /cc @rvnminers-A-and-N

Rebased the three workspace live-state commits onto current `dev`. The overlap with the merged shared-state enum is resolved while retaining both `NODE` and `LOCATION_DISCOVERY`; the duplicate `NODE` case is absent. The focused shared-state-domain test passes. Fresh CI is starting; the temporary checkout has no standalone dependency install, so I did not claim a full renderer test run. /cc @rvnminers-A-and-N
Author
Owner

Thanks for the careful review. I checked the current dev head (84ef65ec, including #749/#750): shared-state-domain.ts there has no NODE mapping, and this PR adds exactly one case 1 / SHARED_STATE_DOMAIN_NODE mapping. I also searched the PR diff and don't see a duplicate case; the earlier copy appears to have been removed during the restack.

On ordering, I would land this focused workspace PR directly on dev first, then restack the #741 stream stack on top. It is independently green and fixes the live workspace globe state; the stack is a separate series and should not need to be a prerequisite for it. I won't merge the daemon-backed stream stack before you review it.

WatchWorkers on a node without a coordinator ends with FailedPrecondition (apps/sisyphusd/api/local.go, with an explicit TestWatchWorkersRequiresAccessAndCoordinator). In this PR, that triggers one compatibility ListWorkers fallback and a bounded reconnect backoff (500 ms up to 30 s), not a fixed 30-second polling loop; once a stream update arrives the fallback is suppressed. The owner's pin has the same one-shot fallback behavior. The AccessPage draft dirty-state concern you raised on #753 is already addressed in the current PR diff by comparing effective draft values with the last saved config (accessDraftIsDirty), so reverting a toggle clears dirty state and remote updates replace a clean draft. #762 carries the separate coverage fixes you mentioned. Could you confirm you're comfortable with #761 landing first, and review the streaming stack when you have a chance?

/cc @rvnminers-A-and-N

Thanks for the careful review. I checked the current `dev` head (84ef65ec, including #749/#750): `shared-state-domain.ts` there has no `NODE` mapping, and this PR adds exactly one `case 1` / `SHARED_STATE_DOMAIN_NODE` mapping. I also searched the PR diff and don't see a duplicate case; the earlier copy appears to have been removed during the restack. On ordering, I would land this focused workspace PR directly on `dev` first, then restack the #741 stream stack on top. It is independently green and fixes the live workspace globe state; the stack is a separate series and should not need to be a prerequisite for it. I won't merge the daemon-backed stream stack before you review it. `WatchWorkers` on a node without a coordinator ends with `FailedPrecondition` (`apps/sisyphusd/api/local.go`, with an explicit `TestWatchWorkersRequiresAccessAndCoordinator`). In this PR, that triggers one compatibility `ListWorkers` fallback and a bounded reconnect backoff (500 ms up to 30 s), not a fixed 30-second polling loop; once a stream update arrives the fallback is suppressed. The owner's pin has the same one-shot fallback behavior. The AccessPage draft dirty-state concern you raised on #753 is already addressed in the current PR diff by comparing effective draft values with the last saved config (`accessDraftIsDirty`), so reverting a toggle clears dirty state and remote updates replace a clean draft. #762 carries the separate coverage fixes you mentioned. Could you confirm you're comfortable with #761 landing first, and review the streaming stack when you have a chance? /cc @rvnminers-A-and-N
Sign in to join this conversation.
No Reviewers
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Sisyphus/Sisyphus#761