fix: teammate lifecycle robustness — stall detection, orphan recovery, deadlocks, merge/broadcast accuracy - #31
Closed
Agony1023 wants to merge 3 commits into
Closed
Conversation
…, deadlocks, merge/broadcast accuracy ## Summary Six independent fixes found while stress-testing the ensemble plugin's teammate lifecycle under real, extended multi-teammate sessions: a re-engagement gap on completed teammates, two stall-detection gaps on a teammate's first action, an orphaned-team reconciliation gap plus a deadlock its own fix introduced, a false-success report from `team_merge`, and a missing chatty-detection wire on `team_broadcast`. Each was found via live reproduction against a real running server, not inferred from code reading alone. ### `team_message` cannot re-engage a teammate that already reported completion Ensemble intentionally locks a teammate from being woken again once it reports completion to the lead (prevents a courtesy-reply ping-pong loop), but exposed no way to deliberately re-engage one for legitimate follow-on work — e.g. a later stage of an extended exchange with the same teammate. Adds a lead-only `force: true` param on `team_message` that bypasses the lock. The natural busy transition already resets the completion flag on the teammate's next idle, so `force` doesn't need its own separate reset path. ### Stall detection has zero coverage for a teammate's first action `isTimeStalled` requires at least one prior step-finish event to establish a baseline — a teammate blocked on its first (or only) long-running tool call never accumulates that baseline before a hard timeout, leaving the most likely stall pattern in practice (a long build, test, or file operation as a first action) completely undetected. Fixed in two passes: first wiring `recordBusyStart()` from the `ready/error -> busy` status-transition event, then discovering live that a freshly spawned member is inserted directly as `status='busy'` with no transition event to hook — so `recordBusyStart()` is now also called directly at spawn time. ### Orphaned-team reconciliation, and the deadlock its own fix introduced A team stays `active` forever if its lead session is deleted outside `team_cleanup` (e.g. via the host application's own session UI), blocking `team_create`/`team_cleanup` for that name indefinitely with no recovery path. Adds a periodic sweep plus an on-demand check in `team_create`'s duplicate-name check (the periodic sweep alone only runs at plugin init, missing the case where the bug is hit mid-session). The first version of this fix awaited the sweep synchronously during plugin init; its liveness check makes an HTTP call back to the same server, which — before the server finishes bootstrapping — hangs indefinitely and takes the whole instance down with it. Fixed by making the sweep fire-and-forget, matching the existing pattern for non-critical recovery passes, plus a bounded timeout on the liveness check itself as defense in depth. ### `recoverStaleMembers`'s own pre-existing instance of the same deadlock class Once the above was found, `recoverStaleMembers` — which predates this batch of fixes entirely — turned out to have the identical shape: awaited synchronously at init, with an unbounded `session.abort()` call back to the same server. It hadn't fired in practice only because its query happens to filter on a genuinely-busy member at restart time, not because of any structural guard. Fixed with a bounded timeout on the abort call, kept synchronous (unlike the fix above) because a later init step depends on this function completing first. ### `team_merge` reports success when there was nothing to merge A teammate that writes a file but never commits leaves its branch with zero new commits. `git merge --squash` against such a branch is a legitimate no-op that `team_merge` reported as a successful merge regardless — technically accurate, actively misleading: nothing was captured, and the uncommitted work is permanently lost the moment the worktree is removed, with no signal at that exact point. Now checks commit count before merging: zero commits with a dirty worktree produces an explicit warning naming the worktree path and leaves the branch/ worktree in place for manual recovery instead of attempting a merge or a delete. ### `team_broadcast` invisible to chatty-teammate detection The chatty-detection rate limiter tracks peer-message activity via a call that `team_message`'s handler makes but `team_broadcast`'s handler never did — broadcasting to the entire team, arguably the most "chatty" action possible, was completely invisible to the rate limiter. ## Tests added New and updated tests across `team-spawn`, `team-message`, `team-create`, `team-merge`, `recovery`, and `progress` covering: force re-engagement of a completed teammate (and rejection when a non-lead attempts it), stall-baseline recording at both the transition-event and spawn-time hook points, orphaned-team reconciliation via both the periodic sweep and the on-demand check, the fire-and-forget + bounded-timeout behavior for both deadlock-prone recovery passes, honest-vs-misleading `team_merge` reporting across the zero-commit/ dirty-worktree and zero-commit/clean-worktree cases, and broadcast activity now registering with the chatty-detection tracker. ## Verification - `bun run typecheck` — clean - `bun test` — 715 pass, 0 fail - `bun run build` — clean Each fix was additionally live-reverified against a real running server and real spawned teammates reproducing the exact failure mode described above, not just against the added unit tests.
Agony1023
force-pushed
the
bugfix/teammate-lifecycle-robustness
branch
from
August 17, 2026 21:29
933ce33 to
b2ea998
Compare
…2,3,4,6 + schema) Based on bugfix/teammate-lifecycle-robustness (this PR) — needs ProgressTracker.recordBusyStart()/hasReportedCompletion() from that branch; does not stand alone on main. - schema.ts: Migration 10 (this branch's migration count differs from local-integration's), additive last_nudged_at INTEGER on team_member. No CHECK constraint touched — dedicated test asserts the 5-literal status enum string is unchanged. - watchdog.ts: checkStalled()/checkChatty() write last_nudged_at at nudge-decision time; both now gate on hasReportedCompletion() before nudging, with a distinguishable skip-path log line; checkStalled() defers markReported() until promptAsync's delivery is confirmed (its .then(), not unconditionally before the call) so a failed delivery no longer permanently orphans the stall state. - index.ts: idle-without-report nudge's previously-silent promptAsync .catch() now logs a structured failure, matching this branch's other nudge paths. - team-status.ts: the 30s-per-team throttle no longer returns a hardcoded 'no changes' string unconditionally — computes a cheap per-team MAX(time_updated/time_created) marker and only short-circuits when nothing actually changed. 726/726 tests (up from this branch's own 720), typecheck clean, build clean. Companion dashboard-layer change (exposes last_nudged_at as lastNudgedAt in /api/state) is a separate commit against feat/dashboard-state-version-clean — that branch depends on this one's migration and should merge after it.
Contributor
Author
|
Pushed an additional commit (
All additive to the schema (no CHECK-constraint change), 726/726 tests, typecheck/build clean. Companion change exposing this via |
3 tasks
Contributor
Author
|
Pushed another additional commit extending this PR further while working nearby — persists a signal that was already being fetched-but-discarded: |
… (backend half) Based on this PR's branch -- needs no additional dependency beyond what's already here. session.status already has a "retry" case in handleSessionStatusEvent (fires a toast) -- the payload was thrown away immediately after. This persists it instead: - Migration 10: four additive, nullable columns on team_member (retry_until, retry_attempt, retry_provider, retry_message). No CHECK constraint change, no new status/execution_status literal. - handleSessionStatusEvent gains an optional retryPayload param; the existing retry branch persists the four columns in addition to the existing toast, not instead of it. status/execution_status untouched. - No hardcoded rate-limit/throttle language: retry_message prefers action.message, falls back to the generic status.message. - retry_until is a TTL; "currently retrying" is a derived, read-time boolean, never a stored enum -- no clear-write anywhere. - team-status.ts's status line gets an additive annotation for a retrying member, same pattern as the existing nudged-timestamp one. status.next resolved as an absolute epoch-ms timestamp (verified against the opencode server bundle's session.retry.scheduled handler and the built-in TUI's own countdown math). Non-goals: no watchdog/stall-detection changes -- checkStalled()/ ProgressTracker.isTimeStalled() never see this signal, by design. 727/727 tests, typecheck/build clean. Companion change surfacing this via /api/state and the dashboard UI is a separate commit against hueyexe#33.
Agony1023
added a commit
to Agony1023/opencode-ensemble
that referenced
this pull request
Aug 20, 2026
… + UI Depends on the retry_until/retry_attempt/retry_provider/retry_message columns added by the companion backend PR (against hueyexe#31) -- this branch's tests will fail standalone until that migration is present. Confirmed: the only failure mode is the retry_* columns not existing, nothing else broken. - dashboard.ts: additive isRetrying (derived, read-time TTL boolean -- never a stored enum), retryUntil, retryAttempt, retryProvider, retryMessage on the member row in /api/state. No version bump needed (additive-only, same reasoning as the earlier lastNudgedAt field). - dashboard-js-render.ts: additive "retrying" chip on agent cards and the member drawer, generic label + attempt number, only the actual provider message text if present -- no hardcoded rate-limit/throttle wording anywhere in this file either. Merge order: land the backend PR first, or rebase this on top of it.
hueyexe
pushed a commit
that referenced
this pull request
Aug 25, 2026
…32) (#35) ## Summary This branch is a full review-and-integration pass over **every currently-open PR** (#29, #31, #33, #34) and **every currently-open issue** (#26, #32). Each contribution was reviewed against a real running OpenCode server, reworked where needed, extended where the review surfaced gaps, and landed as a coherent set of commits. Merging this supersedes all four open PRs and closes both open issues. 36 files changed: +2111 / −104 across src and test, no external dependencies added. --- ### Reviewed from open PRs #### PR #31 — Teammate lifecycle robustness (`4feb15d`, `ab4d3bc`) Six fixes found by live stress-testing of the lifecycle, plus a follow-up hardening commit on top of the review: - `team_message` can now re-engage a teammate stuck in `completed`, closing the re-engagement gap - Stall detection covers a member's **first action**: spawn records its busy-start baseline directly (there is no `ready→busy` status-event for a fresh spawn), so a member stalled before its first step-finish is visible to `checkStalled()` - Orphaned-team reconciliation after crashes, including a deadlock the naive fix introduced - `team_merge` no longer reports false success when the merge silently fails - `team_broadcast` now wires chatty-detection like other send paths - Watchdog nudge display-staleness signal + logging parity (`last_nudged_at` schema, nudges visible in `/api/state`) #### PR #33 — Versioned `/api/state` for external consumers (`e8be872`, `4bab276`, `d4fc294`, `95714e8`) Landed the typed/versioned state contract, then extended it additively during review: - `buildState()` returns an exported, documented type; response carries a `version` field so external consumers can detect payload drift instead of crashing on it - Exposes `leadSessionId` (already persisted in the DB, never selected) so consumers can scope teams to the session that created them - Additive `lastNudgedAt` and provider-retry signals exposed in both the API payload and dashboard UI #### PR #34 — Task status + visualization fixes (`276d2bc`, `f0bc580`) - Task status is now reliably reflected across the whole teammate lifecycle: new `src/tasks.ts` centralizes claim/release/reassign, so tasks return to the board when a member stalls, errors, or is shut down instead of staying pinned to a dead assignee - Freshly-spawned teammates no longer stick at `[starting]` with a frozen timestamp — busy transitions bump execution status and time even when the member was inserted already-busy #### PR #29 — Nullish agent normalization ✅ verified The core fix (`agent: null | undefined` → `"build"` instead of hitting the NOT NULL constraint) was already merged via #30. This branch retains and extends its regression coverage in `test/tools/team-spawn.test.ts`; nothing further required. --- ### Closes open issues #### Issue #26 — Update agent parameters in-place (`009e868`) New `src/member-model.ts`: `team_message` accepts `{ model: "provider/model", to: "<member>" }` and hot-swaps that teammate's model without respawning or re-assigning tasks. Includes permission-safe plumbing through `tool.execute.before`, DB persistence of the override, and tests. Closes #26 #### Issue #32 — Dashboard shows 08:00:00 for historical timestamps (`f0bc580`) OpenCode v1 exposes message time as an object (`{ created: number }`) rather than a bare number; the activity view parsed the object as a number, fell back to epoch-0 rendering, and produced identical `08:00:00` stamps for historical entries. Timestamp extraction now handles both shapes. Fixes #32 --- ### Also in this branch - **Notify-the-lead primitive** (`621387f`): new `src/notify.ts` gives any client a direct channel to wake and message the lead; `team_spawn`'s failure rollback path now uses it, so lead notification survives cases where the old in-band message path could not fire - **Provider-retry observability** (`fb5f7d2`): session.status retry signals are persisted (backend half) and surfaced in the dashboard, so provider flapping is distinguishable from genuine stalls - Test-only scoping fix (`ec6a737`) and doc dedup (`89038ea`) left over from the integration work --- ### Testing - `bun run typecheck && bun test && bun run build` — all green - 783 tests / 38 files passing, including new suites for lifecycle recovery, watchdog staleness, task reassignment, model updates, notify paths, and merge-flow accuracy --------- Co-authored-by: Lennox McKenzie <lennox.mckenzie@gmail.com> Co-authored-by: Jared Davies <jared.davies@cesicorp.com>
Owner
|
Good work Lennox, all six fixes shipped in v0.17.0 through #35 which cherry picked your branch. One small follow up landed on main afterwards so a failed stall nudge cant wake the lead on every tick. Closing here since the work is live, you're credited in the release notes. Thanks for your support on this. |
Owner
|
Closing as shipped via v0.17.0 / #35. |
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
2 participants
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.
Summary
Six independent fixes found while stress-testing the teammate lifecycle under real, extended multi-teammate sessions: a re-engagement gap on completed teammates, two stall-detection gaps on a teammate's first action, an orphaned-team reconciliation gap plus a deadlock its own fix introduced, a false-success report from
team_merge, and a missing chatty-detection wire onteam_broadcast. Each was found via live reproduction against a real running server, not inferred from code reading alone — see the commit body for the fuller narrative on each.team_messagecannot re-engage a teammate that already reported completionThe existing completion lock is correct behavior (prevents a courtesy-reply ping-pong loop) but had no deliberate override for legitimate follow-on work with the same teammate. Adds a lead-only
force: trueparam that bypasses the lock; the existing busy-transition reset already handles clearing the flag afterward.Stall detection has zero coverage for a teammate's first action
isTimeStalledneeds a prior step-finish event to establish a baseline, so a teammate blocked on its first long-running tool call is invisible to stall detection until the hard timeout. Fixed in two passes — wiring the busy-transition event, then discovering live that a freshly-spawned member never fires that transition at all (inserted directly asbusy), requiring a second hook directly at spawn time.Orphaned-team reconciliation, and a deadlock its own fix introduced
A team with an externally-deleted lead session (deleted outside
team_cleanup) staysactiveforever with no recovery path. Added a periodic sweep plus an on-demand check atteam_create. The first version of this fix hung the whole server at init — its liveness check makes an HTTP call back to the same server before it's finished bootstrapping. Fixed with fire-and-forget dispatch plus a bounded timeout on the liveness check.recoverStaleMembers's own pre-existing instance of the same deadlock classSame deadlock shape, pre-existing in code this PR doesn't otherwise touch — awaited synchronously at init with an unbounded
session.abort()call. Never fired in practice by coincidence, not design. Fixed with a bounded timeout, kept synchronous since a later init step depends on it.team_mergereports success when there was nothing to mergeA teammate that writes but never commits leaves a branch with zero new commits;
git merge --squashagainst it is a no-op that reported success anyway. Now checks commit count first — zero commits + dirty worktree produces an explicit warning and leaves everything in place for manual recovery instead of silently losing the work.team_broadcastinvisible to chatty-teammate detectionteam_message's handler feeds the chatty-detection rate limiter;team_broadcast's never did, so the single most "chatty" action possible was invisible to it.Tests added
Coverage across
team-spawn,team-message,team-create,team-merge,recovery, andprogress— force re-engagement (and lead-only rejection), stall-baseline recording at both hook points, orphaned-team reconciliation via both the sweep and the on-demand path, fire-and-forget + timeout behavior for both deadlock-prone recovery passes, honest-vs-misleadingteam_mergereporting across zero-commit cases, and broadcast activity registering with chatty detection.Verification
bun run typecheck— cleanbun test— 715 pass, 0 failbun run build— cleanRebased onto current
main(postv0.16.1/#30) — verified no interaction with the null-agent-spawn and task-claim-accuracy fixes in that release; all tests pass together.Each fix was additionally live-reverified against a real running server and real spawned teammates reproducing the exact failure mode described above, not just against the added unit tests.