Repository navigation
refactor(apple): name runner lease compatibility facts - #3445
Conversation
Each optional lease field now states the release that started writing it and what its absence means; the device-claim gate reads through a named predicate that says when it can go. Remove the device-wide xcodebuild sweep. It matched only session names from before v0.17.1, which carry no owner token; every later launch is reached through the artifact path its lease recorded. runnerStartTime is always present after normalization, so the type no longer admits undefined.
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/platform-apple/src/runner/runner-lease.ts">
<violation number="1" location="packages/platform-apple/src/runner/runner-lease.ts:56">
P3: `classifyRunnerLease` still passes `ownerStartTime` to `classifyOwnerLiveness`, so absence of this field skips only the directory check, not PID-reuse detection. Describe the fallback as checking the PID and recorded start time.</violation>
<violation number="2" location="packages/platform-apple/src/runner/runner-lease.ts:223">
P2: The empty-lease early return drops the only sweep that could reach a stray xcodebuild whose lease file is absent, unreadable, or corrupt. `cleanupLeasedRunnerProcesses` is the sole remaining cleanup path and always requires a parsed lease with an artifact path, so a runner orphaned between xcodebuild launch and lease write (e.g. daemon SIGKILLed mid-startup), or a lease file that fails `normalizeRunnerLease`, is never signaled. This matches the PR's stated intent, but it is still a coverage regression worth confirming: previously `cleanupRunnerXcodebuildProcesses({ deviceId })` reclaimed that case device-wide, and `runner-disposal.ts` now early-returns without a `xctestrunPath` artifact pattern.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| await cleanup.cleanupRunnerXcodebuildProcesses({ deviceId }); | ||
| return; | ||
| } | ||
| if (state.type === 'empty') return; |
There was a problem hiding this comment.
P2: The empty-lease early return drops the only sweep that could reach a stray xcodebuild whose lease file is absent, unreadable, or corrupt. cleanupLeasedRunnerProcesses is the sole remaining cleanup path and always requires a parsed lease with an artifact path, so a runner orphaned between xcodebuild launch and lease write (e.g. daemon SIGKILLed mid-startup), or a lease file that fails normalizeRunnerLease, is never signaled. This matches the PR's stated intent, but it is still a coverage regression worth confirming: previously cleanupRunnerXcodebuildProcesses({ deviceId }) reclaimed that case device-wide, and runner-disposal.ts now early-returns without a xctestrunPath artifact pattern.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/platform-apple/src/runner/runner-lease.ts, line 223:
<comment>The empty-lease early return drops the only sweep that could reach a stray xcodebuild whose lease file is absent, unreadable, or corrupt. `cleanupLeasedRunnerProcesses` is the sole remaining cleanup path and always requires a parsed lease with an artifact path, so a runner orphaned between xcodebuild launch and lease write (e.g. daemon SIGKILLed mid-startup), or a lease file that fails `normalizeRunnerLease`, is never signaled. This matches the PR's stated intent, but it is still a coverage regression worth confirming: previously `cleanupRunnerXcodebuildProcesses({ deviceId })` reclaimed that case device-wide, and `runner-disposal.ts` now early-returns without a `xctestrunPath` artifact pattern.</comment>
<file context>
@@ -198,10 +220,7 @@ export async function prepareRunnerLeaseForStartup(
- await cleanup.cleanupRunnerXcodebuildProcesses({ deviceId });
- return;
- }
+ if (state.type === 'empty') return;
if (state.type === 'busy') {
if (isSameStateDirRunnerLease(state.lease)) {
</file context>
| ownerStartTime: string | null; | ||
| /** | ||
| * The owner's AGENT_DEVICE_STATE_DIR. Absent when the owner had none, and on leases written before | ||
| * v0.17.10; either way the owner is judged by its pid alone. |
There was a problem hiding this comment.
P3: classifyRunnerLease still passes ownerStartTime to classifyOwnerLiveness, so absence of this field skips only the directory check, not PID-reuse detection. Describe the fallback as checking the PID and recorded start time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/platform-apple/src/runner/runner-lease.ts, line 56:
<comment>`classifyRunnerLease` still passes `ownerStartTime` to `classifyOwnerLiveness`, so absence of this field skips only the directory check, not PID-reuse detection. Describe the fallback as checking the PID and recorded start time.</comment>
<file context>
@@ -39,47 +39,71 @@ export function runnerOwnerToken(): string {
ownerStartTime: string | null;
+ /**
+ * The owner's AGENT_DEVICE_STATE_DIR. Absent when the owner had none, and on leases written before
+ * v0.17.10; either way the owner is judged by its pid alone.
+ */
ownerStateDir?: string;
</file context>
| * v0.17.10; either way the owner is judged by its pid alone. | |
| * v0.17.10; either way liveness uses the PID and recorded start time, without a state-directory check. |
|
This PR is ready to merge at 3d554ab. I found no problems in the code, and CI is green with 19 of 19 checks passing. I did not run tests or typecheck locally and relied on CI. No conflicts. Nothing else blocks the merge. Not blocking: you can take or leave these. Startup no longer runs the On the open review threads, one still applies and is a wording fix only. The owner comment at runner-lease.ts:56 says "pid alone", but PID-reuse detection also uses the start time (#3445 (comment)). One does not apply, so please resolve it: the removed sweep only matched pre-v0.17.1 names, so no supported release loses cleanup (#3445 (comment)). I could not tell whether any production path writes a lease whose |
thymikee
added
the
ready-for-human
|
1 participant
Summary
RunnerLeasefield documents the release that started writing it and what its absence means. Fallbacks that exist only for older releases say when they can be deleted (leaseOwnerHoldsDeviceClaimsfor < v0.21.0,leasedRunnerEndpointfor < v0.21.25).buildRunnerSessionXctestrunDeviceCleanupPattern). It matched onlysession-<device>-<port>names from before v0.17.1 (no owner token), so it never reached a launch from a later release. Cleanup now always targets the artifact path a lease recorded; an empty lease signals nothing.runnerStartTimeisstring | null(normalization already turned absence intonull).Validation
pnpm check:affected --run: pass (406 test files).