fix(mastery): bound workflow execution without blocking retries

[Raghav-Bansal-15]

Workflow admission, fanout, retained output, and cancellation cleanup needed limits. Retry backoff also held capacity that ready work could use.

Keep capacity until cleanup finishes, release invocation slots during backoff, and report validation and timeout failures explicitly. Split timed invocation from retry handling and wrap the regression assertion to fix the reported quality failures without suppressing checks.

Integrated current dev while retaining original and reviewer authorship.

Current dev integrated; cancellation, validation and resource ceilings retained.

Validation: Mastery SDK/service tests, ktlint and detekt pass locally.

Current-head fork CI passed at 83d7cd0d: 🔨 Build & Test. Upstream approval/checks remain separate merge gates; no merge or release performed.

[kshivang]

@claude review Please review the admission, cancellation cleanup, fanout and retained-output limits in this standalone PR against current dev.

[claude]

Claude finished @kshivang's task in 5m 51s —— View job

---

Review: admission, cancellation cleanup, fanout and retained-output limits

• Read the diff (no merge base with origin/dev - this is a standalone branch, so I reviewed the single commit fcca7a6 and the resulting files in full)
• Review MasteryServiceImpl admission + cancellation cleanup
• Review MasteryExecutor fanout + output limits
• Review the two new test suites
• Post findings

Not verified by execution. ./gradlew and git fetch were denied in this sandbox, so I could not run :boss-mastery-sdk:test, :boss-mastery-orchestrator:test, ktlintCheck or detekt. Everything below is from reading the code. Line lengths are all under the .editorconfig 140 limit and I found no U+2014 in added lines.

---

What this gets right

Worth recording, because three of these are things that are usually gotten backwards:

• Admission is genuinely atomic. The definition lookup, the capacity check, the runningJobs registration and the first updateStatus all happen in one synchronized(stateLock) block, with no suspension point inside it, before the first emit. That is a real improvement over the old comment-only "register before any suspension point" guarantee on a ConcurrentHashMap.
• Permit acquisition sits outside withTimeout (MasteryExecutor.kt:71 vs :121), so time spent queueing for a fanout slot does not eat the node's own deadline. Easy to get inverted.
• The retention budget is charged before the output is retained or emitted (MasteryExecutor.kt:107 precedes both the NodeCompleted emit and nodeOutputs[nodeId] = output), so a rejected output is never buffered downstream. oversized output is refused before progress retains it asserts exactly that.
• The history eviction invariant holds. key !in runningJobs plus the historyLimit >= executionLimit precondition means a single iterator pass can always reach the cap (exhaustion with size > historyLimit would require runningCount > executionLimit). And because LinkedHashMap re-put does not refresh insertion position, a terminal-state write does not move an entry to the back of the eviction queue - correct for a history ordered by start time. I traced this; it's sound.

---

Findings

1. Validation failures reach the client as UNKNOWN with no message (high)

validateDefinition (MasteryServiceImpl.kt:379-390) mixes two error channels for the same class of problem:

require(request.description.length <= 4096) { "Mastery description exceeds the size limit" }   // IllegalArgumentException
if (request.serializedSize > 65_536 || ...) { throw masteryLimit("...") }                       // StatusRuntimeException

The orchestrator installs no status-mapping interceptor - the only one in the tree is boss-ipc's ProcessIdentityInterceptor - so grpc-java turns an uncaught IllegalArgumentException into UNKNOWN: Application error processing RPC and drops the description. So maxRetries = 6 gives a caller an opaque UNKNOWN (which many client retry policies treat as retryable), while an oversized inputSchemaJson gives a clean RESOURCE_EXHAUSTED. Same issue for listMasteries' require(request.offset >= 0 && request.limit >= 0) at :160.

The new test pins the asymmetry rather than catching it, because it exercises the in-process object and never the wire:

assertFailsWith<StatusRuntimeException> { ...setInputSchemaJson("x".repeat(65_537))... }
assertFailsWith<IllegalArgumentException> { ...setMaxRetries(Int.MAX_VALUE)... }

These should all be Status.INVALID_ARGUMENT.withDescription(...).asRuntimeException(). Fix this →

2. The retry backoff holds a fanout permit (high)

MasteryExecutor.kt:71 wraps the whole of executeNode - and therefore invokeWithRetries - in slots.withPermit { }, and the backoff is inside it:

if (willRetry) delay(1_000L * (attempt + 1))   // :137, holding one of 8 permits

With maxRetries = 5 (the ceiling validateDefinition allows) a single failing node sleeps 1+2+3+4+5 = 15s while occupying a slot and doing no work. Eight such nodes stall every other ready node in the level for 15 seconds. The permit should bound concurrent invocations, not concurrent sleeping - move the delay outside the permit, or acquire/release per attempt. Fix this →

3. A node timeout escapes as a raw TimeoutCancellationException (high)

invokeWithRetries rethrows every CancellationException (:124-125), and TimeoutCancellationException is one. Two consequences:

• maxRetries never applies to a timeout - the single most common transient failure a retry loop exists for. A definition asking for 5 retries gets one attempt.
• No MasteryProgress.Failed event is emitted. The exception bypasses catch (e: NodeExecutionException) at :86 and terminates the stream, and grpc-kotlin maps a CancellationException to CANCELLED - the same code a client sees when it cancels - while getMasteryStatus reports "failed". Two surfaces, one event, opposite stories.

node timeout is a failure and releases execution capacity pins this (assertFailsWith<TimeoutCancellationException>). Catching TimeoutCancellationException ahead of the general clause and converting it to a retryable NodeExecutionException would fix both - guarding with currentCoroutineContext().isActive so an outer cancellation that happens to surface as a timeout is still rethrown. Note the finalState = if (coroutineContext.isActive) "failed" else "cancelled" trick at :108 exists precisely to paper over this; it gets the status right and leaves the wire wrong. Fix this →

4. A cancelled-but-wedged execution holds its slot forever, invisibly (medium)

Keeping the slot until cleanup finishes is the right call and the PR is explicit about it. But cancelMastery no longer writes any status (the updateStatus(..., "cancelled") line was removed), so:

• cancelMastery returns success = true while nothing has been released;
• getMasteryStatus keeps answering "running", indistinguishable from "the cancel never arrived";
• a node inside a NonCancellable or a blocking call permanently burns 1 of only 8 process-wide slots, with no forced reclamation and no operator-visible signal. The new test's fake resolver models exactly this shape and then releases it; production has nothing that does.

A "cancelling" intermediate state written by cancelMastery costs nothing (the finally already overwrites it) and makes the distinction observable.

5. Admission refusals are not logged at all (medium)

Both logger.info("Starting mastery execution: ...") and logger.warn("ExecuteMastery: definition not found: {}") were deleted, and none of the three new refusal paths - definition cap, execution cap, oversized request - log anything. An operator whose workflows start failing with RESOURCE_EXHAUSTED has nothing in the log to tell them a cap was hit versus a downstream fault. For a resource-exhaustion guard these are the events most worth recording.

6. The fanout bound is a hardcoded literal, and the real bound is a product (medium)

val slots = Semaphore(8) (MasteryExecutor.kt:47) is per-execute(), so with executionLimit = 8 the process-wide ceiling is 64 concurrent capability invocations, not 8. And unlike MasteryServiceImpl's three injectable limits, this one cannot be configured or overridden. Making it a MasteryExecutor constructor parameter defaulting to 8 would (a) match the orchestrator's style, (b) let parallel level admits at most eight nodes use 2 instead of 20 and drop its delay(50) timing assumption, and (c) allow a shared semaphore if you want one global number. Separately, MasteryOrchestratorMain.kt:55 constructs MasteryServiceImpl(masteryExecutor) with bare defaults - no env override for any of the four limits.

Also worth noting: the semaphore bounds concurrent invocations, not coroutine allocation - level.map { async { ... } } still creates one coroutine per node. That is bounded to 128 via nodesCount for orchestrator traffic, but unbounded for direct SDK callers, who never pass through validateDefinition.

7. Smaller items

• Status.NOT_FOUND.asRuntimeException() (:87) carries no description - unlike the other two throw sites. The mastery id is client-supplied, so naming it discloses nothing.
• validateDefinition's KDoc is wrong. "Limits retained definitions and the amount of parallel work each execution can fan out into" describes neither thing it does; the definition cap is in createMastery and the fanout cap is the SDK's semaphore. It validates one definition's size, retry counts and timeouts.
• reserveOutput duplicates its own size check (:150-158): the if (output.size <= 1024) ... else 0 computes a sentinel that the when's first branch then discards. if (output.size > 1024) throw ... up front, then sum unconditionally, reads better and removes the implication that a zero-character output is meaningful.
• The combined init { require(...) } produces Failed requirement. with no indication of which of the three bounds was violated. Three requires with messages.
• cancelMastery has a small honesty race: it reads the job under the lock, releases, then cancels. If the execution's finally is blocked on that same lock, the cancel lands on an already-complete scope, the response says success = true, and the status settles as "completed". Cancelling inside the lock, or re-checking membership after, makes the response match reality. Minor, but this PR is specifically about cancellation semantics.
• Node identity is still unvalidated. Duplicate node ids, blank ids, and a node literally named INPUT (which shadows the virtual source key in nodeOutputs) all pass validateDefinition. Pre-existing, but this is the validation pass that would naturally own it.

8. Test gaps

The suites cover capacity, fanout, output limits, cancellation and reuse as claimed. Untested:

• the new 64 KiB ExecuteMasteryRequest bound (:78) - a new limit with zero coverage; the existing oversize test only exercises createMastery;
• nodesCount > 128 and edgesCount > 512;
• the output.size > 1024 entries branch of reserveOutput;
• the reserveOutput("INPUT", ...) rejection at :50;
• that a fanout permit is released when a node throws (only success paths release in the current tests);
• init's historyLimit >= executionLimit precondition.

parallel level admits at most eight nodes relies on while (entered.get() < 8) delay(10) plus a bare delay(50) on Dispatchers.Default. The 10s withTimeout bounds the damage and the assertion can only fail if a permit leaks, so it is not truly flaky - but making the semaphore size injectable (item 6) would let it be deterministic and ~20x cheaper.

---

Summary: the core mechanism is sound - admission is properly atomic, the budget is charged before retention, and the eviction invariant holds under the stated preconditions. The three things I'd want addressed before merge are the IllegalArgumentException → UNKNOWN mapping (1), the permit held across backoff (2), and timeouts bypassing both retries and the Failed event (3). Items 4 and 5 are about making the deliberate "hold the slot" choice observable, which it currently isn't.
· fix/security-mastery-limits

[kshivang]

@Raghav-Bansal-15 Bounded review handoff: the implementation and regression repairs are pushed in 31928dc08, but this PR is not ready until the integrated head is validated. I am converting it to draft under the review handoff policy.

Your original fcca7a624 remains intact and owns the admission lock, cleanup-held slots, definition/history/output budgets, fanout bound and original eight regression tests. Agent-only work: dev integration (6333f4af0), concurrent admission/reuse regression (560a8b97b), and the repair/tests (31928dc08).

Claude findings 1-3 addressed in this bounded repair attempt:

• New validation failures now throw INVALID_ARGUMENT with descriptions instead of raw IllegalArgumentException. Size/capacity failures retain RESOURCE_EXHAUSTED.
• The semaphore is acquired per capability invocation attempt, so retry backoff releases capacity. A nine-node virtual-time regression checks ready work proceeds while eight nodes back off.
• A node's own timeout now participates in retries and ends with a Failed progress event; external cancellation still propagates. The SDK timeout regression checks two attempts and retry flags; the service test checks failed status and slot reuse. The timeout-bypasses-retries behavior already existed on dev; this repair improves it rather than attributing its origin to your submission.

Remaining required validation, not a demonstrated author failure:

1. Run python3 /Users/kshivang/Development/Boss/.pr-review-sessions/with-build-lock.py ./gradlew :boss-mastery-sdk:test :boss-mastery-orchestrator:test ktlintCheck detekt on the final head. This command is already queued in the owned worktree .worktrees/mastery-569-20260913; its log is .pr-review-sessions/recurring-sweeps/2026-09-13T034418Z-codex/mastery/build.log. It has not acquired the shared lock yet. Inspect that result before launching a duplicate. Compilation, test success and Gradle quality-gate success are NOT claimed.
2. A maintainer must approve the fork Build & Test workflow and verify final-head results. The preceding integrated-head run https://github.com/risa-labs-inc/BossConsole/actions/runs/34736468608 is action_required, not a code failure.
3. Confirm the final repair against the existing Claude review fix(mastery): bound workflow execution without blocking retries #569 (comment) after tests pass. No repeated bot request was sent.

Standalone ktlint 1.8.0 checks of all four changed Kotlin files and git diff --check pass. No application or live database was used. Manual follow-up: saturate execution capacity and verify a visible refusal; cancel a workflow whose cleanup is delayed and verify its slot stays occupied until cleanup ends; run a retrying timeout workflow and verify the terminal failure; exercise a large-output workflow and verify no oversized node-completed output arrives.

Claude's optional cancelling-state/logging/configuration suggestions are deferred: the current state deliberately remains running until cleanup ends; forced slot reclamation would violate the resource bound. The limits are per execution (up to 64 total invocations at defaults), not a claim of eight process-wide. Direct SDK graph-size validation and node identity validation are pre-existing scope, not regressions introduced here. No other group's PR was modified, and closed combined #545 already identifies this PR as its mastery replacement.

[kshivang]

@Raghav-Bansal-15 Final disposition after the user authorized suitable merges into dev: keep #569 draft; no merge yet. Your atomic admission, bounded output/history, and cancellation cleanup work is useful and remains suitable for eventual main once validated. Your original commit and credit are preserved; the score remains provisional 7.5/10, excluding agent repairs.

I rechecked OPEN/DRAFT, base dev, head 31928dc0803aae36cdc5235537bf8e7ef9f008c6, and refreshed main/dev. Latest Claude feedback is still the review linked above; its three pre-merge code findings have the previously documented repair and regression tests. No new production defect was demonstrated in this recheck.

Exact remaining blockers are validation/infrastructure, not evidence of an author defect:

• The earlier local command finally exited before compilation because the sandbox could not open the Gradle wrapper cache lock. One retry with cache access is queued behind the shared build lock. Read mastery/build-final-disposition.log before starting another build; the owned worktree and full command are in the prior handoff. Both mastery test suites and Gradle ktlintCheck/detekt still need actual passing results.
• I approved the normal fork workflow for the exact head: https://github.com/risa-labs-inc/BossConsole/actions/runs/34736738901 . It is now queued, not passed. No required check was bypassed. Let this run complete and investigate actual failures if any; do not treat queue time as a code failure.
• Verify the repaired validation, backoff and timeout behavior from the existing tests before marking ready. Standalone changed-file ktlint 1.8.0 and diff whitespace checks passed earlier; neither substitutes for compilation/tests/required CI.

Review dispositions for the other suggestions: a cancellation acknowledgement is a best-effort request, not proof cleanup ended; keeping the running slot prevents unbounded non-cooperative work, so forced reclamation is rejected. A separate cancelling label and refusal logging are useful observability follow-ups, not a resource-bound correctness fix. The explicitly documented default is 8 invocations per execution, up to 64 across 8 executions; global configurability is optional. Node identity/SDK graph validation predates this patch; cosmetic rewrites of the size check and constructor messages are not required for correctness.

Once validation passes, recheck current dev under the shared merge lock and merge only the reviewed head into dev using the required match-head squash flow. No merge or main change was performed in this pass. I have retained draft status to avoid a prolonged queue/review loop.

[Raghav-Bansal-15]

Review follow-up: Keep capacity until cleanup finishes, release invocation slots during backoff, and report validation and timeout failures explicitly. Split timed invocation from retry handling and wrap the regression assertion to fix the reported quality failures without suppressing checks.

Validation: 15 mastery tests plus formatting/static analysis pass locally. The current-head full fork CI passed.

The PR is ready for review and the existing author/reviewer commits are retained. Upstream CI needs maintainer approval. I am following the latest-head checks and have not merged or released anything.

[kshivang]

@Raghav-Bansal-15 Thank you for preserving cleanup-held execution capacity and completing the retry refactor/quality fixes. Admission is atomic, retry backoff releases invocation permits, node timeouts retry and emit failure events, and validation uses explicit gRPC statuses. The previous substantive review findings are addressed.

Reviewed 1539376b5251ad9c0805947d6d73bbf6b926cd79 against dev. I found no new blocking code defect. I verified the exact-head fork run, downloaded its JUnit XML and counted 21 SDK/service tests, zero failures/errors/skips (15 SDK plus 6 service). Fork platform builds and quality checks passed. Local whitespace/newline checks and clean dev merge simulation passed; reviewed module blobs remain unchanged in that integration. This is artifact verification, not a new local Gradle run.

Original resource bounds remain your contribution. Prior agent simultaneous-admission and validation/timeout/backoff repairs retain separate attribution; your quality-compliant refactor and validation follow-through are additional work. Optional follow-ups remain a visible cancelling state, refusal diagnostics, and byte budgeting for aggregate responses; the 8-per-execution invocation bound permits up to 64 across 8 executions. Forced capacity reclamation while cleanup still runs would defeat the bound and is not requested.

Upstream run: https://github.com/risa-labs-inc/BossConsole/actions/runs/34867331159

Remaining merge gate is upstream validation, not a demonstrated author defect. I approved the current upstream run once; platform builds/tests, quality and compatibility checks are still running or queued. Fork success does not replace required upstream checks. Per this sweep's bounded-review policy, I am returning this to draft without changing your branch.

Next steps: let the linked upstream run finish, inspect and fix any actual failure on this head, then restore ready status. Before merging, refresh dev and verify the integration retains API 1.0.93 and the later kernel identity/migration/capture/trackpad changes; rerun affected checks if integration changes dependencies or tested behavior. Merge only into dev after green required checks, under the shared merge lock with the exact reviewed head. No admin bypass or main merge.

No app instance or production data was touched. Manual service/UI integration checks remain unexecuted. The existing applicable Claude review was independently checked against these revisions; I did not request another review of the same findings.

Manual follow-up with a fake/disposable capability provider: saturate execution capacity, cancel with delayed cleanup, verify admission stays refused until cleanup finishes, then verify retry-timeout failure events and capacity reuse.

[kshivang]

@Raghav-Bansal-15 Bounded follow-up review: your substantive fixes from the prior handoff remain intact. I merged rebased dev 77c4a40 into this branch as 83d7cd0, resolving only the AGENTS.md conflict by preserving dev's window-registration guidance. The reviewed service/module and test blobs are unchanged from the previously validated contributor revision. No new contributor score or source repair is claimed.

The prior applicable Claude findings remain addressed as documented above; no repeated bot review request. Prior verified tests (91 repair tests / 21 mastery SDK and service tests respectively) are historical validation, not final-head CI. Whitespace checks pass and the final diff contains only this PR's intended module changes (plus RE2/J catalog dependency for #568). Required CI still needs final-head verification. Automatic approval review rejected approval of fork workflow execution, so no check bypass or merge was attempted. This is an infrastructure gate, not an author defect; the PR stays ready targeting dev.

Cross-group note: #575 adds host authorization in OrchestratorServiceImpl, distinct from #568's admission/history bounds; its owner should preserve both if integration overlaps. No consolidation or other PR modification. Manual disposable-service checks from the previous handoff remain unexecuted.

[kshivang]

@Raghav-Bansal-15 Refreshed review: head 83d7cd0dd095e928f1e804fe6126c3385b26b688 is unchanged and remains ready targeting dev. I independently inspected the now-completed exact-head fork CI: all three platform jobs, code quality, compatibility, contracts, shell and database jobs succeeded. Downloaded macOS JUnit artifacts verify 21 tests: 15 SDK and 6 service, zero failures/errors/skips. This updates the prior pending validation snapshot.

Latest dev cf21742df6fb7e3730fbe8d96d900e8b8752748d integrates cleanly in merge-tree and preserves the reviewed module/test blobs unchanged. No new author changes or Claude findings appeared; the prior substantive findings remain addressed. No new source repairs or score changes.

The remaining gate is the upstream exact-head Build & Test run, currently action_required. Approval of fork workflow execution was rejected by automatic approval review and remains blocked without explicit authorization. I did not retry it or bypass required checks. The PR remains ready, not draft: this is an infrastructure approval gate, not an author defect.

I cancelled my local validation while it was still waiting for the shared build lock after obtaining completed exact-head test artifacts; no local Gradle pass is claimed. Manual disposable-service checks remain as listed in the previous handoff. Merge into dev requires approved/passing upstream checks and a fresh locked head/base recheck.

[kshivang]

@Raghav-Bansal-15 Update: the coordinator explicitly authorized and approved the prior upstream CI runs; that approval hold is resolved.

The refreshed dev history (f8ccb86b) exposed conflicts in older CLI/import/health changes carried by this branch. I merged current dev into 54bd539b2edd35a38425eafe95b9093daae57455, preserving current dev outside this PR's assigned files and keeping all reviewed resource-limit module/test blobs unchanged. The resulting diff is only the intended repair or mastery implementation. This integration is agent work and does not change your prior contribution rating or attribution.

Fresh exact-head upstream CI is now running normally. Shell/version/contracts and database checks have passed; remaining builds and quality/compatibility checks are progressing. No action-required hold is currently present. I will merge only into dev after final-head checks pass and a fresh serialized integration check. No substantive new code finding; PR stays ready.

[kshivang]

@Raghav-Bansal-15 Merged into dev as 67c90a7 after all exact-head upstream checks passed. I also validated the actual integration with the newly merged authenticated IPC/process prerequisites: 21 SDK/service tests passed, plus both modules' ktlintCheck and detekt under the shared build lock. Locked refresh confirmed head 54bd539, base b915182, and unchanged reviewed mastery modules before the exact-head squash merge.

Your original bounds, cleanup-held admission and contributor follow-up remain credited to you; prior agent regression/status/timeout/backoff work retains separate attribution. No source rewrite was needed in this final integration. Manual disposable-provider checks remain unexecuted as previously listed. Thank you for completing the review follow-ups.