Skip to content

docs(runtime): remove a causal claim I merged that measurement disproves - #308

Merged
aviggiano merged 2 commits into
mainfrom
fix/correct-corpus-exclusion-rationale
Aug 6, 2026
Merged

aviggiano merged 2 commits into
mainfrom
fix/correct-corpus-exclusion-rationale

Conversation

@aviggiano

@aviggiano aviggiano commented Aug 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Removes a causal claim I merged in #305 that measurement disproves.

What was wrong

#305's doc comment attributed four sandbox deaths at stateful-invariant-setup to a cgroup OOM inside git add. Reviewers of the follow-up refuted it, and I reproduced the disproof rather than take their word for it:

$ head -c 155000000 /dev/zero > big{1,2,3,4}.bin   # 620 MB across four blobs
$ /usr/bin/time -v git add -A
        Maximum resident set size (kbytes): 155812

git add peak RSS is per-file, not cumulative — 620 MB across four blobs peaks the same as one — and a single blob above core.bigFileThreshold streams at a few megabytes. Against memoryMiB: 32_768 / memoryLimitMiB: 65_536 (packages/modal/src/defaults.ts:28-32), staging the observed 575 MB workspace costs well under 1% of the memory request.

It has since been settled from the other direction too: R47 attempt 1 recorded worker_exit_code: 1 — the first exit code ever captured for one of these deaths, thanks to #303. An OOM kill is 137. Exit 1 is a deliberate process exit, so no memory theory explains it.

Why this is worth its own PR

Leaving a disproven causal story in the source is the exact defect I spent two PRs fixing today in the prompts: text that contradicts what the code and environment actually do, discoverable only by someone burning a node on it. A future maintainer reading that comment would have started from a dead end I already walked.

The comment now carries the measurement, the sandbox limits, and a plain statement that the cause is unidentified — plus an explicit warning not to read it as a diagnosis.

What is unchanged

The exclusion itself, and its justification: harness-generated corpus is not authored content, nothing downstream consumes it from a patch, and MAX_PATCH_BYTES meant no corpus above 16 MB ever survived a handoff. It simply is not the fix for these deaths.

The size-ceiling follow-up (#306) is closed, for the same reason plus three independent defects review found in it.

Validation

Comment only — no behaviour change. workspace-handoff 16/16, typecheck, eslint, prettier clean.

Refs #304, #307

Greptile Summary

This PR replaces an unsupported OOM diagnosis in the workspace-handoff documentation with measured evidence identifying oversized staged diffs and ENOBUFS as the observed failure.

  • Documents the ordering between runGit’s 32 MB buffer limit and the 16 MB patch-size check.
  • Records why generated corpus exclusions are operationally necessary.
  • Clarifies that measured git add memory use does not support the previous OOM claim.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/runtime/src/workspace-handoff.ts Updates an operational comment explaining the workspace-patch exclusion and observed failure without changing runtime behavior.

Reviews (2): Last reviewed commit: "docs(runtime): state the verified cause ..." | Re-trigger Greptile

Context used:

#305's doc comment attributed four sandbox deaths at `stateful-invariant-setup` to
a cgroup OOM inside `git add`. Reviewers of the follow-up PR refuted it, and I
reproduced the disproof rather than take their word for it:

    $ head -c 155000000 /dev/zero > big{1,2,3,4}.bin   # 620 MB across four blobs
    $ /usr/bin/time -v git add -A
            Maximum resident set size (kbytes): 155812

`git add` peak RSS is per-file, not cumulative -- 620 MB across four blobs peaks the
same as one -- and a single blob above `core.bigFileThreshold` streams at a few
megabytes. Against `memoryMiB: 32_768` / `memoryLimitMiB: 65_536`, staging the
observed 575 MB workspace costs well under one percent of the memory request.

Leaving that claim in the source would have been the exact defect I spent two PRs
fixing today: text that contradicts what the code and the environment actually do,
discoverable only by someone burning a node on it. It is replaced by the
measurement, the sandbox limits, and a plain statement that the cause of those
deaths is still unidentified -- plus a warning not to read the comment as a
diagnosis.

The exclusion itself is unchanged and still justified on its own terms: generated
corpus is not authored content, nothing downstream consumes it from a patch, and
`MAX_PATCH_BYTES` meant no corpus above 16 MB ever survived a handoff. It simply is
not the fix for these deaths.

The size-ceiling follow-up (#306) is closed for the same reason, plus three
independent defects review found in it: total bytes is the wrong metric for a
per-file peak, tracked unmodified content counted toward a ceiling it contributes
nothing to, and the check landed in `captureWorkspaceTree` where one polluted
workspace would fail every downstream node before its agent could clean up.

Comment only. workspace-handoff 16/16.

Refs #304

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both reviewers rejected the previous version of this comment, and they were right
twice over.

It said the cause of the sandbox deaths "remains unidentified" and told the reader
"do not let this comment be read as a diagnosis". The cause had been identified hours
earlier, from R47's workflow log: `spawnSync git ENOBUFS` at
`runGit -> withTemporaryIndex -> captureWorkspacePatch`, on a diff of
`echidna/coverage/...`. So a comment written to stop false claims living in the source
was about to install two more, and the second actively instructs the next reader to
discount the answer.

Worse, it retained verbatim the sentence that caused the original misdiagnosis: "an
oversized patch is a loud, catchable ENOBUFS". That is the exact reasoning used to
rule ENOBUFS OUT. "Catchable" was vacuous -- nothing caught it anywhere -- and "loud"
was false, because the worker discarded the reason (#307) and the only surviving copy
was a 68 MB workflow log nothing surfaces. Being throw-able is not being visible, and
the comment now says so.

Also corrected: my reproduction command. `head -c 155000000 /dev/zero > big{1,2,3,4}.bin`
is an ambiguous redirect and writes nothing -- a reviewer caught it, and I confirmed by
running it. So the four-blob measurement I published was not what I had run. Rerun
properly:

    $ for i in 1 2 3 4; do head -c 155000000 /dev/zero > "big$i.bin"; done
    $ /usr/bin/time -v git add -A   ->   Maximum resident set size: 155788 kbytes

620 MB across four blobs, peak RSS 155 MB. The conclusion survives -- `git add` peak is
per-file, not cumulative -- but it was luck that it did, and the record now shows the
command that actually produces it.

The comment also states the ordering that matters: above 32 MB the ENOBUFS fires inside
`runGit` BEFORE the `MAX_PATCH_BYTES` check can produce a clean error, so the
clean-error window is only 16-32 MB. And it says the exclusion list is load-bearing
rather than hygiene, so nobody relaxes it on tidiness grounds and silently restores a
sandbox kill.

Comment only. workspace-handoff 16/16.

Refs #304, #307, #310

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@aviggiano
aviggiano merged commit c6d6ea8 into main Aug 6, 2026
4 checks passed
@aviggiano
aviggiano deleted the fix/correct-corpus-exclusion-rationale branch August 6, 2026 22:44
aviggiano added a commit that referenced this pull request Aug 7, 2026
Integrates origin/main (fa8be64): PR #287's invariant-suite handoff deletion
channel and durable provenance, plus #282, #284, #290, #294, #296, #298, #300,
#303, #305 and #308. CHANGELOG keeps both Unreleased sections; the modal runner
test import list keeps both new symbols.

Fixes the review finding on #165 (issue #212). Compiled task specs can only name
singular `test/...` output roots, because they are built before the worktree
exists. Targets are not consistent: the Aave v4 target at 6959e321 has no `test/`
at all, only plural `tests/`. The generated workflow created the singular tree
regardless, so such a repository grew empty `test/recon`, `test/chimera` and
`test/invariants` directories, which setup.md tells the model to read as the
repository's convention, and retry cleanup then scrubbed that invented tree while
leaving the real `tests/` output behind.

Workspace output roots are now re-anchored at execution time onto the test root
the repository actually uses, restoring the semantics of main's `invariantTestRoots`
helper that this branch dropped in merge 2bbecba, at the layer this branch's retry
preparation uses.

Three review findings on the first cut of that fix are addressed here:

- Exactly ONE root is selected, never the union of those present. Anchoring under
  every existing root would have recreated the same defect from the other side: once
  an ancestor stage introduces the other root, each invariant stage would create an
  empty duplicate tree beside the real one. Preference follows
  INVARIANT_TEST_ROOT_NAMES, so a repository that has `test/` behaves exactly as it
  did before this helper existed and only a repository without it is redirected.
- The resolution is pinned per attempt at preparation time. Rediscovering per call was
  unsafe: prepareTaskWorkspaceOutputRoots finishes by replaying ancestor workspace
  patches, and an ancestor stage can introduce the other test root as a side effect, so
  a second independent discovery could select a root whose subdirectories were never
  created. taskWorkspaceOutputRoots would then lstat a missing path and throw a bare
  ENOENT instead of the typed artifact-contract failure, killing a retries:0 node on
  exactly the restart and resume paths this machinery exists to survive.
- Only ENOENT and ENOTDIR mean "this root is absent". Swallowing EACCES or ELOOP would
  silently fall back to the singular default against a root that does exist.

Test changes, also from review:

- The non-directory case was vacuous: with only a regular file named `test` present,
  "rejected it" and "fell back to the default" both read `test`. It now runs against a
  repository that also has a real `tests/`, so the outputs differ; verified by mutation
  (dropping the isDirectory() guard fails the test).
- taskWorkspaceOutputRoots' use of the helper is now pinned in the generated-source
  assertions. Reverting only that call site previously left every test green while
  every task on a `tests/`-only target died on a bare ENOENT.
- Adds coverage for the pinning behaviour, for per-attempt isolation of the pin, and
  for surfacing an unreadable candidate root.

Also repairs a merge-integration gap the new #217 harness could not have anticipated:
it lifts resetTaskArtifactsForRetry, which on this branch calls prepareAnchoredDirectory
(branch-only, absent from main), so the harness now supplies that collaborator and drops
the dead invariantTestRoots stub.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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