CodeTrainerMan commented on issue #1108: URL: https://github.com/apache/flink-agents/issues/1108#issuecomment-6102424261
Thanks @wenjin272 for the go-ahead, and @emecii for raising this. I'm starting on it now (branch `fix/skill-manager-close-failures-1108`), following the direction @wenjin272 endorsed: - `SkillManager.close()` attempts every distinct repository — iterating `_opened_repos` and deduping by identity, since a duplicate skill name displaces a repo in `_repos` while that repo is still owned and must be closed; - the first `close()` failure is rethrown after the loop, matching the Java `SkillManager.closeRepos()` contract from #987 and the Python `ResourceCache.close()` pattern; - later failures are logged the same way `ResourceCache.close()` logs them; - the cleanup path in `_load_skills()` logs any close failure instead of letting it replace the original load failure (`ExceptionGroup` needs Python 3.11 and this package supports 3.10); - tests with repositories whose `close()` raises, including one proving later repositories are still closed. Two things I'd like to flag before opening the PR. **1. Template and labels.** I can't edit the issue body or set labels myself — both return `403 Must have admin rights to Repository`, and I'm only the assignee. @emecii, could you apply the Technical Debt template below and add the `tech debt` / `priority/major` labels? @wenjin272, if it's easier for a committer to do it, that works too. <details> <summary>Technical Debt template version of the description</summary> ## Search before asking - [x] I searched in the [issues](https://github.com/apache/flink-agents/issues) and found nothing similar. ## Description `python/flink_agents/runtime/skill/skill_manager.py` currently suppresses every exception from `SkillRepository.close()` in `SkillManager.close()`. It still attempts later repositories, but a failure is neither re-raised nor logged. As a result, an extension-provided repository cleanup failure is invisible to callers. This differs from the Java `SkillManager.closeRepos()` contract introduced in #987: it attempts every repository close, then rethrows the first failure after the loop. Python `ResourceCache.close()` follows the same close-everything-then-surface-the-first-failure pattern. The current in-tree repository implementations do not raise on close, so this is a contract/parity defect for the public `SkillRepository` extension point rather than a known incident in a bundled repository. Proposed direction, as endorsed by @wenjin272 in https://github.com/apache/flink-agents/issues/1108#issuecomment-5790781292: - `SkillManager.close()` attempts every distinct repository, iterating `_opened_repos` and deduping by identity; - the first `close()` failure is rethrown after the loop, matching the Java `SkillManager.closeRepos()` contract from #987 and the existing Python `ResourceCache.close()` pattern; - later failures are logged consistently with `ResourceCache.close()`; - the cleanup path in `_load_skills()` must not let a close failure replace the original load failure. Java attaches it with `addSuppressed`; `ExceptionGroup` needs Python 3.11 and this package supports 3.10, so the cleanup failure is logged instead; - focused tests using test repositories whose `close()` raises, including one that proves later repositories are still closed. Still present on the current `main` branch as of 2026-09-23 (confirmed by @wenjin272). ## Are you willing to submit a PR? - [x] I'm willing to submit a PR! </details> **2. Where the shared close helpers should live.** `_failure_of` / `_first_or_logged` currently live in `resource_cache.py`, and `skill_manager` cannot import them at module level — `skill_manager -> resource_cache -> resource_context -> skill_manager` is an import cycle. I'd like to extract them into a small dependency-free `runtime/close_utils.py` and have `resource_cache`, `flink_runner_context` and `skill_manager` share it, so "consistent with `ResourceCache.close()`" is guaranteed by the code rather than by duplication. If you'd rather keep the diff to `skill_manager.py` alone, I'll keep a local copy instead — happy either way. @emecii — if you already have something in flight, just say so and I'll step aside. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
