Conversation
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.
1 issue 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-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>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
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 |
a8a764b to
ae5d445
Compare
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Fix all with cubic | Re-trigger cubic
ae5d445 to
9ad0a2b
Compare
|
This is a follow-up on the earlier review (a8a764b, #2988 (comment)), reviewed at 9ad0a2b. Finding 3 from that round is still open and hasn't been answered. clearDeviceClaim (src/daemon/device/device-claims.ts:362-368) now returns 'unattributable' when a conflicting record has no decoded claim, meaning it's allocator-held or undecodable; on main this case returned 'ownership-changed'. The shutdown ledger's switch (https://github.com/callstack/agent-device/blob/9ad0a2b/src/daemon/server/daemon-shutdown-claims.ts#L74) only lists deleted, absent, and ownership-changed, so 'unattributable' falls to Not blocking: the comment above sessionStore.delete at src/daemon/server/daemon-session-idle-expiry.ts#L479 still says a remover can get there after a settle budget already released the lock and left the task running unheld, but the delta makes that impossible since the lock pair now outlives the budget and daemon shutdown is the only lock-free remover, so that clause can be dropped, and this can be taken or left. CI is green, all 19 checks pass, so there's no failing job to attribute here. I did not run tsc, lint, layering, fallow, or the affected tests, and judged regression validity by reading the pre-delta a8a764b code against each new test. The PR body doesn't name the commit of the live simulator run, doesn't say whether the daemonLeaving:false finalize stopped the iOS runner, and doesn't say whether any settle exceeded the teardown budget on a real device; the over-budget lock-hold path is covered only by unit tests. I didn't verify that isSafeSessionSegment accepts the tenant-scoped address form ('tenant-a:idle-x') outside the added router test. I also didn't examine how long a returning client's request queues behind a stuck release that now holds the lock pair; that's the intended trade-off the prior review asked for, and I didn't measure it. Before this can merge, 'unattributable' needs to be classified explicitly in the shutdown claim ledger and pinned with a test, so |
9ad0a2b to
ca9f8ff
Compare
Size justification~1150 net production lines crosses the guide's ~700 threshold, so this went to an independent design review asking whether a smaller owning interface would suffice. It would not, and the two masses that make it big are both load-bearing: Eager reclaim cannot be replaced by a lazy answer. A ~50-line design (activity field + error graft on The lock/budget/deferral machinery is the feature, not its scaffolding. Settling outside the session+device lock pair either releases a device under a command that still holds it or deadlocks against a request holding the other key. Bounding the wait without tracking the expiry runs two teardowns of one session; bounding it with the locks detached lets a retried Two reductions the review proposed were taken: repair finalization moved to the commit point (see the review-round comment), and the retry clock's threading collapsed to stamping at remember-time, which removed a parameter path across three functions and fixed a defect. What the review offered as remaining candidates are not reductions. |
Findings fixed after the review round (
|
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
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/server/daemon-shutdown-claims.ts">
<violation number="1" location="src/daemon/server/daemon-shutdown-claims.ts:60">
P3: The new `CLEAR_UNRECORDED` branch (sentinel set here and classified at `case CLEAR_UNRECORDED:`) has no test: `daemon-shutdown-claims.test.ts` only covers released, failed-teardown, superseded, undecodable ('unattributable'), and no-claim sessions, and `releaseClaim` has no other callers. Add a test that forces `clearDeviceClaim` to reject (e.g., `vi.mock`/spy the module, or arrange claim file state so `fs.unlinkSync` throws a non-ENOENT error) and asserts the session lands in `orphaned` via the sentinel.</violation>
</file>
<file name="src/daemon/server/daemon-session-idle-expiry.ts">
<violation number="1" location="src/daemon/server/daemon-session-idle-expiry.ts:479">
P3: `finalizeRepairTeardown` runs before the `delete` guard, so a settle whose delete reports false (superseded by shutdown) has already published the healed `.ad`, possibly written a repair tombstone, and stamped COMMITTED before learning the session was ended by someone else. Today that is harmless because the only racing remover (daemon shutdown at daemon-runtime.ts:200) finalizes the same live object idempotently, but it contradicts the settle's documented contract that "a failed settle... changes nothing" — this path returns `undefined` yet changes the record and publishes. Placing the finalize after the `delete` guard keeps the one-way commit conditional on this settle actually ending the session.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| outcomes.set(session.name, await clearDeviceClaim(session.deviceClaim)); | ||
| } catch (error) { | ||
| // An unrecorded outcome stays orphaned: the claim may still be on disk. | ||
| outcomes.set(session.name, CLEAR_UNRECORDED); |
There was a problem hiding this comment.
P3: The new CLEAR_UNRECORDED branch (sentinel set here and classified at case CLEAR_UNRECORDED:) has no test: daemon-shutdown-claims.test.ts only covers released, failed-teardown, superseded, undecodable ('unattributable'), and no-claim sessions, and releaseClaim has no other callers. Add a test that forces clearDeviceClaim to reject (e.g., vi.mock/spy the module, or arrange claim file state so fs.unlinkSync throws a non-ENOENT error) and asserts the session lands in orphaned via the sentinel.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/server/daemon-shutdown-claims.ts, line 60:
<comment>The new `CLEAR_UNRECORDED` branch (sentinel set here and classified at `case CLEAR_UNRECORDED:`) has no test: `daemon-shutdown-claims.test.ts` only covers released, failed-teardown, superseded, undecodable ('unattributable'), and no-claim sessions, and `releaseClaim` has no other callers. Add a test that forces `clearDeviceClaim` to reject (e.g., `vi.mock`/spy the module, or arrange claim file state so `fs.unlinkSync` throws a non-ENOENT error) and asserts the session lands in `orphaned` via the sentinel.</comment>
<file context>
@@ -42,7 +57,7 @@ export function createDaemonShutdownClaimLedger(): DaemonShutdownClaimLedger {
outcomes.set(session.name, await clearDeviceClaim(session.deviceClaim));
} catch (error) {
- // An unrecorded outcome stays orphaned: the claim may still be on disk.
+ outcomes.set(session.name, CLEAR_UNRECORDED);
emitDiagnostic({
level: 'warn',
</file context>
There was a problem hiding this comment.
Added in 3f1b9e7: a claim clear that throws is reported orphaned, the one bucket with a working remedy.
It pins the sentinel the way you suggested second — arranging claim-file state so the unlink throws a non-ENOENT error — by spying fs.unlinkSync to throw EACCES inside the claim lock. That is exactly the branch releaseClaim's catch writes CLEAR_UNRECORDED for, and it is what your first suggestion (vi.mock/spy the module) also lands on; spying fs keeps it below the module seam so the real clearDeviceClaim still runs.
I measured the chmod variant you implied rather than guessing, and it does reach the sentinel (4ms, orphaned), so it would have worked. I still preferred the spy because the chmod reaches it through acquireProcessLock's mkdirSync on resolveDeviceClaimPath(key) + '.lock' — that is, through the lock, not through the clear. The assertion would then pass for a fault in lock acquisition, which is a different invariant than "the clear reported no verdict". The spy puts the fault on the unlink itself, inside the held lock.
The test asserts the full four-bucket shape, so the sentinel cannot migrate to another bucket silently. Its title is deliberately the inverse of the new unattributable case: orphaned is kept precisely because our own owner identity does die with the exiting daemon, which is the one proof --stale can still use here.
| // stamps COMMITTED onto the record, and a write onto an already-committed transaction is an | ||
| // idempotent no-op. Finalizing a settle that is being held back would therefore mark a still-live | ||
| // session's healed script as already published, and no later teardown would ever publish it. | ||
| params.sessionStore.finalizeRepairTeardown(session); |
There was a problem hiding this comment.
P3: finalizeRepairTeardown runs before the delete guard, so a settle whose delete reports false (superseded by shutdown) has already published the healed .ad, possibly written a repair tombstone, and stamped COMMITTED before learning the session was ended by someone else. Today that is harmless because the only racing remover (daemon shutdown at daemon-runtime.ts:200) finalizes the same live object idempotently, but it contradicts the settle's documented contract that "a failed settle... changes nothing" — this path returns undefined yet changes the record and publishes. Placing the finalize after the delete guard keeps the one-way commit conditional on this settle actually ending the session.
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 479:
<comment>`finalizeRepairTeardown` runs before the `delete` guard, so a settle whose delete reports false (superseded by shutdown) has already published the healed `.ad`, possibly written a repair tombstone, and stamped COMMITTED before learning the session was ended by someone else. Today that is harmless because the only racing remover (daemon shutdown at daemon-runtime.ts:200) finalizes the same live object idempotently, but it contradicts the settle's documented contract that "a failed settle... changes nothing" — this path returns `undefined` yet changes the record and publishes. Placing the finalize after the `delete` guard keeps the one-way commit conditional on this settle actually ending the session.</comment>
<file context>
@@ -473,6 +469,14 @@ async function settleExpiredSession(params: {
+ // stamps COMMITTED onto the record, and a write onto an already-committed transaction is an
+ // idempotent no-op. Finalizing a settle that is being held back would therefore mark a still-live
+ // session's healed script as already published, and no later teardown would ever publish it.
+ params.sessionStore.finalizeRepairTeardown(session);
// `delete` reports whether a record was still here to remove. The usual request-path removers —
// `close`, a replacing `open`, a lease-expiry teardown — all remove a session from inside
</file context>
There was a problem hiding this comment.
Not moving it, but you are right that the comment above it was wrong. Corrected in 3f1b9e7.
The suggested placement is not safe. finalizeRepairTeardown does not only write files under the session dir: for a committable transaction it calls recordRepairFinalizeCloseIfCommitting → recordAction → resolveStoredSessionName(session) (src/daemon/session-store.ts:433), which finds the store address by map identity and falls back to session.name once the record is gone. Run after delete, that fallback names the wrong artifact directory for any session whose store address is tenant- or cwd-scoped — so the placement you propose moves the commit to the wrong path precisely in the case the guard detects.
And the cost of keeping it where it is, is nil: the only remover that can win this race is daemon shutdown, which tears the session set down by finalizing the same live SessionState object, and a write onto an already-committed transaction is the idempotent no-op described. Verified by mutation: moving the finalize below the delete guard changes no test outcome; moving it above the claim gate fails the held-back test. The gate worth pinning is the claim gate, and it is pinned.
What was genuinely stale is the prose, and I fixed the sentence you and the review both caught: a settle budget bounds only the sweep's wait, never the lock pair, so the "any of those three after a settle budget already released this lock" remover cannot happen. The comment now says shutdown is the only way here, and records why publishing precedes the guard. The settleExpiredSession header's "changes nothing, not even the session record" was overclaiming for the same reason and now describes the claim/record invariant it actually guarantees.
ca9f8ff to
1e92484
Compare
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
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/__tests__/session-idle-expiry-harness.ts">
<violation number="1" location="src/daemon/__tests__/session-idle-expiry-harness.ts:113">
P2: `runUntilIdle` only sleeps a fixed `ms` after triggering the sweep; it never actually waits for the expiry to complete. The sweep is scheduled with a real `setTimeout(..., 0)` in `arm()` and its settle chain performs real filesystem work (`withDeviceClaimLock`/`acquireProcessLock`, claim unlink, record delete, tombstone write), so the downstream assertions in both test files (`claimFileHeld(...) === false`, `sessionStore.get(...) === undefined`, tombstone existence) can run before the settle finishes and flake under CI load. This also runs against the repo's explicit policy "unit tests must not wait real time" (vitest.config.ts), enforced by `slowTestThreshold: 500` and the ratcheting slow-test reporter — the two new test files together add ~20 fixed 10–60 ms sleeps. Await completion instead: poll for the expected outcome with an overall budget, or await a settle-completion signal (the `settleSession` seam already returns a promise), or drive the controller with fake timers via `vi.advanceTimersByTimeAsync` as the long-window test already does.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| claimFileHeld: (deviceClaim) => fs.existsSync(resolveDeviceClaimPath(deviceClaim.deviceKey)), | ||
| runUntilIdle: (controller, ms) => { | ||
| controller.noteSessionsChanged(); | ||
| return new Promise((resolve) => setTimeout(resolve, ms)); |
There was a problem hiding this comment.
P2: runUntilIdle only sleeps a fixed ms after triggering the sweep; it never actually waits for the expiry to complete. The sweep is scheduled with a real setTimeout(..., 0) in arm() and its settle chain performs real filesystem work (withDeviceClaimLock/acquireProcessLock, claim unlink, record delete, tombstone write), so the downstream assertions in both test files (claimFileHeld(...) === false, sessionStore.get(...) === undefined, tombstone existence) can run before the settle finishes and flake under CI load. This also runs against the repo's explicit policy "unit tests must not wait real time" (vitest.config.ts), enforced by slowTestThreshold: 500 and the ratcheting slow-test reporter — the two new test files together add ~20 fixed 10–60 ms sleeps. Await completion instead: poll for the expected outcome with an overall budget, or await a settle-completion signal (the settleSession seam already returns a promise), or drive the controller with fake timers via vi.advanceTimersByTimeAsync as the long-window test already does.
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-harness.ts, line 113:
<comment>`runUntilIdle` only sleeps a fixed `ms` after triggering the sweep; it never actually waits for the expiry to complete. The sweep is scheduled with a real `setTimeout(..., 0)` in `arm()` and its settle chain performs real filesystem work (`withDeviceClaimLock`/`acquireProcessLock`, claim unlink, record delete, tombstone write), so the downstream assertions in both test files (`claimFileHeld(...) === false`, `sessionStore.get(...) === undefined`, tombstone existence) can run before the settle finishes and flake under CI load. This also runs against the repo's explicit policy "unit tests must not wait real time" (vitest.config.ts), enforced by `slowTestThreshold: 500` and the ratcheting slow-test reporter — the two new test files together add ~20 fixed 10–60 ms sleeps. Await completion instead: poll for the expected outcome with an overall budget, or await a settle-completion signal (the `settleSession` seam already returns a promise), or drive the controller with fake timers via `vi.advanceTimersByTimeAsync` as the long-window test already does.</comment>
<file context>
@@ -0,0 +1,116 @@
+ claimFileHeld: (deviceClaim) => fs.existsSync(resolveDeviceClaimPath(deviceClaim.deviceKey)),
+ runUntilIdle: (controller, ms) => {
+ controller.noteSessionsChanged();
+ return new Promise((resolve) => setTimeout(resolve, ms));
+ },
+ };
</file context>
There was a problem hiding this comment.
Fixed in 3f1b9e7, using the barrier option rather than polling everywhere.
runUntilIdle's fixed sleep is no longer load-bearing on any assertion that a settle landed. Both files now drive completion through withinDiagnosticsScope: the scope wraps a sweep and awaits it, and a sweep configured with no settle budget awaits every session's settle to the end, so a scope whose run() resolved is a sweep whose work is finished. createSweepBarrier in the harness exposes that as await sweeps.swept(n).
Two cases cannot use it and are handled honestly:
- Where a settle budget is configured, the scope resolves when the wait ends, not when the release does, so the barrier would resolve early. Those poll the outcome they assert via
waitFor. - Where the assertion is that something did not happen, there is no fact to poll. Those keep a bounded wait, but each now says so at the call site and is paired with a completion assertion that proves the loop was live — e.g. the lock tests assert the settle count is 0 while held and then await the settle that happens after the lock is released, so the zero means "waited", not "never started". I had to learn this the hard way: a first cut converted one such bound to
sweeps.swept(1)and it passed against a sweep that had merely skipped the address, so the barrier now waits for the settle itself there.
Inventory: the two files went from 43 fixed sleeps (16 + 27) to 16 bounded waits, and each of those now guards an assertion that something did not happen (settleCalls === 0, expired still empty, acquiredWhileStuck === false, "and it stays that way" after cancel()). Every assertion that something arrived is preceded by a barrier or a waitFor, so none of them can be reading a half-finished settle. Slowest test went 470ms → 111ms, and the ~20ms-margin "outlives its window" test is now fake-timer driven (vi.advanceTimersByTimeAsync) and has moved to the scheduling file, where it belongs by the file headers. It still fails on the pre-fix ordering, which I checked by routing both rememberRetry calls through a clock captured before the locks.
|
Reviewed at 1e92484. This is a follow-up to the earlier review (#2988 (comment)). The fix is only half done. The new test in src/daemon/server/daemon-shutdown-claims.test.ts#L100 writes CI is green across all 19 checks. I did not run tsc, the affected tests, or the compile-fails-on-new-member mutation; exhaustiveness was judged by reading the A few non-blocking notes: the "held-back settle leaves a repair transaction uncommitted" test at daemon-session-idle-expiry.test.ts#L267 stubs Before merge, |
140a0e3 to
6453799
Compare
#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 positive window below one millisecond is the shortest window, not off. 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. The lock pair belongs to the release, not to the sweep's wait for it. A teardown budget bounds only that wait, so one stuck recorder cannot hang a sweep while a budget releasing the locks would let a retried `close` join a teardown in progress, or a late release delete a session a retried `open` just created and mark it expired. A sweep that finds a release in flight defers that address rather than re-arming at a deadline already past, and a release that lands after its budget still reports the session it freed.
…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.
6453799 to
6eeab91
Compare
|
Re-reviewed and pushed as The blocking item is fixed, with the dedicated bucket rather than
Both cases you asked for are pinned: the undecodable Also corrected, each in its thread: the One thing I got wrong while working and corrected rather than shipped: I first justified the Gates at I did not run a fresh live device pass this round. The change is the shutdown-bucket reporting plus test timing, and the allocator-held and undecodable records cannot be produced by a normal simulator workflow — the unit tests cover them through the real claim store and the real decoder. Say the word if you want the simulator pass repeated anyway before merge. |
There was a problem hiding this comment.
3 issues found across 12 files (changes from recent commits).
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/__tests__/daemon-shutdown-report.test.ts">
<violation number="1" location="src/daemon/__tests__/daemon-shutdown-report.test.ts:30">
P3: No test round-trips the new `unattributable` bucket with actual records: the write/read cases all use `unattributable: []`, and the non-empty values elsewhere (device-claims / daemon-shutdown-claims tests) only exercise the producer, not the report serialization. Put a claim in `unattributable` on one write+read pair so the new bucket's serialization (write spread + `readClaimSection` filter) is validated.</violation>
</file>
<file name="src/daemon/server/daemon-shutdown-claims.ts">
<violation number="1" location="src/daemon/server/daemon-shutdown-claims.ts:106">
P2: This reclassification removes the claim from shutdown diagnostics because the daemon emits no `unattributableDeviceKeys` field. Add the new bucket to that diagnostic so operators can identify claims whose ownership could not be determined.</violation>
</file>
<file name="src/daemon/__tests__/session-idle-expiry-harness.ts">
<violation number="1" location="src/daemon/__tests__/session-idle-expiry-harness.ts:163">
P3: `createSweepBarrier` returns a `sweeps()` counter that no test ever calls: the barrier's JSDoc advertises it as the proof that "a sweep really ran", but every test either awaits `swept()` (which already proves completion) or, where a sweep count is genuinely asserted, uses its own `withinDiagnosticsScope` closure (`sweeps++` in the long-window tests, `sweepsCompleted` in the re-stamp test). Remove the member (and its type/JSDoc mention) or use it in one of the `boundAbsence` tests.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| return; | ||
| default: | ||
| case 'unattributable': | ||
| claims.unattributable.push(record); |
There was a problem hiding this comment.
P2: This reclassification removes the claim from shutdown diagnostics because the daemon emits no unattributableDeviceKeys field. Add the new bucket to that diagnostic so operators can identify claims whose ownership could not be determined.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/server/daemon-shutdown-claims.ts, line 106:
<comment>This reclassification removes the claim from shutdown diagnostics because the daemon emits no `unattributableDeviceKeys` field. Add the new bucket to that diagnostic so operators can identify claims whose ownership could not be determined.</comment>
<file context>
@@ -90,12 +103,7 @@ export function createDaemonShutdownClaimLedger(): DaemonShutdownClaimLedger {
- // and `--stale` would prove the claim live — the record means different things to a process
- // that is leaving and one that is staying.)
- claims.orphaned.push(record);
+ claims.unattributable.push(record);
return;
case CLEAR_UNRECORDED:
</file context>
| writeDaemonShutdownReport(stateDir, { | ||
| providerReleases: { released: [lease], pending: [lease] }, | ||
| claims: { released: [claim], orphaned: [], superseded: [claim] }, | ||
| claims: { released: [claim], orphaned: [], superseded: [claim], unattributable: [] }, |
There was a problem hiding this comment.
P3: No test round-trips the new unattributable bucket with actual records: the write/read cases all use unattributable: [], and the non-empty values elsewhere (device-claims / daemon-shutdown-claims tests) only exercise the producer, not the report serialization. Put a claim in unattributable on one write+read pair so the new bucket's serialization (write spread + readClaimSection filter) is validated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/daemon/__tests__/daemon-shutdown-report.test.ts, line 30:
<comment>No test round-trips the new `unattributable` bucket with actual records: the write/read cases all use `unattributable: []`, and the non-empty values elsewhere (device-claims / daemon-shutdown-claims tests) only exercise the producer, not the report serialization. Put a claim in `unattributable` on one write+read pair so the new bucket's serialization (write spread + `readClaimSection` filter) is validated.</comment>
<file context>
@@ -27,7 +27,7 @@ test('round-trips provider release and device claim records without lease creden
writeDaemonShutdownReport(stateDir, {
providerReleases: { released: [lease], pending: [lease] },
- claims: { released: [claim], orphaned: [], superseded: [claim] },
+ claims: { released: [claim], orphaned: [], superseded: [claim], unattributable: [] },
});
</file context>
| }); | ||
| } | ||
| }, | ||
| sweeps: () => completed, |
There was a problem hiding this comment.
P3: createSweepBarrier returns a sweeps() counter that no test ever calls: the barrier's JSDoc advertises it as the proof that "a sweep really ran", but every test either awaits swept() (which already proves completion) or, where a sweep count is genuinely asserted, uses its own withinDiagnosticsScope closure (sweeps++ in the long-window tests, sweepsCompleted in the re-stamp test). Remove the member (and its type/JSDoc mention) or use it in one of the boundAbsence tests.
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-harness.ts, line 163:
<comment>`createSweepBarrier` returns a `sweeps()` counter that no test ever calls: the barrier's JSDoc advertises it as the proof that "a sweep really ran", but every test either awaits `swept()` (which already proves completion) or, where a sweep count is genuinely asserted, uses its own `withinDiagnosticsScope` closure (`sweeps++` in the long-window tests, `sweepsCompleted` in the re-stamp test). Remove the member (and its type/JSDoc mention) or use it in one of the `boundAbsence` tests.</comment>
<file context>
@@ -112,5 +144,35 @@ export function createIdleExpiryHarness(): Readonly<{
+ });
+ }
+ },
+ sweeps: () => completed,
+ };
+ },
</file context>
|
Reviewed at 6eeab91. The earlier blocking finding is fixed: All 19 checks pass at this commit. Two non-blocking notes. The "could not be read" warning text in src/cli/commands/daemon.ts#L110 also covers an allocator-held record with a readable principal, which I did not run the affected tests, tsc, or a mutation; I judged the regression tests from the 1e92484 code and the new assertions. Nothing else blocks a human review. |
thymikee
added
the
ready-for-human
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, so other agents readDEVICE_IN_USEand cannot tell active from abandoned.Off by default. When set, the owning daemon expires a claim-holding session that has taken no attached commands for that long: it settles under that session's own execution lock pair, releases the claim, and leaves a bounded marker so the next command answers
SESSION_NOT_FOUNDwithdetails.reason: SESSION_IDLE_EXPIRED, naming the window and device. A session holding a remote lease (ADR 0007) or an active capture is excluded.A settle that cannot confirm its claim gone holds the record back and retries a full window after it ends: forgetting it would strand a claim owned by a process that no longer knows it holds one. The lock pair belongs to the release, not to the sweep's wait for it.
Validation
tsc,format,lint,build,check:layering,check:affected --run,gate fallow,gate production-exportsand the size ratchet green at6eeab911b5. 33 files, ~1210 production lines.Live run at
ca9f8ff74e, disposable simulator, 8000ms window: asnapshotre-stamped and the claim survived 5s of an 8s window; expiry fired atidleForMs: 8004withclaim: "deleted", freeing the claim file and writingidle-expiry.json;closeansweredSESSION_IDLE_EXPIRED; a reopen reclaimed the device with no stale marker. Review rounds since touched shutdown reporting and test timing only.Size · post-review findings