Skip to content

fix(core): keep subagent worker lock off child-runtime I/O - #259

Merged
akonwi merged 1 commit into
mainfrom
core/concurrency-hardening
Oct 22, 2026
Merged

akonwi merged 1 commit into
mainfrom
core/concurrency-hardening

Conversation

@akonwi

@akonwi akonwi commented Oct 22, 2026

Copy link
Copy Markdown
Owner

First of three concurrency-hardening PRs.

Changes

  • Subagent Shutdown can exceed its deadline: worker lock held across child-runtime I/O #244: worker.mu no longer wraps child-runtime I/O, so Shutdown(ctx) honors its deadline and Cancel/Dismiss don't wait behind a slow runtime.
    • Steer, Transcript and ConfigureThinking take a runtime lease under the lock and call the runtime without it.
    • execute refuses new leases, waits for in-flight calls to return (each is bounded by its caller's context), then calls Close unlocked.
    • Shutdown drops the serial cancelWorker pass before its select; s.cancel(errSupervisorStop) already cancels every worker context.
  • Subagent cancellation generation can regress under concurrent Cancel #245: the cancellation generation now only moves forward (compare-and-swap), so claim reconciliation can't overwrite a newer Cancel/Dismiss generation.
  • Subagent half of Track two archived flaky Go tests (subagent model change, plugin pipelined init) #246: a finished worker held its conversation lock through MailboxAdded, so a model change made as soon as the conversation settled could get ErrConflict. That's what made TestSupervisorPreparesSettledModelChangeAndPreservesThinking flaky: a 50 ms delay after the running removal reproduces it 3/3. The lock is now released after the durable completion and child-store cleanup, before the worker leaves running.
  • Docs: updated the ChildRuntime contract and the go-concurrency skill (lock order; removed the known exceptions this PR fixes).

Tests

New file internal/subagent/supervisor_lifecycle_test.go. Each regression test fails deterministically on main and passes here; the waits use channel gates, not timing:

  • Shutdown deadline while Close blocks: blocked >5 s on main.
  • Cancel during an in-flight Steer: blocked on main.
  • Generation regression: first completion used generation 1, want 2.
  • Model change while parent notification runs: subagent state conflict on main.
  • One more test guards the new behavior: Close waits for an in-flight Steer. Removing the drain makes it fail.

Validation

macOS arm64, disposable KIT_HOME:

  • gofmt -l . (clean), go build ./..., go vet ./....
  • go test -race -count=50 ./internal/subagent/: pass.
  • go test -race -count=5 ./internal/subagent/ ./internal/session/: pass.
  • go test -race -count=20 -run TestSupervisor ./internal/subagent/ after review fixes: pass.
  • Full go test ./... and go test -race ./...: everything passes except TestObserverPRCompletionPushesWithoutGitPollOrClientRead, a known flake (Flaky: TestObserverPRCompletionPushesWithoutGitPollOrClientRead times out under full-suite load #236) that the next PR fixes.

The code-reviewer subagent found nothing blocking. Its nits are addressed: Steer now releases its lease with defer, and a probe test fails instead of hanging if the bug returns. It also found a pre-existing dismissal/cleanup window, filed as #258.

#246 stays open until the plugin half is resolved in the third PR.

Closes #244
Closes #245

Supervisor.Shutdown could exceed its deadline, and Cancel/Dismiss could
block behind a slow child runtime, because worker.mu was held across
runtime Close, Steer, Transcript, and ConfigureThinking, while
cancelWorker needed it before Shutdown reached its bounded wait.

worker.mu now guards only short state flips. Runtime calls take a lease
under the lock and run without it; execute stops new leases, waits for
in-flight calls to return, then closes the runtime unlocked. Shutdown
relies on s.cancel to cancel every worker context instead of a serial
per-worker pass before its select.

The cancellation generation is now advanced with a compare-and-swap so
claim reconciliation cannot overwrite a newer Cancel/Dismiss generation
with a value it read earlier (#245).

A finished worker kept its conversation lock through parent
notification (MailboxAdded), so a model change made as soon as the
conversation settled could fail with ErrConflict. The worker now
releases the lock after the durable completion and child-store cleanup,
before leaving the running set (#246).

Closes #244
Closes #245
@akonwi
akonwi merged commit f3fb5ca into main Oct 22, 2026
9 of 10 checks passed
@akonwi
akonwi deleted the core/concurrency-hardening branch October 22, 2026 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant