Skip to content

refactor(capture-kit): share the bounded app-log poller (#2616) - #2624

Open
thymikee wants to merge 1 commit into
mainfrom
refactor/2616-shared-app-log-poller
Open

thymikee wants to merge 1 commit into
mainfrom
refactor/2616-shared-app-log-poller

Conversation

@thymikee

@thymikee thymikee commented Sep 15, 2026

Copy link
Copy Markdown
Member

Refs #2616

Extracts the bounded app-log poller into @agent-device/capture-kit/app-log-polling and makes Limrun its first consumer. Deduplicates acquisition rollback, the poll state machine, the bounded read with its own settle-on-timeout, mark filtering, KMP tail-overlap de-duplication, finish memoization, disposal, and cleanup-pending semantics.

Repurposed. This PR previously based on the #2112 Doublespeed stack to convert both providers at once. That base predates the released v0.21.x wire ledger, so Compatibility & Provenance could never go green here (its "removing wire surface a released peer still sends … an ack cannot cover it" group is exactly the behind-a-release case). Doublespeed is not merged, so this now lands on main alone and Doublespeed's 30-line adoption follows #2112.

Scope

  • packages/capture-kit/src/app-log-polling.ts — the shared poller, published as the ./app-log-polling subpath.
  • packages/provider-limrun/src/app-log-poller.ts — 182 lines become a 32-line adapter. Provider identity stays where it belongs: backendForReader and the cleanup wording are adapter-local.
  • packages/provider-limrun/src/request-cancellation.tsawaitLimrunOperation loses export; this poller was its only external caller, and it is still used by the request-operation drain in the same module.

Two deliberate narrowing decisions

  • Subpath export, not the façade. capture-kit/src/index.ts falls under ADR-0019's merge-base NO-GROWTH rule, so an eager façade export would grow a locked closure. The subpath matches the existing ./ios-snapshot-acquisition convention, and all 668 eager-closure-budgets cases pass with no pin edit.
  • AppLogPollerReader.readLogs takes no signal. main's Limrun reader is uncancelable and is currently called without one, so the shared contract forwards nothing and stays honest. The AbortController is internal: it drives clock.sleep and settles the bounded read on timeout, which is what makes teardown safe against a hung provider read. Widening the reader contract belongs with the first provider that can actually honour it.

The already-aborted path keeps the abandoned read's rejection handled, preserving awaitLimrunOperation's behaviour rather than the leakier shape this branch had on the old base.

Checks

check:affected --run passes. Locally: typecheck, check:layering (new subpath declared in the package-boundaries enumeration), pnpm gate fallow --base <main>no issues in 8 changed files, lint, 18 app-log/request-cancellation tests, 668 eager-closure cases, and daemon-wire-compat178 declarations, 0 changed, 0 removed, 0 added against v0.21.6.

Follow-up

Doublespeed converts on top of #2112; #2616 stays open until both providers read one implementation.

@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.62 MB 4.62 MB +445 B
Package (unpacked) 4.62 MB 4.62 MB +445 B
Package (download) 1.37 MB 1.37 MB +149 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.4 ms 27.9 ms +0.5 ms
CLI --help 79.2 ms 82.8 ms +3.5 ms

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed at e5f4096. This is already the smaller design: two ~200-line copies become one shared module and two ~20-line adapters, for -37 net production lines, and the extraction matches both providers by code comparison.

One question: could the Limrun adapter use AppLogPollerReader and drop its narrower LimrunAppLogReader type, or does it need to stay narrower? The two follow-ups the PR body lists (the disposeAll duplication and the MARK_PREFIX literal) are fine to leave for later.

Compatibility & Provenance fails in the daemon wire-compat gate on files the #2112 base stack touches (http-server.ts, daemon-artifacts.ts and others), none of which this diff touches, so it looks unrelated to this PR. Smoke Tests was still running.

Next: #2112 lands, then this retargets to main with the wire-compat gate green.

@thymikee

Copy link
Copy Markdown
Member Author

Answering the review at e5f4096 and the CI status.

LimrunAppLogReader vs AppLogPollerReader — it has to stay. AppLogPollerReader is only { readLogs, asyncDispose }; LimrunAppLogReader additionally carries platform/leaseId/instanceId, which app-log-runtime.ts reads when building the descriptor and which reconnect.ts/runtime.ts construct. The adapter can't hand the poller those identity fields through the bare contract. The call-site options.reader satisfies AppLogPollerReader already states the subtype relationship without widening the provider type, and keeping readLogs(appBundleId, lineLimit) (no signal) is what encodes Limrun's uncancelable read at the type level — intersecting with AppLogPollerReader would launder the optional signal back in and erase that. So the satisfies at the boundary is the narrowest honest link; leaving as is.

CI — Compatibility & Provenance (the only red gate). Reproduced locally. It is entirely inherited from the stack base and cannot be greened in this diff:

  • git diff --name-only <base> HEAD is exactly the 9 PR files; none are wire-surface files. The gate compares the tree's wire digests + test/wire-compat/ledger.json against the newest released tag (v0.21.3) — it ignores the PR base.
  • The stack base (8dad650, feat: add direct Doublespeed provider runtime #2112's fork head) is not a descendant of v0.21.3 and is ~303 commits behind main. The flagged declarations (LEASE_BACKENDS, daemonRuntimeSchema, SessionRuntimeHints, DaemonArtifactKnownType, http-server.ts, daemon-artifacts.ts, session-tenant-scope.ts, request-progress-protocol.ts) are main-side wire changes the stale base simply predates.
  • The gate's own output lists a "removing wire surface a released peer still sends … an ack cannot cover it" group. Being behind a release is exactly the case a compatibleChanges entry or a protocol bump cannot acknowledge — only a base at/after the release fixes it. feat: add direct Doublespeed provider runtime #2112 itself doesn't touch any of those files, so this is main-vs-v0.21.3 wire debt carried by the fork snapshot, not this PR's.

I checked the two ways to green it now and both mean doing #2112's own rebase (merge origin/main — or even v0.21.3 — into the fork head): ~7–14 real conflicts including package.json/pnpm-lock.yaml, snapshot.ts, cli-help.ts, and src/__tests__/eager-closure-budgets.ts which main deleted/restructured (so my budget-pin edit lands at a dead path there too). That's #2112's landing conflict resolution, not work to fold into this PR.

Net: everything else is green on this head (Lint, Typecheck & Package, Repo Guards, Coverage, Integration, all Smoke, Bundle). This matches the plan in your note — retarget to main once #2112 lands; the wire-compat gate and the eager-closure budget pin both resolve then (Doublespeed hunks become pure edits, budget moves to main's restructured registry). Not marking merge-ready while base-only wire-compat is red.

The two follow-ups (disposeAll reuse, MARK_PREFIX shared with the daemon writer) stay out of scope as agreed.

@thymikee

Copy link
Copy Markdown
Member Author

Thanks, that answers the question. LimrunAppLogReader carries platform, leaseId and instanceId, and backendForReader and app-log-runtime.ts read them, so the narrower type has to stay. satisfies AppLogPollerReader at the call site is the right link. No code questions remain at e5f4096.

Compatibility & Provenance still fails on wire files from the #2112 base, which this diff does not touch. Next: #2112 lands, then this retargets to main.

@thymikee thymikee added ready-for-human Valid work that needs human implementation, judgment, or maintainer merge on-hold labels Sep 15, 2026
@thymikee
thymikee force-pushed the refactor/2616-shared-app-log-poller branch from e5f4096 to f1d48c2 Compare September 19, 2026 12:11
@thymikee thymikee changed the title refactor(capture-kit): share the bounded provider app-log poller refactor(capture-kit): share the bounded app-log poller (#2616) Sep 19, 2026
@thymikee
thymikee changed the base branch from stack/2616-on-2112 to main September 19, 2026 12:11
@thymikee

thymikee commented Sep 19, 2026

Copy link
Copy Markdown
Member Author

Rebased onto main and repurposed, per review: 6322069 replaces (one no-op amend to re-trigger CI after the base change; tree unchanged) e5f4096, base is now main, and the two packages/provider-doublespeed/** files are out of this PR.

Supersedes my previous comment on the CI question: rather than document Compatibility & Provenance as base-only debt, the base is gone. pnpm gate daemon-wire-compat now reports 178 declarations, 0 changed, 0 removed, 0 added, 0 moved against v0.21.6.

What the port changed relative to the old stack base, beyond dropping Doublespeed:

  • Shared poller ships as the ./app-log-polling subpath instead of a capture-kit/src/index.ts export. main replaced numeric closure pins with the ADR-0019 merge-base NO-GROWTH rule, so an eager façade export would grow a locked closure; the subpath keeps all 668 eager-closure-budgets cases green with no pin edit.
  • AppLogPollerReader.readLogs no longer declares signal. main's Limrun reader is uncancelable and is already called without one, so forwarding a signal was an affordance with no implementer. The internal AbortController still drives clock.sleep and the bounded read's settle-on-timeout. Doublespeed can widen the contract when it lands.
  • Ported awaitLimrunOperation's already-aborted path, which keeps the abandoned read's rejection handled; the shape on the old base could leave it unobserved.
  • awaitLimrunOperation is no longer exported — this poller was its only external caller.

The LimrunAppLogReader question keeps its previous answer: it stays narrower, since platform/leaseId/instanceId are read by app-log-runtime.ts, reconnect.ts, and runtime.ts.

Locally green: typecheck, check:layering, check:affected --run, gate fallow --base main (no issues in 8 changed files), lint, 18 app-log/request-cancellation tests, 668 eager-closure cases.

@thymikee
thymikee force-pushed the refactor/2616-shared-app-log-poller branch from f1d48c2 to 6322069 Compare September 19, 2026 12:18
@thymikee

Copy link
Copy Markdown
Member Author

Reviewed at 6322069. The move matches main: rollback, tail overlap, finish memoization and disposal in the new app-log-polling.ts are the Limrun code with named constants, and readLogs can drop signal because no Limrun reader takes one.

The retarget changes the size question from the first review. That review called this the smaller design because two poller copies became one. With the Doublespeed commit gone and #2112 still open, main has only the Limrun poller. So this PR now adds a public capture-kit subpath, a provider-neutral reader type and a second copy of the abort-settle logic, for about +60 production lines, and removes no duplication. Would it be smaller to keep the poller in provider-limrun and extract it in the PR that adds Doublespeed, when the second consumer shows what is really shared? If you prefer to land the extraction first, can settleOnAbort and the signal branch of awaitLimrunOperation share one helper, so this PR does not add a copy?

Not blocking: the Limrun test keeps its own copies of the poller-host and deferred-sleep fixtures, and the title at app-log-poller.test.ts#L66 names a poller signal the reader no longer gets.

Checks were still running when I reviewed, and none had failed. There are no conflicts. Next: an answer to the one-consumer question.

@thymikee thymikee removed the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant