Skip to content

fix(server): release device captures when start fails - #595

Merged
lukemaj merged 1 commit into
mainfrom
fix/592-device-capture-cleanup
Oct 20, 2026
Merged

lukemaj merged 1 commit into
mainfrom
fix/592-device-capture-cleanup

Conversation

@lukemaj

@lukemaj lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

What: Failed device record and trace starts now release their capture registration.
Why: A stale commandCaptures entry rejected later starts on the same session.
So what: Agents can start a fresh capture after failure; maintainer review and merge remain next.

Problem

#592 reports a capture registration left behind when the external start command fails. The next start is rejected with Capture sequence does not belong to this exact opening. This fixes the existing server component and advances remote device reliability under #379/#383.

Change

  • Remove the session's record or trace entry when its start effect fails, preserving the original failure.
  • Exercise both kinds through the registered device command path with a failed external start, a fresh start on the same target, and a successful stop publishing an artifact.
  • Associate the fix with remote device control in the project map.

Upstream main has no commandCaptures or typed record/trace command path; open PR and Issue searches found no matching fix. Details are recorded in the Issue.

Verification

  • Before the fix: node_modules/.bin/vp test run apps/server/src/device/DeviceCommands.registered.test.ts failed both new cases at the fresh-start assertion with invalid_request; 11 existing tests passed.
  • After the fix: node_modules/.bin/vp test run apps/server/src/device/DeviceCommands.registered.test.ts apps/server/src/device/DeviceService.test.ts passed all 40 tests.
  • Targeted lint and formatting checks cover the changed files.
  • No repo-wide checks, sleeps/polling in tests, version bumps, dev servers, browsers, or devices. The external process is a deterministic fixture; no live device verification was performed.
  • Website impact: none; existing server failure cleanup only.

Elon record

Requirements and who asked: User requested #592: failed record and trace starts must release registration and allow a fresh start on the same session, with failing-before/passing-after proof.
Deleted: Capture redesign, success-path changes, versioning, and UI/runtime work.
Bottleneck: The stale in-memory registration prevents the next start during sequence validation.
Checked myself: Read #592 and the registration/validation/stop paths, verified #584 merged, observed both regressions fail before cleanup and all 40 targeted tests pass after it.

Closes #592

Model: gpt-6.1-sol. Harness: Codex.

@lukemaj
lukemaj force-pushed the fix/592-device-capture-cleanup branch from 666ca1c to db0cbb1 Compare October 20, 2026 17:01
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:S labels Oct 20, 2026
@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent review found no actionable findings on db0cbb1.
Why: Failed record and trace atomic commands remove their session registration before the command lock is released, while successful starts retain it.
So what: Review passes for this exact candidate; maintainer merge approval remains required.

Reviewed the complete three-file diff against f5f8604, Issue #592 and its comments, and relevant registered MCP handlers, sequence validation, capture registration/stop/session retirement, external receipt handling, and AgentDeviceCommand validation.

  • The cleanup surrounds the complete atomic command effect. Installed Effect 4.0.5 implements onError through a failure-exit finalizer, including defects and interruption, with uninterruptible cleanup and preservation of the original failure. The outer command semaphore prevents a fresh command from racing the deletion.
  • Cleanup is restricted to record/trace starts. Successful starts, stop artifact publication, unrelated commands, and successful earlier steps in a subsequently failing batch keep their prior behavior. Pre-dispatch failures before registration need no removal.
  • Both parameterized regressions use the registered command path and a deterministic failing process leaf, then require a fresh request on the same opening to start and stop successfully with an artifacts result. Existing media coverage checks successful capture publication and exact-session replay. The map references match the owning Issue and PR.

Proof reused from the task and Issue: both new cases failed before the fix (2 failed, 11 passed); the two focused files passed after the fix (40 passed); the final parameterized registered file passed (13 passed), with unchanged DeviceService source. Targeted lint, formatting, and diff checks were reported passing. I inspected the tests and implementation but did not rerun tests, CI, or live devices; this verdict is an independent code review, not a claim of complete runtime coverage.

Elon record:
Requirements and who asked: Issue #592 and the review request require failed record/trace starts to release registration and allow a fresh start on the same session without changing successful behavior.
Deleted: Capture redesign, unrelated cleanup, new interfaces, version changes, and additional runtime work.
Bottleneck: A stale commandCaptures key rejects the next start during sequence validation.
Checked myself: Read the exact full diff and relevant callers, failure/interrupt finalizer implementation, semaphore ordering, receipt boundary, and both regression cases; verified the live PR base and head match the requested candidate.

Model: gpt-6.1-sol. Harness: Codex. Verdict: success, no actionable findings.

@lukemaj

lukemaj commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

Agent work on this PR

Estimated cost at least $0.17 · 26 responses · 47 sessions · 3.0 h wall time

Model Responses Tokens Estimated cost
grok-4.7 (grok, medium) 15 95,749,479 unknown
gpt-6.1-sol (codex, medium) 11 974,621 $0.17

Flags: 1 human correction · 18 large tool outputs · 1 repeated command · 1 repeated failure · 344 repeated reads · 14 repeated skill loads · 1 session without usage records · 15 unpriced responses

Details: snapshot, prices, coverage, counters
  • Task toolboxmd/chromeria#595: outcome unknown (recorded acceptance only; a finished process never implies it).
  • Proof: fix(server): release device captures when start fails #595
  • Snapshot f3db2f4d08fa0365facaebb72d4edfa9af38d713e8f370fa97a48ee4af9e54ac, records up to 2026-10-20 17:04 UTC.
  • 47 sessions on claude, codex, grok; AgentsMD 17.4.6.
  • The total includes work shared with 7 other tasks, counted whole in each, so sums across PRs overlap.
  • Not counted: 1,308 responses (at least $58.84) in sessions shared with other PRs that worked in no single PR's checkout.
  • 43 responses in these sessions worked on other PRs and are counted there.
  • Totals reconcile with the measured sessions: yes. Evidence complete: no.
  • Prices: list-price estimate from T3 local rate table (path withheld) as of 2026-10-20, schedule 42ef195f9bb07476e283db469fbf2854941cef35991d58821602b020c403160c. Unknown prices stay unknown, never zero.
    • T3 LiteLLM rate table when present; bundled schedule covers the rest.
    • Model price sources: T3 local rate table (path withheld).
    • Unpriced: unknown model (15).
  • Native session usage or worker ownership is unavailable.
  • Harness-reported cost: none reported.
  • Usage totals are not billing. Subscription spending is separate and is never posted as spend.
  • Crashed runs are counted separately: 0.

Token counters by model (native counter semantics; never added across semantics):

  • grok-4.7 (grok, medium), grok counters: input includes cached, output includes reasoning: input 95,241,215; cache-read 90,695,552; cache-write 0; output 508,264; reasoning 405,463; harness total 95,749,479.
  • gpt-6.1-sol (codex, medium), codex counters: input includes cached, output includes reasoning: input 970,415; cache-read 952,448; cache-write 0; output 4,206; reasoning 567; harness total 974,621.

Generated tokens (reasoning and other output are disjoint; other output includes code, tool calls and replies):

Model Reasoning Other output Reasoning share Split known for
grok-4.7 (grok, medium) unknown unknown unknown 0 of 15
gpt-6.1-sol (codex, medium) 567 3,639 13.5% 11 of 11

Usage by work phase (this task's own responses):

Phase Model Responses Input Cache read Cache write Output Reasoning Total Priced part
unclassified gpt-6.1-sol (codex, medium) 11 970,415 952,448 0 4,206 567 974,621 $0.173239

Recorded activity by phase (observations, not separate token bills):

Phase Tool calls MCP results Reads Skill invocations File changes Failed tool results
unclassified 36 4 10 unavailable 5 2

Selected rates (USD per million tokens). These rates value the report at the selected schedule date; they do not establish historical prices or subscription spending.

Model Input tier Input Cache read Cache write Other output Reasoning
gpt-6.1-sol all 2 0.1 2.5 10 10
grok-4.7 all unknown unknown unknown unknown unknown

Other output and reasoning are priced without double counting inclusive native output. Missing rates remain unknown.

Local measurement from native records; usage totals are not billing. Updated in place by agent-observer publish.

@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent review passed for db0cbb1 with no actionable findings.
Why: The failure finalizer clears failed start registration before releasing the command lock and preserves the original failure.
So what: PR #595 remains open and unmerged for maintainer approval.

Review findings: #595 (comment). Verified the newest review/independent status is success, created by lukemaj on the current PR head. Both new regressions failed before the fix; 40 focused tests passed afterward, followed by 13/13 after converting declarations to parameterized cases. Lint, formatting, and diff checks passed.

Agent Observer report: #595 (comment). Measurement gap: that report includes shared sessions from other work and reports incomplete evidence and unknown cost; its aggregate usage/time must not be interpreted as this microfix's isolated consumption.

@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 4.9 KiB 4.9 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.8 KiB 20.8 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 1 1 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB 0 B (0.0%) 29.3 KiB ✅
Claude Live turn messages 2 2 0 (0.0%) 8 ✅

Baseline: 14bd851 · PR result: a0037e0 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@lukemaj
lukemaj force-pushed the fix/592-device-capture-cleanup branch from db0cbb1 to a0037e0 Compare October 20, 2026 18:21
@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What: Fixed the test fixture type error and rebased PR #595 linearly onto current origin/main.
Why: The failed-start fixture omitted four required ProcessRunOutput booleans, causing CI TS2375.
So what: The corrected head is pushed with lease; review and CI apply to the new head, and no merge was performed.

Head: a0037e0.
Base: 14bd851.

Added false values for stdoutTruncated, stderrTruncated, stdoutInvalidUtf8, and stderrInvalidUtf8 to the failed process result. The rebase conflict in docs/map.md retained both main's PR #596 and this fix's #592 / PR #595 references. History contains one task commit above the current base.

Verification on the corrected candidate:

  • In apps/server: ../../node_modules/.bin/tsc-rs --noEmit, exit 0, no error TS diagnostics (existing Effect suggestions remain).
  • node_modules/.bin/vp test run apps/server/src/device/DeviceCommands.registered.test.ts apps/server/src/device/DeviceService.test.ts, exit 0, 40 tests passed.
  • Targeted lint and git diff --check passed; commit hook formatted the changed test.
  • Force-pushed the owned branch with an explicit lease against db0cbb1.

Elon record

Requirements and who asked: User requested fixing CI's TS2375, rebasing onto current origin/main, server typecheck, focused tests, lease push, and no merge.
Deleted: No runtime redesign, version bump, broad checks, or merge.
Bottleneck: The fake process result lacked required fields, preventing server typechecking.
Checked myself: Read the failing CI log and ProcessRunOutput definition; server typecheck and all 40 focused tests pass on the corrected rebased candidate.

@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What: Independent exact-SHA review passes with no actionable findings on a0037e0.
Why: Failed record and trace starts clear their registration before releasing the command lock, and the corrected fixture satisfies ProcessRunOutput without changing runtime behavior.
So what: The independent review gate passes for this candidate; CI and maintainer merge approval remain separate, and no merge was performed.

Reviewed the complete three-file diff against base 14bd851, Issue #592 and its comments, all PR comments, and relevant command callers, sequence validation, registration, stop publication, session cleanup, and external receipt boundaries. Verified the live PR base/head and clean local HEAD match the requested candidate.

  • git range-diff against prior candidate db0cbb1 shows only the four added fixture booleans and the map reference adaptation. DeviceService.ts is identical between the two candidates. The new candidate has one task commit directly above the requested current base, with no merge commit.
  • The failure fixture explicitly supplies false for stdoutTruncated, stderrTruncated, stdoutInvalidUtf8, and stderrInvalidUtf8, matching the required ProcessRunOutput fields. Exit code 1 still triggers the real AgentDeviceCommand failure path and the registered MCP result remains outcome_unknown.
  • The per-atomic-command onError cleanup applies only to failed record/trace starts. Installed Effect 4.0.5 runs it as an uninterruptible failure finalizer, preserving the original failure. The enclosing command semaphore prevents a fresh command from racing registration deletion. Successful starts retain registration; successful stop artifact publication and earlier successful batch steps retain their behavior.
  • Both parameterized regressions require a failed external start followed by a fresh start and successful artifact-producing stop on the same opening. Existing media coverage checks successful publication and replay. The map keeps main's PR fix(preview): honour destination expiry when renewing viewers #596 and this fix's Issue fix(device): a failed record or trace start stays in commandCaptures #592 / PR fix(server): release device captures when start fails #595 references, with no unrelated change relative to the new base.

Proof reused for this exact candidate from the task and correction comment: server ../../node_modules/.bin/tsc-rs --noEmit in apps/server exited 0 with no error TS diagnostics; node_modules/.bin/vp test run apps/server/src/device/DeviceCommands.registered.test.ts apps/server/src/device/DeviceService.test.ts exited 0 with 40 passing tests; targeted lint and diff checks passed. Earlier failing-before evidence is retained on Issue #592. I inspected implementation, tests, dependency semantics, and candidate changes independently; I did not rerun checks or launch servers, browsers, or devices. This is a code-review verdict, not a claim of complete CI or live-device coverage.

Elon record:
Requirements and who asked: Issue #592 and the review request require failed record/trace starts to release registration, permit a fresh start, preserve successful behavior, and review the fixture correction and linear rebase on the exact candidate.
Deleted: No redesign, new interfaces, runtime changes beyond the original microfix, version changes, repeated proof runs, or merge.
Bottleneck: The stale capture registration blocks fresh starts; the missing fixture booleans previously blocked server typechecking.
Checked myself: Read the complete rebased diff and range-diff, required output type, failure finalizer and semaphore ordering, registered command path, receipt boundary, successful capture publication, regressions, and live PR identity.

Model: gpt-6.1-sol. Harness: Codex. Verdict: success, no actionable findings.

@lukemaj

lukemaj commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

What: APPROVE at exact head a0037e064a06f42550062df73c719556d45e78c9, with no actionable findings.
Why: Failed record and trace starts now release their session capture registration while preserving failure propagation and successful capture behavior.
So what: Independent review is complete; CI remains the compilation and execution gate, and merge remains a maintainer decision.

Read the complete three-file output of gh pr diff 595 -R toolboxmd/chromeria, Issue #592, and the surrounding capture validation, registration, dispatch, artifact publication, session retirement, and durable request paths.

Findings: none.

  • apps/server/src/device/DeviceService.ts:1805-1814: the error finalizer removes only record/trace start registrations for the exact session and type. It wraps the registration and external start path, leaves stop/success behavior unchanged, and does not turn errors into successes. The command semaphore serializes executions, and batch steps are sequential, so this cleanup cannot remove a newer concurrent start from another command.
  • apps/server/src/device/DeviceCommands.registered.test.ts:648-669: both capture types exercise the registered MCP path with a failed external start, a new request key on the same target, and stop returning an artifact. The assertions distinguish the stale-registration failure from successful recovery.
  • Previous TS2375 at apps/server/src/device/DeviceCommands.registered.test.ts:197: corrected in source. Compared with previous head db0cbb1d1678a7a44cf50f936473c3331be49a16, lines 259-262 now supply stdoutTruncated, stderrTruncated, stdoutInvalidUtf8, and stderrInvalidUtf8. These are required booleans in apps/server/src/processRunner.ts:40-47; both fixture return branches now satisfy that shape.
  • docs/map.md:112: associates this fix with the existing remote browser/device part.

Proof limits: created the authorized detached worktree at the exact SHA; it had no node_modules, so targeted tests were skipped as instructed and no local typecheck was run. git diff a0037e064a^ a0037e064a --check passed. The exact-head CI Build and Typecheck job was still in progress, with its Typecheck step pending when checked; this review does not claim a completed compiler pass or live device verification. Removed the temporary worktree and verified its absence; the original checkout is unchanged.

Requirements and who asked: User requested an independent whole review of PR #595 at the exact SHA, including failure cleanup and the previous TS2375 error.
Deleted: Implementation changes, dependency installation, broad local checks, and live device work.
Bottleneck: Validate cleanup ownership and the fixture return type; local execution is unavailable without dependencies.
Checked myself: Complete diff, surrounding capture and receipt code, both fixture branches against ProcessRunOutput, exact PR head, current CI state, and temporary worktree cleanup.

Model: gpt-6.1-sol. Harness: Codex.

@lukemaj
lukemaj merged commit ac0207f into main Oct 20, 2026
25 checks passed
@lukemaj
lukemaj deleted the fix/592-device-capture-cleanup branch October 20, 2026 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(device): a failed record or trace start stays in commandCaptures

1 participant