fix: prevent transfer warning for non-owned leases#570
fix: prevent transfer warning for non-owned leases#570bennyz wants to merge 3 commits intojumpstarter-dev:mainfrom
Conversation
📝 WalkthroughWalkthroughAdded a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bennyz
force-pushed
the
fix-lease-transfer-error
branch
from
208de76 to
94ab62b
Compare
A user might receive a warning about a lease being transfer, even if the lease was never theirs to begin with:
[04/16/26 13:00:06] INFO [jumpstarter.client.lease] Waiting for ready connection at /var/folders/4m/llxcfbf1431fz8bjyr5cxtgr0000gn/T/jumpstarter-yk94a7bt/socket
WARNING Lease 019d95ac-b0d2-7155-9ee1-60c034b959f9 has been transferred to another client. Your session is no longer valid.
WARNING Lease 019d95ac-b0d2-7155-9ee1-60c034b959f9 has been transferred to another client. Your session is no longer valid.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Assisted-by: claude-opus-4.6
Signed-off by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
The bare --cov flag produces coverage.xml with an empty <source> element, making diff-cover unable to match coverage data to git diff paths. Change to --cov=. so each package's coverage.xml includes the correct source root, and remove the redundant bare --cov from PYTEST_ADDOPTS.
bennyz
force-pushed
the
fix-lease-transfer-error
branch
from
94ab62b to
e4b080a
Compare
![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
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/client/lease_test.py (1)
343-371: Good coverage of the ownership check; consider adding the empty-string case.The two tests nicely cover "different client" and "client_name is None". Given the production guard uses
if self.client_name and ...(truthy check), an additional case whereclient_name=""would document/lock behavior for a potentially-common misconfiguration (emptymetadata.name) and catch any future change tois not None.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py` around lines 343 - 371, Add a new test in TestRequestAsyncOwnership that covers the empty-string client_name case: create a lease via _make_lease(client_name=""), stub lease.get.return_value = Mock(client="other-client", selector=None) and lease._acquire = AsyncMock(return_value=lease), then await lease.request_async() and assert it returns the lease (i.e., ownership check is skipped). Name the test something like test_skips_check_when_client_name_is_empty_string and mark it with `@pytest.mark.anyio` so it mirrors test_skips_check_when_client_name_is_none and guards against future changes to the truthy check in Lease.request_async.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter/jumpstarter/config/client.py`:
- Line 332: The ownership check in Lease.request_async() currently uses a truthy
test on self.client_name which allows an empty string to bypass validation;
update the logic to explicitly reject empty client names by checking that
self.client_name is not None and not an empty string (e.g., self.client_name !=
"") or raise a validation error when metadata.name is empty before passing it to
client_name (where client_name=self.metadata.name is set); ensure the comparison
against existing_lease.client uses the explicit non-empty check so empty
metadata.name cannot reuse a lease.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/client/lease_test.py`:
- Around line 343-371: Add a new test in TestRequestAsyncOwnership that covers
the empty-string client_name case: create a lease via
_make_lease(client_name=""), stub lease.get.return_value =
Mock(client="other-client", selector=None) and lease._acquire =
AsyncMock(return_value=lease), then await lease.request_async() and assert it
returns the lease (i.e., ownership check is skipped). Name the test something
like test_skips_check_when_client_name_is_empty_string and mark it with
`@pytest.mark.anyio` so it mirrors test_skips_check_when_client_name_is_none and
guards against future changes to the truthy check in Lease.request_async.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4dcc6453-6214-4839-ac18-7480728fe0bc
📒 Files selected for processing (5)
.github/workflows/python-tests.yamlpython/packages/jumpstarter/jumpstarter/client/lease.pypython/packages/jumpstarter/jumpstarter/client/lease_test.pypython/packages/jumpstarter/jumpstarter/config/client.pypython/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- python/packages/jumpstarter/jumpstarter/client/lease.py
| release=release_lease, | ||
| tls_config=self.tls, | ||
| grpc_options=self.grpcOptions, | ||
| client_name=self.metadata.name, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm ObjectMeta.name default and any callers that could leave it empty.
rg -nP -C3 '\bclass\s+ObjectMeta\b'
rg -nP -C2 'ObjectMeta\('Repository: jumpstarter-dev/jumpstarter
Length of output: 36922
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/config/common.py | head -30Repository: jumpstarter-dev/jumpstarter
Length of output: 650
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/config/client.py | sed -n '320,350p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1454
🏁 Script executed:
rg -nP 'def request_async|def lease_async' python/packages/jumpstarter/jumpstarter/client/Repository: jumpstarter-dev/jumpstarter
Length of output: 165
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/client/lease.py | sed -n '166,220p'Repository: jumpstarter-dev/jumpstarter
Length of output: 2951
🏁 Script executed:
rg -nP 'if.*client_name.*and.*existing_lease' python/packages/jumpstarter/Repository: jumpstarter-dev/jumpstarter
Length of output: 209
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/config/common.py | sed -n '13,40p'Repository: jumpstarter-dev/jumpstarter
Length of output: 235
🏁 Script executed:
rg -nP 'class ObjectMeta|name.*=' python/packages/jumpstarter/jumpstarter/config/common.py | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 105
🏁 Script executed:
rg -nP 'model_validate|from_file' python/packages/jumpstarter/jumpstarter/config/client.py | head -5Repository: jumpstarter-dev/jumpstarter
Length of output: 257
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/config/common.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 650
🏁 Script executed:
rg -nP 'Field|validator|field_validator' python/packages/jumpstarter/jumpstarter/config/common.pyRepository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
rg -nP 'name.*=""' python/packages/jumpstarter/ | head -10Repository: jumpstarter-dev/jumpstarter
Length of output: 604
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/exporter/lease_context.py | sed -n '40,60p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1219
🏁 Script executed:
rg -nP 'class Lease' python/packages/jumpstarter/jumpstarter/client/lease.py | head -5Repository: jumpstarter-dev/jumpstarter
Length of output: 168
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/client/lease.py | sed -n '1,80p'Repository: jumpstarter-dev/jumpstarter
Length of output: 3324
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/client/lease.py | sed -n '78,130p'Repository: jumpstarter-dev/jumpstarter
Length of output: 3103
🏁 Script executed:
rg -nP 'class ClientConfigV1Alpha1|class ExporterConfigV1Alpha1' python/packages/jumpstarter/jumpstarter/config/Repository: jumpstarter-dev/jumpstarter
Length of output: 979
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/config/client.py | sed -n '110,160p'Repository: jumpstarter-dev/jumpstarter
Length of output: 2421
🏁 Script executed:
rg -nP 'metadata.*=' python/packages/jumpstarter/jumpstarter/config/client.py | head -20Repository: jumpstarter-dev/jumpstarter
Length of output: 135
🏁 Script executed:
rg -nP 'ObjectMeta\(' python/packages/jumpstarter/jumpstarter/config/client.py -B2 -A2Repository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
python3 << 'EOF'
from pydantic_settings import BaseSettings, SettingsConfigDict
class ObjectMeta(BaseSettings):
model_config = SettingsConfigDict(env_prefix="JMP_")
namespace: str | None
name: str
try:
obj = ObjectMeta()
print(f"ObjectMeta created with defaults: name={repr(obj.name)}, namespace={repr(obj.namespace)}")
except Exception as e:
print(f"Error creating ObjectMeta(): {type(e).__name__}: {e}")
try:
obj = ObjectMeta(name="test")
print(f"ObjectMeta with name: name={repr(obj.name)}, namespace={repr(obj.namespace)}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: jumpstarter-dev/jumpstarter
Length of output: 657
🏁 Script executed:
rg -nP 'class ObjectMeta|Field|validator' python/packages/jumpstarter/jumpstarter/config/common.py -A15Repository: jumpstarter-dev/jumpstarter
Length of output: 215
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/config/client.py | sed -n '315,345p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1492
🏁 Script executed:
rg -nP 'ObjectMeta\(' python/packages/jumpstarter/ --type py | grep -v test | head -10Repository: jumpstarter-dev/jumpstarter
Length of output: 162
🏁 Script executed:
rg -nP 'Field.*name|validator.*name' python/packages/jumpstarter/jumpstarter/config/ | grep -i metaRepository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
python3 << 'EOF'
from pydantic_settings import BaseSettings, SettingsConfigDict
class ObjectMeta(BaseSettings):
model_config = SettingsConfigDict(env_prefix="JMP_")
namespace: str | None
name: str
try:
obj = ObjectMeta(name="", namespace=None)
print(f"ObjectMeta created with empty name: name={repr(obj.name)}, namespace={repr(obj.namespace)}")
except Exception as e:
print(f"Error: {type(e).__name__}: {e}")
EOFRepository: jumpstarter-dev/jumpstarter
Length of output: 130
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/client/lease.py | sed -n '175,200p'Repository: jumpstarter-dev/jumpstarter
Length of output: 1452
🏁 Script executed:
python3 << 'EOF'
from pydantic_settings import BaseSettings, SettingsConfigDict
from pydantic import Field
class ObjectMeta(BaseSettings):
model_config = SettingsConfigDict(env_prefix="JMP_")
namespace: str | None
name: str
class TestConfig(BaseSettings):
metadata: ObjectMeta = Field(default_factory=ObjectMeta)
# Test 1: Can we create TestConfig with default ObjectMeta?
try:
config = TestConfig()
print(f"TestConfig() succeeded with metadata: {config.metadata}")
except Exception as e:
print(f"TestConfig() failed: {type(e).__name__}: {str(e)[:100]}")
# Test 2: Can we create with empty name via YAML-like dict?
try:
config = TestConfig.model_validate({"metadata": {"name": "", "namespace": None}})
print(f"TestConfig with empty name succeeded: name={repr(config.metadata.name)}")
except Exception as e:
print(f"TestConfig with empty name failed: {e}")
EOFRepository: jumpstarter-dev/jumpstarter
Length of output: 255
🏁 Script executed:
rg -nP 'from_file|model_validate' python/packages/jumpstarter/jumpstarter/config/client.py -B2 -A5 | head -40Repository: jumpstarter-dev/jumpstarter
Length of output: 1147
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/config/client_config_test.py | grep -A10 "ObjectMeta" | head -30Repository: jumpstarter-dev/jumpstarter
Length of output: 1705
🏁 Script executed:
rg -nP 'metadata:.*name:|name:' python/packages/jumpstarter/jumpstarter/config/ --type yaml -lRepository: jumpstarter-dev/jumpstarter
Length of output: 53
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/config/client.py | sed -n '300,340p'Repository: jumpstarter-dev/jumpstarter
Length of output: 2061
🏁 Script executed:
rg -nP 'metadata' python/packages/jumpstarter/jumpstarter/config/client.py | grep -i "field\|default"Repository: jumpstarter-dev/jumpstarter
Length of output: 135
🏁 Script executed:
cat -n python/packages/jumpstarter/jumpstarter/config/client.py | sed -n '110,125p'Repository: jumpstarter-dev/jumpstarter
Length of output: 822
🏁 Script executed:
python3 << 'EOF'
# Test if ObjectMeta can be instantiated via Field(default_factory=ObjectMeta)
from pydantic import Field, BaseModel
from pydantic_settings import BaseSettings, SettingsConfigDict
class ObjectMeta(BaseSettings):
model_config = SettingsConfigDict(env_prefix="JMP_")
namespace: str | None
name: str
class ClientConfig(BaseSettings):
metadata: ObjectMeta = Field(default_factory=ObjectMeta)
# Can we instantiate without metadata?
try:
config = ClientConfig()
print(f"✗ ClientConfig() succeeded: {config.metadata}")
except Exception as e:
print(f"✓ ClientConfig() failed (expected): name and namespace required")
# Can we provide empty name via YAML-like input?
try:
config = ClientConfig.model_validate({"metadata": {"name": "", "namespace": "default"}})
print(f"✓ Empty name allowed: name={repr(config.metadata.name)}")
print(f" This would pass empty string to client_name parameter")
except Exception as e:
print(f"✗ Failed: {e}")
EOFRepository: jumpstarter-dev/jumpstarter
Length of output: 220
Tighten the ownership check to reject empty client_name.
The ownership validation in Lease.request_async() at line 179 uses a truthy check: if self.client_name and existing_lease.client != self.client_name:. Since ObjectMeta.name is typed as str (not str | None), it can be instantiated with an empty string via YAML or config input. When self.metadata.name is empty and passed as client_name at line 332, the ownership check silently no-ops (empty string is falsy), allowing a lease to be reused without validation. Change the check to explicitly reject empty client names, either by using is not None/!= "" or raising a validation error on empty metadata.name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter/jumpstarter/config/client.py` at line 332, The
ownership check in Lease.request_async() currently uses a truthy test on
self.client_name which allows an empty string to bypass validation; update the
logic to explicitly reject empty client names by checking that self.client_name
is not None and not an empty string (e.g., self.client_name != "") or raise a
validation error when metadata.name is empty before passing it to client_name
(where client_name=self.metadata.name is set); ensure the comparison against
existing_lease.client uses the explicit non-empty check so empty metadata.name
cannot reuse a lease.
2 participants
A user might receive a warning about a lease being transfer, even if the lease was never theirs to begin with:
[04/16/26 13:00:06] INFO [jumpstarter.client.lease] Waiting for ready connection at /var/folders/4m/llxcfbf1431fz8bjyr5cxtgr0000gn/T/jumpstarter-yk94a7bt/socket
WARNING Lease 019d95ac-b0d2-7155-9ee1-60c034b959f9 has been transferred to another client. Your session is no longer valid.
WARNING Lease 019d95ac-b0d2-7155-9ee1-60c034b959f9 has been transferred to another client. Your session is no longer valid.