Conversation
#2833: on a host shared by several agents, a session that never runs `close` keeps its host-global device claim until the daemon stops, and every other agent reads DEVICE_IN_USE with no way to tell active from abandoned. Set AGENT_DEVICE_SESSION_IDLE_TIMEOUT_MS to expire a claim-holding session with no remote lease and no capture running for it, once it has taken no attached commands for that long. The daemon that owns the session settles it under that session's own execution lock pair, releases the claim, and leaves a bounded marker so the next command answers SESSION_NOT_FOUND with details.reason SESSION_IDLE_EXPIRED naming the window and the released device. Off by default. A settle that cannot confirm its claim gone holds the session record back and retries a window later: this daemon stays alive, so forgetting the record would strand a claim owned by a process that no longer knows what it holds, which `device release --stale` cannot reclaim. `clearDeviceClaim` therefore gains `unattributable` for a record that yields no attributable owner, because that says nothing about a successor having taken the device. A teardown budget bounds only how long a sweep waits, never the release itself, and the reaper keeps tracking the release after it stops waiting, so one stuck recorder cannot invite a second teardown of the same session.
…field R7 holds every SessionState field to one declared writer. The #2833 request path reports activity through `SessionStore.noteSessionActivity` rather than mutating the record itself, so the store that owns the record is the only writer here and the field now says so.
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.
14 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/daemon/session-idle-tombstone.ts">
<violation number="1" location="src/daemon/session-idle-tombstone.ts:47">
P2: `readIdleSessionTombstoneFile` accepts non-finite timestamps and non-string `deviceKey` values. `JSON.parse('1e400')` produces `Infinity`, so a marker can shadow a session key forever; malformed device data also reaches the error details. Reject non-finite numbers and non-string devices before returning.</violation>
</file>
<file name="src/daemon/__tests__/session-store.test.ts">
<violation number="1" location="src/daemon/__tests__/session-store.test.ts:928">
P3: This test does not actually verify the write half of its claim. `readIdleExpiryTombstone('..')` returns `undefined` unconditionally because the read short-circuits on `isSafeSessionSegment` before any filesystem access (session-store.ts `readIdleExpiryTombstone`), so the assertion passes whether or not `writeIdleExpiryTombstone('..', ...)` leaked a marker file. The write is only safe today because `resolveSessionDir` throws before `mkdir`/`writeFileSync`; a regression that removed that guard would write `idle-expiry.json` outside the sessions dir and this test would stay green. Assert the marker is absent from the filesystem the traversal is guarded against, e.g. `fs.existsSync(path.join(root, '..', 'idle-expiry.json')) === false` (requires destructuring `root` from `makeFixture`).</violation>
</file>
<file name="src/commands/schema/cli-help.ts">
<violation number="1" location="src/commands/schema/cli-help.ts:705">
P2: The help text understates the expiry exclusions: active logs, audio probes, performance captures, and traces are also protected, not just active recordings. Document this as an active-capture exclusion so operators know which long-running captures keep a claim-holding session alive.</violation>
</file>
<file name="src/daemon/__tests__/session-idle-expiry.test.ts">
<violation number="1" location="src/daemon/__tests__/session-idle-expiry.test.ts:62">
P3: The isIdleExpirableSession exclusion policy is tested for only one of its six resource guards: appLog, audioProbe, perfCapture, and trace never appear in any test session. Deleting any of those `=== undefined` checks in session-idle-expiry.ts would leave this suite green — precisely the regression (expiring a live capture) the policy exists to prevent. Add one claim-holding case per resource and assert `false`, e.g. `claimHoldingSession({ trace: { outPath: '/tmp/trace.trace', startedAt: 1 } })`, and the same for appLog/audioProbe/perfCapture using the handle builders in src/__tests__/test-utils.</violation>
</file>
<file name="src/daemon/request-router.ts">
<violation number="1" location="src/daemon/request-router.ts:638">
P1: Tenant-isolated requests do not read their own idle tombstone here. `createRequestExecutionScope` prefixes `default` as `<tenant>:default`, but this helper resolves the raw request as `default`. Scope the request before resolving the tombstone key; otherwise the reason is missed and an unscoped marker can expose its `deviceKey`.</violation>
</file>
<file name="src/daemon/session-idle-expiry.ts">
<violation number="1" location="src/daemon/session-idle-expiry.ts:48">
P2: A positive sub-millisecond timeout is silently normalized to `0`, disabling idle expiry despite the positive-number opt-in contract. Clamp the floored value to at least `1` (or reject sub-millisecond values explicitly).</violation>
</file>
<file name="src/daemon/server/daemon-session-idle-expiry.test.ts">
<violation number="1" location="src/daemon/server/daemon-session-idle-expiry.test.ts:72">
P2: This fixture is still claim-bearing, so the test does not verify that sessions without a device claim are ignored. Remove `deviceClaim` from `settledSession()` so the `plain` case actually covers the intended branch.</violation>
</file>
<file name="src/daemon/__tests__/session-idle-activity.test.ts">
<violation number="1" location="src/daemon/__tests__/session-idle-activity.test.ts:148">
P3: This test claims to guard the reaper-vs-request lock discipline, but it never exercises the reaper: it compares `existingSessionExecutionLockKeys('default', 'sim-1')` against `requestPlan.keys`, and for an existing session `resolveRequestExecutionLockPlan` returns exactly `existingSessionExecutionLockKeys(sessionName, existingSession.device.id)` (src/daemon/request-binding.ts). Both sides are the same function with the same inputs, so any divergence in the reaper's actual lock keys (src/daemon/server/daemon-session-idle-expiry.ts builds its own pair at line 248) would still pass this test. Rename/reword the test to describe what it actually pins — that the request plan keeps the canonical pair — and derive the device id from the seeded session instead of hardcoding `'sim-1'`, otherwise the assertion fails for an unrelated reason if the fixture id changes.</violation>
</file>
<file name="src/daemon/server/daemon-runtime.ts">
<violation number="1" location="src/daemon/server/daemon-runtime.ts:439">
P2: Finalize repair teardown with the store address, not `session.name`; otherwise idle expiry of a scoped repair session publishes its script/tombstone where the next request cannot find it.</violation>
</file>
<file name="src/daemon/session-store.ts">
<violation number="1" location="src/daemon/session-store.ts:315">
P2: The idle-expiry marker is not actually keyed uniquely by the session address: `a/b` and `a_b` both resolve to the same session directory. An expiry for one session can therefore overwrite or explain a later `SESSION_NOT_FOUND` for the other with the wrong owner, timeout, and released device; use an injective session-directory encoding for the marker path and keep read/write/clear consistent.</violation>
</file>
<file name="src/daemon/server/daemon-session-idle-expiry.ts">
<violation number="1" location="src/daemon/server/daemon-session-idle-expiry.ts:242">
P2: A retry that finds the address still settling must defer its next due time or be excluded from `nextDueMs`; otherwise a stuck cleanup causes a continuous zero-delay sweep loop and CPU churn.</violation>
<violation number="2" location="src/daemon/server/daemon-session-idle-expiry.ts:336">
P2: When a settle exceeds `settleBudgetMs`, the enclosing `withRequestExecutionLockKeys` task resolves at budget expiry and both execution locks are released, even though the settle keeps tearing down — contradicting the module's invariant that "it keeps holding the session's execution lock until it actually finishes". A new request can then take those locks and run against the session mid-teardown (a retried `close` double-tears-down), and the late settle's `sessionStore.delete` can remove a fresh record a retried `open` re-created at the same address, then write an idle-expiry marker for it and orphan its claim. Keep the lock pair held by the settle itself (acquire it inside `settleExpiredSession`) so the budget detaches only the sweep's wait, not the settle's exclusivity.</violation>
<violation number="3" location="src/daemon/server/daemon-session-idle-expiry.ts:351">
P2: When an over-budget settle eventually succeeds, notify `onSessionExpired` from the detached completion path; otherwise a daemon whose last session expires after the budget can remain alive indefinitely.</violation>
<violation number="4" location="src/daemon/server/daemon-session-idle-expiry.ts:358">
P2: Use a cancelable, unref'ed timeout for the budget race so an idle-expiry timer cannot keep the daemon event loop alive after completion or during shutdown.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| // which is right here because this read is a question about an absent session, not a request to | ||
| // act through one. | ||
| return sessionStore.readIdleExpiryTombstone( | ||
| resolveEffectiveSessionName(req, sessionStore, { attachesToSession: false }), |
There was a problem hiding this comment.
P1: Tenant-isolated requests do not read their own idle tombstone here. createRequestExecutionScope prefixes default as <tenant>:default, but this helper resolves the raw request as default. Scope the request before resolving the tombstone key; otherwise the reason is missed and an unscoped marker can expose its deviceKey.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/request-router.ts, line 638:
<comment>Tenant-isolated requests do not read their own idle tombstone here. `createRequestExecutionScope` prefixes `default` as `<tenant>:default`, but this helper resolves the raw request as `default`. Scope the request before resolving the tombstone key; otherwise the reason is missed and an unscoped marker can expose its `deviceKey`.</comment>
<file context>
@@ -596,6 +604,44 @@ function repairExpiredIfTombstoned(
+ // which is right here because this read is a question about an absent session, not a request to
+ // act through one.
+ return sessionStore.readIdleExpiryTombstone(
+ resolveEffectiveSessionName(req, sessionStore, { attachesToSession: false }),
+ );
+ } catch {
</file context>
| } catch { | ||
| return undefined; | ||
| } | ||
| if (typeof parsed?.expiresAt !== 'number' || parsed.expiresAt <= Date.now()) return undefined; |
There was a problem hiding this comment.
P2: readIdleSessionTombstoneFile accepts non-finite timestamps and non-string deviceKey values. JSON.parse('1e400') produces Infinity, so a marker can shadow a session key forever; malformed device data also reaches the error details. Reject non-finite numbers and non-string devices before returning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/session-idle-tombstone.ts, line 47:
<comment>`readIdleSessionTombstoneFile` accepts non-finite timestamps and non-string `deviceKey` values. `JSON.parse('1e400')` produces `Infinity`, so a marker can shadow a session key forever; malformed device data also reaches the error details. Reject non-finite numbers and non-string devices before returning.</comment>
<file context>
@@ -0,0 +1,53 @@
+ } catch {
+ return undefined;
+ }
+ if (typeof parsed?.expiresAt !== 'number' || parsed.expiresAt <= Date.now()) return undefined;
+ if (typeof parsed.owner !== 'string') return undefined;
+ if (typeof parsed.expiredAtMs !== 'number' || typeof parsed.idleExpiryMs !== 'number') {
</file context>
| No runner read launches a session app that is not running: snapshot, wait, is, get, a reading find, and an interaction's leading reads (a gesture's viewport read, the capture that resolves a selector click/fill) answer the retriable APP_NOT_RUNNING instead of bare-launching over a launch SpringBoard still holds behind its deep-link confirmation. Only open, activate, and a command that mutates without a leading read bring a stopped app up. | ||
| close keeps a healthy iOS simulator XCTest runner warm by default so the next open on that simulator (same udid in the same simulator set) skips the runner build, unless --shutdown was requested, the session was recording, or the session held a device lease. A retained runner auto-stops after an idle window (default 5 minutes); set AGENT_DEVICE_IOS_RUNNER_IDLE_STOP_MS to override, or 0 to disable idle stop and retain until daemon exit. | ||
| Each AGENT_DEVICE_STATE_DIR runs its own daemon. It self-exits after an idle window (default 5 minutes, matching the runner idle-stop default) once it has no open sessions, no in-flight requests, and no active recording; set AGENT_DEVICE_DAEMON_IDLE_TIMEOUT_MS to override, or 0 to disable idle reap. | ||
| On a machine shared by several agents, set AGENT_DEVICE_SESSION_IDLE_TIMEOUT_MS to also expire an individual session that has taken a host-global device claim and then received no commands for that long; the claim is released and the next command on that session answers SESSION_NOT_FOUND with details.reason SESSION_IDLE_EXPIRED naming the window and the device. Off by default, because on a shared host "idle" and "thinking" are indistinguishable: only set it where an abandoned claim blocking every other agent is the worse failure. Sessions holding a remote lease or an active recording are never expired this way. |
There was a problem hiding this comment.
P2: The help text understates the expiry exclusions: active logs, audio probes, performance captures, and traces are also protected, not just active recordings. Document this as an active-capture exclusion so operators know which long-running captures keep a claim-holding session alive.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/commands/schema/cli-help.ts, line 705:
<comment>The help text understates the expiry exclusions: active logs, audio probes, performance captures, and traces are also protected, not just active recordings. Document this as an active-capture exclusion so operators know which long-running captures keep a claim-holding session alive.</comment>
<file context>
@@ -702,6 +702,7 @@ Runner and daemon lifecycle (applies to simulators too):
No runner read launches a session app that is not running: snapshot, wait, is, get, a reading find, and an interaction's leading reads (a gesture's viewport read, the capture that resolves a selector click/fill) answer the retriable APP_NOT_RUNNING instead of bare-launching over a launch SpringBoard still holds behind its deep-link confirmation. Only open, activate, and a command that mutates without a leading read bring a stopped app up.
close keeps a healthy iOS simulator XCTest runner warm by default so the next open on that simulator (same udid in the same simulator set) skips the runner build, unless --shutdown was requested, the session was recording, or the session held a device lease. A retained runner auto-stops after an idle window (default 5 minutes); set AGENT_DEVICE_IOS_RUNNER_IDLE_STOP_MS to override, or 0 to disable idle stop and retain until daemon exit.
Each AGENT_DEVICE_STATE_DIR runs its own daemon. It self-exits after an idle window (default 5 minutes, matching the runner idle-stop default) once it has no open sessions, no in-flight requests, and no active recording; set AGENT_DEVICE_DAEMON_IDLE_TIMEOUT_MS to override, or 0 to disable idle reap.
+ On a machine shared by several agents, set AGENT_DEVICE_SESSION_IDLE_TIMEOUT_MS to also expire an individual session that has taken a host-global device claim and then received no commands for that long; the claim is released and the next command on that session answers SESSION_NOT_FOUND with details.reason SESSION_IDLE_EXPIRED naming the window and the device. Off by default, because on a shared host "idle" and "thinking" are indistinguishable: only set it where an abandoned claim blocking every other agent is the worse failure. Sessions holding a remote lease or an active recording are never expired this way.
A stale iOS runner lease — its owner process dead, or its AGENT_DEVICE_STATE_DIR deleted — is reclaimed automatically instead of failing with "is already owned by another agent-device daemon". A live owner's runner is also reclaimed when the requesting daemon holds the host-global device claim for that device: claims are exclusive, so holding one proves the runner's owner released the device and merely kept the runner warm. The error remains only for owners outside claim arbitration (a pre-claims build, or daemons pointed at different claim stores).
</file context>
| On a machine shared by several agents, set AGENT_DEVICE_SESSION_IDLE_TIMEOUT_MS to also expire an individual session that has taken a host-global device claim and then received no commands for that long; the claim is released and the next command on that session answers SESSION_NOT_FOUND with details.reason SESSION_IDLE_EXPIRED naming the window and the device. Off by default, because on a shared host "idle" and "thinking" are indistinguishable: only set it where an abandoned claim blocking every other agent is the worse failure. Sessions holding a remote lease or an active recording are never expired this way. | |
| On a machine shared by several agents, set AGENT_DEVICE_SESSION_IDLE_TIMEOUT_MS to also expire an individual session that has taken a host-global device claim and then received no commands for that long; the claim is released and the next command on that session answers SESSION_NOT_FOUND with details.reason SESSION_IDLE_EXPIRED naming the window and the device. Off by default, because on a shared host "idle" and "thinking" are indistinguishable: only set it where an abandoned claim blocking every other agent is the worse failure. Sessions holding a remote lease or any active capture (recording, logs, audio, performance, or trace) are never expired this way. |
| if (!raw) return 0; | ||
| const parsed = Number(raw); | ||
| if (!Number.isFinite(parsed) || parsed <= 0) return 0; | ||
| return Math.floor(parsed); |
There was a problem hiding this comment.
P2: A positive sub-millisecond timeout is silently normalized to 0, disabling idle expiry despite the positive-number opt-in contract. Clamp the floored value to at least 1 (or reject sub-millisecond values explicitly).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/session-idle-expiry.ts, line 48:
<comment>A positive sub-millisecond timeout is silently normalized to `0`, disabling idle expiry despite the positive-number opt-in contract. Clamp the floored value to at least `1` (or reject sub-millisecond values explicitly).</comment>
<file context>
@@ -0,0 +1,180 @@
+ if (!raw) return 0;
+ const parsed = Number(raw);
+ if (!Number.isFinite(parsed) || parsed <= 0) return 0;
+ return Math.floor(parsed);
+}
+
</file context>
| return Math.floor(parsed); | |
| return Math.max(1, Math.floor(parsed)); |
| } | ||
|
|
||
| function settledSession(): SessionState { | ||
| return makeIosSession('default', { deviceClaim: { ...CLAIM } }); |
There was a problem hiding this comment.
P2: This fixture is still claim-bearing, so the test does not verify that sessions without a device claim are ignored. Remove deviceClaim from settledSession() so the plain case actually covers the intended branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/server/daemon-session-idle-expiry.test.ts, line 72:
<comment>This fixture is still claim-bearing, so the test does not verify that sessions without a device claim are ignored. Remove `deviceClaim` from `settledSession()` so the `plain` case actually covers the intended branch.</comment>
<file context>
@@ -0,0 +1,764 @@
+}
+
+function settledSession(): SessionState {
+ return makeIosSession('default', { deviceClaim: { ...CLAIM } });
+}
+
</file context>
| if (params.closing()) return undefined; | ||
| const notBefore = params.retryNotBeforeMs.get(address); | ||
| if (notBefore !== undefined && params.atMs < notBefore) return undefined; | ||
| if (params.settling.has(address)) return undefined; |
There was a problem hiding this comment.
P2: A retry that finds the address still settling must defer its next due time or be excluded from nextDueMs; otherwise a stuck cleanup causes a continuous zero-delay sweep loop and CPU churn.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/server/daemon-session-idle-expiry.ts, line 242:
<comment>A retry that finds the address still settling must defer its next due time or be excluded from `nextDueMs`; otherwise a stuck cleanup causes a continuous zero-delay sweep loop and CPU churn.</comment>
<file context>
@@ -0,0 +1,502 @@
+ if (params.closing()) return undefined;
+ const notBefore = params.retryNotBeforeMs.get(address);
+ if (notBefore !== undefined && params.atMs < notBefore) return undefined;
+ if (params.settling.has(address)) return undefined;
+ // The device the settling fences is read INSIDE the pair rather than from the swept reference:
+ // a session can only change device while its own session lock is free, which this pair holds
</file context>
| const outcome = | ||
| settleBudgetMs === undefined | ||
| ? await settle | ||
| : await Promise.race([settle, timedOutAfter(settleBudgetMs)]); |
There was a problem hiding this comment.
P2: When a settle exceeds settleBudgetMs, the enclosing withRequestExecutionLockKeys task resolves at budget expiry and both execution locks are released, even though the settle keeps tearing down — contradicting the module's invariant that "it keeps holding the session's execution lock until it actually finishes". A new request can then take those locks and run against the session mid-teardown (a retried close double-tears-down), and the late settle's sessionStore.delete can remove a fresh record a retried open re-created at the same address, then write an idle-expiry marker for it and orphan its claim. Keep the lock pair held by the settle itself (acquire it inside settleExpiredSession) so the budget detaches only the sweep's wait, not the settle's exclusivity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/server/daemon-session-idle-expiry.ts, line 336:
<comment>When a settle exceeds `settleBudgetMs`, the enclosing `withRequestExecutionLockKeys` task resolves at budget expiry and both execution locks are released, even though the settle keeps tearing down — contradicting the module's invariant that "it keeps holding the session's execution lock until it actually finishes". A new request can then take those locks and run against the session mid-teardown (a retried `close` double-tears-down), and the late settle's `sessionStore.delete` can remove a fresh record a retried `open` re-created at the same address, then write an idle-expiry marker for it and orphan its claim. Keep the lock pair held by the settle itself (acquire it inside `settleExpiredSession`) so the budget detaches only the sweep's wait, not the settle's exclusivity.</comment>
<file context>
@@ -0,0 +1,502 @@
+ const outcome =
+ settleBudgetMs === undefined
+ ? await settle
+ : await Promise.race([settle, timedOutAfter(settleBudgetMs)]);
+ if (outcome !== SETTLE_TIMED_OUT) {
+ rememberRetry(retryNotBeforeMs, address, expiredAtMs, idleExpiryMs, outcome);
</file context>
| expiresAt: Date.now() + 60_000, | ||
| idleExpiryMs: 1_000, | ||
| }); | ||
| assert.equal(store.readIdleExpiryTombstone('..'), undefined); |
There was a problem hiding this comment.
P3: This test does not actually verify the write half of its claim. readIdleExpiryTombstone('..') returns undefined unconditionally because the read short-circuits on isSafeSessionSegment before any filesystem access (session-store.ts readIdleExpiryTombstone), so the assertion passes whether or not writeIdleExpiryTombstone('..', ...) leaked a marker file. The write is only safe today because resolveSessionDir throws before mkdir/writeFileSync; a regression that removed that guard would write idle-expiry.json outside the sessions dir and this test would stay green. Assert the marker is absent from the filesystem the traversal is guarded against, e.g. fs.existsSync(path.join(root, '..', 'idle-expiry.json')) === false (requires destructuring root from makeFixture).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/__tests__/session-store.test.ts, line 928:
<comment>This test does not actually verify the write half of its claim. `readIdleExpiryTombstone('..')` returns `undefined` unconditionally because the read short-circuits on `isSafeSessionSegment` before any filesystem access (session-store.ts `readIdleExpiryTombstone`), so the assertion passes whether or not `writeIdleExpiryTombstone('..', ...)` leaked a marker file. The write is only safe today because `resolveSessionDir` throws before `mkdir`/`writeFileSync`; a regression that removed that guard would write `idle-expiry.json` outside the sessions dir and this test would stay green. Assert the marker is absent from the filesystem the traversal is guarded against, e.g. `fs.existsSync(path.join(root, '..', 'idle-expiry.json')) === false` (requires destructuring `root` from `makeFixture`).</comment>
<file context>
@@ -880,3 +880,50 @@ test('BLOCKER 3: finalizeRepairTeardown auto-commit records a terminal close, pr
+ expiresAt: Date.now() + 60_000,
+ idleExpiryMs: 1_000,
+ });
+ assert.equal(store.readIdleExpiryTombstone('..'), undefined);
+});
</file context>
| // evidence the daemon-process reap also honors. | ||
| const recording = claimHoldingSession(); | ||
| recording.screenRecording = makeTestScreenRecordingResource(recording); | ||
| assert.equal(isIdleExpirableSession(recording), false); |
There was a problem hiding this comment.
P3: The isIdleExpirableSession exclusion policy is tested for only one of its six resource guards: appLog, audioProbe, perfCapture, and trace never appear in any test session. Deleting any of those === undefined checks in session-idle-expiry.ts would leave this suite green — precisely the regression (expiring a live capture) the policy exists to prevent. Add one claim-holding case per resource and assert false, e.g. claimHoldingSession({ trace: { outPath: '/tmp/trace.trace', startedAt: 1 } }), and the same for appLog/audioProbe/perfCapture using the handle builders in src/tests/test-utils.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/__tests__/session-idle-expiry.test.ts, line 62:
<comment>The isIdleExpirableSession exclusion policy is tested for only one of its six resource guards: appLog, audioProbe, perfCapture, and trace never appear in any test session. Deleting any of those `=== undefined` checks in session-idle-expiry.ts would leave this suite green — precisely the regression (expiring a live capture) the policy exists to prevent. Add one claim-holding case per resource and assert `false`, e.g. `claimHoldingSession({ trace: { outPath: '/tmp/trace.trace', startedAt: 1 } })`, and the same for appLog/audioProbe/perfCapture using the handle builders in src/__tests__/test-utils.</comment>
<file context>
@@ -0,0 +1,118 @@
+ // evidence the daemon-process reap also honors.
+ const recording = claimHoldingSession();
+ recording.screenRecording = makeTestScreenRecordingResource(recording);
+ assert.equal(isIdleExpirableSession(recording), false);
+ assert.equal(isIdleExpirableSession(claimHoldingSession()), true);
+});
</file context>
| test('the reaper and a command on the same session take the identical lock pair in the same order', async () => { | ||
| const sessionStore = storeWithSession(); |
There was a problem hiding this comment.
P3: This test claims to guard the reaper-vs-request lock discipline, but it never exercises the reaper: it compares existingSessionExecutionLockKeys('default', 'sim-1') against requestPlan.keys, and for an existing session resolveRequestExecutionLockPlan returns exactly existingSessionExecutionLockKeys(sessionName, existingSession.device.id) (src/daemon/request-binding.ts). Both sides are the same function with the same inputs, so any divergence in the reaper's actual lock keys (src/daemon/server/daemon-session-idle-expiry.ts builds its own pair at line 248) would still pass this test. Rename/reword the test to describe what it actually pins — that the request plan keeps the canonical pair — and derive the device id from the seeded session instead of hardcoding 'sim-1', otherwise the assertion fails for an unrelated reason if the fixture id changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/__tests__/session-idle-activity.test.ts, line 148:
<comment>This test claims to guard the reaper-vs-request lock discipline, but it never exercises the reaper: it compares `existingSessionExecutionLockKeys('default', 'sim-1')` against `requestPlan.keys`, and for an existing session `resolveRequestExecutionLockPlan` returns exactly `existingSessionExecutionLockKeys(sessionName, existingSession.device.id)` (src/daemon/request-binding.ts). Both sides are the same function with the same inputs, so any divergence in the reaper's actual lock keys (src/daemon/server/daemon-session-idle-expiry.ts builds its own pair at line 248) would still pass this test. Rename/reword the test to describe what it actually pins — that the request plan keeps the canonical pair — and derive the device id from the seeded session instead of hardcoding `'sim-1'`, otherwise the assertion fails for an unrelated reason if the fixture id changes.</comment>
<file context>
@@ -0,0 +1,180 @@
+ expect(settledFirst).toBe(false);
+});
+
+test('the reaper and a command on the same session take the identical lock pair in the same order', async () => {
+ const sessionStore = storeWithSession();
+ const requestPlan = await resolveRequestExecutionLockPlan({
</file context>
| test('the reaper and a command on the same session take the identical lock pair in the same order', async () => { | |
| const sessionStore = storeWithSession(); | |
| test('the request plan for an existing session keeps the canonical session-first lock pair', async () => { | |
| const sessionStore = storeWithSession(); | |
| const session = sessionStore.get('default'); | |
| const requestPlan = await resolveRequestExecutionLockPlan({ | |
| req: makeRequest({ command: 'snapshot' }), | |
| sessionName: 'default', | |
| sessionStore, | |
| }); | |
| expect(existingSessionExecutionLockKeys(session.name, session.device.id)).toEqual(requestPlan.keys); | |
| }); |
|
Reviewed at a8a764b. This needs another pass before merge, mainly around lock lifetime and the shutdown ledger. In Following on from that: if a stuck settle outlives one retry window plus the budget, The new Not blocking: the tombstone lookup in Is there a simpler shape here? The linked issue asks to close an idle session "the normal way." Dispatching an internal I didn't re-run tsc, lint, layering, fallow, or the affected tests, and the PR body's validation claims are unverified from my side. The live simulator run described in the PR body doesn't say whether the The main things to resolve before merge: keep the session and device locks held until the settle finishes, with the budget only bounding the sweep's wait outside the locks; stop the zero-delay re-arm while a settle is still in flight; and scope the |
This branch has not been deployed
1 participant
Summary
Closes #2833
On a host shared by several agents, a session that never runs
closekeeps its host-global device claim until its daemon stops, and every other agent readsDEVICE_IN_USEwith no way to tell an active session from an abandoned one.AGENT_DEVICE_SESSION_IDLE_TIMEOUT_MS(off by default) expires a claim-holding session once it has taken no attached commands for that long. The owning daemon settles it under that session's own execution lock pair, releases the claim, and leaves a bounded marker so the next command answersSESSION_NOT_FOUNDwithdetails.reason: SESSION_IDLE_EXPIREDnaming the window and the released device.Sessions owning a remote lease are excluded, since their lease owns that session's ownership. So are sessions with a capture running —
record,logs start,audio start,perf start,traceall stamp once and then go silent while the capture runs.A settle that cannot confirm its claim gone holds the session record back and retries a window later. This daemon stays alive, so forgetting the record would strand a claim owned by a process that no longer knows what it holds — un-reclaimable by
device release --stale, which proves staleness from owner liveness.clearDeviceClaimtherefore gainsunattributablefor a record that yields no attributable owner, because that is not evidence a successor took the device.A teardown budget bounds only how long a sweep waits, never the release itself, and the reaper keeps tracking the release after it stops waiting, so one stuck recorder cannot invite a second teardown of the same session.
Validation
pnpm exec tsc --noEmit,pnpm format,pnpm lint,pnpm build,pnpm check:layering,pnpm check:affected --run(all runnable checks passed) andpnpm gate fallow/gate production-exportsgreen ata8a764b497. 23 files.Live run (own
AGENT_DEVICE_CLAIMS_DIR/STATE_DIR, disposable simulator, window 8000ms):openstood a real claim file; an attachedsnapshotre-stamped and the claim survived 24s on an 8s window; expiry then fired 8003ms after that command's end withclaim: "deleted", released the claim file, and wroteidle-expiry.json;closethen answeredSESSION_NOT_FOUND/SESSION_IDLE_EXPIREDnaming the device andAGENT_DEVICE_SESSION_IDLE_TIMEOUT_MS=0. Claims belonging to other worktrees were untouched. Simulator and both temp dirs deleted after.