fix(concurrency): make lifecycle and durable state transitions ownership-safe (#766 + #767) - #791
Merged
Merged
Conversation
…hip-safe Salvages the unique work of draft PRs #766 and #767 into one change. - fileLock: add withFileLockSync (Atomics wait) for sync RMW callers, with the ownership-safe stale reclaim and ENOTEMPTY-tolerant unlink; lock waits use the real timer so a test that fakes setTimeout cannot freeze lock hand-off. - pairPipeline: replace the per-instance abortSignal/stuckDetector with an AsyncLocalStorage RunControl, so concurrent run() calls on one instance cannot observe or overwrite each other's cancellation state. - processRegistry: taskId+spawnedAt identity ownership on activity, close and health-check, plus a SIGKILL escalation timer that is unref'd, cancelled on child close, and cleared by a force kill. - store: tryClaimTaskAdmission claims a task in one locked read-modify-write; stale-lock reclaim tolerates ENOTEMPTY; main's updateTaskLinearState lock invariant is preserved. - runnerState: cross-process locks on the rejection, decomposition, pace, project-selection and pipeline-history RMW paths; main's withRunnerStateLock and reserveDailyCreations are kept, and the daily reset runs outside the decomposition lock because withFileLockSync is not reentrant. - decisionEngine: admission claims on the auto-execute paths and an in_progress filter; file-locked engine state and backlog appends. - oauthStore, dev, logRotation, codex, reembed, taskParser, gitInfo, locale: the locked or scoped variant of the same invariant. Dropped: telemetry.ts (main has withTelemetryLock), workflow.ts (owned by another salvage), prProcessor.ts (its state lock cannot fit the repo's 1500-line gate, which main's copy already sits on), task_state_model.py, agt3420-probe.txt, and package.json/lock version churn.
unohee
force-pushed
the
salvage/concurrency-locking
branch
from
September 28, 2026 07:02
2d67be1 to
1b05145
Compare
This was referenced Sep 28, 2026
unohee
force-pushed
the
salvage/concurrency-locking
branch
from
September 28, 2026 07:11
44e19e6 to
a953bd4
Compare
…hip-safe Salvages the worthwhile work from draft PRs #766 and #767 (both 'failed 4 times … very likely incomplete', 174-247 commits behind) into one change, rebased onto current main. Kept: unified withFileLockSync (Atomics.wait + ownership-safe reclaim + ENOTEMPTY tolerance); AsyncLocalStorage per-run RunControl in pairPipeline; processRegistry taskId/spawnedAt ownership + cancellable unref'd timer; taskState tryClaimTaskAdmission + ENOTEMPTY reclaim; decisionEngine admission claims + in_progress filter; taskParser zod schemas; cross-process locks in memoryCore/codex/reembed/runnerState; gitInfo enrichment now persists via graph.addNode+saveGraph; locale AsyncLocalStorage scope; oauthStore refresh serialized on a store lock with reload-under-lock. Also updates the duck-typed AuthProfileStore double in codexResponses.test.ts (reloadProfileFromDisk/setProfileUnlocked) so it implements the interface its declared collaborator now requires — the adapter's tests are the only caller that is not a real AuthProfileStore instance.
unohee
force-pushed
the
salvage/concurrency-locking
branch
from
September 28, 2026 07:11
a953bd4 to
ca2805a
Compare
This was referenced Sep 28, 2026
unohee
added a commit
that referenced
this pull request
Sep 29, 2026
…dict reuse (#794) * feat(agents): port harness agent patterns — advisor role + declarative per-role tool/effort scoping Cherry-picked onto origin/main (the original branch drifted 10 commits). Conflicts were additive on both sides (main's --harness-only path + this advisor wiring) and were resolved by keeping both. * fix(ledger): recovery is idempotent — a recomputed completion effect no longer kills the heartbeat (AGT-4518) Cherry-picked onto origin/main. Verified main still has the defect: recoverPublishedRun compares payload_json (~runLedger.ts:1071) and reconcileDurableArtifacts has no per-row isolation. * fix(taskSource): paginate the local queue instead of silently dropping past 200 (AGT-3421) Cherry-picked onto origin/main (main still has the fixed limit:200/offset:0). * fix(adapters): name protectedFiles/forbidPublication in the delegated-CLI dropped-options warning (AGT-4444) Cherry-picked onto origin/main; main warns for mcpTools/coordinationContext only. * feat: port the still-unique half of the audit remediation onto main Main advanced 10 commits while `feat/omp-agent-patterns` was open, and its salvage PRs (#785-#791) fixed several of the same issues independently — its own way. Rather than rebase 10 commits through wide conflicts, this branch was recreated from `origin/main` and only the parts main does NOT already have were ported. Every ported item was checked against main first, with the evidence recorded, and where main's version was newer or better (memory pagination, the tester prompt bound, TUI width clipping, IPv6 callbacks, pipeline embed budgets) main's version was kept and nothing was ported. Still missing on main, now ported: - `shellCommandGuard.ts` — the guard is lexical on main (`BLOCKED_COMMANDS`), so 6 of 21 destructive forms execute (`r{m,} -rf`, `$'\x72\x6d' -rf`, `r"m" -rf`, `git clean -fdx`, …) while 4 of 51 harmless mentions are falsely blocked. Replaced with a bash-faithful resolver: 72/72 verdicts correct. - `read_file` — main reads the whole file before slicing; a 512 MiB file with `limit=1` throws `RangeError: Invalid string length`. Now a bounded window. - `recoverPublishedRun` idempotency + per-row reconcile isolation (AGT-4518): main still compares `payload_json`, so a recovery of an already-enqueued completion throws and kills the heartbeat. - `taskSource` pagination (AGT-3421): main still caps the local queue at 200. - Knowledge scanning incompleteness (AGT-3490): a walk stopped by depth, timeout or size still persists a graph indistinguishable from a complete one. - `gitInfo` discovery: main misses staged-but-uncommitted files entirely and newline-splits the log query, so a path containing a newline is corrupted. Also main strips a leading `\n` from EVERY filename token, though real `git log -z` prefixes only the first — verified against real git bytes. - String-aware JSON extraction for auditor/documenter (main is brace-blind), and the documenter prompt now delimits untrusted task/worker text. - Verification-evidence log bound: main joins every failing log before capping, so one suite's output displaces another's and the total is measured post-join. - Memory: LanceDB returns the vector column as an Arrow `Vector`, which main's `normalizeRecords` zeroes — embeddings are destroyed on rewrite and cosine similarity over stored rows is NaN, so dedup matched NOTHING. Confirmed against a live store. Plus the survivor refusal bound and non-quadratic consolidation (main's is still all-pairs under the global write lock). - Automation: `fixOne` bypasses the PR lease; decomposition capacity reads a PROCESS-LOCAL counter, so two real processes are both granted a slot against a cap of 1 (reproduced with real spawned processes). - Daily reporter: main republishes every already-succeeded project on retry. - Python task-state strictness (main's own test file is currently red: 5 failed), CI-wait duration validation, verify-manifest command/runtime caps, and the notifier delegating to the shared predicate — with TEST-NET-2/3 added so delegating does not regress what main's local table rejected. - CLI output bound, tester/dashboard hardening, Retry-After HTTP-date parsing where main still parses with `parseInt`, and the delegated-CLI warning now naming `protectedFiles`/`forbidPublication` in addition to the tool list. - `advisor` role + declarative per-role `tools`/`effort` (the harness patterns), which main does not have at all. Verification: `tsc` clean, `oxlint` clean, full suite 7167 passing. Each ported item carries a test that fails against `origin/main` and passes here; several were additionally mutation-checked (removing the bound/coercion/bucket key makes the named test fail). * fix(review --max): make the audit verdict independent of --concurrency (AGT-4597) `--concurrency` was a partition input on the audit path, not just a resource knob: `balanceAreasToConcurrency` re-split the source with a smaller per-area cap until `areas.length >= concurrency`, and `reviewMaxCommand` called it. So the same files and the same reviewer were judged in 2 units at concurrency 1 and 10 units at concurrency 8 — and since `aggregateAuditResults` is worst-wins (`decision: failed ? 'reject' : worst`), a finer split could only turn an approve into a revise/reject. Reproduced deterministically (20 files in one dir, cap 12, a reviewer that judges the unit it is handed): concurrency 1 -> 2 areas -> approve; concurrency 8 -> 10 areas -> reject. No LLM call, no timing. The feature that introduced it, INT-2249 / PR #183, records its own verification as "2 dirs, 10 files: concurrency 2 -> 2 areas, concurrency 8 -> 10 areas". The audit now partitions from `--max-files-per-area` alone via `planAuditAreas`; `--concurrency` only decides how many reviewers run at once. The capability INT-2249 wanted is kept, not dropped: the finer, more parallel fan-out is reached deterministically with `--max-files-per-area <n>` (cap 2 yields the same 10 areas the concurrency-8 path fabricated), now reproducible at any concurrency. The `--fix` path keeps `balanceAreasToConcurrency` unchanged — there is no verdict there and more areas just mean more parallel fix workers. Verified on the real CLI (`--dry-run` over this repo): 478 files -> 61 areas at concurrency 1, 8 and 32 alike; cap 4 -> 134 areas at 4 and 32. The regression test drives the real command entry and fails when the partition is re-derived from concurrency. * release: 0.24.4 — durable review-verdict reuse (Tier 1) Promotes the Unreleased section to 0.24.4 and adds the durable review-verdict store: a review of content this deployment already reviewed is replayed from `(kind, base, content digest)` instead of paying for the reviewer again, and the store survives a daemon restart. Measured over the recorded history (530 records, 512 with a verdict): 68 byte-identical pairs, 15 same-mode, 10 repeating the same verdict — ~10 of 512 reviews, at ~$0.26 and 93s p50 per reviewer call. Proven across two separate processes: 27.0s / 6 model calls became a 1.58s replay with zero model calls, same verdict. The advisor pass still runs over a reused verdict, so a replay can tighten but never fail open. Also corrects the README's `review --max` claim that areas "auto-split to fill --concurrency" — the partition has been derived from --max-files-per-area alone since AGT-4597, so --concurrency no longer changes the verdict.
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
1 participant
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
Salvaged from two drafts left by a failed autonomous run (both said "failed 4 times … very likely incomplete"): #766 (lifecycle/durable state transitions ownership-safe) and #767 (atomic + validated task and graph state updates). Merged into one coherent change and rebased onto current
main(they were 174-247 commits behind, and #766/#767 collided with each other onfileLock.ts/taskState/store.ts/automation/runnerState.ts).Kept
src/support/fileLock.ts: onewithFileLockSync(Atomics.wait for the wait) with ownership-safe stale-lock reclaim andENOTEMPTYtolerance folded in from both drafts.src/agents/pairPipeline.ts+pairPipelineTypes.ts: anAsyncLocalStorageper-runRunControlreplaces the shared mutableabortSignal/stuckDetector(cross-run cancellation isolation), with a regression test.src/adapters/processRegistry.ts:taskId/spawnedAtownership checks + a cancellable,unref'd escalation timer.src/taskState/store.ts: atomictryClaimTaskAdmissionclaim +ENOTEMPTY-tolerant reclaim;src/orchestration/decisionEngine.tsadmission claims +in_progressfilter;src/orchestration/taskParser.tszod schemas.memoryCore(withCrossProcessMemoryMutationLock),codex,reembed,runnerStatelocked RMW sites;automation/runnerState.tsrejection counters;support/logRotation.tsrestore-from-.1.src/knowledge/gitInfo.ts: enrichment now actually persists (graph.addNode+saveGraph);src/locale/index.tsAsyncLocalStoragelocale scope;auth/oauthStore.ts.Dropped (with reason)
telemetry.ts(main already haswithTelemetryLock),orchestration/workflow.ts(landed separately via fix(state-integrity): salvage durable state fences + review quality harness (#757 #771 #773 #777) #786),memory/compaction.ts(landed via fix(memory): paginate memory lifecycle sweeps and stop dropping rows past the first page (salvage #772 + #776) #785), the#759gitInfo parser hunk (landed via fix(streaming): bound SSE payloads, sockets, streams and subprocesses (salvage #759, #762, #765) #789),processRegistryduplicate variant,task_state_model.py+ probe files + package version churn.Conflict resolution — main's hardening wins wherever a draft conflicted with it (
withRunnerStateLock,reserveDailyCreations,upsertTaskStateUnlocked, the documentedupdateTaskLinearStatelock invariant), while keeping each draft's unique value (tryClaimTaskAdmission,ENOTEMPTYreclaim).pairPipelinere-derivesPipelineContext.abortSignal/stuckDetectoron top of theAsyncLocalStoragecontrol rather than picking a side.Bugs found and fixed during verification
withFileLockSyncis not reentrant →resetDailyCounterIfNeedednow runs outside the decomposition lock.setTimeout→ the real timer is captured at module load.OPENSWARM_PR_PROCESSOR_STATE_FILEoverride (that file change itself was dropped: the repo LOC gate capsprProcessor.tsat 1500 lines and main is already exactly there).Verification
npx tsc --noEmit→ cleanfileLock.test.ts14/14 and combined runs 114/114 (one earlier fileLock failure did not reproduce: shared-timer load from parallel workers)mainata8c8b9f(includes fix(memory): paginate memory lifecycle sweeps and stop dropping rows past the first page (salvage #772 + #776) #785/fix(state-integrity): salvage durable state fences + review quality harness (#757 #771 #773 #777) #786/fix(output): unify sanitize/budget layer across Discord, TUI, CLI, and agents (salvage #758+#775+#782) #787/fix(boundaries): harden DNS pinning, GraphQL cost limits, and CLI/provider validation (salvage #764, #774, #778) #788/fix(streaming): bound SSE payloads, sockets, streams and subprocesses (salvage #759, #762, #765) #789) and re-verified