Skip to content

turbo-tasks-gc: Invalidating a deleted task should be a no-op - #98614

Draft
lukesandberg wants to merge 1 commit into
gc-08-gc-fixesfrom
gc-09-weak-access
Draft

turbo-tasks-gc: Invalidating a deleted task should be a no-op#98614
lukesandberg wants to merge 1 commit into
gc-08-gc-fixesfrom
gc-09-weak-access

Conversation

@lukesandberg

@lukesandberg lukesandberg commented Sep 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Make invalidating deleted tasks a no-op.

Invalidators now represent a 'weak' reference to a task, and invalidations don't assert task existence. We know that the task exists when the invalidator is created but by the time an invalidation occurs it might not exist anymore. So treat the dependency as weak.

Take care to not create 'blank' tasks when querying for them, this can confuse assertions that occur later.

@lukesandberg
lukesandberg added this pull request to stack #98592 September 13, 2026 17:21
@github-actions

github-actions Bot commented Sep 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Failing test suites

Commit: 4e3ed32 | About building and testing Next.js

pnpm test-dev-turbo test/development/app-dir/instant-navs-devtools/instant-navs-devtools.test.ts (turbopack) (job)

  • instant-nav-panel > SPA captures > should capture when CookieStore writes are not reflected in document.cookie (DD)
Expand output

● instant-nav-panel › SPA captures › should capture when CookieStore writes are not reflected in document.cookie

locator.waitFor: Timeout 60000ms exceeded.
Call log:
  - waiting for locator('[data-testid="dynamic-skeleton"]') to be visible

  278 |     await browser
  279 |       .locator('[data-testid="dynamic-skeleton"]')
> 280 |       .waitFor({ state: 'visible' })
      |        ^
  281 |     await browser
  282 |       .locator('[data-testid="param-skeleton"]')
  283 |       .waitFor({ state: 'visible' })

  at waitFor (development/app-dir/instant-navs-devtools/instant-navs-devtools.test.ts:280:8)
  at Object.expectTargetPageSpaShell (development/app-dir/instant-navs-devtools/instant-navs-devtools.test.ts:607:13)

lukesandberg
lukesandberg commented Sep 13, 2026
View reviewed changes
Comment thread turbopack/crates/turbo-tasks-backend/src/backend/operation/invalidate.rs
lukesandberg
lukesandberg commented Sep 13, 2026
View reviewed changes
Comment thread turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs Outdated
Comment on lines +132 to +141
/// Opens a task the caller holds only a **weak** reference to, returning `None` if it is gone.
///
/// "Gone" covers both a task that exists in neither memory nor persistent storage and one that
/// is soft-deleted but still resident: a weak holder must not distinguish them, since only the
/// timing of the next eviction separates the two.
///
/// Use this where a stale reference is *expected* rather than a bug — notably invalidators,
/// which a `State` keeps by raw [`TaskId`] and which GC does not scrub, so the reader they name
/// may have been collected long before the state is mutated. Everywhere else, prefer
/// [`Self::task`]: silently skipping a task that was supposed to exist hides graph corruption.

@lukesandberg lukesandberg Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

simplify comment

lukesandberg
lukesandberg commented Sep 13, 2026
View reviewed changes
Comment thread turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs Outdated
// Soft-deleted but still resident: GC has already unlinked this task and eviction will drop
// it outright. Report it as gone so callers see one state rather than a window that depends
// on snapshot timing — and so they cannot revive a tombstone by writing to it.
if task.task.flags.deleted() {

@lukesandberg lukesandberg Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this condition should be part of open_task, it should not attempt to restore deleted tasks

lukesandberg
lukesandberg commented Sep 13, 2026
View reviewed changes
Comment thread turbopack/crates/turbo-tasks-backend/src/backend/storage.rs Outdated
let mut roots = Vec::new();
self.for_each_resident_persistent_in_shard(index, |task_id, storage| {
if storage.gc_is_root() {
if !task_id.is_transient() && storage.gc_is_root() {

@lukesandberg lukesandberg Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

is this diff in the wrong PR?

@lukesandberg lukesandberg changed the title turbo-tasks-backend: treat stale task references as weak turbo-tasks-gc: Invalidating a deleted task should be a no-op Sep 13, 2026
vercel[bot]
vercel Bot reviewed Sep 13, 2026
View reviewed changes
Comment thread turbopack/crates/turbo-tasks-backend/src/backend/operation/mod.rs Outdated
@lukesandberg @claude
turbo-tasks-backend: treat stale task references as weak
4e3ed32 
Some references to a task outlive the task. A `State` keeps its readers by raw
`TaskId` and GC does not scrub those lists, so mutating a state can try to
invalidate a reader that was collected long before. Today that panics: the open
is `MustExist`, which is right for a graph edge but wrong for a reference the
holder was never promised would stay valid.

Add a third access mode. `TaskAccess::Weak` / `ExecuteContext::try_get_task`
return `None` both when a task exists nowhere and when it is soft-deleted --- a
weak holder cannot tell those apart, since only the timing of the next eviction
separates them. The soft-delete check happens before the restore attempt, so a
weak open never pays disk I/O to rehydrate a tombstone.

Only invalidator-driven invalidation is weak. `make_task_dirty` keeps its
`MustExist` open for invalidation that originates inside the graph --- task
completion, cell updates, a collectibles change --- where the id came from an
edge that is supposed to still be valid and a miss means corruption. The
`InvalidateOperation::MakeDirty` path, which is what `Invalidator::invalidate`
drives, uses the new weak variant.

Removing the entry that a weak open had to materialize needs care.
`Storage::access_mut` discards the `OccupiedEntry` that `entry()` produced, and
a plain `RefMut` holds only `&mut V` --- the hashtable slot is unreachable
through it, so removal would cost a second write lock on the same shard. Add
`access_entry_mut`, which keeps the entry, and `TaskEntryGuard::discard`, which
removes it under the lock already held. `StorageWriteGuard` cannot simply hold
an entry instead: `access_pair_mut` builds guards from `get_disjoint_mut`, which
has none to give.

Left behind, that entry is not inert. It is indistinguishable from a live task
that has not been restored yet, so GC treats it as collectible and deletes it,
producing a `Delete` snapshot item for a task with no persistent task type.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vercel vercel[bot] vercel[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lukesandberg