Skip to content

ECO-2484: feat(datasets): cache HF rows pages and task repo checkouts on disk - #44

Open
abhinav-pola wants to merge 8 commits into
mainfrom
feat/dataset-cache
Open

ECO-2484: feat(datasets): cache HF rows pages and task repo checkouts on disk#44
abhinav-pola wants to merge 8 commits into
mainfrom
feat/dataset-cache

Conversation

@abhinav-pola

@abhinav-pola abhinav-pola commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes ECO-2484 — stop re-fetching HF/GitHub task datasets on every run.

  • HF /rows pages cached on disk per (token scope, dataset, config, split, revision, offset) under BENCH_DATASET_CACHE_DIR (default ~/.cache/openrouter-bench-harness). Revision-pinned pages never expire; unpinned (HEAD) pages use a 24h TTL (BENCH_HF_CACHE_TTL_MS).
  • GitHub task sources (harbor, terminal-bench) clone into stable <cache>/repos/<label>-<commit> dirs reused across runs and processes, instead of a fresh tmp dir per process.
  • BENCH_DATASET_CACHE_DISABLE=1 bypasses the cache entirely; cache write failures never fail a run.

Review hardening (from Devin review)

  • No test pollution of the real cache: under bun test the on-disk cache is disabled by default unless BENCH_DATASET_CACHE_DIR is set (or BENCH_DATASET_CACHE_DISABLE=0 opts in explicitly). Tests that stub fetch can no longer persist fake rows into — or read stale entries from — the developer's real cache. The terminal-bench network integration test opts in explicitly since it intentionally reuses the shared checkout.
  • Cross-process checkout races: task repos are now cloned into a private staging dir and atomically renamed into the shared path (publishStagedCheckout in src/datasets/local-cache.ts). Replacement of an existing shared dir is decided by a .bench-checkout-complete marker written only after a successful checkout (a plain statSync — no git spawn that could transiently fail and cause a false-negative deletion of an in-use checkout): a marked dir is always reused (or errors out asking for manual removal if its tasks are missing), a marker-less dir was never adopted by any process running this code and is safely replaced wholesale. Checkouts are also only reused on the fast path when the marker, the pinned commit, and the tasks dir all check out.
  • Token-scoped cache entries + owner-only permissions: HF rows pages fetched with HF_TOKEN are keyed by a hash of the token (under <cache>/hf/<token-scope>/...), so gated dataset contents are never served to callers without that token. Cache files are written 0o600, and <cache>/repos plus published task-repo checkouts have group/other permissions stripped (0o700 dirs, 0o600/0o700 files) before publishing.
  • No abandoned partial downloads: if a clone into a staging dir fails (network error, bad commit, aborted run), the staging dir is removed before the error propagates, so repeated failures can't fill the cache with partial copies.

Test plan

  • bun run format:check
  • bun run check (0 errors)
  • bun run typecheck (0 errors)
  • bun test — 1379 pass, 0 fail (incl. local-cache marker/permissions/publishStagedCheckout tests, huggingface-cache token-scoping, and harbor shared-checkout tests covering staging cleanup on clone failure, owner-only permissions, marker-gated reuse, and corrupt-leftover replacement)
  • bun run build
  • Verified the full test run leaves the real cache at ~/.cache/openrouter-bench-harness untouched (removed the pre-existing poisoned hf/ entries written by the old mmlu-pro test)

Open in Devin Review

@abhinav-pola
abhinav-pola requested a review from a team as a code owner August 19, 2026 21:39
devin-ai-integration[bot]

This comment was marked as resolved.

HF /rows pages are cached per (dataset, config, split, revision, offset)
under BENCH_DATASET_CACHE_DIR (default ~/.cache/openrouter-bench-harness);
revision-pinned pages never expire, unpinned (HEAD) pages use a 24h TTL
(BENCH_HF_CACHE_TTL_MS). GitHub task sources clone into stable
<cache>/repos/<label>-<commit> dirs reused across runs and processes
instead of a fresh tmp dir per process. BENCH_DATASET_CACHE_DISABLE=1
bypasses the cache entirely; cache write failures never fail a run.

Closes ECO-2484.
…leakage

- disable the on-disk cache by default under bun test unless
  BENCH_DATASET_CACHE_DIR is set, so stubbed fetch responses never
  leak into (or get served from) the developer's real cache
- clone task repos into a private staging dir and atomically rename it
  into the shared path, so concurrent runs never delete a checkout
  another process is cloning into or reading from; corrupt leftovers
  are replaced wholesale
- scope HF rows cache entries by a hash of the HF token so gated
  dataset contents are never served to callers without that token, and
  write cache files with owner-only permissions
@abhinav-pola abhinav-pola changed the title feat(datasets): cache HF rows pages and task repo checkouts on disk ECO-2484: feat(datasets): cache HF rows pages and task repo checkouts on disk Aug 19, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

…kouts

- key shared-checkout replacement on a completion marker written only
  after a successful checkout, so a transient git-probe failure can
  never cause a valid in-use checkout to be deleted; marked-but-incomplete
  shared dirs are never deleted (error asks for manual removal)
- remove the staging dir when a clone fails, so failed downloads no
  longer accumulate partial copies in the cache
- create <cache>/repos with mode 0o700 and strip group/other permissions
  from published checkouts before rename
@abhinav-pola

Copy link
Copy Markdown
Contributor Author

Addressed the remaining review findings in ed8c5a3:

  1. Shared checkout deleted while another run reads it — replacement is now decided by a .bench-checkout-complete marker written only after a successful git checkout (statSync-based, no git spawn whose transient failure/timeout could produce a false negative and delete a valid in-use checkout). A marked shared dir is always reused; a marked-but-taskless dir is never deleted (it errors asking for manual removal); only marker-less dirs — never adopted by any process running this code — are replaced. The reuse fast path also now requires the marker.
  2. Failed downloads leave staging dirs behindcloneInto(staging) is wrapped so the staging dir is removed before the error propagates (both harbor and terminal-bench sources), with a regression test.
  3. World-readable checkouts<cache>/repos is created 0o700 and staged checkouts have group/other permissions stripped (restrictPermissionsRecursive) before the atomic rename; regression tests assert dir/file modes.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

…ten perms

- an empty tasks-dir override (BENCH_TASKS_DIR / BENCH_<BENCH>_TASKS_DIR)
  is once again the clone target even when a shared cache checkout
  exists, restoring the pre-cache precedence for users who pin the
  checkout location
- sweep .staging- dirs older than 24h from <cache>/repos before each
  clone, so staging copies abandoned by killed processes no longer
  accumulate; publishStagedCheckout also cleans staging when the
  replacement path throws
- mkdirOwnerOnly chmods the target dir even when it pre-exists, so
  cache dirs created by older versions or loose operator-provided
  paths get tightened to 0700 instead of keeping inherited
  group/world-traversable permissions
@abhinav-pola

Copy link
Copy Markdown
Contributor Author

Addressed the latest review findings in ec8d328:

  1. Tasks-dir override precedence restored — when the env override (BENCH_TASKS_DIR / BENCH_<BENCH>_TASKS_DIR) points at an empty or missing directory, ensureTasksCheckedOut now skips the shared-cache reuse branch entirely so cloneTasks clones into the override, matching the pre-PR behavior. An explicit user-specified checkout location always wins (also fixes BENCH_TASKS_DIR consumers like swe-atlas). Regression test covers "empty override + valid shared checkout → clone into override".
  2. Stale staging dirs from killed runs — each clone now sweeps .staging-* entries older than 24h (sweepStaleStagingDirs) from <cache>/repos before creating a new staging dir, so copies abandoned by SIGKILL'd processes no longer accumulate; 24h is far beyond any live clone, so concurrent publishers are unaffected. publishStagedCheckout also no longer leaks staging when the replace path throws (the removeDirRecursive(shared) moved inside the guarded try). Unit tests cover sweep age-filtering plus a harbor-level interrupted-run scenario.
  3. Pre-existing loose cache dirsmkdirOwnerOnly walks the created chain and chmods the target dir and every newly created level to 0o700, even when the target already existed with looser permissions (mkdir's mode is ignored on existing dirs). Ancestors outside the paths we manage are deliberately left alone. Covers the JSON page cache path and <cache>/repos; tests cover nested creation, pre-existing targets, and ancestor non-interference.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

Comment thread src/datasets/huggingface.ts
…ges too

Gated dataset rows cached under a pinned revision previously never
expired, leaving restricted content on disk indefinitely with no
eviction path. An explicitly configured TTL now applies to pinned
entries as well, giving operators a data-retention lever; the default
semantics (pinned never expires, unpinned 24h) are unchanged.
@abhinav-pola

Copy link
Copy Markdown
Contributor Author

Addressed the residual retention concern in a76db84, plus design context:

Change: an explicitly set BENCH_HF_CACHE_TTL_MS now applies to all HF cache entries, including revision-pinned ones — so operators with data-retention requirements for gated content get an eviction lever (BENCH_HF_CACHE_TTL_MS=0 effectively disables the page cache). Default semantics are unchanged (pinned never expires since the content is immutable; unpinned uses the 24h default). README table updated; regression test added.

Design context on the residual risk (why long-lived caching of gated rows is intended here):

  • Caching gated dataset contents locally is the point of ECO-2484, and matches upstream precedent: huggingface_hub (Python) likewise caches gated repos on disk in ~/.cache/huggingface with no expiry.
  • The exposure surface is already minimized: entries are token-scoped (SHA-256 prefix, no cross-token reads), files are 0o600, the tree is 0o700, and nothing under the cache is ever committed to the repo (AGENTS.md's restricted-contents rule concerns the repo, which stays clean).
  • Operators who want zero persistence already have BENCH_DATASET_CACHE_DISABLE=1, and ephemeral/CI deployments can point BENCH_DATASET_CACHE_DIR at scratch storage; rm -rf of the cache dir is a full purge.

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