Skip to content

fix(runtime): stop Smithers persisting per-pulse heartbeat events and fetching/rebasing task worktrees - #1166

Merged
aviggiano merged 3 commits into
mainfrom
claude/w09-smithers-unused-behaviors
Sep 29, 2026
Merged

aviggiano merged 3 commits into
mainfrom
claude/w09-smithers-unused-behaviors

Conversation

@aviggiano

@aviggiano aviggiano commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Two behaviours of the pinned Smithers engine (@smthrs/engine 0.35.0) run in every Ultrafuzz campaign, although Ultrafuzz never uses them.

  • A TaskHeartbeat event after every heartbeat write (Reduce durable-state duplication and heartbeat volume #1147). Each successful fenced attempt-row heartbeat write is followed by a TaskHeartbeat event: one _smithers_events row plus one stream.ndjson line. The event carries no heartbeat data, because Ultrafuzz never passes any.
    • A quiet agent task writes one per throttled liveness pulse. The watchdog pulses every 250 ms and writes are throttled to 500 ms, so that is up to two a second.
    • An agent that streams output writes one per ownership check. Every stdout, stderr and tool callback forces a check (flushHeartbeat(true)), and forced writes bypass the throttle. In a scratch run of this PR's harness with a stdout write every 10 ms, one 3.5 s task appended 384-588 TaskHeartbeat rows (9 runs, load average about 9). The same task without output appended 8.
    • In the baseline tiny-vault campaign database examined while scoping this work, 3925 of 4740 event rows (83%) are TaskHeartbeat. I re-counted that.
    • Smithers files them in its node event category, so they fill ultrafuzz events --type node.
  • Fetching and rebasing task worktrees (Create worktrees directly from pinned local commits #1148). Smithers treats <Worktree baseBranch> as a branch to track.
    • Creating a worktree always runs git fetch origin first.
    • Each time a task re-enters an existing worktree, the engine retries git rebase origin/<base>. That happens for the agent and the verifier after preparation, and again on each retry or resume. Before the rebase it runs git fetch origin again, unless a fetch succeeded for that repository in the last 60 s (the sync cache's default TTL). A failed fetch is not cached, so it is retried on every re-entry.
    • Ultrafuzz passes the recorded launch commit (or, for a pinned source, the pinned branch), and origin/<sha> is never a ref. So every re-entry logs worktree sync rebase failed … invalid upstream 'origin/<sha>'. Only a successful rebase is recorded, so the failure repeats.
    • The fetches have no timeout and update the user's remote-tracking refs. On this host, git fetch origin against an unreachable HTTPS remote took 136 s to fail.

Root cause

  • flushHeartbeat() in @smthrs/engine/src/engine.js makes the fenced adapter.heartbeatAttempt(...) write. When the write persists, it then awaits eventBus.emitEventQueued({ type: "TaskHeartbeat", … }). Nothing that depends on liveness reads that event:

    • the heartbeat-timeout watchdog advances its evidence only when the fenced write succeeds;
    • smithers why reads attempt.heartbeatAtMs;
    • Ultrafuzz's sync handles only TaskHeartbeatTimeout.

    Inside Smithers the event is only formatted for display, filtered out as noise (tail, bug), counted in in-memory metrics, or relayed by the gateway. Ultrafuzz acts on none of these. The only other emitter, the compute-task bridge, fires only on explicit heartbeat() calls, and Ultrafuzz makes none.

  • ensureWorktree() asks getWorktreeSyncCache() whether to fetch and rebase an existing worktree. Its create path runs git fetch origin unconditionally before git worktree add, and git worktree add already tries the local <base> first. A rebase that moved HEAD off the launch commit would fail the task in assertWorkspaceSourceRevision. So in Ultrafuzz this sync can only fail, or break the run's source invariant.

Change

Three deletions in the operator controller's engine, through the existing SMITHERS_COMPATIBILITY_PATCHES mechanism. Each has a registry entry, a SmithersCompatibilityPatchId member, and an entry in the fresh-install list in applySmithersCompatibilityPatches.

id effect
engine_task_heartbeat_event Removes the TaskHeartbeat emit. The fenced attempt-row write and the rest of flushHeartbeat are unchanged.
engine_worktree_sync getWorktreeSyncCache() returns an inert cache, so re-entering a worktree never fetches or rebases (git and jj paths).
engine_worktree_create_fetch Removes the git fetch origin before git worktree add.

A script over the registry checked the three anchors:

  • each occurs exactly once in the pinned 0.35.0 engine.js;
  • none overlaps another registered patch;
  • the patched engine is identical whether they are applied in registry order or first.

upstreamAbsent is [] for all three, as for the other entries that have no known upstream marker.

The new packages/runtime/test/smithers-controller-engine.integration.test.ts runs a one-off workflow under Bun on the pinned Smithers release. A Bun preload plugin applies every registered compatibility patch as each patched module loads, using the same exactly-once replacement as the controller. The test therefore runs the engine Ultrafuzz ships, without copying or modifying the package store. The runtime-supporting lane picks the file up.

The change is two commits, one per issue: +91 lines in smithers.ts, a 187-line test file, and 2 CHANGELOG lines. Nothing is removed.

Revised after review. The code is unchanged. The comments, CHANGELOG, commit messages and this description now describe the streaming heartbeat rate, the 60 s fetch cache, and which runs pick the change up. The #1148 reference is now Refs rather than a closing keyword, in the commit message too, so the squash commit cannot close the issue. The branch is rebased onto current main.

Deliberately not built (and why)

  • Bound worker resources and coalesce durable snapshots #1156's heartbeat coalescing (one event per 30 s, with per-attempt state). The events carry no data, and NodeStarted/NodeFinished already bound the interval. Deleting the emit is smaller and removes more.
  • The rest of Reduce durable-state duplication and heartbeat volume #1147: content-addressed closures, per-continuation dedup, event-log rotation or compaction, attempt retention, and a storage-breakdown command. These are separate decisions. Compacting _smithers_events would break Smithers' seq pagination and Ultrafuzz's full-history event reads. That is why this PR says Refs #1147, not Closes.
  • Create worktrees directly from pinned local commits #1148's git cat-file -e probe, fetch-only-when-absent, and "one actionable fetch error". In Ultrafuzz the launch object is always local: refs/ultrafuzz/runs/<id>/source or the pinned branch keeps it, and the Modal worker fetches it explicitly. Smithers already tries the local <base> first. A conditional fetch would bring back an untimed network call. If the object were ever missing, creation would fall back to HEAD and assertWorkspaceSourceRevision would fail the task with a source-revision error, which is today's behaviour.
  • Create worktrees directly from pinned local commits #1148's warning dedup. With no rebase, there is no warning left to deduplicate.
  • Worktree retention ("keeping worktree with unsaved work"). This has a different cause. Ultrafuzz's own artifacts/ and .ultrafuzz/schemas/ mirrors make task worktrees dirty, so the Smithers reaper correctly keeps them. I left it unchanged. The sentence "Successful runs remove their generated workspaces by default" in docs/reference/configuration.md needs a separate look. Because Create worktrees directly from pinned local commits #1148's retention and missing-object criteria stay open, this PR says Refs #1148, not Closes.
  • Replacing the hard-coded engine patch list in applySmithersCompatibilityPatches with a registry filter, as the @smthrs/agents path already does (about -85 lines). Worth doing as the next PR, but not here: it changes how every existing engine.js patch is applied at controller install, so it needs its own byte-identical-output evidence. An earlier version of this description gave a different reason, that it would collide with w06's edits. A reviewer checked that w06 does not touch that list, so that reason was wrong.
  • Upstream Smithers issues. I did not file these from here: stop persisting TaskHeartbeat (or make it opt-in), and treat a commit-id baseBranch as immutable.

Verification

Discriminating (re-run after the rebase onto b6dd1da9). I compiled the same test file against origin/main's smithers.ts and against this branch. Both new tests fail on main and pass on the branch.

  • controller agent tasks stay live on their attempt row without a TaskHeartbeat event per pulse: a quiet agent owns a 3.5 s sleep child under a 3 s heartbeat timeout.
    • main: fails with TaskHeartbeat: 8 of 22 event rows.
    • Branch: 0 such rows. The task finishes, so it outlived its heartbeat timeout on the attempt-row write alone, and heartbeat_at_ms is at least 2 s after started_at_ms.
  • controller task worktrees stay on a local-only launch commit without fetching or rebasing: a <Worktree> is seeded from an unpushed commit. A preparation task creates it, a verifier task re-enters it, and SMITHERS_GIT_PATH records every git call.
    • main: fails with ['fetch origin', 'fetch origin', 'rebase origin/<sha>'].
    • Branch: no fetch or rebase. The run finishes, and the worktree HEAD is the launch commit on ultrafuzz/r1/task-a.

Other evidence (scratch runs, not committed)

  • Liveness guard sensitivity. I removed heartbeatEvidenceAtMs = heartbeatAtMs from the patched engine. The same quiet-agent workflow then failed with TASK_HEARTBEAT_TIMEOUT … has not heartbeated in 4434ms (timeout: 3000ms). So the test's finished assertion catches a patch that breaks attempt-row liveness.
  • Streaming rate. Without the heartbeat patch, one 3.5 s task with a stdout write every 10 ms appended 384-588 TaskHeartbeat rows over 9 runs; with it, 0.
  • Streaming wall time. In 8 concurrent runs of each mode at load average about 9, that task took 5.03-5.13 s end to end without the patch and 4.55-4.60 s with it.
  • Timeouts. No run hit its heartbeat timeout in either mode. So I did not reproduce the timeout failures a reviewer saw on main at load 30-37, and this PR makes no liveness claim.

Other tests (pass on this branch; not discriminating)

  • Compatibility guards:
    • every runner compatibility patch still anchors in the pinned Smithers release
    • compatibility patcher rewrites every described workaround (registry vs. fresh-install list)
    • compatibility patches declare every ultrafuzz helper inside the module they patch
    • patched engine admits authenticated controller path changes without accepting VCS relocation (imports the fully patched engine)
    • diagnoseProject reports a posture for every tracked compatibility patch
  • Controller refresh and generation: the 11 controller refresh / controller generation tests in runtime.test.ts.
  • Supporting files (39 tests, including the 2 new ones): controller-source, operator-npm, smithers-attempt-authority, smithers-diagnostic, smithers-executable-capability, smithers-package.
  • Native continuation: native continuation keeps a finished producer and runs only a newly rendered downstream task (see Risk).

Static checks (all pass)

  • npx prettier --check and npx eslint on the changed files
  • CI=1 ESLINT_PLUGIN_DIFF_COMMIT=origin/main pnpm -w lint:strict:ci
  • pnpm --filter @ultrafuzz/runtime typecheck
  • pnpm -w knip
  • node scripts/docs-check.mjs

Not run

Risk / compatibility

  • Which runs get this. Every Ultrafuzz process installs a fresh operator controller and applies the current registry to it. The points below are from code reading unless marked otherwise.
    • A run launched after upgrading seals a copy of that patched engine into its execution snapshot, and launch runs it.
    • ultrafuzz resume does not use the sealed runner. execSmithersCli resolves the current operator controller (prepareSmithersExecutableEnvironment), and a native continuation resolves the workflow's packages through that controller's node_modules. So a run launched before the upgrade should pick up the change once it is stopped and resumed.
    • Executed evidence for that resume path: the existing native continuation keeps a finished producer… test, run above, shows resumed tasks resolving packages from a freshly installed controller. I did not resume a run launched on an older version.
    • ultrafuzz replay and fork run the runner sealed at launch (linkedWorkflowExecutionEnvironment). For an older run they keep its old engine, and so does a runner process that is still executing.
    • The registry is read only at controller install, by doctor (which reports a not-yet-applied patch as a warning), and by refreshedSmithersControllerSnapshot, which has no production caller. I found nothing that compares a run's sealed engine against it, so older runs are not rejected.
  • No longer produced: TaskHeartbeat rows in smithers events and stream.ndjson, the gateway's task.heartbeat relay, and Smithers' in-memory heartbeat metrics. Ultrafuzz acts on none of these. Existing runs keep their rows, and ultrafuzz events --type still accepts TaskHeartbeat.
  • Worktrees never move to a newer origin tip. Ultrafuzz only ever passes a commit id, the pinned branch (materialized with no remote), or the governed-source commit. assertWorkspaceSourceRevision already requires HEAD to stay on the launch commit.
  • Merge with Bound worker resources and coalesce durable snapshots #1156. Draft PR Bound worker resources and coalesce durable snapshots #1156 patches the same TaskHeartbeat emit. If both land, the patch applied second cannot find its anchor and every controller install fails, so one of them must be dropped. Git also reports a textual conflict in smithers.ts.
  • Merge with w06, w08 and w15a. A reviewer checked that smithers.ts auto-merges with all three; CHANGELOG.md conflicts trivially.

Refs #1147, #1148, #1156

🤖 Generated with Claude Code

RetriggerConfidence Score: 5/5

No new blocking issue was identified in the changes since the previous review; the PR appears safe to merge on that basis.

Summary

The PR patches the pinned Smithers engine to keep attempt-row heartbeats without emitting TaskHeartbeat events, and to create and re-enter task worktrees without fetching or rebasing. It adds integration tests for both behaviors.

Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Controller installs compatibility patches] --> B[Patched Smithers engine]
  B --> C[Heartbeat]
  C --> D[Fenced attempt-row write]
  B --> E[Task worktree]
  E --> F[Create from local launch commit]
  F --> G[Re-enter without fetch or rebase]
Loading

Reviews (3) · Last reviewed commit: "chore: move the changelog entry to the c..."

@aviggiano
aviggiano requested a review from a team as a code owner September 28, 2026 22:59
aviggiano and others added 2 commits September 29, 2026 02:22
…tbeat write

The pinned @smthrs/engine flushHeartbeat() writes the fenced attempt-row
heartbeat and, when that write succeeds, appends a TaskHeartbeat event:
an _smithers_events row and a stream.ndjson line. Ultrafuzz never
attaches heartbeat data, so the event carries nothing the row lacks. A
quiet agent writes one per throttled liveness pulse (the watchdog pulses
every 250ms and writes are throttled to 500ms). An agent that streams
output writes one per ownership check its stdout, stderr and tool
callbacks force, and forced writes bypass the throttle: in a scratch run
with a stdout write every 10ms, one 3.5s task appended 384-588 of them.
Ultrafuzz never acts on these events: it handles only
TaskHeartbeatTimeout, the engine's heartbeat timeout advances only when
the attempt-row write succeeds, and `smithers why` reads the attempt row.

Add an engine_task_heartbeat_event compatibility patch that deletes the
emit and keeps the fenced write, registered in
SMITHERS_COMPATIBILITY_PATCHES and in the fresh operator-controller patch
list.

The new integration test runs a one-task workflow under Bun with every
registered compatibility patch applied as each Smithers module loads. Its
agent owns a quiet 3.5s child under a 3s heartbeat timeout. On main the
run records TaskHeartbeat rows (6 to 8 in the runs observed); with this
patch it records none, the task still finishes, and the attempt row's
heartbeat_at_ms keeps advancing.

Refs #1147

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Smithers' <Worktree baseBranch> treats its value as a branch to track.
Creating a worktree runs `git fetch origin` first. Each time a task
re-enters an existing worktree, the pinned engine retries
`git rebase origin/<base>`, after another `git fetch origin` unless one
succeeded for that repository in the last 60s (the worktree sync cache's
default TTL). Ultrafuzz passes the recorded launch commit (or the pinned
source branch), and origin/<sha> never resolves, so every re-entry by an
agent or verifier task logs "worktree sync rebase failed"; a failed
rebase is never recorded, so it repeats. The fetches have no timeout,
update the user's remote-tracking refs, and are retried on every
re-entry while they fail; on this host `git fetch origin` against an
unreachable HTTPS remote took 136s to fail. Task worktrees must stay on
the launch commit, which assertWorkspaceSourceRevision enforces.

Add two engine compatibility patches: engine_worktree_sync makes
getWorktreeSyncCache() return an inert cache, so the re-entry path never
fetches or rebases (git and jj), and engine_worktree_create_fetch removes
the fetch before `git worktree add`, which already tried the local base
first. Both are registered in SMITHERS_COMPATIBILITY_PATCHES and in the
fresh operator-controller patch list.

The new integration test runs a <Worktree> seeded from a local-only launch
commit with a preparation task that creates it and a verifier task that
re-enters it, on the controller-patched engine, with every git invocation
recorded. On main it records `fetch origin`, `fetch origin` and
`rebase origin/<sha>`; with this patch it records no fetch or rebase, the
run finishes, and the worktree is on the launch commit and its task
branch.

#1148 also asks for worktree retention and missing-object diagnostics,
which this does not change.

Refs #1148

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@aviggiano
aviggiano force-pushed the claude/w09-smithers-unused-behaviors branch from f6a62a6 to 82b008d Compare September 29, 2026 02:41
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]>
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