Skip to content

fix(runtime): published dynamic expansions are the lock-free authority for fan-out membership - #1163

Merged
aviggiano merged 6 commits into
mainfrom
claude/w07-freeze-dynamic-membership
Sep 29, 2026
Merged

aviggiano merged 6 commits into
mainfrom
claude/w07-freeze-dynamic-membership

Conversation

@aviggiano

@aviggiano aviggiano commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Two bookkeeping failures in dynamic fan-out can stop a campaign permanently (#1142).

  1. A re-run source breaks every render. When a reset re-runs a dynamic source, the source's first agent generation empties its canonical artifact directory (resetTaskArtifactsForRetry in workflow.tsx). From then on every workflow render throws dynamic source artifact does not exist. Once the source writes a plan with different bytes, every render throws DYNAMIC_EXPANSION_CHANGED instead, and that never clears. Two resets can do this: --reset-node on the source, or a --retry-failed reset whose dependent set includes the source (Recovery does not reopen descendants after prerequisites succeed #1141 reports that Smithers builds that set by start time; I did not reproduce that part). The same check runs inside strict lifecycle admission (readLinkedWorkflowEvidence), so cancel, pause, why, timeline, snapshots, node, fork and replay are refused with WORKFLOW_CONTROL_EVIDENCE_INVALID.
  2. A stale lock blocks forever. Every expansion read and create ran under dynamic-expansions/.expansion.lock, which was never reclaimed by design ("never steal a lock based on age"). Observers took it too. So a controller that was SIGKILLed or OOM-killed, or an observer interrupted with Ctrl-C, while holding the lock left every later render and admission check busy-waiting 5 s (Atomics.wait on the event loop) and then throwing DYNAMIC_EXPANSION_LOCKED. A leftover lock also made resume --retry-failed of the source refuse with DYNAMIC_RETRY_EXPANSION_INVALID ("unrecognized retry state").

Root cause

loadOrCreateDynamicExpansion treated the published manifest as a cache to re-prove on every call instead of as the record of membership. It re-read and re-hashed the live source artifact and required the bytes to match source.output_sha256, and it did all of this under a lock that nothing could release after a crash.

Change

  • loadOrCreateDynamicExpansion (dynamic-expansion.ts): if the group already has a manifest, it validates the manifest set and the group's static identity (run, group, source node/attempt, source path, JSON and key paths, node-ID template, prompt template digest and fingerprint, limit) and returns the manifest. It does not read the source. The source is read, hashed and planned only when the manifest is created. source.output_sha256 is still recorded as provenance.
  • withDynamicExpansionLock and its two error helpers are deleted. publishFileDurableExclusive never replaces a published file (a byte-identical duplicate is accepted), and the existing re-read after publication rejects an inconsistent set. That re-read detects a cross-group race but cannot undo it; see Risk. The comment at the call site now says so.
  • planDynamicExpansionRetryArchive (dynamic-expansion-retry.ts) no longer counts dot entries as unrecognized state. readExpansionManifests already skips them, and the directory rename archives them with everything else. That covers a lock file from an older build and the temporary file of an interrupted publication. Other unknown entries are still refused. Its docstring no longer justifies the archive by the render needing the source; the archive's remaining job is to let the group expand again from the retried source's new output.
  • docs/reference/topology-yaml.md no longer says resume rejects changes to the source bytes, and it describes the new behaviour.

Most of the dynamic-expansion.ts diff is re-indentation from unwrapping the lock callback. git diff -w shows the real change: +15/-65 in that file.

Semantic change to review

After a reset re-runs a source, the fan-out keeps its originally published items. Before this change the run failed at every render instead. An operator who resets a source to get a new plan therefore does not get a new fan-out. The same should hold for fork and replay, which run against the same run root and its published manifests (from reading submitLifecycleAction; not run). The only path that re-expands is resume --retry-failed on a source whose verifier failed: the #1064 archive moves the manifests to dynamic-expansion-history/ before the source runs again. That path is unchanged.

Deliberately not built (and why)

  • No stale-lock reclamation (PID liveness or age-based stealing). The removed code's own comment explains why age-based stealing races. And the lock guarded nothing that needs one: reads are of files that are never rewritten, and creation has exclusive publication plus re-validation.
  • No other cross-process serialization of manifest creation (a sequence reservation file, or a loser that withdraws its own manifest) to close the cross-group race below. Each adds the kind of machinery this PR removes, for a race that needs two concurrent renderers of one run.
  • No generation store, "current generation" pointer or repair command (the Fence recovery handoffs and preserve artifact generations #1154 direction). The published dynamic-expansions/<group>.json already is the durable membership record. The bug was re-validating it against mutable state.
  • No check or warning comparing a regenerated source with the published membership. It would bring back the dependency on mutable state that this PR removes.
  • Not in scope: Smithers resetting unrelated finished nodes by start time (Recovery does not reopen descendants after prerequisites succeed #1141), second-writer races (Make pause, hijack, and continuation handoff atomic #1153), and whether the Archive dynamic expansion generations before producer retry #1064 retry archive (dynamic-expansion-retry.ts) is still needed now that membership is frozen. Deleting the archive would also change --retry-failed semantics, so that is the owner's call for a separate PR.

Verification

Discriminating evidence. I compiled this PR's tests against main's dynamic-expansion.ts and dynamic-expansion-retry.ts (fbbcf6c) into a separate out dir and ran them, then ran them at the PR head.

Test main this PR
dynamic-expansion.test.ts › a re-run dynamic source keeps the published fan-out for renders and admission (renders with the source removed, then rewritten with a different plan, then ready again, plus verifyDynamicRuntimeMaterialization) fails: ArtifactPathError missing-file: dynamic source artifact does not exist passes
dynamic-expansion.test.ts › a lock file left by a killed materializer blocks neither expansion nor a source retry fails after 5 s: DYNAMIC_EXPANSION_LOCKED passes
dynamic-lifecycle.test.ts › a re-running dynamic source keeps published controls admissible and observable (strict evidence plus events query/watch with the planner artifact removed, then strict evidence with a plan whose goal has a different key) fails: WORKFLOW_CONTROL_EVIDENCE_INVALID … published dynamic runtime controls no longer re-derive from their sealed base: dynamic source artifact does not exist passes

On main the first assertion of each source test already fails, so their rewritten-source halves are never reached there. To check that those halves test something, I ran both tests against a partial fix: in the compiled dist-test, the reuse branch skipped a missing source but still compared the digest of a present one. Both fail on it: the unit test with DYNAMIC_EXPANSION_CHANGED, and the lifecycle test with WORKFLOW_CONTROL_EVIDENCE_INVALID.

The lock-file test also checks commit 2 separately: run against commit 1's retry source it fails with DYNAMIC_RETRY_EXPANSION_INVALID, and it passes with commit 2.

Coverage for the check this PR now relies on: once the source is no longer re-hashed, the assertCompatibleManifest call in the reuse branch is all that stops a changed group definition from silently reusing the published items. "persisted expansion rejects prompt-template, topology-contract, and dynamic-limit changes" now changes the source JSON path, key path, node-ID template and template fingerprint of a published group and expects DYNAMIC_EXPANSION_CHANGED for each. The behaviour is unchanged, so it passes on main and on this PR. It fails (Missing expected exception) when that call is made a no-op.

Tests removed or rewritten because they encoded the old behaviour:

  • the source-change block of "persisted expansion rejects tampering, …" (a changed source had to throw DYNAMIC_EXPANSION_CHANGED);
  • "dynamic expansion retries when a contended lock disappears before inspection" and "dynamic expansion never steals an old lock owned by another materializer" (they tested the deleted lock by monkeypatching fs.openSync and Date.now);
  • the lifecycle test that required admission to fail while the source was absent;
  • the retry archive's "unrecognized state" case now uses a non-dot entry (notes.txt), which is still refused.

The source re-run test and "explicit source retry re-derives the base runtime controls after archiving an expansion" now share one sealedDynamicRun fixture instead of two hand-built copies.

What I ran at the head:

  • node --test dist-test/test/dynamic-expansion.test.js: 15/15 pass
  • node --test dist-test/test/dynamic-lifecycle.test.js, whole file (run together with the file above): 18/19. The failure was in fixture setup, before any dynamic expansion: WORKFLOW_SUBMISSION_FAILED … dependencies/packages/000003/v4/locales/ka.d.cts changed while reading, a node_modules file read while the fixture snapshots its execution dependencies. That test ("explicit source retry prunes the withdrawn generation from run state and keeps observers admitted") passed when rerun alone. A reviewer hit the same setup failure on a loaded host.
  • At d3ba05c, before the follow-up commits: dynamic-workflow.test.js 2/2; cloud-worker-handoff.test.js, whole file, 14/14 (these fixtures render the real generated workflow.tsx against the rebuilt runtime dist, which calls materializeDynamicRuntime); and --test-name-pattern='resume retries a failed artifact verifier from its agent producer and dependent closure|resume retries stalled nodes alongside failed ones' dist-test/test/runtime.test.js 2/2 (the Archive dynamic expansion generations before producer retry #1064 archive through resumeRun). The follow-up commits change only comments in src, plus the two test files above, so I did not re-run these.
  • All pass: npx prettier --check and npx eslint on the changed files; CI=1 ESLINT_PLUGIN_DIFF_COMMIT=<merge base> pnpm -w lint:strict:ci; pnpm --filter @ultrafuzz/runtime typecheck; pnpm -w knip; node scripts/docs-check.mjs. The strict lint is diffed against the merge base fbbcf6c. Diffed against the current origin/main it also lints packages/cli/src/commands/report/bundle.ts, which main changed after this branch point (fix(cli): record skipped files in the report bundle manifest #1161), and reports max-lines there. This PR does not touch that file, and CI diffs the merge ref against the PR base, so it lints only this PR's files.

Not run: the full runtime and CLI suites, a real Smithers campaign that resets a dynamic source, the two-process race, and an upgrade of a run already stranded by a lock. The lifecycle tests use the repository's fake Smithers inspect and events fixtures.

CI: External static analysis fails here, as it does on every PR that edits CHANGELOG.md. Super-Linter lints the whole file with MD013 at 400 characters and reports every long entry, about 45 existing ones plus this one, so shortening this entry would not help. #1184 turns MD013 off. Because release-validation needs that job, the runtime lanes have not run in CI for this PR yet; the local runs above are the evidence until they do.

Risk / compatibility

  • Residual race without the lock. Two processes that render the same run at the same moment, each creating the first manifest of a different group, can both publish the same sequence. The post-publication re-read then rejects the set, but both files are already durable, so every later render and strict admission fails with DYNAMIC_MANIFEST_SET_INVALID until someone repairs the set by hand.
    • A reviewer reproduced this with two processes that each ran one render's per-group loop against the built dist. With different ready sets every trial stranded the set (on main the lock serialized them and none did). With identical ready sets none did, because identical bytes dedupe. I did not run that experiment myself.
    • It needs two concurrent renderers of one run that see different Smithers outputs. ultrafuzz resume serializes on the run's lifecycle lock and inspects the run first (Ordinary resume preflights an active run instead of attaching #968), and Smithers 0.35 refuses to resume a run whose driver is live, before its detached preflight render (findLiveDriverError runs before preflightDetachedLaunch in @smthrs/cli/src/index.js; --force does not bypass it, and ultrafuzz never passes --steal-ownership). These are launch-time checks, not exclusion held by the running driver, so they narrow the race rather than rule it out.
    • I found no live-driver check on the fork/replay path: neither ultrafuzz's submitLifecycleAction nor the Smithers fork/replay command handlers check the parent run's driver (I did not read @smthrs/time-travel). Forking or replaying a run whose driver is still live would add a second renderer, which would share the whole run root with the live driver, not only the manifests.
    • Two renderers creating the same group are harmless: both plan identical bytes, and the exclusive publisher accepts the duplicate.
    • I judged this narrow race a better trade than a lock that fails every render forever after a routine SIGKILL, OOM or Ctrl-C. It is still a trade, so I'm flagging it.
  • Filesystems without hard links. There publishFileDurableExclusive creates the final manifest name and then writes into it, so without the lock a concurrent observer can read a manifest mid-publication and fail that one admission with DYNAMIC_MANIFEST_INVALID. From reading safe-paths.ts; not run.
  • Manifest format is unchanged (same schema and fields). For a run already stranded by a stale .expansion.lock: strict admission (cancel, pause, why, …) and the source-retry planner run in the installed CLI, so they stop tripping on it once this build is installed. resume points the controller at the installed runtime (ULTRAFUZZ_RUNTIME_MODULE in submitSmithersContinuation), so its renders should stop too. fork and replay controllers load the runtime sealed into the run's execution snapshot, which for a run launched on an older build still takes the lock. I read this in the code; I did not run an upgrade. A process on an older build still takes the lock, and this build ignores it.
  • DYNAMIC_EXPANSION_LOCKED and DYNAMIC_EXPANSION_LOCK_INVALID are no longer raised. No docs or code outside the deleted tests referenced them. For a published group, DYNAMIC_EXPANSION_CHANGED still covers a changed source node, attempt or artifact path, JSON path, key path, node-ID template or template fingerprint. A changed limit fails as DYNAMIC_MANIFEST_SET_INVALID and a changed template file as DYNAMIC_TEMPLATE_CHANGED, as before.
  • No schema, validator, or contract-description files are touched, so the validator build identity is unchanged.

Refs #1142. This covers its fan-out membership part: a re-run source or a stale lock no longer changes or blocks the published membership. It does not make a consumer's admitted authority immutable mid-attempt, and a failed replacement can still destroy the prior verified generation (the source's agent attempt still empties its artifacts before the new attempt succeeds), so it should not close the issue.

🤖 Generated with Claude Code

RetriggerConfidence Score: 4/5

The PR does not appear safe to merge while concurrent renderers can durably publish conflicting expansion sequences.

Fix All in Claude CodeFindings

  1. P1 Concurrent groups duplicate sequences ▶
Fix with agent prompt
### Issue 1
packages/runtime/src/dynamic-expansion.ts:undefined-312
If two renderers of the same run create different groups at the same time, both can read the same manifest count and publish different files with that count as their `sequence`. The check after publication rejects the duplicate, but both files are already durable. Later renders and strict lifecycle admission then keep rejecting the manifest set until an operator repairs it.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR makes published dynamic-expansion manifests authoritative for fan-out membership, removes the unreclaimed expansion lock, permits legacy dot entries during source-retry archiving, and updates documentation and tests.

Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Materialize dynamic group] --> B{Published manifest exists?}
  B -->|Yes| C[Validate manifest set and group identity]
  C --> D[Reuse published membership]
  B -->|No| E[Read source artifact and plan expansion]
  E --> F[Validate candidate set]
  F --> G[Publish manifest exclusively]
  G --> H[Re-read and validate published set]
Loading

Reviews (3) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..."

aviggiano and others added 2 commits September 28, 2026 22:41
…y for fan-out membership

A published dynamic-expansions/<group>.json is the durable record of which
generated nodes a group has, but loadOrCreateDynamicExpansion re-proved it on
every workflow render and every strict lifecycle admission: it re-read and
re-hashed the live source artifact and required it to match
source.output_sha256, all under an O_EXCL lock that was never reclaimed.

Both are self-inflicted stops:

- A reset that re-runs the source (a --reset-node, or a Smithers timetravel
  that sweeps the source into its dependent set) empties the source's
  canonical artifact directory when its agent attempt starts. Every render
  then threw "dynamic source artifact does not exist" and, once the source
  wrote a different plan, DYNAMIC_EXPANSION_CHANGED, with no way back. The
  same check made strict admission refuse cancel, pause, why, fork and replay.
- A controller killed (SIGKILL, OOM) or an observer interrupted while holding
  .expansion.lock left every later render and admission busy-waiting 5 s and
  then throwing DYNAMIC_EXPANSION_LOCKED.

An existing manifest is now validated (the manifest set plus the group's
static identity: run, group, source node, attempt and path, JSON and key
paths, node-ID template, prompt template digest and fingerprint, limit) and
returned without touching the source. The source is read only to create the
manifest, and its digest is still recorded as provenance. The lock is
deleted: reads need none, creation happens in the workflow render, which
Smithers runs for one live driver per run, publishFileDurableExclusive never
replaces a published file, and the existing re-read after publication refuses
an inconsistent set.

Semantic change: after a reset re-runs a source, the fan-out keeps its
originally published items instead of failing. resume --retry-failed of a
failed source verifier still archives the manifests (#1064) and re-expands.

The tests that encoded the old behaviour (a changed source must throw, the
lock must never be reclaimed, admission must fail while the source is absent)
are replaced by tests that a re-run source keeps the published fan-out for
renders and admission, and that a leftover lock does not block expansion.

Refs #1142

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ource retry

planDynamicExpansionRetryArchive refuses a manifest directory that holds
anything but the published manifests, so the .expansion.lock an older build
left behind after a crash (the failure the previous commit removes) made
`resume --retry-failed` of the dynamic source fail with
DYNAMIC_RETRY_EXPANSION_INVALID until an operator deleted it by hand. The
temporary file of an interrupted publishFileDurableExclusive has the same
effect.

Dot entries are never manifests (readExpansionManifests already skips them)
and the directory rename archives them along with everything else, so the
refusal now ignores them. Any other unrecognized entry is still refused; the
existing test for that case now uses a non-dot file.

Refs #1142

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@aviggiano
aviggiano requested a review from a team as a code owner September 28, 2026 22:44
templateDigest: input.templateDigest,
templateFingerprint: input.templateFingerprint,
maxDynamicNodes: input.maxDynamicNodes,
sequence: priorManifests.length,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Concurrent groups duplicate sequences

If two renderers of the same run create different groups at the same time, both can read the same manifest count and publish different files with that count as their sequence. The check after publication rejects the duplicate, but both files are already durable. Later renders and strict lifecycle admission then keep rejecting the manifest set until an operator repairs it.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/runtime/src/dynamic-expansion.ts
Line: 309

Comment:
**Concurrent groups duplicate sequences**

If two renderers of the same run create different groups at the same time, both can read the same manifest count and publish different files with that count as their `sequence`. The check after publication rejects the duplicate, but both files are already durable. Later renders and strict lifecycle admission then keep rejecting the manifest set until an operator repairs it.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code

aviggiano and others added 4 commits September 29, 2026 02:17
…t a race leaves behind

The comment that replaced withDynamicExpansionLock said the re-read after
publication "refuses" a set a racing publisher made inconsistent. It does, but
only after both manifests are durable: two renderers that create different
groups at once can take the same sequence, and every later render and strict
admission then fails with DYNAMIC_MANIFEST_SET_INVALID until the set is
repaired by hand. The comment now says so, and says that creation assumes one
renderer per run (Smithers refuses to resume a run whose driver is live before
it renders), that renderers which load the same outputs publish identical
bytes, and that on a filesystem without hard links a concurrent reader can
catch a manifest mid-publication.

The retry-archive docstring still justified the archive by the render needing
the source artifact, which this branch made untrue. Its remaining job is to let
the group expand again from the retried source's new output.

Refs #1142

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ies on

With the source no longer re-hashed, assertCompatibleManifest is the only check
between a changed group definition and silently reusing the published items,
and the source-change block this branch deleted was the only test that reached
DYNAMIC_EXPANSION_CHANGED. The prompt-template and limit test now also changes
the source JSON path, key path, node-ID template and template fingerprint of a
published group and expects DYNAMIC_EXPANSION_CHANGED for each. It passes on
main and on this branch, and fails when that call is made a no-op.

The lifecycle test's rewritten source now carries a goal with a different key
instead of the published plan plus a newline, so it exercises a different plan
rather than only different bytes.

The source re-run test and the base-controls retry test built the same sealed
run by hand; they now share sealedDynamicRun.

Refs #1142

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Every pull request in this batch inserts its entry at the same place in
CHANGELOG.md, so each merge would conflict with the next. The entries are
collected into one changelog update instead.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@aviggiano
aviggiano merged commit 7fda53e into main Sep 29, 2026
13 checks passed
@aviggiano
aviggiano deleted the claude/w07-freeze-dynamic-membership branch September 29, 2026 06:31
aviggiano added a commit that referenced this pull request Sep 29, 2026
…cord it in the changelog (#1191)

Test and lint fixes for five interactions between the 25 merged PRs #1163-#1188, found by the combined CI run in #1190, plus the batch's consolidated changelog entries.
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