Skip to content

Commit bfc6b18

Browse files
mchwangclaude
andauthored
D: name submodule content and byte-rewriting attributes in the export; check the stderr filter (#80)
* Name submodule content and byte-rewriting attributes in the export; check the stderr filter's status The non-recursive task clone leaves a submodule as an empty directory. Git never lists files there, so an agent's files (or a repository it commits in) vanished from the export; a submodule directory that is not empty, or cannot be read, is now named in a notice. The built-in ident and working-tree-encoding attributes change what the diff shows without any config, so a changed or new file with either set is named too. Line-ending attributes are left alone: overriding them would make every CRLF checkout look changed. The stderr filter's exit status is now checked, since a filter that failed (for example on a full /tmp) could lose a read failure, and SIGPIPE is accepted only when the output really reached its limit. The docs now say the failure carries Git's last two error lines, as the code does. Findings from the PR 74 review session's round 2 on 176a631 (PR 76, already merged). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test the unreadable-submodule, filter-failure and early-SIGPIPE paths of the export An unreadable submodule directory is now in the real-Docker submodule case. The filter failure and a SIGPIPE before the limit need an exactly full /tmp to happen for real, so two tests run the export script with a failing stand-in for awk or head first on PATH; EXPORT_SCRIPT is exported for them. Each test fails when its check is removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 256c336 commit bfc6b18

4 files changed

Lines changed: 96 additions & 10 deletions

File tree

‎agents/container/storage.ts‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -537,13 +537,17 @@ export interface ExportOptions extends PreparationOptions {
537537
// only SIGPIPE (141) from the producer is expected, when `head` stops reading at the limit. A Git warning that it could
538538
// not read a directory or path also fails it, since Git then diffs that path as absent; other warnings do not.
539539
// Repository config is trusted: only codeboost writes the metadata volume, which every agent container mounts
540-
// read-only. Worktree attributes are the agent's, but a filter or diff driver needs config to run anything; external
541-
// diff programs and text conversion are off, and the worktree and attributes file are pinned. A populated submodule's
540+
// read-only. Worktree attributes are the agent's. A filter or diff driver needs config to run anything; external diff
541+
// programs and text conversion are off, and the worktree and attributes file are pinned. Two built-in attributes still
542+
// change what the diff shows without config (`ident` collapses `$Id: ... $`, `working-tree-encoding` re-encodes), so a
543+
// changed or new file with either set is named in a notice. Line-ending attributes change only line endings, and
544+
// overriding them would make every file a repository checks out with CRLF look changed, so they are left alone. A populated submodule's
542545
// own config is the agent's, so Git never looks inside one: every diff passes --ignore-submodules on the command line,
543546
// which, unlike the config default, overrides the worktree's .gitmodules and applies to plumbing diff-index too. A
544547
// submodule's pointer change is still exported, but no `git status` runs inside it, and so none of its filters. core.safecrlf is off
545548
// so ordinary line-ending attributes (`text=auto`, `eol=crlf`) do not warn on stderr and fail a correct export.
546-
const EXPORT_SCRIPT = [
549+
// Exported only so tests can run it with failing stand-ins for the tools it uses; `exportTaskDiff` is the entry point.
550+
export const EXPORT_SCRIPT = [
547551
'set -eu',
548552
'base=$1 limit=$2',
549553
'export HOME=/tmp GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=/dev/null GIT_OPTIONAL_LOCKS=0 GIT_TERMINAL_PROMPT=0 GIT_NO_LAZY_FETCH=1',
@@ -619,6 +623,12 @@ const EXPORT_SCRIPT = [
619623
' for my $part (@part) { $p .= "$part/"; next PATH if $dir{$p} } print $out "$path\\0" }',
620624
' close($out) or die }\'',
621625
' g ls-files -z --stage > /tmp/export-stage',
626+
' # The task clone is not recursive, so a submodule is an empty directory. Git never lists files under it, and',
627+
' # --ignore-submodules keeps it from looking inside, so anything the agent put there (files, or a repository with',
628+
' # commits) would vanish. A submodule directory that is not empty, or cannot be read, is named in a notice.',
629+
' perl -0 -ne \'chomp; next unless /^160000 \\S+ \\d+\\t(.*)\\z/s; my $p = $1; next if -l $p || !-d _;',
630+
' my $d; my $full = !opendir($d, $p) || grep { $_ ne "." && $_ ne ".." } readdir $d;',
631+
' print "$p\\0" if $full\' /tmp/export-stage > /tmp/export-gitlink',
622632
' { find . -path ./.git -prune -o -name .git -print0 -prune -o \\( -type p -o -type s \\) -print0 2>/dev/null || true; } \\',
623633
' | perl -0 -e \'my %skip; for my $f ("/tmp/export-ignored", "/tmp/export-nested") { open(my $h, "<", $f) or die;',
624634
' while (<$h>) { chomp; $skip{$_} = 1 } }',
@@ -642,6 +652,16 @@ const EXPORT_SCRIPT = [
642652
' while IFS= read -r -d "" path; do',
643653
' printf "codeboost: %q is over 8 MiB; if it changed, its content is not exported\\n" "$path"',
644654
' done < /tmp/export-large',
655+
' while IFS= read -r -d "" path; do',
656+
' printf "codeboost: submodule directory %q has content in the task worktree; it is not exported\\n" "$path"',
657+
' done < /tmp/export-gitlink',
658+
' # A changed or new file whose attributes make Git show something other than its bytes (see above).',
659+
' cat /tmp/export-changed /tmp/export-new | g check-attr -z --stdin ident working-tree-encoding \\',
660+
' | perl -0 -ne \'chomp; push @f, $_; if (@f == 3) { my ($path, $attr, $value) = @f; @f = ();',
661+
' print "$path\\0$attr\\0" if $value ne "unspecified" && $value ne "unset" && !$seen{$path}++ }\' > /tmp/export-attrs',
662+
' while IFS= read -r -d "" path && IFS= read -r -d "" attr; do',
663+
' printf "codeboost: %q has the %s attribute, so its diff may not show its real bytes\\n" "$path" "$attr"',
664+
' done < /tmp/export-attrs',
645665
' # An untracked nested repository cannot be diffed and is named in a notice; %q keeps its name on one line.',
646666
' while IFS= read -r -d "" path; do',
647667
' printf "codeboost: untracked directory %q is a nested repository; its contents are not exported\\n" "$path"',
@@ -674,12 +694,16 @@ const EXPORT_SCRIPT = [
674694
' || /^(error|fatal): .*Input\\/output error$/) && n < 101 { print; n++; next }',
675695
' /^(error|fatal): / { before = last; last = $0 }',
676696
' END { if (before != "") print before > "/tmp/export-errors"; if (last != "") print last > "/tmp/export-errors" }\' \\',
677-
' > /tmp/export-failure 3>&-; return "${PIPESTATUS[0]}"; } 3>&1',
697+
' > /tmp/export-failure 3>&-',
698+
' # The filter writes its file when it exits; if it failed (for example /tmp full), a read failure may be missing.',
699+
' local s=("${PIPESTATUS[@]}"); if [ "${s[1]}" -ne 0 ]; then return 98; fi; return "${s[0]}"; } 3>&1',
678700
'}',
679701
': > /tmp/export-errors',
680-
'filtered | head -c "$limit" | base64 -w0',
702+
'# tee keeps what head passed on, so SIGPIPE can be accepted only when head really stopped at the limit.',
703+
'filtered | head -c "$limit" | tee /tmp/export-out | base64 -w0',
681704
'statuses=("${PIPESTATUS[@]}")',
682705
'set -e',
706+
'if [ "${statuses[0]}" -eq 98 ]; then echo "the export could not check Git\'s warnings" >&2; exit 5; fi',
683707
'# Git also looks up attribute and ignore files through a symlink that replaced a tracked directory. What is behind it',
684708
'# is not part of the worktree (the diff shows the directory becoming a link), so a failure there is not one. Past 100',
685709
'# lines the rest were not kept, so that fails too.',
@@ -698,13 +722,16 @@ const EXPORT_SCRIPT = [
698722
' exit 6',
699723
'fi',
700724
'if [ "${statuses[0]}" -eq 7 ]; then echo "more than 1,000 changed files are over 8 MiB; the diff cannot be exported" >&2; exit 7; fi',
725+
'if [ "${statuses[0]}" -eq 141 ] && [ "$(stat -c %s /tmp/export-out)" -ne "$limit" ]; then',
726+
' echo "git stopped on SIGPIPE before the output reached its limit" >&2; exit 5',
727+
'fi',
701728
'if [ "${statuses[0]}" -ne 0 ] && [ "${statuses[0]}" -ne 141 ]; then',
702729
' # Git\'s last two errors say why it stopped (earlier ones can be about paths behind a symlink, which do not fail it).',
703730
' # They can quote agent-chosen names, so they are cut short and kept to printable text.',
704731
' detail=$(awk \'NR > 1 { printf "; " } { printf "%s", $0 }\' /tmp/export-errors | head -c 400 | tr -d "\\000-\\010\\013-\\037")',
705732
' echo "git failed while exporting the diff (status ${statuses[0]})${detail:+: $detail}" >&2; exit 4',
706733
'fi',
707-
'if [ "${statuses[1]}" -ne 0 ] || [ "${statuses[2]}" -ne 0 ]; then echo "the export pipeline failed" >&2; exit 5; fi',
734+
'if [ "${statuses[1]}" -ne 0 ] || [ "${statuses[2]}" -ne 0 ] || [ "${statuses[3]}" -ne 0 ]; then echo "the export pipeline failed" >&2; exit 5; fi',
708735
].join('\n');
709736

710737
function* exportSteps(workVolume: string, metadataVolume: string, owner: ResourceOwner, options: ExportOptions,

‎docs/implementation/agent-isolation.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,9 @@ Use only these entry points to run an agent:
8383
`removeTaskFilesystems`. `base` is the full ID of the last commit codeboost made in that storage. It returns at
8484
most 1 MiB of diff (`truncated` says whether it was cut), and takes `signal` and a deadline for the Docker work.
8585
A changed file over 8 MiB, an untracked nested repository, a path Git will not add, an entry named `.git`, a fifo
86-
or socket, and an ignored untracked path each appear as a `codeboost:` notice line, not a diff. When Git fails, the
87-
error carries Git's first error line.
86+
or socket, an ignored untracked path, a submodule directory with content, and a changed file whose `ident` or
87+
`working-tree-encoding` attribute changes what the diff shows each appear as a `codeboost:` notice line. When Git
88+
fails, the error carries Git's last two error lines.
8889
6. After a crash or restart, call `recoverLeftovers(runnerOwner)` (`agents/recovery.ts`) while holding the
8990
database's single-runner lock and before admitting work. It touches only objects labelled with that runner
9091
token. It removes agent containers, egress proxies, seeders, export containers and networks, and resolves once they are gone. It keeps

‎docs/implementation/runner-lifecycle.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ The server computes `retryable` and sends it to the UI. The UI never works it ou
237237

238238
| Holder | Owner | Rule |
239239
|---|---|---|
240-
| Partial output of a stopped writable attempt | F, using a D export | Done in D by #51 item 6: `exportTaskDiff(storage, { base, imageId, maxBytes, signal, onProcessGroup, timeoutMs })` (`agents/container/storage.ts`) takes the storage value or a recovery handle and `base`, the full ID of the last commit codeboost made there, and returns `{ diff, truncated }`: at most `maxBytes` (1 MiB at most) of raw diff bytes. It checks both volumes' labels, runs Git in a read-only container with no network that writes nothing to either volume, covers committed, staged, unstaged and new untracked files, and cuts the output inside the container, so the Docker work stops at the limit. A changed file over 8 MiB, an untracked nested repository, or an untracked path Git will not add (a part named `.git`, or a symlink named `.gitmodules`) is named in a `codeboost:` notice line instead of diffed, so one large file cannot exhaust the export's memory or deadline; untracked symlinks are diffed as links. What Git skips without a word is named in a notice line too: an entry named `.git` (and, if it holds a repository, the new files beside it), a fifo or socket, and an ignored untracked path. Anything it cannot read fails the export, and a Git failure's reason includes Git's first error line. On abort or at its deadline it stops the client and removes the export container before it settles (an `AbortError`, or an `AggregateError` if that removal did not settle); recovery removes a leftover one. The task filesystems are Docker volumes behind a keeper container. F allocates them with `prepareTaskFilesystems` and removes them with `removeTaskFilesystems`; D's supervisor does not remove them at settlement. But F cannot read a volume from the host. So the order is: `settled` → a **D-provided bounded export** (a new D operation, for example `exportTaskDiff(filesystems, maxBytes)`, which runs `git diff` against the last codeboost commit in a read-only container and returns at most 1 MiB) → F saves it → the terminal write stores `diagnostic_ref` → `removeTaskFilesystems` → the slot is freed. **The export has an overall deadline** (60 seconds, passed to D as an `AbortSignal`), because `maxBytes` bounds only the returned data, not the Docker work. On timeout or any export failure, F records "Partial output could not be exported: <reason>" as the diagnostic and continues with the terminal write and removal; shutdown and recovery never wait on an export past its deadline. F saves the export to a runner-owned diagnostics directory, with a total byte cap. **Retention never deletes a file an attempt row still references.** When the cap is exceeded, retention first deletes unreferenced files, oldest first. If that is not enough, it takes the oldest referenced file and, in one transaction, sets that row's `diagnostic_ref` to null and appends "(partial output removed by retention)" to its diagnostic. Only after that commit does it delete the file. No row is ever left pointing at a missing file. The attempt row references it as `diagnostic_ref`. If the capture fails, the row records that it failed. The task filesystem is never reused. |
240+
| Partial output of a stopped writable attempt | F, using a D export | Done in D by #51 item 6: `exportTaskDiff(storage, { base, imageId, maxBytes, signal, onProcessGroup, timeoutMs })` (`agents/container/storage.ts`) takes the storage value or a recovery handle and `base`, the full ID of the last commit codeboost made there, and returns `{ diff, truncated }`: at most `maxBytes` (1 MiB at most) of raw diff bytes. It checks both volumes' labels, runs Git in a read-only container with no network that writes nothing to either volume, covers committed, staged, unstaged and new untracked files, and cuts the output inside the container, so the Docker work stops at the limit. A changed file over 8 MiB, an untracked nested repository, or an untracked path Git will not add (a part named `.git`, or a symlink named `.gitmodules`) is named in a `codeboost:` notice line instead of diffed, so one large file cannot exhaust the export's memory or deadline; untracked symlinks are diffed as links. What Git skips without a word is named in a notice line too: an entry named `.git` (and, if it holds a repository, the new files beside it), a fifo or socket, an ignored untracked path, and anything in a submodule directory. A changed file whose `ident` or `working-tree-encoding` attribute changes what the diff shows is named too. Anything it cannot read fails the export, and a Git failure's reason includes Git's last two error lines. On abort or at its deadline it stops the client and removes the export container before it settles (an `AbortError`, or an `AggregateError` if that removal did not settle); recovery removes a leftover one. The task filesystems are Docker volumes behind a keeper container. F allocates them with `prepareTaskFilesystems` and removes them with `removeTaskFilesystems`; D's supervisor does not remove them at settlement. But F cannot read a volume from the host. So the order is: `settled` → a **D-provided bounded export** (a new D operation, for example `exportTaskDiff(filesystems, maxBytes)`, which runs `git diff` against the last codeboost commit in a read-only container and returns at most 1 MiB) → F saves it → the terminal write stores `diagnostic_ref` → `removeTaskFilesystems` → the slot is freed. **The export has an overall deadline** (60 seconds, passed to D as an `AbortSignal`), because `maxBytes` bounds only the returned data, not the Docker work. On timeout or any export failure, F records "Partial output could not be exported: <reason>" as the diagnostic and continues with the terminal write and removal; shutdown and recovery never wait on an export past its deadline. F saves the export to a runner-owned diagnostics directory, with a total byte cap. **Retention never deletes a file an attempt row still references.** When the cap is exceeded, retention first deletes unreferenced files, oldest first. If that is not enough, it takes the oldest referenced file and, in one transaction, sets that row's `diagnostic_ref` to null and appends "(partial output removed by retention)" to its diagnostic. Only after that commit does it delete the file. No row is ever left pointing at a missing file. The attempt row references it as `diagnostic_ref`. If the capture fails, the row records that it failed. The task filesystem is never reused. |
241241
| Interrupted rebase | F3 | F3 records `rebase in progress` durably before starting a rebase and clears it after. It is stored as `tasks.rebase_in_progress`: null, or JSON `{ attemptId, oldHead, onto, startedAt }`. F3 sets it by compare-and-swap from null, and clears it by compare-and-swap on the same `attemptId`. Startup recovery aborts every recorded rebase through the runner before it hands the task to I3. |
242242
| Workspace rebuild | I3 | I3 never rebuilds before both rows above are resolved for the task. |
243243

0 commit comments

Comments
 (0)