Skip to content

refactor(daemon): compose claim-recovery gateways at the process root - #3404

Open
thymikee wants to merge 2 commits into
pr-a-platform-servicesfrom
pr-b-claim-recovery
Open

thymikee wants to merge 2 commits into
pr-a-platform-servicesfrom
pr-b-claim-recovery

Conversation

@thymikee

@thymikee thymikee commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

Summary

Stacked on #3403. Stale-claim reconciliation rebuilds the dead owner's runtime gateway — the one act that names platform modules. The daemon keeps the transaction: the owner's recorded state dir, session artifact paths, owned-process store, and disposal (src/daemon/device/device-claim-owner-recovery.ts), and asks root composition for the gateway through a required ClaimRecoveryGatewayFactory (src/daemon/device/claim-recovery-gateway.ts), threaded root → RequestRouterDeps → request scope → createRequestHandler. Deletes the default root-gateway construction the recovery module used to perform itself (createPlatformRuntimeGateway import gone; the current daemon's gateway is never a recovery input). CLI device release and daemon startup pass the composed factory explicitly.

Validation

Commit 54f553740 (+ gate commit 5fa7ed73a, which moves the R76 inventory entry from the retired recovery edge to the new composition edge). Local at tip: build, check:quick, check:layering, check:di-seams, check:affected --run, eager-closure and test-size-ratchet gates, test:integration:provider 213/213, test:concurrency-torture 4/4 — same evidence as #3403 (identical final tree, run on the stack tip). device-claim-owner-recovery.test.ts proves recovery composes from the claim's own state dir per transaction and disposes it. Live claim paths exercised in the #3403 device runs; device status --stale clean afterwards.

View guided diff Turn on auto-fix

@thymikee
thymikee changed the base branch from main to pr-a-platform-services October 10, 2026 22:11
@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.16 MB 5.17 MB +646 B
Package (unpacked) 5.16 MB 5.16 MB +646 B
Package (download) 1.55 MB 1.55 MB +181 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.1 ms 26.9 ms -0.1 ms
CLI --help 83.7 ms 83.5 ms -0.1 ms

@thymikee

Copy link
Copy Markdown
Member Author

The code in 5fa7ed7 looks good, but I could not see live evidence for the stale-claim route, so this is waiting on that. The PR has no run of stale-claim reconciliation against a dead owner's claim. The #3403 device runs and a clean device status --stale do not show that this route ran. I read the diff and the new factory forwards the same three arguments to the same createPlatformRuntimeGateway call as at the base, but I did not rerun the local gates, provider integration or the concurrency torture. I also did not run the eager-closure gate, so I cannot confirm that dropping the static platform-runtime.ts edge from the router's closure causes no regression. Could you add one live run that reconciles a stale claim owned by a dead daemon, with output showing the claim was released through the composed gateway?

Not blocking, and you can take or leave these: the doc at src/daemon/request-router.ts:115 says routes without recovery run un-composed and the recovery refuses, but the field is required and no such refusal exists, and device-claim-owner-recovery.ts:37 has the same problem with its "unless a composer overrides it" clause. ClaimRecoveryGatewayInput.stateDir in src/daemon/device/claim-recovery-gateway.ts:14 is read by no production code, only by the new test. The test at device-claim-owner-recovery.test.ts:143 only checks ownedProcesses !== undefined, so a store built from the current daemon's state dir would still pass. A check that a record for session 'shared' lands under the owner's dir would pin the #2168 hazard. The comment at src/daemon/server/daemon-runtime.ts:416 says both compositions load lazily to avoid the eager closure, but platform-runtime.ts is already imported statically at lines 17-22, so the dynamic import defers only a 17-line wrapper.

I looked for a smaller design. Would passing the createPlatformRuntimeGateway that daemon-runtime already imports drop platform-runtime-claim-recovery.ts? It would split the factory between the CLI and the daemon, since src/cli reaches the root only through dedicated platform-runtime-*.ts modules, so I accept the seam as the smallest way to remove the daemon-to-platform value edge. Only the unused stateDir field and the redundant dynamic import are left to trim.

CI is green, with 18 checks and none failing, and there are no conflicts. Besides the stale-claim run above, the PR needs #3403 to merge first and must leave draft.

@thymikee
thymikee force-pushed the pr-a-platform-services branch 2 times, most recently from 029223a to e53be1d Compare October 11, 2026 07:02
Stale-claim reconciliation rebuilds the dead owner’s runtime gateway — the act that names platform modules. The daemon keeps the transaction (the dead owner’s recorded state dir, artifact paths, owned-process store, disposal) and asks root composition for the gateway through a required ClaimRecoveryGatewayFactory (src/daemon/device/claim-recovery-gateway.ts), threaded root -> RequestRouterDeps -> request scope. Deletes the default root-gateway construction in device-claim-owner-recovery.ts: recovery never touches the current daemon’s gateway.
R76 inventory gains the daemon-runtime -> platform-runtime-claim-recovery edge and retires the superseded device-claim-owner-recovery -> platform-runtime entry; the root-module zone list and planted-violation fixtures follow.
thymikee added a commit that referenced this pull request Oct 11, 2026
- Keep the claim-recovery docs honest: the router's factory and the recovery's
  composeGateway are required, and no un-composed refusal route exists (#3404 review).
- Drop ClaimRecoveryGatewayInput.stateDir: the gateway input only consumes the
  owner's sessions dir and stores; nothing production read the raw dir.
- Make the #2168 test write an owned-process record through the composed store
  and require it on disk under the dead owner's dir, so a store built from the
  current daemon's paths fails.
- Import the claim-recovery gateway composition statically in daemon-runtime;
  it rides the eager edge platform-runtime.ts already carries there.
@thymikee
thymikee force-pushed the pr-b-claim-recovery branch from 5fa7ed7 to eb7b8db Compare October 11, 2026 07:18
@thymikee
thymikee marked this pull request as ready for review October 11, 2026 07:18
@thymikee

Copy link
Copy Markdown
Member Author

Rebased onto the new #3403 head (e53be1d93) — this PR is now 301025b0b + 4d3a5f9ed + fixes eb7b8db93 (the head). One conflict in request-recording-health.test.ts, resolved by keeping the base's shared spy fixture and adding the required claimRecoveryGateway.

Nits (all taken, eb7b8db93):

  • request-router doc: rewritten to what the type says — the factory is required and a process without root composition cannot build a router; the un-composed-refusal claim is gone. Same fix on composeGateway (the clause now names the real override seam, composeRecovery).
  • ClaimRecoveryGatewayInput.stateDir removed — the gateway input consumes only the owner's sessions dir, stores, and artifact resolver; nothing production read the raw dir.
  • The Daemon-side stale-claim reconciliation uses the daemon's own owned-process store for foreign state dirs #2168 test now writes a record through the composed store (replace({kind:'session',sessionId:'shared'}, …)) and requires sessions/shared/owned-processes.json to exist under the claim owner's dir, asserting its pid too. Mutation: composing from the current daemon's paths writes elsewhere and the record is not found.
  • daemon-runtime: claim-recovery is now a plain static import — platform-runtime.ts is already static there, so the 17-line wrapper's dynamic import deferred nothing. The comment says exactly that. Eager-closure gate passes (767 tests, src/daemon.ts hub within its no-growth budget).

Live evidence — stale claim owned by a dead daemon, released through the composed gateway (iPhone 17e sim, isolated state dirs under /private/tmp):

$ agent-device open --platform ios --udid F25C… --session claim-recovery-verify \
    --state-dir /private/tmp/claim-recovery-evidence/owner-state        # claim: local:apple:ios:F25C…, classification live, pid 45331
$ kill -9 45331; agent-device device status --stale
  classification "owner-process-dead", owner pid 45331, stateDir …/owner-state
$ agent-device device release --stale --json --debug
  "status": "released"
  [diag] {"phase":"device_claim_stale_released", "data":{"deviceKey":"local:apple:ios:F25C…",
          "ownerSession":"claim-recovery-verify","ownerStateDir":"…/owner-state"}}

Second leg, the daemon-startup composition through the new static import: a second dead-owner claim (pid 50071) was reconciled by a fresh daemon on a different state dir — daemon.log: {"phase":"device_claim_reconcile","data":{"examined":1,"reconciled":1,"retained":0,"changed":0}}. Cleanup: daemon stopped, sim shut down, temp dirs removed, device status --stale reports none.

Gates (on eb7b8db93): format, check:quick, check:layering (R76: 10 classified edges), check:di-seams, eager-closure + test-file-size ratchet, test:integration:provider (69 files/213 tests), test:concurrency-torture, check:affected --run — all pass. #3391 is still open, so no layer-manifest entries. Leaves draft; needs #3403 to merge first.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed everything; rebased onto the updated #3403.

Live stale-claim run (the blocking ask). On the Android emulator (ReactNative_API_35): opened a session through a daemon on an isolated --state-dir /private/tmp/pr5-owner (claim recorded local:android:none:emulator-5554, ownerPid 3714), killed that daemon with SIGKILL so the owner is provably dead, then ran daemonless device release --stale on the PR-head build — that is createOwnerScopedDeviceClaimReconciler + the root-composed createClaimRecoveryGateway doing the whole transaction:

$ device status --stale
android ReactNative API 35: owner-process-dead session=dead-owner workspace=.../refactor-daemon-platform-services-port
$ device release --stale
released android ReactNative API 35 session=dead-owner workspace=.../refactor-daemon-platform-services-port
$ device status --stale
No stale local device claims found.

The claim was released through the composed gateway: composeOwnerScopedClaimRecovery builds the gateway (and owned-process store/artifact paths) from the claim's own state dir before reconcile runs — there is no code path from the reconciler to a device without it — and the gateway was disposed after. The session had no durable resource envelopes, so recovery was the no-resource path; I'm stating that rather than claiming resource teardown was exercised.

Notes taken:

  • ClaimRecoveryGatewayInput.stateDir deleted — production truly reads only sessionsDir/ownedProcesses/resolveSessionArtifacts, and the dead-owner-dir property is now pinned behaviourally: the owner-recovery test writes through each composed ownedProcesses store and asserts the record lands at <claim owner>/sessions/shared/owned-processes.json per claim (a store bound to the current daemon's or the first claim's dirs fails).
  • Router's claimRecoveryGateway doc and composeGateway's comment no longer describe a refusal path that doesn't exist (required field / used-unless-overridden).
  • daemon-runtime's comment is honest now: platform-runtime.ts is already static there, so the deferred imports keep the thin composition modules (and the adapters only the services module pulls) off src/daemon.ts's eager closure.

Gates rerun at the rebased tip cda216660: build, check:quick, check:layering, check:di-seams, check:affected --run, eager-closure 767, size-ratchet, provider 213/213, torture 4/4, device module tests 87/87.

@thymikee
thymikee force-pushed the pr-b-claim-recovery branch from eb7b8db to cda2166 Compare October 11, 2026 07:21

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found and verified against the latest diff

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="src/daemon/device/claim-recovery-gateway.ts">

<violation number="1" location="src/daemon/device/claim-recovery-gateway.ts:14">
P2: Make `ownedProcesses` required: omitting it selects no-op writers, so stale-claim recovery cannot clear the dead owner’s durable process records.</violation>
</file>

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

*/
export type ClaimRecoveryGatewayInput = Readonly<{
sessionsDir: string;
ownedProcesses?: OwnedProcessRecordWriter;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Make ownedProcesses required: omitting it selects no-op writers, so stale-claim recovery cannot clear the dead owner’s durable process records.

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 src/daemon/device/claim-recovery-gateway.ts, line 15:

<comment>Make `ownedProcesses` required: omitting it selects no-op writers, so stale-claim recovery cannot clear the dead owner’s durable process records.</comment>

<file context>
@@ -0,0 +1,26 @@
+export type ClaimRecoveryGatewayInput = Readonly<{
+  /** The dead owner's sessions dir, resolved from its own claim record's state dir. */
+  sessionsDir: string;
+  ownedProcesses?: OwnedProcessRecordWriter;
+  resolveSessionArtifacts(sessionId: string): AppLogSessionArtifacts;
+}>;
</file context>

@thymikee

Copy link
Copy Markdown
Member Author

This is ready. The claim-recovery gateway wiring at the process root now looks right at cda2166, and the points from the earlier review are fixed.

Not blocking: the earlier author comment says claim-recovery became a plain static import, but daemon-runtime.ts line 420 still uses await import('../../platform-runtime-claim-recovery.ts'). The later comment and the code comment at lines 414-417 describe it correctly, so you can leave the code as is and point to the later comment if anyone asks.

I did not rerun the live stale-claim runs or the local gates. I am relying on the output you quoted. Those runs used device release --stale and the daemon-startup reconcile. The request-time route through request-router.ts:391 was not run live. It uses the same factory value, and the provider and router fixtures use the real createClaimRecoveryGateway. I reviewed only this PR's own patch, because most of the delta diff is rebase churn from the base.

CI is still running at this head. 11 checks are queued or in progress, and none has failed so far. The unit, integration and layering checks cover the changed files, so please wait for them to finish. The previous head was green on 18 checks. There are no conflicts.

Before merge, #3403 must land first and CI at cda2166 must pass. No review findings stand in the way.

On the other reviewer's thread: the cubic-dev-ai P2 about the optional record store field does not apply, so you can resolve it. The only production caller, composeOwnerScopedClaimRecovery, always passes a store built from the claim's own dirs, and the new test writes through that store. Making the field required would be an optional tightening. #3404 (comment)

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 11, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant