GO-7317 Refactor space controller: per-space reconciler, lazy loading by design - #3174
Open
requilence wants to merge 7 commits into
Open
GO-7317 Refactor space controller: per-space reconciler, lazy loading by design#3174requilence wants to merge 7 commits into
requilence wants to merge 7 commits into
Conversation
Merge the personal/shareable/streamable controller copies into a single accountspace implementation parameterized by a Descriptor. Move the personal space-view bootstrap into spacefactory, fix the swallowed streamable key-decode error, and drop dead code (Personal interfaces, WaitMigrations, controller Delete methods, personalLoader wrapper). Phase 1 of the spec in docs/SpaceControllerSpec.md: behavior-preserving, mode.StateMachine untouched.
A single-writer reconciler goroutine converges the running process to computeTarget(status, demand), latest-wins. This fixes the dropped transitions (ErrTransitionInProcess + lastUpdatedStatus pre-set), forever-blocked ChangeMode waiters on Close, and swallowed start errors of the old state machine. Update() no longer blocks on transitions; Start() waits for first convergence and returns the real error. CanTransition is removed: every production process returned true, so offloading was never actually terminal (re-entrant Offloaded per spec). Phase 2 of docs/SpaceControllerSpec.md.
Every space view now registers a controller at startup; controllers sit dormant (ModeInitial) until demanded. Loading triggers are demand wiring: eager mode and the preferred space demand at registration, Get/Wait demand via the new SpaceController.WaitLoad, and the preload release (RPC/timer/preferred-broken fallback) demands all registered controllers. Status-driven offloading/joining proceeds without demand. Deletes the lazy bolt-on from service.go: deferredStatuses backlog, releasing flag, drainDeferred, ensureSpaceStarted, resolveDerivedInfo, and both test-seam hooks. Factory New* methods now construct without starting; Create* flows still start (direct user intents). Adds Demand() and WaitLoad() to the SpaceController interface, removing the Current().(loader.LoadWaiter) assertions in create paths. Phase 3 of docs/SpaceControllerSpec.md.
…istry Join/InviteJoin/AddStreamable/create/CreateOneToOne now only write the desired state into the space view; controllers are registered exclusively by the watcher (plus account-init), then waited on through an event-driven registry (waitCtrl on a broadcast channel). This deletes the waiting singleflight map whose cached errors poisoned a space id for the whole session — registration failures are now stored per-id and cleared on the next attempt — and the 500ms polling space waiter (Wait == Get). The watcher starts before the first space is created so view events are never missed. Factory Create* methods are view-only now; CreatePersonalSpace/CreateInvitingSpace/ CreateActiveSpace were dead or replaced and are removed. Phase 4 of docs/SpaceControllerSpec.md.
Remove Current() any from SpaceController — all consumers now use the typed WaitLoad/WaitMode surface. The deprecated marketplace controller moves out of the controller registry to a service-level special case (like tech space), removing its skip-checks from AllSpaceIds and AllLoadedSpaceIds; full removal stays tracked by GO-6259. Phase 5 of docs/SpaceControllerSpec.md (the loader/offloader retry loops stay separate on purpose: their policies differ — exponential backoff with non-retryable classification vs fixed-interval ticker).
Three parallel reviews (concurrency, semantic regressions, spec conformance) of the refactor surfaced these issues, all fixed here: - Reconciler: a failing transition no longer clobbers an input change that raced it (generation guard) — previously a Delete racing a failing load could park the controller and the space never offloaded. A repeated Demand (user retry via Get/WaitLoad or Join) now clears a parked failure, so failures are never session-permanent. - startStatus is single-flight per id again (constructing map): the removed waiting-map guard had left double-construction possible if the subscription layer ever delivered two events for one view with different mutex instances. Also re-checks isClosing at commit so a registration racing Close cannot leak a controller. - Join/InviteJoin: a swallowed ErrSpaceViewExists creation race now counts as exists so the intent write is never skipped; both (and AddStreamable) wait for their process via the new SpaceController.WaitMode, restoring start-error surfacing that the lazy path had lost (spec's WaitState). - Get is fast again (map-only, no view lookup) while Wait keeps the event-driven registration wait — Get on unknown ids no longer risks a remote view probe. - Restored spaces are no longer remote-deleted: the offloader removes the space from the deletion queue when it closes (CancelLeave → reload), resolving the spec §7 open question (pre-existing exposure). - Marketplace controller Start is no longer called twice; ErrCtrlClosed maps to ErrSpaceIsClosing at the service boundary.
…deletion-queue removal waitIntentMode now returns ErrSpaceIsClosing instead of leaking the internal ErrCtrlClosed sentinel from Join/InviteJoin/AddStreamable, and deletioncontroller gains a regression test asserting a space removed from the queue (restored via CancelLeave) is not remote-deleted.
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.
Linear: GO-7317
Design spec:
docs/SpaceControllerSpec.md(in this PR), research notes indocs/SpaceControllerRefactor.md.Why
The space lifecycle machinery had become error-prone: three near-identical
SpaceControllerimplementations, a blockingmode.StateMachinethat dropped transitions and swallowed errors, awaitingmap that cached failures for the whole session, a 500 ms polling waiter — and lazy multi-space loading bolted on with ~150 lines of lock choreography whose invariants lived in comments.What
Seven commits, one per phase (each independently reviewable):
accountspaceimplementation; kind differences (personal/streamable/one-to-one) areDescriptordata. Dead code dropped.computeTarget(status, demand), latest-wins. Fixes dropped transitions, waiters stranded on Close, and swallowed start errors.CanTransitionremoved (every production process returnedtrue).deferredStatuses/releasing/ensureSpaceStarted/resolveDerivedInfoand both test hooks fromservice.go.waitingsingleflight (and its session-permanent error poisoning) and the polling waiter are deleted.Current() anyremoved (WaitLoad/WaitMode); marketplace leaves the registry (service-level special case, like tech space).ErrCtrlClosedmapped at the service boundary; deletion-queue regression test.Net −500 LOC.
Intentional behavior changes
CanTransitionalready allowed).Update()never blocks on transitions; errors surface throughStart/WaitLoadwith their real cause; registration failures are retryable.Removing(previously only shareable did).AllSpaceIdsincludes dormant spaces (coordinator status checked for them, matching eager mode).Testing
space/...green with-race; new regression tests for the reconciler (input-races-failure, demand-clears-failure, dormant/offload-without-demand), registration single-flight, register/release race, and deletion-queue removal.