From ebc60e239c46b7086d27c5f102d52bf58c44d10b Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Wed, 5 Aug 2026 01:16:21 +0200 Subject: [PATCH] fix(optuna): harden best-params I/O against deployment failure modes Address three P2 review findings; the same latent behaviors were ported from QuickAdapter to ReforceXY, so all fixes are applied to both. - Cross-uid save (C2): make owner preservation best-effort. A non-root process on a bind mount with a differently owned existing file lacks CAP_CHOWN; the fchown now swallows PermissionError (logged at debug) so the atomic write still completes, as the previous in-place write did. fchmod and the successful-chown path are unchanged. - Read-only mount (C3): a shared (read) lock no longer forces O_CREAT. On a read-only mount where the lock file is absent, the load reads without a lock; os.replace atomicity guarantees a consistent read. Exclusive locks keep O_CREAT and still fail closed on read-only filesystems. S_ISREG/O_NOFOLLOW/O_NONBLOCK guards are preserved. - Legacy warm-start (C1): a base-only legacy best-params file is now loaded when exactly one configured pair maps to that base (read in place, no rename, read-only safe). Ambiguous cases (more than one pair sharing a base) still warn and return None, preserving #180 safety. The configured pair list is threaded into the QuickAdapter module-level loader; a legacy payload still passes the same validation. --- ReforceXY/user_data/freqaimodels/ReforceXY.py | 88 ++++++++++++------- .../freqaimodels/QuickAdapterRegressorV3.py | 1 + .../user_data/strategies/QuickAdapterV3.py | 4 +- quickadapter/user_data/strategies/Utils.py | 85 ++++++++++++------ 4 files changed, 118 insertions(+), 60 deletions(-) diff --git a/ReforceXY/user_data/freqaimodels/ReforceXY.py b/ReforceXY/user_data/freqaimodels/ReforceXY.py index 2a8f0b0..d8d7442 100644 --- a/ReforceXY/user_data/freqaimodels/ReforceXY.py +++ b/ReforceXY/user_data/freqaimodels/ReforceXY.py @@ -1740,17 +1740,29 @@ class ReforceXY(BaseReinforcementLearningModel): / f"hyperopt-best-params-{ReforceXY._sanitize_pair(pair)}.json" ) - def _warn_ambiguous_legacy_best_trial_params( + def _resolve_legacy_best_trial_params( self, pair: str, best_trial_params_path: Path - ) -> None: - legacy_path = self.full_path / f"hyperopt-best-params-{pair.split('/')[0]}.json" - if legacy_path != best_trial_params_path and legacy_path.is_file(): - logger.warning( - "Hyperopt [%s]: ignoring ambiguous legacy best params at %s: " - "filename does not encode the complete pair identity", - pair, - legacy_path, - ) + ) -> Optional[Path]: + base = pair.split("/")[0] + legacy_path = self.full_path / f"hyperopt-best-params-{base}.json" + if ( + legacy_path == best_trial_params_path + or legacy_path.is_symlink() + or not legacy_path.is_file() + ): + return None + base_pair_count = sum( + 1 for configured in self.pairs if configured.split("/")[0] == base + ) + if base_pair_count == 1: + return legacy_path + logger.warning( + "Hyperopt [%s]: ignoring ambiguous legacy best params at %s: " + "filename does not encode the complete pair identity", + pair, + legacy_path, + ) + return None @staticmethod @contextmanager @@ -1762,15 +1774,18 @@ class ReforceXY(BaseReinforcementLearningModel): ) # O_NONBLOCK so a pre-existing FIFO (unlike a symlink, not caught by # O_NOFOLLOW) cannot hang this open before the S_ISREG guard rejects it. - lock_fd = os.open( - lock_path, - (os.O_RDWR if exclusive else os.O_RDONLY) - | os.O_CREAT - | os.O_CLOEXEC - | os.O_NOFOLLOW - | os.O_NONBLOCK, - 0o666, - ) + # A shared reader omits O_CREAT: a read-only mount cannot create the lock, + # and os.replace atomicity keeps a lock-free read consistent. + open_flags = ( + (os.O_RDWR | os.O_CREAT) if exclusive else os.O_RDONLY + ) | os.O_CLOEXEC | os.O_NOFOLLOW | os.O_NONBLOCK + try: + lock_fd = os.open(lock_path, open_flags, 0o666) + except FileNotFoundError: + if exclusive: + raise + yield + return try: if not stat.S_ISREG(os.fstat(lock_fd).st_mode): raise OSError( @@ -1857,15 +1872,26 @@ class ReforceXY(BaseReinforcementLearningModel): temporary_metadata.st_uid != existing_metadata.st_uid or temporary_metadata.st_gid != existing_metadata.st_gid ): - os.fchown( - write_file.fileno(), - existing_metadata.st_uid - if temporary_metadata.st_uid != existing_metadata.st_uid - else -1, - existing_metadata.st_gid - if temporary_metadata.st_gid != existing_metadata.st_gid - else -1, - ) + # Best-effort: a non-root process on a cross-uid bind + # mount lacks CAP_CHOWN; the previous in-place write + # never chowned, so a failure must not abort the save. + try: + os.fchown( + write_file.fileno(), + existing_metadata.st_uid + if temporary_metadata.st_uid != existing_metadata.st_uid + else -1, + existing_metadata.st_gid + if temporary_metadata.st_gid != existing_metadata.st_gid + else -1, + ) + except PermissionError as chown_error: + logger.debug( + "Hyperopt [%s]: best params ownership " + "preservation skipped: %r", + pair, + chown_error, + ) os.fchmod( write_file.fileno(), stat.S_IMODE(existing_metadata.st_mode), @@ -1909,10 +1935,12 @@ class ReforceXY(BaseReinforcementLearningModel): with self._locked_best_trial_params(best_trial_params_path, exclusive=False): self._reject_best_trial_params_symlink(best_trial_params_path) if not best_trial_params_path.is_file(): - self._warn_ambiguous_legacy_best_trial_params( + legacy_path = self._resolve_legacy_best_trial_params( pair, best_trial_params_path ) - return None + if legacy_path is None: + return None + best_trial_params_path = legacy_path logger.info( "Hyperopt [%s]: loading best params from %s", pair, diff --git a/quickadapter/user_data/freqaimodels/QuickAdapterRegressorV3.py b/quickadapter/user_data/freqaimodels/QuickAdapterRegressorV3.py index 6146913..03627ac 100644 --- a/quickadapter/user_data/freqaimodels/QuickAdapterRegressorV3.py +++ b/quickadapter/user_data/freqaimodels/QuickAdapterRegressorV3.py @@ -5209,6 +5209,7 @@ class QuickAdapterRegressorV3(BaseRegressionModel): pair, namespace, logger, + pairs=self.pairs, expected_selection_metadata=expected, expected_objective_identity=QuickAdapterRegressorV3._OPTUNA_HP_OBJECTIVE_IDENTITY if namespace == _OPTUNA_NAMESPACES.hp diff --git a/quickadapter/user_data/strategies/QuickAdapterV3.py b/quickadapter/user_data/strategies/QuickAdapterV3.py index 6499291..f4cd6fd 100644 --- a/quickadapter/user_data/strategies/QuickAdapterV3.py +++ b/quickadapter/user_data/strategies/QuickAdapterV3.py @@ -2468,4 +2468,6 @@ class QuickAdapterV3(IStrategy): # tolerable here. The regressor's ``optuna_load_best_params`` # passes ``expected_selection_metadata`` and rejects drift before # re-running HPO selection. - return optuna_load_best_params(self.models_full_path, pair, namespace, logger) + return optuna_load_best_params( + self.models_full_path, pair, namespace, logger, pairs=self.pairs + ) diff --git a/quickadapter/user_data/strategies/Utils.py b/quickadapter/user_data/strategies/Utils.py index 7ab52f5..92061cb 100644 --- a/quickadapter/user_data/strategies/Utils.py +++ b/quickadapter/user_data/strategies/Utils.py @@ -5311,28 +5311,39 @@ def _optuna_best_params_path( return base_path / f"optuna-{namespace}-best-params-{pair_to_filename(pair)}.json" -def _warn_ambiguous_legacy_optuna_best_params( +def _resolve_legacy_optuna_best_params( base_path: Path, pair: str, namespace: OptunaNamespace, best_params_path: Path, + pairs: Sequence[str] | None, logger: Logger | None, -) -> None: - """Warn when a base-only legacy best-params file shadows the pair-safe path.""" - if logger is None: - return +) -> Path | None: + """Return a usable legacy base-only best-params file, or None. + + The legacy filename encodes only the pair base; reusing it is safe only + when exactly one configured pair maps to that base. Otherwise it is + ambiguous and must be ignored (with a warning). + """ + base = pair.split("/")[0] legacy_best_params_path = ( - base_path / f"optuna-{namespace}-best-params-{pair.split('/')[0]}.json" + base_path / f"optuna-{namespace}-best-params-{base}.json" ) if ( - legacy_best_params_path != best_params_path - and legacy_best_params_path.is_file() + legacy_best_params_path == best_params_path + or legacy_best_params_path.is_symlink() + or not legacy_best_params_path.is_file() ): + return None + if pairs is not None and sum(1 for p in pairs if p.split("/")[0] == base) == 1: + return legacy_best_params_path + if logger is not None: logger.warning( f"[{pair}] Ignoring ambiguous legacy Optuna {namespace} best params " f"at {legacy_best_params_path}: filename does not encode the complete " f"pair identity" ) + return None _OPTUNA_LABEL_BEST_PARAMS_SCHEMA_VERSION: Final[int] = 2 @@ -5547,15 +5558,18 @@ def _locked_optuna_best_params( lock_path = best_params_path.parent / ".optuna-best-params.lock" # O_NONBLOCK so a pre-existing FIFO (unlike a symlink, not caught by # O_NOFOLLOW) cannot hang this open before the S_ISREG guard rejects it. - lock_fd = os.open( - lock_path, - (os.O_RDWR if exclusive else os.O_RDONLY) - | os.O_CREAT - | os.O_CLOEXEC - | os.O_NOFOLLOW - | os.O_NONBLOCK, - 0o666, - ) + # A shared reader omits O_CREAT: a read-only mount cannot create the lock, + # and os.replace atomicity keeps a lock-free read consistent. + open_flags = ( + (os.O_RDWR | os.O_CREAT) if exclusive else os.O_RDONLY + ) | os.O_CLOEXEC | os.O_NOFOLLOW | os.O_NONBLOCK + try: + lock_fd = os.open(lock_path, open_flags, 0o666) + except FileNotFoundError: + if exclusive: + raise + yield + return try: if not stat.S_ISREG(os.fstat(lock_fd).st_mode): raise OSError(f"Optuna best params lock {lock_path} must be a regular file") @@ -5579,6 +5593,7 @@ def optuna_load_best_params( namespace: OptunaNamespace, logger: Logger | None = None, *, + pairs: Sequence[str] | None = None, expected_selection_metadata: dict[str, Any] | None = None, expected_objective_identity: str | None = None, ) -> dict[str, Any] | None: @@ -5589,10 +5604,12 @@ def optuna_load_best_params( with _locked_optuna_best_params(best_params_path, exclusive=False): _reject_optuna_best_params_symlink(best_params_path) if not best_params_path.is_file(): - _warn_ambiguous_legacy_optuna_best_params( - base_path, pair, namespace, best_params_path, logger + legacy_best_params_path = _resolve_legacy_optuna_best_params( + base_path, pair, namespace, best_params_path, pairs, logger ) - return None + if legacy_best_params_path is None: + return None + best_params_path = legacy_best_params_path try: with best_params_path.open("r", encoding="utf-8") as read_file: best_params = json.load(read_file) @@ -5691,15 +5708,25 @@ def optuna_save_best_params( temporary_metadata.st_uid != existing_metadata.st_uid or temporary_metadata.st_gid != existing_metadata.st_gid ): - os.fchown( - write_file.fileno(), - existing_metadata.st_uid - if temporary_metadata.st_uid != existing_metadata.st_uid - else -1, - existing_metadata.st_gid - if temporary_metadata.st_gid != existing_metadata.st_gid - else -1, - ) + # Best-effort: a non-root process on a cross-uid bind + # mount lacks CAP_CHOWN; the previous in-place write + # never chowned, so a failure must not abort the save. + try: + os.fchown( + write_file.fileno(), + existing_metadata.st_uid + if temporary_metadata.st_uid != existing_metadata.st_uid + else -1, + existing_metadata.st_gid + if temporary_metadata.st_gid != existing_metadata.st_gid + else -1, + ) + except PermissionError as chown_error: + if logger is not None: + logger.debug( + f"[{pair}] Optuna {namespace} best params " + f"ownership preservation skipped: {chown_error!r}" + ) os.fchmod( write_file.fileno(), stat.S_IMODE(existing_metadata.st_mode), -- 2.53.0