fix(store,supervisor): complete the instance handover on entry update so hot installs do not strand consumers - #593
Merged
Conversation
skhaz
approved these changes
Aug 25, 2026
Every store manager's Update recreates the store instance but only the supervisor learns about it. The resource registry keeps its active generation pointing at the previous instance, so consumers acquiring the store through the registry are handed the superseded object forever. For the memory store the previous instance is stopped first, and its Acquire reports resource.ErrReleased once closed, so the store is unusable for the rest of the process lifetime. For the SQL store the stopped instance is handed out with its cleanup goroutine gone; for the raft and crdt kv stores the superseded namespace view is handed out. Any hot module install bumps entry meta, which fires Update, so a single upgrade strands every consumer of an affected store until a restart. The registry already handles resource.Update by publishing a new acquisition generation while existing borrows keep the old one, which is exactly the transition these managers need. Send that event after installing the new instance, matching service/pg, tokenstore, jet, exec/native, exec/docker, aws/config and aws/s3. Claude-Session: https://claude.ai/code/session_01P1t1gnD4Qwd4afzCwwmtQj
Emitting resource.Update repointed the resource registry but left the supervisor side of a store update broken, so the handover was still only half done. supervisor.ServiceUpdate has no inbound handler. The supervisor consumes ServiceRegister, ServiceRemove, ServiceStart and ServiceStop, and uses ServiceUpdate for its own outbound state notifications carrying a State payload. Every store manager sent ServiceUpdate with a *supervisor.Entry, so the replacement instance never reached the supervisor at all. Switching those managers to ServiceRegister is not enough on its own: execute creates a controller only when none exists for the ID, so a registration naming a live ID was dropped and the controller kept supervising the superseded instance. Remove plus register cannot express the handover either, because entry changes are applied inside a registry transaction and registerService clears a pending remove for the same ID, collapsing the pair back to a bare register. The supervisor now retires a controller whose incoming registration carries a different service instance, letting the create pass adopt the replacement and start it. Re-registering the instance already supervised stays a no-op, so nothing churns. With the supervisor owning retirement, the memory and SQL managers no longer stop the superseded store themselves. The replacement is published before anything stops, so the resource registry never has a stopped provider as its active generation and the eventual-consistency window where an acquisition could observe ErrReleased is gone. The bus drops sends on a cancelled context, and its Send reports nothing, so each Update now fails loudly on a cancelled context instead of installing a replacement whose handover events would be discarded. Claude-Session: https://claude.ai/code/session_01P1t1gnD4Qwd4afzCwwmtQj
…once retirement, ordered repoint Round 3 of the hot-handover fix: a stop failure rejects the whole replacement transaction (no half-commit); the stop batch covers the dependent closure so a running dependent never runs against a stopped dependency; the supervisor owns the resource repoint at adoption (single ordering authority - no cross-subscriber race); retired controllers cancel their contexts (no goroutine leak); retirement skips already-stopped services (pg/terminal self-stopping paths keep exactly-once Stop); instance identity is comparable-safe for value implementations. Claude-Session: https://claude.ai/code/session_01P1t1gnD4Qwd4afzCwwmtQj
…tirement Three residual gaps in the handover, all in code this branch introduced. The resource acknowledgement was matched by resource path. Registrations and updates for one resource share a path, so an outcome published for an earlier operation could satisfy a later update's wait, and two waits on one resource overwrote each other in the await dispatcher. A resource operation now carries an OpID and the registry publishes its outcome under that id, so a reply can only satisfy the operation that asked for it. Callers that do not wait leave OpID empty and keep publishing under the resource path. A rejected retirement restored the services it had already stopped, but a restore that itself failed was only logged. A service that was running before the commit and is not running after it is the worst outcome this path produces, so the restore result now travels with the rejection and names the services left down. Controller status is bookkeeping, not proof that a manager left its instance running. pg and terminal stopped their own instance during Delete while the supervisor still owned it, so a delete-then-register update handed that instance a second Stop once the retirement started working. Stopping belongs to the supervisor: ServiceRemove stops the service on commit, and an update that re-registers a replacement in the same transaction retires it there. Both managers now leave it alone. Claude-Session: https://claude.ai/code/session_01P1t1gnD4Qwd4afzCwwmtQj
Misspellings, test struct field alignment, and comment wording reduced to the constraints the code cannot show. Claude-Session: https://claude.ai/code/session_01KqtSXDfDjEv6GYoAntvDhy
wolfy-j
force-pushed
the
fix/store-managers-resource-update
branch
from
August 25, 2026 18:12
9d96698 to
6e149a5
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
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.
Root cause
Updating a registry entry for a store must hand the running instance over to a replacement. Two independent halves of that handover were broken, so a single hot module install (which bumps entry meta and fires
Updateon every affected entry) left the replacement orphaned.Resource half. The managers recreated the store but never sent
resource.Update, sosystem/resource/registry.gokept its active generation pointing at the superseded instance. Forservice/store/memorythe superseded instance was stopped first andStore.Acquirereturnsresource.ErrReleasedonceclosedis set, so the store was unusable for the rest of the process lifetime.Supervisor half.
supervisor.ServiceUpdatehas no inbound handler. The supervisor consumesServiceRegister,ServiceRemove,ServiceStartandServiceStop(system/supervisor/supervisor.gohandleEvent), and usesServiceUpdatefor its own outbound state notification carrying aStatepayload. All four store managers sentServiceUpdatewith a*supervisor.Entry, so the replacement never reached the supervisor: itsStartwas never called and the controller kept supervising the stopped original. Cleanup loops stayed dead and shutdown targeted the stale instance.Switching to
ServiceRegisteralone does not fix it either.executecreates a controller onlyif _, exists := s.controllers[id]; !exists, so a registration naming a live ID is silently dropped. And remove-plus-register cannot express the handover, because entry changes are applied inside a registry transaction (system/registry/runner/bus_runner.gowraps the whole ChangeSet inTxBegin/TxCommit) andregTx.registerServiceclears a pending remove for the same ID, collapsing the pair back to a bare register. That is whyservice/pg'sDelete+Addmodel, and theServiceRemove-then-ServiceRegisterpattern inservice/queue/amqpandservice/aws/sqs, could not hand over either.Field incident
After a hub hot install on prod C, the settings cache (a
store.memoryentry) went dead: every read through the resource registry returnedresource has been released, and stayed dead until the instance was restarted. The entry itself was healthy; the registry was pointing at the stopped*Store, and the supervisor was still holding it too.Lifecycle design
The supervisor owns service lifecycle, so it owns retirement. An entry update expresses exactly one thing — "this ID is now served by this instance" — and the supervisor performs the handover.
Manager side. The manager builds the replacement, installs it in its own map, then:
service/store/await.go).supervisor.ServiceRegisterwith the replacement.Ordering falls out of step 1. The registry and the supervisor are independent bus subscribers with their own goroutines, so publishing gives no ordering against the supervisor retiring the old instance at commit. Returning only once the registry serves the replacement puts the repoint strictly before the
TxCommitthat stops the old one. The confirmation is an event, not polling: the registry publishesresource.accept/resource.rejectafter its state is visible toAcquire.The wait is correlated by a per-operation
Entry.OpID, which the registry echoes as the outcome event's path. Registrations and updates for one resource share a resource path, and the await dispatcher keys pending waits by path — so a path-keyed wait could be satisfied by an earlier operation's outcome, and two waits on one resource would overwrite each other. Callers that do not wait leaveOpIDempty and keep publishing under the resource path, so the eight other resource managers are unaffected.A dropped or unapplied repoint therefore fails the entry operation and rolls the manager's map back, instead of half-applying. This replaces an earlier
ctx.Err()guard that was dead code: the listener context is the router's long-lived context, not the caller's, so it can never be cancelled at that point.Supervisor side. On commit,
planReplacementscomputes the retirement without side effects, andapplyReplacementsperforms it:sameServiceInstance; comparable dynamic types answer identity directly, maps by pointer, anything unprovable counts as a replacement).buildStopOperationswalks dependents first, which is also the order the sequencer needs).cancel()ed, ending their supervise goroutine and releasing the superseded service; dropping the map entry alone is not the lifetime.Exactly-once retirement. Controller status is bookkeeping, not proof about the instance, so exactly-once cannot rest on it alone.
pgandterminalstopped their own instance insideDeletewhile the supervisor still owned it, which left the controller believing the service was running; once retirement started working, their delete-then-register update handed that instance a secondStop. Stopping belongs to the supervisor —ServiceRemovestops the service on commit, and an update that re-registers a replacement in the same transaction retires it there — so both managers now leave it alone.serviceNeedsStopadditionally skips a controller already known to be down.Sites fixed
Engine:
system/supervisor/supervisor.go—planReplacements,applyReplacements,restoreStopped,serviceNeedsStop,sameServiceInstance, called fromexecutesystem/supervisor/controller.go—Controller.Service()accessorsystem/supervisor/errors.go—NewRetirementRestoreErrorsystem/resource/registry.go— publishesresource.accept/resource.reject, under the operation id when one is carriedapi/resource/resource.go—Entry.OpID,Accept/RejectkindsStore managers (
ServiceUpdatetoServiceRegister, confirmedresource.Update, no manager-side stop):service/store/memory/manager.goservice/store/sql/manager.goservice/store/kv/manager.go(RaftManager)service/store/kv/crdt_manager.go(CRDTManager)service/store/await.go—AwaitResourceUpdateManagers that stopped instances the supervisor owns:
service/pg/manager.goservice/terminal/manager.goTest coverage
service/store/await_test.go:IgnoresOutcomesOfOtherOperations— a stream of accepts on the resource path must not confirm this operation. Red with path-only correlation: "An error is expected but got nil".ConfirmsAppliedUpdate/ReportsRejection— the confirming and declining paths against the real registry.system/supervisor/handover_test.go:RegisterAdoptsReplacementInstance— red: "timeout waiting for service to start".RegisterKeepsIdenticalInstanceRunning,TestSameServiceInstance— no churn; value and pointer identity both directions.ReplacementRetiresRunningDependents— A→B, replace only B; A is stopped and restarted. Red: "timeout waiting for dependent to be restarted".RejectedRetirementRestoresRunningSet— a stop fails; the supervisor keeps its controllers and the service it stopped comes back. Retries disabled so the controller cannot mask a lost service.RejectedRetirementReportsUnrestoredServices— the restore also fails; the failure is visible in supervisor state and in the commit outcome. Red with a log-only restore: "timeout waiting for the rejected commit to report the unrestored service".RetirementCancelsController— 10 hot updates leave one controller and no leaked goroutines. Red: "timeout waiting for retired controllers to release their goroutines".DeleteThenRegisterStopsInstanceOnce— the pg/terminal update shape with a non-idempotentStopthat errors on a second call; asserts exactly one stop of the superseded instance and none of the adopted one. Red with retirement disabled: "timeout waiting for replacement to start".service/store/{memory,sql,kv}/manager_resource_test.go— each drives the real supervisor, real resource registry and real await service over a shared bus, inside realTxBegin/TxCommittransactions: the registry serves the replacement, the superseded store is stopped, the replacement runs, and a laterServiceStopreaches the replacement. Plus the emittedresource.Entryshape (path, id, meta) and the two failure modes (unconfirmed handover, no coordination available) with rollback. kv is table-driven over raft and crdt.Validation
For reviewer attention
ServiceUpdateremains an outbound-only state notification with no inbound handler. Managers outside this change still publish*supervisor.Entryunder that kind, where it is silently ignored — pre-existing, and the same class of bug this PR fixes for the store managers. Worth a follow-up sweep.https://claude.ai/code/session_01P1t1gnD4Qwd4afzCwwmtQj