Repository navigation
fix(config): pin explicit app config path across get_app_config - #7092
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
github-actions
Bot
added
area:backend
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at head abbf7d7 (fix(config): pin explicit app config path across get_app_config).
The fix works as intended and I verified it against the head SHA:
get_app_config()now resolves_app_config_sourcefirst, so every consumer of the singleton — includingDeerFlowClient(config_path=...)(client.pycallsreload_app_config(config_path)thenget_app_config()) — keeps following the explicitly loaded file instead of silently falling back to sibling/ambient default resolution, and no longer raises when no ambient config exists.- The pin is not re-set by ambient refreshes (
get_app_config->_load_and_cache_app_configleaves it alone), is cleared byreset_app_config()/set_app_config()/ argument-lessreload_app_config(), and a failed explicit reload keeps the prior pin. A missing explicit path still fails fast viaAppConfig.resolve_config_path. - Tests: ran
backend/tests/test_app_config_explicit_path.pyagainst head source (PYTHONPATH pinned to the PR tree): 4 passed. No regressions:test_app_config_reload.py47 passed,test_client.py -k config37 passed. No prior human reviews to duplicate.
Two non-blocking observations below, both about consistency with the _extensions_config_source analogue this change explicitly mirrors.
| # prior pin (mirrors ``_extensions_config_source``). Ambient refreshes go | ||
| # through ``get_app_config`` -> ``_load_and_cache_app_config`` and must not | ||
| # pin the resolved path. | ||
| _app_config_source = config_path or None |
There was a problem hiding this comment.
Non-blocking: the analogue this mirrors (_extensions_config_source) mutates its pin under _extensions_config_lock (see _load_and_cache_extensions_config: "The caller holds _extensions_config_lock"), but the app-config cache layer has no lock, and here the pin is written after _load_and_cache_app_config has already swapped _app_config / _app_config_path / _app_config_signature. A concurrent get_app_config() can therefore observe the freshly loaded explicit-path config under the stale pin (e.g. pin=A, cache=B), see _app_config_path != resolved_path, and reload ambient/A over it; interleaving an argument-less reload_app_config() (clears the pin first) with an explicit one can likewise leave _app_config_source pointing at a file the cache no longer holds. This is consistent with the layer's pre-existing lock-free design, so not a regression — but since DeerFlowClient(config_path=...) (which triggers this) can share a process with hot-path get_app_config() reads, consider a module-level lock, or setting the pin inside _load_and_cache_app_config behind a flag, to actually match the extensions contract.
| # ``set_app_config()``, or an argument-less reload; probing default resolution | ||
| # instead would either flip the cache back to a sibling default or raise when | ||
| # no ambient config exists (discarding the explicit load). | ||
| _app_config_source: str | None = None |
There was a problem hiding this comment.
Nit: the pin stores the caller's raw config_path string, and get_app_config() re-resolves it with Path(_app_config_source) against the then-current working directory — so a relative path pinned once (e.g. reload_app_config("custom/config.yaml")) silently resolves elsewhere, or raises, if the process chdir's before the next get_app_config(). _load_and_cache_app_config already computes the resolved Path; recording str(resolved_path) would make the pin CWD-stable. _extensions_config_source shares this behavior, so this is a consistency question rather than a regression.
github-actions
Bot
added
area:docs
66f32f8 to
2034159
Compare
Fixes bytedance#7086. reload_app_config(config_path) now records an _app_config_source pin (mirroring _extensions_config_source) so get_app_config and DeerFlowClient(config_path=...) keep following the explicit file instead of ambient default resolution.
Serialize singleton reads, reloads, resets and injections so a concurrent call cannot publish a config under another file's pin. Keep explicit paths absolute across working-directory changes while preserving ambient discovery and failed-reload behavior. Add deterministic concurrency and relative-path regressions. Validation: 33,875 offline tests and 262 blocking-I/O tests passed; frontend check, backend formatting and agent guidance passed.
2034159 to
6f618be
Compare
Summary
Fixes #7086.
DeerFlowClient(config_path=...)/reload_app_config(explicit_path)did not remember the explicit path, so a laterget_app_config()re-resolved through ambient search and either silently preferred a sibling defaultconfig.yamlor raisedFileNotFoundErrorwhen none existed.This mirrors the existing
_extensions_config_sourcepin:reload_app_config(config_path)records_app_config_sourceget_app_config()resolves through that pin (edits to the pinned file still revalidate)reset_app_config(),set_app_config(), and argument-lessreload_app_config()get_app_config()do not pin the resolved path (soDEER_FLOW_CONFIG_PATHchanges still take effect)DeerFlowClientneeds no change.Test plan
tests/test_app_config_explicit_path.py(4 tests) failed on base, pass with the fixpytest tests/test_app_config_explicit_path.py tests/test_app_config_reload.py— 51 passedruff checkon touched files — clean