Skip to content

perf(workbench): stream the tracked patch into the content digest - #256

Open
rohanpoudel2 wants to merge 1 commit into
openai:mainfrom
rohanpoudel2:fix/streaming-content-digest
Open

perf(workbench): stream the tracked patch into the content digest#256
rohanpoudel2 wants to merge 1 commit into
openai:mainfrom
rohanpoudel2:fix/streaming-content-digest

Conversation

@rohanpoudel2

Copy link
Copy Markdown

Fixes #249

Problem

worktree_content_digest_for_context obtained the working-tree patch through git_bytes, which is subprocess.run(..., capture_output=True), so completed.stdout held the entire git diff --binary output before a single byte was hashed. This is the hotter of the two digest paths: it runs on every setup inspection, not only at scan registration.

Binary patches are larger than the files they describe, because git diff --binary base85-encodes both the forward and the reverse literal. Measured on a fixture with a 20 MiB incompressible change (macOS 15, Python 3.14.5, git 2.50.1), the patch was 51.5 MiB — 2.58x the file — and the digest process peaked at 145.8 MiB RSS, against a 25.2 MiB floor for the same code on a clean worktree.

ea19f24 contains only the working-tree digest function; there is no committed_diff_content_digest helper in this base (PR #241 is not merged here), so there is one buffering call site to fix, not two.

Change

A new git_digest_field next to git_bytes in sdk/typescript/_bundled_plugin/scripts/workbench_target.py:

  • spools Git's stdout directly into a private temporary file by passing the file object as stdout= to the subprocess, so the patch never passes through Python memory;
  • takes the value length from os.fstat on that file;
  • writes exactly the framing update_digest_field writes — 4-byte big-endian label length, label, 8-byte big-endian value length — then hashes the file in 1 MiB chunks;
  • returns whether Git succeeded, mirroring git_bytes returning None, and leaves the digest untouched when it did not.

git_command gained an optional stdout parameter so the streaming path reuses the existing environment scrubbing and core.fsmonitor=false hardening rather than building its own command. capture_output=True became the equivalent explicit stdout=PIPE, stderr=PIPE, so every other caller behaves exactly as before.

worktree_content_digest_for_context uses the helper for the tracked-diff field. git_bytes is unchanged and still serves its many small callers, including the untracked ls-files -z listing in the same function — that output is one NUL-separated path list and does not need spooling.

Digest stability

Recorded digests are compared against freshly computed ones when a saved selection is revalidated, so a changed digest would read as changed reviewed content. Old (ea19f24) and new code were run over the same five fixture repositories, in separate processes, and the digests compared:

fixture digest old == new peak RSS old peak RSS new
clean worktree (empty diff, no untracked) identical 1d74df0bc5da366e… 25.2 MiB 25.5 MiB
small text diff identical 30e5823fd6690d65… 25.1 MiB 25.5 MiB
64 KiB binary diff identical 62fc20ee762a9698… 25.4 MiB 25.6 MiB
diff plus untracked file, directory, and symlink identical 7c835ef170583e74… 25.0 MiB 25.5 MiB
20 MiB binary diff (51.5 MiB patch) identical 3fe4d8677ae38269… 145.8 MiB 27.4 MiB

Full digests:

clean          codex-security-snapshot/v1:sha256:1d74df0bc5da366ec7aad16a4841552de3d91d1cb5319d4e849096130ccb54eb
text-diff      codex-security-snapshot/v1:sha256:30e5823fd6690d659b6eb2835d41259cf8e4cf4e7759799585a9f4b0d3cfaf3c
binary-diff    codex-security-snapshot/v1:sha256:62fc20ee762a969823d3319a480220cb33cf688dd8b63a17ab317bbcd32399c1
untracked      codex-security-snapshot/v1:sha256:7c835ef170583e74afb92a813c02dd91c9d898c2c9026c1c37e2ca6bb15fa363
large-binary   codex-security-snapshot/v1:sha256:3fe4d8677ae382693cdcc48d674752910bada31ef09703850212541abca4f4f6

The clean-worktree sentinel is the case a mismatch would break most quietly, so it is checked explicitly. clean_worktree_content_digest() is a hardcoded value, and the streaming path still reproduces it:

sentinel, old code:                 …:1d74df0bc5da366ec7aad16a4841552de3d91d1cb5319d4e849096130ccb54eb
sentinel, new code:                 …:1d74df0bc5da366ec7aad16a4841552de3d91d1cb5319d4e849096130ccb54eb
clean worktree digest, new code:    …:1d74df0bc5da366ec7aad16a4841552de3d91d1cb5319d4e849096130ccb54eb

Failure behaviour is also unchanged: against a repository with no HEAD, so that git diff HEAD fails, both old and new exit 1 with Could not snapshot the selected working-tree changes.

tests-ts/workbench-content-digest.test.ts pins this permanently rather than relying on a golden hex constant, which would be hostage to the Git version and to core.autocrlf on Windows. The probe computes each digest twice in one process: once through the streaming helper, and once with git_digest_field replaced by the buffered git_bytes plus update_digest_field pair it replaced. The two must be equal for a clean worktree, a text diff, a binary diff, and a tree with untracked entries, and the clean worktree must equal the sentinel.

Why spool to a temp file and not stream directly

update_digest_field writes an 8-byte big-endian value length before the value, so the total byte count is part of the hashed material and has to be known before any content is hashed. Feeding git's stdout pipe straight into hashlib is therefore not possible without changing the framing, which would change every digest.

The alternative is a second git diff pass purely to count bytes. That doubles the work and, worse, is not atomic: the working tree can change between the counting pass and the hashing pass, producing a digest that describes no state that ever existed. Spooling keeps one Git invocation and yields the length from fstat, at the cost of writing the patch to a temporary file.

Temporary-file handling, given how strict this codebase is about scan-directory privacy: the spool is a tempfile.TemporaryFile(), so it lands in the process temporary directory and never in the scan directory or the scanned repository; it is created mode 0600; on POSIX it is unlinked before Git writes to it, verified as st_nlink == 0 with no directory entry, so the patch is never reachable by name and cannot outlive the process even on a kill; and removal is guaranteed by the with block on every path, including the Git-failure return. The test runs each probe with TMPDIR pointed at a private directory and asserts that directory is empty afterwards.

Impact, stated plainly

On the 20 MiB fixture the digest process peak RSS drops from 145.8 MiB to 27.4 MiB, a floor set by the interpreter plus the 1 MiB hashing chunk rather than by the patch size. Small fixtures move by about 0.4 MiB, within noise. Git's own peak RSS while producing the patch (165.6 MiB on that fixture) is untouched by this change — it is Git building the binary delta, not the workbench buffering it.

Digest values, digest framing, the Git invocations and their order, and the error message on failure are all unchanged. git_bytes and its callers are unchanged.

Verification

From sdk/typescript:

  • bun test --timeout 30000 ./tests-ts — 775 pass, 5 skip, 0 fail, 780 tests across 35 files, 39.5 s.
  • The new tests-ts/workbench-content-digest.test.ts — 2 pass, 31 assertions, 1.6 s. The large-binary case is deliberately 4 MiB rather than 20 MiB: it produces a 10.3 MiB patch, keeps the file under a second, and asserts the peak RSS increase across the digest call (measured 1.6 MiB) stays under half the patch size, which buffering could not satisfy. It is POSIX-only because it needs resource.getrusage.
  • pnpm run types (generate-models --check and tsc --noEmit) — clean.
  • pnpm run format (prettier --check) — clean.
  • python3 -m py_compile workbench_target.py — clean; generated __pycache__ removed.

`worktree_content_digest_for_context` read the working-tree patch through
`git_bytes`, which is `subprocess.run(..., capture_output=True)`, so the whole
`git diff --binary` output was materialised in memory before being hashed. A
repository holding a large changed binary could therefore exhaust the workbench
during setup inspection, which runs on every inspection rather than only at
registration. On a fixture with a 20 MiB incompressible change, Git emitted a
51.5 MiB patch and the digest process peaked at 145.8 MiB RSS.

`update_digest_field` frames every value with an 8-byte big-endian length, so
the total byte count must be known before any content is hashed and stdout
cannot simply be fed into the hash. `git_digest_field` spools Git's stdout
straight to a private temporary file, takes the length from `fstat`, writes the
same framing, and then hashes the file in 1 MiB chunks. A single Git invocation
still produces the patch, so the snapshot stays atomic.

The spool file lives in the process temporary directory, never in the scan
directory or the scanned repository, is created owner-only, and is removed by
the `with` block; on POSIX it is unlinked before Git writes to it, so the patch
is never reachable by name.

`git_bytes` keeps its buffered behaviour for its many small callers, including
the untracked `ls-files` listing in the same function.

Digests are unchanged. Old and new code produce identical digests for an empty
diff, a text diff, a binary diff, a diff with untracked files, and a 20 MiB
binary diff, and a clean worktree still hashes to the hardcoded
`clean_worktree_content_digest` sentinel. Peak RSS on the 20 MiB fixture falls
from 145.8 MiB to 27.4 MiB.
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.

Content digests buffer entire git diff --binary patches in memory

1 participant