Skip to content

fix(daemon): retain cleanup locks and preserve healed snapshots - #3760

Open
thymikee wants to merge 7 commits into
mainfrom
fix/adr-drift-lock-and-heal
Open

thymikee wants to merge 7 commits into
mainfrom
fix/adr-drift-lock-and-heal

Conversation

@thymikee

@thymikee thymikee commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Retain session/device execution locks through binding and claim cleanup on every outcome (ADR 0019).
  • Preserve authored snapshots and repair-time --record snapshots in healed scripts; exclude foreground composite captures (ADR 0012). Explicit recording uses existing action flags; only authored provenance needs separate storage.
  • Both fixes close implementation gaps found in the same ADR-conformance audit (Restructure ADRs into short decision records #3744): cleanup must stay serialized, and publication must preserve authored intent.
  • Eleven files; scope expands to foreground composition only. No ADR edits. Binding disposal remains unbounded by the daemon; existing disposers acquire no execution locks.

Validation

  • Tested SHA: eb304913e7daf7e4d5280a8783828b09d131689b.
  • pnpm format; five focused suites: 104 passed. Parser accepts snapshot --record but discards the flag; publication round-trips the retained snapshot without it.
  • Affected gate receipt: pnpm check:affected --run --head HEAD passed (run-XYy1Xj, exactCommit=true, checkoutChanged=false): seven checks, 2,302 tests; seven parked/GitHub-owned skips.
  • Same router test command (pnpm exec vitest run --project unit-core packages/daemon/src/core/__tests__/request-runtime-binding-router.test.ts --maxWorkers=1): merge-base 4c1c786968 6 failed/2 passed, head 8 passed. Only the test was copied; base execution scope stayed unchanged. Queue insertion replaces the timing tick. Disposable checkout removed.
  • Original iOS failure: screenshot shows unanswered launch confirmation; runner reports APP_NOT_RUNNING / wait_capture_stalled. Independent failure has identical symptoms and merge-base execution-scope code. The single original-job rerun was cancelled while queued after the follow-up push; new-head CI is pending.
  • Self-reviewed. New-head integration, coverage and device CI remain required; no local device run.

View guided diff Turn on auto-fix

ADR 0019 requires request bindings to dispose before device/session lock release. Await the shared cleanup outcome under initial and retained execution locks, then let response finalization preserve primary failures and report cleanup failures.

At base 4c1c786, six blocked-cleanup regressions fail: queued same-session or same-device requests enter on success, throw, and cancellation. With the fix all six pass; the scope and open-wait suites pass (42 tests). No request-binding disposal bound exists; no timeout was added. Device dispatch is unchanged.
ADR 0012 requires authored observations and explicit --record corrections to survive healed-script publication. Carry their typed publication intent from recording into the writer instead of stripping every snapshot. Retain ordinary diagnostic-snapshot optimization.

Isolated regression at base 4c1c786: authored and explicit-record persisted snapshots and the correction-count/resume publication scenario all fail (3 regressions); all pass with the fix. Combined before: 9 failed, 43 passed; after: 52 passed. Recorder/resume focused coverage also passes (82 tests). Device dispatch is unchanged.

@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.

No issues found across 7 files

You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

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

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.32 MB 5.32 MB +274 B
Package (unpacked) 5.32 MB 5.32 MB +274 B
Package (download) 1.60 MB 1.60 MB +100 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.6 ms 26.9 ms +0.3 ms
CLI --help 79.6 ms 77.2 ms -2.4 ms

ADR 0012 decision 6 preserves authored observations through replay provenance. Non-observation wait and diff requests must not acquire authored intent from their false observation classification.

Regression proof on 17b30bf plus tests: 2 failed, 31 passed (interactive wait/diff received authored intent). With the fix: 33 passed.
ADR 0012 decision 6 preserves the authored replay path. The initial snapshot composed by open --foreground is not a source step and must not inherit its recording intent, including a configured record flag.

Regression proof on 17b30bf plus tests: 2 failed, 13 passed because composite snapshots entered session.actions. With noRecord on the synthetic capture: 15 passed; capture response remains successful.
ADR 0012 decision 6 requires authored observations to survive the entire repair publication path. Exercise a source snapshot -i through replay, close, healed-file parsing and fresh-session replay.

Isolated regression: restore the writer from dacec4a (before snapshot retention) in the 17b30bf checkout plus this test. The selected case fails because the healed snapshot is missing (1 failed, 5 skipped). With retention: 1 passed, 5 skipped; full repair-recording suite: 6 passed.
ADR 0012 decision 6 scopes --record observation retention to repair-armed sessions. Preserve ordinary authoring snapshot exclusion even when project configuration supplies record=true; keep authored source observations and explicit repair corrections.

Regression proof on 17b30bf plus tests: 1 failed, 1 passed, 39 skipped (ordinary record=true published a diagnostic snapshot). With the fix: 2 passed, 39 skipped. All four focused suites: 95 passed.
@thymikee

Copy link
Copy Markdown
Member Author

The PR is ready. I reviewed commit 49a4033 and found no blocking problems in the code.

Not blocking: these notes can be taken or left. (1) The Unreleased section of CHANGELOG.md has no entry for two user-visible changes: authored and --record'ed snapshot steps now persist in healed scripts (session-script-writer.ts#L315-L321), and requests that share a session or device now queue behind binding disposal. (2) session-action-recorder.ts#L89 stores publicationIntent: 'explicit-record' from flags.record, but sanitizeFlags already keeps that flag on the same action. Could the writer read action.flags.record and keep only the authored-provenance bit? (3) The PR joins two fixes, lock retention and snapshot publication intent. A line in the PR body on why they ship together would help. (4) The lock-retention test in request-runtime-binding-router.test.ts#L95 waits one setImmediate tick for the queued bind call. Any real async work before bind would let it pass without the fix. Could you wait on a deterministic signal, such as the second request entering the lock queue, and show the test failing at merge-base 4c1c786 with the scope change reverted? I did not run it there. I also did not read the per-provider binding disposers, and I did not check whether the .ad parser accepts --record on a snapshot step.

I found no conflicts. The iOS Smoke Tests job failed at "wait for the deep-link destination (5/5)" with APP_NOT_RUNNING, reason wait_capture_stalled, which looks like a held deep-link launch. A lock or disposal fault would show as a delayed or failed request, not a runner-reported app-not-running. The recording changes do not touch that route. I did not compare against the flake history on main. The gesture-pan-duration.ad failure passed on retry. Please rerun the Smoke Tests job and add the CHANGELOG entry before merge.

Address #3760 (comment). Read explicit recording from the existing action flags, retain authored provenance, add the Unreleased notes, and synchronize lock tests with queue insertion.

@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 across 7 files (changes from recent commits).

You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.

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/core/session-script-writer.ts">

<violation number="1" location="packages/daemon/src/core/session-script-writer.ts:318">
P2: This treats the effective `record` flag as proof that the observation explicitly opted into repair retention. Because `record` is also allowed as a project-config default, a repair session with that setting will retain every diagnostic snapshot and can reintroduce the self-inflicted divergences this exclusion is meant to prevent; preserve explicit-vs-config provenance when recording the action.</violation>
</file>

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

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

if (
action.command === 'snapshot' &&
action.publicationIntent !== 'authored' &&
!(repairArmed && action.flags.record === true)

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: This treats the effective record flag as proof that the observation explicitly opted into repair retention. Because record is also allowed as a project-config default, a repair session with that setting will retain every diagnostic snapshot and can reintroduce the self-inflicted divergences this exclusion is meant to prevent; preserve explicit-vs-config provenance when recording the action.

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 packages/daemon/src/core/session-script-writer.ts, line 318:

<comment>This treats the effective `record` flag as proof that the observation explicitly opted into repair retention. Because `record` is also allowed as a project-config default, a repair session with that setting will retain every diagnostic snapshot and can reintroduce the self-inflicted divergences this exclusion is meant to prevent; preserve explicit-vs-config provenance when recording the action.</comment>

<file context>
@@ -315,7 +315,7 @@ function buildOptimizedActions(
       action.command === 'snapshot' &&
       action.publicationIntent !== 'authored' &&
-      !(repairArmed && action.publicationIntent === 'explicit-record')
+      !(repairArmed && action.flags.record === true)
     ) {
       continue;
</file context>

@thymikee

Copy link
Copy Markdown
Member Author

Addressed all four maintainer notes in eb30491:

  • Added both Unreleased entries; explicit recording now reads action.flags.record, retaining only authored provenance separately. Ordinary publication behavior stays covered.
  • Added the shared ADR-audit rationale to the body.
  • Replaced setImmediate with execution-queue insertion. The same router suite gives 6 failed/2 passed on merge-base 4c1c786968 with only the test copied, versus 8 passed on head. The missing predecessor lock causes each base failure; the disposable checkout is removed.

iOS diagnosis: the original artifact screenshot shows unanswered “Open in Agent Device Tester?” confirmation; runner APP_NOT_RUNNING becomes wait_capture_stalled. Independent run fails identically with execution-scope blob 21eef92c7a matching merge-base, without this lock change. Apple binding disposal is a no-op. The single rerun was cancelled while queued after this push.

Validation: five focused suites, 104 passed; affected gate run-XYy1Xj, 2,302 passed, seven checks passed/seven skips, exactCommit=true, checkoutChanged=false. Self-reviewed.

Head CI: lint/typecheck/package, size, native fingerprint and CodeQL passed; smoke, guards, integration, coverage and cubic pending. Not merge-ready yet.

Review threads: 0 fixed / 0 already fixed / 0 rejected (none exist).

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant