Repository navigation
fix(daemon): stop provider lease releases after shutdown closes the releaser - #4134
Conversation
…eleaser A provider release that outlived the drain budget kept a retry timer that called the provider after its plugin had been disposed, and a lease that expired after shutdown could still be released. The releaser now owns an explicit phase: running, draining, closed. A closed releaser starts no provider attempt and arms no timer, and keeps the recoverable lease durable so the next daemon recovers it.
Size Report
Startup median (7 runs, lower is better):
|
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Independent review of f7879cf2d266ee1dd476d0342b2819bdeceb647b..74e46b9eb2240f760c50ace8db48362790602b90 against the coordinator's shutdown contract.
Verdict: needs-changes.
[P2] Refuse release admission explicitly once closed — packages/daemon/src/server/provider-lease-expiry.ts:308-324 (closed guards at :260 and :275; return contract at :56).
After beginShutdown() → a budget-limited drain() → shutdown(), another release(lease) still inserts the lease into a pending map. The attempt helper then returns undefined because the phase is closed, and the public Promise<void> resolves normally. Thus this blocks provider execution but does not enforce the required admission boundary or return the required typed refusal. For a live-only provider such as BrowserStack, the new pending entry has no durable recovery record and cannot be processed; it also arrived after the drain snapshot. The new post-shutdown test exercises this path but accepts the void result. The production expiry callback (http/daemon-runtime.ts:381-383) discards the result, and http/daemon-lease-finalizer.ts:20-25,53-56 treats a resolved promise as completion of the bounded call. Add the closed admission branch to the public release boundary, retain the required durable handoff for recoverable leases, return a typed closed/deferred outcome, and handle that outcome in both callers instead of silently accepting the release. Add a regression assertion for the outcome and caller handling.
Other shutdown paths checked: scheduleRetry and journal retry cannot re-arm after close; the attempt-finish callback reaches the guarded scheduler; both attempt constructors refuse provider execution; recoverInherited/retryPending cannot bypass those constructors. A recoverable attempt is durable before execution, so a late failure retains its record. A late success still removes its pending entry and synchronously persists the remaining record. The normal runtime order remains valid: begin draining → session teardown/finalization → bounded drain → close releaser → shut down owners/plugins. The phase advances only through advancePhase and cannot move backward.
ADR sentence review (docs/adr/0007-remote-device-leases.md:95-96): the final journal write matches shutdown() when a journal retry is pending; the synchronous-write statement matches both persistence paths; the timer prohibition matches the closed guards. These additions preserve the surrounding recovery/durability claims. A narrow amendment is warranted to qualify the preceding unqualified promise that failed journal writes are retried; this owner-local fix does not warrant a new ADR. For the requested ASD-STE100 check, put the condition first in the first added sentence and split its two actions, for example: “If a journal retry is pending, daemon shutdown makes one final journal write. It does not schedule another journal retry.” This is a nonblocking wording correction.
Regression evidence: one focused Vitest invocation in a disposable checkout at base f7879cf2d266ee1dd476d0342b2819bdeceb647b, with the head's test file copied unchanged under an r4134- scratch name, after frozen installation and build: 3 failed, 13 passed. All three added tests fail for the intended reason: the late rejection produces a second provider call, direct post-close release calls the live provider, and post-close expiry calls recovery. Command: pnpm exec vitest run --project unit-core packages/daemon/src/server/__tests__/r4134-provider-lease-expiry.test.ts. No head test run or CI verdict is claimed. The disposable checkout was removed; the reviewed head and source status remain unchanged.
|
The code looks good at 74e46b9, and I found no blocking problems. There are no conflicts. The failing check is the iOS phase of Smoke Tests, which looks unrelated: local iOS simulator runs allocate no lease provider, so the changed code runs on empty state there, and macOS smoke passed on the same shutdown path. Please re-run the iOS phase to confirm, and let Integration Tests and Coverage finish green. Not blocking, and you can take or leave these: in provider-lease-expiry.ts, once the phase is closed, nothing that runs after an await should write lease records, so the two attempt continuations (:265-268 and :278-281) should skip persistPendingLeases when closed and leave the lease in the durable record for the next daemon, or a test with a held release that resolves after shutdown() should assert both record files stay unchanged; the ADR sentence about timers could also name this case. A test that makes the final shutdown journal write fail and then checks vi.getTimerCount() is 0 would cover the closed guard at :185. A live-only lease that reaches release() after close lands in pendingLiveLeases and is never released, so could it return early at :326 with one warn diagnostic? In the ADR, "If a journal retry is owed, daemon shutdown makes one final journal write. It schedules no further retry." fits the style better, with :94 qualified by "while the daemon runs". I did not run the tests or the BrowserStack live run, and the CI excerpt did not include the iOS smoke log, so I could not read its failure cause. |
thymikee
added
the
ready-for-human
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
A release that arrived after the releaser closed still admitted pending work, started no provider attempt, and resolved as if it had completed, so both callers read it as a release the provider had ended. `release` now answers with what it did: released, retrying, not-required, or refused with its reason and whether a lease record names the lease for the next daemon. A closed releaser refuses without admitting pending work, and still writes the expired-lease record of a recoverable lease. The shutdown finalizer and the lease-expiry callback report every refusal, so a device no daemon will end is named in the daemon log instead of resolving quietly.
![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 (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. 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/daemon/src/server/http/daemon-runtime-lease-expiry.test.ts">
<violation number="1" location="packages/daemon/src/server/http/daemon-runtime-lease-expiry.test.ts:118">
P2: This test allocates the lease only after `runtime.close()` and manually dispatches expiry, so it does not verify an abandoned lease that existed before shutdown and expired across the shutdown boundary. Exercise the real pre-shutdown lease/race (or explicitly arrange the in-flight shutdown interleaving) so regressions in that lifecycle path cannot pass.</violation>
<violation number="2" location="packages/daemon/src/server/http/daemon-runtime-lease-expiry.test.ts:152">
P3: Remove this per-test `rmSync`; `mkdtempForTestSync` directories are removed by Vitest global teardown, and repository guidance specifically avoids deleting them from individual tests.
(Based on your team's feedback about Vitest temporary directory cleanup.)</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Review findings addressed (this round: [P2] Refuse release admission explicitly once closed — accepted.
Regression evidence, disposable checkout at base ADR 0007 wording — accepted. The added sentences are now "If a journal retry is pending, daemon shutdown makes one final journal write. It does not schedule another journal retry.", and the preceding promise is qualified ("… is retried until this daemon shuts down"). One further amendment records the admission boundary itself: a shut-down releaser refuses, still writes a recoverable lease's record, and an unrecorded refusal is logged with its lease provider. New head: |
|
The code at 45787dd looks right, and the earlier review's notes on the post-close release path are addressed. No conflicts are known. One thing remains on the code side, and it is not blocking. Not blocking: the new finalizer test in packages/daemon/src/server/http/daemon-lease-finalizer.test.ts closes the releaser before it runs Smoke Tests fails in its iOS phase, which uses local simulators with no lease provider. On that route the diff only runs On the Cubic threads, the summary-level one (#4134 (comment)) still applies, along with two lower-priority threads. Please resolve #4134 (comment) because a post-close release cannot coincide with a next daemon: the sweep is cleared at daemon-runtime.ts:838 and the registry is finalized at :863-869 before Before merge, Smoke Tests needs to go green, so please re-run the iOS phase, and the three open Cubic threads need an answer. |
thymikee
removed
the
ready-for-human
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Round-2 review of 45787dd057dbf98afe97de04f79450730c012714:
The round-1 finding is resolved. A release after shutdown() returns { status: 'refused', reason: 'closed', durable } before admitting work; recoverable leases are written into expired-provider-leases.json, which recoverInherited() loads after daemon registration and retries. Refusals that cannot be persisted are logged with lease ID and provider; the shutdown summary includes refused lease IDs.
I found no other concrete regression. emitDaemonDiagnostic defaults to warn, preserving existing callers; the new severity is passed only for lease refusal events. Outcome and diagnostic shapes are typed, and the touched source stays within the daemon lease owner, its direct runtime/finalizer callers, and the host-kit diagnostic type export. ADR 0007's changed statements match the implementation and surrounding recovery claims; no meaning drift found. ASD-STE100 style is acceptable.
The reported Apple smoke timeout is not plausibly caused by this diff: it changes no recording or native video-validator path. Focused daemon tests were not run because this worktree has no vitest executable (pnpm vitest reported Command "vitest" not found).
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Round-2 independent review of 74e46b9eb2240f760c50ace8db48362790602b90..45787dd057dbf98afe97de04f79450730c012714.
Verdict: approve. The round-1 P2 is resolved. No blocking code findings.
- Closed admission and recovery:
provider-lease-expiry.ts:359-361returns the typed closed refusal before ordinary release admission or provider execution. For recoverable leases,refuseClosedReleaseuses the recovery map as record content, synchronously callspersistPendingLeases, and reports its actual success asdurable(:329-334). This does not admit an executable attempt: both attempt constructors and the retry scheduler reject the closed phase. The expired record is atomically published bywriteLeaseRecord(:523-535). On the next daemon,recoverInheritedreads that same file intopendingRecoveryLeases, marks its ids persisted, adopts the live journal, and callsretryPending(:213-228); the recovery constructor invokes the provider recovery and removes the record entry only on success (:294-308). Runtime calls recovery after registration acquisition (daemon-runtime.ts:658,694-703). This establishes recovery across daemon process restart; it does not add an fsync/power-loss guarantee. A failed write returnsdurable: false, and the refusal diagnostic includes the lease provider. - Callers and typing: expiry consumes the outcome and writes its mapped severity to the daemon log; finalization emits the mapped refusal and returns refused ids for
daemon_shutdown_provider_release_drain.ProviderLeaseReleaseOutcomeis discriminated, refusal reasons are a literal union, and the diagnostic copies the typed reason and boolean without message parsing. These are internal outcomes and diagnostic data, so ADR 0010 does not require turning them into thrown wireAppErrors. All otheremitDaemonDiagnosticcallers still pass three arguments and retain the previouswarnseverity. The host-kit change only exports the existingDiagnosticLevelunion. Scope remains the lease owner, its direct runtime/finalizer/diagnostic callers, associated tests, and ADR 0007. - ADR sentence comparison: qualifying failed journal retries with “until this daemon shuts down” matches the closed timer guard. The condition-first final-write sentence and separate no-retry sentence match
shutdown()(:407-417) and address the prior ASD-STE100 note. The synchronous-write/timer statement is unchanged. The added recovery and unrecorded-refusal sentences match persistence and both caller paths, with write failure covered by the following conditional. The surrounding registration-before-read, recovery-without-live-restoration, and uncovered-allocation claims keep their meaning. - [P3, nonblocking]
docs/adr/0007-remote-device-leases.md:97: qualify “every further release” as “every further provider-backed release”. A post-closereleaseof a lease withoutleaseProviderreturnsnot-requiredbefore the closed guard (provider-lease-expiry.ts:360-361). The current universal wording overstates the implemented boundary. The other changed sentences preserve the surrounding ADR claims and satisfy the requested condition-first/one-instruction-per-sentence style check.
The supplied Apple Smoke failure (record stop: xcrun timed out after 30000ms during cold Swift video-validator compilation), also seen on unrelated #4131, has no plausible causal path in this diff. It changes provider-lease outcomes, shutdown accounting, diagnostics, and prose; it changes neither video validation/compilation nor the xcrun timeout. I did not investigate that failure further or wait for CI.
Validation: static caller/recovery tracing and git diff --check; no tests run in this round. The reviewed checkout remains clean at 45787dd057dbf98afe97de04f79450730c012714.
…eases A lease with no lease provider is answered before the closed guard, so the universal wording overstated the boundary the releaser enforces.
|
P3 accepted.
One-line ADR edit, so no full gate: The push restarted CI at the new head, which re-runs Apple smoke on its own; that lane already had its one rerun at the previous head. New head: |
Apex SentinelVerdict: 8 finding(s): 2 minor, 6 nit. Verdict: approve. reached max rounds (1). 🟡 Minor (2)
🔵 Nit (6)
Reply under an inline comment with |
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 8 new findings on 5a5671a. The verdict and the full list are in the summary comment.
|
Thanks for the update. At 5a5671a I found nothing that needs to change before merge, and the docs wording from the earlier review is now fixed (ADR 0007 line 97 says "provider-backed release"). Checks show 19 passing and 0 failing at this commit, the earlier iOS smoke failure is gone, and there are no conflicts. I did not run the lease suites, a base comparison or check:affected, and I did not re-read the CI logs at this commit. Not blocking, take or leave: the Of the open inline threads, the P2 on the summary level (#4134 (comment)) still applies, and so do two lower-priority nits. The P3 on the ADR wording (#4134 (comment)) is fixed. These do not apply, so you can resolve them: the P1 on the shutdown sweep (#4134 (comment)) because the sweep and registry are finalized before the releaser closes; the P2 on expiry after close (#4134 (comment)) because the finalizer releases every active lease first; #4134 (comment) because the only caller always gets an outcome object; #4134 (comment) because that write only runs for an abandoned request, which keeps the registration; #4134 (comment) because Nothing blocks merge. Before merge, please reply on the summary-level thread: apply the |
|
Thanks, that settles the summary-level thread at 5a5671a. Each refused lease already logs its own row at |
thymikee
added
the
ready-for-human
Summary
createExpiredProviderLeaseReleaserspread shutdown state over flags, timers and maps, so nothing owned the transition: a release outlivingdrain()'s budget survivedshutdown()and plugin dispose, and its failure re-armed a provider call on a disposed runtime. The module now owns one phase —running/draining/closed— advanced only byadvancePhase.closedstarts no provider attempt, arms no timer, and admits no pending work:release()answers a typed outcome (released/retrying/not-required/refused+reason+durable), which both callers report. A recoverable lease still gets its record written for the next daemon; ADR 0007 records that boundary.Validation
Lease suites, disposable checkout: base
f7879cf2d26 failed | 17 passed; reviewed head74e46b9eb25 failed | 18 passed, failing for the review's reason; head45787dd057green. Unit16752and daemon + host-kit4334pass. Gatecheck:affected --run --head 45787dd057: passed,exactCommit=true, receiptrun-hPwNaG.Live: daemon plus a BrowserStack session whose
DELETEa proxy held past the 2s drain budget, then SIGTERM → one provider call, none in the 22s after close. That route's A/B needslimrun(API unresolvable) orrevyl(no plugin), so it shows fixed behaviour only. Round 2 changed no device route: no new live run.Open blocker, cause unproven: Apple smoke failed twice on
stop screen recording/xcrun timed out after 30000ms(runs41291091023,41295528868attempt 2); its first attempt failed elsewhere, and each scenario passed in the other attempt. No lease code there.