Skip to content

fix(exec,api): disabled spill means no spill; durable projects spill into capped project scratch (#1595) - #1609

Merged
DecisionNerd merged 4 commits into
mainfrom
fix/1595-spill-policy
Sep 28, 2026
Merged

DecisionNerd merged 4 commits into
mainfrom
fix/1595-spill-policy

Conversation

@DecisionNerd

@DecisionNerd DecisionNerd commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1595.

What changes

This implements the maintainer decision recorded on #1595.

  • Disabled means disabled. SpillPolicy { enabled: false } now disables DataFusion's disk manager. Before, the session left DataFusion's default in place, which spills into the OS temporary directory with no cap. Now a query over its memory budget fails with a resource error.
  • New default for durable projects. The default is enabled: true with no directory. Queries spill into <project>/.graphforge-query-spill/, capped at 8 GiB per query unless the caller sets max_bytes.
    • Each open instance owns one subdirectory, created on its first query and held by an exclusive lock on a sibling .lock file. Dropping the instance removes both.
    • Acquiring scratch reclaims entries whose lock is free, meaning the owner exited, and never touches a live instance's scratch.
  • In-memory instances have no project volume, so they don't spill.
  • A configured directory works as before.
  • Read-only views of a durable project (checkpoint and inspection views) spill into project scratch like any query.
  • Views that bound their own memory (branch, private checkpoint-materialization and slice views) set enabled: false and now fail closed instead of silently spilling.
  • EXPLAIN renders with a no-spill configuration and never acquires scratch.
  • An unusable spill directory fails that query with a storage error; session build is now fallible instead of panicking.
  • Search-index adjacency builds keep their own stage spill root unless a directory is configured, so the query cap doesn't bound them.

Docs: docs/development/execution-resource-policy.md gains a "Query spill" section.

Evidence

Tests, with each defect-targeting test proven by mutation:

  • graphforge-exec
  • graphforge-storage query_spill
    • An instance owns a locked subdirectory until it is dropped.
    • A live instance is never reclaimed. Mutating reclamation to ignore locks fails this test.
    • Abandoned scratch is reclaimed; unrelated names and symlinks are left alone.
    • A scratch root that is not a directory is refused.
  • graphforge-api runtime_ownership::spill_tests, checking what each policy and instance resolves to:
    • durable default: project scratch with the 8 GiB cap, acquired lazily and released on drop;
    • the caller's cap applies;
    • disabled: no directory, and no scratch created;
    • in-memory: no spill;
    • configured directory: used as given;
    • two instances of one project don't share scratch.

End to end on the release gf (SHA-256 eb9840cf…6b334d), with the 9M-leaf star (#1585, #1591) at default resources:

  • Query: MATCH (a)-[r]->(b) RETURN a.node_uuid AS s, b.node_uuid AS d ORDER BY s, d returned 9,000,000 rows, with peak RSS 830 MB.
  • Spill location: spill peaked at 290,461,408 bytes in 2 files, all under <project>/.graphforge-query-spill/, observed by polling during the query.
  • TMPDIR: pointed at an empty directory, it stayed empty.
  • After exit: the scratch directory is empty.
  • Other commands: gf verify and gf storage-attribution succeed with the scratch root present.

Suites and gates:

Suite / gate Result
cargo test -p graphforge-exec lib 903 passed, all integration targets passed
cargo test -p graphforge-storage --lib 1344 passed (1346 with the review fixes, plus the interleaving tests)
cargo test -p graphforge-api lib 820 passed, all 96 test targets passed
cargo test -p graphforge-cli all 20 targets passed
Node full node --test and checkpoints tests pass
Python CI's pytest selection passes (283); multi_ontology.py, checkpoints.py and concurrency_parity.py pass
cargo fmt --all -- --check, cargo clippy --workspace -- -D warnings clean
make pre-push-fast passes

Review and CI

An independent review found no defects in the policy semantics, and five issues, all verified and fixed in f45df116 and 812c1076:

  1. Lock race. The lock file was created, then locked. A concurrent reclaimer could lock and delete it in that window. CI's shared_directory_semantics_tests hit exactly this on the first head: "a new scratch lock is already held". Now the lock is created under a temporary name, locked, then renamed into place, so a visible <token>.lock is always held. A raced claim retries with a fresh token. Two tests drive each interleaving deterministically through a test-only hook, and both fail when the retry is removed.
  2. Read-only views. They were excluded from scratch; they now spill like any query of a durable project.
  3. Panic on disk error. Session build panicked on a runtime or disk error. It is now a GfError::Storage, with a test.
  4. Blocking reclamation. An unremovable stale entry blocked every query. Reclamation is now best effort, with a test.
  5. EXPLAIN and docs. EXPLAIN acquired scratch, and the docs misstated the configured-directory cap (DataFusion's 100 GiB default when max_bytes is unset). Both are fixed, EXPLAIN with a test.

Not in this change

  • Stale release-bundle fingerprint. Running all of tests/ with pytest (beyond CI's selection) shows tests/release_workflows/atomic-recovery failing: the generator.yaml fingerprint recorded in its scenario.yaml doesn't match the file. It is unchanged since chore: remove opaque mXX milestone shorthand #689 and unrelated to this change.
  • Small budgets cannot spill. With the minimum 16 MiB query budget, DataFusion cannot spill at all: it reserves 10 MiB of merge headroom up front. That is DataFusion behaviour, so the API-level tests check the resolved configuration, and the exec tests check spill behaviour at realistic budgets.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

DecisionNerd and others added 2 commits September 28, 2026 02:47
…into capped project scratch (#1595)

A disabled spill policy used to leave DataFusion's default disk manager
in place, which spills into the OS temporary directory with no cap, so
queries spilled when the policy said they must not. Now:

- spill disabled disables the disk manager: a query over its budget fails
  with a resource error;
- the default policy (enabled, no directory) spills a durable project's
  queries into <project>/.graphforge-query-spill/, one locked subdirectory
  per open instance, capped at 8 GiB per query unless the caller sets
  max_bytes; an in-memory instance does not spill;
- a configured directory works as before;
- scratch left by an exited process is reclaimed when an instance next
  acquires scratch; live instances' scratch is never touched;
- search-index adjacency builds keep their own stage spill root unless a
  directory is configured.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: CurateLabs/graphforge/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: a04b3f0f-9293-4c63-8763-1cd249edc685

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added executor Changes to query executor core Core source code changes documentation Improvements or additions to documentation labels Sep 28, 2026
@blacksmith-sh

This comment has been minimized.

DecisionNerd and others added 2 commits September 28, 2026 04:07
- A lock file is created under a temporary name, locked, then renamed
  into place, so a reclaimer never sees an unheld live lock; a claim a
  reclaimer raced is retried with a fresh token. A subdirectory counts as
  lockless only if its lock is absent when examined.
- Reclaiming other owners' leftovers is best effort: an unremovable entry
  is skipped and never blocks acquiring this instance's scratch.
- Building a spilling session is fallible: an unusable spill directory or
  runtime error is a storage error for that query, not a panic.
- Read-only views of a durable project (checkpoints, inspection) spill
  into project scratch like any query.
- EXPLAIN renders its plan with a no-spill configuration and never
  acquires scratch.
- Docs: DataFusion's 100 GiB default cap for a configured directory
  without max_bytes; the read-only and EXPLAIN rules.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
CI's shared-directory test hit the race the review described: a reader's
reclamation locked another reader's freshly created lock file. The claim
now publishes its lock only once held and retries a raced claim; two
tests drive each interleaving deterministically through a test-only
hook, and fail when the retry is removed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@DecisionNerd
DecisionNerd added this pull request to the merge queue Sep 28, 2026
Merged via the queue into main with commit 2ef9213 Sep 28, 2026
23 checks passed
@DecisionNerd
DecisionNerd deleted the fix/1595-spill-policy branch September 28, 2026 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core source code changes documentation Improvements or additions to documentation executor Changes to query executor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(exec): a disabled spill policy does not stop DataFusion spilling to the OS temp directory

1 participant