Allow DBDagBag TTL cache eviction without a size cap

[aeroyorch]

---

Allow DBDagBag TTL cache eviction without a size cap (i.e. cache_size=0, cache_ttl>0).

Related to #69001 and #69007

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

Claude Opus 4.8 to analyze the impact of this change in terms of docs/code.

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

[SameerMesiah97]

Just one nit. Looks good otherwise.

[aeroyorch]

Just one nit. Looks good otherwise.

Thanks for the review! Changes already implemented

[lohyenshen]

hi, may I know what airflow version would this be released to?

[vatsrahul1001]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

[aeroyorch]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

Thanks for the review. Changes already implemented :)

[vatsrahul1001]

LGTM!, can be merged after code owners review

[vatsrahul1001]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

[aeroyorch]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

No problem. In the meantime, I can work on some of the other items to fix #69001

[ColtenOuO]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

[aeroyorch]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

Thanks for the review!

Fair point, but I'd rather leave it as is for now, the trade-off you mention is the deciding one for me: keeping it at the call site lets us name the actual config option that's wrong, which is what the user needs to fix.

Happy to revisit when the Scheduler part lands for #69001.

[pierrejeambrun]

Do you mind giving more context as to why we are doing this. If your use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbounded cache there.

[aeroyorch]

Do you mind giving more context as to why we are doing this. If you use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbouded cache there.

Hi Pierre, sure.

I don't think this brings back the leak. Today cache_size=0 is a plain dict with no eviction at all. With this change, cache_size=0 + cache_ttl>0 creates a real TTLCache. That evicts exactly what grows and keeps the hot ones. Defaults don't change.

Tuning the size doesn't really help here: the scheduler cycles through all active dag_version_ids, so any cap below the active set evicts each key just before it's needed again. @kaxil raised this in the #69007 review and @potiuk asked for TTL eviction without a size cap in DBDagBag as a first step.

[pierrejeambrun]

Thanks for the context @aeroyorch this makes more sense. I had the wrong assumption that unbounded cache was just not possible anymore. (at all)

I would be for not allowing unbounded cache completely since we know it's a bad practice, but for backward comp I understand why unbounded is still allowed.

Concern answered.

[uranusjr]

This is superceded by #71704

[aeroyorch]

---

Allow DBDagBag TTL cache eviction without a size cap (i.e. cache_size=0, cache_ttl>0).

Related to #69001 and #69007

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

Claude Opus 4.8 to analyze the impact of this change in terms of docs/code.

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

[SameerMesiah97]

Just one nit. Looks good otherwise.

[aeroyorch]

Just one nit. Looks good otherwise.

Thanks for the review! Changes already implemented

[lohyenshen]

hi, may I know what airflow version would this be released to?

[vatsrahul1001]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

[aeroyorch]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

Thanks for the review. Changes already implemented :)

[vatsrahul1001]

LGTM!, can be merged after code owners review

[vatsrahul1001]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

[aeroyorch]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

No problem. In the meantime, I can work on some of the other items to fix #69001

[ColtenOuO]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

[aeroyorch]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

Thanks for the review!

Fair point, but I'd rather leave it as is for now, the trade-off you mention is the deciding one for me: keeping it at the call site lets us name the actual config option that's wrong, which is what the user needs to fix.

Happy to revisit when the Scheduler part lands for #69001.

[pierrejeambrun]

Do you mind giving more context as to why we are doing this. If your use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbounded cache there.

[aeroyorch]

Do you mind giving more context as to why we are doing this. If you use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbouded cache there.

Hi Pierre, sure.

I don't think this brings back the leak. Today cache_size=0 is a plain dict with no eviction at all. With this change, cache_size=0 + cache_ttl>0 creates a real TTLCache. That evicts exactly what grows and keeps the hot ones. Defaults don't change.

Tuning the size doesn't really help here: the scheduler cycles through all active dag_version_ids, so any cap below the active set evicts each key just before it's needed again. @kaxil raised this in the #69007 review and @potiuk asked for TTL eviction without a size cap in DBDagBag as a first step.

[pierrejeambrun]

Thanks for the context @aeroyorch this makes more sense. I had the wrong assumption that unbounded cache was just not possible anymore. (at all)

I would be for not allowing unbounded cache completely since we know it's a bad practice, but for backward comp I understand why unbounded is still allowed.

Concern answered.

[uranusjr]

This is superceded by #71704

[aeroyorch]

---

Allow DBDagBag TTL cache eviction without a size cap (i.e. cache_size=0, cache_ttl>0).

Related to #69001 and #69007

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

Claude Opus 4.8 to analyze the impact of this change in terms of docs/code.

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

[SameerMesiah97]

Just one nit. Looks good otherwise.

[aeroyorch]

Just one nit. Looks good otherwise.

Thanks for the review! Changes already implemented

[lohyenshen]

hi, may I know what airflow version would this be released to?

[vatsrahul1001]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

[aeroyorch]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

Thanks for the review. Changes already implemented :)

[vatsrahul1001]

LGTM!, can be merged after code owners review

[vatsrahul1001]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

[aeroyorch]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

No problem. In the meantime, I can work on some of the other items to fix #69001

[ColtenOuO]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

[aeroyorch]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

Thanks for the review!

Fair point, but I'd rather leave it as is for now, the trade-off you mention is the deciding one for me: keeping it at the call site lets us name the actual config option that's wrong, which is what the user needs to fix.

Happy to revisit when the Scheduler part lands for #69001.

[pierrejeambrun]

Do you mind giving more context as to why we are doing this. If your use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbounded cache there.

[aeroyorch]

Do you mind giving more context as to why we are doing this. If you use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbouded cache there.

Hi Pierre, sure.

I don't think this brings back the leak. Today cache_size=0 is a plain dict with no eviction at all. With this change, cache_size=0 + cache_ttl>0 creates a real TTLCache. That evicts exactly what grows and keeps the hot ones. Defaults don't change.

Tuning the size doesn't really help here: the scheduler cycles through all active dag_version_ids, so any cap below the active set evicts each key just before it's needed again. @kaxil raised this in the #69007 review and @potiuk asked for TTL eviction without a size cap in DBDagBag as a first step.

[pierrejeambrun]

Thanks for the context @aeroyorch this makes more sense. I had the wrong assumption that unbounded cache was just not possible anymore. (at all)

I would be for not allowing unbounded cache completely since we know it's a bad practice, but for backward comp I understand why unbounded is still allowed.

Concern answered.

[uranusjr]

This is superceded by #71704

[aeroyorch]

---

Allow DBDagBag TTL cache eviction without a size cap (i.e. cache_size=0, cache_ttl>0).

Related to #69001 and #69007

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

Claude Opus 4.8 to analyze the impact of this change in terms of docs/code.

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

[SameerMesiah97]

Just one nit. Looks good otherwise.

[aeroyorch]

Just one nit. Looks good otherwise.

Thanks for the review! Changes already implemented

[lohyenshen]

hi, may I know what airflow version would this be released to?

[vatsrahul1001]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

[aeroyorch]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

Thanks for the review. Changes already implemented :)

[vatsrahul1001]

LGTM!, can be merged after code owners review

[vatsrahul1001]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

[aeroyorch]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

No problem. In the meantime, I can work on some of the other items to fix #69001

[ColtenOuO]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

[aeroyorch]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

Thanks for the review!

Fair point, but I'd rather leave it as is for now, the trade-off you mention is the deciding one for me: keeping it at the call site lets us name the actual config option that's wrong, which is what the user needs to fix.

Happy to revisit when the Scheduler part lands for #69001.

[pierrejeambrun]

Do you mind giving more context as to why we are doing this. If your use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbounded cache there.

[aeroyorch]

Do you mind giving more context as to why we are doing this. If you use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbouded cache there.

Hi Pierre, sure.

I don't think this brings back the leak. Today cache_size=0 is a plain dict with no eviction at all. With this change, cache_size=0 + cache_ttl>0 creates a real TTLCache. That evicts exactly what grows and keeps the hot ones. Defaults don't change.

Tuning the size doesn't really help here: the scheduler cycles through all active dag_version_ids, so any cap below the active set evicts each key just before it's needed again. @kaxil raised this in the #69007 review and @potiuk asked for TTL eviction without a size cap in DBDagBag as a first step.

[pierrejeambrun]

Thanks for the context @aeroyorch this makes more sense. I had the wrong assumption that unbounded cache was just not possible anymore. (at all)

I would be for not allowing unbounded cache completely since we know it's a bad practice, but for backward comp I understand why unbounded is still allowed.

Concern answered.

[uranusjr]

This is superceded by #71704

[aeroyorch]

---

Allow DBDagBag TTL cache eviction without a size cap (i.e. cache_size=0, cache_ttl>0).

Related to #69001 and #69007

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

Claude Opus 4.8 to analyze the impact of this change in terms of docs/code.

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

[SameerMesiah97]

Just one nit. Looks good otherwise.

[aeroyorch]

Just one nit. Looks good otherwise.

Thanks for the review! Changes already implemented

[lohyenshen]

hi, may I know what airflow version would this be released to?

[vatsrahul1001]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

[aeroyorch]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

Thanks for the review. Changes already implemented :)

[vatsrahul1001]

LGTM!, can be merged after code owners review

[vatsrahul1001]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

[aeroyorch]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

No problem. In the meantime, I can work on some of the other items to fix #69001

[ColtenOuO]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

[aeroyorch]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

Thanks for the review!

Fair point, but I'd rather leave it as is for now, the trade-off you mention is the deciding one for me: keeping it at the call site lets us name the actual config option that's wrong, which is what the user needs to fix.

Happy to revisit when the Scheduler part lands for #69001.

[pierrejeambrun]

Do you mind giving more context as to why we are doing this. If your use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbounded cache there.

[aeroyorch]

Do you mind giving more context as to why we are doing this. If you use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbouded cache there.

Hi Pierre, sure.

I don't think this brings back the leak. Today cache_size=0 is a plain dict with no eviction at all. With this change, cache_size=0 + cache_ttl>0 creates a real TTLCache. That evicts exactly what grows and keeps the hot ones. Defaults don't change.

Tuning the size doesn't really help here: the scheduler cycles through all active dag_version_ids, so any cap below the active set evicts each key just before it's needed again. @kaxil raised this in the #69007 review and @potiuk asked for TTL eviction without a size cap in DBDagBag as a first step.

[pierrejeambrun]

Thanks for the context @aeroyorch this makes more sense. I had the wrong assumption that unbounded cache was just not possible anymore. (at all)

I would be for not allowing unbounded cache completely since we know it's a bad practice, but for backward comp I understand why unbounded is still allowed.

Concern answered.

[uranusjr]

This is superceded by #71704

[aeroyorch]

---

Allow DBDagBag TTL cache eviction without a size cap (i.e. cache_size=0, cache_ttl>0).

Related to #69001 and #69007

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

Claude Opus 4.8 to analyze the impact of this change in terms of docs/code.

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

[SameerMesiah97]

Just one nit. Looks good otherwise.

[aeroyorch]

Just one nit. Looks good otherwise.

Thanks for the review! Changes already implemented

[lohyenshen]

hi, may I know what airflow version would this be released to?

[vatsrahul1001]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

[aeroyorch]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

Thanks for the review. Changes already implemented :)

[vatsrahul1001]

LGTM!, can be merged after code owners review

[vatsrahul1001]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

[aeroyorch]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

No problem. In the meantime, I can work on some of the other items to fix #69001

[ColtenOuO]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

[aeroyorch]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

Thanks for the review!

Fair point, but I'd rather leave it as is for now, the trade-off you mention is the deciding one for me: keeping it at the call site lets us name the actual config option that's wrong, which is what the user needs to fix.

Happy to revisit when the Scheduler part lands for #69001.

[pierrejeambrun]

Do you mind giving more context as to why we are doing this. If your use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbounded cache there.

[aeroyorch]

Do you mind giving more context as to why we are doing this. If you use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbouded cache there.

Hi Pierre, sure.

I don't think this brings back the leak. Today cache_size=0 is a plain dict with no eviction at all. With this change, cache_size=0 + cache_ttl>0 creates a real TTLCache. That evicts exactly what grows and keeps the hot ones. Defaults don't change.

Tuning the size doesn't really help here: the scheduler cycles through all active dag_version_ids, so any cap below the active set evicts each key just before it's needed again. @kaxil raised this in the #69007 review and @potiuk asked for TTL eviction without a size cap in DBDagBag as a first step.

[pierrejeambrun]

Thanks for the context @aeroyorch this makes more sense. I had the wrong assumption that unbounded cache was just not possible anymore. (at all)

I would be for not allowing unbounded cache completely since we know it's a bad practice, but for backward comp I understand why unbounded is still allowed.

Concern answered.

[uranusjr]

This is superceded by #71704

[aeroyorch]

---

Allow DBDagBag TTL cache eviction without a size cap (i.e. cache_size=0, cache_ttl>0).

Related to #69001 and #69007

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

Claude Opus 4.8 to analyze the impact of this change in terms of docs/code.

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

[SameerMesiah97]

Just one nit. Looks good otherwise.

[aeroyorch]

Just one nit. Looks good otherwise.

Thanks for the review! Changes already implemented

[lohyenshen]

hi, may I know what airflow version would this be released to?

[vatsrahul1001]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

[aeroyorch]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

Thanks for the review. Changes already implemented :)

[vatsrahul1001]

LGTM!, can be merged after code owners review

[vatsrahul1001]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

[aeroyorch]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

No problem. In the meantime, I can work on some of the other items to fix #69001

[ColtenOuO]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

[aeroyorch]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

Thanks for the review!

Fair point, but I'd rather leave it as is for now, the trade-off you mention is the deciding one for me: keeping it at the call site lets us name the actual config option that's wrong, which is what the user needs to fix.

Happy to revisit when the Scheduler part lands for #69001.

[pierrejeambrun]

Do you mind giving more context as to why we are doing this. If your use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbounded cache there.

[aeroyorch]

Do you mind giving more context as to why we are doing this. If you use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbouded cache there.

Hi Pierre, sure.

I don't think this brings back the leak. Today cache_size=0 is a plain dict with no eviction at all. With this change, cache_size=0 + cache_ttl>0 creates a real TTLCache. That evicts exactly what grows and keeps the hot ones. Defaults don't change.

Tuning the size doesn't really help here: the scheduler cycles through all active dag_version_ids, so any cap below the active set evicts each key just before it's needed again. @kaxil raised this in the #69007 review and @potiuk asked for TTL eviction without a size cap in DBDagBag as a first step.

[pierrejeambrun]

Thanks for the context @aeroyorch this makes more sense. I had the wrong assumption that unbounded cache was just not possible anymore. (at all)

I would be for not allowing unbounded cache completely since we know it's a bad practice, but for backward comp I understand why unbounded is still allowed.

Concern answered.

[uranusjr]

This is superceded by #71704

[aeroyorch]

---

Allow DBDagBag TTL cache eviction without a size cap (i.e. cache_size=0, cache_ttl>0).

Related to #69001 and #69007

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

Claude Opus 4.8 to analyze the impact of this change in terms of docs/code.

---

• Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
• For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
• When adding dependency, check compliance with the ASF 3rd Party License Policy.
• For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

[SameerMesiah97]

Just one nit. Looks good otherwise.

[aeroyorch]

Just one nit. Looks good otherwise.

Thanks for the review! Changes already implemented

[lohyenshen]

hi, may I know what airflow version would this be released to?

[vatsrahul1001]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

[aeroyorch]

The DBDagBag fix itself looks right, but I don't think it's reachable from the actual config path yet. create_dag_bag() in api_fastapi/common/dagbag.py still does:

if cache_size <= 0:
return DBDagBag(cache_size=0)

That early-return fires whenever dag_cache_size <= 0 and never even reads cache_ttl_config, so setting dag_cache_size = 0 + dag_cache_ttl = 3600 in airflow.cfg still ends up as a plain unbounded dict with no eviction. test_create_dag_bag_cache_modes still asserts exactly that (the "size_zero_unbounded" case expects dict/_use_cache=False for cache_size=0, cache_ttl=3600), and config.yml's dag_cache_size description still says 0 means "unbounded dict, no eviction" with no mention of TTL-only mode.

Could you also update create_dag_bag() to pass cache_ttl through when cache_size<=0, add a case to test_create_dag_bag_cache_modes covering that combo, and touch up the config.yml wording? Otherwise this fix isn't actually reachable by anyone configuring it through airflow.cfg.

Thanks for the review. Changes already implemented :)

[vatsrahul1001]

LGTM!, can be merged after code owners review

[vatsrahul1001]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

[aeroyorch]

Moving to 3.3.2 as this is still pending code owner review and do not want to rush on merging this as it not critical

No problem. In the meantime, I can work on some of the other items to fix #69001

[ColtenOuO]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

[aeroyorch]

LGTM and Thanks for the improvement! I actually just noticed this issue myself, and was glad to see someone was already working on a fix!

Just a small idea, feel free to take it or leave it if it doesn't quite fit!

Would it make sense to move the warning logs into DBDagBag.__init__ itself? To fully close out #69001, this change will eventually need to be wired up to the Scheduler too , and at that point we'd want to warn users about the exact same negative-value cases again. If the warning lived inside DBDagBag.__init__ from the start, it'd be a lot easier to keep things consistent as more callers get hooked up down the road , one place to maintain, and no risk of some future caller quietly forgetting to warn.

The trade-off is we'd lose the ability to tell the user exactly which config option was the problem (e.g. [api] dag_cache_size vs. a future [scheduler] dag_bag_cache_size)

Thanks for the review!

Fair point, but I'd rather leave it as is for now, the trade-off you mention is the deciding one for me: keeping it at the call site lets us name the actual config option that's wrong, which is what the user needs to fix.

Happy to revisit when the Scheduler part lands for #69001.

[pierrejeambrun]

Do you mind giving more context as to why we are doing this. If your use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbounded cache there.

[aeroyorch]

Do you mind giving more context as to why we are doing this. If you use case is "LRU evicts my hot dags too aggressively", tweaking the settings for your use case is probably better.

It looks like we are introducing back the memory leak this bounded cache was fixing in the first place. I don't think we should allow unbouded cache there.

Hi Pierre, sure.

I don't think this brings back the leak. Today cache_size=0 is a plain dict with no eviction at all. With this change, cache_size=0 + cache_ttl>0 creates a real TTLCache. That evicts exactly what grows and keeps the hot ones. Defaults don't change.

Tuning the size doesn't really help here: the scheduler cycles through all active dag_version_ids, so any cap below the active set evicts each key just before it's needed again. @kaxil raised this in the #69007 review and @potiuk asked for TTL eviction without a size cap in DBDagBag as a first step.

[pierrejeambrun]

Thanks for the context @aeroyorch this makes more sense. I had the wrong assumption that unbounded cache was just not possible anymore. (at all)

I would be for not allowing unbounded cache completely since we know it's a bad practice, but for backward comp I understand why unbounded is still allowed.

Concern answered.

[uranusjr]

This is superceded by #71704