fix(dex-hand): release temp YAML config after optimizer build#479
fix(dex-hand): release temp YAML config after optimizer build#479
Conversation
DexHandRetargeter._update_yaml wrote a tempfile per construction (so the URDF path could be patched without mutating the original config) and never removed it. Long-running sessions or test suites that built many retargeters left a stream of /tmp/dex_retarget_*.yml files behind. Track the temp path on the instance and unlink it once RetargetingConfig.load_from_file(...).build() has consumed the YAML. __del__ acts as a safety net for cases where the optimizer build raises between writing the temp file and the explicit cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses a temporary-file leak in DexHandRetargeter by tracking the generated YAML config path and deleting the temp file once the dex_retargeting optimizer has been built.
Changes:
- Delete the temp YAML config immediately after
RetargetingConfig.load_from_file(...).build()completes. - Track the temp YAML path on the instance and add an idempotent cleanup helper.
- Add a
__del__safety net to attempt cleanup if construction fails after writing the temp file.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if temp_config: | ||
| self._config.hand_retargeting_config = temp_config | ||
| # Track for cleanup once the optimizer has consumed the file. | ||
| self._temp_config_path: Optional[str] = temp_config | ||
| else: | ||
| self._temp_config_path = None |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/retargeters/dex_hand_retargeter.py`:
- Around line 314-333: _prepare_configs currently overwrites
self._config.hand_retargeting_config with a temp YAML path and
_cleanup_temp_config deletes that file but does not restore the original config,
leaving a stale path; change _prepare_configs to save the original value (e.g.,
self._original_hand_retargeting_config) before assigning
self._config.hand_retargeting_config = temp_config, and update
_cleanup_temp_config to restore self._config.hand_retargeting_config from
self._original_hand_retargeting_config (or set it to None if there was no
original), then remove the temp file and clear both self._temp_config_path and
self._original_hand_retargeting_config to avoid stale references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 271c234e-8ab6-4694-a0a0-2ae20bef391f
📒 Files selected for processing (1)
src/retargeters/dex_hand_retargeter.py
| if temp_config: | ||
| self._config.hand_retargeting_config = temp_config | ||
| # Track for cleanup once the optimizer has consumed the file. | ||
| self._temp_config_path: Optional[str] = temp_config | ||
| else: | ||
| self._temp_config_path = None | ||
|
|
||
| def _cleanup_temp_config(self) -> None: | ||
| """Remove the temp YAML written by ``_update_yaml`` once the optimizer | ||
| has been built from it. Safe to call repeatedly.""" | ||
| path = getattr(self, "_temp_config_path", None) | ||
| if not path: | ||
| return | ||
| try: | ||
| os.unlink(path) | ||
| except FileNotFoundError: | ||
| pass | ||
| except OSError as e: | ||
| logger.warning(f"Failed to remove temp dex_retargeting config {path}: {e}") | ||
| self._temp_config_path = None |
There was a problem hiding this comment.
config.hand_retargeting_config is left pointing to a deleted file.
_prepare_configs() overwrites self._config.hand_retargeting_config with the temp YAML path, and _cleanup_temp_config() deletes that file. After construction, the config object now contains a stale path, so reusing the same DexHandRetargeterConfig can fail on the next initialization.
Suggested fix
@@
- self._dex_hand = RetargetingConfig.load_from_file(
- config.hand_retargeting_config
- ).build()
+ self._dex_hand = RetargetingConfig.load_from_file(
+ self._retargeting_config_path
+ ).build()
@@
- if temp_config:
- self._config.hand_retargeting_config = temp_config
- # Track for cleanup once the optimizer has consumed the file.
- self._temp_config_path: Optional[str] = temp_config
- else:
- self._temp_config_path = None
+ # Keep caller-provided config immutable; use an internal load path.
+ self._retargeting_config_path = self._config.hand_retargeting_config
+ if temp_config:
+ self._retargeting_config_path = temp_config
+ # Track for cleanup once the optimizer has consumed the file.
+ self._temp_config_path: Optional[str] = temp_config
+ else:
+ self._temp_config_path = None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/retargeters/dex_hand_retargeter.py` around lines 314 - 333,
_prepare_configs currently overwrites self._config.hand_retargeting_config with
a temp YAML path and _cleanup_temp_config deletes that file but does not restore
the original config, leaving a stale path; change _prepare_configs to save the
original value (e.g., self._original_hand_retargeting_config) before assigning
self._config.hand_retargeting_config = temp_config, and update
_cleanup_temp_config to restore self._config.hand_retargeting_config from
self._original_hand_retargeting_config (or set it to None if there was no
original), then remove the temp file and clear both self._temp_config_path and
self._original_hand_retargeting_config to avoid stale references.
CodeRabbit and Copilot flagged that _prepare_configs was overwriting self._config.hand_retargeting_config with the temp YAML path; once _cleanup_temp_config unlinks that file the config object now points at a deleted path. Callers that reuse the same DexHandRetargeterConfig would then fail to construct another retargeter. Resolve the load path internally (self._retargeting_config_path) and leave the caller's config untouched. The temp file is still tracked separately for cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
DexHandRetargeter._update_yamlwrites a temp file (tempfile.mkstemp(suffix=".yml", prefix="dex_retarget_")) per construction so the URDF path can be patched without mutating the original config. The fd was closed but the file was never unlinked.RetargetingConfig.load_from_file(...).build()returns — the optimizer has parsed the YAML by then and no longer needs the file.__del__as a safety net for the construction path where the optimizer build raises after the temp file was written._cleanup_temp_configis idempotent (safe to call from both the explicit cleanup and__del__).Why this matters
Long-running sessions, test suites, and pytest runs that instantiate many
DexHandRetargeters leak a/tmp/dex_retarget_*.ymlfile each, slowly filling/tmp. No functional impact, but it's the kind of leak that surfaces as an outage on a long-running production deployment.Test plan
/tmp/dex_retarget_*count is unchanged after the run vs. before.🤖 Generated with Claude Code
Summary by CodeRabbit