fix(sdk/python/core): honor kms_cache_file_name override in KSMCache cache path (KSM-1019)

[mgallego-keeper]

Jira: KSM-1019 · Fixes #1044

Summary

PR #1034 (KSM-1004) rerouted KSMCache.save_cache / get_cached_data / remove_cache_file through a new get_cache_file_path() that re-derives the path from KSM_CACHE_DIR and no longer reads the kms_cache_file_name class attribute. As a result, assigning KSMCache.kms_cache_file_name = "/custom/path.bin" was silently ignored — a backward-incompatible change for consumers that set it (and it was the only way to customize the cache filename, since KSM_CACHE_DIR only controls the directory).

Changes

• core.py — get_cache_file_path() returns an explicitly-overridden kms_cache_file_name (detected via an import-time default snapshot), otherwise resolves KSM_CACHE_DIR lazily at call time. KSM-1004: resolve KSMCache path lazily so KSM_CACHE_DIR is honored after import #1034's lazy behavior is preserved; the three cache methods already route through this method, so no caller changes were needed.
• tests/cache_test.py — add test_kms_cache_file_name_override_is_honored.
• Version bump 17.3.0 → 17.3.1 across the three coupled sources: _version.py, the keeper_globals.py hardcoded fallback (version_revision_default), and the smoke_test.py version assertions.

Behavior (verified)

Scenario	Result
Explicit kms_cache_file_name override	honored (regression fixed)
No override, no env var	ksm_cache.bin in CWD (unchanged)
KSM_CACHE_DIR set after import, attribute untouched	honored (KSM-1004 preserved)

Testing

cd sdk/python/core && python -m pytest tests/ → 127 passed, 1 skipped locally.

Note: the Test-Python workflow only triggers on PRs targeting master, so it will not auto-run against this release branch — the suite was run locally instead.

Target

Point release on the v17.3.x line; base branch release/sdk/python/core/v17.3.1 (branched from the v17.3.0 release).

[stas-schaller]

Good work, two things before we go forward: README.md still ends at ### 17.3.0, needs a ### 17.3.1 entry.

[mgallego-keeper]

Thanks for the review — all three points addressed in 6e590b0:

• core.py — override detection now uses object identity against a _DefaultCachePath(str) sentinel instead of value-equality, so an override whose text equals the default is no longer silently dropped.
• cache_test.py — added the collision-case regression test (fails on the old != code, passes after the fix).
• README.md — added the ### 17.3.1 changelog entry above ### 17.3.0.

Full suite green locally: python -m pytest tests/ → 128 passed, 1 skipped. Re-requesting review.

[stas-schaller]

Good progress on the identity-sentinel approach, and both of my previous comments are addressed. Three more things before this can merge.

1. Rewrite the PR description. The body still describes a 17.3.1 point release with a version bump across _version.py, keeper_globals.py, and smoke_test.py. None of that is in this PR — the diff is two files and the version reads 17.3.0 at head. The PR was retargeted to release/sdk/python/core/v17.4.0 and the last commit deliberately dropped the version bump, which is the right call, but the description was never updated to match. Please rewrite it to reflect what the PR actually does: fix lands in 17.4.0, no version bump included here. Also correct the test count from 127 to 128 (the second test was added after the description was written).

2. Extract the duplicated default-path expression (inline comment below).

3. Fix the misleading restore-default note and add a restore test (inline comment below).

[stas-schaller]

This expression is identical to the one on line 1924 that builds _default_cache_file_name. The identity check works because the sentinel and the lazy branch are two separate objects, but the correctness of that check depends on both expressions producing the same text. Change the filename in one place and they silently disagree, with no test catching the drift.

Extract a module-level helper so there is one source of truth:

def _default_cache_path():
    return os.path.join(os.environ.get("KSM_CACHE_DIR", ""), 'ksm_cache.bin')

class _DefaultCachePath(str):
    pass

class KSMCache:
    _default_cache_file_name = _DefaultCachePath(_default_cache_path())
    kms_cache_file_name = _default_cache_file_name

    @classmethod
    def get_cache_file_path(cls):
        if cls.kms_cache_file_name is not cls._default_cache_file_name:
            return cls.kms_cache_file_name
        return _default_cache_path()

[stas-schaller]

The note (Re-assign _default_cache_file_name to restore the default.) reads as an instruction to modify _default_cache_file_name itself, which would corrupt the sentinel for every future caller. The intent is the opposite: reassign kms_cache_file_name back to the sentinel.

Change to: (To restore the default, assign KSMCache.kms_cache_file_name = KSMCache._default_cache_file_name.)

Also worth adding a test that exercises this restore path, since the comment describes it and nothing currently locks in the correct way to do it:

def test_kms_cache_file_name_override_can_be_restored_to_default(self):
    original = KSMCache.kms_cache_file_name
    KSMCache.kms_cache_file_name = "/some/custom/path.bin"
    KSMCache.kms_cache_file_name = KSMCache._default_cache_file_name
    try:
        # After restore, lazy env resolution must work again.
        with unittest.mock.patch.dict(os.environ, {"KSM_CACHE_DIR": "/lazy/dir"}):
            self.assertEqual(
                KSMCache.get_cache_file_path(),
                "/lazy/dir/ksm_cache.bin",
                "restoring kms_cache_file_name to _default_cache_file_name should re-enable lazy KSM_CACHE_DIR resolution",
            )
    finally:
        KSMCache.kms_cache_file_name = original