Skip to content

fix(controller): propagate close-state to all shared-controller holders#24

Open
Nick Hehr (HipsterBrown) wants to merge 1 commit into
mainfrom
fix/controller-close-race
Open

fix(controller): propagate close-state to all shared-controller holders#24
Nick Hehr (HipsterBrown) wants to merge 1 commit into
mainfrom
fix/controller-close-race

Conversation

@HipsterBrown

Copy link
Copy Markdown
Contributor

Summary

The arm and gripper on one SO-101 share a serial port via the ref-counted controller registry. Previously each consumer got its own *SafeSoArmController struct copy wrapping the same bus. When one component closed first, ReleaseController closed the shared bus, but the other holder's struct had no way to observe it — its next call raced against / hit a closed bus.

This is the data-race fix extracted from #11 (which bundled several unrelated refactors). Only the race fix is here; the rest is intentionally left out (see below).

Changes

  • manager.go — add a closed atomic.Bool to SafeSoArmController plus an ErrControllerClosed sentinel. Every public method checks it up-front and returns the sentinel instead of touching a closed bus. Close() is now idempotent (sets closed).
  • registry.gogetExistingController / createNewController return the cached controller pointer instead of a fresh per-caller struct copy, so all holders share one closed flag. ReleaseController and ForceCloseController set closed=true before closing the bus, so surviving holders observe ErrControllerClosed atomically on their next call.
  • Tests — a scripted mock-bus harness (mock_bus_test.go, no hardware) + regression tests (lifecycle_test.go): the gated-method sentinel across all 9 methods, same-pointer-per-port, and release-closes-all-consumers.

checkClosed is best-effort (documented in-code): callers must hold a registry refcount for the duration of a call. The runtime.Caller-based release tracking is left intact here, so the fix is self-contained.

Explicitly out of scope (deferred from #11)

  • Replacing runtime.Caller release tracking with explicit port handles
  • Per-call ctx threading through init/diagnose/verify
  • The motion.FromProvider controller-leak fix and cancelCtx/cancelFunc cleanup
  • Design-spec / implementation-plan docs

Testing

  • gofmt -s clean, go vet ./cmd/module/ . clean
  • go test -race ./cmd/module/ . — full so_arm package passes under the race detector

Note

The cached-pointer approach means a second component reacquiring a port with a different calibration file updates the shared calibratedServos but not the controller struct's calibration field. This is pre-existing behavior carried over from #11's design, unrelated to the race, and only affects the rare reacquire-with-different-calibration path — flagged for awareness.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JNvy6dGUT6wBHLNBv8YPYv

The ref-counted controller registry handed each consumer (arm + gripper on
one SO-101 share a serial port) its own *SafeSoArmController struct copy
wrapping the same bus. When one component closed first, ReleaseController
closed the shared bus but the other holder's struct had no way to observe
it, so its next call raced against / hit a closed bus.

Fix, extracted from the lifecycle-correctness work in #11:

- Add a `closed atomic.Bool` to SafeSoArmController plus an ErrControllerClosed
  sentinel; every public method checks it up-front and returns the sentinel
  instead of touching a closed bus. Close() is now idempotent (sets closed).
- getExistingController / createNewController return the *cached* controller
  pointer instead of a fresh struct copy, so all holders share one closed
  flag. ReleaseController and ForceCloseController set closed=true before
  closing the bus, so surviving holders observe ErrControllerClosed atomically.

checkClosed is best-effort (documented): callers must hold a registry refcount
for the duration of a call. The runtime.Caller-based release tracking is left
intact here; replacing it was a separate change in #11 and is out of scope.

Tests: add a scripted mock-bus harness (no hardware) and cover the gated-method
sentinel across all 9 methods, same-pointer-per-port, and release-closes-all-
consumers. Full package passes under -race.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JNvy6dGUT6wBHLNBv8YPYv
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant