[server] only discard stale contexts that no live context depend on#12918
Conversation
|
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. |
|
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? |
|
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 |
|
I think it doesn't crash for me too at #12846 (comment) |
|
Right.. fixed that. Thanks! |
|
@Simn anything against merging this? |
|
I don't like the magic 600 appearing in two places, but other than that this looks good. |
|
Yeah.. but they're also 5 lines apart in a simple define/reset pattern. |
|
I acknowledge that it seems pedantic, but it's the kind of thing that tends to cause surprises down the road. So yes let's make an effort to only have magic numbers in singular places! |
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 cachecompiler 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:
stale_context_max_age_seconds(mostly for tests)add_context_depto()in order to make the test fail with current development behaviorThe actual way to track those dependencies and read them in
remove_stale_contextsis open to improvements; I asked Claude to come up with this partCloses #12874