Skip to content

[ty] Fix untracked reads in Salsa queries that can lead to backdating panics#24051

Merged
MichaReiser merged 1 commit intomainfrom
micha/backdate-panic
Mar 20, 2026
Merged

[ty] Fix untracked reads in Salsa queries that can lead to backdating panics#24051
MichaReiser merged 1 commit intomainfrom
micha/backdate-panic

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Mar 19, 2026

Summary

This PR fixes two cases that could lead to the panics reported in astral-sh/ty#1565.

After a change to an input (e.g., a file was modified), Salsa re-executes all queries that read that input or depend on any such query. To limit invalidation, Salsa can backdate a query. If a query computes to the same value as in the previous revision, Salsa considers it unchanged and skips re-executing dependent queries. This is called backdating a query. The reason it's called backdating is that Salsa tracks the revision in which a query was last changed; backdating sets the last-changed revision "back" to the revision from before the input change. During backdating, Salsa enforces that the revision is indeed going backwards. That is, the revision Salsa backdates to (or the last changed revision from the previous query run) must be smaller (older) than the last changed revision from the current run.

Under normal circumstances, this invariant should always hold because for a query result to change, the query has to read some input that was changed in the current revision (at least in a revision newer than the last run). This ensures that the last changed revision from the new run is never older than that from the previous run. However, this invariant can be violated if a query branches on a state that isn't tracked in Salsa. Salsa will rerun the query because one of its inputs changed, but the query branches on some untracked state before reading that input, so that the query's last changed revision might be earlier than when the input changed.

Reading an untracked state in a query is always a bug (unless you call db.untracked_read), and it can lead to stale results. But I don't think it's an error severe enough to warrant Salsa panicking. I plan to PR a change to Salsa to panic only in debug builds and otherwise log a warning, notifying users about the untracked read.

The change in this PR is to fix two cases where we ended up with reading untracked global state or failed to correctly invalidate all query dependencies:

  • Project is a Salsa input, but db.project isn't a Salsa tracked field. Therefore, it's important that we never reassign db.project during a DB's lifetime. This PR fixes the one place where we did replace db.project with a new Project
  • There were a few instances where we didn't call File::sync where we should. This can lead to stale results and potentially panic within absolute_desperate_search_paths because the query branches on whether something is a file.

Test Plan

I wrote three integration tests with Codex that all panicked before making the above changes. I decided not to commit them because they all feel a bit artificial and I didn't manage to come up with more real-world tests that reproduce the panic. All tests now pass without panicking.

Tests
use ruff_db::files::{File, system_path_to_file};
use ruff_db::system::{SystemPath, SystemPathBuf, TestSystem};
use ty_module_resolver::UseDefaultStrategy;
use ty_module_resolver::{ModuleName, resolve_module};
use ty_project::metadata::options::{AnalysisOptions, Options};
use ty_project::watch::{ChangeEvent, CreatedKind};
use ty_project::{Db as _, ProjectDatabase, ProjectMetadata};

#[salsa::tracked]
fn dep_a(db: &dyn ty_project::Db, file: File) -> u32 {
    file.read_to_string(db)
        .ok()
        .and_then(|contents| contents.trim().parse().ok())
        .unwrap_or(0)
}

#[salsa::tracked]
fn dep_b(db: &dyn ty_project::Db, file: File) -> u32 {
    file.read_to_string(db)
        .ok()
        .and_then(|contents| contents.trim().parse().ok())
        .unwrap_or(0)
}

#[salsa::tracked]
fn branch_on_project_identity(
    db: &dyn ty_project::Db,
    expected_project: ty_project::Project,
    file_a: File,
    file_b: File,
) -> u32 {
    if db.project() == expected_project {
        dep_a(db, file_a)
    } else {
        dep_b(db, file_b)
    }
}

/// Minimal model of `absolute_desperate_search_paths`' invalidation strategy.
///
/// The real resolver query:
/// - tracks only the enclosing project root revision
/// - then branches on `system.is_file(.../pyproject.toml)`
///
/// Calling the real tracked query here would only reproduce the stale-cache bug:
/// after creating `pyproject.toml`, the query would keep returning its old memo and
/// the outer query's branch would not change. This distilled query keeps the same
/// dependency shape while allowing the branch flip to remain visible, which is what
/// exposes the backdating assertion.
#[salsa::tracked]
fn branch_like_absolute_desperate_search_paths_without_root_invalidation(
    db: &dyn ty_project::Db,
    importing_file: File,
    file_a: File,
    file_b: File,
) -> u32 {
    let importing_path = importing_file.path(db).as_system_path().unwrap();
    let package_path = importing_path.parent().unwrap().parent().unwrap();
    let pyproject_path = package_path.join("pyproject.toml");

    let root = db.files().root(db, package_path).unwrap();
    let _ = root.revision(db);

    if db.system().is_file(&pyproject_path) {
        dep_a(db, file_a)
    } else {
        dep_b(db, file_b)
    }
}

#[test]
#[should_panic(expected = "old_memo.revisions.changed_at <= revisions.changed_at")]
fn project_reload_via_apply_changes_synthetic_write_does_not_avoid_backdating_panic() {
    let system = TestSystem::default();

    let a_path = SystemPath::new("/project/subpkg/a.txt");
    let b_path = SystemPath::new("/project/subpkg/b.txt");

    system
        .memory_file_system()
        .create_directory_all(SystemPath::new("/project/subpkg"))
        .unwrap();
    system.memory_file_system().write_file(a_path, "0").unwrap();
    system.memory_file_system().write_file(b_path, "0").unwrap();

    let metadata = ProjectMetadata::from_options(
        Options {
            analysis: Some(AnalysisOptions {
                respect_type_ignore_comments: Some(true),
                ..AnalysisOptions::default()
            }),
            ..Options::default()
        },
        SystemPathBuf::from("/project/subpkg"),
        None,
        &UseDefaultStrategy,
    )
    .unwrap();

    let mut db = ProjectDatabase::use_defaults(metadata, system.clone());

    let a_file = system_path_to_file(&db, a_path).expect("a.txt to exist");
    let b_file = system_path_to_file(&db, b_path).expect("b.txt to exist");

    let initial_project = db.project();

    assert_eq!(
        branch_on_project_identity(&db, initial_project, a_file, b_file),
        0
    );

    system
        .memory_file_system()
        .write_file(a_path, "1")
        .expect("update a.txt");

    db.apply_changes(
        vec![ChangeEvent::file_content_changed(a_path.to_path_buf())],
        None,
    );
    assert_eq!(
        branch_on_project_identity(&db, initial_project, a_file, b_file),
        1
    );

    system
        .memory_file_system()
        .write_file(a_path, "0")
        .expect("update a.txt");
    db.apply_changes(
        vec![ChangeEvent::file_content_changed(a_path.to_path_buf())],
        None,
    );
    assert_eq!(
        branch_on_project_identity(&db, initial_project, a_file, b_file),
        0
    );

    // Trigger a change that forces the project to reload.
    system
        .memory_file_system()
        .write_file(
            SystemPath::new("/project/ty.toml"),
            "[analysis]\nrespect-type-ignore-comments = false\n",
        )
        .expect("write ty.toml");
    db.apply_changes(
        vec![ChangeEvent::Created {
            path: SystemPathBuf::from("/project/ty.toml"),
            kind: CreatedKind::File,
        }],
        None,
    );

    // assert_ne!(db.project(), initial_project);

    system
        .memory_file_system()
        .write_file(a_path, "1")
        .expect("update a.txt");
    db.apply_changes(
        vec![ChangeEvent::file_content_changed(a_path.to_path_buf())],
        None,
    );

    let _ = branch_on_project_identity(&db, initial_project, a_file, b_file);
}

#[test]
fn nested_pyproject_creation_invalidates_desperate_resolution() {
    let system = TestSystem::default();

    let importing_path = SystemPath::new("/project/subpkg/tests/test.py");
    let nested_package_init = SystemPath::new("/project/subpkg/__init__.py");
    let nested_module_path = SystemPath::new("/project/subpkg/foo.py");

    system
        .memory_file_system()
        .create_directory_all(SystemPath::new("/project/subpkg/tests"))
        .unwrap();
    system
        .memory_file_system()
        .write_file(importing_path, "")
        .unwrap();
    system
        .memory_file_system()
        .write_file(nested_package_init, "")
        .unwrap();
    system
        .memory_file_system()
        .write_file(nested_module_path, "x = 1")
        .unwrap();

    let metadata = ProjectMetadata::new("project".into(), SystemPathBuf::from("/project"));
    let mut db = ProjectDatabase::use_defaults(metadata, system.clone());

    let importing_file = system_path_to_file(&db, importing_path).expect("test.py to exist");
    let nested_module_file = system_path_to_file(&db, nested_module_path).expect("foo.py to exist");
    let foo = ModuleName::new_static("foo").unwrap();

    assert_eq!(resolve_module(&db, importing_file, &foo), None);

    system
        .memory_file_system()
        .write_file(
            SystemPath::new("/project/subpkg/pyproject.toml"),
            "[tool.ty]\n",
        )
        .unwrap();
    db.apply_changes(
        vec![ChangeEvent::Created {
            path: SystemPathBuf::from("/project/subpkg/pyproject.toml"),
            kind: CreatedKind::File,
        }],
        None,
    );

    assert_eq!(
        resolve_module(&db, importing_file, &foo).map(|module| module.file(&db).unwrap()),
        Some(nested_module_file),
        "nested pyproject.toml should invalidate desperate resolution",
    );
}

#[test]
fn project_root_invalidation_prevents_backdate_assertion() {
    let system = TestSystem::default();

    let importing_path = SystemPath::new("/project/subpkg/tests/test.py");
    let a_path = SystemPath::new("/project/subpkg/a.txt");
    let b_path = SystemPath::new("/project/subpkg/b.txt");

    system
        .memory_file_system()
        .create_directory_all(SystemPath::new("/project/subpkg/tests"))
        .unwrap();
    system
        .memory_file_system()
        .write_file(importing_path, "")
        .unwrap();
    system.memory_file_system().write_file(a_path, "0").unwrap();
    system.memory_file_system().write_file(b_path, "0").unwrap();

    let metadata = ProjectMetadata::new("project".into(), SystemPathBuf::from("/project"));
    let mut db = ProjectDatabase::use_defaults(metadata, system.clone());

    let importing_file = system_path_to_file(&db, importing_path).expect("test.py to exist");
    let a_file = system_path_to_file(&db, a_path).expect("a.txt to exist");
    let b_file = system_path_to_file(&db, b_path).expect("b.txt to exist");

    assert_eq!(
        branch_like_absolute_desperate_search_paths_without_root_invalidation(
            &db,
            importing_file,
            a_file,
            b_file,
        ),
        0
    );

    system.memory_file_system().write_file(b_path, "1").unwrap();
    db.apply_changes(
        vec![ChangeEvent::file_content_changed(b_path.to_path_buf())],
        None,
    );
    assert_eq!(
        branch_like_absolute_desperate_search_paths_without_root_invalidation(
            &db,
            importing_file,
            a_file,
            b_file,
        ),
        1
    );

    system.memory_file_system().write_file(b_path, "0").unwrap();
    db.apply_changes(
        vec![ChangeEvent::file_content_changed(b_path.to_path_buf())],
        None,
    );
    assert_eq!(
        branch_like_absolute_desperate_search_paths_without_root_invalidation(
            &db,
            importing_file,
            a_file,
            b_file,
        ),
        0
    );

    system
        .memory_file_system()
        .write_file(
            SystemPath::new("/project/subpkg/pyproject.toml"),
            "[tool.ty]\n",
        )
        .unwrap();
    db.apply_changes(
        vec![ChangeEvent::Created {
            path: SystemPathBuf::from("/project/subpkg/pyproject.toml"),
            kind: CreatedKind::File,
        }],
        None,
    );

    system.memory_file_system().write_file(b_path, "1").unwrap();
    db.apply_changes(
        vec![ChangeEvent::file_content_changed(b_path.to_path_buf())],
        None,
    );

    assert_eq!(
        branch_like_absolute_desperate_search_paths_without_root_invalidation(
            &db,
            importing_file,
            a_file,
            b_file,
        ),
        0
    );
}

@MichaReiser MichaReiser added bug Something isn't working ty Multi-file analysis & type inference labels Mar 19, 2026
@astral-sh-bot
Copy link

astral-sh-bot bot commented Mar 19, 2026

Typing conformance results

No changes detected ✅

Current numbers
The percentage of diagnostics emitted that were expected errors held steady at 85.29%. The percentage of expected errors that received a diagnostic held steady at 78.13%. The number of fully passing files held steady at 64/132.

@astral-sh-bot
Copy link

astral-sh-bot bot commented Mar 19, 2026

Memory usage report

Memory usage unchanged ✅

@astral-sh-bot
Copy link

astral-sh-bot bot commented Mar 19, 2026

ecosystem-analyzer results

Lint rule Added Removed Changed
invalid-await 40 0 0
invalid-return-type 1 0 0
Total 41 0 0

Changes in flaky projects detected. Raw diff output excludes flaky projects; see the HTML report for details.

Full report with detailed diff (timing results)


for change in changes {
tracing::trace!("Handle change: {:?}", change);
tracing::debug!("Handling file watcher change event: {:?}", change);
Copy link
Member Author

@MichaReiser MichaReiser Mar 19, 2026

Choose a reason for hiding this comment

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

I was torn on whether promoting this to debug is a good idea, but debugging production file watcher issues is sort of impossible without knowing what "happened". But it has the downside of being somewhat verbose.

@MichaReiser MichaReiser marked this pull request as ready for review March 19, 2026 17:20
@astral-sh-bot astral-sh-bot bot requested a review from BurntSushi March 19, 2026 17:20
Copy link
Member

@ibraheemdev ibraheemdev left a comment

Choose a reason for hiding this comment

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

Great job tracking this down!

@MichaReiser MichaReiser merged commit 1dfbcf5 into main Mar 20, 2026
49 checks passed
@MichaReiser MichaReiser deleted the micha/backdate-panic branch March 20, 2026 07:25
carljm added a commit that referenced this pull request Mar 25, 2026
* main:
  [`flake8-bandit`] Check tuple arguments for partial paths in `S607` (#24080)
  [ty] Update Salsa (#24081)
  Update Rust toolchain to 1.94 and MSRV to 1.92 (#24076)
  [ty] Move ruffen-docs formatting config to a `ruff.toml` config file (#24074)
  [ty] `reveal_type` diagnostics in unreachable code (#24070)
  [ty] Improve keyword argument narrowing for nested dictionaries (#24010)
  [ty] Preserve blank lines between comments and imports in add-import action (#24066)
  [ty] Add diagnostic hint for invalid assignments involving invariant generics (#24032)
  Clarify `extend-ignore` and `extend-select` settings documentation (#24064)
  [ty] Batch changes to watched paths (#24045)
  replace deprecated `std::f64::EPSILON` with `f64::EPSILON` (#24067)
  [ty] Fix untracked reads in Salsa queries that can lead to backdating panics (#24051)
  [ty] Unions/intersections of gradual types should be assignable to `Never` (#24056)
  Fix incorrect path for ty_python_semantic in fuzzer (#24052)
  Bump 0.15.7 (#24049)
  [ty] ecosystem-analyzer: Fail on newly panicking projects (#24043)
  Don't show noqa hover for non-Python documents (#24040)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants