Skip to content

fix(notebook): ignore stale executor callbacks - #609

Merged
ewen-poch merged 1 commit into
mainfrom
fix/notebook-stale-executor-callbacks
Aug 2, 2026
Merged

fix(notebook): ignore stale executor callbacks#609
ewen-poch merged 1 commit into
mainfrom
fix/notebook-stale-executor-callbacks

Conversation

@ewen-poch

Copy link
Copy Markdown
Member

Problem

Notebook executor callbacks can arrive after their executor or same-ID Session Aggregate has been replaced. The callback previously resolved ownership from the current registry state, so an old executor could persist run.json, mutate a replacement Aggregate, or emit onNotebookChanged for a generation it no longer owned. A teardown/recreate gap could also fall through to the legacy no-live-session persistence path.

Proposed change

Bind lifecycle callbacks to the originating Aggregate and executor generation, and close callback admission synchronously before teardown or replacement.

flowchart LR
  Executor["Executor callback"] --> Token["Aggregate + executor generation"]
  Token --> Gate{"Still current?"}
  Gate -->|yes| Queue["Lifecycle queue"]
  Queue --> Persist["Update Aggregate + run.json + notification"]
  Gate -->|no| Drop["Ignore stale callback"]
  Teardown["Teardown / replacement"] --> Close["Close admission synchronously"]
  Close --> Drain["Drain already-admitted persistence"]
  Drain --> Replace["Publish successor"]
Loading
  • Generated callbacks must still match the live Session Aggregate and executor generation before any mutation, persistence, or notification.
  • Teardown and executor replacement synchronously reject later callbacks, then drain already-admitted persistence before releasing the old owner.
  • A generated callback received while no live Session exists is stale and is ignored; generation-less legacy rehydration keeps its existing behavior.
  • In-place restart preserves the executor generation because the executor instance remains the owner.

Scope and non-goals

  • Covers default and named environments, idle-shutdown and terminated callbacks, executor replacement, overlapping teardown, and same-ID recreation gaps.
  • No IPC, Electron/Web/CLI/local-RPC payload, schema, Session relationship, UI, Specialist, Permission, Compute, or Issue [Feature]: Provider-neutral multi-agent research orchestration #458 orchestration change.
  • No change to normal current-generation execution or persistence semantics.

Acceptance criteria and validation

All checks below ran after the final material edit and an independent fixed-SHA closure review confirmed the evidence mapping with 0 actionable findings.

Behavior Project-owned check Final result
Old callbacks cannot mutate/persist/notify after teardown, replacement, or same-ID recreation npm test -- --run src/main/notebook/runtime-service.test.ts src/main/notebook/session-aggregate.test.ts 174 passed, 9 skipped
Generated no-live-session callbacks are rejected while legacy generation-less rehydration remains Same focused command, including idle and terminated gap regressions passed
Node and renderer types remain compatible npm run typecheck passed
Repository lint npm run lint 0 errors; 23 unrelated baseline warnings
Repository regression suite npm test 649 files passed, 15 skipped; 9,591 tests passed, 184 skipped
Patch hygiene git diff --check passed

Review focus

Please focus on synchronous admission closure, draining already-admitted callback persistence before owner replacement, and the distinction between generated stale callbacks and generation-less legacy rehydration.

Remaining risk: real NotebookKernelExecutor timer/shutdown timing is represented by deterministic injected-lifecycle tests rather than a non-skipped external E2E. Queue draining intentionally allows teardown to wait for already-admitted repository persistence so it cannot write across generations.

Merge by squash only after all required CI and AI review checks pass.

Bind executor lifecycle callbacks to the owning Session Aggregate generation so replaced executors cannot mutate or persist state for a successor.
@github-actions github-actions Bot added the bug Something isn't working label Aug 2, 2026
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown

Codex Review

Verdict: mergeable

No actionable findings.

Summary: Static inspection found no concrete merge-blocking defect introduced by this pull request.

@github-actions github-actions Bot added the ready-to-merge All completed AI reviewers found this pull request mergeable. label Aug 2, 2026
@ewen-poch
ewen-poch merged commit 9095636 into main Aug 2, 2026
23 checks passed
@ewen-poch
ewen-poch deleted the fix/notebook-stale-executor-callbacks branch August 2, 2026 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready-to-merge All completed AI reviewers found this pull request mergeable.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant