[PR] Stop returning task state store keys that are past their expiry [airflow]

["SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>]

SEPURI-SAI-KRISHNA opened a new pull request, #74181:
URL: https://github.com/apache/airflow/pull/74181

   `task_state_store.set(key, value, retention=timedelta(hours=6))` is documented as making the key expire after six hours. It does not. `expires_at` is written on every path and read on none, so `get()` keeps returning the value indefinitely.
   
   Nothing else closes the gap. `airflow state-store clean` is the only thing that acts on `expires_at`, and the cleanup page states that Airflow does not purge those rows on a schedule and that cleanup must be triggered explicitly via the CLI. On a deployment that has not cron-scheduled that command, no key has ever expired.
   
   **The change**
   
   The two read paths, `_get_task_state_store` and `_aget_task_state_store`, now skip rows whose `expires_at` has passed. `NULL` still means never expires, which is what `set()` writes when no expiry is given.
   
   Deliberately not filtered: `_delete_task_state_store` and `_adelete_task_state_store`, because an expired key must still be deletable, and the selects inside `cleanup()` and `_summary_dry_run()`, which exist to find expired rows. `idx_task_state_store_expires_at` already indexes the column.
   
   The two documentation pages contradicted each other, one describing expiry and the other describing deletion eligibility, so both now say the same thing: expiry is enforced on read, and cleanup reclaims the disk space afterwards.
   
   **Tests**
   
   Two tests, one per read path: a key past its expiry reads as absent, synchronously and asynchronously. Both fail without the change and the other 45 tests in the file pass either way. I did not add a test for the `NULL` case, because `set()` without an expiry already writes `NULL` and the existing round-trip tests cover that branch, so removing the `IS NULL` arm fails them.
   
   **Scope**
   
   The admin REST API is left alone. `GET /dags/.../taskStateStore/{key}` queries the model directly rather than through the backend and returns `expires_at` in its response, which reads as an inspection surface, so it still shows rows pending cleanup. Happy to make it 404 expired keys instead if you would rather it match the SDK exactly.
   
   Expired rows are not deleted on read. Keeping reads read-only avoids write amplification on a hot path; `airflow state-store clean` and `airflow db clean` remain the reclaim paths.
   
   The asset state store is unaffected: `AssetStateStoreModel` has no `expires_at` column, so there is nothing to enforce there.
   
   Related but different: #68794 and #68793 were about `cleanup()` skipping rows, and #66459 added the cleanup command itself. This is the read half, which none of them covered.
   
   A newsfragment will follow once the PR number exists.
   
   ---
   
   ##### Was generative AI tooling used to co-author this PR?
   
   - [X] Yes - Claude Code (Opus 5)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Stop returning task state store keys that are past their expiry [airflow]

["SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>]

SEPURI-SAI-KRISHNA commented on PR #74181:
URL: https://github.com/apache/airflow/pull/74181#issuecomment-6165134328

   Gentle ping on this one. It has been open ten days with CI green, 43 checks passing and none failing.
   
   No reviewer was auto-assigned, and I think that is why it has been quiet rather than anything about the change: `.github/CODEOWNERS` has entries for `airflow-core/src/airflow/executors/`, `jobs/`, `models/`, `serialization/` and `dag_processing`, but none that covers `airflow-core/src/airflow/state/`, so nobody was notified when it was opened.
   
   For context on what it does: `task_state_store.set(key, value, retention=timedelta(hours=6))` is documented as making the key expire, and it does not. `expires_at` is written on every path and read on none, so `get()` keeps returning the value indefinitely, and the cleanup command is documented as manual and never scheduled by Airflow. The change adds the expiry predicate to the two read paths and leaves the delete and cleanup queries alone, since those exist to find expired rows.
   
   Happy to rebase or split it if that would help.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org