Skip to content

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

Open
thymikee wants to merge 1 commit into
stack/2616-on-2112from
refactor/2616-shared-app-log-poller
Open

thymikee wants to merge 1 commit into
stack/2616-on-2112from
refactor/2616-shared-app-log-poller

Conversation

@thymikee

Copy link
Copy Markdown
Member

Summary

Extracts the shared bounded provider app-log poller into @agent-device/capture-kit (issue #2616), so Limrun and Doublespeed stop maintaining two ~200-line copies of the same lifecycle.

Both provider-limrun/src/app-log-poller.ts and provider-doublespeed/src/app-log-poller.ts carried the identical acquisition rollback, polling state machine (active/recovering/failed/ended), tail-overlap (KMP suffix/prefix) algorithm, finish memoization, and disposal mechanics. They now share one implementation in capture-kit/src/app-log-polling.ts behind a narrow input — a disposable reader (readLogs(appBundleId, lineLimit, signal?) + async dispose), backend identity, app bundle id, output path, the host clock/output ports, and the cleanup-failure message. Each provider keeps only an adapter.

No user/API/CLI behavior changes. Polling interval (1000 ms), read timeout (5000 ms), line limit (1000), tail budget (256 KiB), the [agent-device][mark] filter, backend selection, append/newline behavior, and every user-visible error string are preserved.

Base / dependency (read first)

Stacked on #2112. provider-doublespeed exists only on #2112's branch (an external-fork PR, currently conflicting with main), and #2616 is explicitly blocked by it. #2112's base lives in a sibling fork that the network parent cannot head a PR into, so this PR's base is stack/2616-on-2112 — a mirror of #2112's head commit (8dad650) pushed here purely to keep the review diff isolated to this change. Retarget to main once #2112 merges; the Doublespeed hunks are then pure edits, not additions. Do not merge toward main independently.

Kept provider-specific (deliberate, characterized — not unified)

  • Backend identity: Limrun derives ios-simulator/android from reader.platform; Doublespeed is the constant ios-simulator. Both flow through the shared backend input; shared policy names no provider.
  • Reader cancellation: Doublespeed's reader forwards the poller's AbortSignal; Limrun's WebSocket-backed reader takes no signal and cannot cancel. The shared bounded read forwards the signal and always settles on its own timeout (settleOnAbort), so neither guarantee changes and abort never implies the provider request ended. Limrun's prior awaitLimrunOperation(...) (called with no drain callback) is behaviorally identical to the shared settleOnAbort on every reachable path.

Validation

  • vitest run for the shared + both provider poller suites: 19 green. Common tests moved with their code; added characterizations: streaming tail dedup across successful reads, ended state, empty-delta no-write, trailing-newline normalization, disposal order (reader → output), the finally-abort of the read timer, timeout with a signal-ignoring reader (would hang without the settle wrap), a signal-honoring reader rejecting on abort, finish-during-in-flight (stop before write), late rejection, repeated finish, and reader/output disposal failure.
  • Provider adapter tests assert backend identity and each provider's cleanup wording independently of the helper; reconnect/descriptor/admission tests retained.
  • pnpm check:layering, fallow audit (unused types: 0), tsc -b for the three packages, and eager-closure-budgets (302) all pass.
  • Mutation-checked the key tests (reverting settleOnAbort, previous = read.text, the if (stopped) return short-circuit, the adopted-flag, and the finally abort each fail a named assertion).
  • Eager-closure pins moved capture-kit/index 32→33 and session-teardown 60→61: the shared poller is one new façade module, consistent with the sibling durable-capture exports.
  • Published, not merge-ready: CI on this head is still authority; provider-integration/Coverage/Apple-device lanes are GitHub-authoritative and were not run against a live Doublespeed/Limrun session locally. The previously-seen android-snapshot / daemon-entrypoint / Apple provider-integration failures pass in isolation and are host contention.

Tradeoffs / follow-ups

  • Left as follow-ups (out of this issue's "do not broaden" scope): disposeAll duplicates a cleanup-settlement pattern already in app-log-pid-process.ts; MARK_PREFIX still duplicates the daemon writer's marker literal across a package boundary.
  • Net production LOC for the touched pollers: 395 → 270 (−125); the tail algorithm and lifecycle now live once, no obsolete copy remains.

Size: 8 files touched (2 new shared files, 2 adapters, 3 tests, 1 budget pin). Scope stayed within the app-log capture path. Docs/skills unchanged (no CLI/help surface change).

Both providers shipped a near-identical ~200-line app-log poller with the same
acquisition rollback, polling state machine, tail-overlap algorithm, finish
memoization and disposal mechanics. Extract the shared lifecycle into capture-kit
(ADR 0019 §5) behind a narrow reader input, leaving each provider a thin adapter
that supplies backend identity and its own cleanup wording.

Cancellation is characterized, not unified. Doublespeed's reader forwards the
poller's abort signal; Limrun's WebSocket-backed read stays uncancelable. The
shared bounded read forwards the signal and always settles on its own timeout, so
neither provider's guarantee changes and abort never implies the request ended.

Provider poller source drops 395 -> 276 lines; the tail algorithm and lifecycle
now live once. No provider name leaks into shared policy.

Issue: #2616 (stacked on #2112)
@github-actions

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.52 MB 2.52 MB -1.6 kB
JS gzip 848.3 kB 847.8 kB -510 B
npm tarball 967.3 kB 967.3 kB -69 B
npm unpacked 3.36 MB 3.36 MB -1.6 kB

npm unpacked components

Component Base Current Diff
JS / dist source 2.68 MB 2.67 MB -1.6 kB
Apple runner source/project 581.1 kB 581.1 kB 0 B
macOS helper source 54.8 kB 54.8 kB 0 B
Android helper artifacts 0 B 0 B 0 B
Other package files 45.4 kB 45.4 kB 0 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 29.2 ms 29.1 ms -0.1 ms
CLI --help 78.3 ms 79.5 ms +1.2 ms

Top changed chunks: no changes in the largest emitted chunks.

Top changed packed files

Packed file Base Current Diff
dist/src/app-log-polling.js 0 B 2.4 kB +2.4 kB
dist/src/app-log-runtime2.js 16.0 kB 13.9 kB -2.1 kB
dist/src/app-log-runtime3.js 17.9 kB 16.0 kB -1.9 kB
dist/src/request-cancellation.js 1.5 kB 1.5 kB -2 B

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

on-hold 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