Skip to content

[server] only discard stale contexts that no live context depend on#12918

Open
kLabz wants to merge 6 commits into
developmentfrom
fix/stale_ctx_needed_by_live_ctx
Open

[server] only discard stale contexts that no live context depend on#12918
kLabz wants to merge 6 commits into
developmentfrom
fix/stale_ctx_needed_by_live_ctx

Conversation

@kLabz
Copy link
Copy Markdown
Contributor

@kLabz kLabz commented May 29, 2026

Since #12807, when a context is not used for 10+ minutes, it is discarded.

This led to some Could not find dependency X of Y in the cache compiler failures: in not so rare cases, the macro context is not accessed for 10+ minutes while the main context is. When this happens, next full request will try to pull some dependencies from macro context, that don't exist any more.

This PR:

  • adds a way to configure stale_context_max_age_seconds (mostly for tests)
  • keeps track of dependencies between contexts
    • Note: change add_context_dep to () in order to make the test fail with current development behavior
    • Also note that these dependencies are not cleared; which I think is fine, given that such dependencies are usually very stable (main <-> macro context), and avoid mistakes/overhead when trying to clear it
  • only drops a stale context if no live context depends on it; meaning a stale macro context will only be dropped if its "sibling" context is also stale

The actual way to track those dependencies and read them in remove_stale_contexts is open to improvements; I asked Claude to come up with this part

Closes #12874

@kLabz kLabz requested a review from Simn May 29, 2026 08:08
@Simn
Copy link
Copy Markdown
Member

Simn commented May 29, 2026

This makes sense to be conceptually. The term "sibling" alerts me a little because the macro context is closer to a child than a sibling. I think the "main" context (we still don't have a good word for that) should be considered the root, and then the macro and core contexts are its children. I'm not sure how exactly to represent that though.

@kLabz
Copy link
Copy Markdown
Contributor Author

kLabz commented May 29, 2026

Yeah, I've been thinking about that too; maybe we could add them to the "main" context (as some new concept of child contexts?) upon creation directly?

@kLabz kLabz linked an issue May 29, 2026 that may be closed by this pull request
@kLabz
Copy link
Copy Markdown
Contributor Author

kLabz commented May 29, 2026

I switched to a parent/child relationship between contexts, which seem much cleaner.

It's not included here, but I also ran a modified version with an assert in cache_module that made sure any module dependency to another context was to a child context (had to register macro context as child before checking the modules). All server tests passed without triggering that, so it seems this is not missing some edge case.

@RblSb
Copy link
Copy Markdown
Member

RblSb commented May 30, 2026

I think it doesn't crash for me too at #12846 (comment)
But class/field completion is much slower in this branch than in development for me.
Regression is not in first commit, but in second one: 76b59db
This is because staleContextMaxAge is 0 by default.

@kLabz
Copy link
Copy Markdown
Contributor Author

kLabz commented May 30, 2026

Right.. fixed that. Thanks!

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.

Compiler failure using -w in .hxml doesn't seem to matter which flag

3 participants