Repository navigation
Conversation
dba6ec3 to
b3702de
Compare
|
First, I'm sorry this sat over a month without any reviews, we really got swarmed with AI-generated contributions and didn't (still don't, TBH) have a good way to filter them out. That said, nice troubleshooting. A permanent 404 for a secret that exists is a real bug, and the The root cause looks like it is that both callers accept a database failure as missing data. except AirflowSecretsBackendAccessDenied:
# Authoritative deny — must NOT fall through to a less-restrictive backend.
raise
except Exception:
...fall through......and then, after the loop, Here's what I'd suggest: keep trying the remaining backends since a metastore blip shouldn't stop Vault from answering, but track that a backend errored. Then, if nothing is found, surface a database error rather than not-found, and skip the One more reason to prefer that over the decorator: Happy to be argued out of it if you think the retry is the better solution. It's an improvement either way, just not one that closes the 404 bug. |
A momentary metadata-database failure during a connection or variable lookup is currently indistinguishable from the secret not existing. Both get_connection_from_secrets and get_variable_from_secrets swallow every exception a backend raises, log it at debug level, and fall through to the next backend, so a dropped connection, a failover, or a deadlock surfaces to the caller as "the conn_id isn't defined" or a 404 rather than as a retryable server error. MetastoreBackend is the one backend in the default chain that talks to the metadata database, and Airflow already has retry_db_transaction for exactly this class of failure. Signed-off-by: 1fanwang <1fannnw@gmail.com>
b3702de to
809aca7
Compare
Signed-off-by: 1fanwang <1fannnw@gmail.com>
A metadata DB error was swallowed by the secrets backend loop and reported as "isn't defined", and the miss was cached. Keep asking the other backends, raise the DB error when none has the secret, and don't cache that miss. Drop the metastore retry, which rolled back a caller-owned session. On workers, only NOT_FOUND from the Execution API now means missing. Signed-off-by: 1fanwang <1fannnw@gmail.com>
…g secrets Under dag.test() the in-process supervisor lets the client's server error through unchanged, so the secrets dispatcher treated a 500 from the Execution API like any backend failure and reported the secret as missing. Convert it to the same API_SERVER_ERROR the subprocess supervisor sends. Signed-off-by: 1fanwang <1fannnw@gmail.com>
…try-refresh Signed-off-by: 1fanwang <1fannnw@gmail.com> # Conflicts: # task-sdk/src/airflow/sdk/execution_time/secrets/execution_api.py
…try-refresh Signed-off-by: 1fanwang <1fannnw@gmail.com>
A short metadata database hiccup makes a connection or variable look missing. The lookup treats the database error like any other backend failure, moves on to the next backend, and ends with "isn't defined" or "does not exist". Tasks get the same answer: the server reports the failed lookup as not found, and the worker reads any server error as not found too. With the secrets cache enabled, the miss is cached, so the key keeps looking missing after the database recovers. A variable read with a default quietly gets the default.
What changes:
Behaviour change: when the database fails and no backend has the key, reading a variable with a default raises the error instead of returning the default, in tasks and on the server.
Testing Done
Lookups against the metadata database
A real SQLite metadata database, migrated with airflow db migrate and seeded with a variable and a connection, with the secrets cache enabled. A second connection holds an exclusive lock, so every read fails with "database is locked", then releases it. The later-backend line puts the environment variables backend after the metastore (in the default chain the metastore is last).
Reproducer source: e2e_locked_db.py
Before this change:
After:
Lookups from a task
A task run with dag.test() reads a variable and a connection while the matching table is renamed away, so the server's query fails with a real "no such table" error. dag.test() runs the task in-process: task, supervisor, Execution API, then the same SQLite database.
Reproducer source: e2e_worker_path.py
Before this change:
After:
Was generative AI tooling used to co-author this PR?
Generated-by: GitHub Copilot CLI (Auto model selection) following the guidelines